On 19.05.2023 12:49, Bhupesh Sharma wrote: > On Fri, 19 May 2023 at 16:12, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote: >> >> On 19.05.2023 12:22, Bhupesh Sharma wrote: >>> Hi Stephan, >>> >>> On Fri, 19 May 2023 at 15:40, Stephan Gerhold <stephan@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx/ >>>> >>>> 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@xxxxxxxxxx> >>>>> --- >>>>> 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.
