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

[Simon Glass]

Hi Stephen,

On 5 October 2015 at 02:22, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 10/03/2015 07:02 PM, Simon Glass wrote:
>> 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.
>
> I believe that a primary difference between the examples above and this
> DT parsing feature are that the examples above are all different options
> for implementing a conceptual feature (e.g. different hash algorithms,
> all of which implement the ability to hash some data), whereas
> supporting ranges in DT is a (fundamental) part of a single feature (DT
> support), rather than a different implementation of "parsing DT".

There was a discussion about implementing a version of printf() for
SPL which just outputs the format string and ignores the parameters.
Arguably this fails your test, but is still useful. I don't see that
DT parsing is any different.

Regards,
Simon