Re: [PATCH 5/7] staging: ccree: add clock management support

[Gilad Ben-Yossef]

On Thu, Jun 22, 2017 at 11:58 AM, Dan Carpenter
 wrote:
> On Thu, Jun 22, 2017 at 10:07:51AM +0300, Gilad Ben-Yossef wrote:
>> +int cc_clk_on(struct ssi_drvdata *drvdata)
>> +{
>> + int rc = 0;
>> + struct clk *clk = drvdata->clk;
>> +
>> + if (IS_ERR(clk))
>> + /* No all devices have a clock associated with CCREE */
>> + goto out;
>
> Ugh...  I hate this.  The "goto out;" here is a waste of time
> do-nothing-goto that returns diretly.  It's equivalent to "return 0;".
> Is that intended?  Even with the comment, it's not clear...
>
> People think do nothing gotos are a great idea but from reviewing tons
> and tons of real life errors, I can assure you that in real life (as
> opposed to theory) they don't prevent any future bugs and only introduce
> "forgot to set the error code" bugs.

I see your point and the code is indeed clearer this way.

Will fix.

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 5/7] staging: ccree: add clock management support

[Dan Carpenter]

On Thu, Jun 22, 2017 at 10:07:51AM +0300, Gilad Ben-Yossef wrote:
> +int cc_clk_on(struct ssi_drvdata *drvdata)
> +{
> + int rc = 0;
> + struct clk *clk = drvdata->clk;
> +
> + if (IS_ERR(clk))
> + /* No all devices have a clock associated with CCREE */
> + goto out;

Ugh...  I hate this.  The "goto out;" here is a waste of time
do-nothing-goto that returns diretly.  It's equivalent to "return 0;".
Is that intended?  Even with the comment, it's not clear...

People think do nothing gotos are a great idea but from reviewing tons
and tons of real life errors, I can assure you that in real life (as
opposed to theory) they don't prevent any future bugs and only introduce
"forgot to set the error code" bugs.

The indenting is messed up and multi-line indents get curly braces.

> +
> + rc = clk_prepare_enable(clk);
> + if (rc) {
> + SSI_LOG_ERR("error enabling clock\n");
> + clk_disable_unprepare(clk);

Don't unprepare something that hasn't been prepared.

> + }
> +
> +out:
> + return rc;
> +}

int cc_clk_on(struct ssi_drvdata *drvdata)
{
struct clk *clk = drvdata->clk;
int rc;

if (IS_ERR(clk)) {
/* Not all devices have a clock associated with CCREE */
return 0;
}

rc = clk_prepare_enable(clk);
if (rc)
return rc;

return 0;
}

regards,
dan carpenter

[PATCH 5/7] staging: ccree: add clock management support

[Gilad Ben-Yossef]

Some SoC which implement CryptoCell have a dedicated clock
tied to it, some do not. Implement clock support if exists
based on device tree data and tie power management to it.

Signed-off-by: Gilad Ben-Yossef 
---
 drivers/staging/ccree/Makefile |  2 +-
 drivers/staging/ccree/ssi_driver.c | 43 +++
 drivers/staging/ccree/ssi_driver.h |  4 +++
 drivers/staging/ccree/ssi_pm.c | 13 +
 drivers/staging/ccree/ssi_pm_ext.c | 60 --
 drivers/staging/ccree/ssi_pm_ext.h | 33 -
 6 files changed, 50 insertions(+), 105 deletions(-)
 delete mode 100644 drivers/staging/ccree/ssi_pm_ext.c
 delete mode 100644 drivers/staging/ccree/ssi_pm_ext.h

diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile
index 44f3e3e..318c2b3 100644
--- a/drivers/staging/ccree/Makefile
+++ b/drivers/staging/ccree/Makefile
@@ -1,3 +1,3 @@
 obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o
-ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o 
ssi_cipher.o ssi_hash.o ssi_aead.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o 
ssi_pm_ext.o
+ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o 
ssi_cipher.o ssi_hash.o ssi_aead.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o
 ccree-$(CCREE_FIPS_SUPPORT) += ssi_fips.o ssi_fips_ll.o ssi_fips_ext.o 
