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

[Simon Glass]

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_support.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?

Regards,
Simon