diff mbox series

[6/6] riscv: dts: starfive: Add crypto and DMA node for VisionFive 2

Message ID 20221130055214.2416888-7-jiajie.ho@starfivetech.com
State New
Headers
Series crypto: starfive: Add driver for cryptographic engine |

Commit Message

JiaJie Ho Nov. 30, 2022, 5:52 a.m. UTC 
  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(+)

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:21 p.m. UTC | #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).


Best regards,
Krzysztof
JiaJie Ho Dec. 1, 2022, 6:17 a.m. UTC | #3 
> -----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, 7:25 a.m. UTC | #4 
> -----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
Conor Dooley Dec. 1, 2022, 6:04 p.m. UTC | #5 
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 | #6 
> -----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
diff mbox series

Patch

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 {
+			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";
+		};
 	};
 };