[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[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 <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
---
 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 <common.h>
 #include <fdtdec.h>
+#include <fdt_support.h>
 #include <malloc.h>
 #include <dm/device.h>
 #include <dm/device-internal.h>
@@ -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] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Simon Glass]

Hi Stefan,

On 2 September 2015 at 00:22, Stefan Roese <sr@denx.de> wrote:
> The current "simple" address translation simple_bus_translate() is not
> working on some platforms (e.g. MVEBU). As here more complex "ranges"
> properties are used in many nodes (multiple tuples etc). This patch
> enables the optional use of the common fdt_translate_address() function
> which handles this translation correctly.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>  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 <common.h>
>  #include <fdtdec.h>
> +#include <fdt_support.h>
>  #include <malloc.h>
>  #include <dm/device.h>
>  #include <dm/device-internal.h>
> @@ -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] [PATCH v2] dm: core: Enable optional use of fdt_translate_address()

[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 <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
---
v2:
- Rework code a bit as suggested by Simon. Also added some comments
  to make the use of the code paths more clear.

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

[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Simon Glass]

+Stephen

Hi Stefan,

On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>
> The current "simple" address translation simple_bus_translate() is not
> working on some platforms (e.g. MVEBU). As here more complex "ranges"
> properties are used in many nodes (multiple tuples etc). This patch
> enables the optional use of the common fdt_translate_address() function
> which handles this translation correctly.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> v2:
> - Rework code a bit as suggested by Simon. Also added some comments
>   to make the use of the code paths more clear.


While this works I'm reluctant to commit it as is. The call to
fdt_parent_offset() is very slow.

I wonder if this code should be copied into a new file in
drivers/core/, tidied up and updated to use dev->parent?

Other options:
- Add a library to unflatten the tree - but this would not be very
useful in SPL or before relocation due to memory/speed constraints
- Add a helper to find a node parent which uses a cached tree scan to
build a table of previous nodes (or some other means to go backwards
in the tree)
- Worry about it later and go ahead with this patch
>
>
>  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 <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


Regards,
Simon

[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Stefan Roese]

Hi Simon,

On 09.09.2015 20:07, Simon Glass wrote:
> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>
>> The current "simple" address translation simple_bus_translate() is not
>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>> properties are used in many nodes (multiple tuples etc). This patch
>> enables the optional use of the common fdt_translate_address() function
>> which handles this translation correctly.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>> v2:
>> - Rework code a bit as suggested by Simon. Also added some comments
>>    to make the use of the code paths more clear.
>
>
> While this works I'm reluctant to commit it as is. The call to
> fdt_parent_offset() is very slow.

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] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Simon Glass]

Hi Stefan,

On 9 September 2015 at 22:54, Stefan Roese <sr@denx.de> wrote:
> Hi Simon,
>
> On 09.09.2015 20:07, Simon Glass wrote:
>>
>> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>>
>>>
>>> The current "simple" address translation simple_bus_translate() is not
>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>> properties are used in many nodes (multiple tuples etc). This patch
>>> enables the optional use of the common fdt_translate_address() function
>>> which handles this translation correctly.
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> Cc: Marek Vasut <marex@denx.de>
>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>> v2:
>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>    to make the use of the code paths more clear.
>>
>>
>>
>> While this works I'm reluctant to commit it as is. The call to
>> fdt_parent_offset() is very slow.
>
>
> 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] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Stefan Roese]

Hi Simon,

On 11.09.2015 02:42, Simon Glass wrote:
>>> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>>
>>>> The current "simple" address translation simple_bus_translate() is not
>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>> enables the optional use of the common fdt_translate_address() function
>>>> which handles this translation correctly.
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>> ---
>>>> v2:
>>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>>     to make the use of the code paths more clear.
>>>
>>>
>>>
>>> While this works I'm reluctant to commit it as is. The call to
>>> fdt_parent_offset() is very slow.
>>
>>
>> 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] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Stephen Warren]

On 09/09/2015 11:07 AM, Simon Glass wrote:
> +Stephen
> 
> Hi Stefan,
> 
> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>
>> The current "simple" address translation simple_bus_translate() is not
>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>> properties are used in many nodes (multiple tuples etc). This patch
>> enables the optional use of the common fdt_translate_address() function
>> which handles this translation correctly.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>> v2:
>> - Rework code a bit as suggested by Simon. Also added some comments
>>   to make the use of the code paths more clear.
> 
> 
> While this works I'm reluctant to commit it as is. The call to
> fdt_parent_offset() is very slow.
> 
> I wonder if this code should be copied into a new file in
> drivers/core/, tidied up and updated to use dev->parent?
> 
> Other options:
> - Add a library to unflatten the tree - but this would not be very
> useful in SPL or before relocation due to memory/speed constraints
> - Add a helper to find a node parent which uses a cached tree scan to
> build a table of previous nodes (or some other means to go backwards
> in the tree)
> - Worry about it later and go ahead with this patch

