[U-Boot] [PATCH] common: image-fdt: correct fdt_blob for IMAGE_FORMAT_LEGACY

[Simon Glass]

On 24 November 2015 at 18:12, Peng Fan <b51431 at freescale.com> wrote:
>
> Hi Simon,
> On Tue, Nov 24, 2015 at 12:04:56PM -0700, Simon Glass wrote:
> >Hi Peng,
> >
> >On 24 November 2015 at 01:54, Peng Fan <Peng.Fan at freescale.com> wrote:
> >> If condition of "(load == image_start || load == image_data)" is true,
> >> should use "fdt_addr = load;", but not "fdt_blob = (char *)image_data;",
> >> or fdt_blob will be overridden by "fdt_blob = map_sysmem(fdt_addr, 0);"
> >> at the end of the switch case.
> >>
> >> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
> >> Cc: Simon Glass <sjg at chromium.org>
> >> Cc: Joe Hershberger <joe.hershberger at ni.com>
> >> Cc: Max Krummenacher <max.krummenacher at toradex.com>
> >> Cc: Marek Vasut <marex at denx.de>
> >> Cc: Suriyan Ramasami <suriyan.r at gmail.com>
> >> Cc: Paul Kocialkowski <contact at paulk.fr>
> >> Cc: Tom Rini <trini at konsulko.com>
> >> ---
> >>  common/image-fdt.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/common/image-fdt.c b/common/image-fdt.c
> >> index 5180a03..5e4e5bd 100644
> >> --- a/common/image-fdt.c
> >> +++ b/common/image-fdt.c
> >> @@ -326,7 +326,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
> >>
> >>                         if (load == image_start ||
> >>                             load == image_data) {
> >> -                               fdt_blob = (char *)image_data;
> >> +                               fdt_addr = load;
> >>                                 break;
> >>                         }
> >
> >Are you sure that should not be:
> >
> >fdt_addr = image_data
> >
> >?
>
> Just code inspection.
>
> See the following code piece:
>
> 319                         image_start = (ulong)fdt_hdr;
> 320                         image_data = (ulong)image_get_data(fdt_hdr);
> 321                         image_end = image_get_image_end(fdt_hdr);
> 322
> 323                         load = image_get_load(fdt_hdr);
> 324                         load_end = load + image_get_data_size(fdt_hdr);
> 325
> 326                         if (load == image_start ||
> 327                             load == image_data) {
> 328                                 fdt_blob = (char *)image_data;
> 329                                 break;
> 330                         }
> 331
> 332                         if ((load < image_end) && (load_end > image_start)) {
> 333                                 fdt_error("fdt overwritten");
> 334                                 goto error;
> 335                         }
> 336
> 337                         debug("   Loading FDT from 0x%08lx to 0x%08lx\n",
> 338                               image_data, load);
> 339
> 340                         memmove((void *)load,
> 341                                 (void *)image_data,
> 342                                 image_get_data_size(fdt_hdr));
> 343
> 344                         fdt_addr = load;
> 345                         break;
>
>                                         .........[snip code].........
>
> 386                 printf("   Booting using the fdt blob at %#08lx\n", fdt_addr);
> 387                 fdt_blob = map_sysmem(fdt_addr, 0);
>
>
> Line 387 will override the settings of line 328.
> To line 328, means we do not need to relocate image_data to load, since they
> are same. So according to line 344, I use same way "fdt_addr = load".

OK I see.

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

>
> >
> >The idea is to pick up the FDT from inside the image, since the load
> >address indicates that it should not be relocated.
> >
> >BTW one more thing I noticed:
> >
> >image_data = (ulong)image_get_data(fdt_hdr);
> >
> >The cast is confusing, and can be removed.
>
> Yeah. But this patch is to avoid override settings of fdt_blob, line 328
> and line 387. This cast can be discarded using another patch.
>

Yes it should be a separate patch. But since you are in there...

Regards,
Simon