mbox series

[0/6] crypto: starfive: Add driver for cryptographic engine

Message ID 20221130055214.2416888-1-jiajie.ho@starfivetech.com
Headers show
Series crypto: starfive: Add driver for cryptographic engine | expand

Message

JiaJie Ho Nov. 30, 2022, 5:52 a.m. UTC
This patch series adds kernel driver support for Starfive crypto engine.
The engine supports hardware acceleration for HMAC/hash functions,
AES block cipher operations and RSA. The first patch adds basic driver
for device probe and DMA init. The subsequent patches adds supported
crypto primitives to the driver which include hash functions, AES and
RSA. Patch 5 adds documentation to describe device tree bindings and the
last patch adds device node to VisionFive 2 dts.

The driver has been tested with crypto selftest and additional test.

This patch series depends on the following patches:
https://patchwork.kernel.org/project/linux-riscv/cover/20221118010627.70576-1-hal.feng@starfivetech.com/
https://patchwork.kernel.org/project/linux-riscv/cover/20221118011714.70877-1-hal.feng@starfivetech.com/

Jia Jie Ho (6):
  crypto: starfive - Add StarFive crypto engine support
  crypto: starfive - Add hash and HMAC support
  crypto: starfive - Add AES skcipher and aead support
  crypto: starfive - Add Public Key algo support
  dt-bindings: crypto: Add bindings for Starfive crypto driver
  riscv: dts: starfive: Add crypto and DMA node for VisionFive 2

 .../bindings/crypto/starfive-crypto.yaml      |  109 ++
 MAINTAINERS                                   |    7 +
 .../jh7110-starfive-visionfive-v2.dts         |    8 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |   36 +
 drivers/crypto/Kconfig                        |    1 +
 drivers/crypto/Makefile                       |    1 +
 drivers/crypto/starfive/Kconfig               |   20 +
 drivers/crypto/starfive/Makefile              |    4 +
 drivers/crypto/starfive/starfive-aes.c        | 1723 +++++++++++++++++
 drivers/crypto/starfive/starfive-cryp.c       |  324 ++++
 drivers/crypto/starfive/starfive-hash.c       | 1152 +++++++++++
 drivers/crypto/starfive/starfive-pka.c        |  683 +++++++
 drivers/crypto/starfive/starfive-regs.h       |  200 ++
 drivers/crypto/starfive/starfive-str.h        |  194 ++
 14 files changed, 4462 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/starfive-crypto.yaml
 create mode 100644 drivers/crypto/starfive/Kconfig
 create mode 100644 drivers/crypto/starfive/Makefile
 create mode 100644 drivers/crypto/starfive/starfive-aes.c
 create mode 100644 drivers/crypto/starfive/starfive-cryp.c
 create mode 100644 drivers/crypto/starfive/starfive-hash.c
 create mode 100644 drivers/crypto/starfive/starfive-pka.c
 create mode 100644 drivers/crypto/starfive/starfive-regs.h
 create mode 100644 drivers/crypto/starfive/starfive-str.h

Comments

Conor Dooley Nov. 30, 2022, 9:31 a.m. UTC | #1
Hey Jia Jie Ho,

On 30/11/2022 05:52, Jia Jie Ho wrote:
> [You don't often get email from jiajie.ho@starfivetech.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Adding StarFive crypto IP and DMA controller node
> to VisionFive 2 SoC.
> 
> Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> Signed-off-by: Huan Feng <huan.feng@starfivetech.com>

Out of curiosity, what was Huan Feng's contribution to this patch?
Did they co-develop it, or is there some other reason?

> ---
>   .../jh7110-starfive-visionfive-v2.dts         |  8 +++++
>   arch/riscv/boot/dts/starfive/jh7110.dtsi      | 36 +++++++++++++++++++

I figure Emil will likely see anyway, but whenever you get actual
review comments and send a v2 - please don't drop people that
get_maintainer.pl tells you are responsible for the code in
question.

>   2 files changed, 44 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> index 450e920236a5..da2aa4d597f3 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> @@ -115,3 +115,11 @@ &tdm_ext {
>   &mclk_ext {
>          clock-frequency = <49152000>;
>   };
> +
> +&sec_dma {
> +       status = "okay";
> +};
> +
> +&crypto {
> +       status = "okay";
> +};

In what scenario would you not want to have these enabled?

Thanks,
Conor.

> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index 4ac159d79d66..745a5650882c 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -455,5 +455,41 @@ uart5: serial@12020000 {
>                          reg-shift = <2>;
>                          status = "disabled";
>                  };
> +
> +               sec_dma: sec_dma@16008000 {
> +                       compatible = "arm,pl080", "arm,primecell";
> +                       arm,primecell-periphid = <0x00041080>;
> +                       reg = <0x0 0x16008000 0x0 0x4000>;
> +                       reg-names = "sec_dma";
> +                       interrupts = <29>;
> +                       clocks = <&stgcrg JH7110_STGCLK_SEC_HCLK>,
> +                                <&stgcrg JH7110_STGCLK_SEC_MISCAHB>;
> +                       clock-names = "sec_hclk","apb_pclk";
> +                       resets = <&stgcrg JH7110_STGRST_SEC_TOP_HRESETN>;
> +                       reset-names = "sec_hre";
> +                       lli-bus-interface-ahb1;
> +                       mem-bus-interface-ahb1;
> +                       memcpy-burst-size = <256>;
> +                       memcpy-bus-width = <32>;
> +                       #dma-cells = <2>;
> +                       status = "disabled";
> +               };
> +
> +               crypto: crypto@16000000 {
> +                       compatible = "starfive,jh7110-crypto";
> +                       reg = <0x0 0x16000000 0x0 0x4000>;
> +                       reg-names = "secreg";
> +                       clocks = <&stgcrg JH7110_STGCLK_SEC_HCLK>,
> +                                <&stgcrg JH7110_STGCLK_SEC_MISCAHB>;
> +                       clock-names = "sec_hclk","sec_ahb";
> +                       resets = <&stgcrg JH7110_STGRST_SEC_TOP_HRESETN>;
> +                       reset-names = "sec_hre";
> +                       enable-side-channel-mitigation;
> +                       enable-dma;
> +                       dmas = <&sec_dma 1 2>,
> +                              <&sec_dma 0 2>;
> +                       dma-names = "sec_m","sec_p";
> +                       status = "disabled";
> +               };
>          };
>   };
> --
> 2.25.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Krzysztof Kozlowski Nov. 30, 2022, 1:15 p.m. UTC | #2
On 30/11/2022 06:52, Jia Jie Ho wrote:
> Adding device probe and DMA init for StarFive
> hardware crypto engine.
> 
> Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> ---


