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. > >>> >>> ... >>> * Note: this Device Tree assumes that the bootloader has remapped the >>> * internal registers to 0xf1000000 (instead of the default >>> * 0xd0000000). The 0xf1000000 is the default used by the recent, >>> * DT-capable, U-Boot bootloaders provided by Marvell. Some earlier >>> * boards were delivered with an older version of the bootloader that >>> * left internal registers mapped at 0xd0000000. If you are in this >>> * situation, you should either update your bootloader (preferred >>> * solution) or the below Device Tree should be adjusted. >>> ... >>> >>> It really boils down to a compile-time option for these platforms, >>> to "detect" where the internal registers are located (SPL or not). >> >> >> Yes. >> >>> >>>> I prefer adding a >>>> new ranges property anyway. >>> >>> >>> So please bear with me, and explain (again?) how exactly this new >>> ranges property could solve this problem. Keeping the undetectable >>> nature of this address change in mind. And without adding some >>> more ugly #ifdef CONFIG_SPL_BUILD again. (see also below with your >>> remark to your similar "pit" solution). >> >> >> We need to get to the bottom of this 'detection' thing first (as >> above). But if the only way of detecting is 'am I in SPL or not' then >> you can always have some code that selects the range based on that. > > > Yes, this is what we need to do. > > >>> >>>>> >>>>>> - Add some sort of 'core' driver model address translation setting, >>>>>> which your board code can set up with a function call, and >>>>>> dev_get_addr() uses >>>>>> - Add a uclass and driver for address translation, and call it from >>>>>> here (ugh...) >>>>> >>>>> >>>>> All this doesn't sound very "promising". At least not to me. But >>>>> I had another idea, which might be a good alternative. Use a new, >>>>> different dts for SPL. This has most likely been discussed before, >>>>> not sure. The idea here is, to re-use the existing dts and include >>>>> it in the new dts. Something like: >>>>> >>>>> U-Boot proper: armada-xp-gp.dts >>>>> SPL U-Boot: armada-xp-gp-spl.dts >>>>> >>>>> And this new SPL dts includes the original dts and only changes >>>>> what needs to get changed for SPL. This could include all the >>>>> "u-boot,dm-pre-reloc" properties as well and look like this: >>>>> >>>>> ---<----------------------- >>>>> /* >>>>> * Device Tree file for Marvell Armada XP development board >>>>> * (DB-MV784MP-GP) >>>>> */ >>>>> >>>>> #include "armada-xp-gp.dts" >>>>> >>>>> / { >>>>> soc { >>>>> /* >>>>> * Use 0xd0000000 as base address for the internal >>>>> registers >>>>> * in SPL U-Boot >>>>> */ >>>>> ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000 >>>>> MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000 >>>>> MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>; >>>>> u-boot,dm-pre-reloc; >>>>> >>>>> internal-regs { >>>>> u-boot,dm-pre-reloc; >>>>> >>>>> serial@12000 { >>>>> u-boot,dm-pre-reloc; >>>>> }; >>>>> }; >>>>> }; >>>>> }; >>>>> ---<----------------------- >>>>> >>>>> Unfortunately this approach does not work right now. The dtc >>>>> seems to not overlay the new values in the resulting dtb. >>>>> >>>>> But has this approach been discussed before? Or if not, what do >>>>> you think of it (if and once it really works)? >>>> >>>> >>>> The dtc should overlay the values - we depend on this bahaviour in >>>> various places. >>> >>> >>> Right, I know. But it just doesn't work here right now. I've >>> unsuccessfully tested it. >> >> >> Hmmm. >> >>> >>>> But to me this approach seems a bit clumsy. >>> >>> >>> I don't find this approach clumsy. Just my personal feeling. But >>> using this approach we could abstract all the U-Boot properties >>> and nodes into a separate file. And don't have to make any >>> changes to original dtsi / dts files. >>> >>> Of course it would be even better, if all of these U-Boot additions >>> would be accepted to the main DeviceTree source. But this does >>> not seem to happen, unfortunately. >> >> >> Right, and I've complained about that, ineffectively. It is really >> unfortunate. Still it is worth sending a patch to the list and see >> what happens. I'll make time to send a few more also. >> >>> >>>> I can't help thinking we are missing something here. The DT is >>>> supposed to represent the hardware, and here we have some hardware >>>> which is not being represented correctly. Your complaint that you will >>>> need to change all the files to add a ranges-initial property (or >>>> whatever) doesn't really seem like a problem to me - you only do it >>>> once. >>> >>> >>> Not really. We need to change this at least for every now SoC >>> having this problem. But yes, this should not too much trouble. >>> >>>> We can have a setting as to which property to use, and the >>>> platform can change it when needed. It can fall back to 'ranges' if >>>> there is no ranges-initial property. I used a similar approach for >>>> memory on pit - definitions for both SRAM and SDRAM, and allowing >>>> selection of which one to use. There is nothing in DT that prevents us >>>> from handling this sort of thing. >>> >>> >>> Again, how does this work exactly? Is there some code for "pit" >>> that I could take as an example? >> >> >> You can see an upstream version of something similar here: >> >> http://patchwork.ozlabs.org/patch/402714/ > > > I'll try to find some time to take a look at it tomorrow. Thanks. > >> I can point you to the Chrome OS tree for the downstream part if you like. > > > Sure. The more the merrier... ;) It's in the chromeos-v2013.06 branch, e.g. https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/chromeos-v2013.06/board/samsung/dts/exynos542x-peach.dtsi You can see the /memory and /iram nodes there, and chromeos-config which has properties to select the correct memory region. It allows U-Boot to run from both IRAM or SDRAM, with run-time control. > > >>> >>>> Perhaps you could ask about this on the device-tree-discuss list? If >>>> you like I can start a thread. >>>> >>>> Also, just to get this off my chest, I would like to expand a bit more >>>> on why I don't think we should call board code from drivers. The >>>> drivers are supposed to be hermetic, and work on any platform. The >>>> information needed by the driver to work should come from data, not >>>> code. Then we can represent it in the device tree, or in platform data >>>> (to which device tree is often converted). If we have the drivers >>>> calling out to code to get their information, then it adds more >>>> coupling between the drivers and the platforms. At present we can drop >>>> a driver in by enabling it in Kconfig and adding a device tree node. >>>> We don't need to modify the board code at all. I think that is worth >>>> protecting. It creates a nice clear wall between the driver and the >>>> board code, conceptually free of hidden interactions, etc. It is much >>>> easier to scale to larger and more complex platforms with this in >>>> place. >>>> >>>> For similar reasons I think we should avoid calling drivers from other >>>> drivers - this should always be done through the driver's API (e.g. >>>> the uclass header file). That way we don't have (e.g.) a SPI flash >>>> driver with a compile- or link-time dependency on a particular SPI >>>> driver. It may depend on features that the SPI driver may or may not >>>> provide, but in principle any SPI driver can provide those features, >>>> and if they are publicly declared in the uclass API it is clear what >>>> these features are. >>> >>> >>> Thanks for the detailed explanation for your reasoning. I understand >>> your point of view. >> >> >> OK good. I do understand that this is restrictive and has costs also. >> I'm interested to hear other points of view, etc. > > > Yes, I would also like to hear other opinions on this. If this > 2 lines of call-back code are acceptable for others or not? > > Thanks, > Stefan > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot