Re: [PATCH 1/2] Add crypto driver support for some MediaTek chips
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. > Rega
Re: [PATCH 1/2] Add crypto driver support for some MediaTek chips
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Add crypto driver support for some MediaTek chips
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; +}; + +/* AES