| Message ID | 1455310750-15080-4-git-send-email-york.sun@nxp.com |
|---|---|
| State | Superseded |
| Delegated to: | York Sun |
| Headers | show |
Hi York, On 12 February 2016 at 13:59, York Sun <york.sun@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@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
On 02/16/2016 08:02 AM, Simon Glass wrote: > Hi York, > > On 12 February 2016 at 13:59, York Sun <york.sun@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@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. York
Hi York, On 24 February 2016 at 15:55, york sun <york.sun@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@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@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@chromium.org> Regards, Simon
On 02/24/2016 04:30 PM, Simon Glass wrote: > Hi York, > > On 24 February 2016 at 15:55, york sun <york.sun@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@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@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
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); } /** @@ -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); } /**
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. Signed-off-by: York Sun <york.sun@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(-)