Hi Simon, On 03.12.2015 18:21, Simon Glass wrote: > Hi Stefan, > > On 3 December 2015 at 04:31, Stefan Roese <s...@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 <s...@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 <s...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot