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 > 'sm8150.dtsi'. > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > arch/arm64/boot/dts/qcom/sm8150.dtsi | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi > index 9491be4a6bf0..c104d0b12dc6 100644 > --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi > @@ -2081,6 +2081,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 0x514 0x0011>, > + <&apps_smmu 0x516 0x0011>; > + }; > + > + crypto: crypto@1dfa000 { > + compatible = "qcom,sm8150-qce", "qcom,qce"; > + reg = <0 0x01dfa000 0 0x6000>; > + dmas = <&cryptobam 4>, <&cryptobam 5>; > + dma-names = "rx", "tx"; > + iommus = <&apps_smmu 0x514 0x0011>, > + <&apps_smmu 0x516 0x0011>; Downstream uses these (sid, mask) combos: qcedev: 0x0506 0x0011 0x0516 0x0011 // equal to 0x506 0x11 qcom_cedev_ns_cb: 0x512 0 0x518 0 0x519 0 0x51f 0 Shouldn't we use them too? Konrad > + interconnects = <&aggre2_noc MASTER_CRYPTO_CORE_0 &mc_virt SLAVE_EBI_CH0>; > + interconnect-names = "memory"; > + }; > + > tcsr_mutex: hwlock@1f40000 { > compatible = "qcom,tcsr-mutex"; > reg = <0x0 0x01f40000 0x0 0x20000>;
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
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/
Hi Konrad, Sorry for the late reply, but I wanted to look at the latest downstream code before responding. On Thu, 6 Apr 2023 at 19:28, 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 > > 'sm8150.dtsi'. > > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/sm8150.dtsi | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi > > index 9491be4a6bf0..c104d0b12dc6 100644 > > --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi > > @@ -2081,6 +2081,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 0x514 0x0011>, > > + <&apps_smmu 0x516 0x0011>; > > + }; > > + > > + crypto: crypto@1dfa000 { > > + compatible = "qcom,sm8150-qce", "qcom,qce"; > > + reg = <0 0x01dfa000 0 0x6000>; > > + dmas = <&cryptobam 4>, <&cryptobam 5>; > > + dma-names = "rx", "tx"; > > + iommus = <&apps_smmu 0x514 0x0011>, > > + <&apps_smmu 0x516 0x0011>; > Downstream uses these (sid, mask) combos: > > qcedev: > 0x0506 0x0011 > 0x0516 0x0011 // equal to 0x506 0x11 > > qcom_cedev_ns_cb: > 0x512 0 > 0x518 0 > 0x519 0 > 0x51f 0 > > Shouldn't we use them too? Sure, but the following are the latest values used in the downstream .dtsi, so I used them in the latest version and tested with the same on sa8115p-adp and sm8150-mtp without any issues: qcedev: 0x0502 0x0641 0x0504 0x0011 qcom_cedev_ns_cb: 0x0506 0x0011 0x0508 0x0011 0x0512 0x0000 I dropped: 0x0514 0x0011 // equal to 0x504 0x11 0x0516 0x0011 // equal to 0x506 0x11 0x0518 0x0011 // equal to 0x508 0x11 Thanks for pointing this out. Regards, Bhupesh
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