[EXT] Re: [PATCH v4 01/16] crypto/fsl: Add support for CAAM Job ring driver model

[Simon Glass]

Hi Gaurav,

On Mon, 29 Nov 2021 at 00:25, Gaurav Jain <gaurav.jain at nxp.com> wrote:
>
> Hello Simon
>
> > -----Original Message-----
> > From: Simon Glass <sjg at chromium.org>
> > Sent: Thursday, November 25, 2021 5:42 AM
> > To: Gaurav Jain <gaurav.jain at nxp.com>
> > Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Stefano Babic
> > <sbabic at denx.de>; Fabio Estevam <festevam at gmail.com>; Peng Fan
> > <peng.fan at nxp.com>; Priyanka Jain <priyanka.jain at nxp.com>; Ye Li
> > <ye.li at nxp.com>; Horia Geanta <horia.geanta at nxp.com>; Ji Luo
> > <ji.luo at nxp.com>; Franck Lenormand <franck.lenormand at nxp.com>; Silvano Di
> > Ninno <silvano.dininno at nxp.com>; Sahil Malhotra <sahil.malhotra at nxp.com>;
> > Pankaj Gupta <pankaj.gupta at nxp.com>; Varun Sethi <V.Sethi at nxp.com>; dl-
> > uboot-imx <uboot-imx at nxp.com>; Shengzhou Liu <shengzhou.liu at nxp.com>;
> > Mingkai Hu <mingkai.hu at nxp.com>; Rajesh Bhagat <rajesh.bhagat at nxp.com>;
> > Meenakshi Aggarwal <meenakshi.aggarwal at nxp.com>; Wasim Khan
> > <wasim.khan at nxp.com>; Alison Wang <alison.wang at nxp.com>; Pramod
> > Kumar <pramod.kumar_1 at nxp.com>; Andy Tang <andy.tang at nxp.com>;
> > Adrian Alonso <adrian.alonso at nxp.com>; Vladimir Oltean <olteanv at gmail.com>
> > Subject: Re: [EXT] Re: [PATCH v4 01/16] crypto/fsl: Add support for CAAM Job
> > ring driver model
> >
> > Caution: EXT Email
> >
> > Hi Gaurav,
> >
> > On Mon, 8 Nov 2021 at 02:30, Gaurav Jain <gaurav.jain at nxp.com> wrote:
> > >
> > > Hello Simon
> > >
> > > > -----Original Message-----
> > > > From: Simon Glass <sjg at chromium.org>
> > > > Sent: Tuesday, November 2, 2021 8:26 PM
> > > > To: Gaurav Jain <gaurav.jain at nxp.com>
> > > > Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Stefano Babic
> > > > <sbabic at denx.de>; Fabio Estevam <festevam at gmail.com>; Peng Fan
> > > > <peng.fan at nxp.com>; Priyanka Jain <priyanka.jain at nxp.com>; Ye Li
> > > > <ye.li at nxp.com>; Horia Geanta <horia.geanta at nxp.com>; Ji Luo
> > > > <ji.luo at nxp.com>; Franck Lenormand <franck.lenormand at nxp.com>;
> > > > Silvano Di Ninno <silvano.dininno at nxp.com>; Sahil Malhotra
> > > > <sahil.malhotra at nxp.com>; Pankaj Gupta <pankaj.gupta at nxp.com>; Varun
> > > > Sethi <V.Sethi at nxp.com>; dl- uboot-imx <uboot-imx at nxp.com>;
> > > > Shengzhou Liu <shengzhou.liu at nxp.com>; Mingkai Hu
> > > > <mingkai.hu at nxp.com>; Rajesh Bhagat <rajesh.bhagat at nxp.com>;
> > > > Meenakshi Aggarwal <meenakshi.aggarwal at nxp.com>; Wasim Khan
> > > > <wasim.khan at nxp.com>; Alison Wang <alison.wang at nxp.com>; Pramod
> > > > Kumar <pramod.kumar_1 at nxp.com>; Andy Tang <andy.tang at nxp.com>;
> > > > Adrian Alonso <adrian.alonso at nxp.com>; Vladimir Oltean
> > > > <olteanv at gmail.com>
> > > > Subject: [EXT] Re: [PATCH v4 01/16] crypto/fsl: Add support for CAAM
> > > > Job ring driver model
> > > >
> > > > Caution: EXT Email
> > > >
> > > > Hi Gaurav,
> > > >
> > > > On Tue, 26 Oct 2021 at 00:56, Gaurav Jain <gaurav.jain at nxp.com> wrote:
> > > > >
> > > > > added device tree support for job ring driver.
> > > > > sec is initialized based on job ring information processed from
> > > > > device tree.
> > > > >
> > > > > Signed-off-by: Gaurav Jain <gaurav.jain at nxp.com>
> > > > > Reviewed-by: Ye Li <ye.li at nxp.com>
> > > > > ---
> > > > >  cmd/Kconfig                 |   1 +
> > > > >  drivers/crypto/fsl/Kconfig  |   7 +
> > > > >  drivers/crypto/fsl/Makefile |   4 +-
> > > > >  drivers/crypto/fsl/jr.c     | 318 ++++++++++++++++++++++++------------
> > > > >  drivers/crypto/fsl/jr.h     |  14 ++
> > > > >  5 files changed, 234 insertions(+), 110 deletions(-)
> > > >
> > > > You should not have CONFIG_ARCH_IMX8 in a driver. Things like that
> > > > should be handled by using a different compatible string. Also please use the
> > livetree API.
> > > >
> > > > I asked about the use of MISC as a uclass earlier. It seems that
> > > > this device provides random numbers and perhaps hashing? It is hard
> > > > to know since I am not sure where the documentation is in this
> > > > series. It seems odd to be modelled as a MISC device. My
> > > > understanding is that there are problems with the size in SPL if a different
> > UCLASS is used.
> > > > Is that correct? I asked about of-platdata but I didn't see any comment on
> > that.
> > > >
> > > > This does seem to be a significant clean-up even as it is, though,
> > > > so these thoughts could be looked at after this series is applied.
> > > >
> > > Kernel dtb uses single compatible string for all imx platforms.
> > > Adding a different compatible string will introduce a difference with kernel dts
> > nodes.
> >
> > That sounds like a bug?
> In jr.c,  for IMX8 we need to do only Jr initialization and skipping RNG instantiation as it is already done.
> This is achieved using CONFIG_ARCH_IMX8. Are you suggesting to use different macro?

