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 <ryder.lee@xxxxxxxxxxxx>
> > ---
> > 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 <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
>
> [...]
> > +
> > + 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 <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
>
> [...]
> > +
> > +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 <linux/crypto.h>
> > +#include <crypto/internal/hash.h>
> > +#include <linux/interrupt.h>
>
> Sort headers in alphabetical order
>
> [...]
> > +#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.
> [...]
> > +
> > +#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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