I haven't looked at the code in detail, but I'm surprised there's a
Kconfig option for this, for either SPL or main U-Boot. In general, this
feature is simply a required part of parsing DT, so surely the code
should always be enabled. Without it, we're only getting lucky if DT
works (lucky the DT doesn't happen to contain a ranges property). 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] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Stefan Roese]

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 <sr@denx.de> wrote:
>>>
>>> The current "simple" address translation simple_bus_translate() is not
>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>> properties are used in many nodes (multiple tuples etc). This patch
>>> enables the optional use of the common fdt_translate_address() function
>>> which handles this translation correctly.
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> Cc: Marek Vasut <marex@denx.de>
>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>> v2:
>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>    to make the use of the code paths more clear.
>>
>>
>> While this works I'm reluctant to commit it as is. The call to
>> fdt_parent_offset() is very slow.
>>
>> I wonder if this code should be copied into a new file in
>> drivers/core/, tidied up and updated to use dev->parent?
>>
>> Other options:
>> - Add a library to unflatten the tree - but this would not be very
>> useful in SPL or before relocation due to memory/speed constraints
>> - Add a helper to find a node parent which uses a cached tree scan to
>> build a table of previous nodes (or some other means to go backwards
>> in the tree)
>> - Worry about it later and go ahead with this patch
>
> I haven't looked at the code in detail, but I'm surprised there's a
> Kconfig option for this, for either SPL or main U-Boot. In general, this
> feature is simply a required part of parsing DT, so surely the code
> should always be enabled. Without it, we're only getting lucky if DT
> works (lucky the DT doesn't happen to contain a ranges property).

Yes. I was also a bit surprised, that this current (limited) 
implementation to translate the address worked on the platforms using 
this interface right now.

> Sure
> the code does some searching through the DT, and that's slower than not
> doing it, but I don't see how we can support DT without parsing DT
> correctly. Now admittedly some platforms' DTs happen not to contain
> ranges that require this code in practice. However, I feel that's a bit
> of a micro-optimization, and a rather error-prone one at that. What if
> someone pulls a more complete DT into U-Boot and suddenly the code is
> required and they have to spend ages tracking down their problem to
> missing functionality in a core DT parsing API - something they'd be
> unlikely to initially suspect.

Ack. However, I definitely understand Simon's arguments about code size 
here. On some platforms with limited RAM for SPL this additional code 
for "correct" ranges parsing and address translation might break the 
size limit. Not sure how to handle this. At least a comment in the code 
would be helpful, explaining that simple_bus_translate() is limited here 
in some aspects.

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Stephen Warren]

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 <sr@denx.de> wrote:
>>>>
>>>> The current "simple" address translation simple_bus_translate() is not
>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>> enables the optional use of the common fdt_translate_address() function
>>>> which handles this translation correctly.
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>> ---
>>>> v2:
>>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>>    to make the use of the code paths more clear.
>>>
>>>
>>> While this works I'm reluctant to commit it as is. The call to
>>> fdt_parent_offset() is very slow.
>>>
>>> I wonder if this code should be copied into a new file in
>>> drivers/core/, tidied up and updated to use dev->parent?
>>>
>>> Other options:
>>> - Add a library to unflatten the tree - but this would not be very
>>> useful in SPL or before relocation due to memory/speed constraints
>>> - Add a helper to find a node parent which uses a cached tree scan to
>>> build a table of previous nodes (or some other means to go backwards
>>> in the tree)
>>> - Worry about it later and go ahead with this patch
>>
>> I haven't looked at the code in detail, but I'm surprised there's a
>> Kconfig option for this, for either SPL or main U-Boot. In general, this
>> feature is simply a required part of parsing DT, so surely the code
>> should always be enabled. Without it, we're only getting lucky if DT
>> works (lucky the DT doesn't happen to contain a ranges property).
>
> Yes. I was also a bit surprised, that this current (limited)
> implementation to translate the address worked on the platforms using
> this interface right now.
>
>> Sure
>> the code does some searching through the DT, and that's slower than not
>> doing it, but I don't see how we can support DT without parsing DT
>> correctly. Now admittedly some platforms' DTs happen not to contain
>> ranges that require this code in practice. However, I feel that's a bit
>> of a micro-optimization, and a rather error-prone one at that. What if
>> someone pulls a more complete DT into U-Boot and suddenly the code is
>> required and they have to spend ages tracking down their problem to
>> missing functionality in a core DT parsing API - something they'd be
>> unlikely to initially suspect.
>
> Ack. However, I definitely understand Simon's arguments about code size
> here. On some platforms with limited RAM for SPL this additional code
> for "correct" ranges parsing and address translation might break the
> size limit. Not sure how to handle this. At least a comment in the code
> would be helpful, explaining that simple_bus_translate() is limited here
> in some aspects.

So in my AArch64 build, fdt_translate_address is 0x270 bytes. I can see 
that might be pushing some extremely constrained binaries over a limit 
if that function isn't already included in the binary. However, if we 
are in that situation, I have a really hard time believing this one 
patch/function will be the only issue; we'll constantly be hitting a 
wall where we can't fix issues in DT parsing, DT handling, or other code 
in these binaries since the fix will bloat the binary too much.

In those cases, I rather question whether DT support is the correct 
approach; completely dropping DT support from those binaries would 
likely remove large amounts of code and replace it with a tiny amount of 
constant data. It seems like that'd be the best approach all around 
since it'd head of the issue completely.

[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Simon Glass]

Hi Stephen,

On 21 September 2015 at 19:06, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/13/2015 11:25 PM, Stefan Roese wrote:
>>
>> Hi Stephen,
>>
>> On 11.09.2015 19:07, Stephen Warren wrote:
>>>
>>> On 09/09/2015 11:07 AM, Simon Glass wrote:
>>>>
>>>> +Stephen
>>>>
>>>> Hi Stefan,
>>>>
>>>> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>>
>>>>> The current "simple" address translation simple_bus_translate() is not
>>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>>> enables the optional use of the common fdt_translate_address() function
>>>>> which handles this translation correctly.
>>>>>
>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>> ---
>>>>> v2:
>>>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>>>    to make the use of the code paths more clear.
>>>>
>>>>
>>>>
>>>> While this works I'm reluctant to commit it as is. The call to
>>>> fdt_parent_offset() is very slow.
>>>>
>>>> I wonder if this code should be copied into a new file in
>>>> drivers/core/, tidied up and updated to use dev->parent?
>>>>
>>>> Other options:
>>>> - Add a library to unflatten the tree - but this would not be very
>>>> useful in SPL or before relocation due to memory/speed constraints
>>>> - Add a helper to find a node parent which uses a cached tree scan to
>>>> build a table of previous nodes (or some other means to go backwards
>>>> in the tree)
>>>> - Worry about it later and go ahead with this patch
>>>
>>>
>>> I haven't looked at the code in detail, but I'm surprised there's a
>>> Kconfig option for this, for either SPL or main U-Boot. In general, this
>>> feature is simply a required part of parsing DT, so surely the code
>>> should always be enabled. Without it, we're only getting lucky if DT
>>> works (lucky the DT doesn't happen to contain a ranges property).
>>
>>
>> Yes. I was also a bit surprised, that this current (limited)
>> implementation to translate the address worked on the platforms using
>> this interface right now.
>>
>>> Sure
>>> the code does some searching through the DT, and that's slower than not
>>> doing it, but I don't see how we can support DT without parsing DT
>>> correctly. Now admittedly some platforms' DTs happen not to contain
>>> ranges that require this code in practice. However, I feel that's a bit
>>> of a micro-optimization, and a rather error-prone one at that. What if
>>> someone pulls a more complete DT into U-Boot and suddenly the code is
>>> required and they have to spend ages tracking down their problem to
>>> missing functionality in a core DT parsing API - something they'd be
>>> unlikely to initially suspect.
>>
>>
>> Ack. However, I definitely understand Simon's arguments about code size
>> here. On some platforms with limited RAM for SPL this additional code
>> for "correct" ranges parsing and address translation might break the
>> size limit. Not sure how to handle this. At least a comment in the code
>> would be helpful, explaining that simple_bus_translate() is limited here
>> in some aspects.
>
>
> So in my AArch64 build, fdt_translate_address is 0x270 bytes. I can see that
> might be pushing some extremely constrained binaries over a limit if that
> function isn't already included in the binary. However, if we are in that
> situation, I have a really hard time believing this one patch/function will
> be the only issue; we'll constantly be hitting a wall where we can't fix
> issues in DT parsing, DT handling, or other code in these binaries since the
> fix will bloat the binary too much.
>
> In those cases, I rather question whether DT support is the correct
> approach; completely dropping DT support from those binaries would likely
> remove large amounts of code and replace it with a tiny amount of constant
> data. It seems like that'd be the best approach all around since it'd head
> of the issue completely.

U-Boot is not Linux - code size is important. We can enable features
when needed. 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] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Stephen Warren]

On 10/03/2015 06:50 AM, Simon Glass wrote:
> Hi Stephen,
> 
> On 21 September 2015 at 19:06, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 09/13/2015 11:25 PM, Stefan Roese wrote:
>>>
>>> Hi Stephen,
>>>
>>> On 11.09.2015 19:07, Stephen Warren wrote:
>>>>
>>>> On 09/09/2015 11:07 AM, Simon Glass wrote:
>>>>>
>>>>> +Stephen
>>>>>
>>>>> Hi Stefan,
>>>>>
>>>>> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>>>>>
>>>>>>
>>>>>> The current "simple" address translation simple_bus_translate() is not
>>>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>>>> enables the optional use of the common fdt_translate_address() function
>>>>>> which handles this translation correctly.
>>>>>>
>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>> ---
>>>>>> v2:
>>>>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>>>>    to make the use of the code paths more clear.
>>>>>
>>>>>
>>>>>
>>>>> While this works I'm reluctant to commit it as is. The call to
>>>>> fdt_parent_offset() is very slow.
>>>>>
>>>>> I wonder if this code should be copied into a new file in
>>>>> drivers/core/, tidied up and updated to use dev->parent?
>>>>>
>>>>> Other options:
>>>>> - Add a library to unflatten the tree - but this would not be very
>>>>> useful in SPL or before relocation due to memory/speed constraints
>>>>> - Add a helper to find a node parent which uses a cached tree scan to
>>>>> build a table of previous nodes (or some other means to go backwards
>>>>> in the tree)
>>>>> - Worry about it later and go ahead with this patch
>>>>
>>>>
>>>> I haven't looked at the code in detail, but I'm surprised there's a
>>>> Kconfig option for this, for either SPL or main U-Boot. In general, this
>>>> feature is simply a required part of parsing DT, so surely the code
>>>> should always be enabled. Without it, we're only getting lucky if DT
>>>> works (lucky the DT doesn't happen to contain a ranges property).
>>>
>>>
>>> Yes. I was also a bit surprised, that this current (limited)
>>> implementation to translate the address worked on the platforms using
>>> this interface right now.
>>>
>>>> Sure
>>>> the code does some searching through the DT, and that's slower than not
>>>> doing it, but I don't see how we can support DT without parsing DT
>>>> correctly. Now admittedly some platforms' DTs happen not to contain
>>>> ranges that require this code in practice. However, I feel that's a bit
>>>> of a micro-optimization, and a rather error-prone one at that. What if
>>>> someone pulls a more complete DT into U-Boot and suddenly the code is
>>>> required and they have to spend ages tracking down their problem to
>>>> missing functionality in a core DT parsing API - something they'd be
>>>> unlikely to initially suspect.
>>>
>>>
>>> Ack. However, I definitely understand Simon's arguments about code size
>>> here. On some platforms with limited RAM for SPL this additional code
>>> for "correct" ranges parsing and address translation might break the
>>> size limit. Not sure how to handle this. At least a comment in the code
>>> would be helpful, explaining that simple_bus_translate() is limited here
>>> in some aspects.
>>
>>
>> So in my AArch64 build, fdt_translate_address is 0x270 bytes. I can see that
>> might be pushing some extremely constrained binaries over a limit if that
>> function isn't already included in the binary. However, if we are in that
>> situation, I have a really hard time believing this one patch/function will
>> be the only issue; we'll constantly be hitting a wall where we can't fix
>> issues in DT parsing, DT handling, or other code in these binaries since the
>> fix will bloat the binary too much.
>>
>> In those cases, I rather question whether DT support is the correct
>> approach; completely dropping DT support from those binaries would likely
>> remove large amounts of code and replace it with a tiny amount of constant
>> data. It seems like that'd be the best approach all around since it'd head
>> of the issue completely.
> 
> U-Boot is not Linux - code size is important. We can enable features
> when needed.

Only if they're not mandatory parts of other features that we've made an
arbitrary decision to use. Correctness trumps optimization in absolutely
all cases.

[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Simon Glass]

Hi Stephen,

On 3 October 2015 at 20:17, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/03/2015 06:50 AM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On 21 September 2015 at 19:06, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 09/13/2015 11:25 PM, Stefan Roese wrote:
>>>>
>>>> Hi Stephen,
>>>>
>>>> On 11.09.2015 19:07, Stephen Warren wrote:
>>>>>
>>>>> On 09/09/2015 11:07 AM, Simon Glass wrote:
>>>>>>
>>>>>> +Stephen
>>>>>>
>>>>>> Hi Stefan,
>>>>>>
>>>>>> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>> The current "simple" address translation simple_bus_translate() is not
>>>>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>>>>> enables the optional use of the common fdt_translate_address() function
>>>>>>> which handles this translation correctly.
>>>>>>>
>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>>>>>    to make the use of the code paths more clear.
>>>>>>
>>>>>>
>>>>>>
>>>>>> While this works I'm reluctant to commit it as is. The call to
>>>>>> fdt_parent_offset() is very slow.
>>>>>>
>>>>>> I wonder if this code should be copied into a new file in
>>>>>> drivers/core/, tidied up and updated to use dev->parent?
>>>>>>
>>>>>> Other options:
>>>>>> - Add a library to unflatten the tree - but this would not be very
>>>>>> useful in SPL or before relocation due to memory/speed constraints
>>>>>> - Add a helper to find a node parent which uses a cached tree scan to
>>>>>> build a table of previous nodes (or some other means to go backwards
>>>>>> in the tree)
>>>>>> - Worry about it later and go ahead with this patch
>>>>>
>>>>>
>>>>> I haven't looked at the code in detail, but I'm surprised there's a
>>>>> Kconfig option for this, for either SPL or main U-Boot. In general, this
>>>>> feature is simply a required part of parsing DT, so surely the code
>>>>> should always be enabled. Without it, we're only getting lucky if DT
>>>>> works (lucky the DT doesn't happen to contain a ranges property).
>>>>
>>>>
>>>> Yes. I was also a bit surprised, that this current (limited)
>>>> implementation to translate the address worked on the platforms using
>>>> this interface right now.
>>>>
>>>>> Sure
>>>>> the code does some searching through the DT, and that's slower than not
>>>>> doing it, but I don't see how we can support DT without parsing DT
>>>>> correctly. Now admittedly some platforms' DTs happen not to contain
>>>>> ranges that require this code in practice. However, I feel that's a bit
>>>>> of a micro-optimization, and a rather error-prone one at that. What if
>>>>> someone pulls a more complete DT into U-Boot and suddenly the code is
>>>>> required and they have to spend ages tracking down their problem to
>>>>> missing functionality in a core DT parsing API - something they'd be
>>>>> unlikely to initially suspect.
>>>>
>>>>
>>>> Ack. However, I definitely understand Simon's arguments about code size
>>>> here. On some platforms with limited RAM for SPL this additional code
>>>> for "correct" ranges parsing and address translation might break the
>>>> size limit. Not sure how to handle this. At least a comment in the code
>>>> would be helpful, explaining that simple_bus_translate() is limited here
>>>> in some aspects.
>>>
>>>
>>> So in my AArch64 build, fdt_translate_address is 0x270 bytes. I can see that
>>> might be pushing some extremely constrained binaries over a limit if that
>>> function isn't already included in the binary. However, if we are in that
>>> situation, I have a really hard time believing this one patch/function will
>>> be the only issue; we'll constantly be hitting a wall where we can't fix
>>> issues in DT parsing, DT handling, or other code in these binaries since the
>>> fix will bloat the binary too much.
>>>
>>> In those cases, I rather question whether DT support is the correct
>>> approach; completely dropping DT support from those binaries would likely
>>> remove large amounts of code and replace it with a tiny amount of constant
>>> data. It seems like that'd be the best approach all around since it'd head
>>> of the issue completely.
>>
>> U-Boot is not Linux - code size is important. We can enable features
>> when needed.
>
> Only if they're not mandatory parts of other features that we've made an
> arbitrary decision to use. Correctness trumps optimization in absolutely
> all cases.

This patch adds the ability to support complex multi-level range
properties for those boards that need it (only one so far). I think it
is a reasonable feature to have. We can perhaps improve the
implementation as I mentioned earlier in this thread, but only at the
cost of more code and development. The only shortcoming I am aware of
is that it moves up the tree looking for parent nodes, and this
involves scanning the device tree repeatedly. We can address this
later if it becomes a performance issue.

While only one platform currently needs this feature, others may
follow, and as you point out if a platform needs this but we do not
support it, then it would be a failing to correctly parse valid device
tree semantics. But I can't agree that we must do everything or
nothing. One might argue that only the hush parser provides a correct
shell, or that simple malloc() does not implement memory allocation
correctly, or that only SHA256 is suitable as a hash, or that
snprintf() should always check its buffer size, or indeed that prinf()
should support every format parameter, even in SPL. U-Boot is full of
such compromises and that contributes to its flexibility.

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 <sjg@chromium.org>

Regards,
Simon

[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Stefan Roese]

Hi Simon,

On 04.10.2015 03:02, Simon Glass wrote:
> Hi Stephen,
>
> On 3 October 2015 at 20:17, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 10/03/2015 06:50 AM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On 21 September 2015 at 19:06, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 09/13/2015 11:25 PM, Stefan Roese wrote:
>>>>>
>>>>> Hi Stephen,
>>>>>
>>>>> On 11.09.2015 19:07, Stephen Warren wrote:
>>>>>>
>>>>>> On 09/09/2015 11:07 AM, Simon Glass wrote:
>>>>>>>
>>>>>>> +Stephen
>>>>>>>
>>>>>>> Hi Stefan,
>>>>>>>
>>>>>>> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> The current "simple" address translation simple_bus_translate() is not
>>>>>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>>>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>>>>>> enables the optional use of the common fdt_translate_address() function
>>>>>>>> which handles this translation correctly.
>>>>>>>>
>>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>>> ---
>>>>>>>> v2:
>>>>>>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>>>>>>     to make the use of the code paths more clear.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> While this works I'm reluctant to commit it as is. The call to
>>>>>>> fdt_parent_offset() is very slow.
>>>>>>>
>>>>>>> I wonder if this code should be copied into a new file in
>>>>>>> drivers/core/, tidied up and updated to use dev->parent?
>>>>>>>
>>>>>>> Other options:
>>>>>>> - Add a library to unflatten the tree - but this would not be very
>>>>>>> useful in SPL or before relocation due to memory/speed constraints
>>>>>>> - Add a helper to find a node parent which uses a cached tree scan to
>>>>>>> build a table of previous nodes (or some other means to go backwards
>>>>>>> in the tree)
>>>>>>> - Worry about it later and go ahead with this patch
>>>>>>
>>>>>>
>>>>>> I haven't looked at the code in detail, but I'm surprised there's a
>>>>>> Kconfig option for this, for either SPL or main U-Boot. In general, this
>>>>>> feature is simply a required part of parsing DT, so surely the code
>>>>>> should always be enabled. Without it, we're only getting lucky if DT
>>>>>> works (lucky the DT doesn't happen to contain a ranges property).
>>>>>
>>>>>
>>>>> Yes. I was also a bit surprised, that this current (limited)
>>>>> implementation to translate the address worked on the platforms using
>>>>> this interface right now.
>>>>>
>>>>>> Sure
>>>>>> the code does some searching through the DT, and that's slower than not
>>>>>> doing it, but I don't see how we can support DT without parsing DT
>>>>>> correctly. Now admittedly some platforms' DTs happen not to contain
>>>>>> ranges that require this code in practice. However, I feel that's a bit
>>>>>> of a micro-optimization, and a rather error-prone one at that. What if
>>>>>> someone pulls a more complete DT into U-Boot and suddenly the code is
>>>>>> required and they have to spend ages tracking down their problem to
>>>>>> missing functionality in a core DT parsing API - something they'd be
>>>>>> unlikely to initially suspect.
>>>>>
>>>>>
>>>>> Ack. However, I definitely understand Simon's arguments about code size
>>>>> here. On some platforms with limited RAM for SPL this additional code
>>>>> for "correct" ranges parsing and address translation might break the
>>>>> size limit. Not sure how to handle this. At least a comment in the code
>>>>> would be helpful, explaining that simple_bus_translate() is limited here
>>>>> in some aspects.
>>>>
>>>>
>>>> So in my AArch64 build, fdt_translate_address is 0x270 bytes. I can see that
>>>> might be pushing some extremely constrained binaries over a limit if that
>>>> function isn't already included in the binary. However, if we are in that
>>>> situation, I have a really hard time believing this one patch/function will
>>>> be the only issue; we'll constantly be hitting a wall where we can't fix
>>>> issues in DT parsing, DT handling, or other code in these binaries since the
>>>> fix will bloat the binary too much.
>>>>
>>>> In those cases, I rather question whether DT support is the correct
>>>> approach; completely dropping DT support from those binaries would likely
>>>> remove large amounts of code and replace it with a tiny amount of constant
>>>> data. It seems like that'd be the best approach all around since it'd head
>>>> of the issue completely.
>>>
>>> U-Boot is not Linux - code size is important. We can enable features
>>> when needed.
>>
>> Only if they're not mandatory parts of other features that we've made an
>> arbitrary decision to use. Correctness trumps optimization in absolutely
>> all cases.
>
> This patch adds the ability to support complex multi-level range
> properties for those boards that need it (only one so far).

Its actually already 2 platforms. As Thomas Chou also needs this for 
NIOS (or NIOS2). Thomas, please correct me if I'm wrong.

> 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 <sjg@chromium.org>

Thanks,
Stefan

[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Thomas Chou]

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 <swarren@wwwdotorg.org> wrote:
>>> On 10/03/2015 06:50 AM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On 21 September 2015 at 19:06, Stephen Warren
>>>> <swarren@wwwdotorg.org> wrote:
>>>>> On 09/13/2015 11:25 PM, Stefan Roese wrote:
>>>>>>
>>>>>> Hi Stephen,
>>>>>>
>>>>>> On 11.09.2015 19:07, Stephen Warren wrote:
>>>>>>>
>>>>>>> On 09/09/2015 11:07 AM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> +Stephen
>>>>>>>>
>>>>>>>> Hi Stefan,
>>>>>>>>
>>>>>>>> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The current "simple" address translation simple_bus_translate()
>>>>>>>>> is not
>>>>>>>>> working on some platforms (e.g. MVEBU). As here more complex
>>>>>>>>> "ranges"
>>>>>>>>> properties are used in many nodes (multiple tuples etc). This
>>>>>>>>> patch
>>>>>>>>> enables the optional use of the common fdt_translate_address()
>>>>>>>>> function
>>>>>>>>> which handles this translation correctly.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>>>> ---
>>>>>>>>> v2:
>>>>>>>>> - Rework code a bit as suggested by Simon. Also added some
>>>>>>>>> comments
>>>>>>>>>     to make the use of the code paths more clear.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> While this works I'm reluctant to commit it as is. The call to
>>>>>>>> fdt_parent_offset() is very slow.
>>>>>>>>
>>>>>>>> I wonder if this code should be copied into a new file in
>>>>>>>> drivers/core/, tidied up and updated to use dev->parent?
>>>>>>>>
>>>>>>>> Other options:
>>>>>>>> - Add a library to unflatten the tree - but this would not be very
>>>>>>>> useful in SPL or before relocation due to memory/speed constraints
>>>>>>>> - Add a helper to find a node parent which uses a cached tree
>>>>>>>> scan to
>>>>>>>> build a table of previous nodes (or some other means to go
>>>>>>>> backwards
>>>>>>>> in the tree)
>>>>>>>> - Worry about it later and go ahead with this patch
>>>>>>>
>>>>>>>
>>>>>>> I haven't looked at the code in detail, but I'm surprised there's a
>>>>>>> Kconfig option for this, for either SPL or main U-Boot. In
>>>>>>> general, this
>>>>>>> feature is simply a required part of parsing DT, so surely the code
>>>>>>> should always be enabled. Without it, we're only getting lucky if DT
>>>>>>> works (lucky the DT doesn't happen to contain a ranges property).
>>>>>>
>>>>>>
>>>>>> Yes. I was also a bit surprised, that this current (limited)
>>>>>> implementation to translate the address worked on the platforms using
>>>>>> this interface right now.
>>>>>>
>>>>>>> Sure
>>>>>>> the code does some searching through the DT, and that's slower
>>>>>>> than not
>>>>>>> doing it, but I don't see how we can support DT without parsing DT
>>>>>>> correctly. Now admittedly some platforms' DTs happen not to contain
>>>>>>> ranges that require this code in practice. However, I feel that's
>>>>>>> a bit
>>>>>>> of a micro-optimization, and a rather error-prone one at that.
>>>>>>> What if
>>>>>>> someone pulls a more complete DT into U-Boot and suddenly the
>>>>>>> code is
>>>>>>> required and they have to spend ages tracking down their problem to
>>>>>>> missing functionality in a core DT parsing API - something they'd be
>>>>>>> unlikely to initially suspect.
>>>>>>
>>>>>>
>>>>>> Ack. However, I definitely understand Simon's arguments about code
>>>>>> size
>>>>>> here. On some platforms with limited RAM for SPL this additional code
>>>>>> for "correct" ranges parsing and address translation might break the
>>>>>> size limit. Not sure how to handle this. At least a comment in the
>>>>>> code
>>>>>> would be helpful, explaining that simple_bus_translate() is
>>>>>> limited here
>>>>>> in some aspects.
>>>>>
>>>>>
>>>>> So in my AArch64 build, fdt_translate_address is 0x270 bytes. I can
>>>>> see that
>>>>> might be pushing some extremely constrained binaries over a limit
>>>>> if that
>>>>> function isn't already included in the binary. However, if we are
>>>>> in that
>>>>> situation, I have a really hard time believing this one
>>>>> patch/function will
>>>>> be the only issue; we'll constantly be hitting a wall where we
>>>>> can't fix
>>>>> issues in DT parsing, DT handling, or other code in these binaries
>>>>> since the
>>>>> fix will bloat the binary too much.
>>>>>
>>>>> In those cases, I rather question whether DT support is the correct
>>>>> approach; completely dropping DT support from those binaries would
>>>>> likely
>>>>> remove large amounts of code and replace it with a tiny amount of
>>>>> constant
>>>>> data. It seems like that'd be the best approach all around since
>>>>> it'd head
>>>>> of the issue completely.
>>>>
>>>> U-Boot is not Linux - code size is important. We can enable features
>>>> when needed.
>>>
>>> Only if they're not mandatory parts of other features that we've made an
>>> arbitrary decision to use. Correctness trumps optimization in absolutely
>>> all cases.
>>
>> This patch adds the ability to support complex multi-level range
>> properties for those boards that need it (only one so far).
>
> Its actually already 2 platforms. As Thomas Chou also needs this for
> NIOS (or NIOS2). Thomas, please correct me if I'm wrong.

Yes, nios2 and socfpga MUST have this ranges translation.

Acked-by: Thomas Chou <thomas@wytron.com.tw>

>
>> I think it
>> is a reasonable feature to have. We can perhaps improve the
>> implementation as I mentioned earlier in this thread, but only at the
>> cost of more code and development. The only shortcoming I am aware of
>> is that it moves up the tree looking for parent nodes, and this
>> involves scanning the device tree repeatedly. We can address this
>> later if it becomes a performance issue.
>>
>> While only one platform currently needs this feature, others may
>> follow, and as you point out if a platform needs this but we do not
>> support it, then it would be a failing to correctly parse valid device
>> tree semantics. But I can't agree that we must do everything or
>> nothing. One might argue that only the hush parser provides a correct
>> shell, or that simple malloc() does not implement memory allocation
>> correctly, or that only SHA256 is suitable as a hash, or that
>> snprintf() should always check its buffer size, or indeed that prinf()
>> should support every format parameter, even in SPL. U-Boot is full of
>> such compromises and that contributes to its flexibility.
>>
>> There is of course the risk that some poor soul may bring in an
>> updated device tree file for a platform which suddenly starts needing
>> ranges where it did not before. Hopefully they will remember that they
>> changed the device tree and hopefully after bit of searching they find
>> this thread and they will know to define CONFIG_OF_TRANSLATE. But I am
>> more worried about the hopeful punter who wants to fit things into a
>> small SPL. We should try to make this easy from the start, and
>> allowing some of device tree's less common features to be optional is
>> the lesser of the two evils IMO.
>>
>> Acked-by: Simon Glass <sjg@chromium.org>
>
> Thanks,
> Stefan
>
>

[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Stephen Warren]

On 10/03/2015 07:02 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On 3 October 2015 at 20:17, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 10/03/2015 06:50 AM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On 21 September 2015 at 19:06, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 09/13/2015 11:25 PM, Stefan Roese wrote:
>>>>>
>>>>> Hi Stephen,
>>>>>
>>>>> On 11.09.2015 19:07, Stephen Warren wrote:
>>>>>>
>>>>>> On 09/09/2015 11:07 AM, Simon Glass wrote:
>>>>>>>
>>>>>>> +Stephen
>>>>>>>
>>>>>>> Hi Stefan,
>>>>>>>
>>>>>>> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> The current "simple" address translation simple_bus_translate() is not
>>>>>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>>>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>>>>>> enables the optional use of the common fdt_translate_address() function
>>>>>>>> which handles this translation correctly.
>>>>>>>>
>>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>>> ---
>>>>>>>> v2:
>>>>>>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>>>>>>    to make the use of the code paths more clear.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> While this works I'm reluctant to commit it as is. The call to
>>>>>>> fdt_parent_offset() is very slow.
>>>>>>>
>>>>>>> I wonder if this code should be copied into a new file in
>>>>>>> drivers/core/, tidied up and updated to use dev->parent?
>>>>>>>
>>>>>>> Other options:
>>>>>>> - Add a library to unflatten the tree - but this would not be very
>>>>>>> useful in SPL or before relocation due to memory/speed constraints
>>>>>>> - Add a helper to find a node parent which uses a cached tree scan to
>>>>>>> build a table of previous nodes (or some other means to go backwards
>>>>>>> in the tree)
>>>>>>> - Worry about it later and go ahead with this patch
>>>>>>
>>>>>>
>>>>>> I haven't looked at the code in detail, but I'm surprised there's a
>>>>>> Kconfig option for this, for either SPL or main U-Boot. In general, this
>>>>>> feature is simply a required part of parsing DT, so surely the code
>>>>>> should always be enabled. Without it, we're only getting lucky if DT
>>>>>> works (lucky the DT doesn't happen to contain a ranges property).
>>>>>
>>>>>
>>>>> Yes. I was also a bit surprised, that this current (limited)
>>>>> implementation to translate the address worked on the platforms using
>>>>> this interface right now.
>>>>>
>>>>>> Sure
>>>>>> the code does some searching through the DT, and that's slower than not
>>>>>> doing it, but I don't see how we can support DT without parsing DT
>>>>>> correctly. Now admittedly some platforms' DTs happen not to contain
>>>>>> ranges that require this code in practice. However, I feel that's a bit
>>>>>> of a micro-optimization, and a rather error-prone one at that. What if
>>>>>> someone pulls a more complete DT into U-Boot and suddenly the code is
>>>>>> required and they have to spend ages tracking down their problem to
>>>>>> missing functionality in a core DT parsing API - something they'd be
>>>>>> unlikely to initially suspect.
>>>>>
>>>>>
>>>>> Ack. However, I definitely understand Simon's arguments about code size
>>>>> here. On some platforms with limited RAM for SPL this additional code
>>>>> for "correct" ranges parsing and address translation might break the
>>>>> size limit. Not sure how to handle this. At least a comment in the code
>>>>> would be helpful, explaining that simple_bus_translate() is limited here
>>>>> in some aspects.
>>>>
>>>>
>>>> So in my AArch64 build, fdt_translate_address is 0x270 bytes. I can see that
>>>> might be pushing some extremely constrained binaries over a limit if that
>>>> function isn't already included in the binary. However, if we are in that
>>>> situation, I have a really hard time believing this one patch/function will
>>>> be the only issue; we'll constantly be hitting a wall where we can't fix
>>>> issues in DT parsing, DT handling, or other code in these binaries since the
>>>> fix will bloat the binary too much.
>>>>
>>>> In those cases, I rather question whether DT support is the correct
>>>> approach; completely dropping DT support from those binaries would likely
>>>> remove large amounts of code and replace it with a tiny amount of constant
>>>> data. It seems like that'd be the best approach all around since it'd head
>>>> of the issue completely.
>>>
>>> U-Boot is not Linux - code size is important. We can enable features
>>> when needed.
>>
>> Only if they're not mandatory parts of other features that we've made an
>> arbitrary decision to use. Correctness trumps optimization in absolutely
>> all cases.
> 
> This patch adds the ability to support complex multi-level range
> properties for those boards that need it (only one so far). I think it
> is a reasonable feature to have. We can perhaps improve the
> implementation as I mentioned earlier in this thread, but only at the
> cost of more code and development. The only shortcoming I am aware of
> is that it moves up the tree looking for parent nodes, and this
> involves scanning the device tree repeatedly. We can address this
> later if it becomes a performance issue.
> 
> While only one platform currently needs this feature, others may
> follow, and as you point out if a platform needs this but we do not
> support it, then it would be a failing to correctly parse valid device
> tree semantics. But I can't agree that we must do everything or
> nothing. One might argue that only the hush parser provides a correct
> shell, or that simple malloc() does not implement memory allocation
> correctly, or that only SHA256 is suitable as a hash, or that
> snprintf() should always check its buffer size, or indeed that prinf()
> should support every format parameter, even in SPL. U-Boot is full of
> such compromises and that contributes to its flexibility.

I believe that a primary difference between the examples above and this
DT parsing feature are that the examples above are all different options
for implementing a conceptual feature (e.g. different hash algorithms,
all of which implement the ability to hash some data), whereas
supporting ranges in DT is a (fundamental) part of a single feature (DT
support), rather than a different implementation of "parsing DT".

[U-Boot] [PATCH] dm: core: Enable optional use of fdt_translate_address()

[Simon Glass]

Hi Stephen,

On 5 October 2015 at 02:22, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/03/2015 07:02 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On 3 October 2015 at 20:17, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 10/03/2015 06:50 AM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On 21 September 2015 at 19:06, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>> On 09/13/2015 11:25 PM, Stefan Roese wrote:
>>>>>>
>>>>>> Hi Stephen,
>>>>>>
>>>>>> On 11.09.2015 19:07, Stephen Warren wrote:
>>>>>>>
>>>>>>> On 09/09/2015 11:07 AM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> +Stephen
>>>>>>>>
>>>>>>>> Hi Stefan,
>>>>>>>>
>>>>>>>> On Thursday, 3 September 2015, Stefan Roese <sr@denx.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The current "simple" address translation simple_bus_translate() is not
>>>>>>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>>>>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>>>>>>> enables the optional use of the common fdt_translate_address() function
>>>>>>>>> which handles this translation correctly.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>>>> ---
>>>>>>>>> v2:
>>>>>>>>> - Rework code a bit as suggested by Simon. Also added some comments
>>>>>>>>>    to make the use of the code paths more clear.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> While this works I'm reluctant to commit it as is. The call to
>>>>>>>> fdt_parent_offset() is very slow.
>>>>>>>>
>>>>>>>> I wonder if this code should be copied into a new file in
>>>>>>>> drivers/core/, tidied up and updated to use dev->parent?
>>>>>>>>
>>>>>>>> Other options:
>>>>>>>> - Add a library to unflatten the tree - but this would not be very
>>>>>>>> useful in SPL or before relocation due to memory/speed constraints
>>>>>>>> - Add a helper to find a node parent which uses a cached tree scan to
>>>>>>>> build a table of previous nodes (or some other means to go backwards
>>>>>>>> in the tree)
>>>>>>>> - Worry about it later and go ahead with this patch
>>>>>>>
>>>>>>>
>>>>>>> I haven't looked at the code in detail, but I'm surprised there's a
>>>>>>> Kconfig option for this, for either SPL or main U-Boot. In general, this
>>>>>>> feature is simply a required part of parsing DT, so surely the code
>>>>>>> should always be enabled. Without it, we're only getting lucky if DT
>>>>>>> works (lucky the DT doesn't happen to contain a ranges property).
>>>>>>
>>>>>>
>>>>>> Yes. I was also a bit surprised, that this current (limited)
>>>>>> implementation to translate the address worked on the platforms using
>>>>>> this interface right now.
>>>>>>
>>>>>>> Sure
>>>>>>> the code does some searching through the DT, and that's slower than not
>>>>>>> doing it, but I don't see how we can support DT without parsing DT
>>>>>>> correctly. Now admittedly some platforms' DTs happen not to contain
>>>>>>> ranges that require this code in practice. However, I feel that's a bit
>>>>>>> of a micro-optimization, and a rather error-prone one at that. What if
>>>>>>> someone pulls a more complete DT into U-Boot and suddenly the code is
>>>>>>> required and they have to spend ages tracking down their problem to
>>>>>>> missing functionality in a core DT parsing API - something they'd be
>>>>>>> unlikely to initially suspect.
>>>>>>
>>>>>>
>>>>>> Ack. However, I definitely understand Simon's arguments about code size
>>>>>> here. On some platforms with limited RAM for SPL this additional code
>>>>>> for "correct" ranges parsing and address translation might break the
>>>>>> size limit. Not sure how to handle this. At least a comment in the code
>>>>>> would be helpful, explaining that simple_bus_translate() is limited here
>>>>>> in some aspects.
>>>>>
>>>>>
>>>>> So in my AArch64 build, fdt_translate_address is 0x270 bytes. I can see that
>>>>> might be pushing some extremely constrained binaries over a limit if that
>>>>> function isn't already included in the binary. However, if we are in that
>>>>> situation, I have a really hard time believing this one patch/function will
>>>>> be the only issue; we'll constantly be hitting a wall where we can't fix
>>>>> issues in DT parsing, DT handling, or other code in these binaries since the
>>>>> fix will bloat the binary too much.
>>>>>
>>>>> In those cases, I rather question whether DT support is the correct
>>>>> approach; completely dropping DT support from those binaries would likely
>>>>> remove large amounts of code and replace it with a tiny amount of constant
>>>>> data. It seems like that'd be the best approach all around since it'd head
>>>>> of the issue completely.
>>>>
>>>> U-Boot is not Linux - code size is important. We can enable features
>>>> when needed.
>>>
>>> Only if they're not mandatory parts of other features that we've made an
>>> arbitrary decision to use. Correctness trumps optimization in absolutely
>>> all cases.
>>
>> This patch adds the ability to support complex multi-level range
>> properties for those boards that need it (only one so far). I think it
>> is a reasonable feature to have. We can perhaps improve the
>> implementation as I mentioned earlier in this thread, but only at the
>> cost of more code and development. The only shortcoming I am aware of
>> is that it moves up the tree looking for parent nodes, and this
>> involves scanning the device tree repeatedly. We can address this
>> later if it becomes a performance issue.
>>
>> While only one platform currently needs this feature, others may
>> follow, and as you point out if a platform needs this but we do not
>> support it, then it would be a failing to correctly parse valid device
>> tree semantics. But I can't agree that we must do everything or
>> nothing. One might argue that only the hush parser provides a correct
>> shell, or that simple malloc() does not implement memory allocation
>> correctly, or that only SHA256 is suitable as a hash, or that
>> snprintf() should always check its buffer size, or indeed that prinf()
>> should support every format parameter, even in SPL. U-Boot is full of
>> such compromises and that contributes to its flexibility.
>
> I believe that a primary difference between the examples above and this
> DT parsing feature are that the examples above are all different options
> for implementing a conceptual feature (e.g. different hash algorithms,
> all of which implement the ability to hash some data), whereas
> supporting ranges in DT is a (fundamental) part of a single feature (DT
> support), rather than a different implementation of "parsing DT".

There was a discussion about implementing a version of printf() for
SPL which just outputs the format string and ignores the parameters.
Arguably this fails your test, but is still useful. I don't see that
DT parsing is any different.

Regards,
Simon

[U-Boot] [PATCH v2] dm: core: Enable optional use of fdt_translate_address()

[Thomas Chou]

Hi Stefan,

On 09/04/2015 01:11 PM, 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 <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> v2:
> - Rework code a bit as suggested by Simon. Also added some comments
>    to make the use of the code paths more clear.
>

It works great on nios2 board. Thanks a lot.

Tested-by: Thomas Chou <thomas@wytron.com.tw>

Best regards,
Thomas Chou

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[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 <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
---
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(+)

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 0bc04d4..92fb854 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -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_size_auto_parent(gd->fdt_blob,
 						dev->parent->of_offset,
 						dev->of_offset, "reg",
-- 
2.5.3

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Stephen Warren]

On 09/29/2015 11:00 PM, 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.

This change makes sense to me, but one comment:

> diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig

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

> diff --git a/drivers/core/device.c b/drivers/core/device.c

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

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Stefan Roese]

Hi Stephen,

On 30.09.2015 18:13, Stephen Warren wrote:
> On 09/29/2015 11:00 PM, 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.
>
> This change makes sense to me, but one comment:
>
>> diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
>
>> +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, ...
>
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>
>> +    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).

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?

Thanks,
Stefan

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Simon Glass]

Hi Stefan,

On 1 October 2015 at 07:59, Stefan Roese <sr@denx.de> wrote:
> Hi Stephen,
>
>
> On 30.09.2015 18:13, Stephen Warren wrote:
>>
>> On 09/29/2015 11:00 PM, 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.
>>
>>
>> This change makes sense to me, but one comment:
>>
>>> diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
>>
>>
>>> +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, ...
>>
>>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>>
>>
>>> +    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).
>
>
> 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?

It's not great but it is enabled by an option so I think it is OK for
now. We talked about the alternatives and they were not too great
either. Things like this suggest that we might move to an unflattened
device tree post-relocation one day.

Regards,
Simon

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Simon Glass]

On 29 September 2015 at 23:00, Stefan Roese <sr@denx.de> wrote:
> The current "simple" address translation simple_bus_translate() is not
> working on some platforms (e.g. MVEBU). As here more complex "ranges"
> properties are used in many nodes (multiple tuples etc). This patch
> enables the optional use of the common fdt_translate_address() function
> which handles this translation correctly.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Lukasz Majewski <l.majewski@samsung.com>
> ---
> 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(+)

Applied to u-boot-dm, thanks!

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Bin Meng]

Hi Stefan, Simon,

On Mon, Oct 19, 2015 at 7:16 AM, Simon Glass <sjg@chromium.org> wrote:
> On 29 September 2015 at 23:00, Stefan Roese <sr@denx.de> wrote:
>> The current "simple" address translation simple_bus_translate() is not
>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>> properties are used in many nodes (multiple tuples etc). This patch
>> enables the optional use of the common fdt_translate_address() function
>> which handles this translation correctly.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Cc: Stephen Warren <swarren@nvidia.com>
>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>> ---
>> 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(+)
>
> 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.

[1]: http://patchwork.ozlabs.org/patch/549799/

Regards,
Bin

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Stefan Roese]

Hi Bin,

On 03.12.2015 14:34, Bin Meng wrote:
> Hi Stefan, Simon,
>
> On Mon, Oct 19, 2015 at 7:16 AM, Simon Glass <sjg@chromium.org> wrote:
>> On 29 September 2015 at 23:00, Stefan Roese <sr@denx.de> wrote:
>>> The current "simple" address translation simple_bus_translate() is not
>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>> properties are used in many nodes (multiple tuples etc). This patch
>>> enables the optional use of the common fdt_translate_address() function
>>> which handles this translation correctly.
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> Cc: Marek Vasut <marex@denx.de>
>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> Cc: Stephen Warren <swarren@nvidia.com>
>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>> ---
>>> 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(+)
>>
>> 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.

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?

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

Thanks,
Stefan

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Stephen Warren]

