[PATCH v3 1/3] imx8: ls1028a: Drop raw image support
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
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*)
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.Id1246d1ff1...
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.Id1246d1ff1...
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.Id1246d1ff1...
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
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
participants (2)
-
Scott Wood -
Simon Glass