On 10/03/2015 06:50 AM, Simon Glass wrote: > Hi Stephen, > > On 21 September 2015 at 19:06, Stephen Warren <swar...@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 <s...@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 <s...@denx.de> >>>>>> Cc: Simon Glass <s...@chromium.org> >>>>>> Cc: Bin Meng <bmeng...@gmail.com> >>>>>> Cc: Marek Vasut <ma...@denx.de> >>>>>> Cc: Masahiro Yamada <yamada.masah...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
