[U-Boot] [PATCH] dm: core: Add platform specific bus translation function
Stefan Roese
sr at denx.de
Fri Dec 11 06:54:39 CET 2015
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.
Thanks,
Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-dm-Add-option-to-configure-an-offset-for-the-address.patch
Type: text/x-diff
Size: 3240 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151211/3a7c623a/attachment-0001.patch>
More information about the U-Boot
mailing list