* [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine
@ 2019-09-06 18:45 Corentin Labbe
2019-09-06 18:45 ` [PATCH 1/9] crypto: Add allwinner subdirectory Corentin Labbe
` (8 more replies)
0 siblings, 9 replies; 32+ messages in thread
From: Corentin Labbe @ 2019-09-06 18:45 UTC (permalink / raw)
To: davem, herbert, linux, mark.rutland, mripard, robh+dt, wens
Cc: devicetree, linux-arm-kernel, linux-crypto, linux-kernel,
linux-sunxi, Corentin Labbe
Hello
This patch serie adds support for the Allwinner crypto engine.
The Crypto Engine is the third generation of Allwinner cryptogaphic offloader.
The first generation is the Security System already handled by the
sun4i-ss driver.
The second is named also Security System and is present on A80 and A83T
SoCs, originaly this driver supported it also, but supporting both IP bringing
too much complexity and another driver (sun8i-ss) will came for it.
For the moment, the driver support only DES3/AES in ECB/CBC mode.
Patchs for CTR/CTS/XTS and RNGs will came later.
Regards
Corentin Labbe (9):
crypto: Add allwinner subdirectory
crypto: Add Allwinner sun8i-ce Crypto Engine
dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto
Engine
ARM: dts: sun8i: r40: add crypto engine node
ARM: dts: sun8i: h3: Add Crypto Engine node
ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64
ARM64: dts: allwinner: sun50i: Add crypto engine node on H5
ARM64: dts: allwinner: sun50i: Add Crypto Engine node on H6
sunxi_defconfig: add new crypto options
.../bindings/crypto/allwinner,sun8i-ce.yaml | 84 +++
MAINTAINERS | 6 +
arch/arm/boot/dts/sun8i-h3.dtsi | 11 +
arch/arm/boot/dts/sun8i-r40.dtsi | 11 +
arch/arm/configs/sunxi_defconfig | 2 +
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +
arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 11 +
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 +
drivers/crypto/Kconfig | 2 +
drivers/crypto/Makefile | 1 +
drivers/crypto/allwinner/Kconfig | 32 +
drivers/crypto/allwinner/Makefile | 1 +
drivers/crypto/allwinner/sun8i-ce/Makefile | 2 +
.../allwinner/sun8i-ce/sun8i-ce-cipher.c | 390 +++++++++++
.../crypto/allwinner/sun8i-ce/sun8i-ce-core.c | 630 ++++++++++++++++++
drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h | 256 +++++++
16 files changed, 1460 insertions(+)
create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
create mode 100644 drivers/crypto/allwinner/Kconfig
create mode 100644 drivers/crypto/allwinner/Makefile
create mode 100644 drivers/crypto/allwinner/sun8i-ce/Makefile
create mode 100644 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
create mode 100644 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
create mode 100644 drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
--
2.21.0
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 1/9] crypto: Add allwinner subdirectory 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe @ 2019-09-06 18:45 ` Corentin Labbe 2019-09-07 3:54 ` Maxime Ripard 2019-09-06 18:45 ` [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine Corentin Labbe ` (7 subsequent siblings) 8 siblings, 1 reply; 32+ messages in thread From: Corentin Labbe @ 2019-09-06 18:45 UTC (permalink / raw) To: davem, herbert, linux, mark.rutland, mripard, robh+dt, wens Cc: devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi, Corentin Labbe Since a second Allwinner crypto driver will be added, it is better to create a dedicated subdirectory. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- MAINTAINERS | 6 ++++++ drivers/crypto/Kconfig | 2 ++ drivers/crypto/Makefile | 1 + drivers/crypto/allwinner/Kconfig | 6 ++++++ 4 files changed, 15 insertions(+) create mode 100644 drivers/crypto/allwinner/Kconfig diff --git a/MAINTAINERS b/MAINTAINERS index ee4e873c0f9a..d62ddf8ff262 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -681,6 +681,12 @@ L: linux-crypto@vger.kernel.org S: Maintained F: drivers/crypto/sunxi-ss/ +ALLWINNER CRYPTO ENGINE +M: Corentin Labbe <clabbe.montjoie@gmail.com> +L: linux-crypto@vger.kernel.org +S: Maintained +F: drivers/crypto/allwinner/ + ALLWINNER VPU DRIVER M: Maxime Ripard <mripard@kernel.org> M: Paul Kocialkowski <paul.kocialkowski@bootlin.com> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index 83271d944a96..fe90dd74797c 100644 --- a/drivers/crypto/Kconfig +++ b/drivers/crypto/Kconfig @@ -11,6 +11,8 @@ menuconfig CRYPTO_HW if CRYPTO_HW +source "drivers/crypto/allwinner/Kconfig" + config CRYPTO_DEV_PADLOCK tristate "Support for VIA PadLock ACE" depends on X86 && !UML diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile index afc4753b5d28..90d60eff5ecc 100644 --- a/drivers/crypto/Makefile +++ b/drivers/crypto/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_CRYPTO_DEV_ALLWINNER) += allwinner/ obj-$(CONFIG_CRYPTO_DEV_ATMEL_AES) += atmel-aes.o obj-$(CONFIG_CRYPTO_DEV_ATMEL_SHA) += atmel-sha.o obj-$(CONFIG_CRYPTO_DEV_ATMEL_TDES) += atmel-tdes.o diff --git a/drivers/crypto/allwinner/Kconfig b/drivers/crypto/allwinner/Kconfig new file mode 100644 index 000000000000..0c8a99f7959d --- /dev/null +++ b/drivers/crypto/allwinner/Kconfig @@ -0,0 +1,6 @@ +config CRYPTO_DEV_ALLWINNER + bool "Support for Allwinner cryptographic offloader" + depends on ARCH_SUNXI || COMPILE_TEST + default y if ARCH_SUNXI + help + Say Y here to get to see options for Allwinner hardware crypto devices -- 2.21.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/9] crypto: Add allwinner subdirectory 2019-09-06 18:45 ` [PATCH 1/9] crypto: Add allwinner subdirectory Corentin Labbe @ 2019-09-07 3:54 ` Maxime Ripard 2019-09-07 17:53 ` Corentin Labbe 0 siblings, 1 reply; 32+ messages in thread From: Maxime Ripard @ 2019-09-07 3:54 UTC (permalink / raw) To: Corentin Labbe Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Fri, Sep 06, 2019 at 08:45:43PM +0200, Corentin Labbe wrote: > Since a second Allwinner crypto driver will be added, it is better to > create a dedicated subdirectory. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > MAINTAINERS | 6 ++++++ > drivers/crypto/Kconfig | 2 ++ > drivers/crypto/Makefile | 1 + > drivers/crypto/allwinner/Kconfig | 6 ++++++ I guess it would make sense to move the sun4i driver there too? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/9] crypto: Add allwinner subdirectory 2019-09-07 3:54 ` Maxime Ripard @ 2019-09-07 17:53 ` Corentin Labbe 0 siblings, 0 replies; 32+ messages in thread From: Corentin Labbe @ 2019-09-07 17:53 UTC (permalink / raw) To: Maxime Ripard Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Sat, Sep 07, 2019 at 06:54:53AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:43PM +0200, Corentin Labbe wrote: > > Since a second Allwinner crypto driver will be added, it is better to > > create a dedicated subdirectory. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > MAINTAINERS | 6 ++++++ > > drivers/crypto/Kconfig | 2 ++ > > drivers/crypto/Makefile | 1 + > > drivers/crypto/allwinner/Kconfig | 6 ++++++ > > I guess it would make sense to move the sun4i driver there too? > Yes it is planned. I will add this move patch in the next iteration. Regards ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe 2019-09-06 18:45 ` [PATCH 1/9] crypto: Add allwinner subdirectory Corentin Labbe @ 2019-09-06 18:45 ` Corentin Labbe 2019-09-07 8:19 ` Maxime Ripard 2019-09-06 18:45 ` [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for " Corentin Labbe ` (6 subsequent siblings) 8 siblings, 1 reply; 32+ messages in thread From: Corentin Labbe @ 2019-09-06 18:45 UTC (permalink / raw) To: davem, herbert, linux, mark.rutland, mripard, robh+dt, wens Cc: devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi, Corentin Labbe The Crypto Engine is an hardware cryptographic offloader present on all recent Allwinner SoCs H2+, H3, R40, A64, H5, H6 This driver supports AES cipher in CBC/ECB mode. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- drivers/crypto/allwinner/Kconfig | 26 + drivers/crypto/allwinner/Makefile | 1 + drivers/crypto/allwinner/sun8i-ce/Makefile | 2 + .../allwinner/sun8i-ce/sun8i-ce-cipher.c | 390 +++++++++++ .../crypto/allwinner/sun8i-ce/sun8i-ce-core.c | 630 ++++++++++++++++++ drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h | 256 +++++++ 6 files changed, 1305 insertions(+) create mode 100644 drivers/crypto/allwinner/Makefile create mode 100644 drivers/crypto/allwinner/sun8i-ce/Makefile create mode 100644 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c create mode 100644 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c create mode 100644 drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h diff --git a/drivers/crypto/allwinner/Kconfig b/drivers/crypto/allwinner/Kconfig index 0c8a99f7959d..742a08b64c90 100644 --- a/drivers/crypto/allwinner/Kconfig +++ b/drivers/crypto/allwinner/Kconfig @@ -4,3 +4,29 @@ config CRYPTO_DEV_ALLWINNER default y if ARCH_SUNXI help Say Y here to get to see options for Allwinner hardware crypto devices + +config CRYPTO_DEV_SUN8I_CE + tristate "Support for Allwinner Crypto Engine cryptographic offloader" + select CRYPTO_BLKCIPHER + select CRYPTO_ENGINE + select CRYPTO_ECB + select CRYPTO_CBC + select CRYPTO_AES + select CRYPTO_DES + depends on CRYPTO_DEV_ALLWINNER + help + Select y here for having support for the crypto Engine availlable on + Allwinner SoC H2+, H3, H5, H6, R40 and A64. + The Crypto Engine handle AES/3DES ciphers in ECB/CBC mode. + + To compile this driver as a module, choose M here: the module + will be called sun8i-ce. + +config CRYPTO_DEV_SUN8I_CE_DEBUG + bool "Enabled sun8i-ce stats" + depends on CRYPTO_DEV_SUN8I_CE + depends on DEBUG_FS + help + Say y to enabled sun8i-ce debug stats. + This will create /sys/kernel/debug/sun8i-ce/stats for displaying + the number of requests per flow and per algorithm. diff --git a/drivers/crypto/allwinner/Makefile b/drivers/crypto/allwinner/Makefile new file mode 100644 index 000000000000..11f02db9ee06 --- /dev/null +++ b/drivers/crypto/allwinner/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_CRYPTO_DEV_SUN8I_CE) += sun8i-ce/ diff --git a/drivers/crypto/allwinner/sun8i-ce/Makefile b/drivers/crypto/allwinner/sun8i-ce/Makefile new file mode 100644 index 000000000000..08b68c3c1ca9 --- /dev/null +++ b/drivers/crypto/allwinner/sun8i-ce/Makefile @@ -0,0 +1,2 @@ +obj-$(CONFIG_CRYPTO_DEV_SUN8I_CE) += sun8i-ce.o +sun8i-ce-y += sun8i-ce-core.o sun8i-ce-cipher.o diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c new file mode 100644 index 000000000000..c22f0592f168 --- /dev/null +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c @@ -0,0 +1,390 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * sun8i-ce-cipher.c - hardware cryptographic offloader for + * Allwinner H3/A64/H5/H2+/H6/R40 SoC + * + * Copyright (C) 2016-2019 Corentin LABBE <clabbe.montjoie@gmail.com> + * + * This file add support for AES cipher with 128,192,256 bits keysize in + * CBC and ECB mode. + * + * You could find a link for the datasheet in Documentation/arm/sunxi/README + */ + +#include <linux/crypto.h> +#include <linux/dma-mapping.h> +#include <linux/io.h> +#include <crypto/scatterwalk.h> +#include <crypto/internal/des.h> +#include <crypto/internal/skcipher.h> +#include "sun8i-ce.h" + +static int sun8i_ce_cipher(struct skcipher_request *areq) +{ + struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq); + struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm); + struct sun8i_ce_dev *ce = op->ce; + struct sun8i_cipher_req_ctx *rctx = skcipher_request_ctx(areq); + struct skcipher_alg *alg = crypto_skcipher_alg(tfm); + struct sun8i_ce_alg_template *algt; + struct sun8i_ce_flow *chan; + struct ce_task *cet; + struct scatterlist *sg; + bool need_fallback = false; + unsigned int todo, len, offset, ivsize; + void *backup_iv = NULL; + int flow, i; + int nr_sgs = 0; + int nr_sgd = 0; + int err = 0; + + algt = container_of(alg, struct sun8i_ce_alg_template, alg.skcipher); + + dev_dbg(ce->dev, "%s %s %u %x IV(%p %u) key=%u\n", __func__, + crypto_tfm_alg_name(areq->base.tfm), + areq->cryptlen, + rctx->op_dir, areq->iv, crypto_skcipher_ivsize(tfm), + op->keylen); + + if (sg_nents(areq->src) > MAX_SG || sg_nents(areq->dst) > MAX_SG) + need_fallback = true; + + if (areq->cryptlen < crypto_skcipher_ivsize(tfm)) + need_fallback = true; + + if (areq->cryptlen == 0) + need_fallback = true; + if (areq->cryptlen % algt->alg.skcipher.base.cra_blocksize) + need_fallback = true; + + sg = areq->src; + while (sg && !need_fallback) { + if (sg->length % 4 || !IS_ALIGNED(sg->offset, sizeof(u32))) { + need_fallback = true; + break; + } + sg = sg_next(sg); + } + sg = areq->dst; + while (sg && !need_fallback) { + if (sg->length % 4 || !IS_ALIGNED(sg->offset, sizeof(u32))) { + need_fallback = true; + break; + } + sg = sg_next(sg); + } + +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG + algt->stat_req++; +#endif + + if (need_fallback) { + SYNC_SKCIPHER_REQUEST_ON_STACK(subreq, op->fallback_tfm); +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG + algt->stat_fb++; +#endif + skcipher_request_set_sync_tfm(subreq, op->fallback_tfm); + skcipher_request_set_callback(subreq, areq->base.flags, NULL, + NULL); + skcipher_request_set_crypt(subreq, areq->src, areq->dst, + areq->cryptlen, areq->iv); + if (rctx->op_dir & CE_DECRYPTION) + err = crypto_skcipher_decrypt(subreq); + else + err = crypto_skcipher_encrypt(subreq); + skcipher_request_zero(subreq); + return err; + } + + flow = rctx->flow; + + chan = &ce->chanlist[flow]; + mutex_lock(&chan->lock); + + cet = chan->tl; + memset(cet, 0, sizeof(struct ce_task)); + + cet->t_id = flow; + cet->t_common_ctl = ce->variant->alg_cipher[algt->ce_algo_id]; + cet->t_common_ctl |= rctx->op_dir | CE_COMM_INT; + cet->t_dlen = areq->cryptlen / 4; + /* CTS and recent CE (H6) need length in bytes, in word otherwise */ + if (ce->variant->model == CE_v2) + cet->t_dlen = areq->cryptlen; + + cet->t_sym_ctl = ce->variant->op_mode[algt->ce_blockmode]; + len = op->keylen; + switch (len) { + case 128 / 8: + cet->t_sym_ctl |= CE_AES_128BITS; + break; + case 192 / 8: + cet->t_sym_ctl |= CE_AES_192BITS; + break; + case 256 / 8: + cet->t_sym_ctl |= CE_AES_256BITS; + break; + } + + cet->t_asym_ctl = 0; + + chan->op_mode = ce->variant->op_mode[algt->ce_blockmode]; + chan->op_dir = rctx->op_dir; + chan->method = ce->variant->alg_cipher[algt->ce_algo_id]; + chan->keylen = op->keylen; + + cet->t_key = dma_map_single(ce->dev, op->key, op->keylen, + DMA_TO_DEVICE); + if (dma_mapping_error(ce->dev, cet->t_key)) { + dev_err(ce->dev, "Cannot DMA MAP KEY\n"); + err = -EFAULT; + goto theend; + } + + ivsize = crypto_skcipher_ivsize(tfm); + if (areq->iv && crypto_skcipher_ivsize(tfm) > 0) { + chan->ivlen = ivsize; + chan->bounce_iv = kzalloc(ivsize, GFP_KERNEL | GFP_DMA); + if (!chan->bounce_iv) { + err = -ENOMEM; + goto theend_key; + } + if (rctx->op_dir & CE_DECRYPTION) { + backup_iv = kzalloc(ivsize, GFP_KERNEL); + if (!backup_iv) { + err = -ENOMEM; + goto theend_key; + } + offset = areq->cryptlen - ivsize; + scatterwalk_map_and_copy(backup_iv, areq->src, offset, + ivsize, 0); + } + memcpy(chan->bounce_iv, areq->iv, ivsize); + } + + if (areq->src == areq->dst) { + nr_sgs = dma_map_sg(ce->dev, areq->src, sg_nents(areq->src), + DMA_BIDIRECTIONAL); + if (nr_sgs <= 0 || nr_sgs > MAX_SG) { + dev_err(ce->dev, "Invalid sg number %d\n", nr_sgs); + err = -EINVAL; + goto theend_iv; + } + nr_sgd = nr_sgs; + } else { + nr_sgs = dma_map_sg(ce->dev, areq->src, sg_nents(areq->src), + DMA_TO_DEVICE); + if (nr_sgs <= 0 || nr_sgs > MAX_SG) { + dev_err(ce->dev, "Invalid sg number %d\n", nr_sgs); + err = -EINVAL; + goto theend_iv; + } + nr_sgd = dma_map_sg(ce->dev, areq->dst, sg_nents(areq->dst), + DMA_FROM_DEVICE); + if (nr_sgd <= 0 || nr_sgd > MAX_SG) { + dev_err(ce->dev, "Invalid sg number %d\n", nr_sgd); + err = -EINVAL; + goto theend_sgs; + } + } + + len = areq->cryptlen; + for_each_sg(areq->src, sg, nr_sgs, i) { + cet->t_src[i].addr = sg_dma_address(sg); + todo = min(len, sg_dma_len(sg)); + cet->t_src[i].len = todo / 4; + dev_dbg(ce->dev, "%s total=%u SG(%d %u off=%d) todo=%u\n", __func__, + areq->cryptlen, i, cet->t_src[i].len, sg->offset, todo); + len -= todo; + } + if (len > 0) + dev_err(ce->dev, "remaining len %d\n", len); + + len = areq->cryptlen; + for_each_sg(areq->dst, sg, nr_sgd, i) { + cet->t_dst[i].addr = sg_dma_address(sg); + todo = min(len, sg_dma_len(sg)); + cet->t_dst[i].len = todo / 4; + dev_dbg(ce->dev, "%s total=%u SG(%d %u off=%d) todo=%u\n", __func__, + areq->cryptlen, i, cet->t_dst[i].len, sg->offset, todo); + len -= todo; + } + if (len > 0) + dev_err(ce->dev, "remaining len %d\n", len); + + chan->timeout = areq->cryptlen; + err = sun8i_ce_run_task(ce, flow, "cipher"); + if (err) + dev_err(ce->dev, "Error with len=%u\n", areq->cryptlen); + +theend_sgs: + if (areq->src == areq->dst) { + dma_unmap_sg(ce->dev, areq->src, nr_sgs, DMA_BIDIRECTIONAL); + } else { + if (nr_sgs > 0) + dma_unmap_sg(ce->dev, areq->src, nr_sgs, DMA_TO_DEVICE); + dma_unmap_sg(ce->dev, areq->dst, nr_sgd, DMA_FROM_DEVICE); + } + +theend_iv: + if (areq->iv && ivsize > 0) { + offset = areq->cryptlen - ivsize; + if (rctx->op_dir & CE_DECRYPTION) { + memcpy(areq->iv, backup_iv, ivsize); + kzfree(backup_iv); + } else { + scatterwalk_map_and_copy(areq->iv, areq->dst, offset, + ivsize, 0); + } + kfree(chan->bounce_iv); + } + +theend_key: + dma_unmap_single(ce->dev, cet->t_key, op->keylen, DMA_TO_DEVICE); + +theend: + mutex_unlock(&chan->lock); + + return err; +} + +static int handle_cipher_request(struct crypto_engine *engine, void *areq) +{ + int err; + struct skcipher_request *breq = container_of(areq, struct skcipher_request, base); + + err = sun8i_ce_cipher(breq); + crypto_finalize_skcipher_request(engine, breq, err); + + return 0; +} + +int sun8i_ce_skdecrypt(struct skcipher_request *areq) +{ + struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq); + struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm); + struct sun8i_cipher_req_ctx *rctx = skcipher_request_ctx(areq); + int e = sun8i_ce_get_engine_number(op->ce); + struct crypto_engine *engine = op->ce->chanlist[e].engine; + + rctx->op_dir = CE_DECRYPTION; + rctx->flow = e; + + return crypto_transfer_skcipher_request_to_engine(engine, areq); +} + +int sun8i_ce_skencrypt(struct skcipher_request *areq) +{ + struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq); + struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm); + struct sun8i_cipher_req_ctx *rctx = skcipher_request_ctx(areq); + int e = sun8i_ce_get_engine_number(op->ce); + struct crypto_engine *engine = op->ce->chanlist[e].engine; + + rctx->op_dir = CE_ENCRYPTION; + rctx->flow = e; + + return crypto_transfer_skcipher_request_to_engine(engine, areq); +} + +int sun8i_ce_cipher_init(struct crypto_tfm *tfm) +{ + struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm); + struct sun8i_ce_alg_template *algt; + const char *name = crypto_tfm_alg_name(tfm); + struct crypto_skcipher *sktfm = __crypto_skcipher_cast(tfm); + struct skcipher_alg *alg = crypto_skcipher_alg(sktfm); + + memset(op, 0, sizeof(struct sun8i_cipher_tfm_ctx)); + + algt = container_of(alg, struct sun8i_ce_alg_template, alg.skcipher); + op->ce = algt->ce; + + sktfm->reqsize = sizeof(struct sun8i_cipher_req_ctx); + + op->fallback_tfm = crypto_alloc_sync_skcipher(name, 0, CRYPTO_ALG_NEED_FALLBACK); + if (IS_ERR(op->fallback_tfm)) { + dev_err(op->ce->dev, "ERROR: Cannot allocate fallback for %s %ld\n", + name, PTR_ERR(op->fallback_tfm)); + return PTR_ERR(op->fallback_tfm); + } + + dev_info(op->ce->dev, "Fallback is %s\n", crypto_tfm_alg_driver_name(crypto_skcipher_tfm(&op->fallback_tfm->base))); + + op->enginectx.op.do_one_request = handle_cipher_request; + op->enginectx.op.prepare_request = NULL; + op->enginectx.op.unprepare_request = NULL; + + return 0; +} + +void sun8i_ce_cipher_exit(struct crypto_tfm *tfm) +{ + struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm); + + if (op->key) { + memzero_explicit(op->key, op->keylen); + kfree(op->key); + } + crypto_free_sync_skcipher(op->fallback_tfm); +} + +int sun8i_ce_aes_setkey(struct crypto_skcipher *tfm, const u8 *key, + unsigned int keylen) +{ + struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm); + struct sun8i_ce_dev *ce = op->ce; + + switch (keylen) { + case 128 / 8: + break; + case 192 / 8: + break; + case 256 / 8: + break; + default: + dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen); + crypto_skcipher_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN); + return -EINVAL; + } + if (op->key) { + memzero_explicit(op->key, op->keylen); + kfree(op->key); + } + op->keylen = keylen; + op->key = kmalloc(keylen, GFP_KERNEL | GFP_DMA); + if (!op->key) + return -ENOMEM; + memcpy(op->key, key, keylen); + + crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK); + crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags & CRYPTO_TFM_REQ_MASK); + + return crypto_sync_skcipher_setkey(op->fallback_tfm, key, keylen); +} + +int sun8i_ce_des3_setkey(struct crypto_skcipher *tfm, const u8 *key, + unsigned int keylen) +{ + struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm); + int err; + + err = verify_skcipher_des3_key(tfm, key); + if (err) + return err; + + if (op->key) { + memzero_explicit(op->key, op->keylen); + kfree(op->key); + } + op->keylen = keylen; + op->key = kmalloc(keylen, GFP_KERNEL | GFP_DMA); + if (!op->key) + return -ENOMEM; + memcpy(op->key, key, keylen); + + crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK); + crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags & CRYPTO_TFM_REQ_MASK); + + return crypto_sync_skcipher_setkey(op->fallback_tfm, key, keylen); +} diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c new file mode 100644 index 000000000000..c609b9943296 --- /dev/null +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c @@ -0,0 +1,630 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * sun8i-ce-core.c - hardware cryptographic offloader for + * Allwinner H3/A64/H5/H2+/H6/R40 SoC + * + * Copyright (C) 2015-2019 Corentin Labbe <clabbe.montjoie@gmail.com> + * + * Core file which registers crypto algorithms supported by the CryptoEngine. + * + * You could find a link for the datasheet in Documentation/arm/sunxi/README + */ +#include <linux/clk.h> +#include <linux/crypto.h> +#include <linux/delay.h> +#include <linux/dma-mapping.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <crypto/internal/skcipher.h> + +#include "sun8i-ce.h" + +static const struct ce_variant ce_h3_variant = { + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, + CE_ID_NOTSUPP, + }, + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC + }, + .intreg = CE_ISR, + .maxflow = 4, + .ce_clks = { + { "ahb", 200000000 }, + { "mod", 48000000 }, + } +}; + +static const struct ce_variant ce_h5_variant = { + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, + CE_ID_NOTSUPP, + }, + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC + }, + .intreg = CE_ISR, + .maxflow = 4, + .ce_clks = { + { "ahb", 200000000 }, + { "mod", 300000000 }, + } +}; + +static const struct ce_variant ce_h6_variant = { + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, + CE_ALG_RAES, + }, + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC + }, + .model = CE_v2, + .intreg = CE_ISR, + .maxflow = 4, + .ce_clks = { + { "ahb", 200000000 }, + { "mod", 300000000 }, + { "mbus", 400000000 }, + } +}; + +static const struct ce_variant ce_a64_variant = { + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, + CE_ID_NOTSUPP, + }, + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC + }, + .intreg = CE_ISR, + .maxflow = 4, + .ce_clks = { + { "ahb", 200000000 }, + { "mod", 300000000 }, + } +}; + +static const struct ce_variant ce_r40_variant = { + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, + CE_ID_NOTSUPP, + }, + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC + }, + .intreg = CE_ISR, + .maxflow = 4, + .ce_clks = { + { "ahb", 200000000 }, + { "mod", 300000000 }, + } +}; + +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) +{ + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; +} + +int sun8i_ce_run_task(struct sun8i_ce_dev *ce, int flow, const char *name) +{ + u32 v; + int err = 0; + struct ce_task *cet = ce->chanlist[flow].tl; + + if (ce->chanlist[flow].bounce_iv) { + cet->t_iv = dma_map_single(ce->dev, + ce->chanlist[flow].bounce_iv, + ce->chanlist[flow].ivlen, + DMA_TO_DEVICE); + if (dma_mapping_error(ce->dev, cet->t_iv)) { + dev_err(ce->dev, "Cannot DMA MAP IV\n"); + return -EFAULT; + } + } + +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG + ce->chanlist[flow].stat_req++; +#endif + + mutex_lock(&ce->mlock); + + v = readl(ce->base + CE_ICR); + v |= 1 << flow; + writel(v, ce->base + CE_ICR); + + reinit_completion(&ce->chanlist[flow].complete); + writel(ce->chanlist[flow].t_phy, ce->base + CE_TDQ); + + ce->chanlist[flow].status = 0; + /* Be sure all data is written before enabling the task */ + wmb(); + + v = 1 | (ce->chanlist[flow].tl->t_common_ctl & 0x7F) << 8; + writel(v, ce->base + CE_TLR); + mutex_unlock(&ce->mlock); + + wait_for_completion_interruptible_timeout(&ce->chanlist[flow].complete, + msecs_to_jiffies(ce->chanlist[flow].timeout)); + + if (ce->chanlist[flow].status == 0) { + dev_err(ce->dev, "DMA timeout for %s\n", name); + err = -EFAULT; + } + /* No need to lock for this read, the channel is locked so + * nothing could modify the error value for this channel + */ + v = readl(ce->base + CE_ESR); + if (v) { + v >>= (flow * 4); + v &= 0xFF; + if (v) { + dev_err(ce->dev, "CE ERROR: %x for flow %x\n", v, flow); + err = -EFAULT; + } + if (v & CE_ERR_ALGO_NOTSUP) + dev_err(ce->dev, "CE ERROR: algorithm not supported\n"); + if (v & CE_ERR_DATALEN) + dev_err(ce->dev, "CE ERROR: data length error\n"); + if (v & CE_ERR_KEYSRAM) + dev_err(ce->dev, "CE ERROR: keysram access error for AES\n"); + if (v & CE_ERR_ADDR_INVALID) + dev_err(ce->dev, "CE ERROR: address invalid\n"); + } + + if (ce->chanlist[flow].bounce_iv) { + dma_unmap_single(ce->dev, cet->t_iv, + ce->chanlist[flow].ivlen, + DMA_TO_DEVICE); + } + + return err; +} + +static irqreturn_t ce_irq_handler(int irq, void *data) +{ + struct sun8i_ce_dev *ce = (struct sun8i_ce_dev *)data; + int flow = 0; + u32 p; + + p = readl(ce->base + ce->variant->intreg); + for (flow = 0; flow < ce->variant->maxflow; flow++) { + if (p & (BIT(flow))) { + writel(BIT(flow), ce->base + ce->variant->intreg); + ce->chanlist[flow].status = 1; + complete(&ce->chanlist[flow].complete); + } + } + + return IRQ_HANDLED; +} + +static struct sun8i_ce_alg_template ce_algs[] = { +{ + .type = CRYPTO_ALG_TYPE_SKCIPHER, + .ce_algo_id = CE_ID_CIPHER_AES, + .ce_blockmode = CE_ID_OP_CBC, + .alg.skcipher = { + .base = { + .cra_name = "cbc(aes)", + .cra_driver_name = "cbc-aes-sun8i-ce", + .cra_priority = 400, + .cra_blocksize = AES_BLOCK_SIZE, + .cra_flags = CRYPTO_ALG_TYPE_SKCIPHER | + CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK, + .cra_ctxsize = sizeof(struct sun8i_cipher_tfm_ctx), + .cra_module = THIS_MODULE, + .cra_alignmask = 0xf, + .cra_init = sun8i_ce_cipher_init, + .cra_exit = sun8i_ce_cipher_exit, + }, + .min_keysize = AES_MIN_KEY_SIZE, + .max_keysize = AES_MAX_KEY_SIZE, + .ivsize = AES_BLOCK_SIZE, + .setkey = sun8i_ce_aes_setkey, + .encrypt = sun8i_ce_skencrypt, + .decrypt = sun8i_ce_skdecrypt, + } +}, +{ + .type = CRYPTO_ALG_TYPE_SKCIPHER, + .ce_algo_id = CE_ID_CIPHER_AES, + .ce_blockmode = CE_ID_OP_ECB, + .alg.skcipher = { + .base = { + .cra_name = "ecb(aes)", + .cra_driver_name = "ecb-aes-sun8i-ce", + .cra_priority = 400, + .cra_blocksize = AES_BLOCK_SIZE, + .cra_flags = CRYPTO_ALG_TYPE_SKCIPHER | + CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK, + .cra_ctxsize = sizeof(struct sun8i_cipher_tfm_ctx), + .cra_module = THIS_MODULE, + .cra_alignmask = 0xf, + .cra_init = sun8i_ce_cipher_init, + .cra_exit = sun8i_ce_cipher_exit, + }, + .min_keysize = AES_MIN_KEY_SIZE, + .max_keysize = AES_MAX_KEY_SIZE, + .setkey = sun8i_ce_aes_setkey, + .encrypt = sun8i_ce_skencrypt, + .decrypt = sun8i_ce_skdecrypt, + } +}, +{ + .type = CRYPTO_ALG_TYPE_SKCIPHER, + .ce_algo_id = CE_ID_CIPHER_DES3, + .ce_blockmode = CE_ID_OP_CBC, + .alg.skcipher = { + .base = { + .cra_name = "cbc(des3_ede)", + .cra_driver_name = "cbc-des3-sun8i-ce", + .cra_priority = 400, + .cra_blocksize = DES3_EDE_BLOCK_SIZE, + .cra_flags = CRYPTO_ALG_TYPE_SKCIPHER | + CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK, + .cra_ctxsize = sizeof(struct sun8i_cipher_tfm_ctx), + .cra_module = THIS_MODULE, + .cra_alignmask = 0xf, + .cra_init = sun8i_ce_cipher_init, + .cra_exit = sun8i_ce_cipher_exit, + }, + .min_keysize = DES3_EDE_KEY_SIZE, + .max_keysize = DES3_EDE_KEY_SIZE, + .ivsize = DES3_EDE_BLOCK_SIZE, + .setkey = sun8i_ce_des3_setkey, + .encrypt = sun8i_ce_skencrypt, + .decrypt = sun8i_ce_skdecrypt, + } +}, +{ + .type = CRYPTO_ALG_TYPE_SKCIPHER, + .ce_algo_id = CE_ID_CIPHER_DES3, + .ce_blockmode = CE_ID_OP_ECB, + .alg.skcipher = { + .base = { + .cra_name = "ecb(des3_ede)", + .cra_driver_name = "ecb-des3-sun8i-ce", + .cra_priority = 400, + .cra_blocksize = DES3_EDE_BLOCK_SIZE, + .cra_flags = CRYPTO_ALG_TYPE_SKCIPHER | + CRYPTO_ALG_ASYNC | CRYPTO_ALG_NEED_FALLBACK, + .cra_ctxsize = sizeof(struct sun8i_cipher_tfm_ctx), + .cra_module = THIS_MODULE, + .cra_alignmask = 0xf, + .cra_init = sun8i_ce_cipher_init, + .cra_exit = sun8i_ce_cipher_exit, + }, + .min_keysize = DES3_EDE_KEY_SIZE, + .max_keysize = DES3_EDE_KEY_SIZE, + .setkey = sun8i_ce_des3_setkey, + .encrypt = sun8i_ce_skencrypt, + .decrypt = sun8i_ce_skdecrypt, + } +}, +}; + +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG +static int sun8i_ce_dbgfs_read(struct seq_file *seq, void *v) +{ + struct sun8i_ce_dev *ce = seq->private; + int i; + + for (i = 0; i < ce->variant->maxflow; i++) + seq_printf(seq, "Channel %d: req %lu\n", i, ce->chanlist[i].stat_req); + + for (i = 0; i < ARRAY_SIZE(ce_algs); i++) { + if (!ce_algs[i].ce) + continue; + switch (ce_algs[i].type) { + case CRYPTO_ALG_TYPE_SKCIPHER: + seq_printf(seq, "%s %s %lu %lu\n", + ce_algs[i].alg.skcipher.base.cra_driver_name, + ce_algs[i].alg.skcipher.base.cra_name, + ce_algs[i].stat_req, ce_algs[i].stat_fb); + break; + } + } + return 0; +} + +static int sun8i_ce_dbgfs_open(struct inode *inode, struct file *file) +{ + return single_open(file, sun8i_ce_dbgfs_read, inode->i_private); +} + +static const struct file_operations sun8i_ce_debugfs_fops = { + .owner = THIS_MODULE, + .open = sun8i_ce_dbgfs_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; +#endif + +static int sun8i_ce_probe(struct platform_device *pdev) +{ + struct resource *res; + u32 v; + int err, i, ce_method, id, irq; + unsigned long cr; + struct sun8i_ce_dev *ce; + + if (!pdev->dev.of_node) + return -ENODEV; + + ce = devm_kzalloc(&pdev->dev, sizeof(*ce), GFP_KERNEL); + if (!ce) + return -ENOMEM; + + ce->variant = of_device_get_match_data(&pdev->dev); + if (!ce->variant) { + dev_err(&pdev->dev, "Missing Crypto Engine variant\n"); + return -EINVAL; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ce->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(ce->base)) { + err = PTR_ERR(ce->base); + dev_err(&pdev->dev, "Cannot request MMIO err=%d\n", err); + return err; + } + + for (i = 0; i < CE_MAX_CLOCKS; i++) { + if (!ce->variant->ce_clks[i].name) + continue; + dev_info(&pdev->dev, "Get %s clock\n", ce->variant->ce_clks[i].name); + ce->ceclks[i] = devm_clk_get(&pdev->dev, ce->variant->ce_clks[i].name); + if (IS_ERR(ce->ceclks[i])) { + err = PTR_ERR(ce->ceclks[i]); + dev_err(&pdev->dev, "Cannot get %s CE clock err=%d\n", + ce->variant->ce_clks[i].name, err); + } + cr = clk_get_rate(ce->ceclks[i]); + if (ce->variant->ce_clks[i].freq) { + dev_info(&pdev->dev, "Set %s clock to %lu (%lu Mhz) from %lu (%lu Mhz)\n", + ce->variant->ce_clks[i].name, + ce->variant->ce_clks[i].freq, + ce->variant->ce_clks[i].freq / 1000000, + cr, + cr / 1000000); + err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq); + if (err) + dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n", + ce->variant->ce_clks[i].name, + ce->variant->ce_clks[i].freq); + } else { + dev_info(&pdev->dev, "%s run at %lu\n", + ce->variant->ce_clks[i].name, cr); + } + err = clk_prepare_enable(ce->ceclks[i]); + if (err) { + dev_err(&pdev->dev, "Cannot prepare_enable %s\n", + ce->variant->ce_clks[i].name); + return err; + } + } + + /* Get Non Secure IRQ */ + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(ce->dev, "Cannot get NS IRQ\n"); + return irq; + } + + err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, + "sun8i-ce-ns", ce); + if (err < 0) { + dev_err(ce->dev, "Cannot request NS IRQ\n"); + return err; + } + + ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb"); + if (IS_ERR(ce->reset)) { + if (PTR_ERR(ce->reset) == -EPROBE_DEFER) + return PTR_ERR(ce->reset); + dev_info(&pdev->dev, "No reset control found\n"); + ce->reset = NULL; + } + + err = reset_control_deassert(ce->reset); + if (err) { + dev_err(&pdev->dev, "Cannot deassert reset control\n"); + goto error_clk; + } + + v = readl(ce->base + CE_CTR); + v >>= 16; + v &= 0x07; + dev_info(&pdev->dev, "CE_NS Die ID %x\n", v); + + ce->dev = &pdev->dev; + platform_set_drvdata(pdev, ce); + + mutex_init(&ce->mlock); + + ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow, + sizeof(struct sun8i_ce_flow), GFP_KERNEL); + if (!ce->chanlist) { + err = -ENOMEM; + goto error_flow; + } + + for (i = 0; i < ce->variant->maxflow; i++) { + init_completion(&ce->chanlist[i].complete); + mutex_init(&ce->chanlist[i].lock); + + ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true); + if (!ce->chanlist[i].engine) { + dev_err(ce->dev, "Cannot allocate engine\n"); + i--; + goto error_engine; + } + err = crypto_engine_start(ce->chanlist[i].engine); + if (err) { + dev_err(ce->dev, "Cannot start engine\n"); + goto error_engine; + } + ce->chanlist[i].tl = dma_alloc_coherent(ce->dev, + sizeof(struct ce_task), + &ce->chanlist[i].t_phy, + GFP_KERNEL); + if (!ce->chanlist[i].tl) { + dev_err(ce->dev, "Cannot get DMA memory for task %d\n", + i); + err = -ENOMEM; + goto error_engine; + } + } + +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG + ce->dbgfs_dir = debugfs_create_dir("sun8i-ce", NULL); + if (IS_ERR_OR_NULL(ce->dbgfs_dir)) { + dev_err(ce->dev, "Fail to create debugfs dir"); + err = -ENOMEM; + goto error_engine; + } + ce->dbgfs_stats = debugfs_create_file("stats", 0444, + ce->dbgfs_dir, ce, + &sun8i_ce_debugfs_fops); + if (IS_ERR_OR_NULL(ce->dbgfs_stats)) { + dev_err(ce->dev, "Fail to create debugfs stat"); + err = -ENOMEM; + goto error_debugfs; + } +#endif + for (i = 0; i < ARRAY_SIZE(ce_algs); i++) { + ce_algs[i].ce = ce; + switch (ce_algs[i].type) { + case CRYPTO_ALG_TYPE_SKCIPHER: + id = ce_algs[i].ce_algo_id; + ce_method = ce->variant->alg_cipher[id]; + if (ce_method == CE_ID_NOTSUPP) { + dev_info(ce->dev, + "DEBUG: Algo of %s not supported\n", + ce_algs[i].alg.skcipher.base.cra_name); + ce_algs[i].ce = NULL; + break; + } + id = ce_algs[i].ce_blockmode; + ce_method = ce->variant->op_mode[id]; + if (ce_method == CE_ID_NOTSUPP) { + dev_info(ce->dev, "DEBUG: Blockmode of %s not supported\n", + ce_algs[i].alg.skcipher.base.cra_name); + ce_algs[i].ce = NULL; + break; + } + dev_info(ce->dev, "DEBUG: Register %s\n", + ce_algs[i].alg.skcipher.base.cra_name); + err = crypto_register_skcipher(&ce_algs[i].alg.skcipher); + if (err) { + dev_err(ce->dev, "Fail to register %s\n", + ce_algs[i].alg.skcipher.base.cra_name); + ce_algs[i].ce = NULL; + goto error_alg; + } + break; + default: + dev_err(ce->dev, "ERROR: tryed to register an unknown algo\n"); + } + } + + return 0; +error_alg: + i--; + for (; i >= 0; i--) { + switch (ce_algs[i].type) { + case CRYPTO_ALG_TYPE_SKCIPHER: + if (ce_algs[i].ce) + crypto_unregister_skcipher(&ce_algs[i].alg.skcipher); + break; + } + } +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG +error_debugfs: + debugfs_remove_recursive(ce->dbgfs_dir); +#endif + i = ce->variant->maxflow; +error_engine: + while (i >= 0) { + crypto_engine_exit(ce->chanlist[i].engine); + if (ce->chanlist[i].tl) + dma_free_coherent(ce->dev, sizeof(struct ce_task), + ce->chanlist[i].tl, + ce->chanlist[i].t_phy); + i--; + } +error_flow: + reset_control_assert(ce->reset); +error_clk: + for (i = 0; i < CE_MAX_CLOCKS; i++) + clk_disable_unprepare(ce->ceclks[i]); + return err; +} + +static int sun8i_ce_remove(struct platform_device *pdev) +{ + int i, timeout; + struct sun8i_ce_dev *ce = platform_get_drvdata(pdev); + + for (i = 0; i < ARRAY_SIZE(ce_algs); i++) { + switch (ce_algs[i].type) { + case CRYPTO_ALG_TYPE_SKCIPHER: + if (ce_algs[i].ce) { + dev_dbg(ce->dev, "Unregister %d %s\n", i, + ce_algs[i].alg.skcipher.base.cra_name); + crypto_unregister_skcipher(&ce_algs[i].alg.skcipher); + } + break; + } + } + +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG + debugfs_remove_recursive(ce->dbgfs_dir); +#endif + + for (i = 0; i < ce->variant->maxflow; i++) { + crypto_engine_exit(ce->chanlist[i].engine); + timeout = 0; + while (mutex_is_locked(&ce->chanlist[i].lock) && timeout < 10) { + dev_info(ce->dev, "Wait for %d %d\n", i, timeout); + timeout++; + msleep(20); + } + dma_free_coherent(ce->dev, sizeof(struct ce_task), + ce->chanlist[i].tl, + ce->chanlist[i].t_phy); + } + + reset_control_assert(ce->reset); + for (i = 0; i < CE_MAX_CLOCKS; i++) + clk_disable_unprepare(ce->ceclks[i]); + return 0; +} + +static const struct of_device_id sun8i_ce_crypto_of_match_table[] = { + { .compatible = "allwinner,sun8i-h3-crypto", + .data = &ce_h3_variant }, + { .compatible = "allwinner,sun50i-h5-crypto", + .data = &ce_h5_variant }, + { .compatible = "allwinner,sun50i-h6-crypto", + .data = &ce_h6_variant }, + { .compatible = "allwinner,sun50i-a64-crypto", + .data = &ce_a64_variant }, + { .compatible = "allwinner,sun8i-r40-crypto", + .data = &ce_r40_variant }, + {} +}; +MODULE_DEVICE_TABLE(of, sun8i_ce_crypto_of_match_table); + +static struct platform_driver sun8i_ce_driver = { + .probe = sun8i_ce_probe, + .remove = sun8i_ce_remove, + .driver = { + .name = "sun8i-ce", + .of_match_table = sun8i_ce_crypto_of_match_table, + }, +}; + +module_platform_driver(sun8i_ce_driver); + +MODULE_DESCRIPTION("Allwinner Crypto Engine cryptographic offloader"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Corentin Labbe <clabbe.montjoie@gmail.com>"); diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h new file mode 100644 index 000000000000..fe51f9569fa0 --- /dev/null +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h @@ -0,0 +1,256 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * sun8i-ce.h - hardware cryptographic offloader for + * Allwinner H3/A64/H5/H2+/H6 SoC + * + * Copyright (C) 2016-2019 Corentin LABBE <clabbe.montjoie@gmail.com> + */ +#include <crypto/aes.h> +#include <crypto/des.h> +#include <crypto/engine.h> +#include <crypto/skcipher.h> +#include <linux/atomic.h> +#include <linux/debugfs.h> +#include <linux/crypto.h> + +#define MAX_SG 8 + +#define CE_STD 0 +#define CE_v2 1 + +#define CE_MAX_CLOCKS 3 + +/* CE Registers */ +#define CE_TDQ 0x00 +#define CE_CTR 0x04 +#define CE_ICR 0x08 +#define CE_ISR 0x0C +#define CE_TLR 0x10 +#define CE_TSR 0x14 +#define CE_ESR 0x18 +#define CE_CSSGR 0x1C +#define CE_CDSGR 0x20 +#define CE_CSAR 0x24 +#define CE_CDAR 0x28 +#define CE_TPR 0x2C + +/* Used in struct ce_task */ +/* ce_task common */ +#define CE_ENCRYPTION 0 +#define CE_DECRYPTION BIT(8) + +#define CE_COMM_INT BIT(31) + +/* ce_task symmetric */ +#define CE_AES_128BITS 0 +#define CE_AES_192BITS 1 +#define CE_AES_256BITS 2 + +#define CE_OP_ECB 0 +#define CE_OP_CBC (1 << 8) + +#define CE_ALG_AES 0 +#define CE_ALG_DES 1 +#define CE_ALG_3DES 2 +#define CE_ALG_RAES 48 + +/* Used in ce_variant */ +#define CE_ID_NOTSUPP 0xFF + +#define CE_ID_CIPHER_AES 1 +#define CE_ID_CIPHER_DES 2 +#define CE_ID_CIPHER_DES3 3 +#define CE_ID_CIPHER_RAES 4 +#define CE_ID_CIPHER_MAX 5 + +#define CE_ID_OP_ECB 1 +#define CE_ID_OP_CBC 2 +#define CE_ID_OP_MAX 3 + +/* Used in CE registers */ +#define CE_ERR_ALGO_NOTSUP BIT(0) +#define CE_ERR_DATALEN BIT(1) +#define CE_ERR_KEYSRAM BIT(2) +#define CE_ERR_ADDR_INVALID BIT(5) +#define CE_ERR_KEYLADDER BIT(6) + +/* + * struct ce_clock - Describe clocks used by sun8i-ce + * @name: Name of clock needed by this variant + * @freq: Maximum frequency for each clock + */ +struct ce_clock { + const char *name; + unsigned long freq; +}; + +/* + * struct ce_variant - Describe CE capability for each variant hardware + * @alg_cipher: list of supported ciphers. for each CE_ID_ this will give the + * coresponding CE_ALG_XXX/SS_ALG_XXX value + * @op_mode: list of supported block modes + * @model: The minor variant CE_STD/CE_SS/CE_v2 + * @intreg: reg offset for Interrupt register + * @maxflow: Numbers of flow for the current engine + */ +struct ce_variant { + char alg_cipher[CE_ID_CIPHER_MAX]; + u32 op_mode[CE_ID_OP_MAX]; + int model; + u32 intreg; + unsigned int maxflow; + struct ce_clock ce_clks[CE_MAX_CLOCKS]; +}; + +struct sginfo { + u32 addr; + u32 len; +} __packed; + +/* + * struct ce_task - CE Task descriptor + * The structure of this descriptor could be found in the datasheet + */ +struct ce_task { + u32 t_id; + u32 t_common_ctl; + u32 t_sym_ctl; + u32 t_asym_ctl; + u32 t_key; + u32 t_iv; + u32 t_ctr; + u32 t_dlen; + struct sginfo t_src[MAX_SG]; + struct sginfo t_dst[MAX_SG]; + u32 next; + u32 reserved[3]; +} __packed __aligned(8); + +/* + * struct sun8i_ce_flow - Information used by each flow + * @lock: lock protecting access of sun8i_ce_flow + * @engine: ptr to the crypto_engine for this flow + * @bounce_iv: buffer which contain the IV + * @ivlen: size of bounce_iv + * @keylen: keylen for this flow operation + * @complete: completion for the current task on this flow + * @status: set to 1 by interrupt if task is done + * @method: current method for flow + * @op_dir: direction (encrypt vs decrypt) of this flow + * @op_mode: op_mode for this flow + * @t_phy: Physical address of task + * @tl: pointer to the current ce_task for this flow + * @stat_req: number of request done by this flow + */ +struct sun8i_ce_flow { + struct mutex lock; + struct crypto_engine *engine; + void *bounce_iv; + unsigned int ivlen; + unsigned int keylen; + struct completion complete; + int status; + u32 method; + u32 op_dir; + u32 op_mode; + dma_addr_t t_phy; + int timeout; + struct ce_task *tl; +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG + unsigned long stat_req; +#endif +}; + +/* + * struct sun8i_ce_dev - main container for all this driver information + * @base: base address of SS/CE + * @ceclks: clocks used by SS/CE + * @reset: pointer to reset controller + * @dev: the platform device + * @mlock: Control access to device registers + * @chanlist: array of all flow + * @flow: flow to use in next request + * @variant: pointer to variant specific data + * @dbgfs_dir: Debugfs dentry for statistic directory + * @dbgfs_stats: Debugfs dentry for statistic counters + */ +struct sun8i_ce_dev { + void __iomem *base; + struct clk *ceclks[CE_MAX_CLOCKS]; + struct reset_control *reset; + struct device *dev; + struct mutex mlock; + struct sun8i_ce_flow *chanlist; + atomic_t flow; + const struct ce_variant *variant; +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG + struct dentry *dbgfs_dir; + struct dentry *dbgfs_stats; +#endif +}; + +/* + * struct sun8i_cipher_req_ctx - context for a skcipher request + * @op_dir: direction (encrypt vs decrypt) for this request + * @flow: the flow to use for this request + */ +struct sun8i_cipher_req_ctx { + u32 op_dir; + int flow; +}; + +/* + * struct sun8i_cipher_tfm_ctx - context for a skcipher TFM + * @enginectx: crypto_engine used by this TFM + * @key: pointer to key data + * @keylen: len of the key + * @ce: pointer to the private data of driver handling this TFM + * @fallback_tfm: pointer to the fallback TFM + */ +struct sun8i_cipher_tfm_ctx { + struct crypto_engine_ctx enginectx; + u32 *key; + u32 keylen; + struct sun8i_ce_dev *ce; + struct crypto_sync_skcipher *fallback_tfm; +}; + +/* + * struct sun8i_ce_alg_template - crypto_alg template + * @type: the CRYPTO_ALG_TYPE for this template + * @ce_algo_id: the CE_ID for this template + * @ce_blockmode: the type of block operation CE_ID + * @ce: pointer to the sun8i_ce_dev structure associated with + * this template + * @alg: one of sub struct must be used + * @stat_req: number of request done on this template + * @stat_fb: total of all data len done on this template + */ +struct sun8i_ce_alg_template { + u32 type; + u32 ce_algo_id; + u32 ce_blockmode; + struct sun8i_ce_dev *ce; + union { + struct skcipher_alg skcipher; + } alg; +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG + unsigned long stat_req; + unsigned long stat_fb; +#endif +}; + +int sun8i_ce_enqueue(struct crypto_async_request *areq, u32 type); + +int sun8i_ce_aes_setkey(struct crypto_skcipher *tfm, const u8 *key, + unsigned int keylen); +int sun8i_ce_des3_setkey(struct crypto_skcipher *tfm, const u8 *key, + unsigned int keylen); +int sun8i_ce_cipher_init(struct crypto_tfm *tfm); +void sun8i_ce_cipher_exit(struct crypto_tfm *tfm); +int sun8i_ce_skdecrypt(struct skcipher_request *areq); +int sun8i_ce_skencrypt(struct skcipher_request *areq); + +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce); + +int sun8i_ce_run_task(struct sun8i_ce_dev *ce, int flow, const char *name); -- 2.21.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine 2019-09-06 18:45 ` [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine Corentin Labbe @ 2019-09-07 8:19 ` Maxime Ripard 2019-09-07 19:04 ` Corentin Labbe 0 siblings, 1 reply; 32+ messages in thread From: Maxime Ripard @ 2019-09-07 8:19 UTC (permalink / raw) To: Corentin Labbe Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi Hi, I can't really comment on the crypto side, so my review is going to be pretty boring. On Fri, Sep 06, 2019 at 08:45:44PM +0200, Corentin Labbe wrote: > +static const struct ce_variant ce_h3_variant = { > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > + CE_ID_NOTSUPP, > + }, As far as I can see, it's always the same value, so I'm not sure why it's a parameter. > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > + }, Ditto > + .intreg = CE_ISR, Ditto > + .maxflow = 4, Ditto > + .ce_clks = { > + { "ahb", 200000000 }, This is the default IIRC, and the clock driver will ignore any clock rate change on it anyway, so the clock rate is pretty much useless there. > + { "mod", 48000000 }, 48MHz seems pretty slow, especially compared to the other rates you have listed there. Is that on purpose? Also, I'm not sure what is the point of having the clocks names be parameters there as well. It's constant across all the compatibles, the only thing that isn't is the number of clocks and the module clock rate. It's what you should have in there. > + } > +}; > + > +static const struct ce_variant ce_h5_variant = { > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > + CE_ID_NOTSUPP, > + }, > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > + }, > + .intreg = CE_ISR, > + .maxflow = 4, > + .ce_clks = { > + { "ahb", 200000000 }, > + { "mod", 300000000 }, > + } > +}; > + > +static const struct ce_variant ce_h6_variant = { > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > + CE_ALG_RAES, > + }, > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > + }, > + .model = CE_v2, Can't that be derived from the version register and / or the compatible? This seems to be redundant with each. > + .intreg = CE_ISR, > + .maxflow = 4, > + .ce_clks = { > + { "ahb", 200000000 }, > + { "mod", 300000000 }, > + { "mbus", 400000000 }, That rate is going to be ignored as well. > + } > +}; > + > +static const struct ce_variant ce_a64_variant = { > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > + CE_ID_NOTSUPP, > + }, > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > + }, > + .intreg = CE_ISR, > + .maxflow = 4, > + .ce_clks = { > + { "ahb", 200000000 }, > + { "mod", 300000000 }, > + } > +}; You should order your variants by alphabetical order. > +static const struct ce_variant ce_r40_variant = { > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > + CE_ID_NOTSUPP, > + }, > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > + }, > + .intreg = CE_ISR, > + .maxflow = 4, > + .ce_clks = { > + { "ahb", 200000000 }, > + { "mod", 300000000 }, > + } > +}; > + ... > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > +{ > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > +} I'm not sure what this is supposed to be doing, but that mod there seems pretty dangerous. ... > +static int sun8i_ce_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + u32 v; > + int err, i, ce_method, id, irq; > + unsigned long cr; > + struct sun8i_ce_dev *ce; > + > + if (!pdev->dev.of_node) > + return -ENODEV; This is pretty much guaranteed already > + ce = devm_kzalloc(&pdev->dev, sizeof(*ce), GFP_KERNEL); > + if (!ce) > + return -ENOMEM; > + > + ce->variant = of_device_get_match_data(&pdev->dev); > + if (!ce->variant) { > + dev_err(&pdev->dev, "Missing Crypto Engine variant\n"); > + return -EINVAL; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + ce->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(ce->base)) { > + err = PTR_ERR(ce->base); > + dev_err(&pdev->dev, "Cannot request MMIO err=%d\n", err); > + return err; ioremap_resource already prints an error message on failure, so this is redundant. > + } > + > + for (i = 0; i < CE_MAX_CLOCKS; i++) { > + if (!ce->variant->ce_clks[i].name) > + continue; > + dev_info(&pdev->dev, "Get %s clock\n", ce->variant->ce_clks[i].name); There's no reason to print this at the info level > + ce->ceclks[i] = devm_clk_get(&pdev->dev, ce->variant->ce_clks[i].name); > + if (IS_ERR(ce->ceclks[i])) { > + err = PTR_ERR(ce->ceclks[i]); > + dev_err(&pdev->dev, "Cannot get %s CE clock err=%d\n", > + ce->variant->ce_clks[i].name, err); > + } > + cr = clk_get_rate(ce->ceclks[i]); So on error you'd call clk_get_rate on the clock still? That seems pretty fragile, you should return there, it's a hard error. > + if (ce->variant->ce_clks[i].freq) { > + dev_info(&pdev->dev, "Set %s clock to %lu (%lu Mhz) from %lu (%lu Mhz)\n", > + ce->variant->ce_clks[i].name, > + ce->variant->ce_clks[i].freq, > + ce->variant->ce_clks[i].freq / 1000000, > + cr, > + cr / 1000000); Same remark about that message than the previous one. > + err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq); > + if (err) > + dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n", > + ce->variant->ce_clks[i].name, > + ce->variant->ce_clks[i].freq); > + } else { > + dev_info(&pdev->dev, "%s run at %lu\n", > + ce->variant->ce_clks[i].name, cr); Ditto. > + } > + err = clk_prepare_enable(ce->ceclks[i]); Do you really need this right now though? > + if (err) { > + dev_err(&pdev->dev, "Cannot prepare_enable %s\n", > + ce->variant->ce_clks[i].name); > + return err; > + } > + } > + > + /* Get Non Secure IRQ */ > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(ce->dev, "Cannot get NS IRQ\n"); > + return irq; > + } > + > + err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, > + "sun8i-ce-ns", ce); > + if (err < 0) { > + dev_err(ce->dev, "Cannot request NS IRQ\n"); > + return err; > + } > + > + ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb"); > + if (IS_ERR(ce->reset)) { > + if (PTR_ERR(ce->reset) == -EPROBE_DEFER) > + return PTR_ERR(ce->reset); > + dev_info(&pdev->dev, "No reset control found\n"); It's not optional though. > + ce->reset = NULL; > + } > + > + err = reset_control_deassert(ce->reset); > + if (err) { > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > + goto error_clk; > + } Again, you don't really need this at this moment. Using runtime_pm would make more sense. > + v = readl(ce->base + CE_CTR); > + v >>= 16; > + v &= 0x07; This should be in a define > + dev_info(&pdev->dev, "CE_NS Die ID %x\n", v); And if that really makes sense to print it, the error message should be made less cryptic. > + > + ce->dev = &pdev->dev; > + platform_set_drvdata(pdev, ce); > + > + mutex_init(&ce->mlock); > + > + ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow, > + sizeof(struct sun8i_ce_flow), GFP_KERNEL); > + if (!ce->chanlist) { > + err = -ENOMEM; > + goto error_flow; > + } > + > + for (i = 0; i < ce->variant->maxflow; i++) { > + init_completion(&ce->chanlist[i].complete); > + mutex_init(&ce->chanlist[i].lock); > + > + ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true); > + if (!ce->chanlist[i].engine) { > + dev_err(ce->dev, "Cannot allocate engine\n"); > + i--; > + goto error_engine; > + } > + err = crypto_engine_start(ce->chanlist[i].engine); > + if (err) { > + dev_err(ce->dev, "Cannot start engine\n"); > + goto error_engine; > + } > + ce->chanlist[i].tl = dma_alloc_coherent(ce->dev, > + sizeof(struct ce_task), > + &ce->chanlist[i].t_phy, > + GFP_KERNEL); > + if (!ce->chanlist[i].tl) { > + dev_err(ce->dev, "Cannot get DMA memory for task %d\n", > + i); > + err = -ENOMEM; > + goto error_engine; > + } > + } All this initialization should be done before calling request_irq. You're using some of those fields in your handler. > + > +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG > + ce->dbgfs_dir = debugfs_create_dir("sun8i-ce", NULL); > + if (IS_ERR_OR_NULL(ce->dbgfs_dir)) { > + dev_err(ce->dev, "Fail to create debugfs dir"); > + err = -ENOMEM; > + goto error_engine; > + } > + ce->dbgfs_stats = debugfs_create_file("stats", 0444, > + ce->dbgfs_dir, ce, > + &sun8i_ce_debugfs_fops); > + if (IS_ERR_OR_NULL(ce->dbgfs_stats)) { > + dev_err(ce->dev, "Fail to create debugfs stat"); > + err = -ENOMEM; > + goto error_debugfs; > + } > +#endif > + for (i = 0; i < ARRAY_SIZE(ce_algs); i++) { > + ce_algs[i].ce = ce; > + switch (ce_algs[i].type) { > + case CRYPTO_ALG_TYPE_SKCIPHER: > + id = ce_algs[i].ce_algo_id; > + ce_method = ce->variant->alg_cipher[id]; > + if (ce_method == CE_ID_NOTSUPP) { > + dev_info(ce->dev, > + "DEBUG: Algo of %s not supported\n", > + ce_algs[i].alg.skcipher.base.cra_name); > + ce_algs[i].ce = NULL; > + break; > + } > + id = ce_algs[i].ce_blockmode; > + ce_method = ce->variant->op_mode[id]; > + if (ce_method == CE_ID_NOTSUPP) { > + dev_info(ce->dev, "DEBUG: Blockmode of %s not supported\n", > + ce_algs[i].alg.skcipher.base.cra_name); > + ce_algs[i].ce = NULL; > + break; > + } > + dev_info(ce->dev, "DEBUG: Register %s\n", > + ce_algs[i].alg.skcipher.base.cra_name); > + err = crypto_register_skcipher(&ce_algs[i].alg.skcipher); > + if (err) { > + dev_err(ce->dev, "Fail to register %s\n", > + ce_algs[i].alg.skcipher.base.cra_name); > + ce_algs[i].ce = NULL; > + goto error_alg; > + } > + break; > + default: > + dev_err(ce->dev, "ERROR: tryed to register an unknown algo\n"); > + } > + } > + > + return 0; > +error_alg: > + i--; > + for (; i >= 0; i--) { > + switch (ce_algs[i].type) { > + case CRYPTO_ALG_TYPE_SKCIPHER: > + if (ce_algs[i].ce) > + crypto_unregister_skcipher(&ce_algs[i].alg.skcipher); > + break; > + } > + } > +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG > +error_debugfs: > + debugfs_remove_recursive(ce->dbgfs_dir); > +#endif > + i = ce->variant->maxflow; > +error_engine: > + while (i >= 0) { > + crypto_engine_exit(ce->chanlist[i].engine); > + if (ce->chanlist[i].tl) > + dma_free_coherent(ce->dev, sizeof(struct ce_task), > + ce->chanlist[i].tl, > + ce->chanlist[i].t_phy); > + i--; > + } > +error_flow: > + reset_control_assert(ce->reset); > +error_clk: > + for (i = 0; i < CE_MAX_CLOCKS; i++) > + clk_disable_unprepare(ce->ceclks[i]); > + return err; > +} So that function takes around 200-250 LoC, this is definitely too much and should be split into multiple functions. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine 2019-09-07 8:19 ` Maxime Ripard @ 2019-09-07 19:04 ` Corentin Labbe 2019-09-09 11:38 ` Maxime Ripard 0 siblings, 1 reply; 32+ messages in thread From: Corentin Labbe @ 2019-09-07 19:04 UTC (permalink / raw) To: Maxime Ripard Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Sat, Sep 07, 2019 at 11:19:51AM +0300, Maxime Ripard wrote: > Hi, > > I can't really comment on the crypto side, so my review is going to be > pretty boring. > > On Fri, Sep 06, 2019 at 08:45:44PM +0200, Corentin Labbe wrote: > > +static const struct ce_variant ce_h3_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > As far as I can see, it's always the same value, so I'm not sure why > it's a parameter. > No it is not the same value. If by same value you mean "the list is the same accross variant", it will be different when I will add CTS/CTR/XTS. Note that .alg_cipher was already different on h6, since I forgot to remove the RAES. So it will be the same on PATChv2, but again il will be different after. > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > Ditto > > > + .intreg = CE_ISR, > > Ditto > > > + .maxflow = 4, > > Ditto > Both .intreg and .maxflow are remains of sun8i-ss support. I will remove them. > > + .ce_clks = { > > + { "ahb", 200000000 }, > > This is the default IIRC, and the clock driver will ignore any clock > rate change on it anyway, so the clock rate is pretty much useless > there. > > > + { "mod", 48000000 }, > > 48MHz seems pretty slow, especially compared to the other rates you > have listed there. Is that on purpose? On H3, the value used on others SoC bring to random fail. I will add a comment. > > Also, I'm not sure what is the point of having the clocks names be > parameters there as well. It's constant across all the compatibles, > the only thing that isn't is the number of clocks and the module clock > rate. It's what you should have in there. > Since the datasheet give some max frequency, I think I will add a max_freq and add a check to verify if the clock is in the right range > > + } > > +}; > > + > > +static const struct ce_variant ce_h5_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + } > > +}; > > + > > +static const struct ce_variant ce_h6_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ALG_RAES, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .model = CE_v2, > > Can't that be derived from the version register and / or the > compatible? This seems to be redundant with each. > I could use the compatible, but I want to avoid a string comparison on each request. > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + { "mbus", 400000000 }, > > That rate is going to be ignored as well. > > > + } > > +}; > > + > > +static const struct ce_variant ce_a64_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + } > > +}; > > You should order your variants by alphabetical order. > Will do. > > +static const struct ce_variant ce_r40_variant = { > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > + CE_ID_NOTSUPP, > > + }, > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > + }, > > + .intreg = CE_ISR, > > + .maxflow = 4, > > + .ce_clks = { > > + { "ahb", 200000000 }, > > + { "mod", 300000000 }, > > + } > > +}; > > + > > ... > > > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > > +{ > > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > > +} > > I'm not sure what this is supposed to be doing, but that mod there > seems pretty dangerous. > > ... > This mod do a round robin on each channel. I dont see why it is dangerous. > > +static int sun8i_ce_probe(struct platform_device *pdev) > > +{ > > + struct resource *res; > > + u32 v; > > + int err, i, ce_method, id, irq; > > + unsigned long cr; > > + struct sun8i_ce_dev *ce; > > + > > + if (!pdev->dev.of_node) > > + return -ENODEV; > > This is pretty much guaranteed already > Ok, removed > > + ce = devm_kzalloc(&pdev->dev, sizeof(*ce), GFP_KERNEL); > > + if (!ce) > > + return -ENOMEM; > > + > > + ce->variant = of_device_get_match_data(&pdev->dev); > > + if (!ce->variant) { > > + dev_err(&pdev->dev, "Missing Crypto Engine variant\n"); > > + return -EINVAL; > > + } > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + ce->base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(ce->base)) { > > + err = PTR_ERR(ce->base); > > + dev_err(&pdev->dev, "Cannot request MMIO err=%d\n", err); > > + return err; > > ioremap_resource already prints an error message on failure, so this > is redundant. > Will remove. > > + } > > + > > + for (i = 0; i < CE_MAX_CLOCKS; i++) { > > + if (!ce->variant->ce_clks[i].name) > > + continue; > > + dev_info(&pdev->dev, "Get %s clock\n", ce->variant->ce_clks[i].name); > > There's no reason to print this at the info level > Will remove. > > + ce->ceclks[i] = devm_clk_get(&pdev->dev, ce->variant->ce_clks[i].name); > > + if (IS_ERR(ce->ceclks[i])) { > > + err = PTR_ERR(ce->ceclks[i]); > > + dev_err(&pdev->dev, "Cannot get %s CE clock err=%d\n", > > + ce->variant->ce_clks[i].name, err); > > + } > > + cr = clk_get_rate(ce->ceclks[i]); > > So on error you'd call clk_get_rate on the clock still? That seems > pretty fragile, you should return there, it's a hard error. > I will add the missing "return err" > > + if (ce->variant->ce_clks[i].freq) { > > + dev_info(&pdev->dev, "Set %s clock to %lu (%lu Mhz) from %lu (%lu Mhz)\n", > > + ce->variant->ce_clks[i].name, > > + ce->variant->ce_clks[i].freq, > > + ce->variant->ce_clks[i].freq / 1000000, > > + cr, > > + cr / 1000000); > > Same remark about that message than the previous one. > > > + err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq); > > + if (err) > > + dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n", > > + ce->variant->ce_clks[i].name, > > + ce->variant->ce_clks[i].freq); > > + } else { > > + dev_info(&pdev->dev, "%s run at %lu\n", > > + ce->variant->ce_clks[i].name, cr); > > Ditto. > > > + } > > + err = clk_prepare_enable(ce->ceclks[i]); > > Do you really need this right now though? > Not sure to understand, why I shouldnt do it now ? Does it is related to your pm_runtime remark below ? My feeling was to submit the driver without PM and convert it after. > > + if (err) { > > + dev_err(&pdev->dev, "Cannot prepare_enable %s\n", > > + ce->variant->ce_clks[i].name); > > + return err; > > + } > > + } > > + > > + /* Get Non Secure IRQ */ > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) { > > + dev_err(ce->dev, "Cannot get NS IRQ\n"); > > + return irq; > > + } > > + > > + err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, > > + "sun8i-ce-ns", ce); > > + if (err < 0) { > > + dev_err(ce->dev, "Cannot request NS IRQ\n"); > > + return err; > > + } > > + > > + ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb"); > > + if (IS_ERR(ce->reset)) { > > + if (PTR_ERR(ce->reset) == -EPROBE_DEFER) > > + return PTR_ERR(ce->reset); > > + dev_info(&pdev->dev, "No reset control found\n"); > > It's not optional though. > I dont understand why. > > + ce->reset = NULL; > > + } > > + > > + err = reset_control_deassert(ce->reset); > > + if (err) { > > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > > + goto error_clk; > > + } > > Again, you don't really need this at this moment. Using runtime_pm > would make more sense. > > > + v = readl(ce->base + CE_CTR); > > + v >>= 16; > > + v &= 0x07; > > This should be in a define > Will fix. > > + dev_info(&pdev->dev, "CE_NS Die ID %x\n", v); > > And if that really makes sense to print it, the error message should > be made less cryptic. > Will fix. > > + > > + ce->dev = &pdev->dev; > > + platform_set_drvdata(pdev, ce); > > + > > + mutex_init(&ce->mlock); > > + > > + ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow, > > + sizeof(struct sun8i_ce_flow), GFP_KERNEL); > > + if (!ce->chanlist) { > > + err = -ENOMEM; > > + goto error_flow; > > + } > > + > > + for (i = 0; i < ce->variant->maxflow; i++) { > > + init_completion(&ce->chanlist[i].complete); > > + mutex_init(&ce->chanlist[i].lock); > > + > > + ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true); > > + if (!ce->chanlist[i].engine) { > > + dev_err(ce->dev, "Cannot allocate engine\n"); > > + i--; > > + goto error_engine; > > + } > > + err = crypto_engine_start(ce->chanlist[i].engine); > > + if (err) { > > + dev_err(ce->dev, "Cannot start engine\n"); > > + goto error_engine; > > + } > > + ce->chanlist[i].tl = dma_alloc_coherent(ce->dev, > > + sizeof(struct ce_task), > > + &ce->chanlist[i].t_phy, > > + GFP_KERNEL); > > + if (!ce->chanlist[i].tl) { > > + dev_err(ce->dev, "Cannot get DMA memory for task %d\n", > > + i); > > + err = -ENOMEM; > > + goto error_engine; > > + } > > + } > > All this initialization should be done before calling > request_irq. You're using some of those fields in your handler. > No interrupt could fire, since algorithms are still not registred. > > + > > +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG > > + ce->dbgfs_dir = debugfs_create_dir("sun8i-ce", NULL); > > + if (IS_ERR_OR_NULL(ce->dbgfs_dir)) { > > + dev_err(ce->dev, "Fail to create debugfs dir"); > > + err = -ENOMEM; > > + goto error_engine; > > + } > > + ce->dbgfs_stats = debugfs_create_file("stats", 0444, > > + ce->dbgfs_dir, ce, > > + &sun8i_ce_debugfs_fops); > > + if (IS_ERR_OR_NULL(ce->dbgfs_stats)) { > > + dev_err(ce->dev, "Fail to create debugfs stat"); > > + err = -ENOMEM; > > + goto error_debugfs; > > + } > > +#endif > > + for (i = 0; i < ARRAY_SIZE(ce_algs); i++) { > > + ce_algs[i].ce = ce; > > + switch (ce_algs[i].type) { > > + case CRYPTO_ALG_TYPE_SKCIPHER: > > + id = ce_algs[i].ce_algo_id; > > + ce_method = ce->variant->alg_cipher[id]; > > + if (ce_method == CE_ID_NOTSUPP) { > > + dev_info(ce->dev, > > + "DEBUG: Algo of %s not supported\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + ce_algs[i].ce = NULL; > > + break; > > + } > > + id = ce_algs[i].ce_blockmode; > > + ce_method = ce->variant->op_mode[id]; > > + if (ce_method == CE_ID_NOTSUPP) { > > + dev_info(ce->dev, "DEBUG: Blockmode of %s not supported\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + ce_algs[i].ce = NULL; > > + break; > > + } > > + dev_info(ce->dev, "DEBUG: Register %s\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + err = crypto_register_skcipher(&ce_algs[i].alg.skcipher); > > + if (err) { > > + dev_err(ce->dev, "Fail to register %s\n", > > + ce_algs[i].alg.skcipher.base.cra_name); > > + ce_algs[i].ce = NULL; > > + goto error_alg; > > + } > > + break; > > + default: > > + dev_err(ce->dev, "ERROR: tryed to register an unknown algo\n"); > > + } > > + } > > + > > + return 0; > > +error_alg: > > + i--; > > + for (; i >= 0; i--) { > > + switch (ce_algs[i].type) { > > + case CRYPTO_ALG_TYPE_SKCIPHER: > > + if (ce_algs[i].ce) > > + crypto_unregister_skcipher(&ce_algs[i].alg.skcipher); > > + break; > > + } > > + } > > +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG > > +error_debugfs: > > + debugfs_remove_recursive(ce->dbgfs_dir); > > +#endif > > + i = ce->variant->maxflow; > > +error_engine: > > + while (i >= 0) { > > + crypto_engine_exit(ce->chanlist[i].engine); > > + if (ce->chanlist[i].tl) > > + dma_free_coherent(ce->dev, sizeof(struct ce_task), > > + ce->chanlist[i].tl, > > + ce->chanlist[i].t_phy); > > + i--; > > + } > > +error_flow: > > + reset_control_assert(ce->reset); > > +error_clk: > > + for (i = 0; i < CE_MAX_CLOCKS; i++) > > + clk_disable_unprepare(ce->ceclks[i]); > > + return err; > > +} > > So that function takes around 200-250 LoC, this is definitely too > much and should be split into multiple functions. > Will do. Thanks for your review. Regards ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine 2019-09-07 19:04 ` Corentin Labbe @ 2019-09-09 11:38 ` Maxime Ripard 2019-09-09 13:19 ` Corentin Labbe 0 siblings, 1 reply; 32+ messages in thread From: Maxime Ripard @ 2019-09-09 11:38 UTC (permalink / raw) To: Corentin Labbe Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Sat, Sep 07, 2019 at 09:04:08PM +0200, Corentin Labbe wrote: > > Also, I'm not sure what is the point of having the clocks names be > > parameters there as well. It's constant across all the compatibles, > > the only thing that isn't is the number of clocks and the module clock > > rate. It's what you should have in there. > > Since the datasheet give some max frequency, I think I will add a > max_freq and add a check to verify if the clock is in the right > range It's a bit pointless. What are you going to do if it's not correct? What are you trying to fix / report with this? > > > + } > > > +}; > > > + > > > +static const struct ce_variant ce_h5_variant = { > > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > > + CE_ID_NOTSUPP, > > > + }, > > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > > + }, > > > + .intreg = CE_ISR, > > > + .maxflow = 4, > > > + .ce_clks = { > > > + { "ahb", 200000000 }, > > > + { "mod", 300000000 }, > > > + } > > > +}; > > > + > > > +static const struct ce_variant ce_h6_variant = { > > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > > + CE_ALG_RAES, > > > + }, > > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > > + }, > > > + .model = CE_v2, > > > > Can't that be derived from the version register and / or the > > compatible? This seems to be redundant with each. > > I could use the compatible, but I want to avoid a string comparison > on each request. Well, this is specifically what this structure is for then, right? So instead of having the model, just add the information that you want there. > > > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > > > +{ > > > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > > > +} > > > > I'm not sure what this is supposed to be doing, but that mod there > > seems pretty dangerous. > > > > ... > > This mod do a round robin on each channel. > I dont see why it is dangerous. Well, you're using the atomic API here which is most commonly used for refcounting, while you're using a mod. Plus, while the increment is atomic, the modulo isn't, so you can end up in a case where you would be preempted between the atomic_inc_return and the mod, which is dangerous. Again, I'm not sure what this function is doing (which is also a problem in itself). I guess you should just make it clearer what it does, and then we can discuss it properly. > > > + err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq); > > > + if (err) > > > + dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n", > > > + ce->variant->ce_clks[i].name, > > > + ce->variant->ce_clks[i].freq); > > > + } else { > > > + dev_info(&pdev->dev, "%s run at %lu\n", > > > + ce->variant->ce_clks[i].name, cr); > > > > Ditto. > > > > > + } > > > + err = clk_prepare_enable(ce->ceclks[i]); > > > > Do you really need this right now though? > > Not sure to understand, why I shouldnt do it now ? > Does it is related to your pm_runtime remark below ? > > My feeling was to submit the driver without PM and convert it after. runtime_pm would be pretty cheap to add though judging by what you're doing there. > > > + if (err) { > > > + dev_err(&pdev->dev, "Cannot prepare_enable %s\n", > > > + ce->variant->ce_clks[i].name); > > > + return err; > > > + } > > > + } > > > + > > > + /* Get Non Secure IRQ */ > > > + irq = platform_get_irq(pdev, 0); > > > + if (irq < 0) { > > > + dev_err(ce->dev, "Cannot get NS IRQ\n"); > > > + return irq; > > > + } > > > + > > > + err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, > > > + "sun8i-ce-ns", ce); > > > + if (err < 0) { > > > + dev_err(ce->dev, "Cannot request NS IRQ\n"); > > > + return err; > > > + } > > > + > > > + ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb"); > > > + if (IS_ERR(ce->reset)) { > > > + if (PTR_ERR(ce->reset) == -EPROBE_DEFER) > > > + return PTR_ERR(ce->reset); > > > + dev_info(&pdev->dev, "No reset control found\n"); > > > > It's not optional though. > > I dont understand why. On all the SoCs, you need that reset line to be deasserted, otherwise the IP (and therefore the driver) will be non-functional. It's not an option to run without it. > > > + ce->reset = NULL; > > > + } > > > + > > > + err = reset_control_deassert(ce->reset); > > > + if (err) { > > > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > > > + goto error_clk; > > > + } > > > > Again, you don't really need this at this moment. Using runtime_pm > > would make more sense. > > > > > + v = readl(ce->base + CE_CTR); > > > + v >>= 16; > > > + v &= 0x07; > > > > This should be in a define > > > > Will fix. > > > > + dev_info(&pdev->dev, "CE_NS Die ID %x\n", v); > > > > And if that really makes sense to print it, the error message should > > be made less cryptic. > > > > Will fix. > > > > + > > > + ce->dev = &pdev->dev; > > > + platform_set_drvdata(pdev, ce); > > > + > > > + mutex_init(&ce->mlock); > > > + > > > + ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow, > > > + sizeof(struct sun8i_ce_flow), GFP_KERNEL); > > > + if (!ce->chanlist) { > > > + err = -ENOMEM; > > > + goto error_flow; > > > + } > > > + > > > + for (i = 0; i < ce->variant->maxflow; i++) { > > > + init_completion(&ce->chanlist[i].complete); > > > + mutex_init(&ce->chanlist[i].lock); > > > + > > > + ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true); > > > + if (!ce->chanlist[i].engine) { > > > + dev_err(ce->dev, "Cannot allocate engine\n"); > > > + i--; > > > + goto error_engine; > > > + } > > > + err = crypto_engine_start(ce->chanlist[i].engine); > > > + if (err) { > > > + dev_err(ce->dev, "Cannot start engine\n"); > > > + goto error_engine; > > > + } > > > + ce->chanlist[i].tl = dma_alloc_coherent(ce->dev, > > > + sizeof(struct ce_task), > > > + &ce->chanlist[i].t_phy, > > > + GFP_KERNEL); > > > + if (!ce->chanlist[i].tl) { > > > + dev_err(ce->dev, "Cannot get DMA memory for task %d\n", > > > + i); > > > + err = -ENOMEM; > > > + goto error_engine; > > > + } > > > + } > > > > All this initialization should be done before calling > > request_irq. You're using some of those fields in your handler. > > No interrupt could fire, since algorithms are still not registred. That's not true. Spurious interrupts are a thing, the engine could have been left in a weird state by the bootloader / kexec / reboot with some pending interrupts, etc. You have registered that handler already, you should expect it to be called at any point in time. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine 2019-09-09 11:38 ` Maxime Ripard @ 2019-09-09 13:19 ` Corentin Labbe 2019-09-09 13:59 ` Maxime Ripard 0 siblings, 1 reply; 32+ messages in thread From: Corentin Labbe @ 2019-09-09 13:19 UTC (permalink / raw) To: Maxime Ripard Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Mon, Sep 09, 2019 at 01:38:37PM +0200, Maxime Ripard wrote: > On Sat, Sep 07, 2019 at 09:04:08PM +0200, Corentin Labbe wrote: > > > Also, I'm not sure what is the point of having the clocks names be > > > parameters there as well. It's constant across all the compatibles, > > > the only thing that isn't is the number of clocks and the module clock > > > rate. It's what you should have in there. > > > > Since the datasheet give some max frequency, I think I will add a > > max_freq and add a check to verify if the clock is in the right > > range > > It's a bit pointless. What are you going to do if it's not correct? > What are you trying to fix / report with this? I thinked to print a warning. If someone want to play with overclocking for example, the driver should said that probably some result could be invalid. > > > > > + } > > > > +}; > > > > + > > > > +static const struct ce_variant ce_h5_variant = { > > > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > > > + CE_ID_NOTSUPP, > > > > + }, > > > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > > > + }, > > > > + .intreg = CE_ISR, > > > > + .maxflow = 4, > > > > + .ce_clks = { > > > > + { "ahb", 200000000 }, > > > > + { "mod", 300000000 }, > > > > + } > > > > +}; > > > > + > > > > +static const struct ce_variant ce_h6_variant = { > > > > + .alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES, > > > > + CE_ALG_RAES, > > > > + }, > > > > + .op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC > > > > + }, > > > > + .model = CE_v2, > > > > > > Can't that be derived from the version register and / or the > > > compatible? This seems to be redundant with each. > > > > I could use the compatible, but I want to avoid a string comparison > > on each request. > > Well, this is specifically what this structure is for then, right? So > instead of having the model, just add the information that you want > there. > ok, I will change to a "bool all_size_in_bytes" > > > > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > > > > +{ > > > > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > > > > +} > > > > > > I'm not sure what this is supposed to be doing, but that mod there > > > seems pretty dangerous. > > > > > > ... > > > > This mod do a round robin on each channel. > > I dont see why it is dangerous. > > Well, you're using the atomic API here which is most commonly used for > refcounting, while you're using a mod. > > Plus, while the increment is atomic, the modulo isn't, so you can end > up in a case where you would be preempted between the > atomic_inc_return and the mod, which is dangerous. > > Again, I'm not sure what this function is doing (which is also a > problem in itself). I guess you should just make it clearer what it > does, and then we can discuss it properly. Each request need to be assigned to a channel. Each channel are identified by a number from 1 to 4. So this function return the channel to use, 1 then 2 then 3 then 4 then 1... Note that this is uncritical. If, due to anything, two request are assigned to the same channel, nothing will break. > > > > > + err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq); > > > > + if (err) > > > > + dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n", > > > > + ce->variant->ce_clks[i].name, > > > > + ce->variant->ce_clks[i].freq); > > > > + } else { > > > > + dev_info(&pdev->dev, "%s run at %lu\n", > > > > + ce->variant->ce_clks[i].name, cr); > > > > > > Ditto. > > > > > > > + } > > > > + err = clk_prepare_enable(ce->ceclks[i]); > > > > > > Do you really need this right now though? > > > > Not sure to understand, why I shouldnt do it now ? > > Does it is related to your pm_runtime remark below ? > > > > My feeling was to submit the driver without PM and convert it after. > > runtime_pm would be pretty cheap to add though judging by what you're > doing there. > I will try to add runtime_pm > > > > + if (err) { > > > > + dev_err(&pdev->dev, "Cannot prepare_enable %s\n", > > > > + ce->variant->ce_clks[i].name); > > > > + return err; > > > > + } > > > > + } > > > > + > > > > + /* Get Non Secure IRQ */ > > > > + irq = platform_get_irq(pdev, 0); > > > > + if (irq < 0) { > > > > + dev_err(ce->dev, "Cannot get NS IRQ\n"); > > > > + return irq; > > > > + } > > > > + > > > > + err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0, > > > > + "sun8i-ce-ns", ce); > > > > + if (err < 0) { > > > > + dev_err(ce->dev, "Cannot request NS IRQ\n"); > > > > + return err; > > > > + } > > > > + > > > > + ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb"); > > > > + if (IS_ERR(ce->reset)) { > > > > + if (PTR_ERR(ce->reset) == -EPROBE_DEFER) > > > > + return PTR_ERR(ce->reset); > > > > + dev_info(&pdev->dev, "No reset control found\n"); > > > > > > It's not optional though. > > > > I dont understand why. > > On all the SoCs, you need that reset line to be deasserted, otherwise > the IP (and therefore the driver) will be non-functional. It's not an > option to run without it. Currently all the SoC have a reset, but nothing prevent a new SoC with CE without reset. Anyway, I will made the reset mandatory for the moment. > > > > > + ce->reset = NULL; > > > > + } > > > > + > > > > + err = reset_control_deassert(ce->reset); > > > > + if (err) { > > > > + dev_err(&pdev->dev, "Cannot deassert reset control\n"); > > > > + goto error_clk; > > > > + } > > > > > > Again, you don't really need this at this moment. Using runtime_pm > > > would make more sense. > > > > > > > + v = readl(ce->base + CE_CTR); > > > > + v >>= 16; > > > > + v &= 0x07; > > > > > > This should be in a define > > > > > > > Will fix. > > > > > > + dev_info(&pdev->dev, "CE_NS Die ID %x\n", v); > > > > > > And if that really makes sense to print it, the error message should > > > be made less cryptic. > > > > > > > Will fix. > > > > > > + > > > > + ce->dev = &pdev->dev; > > > > + platform_set_drvdata(pdev, ce); > > > > + > > > > + mutex_init(&ce->mlock); > > > > + > > > > + ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow, > > > > + sizeof(struct sun8i_ce_flow), GFP_KERNEL); > > > > + if (!ce->chanlist) { > > > > + err = -ENOMEM; > > > > + goto error_flow; > > > > + } > > > > + > > > > + for (i = 0; i < ce->variant->maxflow; i++) { > > > > + init_completion(&ce->chanlist[i].complete); > > > > + mutex_init(&ce->chanlist[i].lock); > > > > + > > > > + ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true); > > > > + if (!ce->chanlist[i].engine) { > > > > + dev_err(ce->dev, "Cannot allocate engine\n"); > > > > + i--; > > > > + goto error_engine; > > > > + } > > > > + err = crypto_engine_start(ce->chanlist[i].engine); > > > > + if (err) { > > > > + dev_err(ce->dev, "Cannot start engine\n"); > > > > + goto error_engine; > > > > + } > > > > + ce->chanlist[i].tl = dma_alloc_coherent(ce->dev, > > > > + sizeof(struct ce_task), > > > > + &ce->chanlist[i].t_phy, > > > > + GFP_KERNEL); > > > > + if (!ce->chanlist[i].tl) { > > > > + dev_err(ce->dev, "Cannot get DMA memory for task %d\n", > > > > + i); > > > > + err = -ENOMEM; > > > > + goto error_engine; > > > > + } > > > > + } > > > > > > All this initialization should be done before calling > > > request_irq. You're using some of those fields in your handler. > > > > No interrupt could fire, since algorithms are still not registred. > > That's not true. Spurious interrupts are a thing, the engine could > have been left in a weird state by the bootloader / kexec / reboot > with some pending interrupts, etc. > > You have registered that handler already, you should expect it to be > called at any point in time. > Ok will fix. Thanks for your review. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine 2019-09-09 13:19 ` Corentin Labbe @ 2019-09-09 13:59 ` Maxime Ripard 0 siblings, 0 replies; 32+ messages in thread From: Maxime Ripard @ 2019-09-09 13:59 UTC (permalink / raw) To: Corentin Labbe Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Mon, Sep 09, 2019 at 03:19:06PM +0200, Corentin Labbe wrote: > On Mon, Sep 09, 2019 at 01:38:37PM +0200, Maxime Ripard wrote: > > On Sat, Sep 07, 2019 at 09:04:08PM +0200, Corentin Labbe wrote: > > > > Also, I'm not sure what is the point of having the clocks names be > > > > parameters there as well. It's constant across all the compatibles, > > > > the only thing that isn't is the number of clocks and the module clock > > > > rate. It's what you should have in there. > > > > > > Since the datasheet give some max frequency, I think I will add a > > > max_freq and add a check to verify if the clock is in the right > > > range > > > > It's a bit pointless. What are you going to do if it's not correct? > > What are you trying to fix / report with this? > > I thinked to print a warning. If someone want to play with > overclocking for example, the driver should said that probably some > result could be invalid. If someone wants to play with overclocking, the crypto engine is going to be the least of their concern. > > > > > +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce) > > > > > +{ > > > > > + return atomic_inc_return(&ce->flow) % ce->variant->maxflow; > > > > > +} > > > > > > > > I'm not sure what this is supposed to be doing, but that mod there > > > > seems pretty dangerous. > > > > > > > > ... > > > > > > This mod do a round robin on each channel. > > > I dont see why it is dangerous. > > > > Well, you're using the atomic API here which is most commonly used for > > refcounting, while you're using a mod. > > > > Plus, while the increment is atomic, the modulo isn't, so you can end > > up in a case where you would be preempted between the > > atomic_inc_return and the mod, which is dangerous. > > > > Again, I'm not sure what this function is doing (which is also a > > problem in itself). I guess you should just make it clearer what it > > does, and then we can discuss it properly. > > Each request need to be assigned to a channel. > Each channel are identified by a number from 1 to 4. > > So this function return the channel to use, 1 then 2 then 3 then 4 then 1... > > Note that this is uncritical. If, due to anything, two request are > assigned to the same channel, nothing will break. I'm not sure why you're using the atomic API then? Also, I guess a bitfield and find_first_bit (and a different function name) would be more obvious to the reader. Thanks! Maxime ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto Engine 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe 2019-09-06 18:45 ` [PATCH 1/9] crypto: Add allwinner subdirectory Corentin Labbe 2019-09-06 18:45 ` [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine Corentin Labbe @ 2019-09-06 18:45 ` Corentin Labbe 2019-09-07 4:01 ` Maxime Ripard 2019-09-06 18:45 ` [PATCH 4/9] ARM: dts: sun8i: r40: add crypto engine node Corentin Labbe ` (5 subsequent siblings) 8 siblings, 1 reply; 32+ messages in thread From: Corentin Labbe @ 2019-09-06 18:45 UTC (permalink / raw) To: davem, herbert, linux, mark.rutland, mripard, robh+dt, wens Cc: devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi, Corentin Labbe This patch adds documentation for Device-Tree bindings for the Crypto Engine cryptographic accelerator driver. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- .../bindings/crypto/allwinner,sun8i-ce.yaml | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml new file mode 100644 index 000000000000..bd8ccedd6059 --- /dev/null +++ b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml @@ -0,0 +1,84 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/crypto/allwinner,sun8i-ce.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Allwinner Crypto Engine driver + +maintainers: + - Corentin Labbe <clabbe@baylibre.com> + +properties: + compatible: + oneOf: + - const: allwinner,sun8i-h3-crypto + - const: allwinner,sun8i-r40-crypto + - const: allwinner,sun50i-a64-crypto + - const: allwinner,sun50i-h5-crypto + - const: allwinner,sun50i-h6-crypto + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + +if: + properties: + compatible: + contains: + const: allwinner,sun50i-h6-crypto +then: + clocks: + items: + - description: Bus clock + - description: Module clock + - description: MBus clock + + clock-names: + items: + - const: ahb + - const: mod + - const: mbus +else: + clocks: + items: + - description: Bus clock + - description: Module clock + + clock-names: + items: + - const: ahb + - const: mod + + resets: + maxItems: 1 + + reset-names: + const: ahb + + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/sun50i-a64-ccu.h> + #include <dt-bindings/reset/sun50i-a64-ccu.h> + + crypto: crypto@1c15000 { + compatible = "allwinner,sun8i-h3-crypto"; + reg = <0x01c15000 0x1000>; + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>; + clock-names = "ahb", "mod"; + resets = <&ccu RST_BUS_CE>; + reset-names = "ahb"; + }; + -- 2.21.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto Engine 2019-09-06 18:45 ` [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for " Corentin Labbe @ 2019-09-07 4:01 ` Maxime Ripard 2019-09-11 18:31 ` Corentin Labbe 0 siblings, 1 reply; 32+ messages in thread From: Maxime Ripard @ 2019-09-07 4:01 UTC (permalink / raw) To: Corentin Labbe Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Fri, Sep 06, 2019 at 08:45:45PM +0200, Corentin Labbe wrote: > This patch adds documentation for Device-Tree bindings for the > Crypto Engine cryptographic accelerator driver. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > .../bindings/crypto/allwinner,sun8i-ce.yaml | 84 +++++++++++++++++++ > 1 file changed, 84 insertions(+) > create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml > > diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml > new file mode 100644 > index 000000000000..bd8ccedd6059 > --- /dev/null > +++ b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml So, usually we're using the first compatible supported here as the name. > @@ -0,0 +1,84 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/crypto/allwinner,sun8i-ce.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Allwinner Crypto Engine driver > + > +maintainers: > + - Corentin Labbe <clabbe@baylibre.com> > + > +properties: > + compatible: > + oneOf: > + - const: allwinner,sun8i-h3-crypto > + - const: allwinner,sun8i-r40-crypto > + - const: allwinner,sun50i-a64-crypto > + - const: allwinner,sun50i-h5-crypto > + - const: allwinner,sun50i-h6-crypto An enum would be better here, it provides a more obvious error message. > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > +if: > + properties: > + compatible: > + contains: > + const: allwinner,sun50i-h6-crypto > +then: > + clocks: > + items: > + - description: Bus clock > + - description: Module clock > + - description: MBus clock > + > + clock-names: > + items: > + - const: ahb > + - const: mod > + - const: mbus It looks like there's a reset line on the H6 as well for that controller (register 0x68c of the CCU, "CE_BGR_REG"). > +else: > + clocks: > + items: > + - description: Bus clock > + - description: Module clock > + > + clock-names: > + items: > + - const: ahb > + - const: mod > + > + resets: > + maxItems: 1 > + > + reset-names: > + const: ahb This prevents the usage of the additionalProperties property, which you should really use. What you can do instead is moving the clocks and clock-names description under properties, with a minItems of 2 and a maxItems of 3. Then you can restrict the length of that property to either 2 or 3 depending on the case here. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto Engine 2019-09-07 4:01 ` Maxime Ripard @ 2019-09-11 18:31 ` Corentin Labbe 2019-09-12 9:37 ` Maxime Ripard 0 siblings, 1 reply; 32+ messages in thread From: Corentin Labbe @ 2019-09-11 18:31 UTC (permalink / raw) To: Maxime Ripard Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Sat, Sep 07, 2019 at 07:01:16AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:45PM +0200, Corentin Labbe wrote: > > This patch adds documentation for Device-Tree bindings for the > > Crypto Engine cryptographic accelerator driver. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > .../bindings/crypto/allwinner,sun8i-ce.yaml | 84 +++++++++++++++++++ > > 1 file changed, 84 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml > > > > diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml [...] > > +else: > > + clocks: > > + items: > > + - description: Bus clock > > + - description: Module clock > > + > > + clock-names: > > + items: > > + - const: ahb > > + - const: mod > > + > > + resets: > > + maxItems: 1 > > + > > + reset-names: > > + const: ahb > > This prevents the usage of the additionalProperties property, which > you should really use. > > What you can do instead is moving the clocks and clock-names > description under properties, with a minItems of 2 and a maxItems of > 3. Then you can restrict the length of that property to either 2 or 3 > depending on the case here. > Hello I fail to do this. I do the following (keeped only clock stuff) properties: clocks: items: - description: Bus clock - description: Module clock - description: MBus clock clock-names: items: - const: ahb - const: mod - const: mbus if: properties: compatible: items: const: allwinner,sun50i-h6-crypto then: properties: clocks: minItems: 3 maxItems: 3 clock-names: minItems: 3 maxItems: 3 else: properties: clocks: minItems: 2 maxItems: 2 clock-names: minItems: 2 maxItems: 2 With this, the dtb_check keep complain that a64 have two short clocks. Regards ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto Engine 2019-09-11 18:31 ` Corentin Labbe @ 2019-09-12 9:37 ` Maxime Ripard 2019-09-12 20:26 ` Chen-Yu Tsai 0 siblings, 1 reply; 32+ messages in thread From: Maxime Ripard @ 2019-09-12 9:37 UTC (permalink / raw) To: Corentin Labbe Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi [-- Attachment #1: Type: text/plain, Size: 2452 bytes --] Hi Corentin, On Wed, Sep 11, 2019 at 08:31:58PM +0200, Corentin Labbe wrote: > On Sat, Sep 07, 2019 at 07:01:16AM +0300, Maxime Ripard wrote: > > On Fri, Sep 06, 2019 at 08:45:45PM +0200, Corentin Labbe wrote: > > > This patch adds documentation for Device-Tree bindings for the > > > Crypto Engine cryptographic accelerator driver. > > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > --- > > > .../bindings/crypto/allwinner,sun8i-ce.yaml | 84 +++++++++++++++++++ > > > 1 file changed, 84 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml > [...] > > > +else: > > > + clocks: > > > + items: > > > + - description: Bus clock > > > + - description: Module clock > > > + > > > + clock-names: > > > + items: > > > + - const: ahb > > > + - const: mod > > > + > > > + resets: > > > + maxItems: 1 > > > + > > > + reset-names: > > > + const: ahb > > > > This prevents the usage of the additionalProperties property, which > > you should really use. > > > > What you can do instead is moving the clocks and clock-names > > description under properties, with a minItems of 2 and a maxItems of > > 3. Then you can restrict the length of that property to either 2 or 3 > > depending on the case here. > > > > Hello > > I fail to do this. > I do the following (keeped only clock stuff) > properties: > > clocks: > items: > - description: Bus clock > - description: Module clock > - description: MBus clock Add minItems: 2 and maxItems: 3 at the same level than items > > clock-names: > items: > - const: ahb > - const: mod > - const: mbus And here as well Something I missed earlier though was that we've tried to unify as much as possible the ahb / apb / axi clocks around the bus name, it would be great if you could do it. > > if: > properties: > compatible: > items: > const: allwinner,sun50i-h6-crypto > then: > properties: > clocks: > minItems: 3 > maxItems: 3 > clock-names: > minItems: 3 > maxItems: 3 You don't need to duplicate the min and maxItems here Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto Engine 2019-09-12 9:37 ` Maxime Ripard @ 2019-09-12 20:26 ` Chen-Yu Tsai 2019-09-12 20:33 ` Maxime Ripard 0 siblings, 1 reply; 32+ messages in thread From: Chen-Yu Tsai @ 2019-09-12 20:26 UTC (permalink / raw) To: Maxime Ripard Cc: Corentin Labbe, David Miller, Herbert Xu, Russell King, Mark Rutland, Rob Herring, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Thu, Sep 12, 2019 at 10:37 AM Maxime Ripard <mripard@kernel.org> wrote: > > Hi Corentin, > > On Wed, Sep 11, 2019 at 08:31:58PM +0200, Corentin Labbe wrote: > > On Sat, Sep 07, 2019 at 07:01:16AM +0300, Maxime Ripard wrote: > > > On Fri, Sep 06, 2019 at 08:45:45PM +0200, Corentin Labbe wrote: > > > > This patch adds documentation for Device-Tree bindings for the > > > > Crypto Engine cryptographic accelerator driver. > > > > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > > --- > > > > .../bindings/crypto/allwinner,sun8i-ce.yaml | 84 +++++++++++++++++++ > > > > 1 file changed, 84 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml > > [...] > > > > +else: > > > > + clocks: > > > > + items: > > > > + - description: Bus clock > > > > + - description: Module clock > > > > + > > > > + clock-names: > > > > + items: > > > > + - const: ahb > > > > + - const: mod > > > > + > > > > + resets: > > > > + maxItems: 1 > > > > + > > > > + reset-names: > > > > + const: ahb > > > > > > This prevents the usage of the additionalProperties property, which > > > you should really use. > > > > > > What you can do instead is moving the clocks and clock-names > > > description under properties, with a minItems of 2 and a maxItems of > > > 3. Then you can restrict the length of that property to either 2 or 3 > > > depending on the case here. > > > > > > > Hello > > > > I fail to do this. > > I do the following (keeped only clock stuff) > > properties: > > > > clocks: > > items: > > - description: Bus clock > > - description: Module clock > > - description: MBus clock > > Add minItems: 2 and maxItems: 3 at the same level than items > > > > > clock-names: > > items: > > - const: ahb > > - const: mod > > - const: mbus > > And here as well > > Something I missed earlier though was that we've tried to unify as > much as possible the ahb / apb / axi clocks around the bus name, it > would be great if you could do it. I think we also want to standardize "mbus" as "dram"? ChenYu > > > > if: > > properties: > > compatible: > > items: > > const: allwinner,sun50i-h6-crypto > > then: > > properties: > > clocks: > > minItems: 3 > > maxItems: 3 > > clock-names: > > minItems: 3 > > maxItems: 3 > > You don't need to duplicate the min and maxItems here > > Maxime ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto Engine 2019-09-12 20:26 ` Chen-Yu Tsai @ 2019-09-12 20:33 ` Maxime Ripard 2019-09-12 20:37 ` Chen-Yu Tsai 0 siblings, 1 reply; 32+ messages in thread From: Maxime Ripard @ 2019-09-12 20:33 UTC (permalink / raw) To: Chen-Yu Tsai Cc: Corentin Labbe, David Miller, Herbert Xu, Russell King, Mark Rutland, Rob Herring, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Thu, Sep 12, 2019 at 09:26:27PM +0100, Chen-Yu Tsai wrote: > > > > > > clock-names: > > > items: > > > - const: ahb > > > - const: mod > > > - const: mbus > > > > And here as well > > > > Something I missed earlier though was that we've tried to unify as > > much as possible the ahb / apb / axi clocks around the bus name, it > > would be great if you could do it. > > I think we also want to standardize "mbus" as "dram"? Do we? The only user so far seems to be sun9i-de, while mbus has more users. I don't really care though, both mbus and dram are pretty generic to me. What makes you prefer dram over mbus? Maxime ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto Engine 2019-09-12 20:33 ` Maxime Ripard @ 2019-09-12 20:37 ` Chen-Yu Tsai 2019-09-13 12:11 ` Maxime Ripard 0 siblings, 1 reply; 32+ messages in thread From: Chen-Yu Tsai @ 2019-09-12 20:37 UTC (permalink / raw) To: Maxime Ripard Cc: Corentin Labbe, David Miller, Herbert Xu, Russell King, Mark Rutland, Rob Herring, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Thu, Sep 12, 2019 at 9:33 PM Maxime Ripard <mripard@kernel.org> wrote: > > On Thu, Sep 12, 2019 at 09:26:27PM +0100, Chen-Yu Tsai wrote: > > > > > > > > clock-names: > > > > items: > > > > - const: ahb > > > > - const: mod > > > > - const: mbus > > > > > > And here as well > > > > > > Something I missed earlier though was that we've tried to unify as > > > much as possible the ahb / apb / axi clocks around the bus name, it > > > would be great if you could do it. > > > > I think we also want to standardize "mbus" as "dram"? > > Do we? The only user so far seems to be sun9i-de, while mbus has more > users. I don't really care though, both mbus and dram are pretty > generic to me. What makes you prefer dram over mbus? Argh... it's actually "ram" we use the most. Both "dram" and "mbus" have only one instance each. ChenYu ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for sun8i-ce Crypto Engine 2019-09-12 20:37 ` Chen-Yu Tsai @ 2019-09-13 12:11 ` Maxime Ripard 0 siblings, 0 replies; 32+ messages in thread From: Maxime Ripard @ 2019-09-13 12:11 UTC (permalink / raw) To: Chen-Yu Tsai Cc: Corentin Labbe, David Miller, Herbert Xu, Russell King, Mark Rutland, Rob Herring, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi [-- Attachment #1: Type: text/plain, Size: 1016 bytes --] On Thu, Sep 12, 2019 at 09:37:17PM +0100, Chen-Yu Tsai wrote: > On Thu, Sep 12, 2019 at 9:33 PM Maxime Ripard <mripard@kernel.org> wrote: > > On Thu, Sep 12, 2019 at 09:26:27PM +0100, Chen-Yu Tsai wrote: > > > > > > > > > > clock-names: > > > > > items: > > > > > - const: ahb > > > > > - const: mod > > > > > - const: mbus > > > > > > > > And here as well > > > > > > > > Something I missed earlier though was that we've tried to unify as > > > > much as possible the ahb / apb / axi clocks around the bus name, it > > > > would be great if you could do it. > > > > > > I think we also want to standardize "mbus" as "dram"? > > > > Do we? The only user so far seems to be sun9i-de, while mbus has more > > users. I don't really care though, both mbus and dram are pretty > > generic to me. What makes you prefer dram over mbus? > > Argh... it's actually "ram" we use the most. Both "dram" and "mbus" > have only one instance each. Let's use ram then :) Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 4/9] ARM: dts: sun8i: r40: add crypto engine node 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe ` (2 preceding siblings ...) 2019-09-06 18:45 ` [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for " Corentin Labbe @ 2019-09-06 18:45 ` Corentin Labbe 2019-09-07 4:02 ` Maxime Ripard 2019-09-06 18:45 ` [PATCH 5/9] ARM: dts: sun8i: h3: Add Crypto Engine node Corentin Labbe ` (4 subsequent siblings) 8 siblings, 1 reply; 32+ messages in thread From: Corentin Labbe @ 2019-09-06 18:45 UTC (permalink / raw) To: davem, herbert, linux, mark.rutland, mripard, robh+dt, wens Cc: devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi, Corentin Labbe The Crypto Engine is a hardware cryptographic offloader that supports many algorithms. It could be found on most Allwinner SoCs. This patch enables the Crypto Engine on the Allwinner R40 SoC Device-tree. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- arch/arm/boot/dts/sun8i-r40.dtsi | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi index bde068111b85..7eb649cea163 100644 --- a/arch/arm/boot/dts/sun8i-r40.dtsi +++ b/arch/arm/boot/dts/sun8i-r40.dtsi @@ -266,6 +266,17 @@ #phy-cells = <1>; }; + crypto: crypto-engine@1c15000 { + compatible = "allwinner,sun8i-r40-crypto"; + reg = <0x01c15000 0x1000>; + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>; + clock-names = "ahb", "mod"; + resets = <&ccu RST_BUS_CE>; + reset-names = "ahb"; + status = "okay"; + }; + ehci1: usb@1c19000 { compatible = "allwinner,sun8i-r40-ehci", "generic-ehci"; reg = <0x01c19000 0x100>; -- 2.21.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 4/9] ARM: dts: sun8i: r40: add crypto engine node 2019-09-06 18:45 ` [PATCH 4/9] ARM: dts: sun8i: r40: add crypto engine node Corentin Labbe @ 2019-09-07 4:02 ` Maxime Ripard 2019-09-07 17:54 ` Corentin Labbe 0 siblings, 1 reply; 32+ messages in thread From: Maxime Ripard @ 2019-09-07 4:02 UTC (permalink / raw) To: Corentin Labbe Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Fri, Sep 06, 2019 at 08:45:46PM +0200, Corentin Labbe wrote: > The Crypto Engine is a hardware cryptographic offloader that supports > many algorithms. > It could be found on most Allwinner SoCs. > > This patch enables the Crypto Engine on the Allwinner R40 SoC Device-tree. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > arch/arm/boot/dts/sun8i-r40.dtsi | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > index bde068111b85..7eb649cea163 100644 > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > @@ -266,6 +266,17 @@ > #phy-cells = <1>; > }; > > + crypto: crypto-engine@1c15000 { > + compatible = "allwinner,sun8i-r40-crypto"; > + reg = <0x01c15000 0x1000>; > + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>; > + clock-names = "ahb", "mod"; > + resets = <&ccu RST_BUS_CE>; > + reset-names = "ahb"; > + status = "okay"; The driver will probe if status is not declared, so if you want it always enabled you should simply remove status Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/9] ARM: dts: sun8i: r40: add crypto engine node 2019-09-07 4:02 ` Maxime Ripard @ 2019-09-07 17:54 ` Corentin Labbe 0 siblings, 0 replies; 32+ messages in thread From: Corentin Labbe @ 2019-09-07 17:54 UTC (permalink / raw) To: Maxime Ripard Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Sat, Sep 07, 2019 at 07:02:17AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:46PM +0200, Corentin Labbe wrote: > > The Crypto Engine is a hardware cryptographic offloader that supports > > many algorithms. > > It could be found on most Allwinner SoCs. > > > > This patch enables the Crypto Engine on the Allwinner R40 SoC Device-tree. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > arch/arm/boot/dts/sun8i-r40.dtsi | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > > index bde068111b85..7eb649cea163 100644 > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > @@ -266,6 +266,17 @@ > > #phy-cells = <1>; > > }; > > > > + crypto: crypto-engine@1c15000 { > > + compatible = "allwinner,sun8i-r40-crypto"; > > + reg = <0x01c15000 0x1000>; > > + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>; > > + clock-names = "ahb", "mod"; > > + resets = <&ccu RST_BUS_CE>; > > + reset-names = "ahb"; > > + status = "okay"; > > The driver will probe if status is not declared, so if you want it > always enabled you should simply remove status > Fixed, thanks. ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 5/9] ARM: dts: sun8i: h3: Add Crypto Engine node 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe ` (3 preceding siblings ...) 2019-09-06 18:45 ` [PATCH 4/9] ARM: dts: sun8i: r40: add crypto engine node Corentin Labbe @ 2019-09-06 18:45 ` Corentin Labbe 2019-09-06 18:45 ` [PATCH 6/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64 Corentin Labbe ` (3 subsequent siblings) 8 siblings, 0 replies; 32+ messages in thread From: Corentin Labbe @ 2019-09-06 18:45 UTC (permalink / raw) To: davem, herbert, linux, mark.rutland, mripard, robh+dt, wens Cc: devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi, Corentin Labbe The Crypto Engine is a hardware cryptographic accelerator that supports many algorithms. It could be found on most Allwinner SoCs. This patch enables the Crypto Engine on the Allwinner H3 SoC Device-tree. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- arch/arm/boot/dts/sun8i-h3.dtsi | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi index e37c30e811d3..873abe1b279b 100644 --- a/arch/arm/boot/dts/sun8i-h3.dtsi +++ b/arch/arm/boot/dts/sun8i-h3.dtsi @@ -153,6 +153,17 @@ allwinner,sram = <&ve_sram 1>; }; + crypto: crypto@1c15000 { + compatible = "allwinner,sun8i-h3-crypto"; + reg = <0x01c15000 0x1000>; + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "ce_ns"; + resets = <&ccu RST_BUS_CE>; + reset-names = "ahb"; + clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>; + clock-names = "ahb", "mod"; + }; + mali: gpu@1c40000 { compatible = "allwinner,sun8i-h3-mali", "arm,mali-400"; reg = <0x01c40000 0x10000>; -- 2.21.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 6/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe ` (4 preceding siblings ...) 2019-09-06 18:45 ` [PATCH 5/9] ARM: dts: sun8i: h3: Add Crypto Engine node Corentin Labbe @ 2019-09-06 18:45 ` Corentin Labbe 2019-09-07 4:02 ` Maxime Ripard 2019-09-06 18:45 ` [PATCH 7/9] ARM64: dts: allwinner: sun50i: Add crypto engine node on H5 Corentin Labbe ` (2 subsequent siblings) 8 siblings, 1 reply; 32+ messages in thread From: Corentin Labbe @ 2019-09-06 18:45 UTC (permalink / raw) To: davem, herbert, linux, mark.rutland, mripard, robh+dt, wens Cc: devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi, Corentin Labbe The Crypto Engine is a hardware cryptographic accelerator that supports many algorithms. It could be found on most Allwinner SoCs. This patch enables the Crypto Engine on the Allwinner A64 SoC Device-tree. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index 69128a6dfc46..c9e30d462ab1 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -487,6 +487,17 @@ reg = <0x1c14000 0x400>; }; + crypto: crypto@1c15000 { + compatible = "allwinner,sun50i-a64-crypto"; + reg = <0x01c15000 0x1000>; + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "ce_ns"; + resets = <&ccu RST_BUS_CE>; + reset-names = "ahb"; + clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>; + clock-names = "ahb", "mod"; + }; + usb_otg: usb@1c19000 { compatible = "allwinner,sun8i-a33-musb"; reg = <0x01c19000 0x0400>; -- 2.21.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 6/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64 2019-09-06 18:45 ` [PATCH 6/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64 Corentin Labbe @ 2019-09-07 4:02 ` Maxime Ripard 2019-09-07 17:54 ` Corentin Labbe 0 siblings, 1 reply; 32+ messages in thread From: Maxime Ripard @ 2019-09-07 4:02 UTC (permalink / raw) To: Corentin Labbe Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Fri, Sep 06, 2019 at 08:45:48PM +0200, Corentin Labbe wrote: > The Crypto Engine is a hardware cryptographic accelerator that supports > many algorithms. > It could be found on most Allwinner SoCs. > > This patch enables the Crypto Engine on the Allwinner A64 SoC Device-tree. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > index 69128a6dfc46..c9e30d462ab1 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > @@ -487,6 +487,17 @@ > reg = <0x1c14000 0x400>; > }; > > + crypto: crypto@1c15000 { > + compatible = "allwinner,sun50i-a64-crypto"; > + reg = <0x01c15000 0x1000>; > + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "ce_ns"; You didn't document that property Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 6/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64 2019-09-07 4:02 ` Maxime Ripard @ 2019-09-07 17:54 ` Corentin Labbe 0 siblings, 0 replies; 32+ messages in thread From: Corentin Labbe @ 2019-09-07 17:54 UTC (permalink / raw) To: Maxime Ripard Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Sat, Sep 07, 2019 at 07:02:54AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:48PM +0200, Corentin Labbe wrote: > > The Crypto Engine is a hardware cryptographic accelerator that supports > > many algorithms. > > It could be found on most Allwinner SoCs. > > > > This patch enables the Crypto Engine on the Allwinner A64 SoC Device-tree. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > > index 69128a6dfc46..c9e30d462ab1 100644 > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > > @@ -487,6 +487,17 @@ > > reg = <0x1c14000 0x400>; > > }; > > > > + crypto: crypto@1c15000 { > > + compatible = "allwinner,sun50i-a64-crypto"; > > + reg = <0x01c15000 0x1000>; > > + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "ce_ns"; > > You didn't document that property > It is unnecessary, I forgot to remove it. Fixed, thanks. ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 7/9] ARM64: dts: allwinner: sun50i: Add crypto engine node on H5 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe ` (5 preceding siblings ...) 2019-09-06 18:45 ` [PATCH 6/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64 Corentin Labbe @ 2019-09-06 18:45 ` Corentin Labbe 2019-09-06 18:45 ` [PATCH 8/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on H6 Corentin Labbe 2019-09-06 18:45 ` [PATCH 9/9] sunxi_defconfig: add new crypto options Corentin Labbe 8 siblings, 0 replies; 32+ messages in thread From: Corentin Labbe @ 2019-09-06 18:45 UTC (permalink / raw) To: davem, herbert, linux, mark.rutland, mripard, robh+dt, wens Cc: devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi, Corentin Labbe The Crypto Engine is a hardware cryptographic accelerator that supports many algorithms. It could be found on most Allwinner SoCs. This patch enables the Crypto Engine on the Allwinner H5 SoC Device-tree. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi index f002a496d7cb..174fb3dcb3f7 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi @@ -127,6 +127,17 @@ allwinner,sram = <&ve_sram 1>; }; + crypto: crypto@1c15000 { + compatible = "allwinner,sun50i-h5-crypto"; + reg = <0x01c15000 0x1000>; + interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "ce_ns"; + resets = <&ccu RST_BUS_CE>; + reset-names = "ahb"; + clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>; + clock-names = "ahb", "mod"; + }; + mali: gpu@1e80000 { compatible = "allwinner,sun50i-h5-mali", "arm,mali-450"; reg = <0x01e80000 0x30000>; -- 2.21.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 8/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on H6 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe ` (6 preceding siblings ...) 2019-09-06 18:45 ` [PATCH 7/9] ARM64: dts: allwinner: sun50i: Add crypto engine node on H5 Corentin Labbe @ 2019-09-06 18:45 ` Corentin Labbe 2019-09-06 18:45 ` [PATCH 9/9] sunxi_defconfig: add new crypto options Corentin Labbe 8 siblings, 0 replies; 32+ messages in thread From: Corentin Labbe @ 2019-09-06 18:45 UTC (permalink / raw) To: davem, herbert, linux, mark.rutland, mripard, robh+dt, wens Cc: devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi, Corentin Labbe The Crypto Engine is a hardware cryptographic accelerator that supports many algorithms. This patch enables the Crypto Engine on the Allwinner H6 SoC Device-tree. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi index 0754f01fd731..51762499ed06 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi @@ -149,6 +149,16 @@ allwinner,sram = <&ve_sram 1>; }; + crypto: crypto@1904000 { + compatible = "allwinner,sun50i-h6-crypto"; + reg = <0x01904000 0x1000>; + clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>, <&ccu CLK_MBUS_CE>; + clock-names = "ahb", "mod", "mbus"; + resets = <&ccu RST_BUS_CE>; + reset-names = "ahb"; + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; + }; + syscon: syscon@3000000 { compatible = "allwinner,sun50i-h6-system-control", "allwinner,sun50i-a64-system-control"; -- 2.21.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 9/9] sunxi_defconfig: add new crypto options 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe ` (7 preceding siblings ...) 2019-09-06 18:45 ` [PATCH 8/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on H6 Corentin Labbe @ 2019-09-06 18:45 ` Corentin Labbe 2019-09-07 4:03 ` Maxime Ripard 8 siblings, 1 reply; 32+ messages in thread From: Corentin Labbe @ 2019-09-06 18:45 UTC (permalink / raw) To: davem, herbert, linux, mark.rutland, mripard, robh+dt, wens Cc: devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi, Corentin Labbe This patch adds the new allwinner crypto configs to sunxi_defconfig Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- arch/arm/configs/sunxi_defconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig index df433abfcb02..d0ab8ba7710a 100644 --- a/arch/arm/configs/sunxi_defconfig +++ b/arch/arm/configs/sunxi_defconfig @@ -150,4 +150,6 @@ CONFIG_NLS_CODEPAGE_437=y CONFIG_NLS_ISO8859_1=y CONFIG_PRINTK_TIME=y CONFIG_DEBUG_FS=y +CONFIG_CRYPTO_DEV_ALLWINNER=y +CONFIG_CRYPTO_DEV_SUN8I_CE=y CONFIG_CRYPTO_DEV_SUN4I_SS=y -- 2.21.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 9/9] sunxi_defconfig: add new crypto options 2019-09-06 18:45 ` [PATCH 9/9] sunxi_defconfig: add new crypto options Corentin Labbe @ 2019-09-07 4:03 ` Maxime Ripard 2019-09-07 17:55 ` Corentin Labbe 2019-09-13 8:15 ` Corentin Labbe 0 siblings, 2 replies; 32+ messages in thread From: Maxime Ripard @ 2019-09-07 4:03 UTC (permalink / raw) To: Corentin Labbe Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Fri, Sep 06, 2019 at 08:45:51PM +0200, Corentin Labbe wrote: > This patch adds the new allwinner crypto configs to sunxi_defconfig > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > arch/arm/configs/sunxi_defconfig | 2 ++ > 1 file changed, 2 insertions(+) Can you also enable it in arm64's defconfig as a module? > > diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig > index df433abfcb02..d0ab8ba7710a 100644 > --- a/arch/arm/configs/sunxi_defconfig > +++ b/arch/arm/configs/sunxi_defconfig > @@ -150,4 +150,6 @@ CONFIG_NLS_CODEPAGE_437=y > CONFIG_NLS_ISO8859_1=y > CONFIG_PRINTK_TIME=y > CONFIG_DEBUG_FS=y > +CONFIG_CRYPTO_DEV_ALLWINNER=y > +CONFIG_CRYPTO_DEV_SUN8I_CE=y > CONFIG_CRYPTO_DEV_SUN4I_SS=y > -- > 2.21.0 > -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 9/9] sunxi_defconfig: add new crypto options 2019-09-07 4:03 ` Maxime Ripard @ 2019-09-07 17:55 ` Corentin Labbe 2019-09-13 8:15 ` Corentin Labbe 1 sibling, 0 replies; 32+ messages in thread From: Corentin Labbe @ 2019-09-07 17:55 UTC (permalink / raw) To: Maxime Ripard Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Sat, Sep 07, 2019 at 07:03:53AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:51PM +0200, Corentin Labbe wrote: > > This patch adds the new allwinner crypto configs to sunxi_defconfig > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > arch/arm/configs/sunxi_defconfig | 2 ++ > > 1 file changed, 2 insertions(+) > > Can you also enable it in arm64's defconfig as a module? > Will do. Regards ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 9/9] sunxi_defconfig: add new crypto options 2019-09-07 4:03 ` Maxime Ripard 2019-09-07 17:55 ` Corentin Labbe @ 2019-09-13 8:15 ` Corentin Labbe 2019-09-13 12:10 ` Maxime Ripard 1 sibling, 1 reply; 32+ messages in thread From: Corentin Labbe @ 2019-09-13 8:15 UTC (permalink / raw) To: Maxime Ripard Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi On Sat, Sep 07, 2019 at 07:03:53AM +0300, Maxime Ripard wrote: > On Fri, Sep 06, 2019 at 08:45:51PM +0200, Corentin Labbe wrote: > > This patch adds the new allwinner crypto configs to sunxi_defconfig > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > arch/arm/configs/sunxi_defconfig | 2 ++ > > 1 file changed, 2 insertions(+) > > Can you also enable it in arm64's defconfig as a module? > Does you prefer adding a Kconfig "DEFAULT m if ARCH_SUNXI" which permit to not touch any defconfig ? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 9/9] sunxi_defconfig: add new crypto options 2019-09-13 8:15 ` Corentin Labbe @ 2019-09-13 12:10 ` Maxime Ripard 0 siblings, 0 replies; 32+ messages in thread From: Maxime Ripard @ 2019-09-13 12:10 UTC (permalink / raw) To: Corentin Labbe Cc: davem, herbert, linux, mark.rutland, robh+dt, wens, devicetree, linux-arm-kernel, linux-crypto, linux-kernel, linux-sunxi [-- Attachment #1: Type: text/plain, Size: 688 bytes --] On Fri, Sep 13, 2019 at 10:15:55AM +0200, Corentin Labbe wrote: > On Sat, Sep 07, 2019 at 07:03:53AM +0300, Maxime Ripard wrote: > > On Fri, Sep 06, 2019 at 08:45:51PM +0200, Corentin Labbe wrote: > > > This patch adds the new allwinner crypto configs to sunxi_defconfig > > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > --- > > > arch/arm/configs/sunxi_defconfig | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > Can you also enable it in arm64's defconfig as a module? > > > > Does you prefer adding a Kconfig "DEFAULT m if ARCH_SUNXI" which > permit to not touch any defconfig ? It's not the preferred solution, unfortunately Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2019-09-13 12:11 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe 2019-09-06 18:45 ` [PATCH 1/9] crypto: Add allwinner subdirectory Corentin Labbe 2019-09-07 3:54 ` Maxime Ripard 2019-09-07 17:53 ` Corentin Labbe 2019-09-06 18:45 ` [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine Corentin Labbe 2019-09-07 8:19 ` Maxime Ripard 2019-09-07 19:04 ` Corentin Labbe 2019-09-09 11:38 ` Maxime Ripard 2019-09-09 13:19 ` Corentin Labbe 2019-09-09 13:59 ` Maxime Ripard 2019-09-06 18:45 ` [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for " Corentin Labbe 2019-09-07 4:01 ` Maxime Ripard 2019-09-11 18:31 ` Corentin Labbe 2019-09-12 9:37 ` Maxime Ripard 2019-09-12 20:26 ` Chen-Yu Tsai 2019-09-12 20:33 ` Maxime Ripard 2019-09-12 20:37 ` Chen-Yu Tsai 2019-09-13 12:11 ` Maxime Ripard 2019-09-06 18:45 ` [PATCH 4/9] ARM: dts: sun8i: r40: add crypto engine node Corentin Labbe 2019-09-07 4:02 ` Maxime Ripard 2019-09-07 17:54 ` Corentin Labbe 2019-09-06 18:45 ` [PATCH 5/9] ARM: dts: sun8i: h3: Add Crypto Engine node Corentin Labbe 2019-09-06 18:45 ` [PATCH 6/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64 Corentin Labbe 2019-09-07 4:02 ` Maxime Ripard 2019-09-07 17:54 ` Corentin Labbe 2019-09-06 18:45 ` [PATCH 7/9] ARM64: dts: allwinner: sun50i: Add crypto engine node on H5 Corentin Labbe 2019-09-06 18:45 ` [PATCH 8/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on H6 Corentin Labbe 2019-09-06 18:45 ` [PATCH 9/9] sunxi_defconfig: add new crypto options Corentin Labbe 2019-09-07 4:03 ` Maxime Ripard 2019-09-07 17:55 ` Corentin Labbe 2019-09-13 8:15 ` Corentin Labbe 2019-09-13 12:10 ` Maxime Ripard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).