diff mbox series

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

Message ID 20230731224304.111081-11-sean.anderson@seco.com
State Changes Requested
Delegated to: Tom Rini
Headers show
Series spl: Use common function for loading/parsing images | expand

Commit Message

Sean Anderson July 31, 2023, 10:43 p.m. UTC 
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(-)

Comments

Xavier Drudis Ferran Aug. 3, 2023, 8:45 a.m. UTC | #1 
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
>
Sean Anderson Aug. 3, 2023, 4:27 p.m. UTC | #2 
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
>>
diff mbox series

Patch

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