[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Stefan Roese]

Hi Stephen,

On 11.09.2015 19:07, Stephen Warren wrote:
> On 09/09/2015 11:07 AM, Simon Glass wrote:
>> +Stephen
>>
>> Hi Stefan,
>>
>> On Thursday, 3 September 2015, Stefan Roese <sr at denx.de> wrote:
>>>
>>> The current "simple" address translation simple_bus_translate() is not
>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>> properties are used in many nodes (multiple tuples etc). This patch
>>> enables the optional use of the common fdt_translate_address() function
>>> which handles this translation correctly.
>>>
>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>> Cc: Simon Glass <sjg at chromium.org>
>>> Cc: Bin Meng <bmeng.cn at gmail.com>
>>> Cc: Marek Vasut <marex at denx.de>
>>> Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
>>> ---
>>> v2:
>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>    to make the use of the code paths more clear.
>>
>>
>> While this works I'm reluctant to commit it as is. The call to
>> fdt_parent_offset() is very slow.
>>
>> I wonder if this code should be copied into a new file in
>> drivers/core/, tidied up and updated to use dev->parent?
>>
>> Other options:
>> - Add a library to unflatten the tree - but this would not be very
>> useful in SPL or before relocation due to memory/speed constraints
>> - Add a helper to find a node parent which uses a cached tree scan to
>> build a table of previous nodes (or some other means to go backwards
>> in the tree)
>> - Worry about it later and go ahead with this patch
>
> I haven't looked at the code in detail, but I'm surprised there's a
> Kconfig option for this, for either SPL or main U-Boot. In general, this
> feature is simply a required part of parsing DT, so surely the code
> should always be enabled. Without it, we're only getting lucky if DT
> works (lucky the DT doesn't happen to contain a ranges property).

Yes. I was also a bit surprised, that this current (limited) 
implementation to translate the address worked on the platforms using 
this interface right now.

> Sure
> the code does some searching through the DT, and that's slower than not
> doing it, but I don't see how we can support DT without parsing DT
> correctly. Now admittedly some platforms' DTs happen not to contain
> ranges that require this code in practice. However, I feel that's a bit
> of a micro-optimization, and a rather error-prone one at that. What if
> someone pulls a more complete DT into U-Boot and suddenly the code is
> required and they have to spend ages tracking down their problem to
> missing functionality in a core DT parsing API - something they'd be
> unlikely to initially suspect.

Ack. However, I definitely understand Simon's arguments about code size 
here. On some platforms with limited RAM for SPL this additional code 
for "correct" ranges parsing and address translation might break the 
size limit. Not sure how to handle this. At least a comment in the code 
would be helpful, explaining that simple_bus_translate() is limited here 
in some aspects.

Thanks,
Stefan