[RFC PATCH 1/3] spl: spl_nor: Move legacy image loading into spl_legacy.c

[Stefan Roese]

On 15.04.20 13:46, Daniel Schwierzeck wrote:
> 
> 
> Am 10.04.20 um 13:04 schrieb Stefan Roese:
>> Move the legacy image loading into spl_legacy.c. This makes it easier
>> to extend the legacy image handling with new features that other
>> SPL loaders might use (e.g. spl_spi.c etc).
>>
>> No functional change intended.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Weijie Gao <weijie.gao at mediatek.com>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
>> ---
>> RFC comment: I'm sendig these 3 patches as RFC and once we've come to an
>> agreement on these (Acked etc), I'll integrate them into the mtmips SPL
>> patchset from Weijie and will send v7.
>>
>> Thanks,
>> Stefan
>>
>>   common/spl/spl_legacy.c | 20 ++++++++++++++++++++
>>   common/spl/spl_nor.c    | 27 +++++++++++++--------------
>>   include/spl.h           | 13 +++++++++++++
>>   3 files changed, 46 insertions(+), 14 deletions(-)
> 
> Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> 
> nits below
> 
>>
>> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
>> index 772135193e..7f00fc8885 100644
>> --- a/common/spl/spl_legacy.c
>> +++ b/common/spl/spl_legacy.c
>> @@ -51,3 +51,23 @@ int spl_parse_legacy_header(struct spl_image_info *spl_image,
>>   
>>   	return 0;
>>   }
>> +
>> +int spl_load_legacy_img(struct spl_image_info *spl_image,
>> +			struct spl_load_info *load, ulong header)
>> +{
>> +	struct image_header hdr;
>> +	int ret;
>> +
>> +	/* Read header into local struct */
>> +	load->read(load, header, sizeof(hdr), &hdr);
>> +
>> +	ret = spl_parse_image_header(spl_image, &hdr);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Read image */
>> +	load->read(load, header + sizeof(hdr), spl_image->size,
>> +		   (void *)(unsigned long)spl_image->load_addr);
>> +
>> +	return 0;
>> +}
>> diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
>> index b1e79b9ded..3f03ffe6a3 100644
>> --- a/common/spl/spl_nor.c
>> +++ b/common/spl/spl_nor.c
>> @@ -24,7 +24,6 @@ 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)
>>   {
>> -	int ret;
>>   	__maybe_unused const struct image_header *header;
>>   	__maybe_unused struct spl_load_info load;
>>   
>> @@ -43,6 +42,8 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>>   		header = (const struct image_header *)CONFIG_SYS_OS_BASE;
>>   #ifdef CONFIG_SPL_LOAD_FIT
>>   		if (image_get_magic(header) == FDT_MAGIC) {
>> +			int ret;
>> +
>>   			debug("Found FIT\n");
>>   			load.bl_len = 1;
>>   			load.read = spl_nor_load_read;
>> @@ -61,6 +62,7 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>>   #endif
>>   		if (image_get_os(header) == IH_OS_LINUX) {
>>   			/* happy - was a Linux */
>> +			int ret;
>>   
>>   			ret = spl_parse_image_header(spl_image, header);
>>   			if (ret)
>> @@ -93,11 +95,9 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>>   		debug("Found FIT format U-Boot\n");
>>   		load.bl_len = 1;
>>   		load.read = spl_nor_load_read;
>> -		ret = spl_load_simple_fit(spl_image, &load,
>> -					  spl_nor_get_uboot_base(),
>> -					  (void *)header);
>> -
>> -		return ret;
>> +		return spl_load_simple_fit(spl_image, &load,
>> +					   spl_nor_get_uboot_base(),
>> +					   (void *)header);
> 
> looks like an unrelated optimisation

It looks this way. But its related: This is needed, as the if statement
below now results in a warning when the usage of the "ret" variable is
not changed:

common/spl/spl_nor.c:27:6: warning: unused variable 'ret' 
[-Wunused-variable]

That's why I removed "ret" completely in this patch.

I could either generate a small patch "before" this one to remove "ret"
or mention this removal in the commit text of this patch.

Thanks,
Stefan

>>   	}
>>   #endif
>>   	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
>> @@ -107,14 +107,13 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
>>   					      spl_nor_get_uboot_base());
>>   	}
>>   
>> -	ret = spl_parse_image_header(spl_image,
>> -			(const struct image_header *)spl_nor_get_uboot_base());
>> -	if (ret)
>> -		return ret;
>> -
>> -	memcpy((void *)(unsigned long)spl_image->load_addr,
>> -	       (void *)(spl_nor_get_uboot_base() + sizeof(struct image_header)),
>> -	       spl_image->size);
>> +	/* Legacy image handling */
>> +	if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_SUPPORT)) {
>> +		load.bl_len = 1;
>> +		load.read = spl_nor_load_read;
>> +		return spl_load_legacy_img(spl_image, &load,
>> +					   spl_nor_get_uboot_base());
>> +	}
>>   
>>   	return 0;
>>   }
>> diff --git a/include/spl.h b/include/spl.h
>> index 6087cd793c..c6c64b6a72 100644
>> --- a/include/spl.h
>> +++ b/include/spl.h
>> @@ -219,6 +219,19 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
>>   #define SPL_COPY_PAYLOAD_ONLY	1
>>   #define SPL_FIT_FOUND		2
>>   
>> +/**
>> + * spl_load_legacy_img() - Loads a legacy image from a device.
>> + * @spl_image:	Image description to set up
>> + * @load:	Structure containing the information required to load data.
>> + * @header:	Pointer to image header (including appended image)
>> + *
>> + * Reads an legacy image from the device. Loads u-boot image to
>> + * specified load address.
>> + * Returns 0 on success.
>> + */
>> +int spl_load_legacy_img(struct spl_image_info *spl_image,
>> +			struct spl_load_info *load, ulong header);
>> +
>>   /**
>>    * spl_load_imx_container() - Loads a imx container image from a device.
>>    * @spl_image:	Image description to set up
>>
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de