> +
> +static const struct of_device_id starfive_dt_ids[] = {
> +	{ .compatible = "starfive,jh7110-crypto", .data = NULL},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, starfive_dt_ids);

Keep your table close to its usage, just like in many other drivers.

> +
> +static int starfive_dma_init(struct starfive_sec_dev *sdev)
> +{
> +	dma_cap_mask_t mask;
> +	int err;
> +
> +	sdev->sec_xm_m = NULL;
> +	sdev->sec_xm_p = NULL;
> +
> +	dma_cap_zero(mask);
> +	dma_cap_set(DMA_SLAVE, mask);
> +
> +	sdev->sec_xm_m = dma_request_chan(sdev->dev, "sec_m");
> +	if (IS_ERR(sdev->sec_xm_m)) {
> +		dev_err(sdev->dev, "sec_m dma channel request failed.\n");

return dev_err_probe

> +		return PTR_ERR(sdev->sec_xm_m);
> +	}
> +
> +	sdev->sec_xm_p = dma_request_chan(sdev->dev, "sec_p");
> +	if (IS_ERR(sdev->sec_xm_p)) {
> +		dev_err(sdev->dev, "sec_p dma channel request failed.\n");

dev_err_probe

> +		goto err_dma_out;
> +	}
> +
> +	init_completion(&sdev->sec_comp_m);
> +	init_completion(&sdev->sec_comp_p);
> +
> +	return 0;
> +
> +err_dma_out:
> +	dma_release_channel(sdev->sec_xm_m);

I don't think you tested it. Not even built with proper tools liek
smatch, sparse, W=1. Where do you set err?

> +
> +	return err;
> +}
> +
> +static void starfive_dma_cleanup(struct starfive_sec_dev *sdev)
> +{
> +	dma_release_channel(sdev->sec_xm_p);
> +	dma_release_channel(sdev->sec_xm_m);
> +}
> +
> +static int starfive_cryp_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct starfive_sec_dev *sdev;
> +	struct resource *res;
> +	int pages = 0;
> +	int ret;
> +
> +	sdev = devm_kzalloc(dev, sizeof(*sdev), GFP_KERNEL);
> +	if (!sdev)
> +		return -ENOMEM;
> +
> +	sdev->dev = dev;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "secreg");
> +	sdev->io_base = devm_ioremap_resource(dev, res);

I think there is a wrapper for both calls...

> +
> +	if (IS_ERR(sdev->io_base))
> +		return PTR_ERR(sdev->io_base);
> +
> +	sdev->use_side_channel_mitigation =
> +		device_property_read_bool(dev, "enable-side-channel-mitigation");
> +	sdev->use_dma = device_property_read_bool(dev, "enable-dma");
> +	sdev->dma_maxburst = 32;
> +
> +	sdev->sec_hclk = devm_clk_get(dev, "sec_hclk");
> +	if (IS_ERR(sdev->sec_hclk)) {
> +		dev_err(dev, "Failed to get sec_hclk.\n");
> +		return PTR_ERR(sdev->sec_hclk);

return dev_err_probe

> +	}
> +
> +	sdev->sec_ahb = devm_clk_get(dev, "sec_ahb");
> +	if (IS_ERR(sdev->sec_ahb)) {
> +		dev_err(dev, "Failed to get sec_ahb.\n");
> +		return PTR_ERR(sdev->sec_ahb);

return dev_err_probe

> +	}
> +
> +	sdev->rst_hresetn = devm_reset_control_get_shared(sdev->dev, "sec_hre");
> +	if (IS_ERR(sdev->rst_hresetn)) {
> +		dev_err(sdev->dev, "Failed to get sec_hre.\n");
> +		return PTR_ERR(sdev->rst_hresetn);

return dev_err_probe

> +	}
> +
> +	clk_prepare_enable(sdev->sec_hclk);
> +	clk_prepare_enable(sdev->sec_ahb);
> +	reset_control_deassert(sdev->rst_hresetn);
> +
> +	platform_set_drvdata(pdev, sdev);
> +
> +	spin_lock(&dev_list.lock);
> +	list_add(&sdev->list, &dev_list.dev_list);
> +	spin_unlock(&dev_list.lock);
> +
> +	if (sdev->use_dma) {
> +		ret = starfive_dma_init(sdev);
> +		if (ret) {
> +			dev_err(dev, "Failed to initialize DMA channel.\n");
> +			goto err_dma_init;
> +		}
> +	}
> +
> +	pages = get_order(STARFIVE_MSG_BUFFER_SIZE);
> +
> +	sdev->pages_count = pages >> 1;
> +	sdev->data_buf_len = STARFIVE_MSG_BUFFER_SIZE >> 1;
> +
> +	/* Initialize crypto engine */
> +	sdev->engine = crypto_engine_alloc_init(dev, 1);
> +	if (!sdev->engine) {
> +		ret = -ENOMEM;
> +		goto err_engine;
> +	}
> +
> +	ret = crypto_engine_start(sdev->engine);
> +	if (ret)
> +		goto err_engine_start;
> +
> +	dev_info(dev, "Crypto engine started\n");

Drop silly probe success messages.

> +
> +	return 0;
> +
> +err_engine_start:
> +	crypto_engine_exit(sdev->engine);
> +err_engine:
> +	starfive_dma_cleanup(sdev);
> +err_dma_init:
> +	spin_lock(&dev_list.lock);
> +	list_del(&sdev->list);
> +	spin_unlock(&dev_list.lock);
> +
> +	return ret;
> +}
> +
> +static int starfive_cryp_remove(struct platform_device *pdev)
> +{
> +	struct starfive_sec_dev *sdev = platform_get_drvdata(pdev);
> +
> +	if (!sdev)
> +		return -ENODEV;
> +
> +	crypto_engine_stop(sdev->engine);
> +	crypto_engine_exit(sdev->engine);
> +
> +	starfive_dma_cleanup(sdev);
> +
> +	spin_lock(&dev_list.lock);
> +	list_del(&sdev->list);
> +	spin_unlock(&dev_list.lock);
> +
> +	clk_disable_unprepare(sdev->sec_hclk);
> +	clk_disable_unprepare(sdev->sec_ahb);
> +	reset_control_assert(sdev->rst_hresetn);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int starfive_cryp_runtime_suspend(struct device *dev)
> +{
> +	struct starfive_sec_dev *sdev = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(sdev->sec_ahb);
> +	clk_disable_unprepare(sdev->sec_hclk);
> +
> +	return 0;
> +}
> +
> +static int starfive_cryp_runtime_resume(struct device *dev)
> +{
> +	struct starfive_sec_dev *sdev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(sdev->sec_ahb);
> +	if (ret) {
> +		dev_err(sdev->dev, "Failed to prepare_enable sec_ahb clock\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(sdev->sec_hclk);
> +	if (ret) {
> +		dev_err(sdev->dev, "Failed to prepare_enable sec_hclk clock\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops starfive_cryp_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(starfive_cryp_runtime_suspend,
> +			   starfive_cryp_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver starfive_cryp_driver = {
> +	.probe  = starfive_cryp_probe,
> +	.remove = starfive_cryp_remove,
> +	.driver = {
> +		.name           = DRIVER_NAME,
> +		.pm		= &starfive_cryp_pm_ops,
> +		.of_match_table = starfive_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(starfive_cryp_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("StarFive hardware crypto acceleration");
> diff --git a/drivers/crypto/starfive/starfive-regs.h b/drivers/crypto/starfive/starfive-regs.h
> new file mode 100644
> index 000000000000..0d680cb1f502
> --- /dev/null
> +++ b/drivers/crypto/starfive/starfive-regs.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __STARFIVE_REGS_H__
> +#define __STARFIVE_REGS_H__
> +
> +#define STARFIVE_ALG_CR_OFFSET			0x0
> +#define STARFIVE_ALG_FIFO_OFFSET		0x4
> +#define STARFIVE_IE_MASK_OFFSET			0x8
> +#define STARFIVE_IE_FLAG_OFFSET			0xc
> +#define STARFIVE_DMA_IN_LEN_OFFSET		0x10
> +#define STARFIVE_DMA_OUT_LEN_OFFSET		0x14
> +
> +union starfive_alg_cr {
> +	u32 v;
> +	struct {
> +		u32 start			:1;
> +		u32 aes_dma_en			:1;
> +		u32 rsvd_0			:1;
> +		u32 hash_dma_en			:1;
> +		u32 alg_done			:1;
> +		u32 rsvd_1			:3;
> +		u32 clear			:1;
> +		u32 rsvd_2			:23;
> +	};
> +};
> +
> +#endif
> diff --git a/drivers/crypto/starfive/starfive-str.h b/drivers/crypto/starfive/starfive-str.h
> new file mode 100644
> index 000000000000..4ba3c56f0573
> --- /dev/null
> +++ b/drivers/crypto/starfive/starfive-str.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __STARFIVE_STR_H__
> +#define __STARFIVE_STR_H__
> +
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +
> +#include <crypto/engine.h>
> +
> +#include "starfive-regs.h"
> +
> +#define STARFIVE_MSG_BUFFER_SIZE		SZ_16K
> +
> +struct starfive_sec_ctx {
> +	struct crypto_engine_ctx		enginectx;
> +	struct starfive_sec_dev			*sdev;
> +
> +	u8					*buffer;
> +};
> +
> +struct starfive_sec_dev {
> +	struct list_head			list;
> +	struct device				*dev;
> +
> +	struct clk				*sec_hclk;
> +	struct clk				*sec_ahb;
> +	struct reset_control			*rst_hresetn;
> +
> +	void __iomem				*io_base;
> +	phys_addr_t				io_phys_base;
> +
> +	size_t					data_buf_len;
> +	int					pages_count;
> +	u32					use_side_channel_mitigation;
> +	u32					use_dma;
> +	u32					dma_maxburst;
> +	struct dma_chan				*sec_xm_m;
> +	struct dma_chan				*sec_xm_p;
> +	struct dma_slave_config			cfg_in;
> +	struct dma_slave_config			cfg_out;
> +	struct completion			sec_comp_m;
> +	struct completion			sec_comp_p;
> +
> +	struct crypto_engine			*engine;
> +
> +	union starfive_alg_cr			alg_cr;
> +};
> +
> +static inline u32 starfive_sec_read(struct starfive_sec_dev *sdev, u32 offset)
> +{
> +	return __raw_readl(sdev->io_base + offset);

I don't think these read/write wrappers help anyhow...


Best regards,
Krzysztof
Krzysztof Kozlowski Nov. 30, 2022, 1:21 p.m. UTC | #3
On 30/11/2022 06:52, Jia Jie Ho wrote:
> Adding StarFive crypto IP and DMA controller node
> to VisionFive 2 SoC.
> 
> Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> ---
>  .../jh7110-starfive-visionfive-v2.dts         |  8 +++++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 36 +++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> index 450e920236a5..da2aa4d597f3 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> @@ -115,3 +115,11 @@ &tdm_ext {
>  &mclk_ext {
>  	clock-frequency = <49152000>;
>  };
> +
> +&sec_dma {
> +	status = "okay";
> +};
> +
> +&crypto {
> +	status = "okay";
> +};
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index 4ac159d79d66..745a5650882c 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -455,5 +455,41 @@ uart5: serial@12020000 {
>  			reg-shift = <2>;
>  			status = "disabled";
>  		};
> +
> +		sec_dma: sec_dma@16008000 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

No underscores in node names.

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).


Best regards,
Krzysztof
JiaJie Ho Dec. 1, 2022, 6:17 a.m. UTC | #4
> -----Original Message-----
> From: Conor.Dooley@microchip.com <Conor.Dooley@microchip.com>
> Sent: Wednesday, November 30, 2022 5:31 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: robh+dt@kernel.org; herbert@gondor.apana.org.au; linux-
> crypto@vger.kernel.org; kernel@esmil.dk; davem@davemloft.net;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> riscv@lists.infradead.org; krzysztof.kozlowski+dt@linaro.org
> Subject: Re: [PATCH 6/6] riscv: dts: starfive: Add crypto and DMA node for
> VisionFive 2
> 
> Hey Jia Jie Ho,
> 
> On 30/11/2022 05:52, Jia Jie Ho wrote:
> > [You don't often get email from jiajie.ho@starfivetech.com. Learn why
> > this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > Adding StarFive crypto IP and DMA controller node to VisionFive 2 SoC.
> >
> > Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> 
> Out of curiosity, what was Huan Feng's contribution to this patch?
> Did they co-develop it, or is there some other reason?
> 
Hi Conor, 
Yes, Huan Feng co-developed this driver.

> > ---
> >   .../jh7110-starfive-visionfive-v2.dts         |  8 +++++
> >   arch/riscv/boot/dts/starfive/jh7110.dtsi      | 36 +++++++++++++++++++
> 
> I figure Emil will likely see anyway, but whenever you get actual review
> comments and send a v2 - please don't drop people that get_maintainer.pl
> tells you are responsible for the code in question.
> 
I will include everyone involved when sending the new patch series.

> >   2 files changed, 44 insertions(+)
> >
> > diff --git
> > a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > index 450e920236a5..da2aa4d597f3 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > @@ -115,3 +115,11 @@ &tdm_ext {
> >   &mclk_ext {
> >          clock-frequency = <49152000>;
> >   };
> > +
> > +&sec_dma {
> > +       status = "okay";
> > +};
> > +
> > +&crypto {
> > +       status = "okay";
> > +};
> 
> In what scenario would you not want to have these enabled?
> 
> Thanks,
> Conor.
> 
These drivers are always enabled. 
Is everything ok with the dts node entries?
Thank you for spending time reviewing and providing suggestions for this patch.

Regards,
Jia Jie

> > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > index 4ac159d79d66..745a5650882c 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > @@ -455,5 +455,41 @@ uart5: serial@12020000 {
> >                          reg-shift = <2>;
> >                          status = "disabled";
> >                  };
> > +
> > +               sec_dma: sec_dma@16008000 {
> > +                       compatible = "arm,pl080", "arm,primecell";
> > +                       arm,primecell-periphid = <0x00041080>;
> > +                       reg = <0x0 0x16008000 0x0 0x4000>;
> > +                       reg-names = "sec_dma";
> > +                       interrupts = <29>;
> > +                       clocks = <&stgcrg JH7110_STGCLK_SEC_HCLK>,
> > +                                <&stgcrg JH7110_STGCLK_SEC_MISCAHB>;
> > +                       clock-names = "sec_hclk","apb_pclk";
> > +                       resets = <&stgcrg JH7110_STGRST_SEC_TOP_HRESETN>;
> > +                       reset-names = "sec_hre";
> > +                       lli-bus-interface-ahb1;
> > +                       mem-bus-interface-ahb1;
> > +                       memcpy-burst-size = <256>;
> > +                       memcpy-bus-width = <32>;
> > +                       #dma-cells = <2>;
> > +                       status = "disabled";
> > +               };
> > +
> > +               crypto: crypto@16000000 {
> > +                       compatible = "starfive,jh7110-crypto";
> > +                       reg = <0x0 0x16000000 0x0 0x4000>;
> > +                       reg-names = "secreg";
> > +                       clocks = <&stgcrg JH7110_STGCLK_SEC_HCLK>,
> > +                                <&stgcrg JH7110_STGCLK_SEC_MISCAHB>;
> > +                       clock-names = "sec_hclk","sec_ahb";
> > +                       resets = <&stgcrg JH7110_STGRST_SEC_TOP_HRESETN>;
> > +                       reset-names = "sec_hre";
> > +                       enable-side-channel-mitigation;
> > +                       enable-dma;
> > +                       dmas = <&sec_dma 1 2>,
> > +                              <&sec_dma 0 2>;
> > +                       dma-names = "sec_m","sec_p";
> > +                       status = "disabled";
> > +               };
> >          };
> >   };
> > --
> > 2.25.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
JiaJie Ho Dec. 1, 2022, 6:52 a.m. UTC | #5
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, November 30, 2022 9:16 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>; Herbert Xu
> <herbert@gondor.apana.org.au>; David S . Miller <davem@davemloft.net>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH 1/6] crypto: starfive - Add StarFive crypto engine
> support
> 

Hi Krzysztof Kozlowski,

> On 30/11/2022 06:52, Jia Jie Ho wrote:
> > Adding device probe and DMA init for StarFive hardware crypto engine.
> >
> > Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> > ---
> 
> 
> > +
> > +static const struct of_device_id starfive_dt_ids[] = {
> > +	{ .compatible = "starfive,jh7110-crypto", .data = NULL},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, starfive_dt_ids);
> 
> Keep your table close to its usage, just like in many other drivers.
> 

Moving this in the next version.

> > +
> > +static int starfive_dma_init(struct starfive_sec_dev *sdev) {
> > +	dma_cap_mask_t mask;
> > +	int err;
> > +
> > +	sdev->sec_xm_m = NULL;
> > +	sdev->sec_xm_p = NULL;
> > +
> > +	dma_cap_zero(mask);
> > +	dma_cap_set(DMA_SLAVE, mask);
> > +
> > +	sdev->sec_xm_m = dma_request_chan(sdev->dev, "sec_m");
> > +	if (IS_ERR(sdev->sec_xm_m)) {
> > +		dev_err(sdev->dev, "sec_m dma channel request failed.\n");
> 
> return dev_err_probe
> 

I'll replace this and the following return error with dev_err_probe .

> > +		return PTR_ERR(sdev->sec_xm_m);
> > +	}
> > +
> > +	sdev->sec_xm_p = dma_request_chan(sdev->dev, "sec_p");
> > +	if (IS_ERR(sdev->sec_xm_p)) {
> > +		dev_err(sdev->dev, "sec_p dma channel request failed.\n");
> 
> dev_err_probe
> 
> > +		goto err_dma_out;
> > +	}
> > +
> > +	init_completion(&sdev->sec_comp_m);
> > +	init_completion(&sdev->sec_comp_p);
> > +
> > +	return 0;
> > +
> > +err_dma_out:
> > +	dma_release_channel(sdev->sec_xm_m);
> 
> I don't think you tested it. Not even built with proper tools liek smatch,
> sparse, W=1. Where do you set err?
> 

I'll compile this driver with proper tools before submitting the new patch series.

> > +
> > +	return err;
> > +}
> > +
> > +static void starfive_dma_cleanup(struct starfive_sec_dev *sdev) {
> > +	dma_release_channel(sdev->sec_xm_p);
> > +	dma_release_channel(sdev->sec_xm_m);
> > +}
> > +
> > +static int starfive_cryp_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct starfive_sec_dev *sdev;
> > +	struct resource *res;
> > +	int pages = 0;
> > +	int ret;
> > +
> > +	sdev = devm_kzalloc(dev, sizeof(*sdev), GFP_KERNEL);
> > +	if (!sdev)
> > +		return -ENOMEM;
> > +
> > +	sdev->dev = dev;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "secreg");
> > +	sdev->io_base = devm_ioremap_resource(dev, res);
> 
> I think there is a wrapper for both calls...
> 

I'll replace this with devm_platform_ioremap_resource_byname()

> > +
> > +	if (IS_ERR(sdev->io_base))
> > +		return PTR_ERR(sdev->io_base);
> > +
> > +	sdev->use_side_channel_mitigation =
> > +		device_property_read_bool(dev, "enable-side-channel-
> mitigation");
> > +	sdev->use_dma = device_property_read_bool(dev, "enable-dma");
> > +	sdev->dma_maxburst = 32;
> > +
> > +	sdev->sec_hclk = devm_clk_get(dev, "sec_hclk");
> > +	if (IS_ERR(sdev->sec_hclk)) {
> > +		dev_err(dev, "Failed to get sec_hclk.\n");
> > +		return PTR_ERR(sdev->sec_hclk);
> 
> return dev_err_probe
> 
> > +	}
> > +
> > +	sdev->sec_ahb = devm_clk_get(dev, "sec_ahb");
> > +	if (IS_ERR(sdev->sec_ahb)) {
> > +		dev_err(dev, "Failed to get sec_ahb.\n");
> > +		return PTR_ERR(sdev->sec_ahb);
> 
> return dev_err_probe
> 
> > +	}
> > +
> > +	sdev->rst_hresetn = devm_reset_control_get_shared(sdev->dev,
> "sec_hre");
> > +	if (IS_ERR(sdev->rst_hresetn)) {
> > +		dev_err(sdev->dev, "Failed to get sec_hre.\n");
> > +		return PTR_ERR(sdev->rst_hresetn);
> 
> return dev_err_probe
> 
> > +	}
> > +
> > +	clk_prepare_enable(sdev->sec_hclk);
> > +	clk_prepare_enable(sdev->sec_ahb);
> > +	reset_control_deassert(sdev->rst_hresetn);
> > +
> > +	platform_set_drvdata(pdev, sdev);
> > +
> > +	spin_lock(&dev_list.lock);
> > +	list_add(&sdev->list, &dev_list.dev_list);
> > +	spin_unlock(&dev_list.lock);
> > +
> > +	if (sdev->use_dma) {
> > +		ret = starfive_dma_init(sdev);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to initialize DMA channel.\n");
> > +			goto err_dma_init;
> > +		}
> > +	}
> > +
> > +	pages = get_order(STARFIVE_MSG_BUFFER_SIZE);
> > +
> > +	sdev->pages_count = pages >> 1;
> > +	sdev->data_buf_len = STARFIVE_MSG_BUFFER_SIZE >> 1;
> > +
> > +	/* Initialize crypto engine */
> > +	sdev->engine = crypto_engine_alloc_init(dev, 1);
> > +	if (!sdev->engine) {
> > +		ret = -ENOMEM;
> > +		goto err_engine;
> > +	}
> > +
> > +	ret = crypto_engine_start(sdev->engine);
> > +	if (ret)
> > +		goto err_engine_start;
> > +
> > +	dev_info(dev, "Crypto engine started\n");
> 
> Drop silly probe success messages.
> 

Removing this in the next version.

> > +
> > +	return 0;
> > +
> > +err_engine_start:
> > +	crypto_engine_exit(sdev->engine);
> > +err_engine:
> > +	starfive_dma_cleanup(sdev);
> > +err_dma_init:
> > +	spin_lock(&dev_list.lock);
> > +	list_del(&sdev->list);
> > +	spin_unlock(&dev_list.lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int starfive_cryp_remove(struct platform_device *pdev) {
> > +	struct starfive_sec_dev *sdev = platform_get_drvdata(pdev);
> > +
> > +	if (!sdev)
> > +		return -ENODEV;
> > +
> > +	crypto_engine_stop(sdev->engine);
> > +	crypto_engine_exit(sdev->engine);
> > +
> > +	starfive_dma_cleanup(sdev);
> > +
> > +	spin_lock(&dev_list.lock);
> > +	list_del(&sdev->list);
> > +	spin_unlock(&dev_list.lock);
> > +
> > +	clk_disable_unprepare(sdev->sec_hclk);
> > +	clk_disable_unprepare(sdev->sec_ahb);
> > +	reset_control_assert(sdev->rst_hresetn);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int starfive_cryp_runtime_suspend(struct device *dev) {
> > +	struct starfive_sec_dev *sdev = dev_get_drvdata(dev);
> > +
> > +	clk_disable_unprepare(sdev->sec_ahb);
> > +	clk_disable_unprepare(sdev->sec_hclk);
> > +
> > +	return 0;
> > +}
> > +
> > +static int starfive_cryp_runtime_resume(struct device *dev) {
> > +	struct starfive_sec_dev *sdev = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(sdev->sec_ahb);
> > +	if (ret) {
> > +		dev_err(sdev->dev, "Failed to prepare_enable sec_ahb
> clock\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(sdev->sec_hclk);
> > +	if (ret) {
> > +		dev_err(sdev->dev, "Failed to prepare_enable sec_hclk
> clock\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops starfive_cryp_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > +				pm_runtime_force_resume)
> > +	SET_RUNTIME_PM_OPS(starfive_cryp_runtime_suspend,
> > +			   starfive_cryp_runtime_resume, NULL) };
> > +
> > +static struct platform_driver starfive_cryp_driver = {
> > +	.probe  = starfive_cryp_probe,
> > +	.remove = starfive_cryp_remove,
> > +	.driver = {
> > +		.name           = DRIVER_NAME,
> > +		.pm		= &starfive_cryp_pm_ops,
> > +		.of_match_table = starfive_dt_ids,
> > +	},
> > +};
> > +
> > +module_platform_driver(starfive_cryp_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("StarFive hardware crypto acceleration");
> > diff --git a/drivers/crypto/starfive/starfive-regs.h
> > b/drivers/crypto/starfive/starfive-regs.h
> > new file mode 100644
> > index 000000000000..0d680cb1f502
> > --- /dev/null
> > +++ b/drivers/crypto/starfive/starfive-regs.h
> > @@ -0,0 +1,26 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */ #ifndef __STARFIVE_REGS_H__
> > +#define __STARFIVE_REGS_H__
> > +
> > +#define STARFIVE_ALG_CR_OFFSET			0x0
> > +#define STARFIVE_ALG_FIFO_OFFSET		0x4
> > +#define STARFIVE_IE_MASK_OFFSET			0x8
> > +#define STARFIVE_IE_FLAG_OFFSET			0xc
> > +#define STARFIVE_DMA_IN_LEN_OFFSET		0x10
> > +#define STARFIVE_DMA_OUT_LEN_OFFSET		0x14
> > +
> > +union starfive_alg_cr {
> > +	u32 v;
> > +	struct {
> > +		u32 start			:1;
> > +		u32 aes_dma_en			:1;
> > +		u32 rsvd_0			:1;
> > +		u32 hash_dma_en			:1;
> > +		u32 alg_done			:1;
> > +		u32 rsvd_1			:3;
> > +		u32 clear			:1;
> > +		u32 rsvd_2			:23;
> > +	};
> > +};
> > +
> > +#endif
> > diff --git a/drivers/crypto/starfive/starfive-str.h
> > b/drivers/crypto/starfive/starfive-str.h
> > new file mode 100644
> > index 000000000000..4ba3c56f0573
> > --- /dev/null
> > +++ b/drivers/crypto/starfive/starfive-str.h
> > @@ -0,0 +1,74 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */ #ifndef __STARFIVE_STR_H__
> > +#define __STARFIVE_STR_H__
> > +
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/dmaengine.h>
> > +
> > +#include <crypto/engine.h>
> > +
> > +#include "starfive-regs.h"
> > +
> > +#define STARFIVE_MSG_BUFFER_SIZE		SZ_16K
> > +
> > +struct starfive_sec_ctx {
> > +	struct crypto_engine_ctx		enginectx;
> > +	struct starfive_sec_dev			*sdev;
> > +
> > +	u8					*buffer;
> > +};
> > +
> > +struct starfive_sec_dev {
> > +	struct list_head			list;
> > +	struct device				*dev;
> > +
> > +	struct clk				*sec_hclk;
> > +	struct clk				*sec_ahb;
> > +	struct reset_control			*rst_hresetn;
> > +
> > +	void __iomem				*io_base;
> > +	phys_addr_t				io_phys_base;
> > +
> > +	size_t					data_buf_len;
> > +	int					pages_count;
> > +	u32					use_side_channel_mitigation;
> > +	u32					use_dma;
> > +	u32					dma_maxburst;
> > +	struct dma_chan				*sec_xm_m;
> > +	struct dma_chan				*sec_xm_p;
> > +	struct dma_slave_config			cfg_in;
> > +	struct dma_slave_config			cfg_out;
> > +	struct completion			sec_comp_m;
> > +	struct completion			sec_comp_p;
> > +
> > +	struct crypto_engine			*engine;
> > +
> > +	union starfive_alg_cr			alg_cr;
> > +};
> > +
> > +static inline u32 starfive_sec_read(struct starfive_sec_dev *sdev,
> > +u32 offset) {
> > +	return __raw_readl(sdev->io_base + offset);
> 
> I don't think these read/write wrappers help anyhow...
> 

These wrappers are used by the crypto primitives in this patch series.
I'll move these to subsequent patches when they are first used.

Thank you for spending time reviewing and providing helpful comments
for this driver.

Regards,
Jia Jie
JiaJie Ho Dec. 1, 2022, 7:25 a.m. UTC | #6
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, November 30, 2022 9:22 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>; Herbert Xu
> <herbert@gondor.apana.org.au>; David S . Miller <davem@davemloft.net>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH 6/6] riscv: dts: starfive: Add crypto and DMA node for
> VisionFive 2
> 
> On 30/11/2022 06:52, Jia Jie Ho wrote:
> > Adding StarFive crypto IP and DMA controller node to VisionFive 2 SoC.
> >
> > Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> > ---
> >  .../jh7110-starfive-visionfive-v2.dts         |  8 +++++
> >  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 36 +++++++++++++++++++
> >  2 files changed, 44 insertions(+)
> >
> > diff --git
> > a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > index 450e920236a5..da2aa4d597f3 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > @@ -115,3 +115,11 @@ &tdm_ext {
> >  &mclk_ext {
> >  	clock-frequency = <49152000>;
> >  };
> > +
> > +&sec_dma {
> > +	status = "okay";
> > +};
> > +
> > +&crypto {
> > +	status = "okay";
> > +};
> > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > index 4ac159d79d66..745a5650882c 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > @@ -455,5 +455,41 @@ uart5: serial@12020000 {
> >  			reg-shift = <2>;
> >  			status = "disabled";
> >  		};
> > +
> > +		sec_dma: sec_dma@16008000 {
> 
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-
> devicetree-basics.html#generic-names-recommendation
> 
> No underscores in node names.
> 
> Does not look like you tested the DTS against bindings. Please run `make
> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
> for instructions).

I'll fix the node name and run dtbs_check for the revised series.
Really appreciate your time and effort reviewing this patch.

Thanks,
Jia Jie
Krzysztof Kozlowski Dec. 1, 2022, 9:28 a.m. UTC | #7
On 01/12/2022 07:52, JiaJie Ho wrote:

>>> +
>>> +static inline u32 starfive_sec_read(struct starfive_sec_dev *sdev,
>>> +u32 offset) {
>>> +	return __raw_readl(sdev->io_base + offset);
>>
>> I don't think these read/write wrappers help anyhow...
>>
> 
> These wrappers are used by the crypto primitives in this patch series.
> I'll move these to subsequent patches when they are first used.
> 
> Thank you for spending time reviewing and providing helpful comments
> for this driver.
> 

Just drop the wrappers. I said they do not help and your answer "are
used" does not explain anything. If you insist on keeping them, please
explain what are the benefits except more code and more
indirections/layers making it more difficult to read?

Best regards,
Krzysztof
JiaJie Ho Dec. 1, 2022, 9:43 a.m. UTC | #8
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, December 1, 2022 5:29 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>; Herbert Xu
> <herbert@gondor.apana.org.au>; David S . Miller <davem@davemloft.net>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH 1/6] crypto: starfive - Add StarFive crypto engine
> support
> 
> On 01/12/2022 07:52, JiaJie Ho wrote:
> 
> >>> +
> >>> +static inline u32 starfive_sec_read(struct starfive_sec_dev *sdev,
> >>> +u32 offset) {
> >>> +	return __raw_readl(sdev->io_base + offset);
> >>
> >> I don't think these read/write wrappers help anyhow...
> >>
> >
> > These wrappers are used by the crypto primitives in this patch series.
> > I'll move these to subsequent patches when they are first used.
> >
> > Thank you for spending time reviewing and providing helpful comments
> > for this driver.
> >
> 
> Just drop the wrappers. I said they do not help and your answer "are used"
> does not explain anything. If you insist on keeping them, please explain what
> are the benefits except more code and more indirections/layers making it
> more difficult to read?
> 

I'll drop these in the next version.
Thanks again for the suggestion.

Regards,
Jia Jie
Conor Dooley Dec. 1, 2022, 6:04 p.m. UTC | #9
On Thu, Dec 01, 2022 at 06:17:26AM +0000, JiaJie Ho wrote:
> > -----Original Message-----
> > From: Conor.Dooley@microchip.com <Conor.Dooley@microchip.com>

> > Hey Jia Jie Ho,
> > 
> > On 30/11/2022 05:52, Jia Jie Ho wrote:
> > > [You don't often get email from jiajie.ho@starfivetech.com. Learn why
> > > this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > > the content is safe
> > >
> > > Adding StarFive crypto IP and DMA controller node to VisionFive 2 SoC.
> > >
> > > Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> > 
> > Out of curiosity, what was Huan Feng's contribution to this patch?
> > Did they co-develop it, or is there some other reason?
> > 
> Hi Conor, 
> Yes, Huan Feng co-developed this driver.

In that case, the SoB block should look like:

Co-developed-by: Huan Feng <huan.feng@starfivetech.com>
Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>

Similarly for any other patches they may have co-developed :)

> > >
> > > diff --git
> > > a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > > b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > > index 450e920236a5..da2aa4d597f3 100644
> > > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > > @@ -115,3 +115,11 @@ &tdm_ext {
> > >   &mclk_ext {
> > >          clock-frequency = <49152000>;
> > >   };
> > > +
> > > +&sec_dma {
> > > +       status = "okay";
> > > +};
> > > +
> > > +&crypto {
> > > +       status = "okay";
> > > +};
> > 
> > In what scenario would you not want to have these enabled?

> These drivers are always enabled.
> Is everything ok with the dts node entries?

If the hardware is always present, why not drop the "disabled" in
jh7110.dtsi & these two entries marking them as "okay" in the .dts?

> > > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > > b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > > index 4ac159d79d66..745a5650882c 100644
> > > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > > @@ -455,5 +455,41 @@ uart5: serial@12020000 {
> > >                          reg-shift = <2>;
> > >                          status = "disabled";
> > >                  };
> > > +
> > > +               sec_dma: sec_dma@16008000 {

> > > +                       status = "disabled";

> > > +               };
> > > +
> > > +               crypto: crypto@16000000 {

> > > +                       status = "disabled";

> > > +               };
> > >          };
> > >   };

Thanks,
Conor.
JiaJie Ho Dec. 6, 2022, 3:55 a.m. UTC | #10
> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Friday, December 2, 2022 2:04 AM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: Conor.Dooley@microchip.com; robh+dt@kernel.org;
> herbert@gondor.apana.org.au; linux-crypto@vger.kernel.org;
> kernel@esmil.dk; davem@davemloft.net; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-riscv@lists.infradead.org;
> krzysztof.kozlowski+dt@linaro.org
> Subject: Re: [PATCH 6/6] riscv: dts: starfive: Add crypto and DMA node for
> VisionFive 2
> 
> On Thu, Dec 01, 2022 at 06:17:26AM +0000, JiaJie Ho wrote:
> > > -----Original Message-----
> > > From: Conor.Dooley@microchip.com <Conor.Dooley@microchip.com>
> 
> In that case, the SoB block should look like:
> 
> Co-developed-by: Huan Feng <huan.feng@starfivetech.com>
> Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
> 
> Similarly for any other patches they may have co-developed :)
> 

I'll add these in the v2.

> If the hardware is always present, why not drop the "disabled" in jh7110.dtsi
> & these two entries marking them as "okay" in the .dts?
> 

Okay, I'll update these too. 
Thanks again for the suggestions.

Best regards,
Jia Jie
JiaJie Ho Dec. 8, 2022, 9:09 a.m. UTC | #11
> -----Original Message-----
> From: JiaJie Ho <jiajie.ho@starfivetech.com>
> Sent: Wednesday, November 30, 2022 1:52 PM
> To: Herbert Xu <herbert@gondor.apana.org.au>; David S . Miller
> <davem@davemloft.net>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org; JiaJie Ho
> <jiajie.ho@starfivetech.com>
> Subject: [PATCH 0/6] crypto: starfive: Add driver for cryptographic engine
> 
> This patch series adds kernel driver support for Starfive crypto engine.
> The engine supports hardware acceleration for HMAC/hash functions, AES
> block cipher operations and RSA. The first patch adds basic driver for device
> probe and DMA init. The subsequent patches adds supported crypto
> primitives to the driver which include hash functions, AES and RSA. Patch 5
> adds documentation to describe device tree bindings and the last patch adds
> device node to VisionFive 2 dts.
> 
> The driver has been tested with crypto selftest and additional test.
> 
> This patch series depends on the following patches:
> https://patchwork.kernel.org/project/linux-
> riscv/cover/20221118010627.70576-1-hal.feng@starfivetech.com/
> https://patchwork.kernel.org/project/linux-
> riscv/cover/20221118011714.70877-1-hal.feng@starfivetech.com/
> 
> Jia Jie Ho (6):
>   crypto: starfive - Add StarFive crypto engine support
>   crypto: starfive - Add hash and HMAC support
>   crypto: starfive - Add AES skcipher and aead support
>   crypto: starfive - Add Public Key algo support
>   dt-bindings: crypto: Add bindings for Starfive crypto driver
>   riscv: dts: starfive: Add crypto and DMA node for VisionFive 2
> 

Hi Herbert/David,

Could you please help to review and provide comments on this patch series?
Thank you in advance.

Best regards,
Jia Jie
Krzysztof Kozlowski Dec. 8, 2022, 9:28 a.m. UTC | #12
On 08/12/2022 10:09, JiaJie Ho wrote:
>>
>> The driver has been tested with crypto selftest and additional test.
>>
>> This patch series depends on the following patches:
>> https://patchwork.kernel.org/project/linux-
>> riscv/cover/20221118010627.70576-1-hal.feng@starfivetech.com/
>> https://patchwork.kernel.org/project/linux-
>> riscv/cover/20221118011714.70877-1-hal.feng@starfivetech.com/
>>
>> Jia Jie Ho (6):
>>   crypto: starfive - Add StarFive crypto engine support
>>   crypto: starfive - Add hash and HMAC support
>>   crypto: starfive - Add AES skcipher and aead support
>>   crypto: starfive - Add Public Key algo support
>>   dt-bindings: crypto: Add bindings for Starfive crypto driver
>>   riscv: dts: starfive: Add crypto and DMA node for VisionFive 2
>>
> 
> Hi Herbert/David,
> 
> Could you please help to review and provide comments on this patch series?
> Thank you in advance.

You received some comments so the expectation is to send a v2.

Best regards,
Krzysztof
JiaJie Ho Dec. 8, 2022, 9:35 a.m. UTC | #13
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, December 8, 2022 5:28 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>; Herbert Xu
> <herbert@gondor.apana.org.au>; David S . Miller <davem@davemloft.net>;
> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH 0/6] crypto: starfive: Add driver for cryptographic engine
> 
> On 08/12/2022 10:09, JiaJie Ho wrote:
> >
> > Hi Herbert/David,
> >
> > Could you please help to review and provide comments on this patch series?
> > Thank you in advance.
> 
> You received some comments so the expectation is to send a v2.
> 

Sure, I'll do that then.

Thanks
Jia Jie
Palmer Dabbelt Dec. 13, 2022, 6:20 a.m. UTC | #14
On Thu, 08 Dec 2022 01:35:10 PST (-0800), jiajie.ho@starfivetech.com wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Thursday, December 8, 2022 5:28 PM
>> To: JiaJie Ho <jiajie.ho@starfivetech.com>; Herbert Xu
>> <herbert@gondor.apana.org.au>; David S . Miller <davem@davemloft.net>;
>> Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>
>> Cc: linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
>> Subject: Re: [PATCH 0/6] crypto: starfive: Add driver for cryptographic engine
>> 
>> On 08/12/2022 10:09, JiaJie Ho wrote:
>> >
>> > Hi Herbert/David,
>> >
>> > Could you please help to review and provide comments on this patch series?
>> > Thank you in advance.
>> 
>> You received some comments so the expectation is to send a v2.
>> 
> 
> Sure, I'll do that then.

Not sure if I missed it, but I can't find a v2.
JiaJie Ho Dec. 13, 2022, 6:32 a.m. UTC | #15
> -----Original Message-----
> From: Palmer Dabbelt <palmer@dabbelt.com>
> Sent: Tuesday, December 13, 2022 2:20 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: krzysztof.kozlowski@linaro.org; herbert@gondor.apana.org.au;
> davem@davemloft.net; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; linux-crypto@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> riscv@lists.infradead.org
> Subject: RE: [PATCH 0/6] crypto: starfive: Add driver for cryptographic engine
> 
> >>
> >> You received some comments so the expectation is to send a v2.
> >>
> >
> > Sure, I'll do that then.
> 
> Not sure if I missed it, but I can't find a v2.

Hi Palmer,

I'm still working on the patches.  I'll send them out soon.

Thanks,
Jia Jie