Re: [PATCH v4 6/7] Add SPAcc dts overlay

Re: [PATCH v4 6/7] Add SPAcc dts overlay

[Geert Uytterhoeven]

 	Hi Pavitrakumar,

CC devicetree

On Tue, 18 Jun 2024, Pavitrakumar M wrote:
> Signed-off-by: Manjunath Hadli <manjunath.hadli@vayavyalabs.com>
> Signed-off-by: Pavitrakumar M <pavitrakumarm@vayavyalabs.com>
> Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com>

Thanks for your patch!

Please provide a patch description.
The one-line summary is also not very informative and lacks a suitable prefix.

> --- /dev/null
> +++ b/arch/arm64/boot/dts/xilinx/snps-dwc-spacc.dtso
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * dts file for Synopsys DWC SPAcc
> + *
> + * (C) Copyright 2024 Synopsys
> + *
> + * Ruud Derwig <Ruud.Derwig@synopsys.com>
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	fragment@0 {
> +		target = <&amba>;
> +
> +		overlay1: __overlay__ {

Please use sugar syntax.  See e.g. commit db2f3762d609318e ("of:
convert unittest overlay devicetree source to sugar syntax").

> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +
> +			dwc_spacc: spacc@400000000 {
> +				compatible = "snps-dwc-spacc";

Where can we find the DT bindings?

> +				reg = /bits/ 64 <0x400000000 0x3FFFF>;
> +				interrupts = <0 89 4>;
> +				interrupt-parent = <&gic>;
> +				clock-names = "ref_clk";
> +				spacc_priority = <0>;
> +				spacc_index = <0>;
> +			};
> +		};
> +	};
> +};

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH v4 0/7] Add SPAcc Crypto Driver Support

[Geert Uytterhoeven]

 	Hi Pavitrakumar,

CC devicetree

On Tue, 18 Jun 2024, Pavitrakumar M wrote:
> Add the driver for SPAcc(Security Protocol Accelerator), which is a
> crypto acceleration IP from Synopsys. The SPAcc supports many cipher,
> hash, aead algorithms and various modes.The driver currently supports
> below,

[...]

> Pavitrakumar M (7):
>  Add SPAcc Skcipher support
>  Enable SPAcc AUTODETECT
>  Add SPAcc ahash support
>  Add SPAcc aead support
>  Add SPAcc Kconfig and Makefile
>  Add SPAcc dts overlay
>  Enable Driver compilation in crypto Kconfig and Makefile

