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

[Stefan Roese]

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.

Thanks,
Stefan