[U-Boot] [PATCH] dm: core: Add platform specific bus translation function
Simon Glass
sjg at chromium.org
Wed Dec 2 15:49:33 CET 2015
Hi Stefan,
On 2 December 2015 at 07:11, Stefan Roese <sr at denx.de> wrote:
> Hi Simon,
>
> On 01.12.2015 21:02, Simon Glass wrote:
>> Hi Stefan,
>>
>> On 30 November 2015 at 23:05, Stefan Roese <sr at denx.de> wrote:
>>> Hi Simon,
>>>
>>>
>>> On 01.12.2015 00:17, Simon Glass wrote:
>>>>
>>>> Hi Stefan,
>>>>
>>>> On 29 November 2015 at 23:52, Stefan Roese <sr at denx.de> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>> On 27.11.2015 19:36, Simon Glass wrote:
>>>>>>
>>>>>> On 27 November 2015 at 02:22, Stefan Roese <sr at denx.de> wrote:
>>>>>>>
>>>>>>> This patch adds the additional platform_translate_address() call to
>>>>>>> dev_get_addr(). A weak default with a 1-to-1 translation is also
>>>>>>> provided. Platforms that need a special address translation can
>>>>>>> overwrite this function.
>>>>>>>
>>>>>>> Here the explanation, why this is needed for MVEBU:
>>>>>>>
>>>>>>> When using DM with DT address translation, this does not work
>>>>>>> with the standard fdt_translate_address() function on MVEBU
>>>>>>> in SPL. Since the DT translates to the 0xf100.0000 base
>>>>>>> address for the internal registers. But SPL still has the
>>>>>>> registers mapped to the 0xd000.0000 (SOC_REGS_PHY_BASE)
>>>>>>> address that is used by the BootROM. This is because SPL
>>>>>>> may return to the BootROM for boot continuation (e.g. UART
>>>>>>> xmodem boot mode).
>>>>>>>
>>>>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>>>> Cc: Luka Perkov <luka.perkov at sartura.hr>
>>>>>>> Cc: Dirk Eibach <dirk.eibach at gdsys.cc>
>>>>>>> ---
>>>>>>> drivers/core/device.c | 36 +++++++++++++++++++++++++-----------
>>>>>>> 1 file changed, 25 insertions(+), 11 deletions(-)
>>>>>>
>>>>>>
>>>>>> I wonder if there is a way to handle this with device tree? I would
>>>>>> very much like to avoid adding weak functions and other types of
>>>>>> hooks.
>>>>>
>>>>>
>>>>> I've thought about this also for quite a bit. But couldn't come
>>>>> up with a "better", less intrusive implementation for this
>>>>> problem yet.
>>>>>
>>>>>> Are you saying that there are two values for 'ranges', one in
>>>>>> SPL and one for U-Boot proper?
>>>>>
>>>>>
>>>>> You can think of it as 2 values for "ranges", yes. Basically
>>>>> its a difference in the upper 8 bits of all addresses of the
>>>>> internal registers (e.g. UART, SDRAM controller...).
>>>>>
>>>>> The BootROM starts with 0xd000.0000 and SPL also needs to
>>>>> use this value. As SPL returns back into the BootROM in
>>>>> some cases. And Linux (and other OS'es) expect 0xf100.0000
>>>>> as base address. So the main U-Boot reconfigured the base
>>>>> address to this value quite early.
>>>>>
>>>>>> What actually triggers the change?
>>>>>
>>>>>
>>>>> This is no change. Its just, that now SPL has added DM and DTS
>>>>> support. Before this SPL-DM support this was handled by
>>>>> something like this:
>>>>>
>>>>> #if defined(CONFIG_SPL_BUILD)
>>>>> #define SOC_REGS_PHY_BASE 0xd0000000
>>>>> #else
>>>>> #define SOC_REGS_PHY_BASE 0xf1000000
>>>>> #endif
>>>>> #define MVEBU_REGISTER(x) (SOC_REGS_PHY_BASE + x)
>>>>> #define MVEBU_SDRAM_SCRATCH (MVEBU_REGISTER(0x01504))
>>>>> #define MVEBU_L2_CACHE_BASE (MVEBU_REGISTER(0x08000))
>>>>> ...
>>>>>
>>>>> And now (nearly) all addresses are taken from the DT. And the
>>>>> SPL vs. U-Boot proper base address difference needs to get
>>>>> handled otherwise - here in the DT.
>>>>
>>>>
>>>> No, I mean what causes the hardware address to move? Is there a
>>>> register somewhere that it adjusted to tell the addressing to change?
>>>
>>>
>>> Yes. U-Boot proper reconfigures this base address. Quite early
>>> in arch_cpu_init(). Note that this change / configuration can't
>>> be detected. So you have to know, where the internal registers
>>> are mapped.
>>>
>>>>>
>>>>>> One option would be to have a ranges-spl property, or similar.
>>>>>
>>>>>
>>>>> Hmmm. you mean to add these "ranges-spl" properties additionally
>>>>> to the normal "ranges" properties? I would really like to not
>>>>> change the "ranges" in the dts files. As especially in the
>>>>> MVEBU cases (Armada XP / 38x etc), the occurrences are very
>>>>> high. And this would result in quite a big difference to the
>>>>> "mainline Linux dts" version.
>>>>
>>>>
>>>> Yes I mean a new property. After all, the existing one is incorrect
>>>> for your hardware at least in some configuration.
>>>>
>>>>>
>>>>> I could also add this functionality via a new Kconfig option.
>>>>> Like this:
>>>>>
>>>>> + if (CONFIG_IS_ENABLED(PLATFORM_TRANSLATE_ADDRESS)) {
>>>>> + addr = platform_translate_address((void *)gd->fdt_blob,
>>>>> + dev->of_offset, addr);
>>>>> + }
>>>>>
>>>>> So no weak default would be needed. Just let me know if you
>>>>> would prefer it this way. And I'll send a v2 using this
>>>>> approach.
>>>>
>>>>
>>>> I'd like to exhaust the DT option first, as this adds another level of
>>>> complexity...the DT is supposed to describe the hardware.
>>>
>>>
>>> I understand. But your suggestion of a new "ranges-spl" property
>>> would result in changes to the dts files (for all MVEBU boards)
>>> and additional support for this "ranges-spl" property in the
>>> U-Boot code. This looks more complex than the 2 lines to the
>>> common code I suggested above. And definitely easier to maintain.
>>> As new MVEBU boards would always have to patch their dts files
>>> for U-Boot.
>>
>> But from another perspective, the dts file is clearly wrong IMO - it
>> does not describe the memory of the system fully. It is only accurate
>> once you 'flip the bit' to change the address space.
>>
>> This creates a callback to board-specific code. That's something I'm
>> really trying to avoid with driver model. We should not have drivers
>> calling into board code for things.
>>
>> Other options, none of which I am recommending, just trying to
>> suggestion options we can discuss:
>>
>> - Describe the mapping somewhere else - e.g. in a /config node
>> property like 'u-boot,range-offset'
>
> I've started with a test version for this alternative. But this
> also seems a bit clumsy, as I now need to conditionally use
> this 'u-boot,range-offset' in get_dev_addr() - depending on
> CONFIG_SPL_BUILD.
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. I prefer adding a
new ranges property anyway.
>
>> - 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. But to me this approach seems a bit clumsy.
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. 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.
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.
Regards,
Simon
More information about the U-Boot
mailing list