Hi Gaurav,
On Thu, 6 Jan 2022 at 22:54, Gaurav Jain <[email protected]> wrote:
>
> Hi
>
> > -----Original Message-----
> > From: Simon Glass <[email protected]>
> > Sent: Tuesday, December 28, 2021 2:02 PM
> > To: Gaurav Jain <[email protected]>
> > Cc: U-Boot Mailing List <[email protected]>; Stefano Babic
> > <[email protected]>; Fabio Estevam <[email protected]>; Peng Fan
> > <[email protected]>; Priyanka Jain <[email protected]>; Ye Li
> > <[email protected]>; Horia Geanta <[email protected]>; Ji Luo
> > <[email protected]>; Franck Lenormand <[email protected]>; Silvano Di
> > Ninno <[email protected]>; Sahil Malhotra <[email protected]>;
> > Pankaj Gupta <[email protected]>; Varun Sethi <[email protected]>; dl-
> > uboot-imx <[email protected]>; Shengzhou Liu <[email protected]>;
> > Mingkai Hu <[email protected]>; Rajesh Bhagat <[email protected]>;
> > Meenakshi Aggarwal <[email protected]>; Wasim Khan
> > <[email protected]>; Alison Wang <[email protected]>; Pramod
> > Kumar <[email protected]>; Andy Tang <[email protected]>;
> > Adrian Alonso <[email protected]>; Vladimir Oltean <[email protected]>
> > 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 <[email protected]> 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 <[email protected]>
> > > Reviewed-by: Ye Li <[email protected]>
> > > ---
> > > 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
