[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 at 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
>