[PATCH 1/2] of: addr: Abort address translation for parent nodes missing 'ranges'
Dario Binacchi
dariobin at libero.it
Tue Mar 23 18:27:12 CET 2021
Hi Bin,
> Il 22/03/2021 02:25 Bin Meng <bmeng.cn at gmail.com> ha scritto:
>
>
> Hi Dario,
>
> On Sun, Mar 21, 2021 at 11:19 PM Dario Binacchi <dariobin at libero.it> wrote:
> >
> > Hi Tom,
> >
> > > Il 18/03/2021 20:51 Tom Rini <trini at 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 at gmail.com> ha scritto:
> > > > >
> > > > >
> > > > > 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?
> > > > > >
> > > > > > 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 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>.
> >
> > 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
More information about the U-Boot
mailing list