[v4,0/4] crypto: starfive - Add drivers for crypto engine

This patch series adds kernel driver support for StarFive JH7110 crypto
engine. The first patch adds Documentations for the device and Patch 2
adds device probe and DMA init for the module. Patch 3 adds crypto and
DMA dts node for VisionFive 2 board. Patch 4 adds hash/hmac support to
the module.

Patch 3 needs to be applied on top of:
https://patchwork.kernel.org/project/linux-riscv/cover/20230120024445.244345-1-xingyu.wu@starfivetech.com/

Changes v3->v4:
- Use fallback for non-aligned cases as hardware doesn't support
  hashing piece-meal (Herbert)
- Use ahash_request_set_* helpers to update members of ahash_request
  (Herbert)
- Set callbacks for async fallback (Herbert)
- Remove completion variable and use dma_callback to do the rest of
  processing instead. (Herbert)

Changes v2->v3:
- Only implement digest and use fallback for other ops (Herbert)
- Use interrupt instead of polling for hash complete (Herbert)
- Remove manual data copy from out-of-bound memory location as it will
  be handled by DMA API. (Christoph & Herbert)

Changes v1->v2:
- Fixed yaml filename and format (Krzysztof)
- Removed unnecessary property names in yaml (Krzysztof)
- Moved of_device_id table close to usage (Krzysztof)
- Use dev_err_probe for error returns (Krzysztof)
- Dropped redundant readl and writel wrappers (Krzysztof)
- Updated commit signed offs (Conor)
- Dropped redundant node in dts, module set to on in dtsi (Conor)

Jia Jie Ho (4):
  dt-bindings: crypto: Add StarFive crypto module
  crypto: starfive - Add crypto engine support
  riscv: dts: starfive: Add crypto and DMA node for VisionFive 2
  crypto: starfive - Add hash and HMAC support

 .../crypto/starfive,jh7110-crypto.yaml        |  70 ++
 MAINTAINERS                                   |   7 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  28 +
 drivers/crypto/Kconfig                        |   1 +
 drivers/crypto/Makefile                       |   1 +
 drivers/crypto/starfive/Kconfig               |  21 +
 drivers/crypto/starfive/Makefile              |   4 +
 drivers/crypto/starfive/jh7110-cryp.c         | 234 +++++
 drivers/crypto/starfive/jh7110-cryp.h         | 127 +++
 drivers/crypto/starfive/jh7110-hash.c         | 990 ++++++++++++++++++
 10 files changed, 1483 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/starfive,jh7110-crypto.yaml
 create mode 100644 drivers/crypto/starfive/Kconfig
 create mode 100644 drivers/crypto/starfive/Makefile
 create mode 100644 drivers/crypto/starfive/jh7110-cryp.c
 create mode 100644 drivers/crypto/starfive/jh7110-cryp.h
 create mode 100644 drivers/crypto/starfive/jh7110-hash.c

On Mon, Apr 10, 2023 at 03:37:52PM +0800, Jia Jie Ho wrote:
>
> +static void starfive_hash_start(void *param)
> +{
> +	struct starfive_cryp_ctx *ctx = param;
> +	struct starfive_cryp_request_ctx *rctx = ctx->rctx;
> +	struct starfive_cryp_dev *cryp = ctx->cryp;
> +	union starfive_alg_cr alg_cr;
> +	union starfive_hash_csr csr;
> +
> +	dma_unmap_sg(cryp->dev, rctx->in_sg, rctx->in_sg_len, DMA_TO_DEVICE);
> +
> +	alg_cr.v = 0;
> +	alg_cr.clear = 1;
> +
> +	writel(alg_cr.v, cryp->base + STARFIVE_ALG_CR_OFFSET);
> +
> +	csr.v = readl(cryp->base + STARFIVE_HASH_SHACSR);
> +	csr.firstb = 0;
> +	csr.final = 1;
> +
> +	reinit_completion(&cryp->hash_done);
> +	writel(~STARFIVE_IE_MASK_HASH_DONE, cryp->base + STARFIVE_IE_MASK_OFFSET);
> +	writel(csr.v, cryp->base + STARFIVE_HASH_SHACSR);
> +}

