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

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

Hello Gaurav,

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Gaurav Jain
> Sent: Thursday, June 9, 2022 3:13 PM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.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: RE: [EXT] RE: [PATCH] i.MX8M: crypto: disable JR0 in SPL, U-Boot
> 
> 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.

Great! Then I think DTB part will not be required in this patch.

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

This shall be converted to config option, as each SoC family defines this
now differently.

Ideally, only JR ID shall be provided by config option, ADDR and OFFSET
can then be deducted based on the index.

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

Once config option is introduced, it can be used here instead of macro.
Reserved JR ID can be used to deduct both ADDR and OFFSET parameters.
Then this re-use would not be necessary.

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

See suggestions above.

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

This will not be necessary, and shall rather be replaced by JR ID config option.

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

-- andrey