On 02/24/2016 04:30 PM, Simon Glass wrote:
> Hi York,
>
> On 24 February 2016 at 15:55, york sun <york....@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....@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....@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.
>
I see. Let me work on it.
York
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
