Re: [PATCH V2 10/46] arm: imx8ulp: add container support

[Stefano Babic <sbabic@denx.de>]

Hi peng, Ye,

On 29.06.21 04:32, Peng Fan (OSS) wrote:
> From: Ye Li <ye.li@nxp.com>
> 
> i.MX8ULP support using ROM API to load container image,
> it use same ROM API as i.MX8MN/MP, and use same container format
> as i.MX8QM/QXP.
> 
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   arch/arm/include/asm/arch-imx8ulp/sys_proto.h |   2 +
>   arch/arm/include/asm/mach-imx/image.h         |   2 +
>   arch/arm/include/asm/mach-imx/sys_proto.h     |   2 +-
>   arch/arm/mach-imx/Kconfig                     |   2 +-
>   arch/arm/mach-imx/image-container.c           |  42 ++++-
>   arch/arm/mach-imx/imx8ulp/soc.c               |   2 +
>   arch/arm/mach-imx/spl_imx_romapi.c            | 172 ++++++++++++++----
>   7 files changed, 177 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-imx8ulp/sys_proto.h b/arch/arm/include/asm/arch-imx8ulp/sys_proto.h
> index 8894611a0f..a8f632f45e 100644
> --- a/arch/arm/include/asm/arch-imx8ulp/sys_proto.h
> +++ b/arch/arm/include/asm/arch-imx8ulp/sys_proto.h
> @@ -10,5 +10,7 @@
>   

This breaks after bisecting all mx8mn boards. For example:

    aarch64:  +   imx8mn_beacon_2g
+arch/arm/mach-imx/imx8ulp/Kconfig:10:warning: config symbol defined 
without type
+===================== WARNING ======================
+This board uses CONFIG_SPL_FIT_GENERATOR. Please migrate
+to binman instead, to avoid the proliferation of
+arch-specific scripts with no tests.
+====================================================
+WARNING 'lpddr4_pmu_train_1d_imem.bin' not found, resulting binary is 
not-functional
+arch/arm/mach-imx/spl_imx_romapi.c: In function 
'spl_romapi_load_image_stream':
+arch/arm/mach-imx/spl_imx_romapi.c:352:5: error: lvalue required as 
left operand of assignment
+  352 |  gd = pgd;
+      |     ^
+make[2]: *** [scripts/Makefile.build:266: 
arch/arm/mach-imx/spl_imx_romapi.o] Error 1
+make[1]: *** [Makefile:1815: arch/arm/mach-imx] Error 2
+make: *** [Makefile:171: sub-make] Error 2

Peng, in the series there are 2 patches related to i.MX7ULP that are 
unrelated to the object this series. I am taking them out and applying 
to -master, you do not need to repost them.

I have checked if I can  apply some of the patches here, but it becomes 
a mess if I do. So please repost them after fixing this.

Best regards,
Stefano

>   extern unsigned long rom_pointer[];
>   
> +ulong spl_romapi_raw_seekable_read(u32 offset, u32 size, void *buf);
> +ulong spl_romapi_get_uboot_base(u32 image_offset, u32 rom_bt_dev);
>   enum bt_mode get_boot_mode(void);
>   #endif
> diff --git a/arch/arm/include/asm/mach-imx/image.h b/arch/arm/include/asm/mach-imx/image.h
> index 547beeb986..ee67ca96f4 100644
> --- a/arch/arm/include/asm/mach-imx/image.h
> +++ b/arch/arm/include/asm/mach-imx/image.h
> @@ -64,4 +64,6 @@ struct generate_key_blob_hdr {
>   	u8 algorithm;
>   	u8 mode;
>   } __packed;
> +
> +int get_container_size(ulong addr, u16 *header_length);
>   #endif
> diff --git a/arch/arm/include/asm/mach-imx/sys_proto.h b/arch/arm/include/asm/mach-imx/sys_proto.h
> index e3ee58c9f8..444834995e 100644
> --- a/arch/arm/include/asm/mach-imx/sys_proto.h
> +++ b/arch/arm/include/asm/mach-imx/sys_proto.h
> @@ -145,7 +145,7 @@ struct rproc_att {
>   	u32 size; /* size of reg range */
>   };
>   
> -#ifdef CONFIG_IMX8M
> +#if defined(CONFIG_IMX8M) || defined(CONFIG_IMX8ULP)
>   struct rom_api {
>   	u16 ver;
>   	u16 tag;
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index 26bfc5ccc4..c089664375 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -161,7 +161,7 @@ config DDRMC_VF610_CALIBRATION
>   
>   config SPL_IMX_ROMAPI_LOADADDR
>   	hex "Default load address to load image through ROM API"
> -	depends on IMX8MN || IMX8MP
> +	depends on IMX8MN || IMX8MP || IMX8ULP
>   
>   config IMX_DCD_ADDR
>   	hex "DCD Blocks location on the image"
> diff --git a/arch/arm/mach-imx/image-container.c b/arch/arm/mach-imx/image-container.c
> index 9e18f6630f..c3f62872c6 100644
> --- a/arch/arm/mach-imx/image-container.c
> +++ b/arch/arm/mach-imx/image-container.c
> @@ -19,8 +19,9 @@
>   #define QSPI_DEV	1
>   #define NAND_DEV	2
>   #define QSPI_NOR_DEV	3
> +#define ROM_API_DEV	4
>   
> -static int __get_container_size(ulong addr)
> +int get_container_size(ulong addr, u16 *header_length)
>   {
>   	struct container_hdr *phdr;
>   	struct boot_img_t *img_entry;
> @@ -34,7 +35,9 @@ static int __get_container_size(ulong addr)
>   		return -EFAULT;
>   	}
>   
> -	max_offset = sizeof(struct container_hdr);
> +	max_offset = phdr->length_lsb + (phdr->length_msb << 8);
> +	if (header_length)
> +		*header_length = max_offset;
>   
>   	img_entry = (struct boot_img_t *)(addr + sizeof(struct container_hdr));
>   	for (i = 0; i < phdr->num_images; i++) {
> @@ -60,7 +63,7 @@ static int __get_container_size(ulong addr)
>   	return max_offset;
>   }
>   
> -static int get_container_size(void *dev, int dev_type, unsigned long offset)
> +static int get_dev_container_size(void *dev, int dev_type, unsigned long offset, u16 *header_length)
>   {
>   	u8 *buf = malloc(CONTAINER_HDR_ALIGNMENT);
>   	int ret = 0;
> @@ -115,7 +118,17 @@ static int get_container_size(void *dev, int dev_type, unsigned long offset)
>   		memcpy(buf, (const void *)offset, CONTAINER_HDR_ALIGNMENT);
>   #endif
>   
> -	ret = __get_container_size((ulong)buf);
> +#ifdef CONFIG_SPL_BOOTROM_SUPPORT
> +	if (dev_type == ROM_API_DEV) {
> +		ret = spl_romapi_raw_seekable_read(offset, CONTAINER_HDR_ALIGNMENT, buf);
> +		if (!ret) {
> +			printf("Read container image from ROM API failed\n");
> +			return -EIO;
> +		}
> +	}
> +#endif
> +
> +	ret = get_container_size((ulong)buf, header_length);
>   
>   	free(buf);
>   
> @@ -149,6 +162,8 @@ static unsigned long get_boot_device_offset(void *dev, int dev_type)
>   		offset = CONTAINER_HDR_NAND_OFFSET;
>   	} else if (dev_type == QSPI_NOR_DEV) {
>   		offset = CONTAINER_HDR_QSPI_OFFSET + 0x08000000;
> +	} else if (dev_type == ROM_API_DEV) {
> +		offset = (unsigned long)dev;
>   	}
>   
>   	return offset;
> @@ -158,11 +173,12 @@ static int get_imageset_end(void *dev, int dev_type)
>   {
>   	unsigned long offset1 = 0, offset2 = 0;
>   	int value_container[2];
> +	u16 hdr_length;
>   
>   	offset1 = get_boot_device_offset(dev, dev_type);
>   	offset2 = CONTAINER_HDR_ALIGNMENT + offset1;
>   
> -	value_container[0] = get_container_size(dev, dev_type, offset1);
> +	value_container[0] = get_dev_container_size(dev, dev_type, offset1, &hdr_length);
>   	if (value_container[0] < 0) {
>   		printf("Parse seco container failed %d\n", value_container[0]);
>   		return value_container[0];
> @@ -170,7 +186,7 @@ static int get_imageset_end(void *dev, int dev_type)
>   
>   	debug("seco container size 0x%x\n", value_container[0]);
>   
> -	value_container[1] = get_container_size(dev, dev_type, offset2);
> +	value_container[1] = get_dev_container_size(dev, dev_type, offset2, &hdr_length);
>   	if (value_container[1] < 0) {
>   		debug("Parse scu container failed %d, only seco container\n",
>   		      value_container[1]);
> @@ -247,3 +263,17 @@ unsigned long spl_nor_get_uboot_base(void)
>   	return end;
>   }
>   #endif
> +
> +#ifdef CONFIG_SPL_BOOTROM_SUPPORT
> +ulong spl_romapi_get_uboot_base(u32 image_offset, u32 rom_bt_dev)
> +{
> +	ulong end;
> +
> +	end = get_imageset_end((void *)(ulong)image_offset, ROM_API_DEV);
> +	end = ROUND(end, SZ_1K);
> +
> +	printf("Load image from 0x%lx by ROM_API\n", end);
> +
> +	return end;
> +}
> +#endif
> diff --git a/arch/arm/mach-imx/imx8ulp/soc.c b/arch/arm/mach-imx/imx8ulp/soc.c
> index f14c6b421a..3029589d5f 100644
> --- a/arch/arm/mach-imx/imx8ulp/soc.c
> +++ b/arch/arm/mach-imx/imx8ulp/soc.c
> @@ -12,6 +12,8 @@
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> +struct rom_api *g_rom_api = (struct rom_api *)0x1980;
> +
>   u32 get_cpu_rev(void)
>   {
>   	return (MXC_CPU_IMX8ULP << 12) | CHIP_REV_1_0;
> diff --git a/arch/arm/mach-imx/spl_imx_romapi.c b/arch/arm/mach-imx/spl_imx_romapi.c
> index d2085dabd3..564cc400c1 100644
> --- a/arch/arm/mach-imx/spl_imx_romapi.c
> +++ b/arch/arm/mach-imx/spl_imx_romapi.c
> @@ -10,11 +10,44 @@
>   #include <asm/global_data.h>
>   #include <linux/libfdt.h>
>   #include <spl.h>
> -
> +#include <asm/mach-imx/image.h>
>   #include <asm/arch/sys_proto.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> +/* Caller need ensure the offset and size to align with page size */
> +ulong spl_romapi_raw_seekable_read(u32 offset, u32 size, void *buf)
> +{
> +	volatile gd_t *pgd = gd;
> +	int ret;
> +
> +	debug("%s 0x%x, size 0x%x\n", __func__, offset, size);
> +
> +	ret = g_rom_api->download_image(buf, offset, size,
> +					((uintptr_t)buf) ^ offset ^ size);
> +
> +	set_gd(pgd);
> +
> +	if (ret == ROM_API_OKAY)
> +		return size;
> +
> +	printf("%s Failure when load 0x%x, size 0x%x\n", __func__, offset, size);
> +
> +	return 0;
> +}
> +
> +ulong __weak spl_romapi_get_uboot_base(u32 image_offset, u32 rom_bt_dev)
> +{
> +	u32 offset;
> +
> +	if (((rom_bt_dev >> 16) & 0xff) ==  BT_DEV_TYPE_FLEXSPINOR)
> +		offset = CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512;
> +	else
> +		offset = image_offset + CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512 - 0x8000;
> +
> +	return offset;
> +}
> +
>   static int is_boot_from_stream_device(u32 boot)
>   {
>   	u32 interface;
> @@ -34,25 +67,12 @@ static ulong spl_romapi_read_seekable(struct spl_load_info *load,
>   				      void *buf)
>   {
>   	u32 pagesize = *(u32 *)load->priv;
> -	volatile gd_t *pgd = gd;
>   	ulong byte = count * pagesize;
> -	int ret;
>   	u32 offset;
>   
>   	offset = sector * pagesize;
>   
> -	debug("ROM API load from 0x%x, size 0x%x\n", offset, (u32)byte);
> -
> -	ret = g_rom_api->download_image(buf, offset, byte,
> -					((uintptr_t)buf) ^ offset ^ byte);
> -	set_gd(pgd);
> -
> -	if (ret == ROM_API_OKAY)
> -		return count;
> -
> -	printf("ROM API Failure when load 0x%x\n", offset);
> -
> -	return 0;
> +	return spl_romapi_raw_seekable_read(offset, byte, buf) / pagesize;
>   }
>   
>   static int spl_romapi_load_image_seekable(struct spl_image_info *spl_image,
> @@ -85,11 +105,7 @@ static int spl_romapi_load_image_seekable(struct spl_image_info *spl_image,
>   	printf("image offset 0x%x, pagesize 0x%x, ivt offset 0x%x\n",
>   	       image_offset, pagesize, offset);
>   
> -	if (((rom_bt_dev >> 16) & 0xff) ==  BT_DEV_TYPE_FLEXSPINOR)
> -		offset = CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512;
> -	else
> -		offset = image_offset +
> -			CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512 - 0x8000;
> +	offset = spl_romapi_get_uboot_base(image_offset, rom_bt_dev);
>   
>   	size = ALIGN(sizeof(struct image_header), pagesize);
>   	ret = g_rom_api->download_image((u8 *)header, offset, size,
> @@ -102,16 +118,23 @@ static int spl_romapi_load_image_seekable(struct spl_image_info *spl_image,
>   		return -1;
>   	}
>   
> -	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> -	    image_get_magic(header) == FDT_MAGIC) {
> +	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) && image_get_magic(header) == FDT_MAGIC) {
>   		struct spl_load_info load;
>   
>   		memset(&load, 0, sizeof(load));
>   		load.bl_len = pagesize;
>   		load.read = spl_romapi_read_seekable;
>   		load.priv = &pagesize;
> -		return spl_load_simple_fit(spl_image, &load,
> -					   offset / pagesize, header);
> +		return spl_load_simple_fit(spl_image, &load, offset / pagesize, header);
> +	} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> +		struct spl_load_info load;
> +
> +		memset(&load, 0, sizeof(load));
> +		load.bl_len = pagesize;
> +		load.read = spl_romapi_read_seekable;
> +		load.priv = &pagesize;
> +
> +		ret = spl_load_imx_container(spl_image, &load, offset / pagesize);
>   	} else {
>   		/* TODO */
>   		puts("Can't support legacy image\n");
> @@ -154,7 +177,7 @@ static ulong get_fit_image_size(void *fit)
>   	return last - (ulong)fit;
>   }
>   
> -u8 *search_fit_header(u8 *p, int size)
> +static u8 *search_fit_header(u8 *p, int size)
>   {
>   	int i;
>   
> @@ -165,6 +188,71 @@ u8 *search_fit_header(u8 *p, int size)
>   	return NULL;
>   }
>   
> +static u8 *search_container_header(u8 *p, int size)
> +{
> +	int i = 0;
> +	u8 *hdr;
> +
> +	for (i = 0; i < size; i += 4) {
> +		hdr = p + i;
> +		if (*(hdr + 3) == 0x87 && *hdr == 0 && (*(hdr + 1) != 0 || *(hdr + 2) != 0))
> +                        return p + i;
> +	}
> +
> +	return NULL;
> +}
> +
> +static u8 *search_img_header(u8 *p, int size)
> +{
> +	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT))
> +		return search_fit_header(p, size);
> +	else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
> +		return search_container_header(p, size);
> +
> +	return NULL;
> +}
> +
> +static u32 img_header_size(void)
> +{
> +	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT))
> +		return sizeof(struct fdt_header);
> +	else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
> +		return sizeof(struct container_hdr);
> +
> +	return 0;
> +}
> +
> +static int img_info_size(void *img_hdr)
> +{
> +#ifdef CONFIG_SPL_LOAD_FIT
> +	return fit_get_size(img_hdr);
> +#elif defined CONFIG_SPL_LOAD_IMX_CONTAINER
> +	struct container_hdr *container = img_hdr;
> +
> +	return (container->length_lsb + (container->length_msb << 8));
> +#else
> +	return 0;
> +#endif
> +}
> +
> +static int img_total_size(void *img_hdr)
> +{
> +	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT)) {
> +		return get_fit_image_size(img_hdr);
> +	} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> +		int total = get_container_size((ulong)img_hdr, NULL);
> +
> +		if (total < 0) {
> +			printf("invalid container image\n");
> +			return 0;
> +		}
> +
> +		return total;
> +	}
> +
> +	return 0;
> +}
> +
>   static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
>   					struct spl_boot_device *bootdev)
>   {
> @@ -174,7 +262,7 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
>   	int ret;
>   	int i = 0;
>   	u8 *p = (u8 *)CONFIG_SPL_IMX_ROMAPI_LOADADDR;
> -	u8 *pfit = NULL;
> +	u8 *phdr = NULL;
>   	int imagesize;
>   	int total;
>   
> @@ -199,19 +287,19 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
>   			return -1;
>   		}
>   
> -		pfit = search_fit_header(p, pg);
> +		phdr = search_img_header(p, pg);
>   		p += pg;
>   
> -		if (pfit)
> +		if (phdr)
>   			break;
>   	}
>   
> -	if (!pfit) {
> -		puts("Can't found uboot FIT image in 640K range \n");
> +	if (!phdr) {
> +		puts("Can't found uboot image in 640K range\n");
>   		return -1;
>   	}
>   
> -	if (p - pfit < sizeof(struct fdt_header)) {
> +	if (p - phdr < img_header_size()) {
>   		ret = g_rom_api->download_image(p, 0, pg,  ((uintptr_t)p) ^ pg);
>   		set_gd(pgd);
>   
> @@ -223,11 +311,11 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
>   		p += pg;
>   	}
>   
> -	imagesize = fit_get_size(pfit);
> -	printf("Find FIT header 0x&%p, size %d\n", pfit, imagesize);
> +	imagesize = img_info_size(phdr);
> +	printf("Find img info 0x&%p, size %d\n", phdr, imagesize);
>   
> -	if (p - pfit < imagesize) {
> -		imagesize -= p - pfit;
> +	if (p - phdr < imagesize) {
> +		imagesize -= p - phdr;
>   		/*need pagesize hear after ROM fix USB problme*/
>   		imagesize += pg - 1;
>   		imagesize /= pg;
> @@ -247,20 +335,21 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
>   		}
>   	}
>   
> -	total = get_fit_image_size(pfit);
> +	total = img_total_size(phdr);
>   	total += 3;
>   	total &= ~0x3;
>   
> -	imagesize = total - (p - pfit);
> +	imagesize = total - (p - phdr);
>   
>   	imagesize += pagesize - 1;
>   	imagesize /= pagesize;
>   	imagesize *= pagesize;
>   
> -	printf("Download %d, total fit %d\n", imagesize, total);
> +	printf("Download %d, Total size %d\n", imagesize, total);
>   
>   	ret = g_rom_api->download_image(p, 0, imagesize,
>   					((uintptr_t)p) ^ imagesize);
> +	gd = pgd;
>   	if (ret != ROM_API_OKAY)
>   		printf("ROM download failure %d\n", imagesize);
>   
> @@ -268,7 +357,12 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
>   	load.bl_len = 1;
>   	load.read = spl_ram_load_read;
>   
> -	return spl_load_simple_fit(spl_image, &load, (ulong)pfit, pfit);
> +	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT))
> +		return spl_load_simple_fit(spl_image, &load, (ulong)phdr, phdr);
> +	else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
> +		return spl_load_imx_container(spl_image, &load, (ulong)phdr);
> +
> +	return -1;
>   }
>   
>   int board_return_to_bootrom(struct spl_image_info *spl_image,
> 

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================