From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Fri, 11 Dec 2015 05:45:32 +0100 Subject: [U-Boot] [PATCH] dm: core: Add platform specific bus translation function In-Reply-To: References: <1448619748-26759-1-git-send-email-sr@denx.de> <565BF232.4030602@denx.de> <565D38B3.6010602@denx.de> <565EFC09.3070209@denx.de> <565F0D37.30700@denx.de> <565F15AE.7010004@denx.de> <565F2DC1.7040608@denx.de> <56602819.5040006@denx.de> <56614486.20300@denx.de> <56692290.6010108@denx.de> Message-ID: <566A54EC.1080409@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On 10.12.2015 16:36, Simon Glass wrote: > Hi Stefan, > > On 9 December 2015 at 23:58, Stefan Roese wrote: >> Hi Simon, >> >> >> On 08.12.2015 03:46, Simon Glass wrote: >>> >>> Hi Stefan, >>> >>> On 4 December 2015 at 00:45, Stefan Roese wrote: >>>> >>>> Hi Simon, >>>> >>>> On 03.12.2015 18:21, Simon Glass wrote: >>>>> >>>>> Hi Stefan, >>>>> >>>>> On 3 December 2015 at 04:31, Stefan Roese wrote: >>>>>> >>>>>> >>>>>> Hi Simon, >>>>>> >>>>>> On 02.12.2015 18:45, Simon Glass wrote: >>>>>>> >>>>>>> Hi Stefan, >>>>>>> >>>>>>> On 2 December 2015 at 10:43, Stefan Roese 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 wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Hi Simon, >>>>>>>>>> >>>>>>>>>> On 02.12.2015 16:50, Simon Glass wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>>>> 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