Hello Michael,
First of, thanks a lot for thorough review!
> -----Original Message-----
> From: Michael Walle <michael@xxxxxxxx>
> Sent: Friday, November 12, 2021 8:23 PM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@xxxxxxxxxxxxxxxxxxxx>
> Cc: horia.geanta@xxxxxxx; pankaj.gupta@xxxxxxx;
> herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> iuliana.prodan@xxxxxxx; linux-crypto@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 1/2] crypto: caam - convert to use capabilities
>
>
> Am 2021-11-11 17:46, schrieb Andrey Zhizhikin:
> > CAAM driver contains several variables, which are used for indication
> > that ertail capabilities are detected during initial probing of the
> > device. They are defined as u8, but mainly used as boolean variables to
> > identify capabillities.
> >
> > Clean-up all assorted variables, collect them into one bitfield value
> > which encodes capabilities as bit, and use them in the execution flow
> > instead.
> >
> > Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > Changes in V2: No change, this patch is newly introduced
> >
> > drivers/crypto/caam/caamalg_qi.c | 2 +-
> > drivers/crypto/caam/ctrl.c | 49 ++++++++++++++++++--------------
> > drivers/crypto/caam/intern.h | 16 +++++------
> > drivers/crypto/caam/regs.h | 2 --
> > 4 files changed, 36 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/crypto/caam/caamalg_qi.c
> > b/drivers/crypto/caam/caamalg_qi.c
> > index 189a7438b29c..372a319e8434 100644
> > --- a/drivers/crypto/caam/caamalg_qi.c
> > +++ b/drivers/crypto/caam/caamalg_qi.c
> > @@ -2610,7 +2610,7 @@ int caam_qi_algapi_init(struct device *ctrldev)
> > bool registered = false;
> >
> > /* Make sure this runs only on (DPAA 1.x) QI */
> > - if (!priv->qi_present || caam_dpaa2)
> > + if (!(priv->caam_caps | CAAM_CAPS_QI_PRESENT) || caam_dpaa2)
>
> Typo?
> if (!(priv->caam_caps & CAAM_CAPS_QI_PRESENT) || caam_dpaa2)
True, good catch! Would address in V3.
>
>
> > return 0;
> >
> > /*
> > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> > index ca0361b2dbb0..7a14a69d89c7 100644
> > --- a/drivers/crypto/caam/ctrl.c
> > +++ b/drivers/crypto/caam/ctrl.c
> > @@ -100,7 +100,7 @@ static inline int run_descriptor_deco0(struct
> > device *ctrldev, u32 *desc,
> > int i;
> >
> >
> > - if (ctrlpriv->virt_en == 1 ||
> > + if ((ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) ||
> > /*
> > * Apparently on i.MX8M{Q,M,N,P} it doesn't matter if virt_en ==
> > 1
> > * and the following steps should be performed regardless
> > @@ -169,7 +169,7 @@ static inline int run_descriptor_deco0(struct
> > device *ctrldev, u32 *desc,
> > *status = rd_reg32(&deco->op_status_hi) &
> > DECO_OP_STATUS_HI_ERR_MASK;
> >
> > - if (ctrlpriv->virt_en == 1)
> > + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED)
> > clrsetbits_32(&ctrl->deco_rsr, DECORSR_JR0, 0);
> >
> > /* Mark the DECO as free */
> > @@ -622,7 +622,6 @@ static int caam_probe(struct platform_device *pdev)
> > struct dentry *dfs_root;
> > u32 scfgr, comp_params;
> > u8 rng_vid;
> > - int pg_size;
> > int BLOCK_OFFSET = 0;
> > bool pr_support = false;
> >
> > @@ -666,11 +665,12 @@ static int caam_probe(struct platform_device
> > *pdev)
> > else
> > caam_ptr_sz = sizeof(u32);
> > caam_dpaa2 = !!(comp_params & CTPR_MS_DPAA2);
> > - ctrlpriv->qi_present = !!(comp_params & CTPR_MS_QI_MASK);
> > + ctrlpriv->caam_caps |= (!!(comp_params & CTPR_MS_QI_MASK)) ?
> > + CAAM_CAPS_QI_PRESENT : 0;
>
> "!!" is superfluous here. and the braces, too. But IMHO this is
> easier on the eye:
> if (comp_params & CTPR_MS_QI_MASK)
> ctrl->priv->caam_cap |= CAAM_CAPS_QI_PRESENT;
Would replace and push in V3.
>
>
> > #ifdef CONFIG_CAAM_QI
> > /* If (DPAA 1.x) QI present, check whether dependencies are available
> > */
> > - if (ctrlpriv->qi_present && !caam_dpaa2) {
> > + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) {
> > ret = qman_is_probed();
> > if (!ret) {
> > return -EPROBE_DEFER;
> > @@ -692,11 +692,14 @@ static int caam_probe(struct platform_device
> > *pdev)
> > /* Allocating the BLOCK_OFFSET based on the supported page size on
> > * the platform
> > */
> > - pg_size = (comp_params & CTPR_MS_PG_SZ_MASK) >>
> CTPR_MS_PG_SZ_SHIFT;
> > - if (pg_size == 0)
> > - BLOCK_OFFSET = PG_SIZE_4K;
> > + ctrlpriv->caam_caps |=
> > + (!!((comp_params & CTPR_MS_PG_SZ_MASK) >>
> CTPR_MS_PG_SZ_SHIFT)) ?
> > + CAAM_CAPS_64K_PAGESIZE : 0;
>
> same here as above.
>
> if (comp_params & CTPR_MS_PG_SZ_MASK)
> ctrl->priv->caam_cap |= CAAM_CAPS_64K_PAGESIZE;
>
> Assuming that SZ_MASK and SZ_SHIFT fits together ;)
Correct, comes in V3.
>
> > +
> > + if (ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE)
> > + BLOCK_OFFSET = SZ_64K;
> > else
> > - BLOCK_OFFSET = PG_SIZE_64K;
> > + BLOCK_OFFSET = SZ_4K;
>
> btw.. that all uppercase BLOCK_OFFSET looks super odd. Can we get
> rid of that too? I haven't checked but pg_size didn't make any
> sense before, did it? At least if SZ_MASK has more than one bit.
This unfortunately be needed, since IM8M and LS family have different
page sizes and therefore - different base address offsets.
I would rename it to match the meaning of this variable.
>
> > ctrlpriv->ctrl = (struct caam_ctrl __iomem __force *)ctrl;
> > ctrlpriv->assure = (struct caam_assurance __iomem __force *)
> > @@ -711,11 +714,11 @@ static int caam_probe(struct platform_device
> > *pdev)
> > /* Get the IRQ of the controller (for security violations only) */
> > ctrlpriv->secvio_irq = irq_of_parse_and_map(nprop, 0);
> > np = of_find_compatible_node(NULL, NULL, "fsl,qoriq-mc");
> > - ctrlpriv->mc_en = !!np;
> > + ctrlpriv->caam_caps |= (!!np) ? CAAM_CAPS_MC_ENABLED : 0;
>
> if (np)
> ctrlpriv->caam_caps |= CAAM_CAPS_MC_ENABLED;
Noted, comes in V3.
>
> > of_node_put(np);
> >
> > #ifdef CONFIG_FSL_MC_BUS
> > - if (ctrlpriv->mc_en) {
> > + if (ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) {
> > struct fsl_mc_version *mc_version;
> >
> > mc_version = fsl_mc_get_version();
> > @@ -732,7 +735,7 @@ static int caam_probe(struct platform_device *pdev)
> > * In case of SoCs with Management Complex, MC f/w performs
> > * the configuration.
> > */
> > - if (!ctrlpriv->mc_en)
> > + if (!(ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED))
> > clrsetbits_32(&ctrl->mcr, MCFGR_AWCACHE_MASK,
> > MCFGR_AWCACHE_CACH | MCFGR_AWCACHE_BUFF |
> > MCFGR_WDENABLE | MCFGR_LARGE_BURST);
> > @@ -745,7 +748,6 @@ static int caam_probe(struct platform_device *pdev)
> > */
> > scfgr = rd_reg32(&ctrl->scfgr);
> >
> > - ctrlpriv->virt_en = 0;
> > if (comp_params & CTPR_MS_VIRT_EN_INCL) {
> > /* VIRT_EN_INCL = 1 & VIRT_EN_POR = 1 or
> > * VIRT_EN_INCL = 1 & VIRT_EN_POR = 0 & SCFGR_VIRT_EN = 1
> > @@ -753,14 +755,14 @@ static int caam_probe(struct platform_device
> > *pdev)
> > if ((comp_params & CTPR_MS_VIRT_EN_POR) ||
> > (!(comp_params & CTPR_MS_VIRT_EN_POR) &&
> > (scfgr & SCFGR_VIRT_EN)))
> > - ctrlpriv->virt_en = 1;
> > + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED;
>
> at first sight it looked like a wrong indendation, but it the old
> code was wrong.
checkpatch.pl --strict forced my hand to align it. ;)
>
> > } else {
> > /* VIRT_EN_INCL = 0 && VIRT_EN_POR_VALUE = 1 */
> > if (comp_params & CTPR_MS_VIRT_EN_POR)
> > - ctrlpriv->virt_en = 1;
> > + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED;
> > }
> >
> > - if (ctrlpriv->virt_en == 1)
> > + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED)
> > clrsetbits_32(&ctrl->jrstart, 0, JRSTART_JR0_START |
> > JRSTART_JR1_START | JRSTART_JR2_START |
> > JRSTART_JR3_START);
> > @@ -785,7 +787,7 @@ static int caam_probe(struct platform_device *pdev)
> > caam_debugfs_init(ctrlpriv, dfs_root);
> >
> > /* Check to see if (DPAA 1.x) QI present. If so, enable */
> > - if (ctrlpriv->qi_present && !caam_dpaa2) {
> > + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) {
> > ctrlpriv->qi = (struct caam_queue_if __iomem __force *)
> > ((__force uint8_t *)ctrl +
> > BLOCK_OFFSET * QI_BLOCK_NUMBER
> > @@ -810,12 +812,13 @@ static int caam_probe(struct platform_device
> > *pdev)
> > (ring + JR_BLOCK_NUMBER) *
> > BLOCK_OFFSET
> > );
> > - ctrlpriv->total_jobrs++;
> > ring++;
> > + ctrlpriv->caam_caps |= BIT(ring);
>
> I think this deserves an own macro. At the moment you assume that
> the lower bits are for the rings, right? I'd like to see something
> like
> ctrlpriv->caam_caps |= JR_PRESENT(ring);
>
> then have
> #define JR_PRESENT_MASK GENMASK(7, 0)
> #define JR_PRESENT(x) (BIT(x) & JR_PRESENT_MASK)
> together with all the other bits in caam_caps. Or something
> along that. I guess you got the idea.
True, would come up with a separate macro for this.
>
>
> > }
> >
> > /* If no QI and no rings specified, quit and go home */
> > - if ((!ctrlpriv->qi_present) && (!ctrlpriv->total_jobrs)) {
> > + if (!(ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) &&
> > + (hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK) == 0))
> > {
> > dev_err(dev, "no queues configured, terminating\n");
> > return -ENOMEM;
> > }
> > @@ -832,7 +835,8 @@ static int caam_probe(struct platform_device *pdev)
> > * already instantiated, do RNG instantiation
> > * In case of SoCs with Management Complex, RNG is managed by MC f/w.
> > */
> > - if (!(ctrlpriv->mc_en && pr_support) && rng_vid >= 4) {
> > + if (!((ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) && pr_support)
> &&
> > + rng_vid >= 4) {
> > ctrlpriv->rng4_sh_init =
> > rd_reg32(&ctrl->r4tst[0].rdsta);
> > /*
> > @@ -900,8 +904,9 @@ static int caam_probe(struct platform_device *pdev)
> > /* Report "alive" for developer to see */
> > dev_info(dev, "device ID = 0x%016llx (Era %d)\n", caam_id,
> > ctrlpriv->era);
> > - dev_info(dev, "job rings = %d, qi = %d\n",
> > - ctrlpriv->total_jobrs, ctrlpriv->qi_present);
> > + dev_info(dev, "job rings = %ld, qi = %s\n",
> > + hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK),
> > + (ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) ? "yes" : "no");
> >
> > ret = devm_of_platform_populate(dev);
> > if (ret)
> > diff --git a/drivers/crypto/caam/intern.h
> > b/drivers/crypto/caam/intern.h
> > index 7d45b21bd55a..37f0b93c7087 100644
> > --- a/drivers/crypto/caam/intern.h
> > +++ b/drivers/crypto/caam/intern.h
> > @@ -86,15 +86,15 @@ struct caam_drv_private {
> >
> > struct iommu_domain *domain;
> >
> > - /*
> > - * Detected geometry block. Filled in from device tree if powerpc,
> > - * or from register-based version detection code
> > - */
> > - u8 total_jobrs; /* Total Job Rings in device */
> > - u8 qi_present; /* Nonzero if QI present in device */
> > - u8 mc_en; /* Nonzero if MC f/w is active */
> > + unsigned long caam_caps; /* CAAM Module capabilities */
> > +
> > +#define CAAM_CAPS_QI_PRESENT BIT(0) /* Queue Manager interface (QI)
> > implemented */
> > +#define CAAM_CAPS_JOBRS_MASK GENMASK(15, 1) /* Job Ring is available
> > in NS World */
>
> ok I see you already have something like that. See above. That BIT()
> in the code above should go away.
>
> > +#define CAAM_CAPS_MC_ENABLED BIT(16) /* Management Complex is
> enabled
> > (F/W is active) */
> > +#define CAAM_CAPS_VIRT_ENABLED BIT(17) /* Virtualization enabled */
> > +#define CAAM_CAPS_64K_PAGESIZE BIT(18) /* CAAM register page size
> > (64KB if set, 4KB if unset) */
> > +
> > int secvio_irq; /* Security violation interrupt number */
> > - int virt_en; /* Virtualization enabled in CAAM */
> > int era; /* CAAM Era (internal HW revision) */
> >
> > #define RNG4_MAX_HANDLES 2
> > diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
> > index 3738625c0250..186e76e6a3e7 100644
> > --- a/drivers/crypto/caam/regs.h
> > +++ b/drivers/crypto/caam/regs.h
> > @@ -1023,6 +1023,4 @@ struct caam_deco {
> > #define ASSURE_BLOCK_NUMBER 6
> > #define QI_BLOCK_NUMBER 7
> > #define DECO_BLOCK_NUMBER 8
> > -#define PG_SIZE_4K 0x1000
> > -#define PG_SIZE_64K 0x10000
>
> nice ;)
>
> > #endif /* REGS_H */
>
> Otherwise, I really like this cleanup.
Thanks a lot! This patch makes the driver more easy to follow.
>
> Thanks,
> -michael
-- andrey
