[PATCH v2 0/6] imx8 ROM API cleanup

[PATCH v2 0/6] imx8 ROM API cleanup

[Rasmus Villemoes]

The meat of this is patch 3, and the main simplification it offers is
patch 4.

I have private board code where I will need to fetch some data over
USB during bootstrapping, so having a more convenient API would be
useful. But I do believe these patches make sense in their own right.

v2: The original patches were accidentally not sent to the main U-Boot
list, but only uboot-imx@nxp.com and a few individuals.

Peng Fan replied with a R-b for the series, which I've included here
for the first five patches. In patch 6, I missed a place to convert in
the same file, so this is done here, but since the patch is not
identical I've elided the R-b.

Rasmus Villemoes (6):
  imx8: add hidden IMX8_ROMAPI Kconfig symbol
  imx8: sys_proto.h: change guard logic around ROM API
  imx8: add rom api wrappers
  imx8: use ROM API wrappers in spl_imx_romapi.c
  imx8m: soc.c: use rom_api_query_boot_infor() wrapper
  imx8ulp: soc.c: use rom_api_query_boot_infor() wrapper

 arch/arm/include/asm/mach-imx/sys_proto.h |  6 ++-
 arch/arm/mach-imx/Kconfig                 |  6 ++-
 arch/arm/mach-imx/Makefile                |  1 +
 arch/arm/mach-imx/imx8m/soc.c             |  5 +--
 arch/arm/mach-imx/imx8ulp/soc.c           | 10 +----
 arch/arm/mach-imx/romapi.c                | 30 +++++++++++++++
 arch/arm/mach-imx/spl_imx_romapi.c        | 47 ++++++-----------------
 7 files changed, 55 insertions(+), 50 deletions(-)
 create mode 100644 arch/arm/mach-imx/romapi.c

-- 
2.31.1

[PATCH v2 1/6] imx8: add hidden IMX8_ROMAPI Kconfig symbol

[Rasmus Villemoes]

In order not to repeat the IMX8MN || IMX8MP || IMX8ULP logic in
multiple places where we need to know if the SOC exposes the ROM API,
add a "def_bool y" Kconfig symbol.

Reviewed-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/mach-imx/Kconfig | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index ad0fb36502..50f4c08ea3 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -164,9 +164,13 @@ config DDRMC_VF610_CALIBRATION
 	  shall perform it on a new PCB and then use those values to program
 	  the ddrmc_cr_setting on relevant board file.
 
+config IMX8_ROMAPI
+	def_bool y
+	depends on IMX8MN || IMX8MP || IMX8ULP
+
 config SPL_IMX_ROMAPI_LOADADDR
 	hex "Default load address to load image through ROM API"
-	depends on IMX8MN || IMX8MP || IMX8ULP
+	depends on IMX8_ROMAPI
 
 config IMX_DCD_ADDR
 	hex "DCD Blocks location on the image"
-- 
2.31.1

[PATCH v2 2/6] imx8: sys_proto.h: change guard logic around ROM API

[Rasmus Villemoes]

This exposes the struct rom_api, the g_rom_api variable declaration
and the associated #defines to slightly fewer boards: namely, those
IMX8M which are not IMX8MN or IMX8MP. But the latter two are the only
IMX8M* ones where the g_rom_api variable is defined (in imx8m/soc.c),
so that should be fine.

Reviewed-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/include/asm/mach-imx/sys_proto.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/mach-imx/sys_proto.h b/arch/arm/include/asm/mach-imx/sys_proto.h
index fdbbfb169c..02816197c1 100644
--- a/arch/arm/include/asm/mach-imx/sys_proto.h
+++ b/arch/arm/include/asm/mach-imx/sys_proto.h
@@ -146,7 +146,7 @@ struct rproc_att {
 	u32 size; /* size of reg range */
 };
 
-#if defined(CONFIG_IMX8M) || defined(CONFIG_IMX8ULP)
+#if defined(CONFIG_IMX8_ROMAPI)
 struct rom_api {
 	u16 ver;
 	u16 tag;
-- 
2.31.1

[PATCH v2 3/6] imx8: add rom api wrappers

[Rasmus Villemoes]

The ROM API is thoroughly undocumented, but apparently passing the xor
of the real arguments as an extra argument is required [1]. Also, we
need to do the "save gd/restore gd" dance. These are both error-prone,
and lead to a lot of code duplication.

Since both imx8m[np] and imx8ulp SOCs have this, add a separate
translation unit which is included precisely when the new
CONFIG_IMX8_ROMAPI symbol is set, which provide convenience wrappers
that take care of computing the xor value as well as doing the gd
dance, and that thus have a more intuitive API. Subsequent patches
will make use of these to reduce boilerplate.

[1] One wonders, for example, if the check is only applied to the
lower 32 bits, or if we're implicitly relying on all 64-bit pointer
values we're passing effectively have 0 in the upper 32 bits.

Reviewed-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/include/asm/mach-imx/sys_proto.h |  4 +++
 arch/arm/mach-imx/Makefile                |  1 +
 arch/arm/mach-imx/romapi.c                | 30 +++++++++++++++++++++++
 3 files changed, 35 insertions(+)
 create mode 100644 arch/arm/mach-imx/romapi.c

diff --git a/arch/arm/include/asm/mach-imx/sys_proto.h b/arch/arm/include/asm/mach-imx/sys_proto.h
index 02816197c1..fadb67d04a 100644
--- a/arch/arm/include/asm/mach-imx/sys_proto.h
+++ b/arch/arm/include/asm/mach-imx/sys_proto.h
@@ -178,6 +178,10 @@ enum boot_dev_type_e {
 #define ROM_API_OKAY		0xF0
 
 extern struct rom_api *g_rom_api;
+
+u32 rom_api_download_image(u8 *dest, u32 offset, u32 size);
+u32 rom_api_query_boot_infor(u32 info_type, u32 *info);
+
 #endif
 
 /* For i.MX ULP */
diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
index aa0b6447f1..3cbcb151b8 100644
--- a/arch/arm/mach-imx/Makefile
+++ b/arch/arm/mach-imx/Makefile
@@ -243,3 +243,4 @@ obj-$(CONFIG_ARCH_IMX8) += imx8/
 obj-$(CONFIG_ARCH_IMXRT) += imxrt/
 
 obj-$(CONFIG_SPL_BOOTROM_SUPPORT) += spl_imx_romapi.o
+obj-$(CONFIG_IMX8_ROMAPI) += romapi.o
diff --git a/arch/arm/mach-imx/romapi.c b/arch/arm/mach-imx/romapi.c
new file mode 100644
index 0000000000..0e7b1d1a00
--- /dev/null
+++ b/arch/arm/mach-imx/romapi.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <asm/global_data.h>
+#include <asm/arch/sys_proto.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+u32 rom_api_download_image(u8 *dest, u32 offset, u32 size)
+{
+	u32 xor = (uintptr_t)dest ^ offset ^ size;
+	volatile gd_t *sgd = gd;
+	u32 ret;
+
+	ret = g_rom_api->download_image(dest, offset, size, xor);
+	set_gd(sgd);
+
+	return ret;
+}
+
+u32 rom_api_query_boot_infor(u32 info_type, u32 *info)
+{
+	u32 xor = info_type ^ (uintptr_t)info;
+	volatile gd_t *sgd = gd;
+	u32 ret;
+
+	ret = g_rom_api->query_boot_infor(info_type, info, xor);
+	set_gd(sgd);
+
+	return ret;
+}
-- 
2.31.1

[PATCH v2 4/6] imx8: use ROM API wrappers in spl_imx_romapi.c

[Rasmus Villemoes]

Simplify the use of the ROM API by using the wrappers that take care
of saving/restoring gd and computing the xor value. This makes the
generated code smaller and the C code easier to read.

Reviewed-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/mach-imx/spl_imx_romapi.c | 47 +++++++-----------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

diff --git a/arch/arm/mach-imx/spl_imx_romapi.c b/arch/arm/mach-imx/spl_imx_romapi.c
index c47f5a6bdb..cc3c1251dc 100644
--- a/arch/arm/mach-imx/spl_imx_romapi.c
+++ b/arch/arm/mach-imx/spl_imx_romapi.c
@@ -18,15 +18,11 @@ 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);
+	ret = rom_api_download_image(buf, offset, size);
 
 	if (ret == ROM_API_OKAY)
 		return size;
@@ -73,21 +69,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");
@@ -102,9 +92,7 @@ static int spl_romapi_load_image_seekable(struct spl_image_info *spl_image,
 	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,
-					((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",
@@ -251,7 +239,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;
@@ -260,9 +247,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");
@@ -272,9 +257,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");
@@ -294,8 +277,7 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
 	}
 
 	if (p - phdr < img_header_size()) {
-		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");
@@ -317,9 +299,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;
 
@@ -341,9 +321,7 @@ static int spl_romapi_load_image_stream(struct spl_image_info *spl_image,
 
 	printf("Download %d, Total size %d\n", imagesize, total);
 
-	ret = g_rom_api->download_image(p, 0, imagesize,
-					((uintptr_t)p) ^ imagesize);
-	set_gd(pgd);
+	ret = rom_api_download_image(p, 0, imagesize);
 	if (ret != ROM_API_OKAY)
 		printf("ROM download failure %d\n", imagesize);
 
@@ -362,13 +340,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

[PATCH v2 5/6] imx8m: soc.c: use rom_api_query_boot_infor() wrapper

[Rasmus Villemoes]

Reviewed-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/mach-imx/imx8m/soc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
index 59335356b5..d5eb50143c 100644
--- a/arch/arm/mach-imx/imx8m/soc.c
+++ b/arch/arm/mach-imx/imx8m/soc.c
@@ -602,16 +602,13 @@ struct rom_api *g_rom_api = (struct rom_api *)0x980;
 
 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");
-- 
2.31.1

[PATCH v2 6/6] imx8ulp: soc.c: use rom_api_query_boot_infor() wrapper

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/mach-imx/imx8ulp/soc.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-imx/imx8ulp/soc.c b/arch/arm/mach-imx/imx8ulp/soc.c
index 35020c9714..c0f0df356e 100644
--- a/arch/arm/mach-imx/imx8ulp/soc.c
+++ b/arch/arm/mach-imx/imx8ulp/soc.c
@@ -36,16 +36,13 @@ struct rom_api *g_rom_api = (struct rom_api *)0x1980;
 
 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");
@@ -91,15 +88,12 @@ __weak int board_mmc_get_env_dev(int devno)
 
 int mmc_get_env_dev(void)
 {
-	volatile gd_t *pgd = gd;
 	int ret;
 	u32 boot;
 	u16 boot_type;
 	u8 boot_instance;
 
-	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

[PATCH v2 5/6] imx8m: soc.c: use rom_api_query_boot_infor() wrapper

[sbabic]

> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
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
=====================================================================

[PATCH v2 3/6] imx8: add rom api wrappers

[sbabic]

> The ROM API is thoroughly undocumented, but apparently passing the xor
> of the real arguments as an extra argument is required [1]. Also, we
> need to do the "save gd/restore gd" dance. These are both error-prone,
> and lead to a lot of code duplication.
> Since both imx8m[np] and imx8ulp SOCs have this, add a separate
> translation unit which is included precisely when the new
> CONFIG_IMX8_ROMAPI symbol is set, which provide convenience wrappers
> that take care of computing the xor value as well as doing the gd
> dance, and that thus have a more intuitive API. Subsequent patches
> will make use of these to reduce boilerplate.
> [1] One wonders, for example, if the check is only applied to the
> lower 32 bits, or if we're implicitly relying on all 64-bit pointer
> values we're passing effectively have 0 in the upper 32 bits.
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
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
=====================================================================

[PATCH v2 4/6] imx8: use ROM API wrappers in spl_imx_romapi.c

[sbabic]

> Simplify the use of the ROM API by using the wrappers that take care
> of saving/restoring gd and computing the xor value. This makes the
> generated code smaller and the C code easier to read.
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
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
=====================================================================

[PATCH v2 2/6] imx8: sys_proto.h: change guard logic around ROM API

[sbabic]

> This exposes the struct rom_api, the g_rom_api variable declaration
> and the associated #defines to slightly fewer boards: namely, those
> IMX8M which are not IMX8MN or IMX8MP. But the latter two are the only
> IMX8M* ones where the g_rom_api variable is defined (in imx8m/soc.c),
> so that should be fine.
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
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
=====================================================================

[PATCH v2 6/6] imx8ulp: soc.c: use rom_api_query_boot_infor() wrapper

[sbabic]

> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
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
=====================================================================

[PATCH v2 1/6] imx8: add hidden IMX8_ROMAPI Kconfig symbol

[sbabic]

> In order not to repeat the IMX8MN || IMX8MP || IMX8ULP logic in
> multiple places where we need to know if the SOC exposes the ROM API,
> add a "def_bool y" Kconfig symbol.
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
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
=====================================================================