From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Fri, 11 Dec 2015 06:54:39 +0100 Subject: [U-Boot] [PATCH] dm: core: Add platform specific bus translation function In-Reply-To: <566A54EC.1080409@denx.de> 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> <566A54EC.1080409@denx.de> Message-ID: <566A651F.5080408@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 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 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. 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: