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

[Gaurav Jain]

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.

> 
> [..]
> 
> > 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().

Regards
Gaurav Jain
> 
> >
> > --
> > 2.17.1
> >
> 
> Regards,
> Simon
> 
> Regards,
> Simon