Why are you still using a completion? The callback function should
invoke the crypto_engine finalize_request call directly.

> +static int starfive_hash_xmit(struct starfive_cryp_ctx *ctx)
> +{
> +	struct starfive_cryp_request_ctx *rctx = ctx->rctx;
> +	struct starfive_cryp_dev *cryp = ctx->cryp;
> +	int ret;
> +
> +	rctx->csr.hash.v = 0;
> +	rctx->csr.hash.reset = 1;
> +	writel(rctx->csr.hash.v, cryp->base + STARFIVE_HASH_SHACSR);
> +
> +	if (starfive_hash_wait_busy(ctx))
> +		return dev_err_probe(cryp->dev, -ETIMEDOUT, "Error resetting engine.\n");
> +
> +	rctx->csr.hash.v = 0;
> +	rctx->csr.hash.mode = ctx->hash_mode & STARFIVE_HASH_MODE_MASK;
> +	rctx->csr.hash.ie = 1;
> +
> +	if (ctx->hash_mode & STARFIVE_HASH_HMAC_FLAGS) {
> +		ret = starfive_hash_hmac_key(ctx);
> +		if (ret)
> +			return ret;
> +	} else {
> +		rctx->csr.hash.start = 1;
> +		rctx->csr.hash.firstb = 1;
> +		writel(rctx->csr.hash.v, cryp->base + STARFIVE_HASH_SHACSR);
> +	}
> +
> +	ret = starfive_hash_xmit_dma(ctx);
> +	if (ret)
> +		return ret;
> +
> +	if (!wait_for_completion_timeout(&cryp->hash_done, msecs_to_jiffies(10000)))
> +		return dev_err_probe(cryp->dev, -ETIMEDOUT, "Timeout waiting for hash done\n");

There is no point in waiting for completion.  Just return 0 and
you're done.

Cheers,

On 10/4/2023 3:50 pm, Herbert Xu wrote:
> On Mon, Apr 10, 2023 at 03:37:52PM +0800, Jia Jie Ho wrote:
>>
>> +static void starfive_hash_start(void *param)
>> +{
>> +	struct starfive_cryp_ctx *ctx = param;
>> +	struct starfive_cryp_request_ctx *rctx = ctx->rctx;
>> +	struct starfive_cryp_dev *cryp = ctx->cryp;
>> +	union starfive_alg_cr alg_cr;
>> +	union starfive_hash_csr csr;
>> +
>> +	dma_unmap_sg(cryp->dev, rctx->in_sg, rctx->in_sg_len, DMA_TO_DEVICE);
>> +
>> +	alg_cr.v = 0;
>> +	alg_cr.clear = 1;
>> +
>> +	writel(alg_cr.v, cryp->base + STARFIVE_ALG_CR_OFFSET);
>> +
>> +	csr.v = readl(cryp->base + STARFIVE_HASH_SHACSR);
>> +	csr.firstb = 0;
>> +	csr.final = 1;
>> +
>> +	reinit_completion(&cryp->hash_done);
>> +	writel(~STARFIVE_IE_MASK_HASH_DONE, cryp->base + STARFIVE_IE_MASK_OFFSET);
>> +	writel(csr.v, cryp->base + STARFIVE_HASH_SHACSR);
>> +}
> 
> Why are you still using a completion? The callback function should
> invoke the crypto_engine finalize_request call directly.
> 

Hi Herbert,
The hardware requires user to set a 'final' bit after data transfer completed.
This completion is to wait for the interrupt signal from device that the final digest 
has been populated to the read registers.

I'll do the finalize_request call directly in the next version.

>> +static int starfive_hash_xmit(struct starfive_cryp_ctx *ctx)
>> +{
>> +	struct starfive_cryp_request_ctx *rctx = ctx->rctx;
>> +	struct starfive_cryp_dev *cryp = ctx->cryp;
>> +	int ret;
>> +
>> +	rctx->csr.hash.v = 0;
>> +	rctx->csr.hash.reset = 1;
>> +	writel(rctx->csr.hash.v, cryp->base + STARFIVE_HASH_SHACSR);
>> +
>> +	if (starfive_hash_wait_busy(ctx))
>> +		return dev_err_probe(cryp->dev, -ETIMEDOUT, "Error resetting engine.\n");
>> +
>> +	rctx->csr.hash.v = 0;
>> +	rctx->csr.hash.mode = ctx->hash_mode & STARFIVE_HASH_MODE_MASK;
>> +	rctx->csr.hash.ie = 1;
>> +
>> +	if (ctx->hash_mode & STARFIVE_HASH_HMAC_FLAGS) {
>> +		ret = starfive_hash_hmac_key(ctx);
>> +		if (ret)
>> +			return ret;
>> +	} else {
>> +		rctx->csr.hash.start = 1;
>> +		rctx->csr.hash.firstb = 1;
>> +		writel(rctx->csr.hash.v, cryp->base + STARFIVE_HASH_SHACSR);
>> +	}
>> +
>> +	ret = starfive_hash_xmit_dma(ctx);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!wait_for_completion_timeout(&cryp->hash_done, msecs_to_jiffies(10000)))
>> +		return dev_err_probe(cryp->dev, -ETIMEDOUT, "Timeout waiting for hash done\n");
> 
> There is no point in waiting for completion.  Just return 0 and
> you're done.
> 

I'll change this in the next version too.

Thanks for taking time reviewing this patch.

Best regards,
Jia Jie

On Mon, Apr 10, 2023 at 04:43:37PM +0800, Jia Jie Ho wrote:
>
> The hardware requires user to set a 'final' bit after data transfer completed.
> This completion is to wait for the interrupt signal from device that the final digest 
> has been populated to the read registers.
> 
> I'll do the finalize_request call directly in the next version.

Instead of the IRQ performing a completion, it could instead schedule
a tasklet and do the callback directly from the tasklet.

Actually, the ordering between the IRQ and DMA callback is a bit
confusing.  Which one is supposed to occur first and how does it
interact with the other event?

Cheers,

> Subject: Re: [PATCH v4 4/4] crypto: starfive - Add hash and HMAC support
> 
> On Mon, Apr 10, 2023 at 04:43:37PM +0800, Jia Jie Ho wrote:
> >
> > The hardware requires user to set a 'final' bit after data transfer completed.
> > This completion is to wait for the interrupt signal from device that
> > the final digest has been populated to the read registers.
> >
> > I'll do the finalize_request call directly in the next version.
> 
> Instead of the IRQ performing a completion, it could instead schedule a tasklet
> and do the callback directly from the tasklet.
> 
> Actually, the ordering between the IRQ and DMA callback is a bit confusing.
> Which one is supposed to occur first and how does it interact with the other
> event?
> 

The sequence of event would be:
1. Wait for DMA transfer to complete.
2. Set bit in device CSR to indicate final block has been transferred.
3. Device will send IRQ once result is ready.
4. Read out final digest value from device.

Thanks,
Jia Jie

On Mon, Apr 10, 2023 at 01:10:29PM +0000, JiaJie Ho wrote:
>
> The sequence of event would be:
> 1. Wait for DMA transfer to complete.
> 2. Set bit in device CSR to indicate final block has been transferred.

OK, 2 should be done in the DMA callback function.

> 3. Device will send IRQ once result is ready.
> 4. Read out final digest value from device.

4 should be done from a tasklet scheduled from the IRQ handler.

Cheers,

> > The sequence of event would be:
> > 1. Wait for DMA transfer to complete.
> > 2. Set bit in device CSR to indicate final block has been transferred.
> 
> OK, 2 should be done in the DMA callback function.
> 
> > 3. Device will send IRQ once result is ready.
> > 4. Read out final digest value from device.
> 
> 4 should be done from a tasklet scheduled from the IRQ handler.
> 

I'll update the driver accordingly. 
Thanks again for the advice.

Regards,
Jia Jie