[PATCH 1/9] crypto: ux500/hash - Prepare clock before enabling it

[PATCH 1/9] crypto: ux500/hash - Prepare clock before enabling it

[Lee Jones]

If we fail to prepare the ux500-hash clock before enabling it the
platform will fail to boot. Here we insure this happens.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: Andreas Westin <andreas.westin@stericsson.com>
Cc: linux-crypto@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/crypto/ux500/hash/hash_core.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/ux500/hash/hash_core.c b/drivers/crypto/ux500/hash/hash_core.c
index 632c333..118386a 100644
--- a/drivers/crypto/ux500/hash/hash_core.c
+++ b/drivers/crypto/ux500/hash/hash_core.c
@@ -308,10 +308,10 @@ static int hash_disable_power(
 		device_data->restore_dev_state = true;
 	}
 
-	clk_disable(device_data->clk);
+	clk_disable_unprepare(device_data->clk);
 	ret = regulator_disable(device_data->regulator);
 	if (ret)
-		dev_err(dev, "[%s] regulator_disable() failed!", __func__);
+		dev_err(dev, "[%s] regulator_disable_unprepare() failed!", __func__);
 
 	device_data->power_state = false;
 
@@ -344,9 +344,9 @@ static int hash_enable_power(
 					__func__);
 			goto out;
 		}
-		ret = clk_enable(device_data->clk);
+		ret = clk_prepare_enable(device_data->clk);
 		if (ret) {
-			dev_err(dev, "[%s]: clk_enable() failed!",
+			dev_err(dev, "[%s]: clk_prepare_enable() failed!",
 					__func__);
 			ret = regulator_disable(
 					device_data->regulator);
-- 
1.7.10.4

[PATCH 1/9 v2] crypto: ux500/hash - Prepare clock before enabling it

[Lee Jones]

Slight change of plan for v2.

Now we're doing a seperate clk_prepare(), as the clk_enable() in the
previous patch turned out to be called inside a spin_lock().

Arnd, can you confirm your Ack please?

---

crypto: ux500/hash - Prepare clock before enabling it

If we fail to prepare the ux500-hash clock before enabling it the
platform will fail to boot. Here we insure this happens.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: Andreas Westin <andreas.westin@stericsson.com>
Cc: linux-crypto@vger.kernel.org
Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/drivers/crypto/ux500/hash/hash_core.c b/drivers/crypto/ux500/hash/hash_core.c
index 632c333..1e8b2f3 100644
--- a/drivers/crypto/ux500/hash/hash_core.c
+++ b/drivers/crypto/ux500/hash/hash_core.c
@@ -1727,11 +1727,17 @@ static int ux500_hash_probe(struct platform_device *pdev)
 		goto out_regulator;
 	}
 
+	ret = clk_prepare(device_data->clk);
+	if (ret) {
+		dev_err(dev, "[%s] clk_prepare() failed!", __func__);
+		goto out_clk;
+	}
+
 	/* Enable device power (and clock) */
 	ret = hash_enable_power(device_data, false);
 	if (ret) {
 		dev_err(dev, "[%s]: hash_enable_power() failed!", __func__);
-		goto out_clk;
+		goto out_clk_unprepare;
 	}
 
 	ret = hash_check_hw(device_data);
@@ -1763,6 +1769,9 @@ static int ux500_hash_probe(struct platform_device *pdev)
 out_power:
 	hash_disable_power(device_data, false);
 
+out_clk_unprepare:
+	clk_unprepare(device_data->clk);
+
 out_clk:
 	clk_put(device_data->clk);
 
@@ -1827,6 +1836,7 @@ static int ux500_hash_remove(struct platform_device *pdev)
 		dev_err(dev, "[%s]: hash_disable_power() failed",
 			__func__);
 
+	clk_unprepare(device_data->clk);
 	clk_put(device_data->clk);
 	regulator_put(device_data->regulator);

Re: [PATCH 1/9 v2] crypto: ux500/hash - Prepare clock before enabling it

[Arnd Bergmann]

On Friday 19 April 2013, Lee Jones wrote:
> Slight change of plan for v2.
> 
> Now we're doing a seperate clk_prepare(), as the clk_enable() in the
> previous patch turned out to be called inside a spin_lock().
> 
> Arnd, can you confirm your Ack please?
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 1/9 v2] crypto: ux500/hash - Prepare clock before enabling it

[Linus Walleij]

On Fri, Apr 19, 2013 at 2:24 PM, Lee Jones <lee.jones@linaro.org> wrote:
> Slight change of plan for v2.
>
> Now we're doing a seperate clk_prepare(), as the clk_enable() in the
> previous patch turned out to be called inside a spin_lock().
>
> Arnd, can you confirm your Ack please?

Do you really want letters to Arnd to be part of the commit log?

>
> ---

Note: stuff following the three dashes (---) will be *omitted*
from the change log. This seems to be turned upside-down.

>
> crypto: ux500/hash - Prepare clock before enabling it
>
> If we fail to prepare the ux500-hash clock before enabling it the
> platform will fail to boot. Here we insure this happens.
>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Andreas Westin <andreas.westin@stericsson.com>
> Cc: linux-crypto@vger.kernel.org
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Pls include Ulf Hansson <ulf.hansson@linaro.org> on this patch.

Yours,
Linus Walleij

Re: [PATCH 1/9 v2] crypto: ux500/hash - Prepare clock before enabling it

[Lee Jones]

On Thu, 25 Apr 2013, Linus Walleij wrote:

> On Fri, Apr 19, 2013 at 2:24 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > Slight change of plan for v2.
> >
> > Now we're doing a seperate clk_prepare(), as the clk_enable() in the
> > previous patch turned out to be called inside a spin_lock().
> >
> > Arnd, can you confirm your Ack please?
> 
> Do you really want letters to Arnd to be part of the commit log?
> 
> >
> > ---
> 
> Note: stuff following the three dashes (---) will be *omitted*
> from the change log. This seems to be turned upside-down.

Ah, I didn't know that, thanks.

This patch wasn't due for 'plucking', just reviewing.

> > crypto: ux500/hash - Prepare clock before enabling it
> >
> > If we fail to prepare the ux500-hash clock before enabling it the
> > platform will fail to boot. Here we insure this happens.
> >
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Andreas Westin <andreas.westin@stericsson.com>
> > Cc: linux-crypto@vger.kernel.org
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Pls include Ulf Hansson <ulf.hansson@linaro.org> on this patch.

Sure.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/9] crypto: ux500/hash - Set DMA configuration though dma_slave_config()

[Lee Jones]

The DMA controller currently takes configuration information from
information passed though dma_channel_request(), but it shouldn't.
Using the API, the DMA channel should only be configured during
a dma_slave_config() call.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: Andreas Westin <andreas.westin@stericsson.com>
Cc: linux-crypto@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/crypto/ux500/hash/hash_alg.h  |    5 ++++-
 drivers/crypto/ux500/hash/hash_core.c |   10 ++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ux500/hash/hash_alg.h b/drivers/crypto/ux500/hash/hash_alg.h
index cd9351c..be6eb54 100644
--- a/drivers/crypto/ux500/hash/hash_alg.h
+++ b/drivers/crypto/ux500/hash/hash_alg.h
@@ -11,6 +11,7 @@
 #include <linux/bitops.h>
 
 #define HASH_BLOCK_SIZE			64
+#define HASH_DMA_FIFO			4
 #define HASH_DMA_ALIGN_SIZE		4
 #define HASH_DMA_PERFORMANCE_MIN_SIZE	1024
 #define HASH_BYTES_PER_WORD		4
@@ -347,7 +348,8 @@ struct hash_req_ctx {
 
 /**
  * struct hash_device_data - structure for a hash device.
- * @base:		Pointer to the hardware base address.
+ * @base:		Pointer to virtual base address of the hash device.
+ * @phybase:		Pointer to physical memory location of the hash device.
  * @list_node:		For inclusion in klist.
  * @dev:		Pointer to the device dev structure.
  * @ctx_lock:		Spinlock for current_ctx.
@@ -361,6 +363,7 @@ struct hash_req_ctx {
  */
 struct hash_device_data {
 	struct hash_register __iomem	*base;
+	phys_addr_t             phybase;
 	struct klist_node	list_node;
 	struct device		*dev;
 	struct spinlock		ctx_lock;
diff --git a/drivers/crypto/ux500/hash/hash_core.c b/drivers/crypto/ux500/hash/hash_core.c
index 118386a..f9126f4 100644
--- a/drivers/crypto/ux500/hash/hash_core.c
+++ b/drivers/crypto/ux500/hash/hash_core.c
@@ -123,6 +123,13 @@ static void hash_dma_setup_channel(struct hash_device_data *device_data,
 				struct device *dev)
 {
 	struct hash_platform_data *platform_data = dev->platform_data;
+	struct dma_slave_config conf = {
+		.direction = DMA_MEM_TO_DEV,
+		.dst_addr = device_data->phybase + HASH_DMA_FIFO,
+		.dst_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES,
+		.dst_maxburst = 16,
+        };
+
 	dma_cap_zero(device_data->dma.mask);
 	dma_cap_set(DMA_SLAVE, device_data->dma.mask);
 
@@ -132,6 +139,8 @@ static void hash_dma_setup_channel(struct hash_device_data *device_data,
 				platform_data->dma_filter,
 				device_data->dma.cfg_mem2hash);
 
+	dmaengine_slave_config(device_data->dma.chan_mem2hash, &conf);
+
 	init_completion(&device_data->dma.complete);
 }
 
@@ -1700,6 +1709,7 @@ static int ux500_hash_probe(struct platform_device *pdev)
 		goto out_kfree;
 	}
 
+	device_data->phybase = res->start;
 	device_data->base = ioremap(res->start, resource_size(res));
 	if (!device_data->base) {
 		dev_err(dev, "[%s] ioremap() failed!",
-- 
1.7.10.4

Re: [PATCH 2/9] crypto: ux500/hash - Set DMA configuration though dma_slave_config()

[Linus Walleij]

Pls include Magnus.p.persson@stericsson.com on all these crypto/hash
postings.

On Thu, Apr 18, 2013 at 12:26 PM, Lee Jones <lee.jones@linaro.org> wrote:

> The DMA controller currently takes configuration information from
> information passed though dma_channel_request(), but it shouldn't.
> Using the API, the DMA channel should only be configured during
> a dma_slave_config() call.
>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Andreas Westin <andreas.westin@stericsson.com>
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

(...)
>  #define HASH_BLOCK_SIZE                        64
> +#define HASH_DMA_FIFO                  4

This is an address so write 0x0004 here please. Some hex notation atleast.

>  /**
>   * struct hash_device_data - structure for a hash device.
> - * @base:              Pointer to the hardware base address.
> + * @base:              Pointer to virtual base address of the hash device.
> + * @phybase:           Pointer to physical memory location of the hash device.
>   * @list_node:         For inclusion in klist.
>   * @dev:               Pointer to the device dev structure.
>   * @ctx_lock:          Spinlock for current_ctx.
> @@ -361,6 +363,7 @@ struct hash_req_ctx {
>   */
>  struct hash_device_data {
>         struct hash_register __iomem    *base;
> +       phys_addr_t             phybase;

What you pass to the config function is a dma_addr_t actually.

This is the same thing on the platform, but generally:

phys_addr_t = in the address space as the memory controller sees it.
dma_addr_t = in the address space as the DMA controller sees it.

Often the same. Not always. So use dma_addr_t.

Apart from this a real nice patch!

Yours,
Linus Walleij

[PATCH 4/9] crypto: ux500/cryp - Prepare clock before enabling it

[Lee Jones]

If we fail to prepare the ux500-cryp clock before enabling it the
platform will fail to boot. Here we insure this happens.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: Andreas Westin <andreas.westin@stericsson.com>
Cc: linux-crypto@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/crypto/ux500/cryp/cryp_core.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/ux500/cryp/cryp_core.c b/drivers/crypto/ux500/cryp/cryp_core.c
index 8bc5fef..a56cbc4 100644
--- a/drivers/crypto/ux500/cryp/cryp_core.c
+++ b/drivers/crypto/ux500/cryp/cryp_core.c
@@ -670,7 +670,7 @@ static int cryp_disable_power(struct device *dev,
 	}
 	spin_unlock(&device_data->ctx_lock);
 
-	clk_disable(device_data->clk);
+	clk_disable_unprepare(device_data->clk);
 	ret = regulator_disable(device_data->pwr_regulator);
 	if (ret)
 		dev_err(dev, "[%s]: "
@@ -703,9 +703,9 @@ static int cryp_enable_power(
 			goto out;
 		}
 
-		ret = clk_enable(device_data->clk);
+		ret = clk_prepare_enable(device_data->clk);
 		if (ret) {
-			dev_err(dev, "[%s]: clk_enable() failed!",
+			dev_err(dev, "[%s]: clk_prepare_enable() failed!",
 					__func__);
 			regulator_disable(device_data->pwr_regulator);
 			goto out;
-- 
1.7.10.4

[PATCH 4/9 v2] crypto: ux500/cryp - Prepare clock before enabling it

[Lee Jones]

Slight change of plan for v2.

Now we're doing a seperate clk_prepare(), as the clk_enable() in the
previous patch turned out to be called inside a spin_lock().

Arnd, can you confirm your Ack please?

----

crypto: ux500/cryp - Prepare clock before enabling it
    
If we fail to prepare the ux500-cryp clock before enabling it the
platform will fail to boot. Here we insure this happens.
    
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: Andreas Westin <andreas.westin@stericsson.com>
Cc: linux-crypto@vger.kernel.org
Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/drivers/crypto/ux500/cryp/cryp_core.c b/drivers/crypto/ux500/cryp/cryp_core.c
index 22c9063..bf78d60 100644
--- a/drivers/crypto/ux500/cryp/cryp_core.c
+++ b/drivers/crypto/ux500/cryp/cryp_core.c
@@ -1459,11 +1459,17 @@ static int ux500_cryp_probe(struct platform_device *pdev)
 		goto out_regulator;
 	}
 
+	ret = clk_prepare(device_data->clk);
+	if (ret) {
+		dev_err(dev, "[%s]: clk_prepare() failed!", __func__);
+		goto out_clk;
+	}
+
 	/* Enable device power (and clock) */
 	ret = cryp_enable_power(device_data->dev, device_data, false);
 	if (ret) {
 		dev_err(dev, "[%s]: cryp_enable_power() failed!", __func__);
-		goto out_clk;
+		goto out_clk_unprepare;
 	}
 
 	cryp_error = cryp_check(device_data);
@@ -1524,6 +1530,9 @@ static int ux500_cryp_probe(struct platform_device *pdev)
 out_power:
 	cryp_disable_power(device_data->dev, device_data, false);
 
+out_clk_unprepare:
+	clk_unprepare(device_data->clk);
+
 out_clk:
 	clk_put(device_data->clk);
 
@@ -1594,6 +1603,7 @@ static int ux500_cryp_remove(struct platform_device *pdev)
 		dev_err(&pdev->dev, "[%s]: cryp_disable_power() failed",
 			__func__);
 
+	clk_unprepare(device_data->clk);
 	clk_put(device_data->clk);
 	regulator_put(device_data->pwr_regulator);

Re: [PATCH 4/9 v2] crypto: ux500/cryp - Prepare clock before enabling it

[Arnd Bergmann]

On Friday 19 April 2013, Lee Jones wrote:
> Slight change of plan for v2.
> 
> Now we're doing a seperate clk_prepare(), as the clk_enable() in the
> previous patch turned out to be called inside a spin_lock().
> 
> Arnd, can you confirm your Ack please?
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 4/9 v2] crypto: ux500/cryp - Prepare clock before enabling it

[Linus Walleij]

On Fri, Apr 19, 2013 at 2:22 PM, Lee Jones <lee.jones@linaro.org> wrote:

> Slight change of plan for v2.
>
> Now we're doing a seperate clk_prepare(), as the clk_enable() in the
> previous patch turned out to be called inside a spin_lock().
>
> Arnd, can you confirm your Ack please?
>
> ----
>
> crypto: ux500/cryp - Prepare clock before enabling it
>
> If we fail to prepare the ux500-cryp clock before enabling it the
> platform will fail to boot. Here we insure this happens.
>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Andreas Westin <andreas.westin@stericsson.com>
> Cc: linux-crypto@vger.kernel.org
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Same formatting problems as the other prepare/enable
patch. Include Ulf Hansson.

(Apart from this it looks OK.)

Yours,
Linus Walleij

[PATCH 5/9] crypto: ux500/cryp - Fix compile error

[Lee Jones]

Clearly this driver hasn't been tested, or even enabled in a while.

drivers/crypto/ux500/cryp/cryp_core.c:1771:3:
	error: request for member ‘pm’ in something not a structure or union

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: Andreas Westin <andreas.westin@stericsson.com>
Cc: linux-crypto@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/crypto/ux500/cryp/cryp_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/ux500/cryp/cryp_core.c b/drivers/crypto/ux500/cryp/cryp_core.c
index a56cbc4..444deaf 100644
--- a/drivers/crypto/ux500/cryp/cryp_core.c
+++ b/drivers/crypto/ux500/cryp/cryp_core.c
@@ -1750,7 +1750,7 @@ static struct platform_driver cryp_driver = {
 	.shutdown = ux500_cryp_shutdown,
 	.driver = {
 		.owner = THIS_MODULE,
-		.name  = "cryp1"
+		.name  = "cryp1",
 		.pm    = &ux500_cryp_pm,
 	}
 };
-- 
1.7.10.4

Re: [PATCH 5/9] crypto: ux500/cryp - Fix compile error

[Linus Walleij]

On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <lee.jones@linaro.org> wrote:

> Clearly this driver hasn't been tested, or even enabled in a while.
>
> drivers/crypto/ux500/cryp/cryp_core.c:1771:3:
>         error: request for member ‘pm’ in something not a structure or union
>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Andreas Westin <andreas.westin@stericsson.com>
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

NAK. I already fixed this. It is in v3.9-rc7.

Yours,
Linus Walleij

Re: [PATCH 5/9] crypto: ux500/cryp - Fix compile error

[Lee Jones]

On Thu, 25 Apr 2013, Linus Walleij wrote:

> On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > Clearly this driver hasn't been tested, or even enabled in a while.
> >
> > drivers/crypto/ux500/cryp/cryp_core.c:1771:3:
> >         error: request for member ‘pm’ in something not a structure or union
> >
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Andreas Westin <andreas.westin@stericsson.com>
> > Cc: linux-crypto@vger.kernel.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> NAK. I already fixed this. It is in v3.9-rc7.

Yes, I saw it when I rebased. I've dropped the patch.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Lee Jones]

The DMA controller currently takes configuration information from
information passed though dma_channel_request(), but it shouldn't.
Using the API, the DMA channel should only be configured during
a dma_slave_config() call.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: Andreas Westin <andreas.westin@stericsson.com>
Cc: linux-crypto@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/crypto/ux500/cryp/cryp.h      |    7 ++++++-
 drivers/crypto/ux500/cryp/cryp_core.c |   17 +++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ux500/cryp/cryp.h b/drivers/crypto/ux500/cryp/cryp.h
index 14cfd05..49b2095 100644
--- a/drivers/crypto/ux500/cryp/cryp.h
+++ b/drivers/crypto/ux500/cryp/cryp.h
@@ -114,6 +114,9 @@ enum cryp_status_id {
 };
 
 /* Cryp DMA interface */
+#define HASH_DMA_TX_FIFO	0x08
+#define HASH_DMA_RX_FIFO	0x10
+
 enum cryp_dma_req_type {
 	CRYP_DMA_DISABLE_BOTH,
 	CRYP_DMA_ENABLE_IN_DATA,
@@ -217,7 +220,8 @@ struct cryp_dma {
 
 /**
  * struct cryp_device_data - structure for a cryp device.
- * @base: Pointer to the hardware base address.
+ * @base: Pointer to virtual base address of the cryp device.
+ * @phybase: Pointer to physical memory location of the cryp device.
  * @dev: Pointer to the devices dev structure.
  * @clk: Pointer to the device's clock control.
  * @pwr_regulator: Pointer to the device's power control.
@@ -232,6 +236,7 @@ struct cryp_dma {
  */
 struct cryp_device_data {
 	struct cryp_register __iomem *base;
+	phys_addr_t phybase;
 	struct device *dev;
 	struct clk *clk;
 	struct regulator *pwr_regulator;
diff --git a/drivers/crypto/ux500/cryp/cryp_core.c b/drivers/crypto/ux500/cryp/cryp_core.c
index 444deaf..6c4f872 100644
--- a/drivers/crypto/ux500/cryp/cryp_core.c
+++ b/drivers/crypto/ux500/cryp/cryp_core.c
@@ -476,6 +476,19 @@ static int cryp_get_device_data(struct cryp_ctx *ctx,
 static void cryp_dma_setup_channel(struct cryp_device_data *device_data,
 				   struct device *dev)
 {
+	struct dma_slave_config mem2cryp = {
+		.direction = DMA_MEM_TO_DEV,
+		.dst_addr = device_data->phybase + HASH_DMA_TX_FIFO,
+		.dst_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES,
+		.dst_maxburst = 4,
+        };
+	struct dma_slave_config cryp2mem = {
+		.direction = DMA_DEV_TO_MEM,
+		.src_addr = device_data->phybase + HASH_DMA_RX_FIFO,
+		.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES,
+		.src_maxburst = 4,
+        };
+
 	dma_cap_zero(device_data->dma.mask);
 	dma_cap_set(DMA_SLAVE, device_data->dma.mask);
 
@@ -491,6 +504,9 @@ static void cryp_dma_setup_channel(struct cryp_device_data *device_data,
 				    stedma40_filter,
 				    device_data->dma.cfg_cryp2mem);
 
+	dmaengine_slave_config(device_data->dma.chan_mem2cryp, &mem2cryp);
+	dmaengine_slave_config(device_data->dma.chan_cryp2mem, &cryp2mem);
+
 	init_completion(&device_data->dma.cryp_dma_complete);
 }
 
@@ -1432,6 +1448,7 @@ static int ux500_cryp_probe(struct platform_device *pdev)
 		goto out_kfree;
 	}
 
+	device_data->phybase = res->start;
 	device_data->base = ioremap(res->start, resource_size(res));
 	if (!device_data->base) {
 		dev_err(dev, "[%s]: ioremap failed!", __func__);
-- 
1.7.10.4

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Linus Walleij]

On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <lee.jones@linaro.org> wrote:

> The DMA controller currently takes configuration information from
> information passed though dma_channel_request(), but it shouldn't.
> Using the API, the DMA channel should only be configured during
> a dma_slave_config() call.
>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Andreas Westin <andreas.westin@stericsson.com>
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

(...)
>  /* Cryp DMA interface */
> +#define HASH_DMA_TX_FIFO       0x08
> +#define HASH_DMA_RX_FIFO       0x10

Yes, this is nice address notation :-)

>  /**
>   * struct cryp_device_data - structure for a cryp device.
> - * @base: Pointer to the hardware base address.
> + * @base: Pointer to virtual base address of the cryp device.
> + * @phybase: Pointer to physical memory location of the cryp device.
>   * @dev: Pointer to the devices dev structure.
>   * @clk: Pointer to the device's clock control.
>   * @pwr_regulator: Pointer to the device's power control.
> @@ -232,6 +236,7 @@ struct cryp_dma {
>   */
>  struct cryp_device_data {
>         struct cryp_register __iomem *base;
> +       phys_addr_t phybase;

Use dma_addr_t. Maybe "phybase" is misleading,
"dmabase" is probably better. (Also applies to the
cryp patch).

Apart from that it looks allright.

Yours,
Linus Walleij

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Lee Jones]

On Thu, 25 Apr 2013, Linus Walleij wrote:

> On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 
> > The DMA controller currently takes configuration information from
> > information passed though dma_channel_request(), but it shouldn't.
> > Using the API, the DMA channel should only be configured during
> > a dma_slave_config() call.
> >
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: David S. Miller <davem@davemloft.net>
> > Cc: Andreas Westin <andreas.westin@stericsson.com>
> > Cc: linux-crypto@vger.kernel.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> (...)
> >  /* Cryp DMA interface */
> > +#define HASH_DMA_TX_FIFO       0x08
> > +#define HASH_DMA_RX_FIFO       0x10
> 
> Yes, this is nice address notation :-)
> 
> >  /**
> >   * struct cryp_device_data - structure for a cryp device.
> > - * @base: Pointer to the hardware base address.
> > + * @base: Pointer to virtual base address of the cryp device.
> > + * @phybase: Pointer to physical memory location of the cryp device.
> >   * @dev: Pointer to the devices dev structure.
> >   * @clk: Pointer to the device's clock control.
> >   * @pwr_regulator: Pointer to the device's power control.
> > @@ -232,6 +236,7 @@ struct cryp_dma {
> >   */
> >  struct cryp_device_data {
> >         struct cryp_register __iomem *base;
> > +       phys_addr_t phybase;
> 
> Use dma_addr_t. Maybe "phybase" is misleading,
> "dmabase" is probably better. (Also applies to the
> cryp patch).

Accept it's not the dmabase.

It's the phybase (U8500_CRYP1_BASE) i.e. the physical base address of
the device's regs.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Linus Walleij]

On Thu, Apr 25, 2013 at 3:44 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 25 Apr 2013, Linus Walleij wrote:
>> On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <lee.jones@linaro.org> wrote:
>>
>> > The DMA controller currently takes configuration information from
>> > information passed though dma_channel_request(), but it shouldn't.
>> > Using the API, the DMA channel should only be configured during
>> > a dma_slave_config() call.
>> >
>> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> > Cc: David S. Miller <davem@davemloft.net>
>> > Cc: Andreas Westin <andreas.westin@stericsson.com>
>> > Cc: linux-crypto@vger.kernel.org
>> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>
>> (...)
>> >  /* Cryp DMA interface */
>> > +#define HASH_DMA_TX_FIFO       0x08
>> > +#define HASH_DMA_RX_FIFO       0x10
>>
>> Yes, this is nice address notation :-)
>>
>> >  /**
>> >   * struct cryp_device_data - structure for a cryp device.
>> > - * @base: Pointer to the hardware base address.
>> > + * @base: Pointer to virtual base address of the cryp device.
>> > + * @phybase: Pointer to physical memory location of the cryp device.
>> >   * @dev: Pointer to the devices dev structure.
>> >   * @clk: Pointer to the device's clock control.
>> >   * @pwr_regulator: Pointer to the device's power control.
>> > @@ -232,6 +236,7 @@ struct cryp_dma {
>> >   */
>> >  struct cryp_device_data {
>> >         struct cryp_register __iomem *base;
>> > +       phys_addr_t phybase;
>>
>> Use dma_addr_t. Maybe "phybase" is misleading,
>> "dmabase" is probably better. (Also applies to the
>> cryp patch).
>
> Accept it's not the dmabase.
>
> It's the phybase (U8500_CRYP1_BASE) i.e. the physical base address of
> the device's regs.

So when you assign the src_addr or dst_addr in struct
dmaengine_slave_config in this code,
you notice that this looks like so:

struct dma_slave_config {
        enum dma_transfer_direction direction;
        dma_addr_t src_addr;
        dma_addr_t dst_addr;
        enum dma_slave_buswidth src_addr_width;
        enum dma_slave_buswidth dst_addr_width;
        u32 src_maxburst;
        u32 dst_maxburst;
        bool device_fc;
};

So when you do this:

+       struct dma_slave_config mem2cryp = {
+               .direction = DMA_MEM_TO_DEV,
+               .dst_addr = device_data->phybase + HASH_DMA_TX_FIFO,
+               .dst_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES,
+               .dst_maxburst = 4,
+        };

on .dst_addr you are effectively casting a phys_addr_t
into a dma_addr_t.

But  we must do this at some point, and now I think
that doing it here may be just as good (because we might
just add a physical-to-dma memory translation if need
be).

So allright.

Yours,
Linus Walleij

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Arnd Bergmann]

On Thursday 25 April 2013, Lee Jones wrote:

> > > @@ -232,6 +236,7 @@ struct cryp_dma {
> > >   */
> > >  struct cryp_device_data {
> > >         struct cryp_register __iomem *base;
> > > +       phys_addr_t phybase;
> > 
> > Use dma_addr_t. Maybe "phybase" is misleading,
> > "dmabase" is probably better. (Also applies to the
> > cryp patch).
> 
> Accept it's not the dmabase.
> 
> It's the phybase (U8500_CRYP1_BASE) i.e. the physical base address of
> the device's regs.

Right, this recently came up in a different context and I agree:

The dma engine driver must know the address in its dma space, while the
slave driver has it available in physical space. These two are often the
same, but there is no generic way to convert between the two, especially
if the dma engine resides behind an IOMMU.

The best assumption we can make is that the dma engine driver knows
how to convert between the two. Interestingly the documentation for
dma_slave_config talks about "physical address", while the structure
itself uses a dma_addr_t. Linus Walleij introduced the structure in
c156d0a5b0 "DMAENGINE: generic slave channel control v3", so I assume
he can shed some light on what he was thinking. I assume the documentation
is right but the structure is not and should be converted to use
phys_add_t or resource_size_t.

	Arnd

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Linus Walleij]

On Thu, Apr 25, 2013 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> The dma engine driver must know the address in its dma space, while the
> slave driver has it available in physical space. These two are often the
> same, but there is no generic way to convert between the two, especially
> if the dma engine resides behind an IOMMU.
>
> The best assumption we can make is that the dma engine driver knows
> how to convert between the two. Interestingly the documentation for
> dma_slave_config talks about "physical address", while the structure
> itself uses a dma_addr_t. Linus Walleij introduced the structure in
> c156d0a5b0 "DMAENGINE: generic slave channel control v3", so I assume
> he can shed some light on what he was thinking. I assume the documentation
> is right but the structure is not and should be converted to use
> phys_add_t or resource_size_t.

OK I could cook a patch for that, but I think I need some input from
Vinod and/or Russell on this.

It does make sense to me that if anything knows how to map a physical
address into the DMA address space it'll be the DMA engine.

However this rings a bell that there may be a possible relation to
DMA-API, since that API syncs memory buffers to the DMA
address space if there is some MMU inbetween the DMA and the
(ordinary, non-device) memory.

So if we think one step ahead, assuming the DMAC is actually behind
an MMU making it see the device in some other address than the
physical (bus) space, where would the address be resolved?

Yours,
Linus Walleij

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Vinod Koul]

On Fri, Apr 26, 2013 at 10:28:39AM +0200, Linus Walleij wrote:
> On Thu, Apr 25, 2013 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > The dma engine driver must know the address in its dma space, while the
> > slave driver has it available in physical space. These two are often the
> > same, but there is no generic way to convert between the two, especially
> > if the dma engine resides behind an IOMMU.
> >
> > The best assumption we can make is that the dma engine driver knows
> > how to convert between the two. Interestingly the documentation for
> > dma_slave_config talks about "physical address", while the structure
> > itself uses a dma_addr_t. Linus Walleij introduced the structure in
> > c156d0a5b0 "DMAENGINE: generic slave channel control v3", so I assume
> > he can shed some light on what he was thinking. I assume the documentation
> > is right but the structure is not and should be converted to use
> > phys_add_t or resource_size_t.
> 
> OK I could cook a patch for that, but I think I need some input from
> Vinod and/or Russell on this.
the dma_slave_config is physical address that should be passed directly to the
controller. Obviosuly it should phys_addr_t :)

The mapping & unmapping of dma buffer (memcpy and memory buffer in this txn) is
required to be performed by the client driver. The dmanegine or dmaengine driver
will not do that for you...

This is true for slave usage and not for async case.
> 
> It does make sense to me that if anything knows how to map a physical
> address into the DMA address space it'll be the DMA engine.
> 
> However this rings a bell that there may be a possible relation to
> DMA-API, since that API syncs memory buffers to the DMA
> address space if there is some MMU inbetween the DMA and the
> (ordinary, non-device) memory.
> 
> So if we think one step ahead, assuming the DMAC is actually behind
> an MMU making it see the device in some other address than the
> physical (bus) space, where would the address be resolved?
> 
> Yours,
> Linus Walleij

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Linus Walleij]

On Fri, Apr 26, 2013 at 10:16 AM, Vinod Koul <vinod.koul@intel.com> wrote:

>> OK I could cook a patch for that, but I think I need some input from
>> Vinod and/or Russell on this.

> the dma_slave_config is physical address that should be passed directly to the
> controller. Obviosuly it should phys_addr_t :)

OK! Sent a patch for this, check it out.

Yours,
Linus Walleij

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Arnd Bergmann]

On Friday 26 April 2013 13:46:46 Vinod Koul wrote:
> 
> The mapping & unmapping of dma buffer (memcpy and memory buffer in this txn) is
> required to be performed by the client driver. The dmanegine or dmaengine driver
> will not do that for you...

I've been wondering about this part: since we need to pass the 'struct device' of
the dma engine (rather than the slave) into dma_map_single, what is the official
way to do that? Should the slave driver just rely on dma_chan->device->dev to
work correctly for the purpose of dma-mapping.h interfaces?

	Arnd

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Russell King - ARM Linux]

On Fri, Apr 26, 2013 at 11:39:20AM +0200, Arnd Bergmann wrote:
> On Friday 26 April 2013 13:46:46 Vinod Koul wrote:
> > 
> > The mapping & unmapping of dma buffer (memcpy and memory buffer in this txn) is
> > required to be performed by the client driver. The dmanegine or dmaengine driver
> > will not do that for you...
> 
> I've been wondering about this part: since we need to pass the 'struct device' of
> the dma engine (rather than the slave) into dma_map_single, what is the official
> way to do that? Should the slave driver just rely on dma_chan->device->dev to
> work correctly for the purpose of dma-mapping.h interfaces?

Yes.

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Russell King - ARM Linux]

On Fri, Apr 26, 2013 at 01:46:46PM +0530, Vinod Koul wrote:
> On Fri, Apr 26, 2013 at 10:28:39AM +0200, Linus Walleij wrote:
> > On Thu, Apr 25, 2013 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > 
> > > The dma engine driver must know the address in its dma space, while the
> > > slave driver has it available in physical space. These two are often the
> > > same, but there is no generic way to convert between the two, especially
> > > if the dma engine resides behind an IOMMU.
> > >
> > > The best assumption we can make is that the dma engine driver knows
> > > how to convert between the two. Interestingly the documentation for
> > > dma_slave_config talks about "physical address", while the structure
> > > itself uses a dma_addr_t. Linus Walleij introduced the structure in
> > > c156d0a5b0 "DMAENGINE: generic slave channel control v3", so I assume
> > > he can shed some light on what he was thinking. I assume the documentation
> > > is right but the structure is not and should be converted to use
> > > phys_add_t or resource_size_t.
> > 
> > OK I could cook a patch for that, but I think I need some input from
> > Vinod and/or Russell on this.
> the dma_slave_config is physical address that should be passed directly to the
> controller. Obviosuly it should phys_addr_t :)

What you've just said is actually confusing.

"physical address" is normally the term used to describe the addresses
seen to the RAM.  phys_addr_t describes this.  This is not necessarily
what needs to be programmed into the DMA controller.

