Re: [PATCH 1/2] Add crypto driver support for some MediaTek chips

2016-12-02 Thread Ryder Lee 
Hello,

On Fri, 2016-12-02 at 09:18 +0100, Corentin Labbe wrote:
> Hello
> 
> I have some minor comment inline
> 
> On Fri, Dec 02, 2016 at 11:26:44AM +0800, Ryder Lee wrote:
> > This adds support for the MediaTek hardware accelerator on
> > mt7623/mt2701/mt8521p SoC.
> > 
> > This driver currently implement:
> > - SHA1 and SHA2 family(HMAC) hash alogrithms.
> > - AES block cipher in CBC/ECB mode with 128/196/256 bits keys.
> 
> I see also a PRNG but is seems not really used.

Yes, PRNG is not implemented yet, i will remove it temporarily.

> > 
> > Signed-off-by: Ryder Lee 
> > ---
> >  drivers/crypto/Kconfig |   17 +
> >  drivers/crypto/Makefile|1 +
> >  drivers/crypto/mediatek/Makefile   |2 +
> >  drivers/crypto/mediatek/mtk-aes.c  |  734 +
> >  drivers/crypto/mediatek/mtk-platform.c |  575 +
> >  drivers/crypto/mediatek/mtk-platform.h |  230 ++
> >  drivers/crypto/mediatek/mtk-regs.h |  194 +
> >  drivers/crypto/mediatek/mtk-sha.c  | 1384

> >  8 files changed, 3137 insertions(+)
> >  create mode 100644 drivers/crypto/mediatek/Makefile
> >  create mode 100644 drivers/crypto/mediatek/mtk-aes.c
> >  create mode 100644 drivers/crypto/mediatek/mtk-platform.c
> >  create mode 100644 drivers/crypto/mediatek/mtk-platform.h
> >  create mode 100644 drivers/crypto/mediatek/mtk-regs.h
> >  create mode 100644 drivers/crypto/mediatek/mtk-sha.c
> > 
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > index 4d2b81f..5d9c803 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -553,6 +553,23 @@ config CRYPTO_DEV_ROCKCHIP
> >   This driver interfaces with the hardware crypto accelerator.
> >   Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher
mode.
> >  
> > +config CRYPTO_DEV_MEDIATEK
> > +   tristate "MediaTek's Cryptographic Engine driver"
> > +   depends on ARM && ARCH_MEDIATEK
> > +   select NEON
> > +   select KERNEL_MODE_NEON
> > +   select ARM_CRYPTO
> > +   select CRYPTO_AES
> > +   select CRYPTO_BLKCIPHER
> > +   select CRYPTO_SHA1_ARM_NEON
> > +   select CRYPTO_SHA256_ARM
> > +   select CRYPTO_SHA512_ARM
> > +   select CRYPTO_HMAC
> 
> Why do you select accelerated algos ?
> Adding COMPILE_TEST could be helpfull also.

Our Hardware has complex procedure on calculate HMAC, and it get a bad
performance So i decide to use ARM NEON instruction as fallback to
speedup it.
I will add COMPILE_TEST.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include "mtk-platform.h"
> > +#include "mtk-regs.h"
> > +
> 
> Sort headers in alphabetical order
> 
> [...]
> > +
> > +   mtk_aes_unregister_algs();
> > +   mtk_aes_record_free(cryp);
> > +}
> > +EXPORT_SYMBOL(mtk_cipher_alg_release);
> 
> Why not EXPORT_SYMBOL_GPL ?
> Furthermore do you really need it to be exported ?

My mistake. I will remove it.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include "mtk-platform.h"
> > +#include "mtk-regs.h"
> > +
> 
> Sort headers in alphabetical order
> 
> [...]
> > +
> > +static void mtk_prng_reseed(struct mtk_cryp *cryp)
> > +{
> > +   /* 8 words to seed the PRNG to provide IVs */
> > +   void __iomem *base = cryp->base;
> > +   const u32 prng_key[8] = {0x48c24cfd, 0x6c07f742,
> > +   0xaee75681, 0x0f27c239,
> > +   0x79947198, 0xe2991275,
> > +   0x21ac3c7c, 0xd008c4b4};
> 
> Why do you seed with thoses constant ?
> 
> [...]
> > +
> > +static int mtk_accelerator_init(struct mtk_cryp *cryp)
> > +{
> > +   int i, err;
> > +
> > +   /* Initialize advanced interrupt controller(AIC) */
> > +   for (i = 0; i < 5; i++) {
> 
> I see this 5 for interrupt away, so perhaps a define could be used
> 
> [...]
> 
> here 
> 
> > +   for (i = 0; i < 5; i++) {
> > +   cryp->irq[i] = platform_get_irq(pdev, i);
> > +   if (cryp->irq[i] < 0) {
> > +   dev_err(cryp->dev, "no IRQ:%d resource info\n",
i);
> > +   return -ENXIO;
> > +   }
> > +   }
> [...]

> > +#ifndef __MTK_PLATFORM_H_
> > +#define __MTK_PLATFORM_H_
> > +
> > +#include 
> > +#include 
> > +#include 
> 
> Sort headers in alphabetical order
> 
> [...]
> > +#define MTK_DESC_FIRST BIT(23)
> > +#define MTK_DESC_BUF_LEN(x)((x) & 0x1)
> > +#define MTK_DESC_CT_LEN(x) (((x) & 0xff) << 24)
> > +
> > +#define WORD(x)((x) >> 2)
> 
> dangerous and ambigous define

I will define a IRQ_NUM and modify ambiguous definition.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> Sort headers in alphabetical order
> [...]
> Generally more function comment could be helpfull.

I will sort all header and add more function comment.
Thanks

Re: [PATCH 1/2] Add crypto driver support for some MediaTek chips

2016-12-02 Thread Ryder Lee 
Hello,

On Fri, 2016-12-02 at 09:18 +0100, Corentin Labbe wrote:
> Hello
> 
> I have some minor comment inline
> 
> On Fri, Dec 02, 2016 at 11:26:44AM +0800, Ryder Lee wrote:
> > This adds support for the MediaTek hardware accelerator on
> > mt7623/mt2701/mt8521p SoC.
> > 
> > This driver currently implement:
> > - SHA1 and SHA2 family(HMAC) hash alogrithms.
> > - AES block cipher in CBC/ECB mode with 128/196/256 bits keys.
> 
> I see also a PRNG but is seems not really used.

Yes, PRNG is not implemented yet, i will remove it temporarily.

> > 
> > Signed-off-by: Ryder Lee 
> > ---
> >  drivers/crypto/Kconfig |   17 +
> >  drivers/crypto/Makefile|1 +
> >  drivers/crypto/mediatek/Makefile   |2 +
> >  drivers/crypto/mediatek/mtk-aes.c  |  734 +
> >  drivers/crypto/mediatek/mtk-platform.c |  575 +
> >  drivers/crypto/mediatek/mtk-platform.h |  230 ++
> >  drivers/crypto/mediatek/mtk-regs.h |  194 +
> >  drivers/crypto/mediatek/mtk-sha.c  | 1384

> >  8 files changed, 3137 insertions(+)
> >  create mode 100644 drivers/crypto/mediatek/Makefile
> >  create mode 100644 drivers/crypto/mediatek/mtk-aes.c
> >  create mode 100644 drivers/crypto/mediatek/mtk-platform.c
> >  create mode 100644 drivers/crypto/mediatek/mtk-platform.h
> >  create mode 100644 drivers/crypto/mediatek/mtk-regs.h
> >  create mode 100644 drivers/crypto/mediatek/mtk-sha.c
> > 
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > index 4d2b81f..5d9c803 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -553,6 +553,23 @@ config CRYPTO_DEV_ROCKCHIP
> >   This driver interfaces with the hardware crypto accelerator.
> >   Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher
mode.
> >  
> > +config CRYPTO_DEV_MEDIATEK
> > +   tristate "MediaTek's Cryptographic Engine driver"
> > +   depends on ARM && ARCH_MEDIATEK
> > +   select NEON
> > +   select KERNEL_MODE_NEON
> > +   select ARM_CRYPTO
> > +   select CRYPTO_AES
> > +   select CRYPTO_BLKCIPHER
> > +   select CRYPTO_SHA1_ARM_NEON
> > +   select CRYPTO_SHA256_ARM
> > +   select CRYPTO_SHA512_ARM
> > +   select CRYPTO_HMAC
> 
> Why do you select accelerated algos ?
> Adding COMPILE_TEST could be helpfull also.

Our Hardware has complex procedure on calculate HMAC, and it get a bad
performance So i decide to use ARM NEON instruction as fallback to
speedup it.
I will add COMPILE_TEST.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include "mtk-platform.h"
> > +#include "mtk-regs.h"
> > +
> 
> Sort headers in alphabetical order
> 
> [...]
> > +
> > +   mtk_aes_unregister_algs();
> > +   mtk_aes_record_free(cryp);
> > +}
> > +EXPORT_SYMBOL(mtk_cipher_alg_release);
> 
> Why not EXPORT_SYMBOL_GPL ?
> Furthermore do you really need it to be exported ?

My mistake. I will remove it.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include "mtk-platform.h"
> > +#include "mtk-regs.h"
> > +
> 
> Sort headers in alphabetical order
> 
> [...]
> > +
> > +static void mtk_prng_reseed(struct mtk_cryp *cryp)
> > +{
> > +   /* 8 words to seed the PRNG to provide IVs */
> > +   void __iomem *base = cryp->base;
> > +   const u32 prng_key[8] = {0x48c24cfd, 0x6c07f742,
> > +   0xaee75681, 0x0f27c239,
> > +   0x79947198, 0xe2991275,
> > +   0x21ac3c7c, 0xd008c4b4};
> 
> Why do you seed with thoses constant ?
> 
> [...]
> > +
> > +static int mtk_accelerator_init(struct mtk_cryp *cryp)
> > +{
> > +   int i, err;
> > +
> > +   /* Initialize advanced interrupt controller(AIC) */
> > +   for (i = 0; i < 5; i++) {
> 
> I see this 5 for interrupt away, so perhaps a define could be used
> 
> [...]
> 
> here 
> 
> > +   for (i = 0; i < 5; i++) {
> > +   cryp->irq[i] = platform_get_irq(pdev, i);
> > +   if (cryp->irq[i] < 0) {
> > +   dev_err(cryp->dev, "no IRQ:%d resource info\n",
i);
> > +   return -ENXIO;
> > +   }
> > +   }
> [...]

> > +#ifndef __MTK_PLATFORM_H_
> > +#define __MTK_PLATFORM_H_
> > +
> > +#include 
> > +#include 
> > +#include 
> 
> Sort headers in alphabetical order
> 
> [...]
> > +#define MTK_DESC_FIRST BIT(23)
> > +#define MTK_DESC_BUF_LEN(x)((x) & 0x1)
> > +#define MTK_DESC_CT_LEN(x) (((x) & 0xff) << 24)
> > +
> > +#define WORD(x)((x) >> 2)
> 
> dangerous and ambigous define

I will define a IRQ_NUM and modify ambiguous definition.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> Sort headers in alphabetical order
> [...]
> Generally more function comment could be helpfull.

I will sort all header and add more function comment.
Thanks for your review.

>

Re: [PATCH 1/2] Add crypto driver support for some MediaTek chips

2016-12-02 Thread mtk09577 

Hello,

On Fri, 2016-12-02 at 09:18 +0100, Corentin Labbe wrote:
> Hello
> 
> I have some minor comment inline
> 
> On Fri, Dec 02, 2016 at 11:26:44AM +0800, Ryder Lee wrote:
> > This adds support for the MediaTek hardware accelerator on
> > mt7623/mt2701/mt8521p SoC.
> > 
> > This driver currently implement:
> > - SHA1 and SHA2 family(HMAC) hash alogrithms.
> > - AES block cipher in CBC/ECB mode with 128/196/256 bits keys.
> 
> I see also a PRNG but is seems not really used.

Yes, PRNG is not implemented yet, i will remove it temporarily.

> > 
> > Signed-off-by: Ryder Lee 
> > ---
> >  drivers/crypto/Kconfig |   17 +
> >  drivers/crypto/Makefile|1 +
> >  drivers/crypto/mediatek/Makefile   |2 +
> >  drivers/crypto/mediatek/mtk-aes.c  |  734 +
> >  drivers/crypto/mediatek/mtk-platform.c |  575 +
> >  drivers/crypto/mediatek/mtk-platform.h |  230 ++
> >  drivers/crypto/mediatek/mtk-regs.h |  194 +
> >  drivers/crypto/mediatek/mtk-sha.c  | 1384 
> > 
> >  8 files changed, 3137 insertions(+)
> >  create mode 100644 drivers/crypto/mediatek/Makefile
> >  create mode 100644 drivers/crypto/mediatek/mtk-aes.c
> >  create mode 100644 drivers/crypto/mediatek/mtk-platform.c
> >  create mode 100644 drivers/crypto/mediatek/mtk-platform.h
> >  create mode 100644 drivers/crypto/mediatek/mtk-regs.h
> >  create mode 100644 drivers/crypto/mediatek/mtk-sha.c
> > 
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > index 4d2b81f..5d9c803 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -553,6 +553,23 @@ config CRYPTO_DEV_ROCKCHIP
> >   This driver interfaces with the hardware crypto accelerator.
> >   Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
> >  
> > +config CRYPTO_DEV_MEDIATEK
> > +   tristate "MediaTek's Cryptographic Engine driver"
> > +   depends on ARM && ARCH_MEDIATEK
> > +   select NEON
> > +   select KERNEL_MODE_NEON
> > +   select ARM_CRYPTO
> > +   select CRYPTO_AES
> > +   select CRYPTO_BLKCIPHER
> > +   select CRYPTO_SHA1_ARM_NEON
> > +   select CRYPTO_SHA256_ARM
> > +   select CRYPTO_SHA512_ARM
> > +   select CRYPTO_HMAC
> 
> Why do you select accelerated algos ?
> Adding COMPILE_TEST could be helpfull also.

Our Hardware has complex procedure on calculate HMAC, and it get a bad
performance So i decide to use ARM NEON instruction as fallback to
speedup it.
I will add COMPILE_TEST.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include "mtk-platform.h"
> > +#include "mtk-regs.h"
> > +
> 
> Sort headers in alphabetical order
> 
> [...]
> > +
> > +   mtk_aes_unregister_algs();
> > +   mtk_aes_record_free(cryp);
> > +}
> > +EXPORT_SYMBOL(mtk_cipher_alg_release);
> 
> Why not EXPORT_SYMBOL_GPL ?
> Furthermore do you really need it to be exported ?

My mistake. I will remove it.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include "mtk-platform.h"
> > +#include "mtk-regs.h"
> > +
> 
> Sort headers in alphabetical order
> 
> [...]
> > +
> > +static void mtk_prng_reseed(struct mtk_cryp *cryp)
> > +{
> > +   /* 8 words to seed the PRNG to provide IVs */
> > +   void __iomem *base = cryp->base;
> > +   const u32 prng_key[8] = {0x48c24cfd, 0x6c07f742,
> > +   0xaee75681, 0x0f27c239,
> > +   0x79947198, 0xe2991275,
> > +   0x21ac3c7c, 0xd008c4b4};
> 
> Why do you seed with thoses constant ?
> 
> [...]
> > +
> > +static int mtk_accelerator_init(struct mtk_cryp *cryp)
> > +{
> > +   int i, err;
> > +
> > +   /* Initialize advanced interrupt controller(AIC) */
> > +   for (i = 0; i < 5; i++) {
> 
> I see this 5 for interrupt away, so perhaps a define could be used
> 
> [...]
> 
> here 
> 
> > +   for (i = 0; i < 5; i++) {
> > +   cryp->irq[i] = platform_get_irq(pdev, i);
> > +   if (cryp->irq[i] < 0) {
> > +   dev_err(cryp->dev, "no IRQ:%d resource info\n", i);
> > +   return -ENXIO;
> > +   }
> > +   }
> [...]

> > +#ifndef __MTK_PLATFORM_H_
> > +#define __MTK_PLATFORM_H_
> > +
> > +#include 
> > +#include 
> > +#include 
> 
> Sort headers in alphabetical order
> 
> [...]
> > +#define MTK_DESC_FIRST BIT(23)
> > +#define MTK_DESC_BUF_LEN(x)((x) & 0x1)
> > +#define MTK_DESC_CT_LEN(x) (((x) & 0xff) << 24)
> > +
> > +#define WORD(x)((x) >> 2)
> 
> dangerous and ambigous define

I will define a IRQ_NUM and modify ambiguous definition.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> Sort headers in alphabetical order
> [...]
> Generally more function comment could be helpfull.

I will sort all header and add more function comment.

Re: [PATCH 1/2] Add crypto driver support for some MediaTek chips

2016-12-02 Thread mtk09577 

Hello,

On Fri, 2016-12-02 at 09:18 +0100, Corentin Labbe wrote:
> Hello
> 
> I have some minor comment inline
> 
> On Fri, Dec 02, 2016 at 11:26:44AM +0800, Ryder Lee wrote:
> > This adds support for the MediaTek hardware accelerator on
> > mt7623/mt2701/mt8521p SoC.
> > 
> > This driver currently implement:
> > - SHA1 and SHA2 family(HMAC) hash alogrithms.
> > - AES block cipher in CBC/ECB mode with 128/196/256 bits keys.
> 
> I see also a PRNG but is seems not really used.

Yes, PRNG is not implemented yet, i will remove it temporarily.

> > 
> > Signed-off-by: Ryder Lee 
> > ---
> >  drivers/crypto/Kconfig |   17 +
> >  drivers/crypto/Makefile|1 +
> >  drivers/crypto/mediatek/Makefile   |2 +
> >  drivers/crypto/mediatek/mtk-aes.c  |  734 +
> >  drivers/crypto/mediatek/mtk-platform.c |  575 +
> >  drivers/crypto/mediatek/mtk-platform.h |  230 ++
> >  drivers/crypto/mediatek/mtk-regs.h |  194 +
> >  drivers/crypto/mediatek/mtk-sha.c  | 1384 
> > 
> >  8 files changed, 3137 insertions(+)
> >  create mode 100644 drivers/crypto/mediatek/Makefile
> >  create mode 100644 drivers/crypto/mediatek/mtk-aes.c
> >  create mode 100644 drivers/crypto/mediatek/mtk-platform.c
> >  create mode 100644 drivers/crypto/mediatek/mtk-platform.h
> >  create mode 100644 drivers/crypto/mediatek/mtk-regs.h
> >  create mode 100644 drivers/crypto/mediatek/mtk-sha.c
> > 
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > index 4d2b81f..5d9c803 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -553,6 +553,23 @@ config CRYPTO_DEV_ROCKCHIP
> >   This driver interfaces with the hardware crypto accelerator.
> >   Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
> >  
> > +config CRYPTO_DEV_MEDIATEK
> > +   tristate "MediaTek's Cryptographic Engine driver"
> > +   depends on ARM && ARCH_MEDIATEK
> > +   select NEON
> > +   select KERNEL_MODE_NEON
> > +   select ARM_CRYPTO
> > +   select CRYPTO_AES
> > +   select CRYPTO_BLKCIPHER
> > +   select CRYPTO_SHA1_ARM_NEON
> > +   select CRYPTO_SHA256_ARM
> > +   select CRYPTO_SHA512_ARM
> > +   select CRYPTO_HMAC
> 
> Why do you select accelerated algos ?
> Adding COMPILE_TEST could be helpfull also.

Our Hardware has complex procedure on calculate HMAC, and it get a bad
performance So i decide to use ARM NEON instruction as fallback to
speedup it.
I will add COMPILE_TEST.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include "mtk-platform.h"
> > +#include "mtk-regs.h"
> > +
> 
> Sort headers in alphabetical order
> 
> [...]
> > +
> > +   mtk_aes_unregister_algs();
> > +   mtk_aes_record_free(cryp);
> > +}
> > +EXPORT_SYMBOL(mtk_cipher_alg_release);
> 
> Why not EXPORT_SYMBOL_GPL ?
> Furthermore do you really need it to be exported ?

My mistake. I will remove it.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include "mtk-platform.h"
> > +#include "mtk-regs.h"
> > +
> 
> Sort headers in alphabetical order
> 
> [...]
> > +
> > +static void mtk_prng_reseed(struct mtk_cryp *cryp)
> > +{
> > +   /* 8 words to seed the PRNG to provide IVs */
> > +   void __iomem *base = cryp->base;
> > +   const u32 prng_key[8] = {0x48c24cfd, 0x6c07f742,
> > +   0xaee75681, 0x0f27c239,
> > +   0x79947198, 0xe2991275,
> > +   0x21ac3c7c, 0xd008c4b4};
> 
> Why do you seed with thoses constant ?
> 
> [...]
> > +
> > +static int mtk_accelerator_init(struct mtk_cryp *cryp)
> > +{
> > +   int i, err;
> > +
> > +   /* Initialize advanced interrupt controller(AIC) */
> > +   for (i = 0; i < 5; i++) {
> 
> I see this 5 for interrupt away, so perhaps a define could be used
> 
> [...]
> 
> here 
> 
> > +   for (i = 0; i < 5; i++) {
> > +   cryp->irq[i] = platform_get_irq(pdev, i);
> > +   if (cryp->irq[i] < 0) {
> > +   dev_err(cryp->dev, "no IRQ:%d resource info\n", i);
> > +   return -ENXIO;
> > +   }
> > +   }
> [...]

> > +#ifndef __MTK_PLATFORM_H_
> > +#define __MTK_PLATFORM_H_
> > +
> > +#include 
> > +#include 
> > +#include 
> 
> Sort headers in alphabetical order
> 
> [...]
> > +#define MTK_DESC_FIRST BIT(23)
> > +#define MTK_DESC_BUF_LEN(x)((x) & 0x1)
> > +#define MTK_DESC_CT_LEN(x) (((x) & 0xff) << 24)
> > +
> > +#define WORD(x)((x) >> 2)
> 
> dangerous and ambigous define

I will define a IRQ_NUM and modify ambiguous definition.

> [...]
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> Sort headers in alphabetical order
> [...]
> Generally more function comment could be helpfull.

I will sort all header and add more function comment.
Thanks for your review.

Re: [PATCH 1/2] Add crypto driver support for some MediaTek chips

2016-12-02 Thread Corentin Labbe 
Hello

I have some minor comment inline

On Fri, Dec 02, 2016 at 11:26:44AM +0800, Ryder Lee wrote:
> This adds support for the MediaTek hardware accelerator on
> mt7623/mt2701/mt8521p SoC.
> 
> This driver currently implement:
> - SHA1 and SHA2 family(HMAC) hash alogrithms.
> - AES block cipher in CBC/ECB mode with 128/196/256 bits keys.

I see also a PRNG but is seems not really used.

> 
> Signed-off-by: Ryder Lee 
> ---
>  drivers/crypto/Kconfig |   17 +
>  drivers/crypto/Makefile|1 +
>  drivers/crypto/mediatek/Makefile   |2 +
>  drivers/crypto/mediatek/mtk-aes.c  |  734 +
>  drivers/crypto/mediatek/mtk-platform.c |  575 +
>  drivers/crypto/mediatek/mtk-platform.h |  230 ++
>  drivers/crypto/mediatek/mtk-regs.h |  194 +
>  drivers/crypto/mediatek/mtk-sha.c  | 1384 
> 
>  8 files changed, 3137 insertions(+)
>  create mode 100644 drivers/crypto/mediatek/Makefile
>  create mode 100644 drivers/crypto/mediatek/mtk-aes.c
>  create mode 100644 drivers/crypto/mediatek/mtk-platform.c
>  create mode 100644 drivers/crypto/mediatek/mtk-platform.h
>  create mode 100644 drivers/crypto/mediatek/mtk-regs.h
>  create mode 100644 drivers/crypto/mediatek/mtk-sha.c
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 4d2b81f..5d9c803 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -553,6 +553,23 @@ config CRYPTO_DEV_ROCKCHIP
> This driver interfaces with the hardware crypto accelerator.
> Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
>  
> +config CRYPTO_DEV_MEDIATEK
> + tristate "MediaTek's Cryptographic Engine driver"
> + depends on ARM && ARCH_MEDIATEK
> + select NEON
> + select KERNEL_MODE_NEON
> + select ARM_CRYPTO
> + select CRYPTO_AES
> + select CRYPTO_BLKCIPHER
> + select CRYPTO_SHA1_ARM_NEON
> + select CRYPTO_SHA256_ARM
> + select CRYPTO_SHA512_ARM
> + select CRYPTO_HMAC

Why do you select accelerated algos ?
Adding COMPILE_TEST could be helpfull also.

[...]
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "mtk-platform.h"
> +#include "mtk-regs.h"
> +

Sort headers in alphabetical order

[...]
> +
> + mtk_aes_unregister_algs();
> + mtk_aes_record_free(cryp);
> +}
> +EXPORT_SYMBOL(mtk_cipher_alg_release);

Why not EXPORT_SYMBOL_GPL ?
Furthermore do you really need it to be exported ?

[...]
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "mtk-platform.h"
> +#include "mtk-regs.h"
> +

Sort headers in alphabetical order

[...]
> +
> +static void mtk_prng_reseed(struct mtk_cryp *cryp)
> +{
> + /* 8 words to seed the PRNG to provide IVs */
> + void __iomem *base = cryp->base;
> + const u32 prng_key[8] = {0x48c24cfd, 0x6c07f742,
> + 0xaee75681, 0x0f27c239,
> + 0x79947198, 0xe2991275,
> + 0x21ac3c7c, 0xd008c4b4};

Why do you seed with thoses constant ?

[...]
> +
> +static int mtk_accelerator_init(struct mtk_cryp *cryp)
> +{
> + int i, err;
> +
> + /* Initialize advanced interrupt controller(AIC) */
> + for (i = 0; i < 5; i++) {

I see this 5 for interrupt away, so perhaps a define could be used

[...]

here 

> + for (i = 0; i < 5; i++) {
> + cryp->irq[i] = platform_get_irq(pdev, i);
> + if (cryp->irq[i] < 0) {
> + dev_err(cryp->dev, "no IRQ:%d resource info\n", i);
> + return -ENXIO;
> + }
> + }
[...]

> +#ifndef __MTK_PLATFORM_H_
> +#define __MTK_PLATFORM_H_
> +
> +#include 
> +#include 
> +#include 

Sort headers in alphabetical order

[...]
> +#define MTK_DESC_FIRST   BIT(23)
> +#define MTK_DESC_BUF_LEN(x)  ((x) & 0x1)
> +#define MTK_DESC_CT_LEN(x)   (((x) & 0xff) << 24)
> +
> +#define WORD(x)  ((x) >> 2)

dangerous and ambigous define

[...]
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Sort headers in alphabetical order
[...]

Generally more function comment could be helpfull.

Regards

Re: [PATCH 1/2] Add crypto driver support for some MediaTek chips

2016-12-02 Thread Corentin Labbe 
Hello

I have some minor comment inline

On Fri, Dec 02, 2016 at 11:26:44AM +0800, Ryder Lee wrote:
> This adds support for the MediaTek hardware accelerator on
> mt7623/mt2701/mt8521p SoC.
> 
> This driver currently implement:
> - SHA1 and SHA2 family(HMAC) hash alogrithms.
> - AES block cipher in CBC/ECB mode with 128/196/256 bits keys.

I see also a PRNG but is seems not really used.

> 
> Signed-off-by: Ryder Lee 
> ---
>  drivers/crypto/Kconfig |   17 +
>  drivers/crypto/Makefile|1 +
>  drivers/crypto/mediatek/Makefile   |2 +
>  drivers/crypto/mediatek/mtk-aes.c  |  734 +
>  drivers/crypto/mediatek/mtk-platform.c |  575 +
>  drivers/crypto/mediatek/mtk-platform.h |  230 ++
>  drivers/crypto/mediatek/mtk-regs.h |  194 +
>  drivers/crypto/mediatek/mtk-sha.c  | 1384 
> 
>  8 files changed, 3137 insertions(+)
>  create mode 100644 drivers/crypto/mediatek/Makefile
>  create mode 100644 drivers/crypto/mediatek/mtk-aes.c
>  create mode 100644 drivers/crypto/mediatek/mtk-platform.c
>  create mode 100644 drivers/crypto/mediatek/mtk-platform.h
>  create mode 100644 drivers/crypto/mediatek/mtk-regs.h
>  create mode 100644 drivers/crypto/mediatek/mtk-sha.c
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 4d2b81f..5d9c803 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -553,6 +553,23 @@ config CRYPTO_DEV_ROCKCHIP
> This driver interfaces with the hardware crypto accelerator.
> Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
>  
> +config CRYPTO_DEV_MEDIATEK
> + tristate "MediaTek's Cryptographic Engine driver"
> + depends on ARM && ARCH_MEDIATEK
> + select NEON
> + select KERNEL_MODE_NEON
> + select ARM_CRYPTO
> + select CRYPTO_AES
> + select CRYPTO_BLKCIPHER
> + select CRYPTO_SHA1_ARM_NEON
> + select CRYPTO_SHA256_ARM
> + select CRYPTO_SHA512_ARM
> + select CRYPTO_HMAC

Why do you select accelerated algos ?
Adding COMPILE_TEST could be helpfull also.

[...]
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "mtk-platform.h"
> +#include "mtk-regs.h"
> +

Sort headers in alphabetical order

[...]
> +
> + mtk_aes_unregister_algs();
> + mtk_aes_record_free(cryp);
> +}
> +EXPORT_SYMBOL(mtk_cipher_alg_release);

Why not EXPORT_SYMBOL_GPL ?
Furthermore do you really need it to be exported ?

[...]
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "mtk-platform.h"
> +#include "mtk-regs.h"
> +

Sort headers in alphabetical order

[...]
> +
> +static void mtk_prng_reseed(struct mtk_cryp *cryp)
> +{
> + /* 8 words to seed the PRNG to provide IVs */
> + void __iomem *base = cryp->base;
> + const u32 prng_key[8] = {0x48c24cfd, 0x6c07f742,
> + 0xaee75681, 0x0f27c239,
> + 0x79947198, 0xe2991275,
> + 0x21ac3c7c, 0xd008c4b4};

Why do you seed with thoses constant ?

[...]
> +
> +static int mtk_accelerator_init(struct mtk_cryp *cryp)
> +{
> + int i, err;
> +
> + /* Initialize advanced interrupt controller(AIC) */
> + for (i = 0; i < 5; i++) {

I see this 5 for interrupt away, so perhaps a define could be used

[...]

here 

> + for (i = 0; i < 5; i++) {
> + cryp->irq[i] = platform_get_irq(pdev, i);
> + if (cryp->irq[i] < 0) {
> + dev_err(cryp->dev, "no IRQ:%d resource info\n", i);
> + return -ENXIO;
> + }
> + }
[...]

> +#ifndef __MTK_PLATFORM_H_
> +#define __MTK_PLATFORM_H_
> +
> +#include 
> +#include 
> +#include 

Sort headers in alphabetical order

[...]
> +#define MTK_DESC_FIRST   BIT(23)
> +#define MTK_DESC_BUF_LEN(x)  ((x) & 0x1)
> +#define MTK_DESC_CT_LEN(x)   (((x) & 0xff) << 24)
> +
> +#define WORD(x)  ((x) >> 2)

dangerous and ambigous define

[...]
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Sort headers in alphabetical order
[...]

Generally more function comment could be helpfull.

Regards

[PATCH 1/2] Add crypto driver support for some MediaTek chips

2016-12-01 Thread Ryder Lee 
This adds support for the MediaTek hardware accelerator on
mt7623/mt2701/mt8521p SoC.

This driver currently implement:
- SHA1 and SHA2 family(HMAC) hash alogrithms.
- AES block cipher in CBC/ECB mode with 128/196/256 bits keys.

Signed-off-by: Ryder Lee 
---
 drivers/crypto/Kconfig |   17 +
 drivers/crypto/Makefile|1 +
 drivers/crypto/mediatek/Makefile   |2 +
 drivers/crypto/mediatek/mtk-aes.c  |  734 +
 drivers/crypto/mediatek/mtk-platform.c |  575 +
 drivers/crypto/mediatek/mtk-platform.h |  230 ++
 drivers/crypto/mediatek/mtk-regs.h |  194 +
 drivers/crypto/mediatek/mtk-sha.c  | 1384 
 8 files changed, 3137 insertions(+)
 create mode 100644 drivers/crypto/mediatek/Makefile
 create mode 100644 drivers/crypto/mediatek/mtk-aes.c
 create mode 100644 drivers/crypto/mediatek/mtk-platform.c
 create mode 100644 drivers/crypto/mediatek/mtk-platform.h
 create mode 100644 drivers/crypto/mediatek/mtk-regs.h
 create mode 100644 drivers/crypto/mediatek/mtk-sha.c

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 4d2b81f..5d9c803 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -553,6 +553,23 @@ config CRYPTO_DEV_ROCKCHIP
  This driver interfaces with the hardware crypto accelerator.
  Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
 
+config CRYPTO_DEV_MEDIATEK
+   tristate "MediaTek's Cryptographic Engine driver"
+   depends on ARM && ARCH_MEDIATEK
+   select NEON
+   select KERNEL_MODE_NEON
+   select ARM_CRYPTO
+   select CRYPTO_AES
+   select CRYPTO_BLKCIPHER
+   select CRYPTO_SHA1_ARM_NEON
+   select CRYPTO_SHA256_ARM
+   select CRYPTO_SHA512_ARM
+   select CRYPTO_HMAC
+   help
+ This driver allows you to utilize the hardware crypto accelerator
+ which can be found on the MT7623 MT2701, MT8521p, etc 
+ Select this if you want to use it for AES/SHA1/SHA2 algorithms.
+
 source "drivers/crypto/chelsio/Kconfig"
 
 endif # CRYPTO_HW
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index ad7250f..272b51a 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_CRYPTO_DEV_IMGTEC_HASH) += img-hash.o
 obj-$(CONFIG_CRYPTO_DEV_IXP4XX) += ixp4xx_crypto.o
 obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o
 obj-$(CONFIG_CRYPTO_DEV_MARVELL_CESA) += marvell/
+obj-$(CONFIG_CRYPTO_DEV_MEDIATEK) += mediatek/
 obj-$(CONFIG_CRYPTO_DEV_MXS_DCP) += mxs-dcp.o
 obj-$(CONFIG_CRYPTO_DEV_NIAGARA2) += n2_crypto.o
 n2_crypto-y := n2_core.o n2_asm.o
diff --git a/drivers/crypto/mediatek/Makefile b/drivers/crypto/mediatek/Makefile
new file mode 100644
index 000..187be79
--- /dev/null
+++ b/drivers/crypto/mediatek/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_CRYPTO_DEV_MEDIATEK) += mtk-crypto.o
+mtk-crypto-objs:= mtk-platform.o mtk-aes.o mtk-sha.o
diff --git a/drivers/crypto/mediatek/mtk-aes.c 
b/drivers/crypto/mediatek/mtk-aes.c
new file mode 100644
index 000..feb0e57
--- /dev/null
+++ b/drivers/crypto/mediatek/mtk-aes.c
@@ -0,0 +1,734 @@
+/*
+ * Cryptographic API.
+ *
+ * Support for MediaTek AES hardware accelerator.
+ *
+ * Copyright (c) 2016 MediaTek Inc.
+ * Author: Ryder Lee 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Some ideas are from atmel-aes.c drivers.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "mtk-platform.h"
+#include "mtk-regs.h"
+
+#define AES_QUEUE_LENGTH   512
+#define AES_BUFFER_ORDER   2
+#define AES_BUFFER_SIZE((PAGE_SIZE << AES_BUFFER_ORDER) \
+   & ~(AES_BLOCK_SIZE - 1))
+
+/* AES command token */
+#define AES_CT_SIZE_ECB2
+#define AES_CT_SIZE_CBC3
+#define AES_CT_CTRL_HDR0x0022
+#define AES_COMMAND0   0x0500
+#define AES_COMMAND1   0x2d06
+#define AES_COMMAND2   0xe4a63806
+
+/* AES transform information */
+#define AES_TFM_ECB(0x0 << 0)
+#define AES_TFM_CBC(0x1 << 0)
+#define AES_TFM_DECRYPT(0x5 << 0)
+#define AES_TFM_ENCRYPT(0x4 << 0)
+#define AES_TFM_SIZE(x)((x) << 8)
+#define AES_TFM_128BITS(0xb << 16)
+#define AES_TFM_192BITS(0xd << 16)
+#define AES_TFM_256BITS(0xf << 16)
+#define AES_TFM_FULL_IV(0xf << 5)
+
+/* AES flags */
+#define AES_FLAGS_MODE_MSK GENMASK(2, 0)
+#define AES_FLAGS_ECB  BIT(0)
+#define AES_FLAGS_CBC  BIT(1)
+#define AES_FLAGS_ENCRYPT  BIT(2)
+#define AES_FLAGS_BUSY BIT(3)
+
+/* AES command token */
+struct mtk_aes_ct {
+   u32 ct_ctrl0;
+

[PATCH 1/2] Add crypto driver support for some MediaTek chips

2016-12-01 Thread Ryder Lee 
This adds support for the MediaTek hardware accelerator on
mt7623/mt2701/mt8521p SoC.

This driver currently implement:
- SHA1 and SHA2 family(HMAC) hash alogrithms.
- AES block cipher in CBC/ECB mode with 128/196/256 bits keys.

Signed-off-by: Ryder Lee 
---
 drivers/crypto/Kconfig |   17 +
 drivers/crypto/Makefile|1 +
 drivers/crypto/mediatek/Makefile   |2 +
 drivers/crypto/mediatek/mtk-aes.c  |  734 +
 drivers/crypto/mediatek/mtk-platform.c |  575 +
 drivers/crypto/mediatek/mtk-platform.h |  230 ++
 drivers/crypto/mediatek/mtk-regs.h |  194 +
 drivers/crypto/mediatek/mtk-sha.c  | 1384 
 8 files changed, 3137 insertions(+)
 create mode 100644 drivers/crypto/mediatek/Makefile
 create mode 100644 drivers/crypto/mediatek/mtk-aes.c
 create mode 100644 drivers/crypto/mediatek/mtk-platform.c
 create mode 100644 drivers/crypto/mediatek/mtk-platform.h
 create mode 100644 drivers/crypto/mediatek/mtk-regs.h
 create mode 100644 drivers/crypto/mediatek/mtk-sha.c

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 4d2b81f..5d9c803 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -553,6 +553,23 @@ config CRYPTO_DEV_ROCKCHIP
  This driver interfaces with the hardware crypto accelerator.
  Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
 
+config CRYPTO_DEV_MEDIATEK
+   tristate "MediaTek's Cryptographic Engine driver"
+   depends on ARM && ARCH_MEDIATEK
+   select NEON
+   select KERNEL_MODE_NEON
+   select ARM_CRYPTO
+   select CRYPTO_AES
+   select CRYPTO_BLKCIPHER
+   select CRYPTO_SHA1_ARM_NEON
+   select CRYPTO_SHA256_ARM
+   select CRYPTO_SHA512_ARM
+   select CRYPTO_HMAC
+   help
+ This driver allows you to utilize the hardware crypto accelerator
+ which can be found on the MT7623 MT2701, MT8521p, etc 
+ Select this if you want to use it for AES/SHA1/SHA2 algorithms.
+
 source "drivers/crypto/chelsio/Kconfig"
 
 endif # CRYPTO_HW
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index ad7250f..272b51a 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_CRYPTO_DEV_IMGTEC_HASH) += img-hash.o
 obj-$(CONFIG_CRYPTO_DEV_IXP4XX) += ixp4xx_crypto.o
 obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o
 obj-$(CONFIG_CRYPTO_DEV_MARVELL_CESA) += marvell/
+obj-$(CONFIG_CRYPTO_DEV_MEDIATEK) += mediatek/
 obj-$(CONFIG_CRYPTO_DEV_MXS_DCP) += mxs-dcp.o
 obj-$(CONFIG_CRYPTO_DEV_NIAGARA2) += n2_crypto.o
 n2_crypto-y := n2_core.o n2_asm.o
diff --git a/drivers/crypto/mediatek/Makefile b/drivers/crypto/mediatek/Makefile
new file mode 100644
index 000..187be79
--- /dev/null
+++ b/drivers/crypto/mediatek/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_CRYPTO_DEV_MEDIATEK) += mtk-crypto.o
+mtk-crypto-objs:= mtk-platform.o mtk-aes.o mtk-sha.o
diff --git a/drivers/crypto/mediatek/mtk-aes.c 
b/drivers/crypto/mediatek/mtk-aes.c
new file mode 100644
index 000..feb0e57
--- /dev/null
+++ b/drivers/crypto/mediatek/mtk-aes.c
@@ -0,0 +1,734 @@
+/*
+ * Cryptographic API.
+ *
+ * Support for MediaTek AES hardware accelerator.
+ *
+ * Copyright (c) 2016 MediaTek Inc.
+ * Author: Ryder Lee 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Some ideas are from atmel-aes.c drivers.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "mtk-platform.h"
+#include "mtk-regs.h"
+
+#define AES_QUEUE_LENGTH   512
+#define AES_BUFFER_ORDER   2
+#define AES_BUFFER_SIZE((PAGE_SIZE << AES_BUFFER_ORDER) \
+   & ~(AES_BLOCK_SIZE - 1))
+
+/* AES command token */
+#define AES_CT_SIZE_ECB2
+#define AES_CT_SIZE_CBC3
+#define AES_CT_CTRL_HDR0x0022
+#define AES_COMMAND0   0x0500
+#define AES_COMMAND1   0x2d06
+#define AES_COMMAND2   0xe4a63806
+
+/* AES transform information */
+#define AES_TFM_ECB(0x0 << 0)
+#define AES_TFM_CBC(0x1 << 0)
+#define AES_TFM_DECRYPT(0x5 << 0)
+#define AES_TFM_ENCRYPT(0x4 << 0)
+#define AES_TFM_SIZE(x)((x) << 8)
+#define AES_TFM_128BITS(0xb << 16)
+#define AES_TFM_192BITS(0xd << 16)
+#define AES_TFM_256BITS(0xf << 16)
+#define AES_TFM_FULL_IV(0xf << 5)
+
+/* AES flags */
+#define AES_FLAGS_MODE_MSK GENMASK(2, 0)
+#define AES_FLAGS_ECB  BIT(0)
+#define AES_FLAGS_CBC  BIT(1)
+#define AES_FLAGS_ENCRYPT  BIT(2)
+#define AES_FLAGS_BUSY BIT(3)
+
+/* AES command token */
+struct mtk_aes_ct {
+   u32 ct_ctrl0;
+   u32 ct_ctrl1;
+   u32 ct_ctrl2;
+};
+
+/*