Re: [PATCH 3/5] crypto: ti - add driver for MCRC64 engine

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 30/07/2023 20:55, Kamlesh Gurudasani wrote:
> Add support for MCRC64 engine to calculate 64-bit CRC in Full-CPU mode.
> 
> In Full-CPU mode, the CPU does the data patterns transfer and signature
> verification all by itself, only CRC calculation is being done by MCRC64
> engine.
> 
> MCRC64 engine calculates 64-bit cyclic redundancy checks (CRC)
> according to the ISO 3309 standard.
> 
> Generator polynomial: x^64 + x^4 + x^3 + x + 1
> Polynomial value: 0x000000000000001b
> 
> Signed-off-by: Kamlesh Gurudasani <kamlesh@ti.com>
> ---
>  MAINTAINERS                |   2 +
>  drivers/crypto/Kconfig     |   1 +
>  drivers/crypto/Makefile    |   1 +
>  drivers/crypto/ti/Kconfig  |  10 +++
>  drivers/crypto/ti/Makefile |   2 +
>  drivers/crypto/ti/mcrc64.c | 360 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 376 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d8680f6969e3..a2f50adb51ac 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21464,8 +21464,10 @@ F:	drivers/iio/adc/ti-lmp92064.c
>  
>  TI MEMORY CYCLIC REDUNDANCY CHECK (MCRC64) DRIVER
>  M:	Kamlesh Gurudasani <kamlesh@ti.com>
> +L:	linux-crypto@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/crypto/ti,mcrc64.yaml
> +F:	drivers/crypto/ti/mcrc64.c
>  
>  TI PCM3060 ASoC CODEC DRIVER
>  M:	Kirill Marinushkin <kmarinushkin@birdec.com>
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index c761952f0dc6..2101f92ead66 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -796,5 +796,6 @@ config CRYPTO_DEV_SA2UL
>  
>  source "drivers/crypto/aspeed/Kconfig"
>  source "drivers/crypto/starfive/Kconfig"
> +source "drivers/crypto/ti/Kconfig"
>  
>  endif # CRYPTO_HW
> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> index d859d6a5f3a4..f1a151b73ff1 100644
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_CRYPTO_DEV_SAHARA) += sahara.o
>  obj-$(CONFIG_CRYPTO_DEV_SL3516) += gemini/
>  obj-y += stm32/
>  obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
> +obj-$(CONFIG_ARCH_K3) += ti/
>  obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio/
>  obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
>  obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
> diff --git a/drivers/crypto/ti/Kconfig b/drivers/crypto/ti/Kconfig
> new file mode 100644
> index 000000000000..8e3b2b8b7623
> --- /dev/null
> +++ b/drivers/crypto/ti/Kconfig
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config CRYPTO_DEV_TI_MCRC64
> +	tristate "Support for TI MCRC64 crc64 accelerators"
> +	depends on ARCH_K3

So it is a part of Soc. Fix the compatible.

> +	select CRYPTO_HASH
> +	help
> +	  This enables support for the MCRC64 hw accelerator
> +	  which can be found on TI SOC.
> +	  MCRC64 engine calculates 64-bit cyclic redundancy checks (CRC)
> +	  according to the ISO 3309 standard using Full-CPU mode.
> \ No newline at end of file

You have warnings...

> diff --git a/drivers/crypto/ti/Makefile b/drivers/crypto/ti/Makefile
> new file mode 100644
> index 000000000000..94ffc2576137
> --- /dev/null
> +++ b/drivers/crypto/ti/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_CRYPTO_DEV_TI_MCRC64) += mcrc64.o
> diff --git a/drivers/crypto/ti/mcrc64.c b/drivers/crypto/ti/mcrc64.c
> new file mode 100644
> index 000000000000..45f8ae6078ff
> --- /dev/null
> +++ b/drivers/crypto/ti/mcrc64.c
> @@ -0,0 +1,360 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) Texas Instruments 2023 - http://www.ti.com
> + * Author: Kamlesh Gurudasani <kamlesh@ti.com>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <crypto/internal/hash.h>
> +
> +#include <asm/unaligned.h>
> +
> +#define DRIVER_NAME		"mcrc64"
> +#define CHKSUM_DIGEST_SIZE	8
> +#define CHKSUM_BLOCK_SIZE	1
> +
> +/* Registers */
> +#define CRC_CTRL0 0x0000 /* CRC Global Control Register 0 */
> +#define CH_PSA_SWRE(ch) BIT(((ch) - 1) << 3) /* PSA Software Reset  */
> +
> +#define CRC_CTRL1 0x0008 /* CRC Global Control Register 1 */
> +#define PWDN BIT(0) /* Power Down  */
> +
> +#define CRC_CTRL2 0x0010 /* CRC Global Control Register 2 */
> +#define CH_MODE(ch, m) ((m) << (((ch) - 1) << 3))
> +
> +#define PSA_SIGREGL(ch) ((0x6 + (4 * ((ch) - 1))) << 4) /* Signature register */
> +
> +#define MCRC64_ALG_MASK 0x8000000000000000
> +#define MCRC64_CRC64_POLY 0x000000000000001b
> +
> +#define MCRC64_AUTOSUSPEND_DELAY	50
> +
> +static struct device *mcrc64_k3_dev;

Nope. How do you support two devices? No, no, drop such approach
entirely from *all your drivers before upstreaming*.

> +
> +enum mcrc64_mode {
> +	MCRC64_MODE_DATA_CAPTURE = 0,
> +	MCRC64_MODE_AUTO,
> +	MCRC64_MODE_SEMI_CPU,
> +	MCRC64_MODE_FULL_CPU,
> +	MCRC64_MODE_INVALID,
> +};
> +
> +enum mcrc64_channel {
> +	MCRC64_CHANNEL_1 = 1,
> +	MCRC64_CHANNEL_2,
> +	MCRC64_CHANNEL_3,
> +	MCRC64_CHANNEL_4,
> +	MCRC64_CHANNEL_INVALID,
> +};
> +
> +struct mcrc64_data {
> +	struct device	 *dev;
> +	void __iomem	 *regs;
> +};
> +
> +struct mcrc64_ctx {
> +	u32 key;
> +};
> +
> +struct mcrc64_desc_ctx {
> +	u64    signature;

Keep consistent indentation/alignment in structures.

> +};
> +

...

> +
> +static int mcrc64_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mcrc64_data *dev_data;
> +
> +	dev_data = devm_kzalloc(dev, sizeof(*dev_data), GFP_KERNEL);
> +	if (!dev_data)
> +		return -ENOMEM;
> +
> +	mcrc64_k3_dev = dev;
> +	dev_data->dev = dev;
> +	dev_data->regs = devm_platform_ioremap_resource(pdev, 0);
> +
> +	platform_set_drvdata(pdev, dev_data);
> +	dev_set_drvdata(mcrc64_k3_dev, dev_data);
> +
> +	crypto_register_shashes(algs, ARRAY_SIZE(algs));
> +
> +	pm_runtime_set_autosuspend_delay(dev, MCRC64_AUTOSUSPEND_DELAY);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	pm_runtime_put_sync(dev);
> +
> +	return 0;
> +}
> +
> +static int mcrc64_remove(struct platform_device *pdev)
> +{
> +	struct mcrc64_data *dev_data = platform_get_drvdata(pdev);
> +
> +	int ret = pm_runtime_get_sync(dev_data->dev);
> +
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(dev_data->dev);
> +		return ret;
> +	}

Isn't there helper doing this for you?


Best regards,
Krzysztof