ssi_fips_local.o
diff --git a/drivers/staging/ccree/ssi_driver.c 
b/drivers/staging/ccree/ssi_driver.c
index 5a62b4f..f1275a6 100644
--- a/drivers/staging/ccree/ssi_driver.c
+++ b/drivers/staging/ccree/ssi_driver.c
@@ -57,6 +57,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "ssi_config.h"
 #include "ssi_driver.h"
@@ -269,6 +270,7 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
hw_rev = (struct cc_hw_data *)dev_id->data;
new_drvdata->hw_rev_name = hw_rev->name;
new_drvdata->hw_rev = hw_rev->rev;
+   new_drvdata->clk = of_clk_get(np, 0);
 
if (hw_rev->rev >= CC_HW_REV_712) {
new_drvdata->hash_len_sz = HASH_LEN_SIZE_712;
@@ -338,6 +340,10 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
 
new_drvdata->plat_dev = plat_dev;
 
+   rc = cc_clk_on(new_drvdata);
+   if (rc)
+   goto init_cc_res_err;
+
if(new_drvdata->plat_dev->dev.dma_mask == NULL)
{
new_drvdata->plat_dev->dev.dma_mask = & 
new_drvdata->plat_dev->dev.coherent_dma_mask;
@@ -504,14 +510,11 @@ static void cleanup_cc_resources(struct platform_device 
*plat_dev)
ssi_sysfs_fini();
 #endif
 
-   /* Mask all interrupts */
-   WRITE_REGISTER(drvdata->cc_base + CC_REG_OFFSET(HOST_RGF, HOST_IMR),
-   0x);
+   fini_cc_regs(drvdata);
+   cc_clk_off(drvdata);
free_irq(drvdata->res_irq->start, drvdata);
drvdata->res_irq = NULL;
 
-   fini_cc_regs(drvdata);
-
if (drvdata->cc_base != NULL) {
iounmap(drvdata->cc_base);
release_mem_region(drvdata->res_mem->start,
@@ -524,6 +527,36 @@ static void cleanup_cc_resources(struct platform_device 
*plat_dev)
dev_set_drvdata(_dev->dev, NULL);
 }
 
+int cc_clk_on(struct ssi_drvdata *drvdata)
+{
+   int rc = 0;
+   struct clk *clk = drvdata->clk;
+
+   if (IS_ERR(clk))
+   /* No all devices have a clock associated with CCREE */
+   goto out;
+
+   rc = clk_prepare_enable(clk);
+   if (rc) {
+   SSI_LOG_ERR("error enabling clock\n");
+   clk_disable_unprepare(clk);
+   }
+
+out:
+   return rc;
+}
+
+void cc_clk_off(struct ssi_drvdata *drvdata)
+{
+   struct clk *clk = drvdata->clk;
+
+   if (IS_ERR(clk))
+   /* No all devices have a clock associated with CCREE */
+   return;
+
+   clk_disable_unprepare(clk);
+}
+
 static int ccree_probe(struct platform_device *plat_dev)
 {
int rc;
diff --git a/drivers/staging/ccree/ssi_driver.h 
b/drivers/staging/ccree/ssi_driver.h
index 52ac43c..a59df59 100644
--- a/drivers/staging/ccree/ssi_driver.h
+++ b/drivers/staging/ccree/ssi_driver.h
@@ -36,6 +36,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Registers definitions from shared/hw/ree_include */
 #include "dx_reg_base_host.h"
@@ -156,6 +157,7 @@ struct ssi_drvdata {
enum cc_hw_rev hw_rev;
u32 hash_len_sz;
u32 axim_mon_offset;
+   struct clk *clk;
 };
 
 struct ssi_crypto_alg {
@@ -201,6 +203,8 @@ void dump_byte_array(const char *name, const u8 *the_array, 
unsigned long size);
 
 int init_cc_regs(struct ssi_drvdata *drvdata, bool is_probe);
 void fini_cc_regs(struct ssi_drvdata *drvdata);
+int cc_clk_on(struct ssi_drvdata *drvdata);
+void cc_clk_off(struct ssi_drvdata *drvdata);
 
 static inline void set_queue_last_ind(struct ssi_drvdata *drvdata,
  struct cc_hw_desc *pdesc)
diff --git a/drivers/staging/ccree/ssi_pm.c