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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

---
 drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
 drivers/core/device.c |  8 +++++++-
 2 files changed, 37 insertions(+), 1 deletion(-)

drivers/core/Kconfig +30 -0

index 41f4e69..15681df 100644
@@ -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

drivers/core/device.c +1 -0

index a6cd936..0cf73cf 100644
@@ -11,6 +11,7 @@
 
 #include <common.h>
 #include <fdtdec.h>
+#include <fdt_support.h>
 #include <malloc.h>
 #include <dm/device.h>
 #include <dm/device-internal.h>

drivers/core/device.c +3 -1

+               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

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

---
v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

 drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
 drivers/core/device.c | 20 ++++++++++++++++++++
 2 files changed, 50 insertions(+)

drivers/core/Kconfig +30 -0

index 41f4e69..15681df 100644
@@ -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

drivers/core/device.c +20 -1

index 0ccd443..c543203 100644
@@ -11,6 +11,7 @@
 
 #include <common.h>
 #include <fdtdec.h>
+#include <fdt_support.h>
 #include <malloc.h>
 #include <dm/device.h>
 #include <dm/device-internal.h>

@@ -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

Quoted 12 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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

Hi Simon,

Quoted 22 lines · Simon Glass

Quoted 17 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

---
v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

Quoted 2 lines

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?

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 25 lines

Quoted 23 lines · Stefan Roese

Quoted 13 lines

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 7 lines

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

Quoted 3 lines

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?

+Stephen

Hi Stefan,

Quoted 12 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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

Quoted 29 lines · Simon Glass

+Stephen

Hi Stefan,

Quoted 12 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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?

Quoted 7 lines

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.

Hi Stephen,

Quoted 38 lines · Stephen Warren

Quoted 29 lines · Simon Glass

+Stephen

Hi Stefan,

Quoted 12 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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?

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 70 lines · Stefan Roese

Hi Stephen,

Quoted 43 lines · Stephen Warren

Quoted 32 lines · Simon Glass

+Stephen

Hi Stefan,

Quoted 13 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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?

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 6 lines

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.

Hi Stephen,

Quoted 87 lines · Stephen Warren

Quoted 70 lines · Stefan Roese

Hi Stephen,

Quoted 43 lines · Stephen Warren

Quoted 32 lines · Simon Glass

+Stephen

Hi Stefan,

Quoted 13 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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?

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 96 lines · Simon Glass

Hi Stephen,

Quoted 92 lines · Stephen Warren

Quoted 70 lines · Stefan Roese

Hi Stephen,

Quoted 43 lines · Stephen Warren

Quoted 32 lines · Simon Glass

+Stephen

Hi Stefan,

Quoted 13 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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?

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Hi Stephen,

Quoted 100 lines · Stephen Warren

Quoted 98 lines · Simon Glass

Hi Stephen,

Quoted 87 lines · Stephen Warren

Quoted 70 lines · Stefan Roese

Hi Stephen,

Quoted 43 lines · Stephen Warren

Quoted 32 lines · Simon Glass

+Stephen

Hi Stefan,

Quoted 13 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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?

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

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 <[email protected]>

Hi Simon,

Quoted 132 lines · Simon Glass

Hi Stephen,

Quoted 128 lines · Stephen Warren

Quoted 121 lines · Simon Glass

Hi Stephen,

Quoted 108 lines · Stephen Warren

Quoted 82 lines · Stefan Roese

Hi Stephen,

Quoted 50 lines · Stephen Warren

Quoted 37 lines · Simon Glass

+Stephen

Hi Stefan,

Quoted 17 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 5 lines

---
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?

Quoted 9 lines

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

Quoted 6 lines

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.

Quoted 6 lines

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

Quoted 8 lines

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.

Quoted 9 lines

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.

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 30 lines

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.

Quoted 1 line

Acked-by Simon Glass <[email protected]>

Thanks,
Stefan

Hi Stephen,

Quoted 97 lines · Stephen Warren

Quoted 96 lines · Simon Glass

Hi Stephen,

Quoted 92 lines · Stephen Warren

Quoted 70 lines · Stefan Roese

Hi Stephen,

Quoted 43 lines · Stephen Warren

Quoted 32 lines · Simon Glass

+Stephen

Hi Stefan,

Quoted 13 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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?

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 116 lines · Simon Glass

Hi Stephen,

Quoted 97 lines · Stephen Warren

Quoted 96 lines · Simon Glass

Hi Stephen,

Quoted 92 lines · Stephen Warren

Quoted 70 lines · Stefan Roese

Hi Stephen,

Quoted 43 lines · Stephen Warren

Quoted 32 lines · Simon Glass

+Stephen

Hi Stefan,

Quoted 13 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Quoted 4 lines

---
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?

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 10 lines

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".

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

---
v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

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.

Quoted 5 lines · Stefan Roese

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.

This change makes sense to me, but one comment:

drivers/core/Kconfig config SPL_SIMPLE_BUS · line 126

Quoted 6 lines

drivers/core/Kconfig +4 -0

 
+config OF_TRANSLATE
+    bool "Translate addresses using fdt_translate_address"
+    depends on DM && OF_CONTROL
+    default y

So this is on by default, which I think is correct since applying this technique is required to parse DT correctly. However, ...

drivers/core/device.c fdt_addr_t dev_get_addr(struct udevice *dev) · line 597

Quoted 10 lines

drivers/core/device.c +7 -0

 
+    if (CONFIG_IS_ENABLED(OF_TRANSLATE)) {
 
+        /*
+         * Use the full-fledged translate function for complex
+         * bus setups.
+         */
+        return fdt_translate_address((void *)gd->fdt_blob,
+                         dev->of_offset, reg);

fdt_translate_address() is a simple wrapper around __of_translate_address(), and that function calls fdt_parent_offset() which is "slow" per Simon. Surely this patch will receive the same objection as when I added a (single) call to fdt_parent_offset() into the DT address parsing routine (and this patch is worse, since it adds a call to fdt_parent_offset() for each level of DT sub-nodes).

Hi Stephen,

Quoted 38 lines · Stephen Warren

Quoted 6 lines · Stefan Roese

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.

This change makes sense to me, but one comment:

drivers/core/Kconfig config SPL_SIMPLE_BUS · line 126

Quoted 7 lines

drivers/core/Kconfig +4 -0

 
 
+config OF_TRANSLATE
+    bool "Translate addresses using fdt_translate_address"
+    depends on DM && OF_CONTROL
+    default y

So this is on by default, which I think is correct since applying this technique is required to parse DT correctly. However, ...

drivers/core/device.c fdt_addr_t dev_get_addr(struct udevice *dev) · line 597

Quoted 12 lines

drivers/core/device.c +7 -0

 
 
+    if (CONFIG_IS_ENABLED(OF_TRANSLATE)) {
 
 
+        /*
+         * Use the full-fledged translate function for complex
+         * bus setups.
+         */
+        return fdt_translate_address((void *)gd->fdt_blob,
+                         dev->of_offset, reg);

Quoted 6 lines

fdt_translate_address() is a simple wrapper around __of_translate_address(), and that function calls fdt_parent_offset() which is "slow" per Simon. Surely this patch will receive the same objection as when I added a (single) call to fdt_parent_offset() into the DT address parsing routine (and this patch is worse, since it adds a call to fdt_parent_offset() for each level of DT sub-nodes).

Yes, Simon already mentioned his speed related concerns. But I think I convinced him that calling this function once for each driver probe can't be that bad this in the v2 thread:

https://patchwork.ozlabs.org/patch/514331/

Simon, is this okay? Will you pull this patch after the v2015.10 release?

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Lukasz Majewski <[email protected]>

---
v3:
- Rebased on current U-Boot version
- Added Stephen and Lukasz to Cc

v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

 drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
 drivers/core/device.c | 20 ++++++++++++++++++++
 2 files changed, 50 insertions(+)

Quoted 22 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Lukasz Majewski <[email protected]>

---
v3:
- Rebased on current U-Boot version
- Added Stephen and Lukasz to Cc

v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

Quoted 3 lines

 drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
 drivers/core/device.c | 20 ++++++++++++++++++++
 2 files changed, 50 insertions(+)

Applied to u-boot-dm, thanks!

Hi Stefan, Simon,

Quoted 23 lines · Simon Glass

Quoted 22 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Lukasz Majewski <[email protected]>

---
v3:
- Rebased on current U-Boot version
- Added Stephen and Lukasz to Cc

v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

Quoted 5 lines

Quoted 3 lines

  drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
  drivers/core/device.c | 20 ++++++++++++++++++++
  2 files changed, 50 insertions(+)

Applied to u-boot-dm, thanks!

When testing Simon's patch [1], I found PCI UART on Intel Crown Bay no longer works. git bisect leads to this commit. Somehow I missed this patch before although I see the commit message get me cc'ed but the email did not bring to my attention.

I see this patch introduced OF_TRANSLATE and by default set it to y. This makes the code logic in dev_get_addr() go through fdt_translate_address(), which breaks the things.

Should we set
OF_TRANSLATE to n by default? If set to y, this requires dts to have complete ranges property everywhere.

Hi Bin,

Quoted 32 lines · Bin Meng

Hi Stefan, Simon,

Quoted 23 lines · Simon Glass

Quoted 22 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Lukasz Majewski <[email protected]>

---
v3:
- Rebased on current U-Boot version
- Added Stephen and Lukasz to Cc

v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

Quoted 5 lines

Quoted 3 lines

  drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
  drivers/core/device.c | 20 ++++++++++++++++++++
  2 files changed, 50 insertions(+)

Applied to u-boot-dm, thanks!

Quoted 8 lines

When testing Simon's patch [1], I found PCI UART on Intel Crown Bay no longer works. git bisect leads to this commit. Somehow I missed this patch before although I see the commit message get me cc'ed but the email did not bring to my attention.

I see this patch introduced OF_TRANSLATE and by default set it to y. This makes the code logic in dev_get_addr() go through fdt_translate_address(), which breaks the things.

I'm a bit surprised that using the common fdt_translate_address() function instead of the DM internal simple_bus_translate() causes problems on your platform. Are you sure that the ranges are described correctly in your dts? Is the dts a copy from the Linux original one? Ah, probably not, since we're talking about x86 which has no DT support in Linux, right?

Quoted 3 lines

Should we set
OF_TRANSLATE to n by default? If set to y, this requires dts to have complete ranges property everywhere.

My understanding here is that x86 is a special case. As it doesn't use the full-blown dts sources from Linux. But most likely some "simple" ones, written exactly for U-Boot / DM.

I would still prefer to have this OF_TRANSLATE set to y as default. As its needed for at least some platforms. But if we decide to set it to n, I can live with it as well.

Hi Bin,

Quoted 37 lines · Bin Meng

Hi Stefan, Simon,

Quoted 25 lines · Simon Glass

Quoted 23 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Lukasz Majewski <[email protected]>

---
v3:
- Rebased on current U-Boot version
- Added Stephen and Lukasz to Cc

v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

Quoted 6 lines

Quoted 3 lines

  drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
  drivers/core/device.c | 20 ++++++++++++++++++++
  2 files changed, 50 insertions(+)

Applied to u-boot-dm, thanks!

Quoted 8 lines

When testing Simon's patch [1], I found PCI UART on Intel Crown Bay no longer works. git bisect leads to this commit. Somehow I missed this patch before although I see the commit message get me cc'ed but the email did not bring to my attention.

I see this patch introduced OF_TRANSLATE and by default set it to y. This makes the code logic in dev_get_addr() go through fdt_translate_address(), which breaks the things.

I'm a bit surprised that using the common fdt_translate_address() function instead of the DM internal simple_bus_translate() causes problems on your platform. Are you sure that the ranges are described correctly in your dts? Is the dts a copy from the Linux original one? Ah, probably not, since we're talking about x86 which has no DT support in Linux, right?

Quoted 3 lines

Should we set
OF_TRANSLATE to n by default? If set to y, this requires dts to have complete ranges property everywhere.

My understanding here is that x86 is a special case. As it doesn't use the full-blown dts sources from Linux. But most likely some "simple" ones, written exactly for U-Boot / DM.

I would still prefer to have this OF_TRANSLATE set to y as default. As its needed for at least some platforms. But if we decide to set it to n, I can live with it as well.

Hi Stefan,

Quoted 51 lines · Stefan Roese

Hi Bin,

Quoted 37 lines · Bin Meng

Hi Stefan, Simon,

Quoted 25 lines · Simon Glass

Quoted 23 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Lukasz Majewski <[email protected]>

---
v3:
- Rebased on current U-Boot version
- Added Stephen and Lukasz to Cc

v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

Quoted 6 lines

Quoted 3 lines

  drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
  drivers/core/device.c | 20 ++++++++++++++++++++
  2 files changed, 50 insertions(+)

Applied to u-boot-dm, thanks!

Quoted 8 lines

When testing Simon's patch [1], I found PCI UART on Intel Crown Bay no longer works. git bisect leads to this commit. Somehow I missed this patch before although I see the commit message get me cc'ed but the email did not bring to my attention.

I see this patch introduced OF_TRANSLATE and by default set it to y. This makes the code logic in dev_get_addr() go through fdt_translate_address(), which breaks the things.

Quoted 7 lines

I'm a bit surprised that using the common fdt_translate_address() function instead of the DM internal simple_bus_translate() causes problems on your platform. Are you sure that the ranges are described correctly in your dts? Is the dts a copy from the Linux original one? Ah, probably not, since we're talking about x86 which has no DT support in Linux, right?

Is fdt_translate_address() able to handle PCI bus ranges property? PCI has special ranges.

The arch/x86/dts/crownbay.dts has something like below:

90 pci {

 91                 #address-cells = <3>;
 92                 #size-cells = <2>;
 93                 compatible = "pci-x86";
 94                 u-boot,dm-pre-reloc;
 95                 ranges = <0x02000000 0x0 0x40000000 0x40000000 0 0x80000000
 96                           0x42000000 0x0 0xc0000000 0xc0000000 0 0x20000000
 97                           0x01000000 0x0 0x2000 0x2000 0 0xe000>;
 98
 99                 pcie at 17,0 {
100                         #address-cells = <3>;
101                         #size-cells = <2>;
102                         compatible = "pci-bridge";
103                         u-boot,dm-pre-reloc;
104                         reg = <0x0000b800 0x0 0x0 0x0 0x0>;

Quoted 13 lines

Quoted 3 lines

Should we set
OF_TRANSLATE to n by default? If set to y, this requires dts to have complete ranges property everywhere.

My understanding here is that x86 is a special case. As it doesn't use the full-blown dts sources from Linux. But most likely some "simple" ones, written exactly for U-Boot / DM.

I would still prefer to have this OF_TRANSLATE set to y as default. As its needed for at least some platforms. But if we decide to set it to n, I can live with it as well.

Hi,

Quoted 68 lines · Bin Meng

Hi Stefan,

Quoted 51 lines · Stefan Roese

Hi Bin,

Quoted 37 lines · Bin Meng

Hi Stefan, Simon,

Quoted 25 lines · Simon Glass

Quoted 23 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Lukasz Majewski <[email protected]>

---
v3:
- Rebased on current U-Boot version
- Added Stephen and Lukasz to Cc

v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

Quoted 6 lines

Quoted 3 lines

   drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
   drivers/core/device.c | 20 ++++++++++++++++++++
   2 files changed, 50 insertions(+)

Applied to u-boot-dm, thanks!

Quoted 8 lines

When testing Simon's patch [1], I found PCI UART on Intel Crown Bay no longer works. git bisect leads to this commit. Somehow I missed this patch before although I see the commit message get me cc'ed but the email did not bring to my attention.

I see this patch introduced OF_TRANSLATE and by default set it to y. This makes the code logic in dev_get_addr() go through fdt_translate_address(), which breaks the things.

Quoted 7 lines

I'm a bit surprised that using the common fdt_translate_address() function instead of the DM internal simple_bus_translate() causes problems on your platform. Are you sure that the ranges are described correctly in your dts? Is the dts a copy from the Linux original one? Ah, probably not, since we're talking about x86 which has no DT support in Linux, right?

Is fdt_translate_address() able to handle PCI bus ranges property? PCI has special ranges.

The arch/x86/dts/crownbay.dts has something like below:

90 pci {

Quoted 28 lines

  91                 #address-cells = <3>;
  92                 #size-cells = <2>;
  93                 compatible = "pci-x86";
  94                 u-boot,dm-pre-reloc;
  95                 ranges = <0x02000000 0x0 0x40000000 0x40000000 0 0x80000000
  96                           0x42000000 0x0 0xc0000000 0xc0000000 0 0x20000000
  97                           0x01000000 0x0 0x2000 0x2000 0 0xe000>;
  98
  99                 pcie at 17,0 {
100                         #address-cells = <3>;
101                         #size-cells = <2>;
102                         compatible = "pci-bridge";
103                         u-boot,dm-pre-reloc;
104                         reg = <0x0000b800 0x0 0x0 0x0 0x0>;

Quoted 13 lines

Quoted 3 lines

Should we set
OF_TRANSLATE to n by default? If set to y, this requires dts to have complete ranges property everywhere.

My understanding here is that x86 is a special case. As it doesn't use the full-blown dts sources from Linux. But most likely some "simple" ones, written exactly for U-Boot / DM.

I would still prefer to have this OF_TRANSLATE set to y as default. As its needed for at least some platforms. But if we decide to set it to n, I can live with it as well.

Looks like the issue is:

dev_get_addr() return value is of type fdt_addr_t, and if no valid address found returns FDT_ADDR_T_NONE. But FDT_ADDR_T_NONE is defined as follows:

#ifdef CONFIG_PHYS_64BIT
#define FDT_ADDR_T_NONE (-1ULL)
#define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
#define fdt_size_to_cpu(reg) be64_to_cpu(reg)
#else
#define FDT_ADDR_T_NONE (-1U)
#define fdt_addr_to_cpu(reg) be32_to_cpu(reg)
#define fdt_size_to_cpu(reg) be32_to_cpu(reg)
#endif

On x86, CONFIG_PHYS_64BIT is not defined, so FDT_ADDR_T_NONE becomes -1U.

In the ns16550 driver, the code logic is:

/* try Processor Local Bus device first */
addr = dev_get_addr(dev);

#ifdef CONFIG_PCI
     if (addr == FDT_ADDR_T_NONE) {
     /* then try pci device */

With OF_TRANSLATE set to y, dev_get_addr() returns OF_BAD_ADDR if no valid address found, but OF_BAD_ADDR is defined as:

#define OF_BAD_ADDR ((u64)-1)

This creates a size mismatch as FDT_ADDR_T_NONE can be -1U or -1ULL depending on CONFIG_PHYS_64BIT but OF_BAD_ADDR is always -1ULL.

The patch below fixes this issue:

common/fdt_support.c +1 -0

index f86365e..8930f34 100644
@@ -16,6 +16,7 @@
  #include <libfdt.h>
  #include <fdt_support.h>
  #include <exports.h>
+#include <fdtdec.h>
 
  /**
   * fdt_getprop_u32_default_node - Return a node's property or a default

Hi Bin,

Quoted 165 lines · Bin Meng

Hi,

Quoted 89 lines · Bin Meng

Hi Stefan,

Quoted 60 lines · Stefan Roese

Hi Bin,

Quoted 44 lines · Bin Meng

Hi Stefan, Simon,

Quoted 29 lines · Simon Glass

Quoted 26 lines · Stefan Roese

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Bin Meng <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Lukasz Majewski <[email protected]>

---
v3:
- Rebased on current U-Boot version
- Added Stephen and Lukasz to Cc

v2:
- Rework code a bit as suggested by Simon. Also added some comments to make the use of the code paths more clear.

Quoted 7 lines

Quoted 3 lines

   drivers/core/Kconfig  | 30 ++++++++++++++++++++++++++++++
   drivers/core/device.c | 20 ++++++++++++++++++++
   2 files changed, 50 insertions(+)

Applied to u-boot-dm, thanks!

Quoted 8 lines

When testing Simon's patch [1], I found PCI UART on Intel Crown Bay no longer works. git bisect leads to this commit. Somehow I missed this patch before although I see the commit message get me cc'ed but the email did not bring to my attention.

I see this patch introduced OF_TRANSLATE and by default set it to y. This makes the code logic in dev_get_addr() go through fdt_translate_address(), which breaks the things.

Quoted 7 lines

I'm a bit surprised that using the common fdt_translate_address() function instead of the DM internal simple_bus_translate() causes problems on your platform. Are you sure that the ranges are described correctly in your dts? Is the dts a copy from the Linux original one? Ah, probably not, since we're talking about x86 which has no DT support in Linux, right?

Is fdt_translate_address() able to handle PCI bus ranges property? PCI has special ranges.

The arch/x86/dts/crownbay.dts has something like below:

90 pci {

  91                 #address-cells = <3>;
  92                 #size-cells = <2>;
  93                 compatible = "pci-x86";
  94                 u-boot,dm-pre-reloc;
  95                 ranges = <0x02000000 0x0 0x40000000 0x40000000 0

0x80000000
96 0x42000000 0x0 0xc0000000 0xc0000000 0
0x20000000
97 0x01000000 0x0 0x2000 0x2000 0 0xe000>;
98
99 pcie at 17,0 {

Quoted 20 lines

100                         #address-cells = <3>;
101                         #size-cells = <2>;
102                         compatible = "pci-bridge";
103                         u-boot,dm-pre-reloc;
104                         reg = <0x0000b800 0x0 0x0 0x0 0x0>;

Quoted 14 lines

Quoted 3 lines

Should we set
OF_TRANSLATE to n by default? If set to y, this requires dts to have complete ranges property everywhere.

My understanding here is that x86 is a special case. As it doesn't use the full-blown dts sources from Linux. But most likely some "simple" ones, written exactly for U-Boot / DM.

I would still prefer to have this OF_TRANSLATE set to y as default. As its needed for at least some platforms. But if we decide to set it to n, I can live with it as well.

Looks like the issue is:

dev_get_addr() return value is of type fdt_addr_t, and if no valid address found returns FDT_ADDR_T_NONE. But FDT_ADDR_T_NONE is defined as follows:

#ifdef CONFIG_PHYS_64BIT
#define FDT_ADDR_T_NONE (-1ULL)
#define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
#define fdt_size_to_cpu(reg) be64_to_cpu(reg)
#else
#define FDT_ADDR_T_NONE (-1U)
#define fdt_addr_to_cpu(reg) be32_to_cpu(reg)
#define fdt_size_to_cpu(reg) be32_to_cpu(reg)
#endif

On x86, CONFIG_PHYS_64BIT is not defined, so FDT_ADDR_T_NONE becomes -1U.

In the ns16550 driver, the code logic is:

/* try Processor Local Bus device first */
addr = dev_get_addr(dev);

#ifdef CONFIG_PCI
     if (addr == FDT_ADDR_T_NONE) {
     /* then try pci device */

With OF_TRANSLATE set to y, dev_get_addr() returns OF_BAD_ADDR if no valid address found, but OF_BAD_ADDR is defined as:

#define OF_BAD_ADDR ((u64)-1)

This creates a size mismatch as FDT_ADDR_T_NONE can be -1U or -1ULL depending on CONFIG_PHYS_64BIT but OF_BAD_ADDR is always -1ULL.

The patch below fixes this issue:

common/fdt_support.c +1 -0

index f86365e..8930f34 100644
@@ -16,6 +16,7 @@
  #include <libfdt.h>
  #include <fdt_support.h>
  #include <exports.h>
+#include <fdtdec.h>
 
  /**
   * fdt_getprop_u32_default_node - Return a node's property or a default

@@ -945,7 +946,7 @@ void fdt_del_node_and_alias(void *blob, const char
 *alias)
 

Quoted 7 lines

common/fdt_support.c +1 -1

  /* Max address size we deal with */
  #define OF_MAX_ADDR_CELLS      4
-#define OF_BAD_ADDR    ((u64)-1)
+#define OF_BAD_ADDR    FDT_ADDR_T_NONE
  #define OF_CHECK_COUNTS(na, ns)        ((na) > 0 && (na) <=
 OF_MAX_ADDR_CELLS && \

                         (ns) > 0)

I remember stumbling over such a related problem as well a few weeks ago. With a mismatch of address-cells size and non-64bit platform support. But I got distracted from this issue at that time.

Thanks for looking into this. This change looks good to me. Please send a patch to the list.