[PATCH 0/2] sunxi: binman: Fix U-Boot offset when SPL is not 32 KiB

This was originally part of my series adding SPL FIT support for 32-bit sunxi boards[0]. I hit this issue building a minimal SPI-only SPL for H5 that ended up fitting in 24 KiB. So it turns out this is a bug and not just a dependency for a new feature.
I followed Simon's second suggestion from his response to my original series[1].
[0]: https://lore.kernel.org/u-boot/20211013023022.58829-1-samuel@sholland.org/ [1]: https://lore.kernel.org/u-boot/CAPnjgZ1_ee7uoDLT36LS5gQa12A2zsWRqXWtEm5g-oPi...
Samuel Holland (2): binman: Add 'min-size' entry property sunxi: binman: Fix U-Boot offset when SPL is not 32 KiB
arch/arm/dts/sunxi-u-boot.dtsi | 6 +++++- tools/binman/binman.rst | 8 ++++++++ tools/binman/entry.py | 4 ++++ tools/binman/ftest.py | 21 +++++++++++++++++---- tools/binman/test/009_pack_extra.dts | 7 +++++++ 5 files changed, 41 insertions(+), 5 deletions(-)

This property sets the minimum size of an entry, including padding but not alignment. It can be used to reserve space for growth of an entry, or to enforce a minimum offset for later entries in the section.
Signed-off-by: Samuel Holland samuel@sholland.org ---
tools/binman/binman.rst | 8 ++++++++ tools/binman/entry.py | 4 ++++ tools/binman/ftest.py | 21 +++++++++++++++++---- tools/binman/test/009_pack_extra.dts | 7 +++++++ 4 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index fa8abdcd86..03a99a19bc 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -615,6 +615,14 @@ size: this size. If this is not provided, it will be set to the size of the contents.
+min-size: + Sets the minimum size of the entry. This size includes explicit padding + ('pad-before' and 'pad-after'), but not padding added to meet alignment + requirements. While this does not affect the contents of the entry within + binman itself (the padding is performed only when its parent section is + assembled), the end result will be that the entry ends with the padding + bytes, so may grow. Defaults to 0. + pad-before: Padding before the contents of the entry. Normally this is 0, meaning that the contents start at the beginning of the entry. This can be used diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 5d8696e32a..5eacc5fa6c 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -49,6 +49,7 @@ class Entry(object): offset: Offset of entry within the section, None if not known yet (in which case it will be calculated by Pack()) size: Entry size in bytes, None if not known + min_size: Minimum entry size in bytes pre_reset_size: size as it was before ResetForPack(). This allows us to keep track of the size we started with and detect size changes uncomp_size: Size of uncompressed data in bytes, if the entry is @@ -114,6 +115,7 @@ class Entry(object): self.name = node and (name_prefix + node.name) or 'none' self.offset = None self.size = None + self.min_size = 0 self.pre_reset_size = None self.uncomp_size = None self.data = None @@ -270,6 +272,7 @@ class Entry(object): self.Raise("Please use 'extend-size' instead of 'expand-size'") self.offset = fdt_util.GetInt(self._node, 'offset') self.size = fdt_util.GetInt(self._node, 'size') + self.min_size = fdt_util.GetInt(self._node, 'min-size', 0) self.orig_offset = fdt_util.GetInt(self._node, 'orig-offset') self.orig_size = fdt_util.GetInt(self._node, 'orig-size') if self.GetImage().copy_to_orig: @@ -507,6 +510,7 @@ class Entry(object): else: self.offset = tools.align(offset, self.align) needed = self.pad_before + self.contents_size + self.pad_after + needed = max(needed, self.min_size) needed = tools.align(needed, self.align_size) size = self.size if not size: diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index be0aea49ce..93b332972a 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -883,9 +883,9 @@ class TestFunctional(unittest.TestCase): self.assertIn('image', control.images) image = control.images['image'] entries = image.GetEntries() - self.assertEqual(5, len(entries)) + self.assertEqual(6, len(entries))
- # First u-boot with padding before and after + # First u-boot with padding before and after (included in minimum size) self.assertIn('u-boot', entries) entry = entries['u-boot'] self.assertEqual(0, entry.offset) @@ -934,8 +934,17 @@ class TestFunctional(unittest.TestCase): self.assertEqual(U_BOOT_DATA + tools.get_bytes(0, 64 - len(U_BOOT_DATA)), data[pos:pos + entry.size])
+ # Sixth u-boot with both minimum size and aligned size + self.assertIn('u-boot-min-size', entries) + entry = entries['u-boot-min-size'] + self.assertEqual(128, entry.offset) + self.assertEqual(32, entry.size) + self.assertEqual(U_BOOT_DATA, entry.data[:len(U_BOOT_DATA)]) + self.assertEqual(U_BOOT_DATA + tools.get_bytes(0, 32 - len(U_BOOT_DATA)), + data[pos:pos + entry.size]) + self.CheckNoGaps(entries) - self.assertEqual(128, image.size) + self.assertEqual(160, image.size)
dtb = fdt.Fdt(out_dtb_fname) dtb.Scan() @@ -943,7 +952,7 @@ class TestFunctional(unittest.TestCase): expected = { 'image-pos': 0, 'offset': 0, - 'size': 128, + 'size': 160,
'u-boot:image-pos': 0, 'u-boot:offset': 0, @@ -964,6 +973,10 @@ class TestFunctional(unittest.TestCase): 'u-boot-align-both:image-pos': 64, 'u-boot-align-both:offset': 64, 'u-boot-align-both:size': 64, + + 'u-boot-min-size:image-pos': 128, + 'u-boot-min-size:offset': 128, + 'u-boot-min-size:size': 32, } self.assertEqual(expected, props)
diff --git a/tools/binman/test/009_pack_extra.dts b/tools/binman/test/009_pack_extra.dts index 1b31555771..8d6f4910c9 100644 --- a/tools/binman/test/009_pack_extra.dts +++ b/tools/binman/test/009_pack_extra.dts @@ -6,6 +6,7 @@
binman { u-boot { + min-size = <12>; pad-before = <3>; pad-after = <5>; }; @@ -31,5 +32,11 @@ align = <64>; align-end = <128>; }; + + u-boot-min-size { + type = "u-boot"; + min-size = <24>; + align-size = <16>; + }; }; };

On Sat, 21 Jan 2023 at 16:25, Samuel Holland samuel@sholland.org wrote:
This property sets the minimum size of an entry, including padding but not alignment. It can be used to reserve space for growth of an entry, or to enforce a minimum offset for later entries in the section.
Signed-off-by: Samuel Holland samuel@sholland.org
tools/binman/binman.rst | 8 ++++++++ tools/binman/entry.py | 4 ++++ tools/binman/ftest.py | 21 +++++++++++++++++---- tools/binman/test/009_pack_extra.dts | 7 +++++++ 4 files changed, 36 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org

On Sat, 21 Jan 2023 at 16:25, Samuel Holland samuel@sholland.org wrote:
This property sets the minimum size of an entry, including padding but not alignment. It can be used to reserve space for growth of an entry, or to enforce a minimum offset for later entries in the section.
Signed-off-by: Samuel Holland samuel@sholland.org
tools/binman/binman.rst | 8 ++++++++ tools/binman/entry.py | 4 ++++ tools/binman/ftest.py | 21 +++++++++++++++++---- tools/binman/test/009_pack_extra.dts | 7 +++++++ 4 files changed, 36 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm, thanks!

On sunxi boards, SPL looks for U-Boot at a 32 KiB offset, unless SPL is larger than 32 KiB, in which case U-Boot immediately follows SPL. See the logic in spl_mmc_get_uboot_raw_sector() and spl_spi_load_image().
In two cases, the existing binman description mismatches the SPL code. For 64-bit boards, binman would place U-Boot immediately following SPL, even if SPL is smaller than 32 KiB. This can happen when SPL MMC support is disabled (i.e. when booting from SPI flash).
In contrast, for 32-bit boards, binman would place U-Boot at 32 KiB, even if SPL is larger than that. This happens because the 'offset' property does not consider the size of previous entries.
Fix both issues by setting a minimum size for the SPL entry, which exactly matches the logic in the SPL code. Unfortunately, this size must be provided as a magic number, since none of the relevant config symbols (SPL_PAD_TO, SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, and SYS_SPI_U_BOOT_OFFS) are guaranteed to be defined in all cases.
Fixes: cfa3db602caf ("sunxi: Convert 64-bit boards to use binman") Signed-off-by: Samuel Holland samuel@sholland.org ---
arch/arm/dts/sunxi-u-boot.dtsi | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi index 2028d5b6a9..8a6c9e901a 100644 --- a/arch/arm/dts/sunxi-u-boot.dtsi +++ b/arch/arm/dts/sunxi-u-boot.dtsi @@ -31,6 +31,11 @@ pad-byte = <0xff>;
blob { + /* + * This value matches SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR + * and SYS_SPI_U_BOOT_OFFS if those are defined. + */ + min-size = <0x8000>; filename = "spl/sunxi-spl.bin"; };
@@ -107,7 +112,6 @@ }; #else u-boot-img { - offset = <CONFIG_SPL_PAD_TO>; }; #endif };

On Sat, 21 Jan 2023 at 16:25, Samuel Holland samuel@sholland.org wrote:
On sunxi boards, SPL looks for U-Boot at a 32 KiB offset, unless SPL is larger than 32 KiB, in which case U-Boot immediately follows SPL. See the logic in spl_mmc_get_uboot_raw_sector() and spl_spi_load_image().
In two cases, the existing binman description mismatches the SPL code. For 64-bit boards, binman would place U-Boot immediately following SPL, even if SPL is smaller than 32 KiB. This can happen when SPL MMC support is disabled (i.e. when booting from SPI flash).
In contrast, for 32-bit boards, binman would place U-Boot at 32 KiB, even if SPL is larger than that. This happens because the 'offset' property does not consider the size of previous entries.
Fix both issues by setting a minimum size for the SPL entry, which exactly matches the logic in the SPL code. Unfortunately, this size must be provided as a magic number, since none of the relevant config symbols (SPL_PAD_TO, SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, and SYS_SPI_U_BOOT_OFFS) are guaranteed to be defined in all cases.
Fixes: cfa3db602caf ("sunxi: Convert 64-bit boards to use binman") Signed-off-by: Samuel Holland samuel@sholland.org
arch/arm/dts/sunxi-u-boot.dtsi | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi index 2028d5b6a9..8a6c9e901a 100644 --- a/arch/arm/dts/sunxi-u-boot.dtsi +++ b/arch/arm/dts/sunxi-u-boot.dtsi @@ -31,6 +31,11 @@ pad-byte = <0xff>;
blob {
/*
* This value matches SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR
* and SYS_SPI_U_BOOT_OFFS if those are defined.
*/
min-size = <0x8000>; filename = "spl/sunxi-spl.bin"; };
@@ -107,7 +112,6 @@ }; #else u-boot-img {
offset = <CONFIG_SPL_PAD_TO>; };
#endif }; -- 2.37.4

On Sat, 21 Jan 2023 17:25:17 -0600 Samuel Holland samuel@sholland.org wrote:
Hi Samuel,
On sunxi boards, SPL looks for U-Boot at a 32 KiB offset, unless SPL is larger than 32 KiB, in which case U-Boot immediately follows SPL. See the logic in spl_mmc_get_uboot_raw_sector() and spl_spi_load_image().
In two cases, the existing binman description mismatches the SPL code. For 64-bit boards, binman would place U-Boot immediately following SPL, even if SPL is smaller than 32 KiB. This can happen when SPL MMC support is disabled (i.e. when booting from SPI flash).
In contrast, for 32-bit boards, binman would place U-Boot at 32 KiB, even if SPL is larger than that. This happens because the 'offset' property does not consider the size of previous entries.
Fix both issues by setting a minimum size for the SPL entry, which exactly matches the logic in the SPL code. Unfortunately, this size must be provided as a magic number, since none of the relevant config symbols (SPL_PAD_TO, SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, and SYS_SPI_U_BOOT_OFFS) are guaranteed to be defined in all cases.
Fixes: cfa3db602caf ("sunxi: Convert 64-bit boards to use binman") Signed-off-by: Samuel Holland samuel@sholland.org
thanks for this patch. Indeed a 24K arm64 SPL would break the image, and this patch fixes it. The exact inner workings of binman are beyond me, but I build tested various key platforms and it seems to work now. Maybe that would also fix the build problem I saw with the 32-bit FIT series.
Reviewed-by: Andre Przywara andre.przywara@arm.com
Queued for sunxi/master.
Cheers, Andre
arch/arm/dts/sunxi-u-boot.dtsi | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi index 2028d5b6a9..8a6c9e901a 100644 --- a/arch/arm/dts/sunxi-u-boot.dtsi +++ b/arch/arm/dts/sunxi-u-boot.dtsi @@ -31,6 +31,11 @@ pad-byte = <0xff>;
blob {
/*
* This value matches SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR
* and SYS_SPI_U_BOOT_OFFS if those are defined.
*/
};min-size = <0x8000>; filename = "spl/sunxi-spl.bin";
@@ -107,7 +112,6 @@ }; #else u-boot-img {
};offset = <CONFIG_SPL_PAD_TO>;
#endif };
participants (3)
-
Andre Przywara
-
Samuel Holland
-
Simon Glass