On 12/03/2015 07:12 AM, Stefan Roese wrote:
> Hi Bin,
>
> On 03.12.2015 14:34, Bin Meng wrote:
>> Hi Stefan, Simon,
>>
>> On Mon, Oct 19, 2015 at 7:16 AM, Simon Glass <sjg@chromium.org> wrote:
>>> On 29 September 2015 at 23:00, Stefan Roese <sr@denx.de> wrote:
>>>> The current "simple" address translation simple_bus_translate() is not
>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>> enables the optional use of the common fdt_translate_address() function
>>>> which handles this translation correctly.
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>> ---
>>>> 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(+)
>>>
>>> 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.
>
> 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?
>
>> 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.

Is this the driver that uses U-Boot functions that were intended to 
parse a standard "reg" property to parse some property other than reg, 
with different semantics? Actually, I think I'm remembering an issue 
with SPI on some x86 device, but perhaps the problem here is something 
similar.

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Bin Meng]

Hi Stefan,

On Thu, Dec 3, 2015 at 10:12 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Bin,
>
>
> On 03.12.2015 14:34, Bin Meng wrote:
>>
>> Hi Stefan, Simon,
>>
>> On Mon, Oct 19, 2015 at 7:16 AM, Simon Glass <sjg@chromium.org> wrote:
>>>
>>> On 29 September 2015 at 23:00, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> The current "simple" address translation simple_bus_translate() is not
>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>> enables the optional use of the common fdt_translate_address() function
>>>> which handles this translation correctly.
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>> ---
>>>> 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(+)
>>>
>>>
>>> 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.
>
>
> 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>;

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

