[PATCH v2 1/2] ARM: i.MX8M: bootrom-cmd: clean up pointer casting

[PATCH v2 1/2] ARM: i.MX8M: bootrom-cmd: clean up pointer casting

[Ahmad Fatoum]

At first glance:

	const u32 *rom_log_addr;
	kstrtoul(optarg, 0, (ulong *)&rom_log_addr) with

looks like it would conflate u32/ulong. While the code is fine,
it is better to just use the correct types and forego the case.

While at it, let's guard against the ROM log pointer being NULL
as well.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - new patch
---
 arch/arm/mach-imx/bootrom-cmd.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-imx/bootrom-cmd.c b/arch/arm/mach-imx/bootrom-cmd.c
index 0238d09b169f..63197e02ae2c 100644
--- a/arch/arm/mach-imx/bootrom-cmd.c
+++ b/arch/arm/mach-imx/bootrom-cmd.c
@@ -51,6 +51,9 @@ static int imx8m_bootrom_decode_log(const u32 *rom_log)
 {
 	int i;
 
+	if (!rom_log)
+		return -ENODATA;
+
 	for (i = 0; i < 128; i++) {
 		u8 event_id = FIELD_GET(ROM_EVENT_FORMAT_V1_ID, rom_log[i]);
 		u8 event_id_idx = FIELD_GET(ROM_EVENT_FORMAT_V1_ID_IDX, rom_log[i]);
@@ -178,18 +181,19 @@ static int imx8m_bootrom_decode_log(const u32 *rom_log)
 
 static int do_bootrom(int argc, char *argv[])
 {
-	const struct imx_scratch_space *scratch = arm_mem_scratch_get();
-	const u32 *rom_log_addr = scratch->bootrom_log;
+	union {
+		const u32 *ptr;
+		ulong addr;
+	} rom_log = { NULL };
 	bool log = false;
 	int ret, opt;
 
 	while((opt = getopt(argc, argv, "la:")) > 0) {
 		switch(opt) {
 		case 'a':
-			ret = kstrtoul(optarg, 0, (ulong *)&rom_log_addr);
+			ret = kstrtoul(optarg, 0, &rom_log.addr);
 			if (ret)
 				return ret;
-			rom_log_addr = (const u32 *)rom_log_addr;
 		case 'l':
 			log = true;
 			break;
@@ -198,8 +202,13 @@ static int do_bootrom(int argc, char *argv[])
 		}
 	}
 
+	if (!rom_log.addr) {
+		const struct imx_scratch_space *scratch = arm_mem_scratch_get();
+		rom_log.ptr = scratch->bootrom_log;
+	}
+
 	if (log)
-		return imx8m_bootrom_decode_log(rom_log_addr);
+		return imx8m_bootrom_decode_log(rom_log.ptr);
 
 	return COMMAND_ERROR_USAGE;
 }
-- 
2.30.2

[PATCH v2 2/2] ARM: i.MX8M: bootrom: access OCRAM directly if running in EL3

[Ahmad Fatoum]

It's straight-forward to hack barebox PBL to skip ATF installation
and to start barebox proper in EL3. This can be useful for debugging
and may in future be just a Kconfig option like we now have with
Rockchip.

In such a configuration, we don't need to copy the ROM log out of OCRAM,
because there's no ATF installed there that might overwrite it.
Therefore, just directly access the event log pointer in BootROM if
running in EL3. As 0x9e0 is in the first zero page, we use the function
in zero_page.h to temporarily disable traps in the zero page. We don't
need to do that in PBL though as even early MMU will be enabled later.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - guard against NULL pointer dereference in imx8m_save_bootrom_log
    (Sascha)
  - move EL2/3 specifics into imx8m_get_bootrom_log and out of
    bootrom-cmd.c (Sascha)
---
 arch/arm/mach-imx/Makefile              |  4 +--
 arch/arm/mach-imx/bootrom-cmd.c         |  9 ++---
 arch/arm/mach-imx/include/mach/romapi.h |  2 ++
 arch/arm/mach-imx/romapi.c              | 46 ++++++++++++++++++++-----
 include/zero_page.h                     |  2 +-
 5 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
index cc834fed7be7..5d70e79b57f1 100644
--- a/arch/arm/mach-imx/Makefile
+++ b/arch/arm/mach-imx/Makefile
@@ -18,8 +18,8 @@ lwl-$(CONFIG_ARCH_IMX6) += imx6-mmdc.o
 obj-$(CONFIG_ARCH_IMX7) += imx7.o
 obj-$(CONFIG_ARCH_VF610) += vf610.o
 obj-pbl-$(CONFIG_ARCH_IMX8M) += imx8m.o
-lwl-$(CONFIG_ARCH_IMX8M) += atf.o romapi.o
-obj-pbl-$(CONFIG_ARCH_IMX8M) += tzasc.o
+lwl-$(CONFIG_ARCH_IMX8M) += atf.o
+obj-pbl-$(CONFIG_ARCH_IMX8M) += romapi.o tzasc.o
 obj-$(CONFIG_IMX_IIM)	+= iim.o
 obj-$(CONFIG_NAND_IMX) += nand.o
 lwl-$(CONFIG_ARCH_IMX_EXTERNAL_BOOT_NAND) += external-nand-boot.o
diff --git a/arch/arm/mach-imx/bootrom-cmd.c b/arch/arm/mach-imx/bootrom-cmd.c
index 63197e02ae2c..35e52b0f92aa 100644
--- a/arch/arm/mach-imx/bootrom-cmd.c
+++ b/arch/arm/mach-imx/bootrom-cmd.c
@@ -7,8 +7,7 @@
 #include <linux/bitops.h>
 #include <linux/bitfield.h>
 #include <mach/imx8m-regs.h>
-#include <mach/xload.h>
-#include <asm/barebox-arm.h>
+#include <mach/romapi.h>
 
 /* i.MX7 and later ID field is swapped compared to i.MX6 */
 #define ROM_EVENT_FORMAT_V0_RES	GENMASK(31, 24)
@@ -202,10 +201,8 @@ static int do_bootrom(int argc, char *argv[])
 		}
 	}
 
