[PATCH 1/2] crypto: bfin_crc: ignore duplicated registration of the same algorithm

[PATCH 1/2] crypto: bfin_crc: ignore duplicated registration of the same algorithm

[Sonic Zhang]

From: Sonic Zhang <sonic.zhang@analog.com>

in case of multiple crc devices are probed.

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
---
 drivers/crypto/bfin_crc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/bfin_crc.c b/drivers/crypto/bfin_crc.c
index cea3e8c..5f5f427 100644
--- a/drivers/crypto/bfin_crc.c
+++ b/drivers/crypto/bfin_crc.c
@@ -672,7 +672,7 @@ static int bfin_crypto_crc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, crc);
 
 	ret = crypto_register_ahash(&algs);
-	if (ret) {
+	if (ret && ret != -EEXIST) {
 		spin_lock(&crc_list.lock);
 		list_del(&crc->list);
 		spin_unlock(&crc_list.lock);
-- 
1.8.2.3

[PATCH 2/2] crypto: bfin_crc: avoid get physical address of coherence memory by dma_map_single

[Sonic Zhang]

From: Sonic Zhang <sonic.zhang@analog.com>

- The 4-byte sg_mid_buf is located in the middle of the coherence memory
sg_cpu. Don't call dma_map_single to get its physical address. Get the its
base physical address from the physical address of sg_cpu instead.
- Should set up the dma descriptor data after the 4-byte sg_mid_buf is
filled in completely from next sg buffer.
- memory copy from sg buffer should be done via virtual address.
- Remove unused reference to blackfin header

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
---
 drivers/crypto/bfin_crc.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/crypto/bfin_crc.c b/drivers/crypto/bfin_crc.c
index 5f5f427..94a0792 100644
--- a/drivers/crypto/bfin_crc.c
+++ b/drivers/crypto/bfin_crc.c
@@ -29,7 +29,6 @@
 #include <crypto/hash.h>
 #include <crypto/internal/hash.h>
 
-#include <asm/blackfin.h>
 #include <asm/dma.h>
 #include <asm/portmux.h>
 #include <asm/io.h>
@@ -62,6 +61,7 @@ struct bfin_crypto_crc {
 	struct dma_desc_array	*sg_cpu; /* virt addr of sg dma descriptors */
 	dma_addr_t		sg_dma; /* phy addr of sg dma descriptors */
 	u8			*sg_mid_buf;
+	dma_addr_t		sg_mid_dma; /* phy addr of sg mid buffer */
 
 	struct tasklet_struct	done_task;
 	struct crypto_queue	queue; /* waiting requests */
@@ -196,7 +196,6 @@ static void bfin_crypto_crc_config_dma(struct bfin_crypto_crc *crc)
 	dma_map_sg(crc->dev, ctx->sg, ctx->sg_nents, DMA_TO_DEVICE);
 
 	for_each_sg(ctx->sg, sg, ctx->sg_nents, j) {
-		dma_config = DMAFLOW_ARRAY | RESTART | NDSIZE_3 | DMAEN | PSIZE_32;
 		dma_addr = sg_dma_address(sg);
 		/* deduce extra bytes in last sg */
 		if (sg_is_last(sg))
@@ -209,12 +208,29 @@ static void bfin_crypto_crc_config_dma(struct bfin_crypto_crc *crc)
 			   bytes in current sg buffer. Move addr of current
 			   sg and deduce the length of current sg.
 			 */
-			memcpy(crc->sg_mid_buf +((i-1) << 2) + mid_dma_count,
-				(void *)dma_addr,
+			memcpy(crc->sg_mid_buf +(i << 2) + mid_dma_count,
+				sg_virt(sg),
 				CHKSUM_DIGEST_SIZE - mid_dma_count);
 			dma_addr += CHKSUM_DIGEST_SIZE - mid_dma_count;
 			dma_count -= CHKSUM_DIGEST_SIZE - mid_dma_count;
+
+			dma_config = DMAFLOW_ARRAY | RESTART | NDSIZE_3 |
+				DMAEN | PSIZE_32 | WDSIZE_32;
+
+			/* setup new dma descriptor for next middle dma */
+			crc->sg_cpu[i].start_addr = crc->sg_mid_dma + (i << 2);
+			crc->sg_cpu[i].cfg = dma_config;
+			crc->sg_cpu[i].x_count = 1;
+			crc->sg_cpu[i].x_modify = CHKSUM_DIGEST_SIZE;
+			dev_dbg(crc->dev, "%d: crc_dma: start_addr:0x%lx, "
+				"cfg:0x%lx, x_count:0x%lx, x_modify:0x%lx\n",
+				i, crc->sg_cpu[i].start_addr,
+				crc->sg_cpu[i].cfg, crc->sg_cpu[i].x_count,
+				crc->sg_cpu[i].x_modify);
+			i++;
 		}
+
+		dma_config = DMAFLOW_ARRAY | RESTART | NDSIZE_3 | DMAEN | PSIZE_32;
 		/* chop current sg dma len to multiple of 32 bits */
 		mid_dma_count = dma_count % 4;
 		dma_count &= ~0x3;
@@ -245,24 +261,9 @@ static void bfin_crypto_crc_config_dma(struct bfin_crypto_crc *crc)
 
 		if (mid_dma_count) {
 			/* copy extra bytes to next middle dma buffer */
-			dma_config = DMAFLOW_ARRAY | RESTART | NDSIZE_3 |
-				DMAEN | PSIZE_32 | WDSIZE_32;
 			memcpy(crc->sg_mid_buf + (i << 2),
-				(void *)(dma_addr + (dma_count << 2)),
+				(u8*)sg_virt(sg) + (dma_count << 2),
 				mid_dma_count);
-			/* setup new dma descriptor for next middle dma */
-			crc->sg_cpu[i].start_addr = dma_map_single(crc->dev,
-					crc->sg_mid_buf + (i << 2),
-					CHKSUM_DIGEST_SIZE, DMA_TO_DEVICE);
-			crc->sg_cpu[i].cfg = dma_config;
-			crc->sg_cpu[i].x_count = 1;
-			crc->sg_cpu[i].x_modify = CHKSUM_DIGEST_SIZE;
-			dev_dbg(crc->dev, "%d: crc_dma: start_addr:0x%lx, "
-				"cfg:0x%lx, x_count:0x%lx, x_modify:0x%lx\n",
-				i, crc->sg_cpu[i].start_addr,
-				crc->sg_cpu[i].cfg, crc->sg_cpu[i].x_count,
-				crc->sg_cpu[i].x_modify);
-			i++;
 		}
 	}
 
@@ -654,6 +655,8 @@ static int bfin_crypto_crc_probe(struct platform_device *pdev)
 	 * 1 last + 1 next dma descriptors
 	 */
 	crc->sg_mid_buf = (u8 *)(crc->sg_cpu + ((CRC_MAX_DMA_DESC + 1) << 1));
+	crc->sg_mid_dma = crc->sg_dma + sizeof(struct dma_desc_array)
+			* ((CRC_MAX_DMA_DESC + 1) << 1);
 
 	writel(0, &crc->regs->control);
 	crc->poly = (u32)pdev->dev.platform_data;
-- 
1.8.2.3

Re: [PATCH 2/2] crypto: bfin_crc: avoid get physical address of coherence memory by dma_map_single

[Marek Vasut]

On Thursday, April 10, 2014 at 10:40:59 AM, Sonic Zhang wrote:
> From: Sonic Zhang <sonic.zhang@analog.com>
> 
> - The 4-byte sg_mid_buf is located in the middle of the coherence memory
> sg_cpu. Don't call dma_map_single to get its physical address. Get the its
> base physical address from the physical address of sg_cpu instead.
> - Should set up the dma descriptor data after the 4-byte sg_mid_buf is
> filled in completely from next sg buffer.
> - memory copy from sg buffer should be done via virtual address.
> - Remove unused reference to blackfin header
> 
> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>

I don't see any obvious problems,

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 2/2] crypto: bfin_crc: avoid get physical address of coherence memory by dma_map_single

[Sonic Zhang]

Hi Herbert,

Could you please review this patch?

Thanks

Sonic Zhang


On Thu, Apr 10, 2014 at 5:02 PM, Marek Vasut <marex@denx.de> wrote:
> On Thursday, April 10, 2014 at 10:40:59 AM, Sonic Zhang wrote:
>> From: Sonic Zhang <sonic.zhang@analog.com>
>>
>> - The 4-byte sg_mid_buf is located in the middle of the coherence memory
>> sg_cpu. Don't call dma_map_single to get its physical address. Get the its
>> base physical address from the physical address of sg_cpu instead.
>> - Should set up the dma descriptor data after the 4-byte sg_mid_buf is
>> filled in completely from next sg buffer.
>> - memory copy from sg buffer should be done via virtual address.
>> - Remove unused reference to blackfin header
>>
>> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
>
> I don't see any obvious problems,
>
> Reviewed-by: Marek Vasut <marex@denx.de>
>
> Best regards,
> Marek Vasut

Re: [PATCH 1/2] crypto: bfin_crc: ignore duplicated registration of the same algorithm

[Marek Vasut]

On Thursday, April 10, 2014 at 10:40:58 AM, Sonic Zhang wrote:
> From: Sonic Zhang <sonic.zhang@analog.com>
> 
> in case of multiple crc devices are probed.
> 
> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
> ---
>  drivers/crypto/bfin_crc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/bfin_crc.c b/drivers/crypto/bfin_crc.c
> index cea3e8c..5f5f427 100644
> --- a/drivers/crypto/bfin_crc.c
> +++ b/drivers/crypto/bfin_crc.c
> @@ -672,7 +672,7 @@ static int bfin_crypto_crc_probe(struct platform_device
> *pdev) platform_set_drvdata(pdev, crc);

Rant: Reading through the code a little, the platform_set_drvdata() should 
happen before you add the new CRC device into the list (above this code).

>  	ret = crypto_register_ahash(&algs);
> -	if (ret) {
> +	if (ret && ret != -EEXIST) {

Uh oh, how is this supposed to work ? Does blackfin have multiple crc32 
processing units ? Can you simply not check if the crc_list is not empty and 
avoid calling crypto_register_ahash() for subsequent crc32 units at all ?

>  		spin_lock(&crc_list.lock);
>  		list_del(&crc->list);
>  		spin_unlock(&crc_list.lock);

Best regards,
Marek Vasut

Re: [PATCH 1/2] crypto: bfin_crc: ignore duplicated registration of the same algorithm

[Sonic Zhang]

Hi Marek,

On Thu, Apr 10, 2014 at 4:59 PM, Marek Vasut <marex@denx.de> wrote:
> On Thursday, April 10, 2014 at 10:40:58 AM, Sonic Zhang wrote:
>> From: Sonic Zhang <sonic.zhang@analog.com>
>>
>> in case of multiple crc devices are probed.
>>
>> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
>> ---
>>  drivers/crypto/bfin_crc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/crypto/bfin_crc.c b/drivers/crypto/bfin_crc.c
>> index cea3e8c..5f5f427 100644
>> --- a/drivers/crypto/bfin_crc.c
>> +++ b/drivers/crypto/bfin_crc.c
>> @@ -672,7 +672,7 @@ static int bfin_crypto_crc_probe(struct platform_device
>> *pdev) platform_set_drvdata(pdev, crc);
>
> Rant: Reading through the code a little, the platform_set_drvdata() should
> happen before you add the new CRC device into the list (above this code).

OK

>
>>       ret = crypto_register_ahash(&algs);
>> -     if (ret) {
>> +     if (ret && ret != -EEXIST) {
>
> Uh oh, how is this supposed to work ? Does blackfin have multiple crc32
> processing units ? Can you simply not check if the crc_list is not empty and
> avoid calling crypto_register_ahash() for subsequent crc32 units at all ?

Yes, Blackfin has 2 crc32 units CRC0 and CRC1.
I will have a try to see if it works.

Thanks

Sonic

Re: [PATCH 1/2] crypto: bfin_crc: ignore duplicated registration of the same algorithm

[Marek Vasut]

On Thursday, April 10, 2014 at 11:51:29 AM, Sonic Zhang wrote:
> Hi Marek,
> 
> On Thu, Apr 10, 2014 at 4:59 PM, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, April 10, 2014 at 10:40:58 AM, Sonic Zhang wrote:
> >> From: Sonic Zhang <sonic.zhang@analog.com>
> >> 
> >> in case of multiple crc devices are probed.
> >> 
> >> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
> >> ---
> >> 
> >>  drivers/crypto/bfin_crc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/crypto/bfin_crc.c b/drivers/crypto/bfin_crc.c
> >> index cea3e8c..5f5f427 100644
> >> --- a/drivers/crypto/bfin_crc.c
> >> +++ b/drivers/crypto/bfin_crc.c
> >> @@ -672,7 +672,7 @@ static int bfin_crypto_crc_probe(struct
> >> platform_device *pdev) platform_set_drvdata(pdev, crc);
> > 
> > Rant: Reading through the code a little, the platform_set_drvdata()
> > should happen before you add the new CRC device into the list (above
> > this code).
> 
> OK
> 
> >>       ret = crypto_register_ahash(&algs);
> >> 
> >> -     if (ret) {
> >> +     if (ret && ret != -EEXIST) {
> > 
> > Uh oh, how is this supposed to work ? Does blackfin have multiple crc32
> > processing units ? Can you simply not check if the crc_list is not empty
> > and avoid calling crypto_register_ahash() for subsequent crc32 units at
> > all ?
> 
> Yes, Blackfin has 2 crc32 units CRC0 and CRC1.
> I will have a try to see if it works.

Thank you !

Best regards,
Marek Vasut