[U-Boot] [PATCH] dm: core: Add platform specific bus translation function

[Simon Glass]

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'
- 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...)

Regards,
Simon