| Message ID | 20210801205951.2202789-2-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [v3,1/3] imx8: ls1028a: Drop raw image support | expand |
On Sun, 2021-08-01 at 14:59 -0600, Simon Glass wrote: > This symbol is needed for binman to locate the start of the image. Add it. > > Note: the existing line to bring in the .__image_copy_start symbol does > not appear to do anything. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > I have copied Scott Wood who originally added the line about the > __image_copy_start in the hope that he can decide if we should remove it. It's been a long time since I looked at this stuff, but __image_copy_start is used for relocation and that code does not seem to be in the SPL, so the *(.__image_copy_start) was probably just a copy-and-paste leftover from the main SPL that can go away. Of course, that doesn't resolve the binman issue. :-) > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot- > spl.lds > index 9edb662b094..2827a07590d 100644 > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > @@ -22,6 +22,7 @@ ENTRY(_start) > SECTIONS > { > .text : { > + __image_copy_start = .; > . = ALIGN(8); > *(.__image_copy_start) > CPUDIR/start.o (.text*) If for whatever reason you did need to define the symbol this way, shouldn't it be after the alignment? -Scott
Hi Scott, On Wed, 4 Aug 2021 at 13:53, Scott Wood <oss@buserror.net> wrote: > > On Sun, 2021-08-01 at 14:59 -0600, Simon Glass wrote: > > This symbol is needed for binman to locate the start of the image. Add it. > > > > Note: the existing line to bring in the .__image_copy_start symbol does > > not appear to do anything. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > I have copied Scott Wood who originally added the line about the > > __image_copy_start in the hope that he can decide if we should remove it. > > It's been a long time since I looked at this stuff, but __image_copy_start is > used for relocation and that code does not seem to be in the SPL, so the > *(.__image_copy_start) was probably just a copy-and-paste leftover from the > main SPL that can go away. > > Of course, that doesn't resolve the binman issue. :-) > > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot- > > spl.lds > > index 9edb662b094..2827a07590d 100644 > > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > > @@ -22,6 +22,7 @@ ENTRY(_start) > > SECTIONS > > { > > .text : { > > + __image_copy_start = .; > > . = ALIGN(8); > > *(.__image_copy_start) > > CPUDIR/start.o (.text*) > > If for whatever reason you did need to define the symbol this way, shouldn't > it be after the alignment? Well I don't want to miss out any of the image, otherwise the offsets would be off. But perhaps that is another question. Since this is the first section, it should already be aligned (to 16 I suspect). So why do we need it? Regards, Simon
Hi Scott, On Thu, 5 Aug 2021 at 13:20, Simon Glass <sjg@chromium.org> wrote: > > Hi Scott, > > On Wed, 4 Aug 2021 at 13:53, Scott Wood <oss@buserror.net> wrote: > > > > On Sun, 2021-08-01 at 14:59 -0600, Simon Glass wrote: > > > This symbol is needed for binman to locate the start of the image. Add it. > > > > > > Note: the existing line to bring in the .__image_copy_start symbol does > > > not appear to do anything. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > --- > > > I have copied Scott Wood who originally added the line about the > > > __image_copy_start in the hope that he can decide if we should remove it. > > > > It's been a long time since I looked at this stuff, but __image_copy_start is > > used for relocation and that code does not seem to be in the SPL, so the > > *(.__image_copy_start) was probably just a copy-and-paste leftover from the > > main SPL that can go away. > > > > Of course, that doesn't resolve the binman issue. :-) > > > > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot- > > > spl.lds > > > index 9edb662b094..2827a07590d 100644 > > > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > > > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > > > @@ -22,6 +22,7 @@ ENTRY(_start) > > > SECTIONS > > > { > > > .text : { > > > + __image_copy_start = .; > > > . = ALIGN(8); > > > *(.__image_copy_start) > > > CPUDIR/start.o (.text*) > > > > If for whatever reason you did need to define the symbol this way, shouldn't > > it be after the alignment? > > Well I don't want to miss out any of the image, otherwise the offsets > would be off. > > But perhaps that is another question. Since this is the first section, > it should already be aligned (to 16 I suspect). So why do we need it? I'd like to get this applied, assuming it is correct, because at present binman is silently ignoring problems where it cannot find symbols. The fix for that is: http://patchwork.ozlabs.org/project/uboot/patch/20210722210216.1.Id1246d1ff1cb5750f8c7ddde9665cf6f09615a7c@changeid/ which has been sitting there for a while. Regards, Simon
On Sat, 2021-09-25 at 09:46 -0600, Simon Glass wrote: > Hi Scott, > > On Thu, 5 Aug 2021 at 13:20, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Scott, > > > > On Wed, 4 Aug 2021 at 13:53, Scott Wood <oss@buserror.net> wrote: > > > > > > On Sun, 2021-08-01 at 14:59 -0600, Simon Glass wrote: > > > > This symbol is needed for binman to locate the start of the image. Add > > > > it. > > > > > > > > Note: the existing line to bring in the .__image_copy_start symbol > > > > does > > > > not appear to do anything. > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > --- > > > > I have copied Scott Wood who originally added the line about the > > > > __image_copy_start in the hope that he can decide if we should remove > > > > it. > > > > > > It's been a long time since I looked at this stuff, but > > > __image_copy_start is > > > used for relocation and that code does not seem to be in the SPL, so the > > > *(.__image_copy_start) was probably just a copy-and-paste leftover from > > > the > > > main SPL that can go away. > > > > > > Of course, that doesn't resolve the binman issue. :-) > > > > > > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u- > > > > boot- > > > > spl.lds > > > > index 9edb662b094..2827a07590d 100644 > > > > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > > > > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > > > > @@ -22,6 +22,7 @@ ENTRY(_start) > > > > SECTIONS > > > > { > > > > .text : { > > > > + __image_copy_start = .; > > > > . = ALIGN(8); > > > > *(.__image_copy_start) > > > > CPUDIR/start.o (.text*) > > > > > > If for whatever reason you did need to define the symbol this way, > > > shouldn't > > > it be after the alignment? > > > > Well I don't want to miss out any of the image, otherwise the offsets > > would be off. > > > > But perhaps that is another question. Since this is the first section, > > it should already be aligned (to 16 I suspect). So why do we need it? > > I'd like to get this applied, assuming it is correct, because at > present binman is silently ignoring problems where it cannot find > symbols. The fix for that is: > > http://patchwork.ozlabs.org/project/uboot/patch/20210722210216.1.Id1246d1ff1cb5750f8c7ddde9665cf6f09615a7c@changeid/ > > which has been sitting there for a while. I'm not sure what you need from me... as I said before, as far as I can tell the __image_copy_start stuff should not be needed by the SPL itself. If you want to add it just for binman, there should probably be a comment to that effect, but I'm not the ARM maintainer (or involved in U-Boot development at all these days) so I can't help you get anything applied. As for why the alignment is there, it's probably just paranoia, but in any case, the SPL linker script probably shouldn't handle alignment in a gratuitously different way from the main linker script. -Scott
+Stefano Babic who might know On Sat, 25 Sept 2021 at 16:36, Scott Wood <oss@buserror.net> wrote: > > On Sat, 2021-09-25 at 09:46 -0600, Simon Glass wrote: > > Hi Scott, > > > > On Thu, 5 Aug 2021 at 13:20, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Scott, > > > > > > On Wed, 4 Aug 2021 at 13:53, Scott Wood <oss@buserror.net> wrote: > > > > > > > > On Sun, 2021-08-01 at 14:59 -0600, Simon Glass wrote: > > > > > This symbol is needed for binman to locate the start of the image. Add > > > > > it. > > > > > > > > > > Note: the existing line to bring in the .__image_copy_start symbol > > > > > does > > > > > not appear to do anything. > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > --- > > > > > I have copied Scott Wood who originally added the line about the > > > > > __image_copy_start in the hope that he can decide if we should remove > > > > > it. > > > > > > > > It's been a long time since I looked at this stuff, but > > > > __image_copy_start is > > > > used for relocation and that code does not seem to be in the SPL, so the > > > > *(.__image_copy_start) was probably just a copy-and-paste leftover from > > > > the > > > > main SPL that can go away. > > > > > > > > Of course, that doesn't resolve the binman issue. :-) > > > > > > > > > diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u- > > > > > boot- > > > > > spl.lds > > > > > index 9edb662b094..2827a07590d 100644 > > > > > --- a/arch/arm/cpu/armv8/u-boot-spl.lds > > > > > +++ b/arch/arm/cpu/armv8/u-boot-spl.lds > > > > > @@ -22,6 +22,7 @@ ENTRY(_start) > > > > > SECTIONS > > > > > { > > > > > .text : { > > > > > + __image_copy_start = .; > > > > > . = ALIGN(8); > > > > > *(.__image_copy_start) > > > > > CPUDIR/start.o (.text*) > > > > > > > > If for whatever reason you did need to define the symbol this way, > > > > shouldn't > > > > it be after the alignment? > > > > > > Well I don't want to miss out any of the image, otherwise the offsets > > > would be off. > > > > > > But perhaps that is another question. Since this is the first section, > > > it should already be aligned (to 16 I suspect). So why do we need it? > > > > I'd like to get this applied, assuming it is correct, because at > > present binman is silently ignoring problems where it cannot find > > symbols. The fix for that is: > > > > http://patchwork.ozlabs.org/project/uboot/patch/20210722210216.1.Id1246d1ff1cb5750f8c7ddde9665cf6f09615a7c@changeid/ > > > > which has been sitting there for a while. > > I'm not sure what you need from me... as I said before, as far as I can tell > the __image_copy_start stuff should not be needed by the SPL itself. If you > want to add it just for binman, there should probably be a comment to that > effect, but I'm not the ARM maintainer (or involved in U-Boot development at > all these days) so I can't help you get anything applied. > > As for why the alignment is there, it's probably just paranoia, but in any > case, the SPL linker script probably shouldn't handle alignment in a > gratuitously different way from the main linker script. > > -Scott > >
diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds index 9edb662b094..2827a07590d 100644 --- a/arch/arm/cpu/armv8/u-boot-spl.lds +++ b/arch/arm/cpu/armv8/u-boot-spl.lds @@ -22,6 +22,7 @@ ENTRY(_start) SECTIONS { .text : { + __image_copy_start = .; . = ALIGN(8); *(.__image_copy_start) CPUDIR/start.o (.text*)
This symbol is needed for binman to locate the start of the image. Add it. Note: the existing line to bring in the .__image_copy_start symbol does not appear to do anything. Signed-off-by: Simon Glass <sjg@chromium.org> --- I have copied Scott Wood who originally added the line about the __image_copy_start in the hope that he can decide if we should remove it. (no changes since v2) Changes in v2: - Add new patch to add an __image_copy_start symbol for ARMv8 arch/arm/cpu/armv8/u-boot-spl.lds | 1 + 1 file changed, 1 insertion(+)