[PATCH 1/2] of: addr: Abort address translation for parent nodes missing 'ranges'
The implementation of of_translate_one() was taken from the one in Linux kernel drivers/of/address.c, and the Linux one added a quirk for Apple Macs that don't have the <ranges> property in the parent node. Since U-Boot does not support Apple Macs, remove the comment block and adhere to the spec to abort the translation.
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
drivers/core/of_addr.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/drivers/core/of_addr.c b/drivers/core/of_addr.c index 5bc6ca1..36cdfd3 100644 --- a/drivers/core/of_addr.c +++ b/drivers/core/of_addr.c @@ -173,25 +173,13 @@ static int of_translate_one(const struct device_node *parent, int rone; u64 offset = OF_BAD_ADDR;
- /* - * Normally, an absence of a "ranges" property means we are - * crossing a non-translatable boundary, and thus the addresses - * below the current cannot be converted to CPU physical ones. - * Unfortunately, while this is very clear in the spec, it's not - * what Apple understood, and they do have things like /uni-n or - * /ht nodes with no "ranges" property and a lot of perfectly - * useable mapped devices below them. Thus we treat the absence of - * "ranges" as equivalent to an empty "ranges" property which means - * a 1:1 translation at that level. It's up to the caller not to try - * to translate addresses that aren't supposed to be translated in - * the first place. --BenH. - * - * As far as we know, this damage only exists on Apple machines, so - * This code is only enabled on powerpc. --gcl - */ - ranges = of_get_property(parent, rprop, &rlen); - if (ranges == NULL || rlen == 0) { + if (ranges == NULL) { + debug("no ranges; cannot translate\n"); + return 1; + } + + if (rlen == 0) { offset = of_read_number(addr, na); memset(addr, 0, pna * 4); debug("empty ranges; 1:1 translation\n");
'dma-ranges' frequently exists without parent nodes having 'dma-ranges'. While this is an error for 'ranges', this is fine because DMA capable devices always have a translatable DMA address. Also, with no 'dma-ranges' at all, the assumption is that DMA addresses are 1:1 with no restrictions unless perhaps the device itself has implicit restrictions.
This keeps in sync with Linux kernel commit:
81db12ee15cb: of/address: Translate 'dma-ranges' for parent nodes missing 'dma-ranges'
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
drivers/core/of_addr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/core/of_addr.c b/drivers/core/of_addr.c index 36cdfd3..b9abf90 100644 --- a/drivers/core/of_addr.c +++ b/drivers/core/of_addr.c @@ -174,12 +174,12 @@ static int of_translate_one(const struct device_node *parent, u64 offset = OF_BAD_ADDR;
ranges = of_get_property(parent, rprop, &rlen); - if (ranges == NULL) { + if (ranges == NULL && strcmp(rprop, "dma-ranges")) { debug("no ranges; cannot translate\n"); return 1; }
- if (rlen == 0) { + if (ranges == NULL || rlen == 0) { offset = of_read_number(addr, na); memset(addr, 0, pna * 4); debug("empty ranges; 1:1 translation\n");
On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote:
'dma-ranges' frequently exists without parent nodes having 'dma-ranges'. While this is an error for 'ranges', this is fine because DMA capable devices always have a translatable DMA address. Also, with no 'dma-ranges' at all, the assumption is that DMA addresses are 1:1 with no restrictions unless perhaps the device itself has implicit restrictions.
This keeps in sync with Linux kernel commit:
81db12ee15cb: of/address: Translate 'dma-ranges' for parent nodes missing 'dma-ranges'
Signed-off-by: Bin Meng bmeng.cn@gmail.com
drivers/core/of_addr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote:
The implementation of of_translate_one() was taken from the one in Linux kernel drivers/of/address.c, and the Linux one added a quirk for Apple Macs that don't have the <ranges> property in the parent node. Since U-Boot does not support Apple Macs, remove the comment block and adhere to the spec to abort the translation.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
drivers/core/of_addr.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Hi Bin,
On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote:
On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote:
The implementation of of_translate_one() was taken from the one in Linux kernel drivers/of/address.c, and the Linux one added a quirk for Apple Macs that don't have the <ranges> property in the parent node. Since U-Boot does not support Apple Macs, remove the comment block and adhere to the spec to abort the translation.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
drivers/core/of_addr.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Unfortunately this seems to cause a test failure for ut_dm_fdt_translation. Can you please take a look?
Regards, Simon
+Dario Binacchi
On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote:
Hi Bin,
On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote:
On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote:
The implementation of of_translate_one() was taken from the one in Linux kernel drivers/of/address.c, and the Linux one added a quirk for Apple Macs that don't have the <ranges> property in the parent node. Since U-Boot does not support Apple Macs, remove the comment block and adhere to the spec to abort the translation.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
drivers/core/of_addr.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Unfortunately this seems to cause a test failure for ut_dm_fdt_translation. Can you please take a look?
It seems the no "ranges" property was intentionally removed by the following commit:
commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0>
The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>.
It looks the commit was needed for beaglebone board.
Dario, could you please comment on why U-Boot needs to done like this, while Linux kernel has this check? Is the beaglebone board not working in Linux?
Regards, Bin
+Tom Rini too
On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote:
+Dario Binacchi
On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote:
Hi Bin,
On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote:
On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote:
The implementation of of_translate_one() was taken from the one in Linux kernel drivers/of/address.c, and the Linux one added a quirk for Apple Macs that don't have the <ranges> property in the parent node. Since U-Boot does not support Apple Macs, remove the comment block and adhere to the spec to abort the translation.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
drivers/core/of_addr.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Unfortunately this seems to cause a test failure for ut_dm_fdt_translation. Can you please take a look?
It seems the no "ranges" property was intentionally removed by the following commit:
commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0> The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>.It looks the commit was needed for beaglebone board.
Dario, could you please comment on why U-Boot needs to done like this, while Linux kernel has this check? Is the beaglebone board not working in Linux?
Regards, Bin
Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto:
+Tom Rini too
On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote:
+Dario Binacchi
On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote:
Hi Bin,
On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote:
On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote:
The implementation of of_translate_one() was taken from the one in Linux kernel drivers/of/address.c, and the Linux one added a quirk for Apple Macs that don't have the <ranges> property in the parent node. Since U-Boot does not support Apple Macs, remove the comment block and adhere to the spec to abort the translation.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
drivers/core/of_addr.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Unfortunately this seems to cause a test failure for ut_dm_fdt_translation. Can you please take a look?
It seems the no "ranges" property was intentionally removed by the following commit:
commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0> The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>.It looks the commit was needed for beaglebone board.
Dario, could you please comment on why U-Boot needs to done like this, while Linux kernel has this check? Is the beaglebone board not working in Linux?
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Thanks and regards, Dario
Regards, Bin
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote:
Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto:
+Tom Rini too
On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote:
+Dario Binacchi
On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote:
Hi Bin,
On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote:
On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote:
The implementation of of_translate_one() was taken from the one in Linux kernel drivers/of/address.c, and the Linux one added a quirk for Apple Macs that don't have the <ranges> property in the parent node. Since U-Boot does not support Apple Macs, remove the comment block and adhere to the spec to abort the translation.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
drivers/core/of_addr.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Unfortunately this seems to cause a test failure for ut_dm_fdt_translation. Can you please take a look?
It seems the no "ranges" property was intentionally removed by the following commit:
commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0> The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>.It looks the commit was needed for beaglebone board.
Dario, could you please comment on why U-Boot needs to done like this, while Linux kernel has this check? Is the beaglebone board not working in Linux?
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
Regards, Bin
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote:
Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto:
+Tom Rini too
On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote:
+Dario Binacchi
On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote:
Hi Bin,
On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote:
On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > The implementation of of_translate_one() was taken from the one in > Linux kernel drivers/of/address.c, and the Linux one added a quirk > for Apple Macs that don't have the <ranges> property in the parent > node. Since U-Boot does not support Apple Macs, remove the comment > block and adhere to the spec to abort the translation. > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > --- > > drivers/core/of_addr.c | 24 ++++++------------------ > 1 file changed, 6 insertions(+), 18 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Unfortunately this seems to cause a test failure for ut_dm_fdt_translation. Can you please take a look?
It seems the no "ranges" property was intentionally removed by the following commit:
commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0> The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>.It looks the commit was needed for beaglebone board.
Dario, could you please comment on why U-Boot needs to done like this, while Linux kernel has this check? Is the beaglebone board not working in Linux?
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Best regards, Dario
Regards, Bin
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote:
Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto:
+Tom Rini too
On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote:
+Dario Binacchi
On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote:
Hi Bin,
On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > The implementation of of_translate_one() was taken from the one in > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > for Apple Macs that don't have the <ranges> property in the parent > > node. Since U-Boot does not support Apple Macs, remove the comment > > block and adhere to the spec to abort the translation. > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > --- > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > 1 file changed, 6 insertions(+), 18 deletions(-) > > Reviewed-by: Simon Glass sjg@chromium.org
Unfortunately this seems to cause a test failure for ut_dm_fdt_translation. Can you please take a look?
It seems the no "ranges" property was intentionally removed by the following commit:
commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0> The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>.It looks the commit was needed for beaglebone board.
Dario, could you please comment on why U-Boot needs to done like this, while Linux kernel has this check? Is the beaglebone board not working in Linux?
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
Maybe the beagleboard needs to use another API to decode the address? What API is used in Linux?
Regards, Bin
Hi Bin,
Il 17/03/2021 02:26 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote:
Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto:
+Tom Rini too
On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote:
+Dario Binacchi
On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > Hi Bin, > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > The implementation of of_translate_one() was taken from the one in > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > for Apple Macs that don't have the <ranges> property in the parent > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > block and adhere to the spec to abort the translation. > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > --- > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > Reviewed-by: Simon Glass sjg@chromium.org > > Unfortunately this seems to cause a test failure for > ut_dm_fdt_translation. Can you please take a look?
It seems the no "ranges" property was intentionally removed by the following commit:
commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0> The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>.It looks the commit was needed for beaglebone board.
Dario, could you please comment on why U-Boot needs to done like this, while Linux kernel has this check? Is the beaglebone board not working in Linux?
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
Maybe the beagleboard needs to use another API to decode the address? What API is used in Linux?
I'll do some checking, although I think the Linux device binding / probing flow is more complex than u-boot.
Thanks and regards, Dario
Regards, Bin
On Thu, Mar 18, 2021 at 08:27:49AM +0100, Dario Binacchi wrote:
Hi Bin,
Il 17/03/2021 02:26 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote:
Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto:
+Tom Rini too
On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote: > > +Dario Binacchi > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > > > Hi Bin, > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > The implementation of of_translate_one() was taken from the one in > > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > > for Apple Macs that don't have the <ranges> property in the parent > > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > > block and adhere to the spec to abort the translation. > > > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > > --- > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > Reviewed-by: Simon Glass sjg@chromium.org > > > > Unfortunately this seems to cause a test failure for > > ut_dm_fdt_translation. Can you please take a look? > > It seems the no "ranges" property was intentionally removed by the > following commit: > > commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc > Author: Dario Binacchi dariobin@libero.it > Date: Wed Dec 30 00:16:21 2020 +0100 > > fdt: translate address if #size-cells = <0> > > The __of_translate_address routine translates an address from the > device tree into a CPU physical address. A note in the description of > the routine explains that the crossing of any level with > since inherited from IBM. This does not happen for Texas Instruments, or > at least for the beaglebone device tree. Without this patch, in fact, > the translation into physical addresses of the registers contained in the > am33xx-clocks.dtsi nodes would not be possible. They all have a parent > with #size-cells = <0>. > > It looks the commit was needed for beaglebone board. > > Dario, could you please comment on why U-Boot needs to done like this, > while Linux kernel has this check? Is the beaglebone board not working > in Linux? >
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
Maybe the beagleboard needs to use another API to decode the address? What API is used in Linux?
I'll do some checking, although I think the Linux device binding / probing flow is more complex than u-boot.
I guess at the high level, we need to understand what's going on here and why. With the whole "dt describes hardware", it shouldn't matter so much how Linux does its probing, both cases should work with the same DT, so someones (our?) logic needs to be corrected somewhere along the line.
Hi Tom,
Il 18/03/2021 20:51 Tom Rini trini@konsulko.com ha scritto:
On Thu, Mar 18, 2021 at 08:27:49AM +0100, Dario Binacchi wrote:
Hi Bin,
Il 17/03/2021 02:26 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote:
> Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto: > > > +Tom Rini too > > > On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote: > > > > +Dario Binacchi > > > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > > > > > Hi Bin, > > > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > > > The implementation of of_translate_one() was taken from the one in > > > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > > > for Apple Macs that don't have the <ranges> property in the parent > > > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > > > block and adhere to the spec to abort the translation. > > > > > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > > > --- > > > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > > > Reviewed-by: Simon Glass sjg@chromium.org > > > > > > Unfortunately this seems to cause a test failure for > > > ut_dm_fdt_translation. Can you please take a look? > > > > It seems the no "ranges" property was intentionally removed by the > > following commit: > > > > commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc > > Author: Dario Binacchi dariobin@libero.it > > Date: Wed Dec 30 00:16:21 2020 +0100 > > > > fdt: translate address if #size-cells = <0> > > > > The __of_translate_address routine translates an address from the > > device tree into a CPU physical address. A note in the description of > > the routine explains that the crossing of any level with > > since inherited from IBM. This does not happen for Texas Instruments, or > > at least for the beaglebone device tree. Without this patch, in fact, > > the translation into physical addresses of the registers contained in the > > am33xx-clocks.dtsi nodes would not be possible. They all have a parent > > with #size-cells = <0>. > > > > It looks the commit was needed for beaglebone board. > > > > Dario, could you please comment on why U-Boot needs to done like this, > > while Linux kernel has this check? Is the beaglebone board not working > > in Linux? > >
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
Maybe the beagleboard needs to use another API to decode the address? What API is used in Linux?
I'll do some checking, although I think the Linux device binding / probing flow is more complex than u-boot.
I guess at the high level, we need to understand what's going on here and why. With the whole "dt describes hardware", it shouldn't matter so much how Linux does its probing, both cases should work with the same DT, so someones (our?) logic needs to be corrected somewhere along the line.
My commit d64b9cdcd4 (fdt: translate address if #size-cells = <0>) previously reported by Bin is partial. In the missing lines it is explained why I removed the of_empty_ranges_quirk(). It itself also differed from the Linux kernel version before my commit.
Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0>
The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>.
The CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol makes translation possible even in the case of crossing levels with #size-cells = <0>.
The patch acts conservatively on address translation, except for removing a check within the of_translate_one function in the drivers/core/of_addr.c file:
+ ranges = of_get_property(parent, rprop, &rlen); - if (ranges == NULL && !of_empty_ranges_quirk(parent)) { - debug("no ranges; cannot translate\n"); - return 1; - } if (ranges == NULL || rlen == 0) { offset = of_read_number(addr, na); memset(addr, 0, pna * 4); debug("empty ranges; 1:1 translation\n");
There are two reasons: 1 The function of_empty_ranges_quirk always returns false, invalidating the following if statement in case of null ranges. Therefore one of the two checks is useless.
2 The implementation of the of_translate_one function found in the common/fdt_support.c file has removed this check while keeping the one about the 1:1 translation.
The patch adds a test and modifies a check for the correctness of an address in the case of enabling translation also for zero size cells. The added test checks translations of addresses generated by nodes of a device tree similar to those you can find in the files am33xx.dtsi and am33xx-clocks.dtsi for which the patch was created.
The patch was also tested on a beaglebone black board. The addresses generated for the registers of the loaded drivers are those specified by the AM335x reference manual.
I didn't want to wire any peripheral base addresses but just retrieve them from the device tree. To achieve this, this patch was crucial. I think this does not happen in the kernel, where such structures contain base addresses without querying the device tree. https://elixir.bootlin.com/linux/latest/source/drivers/clk/ti/clk-33xx.c#L23...
Thanks and regards, Dario
-- Tom
Hi Dario,
On Sun, Mar 21, 2021 at 11:19 PM Dario Binacchi dariobin@libero.it wrote:
Hi Tom,
Il 18/03/2021 20:51 Tom Rini trini@konsulko.com ha scritto:
On Thu, Mar 18, 2021 at 08:27:49AM +0100, Dario Binacchi wrote:
Hi Bin,
Il 17/03/2021 02:26 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote: > > > > Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto: > > > > > > +Tom Rini too > > > > > > On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > +Dario Binacchi > > > > > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > > > > > > > Hi Bin, > > > > > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > > > > > The implementation of of_translate_one() was taken from the one in > > > > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > > > > for Apple Macs that don't have the <ranges> property in the parent > > > > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > > > > block and adhere to the spec to abort the translation. > > > > > > > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > > > > --- > > > > > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > > > > > Reviewed-by: Simon Glass sjg@chromium.org > > > > > > > > Unfortunately this seems to cause a test failure for > > > > ut_dm_fdt_translation. Can you please take a look? > > > > > > It seems the no "ranges" property was intentionally removed by the > > > following commit: > > > > > > commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc > > > Author: Dario Binacchi dariobin@libero.it > > > Date: Wed Dec 30 00:16:21 2020 +0100 > > > > > > fdt: translate address if #size-cells = <0> > > > > > > The __of_translate_address routine translates an address from the > > > device tree into a CPU physical address. A note in the description of > > > the routine explains that the crossing of any level with > > > since inherited from IBM. This does not happen for Texas Instruments, or > > > at least for the beaglebone device tree. Without this patch, in fact, > > > the translation into physical addresses of the registers contained in the > > > am33xx-clocks.dtsi nodes would not be possible. They all have a parent > > > with #size-cells = <0>. > > > > > > It looks the commit was needed for beaglebone board. > > > > > > Dario, could you please comment on why U-Boot needs to done like this, > > > while Linux kernel has this check? Is the beaglebone board not working > > > in Linux? > > > > > Beaglebone is working in Linux, but I think Linux walks the device tree less > fully than u-boot. > I was surprised by the address translation error when traversing nodes with > size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS > symbol to fix the issue, not enabled by default, thus making the change backwards > compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
Maybe the beagleboard needs to use another API to decode the address? What API is used in Linux?
I'll do some checking, although I think the Linux device binding / probing flow is more complex than u-boot.
I guess at the high level, we need to understand what's going on here and why. With the whole "dt describes hardware", it shouldn't matter so much how Linux does its probing, both cases should work with the same DT, so someones (our?) logic needs to be corrected somewhere along the line.
My commit d64b9cdcd4 (fdt: translate address if #size-cells = <0>) previously reported by Bin is partial. In the missing lines it is explained why I removed the of_empty_ranges_quirk(). It itself also differed from the Linux kernel version before my commit.
Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0> The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>. The CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol makes translation possible even in the case of crossing levels with #size-cells = <0>. The patch acts conservatively on address translation, except for removing a check within the of_translate_one function in the drivers/core/of_addr.c file: + ranges = of_get_property(parent, rprop, &rlen); - if (ranges == NULL && !of_empty_ranges_quirk(parent)) { - debug("no ranges; cannot translate\n"); - return 1; - } if (ranges == NULL || rlen == 0) { offset = of_read_number(addr, na); memset(addr, 0, pna * 4); debug("empty ranges; 1:1 translation\n"); There are two reasons: 1 The function of_empty_ranges_quirk always returns false, invalidating the following if statement in case of null ranges. Therefore one of the two checks is useless. 2 The implementation of the of_translate_one function found in the common/fdt_support.c file has removed this check while keeping the one about the 1:1 translation. The patch adds a test and modifies a check for the correctness of an address in the case of enabling translation also for zero size cells. The added test checks translations of addresses generated by nodes of a device tree similar to those you can find in the files am33xx.dtsi and am33xx-clocks.dtsi for which the patch was created. The patch was also tested on a beaglebone black board. The addresses generated for the registers of the loaded drivers are those specified by the AM335x reference manual.I didn't want to wire any peripheral base addresses but just retrieve them from the device tree. To achieve this, this patch was crucial. I think this does not happen in the kernel, where such structures contain base addresses without querying the device tree. https://elixir.bootlin.com/linux/latest/source/drivers/clk/ti/clk-33xx.c#L23...
Thanks for the details. So Linux kernel is using hardcoded address from the driver file, instead of getting it from device tree, which masks the issue in the kernel.
Could you please send a patch (the one that currently U-Boot does) to the Linux kernel then?
Regards, Bin
Hi Bin,
Il 22/03/2021 02:25 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Sun, Mar 21, 2021 at 11:19 PM Dario Binacchi dariobin@libero.it wrote:
Hi Tom,
Il 18/03/2021 20:51 Tom Rini trini@konsulko.com ha scritto:
On Thu, Mar 18, 2021 at 08:27:49AM +0100, Dario Binacchi wrote:
Hi Bin,
Il 17/03/2021 02:26 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
> Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto: > > > Hi Dario, > > On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote: > > > > > > > Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto: > > > > > > > > > +Tom Rini too > > > > > > > > > On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > +Dario Binacchi > > > > > > > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > > > > > > > > > Hi Bin, > > > > > > > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > > > > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > > > > > > > The implementation of of_translate_one() was taken from the one in > > > > > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > > > > > for Apple Macs that don't have the <ranges> property in the parent > > > > > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > > > > > block and adhere to the spec to abort the translation. > > > > > > > > > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > > > > > --- > > > > > > > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > > > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > > > > > > > Reviewed-by: Simon Glass sjg@chromium.org > > > > > > > > > > Unfortunately this seems to cause a test failure for > > > > > ut_dm_fdt_translation. Can you please take a look? > > > > > > > > It seems the no "ranges" property was intentionally removed by the > > > > following commit: > > > > > > > > commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc > > > > Author: Dario Binacchi dariobin@libero.it > > > > Date: Wed Dec 30 00:16:21 2020 +0100 > > > > > > > > fdt: translate address if #size-cells = <0> > > > > > > > > The __of_translate_address routine translates an address from the > > > > device tree into a CPU physical address. A note in the description of > > > > the routine explains that the crossing of any level with > > > > since inherited from IBM. This does not happen for Texas Instruments, or > > > > at least for the beaglebone device tree. Without this patch, in fact, > > > > the translation into physical addresses of the registers contained in the > > > > am33xx-clocks.dtsi nodes would not be possible. They all have a parent > > > > with #size-cells = <0>. > > > > > > > > It looks the commit was needed for beaglebone board. > > > > > > > > Dario, could you please comment on why U-Boot needs to done like this, > > > > while Linux kernel has this check? Is the beaglebone board not working > > > > in Linux? > > > > > > > > Beaglebone is working in Linux, but I think Linux walks the device tree less > > fully than u-boot. > > I was surprised by the address translation error when traversing nodes with > > size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS > > symbol to fix the issue, not enabled by default, thus making the change backwards > > compatible. > > Could you please prepare a patch against upstream Linux kernel, so > that U-Boot can be in sync with it?
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
Maybe the beagleboard needs to use another API to decode the address? What API is used in Linux?
I'll do some checking, although I think the Linux device binding / probing flow is more complex than u-boot.
I guess at the high level, we need to understand what's going on here and why. With the whole "dt describes hardware", it shouldn't matter so much how Linux does its probing, both cases should work with the same DT, so someones (our?) logic needs to be corrected somewhere along the line.
My commit d64b9cdcd4 (fdt: translate address if #size-cells = <0>) previously reported by Bin is partial. In the missing lines it is explained why I removed the of_empty_ranges_quirk(). It itself also differed from the Linux kernel version before my commit.
Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0> The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>. The CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol makes translation possible even in the case of crossing levels with #size-cells = <0>. The patch acts conservatively on address translation, except for removing a check within the of_translate_one function in the drivers/core/of_addr.c file: + ranges = of_get_property(parent, rprop, &rlen); - if (ranges == NULL && !of_empty_ranges_quirk(parent)) { - debug("no ranges; cannot translate\n"); - return 1; - } if (ranges == NULL || rlen == 0) { offset = of_read_number(addr, na); memset(addr, 0, pna * 4); debug("empty ranges; 1:1 translation\n"); There are two reasons: 1 The function of_empty_ranges_quirk always returns false, invalidating the following if statement in case of null ranges. Therefore one of the two checks is useless. 2 The implementation of the of_translate_one function found in the common/fdt_support.c file has removed this check while keeping the one about the 1:1 translation. The patch adds a test and modifies a check for the correctness of an address in the case of enabling translation also for zero size cells. The added test checks translations of addresses generated by nodes of a device tree similar to those you can find in the files am33xx.dtsi and am33xx-clocks.dtsi for which the patch was created. The patch was also tested on a beaglebone black board. The addresses generated for the registers of the loaded drivers are those specified by the AM335x reference manual.I didn't want to wire any peripheral base addresses but just retrieve them from the device tree. To achieve this, this patch was crucial. I think this does not happen in the kernel, where such structures contain base addresses without querying the device tree. https://elixir.bootlin.com/linux/latest/source/drivers/clk/ti/clk-33xx.c#L23...
Thanks for the details. So Linux kernel is using hardcoded address from the driver file, instead of getting it from device tree, which masks the issue in the kernel.
Could you please send a patch (the one that currently U-Boot does) to the Linux kernel then?
I'll do it as soon as possible. Can I CC your email address?
Thanks and regards, Dario
Regards, Bin
Hi Dario,
On Wed, Mar 24, 2021 at 1:27 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 22/03/2021 02:25 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Sun, Mar 21, 2021 at 11:19 PM Dario Binacchi dariobin@libero.it wrote:
Hi Tom,
Il 18/03/2021 20:51 Tom Rini trini@konsulko.com ha scritto:
On Thu, Mar 18, 2021 at 08:27:49AM +0100, Dario Binacchi wrote:
Hi Bin,
Il 17/03/2021 02:26 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote: > > Hi Bin, > > > Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto: > > > > > > Hi Dario, > > > > On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote: > > > > > > > > > > Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto: > > > > > > > > > > > > +Tom Rini too > > > > > > > > > > > > On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > > > +Dario Binacchi > > > > > > > > > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > > > > > > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > > > > > > > > > The implementation of of_translate_one() was taken from the one in > > > > > > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > > > > > > for Apple Macs that don't have the <ranges> property in the parent > > > > > > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > > > > > > block and adhere to the spec to abort the translation. > > > > > > > > > > > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > > > > > > --- > > > > > > > > > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > > > > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > > > > > > > > > Reviewed-by: Simon Glass sjg@chromium.org > > > > > > > > > > > > Unfortunately this seems to cause a test failure for > > > > > > ut_dm_fdt_translation. Can you please take a look? > > > > > > > > > > It seems the no "ranges" property was intentionally removed by the > > > > > following commit: > > > > > > > > > > commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc > > > > > Author: Dario Binacchi dariobin@libero.it > > > > > Date: Wed Dec 30 00:16:21 2020 +0100 > > > > > > > > > > fdt: translate address if #size-cells = <0> > > > > > > > > > > The __of_translate_address routine translates an address from the > > > > > device tree into a CPU physical address. A note in the description of > > > > > the routine explains that the crossing of any level with > > > > > since inherited from IBM. This does not happen for Texas Instruments, or > > > > > at least for the beaglebone device tree. Without this patch, in fact, > > > > > the translation into physical addresses of the registers contained in the > > > > > am33xx-clocks.dtsi nodes would not be possible. They all have a parent > > > > > with #size-cells = <0>. > > > > > > > > > > It looks the commit was needed for beaglebone board. > > > > > > > > > > Dario, could you please comment on why U-Boot needs to done like this, > > > > > while Linux kernel has this check? Is the beaglebone board not working > > > > > in Linux? > > > > > > > > > > > Beaglebone is working in Linux, but I think Linux walks the device tree less > > > fully than u-boot. > > > I was surprised by the address translation error when traversing nodes with > > > size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS > > > symbol to fix the issue, not enabled by default, thus making the change backwards > > > compatible. > > > > Could you please prepare a patch against upstream Linux kernel, so > > that U-Boot can be in sync with it? > > With pleasure. But how do I justify the patch since it doesn't fix any bugs? > Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
Maybe the beagleboard needs to use another API to decode the address? What API is used in Linux?
I'll do some checking, although I think the Linux device binding / probing flow is more complex than u-boot.
I guess at the high level, we need to understand what's going on here and why. With the whole "dt describes hardware", it shouldn't matter so much how Linux does its probing, both cases should work with the same DT, so someones (our?) logic needs to be corrected somewhere along the line.
My commit d64b9cdcd4 (fdt: translate address if #size-cells = <0>) previously reported by Bin is partial. In the missing lines it is explained why I removed the of_empty_ranges_quirk(). It itself also differed from the Linux kernel version before my commit.
Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0> The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>. The CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol makes translation possible even in the case of crossing levels with #size-cells = <0>. The patch acts conservatively on address translation, except for removing a check within the of_translate_one function in the drivers/core/of_addr.c file: + ranges = of_get_property(parent, rprop, &rlen); - if (ranges == NULL && !of_empty_ranges_quirk(parent)) { - debug("no ranges; cannot translate\n"); - return 1; - } if (ranges == NULL || rlen == 0) { offset = of_read_number(addr, na); memset(addr, 0, pna * 4); debug("empty ranges; 1:1 translation\n"); There are two reasons: 1 The function of_empty_ranges_quirk always returns false, invalidating the following if statement in case of null ranges. Therefore one of the two checks is useless. 2 The implementation of the of_translate_one function found in the common/fdt_support.c file has removed this check while keeping the one about the 1:1 translation. The patch adds a test and modifies a check for the correctness of an address in the case of enabling translation also for zero size cells. The added test checks translations of addresses generated by nodes of a device tree similar to those you can find in the files am33xx.dtsi and am33xx-clocks.dtsi for which the patch was created. The patch was also tested on a beaglebone black board. The addresses generated for the registers of the loaded drivers are those specified by the AM335x reference manual.I didn't want to wire any peripheral base addresses but just retrieve them from the device tree. To achieve this, this patch was crucial. I think this does not happen in the kernel, where such structures contain base addresses without querying the device tree. https://elixir.bootlin.com/linux/latest/source/drivers/clk/ti/clk-33xx.c#L23...
Thanks for the details. So Linux kernel is using hardcoded address from the driver file, instead of getting it from device tree, which masks the issue in the kernel.
Could you please send a patch (the one that currently U-Boot does) to the Linux kernel then?
I'll do it as soon as possible.
Thanks!
Can I CC your email address?
Sure.
Regards, Bin
On Tue, Mar 16, 2021 at 8:26 PM Bin Meng bmeng.cn@gmail.com wrote:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote:
Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto:
+Tom Rini too
On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote:
+Dario Binacchi
On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > Hi Bin, > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > The implementation of of_translate_one() was taken from the one in > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > for Apple Macs that don't have the <ranges> property in the parent > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > block and adhere to the spec to abort the translation. > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > --- > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > Reviewed-by: Simon Glass sjg@chromium.org > > Unfortunately this seems to cause a test failure for > ut_dm_fdt_translation. Can you please take a look?
It seems the no "ranges" property was intentionally removed by the following commit:
commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc Author: Dario Binacchi dariobin@libero.it Date: Wed Dec 30 00:16:21 2020 +0100
fdt: translate address if #size-cells = <0> The __of_translate_address routine translates an address from the device tree into a CPU physical address. A note in the description of the routine explains that the crossing of any level with since inherited from IBM. This does not happen for Texas Instruments, or at least for the beaglebone device tree. Without this patch, in fact, the translation into physical addresses of the registers contained in the am33xx-clocks.dtsi nodes would not be possible. They all have a parent with #size-cells = <0>.It looks the commit was needed for beaglebone board.
Dario, could you please comment on why U-Boot needs to done like this, while Linux kernel has this check? Is the beaglebone board not working in Linux?
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
I've replied on that patch...
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
IMO, yes it is. Simply put, 'ranges' must be present to be translatable. Using '#size-cells == 0' is at least questionable, but could maybe be supported (depends what happens with non-empty 'ranges').
The 'fix' makes every 'reg' translatable. Give the function an I2C address 'reg' and you'll get back 'I2C controller address + I2C address'.
Maybe the beagleboard needs to use another API to decode the address? What API is used in Linux?
The quirk for the platform should live in the code for the platform.
Rob
Il 06/04/2021 16:26 Rob Herring robh@kernel.org ha scritto:
On Tue, Mar 16, 2021 at 8:26 PM Bin Meng bmeng.cn@gmail.com wrote:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote:
Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto:
+Tom Rini too
On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote: > > +Dario Binacchi > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > > > Hi Bin, > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > The implementation of of_translate_one() was taken from the one in > > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > > for Apple Macs that don't have the <ranges> property in the parent > > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > > block and adhere to the spec to abort the translation. > > > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > > --- > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > Reviewed-by: Simon Glass sjg@chromium.org > > > > Unfortunately this seems to cause a test failure for > > ut_dm_fdt_translation. Can you please take a look? > > It seems the no "ranges" property was intentionally removed by the > following commit: > > commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc > Author: Dario Binacchi dariobin@libero.it > Date: Wed Dec 30 00:16:21 2020 +0100 > > fdt: translate address if #size-cells = <0> > > The __of_translate_address routine translates an address from the > device tree into a CPU physical address. A note in the description of > the routine explains that the crossing of any level with > since inherited from IBM. This does not happen for Texas Instruments, or > at least for the beaglebone device tree. Without this patch, in fact, > the translation into physical addresses of the registers contained in the > am33xx-clocks.dtsi nodes would not be possible. They all have a parent > with #size-cells = <0>. > > It looks the commit was needed for beaglebone board. > > Dario, could you please comment on why U-Boot needs to done like this, > while Linux kernel has this check? Is the beaglebone board not working > in Linux? >
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
I've replied on that patch...
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
IMO, yes it is. Simply put, 'ranges' must be present to be translatable. Using '#size-cells == 0' is at least questionable, but could maybe be supported (depends what happens with non-empty 'ranges').
The 'fix' makes every 'reg' translatable. Give the function an I2C address 'reg' and you'll get back 'I2C controller address + I2C address'.
IMHO this could mean that using size-cells = <0> in the prcm_clocks and scm_clocks nodes is perhaps not quite correct. We need an address translation and if possible, better without having to develop code to handle this special case.
With the following changes in the am33xx-l4.dtsi and am33xx-clocks.dtsi files we get an address translation that does not require special case handling code and the device tree would not be an exception to what we usually see. I have tested these changes in the kernel and they seem to work.
diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi index 1fb22088caeb..59b0a0cf211e 100644 --- a/arch/arm/boot/dts/am33xx-l4.dtsi +++ b/arch/arm/boot/dts/am33xx-l4.dtsi @@ -110,7 +110,8 @@
prcm_clocks: clocks { #address-cells = <1>; - #size-cells = <0>; + #size-cells = <1>; + ranges = <0 0 0x2000>; };
prcm_clockdomains: clockdomains { @@ -320,7 +321,8 @@
scm_clocks: clocks { #address-cells = <1>; - #size-cells = <0>; + #size-cells = <1>; + ranges = <0 0 0x800>; }; };
--- a/arch/arm/boot/dts/am33xx-clocks.dtsi +++ b/arch/arm/boot/dts/am33xx-clocks.dtsi @@ -10,7 +10,7 @@ compatible = "ti,mux-clock"; clocks = <&virt_19200000_ck>, <&virt_24000000_ck>, <&virt_25000000_ck>, <&virt_26000000_ck>; ti,bit-shift = <22>; - reg = <0x0040>; + reg = <0x0040 0x4>; };
adc_tsc_fck: adc_tsc_fck { @@ -98,7 +98,7 @@ compatible = "ti,gate-clock"; clocks = <&l4ls_gclk>; ti,bit-shift = <0>; - reg = <0x0664>; + reg = <0x0664 0x04>; };
ehrpwm1_tbclk: ehrpwm1_tbclk@44e10664 { @@ -106,7 +106,7 @@ compatible = "ti,gate-clock"; clocks = <&l4ls_gclk>; ti,bit-shift = <1>; - reg = <0x0664>; + reg = <0x0664 0x04>; };
ehrpwm2_tbclk: ehrpwm2_tbclk@44e10664 { @@ -114,7 +114,7 @@ compatible = "ti,gate-clock"; clocks = <&l4ls_gclk>; ti,bit-shift = <2>; - reg = <0x0664>; + reg = <0x0664 0x04>; }; }; &prcm_clocks { @@ -164,7 +164,7 @@ #clock-cells = <0>; compatible = "ti,am3-dpll-core-clock"; clocks = <&sys_clkin_ck>, <&sys_clkin_ck>; - reg = <0x0490>, <0x045c>, <0x0468>; + reg = <0x0490 0x04>, <0x045c 0x04>, <0x0468 0x04>; };
dpll_core_x2_ck: dpll_core_x2_ck { @@ -178,7 +178,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_core_x2_ck>; ti,max-div = <31>; - reg = <0x0480>; + reg = <0x0480 0x04>; ti,index-starts-at-one; };
@@ -187,7 +187,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_core_x2_ck>; ti,max-div = <31>; - reg = <0x0484>; + reg = <0x0484 0x04>; ti,index-starts-at-one; };
@@ -196,7 +196,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_core_x2_ck>; ti,max-div = <31>; - reg = <0x04d8>; + reg = <0x04d8 0x04>; ti,index-starts-at-one; };
@@ -204,7 +204,7 @@ #clock-cells = <0>; compatible = "ti,am3-dpll-clock"; clocks = <&sys_clkin_ck>, <&sys_clkin_ck>; - reg = <0x0488>, <0x0420>, <0x042c>; + reg = <0x0488 0x04>, <0x0420 0x04>, <0x042c 0x04>; };
dpll_mpu_m2_ck: dpll_mpu_m2_ck@4a8 { @@ -212,7 +212,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_mpu_ck>; ti,max-div = <31>; - reg = <0x04a8>; + reg = <0x04a8 0x04>; ti,index-starts-at-one; };
@@ -220,7 +220,7 @@ #clock-cells = <0>; compatible = "ti,am3-dpll-no-gate-clock"; clocks = <&sys_clkin_ck>, <&sys_clkin_ck>; - reg = <0x0494>, <0x0434>, <0x0440>; + reg = <0x0494 0x04>, <0x0434 0x04>, <0x0440 0x04>; };
dpll_ddr_m2_ck: dpll_ddr_m2_ck@4a0 { @@ -244,7 +244,7 @@ #clock-cells = <0>; compatible = "ti,am3-dpll-no-gate-clock"; clocks = <&sys_clkin_ck>, <&sys_clkin_ck>; - reg = <0x0498>, <0x0448>, <0x0454>; + reg = <0x0498 0x04>, <0x0448 0x04>, <0x0454 0x04>; };
dpll_disp_m2_ck: dpll_disp_m2_ck@4a4 { @@ -252,7 +252,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_disp_ck>; ti,max-div = <31>; - reg = <0x04a4>; + reg = <0x04a4 0x04>; ti,index-starts-at-one; ti,set-rate-parent; }; @@ -261,7 +261,7 @@ #clock-cells = <0>; compatible = "ti,am3-dpll-no-gate-j-type-clock"; clocks = <&sys_clkin_ck>, <&sys_clkin_ck>; - reg = <0x048c>, <0x0470>, <0x049c>; + reg = <0x048c 0x04>, <0x0470 0x04>, <0x049c 0x04>; };
dpll_per_m2_ck: dpll_per_m2_ck@4ac { @@ -269,7 +269,7 @@ compatible = "ti,divider-clock"; clocks = <&dpll_per_ck>; ti,max-div = <31>; - reg = <0x04ac>; + reg = <0x04ac 0x04>; ti,index-starts-at-one; };
@@ -317,7 +317,7 @@ #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&l3_gclk>, <&dpll_disp_m2_ck>; - reg = <0x0530>; + reg = <0x0530 0x04>; };
mmu_fck: mmu_fck@914 { @@ -325,56 +325,56 @@ compatible = "ti,gate-clock"; clocks = <&dpll_core_m4_ck>; ti,bit-shift = <1>; - reg = <0x0914>; + reg = <0x0914 0x04>; };
timer1_fck: timer1_fck@528 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>, <&tclkin_ck>, <&clk_rc32k_ck>, <&clk_32768_ck>; - reg = <0x0528>; + reg = <0x0528 0x04>; };
timer2_fck: timer2_fck@508 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x0508>; + reg = <0x0508 0x04>; };
timer3_fck: timer3_fck@50c { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x050c>; + reg = <0x050c 0x04>; };
timer4_fck: timer4_fck@510 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x0510>; + reg = <0x0510 0x04>; };
timer5_fck: timer5_fck@518 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x0518>; + reg = <0x0518 0x04>; };
timer6_fck: timer6_fck@51c { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x051c>; + reg = <0x051c 0x04>; };
timer7_fck: timer7_fck@504 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&tclkin_ck>, <&sys_clkin_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x0504>; + reg = <0x0504 0x04>; };
usbotg_fck: usbotg_fck@47c { @@ -382,7 +382,7 @@ compatible = "ti,gate-clock"; clocks = <&dpll_per_ck>; ti,bit-shift = <8>; - reg = <0x047c>; + reg = <0x047c 0x04>; };
dpll_core_m4_div2_ck: dpll_core_m4_div2_ck { @@ -398,14 +398,14 @@ compatible = "ti,gate-clock"; clocks = <&dpll_core_m4_div2_ck>; ti,bit-shift = <1>; - reg = <0x00e4>; + reg = <0x00e4 0x04>; };
wdt1_fck: wdt1_fck@538 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&clk_rc32k_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x0538>; + reg = <0x0538 0x04>; };
l4_rtc_gclk: l4_rtc_gclk { @@ -468,21 +468,21 @@ #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&dpll_core_m5_ck>, <&dpll_core_m4_ck>; - reg = <0x0520>; + reg = <0x0520 4>; };
gpio0_dbclk_mux_ck: gpio0_dbclk_mux_ck@53c { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&clk_rc32k_ck>, <&clk_32768_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>; - reg = <0x053c>; + reg = <0x053c 0x04>; };
lcd_gclk: lcd_gclk@534 { #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&dpll_disp_m2_ck>, <&dpll_core_m5_ck>, <&dpll_per_m2_ck>; - reg = <0x0534>; + reg = <0x0534 0x04>; ti,set-rate-parent; };
@@ -499,14 +499,14 @@ compatible = "ti,mux-clock"; clocks = <&dpll_core_m4_ck>, <&dpll_per_m2_ck>; ti,bit-shift = <1>; - reg = <0x052c>; + reg = <0x052c 0x04>; };
gfx_fck_div_ck: gfx_fck_div_ck@52c { #clock-cells = <0>; compatible = "ti,divider-clock"; clocks = <&gfx_fclk_clksel_ck>; - reg = <0x052c>; + reg = <0x052c 0x04>; ti,max-div = <2>; };
@@ -514,7 +514,7 @@ #clock-cells = <0>; compatible = "ti,mux-clock"; clocks = <&clk_32768_ck>, <&l3_gclk>, <&dpll_ddr_m2_ck>, <&dpll_per_m2_ck>, <&lcd_gclk>; - reg = <0x0700>; + reg = <0x0700 0x04>; };
clkout2_div_ck: clkout2_div_ck@700 { @@ -523,7 +523,7 @@ clocks = <&sysclkout_pre_ck>; ti,bit-shift = <3>; ti,max-div = <8>; - reg = <0x0700>; + reg = <0x0700 0x04>; };
clkout2_ck: clkout2_ck@700 { @@ -531,7 +531,7 @@ compatible = "ti,gate-clock"; clocks = <&clkout2_div_ck>; ti,bit-shift = <7>; - reg = <0x0700>; + reg = <0x0700 0x04>; }; };
Thanks and regards, Dario
Maybe the beagleboard needs to use another API to decode the address? What API is used in Linux?
The quirk for the platform should live in the code for the platform.
Rob
On Tue, Apr 6, 2021 at 4:53 PM Dario Binacchi dariobin@libero.it wrote:
Il 06/04/2021 16:26 Rob Herring robh@kernel.org ha scritto:
On Tue, Mar 16, 2021 at 8:26 PM Bin Meng bmeng.cn@gmail.com wrote:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote:
> Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto: > > > +Tom Rini too > > > On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote: > > > > +Dario Binacchi > > > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > > > > > Hi Bin, > > > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > > > The implementation of of_translate_one() was taken from the one in > > > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > > > for Apple Macs that don't have the <ranges> property in the parent > > > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > > > block and adhere to the spec to abort the translation. > > > > > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > > > --- > > > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > > > Reviewed-by: Simon Glass sjg@chromium.org > > > > > > Unfortunately this seems to cause a test failure for > > > ut_dm_fdt_translation. Can you please take a look? > > > > It seems the no "ranges" property was intentionally removed by the > > following commit: > > > > commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc > > Author: Dario Binacchi dariobin@libero.it > > Date: Wed Dec 30 00:16:21 2020 +0100 > > > > fdt: translate address if #size-cells = <0> > > > > The __of_translate_address routine translates an address from the > > device tree into a CPU physical address. A note in the description of > > the routine explains that the crossing of any level with > > since inherited from IBM. This does not happen for Texas Instruments, or > > at least for the beaglebone device tree. Without this patch, in fact, > > the translation into physical addresses of the registers contained in the > > am33xx-clocks.dtsi nodes would not be possible. They all have a parent > > with #size-cells = <0>. > > > > It looks the commit was needed for beaglebone board. > > > > Dario, could you please comment on why U-Boot needs to done like this, > > while Linux kernel has this check? Is the beaglebone board not working > > in Linux? > >
Beaglebone is working in Linux, but I think Linux walks the device tree less fully than u-boot. I was surprised by the address translation error when traversing nodes with size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS symbol to fix the issue, not enabled by default, thus making the change backwards compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
I've replied on that patch...
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
IMO, yes it is. Simply put, 'ranges' must be present to be translatable. Using '#size-cells == 0' is at least questionable, but could maybe be supported (depends what happens with non-empty 'ranges').
The 'fix' makes every 'reg' translatable. Give the function an I2C address 'reg' and you'll get back 'I2C controller address + I2C address'.
IMHO this could mean that using size-cells = <0> in the prcm_clocks and scm_clocks nodes is perhaps not quite correct. We need an address translation and if possible, better without having to develop code to handle this special case.
I agree as with a size of 0 there's an implicit assumption as to what the size of the registers are. It's 4 bytes in your case, but what if I have 2 byte registers?
With the following changes in the am33xx-l4.dtsi and am33xx-clocks.dtsi files we get an address translation that does not require special case handling code and the device tree would not be an exception to what we usually see. I have tested these changes in the kernel and they seem to work.
That is the correct change, but compatibility is the issue here.
Rob
Hi,
On Thu, 8 Apr 2021 at 02:42, Rob Herring robh@kernel.org wrote:
On Tue, Apr 6, 2021 at 4:53 PM Dario Binacchi dariobin@libero.it wrote:
Il 06/04/2021 16:26 Rob Herring robh@kernel.org ha scritto:
On Tue, Mar 16, 2021 at 8:26 PM Bin Meng bmeng.cn@gmail.com wrote:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto:
Hi Dario,
On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote: > > > > Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto: > > > > > > +Tom Rini too > > > > > > On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > +Dario Binacchi > > > > > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > > > > > > > Hi Bin, > > > > > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > > > > > The implementation of of_translate_one() was taken from the one in > > > > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > > > > for Apple Macs that don't have the <ranges> property in the parent > > > > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > > > > block and adhere to the spec to abort the translation. > > > > > > > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > > > > --- > > > > > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > > > > > Reviewed-by: Simon Glass sjg@chromium.org > > > > > > > > Unfortunately this seems to cause a test failure for > > > > ut_dm_fdt_translation. Can you please take a look? > > > > > > It seems the no "ranges" property was intentionally removed by the > > > following commit: > > > > > > commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc > > > Author: Dario Binacchi dariobin@libero.it > > > Date: Wed Dec 30 00:16:21 2020 +0100 > > > > > > fdt: translate address if #size-cells = <0> > > > > > > The __of_translate_address routine translates an address from the > > > device tree into a CPU physical address. A note in the description of > > > the routine explains that the crossing of any level with > > > since inherited from IBM. This does not happen for Texas Instruments, or > > > at least for the beaglebone device tree. Without this patch, in fact, > > > the translation into physical addresses of the registers contained in the > > > am33xx-clocks.dtsi nodes would not be possible. They all have a parent > > > with #size-cells = <0>. > > > > > > It looks the commit was needed for beaglebone board. > > > > > > Dario, could you please comment on why U-Boot needs to done like this, > > > while Linux kernel has this check? Is the beaglebone board not working > > > in Linux? > > > > > Beaglebone is working in Linux, but I think Linux walks the device tree less > fully than u-boot. > I was surprised by the address translation error when traversing nodes with > size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS > symbol to fix the issue, not enabled by default, thus making the change backwards > compatible.
Could you please prepare a patch against upstream Linux kernel, so that U-Boot can be in sync with it?
I've replied on that patch...
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
IMO, yes it is. Simply put, 'ranges' must be present to be translatable. Using '#size-cells == 0' is at least questionable, but could maybe be supported (depends what happens with non-empty 'ranges').
The 'fix' makes every 'reg' translatable. Give the function an I2C address 'reg' and you'll get back 'I2C controller address + I2C address'.
IMHO this could mean that using size-cells = <0> in the prcm_clocks and scm_clocks nodes is perhaps not quite correct. We need an address translation and if possible, better without having to develop code to handle this special case.
I agree as with a size of 0 there's an implicit assumption as to what the size of the registers are. It's 4 bytes in your case, but what if I have 2 byte registers?
With the following changes in the am33xx-l4.dtsi and am33xx-clocks.dtsi files we get an address translation that does not require special case handling code and the device tree would not be an exception to what we usually see. I have tested these changes in the kernel and they seem to work.
That is the correct change, but compatibility is the issue here.
Just to dig into this a bit, at least within U-Boot the DT is captive, so we should be able to make this change. If there is a quirk needed, then perhaps it can be added with a property to enable it.
Can you give a few more details about the compatibility problem?
Regards, Simon
On Thu, Apr 08, 2021 at 04:14:07AM +1200, Simon Glass wrote:
Hi,
On Thu, 8 Apr 2021 at 02:42, Rob Herring robh@kernel.org wrote:
On Tue, Apr 6, 2021 at 4:53 PM Dario Binacchi dariobin@libero.it wrote:
Il 06/04/2021 16:26 Rob Herring robh@kernel.org ha scritto:
On Tue, Mar 16, 2021 at 8:26 PM Bin Meng bmeng.cn@gmail.com wrote:
Hi Dario,
On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi dariobin@libero.it wrote:
Hi Bin,
> Il 16/03/2021 02:28 Bin Meng bmeng.cn@gmail.com ha scritto: > > > Hi Dario, > > On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi dariobin@libero.it wrote: > > > > > > > Il 15/03/2021 19:23 Simon Glass sjg@chromium.org ha scritto: > > > > > > > > > +Tom Rini too > > > > > > > > > On Tue, 16 Mar 2021 at 03:48, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > +Dario Binacchi > > > > > > > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass sjg@chromium.org wrote: > > > > > > > > > > Hi Bin, > > > > > > > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass sjg@chromium.org wrote: > > > > > > > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng bmeng.cn@gmail.com wrote: > > > > > > > > > > > > > > The implementation of of_translate_one() was taken from the one in > > > > > > > Linux kernel drivers/of/address.c, and the Linux one added a quirk > > > > > > > for Apple Macs that don't have the <ranges> property in the parent > > > > > > > node. Since U-Boot does not support Apple Macs, remove the comment > > > > > > > block and adhere to the spec to abort the translation. > > > > > > > > > > > > > > Signed-off-by: Bin Meng bmeng.cn@gmail.com > > > > > > > --- > > > > > > > > > > > > > > drivers/core/of_addr.c | 24 ++++++------------------ > > > > > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > > > > > > > Reviewed-by: Simon Glass sjg@chromium.org > > > > > > > > > > Unfortunately this seems to cause a test failure for > > > > > ut_dm_fdt_translation. Can you please take a look? > > > > > > > > It seems the no "ranges" property was intentionally removed by the > > > > following commit: > > > > > > > > commit d64b9cdcd475eb7f07b49741ded87e24dae4a5fc > > > > Author: Dario Binacchi dariobin@libero.it > > > > Date: Wed Dec 30 00:16:21 2020 +0100 > > > > > > > > fdt: translate address if #size-cells = <0> > > > > > > > > The __of_translate_address routine translates an address from the > > > > device tree into a CPU physical address. A note in the description of > > > > the routine explains that the crossing of any level with > > > > since inherited from IBM. This does not happen for Texas Instruments, or > > > > at least for the beaglebone device tree. Without this patch, in fact, > > > > the translation into physical addresses of the registers contained in the > > > > am33xx-clocks.dtsi nodes would not be possible. They all have a parent > > > > with #size-cells = <0>. > > > > > > > > It looks the commit was needed for beaglebone board. > > > > > > > > Dario, could you please comment on why U-Boot needs to done like this, > > > > while Linux kernel has this check? Is the beaglebone board not working > > > > in Linux? > > > > > > > > Beaglebone is working in Linux, but I think Linux walks the device tree less > > fully than u-boot. > > I was surprised by the address translation error when traversing nodes with > > size cells 0. And for this reason I added the CONFIG_OF_TRANSLATE_ZERO_SIZE_CELLS > > symbol to fix the issue, not enabled by default, thus making the change backwards > > compatible. > > Could you please prepare a patch against upstream Linux kernel, so > that U-Boot can be in sync with it?
I've replied on that patch...
With pleasure. But how do I justify the patch since it doesn't fix any bugs? Can I refer to the patches developed for U-boot?
Good question :)
If Linux does not have any issue, maybe U-Boot's fix is questionable?
IMO, yes it is. Simply put, 'ranges' must be present to be translatable. Using '#size-cells == 0' is at least questionable, but could maybe be supported (depends what happens with non-empty 'ranges').
The 'fix' makes every 'reg' translatable. Give the function an I2C address 'reg' and you'll get back 'I2C controller address + I2C address'.
IMHO this could mean that using size-cells = <0> in the prcm_clocks and scm_clocks nodes is perhaps not quite correct. We need an address translation and if possible, better without having to develop code to handle this special case.
I agree as with a size of 0 there's an implicit assumption as to what the size of the registers are. It's 4 bytes in your case, but what if I have 2 byte registers?
With the following changes in the am33xx-l4.dtsi and am33xx-clocks.dtsi files we get an address translation that does not require special case handling code and the device tree would not be an exception to what we usually see. I have tested these changes in the kernel and they seem to work.
That is the correct change, but compatibility is the issue here.
Just to dig into this a bit, at least within U-Boot the DT is captive, so we should be able to make this change. If there is a quirk needed, then perhaps it can be added with a property to enable it.
Can you give a few more details about the compatibility problem?
These are files that in U-Boot we keep untouched and in-sync with the kernel. We might be able to whack something in to -u-boot.dtsi but that's not ideal when there's an actual problem being exposed that needs fixing.
participants (5)
-
Bin Meng -
Dario Binacchi -
Rob Herring -
Simon Glass -
Tom Rini