Don't add #ifdefs to drivers. Use a compatible string instead.

>
> >
> > > Will update the driver with livetree API in next version of this series.
> > >
> > > RNG instantiation is done as part of caam initialization. hwrng_generate()
> > function is added by me to generate random number via caam rng.
> > > I am not sure where to document but cover letter was updated.
> > >
> > > using OF_PLATDATA also have memory issues in SPL. It needs 15KB more
> > memory compared to no driver model in SPL. We don't have enough OCRAM on
> > few platforms.
> > > SPL size issue is not caused by MISC UCLASS but it is due to limited OCRAM.
> >
> > That's a very large increase. Can you point me to the tree which enables that so I
> > can take a look? With of-platadata the overhead should be much smaller than
> > that, perhaps 4KB.
>
> 1. Tried imx8mm_evk_defconfig with downstream uboot, which results in 15K increase in u-boot-spl.bin  when enable OF_PLATDATA.

Can you please push a patch or push your tree somewhere? That board
already uses SPL_OF_CONTROL so I cannot fathom why enabling
SPL_OF_PLATDATA as well would increase the size.

> 2. then tried imx8mq_evk_defconfig with upstream. Lot of build errors reported with OF_PLATDATA. Just commented and build, which resulted in 11K increase in u-boot-spl.bin(77K to 88K).

Well let's ignore that one for now, for this issue.

Regards,
Simon