[PATCH v3 1/3] imx8: ls1028a: Drop raw image support

[PATCH v3 1/3] imx8: ls1028a: Drop raw image support

[Simon Glass]

The CONFIG_SPL_RAW_IMAGE_SUPPORT option requires that binman provides an
offset for the image (see spl_set_header_raw_uboot()), if binman is used.
These boards use FIT to store U-Boot, so raw image support is not used.

Drop this option to avoid errors once binman starts checking this.

Signed-off-by: Simon Glass <sjg@chromium.org>
Acked-by: Michael Walle <michael@walle.cc> # for kontron_sl28
---

Changes in v3:
- Add ls1028a tag since kontron_sl28 is not imx8

Changes in v2:
- Add a new patch to drop raw image support for some imx8 boards

 configs/imx8mm-cl-iot-gate_defconfig | 1 +
 configs/imx8mm_evk_defconfig         | 1 +
 configs/imx8mn_ddr4_evk_defconfig    | 1 +
 configs/imx8mp_evk_defconfig         | 1 +
 configs/imx8mq_cm_defconfig          | 1 +
 configs/kontron_sl28_defconfig       | 1 +
 configs/phycore-imx8mp_defconfig     | 1 +
 7 files changed, 7 insertions(+)

diff --git a/configs/imx8mm-cl-iot-gate_defconfig b/configs/imx8mm-cl-iot-gate_defconfig
index 79e4bde0703..7a7f9413dbc 100644
--- a/configs/imx8mm-cl-iot-gate_defconfig
+++ b/configs/imx8mm-cl-iot-gate_defconfig
@@ -30,6 +30,7 @@ CONFIG_OF_SYSTEM_SETUP=y
 CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/compulab/imx8mm-cl-iot-gate/imximage-8mm-lpddr4.cfg"
 CONFIG_BOARD_LATE_INIT=y
 CONFIG_SPL_BOARD_INIT=y
+# CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_I2C=y
 CONFIG_SPL_POWER=y
diff --git a/configs/imx8mm_evk_defconfig b/configs/imx8mm_evk_defconfig
index f7f39b8dc63..8862e10ea57 100644
--- a/configs/imx8mm_evk_defconfig
+++ b/configs/imx8mm_evk_defconfig
@@ -27,6 +27,7 @@ CONFIG_OF_SYSTEM_SETUP=y
 CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/imx8mm_evk/imximage-8mm-lpddr4.cfg"
 CONFIG_BOARD_LATE_INIT=y
 CONFIG_SPL_BOARD_INIT=y
+# CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_I2C=y
 CONFIG_SPL_POWER=y
diff --git a/configs/imx8mn_ddr4_evk_defconfig b/configs/imx8mn_ddr4_evk_defconfig
index 78943dd91d3..ee9960e3cee 100644
--- a/configs/imx8mn_ddr4_evk_defconfig
+++ b/configs/imx8mn_ddr4_evk_defconfig
@@ -30,6 +30,7 @@ CONFIG_DEFAULT_FDT_FILE="imx8mn-ddr4-evk.dtb"
 CONFIG_BOARD_LATE_INIT=y
 CONFIG_SPL_BOARD_INIT=y
 CONFIG_SPL_BOOTROM_SUPPORT=y
+# CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_I2C=y
 CONFIG_SPL_WATCHDOG=y
diff --git a/configs/imx8mp_evk_defconfig b/configs/imx8mp_evk_defconfig
index 2c6fc16cdf5..b39333eff7e 100644
--- a/configs/imx8mp_evk_defconfig
+++ b/configs/imx8mp_evk_defconfig
@@ -31,6 +31,7 @@ CONFIG_BOARD_EARLY_INIT_F=y
 CONFIG_BOARD_LATE_INIT=y
 CONFIG_SPL_BOARD_INIT=y
 CONFIG_SPL_BOOTROM_SUPPORT=y
+# CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_I2C=y
diff --git a/configs/imx8mq_cm_defconfig b/configs/imx8mq_cm_defconfig
index e0a038b168c..ab02312c143 100644
--- a/configs/imx8mq_cm_defconfig
+++ b/configs/imx8mq_cm_defconfig
@@ -25,6 +25,7 @@ CONFIG_OF_SYSTEM_SETUP=y
 CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/ronetix/imx8mq-cm/imximage-8mq-lpddr4.cfg"
 CONFIG_BOARD_LATE_INIT=y
 CONFIG_SPL_BOARD_INIT=y
+# CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_I2C=y
 CONFIG_SYS_PROMPT="u-boot=> "
diff --git a/configs/kontron_sl28_defconfig b/configs/kontron_sl28_defconfig
index 29a45ec54bc..3868d7c39b0 100644
--- a/configs/kontron_sl28_defconfig
+++ b/configs/kontron_sl28_defconfig
@@ -31,6 +31,7 @@ CONFIG_USE_BOOTARGS=y
 CONFIG_BOARD_LATE_INIT=y
 CONFIG_PCI_INIT_R=y
 CONFIG_SPL_BOARD_INIT=y
+# CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_MPC8XXX_INIT_DDR_SUPPORT=y
 CONFIG_SPL_SPI_LOAD=y
diff --git a/configs/phycore-imx8mp_defconfig b/configs/phycore-imx8mp_defconfig
index 84a0a5cbaf2..e90c583c85c 100644
--- a/configs/phycore-imx8mp_defconfig
+++ b/configs/phycore-imx8mp_defconfig
@@ -27,6 +27,7 @@ CONFIG_DEFAULT_FDT_FILE="oftree"
 CONFIG_BOARD_LATE_INIT=y
 CONFIG_SPL_BOARD_INIT=y
 CONFIG_SPL_BOOTROM_SUPPORT=y
+# CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_SYS_MALLOC_SIMPLE=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SPL_I2C=y
-- 
2.32.0.554.ge1b32706d8-goog

[PATCH v3 2/3] arm: Add an __image_copy_start symbol for ARMv8

[Simon Glass]

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(+)

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*)
-- 
2.32.0.554.ge1b32706d8-goog

[PATCH v3 3/3] binman: Show an error if __image_copy_start is missing

[Simon Glass]

Binman needs this symbol to be able to figure out the start of the image.
Detect if it is missing and report an error if any symbols are needed.

Add more documentation about possible binman warnings.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Tim Harvey <tharvey@gatewrorks.com>
---

(no changes since v1)

 tools/binman/binman.rst  | 109 +++++++++++++++++++++++++++++++++++++++
 tools/binman/elf.py      |   6 ++-
 tools/binman/elf_test.py |   7 ++-
 3 files changed, 118 insertions(+), 4 deletions(-)

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
index 09e7b571982..81e0a1364ff 100644
--- a/tools/binman/binman.rst
+++ b/tools/binman/binman.rst
@@ -1158,6 +1158,115 @@ development, since dealing with exceptions and problems in threads is more
 difficult. This avoids any use of ThreadPoolExecutor.
 
 
