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

[Thomas Chou]

On 10/04/2015 03:35 PM, Stefan Roese wrote:
> 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.

Yes, nios2 and socfpga MUST have this ranges translation.

Acked-by: Thomas Chou <thomas at wytron.com.tw>

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