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

[Stephen Warren]

On 09/13/2015 11:25 PM, Stefan Roese wrote:
> 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.

So in my AArch64 build, fdt_translate_address is 0x270 bytes. I can see 
that might be pushing some extremely constrained binaries over a limit 
if that function isn't already included in the binary. However, if we 
are in that situation, I have a really hard time believing this one 
patch/function will be the only issue; we'll constantly be hitting a 
wall where we can't fix issues in DT parsing, DT handling, or other code 
in these binaries since the fix will bloat the binary too much.

In those cases, I rather question whether DT support is the correct 
approach; completely dropping DT support from those binaries would 
likely remove large amounts of code and replace it with a tiny amount of 
constant data. It seems like that'd be the best approach all around 
since it'd head of the issue completely.