Hello Gurav,
I'll Cc: Michael Walle here for the reason described below.
> -----Original Message-----
> From: Gaurav Jain <gaurav.j...@nxp.com>
> Sent: Monday, November 8, 2021 9:21 AM
> To: ZHIZHIKIN Andrey <andrey.zhizhi...@leica-geosystems.com>; Adam Ford
> <aford...@gmail.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stefano Babic
> <sba...@denx.de>; Fabio Estevam <feste...@gmail.com>; Peng Fan
> <peng....@nxp.com>; Simon Glass <s...@chromium.org>; Priyanka Jain
> <priyanka.j...@nxp.com>; Ye Li <ye...@nxp.com>; Horia Geanta
> <horia.gea...@nxp.com>; Ji Luo <ji....@nxp.com>; Franck Lenormand
> <franck.lenorm...@nxp.com>; Silvano Di Ninno <silvano.dini...@nxp.com>;
> Sahil Malhotra <sahil.malho...@nxp.com>; Pankaj Gupta
> <pankaj.gu...@nxp.com>; Varun Sethi <v.se...@nxp.com>; dl-uboot-imx
> <uboot-...@nxp.com>; Shengzhou Liu <shengzhou....@nxp.com>; Mingkai Hu
> <mingkai...@nxp.com>; Rajesh Bhagat <rajesh.bha...@nxp.com>; Meenakshi
> Aggarwal <meenakshi.aggar...@nxp.com>; Wasim Khan
> <wasim.k...@nxp.com>; Alison Wang <alison.w...@nxp.com>; Pramod Kumar
> <pramod.kuma...@nxp.com>; Andy Tang <andy.t...@nxp.com>; Adrian
> Alonso <adrian.alo...@nxp.com>; Vladimir Oltean <olte...@gmail.com>
> Subject: RE: [EXT] Re: [PATCH v4 03/16] i.MX8M: crypto: updated device tree
> for
> supporting DM in SPL
>
>
> Hello Andrey
>
> > -----Original Message-----
> > From: ZHIZHIKIN Andrey <andrey.zhizhi...@leica-geosystems.com>
> > Sent: Wednesday, November 3, 2021 5:05 PM
> > To: Adam Ford <aford...@gmail.com>; Gaurav Jain <gaurav.j...@nxp.com>
> > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stefano Babic
> > <sba...@denx.de>; Fabio Estevam <feste...@gmail.com>; Peng Fan
> > <peng....@nxp.com>; Simon Glass <s...@chromium.org>; Priyanka Jain
> > <priyanka.j...@nxp.com>; Ye Li <ye...@nxp.com>; Horia Geanta
> > <horia.gea...@nxp.com>; Ji Luo <ji....@nxp.com>; Franck Lenormand
> > <franck.lenorm...@nxp.com>; Silvano Di Ninno
> > <silvano.dini...@nxp.com>; Sahil Malhotra <sahil.malho...@nxp.com>;
> > Pankaj Gupta <pankaj.gu...@nxp.com>; Varun Sethi <v.se...@nxp.com>;
> > dl-uboot-imx <uboot-...@nxp.com>; Shengzhou Liu
> > <shengzhou....@nxp.com>; Mingkai Hu <mingkai...@nxp.com>; Rajesh
> > Bhagat <rajesh.bha...@nxp.com>; Meenakshi Aggarwal
> > <meenakshi.aggar...@nxp.com>; Wasim Khan <wasim.k...@nxp.com>;
> Alison
> > Wang <alison.w...@nxp.com>; Pramod Kumar <pramod.kuma...@nxp.com>;
> > Andy Tang <andy.t...@nxp.com>; Adrian Alonso <adrian.alo...@nxp.com>;
> > Vladimir Oltean <olte...@gmail.com>
> > Subject: RE: [EXT] Re: [PATCH v4 03/16] i.MX8M: crypto: updated device
> > tree for supporting DM in SPL
> >
> > Caution: EXT Email
> >
> > Hello Gurav/Adam ,
> >
> > > -----Original Message-----
> > > From: U-Boot <u-boot-boun...@lists.denx.de> On Behalf Of Adam Ford
> > > Sent: Tuesday, November 2, 2021 12:19 PM
> > > To: Gaurav Jain <gaurav.j...@nxp.com>
> > > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stefano Babic
> > > <sba...@denx.de>; Fabio Estevam <feste...@gmail.com>; Peng Fan
> > > <peng....@nxp.com>; Simon Glass <s...@chromium.org>; Priyanka Jain
> > > <priyanka.j...@nxp.com>; Ye Li <ye...@nxp.com>; Horia Geanta
> > > <horia.gea...@nxp.com>; Ji Luo <ji....@nxp.com>; Franck Lenormand
> > > <franck.lenorm...@nxp.com>; Silvano Di Ninno
> > > <silvano.dini...@nxp.com>; Sahil Malhotra <sahil.malho...@nxp.com>;
> > > Pankaj Gupta <pankaj.gu...@nxp.com>; Varun Sethi <v.se...@nxp.com>;
> > > dl-uboot-imx <uboot-...@nxp.com>; Shengzhou Liu
> > > <shengzhou....@nxp.com>; Mingkai Hu <mingkai...@nxp.com>; Rajesh
> > > Bhagat <rajesh.bha...@nxp.com>; Meenakshi Aggarwal
> > > <meenakshi.aggar...@nxp.com>; Wasim Khan <wasim.k...@nxp.com>;
> > Alison
> > > Wang <alison.w...@nxp.com>; Pramod Kumar
> > <pramod.kuma...@nxp.com>;
> > > Andy Tang <andy.t...@nxp.com>; Adrian Alonso
> > > <adrian.alo...@nxp.com>; Vladimir Oltean <olte...@gmail.com>
> > > Subject: Re: [EXT] Re: [PATCH v4 03/16] i.MX8M: crypto: updated
> > > device tree for supporting DM in SPL
> > >
> > >
> > > On Tue, Nov 2, 2021 at 3:17 AM Gaurav Jain <gaurav.j...@nxp.com> wrote:
> > > >
> > > > Hello Adam
> > > >
> > > > > -----Original Message-----
> > > > > From: Adam Ford <aford...@gmail.com>
> > > > > Sent: Monday, November 1, 2021 6:30 PM
> > > > > To: Gaurav Jain <gaurav.j...@nxp.com>
> > > > > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stefano Babic
> > > > > <sba...@denx.de>; Fabio Estevam <feste...@gmail.com>; Peng Fan
> > > > > <peng....@nxp.com>; Simon Glass <s...@chromium.org>; Priyanka
> > > > > Jain <priyanka.j...@nxp.com>; Ye Li <ye...@nxp.com>; Horia
> > > > > Geanta <horia.gea...@nxp.com>; Ji Luo <ji....@nxp.com>; Franck
> > > > > Lenormand <franck.lenorm...@nxp.com>; Silvano Di Ninno
> > > > > <silvano.dini...@nxp.com>; Sahil Malhotra
> > > > > <sahil.malho...@nxp.com>; Pankaj Gupta <pankaj.gu...@nxp.com>;
> > > > > Varun Sethi <v.se...@nxp.com>; dl-uboot-imx <uboot-...@nxp.com>;
> > > > > Shengzhou Liu <shengzhou....@nxp.com>; Mingkai Hu
> > > > > <mingkai...@nxp.com>; Rajesh Bhagat <rajesh.bha...@nxp.com>;
> > > > > Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Wasim Khan
> > > > > <wasim.k...@nxp.com>; Alison Wang <alison.w...@nxp.com>; Pramod
> > > > > Kumar <pramod.kuma...@nxp.com>; Andy Tang <andy.t...@nxp.com>;
> > > > > Adrian Alonso <adrian.alo...@nxp.com>; Vladimir Oltean
> > > > > <olte...@gmail.com>
> > > > > Subject: [EXT] Re: [PATCH v4 03/16] i.MX8M: crypto: updated
> > > > > device tree for supporting DM in SPL
> > > > >
> > > > > Caution: EXT Email
> > > > >
> > > > > On Tue, Oct 26, 2021 at 1:57 AM Gaurav Jain
> > > > > <gaurav.j...@nxp.com>
> > wrote:
> > > > > >
> > > > > > disabled use of JR0 in SPL and uboot, as JR0 is reserved for
> > > > > > secure boot.
> > > > > >
> > > > > > Signed-off-by: Gaurav Jain <gaurav.j...@nxp.com>
> > > > > > Reviewed-by: Ye Li <ye...@nxp.com>
> > > > > > ---
> > > > > > arch/arm/dts/imx8mm-evk-u-boot.dtsi | 18 +++++++++++++++++-
> > > > > > arch/arm/dts/imx8mm.dtsi | 1 +
> > > > > > arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi | 18
> +++++++++++++++++-
> > > > > > arch/arm/dts/imx8mn.dtsi | 1 +
> > > > > > arch/arm/dts/imx8mp-evk-u-boot.dtsi | 18 +++++++++++++++++-
> > > > > > arch/arm/dts/imx8mp.dtsi | 1 +
> > > > > > arch/arm/dts/imx8mq.dtsi | 1 +
> > > > > > 7 files changed, 55 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm/dts/imx8mm-evk-u-boot.dtsi
> > > > > > b/arch/arm/dts/imx8mm-evk-u-boot.dtsi
> > > > > > index 3c75415e8f..40f5cfda9a 100644
> > > > > > --- a/arch/arm/dts/imx8mm-evk-u-boot.dtsi
> > > > > > +++ b/arch/arm/dts/imx8mm-evk-u-boot.dtsi
> > > > > > @@ -1,6 +1,6 @@
> > > > > > // SPDX-License-Identifier: GPL-2.0+
> > > > > > /*
> > > > > > - * Copyright 2019 NXP
> > > > > > + * Copyright 2019, 2021 NXP
> > > > > > */
> > > > > >
> > > > > > #include "imx8mm-u-boot.dtsi"
> > > > > > @@ -72,6 +72,22 @@
> > > > > > u-boot,dm-spl;
> > > > > > };
> > > > > >
> > > > > > +&crypto {
> > > > > > + u-boot,dm-spl;
> > > > > > +};
> > > > > > +
> > > > > > +&sec_jr0 {
> > > > > > + u-boot,dm-spl;
> > > > > > +};
> > > > > > +
> > > > > > +&sec_jr1 {
> > > > > > + u-boot,dm-spl;
> > > > > > +};
> > > > > > +
> > > > > > +&sec_jr2 {
> > > > > > + u-boot,dm-spl;
> > > > > > +};
> > > > > > +
> > > > > > &usdhc1 {
> > > > > > u-boot,dm-spl;
> > > > > > };
> > > > > > diff --git a/arch/arm/dts/imx8mm.dtsi
> > > > > > b/arch/arm/dts/imx8mm.dtsi index b142b80734..009999bf3a 100644
> > > > > > --- a/arch/arm/dts/imx8mm.dtsi
> > > > > > +++ b/arch/arm/dts/imx8mm.dtsi
> > > > > > @@ -824,6 +824,7 @@
> > > > > > compatible =
> > > > > > "fsl,sec-v4.0-job-ring";
> > > > > > reg = <0x1000 0x1000>;
> > > > > > interrupts = <GIC_SPI
> > > > > > 105 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > + status = "disabled";
> > > > >
> > > > > Changing the SoC DTSI files makes future re-synchronizing difficult.
> > > > > If you mark these as disabled in the respective u-boot.dtsi
> > > > > files, it will create less work in the future. You're already
> > > > > enabling a bunch of new features in the respective -u-boot.dtsi
> > > > > files, I would suggest
> > > doing the disable in the same way.
> > > >
> > > > JR0 status is marked as disabled, as it is reserved in ATF and
> > > > cannot be accessed
> > > in SPL and Uboot.
> > > > For JR driver model(new feature) to work, at least one Jobring has
> > > > to be
> > > initialized, which is not possible with JR0.
> > > > JR1 will be initialized.
> > >
> > > I wasn't suggesting that it should not be disabled. I was asking
> > > that it be disabled in the -u-boot.dtsi file instead of the
> > > imx8mm.dtsi file because when the imx8mm.dtsi file gets resync'd
> > > with Linux, it might be lost. Having it done in the - u-boot.dtsi
> > > file instead helps reduce the likelihood of being undone later, and
> > > that is the purpose of the -u-
> > boot.dtsi files.
> >
> > If I may add my 2c here:
> > It seems to me that this patch should be split into at least 2 separate
> > patches:
> > one that disables the JR0 node, and one - for the rest.
> >
> > The reason being: if the JR0 node is reserved for ATF and should not
> > be available
> > - then it should be disabled in the Kernel DTB as well as here.
> >
> > This part is missing in upstream Kernel, so the boot process throws
> > following errors during JR probing:
> > ----
> > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0
> > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0
> > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5
> > ----
> >
> > Disabling the JR0 node in the kernel makes this error go away, and
> > decreases the JR count to 2 which can be observed in the NXP vendor kernel.
> >
> > I suggest you to extract the node disabling from this patch and send it to
> > Kernel.
> > Once accepted in upstream - this change would be picked up by the
> > U-Boot at next DTB re-sync.
> >
> > -- andrey
> >
>
> Removing the JR0 disable change from this patch will cause CAAM failed to
> initialize on iMX8M series.
> As Adam suggested, I will disable the JR0 in the - u-boot.dtsi. I will also
> send a
> patch to kernel for disabling the JRO.
> When U-boot DTB will be synced with kernel, I will remove the JR0 disable
> change
> from - u-boot.dtsi
I'm currently working on dynamic recognition of Job Rings which are reserved
for TZ
at the early boot, there is a patch proposed to address this [1]. I was
planning to re-work
this to also include a small clean-up of the CAAM driver in a way how JRs are
probed.
The patch does not require nodes to be permanently disabled in DTB, rather they
would
need to be omitted from the probing instead.
I was wondering perhaps similar changes can be introduced in the U-Boot, since
it would
allow to account for cases where JRs reserved for specific purposes (e.g. HAB)
can be
dynamically changed and would not require modifications of DTB to align.
In generic case, any JR can be "taken away" into TZ. Those modifications should
be
sync'ed with DTB changes, while having this dynamically recognized would have a
benefit of not touching the DTB at all.
>
> Regards
> Gaurav Jain
>
Link: [1]:
https://lore.kernel.org/lkml/20211104162114.2019509-1-andrey.zhizhi...@leica-geosystems.com/
-- andrey
