[PATCH 20/26 v6] spl: spl_nor: Move legacy image loading into spl_legacy.c

[Daniel Schwierzeck]

Am 08.04.20 um 10:09 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 indended.
> 
> Signed-off-by: Stefan Roese <sr at denx.de>
> Cc: Weijie Gao <weijie.gao at mediatek.com>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> ---
> Changes in v6:
> - New patch
> 
>  common/spl/spl_legacy.c | 17 +++++++++++++++++
>  common/spl/spl_nor.c    | 34 +++++++++++++++++-----------------
>  include/spl.h           | 14 ++++++++++++++
>  3 files changed, 48 insertions(+), 17 deletions(-)
> 
> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
> index 772135193e..d0fc4b111e 100644
> --- a/common/spl/spl_legacy.c
> +++ b/common/spl/spl_legacy.c
> @@ -51,3 +51,20 @@ int spl_parse_legacy_header(struct spl_image_info *spl_image,
>  
>  	return 0;
>  }
> +
> +int spl_load_legacy_img(struct spl_image_info *spl_image,
> +			const struct image_header *header,
> +			uintptr_t dataptr, struct spl_load_info *info)
> +{
> +	int ret;
> +
> +	ret = spl_parse_image_header(spl_image, header);
> +	if (ret)
> +		return ret;
> +
> +	dataptr = (uintptr_t)header + sizeof(struct image_header);

how should this work? You call this function with

spl_load_legacy_img(spl_image, &hdr, dataptr, &load);

so "header" points to the copy on the stack. "dataptr" already holds the
address of the SPL payload. If you overwrite "dataptr" now, it will
point to the stack area after the header copy.


> +	info->read(info, dataptr, 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 cb8e06fe1f..d55165b772 100644
> --- a/common/spl/spl_nor.c
> +++ b/common/spl/spl_nor.c
> @@ -24,11 +24,8 @@ 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;
> -	struct image_header hdr;
> -	uintptr_t dataptr;
>  
>  	/*
>  	 * Loading of the payload to SDRAM is done with skipping of
> @@ -45,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;
> @@ -63,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)
> @@ -95,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);
>  	}
>  #endif
>  	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> @@ -109,17 +107,19 @@ 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;
> +	if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_SUPPORT)) {
> +		struct image_header hdr;
> +		uintptr_t dataptr;
>  
> -	/* Payload image may not be aligned, so copy it for safety */
> -	memcpy(&hdr, (void *)spl_nor_get_uboot_base(), sizeof(hdr));
> -	dataptr = spl_nor_get_uboot_base() + sizeof(struct image_header);
> +		/* Payload image may not be aligned, so copy it for safety */
> +		memcpy(&hdr, (void *)spl_nor_get_uboot_base(), sizeof(hdr));
> +		dataptr = spl_nor_get_uboot_base() + sizeof(hdr);
>  
> -	memcpy((void *)(unsigned long)spl_image->load_addr,
> -	       (void *)dataptr, spl_image->size);
> +		/* Legacy image handling */
> +		load.bl_len = 1;
> +		load.read = spl_nor_load_read;
> +		return spl_load_legacy_img(spl_image, &hdr, dataptr, &load);
> +	}

I don't understand it fully. Is the purpose of the header copy to avoid
unaligned access to the header in spl_parse_image_header()?

If so then the copy should be made in spl_load_legacy_img() itself.

>  
>  	return 0;
>  }
> diff --git a/include/spl.h b/include/spl.h
> index 6087cd793c..5331591db5 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -219,6 +219,20 @@ 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
> + * @dataptr:	Source location of the image
> + * @header:	Image header to parse
> + *
> + * 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,
> +			const struct image_header *header,
> +			uintptr_t dataptr, struct spl_load_info *load);
> +
>  /**
>   * spl_load_imx_container() - Loads a imx container image from a device.
>   * @spl_image:	Image description to set up
> 

-- 
- Daniel