[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
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
---
drivers/core/Kconfig | 30 ++
drivers/core/device.c | 8 +++-
2 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
index 41f4e69..15681df 100644
--- a/drivers/core/Kconfig
+++ b/drivers/core/Kconfig
@@ -120,4 +120,34 @@ config SPL_SIMPLE_BUS
Supports the 'simple-bus' driver, which is used on some systems
in SPL.
+config OF_TRANSLATE
+ bool "Translate addresses using fdt_translate_address"
+ depends on DM && OF_CONTROL
+ default y
+ help
+ If this option is enabled, the reg property will be translated
+ using the fdt_translate_address() function. This is necessary
+ on some platforms (e.g. MVEBU) using complex "ranges"
+ properties in many nodes. As this translation is not handled
+ correctly in the default simple_bus_translate() function.
+
+ If this option is not enabled, simple_bus_translate() will be
+ used for the address translation. This function is faster and
+ smaller in size than fdt_translate_address().
+
+config SPL_OF_TRANSLATE
+ bool "Translate addresses using fdt_translate_address"
+ depends on SPL_DM && SPL_OF_CONTROL
+ default n
+ help
+ If this option is enabled, the reg property will be translated
+ using the fdt_translate_address() function. This is necessary
+ on some platforms (e.g. MVEBU) using complex "ranges"
+ properties in many nodes. As this translation is not handled
+ correctly in the default simple_bus_translate() function.
+
+ If this option is not enabled, simple_bus_translate() will be
+ used for the address translation. This function is faster and
+ smaller in size than fdt_translate_address().
+
endmenu
diff --git a/drivers/core/device.c b/drivers/core/device.c
index a6cd936..0cf73cf 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -11,6 +11,7 @@
#include
#include
+#include
#include
#include
#include
@@ -580,7 +581,12 @@ fdt_addr_t dev_get_addr(struct udevice *dev)
fdt_addr_t addr;
addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
- if (CONFIG_IS_ENABLED(SIMPLE_BUS) && addr != FDT_ADDR_T_NONE) {
+ if (CONFIG_IS_ENABLED(OF_TRANSLATE)) {
+ const fdt32_t *reg = fdt_getprop(gd->fdt_blob, dev->of_offset,
+"reg", NULL);
+ addr = fdt_translate_address((void *)gd->fdt_blob,
+dev->of_offset, reg);
+ } else if (CONFIG_IS_ENABLED(SIMPLE_BUS) && addr != FDT_ADDR_T_NONE) {
if (device_get_uclass_id(dev->parent) == UCLASS_SIMPLE_BUS)
addr = simple_bus_translate(dev->parent, addr);
}
--
2.5.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
Hi Stephen, On 21 September 2015 at 19:06, Stephen Warren 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 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. 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 U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
On 10/03/2015 06:50 AM, Simon Glass wrote: > Hi Stephen, > > On 21 September 2015 at 19:06, Stephen Warren 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 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. > > 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
Hi Stephen, On 3 October 2015 at 20:17, Stephen Warren wrote: > On 10/03/2015 06:50 AM, Simon Glass wrote: >> Hi Stephen, >> >> On 21 September 2015 at 19:06, Stephen Warren 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 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. >> >> 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 invo
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
Hi Simon, On 04.10.2015 03:02, Simon Glass wrote: Hi Stephen, On 3 October 2015 at 20:17, Stephen Warren wrote: On 10/03/2015 06:50 AM, Simon Glass wrote: Hi Stephen, On 21 September 2015 at 19:06, Stephen Warren 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 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. 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. 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
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
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 wrote: On 10/03/2015 06:50 AM, Simon Glass wrote: Hi Stephen, On 21 September 2015 at 19:06, Stephen Warren 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 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. 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 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
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
On 10/03/2015 07:02 PM, Simon Glass wrote: > Hi Stephen, > > On 3 October 2015 at 20:17, Stephen Warren wrote: >> On 10/03/2015 06:50 AM, Simon Glass wrote: >>> Hi Stephen, >>> >>> On 21 September 2015 at 19:06, Stephen Warren 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 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. >>> >>> 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 earl
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
Hi Stephen, On 5 October 2015 at 02:22, Stephen Warren wrote: > On 10/03/2015 07:02 PM, Simon Glass wrote: >> Hi Stephen, >> >> On 3 October 2015 at 20:17, Stephen Warren wrote: >>> On 10/03/2015 06:50 AM, Simon Glass wrote: Hi Stephen, On 21 September 2015 at 19:06, Stephen Warren 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 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. 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
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
Hi Stefan,
On 2 September 2015 at 00:22, 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
> ---
> drivers/core/Kconfig | 30 ++
> drivers/core/device.c | 8 +++-
> 2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
> index 41f4e69..15681df 100644
> --- a/drivers/core/Kconfig
> +++ b/drivers/core/Kconfig
> @@ -120,4 +120,34 @@ config SPL_SIMPLE_BUS
> Supports the 'simple-bus' driver, which is used on some systems
> in SPL.
>
> +config OF_TRANSLATE
> + bool "Translate addresses using fdt_translate_address"
> + depends on DM && OF_CONTROL
> + default y
> + help
> + If this option is enabled, the reg property will be translated
> + using the fdt_translate_address() function. This is necessary
> + on some platforms (e.g. MVEBU) using complex "ranges"
> + properties in many nodes. As this translation is not handled
> + correctly in the default simple_bus_translate() function.
> +
> + If this option is not enabled, simple_bus_translate() will be
> + used for the address translation. This function is faster and
> + smaller in size than fdt_translate_address().
> +
> +config SPL_OF_TRANSLATE
> + bool "Translate addresses using fdt_translate_address"
> + depends on SPL_DM && SPL_OF_CONTROL
> + default n
> + help
> + If this option is enabled, the reg property will be translated
> + using the fdt_translate_address() function. This is necessary
> + on some platforms (e.g. MVEBU) using complex "ranges"
> + properties in many nodes. As this translation is not handled
> + correctly in the default simple_bus_translate() function.
> +
> + If this option is not enabled, simple_bus_translate() will be
> + used for the address translation. This function is faster and
> + smaller in size than fdt_translate_address().
> +
> endmenu
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index a6cd936..0cf73cf 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -11,6 +11,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -580,7 +581,12 @@ fdt_addr_t dev_get_addr(struct udevice *dev)
> fdt_addr_t addr;
>
> addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
> - if (CONFIG_IS_ENABLED(SIMPLE_BUS) && addr != FDT_ADDR_T_NONE) {
> + if (CONFIG_IS_ENABLED(OF_TRANSLATE)) {
> + const fdt32_t *reg = fdt_getprop(gd->fdt_blob, dev->of_offset,
> +"reg", NULL);
You could put the assignment in a separate statement. Also check if
!reg and return FDT_ADDR_T_NONE.
> + addr = fdt_translate_address((void *)gd->fdt_blob,
> +dev->of_offset, reg);
> + } else if (CONFIG_IS_ENABLED(SIMPLE_BUS) && addr != FDT_ADDR_T_NONE) {
> if (device_get_uclass_id(dev->parent) == UCLASS_SIMPLE_BUS)
> addr = simple_bus_translate(dev->parent, addr);
> }
> --
> 2.5.1
>
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
+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
>
>
> drivers/core/Kconfig | 30 ++
> drivers/core/device.c | 20
> 2 files changed, 50 insertions(+)
>
> diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
> index 41f4e69..15681df 100644
> --- a/drivers/core/Kconfig
> +++ b/drivers/core/Kconfig
> @@ -120,4 +120,34 @@ config SPL_SIMPLE_BUS
> Supports the 'simple-bus' driver, which is used on some systems
> in SPL.
>
> +config OF_TRANSLATE
> + bool "Translate addresses using fdt_translate_address"
> + depends on DM && OF_CONTROL
> + default y
> + help
> + If this option is enabled, the reg property will be translated
> + using the fdt_translate_address() function. This is necessary
> + on some platforms (e.g. MVEBU) using complex "ranges"
> + properties in many nodes. As this translation is not handled
> + correctly in the default simple_bus_translate() function.
> +
> + If this option is not enabled, simple_bus_translate() will be
> + used for the address translation. This function is faster and
> + smaller in size than fdt_translate_address().
> +
> +config SPL_OF_TRANSLATE
> + bool "Translate addresses using fdt_translate_address"
> + depends on SPL_DM && SPL_OF_CONTROL
> + default n
> + help
> + If this option is enabled, the reg property will be translated
> + using the fdt_translate_address() function. This is necessary
> + on some platforms (e.g. MVEBU) using complex "ranges"
> + properties in many nodes. As this translation is not handled
> + correctly in the default simple_bus_translate() function.
> +
> + If this option is not enabled, simple_bus_translate() will be
> + used for the address translation. This function is faster and
> + smaller in size than fdt_translate_address().
> +
> endmenu
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 0ccd443..c543203 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -11,6 +11,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -581,6 +582,25 @@ fdt_addr_t dev_get_addr(struct udevice *dev)
> #if CONFIG_IS_ENABLED(OF_CONTROL)
> fdt_addr_t addr;
>
> + if (CONFIG_IS_ENABLED(OF_TRANSLATE)) {
> + const fdt32_t *reg;
> +
> + reg = fdt_getprop(gd->fdt_blob, dev->of_offset, "reg", NULL);
> + if (!reg)
> + return FDT_ADDR_T_NONE;
> +
> + /*
> +* Use the full-fledged translate function for complex
> +* bus setups.
> +*/
> + return fdt_translate_address((void *)gd->fdt_blob,
> +dev->of_offset, reg);
> + }
> +
> + /*
> +* Use the "simple" translate function for less complex
> +* bus setups.
> +*/
> addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
> if (CONFIG_IS_ENABLED(SIMPLE_BUS) && addr != FDT_ADDR_T_NONE) {
> if (device_get_uclass_id(dev->parent) == UCLASS_SIMPLE_BUS)
> --
> 2.5.1
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
Hi Simon, On 09.09.2015 20:07, Simon Glass wrote: 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. You've mentioned this before. But how slow could this function really be? And it should not be called that often via dev_get_addr(). Usually only once for each driver in the probe function. Or am I missing something? I wonder if this code should be copied into a new file in drivers/core/, tidied up and updated to use dev->parent? You mean fdt_translate_address()? It references many functions from fdt_support.c though which we would need to duplicate here as well. 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 see no problems to defer this patch (or a "better" version of it) to after this release. The Marvell mvebu DM patches are also not targeted for this release. Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
Hi Stefan, On 9 September 2015 at 22:54, Stefan Roese wrote: > Hi Simon, > > On 09.09.2015 20:07, Simon Glass wrote: >> >> 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. > > > You've mentioned this before. But how slow could this function really be? It scans the tree from the start. There is no back link. > And it should not be called that often via dev_get_addr(). Usually only once > for each driver in the probe function. Or am I missing something? Sounds correct. > >> I wonder if this code should be copied into a new file in >> drivers/core/, tidied up and updated to use dev->parent? > > > You mean fdt_translate_address()? It references many functions from > fdt_support.c though which we would need to duplicate here as well. > Right. Seems like a pain. >> 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 see no problems to defer this patch (or a "better" version of it) to after > this release. The Marvell mvebu DM patches are also not targeted for this > release. OK - and if the time slowdown is not too large then we can just use this patch, particularly as it is an optional CONFIG. Can you check how much slower it is to use your new case versus the original code? Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
Hi Simon, On 11.09.2015 02:42, Simon Glass wrote: 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. You've mentioned this before. But how slow could this function really be? It scans the tree from the start. There is no back link. And it should not be called that often via dev_get_addr(). Usually only once for each driver in the probe function. Or am I missing something? Sounds correct. So it really shouldn't make a big difference. I wonder if this code should be copied into a new file in drivers/core/, tidied up and updated to use dev->parent? You mean fdt_translate_address()? It references many functions from fdt_support.c though which we would need to duplicate here as well. Right. Seems like a pain. 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 see no problems to defer this patch (or a "better" version of it) to after this release. The Marvell mvebu DM patches are also not targeted for this release. OK - and if the time slowdown is not too large then we can just use this patch, particularly as it is an optional CONFIG. Can you check how much slower it is to use your new case versus the original code? Marvell MVEBU won't boot without this option enabled. So I can't really compare it here. Someone with a platform that doesn't need this option enabled can definitely better do this test and compare the results. Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
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). 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
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. Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()
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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
