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

[Simon Glass]

Hi York,

On 24 February 2016 at 15:55, york sun <york.sun at nxp.com> wrote:
> On 02/16/2016 08:02 AM, Simon Glass wrote:
>> 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
>>
>
> Sure. I can fix that.
>
>>> 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.
>
> The new fit_image_get_address() gets correct address. The rest of change is to
> use the new function. I don't think they can be separated. Maybe I don't
> understand your comment.
>
> I am preparing a new version. Please comment on that if you still feel the same.

I mean:

- patch 1: take the existing 32-bit-only code and put it in a new
fit_image_get_address() functoin
- patch 2: enhance your new function to support 64-bit

At present you have these two things co-mingled which I don't think is ideal.

if you don't agree or this doesn't make sense, that is fine too. For that case:

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

Regards,
Simon