[PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling

[PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling

[Gilad Ben-Yossef]

The patch set enables the use of CryptoCell found in some Renesas R-Car
Salvator-X boards and fixes some driver issues uncovered that prevented
to work properly.

Changes from v1:
- Properly fix the bug that caused us to read a bad signature register
  rather than dropping the check
- Proper DT fields as indicated by Geert Uytterhoeven.
- Better clock enabling as suggested by Geert Uytterhoeven.

Note! the last two patches in the set depend on the
"clk: renesas: r8a7795: Add CR clock" patch from Geert Uytterhoeven.

Gilad Ben-Yossef (5):
  crypto: ccree: correct host regs offset
  crypto: ccree: better clock handling
  crypto: ccree: silence debug prints
  clk: renesas: r8a7795: add ccree clock bindings
  arm64: dts: renesas: r8a7795: add ccree binding

 arch/arm64/boot/dts/renesas/r8a7795.dtsi |  9 +++++++++
 drivers/clk/renesas/r8a7795-cpg-mssr.c   |  1 +
 drivers/crypto/ccree/cc_debugfs.c        |  7 +++++--
 drivers/crypto/ccree/cc_driver.c         | 34 ++++++++++++++++++++++++++------
 drivers/crypto/ccree/cc_driver.h         |  2 ++
 drivers/crypto/ccree/cc_host_regs.h      |  6 ++++--
 6 files changed, 49 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] crypto: ccree: correct host regs offset

[Gilad Ben-Yossef]

The product signature and HW revision register have different offset on the
older HW revisions.
This fixes the problem of the driver failing sanity check on silicon
despite working on the FPGA emulation systems.

Fixes: 27b3b22dd98c ("crypto: ccree - add support for older HW revs")
Cc: stable@vger.kernel.org
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
 drivers/crypto/ccree/cc_debugfs.c   | 7 +++++--
 drivers/crypto/ccree/cc_driver.c    | 8 ++++++--
 drivers/crypto/ccree/cc_driver.h    | 2 ++
 drivers/crypto/ccree/cc_host_regs.h | 6 ++++--
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/ccree/cc_debugfs.c b/drivers/crypto/ccree/cc_debugfs.c
index 08f8db4..5ca184e 100644
--- a/drivers/crypto/ccree/cc_debugfs.c
+++ b/drivers/crypto/ccree/cc_debugfs.c
@@ -26,7 +26,8 @@ struct cc_debugfs_ctx {
 static struct dentry *cc_debugfs_dir;
 
 static struct debugfs_reg32 debug_regs[] = {
-	CC_DEBUG_REG(HOST_SIGNATURE),
+	{ .name = "SIGNATURE" }, /* Must be 0th */
+	{ .name = "VERSION" }, /* Must be 1st */
 	CC_DEBUG_REG(HOST_IRR),
 	CC_DEBUG_REG(HOST_POWER_DOWN_EN),
 	CC_DEBUG_REG(AXIM_MON_ERR),
@@ -34,7 +35,6 @@ static struct debugfs_reg32 debug_regs[] = {
 	CC_DEBUG_REG(HOST_IMR),
 	CC_DEBUG_REG(AXIM_CFG),
 	CC_DEBUG_REG(AXIM_CACHE_PARAMS),
-	CC_DEBUG_REG(HOST_VERSION),
 	CC_DEBUG_REG(GPR_HOST),
 	CC_DEBUG_REG(AXIM_MON_COMP),
 };
@@ -58,6 +58,9 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
 	struct debugfs_regset32 *regset;
 	struct dentry *file;
 
+	debug_regs[0].offset = drvdata->sig_offset;
+	debug_regs[1].offset = drvdata->ver_offset;
+
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 89ce013..6f93ce7 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -207,9 +207,13 @@ static int init_cc_resources(struct platform_device *plat_dev)
 	if (hw_rev->rev >= CC_HW_REV_712) {
 		new_drvdata->hash_len_sz = HASH_LEN_SIZE_712;
 		new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP);
+		new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_712);
+		new_drvdata->ver_offset = CC_REG(HOST_VERSION_712);
 	} else {
 		new_drvdata->hash_len_sz = HASH_LEN_SIZE_630;
 		new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP8);
+		new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_630);
+		new_drvdata->ver_offset = CC_REG(HOST_VERSION_630);
 	}
 
 	platform_set_drvdata(plat_dev, new_drvdata);
