diff mbox

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

Message ID 1523411644-10156-2-git-send-email-festevam@gmail.com (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show

Commit Message

Fabio Estevam April 11, 2018, 1:54 a.m. UTC 
From: Fabio Estevam <fabio.estevam@nxp.com>

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.

This function is based on the U-Boot implementation from
drivers/crypto/fsl/sec.c

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
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(-)

Comments

Horia Geanta April 11, 2018, 7:47 a.m. UTC | #1 
On 4/11/2018 4:54 AM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> 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@nxp.com>
> ---
> 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
Fabio Estevam April 11, 2018, 12:05 p.m. UTC | #2 
Hi Horia,

On Wed, Apr 11, 2018 at 4:47 AM, Horia Geantă <horia.geanta@nxp.com> wrote:

> Have you actually hit a case where the property was missing from DT?

Yes, on imx7s.dtsi it is missing.

I also started adding CAAM support to mx6ul and I did not pass the
""fsl,sec-era"

Thanks for your review.

I addressed all of your comments and sent a v3.
diff mbox

Patch

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[] = {
+		{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);
+	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;
+}
+
 /**
  * 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.
  **/
-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
 	u32 rtic_id;		/* RVID - RTIC Version ID	*/
 	u32 ccb_id;		/* CCBVID - CCB Version ID	*/
 	u32 cha_id_ms;		/* CHAVID - CHA Version ID Most Significant*/