[PATCH 1/2] of: addr: Abort address translation for parent nodes missing 'ranges'

[Simon Glass]

Hi,

On Thu, 8 Apr 2021 at 02:42, Rob Herring <robh at kernel.org> wrote:
>
> On Tue, Apr 6, 2021 at 4:53 PM Dario Binacchi <dariobin at libero.it> wrote:
> >
> >
> > > Il 06/04/2021 16:26 Rob Herring <robh at kernel.org> ha scritto:
> > >
> > >
> > > On Tue, Mar 16, 2021 at 8:26 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > Hi Dario,
> > > >
> > > > On Wed, Mar 17, 2021 at 4:57 AM Dario Binacchi <dariobin at libero.it> wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > > Il 16/03/2021 02:28 Bin Meng <bmeng.cn at gmail.com> ha scritto:
> > > > > >
> > > > > >
> > > > > > Hi Dario,
> > > > > >
> > > > > > On Tue, Mar 16, 2021 at 6:49 AM Dario Binacchi <dariobin at libero.it> wrote:
> > > > > > >
> > > > > > >
> > > > > > > > Il 15/03/2021 19:23 Simon Glass <sjg at chromium.org> ha scritto:
> > > > > > > >
> > > > > > > >
> > > > > > > > +Tom Rini too
> > > > > > > >
> > > > > > > >
> > > > > > > > On Tue, 16 Mar 2021 at 03:48, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > +Dario Binacchi
> > > > > > > > >
> > > > > > > > > On Mon, Mar 15, 2021 at 3:11 PM Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Bin,
> > > > > > > > > >
> > > > > > > > > > On Wed, 3 Mar 2021 at 14:54, Simon Glass <sjg at chromium.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 26 Feb 2021 at 00:36, Bin Meng <bmeng.cn at 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 at gmail.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >
> > > > > > > > > > > >  drivers/core/of_addr.c | 24 ++++++------------------
> > > > > > > > > > > >  1 file changed, 6 insertions(+), 18 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > Reviewed-by: Simon Glass <sjg at 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 at 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