@@ -276,7 +280,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
 	}
 
 	/* Verify correct mapping */
-	signature_val = cc_ioread(new_drvdata, CC_REG(HOST_SIGNATURE));
+	signature_val = cc_ioread(new_drvdata, new_drvdata->sig_offset);
 	if (signature_val != hw_rev->sig) {
 		dev_err(dev, "Invalid CC signature: SIGNATURE=0x%08X != expected=0x%08X\n",
 			signature_val, hw_rev->sig);
@@ -287,7 +291,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
 
 	/* Display HW versions */
 	dev_info(dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver version %s\n",
-		 hw_rev->name, cc_ioread(new_drvdata, CC_REG(HOST_VERSION)),
+		 hw_rev->name, cc_ioread(new_drvdata, new_drvdata->ver_offset),
 		 DRV_MODULE_VERSION);
 
 	rc = init_cc_regs(new_drvdata, true);
diff --git a/drivers/crypto/ccree/cc_driver.h b/drivers/crypto/ccree/cc_driver.h
index 2048fde..95f82b2 100644
--- a/drivers/crypto/ccree/cc_driver.h
+++ b/drivers/crypto/ccree/cc_driver.h
@@ -129,6 +129,8 @@ struct cc_drvdata {
 	enum cc_hw_rev hw_rev;
 	u32 hash_len_sz;
 	u32 axim_mon_offset;
+	u32 sig_offset;
+	u32 ver_offset;
 };
 
 struct cc_crypto_alg {
diff --git a/drivers/crypto/ccree/cc_host_regs.h b/drivers/crypto/ccree/cc_host_regs.h
index f510018..616b2e1 100644
--- a/drivers/crypto/ccree/cc_host_regs.h
+++ b/drivers/crypto/ccree/cc_host_regs.h
@@ -45,7 +45,8 @@
 #define CC_HOST_ICR_DSCRPTR_WATERMARK_QUEUE0_CLEAR_BIT_SIZE	0x1UL
 #define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SHIFT	0x17UL
 #define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SIZE	0x1UL
-#define CC_HOST_SIGNATURE_REG_OFFSET	0xA24UL
+#define CC_HOST_SIGNATURE_712_REG_OFFSET	0xA24UL
+#define CC_HOST_SIGNATURE_630_REG_OFFSET	0xAC8UL
 #define CC_HOST_SIGNATURE_VALUE_BIT_SHIFT	0x0UL
 #define CC_HOST_SIGNATURE_VALUE_BIT_SIZE	0x20UL
 #define CC_HOST_BOOT_REG_OFFSET	0xA28UL
@@ -105,7 +106,8 @@
 #define CC_HOST_BOOT_ONLY_ENCRYPT_LOCAL_BIT_SIZE	0x1UL
 #define CC_HOST_BOOT_AES_EXISTS_LOCAL_BIT_SHIFT	0x1EUL
 #define CC_HOST_BOOT_AES_EXISTS_LOCAL_BIT_SIZE	0x1UL
-#define CC_HOST_VERSION_REG_OFFSET	0xA40UL
+#define CC_HOST_VERSION_712_REG_OFFSET	0xA40UL
+#define CC_HOST_VERSION_630_REG_OFFSET	0xAD8UL
 #define CC_HOST_VERSION_VALUE_BIT_SHIFT	0x0UL
 #define CC_HOST_VERSION_VALUE_BIT_SIZE	0x20UL
 #define CC_HOST_KFDE0_VALID_REG_OFFSET	0xA60UL
-- 
2.7.4

Re: [PATCH v2 1/5] crypto: ccree: correct host regs offset

[Simon Horman]

On Thu, May 24, 2018 at 03:19:06PM +0100, Gilad Ben-Yossef wrote:
> The product signature and HW revision register have different offset on the
> older HW revisions.
> This fixes the problem of the driver failing sanity check on silicon
> despite working on the FPGA emulation systems.
> 
> Fixes: 27b3b22dd98c ("crypto: ccree - add support for older HW revs")

Did the above introduce a regression that is fixed by this patch
or did it add a feature that only works with this patch?

In the case of the latter I would drop the Fixes tag,
but I don't feel strongly about it.

> Cc: stable@vger.kernel.org
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>

Minor not below not withstanding,

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
>  drivers/crypto/ccree/cc_debugfs.c   | 7 +++++--
>  drivers/crypto/ccree/cc_driver.c    | 8 ++++++--
>  drivers/crypto/ccree/cc_driver.h    | 2 ++
>  drivers/crypto/ccree/cc_host_regs.h | 6 ++++--
>  4 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/crypto/ccree/cc_debugfs.c b/drivers/crypto/ccree/cc_debugfs.c
> index 08f8db4..5ca184e 100644
> --- a/drivers/crypto/ccree/cc_debugfs.c
> +++ b/drivers/crypto/ccree/cc_debugfs.c
> @@ -26,7 +26,8 @@ struct cc_debugfs_ctx {
>  static struct dentry *cc_debugfs_dir;
>  
>  static struct debugfs_reg32 debug_regs[] = {
> -	CC_DEBUG_REG(HOST_SIGNATURE),
> +	{ .name = "SIGNATURE" }, /* Must be 0th */
> +	{ .name = "VERSION" }, /* Must be 1st */
>  	CC_DEBUG_REG(HOST_IRR),
>  	CC_DEBUG_REG(HOST_POWER_DOWN_EN),
>  	CC_DEBUG_REG(AXIM_MON_ERR),
> @@ -34,7 +35,6 @@ static struct debugfs_reg32 debug_regs[] = {
>  	CC_DEBUG_REG(HOST_IMR),
>  	CC_DEBUG_REG(AXIM_CFG),
>  	CC_DEBUG_REG(AXIM_CACHE_PARAMS),
> -	CC_DEBUG_REG(HOST_VERSION),
>  	CC_DEBUG_REG(GPR_HOST),
>  	CC_DEBUG_REG(AXIM_MON_COMP),
>  };
> @@ -58,6 +58,9 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
>  	struct debugfs_regset32 *regset;
>  	struct dentry *file;
>  
> +	debug_regs[0].offset = drvdata->sig_offset;
> +	debug_regs[1].offset = drvdata->ver_offset;
> +
>  	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>  	if (!ctx)
>  		return -ENOMEM;
> diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
> index 89ce013..6f93ce7 100644
> --- a/drivers/crypto/ccree/cc_driver.c
> +++ b/drivers/crypto/ccree/cc_driver.c
> @@ -207,9 +207,13 @@ static int init_cc_resources(struct platform_device *plat_dev)
>  	if (hw_rev->rev >= CC_HW_REV_712) {
>  		new_drvdata->hash_len_sz = HASH_LEN_SIZE_712;
>  		new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP);
> +		new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_712);
> +		new_drvdata->ver_offset = CC_REG(HOST_VERSION_712);
>  	} else {
>  		new_drvdata->hash_len_sz = HASH_LEN_SIZE_630;
>  		new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP8);
> +		new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_630);
> +		new_drvdata->ver_offset = CC_REG(HOST_VERSION_630);
>  	}
>  
>  	platform_set_drvdata(plat_dev, new_drvdata);
> @@ -276,7 +280,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
>  	}
>  
>  	/* Verify correct mapping */
> -	signature_val = cc_ioread(new_drvdata, CC_REG(HOST_SIGNATURE));
> +	signature_val = cc_ioread(new_drvdata, new_drvdata->sig_offset);
>  	if (signature_val != hw_rev->sig) {
>  		dev_err(dev, "Invalid CC signature: SIGNATURE=0x%08X != expected=0x%08X\n",
>  			signature_val, hw_rev->sig);
> @@ -287,7 +291,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
>  
>  	/* Display HW versions */
>  	dev_info(dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver version %s\n",
> -		 hw_rev->name, cc_ioread(new_drvdata, CC_REG(HOST_VERSION)),
> +		 hw_rev->name, cc_ioread(new_drvdata, new_drvdata->ver_offset),
>  		 DRV_MODULE_VERSION);
>  
>  	rc = init_cc_regs(new_drvdata, true);
> diff --git a/drivers/crypto/ccree/cc_driver.h b/drivers/crypto/ccree/cc_driver.h
> index 2048fde..95f82b2 100644
> --- a/drivers/crypto/ccree/cc_driver.h
> +++ b/drivers/crypto/ccree/cc_driver.h
> @@ -129,6 +129,8 @@ struct cc_drvdata {

This patch doesn't make things (much) worse
but struct cc_drvdata has a rather incomplete kdoc.

>  	enum cc_hw_rev hw_rev;
>  	u32 hash_len_sz;
>  	u32 axim_mon_offset;
> +	u32 sig_offset;
> +	u32 ver_offset;
>  };
>  
>  struct cc_crypto_alg {
> diff --git a/drivers/crypto/ccree/cc_host_regs.h b/drivers/crypto/ccree/cc_host_regs.h
> index f510018..616b2e1 100644
> --- a/drivers/crypto/ccree/cc_host_regs.h
> +++ b/drivers/crypto/ccree/cc_host_regs.h
> @@ -45,7 +45,8 @@
>  #define CC_HOST_ICR_DSCRPTR_WATERMARK_QUEUE0_CLEAR_BIT_SIZE	0x1UL
>  #define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SHIFT	0x17UL
>  #define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SIZE	0x1UL
> -#define CC_HOST_SIGNATURE_REG_OFFSET	0xA24UL
> +#define CC_HOST_SIGNATURE_712_REG_OFFSET	0xA24UL
> +#define CC_HOST_SIGNATURE_630_REG_OFFSET	0xAC8UL
>  #define CC_HOST_SIGNATURE_VALUE_BIT_SHIFT	0x0UL
>  #define CC_HOST_SIGNATURE_VALUE_BIT_SIZE	0x20UL
>  #define CC_HOST_BOOT_REG_OFFSET	0xA28UL
> @@ -105,7 +106,8 @@
>  #define CC_HOST_BOOT_ONLY_ENCRYPT_LOCAL_BIT_SIZE	0x1UL
>  #define CC_HOST_BOOT_AES_EXISTS_LOCAL_BIT_SHIFT	0x1EUL
>  #define CC_HOST_BOOT_AES_EXISTS_LOCAL_BIT_SIZE	0x1UL
> -#define CC_HOST_VERSION_REG_OFFSET	0xA40UL
> +#define CC_HOST_VERSION_712_REG_OFFSET	0xA40UL
> +#define CC_HOST_VERSION_630_REG_OFFSET	0xAD8UL
>  #define CC_HOST_VERSION_VALUE_BIT_SHIFT	0x0UL
>  #define CC_HOST_VERSION_VALUE_BIT_SIZE	0x20UL
>  #define CC_HOST_KFDE0_VALID_REG_OFFSET	0xA60UL
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/5] crypto: ccree: correct host regs offset

[Gilad Ben-Yossef]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

On Tue, May 29, 2018 at 7:12 PM, Simon Horman <horms@verge.net.au> wrote:

> On Thu, May 24, 2018 at 03:19:06PM +0100, Gilad Ben-Yossef wrote:
> > The product signature and HW revision register have different offset on
> the
> > older HW revisions.
> > This fixes the problem of the driver failing sanity check on silicon
> > despite working on the FPGA emulation systems.
> >
> > Fixes: 27b3b22dd98c ("crypto: ccree - add support for older HW revs")
>
> Did the above introduce a regression that is fixed by this patch
> or did it add a feature that only works with this patch?
>
>
Sort of in between - the first patch made more devices work but
unreliability (it will sometime work, sometime doesn't).
This one make it work reliably.


> In the case of the latter I would drop the Fixes tag,
> but I don't feel strongly about it.
>



>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>
> Minor not below not withstanding,
>



> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
>
> Thank you for the review and help :-)

Gilad

[-- Attachment #2: Type: text/html, Size: 3303 bytes --]

Re: [PATCH v2 1/5] crypto: ccree: correct host regs offset

[Gilad Ben-Yossef]

On Tue, May 29, 2018 at 7:12 PM, Simon Horman <horms@verge.net.au> wrote:
> On Thu, May 24, 2018 at 03:19:06PM +0100, Gilad Ben-Yossef wrote:
>> The product signature and HW revision register have different offset on the
>> older HW revisions.
>> This fixes the problem of the driver failing sanity check on silicon
>> despite working on the FPGA emulation systems.
>>
>> Fixes: 27b3b22dd98c ("crypto: ccree - add support for older HW revs")
>
> Did the above introduce a regression that is fixed by this patch
> or did it add a feature that only works with this patch?
>

Sort of in between - the first patch made more devices work but
unreliability (it will sometime work, sometime doesn't).
This one make it work reliably.

> In the case of the latter I would drop the Fixes tag,
> but I don't feel strongly about it.
>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>
> Minor not below not withstanding,
>
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Thank you for the review and help :-)

Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

[PATCH v2 2/5] crypto: ccree: better clock handling

[Gilad Ben-Yossef]

Use managed clock handling, differentiate between no clock (possibly OK)
and clock init failure (never OK) and correctly handle clock detection
being deferred.

Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
 drivers/crypto/ccree/cc_driver.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 6f93ce7..b266657 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -190,6 +190,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
 	u64 dma_mask;
 	const struct cc_hw_data *hw_rev;
 	const struct of_device_id *dev_id;
+	struct clk *clk;
 	int rc = 0;
 
 	new_drvdata = devm_kzalloc(dev, sizeof(*new_drvdata), GFP_KERNEL);
@@ -219,7 +220,24 @@ static int init_cc_resources(struct platform_device *plat_dev)
 	platform_set_drvdata(plat_dev, new_drvdata);
 	new_drvdata->plat_dev = plat_dev;
 
-	new_drvdata->clk = of_clk_get(np, 0);
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk))
+		switch (PTR_ERR(clk)) {
+		/* Clock is optional so this might be fine */
+		case -ENOENT:
+			break;
+
+		/* Clock not available, let's try again soon */
+		case -EPROBE_DEFER:
+			return -EPROBE_DEFER;
+
+		default:
+			dev_err(dev, "Error getting clock: %ld\n",
+				PTR_ERR(clk));
+			return PTR_ERR(clk);
+		}
+	new_drvdata->clk = clk;
+
 	new_drvdata->coherent = of_dma_is_coherent(np);
 
 	/* Get device resources */
-- 
2.7.4

Re: [PATCH v2 2/5] crypto: ccree: better clock handling

[Simon Horman]

On Thu, May 24, 2018 at 03:19:07PM +0100, Gilad Ben-Yossef wrote:
> Use managed clock handling, differentiate between no clock (possibly OK)
> and clock init failure (never OK) and correctly handle clock detection
> being deferred.
> 
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
>  drivers/crypto/ccree/cc_driver.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
> index 6f93ce7..b266657 100644
> --- a/drivers/crypto/ccree/cc_driver.c
> +++ b/drivers/crypto/ccree/cc_driver.c
> @@ -190,6 +190,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
>  	u64 dma_mask;
>  	const struct cc_hw_data *hw_rev;
>  	const struct of_device_id *dev_id;
> +	struct clk *clk;
>  	int rc = 0;
>  
>  	new_drvdata = devm_kzalloc(dev, sizeof(*new_drvdata), GFP_KERNEL);
> @@ -219,7 +220,24 @@ static int init_cc_resources(struct platform_device *plat_dev)
>  	platform_set_drvdata(plat_dev, new_drvdata);
>  	new_drvdata->plat_dev = plat_dev;
>  
> -	new_drvdata->clk = of_clk_get(np, 0);
> +	clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(clk))
> +		switch (PTR_ERR(clk)) {
> +		/* Clock is optional so this might be fine */
> +		case -ENOENT:
> +			break;
> +
> +		/* Clock not available, let's try again soon */
> +		case -EPROBE_DEFER:
> +			return -EPROBE_DEFER;
> +
> +		default:
> +			dev_err(dev, "Error getting clock: %ld\n",
> +				PTR_ERR(clk));
> +			return PTR_ERR(clk);
> +		}
> +	new_drvdata->clk = clk;
> +
>  	new_drvdata->coherent = of_dma_is_coherent(np);
>  
>  	/* Get device resources */
> -- 
> 2.7.4
> 

[PATCH v2 3/5] crypto: ccree: silence debug prints

[Gilad Ben-Yossef]

The cache parameter register configuration was being too verbose.
Use dev_dbg() to only provide the information if needed.

Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
 drivers/crypto/ccree/cc_driver.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index b266657..1e40e76 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -168,14 +168,14 @@ int init_cc_regs(struct cc_drvdata *drvdata, bool is_probe)
 	val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
 
 	if (is_probe)
-		dev_info(dev, "Cache params previous: 0x%08X\n", val);
+		dev_dbg(dev, "Cache params previous: 0x%08X\n", val);
 
 	cc_iowrite(drvdata, CC_REG(AXIM_CACHE_PARAMS), cache_params);
 	val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
 
 	if (is_probe)
-		dev_info(dev, "Cache params current: 0x%08X (expect: 0x%08X)\n",
-			 val, cache_params);
+		dev_dbg(dev, "Cache params current: 0x%08X (expect: 0x%08X)\n",
+			val, cache_params);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v2 3/5] crypto: ccree: silence debug prints

[Simon Horman]

On Thu, May 24, 2018 at 03:19:08PM +0100, Gilad Ben-Yossef wrote:
> The cache parameter register configuration was being too verbose.
> Use dev_dbg() to only provide the information if needed.
> 
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