+Dealing with warnings and errors
+--------------------------------
+
+__image_copy_start
+~~~~~~~~~~~~~~~~~~
+
+If you see::
+
+   Cannot process symbol 'xxx' since there is no __image_copy_start
+
+this means that your SPL image does not include an `__image_copy_start` symbol.
+You can check this with::
+
+   nm spl/u-boot-spl |grep __image_copy_start
+
+If there is no output them you don't have that symbol. It is normally created
+in a `u-boot-spl.lds` file, like this::
+
+   text :
+   {
+      __image_copy_start = .;
+      *(.vectors)
+      CPUDIR/start.o (.text*)
+      *(.text*)
+      *(.glue*)
+   }
+
+Check the appropriate file for your board, typically in the `arch/xxx/cpu`
+or `arch/xxx/cpu/xxx` directory.
+
+Entry xx not found in list
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If you see something like::
+
+   output: 'binman: Section '/binman/u-boot-spl-ddr':
+      Symbol '_binman_u_boot_any_prop_image_pos'
+      in entry '/binman/u-boot-spl-ddr/u-boot-spl/u-boot-spl-nodtb':
+      Entry 'u-boot-any' not found in list (u-boot-spl-nodtb,u-boot-spl-dtb,
+          u-boot-spl,blob-ext@1,blob-ext@2,blob-ext@3,blob-ext@4,main-section)
+
+this means that binman knows it should set the value of a symbol called
+`_binman_u_boot_any_prop_image_pos` but does not know how. That symbol name is
+generated by the `binman_symname` macro (see `binman_sym.h`)::
+
+   #define binman_symname(_entry_name, _prop_name) \
+      _binman_ ## _entry_name ## _prop_ ## _prop_name
+
+so binman decodes it into:
+
+_binman_
+    prefix for all symbols
+u_boot_any
+    entry to find
+_prop_
+    prefix for property
+image_pos
+    image_pos property
+
+It therefore looks for u-boot-any, which means any U-Boot symbol. Supported ones
+are:
+
+- u-boot
+- u-boot-img
+- u-boot-nodtb
+
+You can see a list of the symbols it tried, in brackets. None of these matches
+the above list. The source definition in this example is::
+
+    &binman {
+        u-boot-spl-ddr {
+          filename = "u-boot-spl-ddr.bin";
+          pad-byte = <0xff>;
+          align-size = <4>;
+          align = <4>;
+
+          u-boot-spl {
+             align-end = <4>;
+          };
+
+          blob-ext-1 {
+             filename = "lpddr4_pmu_train_1d_imem.bin";
+             size = <0x8000>;
+          };
+
+          blob-ext-2 {
+             filename = "lpddr4_pmu_train_1d_dmem.bin";
+             size = <0x4000>;
+          };
+
+          blob-ext-3 {
+             filename = "lpddr4_pmu_train_2d_imem.bin";
+             size = <0x8000>;
+          };
+
+          blob-ext-4 {
+             filename = "lpddr4_pmu_train_2d_dmem.bin";
+             size = <0x4000>;
+          };
+       };
+
+and you can see that, while `u-boot-spl` is present, `u-boot` is not. Binman
+must find the required symbol somewhere in the same image.
+
+In this case the problem is that CONFIG_SPL_RAW_IMAGE_SUPPORT is enabled, even
+though U-Boot is actually stored in a FIT. This means that
+spl_set_header_raw_uboot() is called and it looks for a symbol for U-Boot.
+Disabling that option fixes the error.
+
 History / Credits
 -----------------
 
diff --git a/tools/binman/elf.py b/tools/binman/elf.py
index 03b49d7163c..f14d07da157 100644
--- a/tools/binman/elf.py
+++ b/tools/binman/elf.py
@@ -112,12 +112,14 @@ def LookupAndWriteSymbols(elf_fname, entry, section):
     if not syms:
         return
     base = syms.get('__image_copy_start')
-    if not base:
-        return
     for name, sym in syms.items():
         if name.startswith('_binman'):
             msg = ("Section '%s': Symbol '%s'\n   in entry '%s'" %
                    (section.GetPath(), name, entry.GetPath()))
+            if not base:
+                raise ValueError("Cannot process symbol '%s' since there is no __image_copy_start" %
+                                 name)
+
             offset = sym.address - base.address
             if offset < 0 or offset + sym.size > entry.contents_size:
                 raise ValueError('%s has offset %x (size %x) but the contents '
diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py
index 7a128018d9f..96630502b2f 100644
--- a/tools/binman/elf_test.py
+++ b/tools/binman/elf_test.py
@@ -134,8 +134,11 @@ class TestElf(unittest.TestCase):
         entry = FakeEntry(10)
         section = FakeSection()
         elf_fname = self.ElfTestFile('u_boot_binman_syms_bad')
-        self.assertEqual(elf.LookupAndWriteSymbols(elf_fname, entry, section),
-                         None)
+        with self.assertRaises(ValueError) as e:
+            self.assertEqual(elf.LookupAndWriteSymbols(elf_fname, entry, section),
+                             None)
+        self.assertIn("Cannot process symbol '_binman_u_boot_spl_any_prop_offset' since there is no __image_copy_start",
+                      str(e.exception))
 
     def testBadSymbolSize(self):
         """Test that an attempt to use an 8-bit symbol are detected
-- 
2.32.0.554.ge1b32706d8-goog

Re: [PATCH v3 2/3] arm: Add an __image_copy_start symbol for ARMv8

[Scott Wood]

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

Re: [PATCH v3 2/3] arm: Add an __image_copy_start symbol for ARMv8

[Simon Glass]

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

Re: [PATCH v3 2/3] arm: Add an __image_copy_start symbol for ARMv8

[Simon Glass]

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

Re: [PATCH v3 2/3] arm: Add an __image_copy_start symbol for ARMv8

[Scott Wood]

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

Re: [PATCH v3 2/3] arm: Add an __image_copy_start symbol for ARMv8

[Simon Glass]

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