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

Sat Jan 8 15:53:16 CET 2022

Hi Gaurav,

On Thu, 6 Jan 2022 at 22:54, Gaurav Jain <gaurav.jain at nxp.com> wrote:
>
> Hi
>
> > -----Original Message-----
> > From: Simon Glass <sjg at chromium.org>
> > Sent: Tuesday, December 28, 2021 2:02 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 v7 01/15] crypto/fsl: Add support for CAAM Job ring
> > driver model
> >
> > Caution: EXT Email
> >
> > Hi Gaurav,
> >
> > On Tue, 7 Dec 2021 at 00:42, 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>
> > > ---
> > >  drivers/crypto/fsl/jr.c | 323
> > > ++++++++++++++++++++++++++--------------
> > >  drivers/crypto/fsl/jr.h |  19 +++
> > >  2 files changed, 231 insertions(+), 111 deletions(-)
> > >
> > > diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c index
> > > 22b649219e..441550f348 100644
> > > --- a/drivers/crypto/fsl/jr.c
> > > +++ b/drivers/crypto/fsl/jr.c
> > > @@ -1,7 +1,7 @@
> > >  // SPDX-License-Identifier: GPL-2.0+
> > >  /*
> > >   * Copyright 2008-2014 Freescale Semiconductor, Inc.
> > > - * Copyright 2018 NXP
> > > + * Copyright 2018, 2021 NXP
> > >   *
> > >   * Based on CAAM driver in drivers/crypto/caam in Linux
> > >   */
> > > @@ -11,7 +11,6 @@
> > >  #include <linux/kernel.h>
> > >  #include <log.h>
> > >  #include <malloc.h>
> > > -#include "fsl_sec.h"
> > >  #include "jr.h"
> > >  #include "jobdesc.h"
> > >  #include "desc_constr.h"
> > > @@ -21,8 +20,11 @@
> > >  #include <asm/cache.h>
> > >  #include <asm/fsl_pamu.h>
> > >  #endif
> > > +#include <dm.h>
> > >  #include <dm/lists.h>
> > >  #include <linux/delay.h>
> > > +#include <dm/root.h>
> > > +#include <dm/device-internal.h>
> >
> > These should go up one line.
> >
> Ok. Will do in next version.
>
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.de
> > nx.de%2Fwiki%2FU-
> > Boot%2FCodingStyle&data=04%7C01%7Cgaurav.jain%40nxp.com%7C021c
> > 004f07e44d796a7408d9c9dc9e41%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> > C0%7C0%7C637762771646492648%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&a
> > mp;sdata=HOzWAz8vqKuavYOZopqZf3iLD5dbenfY2UEF%2BO0h1W4%3D&r
> > eserved=0
> >
> > Re the uclass, this is using MISC. What operations are actually supported? I am
> > still not sure why this doesn't deserve its own uclass.
> >
> Ioctl operation(caam_jr_ioctl) provided by MISC Uclass is sufficient for this driver.
> I do not see a need for introducing a new Uclass to support this driver.

OK, well we can go with that for now.

>
> >
> > [..]
> >
> > > diff --git a/drivers/crypto/fsl/jr.h b/drivers/crypto/fsl/jr.h index
> > > 1047aa772c..c5e5ed5c26 100644
> > > --- a/drivers/crypto/fsl/jr.h
> > > +++ b/drivers/crypto/fsl/jr.h
> > > @@ -1,6 +1,7 @@
> > >  /* SPDX-License-Identifier: GPL-2.0+ */
> > >  /*
> > >   * Copyright 2008-2014 Freescale Semiconductor, Inc.
> > > + * Copyright 2021 NXP
> > >   *
> > >   */
> > >
> > > @@ -8,7 +9,9 @@
> > >  #define __JR_H
> > >
> > >  #include <linux/compiler.h>
> > > +#include "fsl_sec.h"
> > >  #include "type.h"
> > > +#include <misc.h>
> > >
> > >  #define JR_SIZE 4
> > >  /* Timeout currently defined as 10 sec */ @@ -35,12 +38,21 @@
> > >  #define JRSLIODN_SHIFT         0
> > >  #define JRSLIODN_MASK          0x00000fff
> > >
> > > +#define JRDID_MS_PRIM_DID      BIT(0)
> > > +#define JRDID_MS_PRIM_TZ       BIT(4)
> > > +#define JRDID_MS_TZ_OWN                BIT(15)
> > > +
> > >  #define JQ_DEQ_ERR             -1
> > >  #define JQ_DEQ_TO_ERR          -2
> > >  #define JQ_ENQ_ERR             -3
> >
> > Good idea to put brackets around #defines that are a -ve number.
> Will do in next version.
> >
> > >
> > >  #define RNG4_MAX_HANDLES       2
> > >
> > > +enum {
> > > +       /* Run caam jobring descriptor(in buf) */
> > > +       CAAM_JR_RUN_DESC,
> > > +};
> > > +
> > >  struct op_ring {
> > >         caam_dma_addr_t desc;
> > >         uint32_t status;
> > > @@ -102,6 +114,13 @@ struct result {
> > >         uint32_t status;
> > >  };
> > >
> > > +struct caam_regs {
> > > +       ccsr_sec_t *sec;
> > > +       struct jr_regs *regs;
> > > +       u8 jrid;
> > > +       struct jobring jr[CONFIG_SYS_FSL_MAX_NUM_OF_SEC];
> > > +};
> >
> > Don't forget to comment the structs.
> Will add in next version.
>
> > > +
> > >  void caam_jr_strstatus(u32 status);
> > >  int run_descriptor_jr(uint32_t *desc);
> >
> > and exported functions (normally drivers shouldn't have these).
> Yes I agree. Since run_descriptor_jr() is already in use.
> I am planning a separate patch to move other user of run_descriptor_jr() to caam_jr_ioctl().

OK. But that alone might indicate a new uclass would be generally
useful. Anyway, this is fine and it can be considered later.

Regards,
Simon