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@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@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@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@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@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