For RAM addresses, they must be mapped via the DMA API - and this gives
you a dma_addr_t.

"DMA address" is the address to be programmed into a DMA controller to
access a particular address in RAM or device, and has type dma_addr_t.
When you're programming a DMA controller to access a device, you are
clearly telling it the address on the _DMA controller's bus_ to access
that register, which may or may not be the same as the physical address.

There are platforms in existence where phys_addr_t can be 32-bit but
dma_addr_t can be 64-bit.  Getting this stuff wrong can cause problems.

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Vinod Koul]

On Fri, Apr 26, 2013 at 10:41:23AM +0100, Russell King - ARM Linux wrote:
> On Fri, Apr 26, 2013 at 01:46:46PM +0530, Vinod Koul wrote:
> > On Fri, Apr 26, 2013 at 10:28:39AM +0200, Linus Walleij wrote:
> > > On Thu, Apr 25, 2013 at 4:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > > 
> > > > The dma engine driver must know the address in its dma space, while the
> > > > slave driver has it available in physical space. These two are often the
> > > > same, but there is no generic way to convert between the two, especially
> > > > if the dma engine resides behind an IOMMU.
> > > >
> > > > The best assumption we can make is that the dma engine driver knows
> > > > how to convert between the two. Interestingly the documentation for
> > > > dma_slave_config talks about "physical address", while the structure
> > > > itself uses a dma_addr_t. Linus Walleij introduced the structure in
> > > > c156d0a5b0 "DMAENGINE: generic slave channel control v3", so I assume
> > > > he can shed some light on what he was thinking. I assume the documentation
> > > > is right but the structure is not and should be converted to use
> > > > phys_add_t or resource_size_t.
> > > 
> > > OK I could cook a patch for that, but I think I need some input from
> > > Vinod and/or Russell on this.
> > the dma_slave_config is physical address that should be passed directly to the
> > controller. Obviosuly it should phys_addr_t :)
> 
> What you've just said is actually confusing.
> 
> "physical address" is normally the term used to describe the addresses
> seen to the RAM.  phys_addr_t describes this.  This is not necessarily
> what needs to be programmed into the DMA controller.
Yes that would be true when you have MMU
> 
> For RAM addresses, they must be mapped via the DMA API - and this gives
> you a dma_addr_t.
> 
> "DMA address" is the address to be programmed into a DMA controller to
> access a particular address in RAM or device, and has type dma_addr_t.
> When you're programming a DMA controller to access a device, you are
> clearly telling it the address on the _DMA controller's bus_ to access
> that register, which may or may not be the same as the physical address.
> 
> There are platforms in existence where phys_addr_t can be 32-bit but
> dma_addr_t can be 64-bit.  Getting this stuff wrong can cause problems.
Sure, thanks for pointing, so we wont do this change.

