[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Stefan Roese]

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?

Thanks,
Stefan