On 10/03/2015 07:02 PM, Simon Glass wrote: > Hi Stephen, > > On 3 October 2015 at 20:17, Stephen Warren <swar...@wwwdotorg.org> wrote: >> 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. > > This patch adds the ability to support complex multi-level range > properties for those boards that need it (only one so far). 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.
I believe that a primary difference between the examples above and this DT parsing feature are that the examples above are all different options for implementing a conceptual feature (e.g. different hash algorithms, all of which implement the ability to hash some data), whereas supporting ranges in DT is a (fundamental) part of a single feature (DT support), rather than a different implementation of "parsing DT". _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
