[U-Boot] [PATCH] dm: core: Add platform specific bus translation function
Simon Glass
sjg at chromium.org
Wed Dec 2 17:53:25 CET 2015
Hi Stefan,
On 2 December 2015 at 09:00, Stefan Roese <sr at 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 at 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
More information about the U-Boot
mailing list