[PATCH 0/2] imx8m rom api fixups

[PATCH 0/2] imx8m rom api fixups

[Rasmus Villemoes]

While trying to figure out why I can't get an imx8mp to boot via usb
serial download, I stumbled on the distinct lack of documentation on
both the ROM API and the USB protocol, and what appears to be an
actual bug in the rom api interface code.

Rasmus Villemoes (2):
  arm: imx: move spl_imx_romapi.c to imx8m/ subdirectory
  arm: imx8m: sanitize use of ROM API

 arch/arm/include/asm/mach-imx/sys_proto.h     |  5 ++-
 arch/arm/mach-imx/Makefile                    |  2 -
 arch/arm/mach-imx/imx8m/Makefile              |  1 +
 arch/arm/mach-imx/imx8m/soc.c                 | 33 +++++++++++---
 .../arm/mach-imx/{ => imx8m}/spl_imx_romapi.c | 45 +++++--------------
 5 files changed, 42 insertions(+), 44 deletions(-)
 rename arch/arm/mach-imx/{ => imx8m}/spl_imx_romapi.c (80%)

-- 
2.31.1

[PATCH 1/2] arm: imx: move spl_imx_romapi.c to imx8m/ subdirectory

[Rasmus Villemoes]

Currently, if one builds for an iMX platform != imx8m and selects
CONFIG_SPL_BOOTROM_SUPPORT, the build breaks because some
definitions (struct rom_api, the enum boot_dev_type_e and various
QUERY_* macros) are only exposed by the sys_proto.h header when
CONFIG_IMX8M=y.

While it's not necessarily meaningful to enable BOOTROM_SUPPORT for
those other platforms, it makes better sense for code which is
specific to imx8m to live in imx8m/.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/mach-imx/Makefile                     | 2 --
 arch/arm/mach-imx/imx8m/Makefile               | 1 +
 arch/arm/mach-imx/{ => imx8m}/spl_imx_romapi.c | 0
 3 files changed, 1 insertion(+), 2 deletions(-)
 rename arch/arm/mach-imx/{ => imx8m}/spl_imx_romapi.c (100%)

diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
index 82aa39dee7..a9dee38c8d 100644
--- a/arch/arm/mach-imx/Makefile
+++ b/arch/arm/mach-imx/Makefile
@@ -230,5 +230,3 @@ obj-$(CONFIG_ARCH_MX7ULP) += mx7ulp/
 obj-$(CONFIG_IMX8M) += imx8m/
 obj-$(CONFIG_ARCH_IMX8) += imx8/
 obj-$(CONFIG_ARCH_IMXRT) += imxrt/
-
-obj-$(CONFIG_SPL_BOOTROM_SUPPORT) += spl_imx_romapi.o
diff --git a/arch/arm/mach-imx/imx8m/Makefile b/arch/arm/mach-imx/imx8m/Makefile
index d9dee894aa..3911489d2b 100644
--- a/arch/arm/mach-imx/imx8m/Makefile
+++ b/arch/arm/mach-imx/imx8m/Makefile
@@ -6,3 +6,4 @@ obj-y += lowlevel_init.o
 obj-y += clock_slice.o soc.o
 obj-$(CONFIG_IMX8MQ) += clock_imx8mq.o
 obj-$(CONFIG_IMX8MM)$(CONFIG_IMX8MN)$(CONFIG_IMX8MP) += clock_imx8mm.o
+obj-$(CONFIG_SPL_BOOTROM_SUPPORT) += spl_imx_romapi.o
diff --git a/arch/arm/mach-imx/spl_imx_romapi.c b/arch/arm/mach-imx/imx8m/spl_imx_romapi.c
similarity index 100%
rename from arch/arm/mach-imx/spl_imx_romapi.c
rename to arch/arm/mach-imx/imx8m/spl_imx_romapi.c
-- 
2.31.1

[PATCH 2/2] arm: imx8m: sanitize use of ROM API

[Rasmus Villemoes]

There are a couple of bugs regarding the ROM API.

The first is that the API is entirely undocumented. Why do they need
to receive the xor of the preceding arguments as a final argument? And
is it really just the lower 32 bits that need to "match", or are we
relying on the fact that all the pointer arguments that we cast
to (uintptr_t) and mix in to the xor all live in the lower 4G of the
address space? What ABI do those functions follow? Presumably mostly
the standard aarch64 calling convention, since we do call them as
normal C functions (apart from the odd xor argument which is added
manually).

There is some effort to save and restore gd, aka r18, presumably
because the ROM API functions are not guaranteed to preserve that (but
they must then be assumed to preserve the callee-saved registers r19
through r28). This is the second bug: The last invocation of
->download_image in spl_romapi_load_image_stream() fails to do that
restore.

