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:

quoted

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.

quoted

Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
---
 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.

[...]

quoted

+
+#include <linux/dma-mapping.h>
+#include <linux/scatterlist.h>
+#include <crypto/scatterwalk.h>
+#include <crypto/algapi.h>
+#include <crypto/aes.h>
+#include "mtk-platform.h"
+#include "mtk-regs.h"
+

Sort headers in alphabetical order

[...]

quoted

+
+	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.

[...]

quoted

+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/pm_runtime.h>
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include "mtk-platform.h"
+#include "mtk-regs.h"
+

Sort headers in alphabetical order

[...]

quoted

+
+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 ?

[...]

quoted

+
+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 

quoted

+	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;
+		}
+	}

[...]

quoted

+#ifndef __MTK_PLATFORM_H_
+#define __MTK_PLATFORM_H_
+
+#include <linux/crypto.h>
+#include <crypto/internal/hash.h>
+#include <linux/interrupt.h>

Sort headers in alphabetical order

[...]

quoted

+#define MTK_DESC_FIRST		BIT(23)
+#define MTK_DESC_BUF_LEN(x)	((x) & 0x1ffff)
+#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.

[...]

quoted

+
+#include <linux/dma-mapping.h>
+#include <linux/scatterlist.h>
+#include <linux/crypto.h>
+#include <crypto/scatterwalk.h>
+#include <crypto/algapi.h>
+#include <crypto/sha.h>
+#include <crypto/internal/hash.h>

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.

Regards