[PATCH v2 4/5] clk: renesas: r8a7795: add ccree clock bindings

[Gilad Ben-Yossef]

This patch adds the clock used by the CryptoCell 630p instance in the SoC.

Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---

This patch depends upon the "clk: renesas: r8a7795: Add CR clock" patch
from Geert Uytterhoeven.

 drivers/clk/renesas/r8a7795-cpg-mssr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c b/drivers/clk/renesas/r8a7795-cpg-mssr.c
index e5b1865..a85dd50 100644
--- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
@@ -133,6 +133,7 @@ static struct mssr_mod_clk r8a7795_mod_clks[] __initdata = {
 	DEF_MOD("sys-dmac2",		 217,	R8A7795_CLK_S0D3),
 	DEF_MOD("sys-dmac1",		 218,	R8A7795_CLK_S0D3),
 	DEF_MOD("sys-dmac0",		 219,	R8A7795_CLK_S0D3),
+	DEF_MOD("sceg-pub",		 229,	R8A7795_CLK_CR),
 	DEF_MOD("cmt3",			 300,	R8A7795_CLK_R),
 	DEF_MOD("cmt2",			 301,	R8A7795_CLK_R),
 	DEF_MOD("cmt1",			 302,	R8A7795_CLK_R),
-- 
2.7.4

Re: [PATCH v2 4/5] clk: renesas: r8a7795: add ccree clock bindings

[Geert Uytterhoeven]

On Thu, May 24, 2018 at 4:19 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> This patch adds the clock used by the CryptoCell 630p instance in the SoC.
>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Thanks, queueing in clk-renesas-for-v4.19, with "bindings" dropped from the
one-line summary.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 5/5] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

