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

[Simon Glass]

Hi Stephen,

On 3 October 2015 at 20:17, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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.

This patch adds the ability to support complex multi-level range
properties for those boards that need it (only one so far). I think it
is a reasonable feature to have. We can perhaps improve the
implementation as I mentioned earlier in this thread, but only at the
cost of more code and development. The only shortcoming I am aware of
is that it moves up the tree looking for parent nodes, and this
involves scanning the device tree repeatedly. We can address this
later if it becomes a performance issue.

While only one platform currently needs this feature, others may
follow, and as you point out if a platform needs this but we do not
support it, then it would be a failing to correctly parse valid device
tree semantics. But I can't agree that we must do everything or
nothing. One might argue that only the hush parser provides a correct
shell, or that simple malloc() does not implement memory allocation
correctly, or that only SHA256 is suitable as a hash, or that
snprintf() should always check its buffer size, or indeed that prinf()
should support every format parameter, even in SPL. U-Boot is full of
such compromises and that contributes to its flexibility.

There is of course the risk that some poor soul may bring in an
updated device tree file for a platform which suddenly starts needing
ranges where it did not before. Hopefully they will remember that they
changed the device tree and hopefully after bit of searching they find
this thread and they will know to define CONFIG_OF_TRANSLATE. But I am
more worried about the hopeful punter who wants to fit things into a
small SPL. We should try to make this easy from the start, and
allowing some of device tree's less common features to be optional is
the lesser of the two evils IMO.

Acked-by: Simon Glass <sjg at chromium.org>

Regards,
Simon