--
~Vinod

Re: [PATCH 6/9] crypto: ux500/cryp - Set DMA configuration though dma_slave_config()

[Arnd Bergmann]

On Friday 26 April 2013 10:28:39 Linus Walleij wrote:
> 
> However this rings a bell that there may be a possible relation to
> DMA-API, since that API syncs memory buffers to the DMA
> address space if there is some MMU inbetween the DMA and the
> (ordinary, non-device) memory.
> 
> So if we think one step ahead, assuming the DMAC is actually behind
> an MMU making it see the device in some other address than the
> physical (bus) space, where would the address be resolved?

We don't currently have the infrastructure for that I think.
The dma-mapping API has some of the required parts but not all,
in particular it's only designed for mapping pages from the linear
kernel memory into the bus address space, not for devices.

The iommu API could do it for devices that have an IOMMU, but
it's not the best fit, because it does not abstract away the
presence of an IOMMU.

Another missing part is parsing the "dma-ranges" properties in
device tree, which you need to do if the address space translation
is not 1:1, and to find out which side of the IOMMU the DMA master
is connected to: if it's on the bus side, you need 1:1 mapping
and if it's on the host side, you need an IO page table entry.

	Arnd

[PATCH 8/9] crypto: ux500/[cryp|hash] - Show successful start-up in the bootlog

