[PATCH 0/4] crypto: sun8i-ce: add Allwinner H616 support

[PATCH 0/4] crypto: sun8i-ce: add Allwinner H616 support

[Andre Przywara]

This series adds support for the crypto engine in the Allwinner H616
SoC. The IP and its capabilities are very similar to the H6, with the
major difference of the DMA engine supporting 34 bit wide addresses.
This is achieved by just shifting every address by 2 bits in the DMA
descriptors; Allwinner calls this "word addresses".
Patch 2/4 adds support for this by wrapping every address access in a
function that does the shift as needed. Patch 1/4 adds the new
compatible string to the binding, patch 3/4 adds that string to the
driver and enables the address shift for it. The final patch 4/4 adds
the DT node to the SoC .dtsi. Since this is an internal peripheral,
it's always enabled.

Corentin's cryptotest passed for me, though I haven't checked how fast
it is and if it really brings an advantage performance-wise, but maybe
people find it useful to offload that from the CPU cores.
One immediate advantage is the availability of the TRNG device, which
helps to feed the kernel's entropy pool much faster - typically before
we reach userland. Without the driver this sometimes takes minutes, and
delays workloads that rely on the entropy pool.

Please have a look and comment!

Cheers,
Andre

Andre Przywara (4):
  dt-bindings: crypto: sun8i-ce: Add compatible for H616
  crypto: sun8i-ce - wrap accesses to descriptor address fields
  crypto: sun8i-ce - add Allwinner H616 support
  arm64: dts: allwinner: h616: add crypto engine node

 .../bindings/crypto/allwinner,sun8i-ce.yaml   |  2 ++
 .../arm64/boot/dts/allwinner/sun50i-h616.dtsi | 10 +++++++
 .../allwinner/sun8i-ce/sun8i-ce-cipher.c      |  8 ++---
 .../crypto/allwinner/sun8i-ce/sun8i-ce-core.c | 29 ++++++++++++++++++-
 .../crypto/allwinner/sun8i-ce/sun8i-ce-hash.c |  6 ++--
 .../crypto/allwinner/sun8i-ce/sun8i-ce-prng.c |  6 ++--
 .../crypto/allwinner/sun8i-ce/sun8i-ce-trng.c |  2 +-
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h  | 10 +++++++
 8 files changed, 61 insertions(+), 12 deletions(-)

-- 
2.39.4

[PATCH 1/4] dt-bindings: crypto: sun8i-ce: Add compatible for H616

[Andre Przywara]

The Allwinner H616 has a crypto engine very similar to the one in the
H6, although all addresses in the DMA descriptors are shifted by 2 bits,
to accommodate for the larger physical address space. That makes it
incompatible to the H6 variant, and thus requires a new compatible
string. Clock wise it relies on the internal oscillator for the TRNG,
so needs all four possible clocks specified.

Add the compatible string to the list of recognised names, and add the
H616 to list of devices requiring all four clocks.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../devicetree/bindings/crypto/allwinner,sun8i-ce.yaml          | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
index 4287678aa79f4..da47b601c165e 100644
--- a/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
+++ b/Documentation/devicetree/bindings/crypto/allwinner,sun8i-ce.yaml
@@ -18,6 +18,7 @@ properties:
       - allwinner,sun50i-a64-crypto
       - allwinner,sun50i-h5-crypto
       - allwinner,sun50i-h6-crypto
+      - allwinner,sun50i-h616-crypto
 
   reg:
     maxItems: 1
@@ -49,6 +50,7 @@ if:
     compatible:
       enum:
         - allwinner,sun20i-d1-crypto
+        - allwinner,sun50i-h616-crypto
 then:
   properties:
     clocks:
-- 
2.39.4

[PATCH 2/4] crypto: sun8i-ce - wrap accesses to descriptor address fields

[Andre Przywara]

The Allwinner H616 (and later) SoCs support more than 32 bits worth of
physical addresses. To accommodate the larger address space, the CE task
descriptor fields holding addresses are now encoded as "word addresses",
so take the actual address divided by four.
This is true for the fields within the descriptor, but also for the
descriptor base address, in the CE_TDA register.

Wrap all accesses to those fields in a function, which will do the
required division if needed. For now this in unused, so there should be
no change in behaviour.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c |  8 ++++----
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c   |  3 ++-
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c   |  6 +++---
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c   |  6 +++---
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c   |  2 +-
 drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h        | 10 ++++++++++
 6 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index de50c00ba218f..3a5674b1bd3c0 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -190,7 +190,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
 		err = -EFAULT;
 		goto theend;
 	}
-	cet->t_key = cpu_to_le32(rctx->addr_key);
+	cet->t_key = sun8i_ce_desc_addr(ce, rctx->addr_key);
 
 	ivsize = crypto_skcipher_ivsize(tfm);
 	if (areq->iv && crypto_skcipher_ivsize(tfm) > 0) {
@@ -208,7 +208,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
 			err = -ENOMEM;
 			goto theend_iv;
 		}
