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

[Peng Fan <b51431@freescale.com>]

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@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@freescale.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>> Cc: Max Krummenacher <max.krummenacher@toradex.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Suriyan Ramasami <suriyan.r@gmail.com>
>> Cc: Paul Kocialkowski <contact@paulk.fr>
>> Cc: Tom Rini <trini@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".

>
>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.


Thanks,
Peng.
>
>Regards,
>Simon

--