[PATCH v2 15/30] fdt: translate address if #size-cells = <0>

[Simon Glass]

Hi Dario,

On Sun, 6 Sep 2020 at 06:13, Dario Binacchi <dariobin at libero.it> 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 <dariobin at libero.it>
> Tested-by: Dario Binacchi <dariobin at libero.it>
>
> ---
>
> 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 <sjg at chromium.org>

+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