Hi Stephen, On 5 October 2015 at 02:22, Stephen Warren <swar...@wwwdotorg.org> wrote: > 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".
There was a discussion about implementing a version of printf() for SPL which just outputs the format string and ignores the parameters. Arguably this fails your test, but is still useful. I don't see that DT parsing is any different. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