-	if (!rom_log.addr) {
-		const struct imx_scratch_space *scratch = arm_mem_scratch_get();
-		rom_log.ptr = scratch->bootrom_log;
-	}
+	if (!rom_log.addr)
+		rom_log.ptr = imx8m_get_bootrom_log();
 
 	if (log)
 		return imx8m_bootrom_decode_log(rom_log.ptr);
diff --git a/arch/arm/mach-imx/include/mach/romapi.h b/arch/arm/mach-imx/include/mach/romapi.h
index d22ba7259dd0..2cb8e37ce9b7 100644
--- a/arch/arm/mach-imx/include/mach/romapi.h
+++ b/arch/arm/mach-imx/include/mach/romapi.h
@@ -3,6 +3,7 @@
 #define __MACH_IMX_ROMAPI_H
 
 #include <mach/xload.h>
+#include <linux/types.h>
 
 struct rom_api {
 	u16 ver;
@@ -39,6 +40,7 @@ int imx8mn_bootrom_load_image(void);
 
 /* only call after DRAM has been configured */
 void imx8m_save_bootrom_log(void *dst);
+const u32 *imx8m_get_bootrom_log(void);
 
 #define imx8mq_save_bootrom_log() imx8m_save_bootrom_log(imx8mq_scratch_space())
 #define imx8mm_save_bootrom_log() imx8m_save_bootrom_log(imx8mm_scratch_space())
diff --git a/arch/arm/mach-imx/romapi.c b/arch/arm/mach-imx/romapi.c
index 5885fb698ecd..0936c855fd03 100644
--- a/arch/arm/mach-imx/romapi.c
+++ b/arch/arm/mach-imx/romapi.c
@@ -7,6 +7,10 @@
 #include <mach/romapi.h>
 #include <mach/atf.h>
 #include <mach/imx8m-regs.h>
+#include <mach/xload.h>
+#include <asm/barebox-arm.h>
+#include <zero_page.h>
+#include <pbl.h>
 
 static int imx8m_bootrom_load(struct rom_api *rom_api, void *adr, size_t size)
 {
@@ -50,23 +54,49 @@ int imx8mn_bootrom_load_image(void)
 	return imx8mp_bootrom_load_image();
 }
 
+const u32 *imx8m_get_bootrom_log(void)
+{
+	if (current_el() == 3) {
+		ulong rom_log_addr;
+
+		zero_page_access();
+		rom_log_addr = readl(IOMEM(0x9e0));
+		zero_page_faulting();
+
+		if (rom_log_addr < MX8M_OCRAM_BASE_ADDR ||
+		    rom_log_addr >= MX8M_OCRAM_BASE_ADDR + MX8M_OCRAM_MAX_SIZE ||
+		    rom_log_addr & 0x3) {
+			pr_warn("No BootROM log found at address 0x%08lx\n", rom_log_addr);
+			return NULL;
+		}
+
+		return (u32 *)rom_log_addr;
+	}
+
+	if (!IN_PBL) {
+		const struct imx_scratch_space *scratch = arm_mem_scratch_get();
+		return scratch->bootrom_log;
+	}
+
+	return NULL;
+}
+
 void imx8m_save_bootrom_log(void *dest)
 {
-	ulong rom_log_addr;
+	const u32 *rom_log;
 
 	if (!IS_ENABLED(CONFIG_IMX_SAVE_BOOTROM_LOG)) {
 		pr_debug("skipping bootrom log saving\n");
 		return;
 	}
 
-	rom_log_addr = *(u32 *)0x9e0;
-
-	if (rom_log_addr < MX8M_OCRAM_BASE_ADDR ||
-	    rom_log_addr >= MX8M_OCRAM_BASE_ADDR + MX8M_OCRAM_MAX_SIZE ||
-	    rom_log_addr & 0x3) {
-		pr_warn("No BootROM log found at address 0x%08lx\n", rom_log_addr);
+	rom_log = imx8m_get_bootrom_log();
+	if (!rom_log) {
+		pr_warn("bootrom log not found\n");
 		return;
 	}
 
-	memcpy(dest, (u32 *)rom_log_addr, 128 * sizeof(u32));
+	pr_debug("Saving bootrom log to 0x%p\n", dest);
+
+	memcpy(dest, rom_log, 128 * sizeof(u32));
 }
diff --git a/include/zero_page.h b/include/zero_page.h
index 519e65be7628..26a12246f155 100644
--- a/include/zero_page.h
+++ b/include/zero_page.h
@@ -4,7 +4,7 @@
 
 #include <common.h>
 
-#if defined CONFIG_ARCH_HAS_ZERO_PAGE && defined CONFIG_MMU
+#if defined CONFIG_ARCH_HAS_ZERO_PAGE && defined CONFIG_MMU && !defined __PBL__
 
 /*
  * zero_page_faulting - fault when accessing the zero page
-- 
2.30.2

Re: [PATCH v2 1/2] ARM: i.MX8M: bootrom-cmd: clean up pointer casting

[Sascha Hauer]

On Wed, Jan 11, 2023 at 04:28:36PM +0100, Ahmad Fatoum wrote:
> At first glance:
> 
> 	const u32 *rom_log_addr;
> 	kstrtoul(optarg, 0, (ulong *)&rom_log_addr) with
> 
> looks like it would conflate u32/ulong. While the code is fine,
> it is better to just use the correct types and forego the case.
> 
> While at it, let's guard against the ROM log pointer being NULL
> as well.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> v1 -> v2:
>   - new patch
> ---
>  arch/arm/mach-imx/bootrom-cmd.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)

Applied, thanks

Sascha

> 
> diff --git a/arch/arm/mach-imx/bootrom-cmd.c b/arch/arm/mach-imx/bootrom-cmd.c
> index 0238d09b169f..63197e02ae2c 100644
> --- a/arch/arm/mach-imx/bootrom-cmd.c
> +++ b/arch/arm/mach-imx/bootrom-cmd.c
> @@ -51,6 +51,9 @@ static int imx8m_bootrom_decode_log(const u32 *rom_log)
>  {
>  	int i;
>  
> +	if (!rom_log)
> +		return -ENODATA;
> +
>  	for (i = 0; i < 128; i++) {
>  		u8 event_id = FIELD_GET(ROM_EVENT_FORMAT_V1_ID, rom_log[i]);
>  		u8 event_id_idx = FIELD_GET(ROM_EVENT_FORMAT_V1_ID_IDX, rom_log[i]);
> @@ -178,18 +181,19 @@ static int imx8m_bootrom_decode_log(const u32 *rom_log)
>  
>  static int do_bootrom(int argc, char *argv[])
>  {
> -	const struct imx_scratch_space *scratch = arm_mem_scratch_get();
> -	const u32 *rom_log_addr = scratch->bootrom_log;
> +	union {
> +		const u32 *ptr;
> +		ulong addr;
> +	} rom_log = { NULL };
>  	bool log = false;
>  	int ret, opt;
>  
>  	while((opt = getopt(argc, argv, "la:")) > 0) {
>  		switch(opt) {
>  		case 'a':
> -			ret = kstrtoul(optarg, 0, (ulong *)&rom_log_addr);
> +			ret = kstrtoul(optarg, 0, &rom_log.addr);
>  			if (ret)
>  				return ret;
> -			rom_log_addr = (const u32 *)rom_log_addr;
>  		case 'l':
>  			log = true;
>  			break;
> @@ -198,8 +202,13 @@ static int do_bootrom(int argc, char *argv[])
>  		}
>  	}
>  
> +	if (!rom_log.addr) {
> +		const struct imx_scratch_space *scratch = arm_mem_scratch_get();
> +		rom_log.ptr = scratch->bootrom_log;
> +	}
> +
>  	if (log)
> -		return imx8m_bootrom_decode_log(rom_log_addr);
> +		return imx8m_bootrom_decode_log(rom_log.ptr);
>  
>  	return COMMAND_ERROR_USAGE;
>  }
> -- 
> 2.30.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |