RE: [PATCH] i.MX8M: crypto: disable JR0 in SPL, U-Boot

[ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>]

Hello Gaurav,

Cc: Michael Walle here.

I guess this is a re-incarnation of the previous discussions we had
regarding the JR reservation, see [1].

> -----Original Message-----
> From: Gaurav Jain <gaurav.jain@nxp.com>
> Sent: Wednesday, June 8, 2022 3:34 PM
> To: u-boot@lists.denx.de; Stefano Babic <sbabic@denx.de>; Fabio Estevam
> <festevam@gmail.com>; Tommaso Merciai <tommaso.merciai@amarulasolutions.com>;
> ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; Michael Trimarchi
> <michael@amarulasolutions.com>; Marek Vasut <marex@denx.de>; Simon Glass
> <sjg@chromium.org>; Patrick Delaunay <patrick.delaunay@foss.st.com>; Stefan Roese
> <sr@denx.de>; Horia Geanta <horia.geanta@nxp.com>; Pankaj Gupta
> <pankaj.gupta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Ye Li <ye.li@nxp.com>
> Cc: NXP i . MX U-Boot Team <uboot-imx@nxp.com>; Gaurav Jain <gaurav.jain@nxp.com>
> Subject: [PATCH] i.MX8M: crypto: disable JR0 in SPL, U-Boot
> 
> disabled use of JR0 in SPL and uboot, as JR0 is reserved
> for HAB in TF-A.
> 
> Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> ---
>  arch/arm/dts/imx8mm-evk-u-boot.dtsi        |  1 +
>  arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi   |  1 +
>  arch/arm/dts/imx8mp-evk-u-boot.dtsi        |  1 +
>  arch/arm/dts/imx8mq-evk-u-boot.dtsi        |  4 ++++

Shall those DTB changes be sync'd with Kernel?

Now that the JR0 reservation is done in both upstream and
downstream TF-A - Kernel would fail to initialize the JR0.

This is what Fabio just noted and posted as a comment. :-)

I suggest that this is submitted into Kernel, and then picked
up during the next DTB re-sync.