Regards,
Bin

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Bin Meng]

Hi,

On Fri, Dec 4, 2015 at 1:31 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Stefan,
>
> On Thu, Dec 3, 2015 at 10:12 PM, Stefan Roese <sr@denx.de> wrote:
>> Hi Bin,
>>
>>
>> On 03.12.2015 14:34, Bin Meng wrote:
>>>
>>> Hi Stefan, Simon,
>>>
>>> On Mon, Oct 19, 2015 at 7:16 AM, Simon Glass <sjg@chromium.org> wrote:
>>>>
>>>> On 29 September 2015 at 23:00, Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>> The current "simple" address translation simple_bus_translate() is not
>>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>>> enables the optional use of the common fdt_translate_address() function
>>>>> which handles this translation correctly.
>>>>>
>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>>> ---
>>>>> 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(+)
>>>>
>>>>
>>>> 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.
>>
>>
>> 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>;
>
>>> 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:

diff --git a/common/fdt_support.c b/common/fdt_support.c
index f86365e..8930f34 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -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)

 /* 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)

Regards,
Bin

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Stefan Roese]

Hi Bin,

On 04.12.2015 07:17, Bin Meng wrote:
> Hi,
>
> On Fri, Dec 4, 2015 at 1:31 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Stefan,
>>
>> On Thu, Dec 3, 2015 at 10:12 PM, Stefan Roese <sr@denx.de> wrote:
>>> Hi Bin,
>>>
>>>
>>> On 03.12.2015 14:34, Bin Meng wrote:
>>>>
>>>> Hi Stefan, Simon,
>>>>
>>>> On Mon, Oct 19, 2015 at 7:16 AM, Simon Glass <sjg@chromium.org> wrote:
>>>>>
>>>>> On 29 September 2015 at 23:00, Stefan Roese <sr@denx.de> wrote:
>>>>>>
>>>>>> The current "simple" address translation simple_bus_translate() is not
>>>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>>>> enables the optional use of the common fdt_translate_address() function
>>>>>> which handles this translation correctly.
>>>>>>
>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>>>> ---
>>>>>> 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(+)
>>>>>
>>>>>
>>>>> 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.
>>>
>>>
>>> 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>;
>>
>>>> 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:
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index f86365e..8930f34 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -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)
>
>   /* 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.

Thanks,
Stefan

[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()

[Bin Meng]

Hi Stefan,

On Fri, Dec 4, 2015 at 3:52 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Bin,
>
>
> On 04.12.2015 07:17, Bin Meng wrote:
>>
>> Hi,
>>
>> On Fri, Dec 4, 2015 at 1:31 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> Hi Stefan,
>>>
>>> On Thu, Dec 3, 2015 at 10:12 PM, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> Hi Bin,
>>>>
>>>>
>>>> On 03.12.2015 14:34, Bin Meng wrote:
>>>>>
>>>>>
>>>>> Hi Stefan, Simon,
>>>>>
>>>>> On Mon, Oct 19, 2015 at 7:16 AM, Simon Glass <sjg@chromium.org> wrote:
>>>>>>
>>>>>>
>>>>>> On 29 September 2015 at 23:00, Stefan Roese <sr@denx.de> wrote:
>>>>>>>
>>>>>>>
>>>>>>> The current "simple" address translation simple_bus_translate() is
>>>>>>> not
>>>>>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>>>>>> properties are used in many nodes (multiple tuples etc). This patch
>>>>>>> enables the optional use of the common fdt_translate_address()
>>>>>>> function
>>>>>>> which handles this translation correctly.
>>>>>>>
>>>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>>>>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>> ---
>>>>>>> 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(+)
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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.
>>>>
>>>>
>>>>
>>>> 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>;
>>>
>>>>> 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:
>>
>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>> index f86365e..8930f34 100644
>> --- a/common/fdt_support.c
>> +++ b/common/fdt_support.c
>> @@ -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)
>>
>>   /* 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.

Yep, actually I suspect there might still be other potential issue
with this dev_get_addr() API. As its return value is of type
fdt_addr_t which is phys_addr_t which can be either 32-bit or 64-bit,
but fdt_translate_address() always return u64. I see most APIs in
common/fdt_support.c accept or return a u64 address value.

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

Will do.

Regards,
Bin