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

[Stefan Roese]

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.

Thanks,
Stefan