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

[Stephen Warren]

On 10/03/2015 06:50 AM, Simon Glass wrote:
> Hi Stephen,
> 
> On 21 September 2015 at 19:06, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> 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.
> 
> U-Boot is not Linux - code size is important. We can enable features
> when needed.

Only if they're not mandatory parts of other features that we've made an
arbitrary decision to use. Correctness trumps optimization in absolutely
all cases.