[v6,07/11] arm64: dts: qcom: sm6115: Add Crypto Engine support

  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(+)
  

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>;
  

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 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.