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

[Simon Glass]

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. At present we can enable driver model and device tree
with a ~5KB binary hit including a small device tree. I'd like to keep
that down as low as possible. Otherwise we will end up with SPL being
unable to driver model / device tree on lots of platforms. As time
goes by and SoCs become more and more complex, this will be a pain.
We'll end up forking the driver model.

Of course trade-offs can change over time but that's the way I see it
at the moment.

Regards,
Simon