Hi Stephen, On 21 September 2015 at 19:06, Stephen Warren <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>>> Cc: Simon Glass <[email protected]> >>>>> Cc: Bin Meng <[email protected]> >>>>> Cc: Marek Vasut <[email protected]> >>>>> Cc: Masahiro Yamada <[email protected]> >>>>> --- >>>>> 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