Add bindings for CryptoCell instance in the SoC.

Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index d842940..3ac75db 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -528,6 +528,15 @@
 			status = "disabled";
 		};
 
+		arm_cc630p: crypto@e6601000 {
+			compatible = "arm,cryptocell-630p-ree";
+			interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
+			reg = <0x0 0xe6601000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 229>;
+			resets = <&cpg 229>;
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+		};
+
 		i2c3: i2c@e66d0000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
2.7.4

Re: [PATCH v2 5/5] arm64: dts: renesas: r8a7795: add ccree binding

[Simon Horman]

On Thu, May 24, 2018 at 03:19:10PM +0100, Gilad Ben-Yossef wrote:
> Add bindings for CryptoCell instance in the SoC.
> 
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>

In so far as I can review the details of this (which is not much) this
looks fine to me. I am, however, a little unclear in when it should be
accepted.

> ---
>  arch/arm64/boot/dts/renesas/r8a7795.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> index d842940..3ac75db 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> @@ -528,6 +528,15 @@
>  			status = "disabled";
>  		};
>  
> +		arm_cc630p: crypto@e6601000 {
> +			compatible = "arm,cryptocell-630p-ree";
> +			interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
> +			reg = <0x0 0xe6601000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 229>;
> +			resets = <&cpg 229>;
> +			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> +		};
> +
>  		i2c3: i2c@e66d0000 {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> -- 
> 2.7.4
> 

Re: [PATCH v2 5/5] arm64: dts: renesas: r8a7795: add ccree binding

[Gilad Ben-Yossef]

On Tue, May 29, 2018 at 7:19 PM, Simon Horman <horms@verge.net.au> wrote:
> On Thu, May 24, 2018 at 03:19:10PM +0100, Gilad Ben-Yossef wrote:
>> Add bindings for CryptoCell instance in the SoC.
>>
>> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>
> In so far as I can review the details of this (which is not much) this
> looks fine to me. I am, however, a little unclear in when it should be
> accepted.

Since Herbert Xu ACKed the driver changes, I would say the only gating
commit is Geert's CR clock patch.
If that one is in, than I would say this one should go in as well.

Thanks,
Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH v2 5/5] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 31, 2018 at 1:55 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> On Tue, May 29, 2018 at 7:19 PM, Simon Horman <horms@verge.net.au> wrote:
>> On Thu, May 24, 2018 at 03:19:10PM +0100, Gilad Ben-Yossef wrote:
>>> Add bindings for CryptoCell instance in the SoC.
>>>
>>> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>>
>> In so far as I can review the details of this (which is not much) this
>> looks fine to me. I am, however, a little unclear in when it should be
>> accepted.
>
> Since Herbert Xu ACKed the driver changes, I would say the only gating
> commit is Geert's CR clock patch.