Rather than just blindly adding a set_gd(), create wrapper functions
that (1) compute the apparently needed xor argument and (2)
save/restore gd, instead of open-coding these things everywhere.

I expect the NXP folks will fix the first bug.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/include/asm/mach-imx/sys_proto.h |  5 ++-
 arch/arm/mach-imx/imx8m/soc.c             | 33 ++++++++++++++---
 arch/arm/mach-imx/imx8m/spl_imx_romapi.c  | 45 ++++++-----------------
 3 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/arch/arm/include/asm/mach-imx/sys_proto.h b/arch/arm/include/asm/mach-imx/sys_proto.h
index b612189849..e989982acb 100644
--- a/arch/arm/include/asm/mach-imx/sys_proto.h
+++ b/arch/arm/include/asm/mach-imx/sys_proto.h
@@ -153,6 +153,9 @@ struct rom_api {
 	u32 (*query_boot_infor)(u32 info_type, u32 *info, u32 xor);
 };
 
+u32 rom_api_download_image(u8 *dest, u32 offset, u32 size);
+u32 rom_api_query_boot_infor(u32 info_type, u32 *info);
+
 enum boot_dev_type_e {
 	BT_DEV_TYPE_SD = 1,
 	BT_DEV_TYPE_MMC = 2,
@@ -173,8 +176,6 @@ enum boot_dev_type_e {
 #define QUERY_IMG_OFF		6
 
 #define ROM_API_OKAY		0xF0
-
-extern struct rom_api *g_rom_api;
 #endif
 
 u32 get_nr_cpus(void);
diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
index 0c44022a6d..c8beb77e1e 100644
--- a/arch/arm/mach-imx/imx8m/soc.c
+++ b/arch/arm/mach-imx/imx8m/soc.c
@@ -523,21 +523,42 @@ int arch_cpu_init(void)
 	return 0;
 }
 
-#if defined(CONFIG_IMX8MN) || defined(CONFIG_IMX8MP)
-struct rom_api *g_rom_api = (struct rom_api *)0x980;
+u32 rom_api_download_image(u8 *dest, u32 offset, u32 size)
+{
+	struct rom_api *rom_api = (struct rom_api *)0x980;
+	u32 xor = (uintptr_t)dest ^ offset ^ size;
+	volatile gd_t *sgd = gd;
+	u32 ret;
+
+	ret = rom_api->download_image(dest, offset, size, xor);
+	set_gd(sgd);
 
+	return ret;
+}
+
+u32 rom_api_query_boot_infor(u32 info_type, u32 *info)
+{
+	struct rom_api *rom_api = (struct rom_api *)0x980;
+	u32 xor = info_type ^ (uintptr_t)info;
+	volatile gd_t *sgd = gd;
+	u32 ret;
+
+	ret = rom_api->query_boot_infor(info_type, info, xor);
+	set_gd(sgd);
+
+	return ret;
+}
+
+#if defined(CONFIG_IMX8MN) || defined(CONFIG_IMX8MP)
 enum boot_device get_boot_device(void)
 {
-	volatile gd_t *pgd = gd;
 	int ret;
 	u32 boot;
 	u16 boot_type;
 	u8 boot_instance;
 	enum boot_device boot_dev = SD1_BOOT;
 
-	ret = g_rom_api->query_boot_infor(QUERY_BT_DEV, &boot,
-					  ((uintptr_t)&boot) ^ QUERY_BT_DEV);
-	set_gd(pgd);
+	ret = rom_api_query_boot_infor(QUERY_BT_DEV, &boot);
 
 	if (ret != ROM_API_OKAY) {
 		puts("ROMAPI: failure at query_boot_info\n");
diff --git a/arch/arm/mach-imx/imx8m/spl_imx_romapi.c b/arch/arm/mach-imx/imx8m/spl_imx_romapi.c
index d2085dabd3..68a585b52f 100644
--- a/arch/arm/mach-imx/imx8m/spl_imx_romapi.c
+++ b/arch/arm/mach-imx/imx8m/spl_imx_romapi.c
@@ -34,7 +34,6 @@ 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;
@@ -43,9 +42,7 @@ static ulong spl_romapi_read_seekable(struct spl_load_info *load,
 
 	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);
+	ret = rom_api_download_image(buf, offset, byte);
 
 	if (ret == ROM_API_OKAY)
 		return count;
@@ -59,21 +56,15 @@ static int spl_romapi_load_image_seekable(struct spl_image_info *spl_image,
 					  struct spl_boot_device *bootdev,
 					  u32 rom_bt_dev)
 {
-	volatile gd_t *pgd = gd;
 	int ret;
 	u32 offset;
 	u32 pagesize, size;
 	struct image_header *header;
 	u32 image_offset;
 
-	ret = g_rom_api->query_boot_infor(QUERY_IVT_OFF, &offset,
-					  ((uintptr_t)&offset) ^ QUERY_IVT_OFF);
-	ret |= g_rom_api->query_boot_infor(QUERY_PAGE_SZ, &pagesize,
-					   ((uintptr_t)&pagesize) ^ QUERY_PAGE_SZ);
-	ret |= g_rom_api->query_boot_infor(QUERY_IMG_OFF, &image_offset,
-					   ((uintptr_t)&image_offset) ^ QUERY_IMG_OFF);
-
-	set_gd(pgd);
+	ret = rom_api_query_boot_infor(QUERY_IVT_OFF, &offset);
+	ret |= rom_api_query_boot_infor(QUERY_PAGE_SZ, &pagesize);
+	ret |= rom_api_query_boot_infor(QUERY_IMG_OFF, &image_offset);
 
 	if (ret != ROM_API_OKAY) {
 		puts("ROMAPI: Failure query boot infor pagesize/offset\n");
@@ -92,9 +83,7 @@ static int spl_romapi_load_image_seekable(struct spl_image_info *spl_image,
 			CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512 - 0x8000;
 
 	size = ALIGN(sizeof(struct image_header), pagesize);
-	ret = g_rom_api->download_image((u8 *)header, offset, size,
-					((uintptr_t)header) ^ offset ^ size);
-	set_gd(pgd);
+	ret = rom_api_download_image((u8 *)header, offset, size);
 
 	if (ret != ROM_API_OKAY) {
 		printf("ROMAPI: download failure offset 0x%x size 0x%x\n",
@@ -169,7 +158,6 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
 					struct spl_boot_device *bootdev)
 {
 	struct spl_load_info load;
-	volatile gd_t *pgd = gd;
 	u32 pagesize, pg;
 	int ret;
 	int i = 0;
@@ -178,9 +166,7 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
 	int imagesize;
 	int total;
 
-	ret = g_rom_api->query_boot_infor(QUERY_PAGE_SZ, &pagesize,
-					  ((uintptr_t)&pagesize) ^ QUERY_PAGE_SZ);
-	set_gd(pgd);
+	ret = rom_api_query_boot_infor(QUERY_PAGE_SZ, &pagesize);
 
 	if (ret != ROM_API_OKAY)
 		puts("failure at query_boot_info\n");
@@ -190,9 +176,7 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
 		pg = 1024;
 
 	for (i = 0; i < 640; i++) {
-		ret = g_rom_api->download_image(p, 0, pg,
-						((uintptr_t)p) ^ pg);
-		set_gd(pgd);
+		ret = rom_api_download_image(p, 0, pg);
 
 		if (ret != ROM_API_OKAY) {
 			puts("Steam(USB) download failure\n");
@@ -212,8 +196,7 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
 	}
 
 	if (p - pfit < sizeof(struct fdt_header)) {
-		ret = g_rom_api->download_image(p, 0, pg,  ((uintptr_t)p) ^ pg);
-		set_gd(pgd);
+		ret = rom_api_download_image(p, 0, pg);
 
 		if (ret != ROM_API_OKAY) {
 			puts("Steam(USB) download failure\n");
@@ -235,9 +218,7 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
 
 		printf("Need continue download %d\n", imagesize);
 
-		ret = g_rom_api->download_image(p, 0, imagesize,
-						((uintptr_t)p) ^ imagesize);
-		set_gd(pgd);
+		ret = rom_api_download_image(p, 0, imagesize);
 
 		p += imagesize;
 
@@ -259,8 +240,7 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
 
 	printf("Download %d, total fit %d\n", imagesize, total);
 
-	ret = g_rom_api->download_image(p, 0, imagesize,
-					((uintptr_t)p) ^ imagesize);
+	ret = rom_api_download_image(p, 0, imagesize);
 	if (ret != ROM_API_OKAY)
 		printf("ROM download failure %d\n", imagesize);
 
@@ -274,13 +254,10 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
 int board_return_to_bootrom(struct spl_image_info *spl_image,
 			    struct spl_boot_device *bootdev)
 {
-	volatile gd_t *pgd = gd;
 	int ret;
 	u32 boot;
 
-	ret = g_rom_api->query_boot_infor(QUERY_BT_DEV, &boot,
-					  ((uintptr_t)&boot) ^ QUERY_BT_DEV);
-	set_gd(pgd);
+	ret = rom_api_query_boot_infor(QUERY_BT_DEV, &boot);
 
 	if (ret != ROM_API_OKAY) {
 		puts("ROMAPI: failure at query_boot_info\n");
-- 
2.31.1

Re: [PATCH 1/2] arm: imx: move spl_imx_romapi.c to imx8m/ subdirectory

[Peng Fan (OSS)]

On 2021/10/14 20:52, Rasmus Villemoes wrote:
> Currently, if one builds for an iMX platform != imx8m and selects
> CONFIG_SPL_BOOTROM_SUPPORT, the build breaks because some
> definitions (struct rom_api, the enum boot_dev_type_e and various
> QUERY_* macros) are only exposed by the sys_proto.h header when
> CONFIG_IMX8M=y.

i.MX8ULP also use rom api.

Regards,
Peng.
> 
> While it's not necessarily meaningful to enable BOOTROM_SUPPORT for
> those other platforms, it makes better sense for code which is
> specific to imx8m to live in imx8m/.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   arch/arm/mach-imx/Makefile                     | 2 --
>   arch/arm/mach-imx/imx8m/Makefile               | 1 +
>   arch/arm/mach-imx/{ => imx8m}/spl_imx_romapi.c | 0
>   3 files changed, 1 insertion(+), 2 deletions(-)
>   rename arch/arm/mach-imx/{ => imx8m}/spl_imx_romapi.c (100%)
> 
> diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
> index 82aa39dee7..a9dee38c8d 100644
> --- a/arch/arm/mach-imx/Makefile
> +++ b/arch/arm/mach-imx/Makefile
> @@ -230,5 +230,3 @@ obj-$(CONFIG_ARCH_MX7ULP) += mx7ulp/
>   obj-$(CONFIG_IMX8M) += imx8m/
>   obj-$(CONFIG_ARCH_IMX8) += imx8/
>   obj-$(CONFIG_ARCH_IMXRT) += imxrt/
> -
> -obj-$(CONFIG_SPL_BOOTROM_SUPPORT) += spl_imx_romapi.o
> diff --git a/arch/arm/mach-imx/imx8m/Makefile b/arch/arm/mach-imx/imx8m/Makefile
> index d9dee894aa..3911489d2b 100644
> --- a/arch/arm/mach-imx/imx8m/Makefile
> +++ b/arch/arm/mach-imx/imx8m/Makefile
> @@ -6,3 +6,4 @@ obj-y += lowlevel_init.o
>   obj-y += clock_slice.o soc.o
>   obj-$(CONFIG_IMX8MQ) += clock_imx8mq.o
>   obj-$(CONFIG_IMX8MM)$(CONFIG_IMX8MN)$(CONFIG_IMX8MP) += clock_imx8mm.o
> +obj-$(CONFIG_SPL_BOOTROM_SUPPORT) += spl_imx_romapi.o
> diff --git a/arch/arm/mach-imx/spl_imx_romapi.c b/arch/arm/mach-imx/imx8m/spl_imx_romapi.c
> similarity index 100%
> rename from arch/arm/mach-imx/spl_imx_romapi.c
> rename to arch/arm/mach-imx/imx8m/spl_imx_romapi.c
> 

Re: [PATCH 1/2] arm: imx: move spl_imx_romapi.c to imx8m/ subdirectory

[Rasmus Villemoes]

On 15/10/2021 11.02, Peng Fan (OSS) wrote:
> 
> 
> On 2021/10/14 20:52, Rasmus Villemoes wrote:
>> Currently, if one builds for an iMX platform != imx8m and selects
>> CONFIG_SPL_BOOTROM_SUPPORT, the build breaks because some
>> definitions (struct rom_api, the enum boot_dev_type_e and various
>> QUERY_* macros) are only exposed by the sys_proto.h header when
>> CONFIG_IMX8M=y.
> 
> i.MX8ULP also use rom api.

Sorry about that, I thought I was working on top of 2021.10, but now I
see that I was working from 2021.07, which did just have

#ifdef CONFIG_IMX8M
 struct rom_api {

OK, so the first patch should go (which leaves the possibility of
selecting an option for which the build will break, but news at 11 I guess).

That leaves the second (I'll respin), along with the complete lack of
documentation of the ROM API.

There's also no documentation anywhere that I can find on the USB
protocol to use from the host for the iMX8MP, can you please provide me
with a pointer to that? E.g. the reference manual for i.MX 8M Dual/8M
QuadLite/8M Quad has a section "6.1.8.2 Serial Download Protocol (SDP)",
but there's no similar thing in the RM for i.MX 8M Plus.

Rasmus