| Message ID | 20230405072836.1690248-1-bhupesh.sharma@linaro.org |
|---|---|
| Headers | show |
| Series | arm64: qcom: Enable Crypto Engine for a few Qualcomm SoCs | expand |
On 5.04.2023 09:28, Bhupesh Sharma wrote: > Add crypto engine (CE) and CE BAM related nodes and definitions to > 'sm6115.dtsi'. > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Tested-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad > arch/arm64/boot/dts/qcom/sm6115.dtsi | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > index 2a51c938bbcb..ebac026b4cc7 100644 > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > @@ -650,6 +650,28 @@ usb_hsphy: phy@1613000 { > status = "disabled"; > }; > > + cryptobam: dma-controller@1b04000 { > + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0"; > + reg = <0x0 0x01b04000 0x0 0x24000>; > + interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>; > + #dma-cells = <1>; > + qcom,ee = <0>; > + qcom,controlled-remotely; > + num-channels = <8>; > + qcom,num-ees = <2>; > + iommus = <&apps_smmu 0x94 0x11>, > + <&apps_smmu 0x96 0x11>; > + }; > + > + crypto: crypto@1b3a000 { > + compatible = "qcom,sm6115-qce", "qcom,sm8150-qce", "qcom,qce"; > + reg = <0x0 0x01b3a000 0x0 0x6000>; > + dmas = <&cryptobam 6>, <&cryptobam 7>; > + dma-names = "rx", "tx"; > + iommus = <&apps_smmu 0x94 0x11>, > + <&apps_smmu 0x96 0x11>; > + }; > + > qfprom@1b40000 { > compatible = "qcom,sm6115-qfprom", "qcom,qfprom"; > reg = <0x0 0x01b40000 0x0 0x7000>;
On 5.04.2023 09:28, Bhupesh Sharma wrote: > Add crypto engine (CE) and CE BAM related nodes and definitions to > 'sm8250.dtsi'. > > Co-developed-by and Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index 7b78761f2041..2f6b8d4a2d41 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -2222,6 +2222,28 @@ ufs_mem_phy_lanes: phy@1d87400 { > }; > }; > > + cryptobam: dma-controller@1dc4000 { > + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0"; > + reg = <0 0x01dc4000 0 0x24000>; > + interrupts = <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>; > + #dma-cells = <1>; > + qcom,ee = <0>; > + qcom,controlled-remotely; > + iommus = <&apps_smmu 0x594 0x0011>, > + <&apps_smmu 0x596 0x0011>; > + }; > + > + crypto: crypto@1dfa000 { > + compatible = "qcom,sm8250-qce", "qcom,sm8150-qce", "qcom,qce"; > + reg = <0 0x01dfa000 0 0x6000>; > + dmas = <&cryptobam 4>, <&cryptobam 5>; > + dma-names = "rx", "tx"; > + iommus = <&apps_smmu 0x594 0x0011>, > + <&apps_smmu 0x596 0x0011>; > + interconnects = <&aggre2_noc MASTER_CRYPTO_CORE_0 &mc_virt SLAVE_EBI_CH0>; > + interconnect-names = "memory"; Shouldn't we also attach the contexts from qcom_cedev_ns_cb{}? Konrad > + }; > + > tcsr_mutex: hwlock@1f40000 { > compatible = "qcom,tcsr-mutex"; > reg = <0x0 0x01f40000 0x0 0x40000>;
> This patchset enables Crypto Engine support for Qualcomm SoCs like > SM6115, SM8150, SM8250, SM8350 and SM8450. > > Note that: > - SM8250 crypto engine patch utilizes the work already done by myself and > Vladimir. > - SM8350 crypto engine patch utilizes the work already done by Robert. > - SM8450 crypto engine patch utilizes the work already done by Neil. > > Also this patchset is rebased on linux-next/master. These patches tested on top of Linux next-20230406. Tested-by: Anders Roxell <anders.roxell@linaro.org> Tested-by: Linux Kernel Functional Testing <lkft@linaro.org> -- Linaro LKFT https://lkft.linaro.org
On Wed, 05 Apr 2023 12:58:30 +0530, Bhupesh Sharma wrote: > Currently the compatible list available in 'qce' dt-bindings does not > support SM8150 and IPQ4019 SoCs directly which may lead to potential > 'dtbs_check' error(s). > > Fix the same. > > Fixes: 00f3bc2db351 ("dt-bindings: qcom-qce: Add new SoC compatible strings for Qualcomm QCE IP") > Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > Documentation/devicetree/bindings/crypto/qcom-qce.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > Acked-by: Rob Herring <robh@kernel.org>
Hi Bhupesh, Not sure if this is the latest version of this series since it's pretty old but I didn't find a new one. Just came here because you mentioned RB1/RB2 [1] in my bam_dma patch and they don't have any BAM defined upstream yet. [1]: https://lore.kernel.org/linux-arm-msm/CAH=2Ntw0BZH=RGp14mYLhX7D6jV5O5eDKRQbby=uCy85xMDU_g@mail.gmail.com/ On Wed, Apr 05, 2023 at 12:58:32PM +0530, Bhupesh Sharma wrote: > Add crypto engine (CE) and CE BAM related nodes and definitions to > 'sm6115.dtsi'. > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > arch/arm64/boot/dts/qcom/sm6115.dtsi | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > index 2a51c938bbcb..ebac026b4cc7 100644 > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > @@ -650,6 +650,28 @@ usb_hsphy: phy@1613000 { > status = "disabled"; > }; > > + cryptobam: dma-controller@1b04000 { > + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0"; > + reg = <0x0 0x01b04000 0x0 0x24000>; > + interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>; > + #dma-cells = <1>; > + qcom,ee = <0>; > + qcom,controlled-remotely; > + num-channels = <8>; > + qcom,num-ees = <2>; > + iommus = <&apps_smmu 0x94 0x11>, > + <&apps_smmu 0x96 0x11>; > + }; > + > + crypto: crypto@1b3a000 { > + compatible = "qcom,sm6115-qce", "qcom,sm8150-qce", "qcom,qce"; > + reg = <0x0 0x01b3a000 0x0 0x6000>; > + dmas = <&cryptobam 6>, <&cryptobam 7>; > + dma-names = "rx", "tx"; > + iommus = <&apps_smmu 0x94 0x11>, > + <&apps_smmu 0x96 0x11>; Shouldn't you have clocks = <&rpmcc RPM_SMD_CE1_CLK> here to make sure the clock for the crypto engine is on? Your binding patch (PATCH 06/11) says "Crypto Engine block on Qualcomm SoCs SM6115 and QCM2290 do not require clocks strictly" but doesn't say why. Make sure you don't rely on having rpmcc keep unused clocks on permanently. This is the case at the moment, but we would like to change this [2]. Adding new users that rely on this broken behavior would just make this effort even more complicated. If you also add the clock to the cryptobam then you should be able to see the advantage of my bam_dma patch [3]. It allows you to drop "num-channels" and "qcom,num-ees" from the cryptobam in your changes above because it can then be read directly from the BAM registers. Thanks, Stephan [2]: https://lore.kernel.org/linux-arm-msm/20230303-topic-rpmcc_sleep-v2-0-ae80a325fe94@linaro.org/ [3]: https://lore.kernel.org/linux-arm-msm/20230518-bamclk-dt-v1-1-82f738c897d9@gerhold.net/
Hi Stephan, On Fri, 19 May 2023 at 15:40, Stephan Gerhold <stephan@gerhold.net> wrote: > > Hi Bhupesh, > > Not sure if this is the latest version of this series since it's pretty > old but I didn't find a new one. Just came here because you mentioned > RB1/RB2 [1] in my bam_dma patch and they don't have any BAM defined > upstream yet. > > [1]: https://lore.kernel.org/linux-arm-msm/CAH=2Ntw0BZH=RGp14mYLhX7D6jV5O5eDKRQbby=uCy85xMDU_g@mail.gmail.com/ > > On Wed, Apr 05, 2023 at 12:58:32PM +0530, Bhupesh Sharma wrote: > > Add crypto engine (CE) and CE BAM related nodes and definitions to > > 'sm6115.dtsi'. > > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/sm6115.dtsi | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > > index 2a51c938bbcb..ebac026b4cc7 100644 > > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > > @@ -650,6 +650,28 @@ usb_hsphy: phy@1613000 { > > status = "disabled"; > > }; > > > > + cryptobam: dma-controller@1b04000 { > > + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0"; > > + reg = <0x0 0x01b04000 0x0 0x24000>; > > + interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>; > > + #dma-cells = <1>; > > + qcom,ee = <0>; > > + qcom,controlled-remotely; > > + num-channels = <8>; > > + qcom,num-ees = <2>; > > + iommus = <&apps_smmu 0x94 0x11>, > > + <&apps_smmu 0x96 0x11>; > > + }; > > + > > + crypto: crypto@1b3a000 { > > + compatible = "qcom,sm6115-qce", "qcom,sm8150-qce", "qcom,qce"; > > + reg = <0x0 0x01b3a000 0x0 0x6000>; > > + dmas = <&cryptobam 6>, <&cryptobam 7>; > > + dma-names = "rx", "tx"; > > + iommus = <&apps_smmu 0x94 0x11>, > > + <&apps_smmu 0x96 0x11>; > > Shouldn't you have clocks = <&rpmcc RPM_SMD_CE1_CLK> here to make sure > the clock for the crypto engine is on? Your binding patch (PATCH 06/11) > says "Crypto Engine block on Qualcomm SoCs SM6115 and QCM2290 do not > require clocks strictly" but doesn't say why. > > Make sure you don't rely on having rpmcc keep unused clocks on > permanently. This is the case at the moment, but we would like to change > this [2]. Adding new users that rely on this broken behavior would just > make this effort even more complicated. > > If you also add the clock to the cryptobam then you should be able to > see the advantage of my bam_dma patch [3]. It allows you to drop > "num-channels" and "qcom,num-ees" from the cryptobam in your changes > above because it can then be read directly from the BAM registers. Thanks for pointing this out. Actually that's why I was using your patch while testing with RB1/RB2 :) Yes, so the background is that I am preparing a new version of this crypto enablement patchset. Also your assumption about the clocks being turned on by the firmware is true for RB1/RB2 devices, so enabling them via Linux is optional as per Qualcomm enggs. So, I am testing the new patchset right now with 'clock' entries provided in the .dtsi and see if that causes any issue / improvement (etc.) Will come back with updates (and a new version of this patchset) soon. Regards, Bhupesh > Thanks, > Stephan > > [2]: https://lore.kernel.org/linux-arm-msm/20230303-topic-rpmcc_sleep-v2-0-ae80a325fe94@linaro.org/ > [3]: https://lore.kernel.org/linux-arm-msm/20230518-bamclk-dt-v1-1-82f738c897d9@gerhold.net/
On Thu, 6 Apr 2023 at 19:29, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 5.04.2023 09:28, Bhupesh Sharma wrote: > > Add crypto engine (CE) and CE BAM related nodes and definitions to > > 'sm8250.dtsi'. > > > > Co-developed-by and Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/sm8250.dtsi | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > index 7b78761f2041..2f6b8d4a2d41 100644 > > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > @@ -2222,6 +2222,28 @@ ufs_mem_phy_lanes: phy@1d87400 { > > }; > > }; > > > > + cryptobam: dma-controller@1dc4000 { > > + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0"; > > + reg = <0 0x01dc4000 0 0x24000>; > > + interrupts = <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>; > > + #dma-cells = <1>; > > + qcom,ee = <0>; > > + qcom,controlled-remotely; > > + iommus = <&apps_smmu 0x594 0x0011>, > > + <&apps_smmu 0x596 0x0011>; > > + }; > > + > > + crypto: crypto@1dfa000 { > > + compatible = "qcom,sm8250-qce", "qcom,sm8150-qce", "qcom,qce"; > > + reg = <0 0x01dfa000 0 0x6000>; > > + dmas = <&cryptobam 4>, <&cryptobam 5>; > > + dma-names = "rx", "tx"; > > + iommus = <&apps_smmu 0x594 0x0011>, > > + <&apps_smmu 0x596 0x0011>; > > + interconnects = <&aggre2_noc MASTER_CRYPTO_CORE_0 &mc_virt SLAVE_EBI_CH0>; > > + interconnect-names = "memory"; > Shouldn't we also attach the contexts from qcom_cedev_ns_cb{}? Sure, I have fixed this in v7. Will share it shortly. Thanks.
On 19.05.2023 12:22, Bhupesh Sharma wrote: > Hi Stephan, > > On Fri, 19 May 2023 at 15:40, Stephan Gerhold <stephan@gerhold.net> wrote: >> >> Hi Bhupesh, >> >> Not sure if this is the latest version of this series since it's pretty >> old but I didn't find a new one. Just came here because you mentioned >> RB1/RB2 [1] in my bam_dma patch and they don't have any BAM defined >> upstream yet. >> >> [1]: https://lore.kernel.org/linux-arm-msm/CAH=2Ntw0BZH=RGp14mYLhX7D6jV5O5eDKRQbby=uCy85xMDU_g@mail.gmail.com/ >> >> On Wed, Apr 05, 2023 at 12:58:32PM +0530, Bhupesh Sharma wrote: >>> Add crypto engine (CE) and CE BAM related nodes and definitions to >>> 'sm6115.dtsi'. >>> >>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> >>> --- >>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 22 ++++++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>> index 2a51c938bbcb..ebac026b4cc7 100644 >>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>> @@ -650,6 +650,28 @@ usb_hsphy: phy@1613000 { >>> status = "disabled"; >>> }; >>> >>> + cryptobam: dma-controller@1b04000 { >>> + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0"; >>> + reg = <0x0 0x01b04000 0x0 0x24000>; >>> + interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>; >>> + #dma-cells = <1>; >>> + qcom,ee = <0>; >>> + qcom,controlled-remotely; >>> + num-channels = <8>; >>> + qcom,num-ees = <2>; >>> + iommus = <&apps_smmu 0x94 0x11>, >>> + <&apps_smmu 0x96 0x11>; >>> + }; >>> + >>> + crypto: crypto@1b3a000 { >>> + compatible = "qcom,sm6115-qce", "qcom,sm8150-qce", "qcom,qce"; >>> + reg = <0x0 0x01b3a000 0x0 0x6000>; >>> + dmas = <&cryptobam 6>, <&cryptobam 7>; >>> + dma-names = "rx", "tx"; >>> + iommus = <&apps_smmu 0x94 0x11>, >>> + <&apps_smmu 0x96 0x11>; >> >> Shouldn't you have clocks = <&rpmcc RPM_SMD_CE1_CLK> here to make sure >> the clock for the crypto engine is on? Your binding patch (PATCH 06/11) >> says "Crypto Engine block on Qualcomm SoCs SM6115 and QCM2290 do not >> require clocks strictly" but doesn't say why. >> >> Make sure you don't rely on having rpmcc keep unused clocks on >> permanently. This is the case at the moment, but we would like to change >> this [2]. Adding new users that rely on this broken behavior would just >> make this effort even more complicated. >> >> If you also add the clock to the cryptobam then you should be able to >> see the advantage of my bam_dma patch [3]. It allows you to drop >> "num-channels" and "qcom,num-ees" from the cryptobam in your changes >> above because it can then be read directly from the BAM registers. > > Thanks for pointing this out. Actually that's why I was using your > patch while testing with RB1/RB2 :) > > Yes, so the background is that I am preparing a new version of this > crypto enablement patchset. > Also your assumption about the clocks being turned on by the firmware > is true for RB1/RB2 devices, so enabling them via Linux is optional as > per Qualcomm enggs. This is not necessarily true. Currently it's kept always-on on by clk_smd_rpm_handoff, but that's a hack from 10 years ago when smd was still new. > > So, I am testing the new patchset right now with 'clock' entries > provided in the .dtsi and see if that causes any issue / improvement > (etc.) It won't change since it's on anyway, but that won't be a given for long. Konrad > > Will come back with updates (and a new version of this patchset) soon. > > Regards, > Bhupesh > >> Thanks, >> Stephan >> >> [2]: https://lore.kernel.org/linux-arm-msm/20230303-topic-rpmcc_sleep-v2-0-ae80a325fe94@linaro.org/ >> [3]: https://lore.kernel.org/linux-arm-msm/20230518-bamclk-dt-v1-1-82f738c897d9@gerhold.net/
On Fri, 19 May 2023 at 16:12, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 19.05.2023 12:22, Bhupesh Sharma wrote: > > Hi Stephan, > > > > On Fri, 19 May 2023 at 15:40, Stephan Gerhold <stephan@gerhold.net> wrote: > >> > >> Hi Bhupesh, > >> > >> Not sure if this is the latest version of this series since it's pretty > >> old but I didn't find a new one. Just came here because you mentioned > >> RB1/RB2 [1] in my bam_dma patch and they don't have any BAM defined > >> upstream yet. > >> > >> [1]: https://lore.kernel.org/linux-arm-msm/CAH=2Ntw0BZH=RGp14mYLhX7D6jV5O5eDKRQbby=uCy85xMDU_g@mail.gmail.com/ > >> > >> On Wed, Apr 05, 2023 at 12:58:32PM +0530, Bhupesh Sharma wrote: > >>> Add crypto engine (CE) and CE BAM related nodes and definitions to > >>> 'sm6115.dtsi'. > >>> > >>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > >>> --- > >>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 22 ++++++++++++++++++++++ > >>> 1 file changed, 22 insertions(+) > >>> > >>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>> index 2a51c938bbcb..ebac026b4cc7 100644 > >>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>> @@ -650,6 +650,28 @@ usb_hsphy: phy@1613000 { > >>> status = "disabled"; > >>> }; > >>> > >>> + cryptobam: dma-controller@1b04000 { > >>> + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0"; > >>> + reg = <0x0 0x01b04000 0x0 0x24000>; > >>> + interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>; > >>> + #dma-cells = <1>; > >>> + qcom,ee = <0>; > >>> + qcom,controlled-remotely; > >>> + num-channels = <8>; > >>> + qcom,num-ees = <2>; > >>> + iommus = <&apps_smmu 0x94 0x11>, > >>> + <&apps_smmu 0x96 0x11>; > >>> + }; > >>> + > >>> + crypto: crypto@1b3a000 { > >>> + compatible = "qcom,sm6115-qce", "qcom,sm8150-qce", "qcom,qce"; > >>> + reg = <0x0 0x01b3a000 0x0 0x6000>; > >>> + dmas = <&cryptobam 6>, <&cryptobam 7>; > >>> + dma-names = "rx", "tx"; > >>> + iommus = <&apps_smmu 0x94 0x11>, > >>> + <&apps_smmu 0x96 0x11>; > >> > >> Shouldn't you have clocks = <&rpmcc RPM_SMD_CE1_CLK> here to make sure > >> the clock for the crypto engine is on? Your binding patch (PATCH 06/11) > >> says "Crypto Engine block on Qualcomm SoCs SM6115 and QCM2290 do not > >> require clocks strictly" but doesn't say why. > >> > >> Make sure you don't rely on having rpmcc keep unused clocks on > >> permanently. This is the case at the moment, but we would like to change > >> this [2]. Adding new users that rely on this broken behavior would just > >> make this effort even more complicated. > >> > >> If you also add the clock to the cryptobam then you should be able to > >> see the advantage of my bam_dma patch [3]. It allows you to drop > >> "num-channels" and "qcom,num-ees" from the cryptobam in your changes > >> above because it can then be read directly from the BAM registers. > > > > Thanks for pointing this out. Actually that's why I was using your > > patch while testing with RB1/RB2 :) > > > > Yes, so the background is that I am preparing a new version of this > > crypto enablement patchset. > > Also your assumption about the clocks being turned on by the firmware > > is true for RB1/RB2 devices, so enabling them via Linux is optional as > > per Qualcomm enggs. > This is not necessarily true. Currently it's kept always-on on > by clk_smd_rpm_handoff, but that's a hack from 10 years ago when smd > was still new. > > > > > So, I am testing the new patchset right now with 'clock' entries > > provided in the .dtsi and see if that causes any issue / improvement > > (etc.) > It won't change since it's on anyway, but that won't be a given for long. Right, so that's what I observe: RPM_SMD_CE1_CLK is always on by the time crypto _probe gets called. So, IMO let's not mix this patchset with the other fix which probably will fix the 10-year old clk_smd_rpm handoff keeping these clocks on. Probably that should be a separate changeset - requiring very thorough checks to make sure that we don't break working platforms. Thanks.
On 19.05.2023 12:49, Bhupesh Sharma wrote: > On Fri, 19 May 2023 at 16:12, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> On 19.05.2023 12:22, Bhupesh Sharma wrote: >>> Hi Stephan, >>> >>> On Fri, 19 May 2023 at 15:40, Stephan Gerhold <stephan@gerhold.net> wrote: >>>> >>>> Hi Bhupesh, >>>> >>>> Not sure if this is the latest version of this series since it's pretty >>>> old but I didn't find a new one. Just came here because you mentioned >>>> RB1/RB2 [1] in my bam_dma patch and they don't have any BAM defined >>>> upstream yet. >>>> >>>> [1]: https://lore.kernel.org/linux-arm-msm/CAH=2Ntw0BZH=RGp14mYLhX7D6jV5O5eDKRQbby=uCy85xMDU_g@mail.gmail.com/ >>>> >>>> On Wed, Apr 05, 2023 at 12:58:32PM +0530, Bhupesh Sharma wrote: >>>>> Add crypto engine (CE) and CE BAM related nodes and definitions to >>>>> 'sm6115.dtsi'. >>>>> >>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> >>>>> --- >>>>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 22 ++++++++++++++++++++++ >>>>> 1 file changed, 22 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>> index 2a51c938bbcb..ebac026b4cc7 100644 >>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>> @@ -650,6 +650,28 @@ usb_hsphy: phy@1613000 { >>>>> status = "disabled"; >>>>> }; >>>>> >>>>> + cryptobam: dma-controller@1b04000 { >>>>> + compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0"; >>>>> + reg = <0x0 0x01b04000 0x0 0x24000>; >>>>> + interrupts = <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>; >>>>> + #dma-cells = <1>; >>>>> + qcom,ee = <0>; >>>>> + qcom,controlled-remotely; >>>>> + num-channels = <8>; >>>>> + qcom,num-ees = <2>; >>>>> + iommus = <&apps_smmu 0x94 0x11>, >>>>> + <&apps_smmu 0x96 0x11>; >>>>> + }; >>>>> + >>>>> + crypto: crypto@1b3a000 { >>>>> + compatible = "qcom,sm6115-qce", "qcom,sm8150-qce", "qcom,qce"; >>>>> + reg = <0x0 0x01b3a000 0x0 0x6000>; >>>>> + dmas = <&cryptobam 6>, <&cryptobam 7>; >>>>> + dma-names = "rx", "tx"; >>>>> + iommus = <&apps_smmu 0x94 0x11>, >>>>> + <&apps_smmu 0x96 0x11>; >>>> >>>> Shouldn't you have clocks = <&rpmcc RPM_SMD_CE1_CLK> here to make sure >>>> the clock for the crypto engine is on? Your binding patch (PATCH 06/11) >>>> says "Crypto Engine block on Qualcomm SoCs SM6115 and QCM2290 do not >>>> require clocks strictly" but doesn't say why. >>>> >>>> Make sure you don't rely on having rpmcc keep unused clocks on >>>> permanently. This is the case at the moment, but we would like to change >>>> this [2]. Adding new users that rely on this broken behavior would just >>>> make this effort even more complicated. >>>> >>>> If you also add the clock to the cryptobam then you should be able to >>>> see the advantage of my bam_dma patch [3]. It allows you to drop >>>> "num-channels" and "qcom,num-ees" from the cryptobam in your changes >>>> above because it can then be read directly from the BAM registers. >>> >>> Thanks for pointing this out. Actually that's why I was using your >>> patch while testing with RB1/RB2 :) >>> >>> Yes, so the background is that I am preparing a new version of this >>> crypto enablement patchset. >>> Also your assumption about the clocks being turned on by the firmware >>> is true for RB1/RB2 devices, so enabling them via Linux is optional as >>> per Qualcomm enggs. >> This is not necessarily true. Currently it's kept always-on on >> by clk_smd_rpm_handoff, but that's a hack from 10 years ago when smd >> was still new. >> >>> >>> So, I am testing the new patchset right now with 'clock' entries >>> provided in the .dtsi and see if that causes any issue / improvement >>> (etc.) >> It won't change since it's on anyway, but that won't be a given for long. > > Right, so that's what I observe: RPM_SMD_CE1_CLK is always on by the > time crypto _probe gets called. > So, IMO let's not mix this patchset with the other fix which probably > will fix the 10-year old clk_smd_rpm handoff keeping > these clocks on. > > Probably that should be a separate changeset - requiring very thorough > checks to make sure that we don't break > working platforms. It's not about mixing patchsets, the nodes should reflect all the clock/ power-domain/regulator/pinctrl/etc. dependencies from their introduction. Remember, dt describes the hardware, not the software or firmware. That - among other things - ensures backwards compatibility can be preserved. > > Thanks.
On 12/04/2023 13:55, Naresh Kamboju wrote: > > > This patchset enables Crypto Engine support for Qualcomm SoCs like > > SM6115, SM8150, SM8250, SM8350 and SM8450. > > > > Note that: > > - SM8250 crypto engine patch utilizes the work already done by myself and > > Vladimir. > > - SM8350 crypto engine patch utilizes the work already done by Robert. > > - SM8450 crypto engine patch utilizes the work already done by Neil. > > > > Also this patchset is rebased on linux-next/master. > > These patches tested on top of Linux next-20230406. > > > Tested-by: Anders Roxell <anders.roxell@linaro.org> > Tested-by: Linux Kernel Functional Testing <lkft@linaro.org> You provided the tags for entire patchset but it includes different boards. On what boards did you test it? Best regards, Krzysztof
Changes since v5: ----------------- - v5 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230402100509.1154220-1-bhupesh.sharma@linaro.org/ - Collected Ack from Rob for [PATCH 01/11]. - Addressed Georgi's comment about interconnect cells in [PATCH 10/11]. Changes since v4: ----------------- - v4 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230331164323.729093-1-bhupesh.sharma@linaro.org/ - Collected R-Bs from Konrad for a couple of patches sent in v4. - Fixed incorrect email IDs for a couple of patches sent in v3, which I used for some patches created on a different work machine. - No functional changes since v3. Changes since v3: ----------------- - v3 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230328092815.292665-1-bhupesh.sharma@linaro.org/ - Collected Acks from Krzysztof for a couple of patches sent in v3. - Fixed review comments from Krzysztof regarding DMA binding document and also added a couple of new patches which are required to fix the 'dtbs_check' errors highlighted after this fix. Changes since v2: ----------------- - v2 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230322114519.3412469-1-bhupesh.sharma@linaro.org/ - No functional change since v2. As the sdm845 patch from v1 was accepted in linux-next, dropped it from this version. Changes since v1: ----------------- - v1 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230321190118.3327360-1-bhupesh.sharma@linaro.org/ - Folded the BAM DMA dt-binding change. (sent earlier as: https://lore.kernel.org/linux-arm-msm/20230321184811.3325725-1-bhupesh.sharma@linaro.org/) - Folded the QCE dt-binding change. (sent earlier as: https://lore.kernel.org/linux-arm-msm/20230320073816.3012198-1-bhupesh.sharma@linaro.org/) - Folded Neil's SM8450 dts patch in this series. - Addressed review comments from Rob, Stephan and Konrad. - Collected Konrad's R-B for [PATCH 5/9]. This patchset enables Crypto Engine support for Qualcomm SoCs like SM6115, SM8150, SM8250, SM8350 and SM8450. Note that: - SM8250 crypto engine patch utilizes the work already done by myself and Vladimir. - SM8350 crypto engine patch utilizes the work already done by Robert. - SM8450 crypto engine patch utilizes the work already done by Neil. Also this patchset is rebased on linux-next/master. Bhupesh Sharma (10): dt-bindings: dma: Add support for SM6115 and QCM2290 SoCs dt-bindings: dma: Increase iommu maxItems for BAM DMA arm64: dts: qcom: sdm8550: Fix the BAM DMA engine compatible string arm64: dts: qcom: sdm845: Fix the slimbam DMA engine compatible string dt-bindings: qcom-qce: Fix compatible combinations for SM8150 and IPQ4019 SoCs dt-bindings: qcom-qce: Add compatibles for SM6115 and QCM2290 arm64: dts: qcom: sm6115: Add Crypto Engine support arm64: dts: qcom: sm8150: Add Crypto Engine support arm64: dts: qcom: sm8250: Add Crypto Engine support arm64: dts: qcom: sm8350: Add Crypto Engine support Neil Armstrong (1): arm64: dts: qcom: sm8450: add crypto nodes .../devicetree/bindings/crypto/qcom-qce.yaml | 8 ++++++ .../devicetree/bindings/dma/qcom,bam-dma.yaml | 22 +++++++++------ arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +- arch/arm64/boot/dts/qcom/sm6115.dtsi | 22 +++++++++++++++ arch/arm64/boot/dts/qcom/sm8150.dtsi | 22 +++++++++++++++ arch/arm64/boot/dts/qcom/sm8250.dtsi | 22 +++++++++++++++ arch/arm64/boot/dts/qcom/sm8350.dtsi | 22 +++++++++++++++ arch/arm64/boot/dts/qcom/sm8450.dtsi | 28 +++++++++++++++++++ arch/arm64/boot/dts/qcom/sm8550.dtsi | 2 +- 9 files changed, 140 insertions(+), 10 deletions(-)