[U-Boot] [PATCH 1/3] fdt: add memory bank decoding functions for board setup

[Simon Glass]

Hi Nathan,

On 11 December 2016 at 08:58, Nathan Rossi <nathan at nathanrossi.com> wrote:
> Add two functions for use by board implementations to decode the memory
> banks of the /memory node so as to populate the global data with
> ram_size and board info for memory banks.
>
> The fdtdec_setup_memory_size() function decodes the first memory bank
> and sets up the gd->ram_size with the size of the memory bank. This
> function should be called from the boards dram_init().
>
> The fdtdec_setup_memory_banksize() function decode the memory banks
> (up to the CONFIG_NR_DRAM_BANKS) and populates the base address and size
> into the gd->bd->bi_dram array of banks. This function should be called
> from the boards dram_init_banksize().
>
> Signed-off-by: Nathan Rossi <nathan at nathanrossi.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Michal Simek <monstr at monstr.eu>
> ---
> This implementation of decoding has been tested on zynq and zynqmp
> boards with address/size cells of (1, 1), (1, 2), (2, 1), (2, 2) and
> up to 2 memory banks.
> ---
>  include/fdtdec.h | 25 +++++++++++++++++++++++++
>  lib/fdtdec.c     | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 79 insertions(+)

Reviewed-by: Simon Glass <sjg at chromium.org>

Please see nit below.

>
> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index 27887c8c21..59a204b571 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -976,6 +976,31 @@ struct display_timing {
>   */
>  int fdtdec_decode_display_timing(const void *blob, int node, int index,
>                                  struct display_timing *config);
> +
> +/**
> + * fdtdec_setup_memory_size() - decode and setup gd->ram_size
> + *
> + * Decode the /memory 'reg' property to determine the size of the first memory
> + * bank, populate the global data with the size of the first bank of memory.
> + * This function should be called from the boards dram_init().
> + *
> + * @return 0 if OK, -EINVAL if the /memory node or reg property is missing or
> + * invalid
> + */
> +int fdtdec_setup_memory_size(void);
> +
> +/**
> + * fdtdec_setup_memory_banksize() - decode and populate gd->bd->bi_dram
> + *
> + * Decode the /memory 'reg' property to determine the address and size of the
> + * memory banks. Use this data to populate the global data board info with the
> + * phys address and size of memory banks. This function should be called from
> + * the boards dram_init_banksize().
> + *
> + * @return 0 if OK, negative on error

Good to be specific, if e.g. it can only return -EINVAL.

> + */
> +int fdtdec_setup_memory_banksize(void);
> +
>  /**
>   * Set up the device tree ready for use
>   */
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 4e619c49a2..bc3be017b6 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1174,6 +1174,60 @@ int fdtdec_decode_display_timing(const void *blob, int parent, int index,
>         return ret;
>  }
>
> +int fdtdec_setup_memory_size(void)
> +{
> +       int ret, mem;
> +       struct fdt_resource res;
> +
> +       mem = fdt_path_offset(gd->fdt_blob, "/memory");
> +       if (mem < 0) {
> +               debug("%s: Missing /memory node\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       ret = fdt_get_resource(gd->fdt_blob, mem, "reg", 0, &res);
> +       if (ret != 0) {
> +               debug("%s: Unable to decode first memory bank\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       gd->ram_size = (phys_size_t)(res.end - res.start + 1);
> +       debug("%s: Initial DRAM size %llx\n", __func__, (u64)gd->ram_size);
> +
> +       return 0;
> +}
> +
> +int fdtdec_setup_memory_banksize(void)
> +{
> +       int bank, ret, mem;
> +       struct fdt_resource res;
> +
> +       mem = fdt_path_offset(gd->fdt_blob, "/memory");
> +       if (mem < 0) {
> +               debug("%s: Missing /memory node\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> +               ret = fdt_get_resource(gd->fdt_blob, mem, "reg", bank, &res);
> +               if (ret == -FDT_ERR_NOTFOUND)
> +                       break;
> +               if (ret != 0)
> +                       return ret;

The return above return -EINVAL, but this one returns a -FDT_ERR_...
which is different. So my suggestion here would be to return -EINVAL
here, unless you want to change the function to always return an FDT
error (although fdtdec_decode_memory_region() returns an errno error
so perhaps it is better to be consistent with that).

> +
> +               gd->bd->bi_dram[bank].start = (phys_addr_t)res.start;
> +               gd->bd->bi_dram[bank].size =
> +                       (phys_size_t)(res.end - res.start + 1);
> +
> +               debug("%s: DRAM Bank #%d: start = 0x%llx, size = 0x%llx\n",
> +                     __func__, bank,
> +                     (unsigned long long)gd->bd->bi_dram[bank].start,
> +                     (unsigned long long)gd->bd->bi_dram[bank].size);
> +       }
> +
> +       return 0;
> +}
> +
>  int fdtdec_setup(void)
>  {
>  #if CONFIG_IS_ENABLED(OF_CONTROL)
> --
> 2.10.2
>

Regards,
Simon