[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(-)

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

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

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