Re: [PATCH v2 2/2] crypto: caam - allow retrieving 'era' from register

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux