[U-Boot] [RFC PATCH v4 3/3] common: Fix load and entry addresses in FIT image

[Simon Glass]

Hi York,

On 12 February 2016 at 13:59, York Sun <york.sun at nxp.com> wrote:
> FIT image supports more than 32 bits in addresses by using #address-cell
> field. However the address length is not handled when parsing FIT images.
>

nit: How about saying "fix this by adding support for 64-bit
addresses" or similar

> Signed-off-by: York Sun <york.sun at nxp.com>
>
> ---
>
> Changes in v4:
>   Separate ulong to phys_addr_t change to another patch.
>
> Changes in v3:
>   Define PRIpa for host and target in common/image-fit.c so printf works
>   properly for 32-, 64-bit targets and host tools.
>
> Changes in v2:
>   Make a common function for both load and entry addresses.
>   Simplify calculation of addresses in a similar way as fdtdec_get_number()
>   fdtdec_get_number() is not used, or too many files need to be included
>     and/or twisted for host tool
>   Continue to use %08llx for print format for load and entry addresses
>     because %pa does not always work for host tool (mkimage)
>
>  common/image-fit.c |   54 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/common/image-fit.c b/common/image-fit.c
> index bfa76a2..c000475 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -433,7 +433,7 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
>
>         if ((type == IH_TYPE_KERNEL) || (type == IH_TYPE_STANDALONE) ||
>             (type == IH_TYPE_RAMDISK)) {
> -               fit_image_get_entry(fit, image_noffset, &entry);
> +               ret = fit_image_get_entry(fit, image_noffset, &entry);
>                 printf("%s  Entry Point:  ", p);
>                 if (ret)
>                         printf("unavailable\n");
> @@ -675,6 +675,34 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>         return 0;
>  }
>
> +static int fit_image_get_address(const void *fit, int noffset, char *name,
> +                         phys_addr_t *load)
> +{
> +       int len, cell_len;
> +       const fdt32_t *cell;
> +       unsigned long long load64 = 0;
> +
> +       cell = fdt_getprop(fit, noffset, name, &len);
> +       if (cell == NULL) {
> +               fit_get_debug(fit, noffset, name, len);
> +               return -1;
> +       }
> +
> +       if (len > sizeof(phys_addr_t)) {
> +               printf("Unsupported %s address size\n", name);
> +               return -1;
> +       }
> +
> +       cell_len = len >> 2;
> +       /* Use load64 to avoid compiling warning for 32-bit target */
> +       while (cell_len--) {
> +               load64 = (load64 << 32) | uimage_to_cpu(*cell);
> +               cell++;
> +       }
> +       *load = (phys_addr_t)load64;
> +
> +       return 0;
> +}
>  /**
>   * fit_image_get_load() - get load addr property for given component image node
>   * @fit: pointer to the FIT format image header
> @@ -690,17 +718,7 @@ int fit_image_get_comp(const void *fit, int noffset, uint8_t *comp)
>   */
>  int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
>  {
> -       int len;
> -       const uint32_t *data;
> -
> -       data = fdt_getprop(fit, noffset, FIT_LOAD_PROP, &len);
> -       if (data == NULL) {
> -               fit_get_debug(fit, noffset, FIT_LOAD_PROP, len);
> -               return -1;
> -       }
> -
> -       *load = uimage_to_cpu(*data);
> -       return 0;
> +       return fit_image_get_address(fit, noffset, FIT_LOAD_PROP, load);

I think it would make sense to have your new fit_image_get_address()
in one patch, and the enhancement to support more address sizes in
another.

>  }
>
>  /**
> @@ -722,17 +740,7 @@ int fit_image_get_load(const void *fit, int noffset, phys_addr_t *load)
>   */
>  int fit_image_get_entry(const void *fit, int noffset, phys_addr_t *entry)
>  {
> -       int len;
> -       const uint32_t *data;
> -
> -       data = fdt_getprop(fit, noffset, FIT_ENTRY_PROP, &len);
> -       if (data == NULL) {
> -               fit_get_debug(fit, noffset, FIT_ENTRY_PROP, len);
> -               return -1;
> -       }
> -
> -       *entry = uimage_to_cpu(*data);
> -       return 0;
> +       return fit_image_get_address(fit, noffset, FIT_ENTRY_PROP, entry);
>  }
>
>  /**
> --
> 1.7.9.5
>

Regards,
Simon