On 10/04/2015 03:35 PM, Stefan Roese wrote:
Hi Simon,
On 04.10.2015 03:02, 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).
Its actually already 2 platforms. As Thomas Chou also needs this for
NIOS (or NIOS2). Thomas, please correct me if I'm wrong.
Yes, nios2 and socfpga MUST have this ranges translation.
Acked-by: Thomas Chou <tho...@wytron.com.tw>
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.
There is of course the risk that some poor soul may bring in an
updated device tree file for a platform which suddenly starts needing
ranges where it did not before. Hopefully they will remember that they
changed the device tree and hopefully after bit of searching they find
this thread and they will know to define CONFIG_OF_TRANSLATE. But I am
more worried about the hopeful punter who wants to fit things into a
small SPL. We should try to make this easy from the start, and
allowing some of device tree's less common features to be optional is
the lesser of the two evils IMO.
Acked-by: Simon Glass <s...@chromium.org>
Thanks,
Stefan
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
