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

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