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

[Gaurav Jain <gaurav.jain@nxp.com>]

Hello Andrey

> -----Original Message-----
> From: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> Sent: Wednesday, June 8, 2022 8:31 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>; u-boot@lists.denx.de; Stefano Babic
> <sbabic@denx.de>; Fabio Estevam <festevam@gmail.com>; Tommaso Merciai
> <tommaso.merciai@amarulasolutions.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>; Michael Walle <michael@walle.cc>
> Cc: dl-uboot-imx <uboot-imx@nxp.com>
> Subject: [EXT] RE: [PATCH] i.MX8M: crypto: disable JR0 in SPL, U-Boot
> 
> Caution: EXT Email
> 
> 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.

Ok. Fabio has already submitted a patch for this.
> 
> >  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?

I reused the old macro to define for JR1. 
How you want to simplify?

Regards
Gaurav Jain
> 
> >  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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kern
> el.org%2Fu-
> boot%2FVI1PR04MB53426E0295D8E2508BA01302E7089%40VI1PR04MB5342.e
> urprd04.prod.outlook.com%2F&amp;data=05%7C01%7Cgaurav.jain%40nxp.co
> m%7Cc387babc3fed40edb32f08da495fa3f4%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C637902972380396116%7CUnknown%7CTWFpbGZsb3d8ey
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 3000%7C%7C%7C&amp;sdata=WETAijFtyFGHnrimKkeT4jICk0OQSXfMbRb8vWsK
> Mp0%3D&amp;reserved=0
> -- andrey