These are queued for v4.19.

> If that one is in, than I would say this one should go in as well.

As the device node now has a power-domains property, the genpd code will
try to attach it to the CPG/MSSR PM Domain, which is a clock domain.
In the absence of the clock patch, the device's module clock cannot be
found, and dev_pm_domain_attach() and thus platform_drv_probe() will fail,
before calling the device driver's .probe() function.

So there is no longer a dependency on the clock patch, and the DT patch can
go in in parallel (although I prefer its subject to be changed
s/binding/device device/).

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 5/5] arm64: dts: renesas: r8a7795: add ccree binding

[Simon Horman]

On Fri, Jun 01, 2018 at 10:12:24AM +0200, Geert Uytterhoeven wrote:
> Hi Gilad,
> 
> On Thu, May 31, 2018 at 1:55 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > On Tue, May 29, 2018 at 7:19 PM, Simon Horman <horms@verge.net.au> wrote:
> >> On Thu, May 24, 2018 at 03:19:10PM +0100, Gilad Ben-Yossef wrote:
> >>> Add bindings for CryptoCell instance in the SoC.
> >>>
> >>> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> >>
> >> In so far as I can review the details of this (which is not much) this
> >> looks fine to me. I am, however, a little unclear in when it should be
> >> accepted.
> >
> > Since Herbert Xu ACKed the driver changes, I would say the only gating
> > commit is Geert's CR clock patch.
> 
> These are queued for v4.19.
> 
> > If that one is in, than I would say this one should go in as well.
> 
> As the device node now has a power-domains property, the genpd code will
> try to attach it to the CPG/MSSR PM Domain, which is a clock domain.
> In the absence of the clock patch, the device's module clock cannot be
> found, and dev_pm_domain_attach() and thus platform_drv_probe() will fail,
> before calling the device driver's .probe() function.
> 
> So there is no longer a dependency on the clock patch, and the DT patch can
> go in in parallel (although I prefer its subject to be changed
> s/binding/device device/).

Thanks, I have applied the following (but may not push until next week).

From: Gilad Ben-Yossef <gilad@benyossef.com>
Subject: [PATCH] arm64: dts: renesas: r8a7795: add ccree to device tree

Add bindings for CryptoCell instance in the SoC.

Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index d842940b2f43..3ac75dbf2d93 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -528,6 +528,15 @@
 			status = "disabled";
 		};
 
+		arm_cc630p: crypto@e6601000 {
+			compatible = "arm,cryptocell-630p-ree";
+			interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
+			reg = <0x0 0xe6601000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 229>;
+			resets = <&cpg 229>;
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+		};
+
 		i2c3: i2c@e66d0000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
2.11.0

Re: [PATCH v2 5/5] arm64: dts: renesas: r8a7795: add ccree binding

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 24, 2018 at 4:19 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> Add bindings for CryptoCell instance in the SoC.
>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>

Thanks for the update!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling

[Herbert Xu]

On Thu, May 24, 2018 at 03:19:05PM +0100, Gilad Ben-Yossef wrote:
> The patch set enables the use of CryptoCell found in some Renesas R-Car
> Salvator-X boards and fixes some driver issues uncovered that prevented
> to work properly.
> 
> Changes from v1:
> - Properly fix the bug that caused us to read a bad signature register
>   rather than dropping the check
> - Proper DT fields as indicated by Geert Uytterhoeven.
> - Better clock enabling as suggested by Geert Uytterhoeven.
> 
> Note! the last two patches in the set depend on the
> "clk: renesas: r8a7795: Add CR clock" patch from Geert Uytterhoeven.

Patches 1-3 applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling

[Geert Uytterhoeven]

Hi Gilad,

On Thu, May 24, 2018 at 4:19 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> The patch set enables the use of CryptoCell found in some Renesas R-Car
> Salvator-X boards and fixes some driver issues uncovered that prevented
> to work properly.

