[PATCH 1/2] of: addr: Abort address translation for parent nodes missing 'ranges'
Bin Meng
bmeng.cn at gmail.com
Mon Mar 22 02:25:37 CET 2021
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?
Regards,
Bin
More information about the U-Boot
mailing list