On 4/11/2018 4:54 AM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@xxxxxxx>
>
> The 'era' information can be retrieved from CAAM registers, so
> introduce a caam_get_era_from_hw() function that gets it via register
> reads in case the 'fsl,sec-era' property is not passed in the device
> tree.
>
Indeed, "fsl,sec-era" property is marked as optional in DT bindings doc.
This means the previous commit
883619a931e9 ("crypto: caam - fix ERA retrieval function")
should have kept the detection based on registers as a fallback.
Have you actually hit a case where the property was missing from DT?
> This function is based on the U-Boot implementation from
> drivers/crypto/fsl/sec.c
>
> Signed-off-by: Fabio Estevam <fabio.estevam@xxxxxxx>
> ---
> Changes since v1:
> - None. I previously asked to put the linux-crypto list on Cc
>
> drivers/crypto/caam/ctrl.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
> drivers/crypto/caam/regs.h | 6 ++++++
> 2 files changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index bee690a..3f10791 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -396,11 +396,47 @@ static void kick_trng(struct platform_device *pdev, int ent_delay)
> clrsetbits_32(&r4tst->rtmctl, RTMCTL_PRGM, RTMCTL_SAMP_MODE_RAW_ES_SC);
> }
>
> +static u8 caam_get_era_from_hw(struct caam_ctrl __iomem *ctrl)
> +{
> + const struct sec_vid id[] = {
Make the struct static.
sec_vid should not be used since:
-it defines the layout of SECVID_MS register (ip_id, maj_rev, min_rev)
-while the array below contains (ip_id, maj_rev, era) tuples
You could instead use an anonymous struct, just like in kernel commit
82c2f9607b8a4 or as in u-boot.
> + {0x0A10, 1, 1},
> + {0x0A10, 2, 2},
> + {0x0A12, 1, 3},
> + {0x0A14, 1, 3},
> + {0x0A14, 2, 4},
> + {0x0A16, 1, 4},
> + {0x0A10, 3, 4},
> + {0x0A11, 1, 4},
> + {0x0A18, 1, 4},
> + {0x0A11, 2, 5},
> + {0x0A12, 2, 5},
> + {0x0A13, 1, 5},
> + {0x0A1C, 1, 5},
> + };
> + int i;
> +
> + u32 secvid_ms = rd_reg32(&ctrl->perfmon.caam_id_ms);
Reading caam_id_ms should be done only if ccbvid does not provide the era, i.e.
should be moved just before the for loop.
> + u32 ccbvid = rd_reg32(&ctrl->perfmon.ccb_id);
> + u16 ip_id = (secvid_ms & SECVID_MS_IPID_MASK) >> SECVID_MS_IPID_SHIFT;
> + u8 maj_rev = (secvid_ms & SECVID_MS_MAJ_REV_MASK) >>
> + SECVID_MS_MAJ_REV_SHIFT;
> + u8 era = (ccbvid & CCBVID_ERA_MASK) >> CCBVID_ERA_SHIFT;
> +
> + if (era) /* This is '0' prior to CAAM ERA-6 */
> + return era;
> +
> + for (i = 0; i < ARRAY_SIZE(id); i++)
> + if (id[i].ip_id == ip_id && id[i].maj_rev == maj_rev)
> + return id[i].min_rev;
> +
> + return 0;
Should return -ENOTSUPP, to keep the semantics of caam_get_era().
> +}
> +
> /**
> * caam_get_era() - Return the ERA of the SEC on SoC, based
> * on "sec-era" propery in the DTS. This property is updated by u-boot.
While here:
_optional_ ^^^ s/propery/property
Should also mention that ERA detection fallback relies on SEC registers (CCBVID
or SECVID).
> **/
> -static int caam_get_era(void)
> +static int caam_get_era(struct caam_ctrl __iomem *ctrl)
> {
> struct device_node *caam_node;
> int ret;
> @@ -410,7 +446,10 @@ static int caam_get_era(void)
> ret = of_property_read_u32(caam_node, "fsl,sec-era", &prop);
> of_node_put(caam_node);
>
> - return ret ? -ENOTSUPP : prop;
> + if (!ret)
> + return prop;
> + else
> + return caam_get_era_from_hw(ctrl);
> }
>
> static const struct of_device_id caam_match[] = {
> @@ -622,7 +661,7 @@ static int caam_probe(struct platform_device *pdev)
> goto iounmap_ctrl;
> }
>
> - ctrlpriv->era = caam_get_era();
> + ctrlpriv->era = caam_get_era(ctrl);
>
> ret = of_platform_populate(nprop, caam_match, NULL, dev);
> if (ret) {
> diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
> index fee3638..6f96d7b 100644
> --- a/drivers/crypto/caam/regs.h
> +++ b/drivers/crypto/caam/regs.h
> @@ -311,6 +311,12 @@ struct caam_perfmon {
> u64 rsvd3;
>
> /* Component Instantiation Parameters fe0-fff */
> +#define SECVID_MS_IPID_MASK 0xffff0000
> +#define SECVID_MS_IPID_SHIFT 16
> +#define SECVID_MS_MAJ_REV_MASK 0x0000ff00
> +#define SECVID_MS_MAJ_REV_SHIFT 8
> +#define CCBVID_ERA_MASK 0xff000000
> +#define CCBVID_ERA_SHIFT 24
Please add the defines in front of each register:
-SECVID_* before caam_id_ms
-CCBVID_* before ccb_id
> u32 rtic_id; /* RVID - RTIC Version ID */
> u32 ccb_id; /* CCBVID - CCB Version ID */
> u32 cha_id_ms; /* CHAVID - CHA Version ID Most Significant*/
>
Thanks,
Horia