[Lee Jones]

The Cryp driver is currently silent and the Hash driver prints the
name of its probe function unnecessarily. Let's just put a nice
descriptive one-liner there instead.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Cc: Andreas Westin <andreas.westin@stericsson.com>
Cc: linux-crypto@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/crypto/ux500/cryp/cryp_core.c |    2 ++
 drivers/crypto/ux500/hash/hash_core.c |    2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ux500/cryp/cryp_core.c b/drivers/crypto/ux500/cryp/cryp_core.c
index 6c4f872..7366910 100644
--- a/drivers/crypto/ux500/cryp/cryp_core.c
+++ b/drivers/crypto/ux500/cryp/cryp_core.c
@@ -1536,6 +1536,8 @@ static int ux500_cryp_probe(struct platform_device *pdev)
 		goto out_power;
 	}
 
+	dev_info(dev, "successfully registered\n");
+
 	return 0;
 
 out_power:
diff --git a/drivers/crypto/ux500/hash/hash_core.c b/drivers/crypto/ux500/hash/hash_core.c
index f9126f4..d3cbf5d 100644
--- a/drivers/crypto/ux500/hash/hash_core.c
+++ b/drivers/crypto/ux500/hash/hash_core.c
@@ -1767,7 +1767,7 @@ static int ux500_hash_probe(struct platform_device *pdev)
 		goto out_power;
 	}
 
-	dev_info(dev, "[%s] successfully probed\n", __func__);
+	dev_info(dev, "successfully registered\n");
 	return 0;
 
 out_power:
-- 
1.7.10.4

Re: [PATCH 8/9] crypto: ux500/[cryp|hash] - Show successful start-up in the bootlog

[Linus Walleij]

On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <lee.jones@linaro.org> wrote:

> The Cryp driver is currently silent and the Hash driver prints the
> name of its probe function unnecessarily. Let's just put a nice
> descriptive one-liner there instead.
>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Andreas Westin <andreas.westin@stericsson.com>
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij