[U-Boot] [PATCH] dm: core: Add platform specific bus translation function
Stefan Roese
sr at denx.de
Fri Dec 11 05:45:32 CET 2015
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.
Thanks,
Stefan
More information about the U-Boot
mailing list