Thanks for your series, of which all but the dts patches have been
applied to the crypto tree (commits fc61c658c94cb740 ("crypto: spacc -
Enable Driver compilation in crypto Kconfig and Makefile") and before).

This driver uses device tree, but lacks DT bindings, which are a
requirement for new DT drivers.  So please provide DT bindings.

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH v4 1/7] Add SPAcc Skcipher support

[Geert Uytterhoeven]

 	Hi Pavitrakumar,

CC devicetree

On Tue, 18 Jun 2024, Pavitrakumar M wrote:
> Signed-off-by: shwetar <shwetar@vayavyalabs.com>
> Signed-off-by: Pavitrakumar M <pavitrakumarm@vayavyalabs.com>
> Acked-by: Ruud Derwig <Ruud.Derwig@synopsys.com>

Thanks for your patch, which is now commit fc61c658c94cb740 ("crypto:
spacc - Enable Driver compilation in crypto Kconfig and Makefile")
in crypto/master.

> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/spacc_core.c
> +int spacc_probe(struct platform_device *pdev,
> +		const struct of_device_id snps_spacc_id[])

There should not be a need to pass snps_spacc_id[] around.

> +{
> +	int spacc_idx = -1;
> +	struct resource *mem;
> +	int spacc_endian = 0;
> +	void __iomem *baseaddr;
> +	struct pdu_info   info;
> +	int spacc_priority = -1;
> +	struct spacc_priv *priv;
> +	int x = 0, err, oldmode, irq_num;
> +	const struct of_device_id *match, *id;
> +	u64 oldtimer = 100000, timer = 100000;
> +
> +	if (pdev->dev.of_node) {
> +		id = of_match_node(snps_spacc_id, pdev->dev.of_node);
> +		if (!id) {
> +			dev_err(&pdev->dev, "DT node did not match\n");
> +			return -EINVAL;
> +		}
> +	}

This check is not needed.

> +
> +	/* Initialize DDT DMA pools based on this device's resources */
> +	if (pdu_mem_init(&pdev->dev)) {
> +		dev_err(&pdev->dev, "Could not initialize DMA pools\n");
> +		return -ENOMEM;
> +	}
> +
> +	match = of_match_device(of_match_ptr(snps_spacc_id), &pdev->dev);
> +	if (!match) {
> +		dev_err(&pdev->dev, "SPAcc dtb missing");
> +		return -ENODEV;
> +	}

This check is also not needed.
Besides, in case of an error, you leak the ddt mem pool.

> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!mem) {
> +		dev_err(&pdev->dev, "no memory resource for spacc\n");
> +		err = -ENXIO;
> +		goto free_ddt_mem_pool;
> +	}
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		err = -ENOMEM;
> +		goto free_ddt_mem_pool;
> +	}
> +
> +	/* Read spacc priority and index and save inside priv.spacc.config */
> +	if (of_property_read_u32(pdev->dev.of_node, "spacc_priority",

Please no underscores in DT properties.

> +				 &spacc_priority)) {
> +		dev_err(&pdev->dev, "No vspacc priority specified\n");
> +		err = -EINVAL;
> +		goto free_ddt_mem_pool;
> +	}
> +
> +	if (spacc_priority < 0 && spacc_priority > VSPACC_PRIORITY_MAX) {
> +		dev_err(&pdev->dev, "Invalid vspacc priority\n");
> +		err = -EINVAL;
> +		goto free_ddt_mem_pool;
> +	}
> +	priv->spacc.config.priority = spacc_priority;
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "spacc_index",
> +				 &spacc_idx)) {
> +		dev_err(&pdev->dev, "No vspacc index specified\n");
> +		err = -EINVAL;
> +		goto free_ddt_mem_pool;
> +	}
> +	priv->spacc.config.idx = spacc_idx;
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "spacc_endian",

Please use the standard big-endian / little-endian properties.

> +				 &spacc_endian)) {
> +		dev_dbg(&pdev->dev, "No spacc_endian specified\n");
> +		dev_dbg(&pdev->dev, "Default spacc Endianness (0==little)\n");
> +		spacc_endian = 0;
> +	}
> +	priv->spacc.config.spacc_endian = spacc_endian;
> +
> +	if (of_property_read_u64(pdev->dev.of_node, "oldtimer",
> +				 &oldtimer)) {
> +		dev_dbg(&pdev->dev, "No oldtimer specified\n");
> +		dev_dbg(&pdev->dev, "Default oldtimer (100000)\n");
> +		oldtimer = 100000;
> +	}
> +	priv->spacc.config.oldtimer = oldtimer;
> +
> +	if (of_property_read_u64(pdev->dev.of_node, "timer", &timer)) {
> +		dev_dbg(&pdev->dev, "No timer specified\n");
> +		dev_dbg(&pdev->dev, "Default timer (100000)\n");
> +		timer = 100000;
> +	}
> +	priv->spacc.config.timer = timer;

This device lacks DT binding documentation.

> +static struct platform_driver spacc_driver = {
> +	.probe  = spacc_crypto_probe,
> +	.remove = spacc_crypto_remove,
> +	.driver = {
> +		.name  = "spacc",
> +		.of_match_table = of_match_ptr(snps_spacc_id),

Please drop the of_match_ptr(), as your driver requires DT to function.

> +		.owner = THIS_MODULE,
> +	},
> +};

I didn't review the rest.

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds