[PATCH v5 00/11] spl: Use common function for loading/parsing images

[PATCH v5 00/11] spl: Use common function for loading/parsing images

[Sean Anderson]

This series adds support for loading all image types (Legacy, FIT (with
and without LOAD_FIT_FULL), and i.MX) to the MMC, SPI, NOR, NET, FAT,
and EXT load methods. It does this by introducing a helper function
which handles the minutiae of invoking the proper parsing function, and
reading the rest of the image.

Hopefully, this will make it easier for load methods to support all
image types that U-Boot supports, without having undocumented
unsupported image types. I applied this to several loaders which were
invoking spl_load_simple_fit and/or spl_parse_image_header, but I did
not use it with others (e.g. DFU/RAM) which had complications in the
mix.

In order to meet size requirements for some boards, the first two
patches of this series are general size-reduction changes. The ffs/fls
change in particular should reduce code size across the board. The
malloc change also has the potential to reduce code size. I've left it
disabled by default, but maybe we can reverse that in the future.

Here's some bloat-o-meter for j7200_evm_a72_defconfig with ext4 support
enabed:

add/remove: 2/1 grow/shrink: 1/5 up/down: 444/-584 (-140)
Function                                     old     new   delta
spl_load                                       -     216    +216
spl_simple_read                                -     184    +184
spl_fit_read                                  60     104     +44
file_fat_read                                 40       -     -40
spl_nor_load_image                           120      76     -44
spl_load_image_ext                           364     296     -68
spl_load_image_fat                           320     200    -120
spl_spi_load_image                           304     176    -128
spl_mmc_load                                 716     532    -184
Total: Before=233618, After=233478, chg -0.06%

For most boards with a few load methods, this series should break even.
However, in the worst case this series will add around 100 bytes.

I have only tested a few loaders. Please try booting your favorite board
with NOR/SPI flash or SPI falcon mode.

On sama5d27_wlsom1_ek_mmc_defconfig, 100 bytes is too much, so this
series depends on [1] to fit everything in. CI run at [2].

[1] https://lore.kernel.org/u-boot/20230731223327.109865-1-sean.anderson@seco.com/
[2] https://source.denx.de/u-boot/custodians/u-boot-clk/-/pipelines/17116

Changes in v5:
- Make SHOW_ERRORS depend on LIBCOMMON
- Load the header in spl_load as well
- Don't bother trying to DMA-align the buffer, since we can't really fix
  it.
- Convert NVMe to spl_load

Changes in v4:
- Fix format specifiers in debug prints
- Reword/fix some of the doc comments for spl_load
- Rebase on u-boot/master

Changes in v3:
- Fix using ffs instead of fls
- Fix using not initializing bl_len when info->filename was NULL
- Fix failing on success

Changes in v2:
- Use reverse-xmas-tree style for locals in spl_simple_read. This is not
  complete, since overhead depends on bl_mask.
- Convert semihosting as well
- Consolidate spi_load_image_os into spl_spi_load_image

Sean Anderson (11):
  spl: Make SHOW_ERRORS depend on LIBCOMMON
  spl: Add generic spl_load function
  spl: Convert ext to use spl_load
  spl: Convert fat to spl_load
  spl: Convert mmc to spl_load
  spl: Convert net to spl_load
  spl: Convert NVMe to spl_load
  spl: Convert nor to spl_load
  spl: Convert semihosting to spl_load
  spl: Convert spi to spl_load
  spl: spi: Consolidate spi_load_image_os into spl_spi_load_image

 common/spl/Kconfig           |   1 +
 common/spl/spl.c             |  86 +++++++++++++++++++++++++-
 common/spl/spl_blk_fs.c      |  68 ++++-----------------
 common/spl/spl_ext.c         |  34 ++++++-----
 common/spl/spl_fat.c         |  55 ++++++-----------
 common/spl/spl_mmc.c         |  91 +++-------------------------
 common/spl/spl_net.c         |  25 ++------
 common/spl/spl_nor.c         |  41 +++----------
 common/spl/spl_semihosting.c |  43 +++++--------
 common/spl/spl_spi.c         | 113 +++++------------------------------
 include/spl.h                |  27 ++++++++-
 11 files changed, 209 insertions(+), 375 deletions(-)

-- 
2.40.1

[PATCH v5 01/11] spl: Make SHOW_ERRORS depend on LIBCOMMON

[Sean Anderson]

The purpose of SHOW_ERRORS is to print extra information. Make it depend
on LIBCOMMON to avoid having to check for two configs.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- New

 common/spl/Kconfig | 1 +
 common/spl/spl.c   | 3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index bee231b583..ce3efeb0ce 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -183,6 +183,7 @@ config SPL_SYS_REPORT_STACK_F_USAGE
 
 config SPL_SHOW_ERRORS
 	bool "Show more information when something goes wrong"
+	depends on SPL_LIBCOMMON_SUPPORT
 	help
 	  This enabled more verbose error messages and checking when something
 	  goes wrong in SPL. For example, it shows the error code when U-Boot
diff --git a/common/spl/spl.c b/common/spl/spl.c
index d74acec10b..b98a9a062a 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -820,8 +820,7 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 	ret = boot_from_devices(&spl_image, spl_boot_list,
 				ARRAY_SIZE(spl_boot_list));
 	if (ret) {
-		if (CONFIG_IS_ENABLED(SHOW_ERRORS) &&
-		    CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT))
+		if (CONFIG_IS_ENABLED(SHOW_ERRORS))
 			printf(SPL_TPL_PROMPT "failed to boot from all boot devices (err=%d)\n",
 			       ret);
 		else
-- 
2.40.1

[PATCH v5 02/11] spl: Add generic spl_load function

[Sean Anderson]

Implementers of SPL_LOAD_IMAGE_METHOD have to correctly determine what
type of image is being loaded and then call the appropriate image load
function correctly. This is tricky, because some image load functions
expect the whole image to already be loaded (CONFIG_SPL_LOAD_FIT_FULL),
some will load the image automatically using spl_load_info.read()
(CONFIG_SPL_LOAD_FIT/CONFIG_SPL_LOAD_IMX_CONTAINER), and some just parse
the header and expect the caller to do the actual loading afterwards
(legacy/raw images). Load methods often only support a subset of the
above methods, meaning that not all image types can be used with all
load methods. Further, the code to invoke these functions is
duplicated between different load functions.

To address this problem, this commit introduces a "spl_load" function.
It aims to handle image detection and correct invocation of each of the
parse/load functions. spl_simple_read is a wrapper around
spl_load_info.read with get_aligned_image* functions inlined for size
purposes. Additionally, we assume that bl_len is a power of 2 so we can
do bitshifts instead of divisions (which is smaller and faster).

spl_load_fit_image tries to DMA-align buf, but we don't. This is because
there may be conflicting alignment requirements for buf and offset. For
example, if the block and DMA-alignments are both 8, and we are passed
buf=80 and offset=20, then we cannot adjust buf and offset so they are
both aligned. Just focus on ensuring block alignment, and warn if we
don't end up with something DMA-aligned.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---

Changes in v5:
- Load the header in spl_load as well
- Don't bother trying to DMA-align the buffer, since we can't really fix
  it.

Changes in v4:
- Fix format specifiers in debug prints
- Reword/fix some of the doc comments for spl_load

Changes in v3:
- Fix using ffs instead of fls
- Fix using not initializing bl_len when info->filename was NULL

Changes in v2:
- Use reverse-xmas-tree style for locals in spl_simple_read. This is not
  complete, since overhead depends on bl_mask.

 common/spl/spl.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/spl.h    | 27 +++++++++++++++-
 2 files changed, 109 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index b98a9a062a..5374eddc89 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -450,6 +450,89 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
 	return 0;
 }
 
+static int spl_simple_read(struct spl_load_info *info, void *buf, size_t size,
+			   size_t offset)
+{
+	size_t bl_mask = info->bl_len - 1;
+	size_t overhead = offset & bl_mask;
+	size_t bl_shift = fls(bl_mask);
+	int ret;
+
+	debug("%s: buf=%p size=%lx offset=%lx\n", __func__, buf, (long)size,
+	      (long)offset);
+	debug("%s: bl_len=%lx bl_mask=%lx bl_shift=%lx\n", __func__,
+	      (long)info->bl_len, (long)bl_mask, (long)bl_shift);
+
+	/*
+	 * Convert offset and size to units of bl_len. In order for the final
+	 * value of buf to be DMA-aligned, both buf and offset must already be
+	 * DMA-aligned. A bounce buffer is required to correct improper
+	 * alignment if min(bl_len, ARCH_DMA_MINALIGN) > 1, so we don't bother.
+	 */
+	buf -= overhead;
+	size = (size + overhead + bl_mask) >> bl_shift;
+	offset = offset >> bl_shift;
+
+	if (CONFIG_IS_ENABLED(SHOW_ERRORS) &&
+	    !IS_ALIGNED((uintptr_t)buf, ARCH_DMA_MINALIGN))
+		printf("Warning: loading to misaligned address %p\n", buf);
+
+	debug("info->read(info, %lx, %lx, %p)\n", (long)offset, (long)size,
+	      buf);
+	ret = info->read(info, offset, size, buf);
+	return ret == size ? 0 : -EIO;
+}
+
+int spl_load(struct spl_image_info *spl_image,
+	     const struct spl_boot_device *bootdev, struct spl_load_info *info,
+	     size_t size, size_t sector)
+{
+	struct legacy_img_hdr *header =
+		spl_get_load_buffer(-sizeof(*header), sizeof(*header));
+	size_t offset = sector * info->bl_len;
+	int ret;
+
+	ret = spl_simple_read(info, header, sizeof(*header), offset);
+	if (ret)
+		return ret;
+
+	if (image_get_magic(header) == FDT_MAGIC) {
+		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL)) {
+			void *buf;
+
+			/*
+			 * In order to support verifying images in the FIT, we
+			 * need to load the whole FIT into memory. Try and
+			 * guess how much we need to load by using the total
+			 * size. This will fail for FITs with external data,
+			 * but there's not much we can do about that.
+			 */
+			if (!size)
+				size = roundup(fdt_totalsize(header), 4);
+			buf = spl_get_load_buffer(0, size);
+			ret = spl_simple_read(info, buf, size, offset);
+			if (ret)
+				return ret;
+
+			return spl_parse_image_header(spl_image, bootdev, buf);
+		}
+
+		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT))
+			return spl_load_simple_fit(spl_image, info, sector,
+						   header);
+	}
+
+	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
+		return spl_load_imx_container(spl_image, info, sector);
+
+	ret = spl_parse_image_header(spl_image, bootdev, header);
+	if (ret)
+		return ret;
+
+	return spl_simple_read(info, (void *)spl_image->load_addr,
+			       spl_image->size, offset + spl_image->offset);
+}
+
 __weak void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 {
 	typedef void __noreturn (*image_entry_noargs_t)(void);
diff --git a/include/spl.h b/include/spl.h
index 658d36481d..47ce16aabf 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -267,7 +267,7 @@ struct spl_image_info {
  *
  * @dev: Pointer to the device, e.g. struct mmc *
  * @priv: Private data for the device
- * @bl_len: Block length for reading in bytes
+ * @bl_len: Block length for reading in bytes; must be a power of 2
  * @filename: Name of the fit image file.
  * @read: Function to call to read from the device
  */
@@ -676,6 +676,31 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
 		       struct spl_boot_device *bootdev,
 		       enum uclass_id uclass_id, int devnum, int partnum);
 
+/**
+ * spl_load() - Parse a header and load the image
+ * @spl_image: Image data which will be filled in by this function
+ * @bootdev: The device to load from
+ * @info: Describes how to load additional information from @bootdev. At the
+ *        minimum, read() and bl_len must be populated.
+ * @size: The size of the image, in bytes, if it is known in advance. Some boot
+ *        devices (such as filesystems) know how big an image is before parsing
+ *        the header. If 0, then the size will be determined from the header.
+ * @sectors: The offset from the start of @bootdev, in units of @info->bl_len.
+ *           This should have the offset @header was loaded from. It will be
+ *           added to any offsets passed to @info->read().
+ *
+ * This function determines the image type (FIT, legacy, i.MX, raw, etc), calls
+ * the appropriate parsing function, determines the load address, and the loads
+ * the image from storage. It is designed to replace ad-hoc image loading which
+ * may not support all image types (especially when config options are
+ * involved).
+ *
+ * Return: 0 on success, or a negative error on failure
+ */
+int spl_load(struct spl_image_info *spl_image,
+	     const struct spl_boot_device *bootdev, struct spl_load_info *info,
+	     size_t size, size_t sector);
+
 /**
  * spl_early_init() - Set up device tree and driver model in SPL if enabled
  *
-- 
2.40.1

[PATCH v5 03/11] spl: Convert ext to use spl_load

[Sean Anderson]

This converts the ext load method to use spl_load. As a consequence, it
also adds support for FIT and IMX images.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Rework to load header in spl_load

 common/spl/spl_ext.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
index 2bf3434439..2c39799128 100644
--- a/common/spl/spl_ext.c
+++ b/common/spl/spl_ext.c
@@ -9,17 +9,30 @@
 #include <errno.h>
 #include <image.h>
 
+static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
+			  ulong size, void *buf)
+{
+	int ret;
+	loff_t actlen;
+
+	ret = ext4fs_read(buf, file_offset, size, &actlen);
+	if (ret)
+		return ret;
+	return actlen;
+}
+
 int spl_load_image_ext(struct spl_image_info *spl_image,
 		       struct spl_boot_device *bootdev,
 		       struct blk_desc *block_dev, int partition,
 		       const char *filename)
 {
 	s32 err;
-	struct legacy_img_hdr *header;
-	loff_t filelen, actlen;
+	loff_t filelen;
 	struct disk_partition part_info = {};
-
-	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
+	struct spl_load_info load = {
+		.read = spl_fit_read,
+		.bl_len = 1,
+	};
 
 	if (part_get_info(block_dev, partition, &part_info)) {
 		printf("spl: no partition table found\n");
@@ -41,19 +54,8 @@ int spl_load_image_ext(struct spl_image_info *spl_image,
 		puts("spl: ext4fs_open failed\n");
 		goto end;
 	}
-	err = ext4fs_read((char *)header, 0, sizeof(struct legacy_img_hdr), &actlen);
-	if (err < 0) {
-		puts("spl: ext4fs_read failed\n");
-		goto end;
-	}
 
-	err = spl_parse_image_header(spl_image, bootdev, header);
-	if (err < 0) {
-		puts("spl: ext: failed to parse image header\n");
-		goto end;
-	}
-
-	err = ext4fs_read((char *)spl_image->load_addr, 0, filelen, &actlen);
+	err = spl_load(spl_image, bootdev, &load, filelen, 0);
 
 end:
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-- 
2.40.1

[PATCH v5 04/11] spl: Convert fat to spl_load

[Sean Anderson]

This converts the fat loader to use spl_load. Some platforms are very
tight on space, so we take care to only include the code we really need.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Rework to load header in spl_load

Changes in v3:
- Fix failing on success

 common/spl/spl_fat.c | 55 +++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 36 deletions(-)

diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
index f8a5b80a3b..6530bcd5a7 100644
--- a/common/spl/spl_fat.c
+++ b/common/spl/spl_fat.c
@@ -60,57 +60,40 @@ int spl_load_image_fat(struct spl_image_info *spl_image,
 		       const char *filename)
 {
 	int err;
-	struct legacy_img_hdr *header;
+	loff_t size;
+	struct spl_load_info load;
+
+	/* This generates smaller code than using a compound literal */
+	load.read = spl_fit_read;
+	load.bl_len = 1;
+	load.filename = filename;
 
 	err = spl_register_fat_device(block_dev, partition);
 	if (err)
 		goto end;
 
-	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
-
-	err = file_fat_read(filename, header, sizeof(struct legacy_img_hdr));
-	if (err <= 0)
-		goto end;
-
-	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
-	    image_get_magic(header) == FDT_MAGIC) {
-		err = file_fat_read(filename, (void *)CONFIG_SYS_LOAD_ADDR, 0);
-		if (err <= 0)
-			goto end;
-		err = spl_parse_image_header(spl_image, bootdev,
-				(struct legacy_img_hdr *)CONFIG_SYS_LOAD_ADDR);
-		if (err == -EAGAIN)
-			return err;
-		if (err == 0)
-			err = 1;
-	} else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
-	    image_get_magic(header) == FDT_MAGIC) {
-		struct spl_load_info load;
-
-		debug("Found FIT\n");
-		load.read = spl_fit_read;
-		load.bl_len = 1;
-		load.filename = (void *)filename;
-		load.priv = NULL;
-
-		return spl_load_simple_fit(spl_image, &load, 0, header);
-	} else {
-		err = spl_parse_image_header(spl_image, bootdev, header);
+	/*
+	 * Avoid pulling in this function for other image types since we are
+	 * very short on space on some boards.
+	 */
+	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL)) {
+		err = fat_size(filename, &size);
 		if (err)
 			goto end;
-
-		err = file_fat_read(filename,
-				    (u8 *)(uintptr_t)spl_image->load_addr, 0);
+	} else {
+		size = 0;
 	}
 
+	err = spl_load(spl_image, bootdev, &load, 0, size);
+
 end:
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-	if (err <= 0)
+	if (err < 0)
 		printf("%s: error reading image %s, err - %d\n",
 		       __func__, filename, err);
 #endif
 
-	return (err <= 0);
+	return err;
 }
 
 #if CONFIG_IS_ENABLED(OS_BOOT)
-- 
2.40.1

[PATCH v5 05/11] spl: Convert mmc to spl_load

[Sean Anderson]

This converts the mmc loader to spl_load. Legacy images are handled by
spl_load (via spl_parse_image_header), so mmc_load_legacy can be
omitted.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Rework to load header in spl_load

 common/spl/spl_mmc.c | 91 ++++----------------------------------------
 1 file changed, 8 insertions(+), 83 deletions(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index a665091b00..c926a42c0f 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -17,48 +17,6 @@
 #include <mmc.h>
 #include <image.h>
 
-static int mmc_load_legacy(struct spl_image_info *spl_image,
-			   struct spl_boot_device *bootdev,
-			   struct mmc *mmc,
-			   ulong sector, struct legacy_img_hdr *header)
-{
-	u32 image_offset_sectors;
-	u32 image_size_sectors;
-	unsigned long count;
-	u32 image_offset;
-	int ret;
-
-	ret = spl_parse_image_header(spl_image, bootdev, header);
-	if (ret)
-		return ret;
-
-	/* convert offset to sectors - round down */
-	image_offset_sectors = spl_image->offset / mmc->read_bl_len;
-	/* calculate remaining offset */
-	image_offset = spl_image->offset % mmc->read_bl_len;
-
-	/* convert size to sectors - round up */
-	image_size_sectors = (spl_image->size + mmc->read_bl_len - 1) /
-			     mmc->read_bl_len;
-
-	/* Read the header too to avoid extra memcpy */
-	count = blk_dread(mmc_get_blk_desc(mmc),
-			  sector + image_offset_sectors,
-			  image_size_sectors,
-			  (void *)(ulong)spl_image->load_addr);
-	debug("read %x sectors to %lx\n", image_size_sectors,
-	      spl_image->load_addr);
-	if (count != image_size_sectors)
-		return -EIO;
-
-	if (image_offset)
-		memmove((void *)(ulong)spl_image->load_addr,
-			(void *)(ulong)spl_image->load_addr + image_offset,
-			spl_image->size);
-
-	return 0;
-}
-
 static ulong h_spl_load_read(struct spl_load_info *load, ulong sector,
 			     ulong count, void *buf)
 {
@@ -82,47 +40,14 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
 			      struct spl_boot_device *bootdev,
 			      struct mmc *mmc, unsigned long sector)
 {
-	unsigned long count;
-	struct legacy_img_hdr *header;
-	struct blk_desc *bd = mmc_get_blk_desc(mmc);
-	int ret = 0;
-
-	header = spl_get_load_buffer(-sizeof(*header), bd->blksz);
-
-	/* read image header to find the image size & load address */
-	count = blk_dread(bd, sector, 1, header);
-	debug("hdr read sector %lx, count=%lu\n", sector, count);
-	if (count == 0) {
-		ret = -EIO;
-		goto end;
-	}
-
-	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
-	    image_get_magic(header) == FDT_MAGIC) {
-		struct spl_load_info load;
-
-		debug("Found FIT\n");
-		load.dev = mmc;
-		load.priv = NULL;
-		load.filename = NULL;
-		load.bl_len = mmc->read_bl_len;
-		load.read = h_spl_load_read;
-		ret = spl_load_simple_fit(spl_image, &load, sector, header);
-	} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
-		struct spl_load_info load;
-
-		load.dev = mmc;
-		load.priv = NULL;
-		load.filename = NULL;
-		load.bl_len = mmc->read_bl_len;
-		load.read = h_spl_load_read;
-
-		ret = spl_load_imx_container(spl_image, &load, sector);
-	} else {
-		ret = mmc_load_legacy(spl_image, bootdev, mmc, sector, header);
-	}
-
-end:
+	int ret;
+	struct spl_load_info load = {
+		.dev = mmc,
+		.bl_len = mmc->read_bl_len,
+		.read = h_spl_load_read,
+	};
+
+	ret = spl_load(spl_image, bootdev, &load, 0, sector);
 	if (ret) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
 		puts("mmc_load_image_raw_sector: mmc block read error\n");
-- 
2.40.1

[PATCH v5 06/11] spl: Convert net to spl_load

[Sean Anderson]

This converts the net load method to use spl_load. As a result, it also
adds support for LOAD_FIT_FULL and IMX images.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Rework to load header in spl_load

 common/spl/spl_net.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/common/spl/spl_net.c b/common/spl/spl_net.c
index b2c901b554..29e72bc491 100644
--- a/common/spl/spl_net.c
+++ b/common/spl/spl_net.c
@@ -28,7 +28,10 @@ static ulong spl_net_load_read(struct spl_load_info *load, ulong sector,
 static int spl_net_load_image(struct spl_image_info *spl_image,
 			      struct spl_boot_device *bootdev)
 {
-	struct legacy_img_hdr *header = (struct legacy_img_hdr *)image_load_addr;
+	struct spl_load_info load = {
+		.bl_len = 1,
+		.read = spl_net_load_read,
+	};
 	int rv;
 
 	env_init();
@@ -47,25 +50,7 @@ static int spl_net_load_image(struct spl_image_info *spl_image,
 		return rv;
 	}
 
-	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
-	    image_get_magic(header) == FDT_MAGIC) {
-		struct spl_load_info load;
-
-		debug("Found FIT\n");
-		load.bl_len = 1;
-		load.read = spl_net_load_read;
-		rv = spl_load_simple_fit(spl_image, &load, 0, header);
-	} else {
-		debug("Legacy image\n");
-
-		rv = spl_parse_image_header(spl_image, bootdev, header);
-		if (rv)
-			return rv;
-
-		memcpy((void *)spl_image->load_addr, header, spl_image->size);
-	}
-
-	return rv;
+	return spl_load(spl_image, bootdev, &load, 0, 0);
 }
 #endif
 
-- 
2.40.1

[PATCH v5 07/11] spl: Convert NVMe to spl_load

[Sean Anderson]

This converts the blk load method (used exclusively by NVMe) to use
spl_load. As a consequence, it also adds support for LOAD_FIT_FULL and
IMX images.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- New

 common/spl/spl_blk_fs.c | 68 ++++++++---------------------------------
 1 file changed, 12 insertions(+), 56 deletions(-)

diff --git a/common/spl/spl_blk_fs.c b/common/spl/spl_blk_fs.c
index d97adc4d39..eb5775a470 100644
--- a/common/spl/spl_blk_fs.c
+++ b/common/spl/spl_blk_fs.c
@@ -45,24 +45,28 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
 {
 	const char *filename = CONFIG_SPL_PAYLOAD;
 	struct disk_partition part_info = {};
-	struct legacy_img_hdr *header;
 	struct blk_desc *blk_desc;
-	loff_t actlen, filesize;
+	loff_t filesize;
 	struct blk_dev dev;
 	int ret;
+	struct spl_load_info load = {
+		.read = spl_fit_read,
+		.bl_len = 1,
+		.filename = filename,
+		.priv = &dev,
+	};
 
 	blk_desc = blk_get_devnum_by_uclass_id(uclass_id, devnum);
 	if (!blk_desc) {
 		printf("blk desc for %d %d not found\n", uclass_id, devnum);
-		goto out;
+		return ret;
 	}
 
 	blk_show_device(uclass_id, devnum);
-	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
 	ret = part_get_info(blk_desc, 1, &part_info);
 	if (ret) {
 		printf("spl: no partition table found. Err - %d\n", ret);
-		goto out;
+		return ret;
 	}
 
 	dev.ifname = blk_get_uclass_name(uclass_id);
@@ -72,63 +76,15 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
 	if (ret) {
 		printf("spl: unable to set blk_dev %s %s. Err - %d\n",
 		       dev.ifname, dev.dev_part_str, ret);
-		goto out;
-	}
-
-	ret = fs_read(filename, (ulong)header, 0,
-		      sizeof(struct legacy_img_hdr), &actlen);
-	if (ret) {
-		printf("spl: unable to read file %s. Err - %d\n", filename,
-		       ret);
-		goto out;
-	}
-
-	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
-	    image_get_magic(header) == FDT_MAGIC) {
-		struct spl_load_info load;
-
-		debug("Found FIT\n");
-		load.read = spl_fit_read;
-		load.bl_len = 1;
-		load.filename = (void *)filename;
-		load.priv = &dev;
-
-		return spl_load_simple_fit(spl_image, &load, 0, header);
-	}
-
-	ret = spl_parse_image_header(spl_image, bootdev, header);
-	if (ret) {
-		printf("spl: unable to parse image header. Err - %d\n",
-		       ret);
-		goto out;
-	}
-
-	ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str, FS_TYPE_ANY);
-	if (ret) {
-		printf("spl: unable to set blk_dev %s %s. Err - %d\n",
-		       dev.ifname, dev.dev_part_str, ret);
-		goto out;
+		return ret;
 	}
 
 	ret = fs_size(filename, &filesize);
 	if (ret) {
 		printf("spl: unable to get file size: %s. Err - %d\n",
 		       filename, ret);
-		goto out;
+		return ret;
 	}
 
-	ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str, FS_TYPE_ANY);
-	if (ret) {
-		printf("spl: unable to set blk_dev %s %s. Err - %d\n",
-		       dev.ifname, dev.dev_part_str, ret);
-		goto out;
-	}
-
-	ret = fs_read(filename, (ulong)spl_image->load_addr, 0, filesize,
-		      &actlen);
-	if (ret)
-		printf("spl: unable to read file %s. Err - %d\n",
-		       filename, ret);
-out:
-	return ret;
+	return spl_load(spl_image, bootdev, &load, filesize, 0);
 }
-- 
2.40.1

[PATCH v5 08/11] spl: Convert nor to spl_load

[Sean Anderson]

This converts the nor load method to use spl_load. As a result it also
adds support for LOAD_FIT_FULL.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Rework to load header in spl_load

 common/spl/spl_nor.c | 41 +++++++----------------------------------
 1 file changed, 7 insertions(+), 34 deletions(-)

diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
index 5b65b96a77..7328a87024 100644
--- a/common/spl/spl_nor.c
+++ b/common/spl/spl_nor.c
@@ -26,8 +26,10 @@ unsigned long __weak spl_nor_get_uboot_base(void)
 static int spl_nor_load_image(struct spl_image_info *spl_image,
 			      struct spl_boot_device *bootdev)
 {
-	__maybe_unused const struct legacy_img_hdr *header;
-	__maybe_unused struct spl_load_info load;
+	struct spl_load_info load = {
+		.bl_len = 1,
+		.read = spl_nor_load_read,
+	};
 
 	/*
 	 * Loading of the payload to SDRAM is done with skipping of
@@ -41,7 +43,8 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
 		 * Load Linux from its location in NOR flash to its defined
 		 * location in SDRAM
 		 */
-		header = (const struct legacy_img_hdr *)CONFIG_SYS_OS_BASE;
+		const struct legacy_img_hdr *header =
+			(const struct legacy_img_hdr *)CONFIG_SYS_OS_BASE;
 #ifdef CONFIG_SPL_LOAD_FIT
 		if (image_get_magic(header) == FDT_MAGIC) {
 			int ret;
@@ -91,36 +94,6 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
 	 * Load real U-Boot from its location in NOR flash to its
 	 * defined location in SDRAM
 	 */
-#ifdef CONFIG_SPL_LOAD_FIT
-	header = (const struct legacy_img_hdr *)spl_nor_get_uboot_base();
-	if (image_get_magic(header) == FDT_MAGIC) {
-		debug("Found FIT format U-Boot\n");
-		load.bl_len = 1;
-		load.read = spl_nor_load_read;
-		return spl_load_simple_fit(spl_image, &load,
-					   spl_nor_get_uboot_base(),
-					   (void *)header);
-	}
-#endif
-	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
-		load.bl_len = 1;
-		load.read = spl_nor_load_read;
-		return spl_load_imx_container(spl_image, &load,
-					      spl_nor_get_uboot_base());
-	}
-
-	/* Legacy image handling */
-	if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_FORMAT)) {
-		struct legacy_img_hdr hdr;
-
-		load.bl_len = 1;
-		load.read = spl_nor_load_read;
-		spl_nor_load_read(&load, spl_nor_get_uboot_base(), sizeof(hdr), &hdr);
-		return spl_load_legacy_img(spl_image, bootdev, &load,
-					   spl_nor_get_uboot_base(),
-					   &hdr);
-	}
-
-	return -EINVAL;
+	return spl_load(spl_image, bootdev, &load, 0, 0);
 }
 SPL_LOAD_IMAGE_METHOD("NOR", 0, BOOT_DEVICE_NOR, spl_nor_load_image);
-- 
2.40.1

[PATCH v5 09/11] spl: Convert semihosting to spl_load

[Sean Anderson]

This converts the semihosting load method to use spl_load. As a result, it
also adds support for LOAD_FIT_FULL and IMX images.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Rework to load header in spl_load

Changes in v2:
- New

 common/spl/spl_semihosting.c | 43 ++++++++++++------------------------
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/common/spl/spl_semihosting.c b/common/spl/spl_semihosting.c
index 5b5e842a11..e7cb9f4ce6 100644
--- a/common/spl/spl_semihosting.c
+++ b/common/spl/spl_semihosting.c
@@ -9,16 +9,16 @@
 #include <semihosting.h>
 #include <spl.h>
 
-static int smh_read_full(long fd, void *memp, size_t len)
+static ulong spl_smh_fit_read(struct spl_load_info *load, ulong sector,
+			      ulong count, void *buf)
 {
-	long read;
+	int ret, fd = *(int *)load->priv;
 
-	read = smh_read(fd, memp, len);
-	if (read < 0)
-		return read;
-	if (read != len)
-		return -EIO;
-	return 0;
+	if (smh_seek(fd, sector))
+		return 0;
+
+	ret = smh_read(fd, buf, count);
+	return ret < 0 ? 0 : ret;
 }
 
 static int spl_smh_load_image(struct spl_image_info *spl_image,
@@ -27,14 +27,17 @@ static int spl_smh_load_image(struct spl_image_info *spl_image,
 	const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
 	int ret;
 	long fd, len;
-	struct legacy_img_hdr *header =
-		spl_get_load_buffer(-sizeof(*header), sizeof(*header));
+	struct spl_load_info load = {
+		.bl_len = 1,
+		.read = spl_smh_fit_read,
+	};
 
 	fd = smh_open(filename, MODE_READ | MODE_BINARY);
 	if (fd < 0) {
 		log_debug("could not open %s: %ld\n", filename, fd);
 		return fd;
 	}
+	load.priv = &fd;
 
 	ret = smh_flen(fd);
 	if (ret < 0) {
@@ -43,25 +46,7 @@ static int spl_smh_load_image(struct spl_image_info *spl_image,
 	}
 	len = ret;
 
-	ret = smh_read_full(fd, header, sizeof(struct legacy_img_hdr));
-	if (ret) {
-		log_debug("could not read image header: %d\n", ret);
-		goto out;
-	}
-
-	ret = spl_parse_image_header(spl_image, bootdev, header);
-	if (ret) {
-		log_debug("failed to parse image header: %d\n", ret);
-		goto out;
-	}
-
-	ret = smh_seek(fd, 0);
-	if (ret) {
-		log_debug("could not seek to start of image: %d\n", ret);
-		goto out;
-	}
-
-	ret = smh_read_full(fd, (void *)spl_image->load_addr, len);
+	ret = spl_load(spl_image, bootdev, &load, len, 0);
 	if (ret)
 		log_debug("could not read %s: %d\n", filename, ret);
 out:
-- 
2.40.1

[PATCH v5 10/11] spl: Convert spi to spl_load

[Sean Anderson]

This converts the spi load method to use spl_load. As a consequence, it
also adds support for LOAD_FIT_FULL.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Rework to load header in spl_load

 common/spl/spl_spi.c | 72 +++++---------------------------------------
 1 file changed, 8 insertions(+), 64 deletions(-)

diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
index 2aff025f76..14391a1c96 100644
--- a/common/spl/spl_spi.c
+++ b/common/spl/spl_spi.c
@@ -89,12 +89,14 @@ u32 __weak spl_spi_boot_cs(void)
 static int spl_spi_load_image(struct spl_image_info *spl_image,
 			      struct spl_boot_device *bootdev)
 {
-	int err = 0;
 	unsigned int payload_offs;
 	struct spi_flash *flash;
-	struct legacy_img_hdr *header;
 	unsigned int sf_bus = spl_spi_boot_bus();
 	unsigned int sf_cs = spl_spi_boot_cs();
+	struct spl_load_info load = {
+		.bl_len = 1,
+		.read = spl_spi_fit_read,
+	};
 
 	/*
 	 * Load U-Boot image from SPI flash into RAM
@@ -109,77 +111,19 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
 		return -ENODEV;
 	}
 
+	load.dev = flash;
 	payload_offs = spl_spi_get_uboot_offs(flash);
 
-	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
-
 	if (CONFIG_IS_ENABLED(OF_REAL)) {
 		payload_offs = ofnode_conf_read_int("u-boot,spl-payload-offset",
 						    payload_offs);
 	}
 
 #if CONFIG_IS_ENABLED(OS_BOOT)
-	if (spl_start_uboot() || spi_load_image_os(spl_image, bootdev, flash, header))
+	if (!spl_start_uboot() && !spi_load_image_os(spl_image, bootdev, flash, header))
+		return 0;
 #endif
-	{
-		/* Load u-boot, mkimage header is 64 bytes. */
-		err = spi_flash_read(flash, payload_offs, sizeof(*header),
-				     (void *)header);
-		if (err) {
-			debug("%s: Failed to read from SPI flash (err=%d)\n",
-			      __func__, err);
-			return err;
-		}
-
-		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
-		    image_get_magic(header) == FDT_MAGIC) {
-			err = spi_flash_read(flash, payload_offs,
-					     roundup(fdt_totalsize(header), 4),
-					     (void *)CONFIG_SYS_LOAD_ADDR);
-			if (err)
-				return err;
-			err = spl_parse_image_header(spl_image, bootdev,
-					(struct legacy_img_hdr *)CONFIG_SYS_LOAD_ADDR);
-		} else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
-			   image_get_magic(header) == FDT_MAGIC) {
-			struct spl_load_info load;
-
-			debug("Found FIT\n");
-			load.dev = flash;
-			load.priv = NULL;
-			load.filename = NULL;
-			load.bl_len = 1;
-			load.read = spl_spi_fit_read;
-			err = spl_load_simple_fit(spl_image, &load,
-						  payload_offs,
-						  header);
-		} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
-			struct spl_load_info load;
-
-			load.dev = flash;
-			load.priv = NULL;
-			load.filename = NULL;
-			load.bl_len = 1;
-			load.read = spl_spi_fit_read;
-
-			err = spl_load_imx_container(spl_image, &load,
-						     payload_offs);
-		} else {
-			err = spl_parse_image_header(spl_image, bootdev, header);
-			if (err)
-				return err;
-			err = spi_flash_read(flash, payload_offs + spl_image->offset,
-					     spl_image->size,
-					     (void *)spl_image->load_addr);
-		}
-		if (IS_ENABLED(CONFIG_SPI_FLASH_SOFT_RESET)) {
-			err = spi_nor_remove(flash);
-			if (err)
-				return err;
-		}
-	}
-
-	return err;
+	return spl_load(spl_image, bootdev, &load, 0, payload_offs);
 }
 /* Use priorty 1 so that boards can override this */
 SPL_LOAD_IMAGE_METHOD("SPI", 1, BOOT_DEVICE_SPI, spl_spi_load_image);
-- 
2.40.1

[PATCH v5 11/11] spl: spi: Consolidate spi_load_image_os into spl_spi_load_image

[Sean Anderson]

spi_load_image_os performs almost the same steps as the non-falcon-boot
path of spl_spi_load_image. The load address is different, and it also
loads a device tree, but that's it. Refactor the boot process so that
they can both use the same load function.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Rework to load header in spl_load

Changes in v2:
- New

 common/spl/spl_spi.c | 47 ++++++++++----------------------------------
 1 file changed, 10 insertions(+), 37 deletions(-)

diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
index 14391a1c96..fe4d071593 100644
--- a/common/spl/spl_spi.c
+++ b/common/spl/spl_spi.c
@@ -18,41 +18,6 @@
 #include <asm/global_data.h>
 #include <dm/ofnode.h>
 
-#if CONFIG_IS_ENABLED(OS_BOOT)
-/*
- * Load the kernel, check for a valid header we can parse, and if found load
- * the kernel and then device tree.
- */
-static int spi_load_image_os(struct spl_image_info *spl_image,
-			     struct spl_boot_device *bootdev,
-			     struct spi_flash *flash,
-			     struct legacy_img_hdr *header)
-{
-	int err;
-
-	/* Read for a header, parse or error out. */
-	spi_flash_read(flash, CFG_SYS_SPI_KERNEL_OFFS, sizeof(*header),
-		       (void *)header);
-
-	if (image_get_magic(header) != IH_MAGIC)
-		return -1;
-
-	err = spl_parse_image_header(spl_image, bootdev, header);
-	if (err)
-		return err;
-
-	spi_flash_read(flash, CFG_SYS_SPI_KERNEL_OFFS,
-		       spl_image->size, (void *)spl_image->load_addr);
-
-	/* Read device tree. */
-	spi_flash_read(flash, CFG_SYS_SPI_ARGS_OFFS,
-		       CFG_SYS_SPI_ARGS_SIZE,
-		       (void *)CONFIG_SYS_SPL_ARGS_ADDR);
-
-	return 0;
-}
-#endif
-
 static ulong spl_spi_fit_read(struct spl_load_info *load, ulong sector,
 			      ulong count, void *buf)
 {
@@ -120,9 +85,17 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
 	}
 
 #if CONFIG_IS_ENABLED(OS_BOOT)
-	if (!spl_start_uboot() && !spi_load_image_os(spl_image, bootdev, flash, header))
-		return 0;
+	if (spl_start_uboot()) {
+		int err = spl_load(spl_image, bootdev, &load, 0, CFG_SYS_SPI_KERNEL_OFFS);
+
+		if (!err)
+			/* Read device tree. */
+			return spi_flash_read(flash, CFG_SYS_SPI_ARGS_OFFS,
+					      CFG_SYS_SPI_ARGS_SIZE,
+					      (void *)CONFIG_SYS_SPL_ARGS_ADDR);
+	}
 #endif
+
 	return spl_load(spl_image, bootdev, &load, 0, payload_offs);
 }
 /* Use priorty 1 so that boards can override this */
-- 
2.40.1

Re: [PATCH v5 01/11] spl: Make SHOW_ERRORS depend on LIBCOMMON

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

On Mon, Jul 31, 2023 at 06:42:53PM -0400, Sean Anderson wrote:

> The purpose of SHOW_ERRORS is to print extra information. Make it depend
> on LIBCOMMON to avoid having to check for two configs.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v5 00/11] spl: Use common function for loading/parsing images

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

On Mon, Jul 31, 2023 at 06:42:52PM -0400, Sean Anderson wrote:

> This series adds support for loading all image types (Legacy, FIT (with
> and without LOAD_FIT_FULL), and i.MX) to the MMC, SPI, NOR, NET, FAT,
> and EXT load methods. It does this by introducing a helper function
> which handles the minutiae of invoking the proper parsing function, and
> reading the rest of the image.
> 
> Hopefully, this will make it easier for load methods to support all
> image types that U-Boot supports, without having undocumented
> unsupported image types. I applied this to several loaders which were
> invoking spl_load_simple_fit and/or spl_parse_image_header, but I did
> not use it with others (e.g. DFU/RAM) which had complications in the
> mix.
> 
> In order to meet size requirements for some boards, the first two
> patches of this series are general size-reduction changes. The ffs/fls
> change in particular should reduce code size across the board. The
> malloc change also has the potential to reduce code size. I've left it
> disabled by default, but maybe we can reverse that in the future.
> 
> Here's some bloat-o-meter for j7200_evm_a72_defconfig with ext4 support
> enabed:
> 
> add/remove: 2/1 grow/shrink: 1/5 up/down: 444/-584 (-140)
> Function                                     old     new   delta
> spl_load                                       -     216    +216
> spl_simple_read                                -     184    +184
> spl_fit_read                                  60     104     +44
> file_fat_read                                 40       -     -40
> spl_nor_load_image                           120      76     -44
> spl_load_image_ext                           364     296     -68
> spl_load_image_fat                           320     200    -120
> spl_spi_load_image                           304     176    -128
> spl_mmc_load                                 716     532    -184
> Total: Before=233618, After=233478, chg -0.06%
> 
> For most boards with a few load methods, this series should break even.
> However, in the worst case this series will add around 100 bytes.
> 
> I have only tested a few loaders. Please try booting your favorite board
> with NOR/SPI flash or SPI falcon mode.
> 
> On sama5d27_wlsom1_ek_mmc_defconfig, 100 bytes is too much, so this
> series depends on [1] to fit everything in. CI run at [2].
> 
> [1] https://lore.kernel.org/u-boot/20230731223327.109865-1-sean.anderson@seco.com/
> [2] https://source.denx.de/u-boot/custodians/u-boot-clk/-/pipelines/17116

I've boot-tested this on my lab and a handful of SD+FAT or SD+raw boot
devices, and it's all worked.  I also did my usual world build
before/after to check out the size changes and well, I don't know.  The
size wins come on platforms where there's both FAT+EXT support.  And
then on mips with say mt7620_rfb I wonder if we're loosing
functionality. But most platforms grow a bit, especially if they're just
a single load type (which is the most common case). Maybe this problem
needs to be approached another way? I've put the whole log over at
https://gist.github.com/trini/e6772b2134e0eb44393364ea98729e06 and maybe
something will jump out to you.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v5 00/11] spl: Use common function for loading/parsing images

[Sean Anderson]

On 8/2/23 16:11, Tom Rini wrote:
> On Mon, Jul 31, 2023 at 06:42:52PM -0400, Sean Anderson wrote:
> 
>> This series adds support for loading all image types (Legacy, FIT (with
>> and without LOAD_FIT_FULL), and i.MX) to the MMC, SPI, NOR, NET, FAT,
>> and EXT load methods. It does this by introducing a helper function
>> which handles the minutiae of invoking the proper parsing function, and
>> reading the rest of the image.
>>
>> Hopefully, this will make it easier for load methods to support all
>> image types that U-Boot supports, without having undocumented
>> unsupported image types. I applied this to several loaders which were
>> invoking spl_load_simple_fit and/or spl_parse_image_header, but I did
>> not use it with others (e.g. DFU/RAM) which had complications in the
>> mix.
>>
>> In order to meet size requirements for some boards, the first two
>> patches of this series are general size-reduction changes. The ffs/fls
>> change in particular should reduce code size across the board. The
>> malloc change also has the potential to reduce code size. I've left it
>> disabled by default, but maybe we can reverse that in the future.
>>
>> Here's some bloat-o-meter for j7200_evm_a72_defconfig with ext4 support
>> enabed:
>>
> add/remove: 2/1 grow/shrink: 1/5 up/down: 444/-584 (-140)
>> Function                                     old     new   delta
>> spl_load                                       -     216    +216
> spl_simple_read                                -     184    +184
>> spl_fit_read                                  60     104     +44
>> file_fat_read                                 40       -     -40
>> spl_nor_load_image                           120      76     -44
>> spl_load_image_ext                           364     296     -68
> spl_load_image_fat                           320     200    -120
>> spl_spi_load_image                           304     176    -128
>> spl_mmc_load                                 716     532    -184
>> Total: Before=233618, After=233478, chg -0.06%
>>
>> For most boards with a few load methods, this series should break even.
>> However, in the worst case this series will add around 100 bytes.
>>
>> I have only tested a few loaders. Please try booting your favorite board
>> with NOR/SPI flash or SPI falcon mode.
>
>> On sama5d27_wlsom1_ek_mmc_defconfig, 100 bytes is too much, so this
>> series depends on [1] to fit everything in. CI run at [2].
>>
>> [1] https://lore.kernel.org/u-boot/20230731223327.109865-1-sean.anderson@seco.com/
>> [2] https://source.denx.de/u-boot/custodians/u-boot-clk/-/pipelines/17116
> 
> I've boot-tested this on my lab and a handful of SD+FAT or SD+raw boot
> devices, and it's all worked.  I also did my usual world build
> before/after to check out the size changes and well, I don't know.  The
> size wins come on platforms where there's both FAT+EXT support.  And
> then on mips with say mt7620_rfb I wonder if we're loosing
> functionality.

Yes we are. I'll address this in v6.

> But most platforms grow a bit, especially if they're just
> a single load type (which is the most common case). Maybe this problem
> needs to be approached another way? I've put the whole log over at
> https://gist.github.com/trini/e6772b2134e0eb44393364ea98729e06 and maybe
> something will jump out to you.

The fls/ffs patch [1] reduces the growth by around 80 bytes on ARM
platforms. I sent it separately for ease of review, but it really should
be applied before this series, since a good portion of the growth is due
to that one function call. It seems like MIPS also has this instruction
(and Linux uses it), so I can try putting together a patch for it as
well.

In the future, I would like to convert bl_len to bl_shift or something,
which would remove the need for fls (and going from bl_shift to bl_len
is just a shift). spl_load_info is used in a lot of places, so I wanted
to do that in a follow-up. On arches with efficient fls implementations
the difference is only around 3 instructions or so.

But fundamentally some of the problem is that the logic is being moved
into multiple translation units, so the compiler can't see enough to
make optimizations. For example, all filesystems use a bl_len of 1, so
all the multiplications/roundings/etc. can be eliminated if the compiler
can see that. For example, on arm64 copying spl_load into spl_fat.c (as
fat_load) and making it static saves us 100 bytes:

add/remove: 3/1 grow/shrink: 0/1 up/down: 324/-92 (232)
Function                                     old     new   delta
spl_load                                       -     160    +160
spl_simple_read                                -     104    +104
spl_fit_read                                   -      60     +60
file_fat_read                                 40       -     -40
spl_load_image_fat                           252     200     -52
Total: Before=66833, After=67065, chg +0.35%

add/remove: 2/1 grow/shrink: 1/0 up/down: 172/-40 (132)
Function                                     old     new   delta
spl_simple_read                                -     104    +104
spl_fit_read                                   -      60     +60
spl_load_image_fat                           252     260      +8
file_fat_read                                 40       -     -40
Total: Before=66833, After=66965, chg +0.20%

and even then, by inspection spl_simple_read isn't really taking
advantage of bl_len=1. Surprisingly, after enabling LTO this board is
+400 bytes (compared to without this series). It's just hard to beat
handwritten routines with a generic solution.

I would really like to consolidate this stuff, since every load method
ends up only supporting a few image types and having almost identical
implementations for what they do support.

--Sean

[1] https://lore.kernel.org/u-boot/20230731212734.7256-1-sean.anderson@seco.com/

Re: [PATCH v5 00/11] spl: Use common function for loading/parsing images

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6405 bytes --]

On Wed, Aug 02, 2023 at 09:13:21PM -0400, Sean Anderson wrote:
> On 8/2/23 16:11, Tom Rini wrote:
> > On Mon, Jul 31, 2023 at 06:42:52PM -0400, Sean Anderson wrote:
> > 
> > > This series adds support for loading all image types (Legacy, FIT (with
> > > and without LOAD_FIT_FULL), and i.MX) to the MMC, SPI, NOR, NET, FAT,
> > > and EXT load methods. It does this by introducing a helper function
> > > which handles the minutiae of invoking the proper parsing function, and
> > > reading the rest of the image.
> > > 
> > > Hopefully, this will make it easier for load methods to support all
> > > image types that U-Boot supports, without having undocumented
> > > unsupported image types. I applied this to several loaders which were
> > > invoking spl_load_simple_fit and/or spl_parse_image_header, but I did
> > > not use it with others (e.g. DFU/RAM) which had complications in the
> > > mix.
> > > 
> > > In order to meet size requirements for some boards, the first two
> > > patches of this series are general size-reduction changes. The ffs/fls
> > > change in particular should reduce code size across the board. The
> > > malloc change also has the potential to reduce code size. I've left it
> > > disabled by default, but maybe we can reverse that in the future.
> > > 
> > > Here's some bloat-o-meter for j7200_evm_a72_defconfig with ext4 support
> > > enabed:
> > > 
> > add/remove: 2/1 grow/shrink: 1/5 up/down: 444/-584 (-140)
> > > Function                                     old     new   delta
> > > spl_load                                       -     216    +216
> > spl_simple_read                                -     184    +184
> > > spl_fit_read                                  60     104     +44
> > > file_fat_read                                 40       -     -40
> > > spl_nor_load_image                           120      76     -44
> > > spl_load_image_ext                           364     296     -68
> > spl_load_image_fat                           320     200    -120
> > > spl_spi_load_image                           304     176    -128
> > > spl_mmc_load                                 716     532    -184
> > > Total: Before=233618, After=233478, chg -0.06%
> > > 
> > > For most boards with a few load methods, this series should break even.
> > > However, in the worst case this series will add around 100 bytes.
> > > 
> > > I have only tested a few loaders. Please try booting your favorite board
> > > with NOR/SPI flash or SPI falcon mode.
> > 
> > > On sama5d27_wlsom1_ek_mmc_defconfig, 100 bytes is too much, so this
> > > series depends on [1] to fit everything in. CI run at [2].
> > > 
> > > [1] https://lore.kernel.org/u-boot/20230731223327.109865-1-sean.anderson@seco.com/
> > > [2] https://source.denx.de/u-boot/custodians/u-boot-clk/-/pipelines/17116
> > 
> > I've boot-tested this on my lab and a handful of SD+FAT or SD+raw boot
> > devices, and it's all worked.  I also did my usual world build
> > before/after to check out the size changes and well, I don't know.  The
> > size wins come on platforms where there's both FAT+EXT support.  And
> > then on mips with say mt7620_rfb I wonder if we're loosing
> > functionality.
> 
> Yes we are. I'll address this in v6.
> 
> > But most platforms grow a bit, especially if they're just
> > a single load type (which is the most common case). Maybe this problem
> > needs to be approached another way? I've put the whole log over at
> > https://gist.github.com/trini/e6772b2134e0eb44393364ea98729e06 and maybe
> > something will jump out to you.
> 
> The fls/ffs patch [1] reduces the growth by around 80 bytes on ARM
> platforms. I sent it separately for ease of review, but it really should
> be applied before this series, since a good portion of the growth is due
> to that one function call. It seems like MIPS also has this instruction
> (and Linux uses it), so I can try putting together a patch for it as
> well.
> 
> In the future, I would like to convert bl_len to bl_shift or something,
> which would remove the need for fls (and going from bl_shift to bl_len
> is just a shift). spl_load_info is used in a lot of places, so I wanted
> to do that in a follow-up. On arches with efficient fls implementations
> the difference is only around 3 instructions or so.
> 
> But fundamentally some of the problem is that the logic is being moved
> into multiple translation units, so the compiler can't see enough to
> make optimizations. For example, all filesystems use a bl_len of 1, so
> all the multiplications/roundings/etc. can be eliminated if the compiler
> can see that. For example, on arm64 copying spl_load into spl_fat.c (as
> fat_load) and making it static saves us 100 bytes:
> 
> add/remove: 3/1 grow/shrink: 0/1 up/down: 324/-92 (232)
> Function                                     old     new   delta
> spl_load                                       -     160    +160
> spl_simple_read                                -     104    +104
> spl_fit_read                                   -      60     +60
> file_fat_read                                 40       -     -40
> spl_load_image_fat                           252     200     -52
> Total: Before=66833, After=67065, chg +0.35%
> 
> add/remove: 2/1 grow/shrink: 1/0 up/down: 172/-40 (132)
> Function                                     old     new   delta
> spl_simple_read                                -     104    +104
> spl_fit_read                                   -      60     +60
> spl_load_image_fat                           252     260      +8
> file_fat_read                                 40       -     -40
> Total: Before=66833, After=66965, chg +0.20%
> 
> and even then, by inspection spl_simple_read isn't really taking
> advantage of bl_len=1. Surprisingly, after enabling LTO this board is
> +400 bytes (compared to without this series). It's just hard to beat
> handwritten routines with a generic solution.
> 
> I would really like to consolidate this stuff, since every load method
> ends up only supporting a few image types and having almost identical
> implementations for what they do support.
> 
> --Sean
> 
> [1] https://lore.kernel.org/u-boot/20230731212734.7256-1-sean.anderson@seco.com/

Thanks for digging more, lets keep going in this direction then.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v5 00/11] spl: Use common function for loading/parsing images

[Heinrich Schuchardt]

On 8/3/23 03:31, Tom Rini wrote:
> On Wed, Aug 02, 2023 at 09:13:21PM -0400, Sean Anderson wrote:
>> On 8/2/23 16:11, Tom Rini wrote:
>>> On Mon, Jul 31, 2023 at 06:42:52PM -0400, Sean Anderson wrote:
>>>
>>>> This series adds support for loading all image types (Legacy, FIT (with
>>>> and without LOAD_FIT_FULL), and i.MX) to the MMC, SPI, NOR, NET, FAT,
>>>> and EXT load methods. It does this by introducing a helper function
>>>> which handles the minutiae of invoking the proper parsing function, and
>>>> reading the rest of the image.
>>>>
>>>> Hopefully, this will make it easier for load methods to support all
>>>> image types that U-Boot supports, without having undocumented
>>>> unsupported image types. I applied this to several loaders which were
>>>> invoking spl_load_simple_fit and/or spl_parse_image_header, but I did
>>>> not use it with others (e.g. DFU/RAM) which had complications in the
>>>> mix.
>>>>
>>>> In order to meet size requirements for some boards, the first two
>>>> patches of this series are general size-reduction changes. The ffs/fls
>>>> change in particular should reduce code size across the board. The
>>>> malloc change also has the potential to reduce code size. I've left it
>>>> disabled by default, but maybe we can reverse that in the future.
>>>>
>>>> Here's some bloat-o-meter for j7200_evm_a72_defconfig with ext4 support
>>>> enabed:
>>>>
>>> add/remove: 2/1 grow/shrink: 1/5 up/down: 444/-584 (-140)
>>>> Function                                     old     new   delta
>>>> spl_load                                       -     216    +216
>>> spl_simple_read                                -     184    +184
>>>> spl_fit_read                                  60     104     +44
>>>> file_fat_read                                 40       -     -40
>>>> spl_nor_load_image                           120      76     -44
>>>> spl_load_image_ext                           364     296     -68
>>> spl_load_image_fat                           320     200    -120
>>>> spl_spi_load_image                           304     176    -128
>>>> spl_mmc_load                                 716     532    -184
>>>> Total: Before=233618, After=233478, chg -0.06%
>>>>
>>>> For most boards with a few load methods, this series should break even.
>>>> However, in the worst case this series will add around 100 bytes.
>>>>
>>>> I have only tested a few loaders. Please try booting your favorite board
>>>> with NOR/SPI flash or SPI falcon mode.
>>>
>>>> On sama5d27_wlsom1_ek_mmc_defconfig, 100 bytes is too much, so this
>>>> series depends on [1] to fit everything in. CI run at [2].
>>>>
>>>> [1] https://lore.kernel.org/u-boot/20230731223327.109865-1-sean.anderson@seco.com/
>>>> [2] https://source.denx.de/u-boot/custodians/u-boot-clk/-/pipelines/17116
>>>
>>> I've boot-tested this on my lab and a handful of SD+FAT or SD+raw boot
>>> devices, and it's all worked.  I also did my usual world build
>>> before/after to check out the size changes and well, I don't know.  The
>>> size wins come on platforms where there's both FAT+EXT support.  And
>>> then on mips with say mt7620_rfb I wonder if we're loosing
>>> functionality.
>>
>> Yes we are. I'll address this in v6.
>>
>>> But most platforms grow a bit, especially if they're just
>>> a single load type (which is the most common case). Maybe this problem
>>> needs to be approached another way? I've put the whole log over at
>>> https://gist.github.com/trini/e6772b2134e0eb44393364ea98729e06 and maybe
>>> something will jump out to you.
>>
>> The fls/ffs patch [1] reduces the growth by around 80 bytes on ARM
>> platforms. I sent it separately for ease of review, but it really should
>> be applied before this series, since a good portion of the growth is due
>> to that one function call. It seems like MIPS also has this instruction
>> (and Linux uses it), so I can try putting together a patch for it as
>> well.
>>
>> In the future, I would like to convert bl_len to bl_shift or something,
>> which would remove the need for fls (and going from bl_shift to bl_len
>> is just a shift). spl_load_info is used in a lot of places, so I wanted
>> to do that in a follow-up. On arches with efficient fls implementations
>> the difference is only around 3 instructions or so.
>>
>> But fundamentally some of the problem is that the logic is being moved
>> into multiple translation units, so the compiler can't see enough to
>> make optimizations. For example, all filesystems use a bl_len of 1, so
>> all the multiplications/roundings/etc. can be eliminated if the compiler
>> can see that. For example, on arm64 copying spl_load into spl_fat.c (as
>> fat_load) and making it static saves us 100 bytes:
>>
>> add/remove: 3/1 grow/shrink: 0/1 up/down: 324/-92 (232)
>> Function                                     old     new   delta
>> spl_load                                       -     160    +160
>> spl_simple_read                                -     104    +104
>> spl_fit_read                                   -      60     +60
>> file_fat_read                                 40       -     -40
>> spl_load_image_fat                           252     200     -52
>> Total: Before=66833, After=67065, chg +0.35%
>>
>> add/remove: 2/1 grow/shrink: 1/0 up/down: 172/-40 (132)
>> Function                                     old     new   delta
>> spl_simple_read                                -     104    +104
>> spl_fit_read                                   -      60     +60
>> spl_load_image_fat                           252     260      +8
>> file_fat_read                                 40       -     -40
>> Total: Before=66833, After=66965, chg +0.20%
>>
>> and even then, by inspection spl_simple_read isn't really taking
>> advantage of bl_len=1. Surprisingly, after enabling LTO this board is
>> +400 bytes (compared to without this series). It's just hard to beat
>> handwritten routines with a generic solution.
>>
>> I would really like to consolidate this stuff, since every load method
>> ends up only supporting a few image types and having almost identical
>> implementations for what they do support.
>>
>> --Sean
>>
>> [1] https://lore.kernel.org/u-boot/20230731212734.7256-1-sean.anderson@seco.com/
>
> Thanks for digging more, lets keep going in this direction then.
>

Do we need to pass sectors to the info->load() functions?

Only nand and mmc use bl_len != 1. Can we move the conversion to sectors
and the alignment checks to functions called by info->load()?

I would assume that this can avoid part of the code size increase.

Best regards

Heinrich

Re: [PATCH v5 04/11] spl: Convert fat to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:42:56PM -0400, Sean Anderson deia:
> This converts the fat loader to use spl_load. Some platforms are very
> tight on space, so we take care to only include the code we really need.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
> Changes in v3:
> - Fix failing on success
> 
>  common/spl/spl_fat.c | 55 +++++++++++++++-----------------------------
>  1 file changed, 19 insertions(+), 36 deletions(-)
> 
> diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
> index f8a5b80a3b..6530bcd5a7 100644
> --- a/common/spl/spl_fat.c
> +++ b/common/spl/spl_fat.c
> @@ -60,57 +60,40 @@ int spl_load_image_fat(struct spl_image_info *spl_image,
>  		       const char *filename)
>  {
>  	int err;
> -	struct legacy_img_hdr *header;
> +	loff_t size;
> +	struct spl_load_info load;
> +
> +	/* This generates smaller code than using a compound literal */
> +	load.read = spl_fit_read;
> +	load.bl_len = 1;
> +	load.filename = filename;
>  
>  	err = spl_register_fat_device(block_dev, partition);
>  	if (err)
>  		goto end;
>  
> -	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
> -
> -	err = file_fat_read(filename, header, sizeof(struct legacy_img_hdr));
> -	if (err <= 0)
> -		goto end;
> -
> -	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
> -	    image_get_magic(header) == FDT_MAGIC) {
> -		err = file_fat_read(filename, (void *)CONFIG_SYS_LOAD_ADDR, 0);
> -		if (err <= 0)
> -			goto end;
> -		err = spl_parse_image_header(spl_image, bootdev,
> -				(struct legacy_img_hdr *)CONFIG_SYS_LOAD_ADDR);

So this used to load to CONFIG_SYS_LOAD_ADDR and will now load to
CONFIG_TEXT_BASE, or whatever the applicable spl_get_load_buffer()
uses. Is this OK ? or do we need a new parameter for the buffer or
something ?

> -		if (err == -EAGAIN)
> -			return err;
> -		if (err == 0)
> -			err = 1;
> -	} else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> -	    image_get_magic(header) == FDT_MAGIC) {
> -		struct spl_load_info load;
> -
> -		debug("Found FIT\n");
> -		load.read = spl_fit_read;
> -		load.bl_len = 1;
> -		load.filename = (void *)filename;
> -		load.priv = NULL;
> -
> -		return spl_load_simple_fit(spl_image, &load, 0, header);
> -	} else {
> -		err = spl_parse_image_header(spl_image, bootdev, header);
> +	/*
> +	 * Avoid pulling in this function for other image types since we are
> +	 * very short on space on some boards.
> +	 */
> +	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL)) {
> +		err = fat_size(filename, &size);
>  		if (err)
>  			goto end;
> -
> -		err = file_fat_read(filename,
> -				    (u8 *)(uintptr_t)spl_image->load_addr, 0);
> +	} else {
> +		size = 0;
>  	}
>  
> +	err = spl_load(spl_image, bootdev, &load, 0, size);
> +
>  end:
>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> -	if (err <= 0)
> +	if (err < 0)
>  		printf("%s: error reading image %s, err - %d\n",
>  		       __func__, filename, err);
>  #endif
>  
> -	return (err <= 0);
> +	return err;
>  }
>  
>  #if CONFIG_IS_ENABLED(OS_BOOT)
> -- 
> 2.40.1
> 

Re: [PATCH v5 05/11] spl: Convert mmc to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:42:57PM -0400, Sean Anderson deia:
> This converts the mmc loader to spl_load. Legacy images are handled by
> spl_load (via spl_parse_image_header), so mmc_load_legacy can be
> omitted.
>

Yes. I haven't used the legacy case, but by looking at the code, it
seems to me that mmc_load_legacy left the image at some mapped memory
at [ spl_image->load_addr,   spl_image->load_addr + size )
and the new code does too, but when the image is not aligned, the
memory that gets written to
was at [ spl_image->load_addr,   spl_image->load_addr + size + spl_image->offset % mmc->read_bl_len )
and it will
be at [ spl_image->load_addr - spl_image->offset % mmc->read_bl_len,   spl_image->load_addr + size )
after this patch.

Meaning both with or without this patch some memory outside the image
gets written when the image is not aligned on media, but before it was
some part of a block at the end and now is that part before the
beginning.

Whether that's better or worse I don't know. It depends on whether
it's a problem to write in non mapped memory, whether there's
something worth preserving there, and whether some SOC has some sort
of special behaving memory just there, like it happened with the issue
Jerome Forissier found (note in this case it wasn't legacy, it was
simple fit)

https://patchwork.ozlabs.org/project/uboot/patch/c941d835a85255ff81a21c72069c3a9fc9a8a255.1656489154.git.jerome.forissier@linaro.org/
                                                                  
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
>  common/spl/spl_mmc.c | 91 ++++----------------------------------------
>  1 file changed, 8 insertions(+), 83 deletions(-)
> 
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index a665091b00..c926a42c0f 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -17,48 +17,6 @@
>  #include <mmc.h>
>  #include <image.h>
>  
> -static int mmc_load_legacy(struct spl_image_info *spl_image,
> -			   struct spl_boot_device *bootdev,
> -			   struct mmc *mmc,
> -			   ulong sector, struct legacy_img_hdr *header)
> -{
> -	u32 image_offset_sectors;
> -	u32 image_size_sectors;
> -	unsigned long count;
> -	u32 image_offset;
> -	int ret;
> -
> -	ret = spl_parse_image_header(spl_image, bootdev, header);
> -	if (ret)
> -		return ret;
> -
> -	/* convert offset to sectors - round down */
> -	image_offset_sectors = spl_image->offset / mmc->read_bl_len;
> -	/* calculate remaining offset */
> -	image_offset = spl_image->offset % mmc->read_bl_len;
> -
> -	/* convert size to sectors - round up */
> -	image_size_sectors = (spl_image->size + mmc->read_bl_len - 1) /
> -			     mmc->read_bl_len;
> -
> -	/* Read the header too to avoid extra memcpy */
> -	count = blk_dread(mmc_get_blk_desc(mmc),
> -			  sector + image_offset_sectors,
> -			  image_size_sectors,
> -			  (void *)(ulong)spl_image->load_addr);
> -	debug("read %x sectors to %lx\n", image_size_sectors,
> -	      spl_image->load_addr);
> -	if (count != image_size_sectors)
> -		return -EIO;
> -
> -	if (image_offset)
> -		memmove((void *)(ulong)spl_image->load_addr,
> -			(void *)(ulong)spl_image->load_addr + image_offset,
> -			spl_image->size);
> -
> -	return 0;
> -}
> -
>  static ulong h_spl_load_read(struct spl_load_info *load, ulong sector,
>  			     ulong count, void *buf)
>  {
> @@ -82,47 +40,14 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
>  			      struct spl_boot_device *bootdev,
>  			      struct mmc *mmc, unsigned long sector)
>  {
> -	unsigned long count;
> -	struct legacy_img_hdr *header;
> -	struct blk_desc *bd = mmc_get_blk_desc(mmc);
> -	int ret = 0;
> -
> -	header = spl_get_load_buffer(-sizeof(*header), bd->blksz);
> -
> -	/* read image header to find the image size & load address */
> -	count = blk_dread(bd, sector, 1, header);
> -	debug("hdr read sector %lx, count=%lu\n", sector, count);
> -	if (count == 0) {
> -		ret = -EIO;
> -		goto end;
> -	}
> -
> -	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> -	    image_get_magic(header) == FDT_MAGIC) {
> -		struct spl_load_info load;
> -
> -		debug("Found FIT\n");
> -		load.dev = mmc;
> -		load.priv = NULL;
> -		load.filename = NULL;
> -		load.bl_len = mmc->read_bl_len;
> -		load.read = h_spl_load_read;
> -		ret = spl_load_simple_fit(spl_image, &load, sector, header);
> -	} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> -		struct spl_load_info load;
> -
> -		load.dev = mmc;
> -		load.priv = NULL;
> -		load.filename = NULL;
> -		load.bl_len = mmc->read_bl_len;
> -		load.read = h_spl_load_read;
> -
> -		ret = spl_load_imx_container(spl_image, &load, sector);
> -	} else {
> -		ret = mmc_load_legacy(spl_image, bootdev, mmc, sector, header);
> -	}
> -
> -end:
> +	int ret;
> +	struct spl_load_info load = {
> +		.dev = mmc,
> +		.bl_len = mmc->read_bl_len,
> +		.read = h_spl_load_read,
> +	};
> +
> +	ret = spl_load(spl_image, bootdev, &load, 0, sector);
>  	if (ret) {
>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>  		puts("mmc_load_image_raw_sector: mmc block read error\n");
> -- 
> 2.40.1
> 

Re: [PATCH v5 07/11] spl: Convert NVMe to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:42:59PM -0400, Sean Anderson deia:
> This converts the blk load method (used exclusively by NVMe) to use
> spl_load. As a consequence, it also adds support for LOAD_FIT_FULL and
> IMX images.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v5:
> - New
> 
>  common/spl/spl_blk_fs.c | 68 ++++++++---------------------------------
>  1 file changed, 12 insertions(+), 56 deletions(-)
> 
> diff --git a/common/spl/spl_blk_fs.c b/common/spl/spl_blk_fs.c
> index d97adc4d39..eb5775a470 100644
> --- a/common/spl/spl_blk_fs.c
> +++ b/common/spl/spl_blk_fs.c
> @@ -45,24 +45,28 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
>  {
>  	const char *filename = CONFIG_SPL_PAYLOAD;
>  	struct disk_partition part_info = {};
> -	struct legacy_img_hdr *header;
>  	struct blk_desc *blk_desc;
> -	loff_t actlen, filesize;
> +	loff_t filesize;
>  	struct blk_dev dev;
>  	int ret;
> +	struct spl_load_info load = {
> +		.read = spl_fit_read,
> +		.bl_len = 1,
> +		.filename = filename,
> +		.priv = &dev,
> +	};
>  
>  	blk_desc = blk_get_devnum_by_uclass_id(uclass_id, devnum);
>  	if (!blk_desc) {
>  		printf("blk desc for %d %d not found\n", uclass_id, devnum);
> -		goto out;
> +		return ret;

What do you mean return ret ?
ret is unused yet ?
maybe return -1 or return -ENODEV or something ?

I think the error was already there before your patch, but maybe worth
fixing ?

>  	}
>  
>  	blk_show_device(uclass_id, devnum);
> -	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>  	ret = part_get_info(blk_desc, 1, &part_info);
>  	if (ret) {
>  		printf("spl: no partition table found. Err - %d\n", ret);
> -		goto out;
> +		return ret;
>  	}
>  
>  	dev.ifname = blk_get_uclass_name(uclass_id);
> @@ -72,63 +76,15 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
>  	if (ret) {
>  		printf("spl: unable to set blk_dev %s %s. Err - %d\n",
>  		       dev.ifname, dev.dev_part_str, ret);
> -		goto out;
> -	}
> -
> -	ret = fs_read(filename, (ulong)header, 0,
> -		      sizeof(struct legacy_img_hdr), &actlen);
> -	if (ret) {
> -		printf("spl: unable to read file %s. Err - %d\n", filename,
> -		       ret);
> -		goto out;
> -	}
> -
> -	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> -	    image_get_magic(header) == FDT_MAGIC) {
> -		struct spl_load_info load;
> -
> -		debug("Found FIT\n");
> -		load.read = spl_fit_read;
> -		load.bl_len = 1;
> -		load.filename = (void *)filename;
> -		load.priv = &dev;
> -
> -		return spl_load_simple_fit(spl_image, &load, 0, header);
> -	}
> -
> -	ret = spl_parse_image_header(spl_image, bootdev, header);
> -	if (ret) {
> -		printf("spl: unable to parse image header. Err - %d\n",
> -		       ret);
> -		goto out;
> -	}
> -
> -	ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str, FS_TYPE_ANY);
> -	if (ret) {
> -		printf("spl: unable to set blk_dev %s %s. Err - %d\n",
> -		       dev.ifname, dev.dev_part_str, ret);
> -		goto out;
> +		return ret;
>  	}
>  
>  	ret = fs_size(filename, &filesize);
>  	if (ret) {
>  		printf("spl: unable to get file size: %s. Err - %d\n",
>  		       filename, ret);
> -		goto out;
> +		return ret;
>  	}
>  
> -	ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str, FS_TYPE_ANY);
> -	if (ret) {
> -		printf("spl: unable to set blk_dev %s %s. Err - %d\n",
> -		       dev.ifname, dev.dev_part_str, ret);
> -		goto out;
> -	}
> -
> -	ret = fs_read(filename, (ulong)spl_image->load_addr, 0, filesize,
> -		      &actlen);
> -	if (ret)
> -		printf("spl: unable to read file %s. Err - %d\n",
> -		       filename, ret);
> -out:
> -	return ret;
> +	return spl_load(spl_image, bootdev, &load, filesize, 0);
>  }
> -- 
> 2.40.1
> 

Re: [PATCH v5 08/11] spl: Convert nor to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:43:00PM -0400, Sean Anderson deia:
> This converts the nor load method to use spl_load. As a result it also
> adds support for LOAD_FIT_FULL.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
>  common/spl/spl_nor.c | 41 +++++++----------------------------------
>  1 file changed, 7 insertions(+), 34 deletions(-)
> 
> diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> index 5b65b96a77..7328a87024 100644
> --- a/common/spl/spl_nor.c
> +++ b/common/spl/spl_nor.c
> @@ -26,8 +26,10 @@ unsigned long __weak spl_nor_get_uboot_base(void)
>  static int spl_nor_load_image(struct spl_image_info *spl_image,
>  			      struct spl_boot_device *bootdev)
>  {
> -	__maybe_unused const struct legacy_img_hdr *header;
> -	__maybe_unused struct spl_load_info load;
> +	struct spl_load_info load = {
> +		.bl_len = 1,
> +		.read = spl_nor_load_read,
> +	};
>  
>  	/*
>  	 * Loading of the payload to SDRAM is done with skipping of
> @@ -41,7 +43,8 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>  		 * Load Linux from its location in NOR flash to its defined
>  		 * location in SDRAM
>  		 */
> -		header = (const struct legacy_img_hdr *)CONFIG_SYS_OS_BASE;
> +		const struct legacy_img_hdr *header =
> +			(const struct legacy_img_hdr *)CONFIG_SYS_OS_BASE;
>  #ifdef CONFIG_SPL_LOAD_FIT
>  		if (image_get_magic(header) == FDT_MAGIC) {
>  			int ret;
> @@ -91,36 +94,6 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>  	 * Load real U-Boot from its location in NOR flash to its
>  	 * defined location in SDRAM
>  	 */
> -#ifdef CONFIG_SPL_LOAD_FIT
> -	header = (const struct legacy_img_hdr *)spl_nor_get_uboot_base();
> -	if (image_get_magic(header) == FDT_MAGIC) {
> -		debug("Found FIT format U-Boot\n");
> -		load.bl_len = 1;
> -		load.read = spl_nor_load_read;
> -		return spl_load_simple_fit(spl_image, &load,
> -					   spl_nor_get_uboot_base(),
> -					   (void *)header);

this loaded the simple fit from sector=CFG_SYS_UBOOT_BASE and now we'll
call spl_load with sector=0 ?

But spl_nor_get_uboot_base() is overriden by calculations of
concatenated images in

arch/arm/mach-imx/image-container.c
arch/mips/mach-mtmips/mt7621/spl/spl.c
arch/mips/mach-mtmips/spl.c

> -	}
> -#endif
> -	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> -		load.bl_len = 1;
> -		load.read = spl_nor_load_read;
> -		return spl_load_imx_container(spl_image, &load,
> -					      spl_nor_get_uboot_base());

this loaded the imx image from sector=CFG_SYS_UBOOT_BASE or whatever
spl_nor_get_uboot_base() gave and now we'll call spl_load with
sector=0 ?

> -	}
> -
> -	/* Legacy image handling */
> -	if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_FORMAT)) {
> -		struct legacy_img_hdr hdr;
> -
> -		load.bl_len = 1;
> -		load.read = spl_nor_load_read;
> -		spl_nor_load_read(&load, spl_nor_get_uboot_base(), sizeof(hdr), &hdr);
> -		return spl_load_legacy_img(spl_image, bootdev, &load,
> -					   spl_nor_get_uboot_base(),
> -					   &hdr);

This loaded legacy image with potential lzma decompression and now
compressed images are no longer supported ?

> -	}
> -
> -	return -EINVAL;
> +	return spl_load(spl_image, bootdev, &load, 0, 0);

maybe better

+	return spl_load(spl_image, bootdev, &load, 0, spl_nor_get_uboot_base());


and consider calling spl_load_legacy_img(spl_image, bootdev, &info, offset, header)
from spl_load()?

>  }
>  SPL_LOAD_IMAGE_METHOD("NOR", 0, BOOT_DEVICE_NOR, spl_nor_load_image);
> -- 
> 2.40.1
> 

Re: [PATCH v5 09/11] spl: Convert semihosting to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:43:01PM -0400, Sean Anderson deia:
> This converts the semihosting load method to use spl_load. As a result, it
> also adds support for LOAD_FIT_FULL and IMX images.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
> Changes in v2:
> - New
> 
>  common/spl/spl_semihosting.c | 43 ++++++++++++------------------------
>  1 file changed, 14 insertions(+), 29 deletions(-)
> 
> diff --git a/common/spl/spl_semihosting.c b/common/spl/spl_semihosting.c
> index 5b5e842a11..e7cb9f4ce6 100644
> --- a/common/spl/spl_semihosting.c
> +++ b/common/spl/spl_semihosting.c
> @@ -9,16 +9,16 @@
>  #include <semihosting.h>
>  #include <spl.h>
>  
> -static int smh_read_full(long fd, void *memp, size_t len)
> +static ulong spl_smh_fit_read(struct spl_load_info *load, ulong sector,
> +			      ulong count, void *buf)
>  {
> -	long read;
> +	int ret, fd = *(int *)load->priv;
>

should ret be long to hold smh_read() return value ?

> -	read = smh_read(fd, memp, len);
> -	if (read < 0)
> -		return read;
> -	if (read != len)
> -		return -EIO;
> -	return 0;
> +	if (smh_seek(fd, sector))
> +		return 0;
> +

                                                                                                                 
I never used smh, but why would this not be :

+       ret = smh_seek(fd, sector);
+       if (ret)
+               return ret;

(why does smh_seek(), smh_write(), smh_open(), smh_close() return
long, by the way? the implementations return either 0 or smh_errno(),
so int would do ?)
                   
> +	ret = smh_read(fd, buf, count);
> +	return ret < 0 ? 0 : ret;
>  }
>  
>  static int spl_smh_load_image(struct spl_image_info *spl_image,
> @@ -27,14 +27,17 @@ static int spl_smh_load_image(struct spl_image_info *spl_image,
>  	const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
>  	int ret;
>  	long fd, len;
> -	struct legacy_img_hdr *header =
> -		spl_get_load_buffer(-sizeof(*header), sizeof(*header));
> +	struct spl_load_info load = {
> +		.bl_len = 1,
> +		.read = spl_smh_fit_read,
> +	};
>  
>  	fd = smh_open(filename, MODE_READ | MODE_BINARY);
>  	if (fd < 0) {
>  		log_debug("could not open %s: %ld\n", filename, fd);
>  		return fd;
>  	}
> +	load.priv = &fd;
>  
>  	ret = smh_flen(fd);
>  	if (ret < 0) {
> @@ -43,25 +46,7 @@ static int spl_smh_load_image(struct spl_image_info *spl_image,
>  	}
>  	len = ret;
>  
> -	ret = smh_read_full(fd, header, sizeof(struct legacy_img_hdr));
> -	if (ret) {
> -		log_debug("could not read image header: %d\n", ret);
> -		goto out;
> -	}
> -
> -	ret = spl_parse_image_header(spl_image, bootdev, header);
> -	if (ret) {
> -		log_debug("failed to parse image header: %d\n", ret);
> -		goto out;
> -	}
> -
> -	ret = smh_seek(fd, 0);
> -	if (ret) {
> -		log_debug("could not seek to start of image: %d\n", ret);
> -		goto out;
> -	}
> -
> -	ret = smh_read_full(fd, (void *)spl_image->load_addr, len);
> +	ret = spl_load(spl_image, bootdev, &load, len, 0);
>  	if (ret)
>  		log_debug("could not read %s: %d\n", filename, ret);
>  out:
> -- 
> 2.40.1
> 

Re: [PATCH v5 10/11] spl: Convert spi to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:43:02PM -0400, Sean Anderson deia:
> This converts the spi load method to use spl_load. As a consequence, it
> also adds support for LOAD_FIT_FULL.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
>  common/spl/spl_spi.c | 72 +++++---------------------------------------
>  1 file changed, 8 insertions(+), 64 deletions(-)
> 
> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> index 2aff025f76..14391a1c96 100644
> --- a/common/spl/spl_spi.c
> +++ b/common/spl/spl_spi.c
> @@ -89,12 +89,14 @@ u32 __weak spl_spi_boot_cs(void)
>  static int spl_spi_load_image(struct spl_image_info *spl_image,
>  			      struct spl_boot_device *bootdev)
>  {
> -	int err = 0;
>  	unsigned int payload_offs;
>  	struct spi_flash *flash;
> -	struct legacy_img_hdr *header;
>  	unsigned int sf_bus = spl_spi_boot_bus();
>  	unsigned int sf_cs = spl_spi_boot_cs();
> +	struct spl_load_info load = {
> +		.bl_len = 1,
> +		.read = spl_spi_fit_read,
> +	};
>  
>  	/*
>  	 * Load U-Boot image from SPI flash into RAM
> @@ -109,77 +111,19 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
>  		return -ENODEV;
>  	}
>  
> +	load.dev = flash;
>  	payload_offs = spl_spi_get_uboot_offs(flash);
>  
> -	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
> -
>  	if (CONFIG_IS_ENABLED(OF_REAL)) {
>  		payload_offs = ofnode_conf_read_int("u-boot,spl-payload-offset",
>  						    payload_offs);
>  	}
>  
>  #if CONFIG_IS_ENABLED(OS_BOOT)
> -	if (spl_start_uboot() || spi_load_image_os(spl_image, bootdev, flash, header))
> +	if (!spl_start_uboot() && !spi_load_image_os(spl_image, bootdev, flash, header))
> +		return 0;
>  #endif

But with this return 0 we're skipping the soft reset at the end, aren't we ?
This is the same the old code did. Just not sure whether it was right or untested.
If some flash needs reset before running linux, then it might need it with U-Boot or in Falcon,
as long as SPL has probed the flash, mightn't it ?
If it needs fixing, then possibly better in a different commit, though ?
I mean yours is fine, you left things as they were.

> -	{
> -		/* Load u-boot, mkimage header is 64 bytes. */
> -		err = spi_flash_read(flash, payload_offs, sizeof(*header),
> -				     (void *)header);
> -		if (err) {
> -			debug("%s: Failed to read from SPI flash (err=%d)\n",
> -			      __func__, err);
> -			return err;
> -		}
> -
> -		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
> -		    image_get_magic(header) == FDT_MAGIC) {
> -			err = spi_flash_read(flash, payload_offs,
> -					     roundup(fdt_totalsize(header), 4),
> -					     (void *)CONFIG_SYS_LOAD_ADDR);
> -			if (err)
> -				return err;
> -			err = spl_parse_image_header(spl_image, bootdev,
> -					(struct legacy_img_hdr *)CONFIG_SYS_LOAD_ADDR);
                                                                                                            
So this used to load to CONFIG_SYS_LOAD_ADDR and will now load to
CONFIG_TEXT_BASE, or whatever the applicable spl_get_load_buffer()
uses. Is this OK ? or do we need a new parameter for the buffer or
something ?
       
> -		} else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> -			   image_get_magic(header) == FDT_MAGIC) {
> -			struct spl_load_info load;
> -
> -			debug("Found FIT\n");
> -			load.dev = flash;
> -			load.priv = NULL;
> -			load.filename = NULL;
> -			load.bl_len = 1;
> -			load.read = spl_spi_fit_read;
> -			err = spl_load_simple_fit(spl_image, &load,
> -						  payload_offs,
> -						  header);
> -		} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> -			struct spl_load_info load;
> -
> -			load.dev = flash;
> -			load.priv = NULL;
> -			load.filename = NULL;
> -			load.bl_len = 1;
> -			load.read = spl_spi_fit_read;
> -
> -			err = spl_load_imx_container(spl_image, &load,
> -						     payload_offs);
> -		} else {
> -			err = spl_parse_image_header(spl_image, bootdev, header);
> -			if (err)
> -				return err;
> -			err = spi_flash_read(flash, payload_offs + spl_image->offset,
> -					     spl_image->size,
> -					     (void *)spl_image->load_addr);
> -		}


> -		if (IS_ENABLED(CONFIG_SPI_FLASH_SOFT_RESET)) {
> -			err = spi_nor_remove(flash);
> -			if (err)
> -				return err;
> -		}

This soft reset is removed ?
Shouldn't we keep this if after the call to spl_load and before returning its result ?

> -	}
> -
> -	return err;
> +	return spl_load(spl_image, bootdev, &load, 0, payload_offs);
>  }
>  /* Use priorty 1 so that boards can override this */
>  SPL_LOAD_IMAGE_METHOD("SPI", 1, BOOT_DEVICE_SPI, spl_spi_load_image);
> -- 
> 2.40.1
> 

Re: [PATCH v5 09/11] spl: Convert semihosting to spl_load

[Sean Anderson]

On 8/3/23 04:36, Xavier Drudis Ferran wrote:
> El Mon, Jul 31, 2023 at 06:43:01PM -0400, Sean Anderson deia:
>> This converts the semihosting load method to use spl_load. As a result, it
>> also adds support for LOAD_FIT_FULL and IMX images.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v5:
>> - Rework to load header in spl_load
>> 
>> Changes in v2:
>> - New
>> 
>>  common/spl/spl_semihosting.c | 43 ++++++++++++------------------------
>>  1 file changed, 14 insertions(+), 29 deletions(-)
>> 
>> diff --git a/common/spl/spl_semihosting.c b/common/spl/spl_semihosting.c
>> index 5b5e842a11..e7cb9f4ce6 100644
>> --- a/common/spl/spl_semihosting.c
>> +++ b/common/spl/spl_semihosting.c
>> @@ -9,16 +9,16 @@
>>  #include <semihosting.h>
>>  #include <spl.h>
>>  
>> -static int smh_read_full(long fd, void *memp, size_t len)
>> +static ulong spl_smh_fit_read(struct spl_load_info *load, ulong sector,
>> +			      ulong count, void *buf)
>>  {
>> -	long read;
>> +	int ret, fd = *(int *)load->priv;
>>
> 
> should ret be long to hold smh_read() return value ?

Yes.

>> -	read = smh_read(fd, memp, len);
>> -	if (read < 0)
>> -		return read;
>> -	if (read != len)
>> -		return -EIO;
>> -	return 0;
>> +	if (smh_seek(fd, sector))
>> +		return 0;
>> +
> 
>                                                                                                                  
> I never used smh, but why would this not be :
> 
> +       ret = smh_seek(fd, sector);
> +       if (ret)
> +               return ret;

Because we always return the number of bytes read. So by returning 0 we
signal an error.

> (why does smh_seek(), smh_write(), smh_open(), smh_close() return
> long, by the way? the implementations return either 0 or smh_errno(),
> so int would do ?)

Only smh_seek/smh_close always returns 0 or smh_errno. The rest return
values from smh_trap. The reason why we use long instead of int is that
the semihosting ABI always uses native-register-sized values (aka long).
We could use int instead of long for those two functions, but I don't
think its very critical.

--Sean

>> +	ret = smh_read(fd, buf, count);
>> +	return ret < 0 ? 0 : ret;
>>  }
>>  
>>  static int spl_smh_load_image(struct spl_image_info *spl_image,
>> @@ -27,14 +27,17 @@ static int spl_smh_load_image(struct spl_image_info *spl_image,
>>  	const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
>>  	int ret;
>>  	long fd, len;
>> -	struct legacy_img_hdr *header =
>> -		spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>> +	struct spl_load_info load = {
>> +		.bl_len = 1,
>> +		.read = spl_smh_fit_read,
>> +	};
>>  
>>  	fd = smh_open(filename, MODE_READ | MODE_BINARY);
>>  	if (fd < 0) {
>>  		log_debug("could not open %s: %ld\n", filename, fd);
>>  		return fd;
>>  	}
>> +	load.priv = &fd;
>>  
>>  	ret = smh_flen(fd);
>>  	if (ret < 0) {
>> @@ -43,25 +46,7 @@ static int spl_smh_load_image(struct spl_image_info *spl_image,
>>  	}
>>  	len = ret;
>>  
>> -	ret = smh_read_full(fd, header, sizeof(struct legacy_img_hdr));
>> -	if (ret) {
>> -		log_debug("could not read image header: %d\n", ret);
>> -		goto out;
>> -	}
>> -
>> -	ret = spl_parse_image_header(spl_image, bootdev, header);
>> -	if (ret) {
>> -		log_debug("failed to parse image header: %d\n", ret);
>> -		goto out;
>> -	}
>> -
>> -	ret = smh_seek(fd, 0);
>> -	if (ret) {
>> -		log_debug("could not seek to start of image: %d\n", ret);
>> -		goto out;
>> -	}
>> -
>> -	ret = smh_read_full(fd, (void *)spl_image->load_addr, len);
>> +	ret = spl_load(spl_image, bootdev, &load, len, 0);
>>  	if (ret)
>>  		log_debug("could not read %s: %d\n", filename, ret);
>>  out:
>> -- 
>> 2.40.1
>> 

Re: [PATCH v5 08/11] spl: Convert nor to spl_load

[Sean Anderson]

On 8/3/23 04:33, Xavier Drudis Ferran wrote:
> El Mon, Jul 31, 2023 at 06:43:00PM -0400, Sean Anderson deia:
>> This converts the nor load method to use spl_load. As a result it also
>> adds support for LOAD_FIT_FULL.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v5:
>> - Rework to load header in spl_load
>> 
>>  common/spl/spl_nor.c | 41 +++++++----------------------------------
>>  1 file changed, 7 insertions(+), 34 deletions(-)
>> 
>> diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
>> index 5b65b96a77..7328a87024 100644
>> --- a/common/spl/spl_nor.c
>> +++ b/common/spl/spl_nor.c
>> @@ -26,8 +26,10 @@ unsigned long __weak spl_nor_get_uboot_base(void)
>>  static int spl_nor_load_image(struct spl_image_info *spl_image,
>>  			      struct spl_boot_device *bootdev)
>>  {
>> -	__maybe_unused const struct legacy_img_hdr *header;
>> -	__maybe_unused struct spl_load_info load;
>> +	struct spl_load_info load = {
>> +		.bl_len = 1,
>> +		.read = spl_nor_load_read,
>> +	};
>>  
>>  	/*
>>  	 * Loading of the payload to SDRAM is done with skipping of
>> @@ -41,7 +43,8 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>>  		 * Load Linux from its location in NOR flash to its defined
>>  		 * location in SDRAM
>>  		 */
>> -		header = (const struct legacy_img_hdr *)CONFIG_SYS_OS_BASE;
>> +		const struct legacy_img_hdr *header =
>> +			(const struct legacy_img_hdr *)CONFIG_SYS_OS_BASE;
>>  #ifdef CONFIG_SPL_LOAD_FIT
>>  		if (image_get_magic(header) == FDT_MAGIC) {
>>  			int ret;
>> @@ -91,36 +94,6 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>>  	 * Load real U-Boot from its location in NOR flash to its
>>  	 * defined location in SDRAM
>>  	 */
>> -#ifdef CONFIG_SPL_LOAD_FIT
>> -	header = (const struct legacy_img_hdr *)spl_nor_get_uboot_base();
>> -	if (image_get_magic(header) == FDT_MAGIC) {
>> -		debug("Found FIT format U-Boot\n");
>> -		load.bl_len = 1;
>> -		load.read = spl_nor_load_read;
>> -		return spl_load_simple_fit(spl_image, &load,
>> -					   spl_nor_get_uboot_base(),
>> -					   (void *)header);
> 
> this loaded the simple fit from sector=CFG_SYS_UBOOT_BASE and now we'll
> call spl_load with sector=0 ?
> 
> But spl_nor_get_uboot_base() is overriden by calculations of
> concatenated images in
> 
> arch/arm/mach-imx/image-container.c
> arch/mips/mach-mtmips/mt7621/spl/spl.c
> arch/mips/mach-mtmips/spl.c
> 
>> -	}
>> -#endif
>> -	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
>> -		load.bl_len = 1;
>> -		load.read = spl_nor_load_read;
>> -		return spl_load_imx_container(spl_image, &load,
>> -					      spl_nor_get_uboot_base());
> 
> this loaded the imx image from sector=CFG_SYS_UBOOT_BASE or whatever
> spl_nor_get_uboot_base() gave and now we'll call spl_load with
> sector=0 ?
> 
>> -	}
>> -
>> -	/* Legacy image handling */
>> -	if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_FORMAT)) {
>> -		struct legacy_img_hdr hdr;
>> -
>> -		load.bl_len = 1;
>> -		load.read = spl_nor_load_read;
>> -		spl_nor_load_read(&load, spl_nor_get_uboot_base(), sizeof(hdr), &hdr);
>> -		return spl_load_legacy_img(spl_image, bootdev, &load,
>> -					   spl_nor_get_uboot_base(),
>> -					   &hdr);
> 
> This loaded legacy image with potential lzma decompression and now
> compressed images are no longer supported ?
> 
>> -	}
>> -
>> -	return -EINVAL;
>> +	return spl_load(spl_image, bootdev, &load, 0, 0);
> 
> maybe better
> 
> +	return spl_load(spl_image, bootdev, &load, 0, spl_nor_get_uboot_base());
> 
> 
> and consider calling spl_load_legacy_img(spl_image, bootdev, &info, offset, header)
> from spl_load()?

Yeah, I noticed both of these issues when Tom sent the size difference.
I'm going to address these in v6. Ideally, I will incorporate the LZMA
code into spl_load.

--Sean

>>  }
>>  SPL_LOAD_IMAGE_METHOD("NOR", 0, BOOT_DEVICE_NOR, spl_nor_load_image);
>> -- 
>> 2.40.1
>> 

Re: [PATCH v5 07/11] spl: Convert NVMe to spl_load

[Sean Anderson]

On 8/3/23 04:32, Xavier Drudis Ferran wrote:
> El Mon, Jul 31, 2023 at 06:42:59PM -0400, Sean Anderson deia:
>> This converts the blk load method (used exclusively by NVMe) to use
>> spl_load. As a consequence, it also adds support for LOAD_FIT_FULL and
>> IMX images.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v5:
>> - New
>> 
>>  common/spl/spl_blk_fs.c | 68 ++++++++---------------------------------
>>  1 file changed, 12 insertions(+), 56 deletions(-)
>> 
>> diff --git a/common/spl/spl_blk_fs.c b/common/spl/spl_blk_fs.c
>> index d97adc4d39..eb5775a470 100644
>> --- a/common/spl/spl_blk_fs.c
>> +++ b/common/spl/spl_blk_fs.c
>> @@ -45,24 +45,28 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
>>  {
>>  	const char *filename = CONFIG_SPL_PAYLOAD;
>>  	struct disk_partition part_info = {};
>> -	struct legacy_img_hdr *header;
>>  	struct blk_desc *blk_desc;
>> -	loff_t actlen, filesize;
>> +	loff_t filesize;
>>  	struct blk_dev dev;
>>  	int ret;
>> +	struct spl_load_info load = {
>> +		.read = spl_fit_read,
>> +		.bl_len = 1,
>> +		.filename = filename,
>> +		.priv = &dev,
>> +	};
>>  
>>  	blk_desc = blk_get_devnum_by_uclass_id(uclass_id, devnum);
>>  	if (!blk_desc) {
>>  		printf("blk desc for %d %d not found\n", uclass_id, devnum);
>> -		goto out;
>> +		return ret;
> 
> What do you mean return ret ?
> ret is unused yet ?
> maybe return -1 or return -ENODEV or something ?
> 
> I think the error was already there before your patch, but maybe worth
> fixing ?

Yes.

--Sean

>>  	}
>>  
>>  	blk_show_device(uclass_id, devnum);
>> -	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>>  	ret = part_get_info(blk_desc, 1, &part_info);
>>  	if (ret) {
>>  		printf("spl: no partition table found. Err - %d\n", ret);
>> -		goto out;
>> +		return ret;
>>  	}
>>  
>>  	dev.ifname = blk_get_uclass_name(uclass_id);
>> @@ -72,63 +76,15 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
>>  	if (ret) {
>>  		printf("spl: unable to set blk_dev %s %s. Err - %d\n",
>>  		       dev.ifname, dev.dev_part_str, ret);
>> -		goto out;
>> -	}
>> -
>> -	ret = fs_read(filename, (ulong)header, 0,
>> -		      sizeof(struct legacy_img_hdr), &actlen);
>> -	if (ret) {
>> -		printf("spl: unable to read file %s. Err - %d\n", filename,
>> -		       ret);
>> -		goto out;
>> -	}
>> -
>> -	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>> -	    image_get_magic(header) == FDT_MAGIC) {
>> -		struct spl_load_info load;
>> -
>> -		debug("Found FIT\n");
>> -		load.read = spl_fit_read;
>> -		load.bl_len = 1;
>> -		load.filename = (void *)filename;
>> -		load.priv = &dev;
>> -
>> -		return spl_load_simple_fit(spl_image, &load, 0, header);
>> -	}
>> -
>> -	ret = spl_parse_image_header(spl_image, bootdev, header);
>> -	if (ret) {
>> -		printf("spl: unable to parse image header. Err - %d\n",
>> -		       ret);
>> -		goto out;
>> -	}
>> -
>> -	ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str, FS_TYPE_ANY);
>> -	if (ret) {
>> -		printf("spl: unable to set blk_dev %s %s. Err - %d\n",
>> -		       dev.ifname, dev.dev_part_str, ret);
>> -		goto out;
>> +		return ret;
>>  	}
>>  
>>  	ret = fs_size(filename, &filesize);
>>  	if (ret) {
>>  		printf("spl: unable to get file size: %s. Err - %d\n",
>>  		       filename, ret);
>> -		goto out;
>> +		return ret;
>>  	}
>>  
>> -	ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str, FS_TYPE_ANY);
>> -	if (ret) {
>> -		printf("spl: unable to set blk_dev %s %s. Err - %d\n",
>> -		       dev.ifname, dev.dev_part_str, ret);
>> -		goto out;
>> -	}
>> -
>> -	ret = fs_read(filename, (ulong)spl_image->load_addr, 0, filesize,
>> -		      &actlen);
>> -	if (ret)
>> -		printf("spl: unable to read file %s. Err - %d\n",
>> -		       filename, ret);
>> -out:
>> -	return ret;
>> +	return spl_load(spl_image, bootdev, &load, filesize, 0);
>>  }
>> -- 
>> 2.40.1
>> 

Re: [PATCH v5 05/11] spl: Convert mmc to spl_load

[Sean Anderson]

On 8/3/23 04:31, Xavier Drudis Ferran wrote:
> El Mon, Jul 31, 2023 at 06:42:57PM -0400, Sean Anderson deia:
>> This converts the mmc loader to spl_load. Legacy images are handled by
>> spl_load (via spl_parse_image_header), so mmc_load_legacy can be
>> omitted.
>>
> 
> Yes. I haven't used the legacy case, but by looking at the code, it
> seems to me that mmc_load_legacy left the image at some mapped memory
> at [ spl_image->load_addr,   spl_image->load_addr + size )
> and the new code does too, but when the image is not aligned, the
> memory that gets written to
> was at [ spl_image->load_addr,   spl_image->load_addr + size + spl_image->offset % mmc->read_bl_len )
> and it will
> be at [ spl_image->load_addr - spl_image->offset % mmc->read_bl_len,   spl_image->load_addr + size )
> after this patch.
> 
> Meaning both with or without this patch some memory outside the image
> gets written when the image is not aligned on media, but before it was
> some part of a block at the end and now is that part before the
> beginning.
> 
> Whether that's better or worse I don't know. It depends on whether
> it's a problem to write in non mapped memory, whether there's
> something worth preserving there, and whether some SOC has some sort
> of special behaving memory just there, like it happened with the issue
> Jerome Forissier found (note in this case it wasn't legacy, it was
> simple fit)

Fundamentally, we can't really deal with unaligned images without a
bounce-buffer. The method used by SPL_LOAD_FIT_IMAGE_BUFFER_SIZE will
continue working, since we call into the FIT routines to load the image.
I would like to defer bounce buffering for other images until someone
actually needs it.

Note that in the FIT case, you can also do `mkimage -EB`, at least if
you aren't using FIT_LOAD_FULL.

--Sean

> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fpatchwork.ozlabs.org%2fproject%2fuboot%2fpatch%2fc941d835a85255ff81a21c72069c3a9fc9a8a255.1656489154.git.jerome.forissier%40linaro.org%2f&umid=1ca400c8-7d50-4ae9-9abc-31dac6468719&auth=d807158c60b7d2502abde8a2fc01f40662980862-09f1f8fbc507564d04c74bc07523f5da694b0761
>                                                                   
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v5:
>> - Rework to load header in spl_load
>> 
>>  common/spl/spl_mmc.c | 91 ++++----------------------------------------
>>  1 file changed, 8 insertions(+), 83 deletions(-)
>> 
>> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
>> index a665091b00..c926a42c0f 100644
>> --- a/common/spl/spl_mmc.c
>> +++ b/common/spl/spl_mmc.c
>> @@ -17,48 +17,6 @@
>>  #include <mmc.h>
>>  #include <image.h>
>>  
>> -static int mmc_load_legacy(struct spl_image_info *spl_image,
>> -			   struct spl_boot_device *bootdev,
>> -			   struct mmc *mmc,
>> -			   ulong sector, struct legacy_img_hdr *header)
>> -{
>> -	u32 image_offset_sectors;
>> -	u32 image_size_sectors;
>> -	unsigned long count;
>> -	u32 image_offset;
>> -	int ret;
>> -
>> -	ret = spl_parse_image_header(spl_image, bootdev, header);
>> -	if (ret)
>> -		return ret;
>> -
>> -	/* convert offset to sectors - round down */
>> -	image_offset_sectors = spl_image->offset / mmc->read_bl_len;
>> -	/* calculate remaining offset */
>> -	image_offset = spl_image->offset % mmc->read_bl_len;
>> -
>> -	/* convert size to sectors - round up */
>> -	image_size_sectors = (spl_image->size + mmc->read_bl_len - 1) /
>> -			     mmc->read_bl_len;
>> -
>> -	/* Read the header too to avoid extra memcpy */
>> -	count = blk_dread(mmc_get_blk_desc(mmc),
>> -			  sector + image_offset_sectors,
>> -			  image_size_sectors,
>> -			  (void *)(ulong)spl_image->load_addr);
>> -	debug("read %x sectors to %lx\n", image_size_sectors,
>> -	      spl_image->load_addr);
>> -	if (count != image_size_sectors)
>> -		return -EIO;
>> -
>> -	if (image_offset)
>> -		memmove((void *)(ulong)spl_image->load_addr,
>> -			(void *)(ulong)spl_image->load_addr + image_offset,
>> -			spl_image->size);
>> -
>> -	return 0;
>> -}
>> -
>>  static ulong h_spl_load_read(struct spl_load_info *load, ulong sector,
>>  			     ulong count, void *buf)
>>  {
>> @@ -82,47 +40,14 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
>>  			      struct spl_boot_device *bootdev,
>>  			      struct mmc *mmc, unsigned long sector)
>>  {
>> -	unsigned long count;
>> -	struct legacy_img_hdr *header;
>> -	struct blk_desc *bd = mmc_get_blk_desc(mmc);
>> -	int ret = 0;
>> -
>> -	header = spl_get_load_buffer(-sizeof(*header), bd->blksz);
>> -
>> -	/* read image header to find the image size & load address */
>> -	count = blk_dread(bd, sector, 1, header);
>> -	debug("hdr read sector %lx, count=%lu\n", sector, count);
>> -	if (count == 0) {
>> -		ret = -EIO;
>> -		goto end;
>> -	}
>> -
>> -	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>> -	    image_get_magic(header) == FDT_MAGIC) {
>> -		struct spl_load_info load;
>> -
>> -		debug("Found FIT\n");
>> -		load.dev = mmc;
>> -		load.priv = NULL;
>> -		load.filename = NULL;
>> -		load.bl_len = mmc->read_bl_len;
>> -		load.read = h_spl_load_read;
>> -		ret = spl_load_simple_fit(spl_image, &load, sector, header);
>> -	} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
>> -		struct spl_load_info load;
>> -
>> -		load.dev = mmc;
>> -		load.priv = NULL;
>> -		load.filename = NULL;
>> -		load.bl_len = mmc->read_bl_len;
>> -		load.read = h_spl_load_read;
>> -
>> -		ret = spl_load_imx_container(spl_image, &load, sector);
>> -	} else {
>> -		ret = mmc_load_legacy(spl_image, bootdev, mmc, sector, header);
>> -	}
>> -
>> -end:
>> +	int ret;
>> +	struct spl_load_info load = {
>> +		.dev = mmc,
>> +		.bl_len = mmc->read_bl_len,
>> +		.read = h_spl_load_read,
>> +	};
>> +
>> +	ret = spl_load(spl_image, bootdev, &load, 0, sector);
>>  	if (ret) {
>>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>>  		puts("mmc_load_image_raw_sector: mmc block read error\n");
>> -- 
>> 2.40.1
>> 

Re: [PATCH v5 04/11] spl: Convert fat to spl_load

[Sean Anderson]

On 8/3/23 04:30, Xavier Drudis Ferran wrote:
> El Mon, Jul 31, 2023 at 06:42:56PM -0400, Sean Anderson deia:
>> This converts the fat loader to use spl_load. Some platforms are very
>> tight on space, so we take care to only include the code we really need.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v5:
>> - Rework to load header in spl_load
>> 
>> Changes in v3:
>> - Fix failing on success
>> 
>>  common/spl/spl_fat.c | 55 +++++++++++++++-----------------------------
>>  1 file changed, 19 insertions(+), 36 deletions(-)
>> 
>> diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
>> index f8a5b80a3b..6530bcd5a7 100644
>> --- a/common/spl/spl_fat.c
>> +++ b/common/spl/spl_fat.c
>> @@ -60,57 +60,40 @@ int spl_load_image_fat(struct spl_image_info *spl_image,
>>  		       const char *filename)
>>  {
>>  	int err;
>> -	struct legacy_img_hdr *header;
>> +	loff_t size;
>> +	struct spl_load_info load;
>> +
>> +	/* This generates smaller code than using a compound literal */
>> +	load.read = spl_fit_read;
>> +	load.bl_len = 1;
>> +	load.filename = filename;
>>  
>>  	err = spl_register_fat_device(block_dev, partition);
>>  	if (err)
>>  		goto end;
>>  
>> -	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>> -
>> -	err = file_fat_read(filename, header, sizeof(struct legacy_img_hdr));
>> -	if (err <= 0)
>> -		goto end;
>> -
>> -	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
>> -	    image_get_magic(header) == FDT_MAGIC) {
>> -		err = file_fat_read(filename, (void *)CONFIG_SYS_LOAD_ADDR, 0);
>> -		if (err <= 0)
>> -			goto end;
>> -		err = spl_parse_image_header(spl_image, bootdev,
>> -				(struct legacy_img_hdr *)CONFIG_SYS_LOAD_ADDR);
> 
> So this used to load to CONFIG_SYS_LOAD_ADDR and will now load to
> CONFIG_TEXT_BASE, or whatever the applicable spl_get_load_buffer()
> uses. Is this OK ? or do we need a new parameter for the buffer or
> something ?

It's worked OK in my testing. Typically, CONFIG_SYS_LOAD_ADDR is just
CONFIG_TEXT_BASE plus some offset. If there's a discontinuity such as:

TEXT_BASE - 0x000000000
RAM_TOP   - 0x07fffffff
LOAD_ADDR - 0x800000000
RAM2_TOP  - 0xfffffffff

then this only matters if we load something larger than 2G. Obviously
not every RAM layout looks like this, but Most of the Time (TM) the
memory set aside for U-Boot to run in can hold the FIT that holds
U-Boot. If this ends up being a problem, we can add a config to load
things in a malloc'd buffer. This change could be moved to a prep commit
to aid bisecting.

--Sean

>> -		if (err == -EAGAIN)
>> -			return err;
>> -		if (err == 0)
>> -			err = 1;
>> -	} else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>> -	    image_get_magic(header) == FDT_MAGIC) {
>> -		struct spl_load_info load;
>> -
>> -		debug("Found FIT\n");
>> -		load.read = spl_fit_read;
>> -		load.bl_len = 1;
>> -		load.filename = (void *)filename;
>> -		load.priv = NULL;
>> -
>> -		return spl_load_simple_fit(spl_image, &load, 0, header);
>> -	} else {
>> -		err = spl_parse_image_header(spl_image, bootdev, header);
>> +	/*
>> +	 * Avoid pulling in this function for other image types since we are
>> +	 * very short on space on some boards.
>> +	 */
>> +	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL)) {
>> +		err = fat_size(filename, &size);
>>  		if (err)
>>  			goto end;
>> -
>> -		err = file_fat_read(filename,
>> -				    (u8 *)(uintptr_t)spl_image->load_addr, 0);
>> +	} else {
>> +		size = 0;
>>  	}
>>  
>> +	err = spl_load(spl_image, bootdev, &load, 0, size);
>> +
>>  end:
>>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>> -	if (err <= 0)
>> +	if (err < 0)
>>  		printf("%s: error reading image %s, err - %d\n",
>>  		       __func__, filename, err);
>>  #endif
>>  
>> -	return (err <= 0);
>> +	return err;
>>  }
>>  
>>  #if CONFIG_IS_ENABLED(OS_BOOT)
>> -- 
>> 2.40.1
>> 

Re: [PATCH v5 10/11] spl: Convert spi to spl_load

[Sean Anderson]

On 8/3/23 04:45, Xavier Drudis Ferran wrote:
> El Mon, Jul 31, 2023 at 06:43:02PM -0400, Sean Anderson deia:
>> This converts the spi load method to use spl_load. As a consequence, it
>> also adds support for LOAD_FIT_FULL.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v5:
>> - Rework to load header in spl_load
>> 
>>  common/spl/spl_spi.c | 72 +++++---------------------------------------
>>  1 file changed, 8 insertions(+), 64 deletions(-)
>> 
>> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
>> index 2aff025f76..14391a1c96 100644
>> --- a/common/spl/spl_spi.c
>> +++ b/common/spl/spl_spi.c
>> @@ -89,12 +89,14 @@ u32 __weak spl_spi_boot_cs(void)
>>  static int spl_spi_load_image(struct spl_image_info *spl_image,
>>  			      struct spl_boot_device *bootdev)
>>  {
>> -	int err = 0;
>>  	unsigned int payload_offs;
>>  	struct spi_flash *flash;
>> -	struct legacy_img_hdr *header;
>>  	unsigned int sf_bus = spl_spi_boot_bus();
>>  	unsigned int sf_cs = spl_spi_boot_cs();
>> +	struct spl_load_info load = {
>> +		.bl_len = 1,
>> +		.read = spl_spi_fit_read,
>> +	};
>>  
>>  	/*
>>  	 * Load U-Boot image from SPI flash into RAM
>> @@ -109,77 +111,19 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
>>  		return -ENODEV;
>>  	}
>>  
>> +	load.dev = flash;
>>  	payload_offs = spl_spi_get_uboot_offs(flash);
>>  
>> -	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>> -
>>  	if (CONFIG_IS_ENABLED(OF_REAL)) {
>>  		payload_offs = ofnode_conf_read_int("u-boot,spl-payload-offset",
>>  						    payload_offs);
>>  	}
>>  
>>  #if CONFIG_IS_ENABLED(OS_BOOT)
>> -	if (spl_start_uboot() || spi_load_image_os(spl_image, bootdev, flash, header))
>> +	if (!spl_start_uboot() && !spi_load_image_os(spl_image, bootdev, flash, header))
>> +		return 0;
>>  #endif
> 
> But with this return 0 we're skipping the soft reset at the end, aren't we ?
> This is the same the old code did. Just not sure whether it was right or untested.
> If some flash needs reset before running linux, then it might need it with U-Boot or in Falcon,
> as long as SPL has probed the flash, mightn't it ?
> If it needs fixing, then possibly better in a different commit, though ?
> I mean yours is fine, you left things as they were.

I am trying to change things only where they are necessary to combine
load methods. So while this might be a good change to make, I don't want
to do it in this series. I also don't have any SPI flash boards that I
care about so...

>> -	{
>> -		/* Load u-boot, mkimage header is 64 bytes. */
>> -		err = spi_flash_read(flash, payload_offs, sizeof(*header),
>> -				     (void *)header);
>> -		if (err) {
>> -			debug("%s: Failed to read from SPI flash (err=%d)\n",
>> -			      __func__, err);
>> -			return err;
>> -		}
>> -
>> -		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
>> -		    image_get_magic(header) == FDT_MAGIC) {
>> -			err = spi_flash_read(flash, payload_offs,
>> -					     roundup(fdt_totalsize(header), 4),
>> -					     (void *)CONFIG_SYS_LOAD_ADDR);
>> -			if (err)
>> -				return err;
>> -			err = spl_parse_image_header(spl_image, bootdev,
>> -					(struct legacy_img_hdr *)CONFIG_SYS_LOAD_ADDR);
>                                                                                                             
> So this used to load to CONFIG_SYS_LOAD_ADDR and will now load to
> CONFIG_TEXT_BASE, or whatever the applicable spl_get_load_buffer()
> uses. Is this OK ? or do we need a new parameter for the buffer or
> something ?

(commented on on the FAT patch)

>> -		} else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>> -			   image_get_magic(header) == FDT_MAGIC) {
>> -			struct spl_load_info load;
>> -
>> -			debug("Found FIT\n");
>> -			load.dev = flash;
>> -			load.priv = NULL;
>> -			load.filename = NULL;
>> -			load.bl_len = 1;
>> -			load.read = spl_spi_fit_read;
>> -			err = spl_load_simple_fit(spl_image, &load,
>> -						  payload_offs,
>> -						  header);
>> -		} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
>> -			struct spl_load_info load;
>> -
>> -			load.dev = flash;
>> -			load.priv = NULL;
>> -			load.filename = NULL;
>> -			load.bl_len = 1;
>> -			load.read = spl_spi_fit_read;
>> -
>> -			err = spl_load_imx_container(spl_image, &load,
>> -						     payload_offs);
>> -		} else {
>> -			err = spl_parse_image_header(spl_image, bootdev, header);
>> -			if (err)
>> -				return err;
>> -			err = spi_flash_read(flash, payload_offs + spl_image->offset,
>> -					     spl_image->size,
>> -					     (void *)spl_image->load_addr);
>> -		}
> 
> 
>> -		if (IS_ENABLED(CONFIG_SPI_FLASH_SOFT_RESET)) {
>> -			err = spi_nor_remove(flash);
>> -			if (err)
>> -				return err;
>> -		}
> 
> This soft reset is removed ?
> Shouldn't we keep this if after the call to spl_load and before returning its result ?

Yes. I'll re-add this.

--Sean

>> -	}
>> -
>> -	return err;
>> +	return spl_load(spl_image, bootdev, &load, 0, payload_offs);
>>  }
>>  /* Use priorty 1 so that boards can override this */
>>  SPL_LOAD_IMAGE_METHOD("SPI", 1, BOOT_DEVICE_SPI, spl_spi_load_image);
>> -- 
>> 2.40.1
>> 

Re: [PATCH v5 05/11] spl: Convert mmc to spl_load

[Jonas Karlman]

On 2023-08-03 18:11, Sean Anderson wrote:
> On 8/3/23 04:31, Xavier Drudis Ferran wrote:
>> El Mon, Jul 31, 2023 at 06:42:57PM -0400, Sean Anderson deia:
>>> This converts the mmc loader to spl_load. Legacy images are handled by
>>> spl_load (via spl_parse_image_header), so mmc_load_legacy can be
>>> omitted.
>>>
>>
>> Yes. I haven't used the legacy case, but by looking at the code, it
>> seems to me that mmc_load_legacy left the image at some mapped memory
>> at [ spl_image->load_addr,   spl_image->load_addr + size )
>> and the new code does too, but when the image is not aligned, the
>> memory that gets written to
>> was at [ spl_image->load_addr,   spl_image->load_addr + size + spl_image->offset % mmc->read_bl_len )
>> and it will
>> be at [ spl_image->load_addr - spl_image->offset % mmc->read_bl_len,   spl_image->load_addr + size )
>> after this patch.
>>
>> Meaning both with or without this patch some memory outside the image
>> gets written when the image is not aligned on media, but before it was
>> some part of a block at the end and now is that part before the
>> beginning.
>>
>> Whether that's better or worse I don't know. It depends on whether
>> it's a problem to write in non mapped memory, whether there's
>> something worth preserving there, and whether some SOC has some sort
>> of special behaving memory just there, like it happened with the issue
>> Jerome Forissier found (note in this case it wasn't legacy, it was
>> simple fit)
> 
> Fundamentally, we can't really deal with unaligned images without a
> bounce-buffer. The method used by SPL_LOAD_FIT_IMAGE_BUFFER_SIZE will
> continue working, since we call into the FIT routines to load the image.
> I would like to defer bounce buffering for other images until someone
> actually needs it.
> 
> Note that in the FIT case, you can also do `mkimage -EB`, at least if
> you aren't using FIT_LOAD_FULL.

With the following two commits introduced in v2023.04-rc1, the alignment
issue for Rockchip has been fixed and all external data is now accessed
block aligned.

9b2fd2d22852 ("binman: Add support for align argument to mkimage tool")
5ad03fc77dfa ("rockchip: Align FIT image data to SD/MMC block length")

Regards,
Jonas

> 
> --Sean
> 
>> https://patchwork.ozlabs.org/project/uboot/patch/c941d835a85255ff81a21c72069c3a9fc9a8a255.1656489154.git.jerome.forissier@linaro.org/
>>                                                                   
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> ---
>>>
>>> Changes in v5:
>>> - Rework to load header in spl_load
>>>
>>>  common/spl/spl_mmc.c | 91 ++++----------------------------------------
>>>  1 file changed, 8 insertions(+), 83 deletions(-)
>>>

[...]

Re: [PATCH v5 05/11] spl: Convert mmc to spl_load

[Xavier Drudis Ferran]

El Sun, Sep 03, 2023 at 08:17:26AM +0000, Jonas Karlman deia:

> > Fundamentally, we can't really deal with unaligned images without a
> > bounce-buffer. The method used by SPL_LOAD_FIT_IMAGE_BUFFER_SIZE will
> > continue working, since we call into the FIT routines to load the image.

Yes

> > I would like to defer bounce buffering for other images until someone
> > actually needs it.
> >

Fine.

> > Note that in the FIT case, you can also do `mkimage -EB`, at least if
> > you aren't using FIT_LOAD_FULL.
> 
> With the following two commits introduced in v2023.04-rc1, the alignment
> issue for Rockchip has been fixed and all external data is now accessed
> block aligned.
> 
> 9b2fd2d22852 ("binman: Add support for align argument to mkimage tool")
> 5ad03fc77dfa ("rockchip: Align FIT image data to SD/MMC block length")
> 
> Regards,
> Jonas
>

Well, yes, thanks.

I was carrying Jerome's patch still thinking it was needed for me, but
I just tried without and it works too, in mmc. In spi I didn't try but
it should be even easier (bl_len=1).

For me it's still odd to write outside intended memory. Would a warning
in case legacy image loading writes before load_addr be acceptable ?
Just in case someone was using the memory there for something else.

Re: [PATCH v5 05/11] spl: Convert mmc to spl_load

[Sean Anderson]

On 9/4/23 08:59, Xavier Drudis Ferran wrote:
> El Sun, Sep 03, 2023 at 08:17:26AM +0000, Jonas Karlman deia:
> 
>> > Fundamentally, we can't really deal with unaligned images without a
>> > bounce-buffer. The method used by SPL_LOAD_FIT_IMAGE_BUFFER_SIZE will
>> > continue working, since we call into the FIT routines to load the image.
> 
> Yes
> 
>> > I would like to defer bounce buffering for other images until someone
>> > actually needs it.
>> >
> 
> Fine.
> 
>> > Note that in the FIT case, you can also do `mkimage -EB`, at least if
>> > you aren't using FIT_LOAD_FULL.
>> 
>> With the following two commits introduced in v2023.04-rc1, the alignment
>> issue for Rockchip has been fixed and all external data is now accessed
>> block aligned.
>> 
>> 9b2fd2d22852 ("binman: Add support for align argument to mkimage tool")
>> 5ad03fc77dfa ("rockchip: Align FIT image data to SD/MMC block length")
>> 
>> Regards,
>> Jonas
>>
> 
> Well, yes, thanks.
> 
> I was carrying Jerome's patch still thinking it was needed for me, but
> I just tried without and it works too, in mmc. In spi I didn't try but
> it should be even easier (bl_len=1).
> 
> For me it's still odd to write outside intended memory. Would a warning
> in case legacy image loading writes before load_addr be acceptable ?
> Just in case someone was using the memory there for something else.

We could add something, but it would have to be behind SHOW_ERRORS. This
series is already having a very tough time reducing bloat without adding
any (other) new features.

--Sean

Re: [PATCH v5 05/11] spl: Convert mmc to spl_load

[Xavier Drudis Ferran]

El Tue, Sep 05, 2023 at 11:02:44AM -0400, Sean Anderson deia:
> On 9/4/23 08:59, Xavier Drudis Ferran wrote:
> > El Sun, Sep 03, 2023 at 08:17:26AM +0000, Jonas Karlman deia:
> > 
> >> > Fundamentally, we can't really deal with unaligned images without a
> >> > bounce-buffer. The method used by SPL_LOAD_FIT_IMAGE_BUFFER_SIZE will
> >> > continue working, since we call into the FIT routines to load the image.
> > 
> > Yes
> > 
> >> > I would like to defer bounce buffering for other images until someone
> >> > actually needs it.
> >> >
> > 
> > Fine.
> > 
> >> > Note that in the FIT case, you can also do `mkimage -EB`, at least if
> >> > you aren't using FIT_LOAD_FULL.
> >> 
> >> With the following two commits introduced in v2023.04-rc1, the alignment
> >> issue for Rockchip has been fixed and all external data is now accessed
> >> block aligned.
> >> 
> >> 9b2fd2d22852 ("binman: Add support for align argument to mkimage tool")
> >> 5ad03fc77dfa ("rockchip: Align FIT image data to SD/MMC block length")
> >> 
> >> Regards,
> >> Jonas
> >>
> > 
> > Well, yes, thanks.
> > 
> > I was carrying Jerome's patch still thinking it was needed for me, but
> > I just tried without and it works too, in mmc. In spi I didn't try but
> > it should be even easier (bl_len=1).
> > 
> > For me it's still odd to write outside intended memory. Would a warning
> > in case legacy image loading writes before load_addr be acceptable ?
> > Just in case someone was using the memory there for something else.
> 
> We could add something, but it would have to be behind SHOW_ERRORS. This
> series is already having a very tough time reducing bloat without adding
> any (other) new features.
>

When I looked at it I thought the check and warning was only needed at
the end of spl_load, for legacy images. For fit it was already
aligned by calculating offset as a integer multiple of sectors.
I might misremember now.

In cases where the buffer is reserved it could just include the extra
bytes at the start before the load address, as I tried to suggest (v5
02/11), and then no warning is needed. Which is silly because the
spl_get_load_buffer doesn't do much at all. But someday it might do
more...

Your changes moves the extra bytes from the end to the start, and I
find it more unexpected than just having extra bytes at the end for an
image that might not have such a fixed length. I'd expect people to
leave room at the end when planning memory, thinking a future image
might be bigger, but not thinking to leave room at the start. But I'm
not all that sure...

If not a check and warning at least note something in documentation or
comments somewhere...

And I think you are really adding new features, in that even if the
formats and methods stay the same, there'll be more combinations
available after your change. So I'd be tolerant to a small code size
increase in exchange for the new flexibility. But I can understand
some cases can't afford the luxury, maybe.

Whatever you decide, looking forward to test a little your v6 if I can.

Re: [PATCH v5 09/11] spl: Convert semihosting to spl_load

[Heinrich Schuchardt]

On 03.08.23 10:36, Xavier Drudis Ferran wrote:
> El Mon, Jul 31, 2023 at 06:43:01PM -0400, Sean Anderson deia:
>> This converts the semihosting load method to use spl_load. As a result, it
>> also adds support for LOAD_FIT_FULL and IMX images.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>> Changes in v5:
>> - Rework to load header in spl_load
>>
>> Changes in v2:
>> - New
>>
>>   common/spl/spl_semihosting.c | 43 ++++++++++++------------------------
>>   1 file changed, 14 insertions(+), 29 deletions(-)
>>
>> diff --git a/common/spl/spl_semihosting.c b/common/spl/spl_semihosting.c
>> index 5b5e842a11..e7cb9f4ce6 100644
>> --- a/common/spl/spl_semihosting.c
>> +++ b/common/spl/spl_semihosting.c
>> @@ -9,16 +9,16 @@
>>   #include <semihosting.h>
>>   #include <spl.h>
>>
>> -static int smh_read_full(long fd, void *memp, size_t len)
>> +static ulong spl_smh_fit_read(struct spl_load_info *load, ulong sector,
>> +			      ulong count, void *buf)
>>   {
>> -	long read;
>> +	int ret, fd = *(int *)load->priv;
>>
>
> should ret be long to hold smh_read() return value ?
>
>> -	read = smh_read(fd, memp, len);
>> -	if (read < 0)
>> -		return read;
>> -	if (read != len)
>> -		return -EIO;
>> -	return 0;
>> +	if (smh_seek(fd, sector))
>> +		return 0;
>> +
>
>
> I never used smh, but why would this not be :
>
> +       ret = smh_seek(fd, sector);
> +       if (ret)
> +               return ret;
>
> (why does smh_seek(), smh_write(), smh_open(), smh_close() return
> long, by the way? the implementations return either 0 or smh_errno(),
> so int would do ?)

The return type used should match the implementation on the host side.
The ARM documentation explicitly refers to register R0 which will match
the length of long (or unsigned long).

For smh_seek, smh_read(), smh_write()  we should investigate if the type
should not better be unsigned long to handle a number of bytes exceeding
2^31 on a 32bit emulation or system. But that is beyond the scope of
this patch series.

Best regards

Heinrich

Re: [PATCH v5 00/11] spl: Use common function for loading/parsing images

[Sean Anderson]

On 8/3/23 00:41, Heinrich Schuchardt wrote:
> On 8/3/23 03:31, Tom Rini wrote:
>> On Wed, Aug 02, 2023 at 09:13:21PM -0400, Sean Anderson wrote:
>>> On 8/2/23 16:11, Tom Rini wrote:
>>>> On Mon, Jul 31, 2023 at 06:42:52PM -0400, Sean Anderson wrote:
>>>>
>>>>> This series adds support for loading all image types (Legacy, FIT (with
>>>>> and without LOAD_FIT_FULL), and i.MX) to the MMC, SPI, NOR, NET, FAT,
>>>>> and EXT load methods. It does this by introducing a helper function
>>>>> which handles the minutiae of invoking the proper parsing function, and
>>>>> reading the rest of the image.
>>>>>
>>>>> Hopefully, this will make it easier for load methods to support all
>>>>> image types that U-Boot supports, without having undocumented
>>>>> unsupported image types. I applied this to several loaders which were
>>>>> invoking spl_load_simple_fit and/or spl_parse_image_header, but I did
>>>>> not use it with others (e.g. DFU/RAM) which had complications in the
>>>>> mix.
>>>>>
>>>>> In order to meet size requirements for some boards, the first two
>>>>> patches of this series are general size-reduction changes. The ffs/fls
>>>>> change in particular should reduce code size across the board. The
>>>>> malloc change also has the potential to reduce code size. I've left it
>>>>> disabled by default, but maybe we can reverse that in the future.
>>>>>
>>>>> Here's some bloat-o-meter for j7200_evm_a72_defconfig with ext4 support
>>>>> enabed:
>>>>>
>>>> add/remove: 2/1 grow/shrink: 1/5 up/down: 444/-584 (-140)
>>>>> Function                                     old     new   delta
>>>>> spl_load                                       -     216    +216
>>>> spl_simple_read                                -     184    +184
>>>>> spl_fit_read                                  60     104     +44
>>>>> file_fat_read                                 40       -     -40
>>>>> spl_nor_load_image                           120      76     -44
>>>>> spl_load_image_ext                           364     296     -68
>>>> spl_load_image_fat                           320     200    -120
>>>>> spl_spi_load_image                           304     176    -128
>>>>> spl_mmc_load                                 716     532    -184
>>>>> Total: Before=233618, After=233478, chg -0.06%
>>>>>
>>>>> For most boards with a few load methods, this series should break even.
>>>>> However, in the worst case this series will add around 100 bytes.
>>>>>
>>>>> I have only tested a few loaders. Please try booting your favorite board
>>>>> with NOR/SPI flash or SPI falcon mode.
>>>>
>>>>> On sama5d27_wlsom1_ek_mmc_defconfig, 100 bytes is too much, so this
>>>>> series depends on [1] to fit everything in. CI run at [2].
>>>>>
>>>>> [1] https://lore.kernel.org/u-boot/20230731223327.109865-1-sean.anderson@seco.com/
>>>>> [2] https://source.denx.de/u-boot/custodians/u-boot-clk/-/pipelines/17116
>>>>
>>>> I've boot-tested this on my lab and a handful of SD+FAT or SD+raw boot
>>>> devices, and it's all worked.  I also did my usual world build
>>>> before/after to check out the size changes and well, I don't know.  The
>>>> size wins come on platforms where there's both FAT+EXT support.  And
>>>> then on mips with say mt7620_rfb I wonder if we're loosing
>>>> functionality.
>>>
>>> Yes we are. I'll address this in v6.
>>>
>>>> But most platforms grow a bit, especially if they're just
>>>> a single load type (which is the most common case). Maybe this problem
>>>> needs to be approached another way? I've put the whole log over at
>>>> https://gist.github.com/trini/e6772b2134e0eb44393364ea98729e06 and maybe
>>>> something will jump out to you.
>>>
>>> The fls/ffs patch [1] reduces the growth by around 80 bytes on ARM
>>> platforms. I sent it separately for ease of review, but it really should
>>> be applied before this series, since a good portion of the growth is due
>>> to that one function call. It seems like MIPS also has this instruction
>>> (and Linux uses it), so I can try putting together a patch for it as
>>> well.
>>>
>>> In the future, I would like to convert bl_len to bl_shift or something,
>>> which would remove the need for fls (and going from bl_shift to bl_len
>>> is just a shift). spl_load_info is used in a lot of places, so I wanted
>>> to do that in a follow-up. On arches with efficient fls implementations
>>> the difference is only around 3 instructions or so.
>>>
>>> But fundamentally some of the problem is that the logic is being moved
>>> into multiple translation units, so the compiler can't see enough to
>>> make optimizations. For example, all filesystems use a bl_len of 1, so
>>> all the multiplications/roundings/etc. can be eliminated if the compiler
>>> can see that. For example, on arm64 copying spl_load into spl_fat.c (as
>>> fat_load) and making it static saves us 100 bytes:
>>>
>>> add/remove: 3/1 grow/shrink: 0/1 up/down: 324/-92 (232)
>>> Function                                     old     new   delta
>>> spl_load                                       -     160    +160
>>> spl_simple_read                                -     104    +104
>>> spl_fit_read                                   -      60     +60
>>> file_fat_read                                 40       -     -40
>>> spl_load_image_fat                           252     200     -52
>>> Total: Before=66833, After=67065, chg +0.35%
>>>
>>> add/remove: 2/1 grow/shrink: 1/0 up/down: 172/-40 (132)
>>> Function                                     old     new   delta
>>> spl_simple_read                                -     104    +104
>>> spl_fit_read                                   -      60     +60
>>> spl_load_image_fat                           252     260      +8
>>> file_fat_read                                 40       -     -40
>>> Total: Before=66833, After=66965, chg +0.20%
>>>
>>> and even then, by inspection spl_simple_read isn't really taking
>>> advantage of bl_len=1. Surprisingly, after enabling LTO this board is
>>> +400 bytes (compared to without this series). It's just hard to beat
>>> handwritten routines with a generic solution.
>>>
>>> I would really like to consolidate this stuff, since every load method
>>> ends up only supporting a few image types and having almost identical
>>> implementations for what they do support.
>>>
>>> --Sean
>>>
>>> [1] https://lore.kernel.org/u-boot/20230731212734.7256-1-sean.anderson@seco.com/
>>
>> Thanks for digging more, lets keep going in this direction then.
>>
> 
> Do we need to pass sectors to the info->load() functions?
> 
> Only nand and mmc use bl_len != 1. Can we move the conversion to sectors
> and the alignment checks to functions called by info->load()?
> 
> I would assume that this can avoid part of the code size increase.

Unfortunately, while the conversion to sectors and alignment can be put into ->load,
MMC (and ROMAPI) can only read in block-sized chunks. This means that the buffer must
be a multiple of the block size (otherwise we will go over the end), and there must be
a block's worth of space in front of the buffer as well (which we use when we have a
non-block-aligned offset).

These situations generally only matter when reading into a malloc'd buffer, since there
is data to overwrite on either end of the buffer. With things like SYS_TEXT_BASE, there
is nothing to overwrite on either end. From examining the various loaders, malloc'd buffers
are only used when loading the image header (which may include the full image for FITs).
In that situation, under the beginning of the buffer is not an issue, since the load method
should have supplied an offset which is a multiple of the block size. Thus, we only need
to solve the problem of writing past the end of the buffer.

 From my perspective, there are three approaches we could take:

- Keep bl_len so that callers of ->read can allocate a sufficiently-large buffer.
- Use a bounce-buffer. This uses around double the memory and will also increase code size.
- Complete most of the read as normal, but read the final block into a separate buffer which
   is then copied into the passed-in buffer. This requires two reads, which is likely slower
   than one. It will also increase code size.

I think the first approach could be improved by optionally removing bl_len when there are
no load methods which need it. This will let the compiler optimize out any alignment if it
is unnecessary, which it cannot currently do (unless LTO is enabled). I am going to try using
this approach for v6.

--Sean

Re: [PATCH v5 00/11] spl: Use common function for loading/parsing images

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 9481 bytes --]

On Wed, Oct 18, 2023 at 01:33:20PM -0400, Sean Anderson wrote:
> On 8/3/23 00:41, Heinrich Schuchardt wrote:
> > On 8/3/23 03:31, Tom Rini wrote:
> > > On Wed, Aug 02, 2023 at 09:13:21PM -0400, Sean Anderson wrote:
> > > > On 8/2/23 16:11, Tom Rini wrote:
> > > > > On Mon, Jul 31, 2023 at 06:42:52PM -0400, Sean Anderson wrote:
> > > > > 
> > > > > > This series adds support for loading all image types (Legacy, FIT (with
> > > > > > and without LOAD_FIT_FULL), and i.MX) to the MMC, SPI, NOR, NET, FAT,
> > > > > > and EXT load methods. It does this by introducing a helper function
> > > > > > which handles the minutiae of invoking the proper parsing function, and
> > > > > > reading the rest of the image.
> > > > > > 
> > > > > > Hopefully, this will make it easier for load methods to support all
> > > > > > image types that U-Boot supports, without having undocumented
> > > > > > unsupported image types. I applied this to several loaders which were
> > > > > > invoking spl_load_simple_fit and/or spl_parse_image_header, but I did
> > > > > > not use it with others (e.g. DFU/RAM) which had complications in the
> > > > > > mix.
> > > > > > 
> > > > > > In order to meet size requirements for some boards, the first two
> > > > > > patches of this series are general size-reduction changes. The ffs/fls
> > > > > > change in particular should reduce code size across the board. The
> > > > > > malloc change also has the potential to reduce code size. I've left it
> > > > > > disabled by default, but maybe we can reverse that in the future.
> > > > > > 
> > > > > > Here's some bloat-o-meter for j7200_evm_a72_defconfig with ext4 support
> > > > > > enabed:
> > > > > > 
> > > > > add/remove: 2/1 grow/shrink: 1/5 up/down: 444/-584 (-140)
> > > > > > Function                                     old     new   delta
> > > > > > spl_load                                       -     216    +216
> > > > > spl_simple_read                                -     184    +184
> > > > > > spl_fit_read                                  60     104     +44
> > > > > > file_fat_read                                 40       -     -40
> > > > > > spl_nor_load_image                           120      76     -44
> > > > > > spl_load_image_ext                           364     296     -68
> > > > > spl_load_image_fat                           320     200    -120
> > > > > > spl_spi_load_image                           304     176    -128
> > > > > > spl_mmc_load                                 716     532    -184
> > > > > > Total: Before=233618, After=233478, chg -0.06%
> > > > > > 
> > > > > > For most boards with a few load methods, this series should break even.
> > > > > > However, in the worst case this series will add around 100 bytes.
> > > > > > 
> > > > > > I have only tested a few loaders. Please try booting your favorite board
> > > > > > with NOR/SPI flash or SPI falcon mode.
> > > > > 
> > > > > > On sama5d27_wlsom1_ek_mmc_defconfig, 100 bytes is too much, so this
> > > > > > series depends on [1] to fit everything in. CI run at [2].
> > > > > > 
> > > > > > [1] https://lore.kernel.org/u-boot/20230731223327.109865-1-sean.anderson@seco.com/
> > > > > > [2] https://source.denx.de/u-boot/custodians/u-boot-clk/-/pipelines/17116
> > > > > 
> > > > > I've boot-tested this on my lab and a handful of SD+FAT or SD+raw boot
> > > > > devices, and it's all worked.  I also did my usual world build
> > > > > before/after to check out the size changes and well, I don't know.  The
> > > > > size wins come on platforms where there's both FAT+EXT support.  And
> > > > > then on mips with say mt7620_rfb I wonder if we're loosing
> > > > > functionality.
> > > > 
> > > > Yes we are. I'll address this in v6.
> > > > 
> > > > > But most platforms grow a bit, especially if they're just
> > > > > a single load type (which is the most common case). Maybe this problem
> > > > > needs to be approached another way? I've put the whole log over at
> > > > > https://gist.github.com/trini/e6772b2134e0eb44393364ea98729e06 and maybe
> > > > > something will jump out to you.
> > > > 
> > > > The fls/ffs patch [1] reduces the growth by around 80 bytes on ARM
> > > > platforms. I sent it separately for ease of review, but it really should
> > > > be applied before this series, since a good portion of the growth is due
> > > > to that one function call. It seems like MIPS also has this instruction
> > > > (and Linux uses it), so I can try putting together a patch for it as
> > > > well.
> > > > 
> > > > In the future, I would like to convert bl_len to bl_shift or something,
> > > > which would remove the need for fls (and going from bl_shift to bl_len
> > > > is just a shift). spl_load_info is used in a lot of places, so I wanted
> > > > to do that in a follow-up. On arches with efficient fls implementations
> > > > the difference is only around 3 instructions or so.
> > > > 
> > > > But fundamentally some of the problem is that the logic is being moved
> > > > into multiple translation units, so the compiler can't see enough to
> > > > make optimizations. For example, all filesystems use a bl_len of 1, so
> > > > all the multiplications/roundings/etc. can be eliminated if the compiler
> > > > can see that. For example, on arm64 copying spl_load into spl_fat.c (as
> > > > fat_load) and making it static saves us 100 bytes:
> > > > 
> > > > add/remove: 3/1 grow/shrink: 0/1 up/down: 324/-92 (232)
> > > > Function                                     old     new   delta
> > > > spl_load                                       -     160    +160
> > > > spl_simple_read                                -     104    +104
> > > > spl_fit_read                                   -      60     +60
> > > > file_fat_read                                 40       -     -40
> > > > spl_load_image_fat                           252     200     -52
> > > > Total: Before=66833, After=67065, chg +0.35%
> > > > 
> > > > add/remove: 2/1 grow/shrink: 1/0 up/down: 172/-40 (132)
> > > > Function                                     old     new   delta
> > > > spl_simple_read                                -     104    +104
> > > > spl_fit_read                                   -      60     +60
> > > > spl_load_image_fat                           252     260      +8
> > > > file_fat_read                                 40       -     -40
> > > > Total: Before=66833, After=66965, chg +0.20%
> > > > 
> > > > and even then, by inspection spl_simple_read isn't really taking
> > > > advantage of bl_len=1. Surprisingly, after enabling LTO this board is
> > > > +400 bytes (compared to without this series). It's just hard to beat
> > > > handwritten routines with a generic solution.
> > > > 
> > > > I would really like to consolidate this stuff, since every load method
> > > > ends up only supporting a few image types and having almost identical
> > > > implementations for what they do support.
> > > > 
> > > > --Sean
> > > > 
> > > > [1] https://lore.kernel.org/u-boot/20230731212734.7256-1-sean.anderson@seco.com/
> > > 
> > > Thanks for digging more, lets keep going in this direction then.
> > > 
> > 
> > Do we need to pass sectors to the info->load() functions?
> > 
> > Only nand and mmc use bl_len != 1. Can we move the conversion to sectors
> > and the alignment checks to functions called by info->load()?
> > 
> > I would assume that this can avoid part of the code size increase.
> 
> Unfortunately, while the conversion to sectors and alignment can be put into ->load,
> MMC (and ROMAPI) can only read in block-sized chunks. This means that the buffer must
> be a multiple of the block size (otherwise we will go over the end), and there must be
> a block's worth of space in front of the buffer as well (which we use when we have a
> non-block-aligned offset).
> 
> These situations generally only matter when reading into a malloc'd buffer, since there
> is data to overwrite on either end of the buffer. With things like SYS_TEXT_BASE, there
> is nothing to overwrite on either end. From examining the various loaders, malloc'd buffers
> are only used when loading the image header (which may include the full image for FITs).
> In that situation, under the beginning of the buffer is not an issue, since the load method
> should have supplied an offset which is a multiple of the block size. Thus, we only need
> to solve the problem of writing past the end of the buffer.
> 
> From my perspective, there are three approaches we could take:
> 
> - Keep bl_len so that callers of ->read can allocate a sufficiently-large buffer.
> - Use a bounce-buffer. This uses around double the memory and will also increase code size.
> - Complete most of the read as normal, but read the final block into a separate buffer which
>   is then copied into the passed-in buffer. This requires two reads, which is likely slower
>   than one. It will also increase code size.
> 
> I think the first approach could be improved by optionally removing bl_len when there are
> no load methods which need it. This will let the compiler optimize out any alignment if it
> is unnecessary, which it cannot currently do (unless LTO is enabled). I am going to try using
> this approach for v6.

This sounds good, and LTO is / can be an option for SPL for platforms
that need to regain space.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]