From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Wed, 25 Nov 2015 09:12:28 +0800 Subject: [U-Boot] [PATCH] common: image-fdt: correct fdt_blob for IMAGE_FORMAT_LEGACY In-Reply-To: References: <1448355263-545-1-git-send-email-Peng.Fan@freescale.com> <1448355263-545-3-git-send-email-Peng.Fan@freescale.com> Message-ID: <20151125011225.GA28129@shlinux2> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 >> Cc: Simon Glass >> Cc: Joe Hershberger >> Cc: Max Krummenacher >> Cc: Marek Vasut >> Cc: Suriyan Ramasami >> Cc: Paul Kocialkowski >> Cc: Tom Rini >> --- >> 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 --