[U-Boot] [PATCH] dm: core: Add platform specific bus translation function
Simon Glass
sjg at chromium.org
Thu Dec 10 16:36:16 CET 2015
Hi Stefan,
On 9 December 2015 at 23:58, Stefan Roese <sr at denx.de> wrote:
> Hi Simon,
>
>
> On 08.12.2015 03:46, Simon Glass wrote:
>>
>> Hi Stefan,
>>
>> On 4 December 2015 at 00:45, Stefan Roese <sr at denx.de> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 03.12.2015 18:21, Simon Glass wrote:
>>>>
>>>> Hi Stefan,
>>>>
>>>> On 3 December 2015 at 04:31, Stefan Roese <sr at denx.de> wrote:
>>>>>
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On 02.12.2015 18:45, Simon Glass wrote:
>>>>>>
>>>>>> Hi Stefan,
>>>>>>
>>>>>> On 2 December 2015 at 10:43, Stefan Roese <sr at denx.de> wrote:
>>>>>>>
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> ( Last mail for tonight - a glass of quite nice red wine is
>>>>>>> waiting for me ... ;) )
>>>>>>>
>>>>>>
>>>>>> That's the only sad thing about me being so many hours behind. Still I
>>>>>> can do the same thing with people in Asia I suppose :-)
>>>>>
>>>>>
>>>>> Right. I'm not sure about the wine quality in Asia though... ;)
>>>>>
>>>>>>>
>>>>>>> On 02.12.2015 17:53, Simon Glass wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Stefan,
>>>>>>>>
>>>>>>>> On 2 December 2015 at 09:00, Stefan Roese <sr at denx.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Simon,
>>>>>>>>>
>>>>>>>>> On 02.12.2015 16:50, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>> <snip>
>>>>>>>>>
>>>>>>>>>>>> I think it would be better to make it depend on whether the bit
>>>>>>>>>>>> is
>>>>>>>>>>>> flipped, rather than whether you are in SPL or not.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> You simply can't detect if this "bit is flipped". You just have
>>>>>>>>>>> to know. This is a long lasting ugly thing on some Marvell
>>>>>>>>>>> patforms. Here the comment from armada-xp-gp.dts:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Can you point me to the place in U-Boot where this bit is flipped?
>>>>>>>>>> Something, somewhere has to make the change. So something has to
>>>>>>>>>> know.
>>>>>>>>>> Before it makes the change, the range works one way. Afterwards it
>>>>>>>>>> works another way.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Sure. I've mentioned this before. Its here:
>>>>>>>>>
>>>>>>>>> arch/arm/mach-mvebu/cpu.c:
>>>>>>>>>
>>>>>>>>> int arch_cpu_init(void)
>>>>>>>>> {
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>> /* Linux expects the internal registers to be at
>>>>>>>>> 0xf1000000 */
>>>>>>>>> writel(SOC_REGS_PHY_BASE, INTREG_BASE_ADDR_REG);
>>>>>>>>>
>>>>>>>>> This is the line that changes the register base address. And
>>>>>>>>> to change it back you need to write to the new address, as the
>>>>>>>>> address holding this base address is also moved. Quite ugly!
>>>>>>>>>
>>>>>>>>> So its really right at the start of U-Boot proper.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> OK I see. So really we can determine which way the address 'switch'
>>>>>>>> it. It's just a case of making the change when we are ready, and
>>>>>>>> keeping a record of that.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Yes. But how is the "common code" in dev_get_addr() supposed to know
>>>>>>> which version of U-Boot we are running on? This boils down to some
>>>>>>> callback again, or not? Or even worse the ugly #ifdef.
>>>>>>
>>>>>>
>>>>>> You would call a driver-model core function to select the ranges
>>>>>> property to prefer. Then driver model will remember this setting and
>>>>>> use it.
>>>>>
>>>>>
>>>>> Yes. This can be done. I've taken the time to implement such a
>>>>> version. And attached a small patch in a hackish version, just as
>>>>> an RFC. As you will see, I've added the "ranges-spl" property to
>>>>> some of the DT nodes. And added the DM core functions to enable to
>>>>> usage of a different, non-standard "ranges" property name.
>>>>>
>>>>> All this is not really "clean" and will definitely break non-DM
>>>>> usage of fdt_support.c. Not sure where to go from here. I would
>>>>> still prefer my first patch version, even though I know that
>>>>> you don't like to add this hook / callback into the DM core code.
>>>>>
>>>>> Let me know what you think.
>>>>
>>>>
>>>> Actually that looks pretty good to me. I think the root uclass needs
>>>> to grow a private struct, where you store the ranges name. It is
>>>> slightly odd to have fdtdec calling back into DM, but I don't see a
>>>> big problem with it. The two are strongly coupled anyway. You can put
>>>> an #ifdef CONFIG_DM in fdtdec to solve your problem I suppose.
>>>
>>>
>>> Its not only fdtdec.c but also fdt_support.c that needs this callback
>>> into DM. And fdt_support.c is currently not coupled with DM at all.
>>> Making this change generic, we really need to exchange all "ranges"
>>> occurrences in the whole U-Boot source tree:
>>>
>>> $ git grep "\"ranges\""
>>> arch/powerpc/cpu/mpc85xx/portals.c: range =
>>> fdt_getprop_w(blob, off, "ranges", &len);
>>> arch/powerpc/cpu/mpc85xx/portals.c: fdt_setprop_inplace(blob,
>>> off, "ranges", range, len);
>>> arch/powerpc/cpu/ppc4xx/fdt.c: rc = fdt_find_and_setprop(blob, ebc_path,
>>> "ranges", ranges,
>>> arch/sparc/include/asm/prom.h:/* Element of the "ranges" vector */
>>> board/ifm/o2dnt2/o2dnt2.c: prop = fdt_get_property_w(blob, off,
>>> "ranges", &len);
>>> board/ifm/o2dnt2/o2dnt2.c: fdt_setprop(blob, off, "ranges",
>>> reg2, len);
>>> board/intercontrol/digsy_mtc/digsy_mtc.c: prop =
>>> fdt_get_property_w(blob, off, "ranges", &len);
>>> board/intercontrol/digsy_mtc/digsy_mtc.c: fdt_setprop(blob,
>>> off, "ranges", reg2, len);
>>> board/pdm360ng/pdm360ng.c: rc = fdt_find_and_setprop(blob,
>>> "/localbus", "ranges",
>>> board/socrates/socrates.c: rc = fdt_find_and_setprop(blob,
>>> "/localbus", "ranges",
>>> common/fdt_support.c: /* Normally, an absence of a "ranges" property
>>> means we are
>>> common/fdt_support.c: * /ht nodes with no "ranges" property and a lot
>>> of perfectly
>>> common/fdt_support.c: * "ranges" as equivalent to an empty "ranges"
>>> property which means
>>> common/fdt_suppord0-t.c: return __of_translate_address(blob,
>>> node_offset, in_addr, "ranges");
>>> common/fdt_support.c: prop = fdt_getprop(fdt, node, "ranges", &size);
>>> common/fdt_support.c: * a number of the "ranges" property array.
>>> common/fdt_support.c: * The "ranges" property is an array of
>>> common/fdt_support.c: ranges = fdt_getprop(fdt, node, "ranges",
>>> &ranges_len);
>>> drivers/core/Kconfig: on some platforms (e.g. MVEBU) using complex
>>> "ranges"
>>> drivers/core/Kconfig: on some platforms (e.g. MVEBU) using complex
>>> "ranges"
>>> drivers/core/simple-bus.c: ret = fdtdec_get_int_array(gd->fdt_blob,
>>> dev->of_offset, "ranges",
>>> drivers/pci/pci-uclass.c: prop = fdt_getprop(blob, node, "ranges",
>>> &len);
>>>
>>> So at least pci-class.c should get changes as well. This looks not
>>> really promising to me. So yes, this works, but I think its quite
>>> clumsy and generates much more code and necessary changes,
>>> especially to the dts files, where all the ranges properties now
>>> need to get duplicated.
>>>
>>>> What about the device tree mailing list. Should I send an email there?
>>>
>>>
>>> Sure. We could try to ask about their opinion as well.
>>
>>
>> What about the idea of setting up an offset in device core. Is it a
>> simple offset?
>
>
> The internal registers are mapped at 0xd00x.xxxx in the SPL case. And
> as 0xf10x.xxxx in the main U-Boot case. So this difference can
> definitely be described as an offset, yes. Adding 0xdf00.0000 to
> all device addresses should work in the SPL case. This is what my
> first patch version with the platform specific device address fixup
> (with the weak function) does.
>
> So what do you have in mind? Is some other device offset functionality
> acceptable for you?
I just mean that you could make a call to the driver model core code
to set up this offset, and then dev_get_addr() can handle it
automatically from there. A bit like the patch you sent but without
the device tree component. It is just the call out to board code from
drivers that I am not keen on.
Regards,
Simon
More information about the U-Boot
mailing list