Re: [PATCH v2 1/2] crypto: caam - convert to use capabilities
From: Michael Walle
Date: Fri Nov 12 2021 - 14:22:42 EST
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)
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;
#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 ;)
+
+ 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.
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;
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.
} 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.
}
/* 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,
-michael