Hi Dario, On Sun, 6 Sep 2020 at 06:13, Dario Binacchi <[email protected]> wrote: > > 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 > #size-cells = <0> is to be considered an error not by specification but > 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. > > Signed-off-by: Dario Binacchi <[email protected]> > Tested-by: Dario Binacchi <[email protected]> > > --- > > Changes in v2: > - Fix a missing line in the commit message. > - Add dm_flags to global_data structure and GD_DM_FLG_SIZE_CELLS_0 macro > to test without recompiling. > - Update the OF_CHECK_COUNTS macro in order to have just one > #define by bringing the GD_DM_FLG_SIZE_CELLS_0 into the expression. > - Lower-case the 0xC019 hex number. > > arch/sandbox/dts/test.dts | 21 ++++++++++ > common/fdt_support.c | 6 ++- > drivers/core/Kconfig | 12 ++++++ > drivers/core/fdtaddr.c | 2 +- > drivers/core/of_addr.c | 14 ++----- > drivers/core/ofnode.c | 7 +++- > drivers/core/root.c | 3 ++ > include/asm-generic/global_data.h | 6 +++ > test/dm/test-fdt.c | 68 ++++++++++++++++++++++++++++++- > 9 files changed, 123 insertions(+), 16 deletions(-)
Looks very nice. Reviewed-by: Simon Glass <[email protected]> +Stefan Roese as an example of using run-time control in tests to handle both cases in sandbox. Nit below [..] > diff --git a/include/asm-generic/global_data.h > b/include/asm-generic/global_data.h > index d4a4e2215d..d0014e812c 100644 > --- a/include/asm-generic/global_data.h > +++ b/include/asm-generic/global_data.h > @@ -64,6 +64,7 @@ typedef struct global_data { > struct global_data *new_gd; /* relocated global data */ > > #ifdef CONFIG_DM > + unsigned long dm_flags; comment > struct udevice *dm_root; /* Root instance for Driver Model */ > struct udevice *dm_root_f; /* Pre-relocation root instance */ > struct list_head uclass_root; /* Head of core tree */ > @@ -169,4 +170,9 @@ typedef struct global_data { > #define GD_FLG_SKIP_LL_INIT 0x20000 /* Don't perform low-level init */ > #define GD_FLG_SMP_READY 0x40000 /* SMP init is complete */ > > +/* > + * Global Data Driver Model Flags > + */ > +#define GD_DM_FLG_SIZE_CELLS_0 0x00001 /* Enable #size-cells=<0> translation > */ > + > #endif /* __ASM_GENERIC_GBL_DATA_H */ Regards, Simon
