LinuxLists.cc - [PATCH 0/9] Fix ux500 crypto drivers and init DMA in the correct way

[permalink] [raw]

Subject: [PATCH 1/9] 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 <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Andreas Westin <[email protected]>
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
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

2013-04-18 10:27:47

[permalink] [raw]

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

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 <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Andreas Westin <[email protected]>
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
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

2013-04-18 10:27:39

[permalink] [raw]

Subject: [PATCH 4/9] 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 <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Andreas Westin <[email protected]>
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
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

2013-04-18 10:28:09

[permalink] [raw]

Subject: [PATCH 9/9] ARM: ux500: Register Cyrp and Hash platform drivers on Snowball

These drivers are now operational and even use the latest common clk
and DMA APIs. There's no reason why we shouldn't start them up now.

Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/mach-ux500/board-mop500.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 7f756cc..3866fa8 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -656,6 +656,8 @@ static void __init snowball_init_machine(void)

mop500_snowball_ethernet_clock_enable();

+ u8500_cryp1_hash1_init(parent);
+
/* This board has full regulator constraints */
regulator_has_full_constraints();
}
--
1.7.10.4

2013-04-18 10:27:44

[permalink] [raw]

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

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 <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Andreas Westin <[email protected]>
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
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

2013-04-18 10:29:11

[permalink] [raw]

Subject: [PATCH 7/9] ARM: ux500: Stop passing Cryp DMA channel config information though pdata

DMA channel configuration information should be setup in the driver.
The Ux500 Cryp driver now does this, so there's no need to send it
though here too.

Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/mach-ux500/board-mop500.c | 8 --------
1 file changed, 8 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index cb7ae23..7f756cc 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -453,20 +453,12 @@ static struct cryp_platform_data u8500_cryp1_platform_data = {
.mem_to_engine = {
.dir = STEDMA40_MEM_TO_PERIPH,
.dev_type = DB8500_DMA_DEV48_CAC1,
- .src_info.data_width = STEDMA40_WORD_WIDTH,
- .dst_info.data_width = STEDMA40_WORD_WIDTH,
.mode = STEDMA40_MODE_LOGICAL,
- .src_info.psize = STEDMA40_PSIZE_LOG_4,
- .dst_info.psize = STEDMA40_PSIZE_LOG_4,
},
.engine_to_mem = {
.dir = STEDMA40_PERIPH_TO_MEM,
.dev_type = DB8500_DMA_DEV48_CAC1,
- .src_info.data_width = STEDMA40_WORD_WIDTH,
- .dst_info.data_width = STEDMA40_WORD_WIDTH,
.mode = STEDMA40_MODE_LOGICAL,
- .src_info.psize = STEDMA40_PSIZE_LOG_4,
- .dst_info.psize = STEDMA40_PSIZE_LOG_4,
}
};

--
1.7.10.4

2013-04-18 10:29:28

[permalink] [raw]

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

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 <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Andreas Westin <[email protected]>
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
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

2013-04-18 10:27:36

[permalink] [raw]

Subject: [PATCH 3/9] ARM: ux500: Stop passing Hash DMA channel config information though pdata

DMA channel configuration information should be setup in the driver.
The Ux500 Hash driver now does this, so there's no need to send it
though here too.

Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/mach-ux500/board-mop500.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/arch/arm/mach-ux500/board-mop500.c b/arch/arm/mach-ux500/board-mop500.c
index 883dc66..cb7ae23 100644
--- a/arch/arm/mach-ux500/board-mop500.c
+++ b/arch/arm/mach-ux500/board-mop500.c
@@ -473,11 +473,7 @@ static struct cryp_platform_data u8500_cryp1_platform_data = {
static struct stedma40_chan_cfg u8500_hash_dma_cfg_tx = {
.dir = STEDMA40_MEM_TO_PERIPH,
.dev_type = DB8500_DMA_DEV50_HAC1_TX,
- .src_info.data_width = STEDMA40_WORD_WIDTH,
- .dst_info.data_width = STEDMA40_WORD_WIDTH,
.mode = STEDMA40_MODE_LOGICAL,
- .src_info.psize = STEDMA40_PSIZE_LOG_16,
- .dst_info.psize = STEDMA40_PSIZE_LOG_16,
};

static struct hash_platform_data u8500_hash1_platform_data = {
--
1.7.10.4

2013-04-18 10:30:17

[permalink] [raw]

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

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 <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Andreas Westin <[email protected]>
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
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

2013-04-18 10:44:39

[permalink] [raw]

Subject: Re: [PATCH 0/9] Fix ux500 crypto drivers and init DMA in the correct way

On Thursday 18 April 2013, Lee Jones wrote:
> These drivers haven't been touched in a while. They didn't even compile
> or probe successfully. After these changes, both drivers run just fine.
>
> We also split DMA channel allocation and configuration into separate
> invocations, as the API expects.

Very nice series!

Acked-by: Arnd Bergmann <[email protected]>

2013-04-19 12:22:33

[permalink] [raw]

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

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 <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Andreas Westin <[email protected]>
Cc: [email protected]
Acked-by: Arnd Bergmann <[email protected]>
Signed-off-by: Lee Jones <[email protected]>

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);

2013-04-19 12:24:07

[permalink] [raw]

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

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 <[email protected]>

2013-04-19 12:24:36

[permalink] [raw]

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

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 <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Andreas Westin <[email protected]>
Cc: [email protected]
Acked-by: Arnd Bergmann <[email protected]>
Signed-off-by: Lee Jones <[email protected]>

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);

2013-04-19 12:26:53

[permalink] [raw]

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

2013-04-25 11:49:10

[permalink] [raw]

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

On Fri, Apr 19, 2013 at 2:24 PM, Lee Jones <[email protected]> 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 <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Andreas Westin <[email protected]>
> Cc: [email protected]
> Acked-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>

Pls include Ulf Hansson <[email protected]> on this patch.

Yours,
Linus Walleij

2013-04-25 11:55:41

[permalink] [raw]

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

Pls include [email protected] on all these crypto/hash
postings.

On Thu, Apr 18, 2013 at 12:26 PM, Lee Jones <[email protected]> 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 <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Andreas Westin <[email protected]>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>

(...)
> #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

2013-04-25 11:56:20

[permalink] [raw]

Subject: Re: [PATCH 3/9] ARM: ux500: Stop passing Hash DMA channel config information though pdata

On Thu, Apr 18, 2013 at 12:26 PM, Lee Jones <[email protected]> wrote:

> DMA channel configuration information should be setup in the driver.
> The Ux500 Hash driver now does this, so there's no need to send it
> though here too.
>
> Signed-off-by: Lee Jones <[email protected]>

When 2/9 is fixed up:
Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2013-04-25 11:57:54

[permalink] [raw]

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

On Fri, Apr 19, 2013 at 2:22 PM, Lee Jones <[email protected]> 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 <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Andreas Westin <[email protected]>
> Cc: [email protected]
> Acked-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>

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

(Apart from this it looks OK.)

Yours,
Linus Walleij

2013-04-25 12:00:06

[permalink] [raw]

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

On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> 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 <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Andreas Westin <[email protected]>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>

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

Yours,
Linus Walleij

2013-04-25 12:02:13

[permalink] [raw]

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

On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> 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 <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Andreas Westin <[email protected]>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>

(...)
> /* 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

2013-04-25 12:02:59

[permalink] [raw]

Subject: Re: [PATCH 7/9] ARM: ux500: Stop passing Cryp DMA channel config information though pdata

On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> wrote:

> DMA channel configuration information should be setup in the driver.
> The Ux500 Cryp driver now does this, so there's no need to send it
> though here too.
>
> Signed-off-by: Lee Jones <[email protected]>

Provided the deps are in place:
Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2013-04-25 12:03:59

[permalink] [raw]

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

On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> 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 <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Andreas Westin <[email protected]>
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>

Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2013-04-25 12:04:39

[permalink] [raw]

Subject: Re: [PATCH 9/9] ARM: ux500: Register Cyrp and Hash platform drivers on Snowball

On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> wrote:

> These drivers are now operational and even use the latest common clk
> and DMA APIs. There's no reason why we shouldn't start them up now.
>
> Signed-off-by: Lee Jones <[email protected]>

Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2013-04-25 13:44:11

[permalink] [raw]

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

On Thu, 25 Apr 2013, Linus Walleij wrote:

> On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> 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 <[email protected]>
> > Cc: David S. Miller <[email protected]>
> > Cc: Andreas Westin <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Lee Jones <[email protected]>
>
> (...)
> > /* 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

2013-04-25 13:44:50

[permalink] [raw]

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

On Thu, 25 Apr 2013, Linus Walleij wrote:

> On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> 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 <[email protected]>
> > Cc: David S. Miller <[email protected]>
> > Cc: Andreas Westin <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Lee Jones <[email protected]>
>
> 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

2013-04-25 13:46:59

[permalink] [raw]

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

On Thu, 25 Apr 2013, Linus Walleij wrote:

> On Fri, Apr 19, 2013 at 2:24 PM, Lee Jones <[email protected]> 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 <[email protected]>
> > Cc: David S. Miller <[email protected]>
> > Cc: Andreas Westin <[email protected]>
> > Cc: [email protected]
> > Acked-by: Arnd Bergmann <[email protected]>
> > Signed-off-by: Lee Jones <[email protected]>
>
> Pls include Ulf Hansson <[email protected]> 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

2013-04-25 14:05:59

[permalink] [raw]

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

On Thu, Apr 25, 2013 at 3:44 PM, Lee Jones <[email protected]> wrote:
> On Thu, 25 Apr 2013, Linus Walleij wrote:
>> On Thu, Apr 18, 2013 at 12:27 PM, Lee Jones <[email protected]> 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 <[email protected]>
>> > Cc: David S. Miller <[email protected]>
>> > Cc: Andreas Westin <[email protected]>
>> > Cc: [email protected]
>> > Signed-off-by: Lee Jones <[email protected]>
>>
>> (...)
>> > /* 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

2013-04-25 14:11:58

[permalink] [raw]

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

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

2013-04-26 08:28:43

[permalink] [raw]

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

On Thu, Apr 25, 2013 at 4:11 PM, Arnd Bergmann <[email protected]> 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

2013-04-26 08:49:14

by Vinod Koul

[permalink] [raw]

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

On Fri, Apr 26, 2013 at 10:28:39AM +0200, Linus Walleij wrote:
> On Thu, Apr 25, 2013 at 4:11 PM, Arnd Bergmann <[email protected]> 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

2013-04-26 09:07:35

[permalink] [raw]

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

On Fri, Apr 26, 2013 at 10:16 AM, Vinod Koul <[email protected]> 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

2013-04-26 09:35:11

[permalink] [raw]

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

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

2013-04-26 09:39:19