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

[Simon Glass]

HI Stefan,

On 10 December 2015 at 22:54, Stefan Roese <sr at denx.de> wrote:
>
> Hi Simon,
>
>
> On 11.12.2015 05:45, Stefan Roese wrote:
>>
>> Hi Simon,
>>
>> On 10.12.2015 16:36, Simon Glass wrote:
>>>
>>> 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.
>>
>>
>> Okay. I'll try to come up with an RFC patch for this.
>
>
> Please find this RFC patch attached. Its still a bit hackish, as
> its on top of the current implementation. But you will get the
> idea.
>
> Is this what you had in mind? If yes, I'll gladly send a clean
> patch version to the list.

Yes this looks good - the setting is maintained by driver model rather
than looked up each time with a function call. When you send the patch
can you put the offset in the uclass-private data for root, rather
than in struct udevice?

Regards,
Simon