Re: [PATCH v2 7/8] board: ti: j721e: During resume spl restores TF-A and DM-Firmware

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3841 bytes --]

On Tue, Nov 07, 2023 at 05:18:01PM +0100, Thomas Richard wrote:

> During the boot a copy of DM-Firmware is done in a reserved memory
> area before it starts.
> When resuming, R5 SPL uses this copy of DM-Firmware instead of the fit
> image.
> TF-A which saved itself in this same memory area, is restored in
> SRAM by R5 SPL.
> 
> Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> ---
> 
> Changes in v2:
> - Check if TF-A is running in DRAM, if yes no need to restore it
> - Remove BL31_START macro, and get TF-A start address from the fit image
> 
>  arch/arm/mach-k3/common.c                 | 48 ++++++++++++++++++++++-
>  arch/arm/mach-k3/include/mach/j721e_spl.h | 28 +++++++++++++
>  arch/arm/mach-k3/sysfw-loader.c           |  9 +++--
>  3 files changed, 81 insertions(+), 4 deletions(-)

One problem here is we aren't updating something under doc/, possibly
doc/board/ti/k3.rst unless you want to start populating something
entirely new under doc/develop/. And then using kernel-doc comments here
and including the code in the rst.  And so this is a global comment to
the series as well, and I'll leave sorting out when/where to introduce
that part up to your discretion.

[snip]
> diff --git a/arch/arm/mach-k3/include/mach/j721e_spl.h b/arch/arm/mach-k3/include/mach/j721e_spl.h
> index e8947917a6..8e0f141ed6 100644
> --- a/arch/arm/mach-k3/include/mach/j721e_spl.h
> +++ b/arch/arm/mach-k3/include/mach/j721e_spl.h
> @@ -42,4 +42,32 @@
>  #define K3_PRIMARY_BOOTMODE		0x0
>  #define K3_BACKUP_BOOTMODE		0x1
>  
> +/* Starting buffer address is 1MB before the stack address in DDR */
> +#define BUFFER_ADDR (CONFIG_SPL_STACK_R_ADDR - SZ_1M)

Assuming this is the full level of configurability we need to
BUFFER_ADDR please add some Kconfig logic to make sure we can't not set
CONFIG_SPL_STACK_R_ADDR. That might take a little trial-and-error and
if we just cannot enforce this via Kconfig, let me know and I'll think
about what we want to do instead next.

> +/* This is actually the whole size of the SRAM */
> +#define BL31_SIZE    0x20000

If this is all of SRAM then is there not already a define?

> +/* This address belongs to a reserved memory region for the point of view of
> + * Linux, U-boot SPL must use the same address to restore TF-A and resume
> + * entry point address
> + */
> +#define LPM_SAVE		0xA5000000
> +#define LPM_BL31_SAVE		LPM_SAVE
> +#define LPM_BL31_RESUME_SAVE	LPM_BL31_SAVE + BL31_SIZE
> +#define LPM_BL31_START_SAVE	LPM_BL31_RESUME_SAVE + __SIZEOF_POINTER__
> +#define LPM_DM_SAVE		LPM_BL31_START_SAVE  + __SIZEOF_POINTER__

Is any of this somewhere in the devicetree as well? I'm not saying "and
so pull it from that!" is a must, but I'd like to at least know "that
would be horrible to do", and fall back to having this needed
co-operation documented somewhere and ideally somewhere / somehow that
getting the values wrong is clear. Debugging resume failures at run-time
is much more painful than build time.

> +/* Check if the copy of TF-A and DM-Firmware in DRAM does not overlap an
> + * over memory section.
> + * The resume address of TF-A is also saved in DRAM.
> + * At build time we don't know the DM-Firmware size, so we keep 512k to
> + * save it.
> + */
> +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_TARGET_J7200_R5_EVM)
> +#if ((LPM_DM_SAVE + SZ_512K) > BUFFER_ADDR)
> +#error Not enough space to save DM-Firmware, TF-A and context for S2R
> +#endif
> +#endif

This is really the kind of thing I'd like to see enforced via Kconfig if
at all possible. Sometimes yes we just have to CPP and #error, I just
want to know we can't.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]