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

[Sascha Hauer <sha@pengutronix.de>]

On Wed, Jan 11, 2023 at 08:51:14AM +0100, Ahmad Fatoum wrote:
> 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>
> ---
> I wanted to send this along with an option to disable ATF via Kconfig,
> but that will need us to touch all boards and maybe replace current_el()
> checks with comparison whether we run in DRAM to avoid renewed setup.
> 
> I may look later into this, but this patch may still be useful to have.
> ---
>  arch/arm/mach-imx/Makefile              |  4 ++--
>  arch/arm/mach-imx/bootrom-cmd.c         | 14 ++++++++++++--
>  arch/arm/mach-imx/include/mach/romapi.h |  4 ++++
>  arch/arm/mach-imx/romapi.c              | 24 ++++++++++++++++--------
>  include/zero_page.h                     |  2 +-
>  5 files changed, 35 insertions(+), 13 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 0238d09b169f..c891fd2e0640 100644
> --- a/arch/arm/mach-imx/bootrom-cmd.c
> +++ b/arch/arm/mach-imx/bootrom-cmd.c
> @@ -8,6 +8,7 @@
>  #include <linux/bitfield.h>
>  #include <mach/imx8m-regs.h>
>  #include <mach/xload.h>
> +#include <mach/romapi.h>
>  #include <asm/barebox-arm.h>
>  
>  /* i.MX7 and later ID field is swapped compared to i.MX6 */
> @@ -51,6 +52,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,11 +182,17 @@ 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;
> +	const u32 *rom_log_addr;
>  	bool log = false;
>  	int ret, opt;
>  
> +	if (current_el() == 3) {
> +		rom_log_addr = __imx8m_get_bootrom_log();
> +	} else {
> +		const struct imx_scratch_space *scratch = arm_mem_scratch_get();
> +		rom_log_addr = scratch->bootrom_log;
> +	}

Can we put this into a imx8m_get_bootrom_log() in romapi.c?

That way we have all code handling the places where the bootrom log is
stored together in one file and do not need to export a __ prefixed
function.

> +/* only usable in EL3 */
> +const u32 *__imx8m_get_bootrom_log(void);


>  	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;
> +}
> +
> +void imx8m_save_bootrom_log(void *dest)
> +{
> +	if (!IS_ENABLED(CONFIG_IMX_SAVE_BOOTROM_LOG)) {
> +		pr_debug("skipping bootrom log saving\n");
>  		return;
>  	}
>  
> -	memcpy(dest, (u32 *)rom_log_addr, 128 * sizeof(u32));
> +	memcpy(dest, __imx8m_get_bootrom_log(), 128 * sizeof(u32));

__imx8m_get_bootrom_log() may return NULL. You should check for that
before passing it into memcpy.

Sascha

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