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

[Stefan Roese]

Hi Simon,

On 04.10.2015 03:02, 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).

Its actually already 2 platforms. As Thomas Chou also needs this for 
NIOS (or NIOS2). Thomas, please correct me if I'm wrong.

> 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>

Thanks,
Stefan