| Message ID | 1448355263-545-3-git-send-email-Peng.Fan@freescale.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
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 ? 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. Regards, Simon
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
On 24 November 2015 at 18:12, Peng Fan <b51431@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@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". OK I see. Reviewed-by: Simon Glass <sjg@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
On Tue, Nov 24, 2015 at 04:54:22PM +0800, 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 <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> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
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; }
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(-)