From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Mon, 21 Sep 2015 12:06:38 -0600 Subject: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address() In-Reply-To: <55F65A4E.205@denx.de> References: <1441174944-8878-1-git-send-email-sr@denx.de> <1441343506-28473-1-git-send-email-sr@denx.de> <55F30A63.1050201@nvidia.com> <55F65A4E.205@denx.de> Message-ID: <5600472E.102@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 >>>> Cc: Simon Glass >>>> Cc: Bin Meng >>>> Cc: Marek Vasut >>>> Cc: Masahiro Yamada >>>> --- >>>> 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.