>  arch/arm/include/asm/arch-imx8m/imx-regs.h |  1 +
>  drivers/crypto/fsl/jr.c                    | 14 +++++++++++---
>  scripts/config_whitelist.txt               |  1 +
>  7 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/dts/imx8mm-evk-u-boot.dtsi b/arch/arm/dts/imx8mm-evk-u-
> boot.dtsi
> index e9fbf7b802..8cd37b5205 100644
> --- a/arch/arm/dts/imx8mm-evk-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mm-evk-u-boot.dtsi
> @@ -74,6 +74,7 @@
> 
>  &sec_jr0 {
>         u-boot,dm-spl;
> +       status = "disabled";
>  };
> 
>  &sec_jr1 {
> diff --git a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-ddr4-
> evk-u-boot.dtsi
> index 4d0ecb07d4..0c31f2737a 100644
> --- a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
> @@ -114,6 +114,7 @@
> 
>  &sec_jr0 {
>         u-boot,dm-spl;
> +       status = "disabled";
>  };
> 
>  &sec_jr1 {
> diff --git a/arch/arm/dts/imx8mp-evk-u-boot.dtsi b/arch/arm/dts/imx8mp-evk-u-
> boot.dtsi
> index f43eb6238d..28dce55fb9 100644
> --- a/arch/arm/dts/imx8mp-evk-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mp-evk-u-boot.dtsi
> @@ -77,6 +77,7 @@
> 
>  &sec_jr0 {
>         u-boot,dm-spl;
> +       status = "disabled";
>  };
> 
>  &sec_jr1 {
> diff --git a/arch/arm/dts/imx8mq-evk-u-boot.dtsi b/arch/arm/dts/imx8mq-evk-u-
> boot.dtsi
> index 67da69a2eb..37364eb6b4 100644
> --- a/arch/arm/dts/imx8mq-evk-u-boot.dtsi
> +++ b/arch/arm/dts/imx8mq-evk-u-boot.dtsi
> @@ -18,3 +18,7 @@
>  &uart1 {
>         u-boot,dm-spl;
>  };
> +
> +&sec_jr0 {
> +       status = "disabled";
> +};
> diff --git a/arch/arm/include/asm/arch-imx8m/imx-regs.h
> b/arch/arm/include/asm/arch-imx8m/imx-regs.h
> index 1da75528d4..e6e2974df3 100644
> --- a/arch/arm/include/asm/arch-imx8m/imx-regs.h
> +++ b/arch/arm/include/asm/arch-imx8m/imx-regs.h
> @@ -89,6 +89,7 @@
>  #define CONFIG_SYS_FSL_SEC_ADDR         (CAAM_IPS_BASE_ADDR + \
>                                          CONFIG_SYS_FSL_SEC_OFFSET)
>  #define CONFIG_SYS_FSL_JR0_OFFSET       (0x1000)
> +#define CONFIG_SYS_FSL_JR1_OFFSET      (0x2000)
>  #define CONFIG_SYS_FSL_JR0_ADDR         (CONFIG_SYS_FSL_SEC_ADDR + \
>                                          CONFIG_SYS_FSL_JR0_OFFSET)
>  #define CONFIG_SYS_FSL_MAX_NUM_OF_SEC   1
> diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
> index acd29924f7..66dd9cf365 100644
> --- a/drivers/crypto/fsl/jr.c
> +++ b/drivers/crypto/fsl/jr.c
> @@ -44,9 +44,17 @@ struct udevice *caam_dev;
>  #define SEC_ADDR(idx)  \
>         (ulong)((CONFIG_SYS_FSL_SEC_ADDR + sec_offset[idx]))
> 
> -#define SEC_JR0_ADDR(idx)      \
> +#ifndef CONFIG_IMX8M
> +#define SEC_JR_ADDR(idx)       \
>         (ulong)(SEC_ADDR(idx) + \
>          (CONFIG_SYS_FSL_JR0_OFFSET - CONFIG_SYS_FSL_SEC_OFFSET))
> +#define JR_ID 0
> +#else
> +#define SEC_JR_ADDR(idx)       \
> +       (ulong)(SEC_ADDR(idx) + \
> +       (CONFIG_SYS_FSL_JR1_OFFSET - CONFIG_SYS_FSL_SEC_OFFSET))
> +#define JR_ID 1
> +#endif

I believe this whole macro can be simplified, isn't it?

>  struct caam_regs caam_st;
>  #endif
> 
> @@ -685,8 +693,8 @@ int sec_init_idx(uint8_t sec_idx)
>         caam = dev_get_priv(caam_dev);
>  #else
>         caam_st.sec = (void *)SEC_ADDR(sec_idx);
> -       caam_st.regs = (struct jr_regs *)SEC_JR0_ADDR(sec_idx);
> -       caam_st.jrid = 0;
> +       caam_st.regs = (struct jr_regs *)SEC_JR_ADDR(sec_idx);
> +       caam_st.jrid = JR_ID;
>         caam = &caam_st;
>  #endif
>  #if CONFIG_IS_ENABLED(OF_CONTROL)
> diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
> index cecdda6781..b99aeacbc4 100644
> --- a/scripts/config_whitelist.txt
> +++ b/scripts/config_whitelist.txt
> @@ -1040,6 +1040,7 @@ CONFIG_SYS_FSL_IFC_LE
>  CONFIG_SYS_FSL_ISBC_VER
>  CONFIG_SYS_FSL_JR0_ADDR
>  CONFIG_SYS_FSL_JR0_OFFSET
> +CONFIG_SYS_FSL_JR1_OFFSET
>  CONFIG_SYS_FSL_LS1_CLK_ADDR
>  CONFIG_SYS_FSL_LSCH3_SERDES_ADDR
>  CONFIG_SYS_FSL_MAX_NUM_OF_SEC
> --
> 2.25.1


Link: [1]: https://lore.kernel.org/u-boot/VI1PR04MB53426E0295D8E2508BA01302E7089@VI1PR04MB5342.eurprd04.prod.outlook.com/
-- andrey