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

[Tom Rini <trini@konsulko.com>]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210318/1e391f9a/attachment.sig>