[U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
The current "simple" address translation simple_bus_translate() is not
working on some platforms (e.g. MVEBU). As here more complex "ranges"
properties are used in many nodes (multiple tuples etc). This patch
enables the optional use of the common fdt_translate_address() function
which handles this translation correctly.
Signed-off-by: Stefan Roese
Cc: Simon Glass
Cc: Bin Meng
Cc: Marek Vasut
Cc: Masahiro Yamada
Cc: Stephen Warren
Cc: Lukasz Majewski
---
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
#include
+#include
#include
#include
#include
@@ -581,6 +582,25 @@ fdt_addr_t dev_get_addr(struct udevice *dev)
#if CONFIG_IS_ENABLED(OF_CONTROL)
fdt_addr_t addr;
+ if (CONFIG_IS_ENABLED(OF_TRANSLATE)) {
+ const fdt32_t *reg;
+
+ reg = fdt_getprop(gd->fdt_blob, dev->of_offset, "reg", NULL);
+ if (!reg)
+ return FDT_ADDR_T_NONE;
+
+ /*
+* Use the full-fledged translate function for complex
+* bus setups.
+*/
+ return fdt_translate_address((void *)gd->fdt_blob,
+dev->of_offset, reg);
+ }
+
+ /*
+* Use the "simple" translate function for less complex
+* bus setups.
+*/
addr = fdtdec_get_addr_size_auto_parent(gd->fdt_blob,
dev->parent->of_offset,
dev->of_offset, "reg",
--
2.5.3
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
Hi Stefan,
On 1 October 2015 at 07:59, Stefan Roese 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
On 29 September 2015 at 23:00, Stefan Roese wrote: > The current "simple" address translation simple_bus_translate() is not > working on some platforms (e.g. MVEBU). As here more complex "ranges" > properties are used in many nodes (multiple tuples etc). This patch > enables the optional use of the common fdt_translate_address() function > which handles this translation correctly. > > Signed-off-by: Stefan Roese > Cc: Simon Glass > Cc: Bin Meng > Cc: Marek Vasut > Cc: Masahiro Yamada > Cc: Stephen Warren > Cc: Lukasz Majewski > --- > 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
Hi Stefan, Simon, On Mon, Oct 19, 2015 at 7:16 AM, Simon Glass wrote: > On 29 September 2015 at 23:00, Stefan Roese wrote: >> The current "simple" address translation simple_bus_translate() is not >> working on some platforms (e.g. MVEBU). As here more complex "ranges" >> properties are used in many nodes (multiple tuples etc). This patch >> enables the optional use of the common fdt_translate_address() function >> which handles this translation correctly. >> >> Signed-off-by: Stefan Roese >> Cc: Simon Glass >> Cc: Bin Meng >> Cc: Marek Vasut >> Cc: Masahiro Yamada >> Cc: Stephen Warren >> Cc: Lukasz Majewski >> --- >> 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
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 wrote: On 29 September 2015 at 23:00, Stefan Roese wrote: The current "simple" address translation simple_bus_translate() is not working on some platforms (e.g. MVEBU). As here more complex "ranges" properties are used in many nodes (multiple tuples etc). This patch enables the optional use of the common fdt_translate_address() function which handles this translation correctly. Signed-off-by: Stefan Roese Cc: Simon Glass Cc: Bin Meng Cc: Marek Vasut Cc: Masahiro Yamada Cc: Stephen Warren Cc: Lukasz Majewski --- 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
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 wrote: On 29 September 2015 at 23:00, Stefan Roese wrote: The current "simple" address translation simple_bus_translate() is not working on some platforms (e.g. MVEBU). As here more complex "ranges" properties are used in many nodes (multiple tuples etc). This patch enables the optional use of the common fdt_translate_address() function which handles this translation correctly. Signed-off-by: Stefan Roese Cc: Simon Glass Cc: Bin Meng Cc: Marek Vasut Cc: Masahiro Yamada Cc: Stephen Warren Cc: Lukasz Majewski --- 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
Hi Stefan,
On Thu, Dec 3, 2015 at 10:12 PM, 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 wrote:
>>>
>>> On 29 September 2015 at 23:00, Stefan Roese wrote:
The current "simple" address translation simple_bus_translate() is not
working on some platforms (e.g. MVEBU). As here more complex "ranges"
properties are used in many nodes (multiple tuples etc). This patch
enables the optional use of the common fdt_translate_address() function
which handles this translation correctly.
Signed-off-by: Stefan Roese
Cc: Simon Glass
Cc: Bin Meng
Cc: Marek Vasut
Cc: Masahiro Yamada
Cc: Stephen Warren
Cc: Lukasz Majewski
---
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 = <0x0200 0x0 0x4000 0x4000 0 0x8000
96 0x4200 0x0 0xc000 0xc000 0 0x2000
97 0x0100 0x0 0x2000 0x2000 0 0xe000>;
98
99 pcie@17,0 {
100 #address-cells = <3>;
101 #size-cells = <2>;
102 compatible = "pci-bridge";
103 u-boot,dm-pre-reloc;
104 reg = <0xb800 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
Hi,
On Fri, Dec 4, 2015 at 1:31 PM, Bin Meng wrote:
> Hi Stefan,
>
> On Thu, Dec 3, 2015 at 10:12 PM, 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 wrote:
On 29 September 2015 at 23:00, Stefan Roese wrote:
>
> The current "simple" address translation simple_bus_translate() is not
> working on some platforms (e.g. MVEBU). As here more complex "ranges"
> properties are used in many nodes (multiple tuples etc). This patch
> enables the optional use of the common fdt_translate_address() function
> which handles this translation correctly.
>
> Signed-off-by: Stefan Roese
> Cc: Simon Glass
> Cc: Bin Meng
> Cc: Marek Vasut
> Cc: Masahiro Yamada
> Cc: Stephen Warren
> Cc: Lukasz Majewski
> ---
> 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 = <0x0200 0x0 0x4000 0x4000 0
> 0x8000
> 96 0x4200 0x0 0xc000 0xc000 0
> 0x2000
> 97 0x0100 0x0 0x2000 0x2000 0 0xe000>;
> 98
> 99 pcie@17,0 {
> 100 #address-cells = <3>;
> 101 #size-cells = <2>;
> 102 compatible = "pci-bridge";
> 103 u-boot,dm-pre-reloc;
> 104 reg = <0xb800 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
#include
#include
+#include
/**
* fdt_getprop_u32_default_node - Return a node's property or a default
@@ -945,7 +946,7 @@ void fdt_del_node_and_alias(void
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
Hi Bin,
On 04.12.2015 07:17, Bin Meng wrote:
Hi,
On Fri, Dec 4, 2015 at 1:31 PM, Bin Meng wrote:
Hi Stefan,
On Thu, Dec 3, 2015 at 10:12 PM, 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 wrote:
On 29 September 2015 at 23:00, Stefan Roese wrote:
The current "simple" address translation simple_bus_translate() is not
working on some platforms (e.g. MVEBU). As here more complex "ranges"
properties are used in many nodes (multiple tuples etc). This patch
enables the optional use of the common fdt_translate_address() function
which handles this translation correctly.
Signed-off-by: Stefan Roese
Cc: Simon Glass
Cc: Bin Meng
Cc: Marek Vasut
Cc: Masahiro Yamada
Cc: Stephen Warren
Cc: Lukasz Majewski
---
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 = <0x0200 0x0 0x4000 0x4000 0 0x8000
96 0x4200 0x0 0xc000 0xc000 0 0x2000
97 0x0100 0x0 0x2000 0x2000 0 0xe000>;
98
99 pcie@17,0 {
100 #address-cells = <3>;
101 #size-cells = <2>;
102 compatible = "pci-bridge";
103 u-boot,dm-pre-reloc;
104 reg = <0xb800 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
#include
#include
+#include
/**
* 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_ADDRFDT_ADDR_T_NONE
#define OF_CHECK_COUNTS(na, ns)((na) > 0 && (na) <=
OF_MAX_ADDR_CELLS && \
Re: [U-Boot] [PATCH v3] dm: core: Enable optional use of fdt_translate_address()
Hi Stefan,
On Fri, Dec 4, 2015 at 3:52 PM, Stefan Roese wrote:
> Hi Bin,
>
>
> On 04.12.2015 07:17, Bin Meng wrote:
>>
>> Hi,
>>
>> On Fri, Dec 4, 2015 at 1:31 PM, Bin Meng wrote:
>>>
>>> Hi Stefan,
>>>
>>> On Thu, Dec 3, 2015 at 10:12 PM, 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 wrote:
>>
>>
>> On 29 September 2015 at 23:00, Stefan Roese wrote:
>>>
>>>
>>> The current "simple" address translation simple_bus_translate() is
>>> not
>>> working on some platforms (e.g. MVEBU). As here more complex "ranges"
>>> properties are used in many nodes (multiple tuples etc). This patch
>>> enables the optional use of the common fdt_translate_address()
>>> function
>>> which handles this translation correctly.
>>>
>>> Signed-off-by: Stefan Roese
>>> Cc: Simon Glass
>>> Cc: Bin Meng
>>> Cc: Marek Vasut
>>> Cc: Masahiro Yamada
>>> Cc: Stephen Warren
>>> Cc: Lukasz Majewski
>>> ---
>>> 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 = <0x0200 0x0 0x4000 0x4000 0
>>> 0x8000
>>> 96 0x4200 0x0 0xc000 0xc000 0
>>> 0x2000
>>> 97 0x0100 0x0 0x2000 0x2000 0 0xe000>;
>>> 98
>>> 99 pcie@17,0 {
>>> 100 #address-cells = <3>;
>>> 101 #size-cells = <2>;
>>> 102 compatible = "pci-bridge";
>>> 103 u-boot,dm-pre-reloc;
>>> 104 reg = <0xb800 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