With DEBUG enabled on R-Car H3, I see lots of

    ccree e6601000.crypto: IRR includes unknown cause bits (0x00000098)
    ccree e6601000.crypto: IRR includes unknown cause bits (0x000000C0)
    ccree e6601000.crypto: IRR includes unknown cause bits (0x000000D0)
    ccree e6601000.crypto: IRR includes unknown cause bits (0x000000D8)
    ccree e6601000.crypto: IRR includes unknown cause bits (0x000000E0)
    ccree e6601000.crypto: IRR includes unknown cause bits (0x000000F0)
    ccree e6601000.crypto: IRR includes unknown cause bits (0x000000F8)

during boot. Is that expected?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling

[Gilad Ben-Yossef]

On Tue, Jun 19, 2018 at 3:58 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Gilad,
>
> On Thu, May 24, 2018 at 4:19 PM Gilad Ben-Yossef <gilad@benyossef.com> wr=
ote:
>> The patch set enables the use of CryptoCell found in some Renesas R-Car
>> Salvator-X boards and fixes some driver issues uncovered that prevented
>> to work properly.
>
> With DEBUG enabled on R-Car H3, I see lots of
>
>     ccree e6601000.crypto: IRR includes unknown cause bits (0x00000098)
>     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000C0)
>     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000D0)
>     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000D8)
>     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000E0)
>     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000F0)
>     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000F8)
>
> during boot. Is that expected?

Yes. The condition itself it is reporting is not necessarily bad. It
means that driver
did not act on certain HW notification during interrupts and that's
OK, we don't act on all of them
depending on configuration - e.g. if you have CONFIG_FIPS enabled and
an active TEE module or not.

I can rate_limit the message if it bothers you but other than that it
is a harmless debug print.

Thanks,
Gilad



--=20
Gilad Ben-Yossef
Chief Coffee Drinker

values of =CE=B2 will give rise to dom!

Re: [PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling

[Simon Horman]

On Tue, Jun 19, 2018 at 04:57:15PM +0300, Gilad Ben-Yossef wrote:
> On Tue, Jun 19, 2018 at 3:58 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > Hi Gilad,
> >
> > On Thu, May 24, 2018 at 4:19 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> >> The patch set enables the use of CryptoCell found in some Renesas R-Car
> >> Salvator-X boards and fixes some driver issues uncovered that prevented
> >> to work properly.
> >
> > With DEBUG enabled on R-Car H3, I see lots of
> >
> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x00000098)
> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000C0)
> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000D0)
> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000D8)
> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000E0)
> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000F0)
> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000F8)
> >
> > during boot. Is that expected?
> 
> Yes. The condition itself it is reporting is not necessarily bad. It
> means that driver
> did not act on certain HW notification during interrupts and that's
> OK, we don't act on all of them
> depending on configuration - e.g. if you have CONFIG_FIPS enabled and
> an active TEE module or not.
> 
> I can rate_limit the message if it bothers you but other than that it
> is a harmless debug print.

Rate limiting sounds like an excellent idea to me.

Re: [PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling

[Gilad Ben-Yossef]

On Wed, Jun 20, 2018 at 11:51 AM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Jun 19, 2018 at 04:57:15PM +0300, Gilad Ben-Yossef wrote:
>> On Tue, Jun 19, 2018 at 3:58 PM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>> > Hi Gilad,
>> >
>> > On Thu, May 24, 2018 at 4:19 PM Gilad Ben-Yossef <gilad@benyossef.com>=
 wrote:
>> >> The patch set enables the use of CryptoCell found in some Renesas R-C=
ar
>> >> Salvator-X boards and fixes some driver issues uncovered that prevent=
ed
>> >> to work properly.
>> >
>> > With DEBUG enabled on R-Car H3, I see lots of
>> >
>> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x00000098=
)
>> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000C0=
)
>> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000D0=
)
>> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000D8=
)
>> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000E0=
)
>> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000F0=
)
>> >     ccree e6601000.crypto: IRR includes unknown cause bits (0x000000F8=
)
>> >
>> > during boot. Is that expected?
>>
>> Yes. The condition itself it is reporting is not necessarily bad. It
>> means that driver
>> did not act on certain HW notification during interrupts and that's
>> OK, we don't act on all of them
>> depending on configuration - e.g. if you have CONFIG_FIPS enabled and
>> an active TEE module or not.
>>
>> I can rate_limit the message if it bothers you but other than that it
>> is a harmless debug print.
>
> Rate limiting sounds like an excellent idea to me.

Will do.

Thanks!
Gilad



--=20
Gilad Ben-Yossef
Chief Coffee Drinker

values of =CE=B2 will give rise to dom!