-		cet->t_iv = cpu_to_le32(rctx->addr_iv);
+		cet->t_iv = sun8i_ce_desc_addr(ce, rctx->addr_iv);
 	}
 
 	if (areq->src == areq->dst) {
@@ -236,7 +236,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
 
 	len = areq->cryptlen;
 	for_each_sg(areq->src, sg, nr_sgs, i) {
-		cet->t_src[i].addr = cpu_to_le32(sg_dma_address(sg));
+		cet->t_src[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
 		todo = min(len, sg_dma_len(sg));
 		cet->t_src[i].len = cpu_to_le32(todo / 4);
 		dev_dbg(ce->dev, "%s total=%u SG(%d %u off=%d) todo=%u\n", __func__,
@@ -251,7 +251,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
 
 	len = areq->cryptlen;
 	for_each_sg(areq->dst, sg, nr_sgd, i) {
-		cet->t_dst[i].addr = cpu_to_le32(sg_dma_address(sg));
+		cet->t_dst[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
 		todo = min(len, sg_dma_len(sg));
 		cet->t_dst[i].len = cpu_to_le32(todo / 4);
 		dev_dbg(ce->dev, "%s total=%u SG(%d %u off=%d) todo=%u\n", __func__,
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
index 0408b2d5d533b..89ab3e08f0697 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
@@ -172,7 +172,8 @@ int sun8i_ce_run_task(struct sun8i_ce_dev *ce, int flow, const char *name)
 	writel(v, ce->base + CE_ICR);
 
 	reinit_completion(&ce->chanlist[flow].complete);
-	writel(ce->chanlist[flow].t_phy, ce->base + CE_TDQ);
+	writel(sun8i_ce_desc_addr(ce, ce->chanlist[flow].t_phy),
+	       ce->base + CE_TDQ);
 
 	ce->chanlist[flow].status = 0;
 	/* Be sure all data is written before enabling the task */
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
index ee2a28c906ede..a710ec9aa96f1 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
@@ -403,7 +403,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
 
 	len = areq->nbytes;
 	for_each_sg(areq->src, sg, nr_sgs, i) {
-		cet->t_src[i].addr = cpu_to_le32(sg_dma_address(sg));
+		cet->t_src[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
 		todo = min(len, sg_dma_len(sg));
 		cet->t_src[i].len = cpu_to_le32(todo / 4);
 		len -= todo;
@@ -414,7 +414,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
 		goto theend;
 	}
 	addr_res = dma_map_single(ce->dev, result, digestsize, DMA_FROM_DEVICE);
-	cet->t_dst[0].addr = cpu_to_le32(addr_res);
+	cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, addr_res);
 	cet->t_dst[0].len = cpu_to_le32(digestsize / 4);
 	if (dma_mapping_error(ce->dev, addr_res)) {
 		dev_err(ce->dev, "DMA map dest\n");
@@ -445,7 +445,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
 	}
 
 	addr_pad = dma_map_single(ce->dev, buf, j * 4, DMA_TO_DEVICE);
-	cet->t_src[i].addr = cpu_to_le32(addr_pad);
+	cet->t_src[i].addr = sun8i_ce_desc_addr(ce, addr_pad);
 	cet->t_src[i].len = cpu_to_le32(j);
 	if (dma_mapping_error(ce->dev, addr_pad)) {
 		dev_err(ce->dev, "DMA error on padding SG\n");
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
index 80815379f6fc5..f030167f95945 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
@@ -132,10 +132,10 @@ int sun8i_ce_prng_generate(struct crypto_rng *tfm, const u8 *src,
 	cet->t_sym_ctl = cpu_to_le32(sym);
 	cet->t_asym_ctl = 0;
 
-	cet->t_key = cpu_to_le32(dma_iv);
-	cet->t_iv = cpu_to_le32(dma_iv);
+	cet->t_key = sun8i_ce_desc_addr(ce, dma_iv);
+	cet->t_iv = sun8i_ce_desc_addr(ce, dma_iv);
 
-	cet->t_dst[0].addr = cpu_to_le32(dma_dst);
+	cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, dma_dst);
 	cet->t_dst[0].len = cpu_to_le32(todo / 4);
 	ce->chanlist[flow].timeout = 2000;
 
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
index 9c35f2a83eda8..465c1c512eb85 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
@@ -77,7 +77,7 @@ static int sun8i_ce_trng_read(struct hwrng *rng, void *data, size_t max, bool wa
 	cet->t_sym_ctl = 0;
 	cet->t_asym_ctl = 0;
 
-	cet->t_dst[0].addr = cpu_to_le32(dma_dst);
+	cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, dma_dst);
 	cet->t_dst[0].len = cpu_to_le32(todo / 4);
 	ce->chanlist[flow].timeout = todo;
 
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
index 93d4985def87a..8fa58f3bb7f86 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
@@ -149,6 +149,7 @@ struct ce_variant {
 	bool hash_t_dlen_in_bits;
 	bool prng_t_dlen_in_bytes;
 	bool trng_t_dlen_in_bytes;
+	bool needs_word_addresses;
 	struct ce_clock ce_clks[CE_MAX_CLOCKS];
 	int esr;
 	unsigned char prng;
@@ -241,6 +242,15 @@ struct sun8i_ce_dev {
 #endif
 };
 
+static inline __le32 sun8i_ce_desc_addr(struct sun8i_ce_dev *dev,
+					dma_addr_t addr)
+{
+	if (dev->variant->needs_word_addresses)
+		return cpu_to_le32(addr / 4);
+
+	return cpu_to_le32(addr);
+}
+
 /*
  * struct sun8i_cipher_req_ctx - context for a skcipher request
  * @op_dir:		direction (encrypt vs decrypt) for this request
-- 
2.39.4

[PATCH 3/4] crypto: sun8i-ce - add Allwinner H616 support

[Andre Przywara]

The crypto engine in the Allwinner H616 is very similar to the H6, but
needs the base address for the task descriptor and the addresses within
it to be expressed in words, not in bytes.

Add a new variant struct entry for the H616, and set the new flag to
mark the use of 34 bit addresses. Also the internal 32K oscillator is
required for TRNG operation, so specify all four clocks.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../crypto/allwinner/sun8i-ce/sun8i-ce-core.c | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
index 89ab3e08f0697..97955595806ef 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
@@ -92,6 +92,30 @@ static const struct ce_variant ce_h6_variant = {
 	.trng = CE_ALG_TRNG_V2,
 };
 
+static const struct ce_variant ce_h616_variant = {
+	.alg_cipher = { CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES,
+	},
+	.alg_hash = { CE_ALG_MD5, CE_ALG_SHA1, CE_ALG_SHA224, CE_ALG_SHA256,
+		CE_ALG_SHA384, CE_ALG_SHA512
+	},
+	.op_mode = { CE_OP_ECB, CE_OP_CBC
+	},
+	.cipher_t_dlen_in_bytes = true,
+	.hash_t_dlen_in_bits = true,
+	.prng_t_dlen_in_bytes = true,
+	.trng_t_dlen_in_bytes = true,
+	.needs_word_addresses = true,
+	.ce_clks = {
+		{ "bus", 0, 200000000 },
+		{ "mod", 300000000, 0 },
+		{ "ram", 0, 400000000 },
+		{ "trng", 0, 0 },
+		},
+	.esr = ESR_H6,
+	.prng = CE_ALG_PRNG_V2,
+	.trng = CE_ALG_TRNG_V2,
+};
+
 static const struct ce_variant ce_a64_variant = {
 	.alg_cipher = { CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES,
 	},
@@ -1098,6 +1122,8 @@ static const struct of_device_id sun8i_ce_crypto_of_match_table[] = {
 	  .data = &ce_h5_variant },
 	{ .compatible = "allwinner,sun50i-h6-crypto",
 	  .data = &ce_h6_variant },
+	{ .compatible = "allwinner,sun50i-h616-crypto",
+	  .data = &ce_h616_variant },
 	{}
 };
 MODULE_DEVICE_TABLE(of, sun8i_ce_crypto_of_match_table);
-- 
2.39.4

[PATCH 4/4] arm64: dts: allwinner: h616: add crypto engine node

[Andre Przywara]

The Allwinner H616 SoC contains a crypto engine very similar to the H6
version, but with all base addresses in the DMA descriptors shifted by
two bits. This requires a new compatible string.
Also the H616 CE relies on the internal osciallator for the TRNG
operation, so we need to reference this clock.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
index 921d5f61d8d6a..187663d45ed72 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
@@ -113,6 +113,16 @@ soc {
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x0 0x40000000>;
 
+		crypto: crypto@1904000 {
+			compatible = "allwinner,sun50i-h616-crypto";
+			reg = <0x01904000 0x1000>;
+			interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CE>, <&ccu CLK_CE>,
+				 <&ccu CLK_MBUS_CE>, <&rtc CLK_IOSC>;
+			clock-names = "bus", "mod", "ram", "trng";
+			resets = <&ccu RST_BUS_CE>;
+		};
+
 		syscon: syscon@3000000 {
 			compatible = "allwinner,sun50i-h616-system-control";
 			reg = <0x03000000 0x1000>;
-- 
2.39.4

Re: [PATCH 1/4] dt-bindings: crypto: sun8i-ce: Add compatible for H616

[Krzysztof Kozlowski]

On 17/06/2024 00:07, Andre Przywara wrote:
> The Allwinner H616 has a crypto engine very similar to the one in the
> H6, although all addresses in the DMA descriptors are shifted by 2 bits,
> to accommodate for the larger physical address space. That makes it
> incompatible to the H6 variant, and thus requires a new compatible
> string. Clock wise it relies on the internal oscillator for the TRNG,
> so needs all four possible clocks specified.
> 
> Add the compatible string to the list of recognised names, and add the
> H616 to list of devices requiring all four clocks.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 2/4] crypto: sun8i-ce - wrap accesses to descriptor address fields

[kernel test robot]

Hi Andre,

kernel test robot noticed the following build warnings:

[auto build test WARNING on sunxi/sunxi/for-next]
[also build test WARNING on herbert-cryptodev-2.6/master herbert-crypto-2.6/master linus/master v6.10-rc4 next-20240617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andre-Przywara/dt-bindings-crypto-sun8i-ce-Add-compatible-for-H616/20240617-061144
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
patch link:    https://lore.kernel.org/r/20240616220719.26641-3-andre.przywara%40arm.com
patch subject: [PATCH 2/4] crypto: sun8i-ce - wrap accesses to descriptor address fields
config: loongarch-randconfig-r111-20240618 (https://download.01.org/0day-ci/archive/20240618/202406181436.RZPPffYb-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240618/202406181436.RZPPffYb-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406181436.RZPPffYb-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c:175:34: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] value @@     got restricted __le32 @@
   drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c:175:34: sparse:     expected unsigned int [usertype] value
   drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c:175:34: sparse:     got restricted __le32

vim +175 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c

   167	
   168		mutex_lock(&ce->mlock);
   169	
   170		v = readl(ce->base + CE_ICR);
   171		v |= 1 << flow;
   172		writel(v, ce->base + CE_ICR);
   173	
   174		reinit_completion(&ce->chanlist[flow].complete);
 > 175		writel(sun8i_ce_desc_addr(ce, ce->chanlist[flow].t_phy),
   176		       ce->base + CE_TDQ);
   177	
   178		ce->chanlist[flow].status = 0;
   179		/* Be sure all data is written before enabling the task */
   180		wmb();
   181	
   182		/* Only H6 needs to write a part of t_common_ctl along with "1", but since it is ignored
   183		 * on older SoCs, we have no reason to complicate things.
   184		 */
   185		v = 1 | ((le32_to_cpu(ce->chanlist[flow].tl->t_common_ctl) & 0x7F) << 8);
   186		writel(v, ce->base + CE_TLR);
   187		mutex_unlock(&ce->mlock);
   188	
   189		wait_for_completion_interruptible_timeout(&ce->chanlist[flow].complete,
   190				msecs_to_jiffies(ce->chanlist[flow].timeout));
   191	
   192		if (ce->chanlist[flow].status == 0) {
   193			dev_err(ce->dev, "DMA timeout for %s (tm=%d) on flow %d\n", name,
   194				ce->chanlist[flow].timeout, flow);
   195			err = -EFAULT;
   196		}
   197		/* No need to lock for this read, the channel is locked so
   198		 * nothing could modify the error value for this channel
   199		 */
   200		v = readl(ce->base + CE_ESR);
   201		switch (ce->variant->esr) {
   202		case ESR_H3:
   203			/* Sadly, the error bit is not per flow */
   204			if (v) {
   205				dev_err(ce->dev, "CE ERROR: %x for flow %x\n", v, flow);
   206				err = -EFAULT;
   207				print_hex_dump(KERN_INFO, "TASK: ", DUMP_PREFIX_NONE, 16, 4,
   208					       cet, sizeof(struct ce_task), false);
   209			}
   210			if (v & CE_ERR_ALGO_NOTSUP)
   211				dev_err(ce->dev, "CE ERROR: algorithm not supported\n");
   212			if (v & CE_ERR_DATALEN)
   213				dev_err(ce->dev, "CE ERROR: data length error\n");
   214			if (v & CE_ERR_KEYSRAM)
   215				dev_err(ce->dev, "CE ERROR: keysram access error for AES\n");
   216			break;
   217		case ESR_A64:
   218		case ESR_D1:
   219		case ESR_H5:
   220		case ESR_R40:
   221			v >>= (flow * 4);
   222			v &= 0xF;
   223			if (v) {
   224				dev_err(ce->dev, "CE ERROR: %x for flow %x\n", v, flow);
   225				err = -EFAULT;
   226				print_hex_dump(KERN_INFO, "TASK: ", DUMP_PREFIX_NONE, 16, 4,
   227					       cet, sizeof(struct ce_task), false);
   228			}
   229			if (v & CE_ERR_ALGO_NOTSUP)
   230				dev_err(ce->dev, "CE ERROR: algorithm not supported\n");
   231			if (v & CE_ERR_DATALEN)
   232				dev_err(ce->dev, "CE ERROR: data length error\n");
   233			if (v & CE_ERR_KEYSRAM)
   234				dev_err(ce->dev, "CE ERROR: keysram access error for AES\n");
   235			break;
   236		case ESR_H6:
   237			v >>= (flow * 8);
   238			v &= 0xFF;
   239			if (v) {
   240				dev_err(ce->dev, "CE ERROR: %x for flow %x\n", v, flow);
   241				err = -EFAULT;
   242				print_hex_dump(KERN_INFO, "TASK: ", DUMP_PREFIX_NONE, 16, 4,
   243					       cet, sizeof(struct ce_task), false);
   244			}
   245			if (v & CE_ERR_ALGO_NOTSUP)
   246				dev_err(ce->dev, "CE ERROR: algorithm not supported\n");
   247			if (v & CE_ERR_DATALEN)
   248				dev_err(ce->dev, "CE ERROR: data length error\n");
   249			if (v & CE_ERR_KEYSRAM)
   250				dev_err(ce->dev, "CE ERROR: keysram access error for AES\n");
   251			if (v & CE_ERR_ADDR_INVALID)
   252				dev_err(ce->dev, "CE ERROR: address invalid\n");
   253			if (v & CE_ERR_KEYLADDER)
   254				dev_err(ce->dev, "CE ERROR: key ladder configuration error\n");
   255			break;
   256		}
   257	
   258		return err;
   259	}
   260	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/4] crypto: sun8i-ce - wrap accesses to descriptor address fields

[kernel test robot]

Hi Andre,

kernel test robot noticed the following build warnings:

[auto build test WARNING on sunxi/sunxi/for-next]
[also build test WARNING on herbert-cryptodev-2.6/master herbert-crypto-2.6/master linus/master v6.10-rc4 next-20240617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andre-Przywara/dt-bindings-crypto-sun8i-ce-Add-compatible-for-H616/20240617-061144
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
patch link:    https://lore.kernel.org/r/20240616220719.26641-3-andre.przywara%40arm.com
patch subject: [PATCH 2/4] crypto: sun8i-ce - wrap accesses to descriptor address fields
config: mips-randconfig-r121-20240618 (https://download.01.org/0day-ci/archive/20240618/202406182149.eAIruF9N-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 78ee473784e5ef6f0b19ce4cb111fb6e4d23c6b2)
reproduce: (https://download.01.org/0day-ci/archive/20240618/202406182149.eAIruF9N-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406182149.eAIruF9N-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c:175:34: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] val @@     got restricted __le32 @@
   drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c:175:34: sparse:     expected unsigned int [usertype] val
   drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c:175:34: sparse:     got restricted __le32

vim +175 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c

   167	
   168		mutex_lock(&ce->mlock);
   169	
   170		v = readl(ce->base + CE_ICR);
   171		v |= 1 << flow;
   172		writel(v, ce->base + CE_ICR);
   173	
   174		reinit_completion(&ce->chanlist[flow].complete);
 > 175		writel(sun8i_ce_desc_addr(ce, ce->chanlist[flow].t_phy),
   176		       ce->base + CE_TDQ);
   177	
   178		ce->chanlist[flow].status = 0;
   179		/* Be sure all data is written before enabling the task */
   180		wmb();
   181	
   182		/* Only H6 needs to write a part of t_common_ctl along with "1", but since it is ignored
   183		 * on older SoCs, we have no reason to complicate things.
   184		 */
   185		v = 1 | ((le32_to_cpu(ce->chanlist[flow].tl->t_common_ctl) & 0x7F) << 8);
   186		writel(v, ce->base + CE_TLR);
   187		mutex_unlock(&ce->mlock);
   188	
   189		wait_for_completion_interruptible_timeout(&ce->chanlist[flow].complete,
   190				msecs_to_jiffies(ce->chanlist[flow].timeout));
   191	
   192		if (ce->chanlist[flow].status == 0) {
   193			dev_err(ce->dev, "DMA timeout for %s (tm=%d) on flow %d\n", name,
   194				ce->chanlist[flow].timeout, flow);
   195			err = -EFAULT;
   196		}
   197		/* No need to lock for this read, the channel is locked so
   198		 * nothing could modify the error value for this channel
   199		 */
   200		v = readl(ce->base + CE_ESR);
   201		switch (ce->variant->esr) {
   202		case ESR_H3:
   203			/* Sadly, the error bit is not per flow */
   204			if (v) {
   205				dev_err(ce->dev, "CE ERROR: %x for flow %x\n", v, flow);
   206				err = -EFAULT;
   207				print_hex_dump(KERN_INFO, "TASK: ", DUMP_PREFIX_NONE, 16, 4,
   208					       cet, sizeof(struct ce_task), false);
   209			}
   210			if (v & CE_ERR_ALGO_NOTSUP)
   211				dev_err(ce->dev, "CE ERROR: algorithm not supported\n");
   212			if (v & CE_ERR_DATALEN)
   213				dev_err(ce->dev, "CE ERROR: data length error\n");
   214			if (v & CE_ERR_KEYSRAM)
   215				dev_err(ce->dev, "CE ERROR: keysram access error for AES\n");
   216			break;
   217		case ESR_A64:
   218		case ESR_D1:
   219		case ESR_H5:
   220		case ESR_R40:
   221			v >>= (flow * 4);
   222			v &= 0xF;
   223			if (v) {
   224				dev_err(ce->dev, "CE ERROR: %x for flow %x\n", v, flow);
   225				err = -EFAULT;
   226				print_hex_dump(KERN_INFO, "TASK: ", DUMP_PREFIX_NONE, 16, 4,
   227					       cet, sizeof(struct ce_task), false);
   228			}
   229			if (v & CE_ERR_ALGO_NOTSUP)
   230				dev_err(ce->dev, "CE ERROR: algorithm not supported\n");
   231			if (v & CE_ERR_DATALEN)
   232				dev_err(ce->dev, "CE ERROR: data length error\n");
   233			if (v & CE_ERR_KEYSRAM)
   234				dev_err(ce->dev, "CE ERROR: keysram access error for AES\n");
   235			break;
   236		case ESR_H6:
   237			v >>= (flow * 8);
   238			v &= 0xFF;
   239			if (v) {
   240				dev_err(ce->dev, "CE ERROR: %x for flow %x\n", v, flow);
   241				err = -EFAULT;
   242				print_hex_dump(KERN_INFO, "TASK: ", DUMP_PREFIX_NONE, 16, 4,
   243					       cet, sizeof(struct ce_task), false);
   244			}
   245			if (v & CE_ERR_ALGO_NOTSUP)
   246				dev_err(ce->dev, "CE ERROR: algorithm not supported\n");
   247			if (v & CE_ERR_DATALEN)
   248				dev_err(ce->dev, "CE ERROR: data length error\n");
   249			if (v & CE_ERR_KEYSRAM)
   250				dev_err(ce->dev, "CE ERROR: keysram access error for AES\n");
   251			if (v & CE_ERR_ADDR_INVALID)
   252				dev_err(ce->dev, "CE ERROR: address invalid\n");
   253			if (v & CE_ERR_KEYLADDER)
   254				dev_err(ce->dev, "CE ERROR: key ladder configuration error\n");
   255			break;
   256		}
   257	
   258		return err;
   259	}
   260	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/4] dt-bindings: crypto: sun8i-ce: Add compatible for H616

[Chen-Yu Tsai]

On Mon, Jun 17, 2024 at 6:08 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> The Allwinner H616 has a crypto engine very similar to the one in the
> H6, although all addresses in the DMA descriptors are shifted by 2 bits,
> to accommodate for the larger physical address space. That makes it
> incompatible to the H6 variant, and thus requires a new compatible
> string. Clock wise it relies on the internal oscillator for the TRNG,
> so needs all four possible clocks specified.
>
> Add the compatible string to the list of recognised names, and add the
> H616 to list of devices requiring all four clocks.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Chen-Yu Tsai <wens@csie.org>

Re: [PATCH 3/4] crypto: sun8i-ce - add Allwinner H616 support

[Chen-Yu Tsai]

On Mon, Jun 17, 2024 at 6:09 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> The crypto engine in the Allwinner H616 is very similar to the H6, but
> needs the base address for the task descriptor and the addresses within
> it to be expressed in words, not in bytes.
>
> Add a new variant struct entry for the H616, and set the new flag to
> mark the use of 34 bit addresses. Also the internal 32K oscillator is
> required for TRNG operation, so specify all four clocks.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Re: [PATCH 2/4] crypto: sun8i-ce - wrap accesses to descriptor address fields

[Chen-Yu Tsai]

On Mon, Jun 17, 2024 at 6:08 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> The Allwinner H616 (and later) SoCs support more than 32 bits worth of
> physical addresses. To accommodate the larger address space, the CE task
> descriptor fields holding addresses are now encoded as "word addresses",
> so take the actual address divided by four.
> This is true for the fields within the descriptor, but also for the
> descriptor base address, in the CE_TDA register.
>
> Wrap all accesses to those fields in a function, which will do the
> required division if needed. For now this in unused, so there should be
> no change in behaviour.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

though you need to fix up the reported sparse warning in sun8i_ce_run_task().

> ---
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c |  8 ++++----
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c   |  3 ++-
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c   |  6 +++---
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c   |  6 +++---
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c   |  2 +-
>  drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h        | 10 ++++++++++
>  6 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> index de50c00ba218f..3a5674b1bd3c0 100644
> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> @@ -190,7 +190,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
>                 err = -EFAULT;
>                 goto theend;
>         }
> -       cet->t_key = cpu_to_le32(rctx->addr_key);
> +       cet->t_key = sun8i_ce_desc_addr(ce, rctx->addr_key);
>
>         ivsize = crypto_skcipher_ivsize(tfm);
>         if (areq->iv && crypto_skcipher_ivsize(tfm) > 0) {
> @@ -208,7 +208,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
>                         err = -ENOMEM;
>                         goto theend_iv;
>                 }
> -               cet->t_iv = cpu_to_le32(rctx->addr_iv);
> +               cet->t_iv = sun8i_ce_desc_addr(ce, rctx->addr_iv);
>         }
>
>         if (areq->src == areq->dst) {
> @@ -236,7 +236,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
>
>         len = areq->cryptlen;
>         for_each_sg(areq->src, sg, nr_sgs, i) {
> -               cet->t_src[i].addr = cpu_to_le32(sg_dma_address(sg));
> +               cet->t_src[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
>                 todo = min(len, sg_dma_len(sg));
>                 cet->t_src[i].len = cpu_to_le32(todo / 4);
>                 dev_dbg(ce->dev, "%s total=%u SG(%d %u off=%d) todo=%u\n", __func__,
> @@ -251,7 +251,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
>
>         len = areq->cryptlen;
>         for_each_sg(areq->dst, sg, nr_sgd, i) {
> -               cet->t_dst[i].addr = cpu_to_le32(sg_dma_address(sg));
> +               cet->t_dst[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
>                 todo = min(len, sg_dma_len(sg));
>                 cet->t_dst[i].len = cpu_to_le32(todo / 4);
>                 dev_dbg(ce->dev, "%s total=%u SG(%d %u off=%d) todo=%u\n", __func__,
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
> index 0408b2d5d533b..89ab3e08f0697 100644
> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
> @@ -172,7 +172,8 @@ int sun8i_ce_run_task(struct sun8i_ce_dev *ce, int flow, const char *name)
>         writel(v, ce->base + CE_ICR);
>
>         reinit_completion(&ce->chanlist[flow].complete);
> -       writel(ce->chanlist[flow].t_phy, ce->base + CE_TDQ);
> +       writel(sun8i_ce_desc_addr(ce, ce->chanlist[flow].t_phy),
> +              ce->base + CE_TDQ);
>
>         ce->chanlist[flow].status = 0;
>         /* Be sure all data is written before enabling the task */
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> index ee2a28c906ede..a710ec9aa96f1 100644
> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> @@ -403,7 +403,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
>
>         len = areq->nbytes;
>         for_each_sg(areq->src, sg, nr_sgs, i) {
> -               cet->t_src[i].addr = cpu_to_le32(sg_dma_address(sg));
> +               cet->t_src[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
>                 todo = min(len, sg_dma_len(sg));
>                 cet->t_src[i].len = cpu_to_le32(todo / 4);
>                 len -= todo;
> @@ -414,7 +414,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
>                 goto theend;
>         }
>         addr_res = dma_map_single(ce->dev, result, digestsize, DMA_FROM_DEVICE);
> -       cet->t_dst[0].addr = cpu_to_le32(addr_res);
> +       cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, addr_res);
>         cet->t_dst[0].len = cpu_to_le32(digestsize / 4);
>         if (dma_mapping_error(ce->dev, addr_res)) {
>                 dev_err(ce->dev, "DMA map dest\n");
> @@ -445,7 +445,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
>         }
>
>         addr_pad = dma_map_single(ce->dev, buf, j * 4, DMA_TO_DEVICE);
> -       cet->t_src[i].addr = cpu_to_le32(addr_pad);
> +       cet->t_src[i].addr = sun8i_ce_desc_addr(ce, addr_pad);
>         cet->t_src[i].len = cpu_to_le32(j);
>         if (dma_mapping_error(ce->dev, addr_pad)) {
>                 dev_err(ce->dev, "DMA error on padding SG\n");
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
> index 80815379f6fc5..f030167f95945 100644
> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
> @@ -132,10 +132,10 @@ int sun8i_ce_prng_generate(struct crypto_rng *tfm, const u8 *src,
>         cet->t_sym_ctl = cpu_to_le32(sym);
>         cet->t_asym_ctl = 0;
>
> -       cet->t_key = cpu_to_le32(dma_iv);
> -       cet->t_iv = cpu_to_le32(dma_iv);
> +       cet->t_key = sun8i_ce_desc_addr(ce, dma_iv);
> +       cet->t_iv = sun8i_ce_desc_addr(ce, dma_iv);
>
> -       cet->t_dst[0].addr = cpu_to_le32(dma_dst);
> +       cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, dma_dst);
>         cet->t_dst[0].len = cpu_to_le32(todo / 4);
>         ce->chanlist[flow].timeout = 2000;
>
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
> index 9c35f2a83eda8..465c1c512eb85 100644
> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
> @@ -77,7 +77,7 @@ static int sun8i_ce_trng_read(struct hwrng *rng, void *data, size_t max, bool wa
>         cet->t_sym_ctl = 0;
>         cet->t_asym_ctl = 0;
>
> -       cet->t_dst[0].addr = cpu_to_le32(dma_dst);
> +       cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, dma_dst);
>         cet->t_dst[0].len = cpu_to_le32(todo / 4);
>         ce->chanlist[flow].timeout = todo;
>
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
> index 93d4985def87a..8fa58f3bb7f86 100644
> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
> @@ -149,6 +149,7 @@ struct ce_variant {
>         bool hash_t_dlen_in_bits;
>         bool prng_t_dlen_in_bytes;
>         bool trng_t_dlen_in_bytes;
> +       bool needs_word_addresses;
>         struct ce_clock ce_clks[CE_MAX_CLOCKS];
>         int esr;
>         unsigned char prng;
> @@ -241,6 +242,15 @@ struct sun8i_ce_dev {
>  #endif
>  };
>
> +static inline __le32 sun8i_ce_desc_addr(struct sun8i_ce_dev *dev,
> +                                       dma_addr_t addr)
> +{
> +       if (dev->variant->needs_word_addresses)
> +               return cpu_to_le32(addr / 4);
> +
> +       return cpu_to_le32(addr);
> +}
> +
>  /*
>   * struct sun8i_cipher_req_ctx - context for a skcipher request
>   * @op_dir:            direction (encrypt vs decrypt) for this request
> --
> 2.39.4
>

Re: [PATCH 0/4] crypto: sun8i-ce: add Allwinner H616 support

[Ryan Walklin]

On Mon, 17 Jun 2024, at 10:07 AM, Andre Przywara wrote:

Thanks Andre!

> Corentin's cryptotest passed for me, though I haven't checked how fast
> it is and if it really brings an advantage performance-wise, but maybe
> people find it useful to offload that from the CPU cores.

Running the rngtest gives the following output:

localhost:~# rngtest -c 10000 < /dev/random
rngtest 6.16
Copyright (c) 2004 by Henrique de Moraes Holschuh
This is free software; see the source for copying conditions.  There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

rngtest: starting FIPS tests...
rngtest: bits received from input: 200000032
rngtest: FIPS 140-2 successes: 9991
rngtest: FIPS 140-2 failures: 9
rngtest: FIPS 140-2(2001-10-10) Monobit: 0
rngtest: FIPS 140-2(2001-10-10) Poker: 2
rngtest: FIPS 140-2(2001-10-10) Runs: 2
rngtest: FIPS 140-2(2001-10-10) Long run: 5
rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
rngtest: input channel speed: (min=144.496; avg=808.068; max=866.977)Mibits/s
rngtest: FIPS tests speed: (min=17.199; avg=60.937; max=62.949)Mibits/s
rngtest: Program run time: 3369060 microseconds

So looks like a nice performance boost. 

> One immediate advantage is the availability of the TRNG device, which
> helps to feed the kernel's entropy pool much faster - typically before
> we reach userland. Without the driver this sometimes takes minutes, and
> delays workloads that rely on the entropy pool.

CRNG bringup now also very fast:

[    1.114790] sun8i-ce 1904000.crypto: CryptoEngine Die ID 0
[    1.116253] random: crng init done

Tested-by: Ryan Walklin <ryan@testtoast.com>

Regards,

Ryan

Re: [PATCH 2/4] crypto: sun8i-ce - wrap accesses to descriptor address fields

[Andre Przywara]

On Fri, 21 Jun 2024 22:53:47 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

Hi,

> On Mon, Jun 17, 2024 at 6:08 AM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > The Allwinner H616 (and later) SoCs support more than 32 bits worth of
> > physical addresses. To accommodate the larger address space, the CE task
> > descriptor fields holding addresses are now encoded as "word addresses",
> > so take the actual address divided by four.
> > This is true for the fields within the descriptor, but also for the
> > descriptor base address, in the CE_TDA register.
> >
> > Wrap all accesses to those fields in a function, which will do the
> > required division if needed. For now this in unused, so there should be
> > no change in behaviour.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Thanks for that!

> though you need to fix up the reported sparse warning in sun8i_ce_run_task().

Yeah, that turned out to be a nasty one, and uncovered an actual big
endian bug.
I dropped your R-b from v2 (in your inbox after testing), since I split up
the function and used a different variant for this one writel() caller. So
if you are happy with the changes in sun8i-ce.h and sun8i-ce-core.c: the
other files just use the renamed function name and didn't change otherwise.

Cheers,
Andre

> 
> > ---
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c |  8 ++++----
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c   |  3 ++-
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c   |  6 +++---
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c   |  6 +++---
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c   |  2 +-
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h        | 10 ++++++++++
> >  6 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> > index de50c00ba218f..3a5674b1bd3c0 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> > @@ -190,7 +190,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
> >                 err = -EFAULT;
> >                 goto theend;
> >         }
> > -       cet->t_key = cpu_to_le32(rctx->addr_key);
> > +       cet->t_key = sun8i_ce_desc_addr(ce, rctx->addr_key);
> >
> >         ivsize = crypto_skcipher_ivsize(tfm);
> >         if (areq->iv && crypto_skcipher_ivsize(tfm) > 0) {
> > @@ -208,7 +208,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
> >                         err = -ENOMEM;
> >                         goto theend_iv;
> >                 }
> > -               cet->t_iv = cpu_to_le32(rctx->addr_iv);
> > +               cet->t_iv = sun8i_ce_desc_addr(ce, rctx->addr_iv);
> >         }
> >
> >         if (areq->src == areq->dst) {
> > @@ -236,7 +236,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
> >
> >         len = areq->cryptlen;
> >         for_each_sg(areq->src, sg, nr_sgs, i) {
> > -               cet->t_src[i].addr = cpu_to_le32(sg_dma_address(sg));
> > +               cet->t_src[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
> >                 todo = min(len, sg_dma_len(sg));
> >                 cet->t_src[i].len = cpu_to_le32(todo / 4);
> >                 dev_dbg(ce->dev, "%s total=%u SG(%d %u off=%d) todo=%u\n", __func__,
> > @@ -251,7 +251,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
> >
> >         len = areq->cryptlen;
> >         for_each_sg(areq->dst, sg, nr_sgd, i) {
> > -               cet->t_dst[i].addr = cpu_to_le32(sg_dma_address(sg));
> > +               cet->t_dst[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
> >                 todo = min(len, sg_dma_len(sg));
> >                 cet->t_dst[i].len = cpu_to_le32(todo / 4);
> >                 dev_dbg(ce->dev, "%s total=%u SG(%d %u off=%d) todo=%u\n", __func__,
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
> > index 0408b2d5d533b..89ab3e08f0697 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
> > @@ -172,7 +172,8 @@ int sun8i_ce_run_task(struct sun8i_ce_dev *ce, int flow, const char *name)
> >         writel(v, ce->base + CE_ICR);
> >
> >         reinit_completion(&ce->chanlist[flow].complete);
> > -       writel(ce->chanlist[flow].t_phy, ce->base + CE_TDQ);
> > +       writel(sun8i_ce_desc_addr(ce, ce->chanlist[flow].t_phy),
> > +              ce->base + CE_TDQ);
> >
> >         ce->chanlist[flow].status = 0;
> >         /* Be sure all data is written before enabling the task */
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> > index ee2a28c906ede..a710ec9aa96f1 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> > @@ -403,7 +403,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
> >
> >         len = areq->nbytes;
> >         for_each_sg(areq->src, sg, nr_sgs, i) {
> > -               cet->t_src[i].addr = cpu_to_le32(sg_dma_address(sg));
> > +               cet->t_src[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
> >                 todo = min(len, sg_dma_len(sg));
> >                 cet->t_src[i].len = cpu_to_le32(todo / 4);
> >                 len -= todo;
> > @@ -414,7 +414,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
> >                 goto theend;
> >         }
> >         addr_res = dma_map_single(ce->dev, result, digestsize, DMA_FROM_DEVICE);
> > -       cet->t_dst[0].addr = cpu_to_le32(addr_res);
> > +       cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, addr_res);
> >         cet->t_dst[0].len = cpu_to_le32(digestsize / 4);
> >         if (dma_mapping_error(ce->dev, addr_res)) {
> >                 dev_err(ce->dev, "DMA map dest\n");
> > @@ -445,7 +445,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
> >         }
> >
> >         addr_pad = dma_map_single(ce->dev, buf, j * 4, DMA_TO_DEVICE);
> > -       cet->t_src[i].addr = cpu_to_le32(addr_pad);
> > +       cet->t_src[i].addr = sun8i_ce_desc_addr(ce, addr_pad);
> >         cet->t_src[i].len = cpu_to_le32(j);
> >         if (dma_mapping_error(ce->dev, addr_pad)) {
> >                 dev_err(ce->dev, "DMA error on padding SG\n");
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
> > index 80815379f6fc5..f030167f95945 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
> > @@ -132,10 +132,10 @@ int sun8i_ce_prng_generate(struct crypto_rng *tfm, const u8 *src,
> >         cet->t_sym_ctl = cpu_to_le32(sym);
> >         cet->t_asym_ctl = 0;
> >
> > -       cet->t_key = cpu_to_le32(dma_iv);
> > -       cet->t_iv = cpu_to_le32(dma_iv);
> > +       cet->t_key = sun8i_ce_desc_addr(ce, dma_iv);
> > +       cet->t_iv = sun8i_ce_desc_addr(ce, dma_iv);
> >
> > -       cet->t_dst[0].addr = cpu_to_le32(dma_dst);
> > +       cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, dma_dst);
> >         cet->t_dst[0].len = cpu_to_le32(todo / 4);
> >         ce->chanlist[flow].timeout = 2000;
> >
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
> > index 9c35f2a83eda8..465c1c512eb85 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
> > @@ -77,7 +77,7 @@ static int sun8i_ce_trng_read(struct hwrng *rng, void *data, size_t max, bool wa
> >         cet->t_sym_ctl = 0;
> >         cet->t_asym_ctl = 0;
> >
> > -       cet->t_dst[0].addr = cpu_to_le32(dma_dst);
> > +       cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, dma_dst);
> >         cet->t_dst[0].len = cpu_to_le32(todo / 4);
> >         ce->chanlist[flow].timeout = todo;
> >
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
> > index 93d4985def87a..8fa58f3bb7f86 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
> > @@ -149,6 +149,7 @@ struct ce_variant {
> >         bool hash_t_dlen_in_bits;
> >         bool prng_t_dlen_in_bytes;
> >         bool trng_t_dlen_in_bytes;
> > +       bool needs_word_addresses;
> >         struct ce_clock ce_clks[CE_MAX_CLOCKS];
> >         int esr;
> >         unsigned char prng;
> > @@ -241,6 +242,15 @@ struct sun8i_ce_dev {
> >  #endif
> >  };
> >
> > +static inline __le32 sun8i_ce_desc_addr(struct sun8i_ce_dev *dev,
> > +                                       dma_addr_t addr)
> > +{
> > +       if (dev->variant->needs_word_addresses)
> > +               return cpu_to_le32(addr / 4);
> > +
> > +       return cpu_to_le32(addr);
> > +}
> > +
> >  /*
> >   * struct sun8i_cipher_req_ctx - context for a skcipher request
> >   * @op_dir:            direction (encrypt vs decrypt) for this request
> > --
> > 2.39.4
> >  

Re: [PATCH 2/4] crypto: sun8i-ce - wrap accesses to descriptor address fields

[Andre Przywara]

On Tue, 18 Jun 2024 15:39:21 +0800
kernel test robot <lkp@intel.com> wrote:

Dear bot,

> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on sunxi/sunxi/for-next]
> [also build test WARNING on herbert-cryptodev-2.6/master herbert-crypto-2.6/master linus/master v6.10-rc4 next-20240617]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Andre-Przywara/dt-bindings-crypto-sun8i-ce-Add-compatible-for-H616/20240617-061144
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
> patch link:    https://lore.kernel.org/r/20240616220719.26641-3-andre.przywara%40arm.com
> patch subject: [PATCH 2/4] crypto: sun8i-ce - wrap accesses to descriptor address fields
> config: loongarch-randconfig-r111-20240618 (https://download.01.org/0day-ci/archive/20240618/202406181436.RZPPffYb-lkp@intel.com/config)
> compiler: loongarch64-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20240618/202406181436.RZPPffYb-lkp@intel.com/reproduce)

For the records: it looks like MIPS (the other report) and Loongson are the
two architectures that the bot built a big endian kernel for, and which
are using the asm-generic writel() implementation.
If I configure the arm64 kernel for big endian, I get this sparse warning
as well.

> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202406181436.RZPPffYb-lkp@intel.com/
> 
> sparse warnings: (new ones prefixed by >>)
> >> drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c:175:34: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int [usertype] value @@     got restricted __le32 @@  
>    drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c:175:34: sparse:     expected unsigned int [usertype] value
>    drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c:175:34: sparse:     got restricted __le32
> 
> vim +175 drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
> 
>    167	
>    168		mutex_lock(&ce->mlock);
>    169	
>    170		v = readl(ce->base + CE_ICR);
>    171		v |= 1 << flow;
>    172		writel(v, ce->base + CE_ICR);
>    173	
>    174		reinit_completion(&ce->chanlist[flow].complete);
>  > 175		writel(sun8i_ce_desc_addr(ce, ce->chanlist[flow].t_phy),  

So this turns out to be a genuine bug. All the other users of this new
sun8i_ce_desc_addr() function write its return value into a DMA
descriptor, which is a data structure that is interpreted by hardware. So
all fields in there must be little endian, and the type of __le32 enforces
this.
However this one caller here passes the value to writel(), which
expects a "natural" u32 and does a cpu_to_le32 conversion internally -
for related, but slightly different reasons.
So on a BE kernel this would have swapped the bytes twice, leading to the
original BE value wrongly written into the register.

So thanks for catching this, sparse and kernel test bot!

Fixed in v2, to be send out ASAP.

Cheers,
Andre

>    176		       ce->base + CE_TDQ);
>    177	
>    178		ce->chanlist[flow].status = 0;
>    179		/* Be sure all data is written before enabling the task */
>    180		wmb();
>    181	
>    182		/* Only H6 needs to write a part of t_common_ctl along with "1", but since it is ignored
>    183		 * on older SoCs, we have no reason to complicate things.
>    184		 */
>    185		v = 1 | ((le32_to_cpu(ce->chanlist[flow].tl->t_common_ctl) & 0x7F) << 8);
>    186		writel(v, ce->base + CE_TLR);
>    187		mutex_unlock(&ce->mlock);
>    188	
>    189		wait_for_completion_interruptible_timeout(&ce->chanlist[flow].complete,
>    190				msecs_to_jiffies(ce->chanlist[flow].timeout));
>    191	
>    192		if (ce->chanlist[flow].status == 0) {
>    193			dev_err(ce->dev, "DMA timeout for %s (tm=%d) on flow %d\n", name,
>    194				ce->chanlist[flow].timeout, flow);
>    195			err = -EFAULT;
>    196		}
>    197		/* No need to lock for this read, the channel is locked so
>    198		 * nothing could modify the error value for this channel
>    199		 */
>    200		v = readl(ce->base + CE_ESR);
>    201		switch (ce->variant->esr) {
>    202		case ESR_H3:
>    203			/* Sadly, the error bit is not per flow */
>    204			if (v) {
>    205				dev_err(ce->dev, "CE ERROR: %x for flow %x\n", v, flow);
>    206				err = -EFAULT;
>    207				print_hex_dump(KERN_INFO, "TASK: ", DUMP_PREFIX_NONE, 16, 4,
>    208					       cet, sizeof(struct ce_task), false);
>    209			}
>    210			if (v & CE_ERR_ALGO_NOTSUP)
>    211				dev_err(ce->dev, "CE ERROR: algorithm not supported\n");
>    212			if (v & CE_ERR_DATALEN)
>    213				dev_err(ce->dev, "CE ERROR: data length error\n");
>    214			if (v & CE_ERR_KEYSRAM)
>    215				dev_err(ce->dev, "CE ERROR: keysram access error for AES\n");
>    216			break;
>    217		case ESR_A64:
>    218		case ESR_D1:
>    219		case ESR_H5:
>    220		case ESR_R40:
>    221			v >>= (flow * 4);
>    222			v &= 0xF;
>    223			if (v) {
>    224				dev_err(ce->dev, "CE ERROR: %x for flow %x\n", v, flow);
>    225				err = -EFAULT;
>    226				print_hex_dump(KERN_INFO, "TASK: ", DUMP_PREFIX_NONE, 16, 4,
>    227					       cet, sizeof(struct ce_task), false);
>    228			}
>    229			if (v & CE_ERR_ALGO_NOTSUP)
>    230				dev_err(ce->dev, "CE ERROR: algorithm not supported\n");
>    231			if (v & CE_ERR_DATALEN)
>    232				dev_err(ce->dev, "CE ERROR: data length error\n");
>    233			if (v & CE_ERR_KEYSRAM)
>    234				dev_err(ce->dev, "CE ERROR: keysram access error for AES\n");
>    235			break;
>    236		case ESR_H6:
>    237			v >>= (flow * 8);
>    238			v &= 0xFF;
>    239			if (v) {
>    240				dev_err(ce->dev, "CE ERROR: %x for flow %x\n", v, flow);
>    241				err = -EFAULT;
>    242				print_hex_dump(KERN_INFO, "TASK: ", DUMP_PREFIX_NONE, 16, 4,
>    243					       cet, sizeof(struct ce_task), false);
>    244			}
>    245			if (v & CE_ERR_ALGO_NOTSUP)
>    246				dev_err(ce->dev, "CE ERROR: algorithm not supported\n");
>    247			if (v & CE_ERR_DATALEN)
>    248				dev_err(ce->dev, "CE ERROR: data length error\n");
>    249			if (v & CE_ERR_KEYSRAM)
>    250				dev_err(ce->dev, "CE ERROR: keysram access error for AES\n");
>    251			if (v & CE_ERR_ADDR_INVALID)
>    252				dev_err(ce->dev, "CE ERROR: address invalid\n");
>    253			if (v & CE_ERR_KEYLADDER)
>    254				dev_err(ce->dev, "CE ERROR: key ladder configuration error\n");
>    255			break;
>    256		}
>    257	
>    258		return err;
>    259	}
>    260	
>