[PATCH v7 0/2] arm64: dts: qcom: extend the register range for ICE on sm8[56]50

[PATCH v7 0/2] arm64: dts: qcom: extend the register range for ICE on sm8[56]50

[Bartosz Golaszewski]

The following changes extend the register range for ICE IPs on sm8550
and sm8650 in order to cover the registers used for wrapped key support
on these platforms.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Changes in v7:
- bring the ICE register range up to its full size of 0x18000
- Link to v6: https://lore.kernel.org/r/20240906-wrapped-keys-dts-v6-0-3f0287cf167e@linaro.org

Changes in v6:
- split out the DT changes into a separate series
- remove the new DT property from the series
- rework commit messages
Link to v5: https://lore.kernel.org/lkml/20240617005825.1443206-1-quic_gaurkash@quicinc.com/

---
Bartosz Golaszewski (2):
      arm64: dts: qcom: sm8650: extend the register range for UFS ICE
      arm64: dts: qcom: sm8550: extend the register range for UFS ICE

 arch/arm64/boot/dts/qcom/sm8550.dtsi | 3 ++-
 arch/arm64/boot/dts/qcom/sm8650.dtsi | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)
---
base-commit: 77df9e4bb2224d8ffbddec04c333a9d7965dad6c
change-id: 20240906-wrapped-keys-dts-b733dac51d01

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH v7 1/2] arm64: dts: qcom: sm8650: extend the register range for UFS ICE

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The Inline Crypto Engine (ICE) for UFS/EMMC supports the Hardware Key
Manager (HWKM) to securely manage storage keys. Enable using this
hardware on sm8650.

This requires us to increase the register range: HWKM is an additional
piece of hardware sitting alongside ICE, and extends the old ICE's
register space.

Reviewed-by: Om Prakash Singh <quic_omprsing@quicinc.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
Co-developed-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8650.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 01ac3769ffa6..5986a33ddd8b 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -2595,7 +2595,7 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 		ice: crypto@1d88000 {
 			compatible = "qcom,sm8650-inline-crypto-engine",
 				     "qcom,inline-crypto-engine";
-			reg = <0 0x01d88000 0 0x8000>;
+			reg = <0 0x01d88000 0 0x18000>;
 
 			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
 		};

-- 
2.43.0

Re: [PATCH v7 1/2] arm64: dts: qcom: sm8650: extend the register range for UFS ICE

[Neil Armstrong]

On 01/10/2024 10:35, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> The Inline Crypto Engine (ICE) for UFS/EMMC supports the Hardware Key
> Manager (HWKM) to securely manage storage keys. Enable using this
> hardware on sm8650.
> 
> This requires us to increase the register range: HWKM is an additional
> piece of hardware sitting alongside ICE, and extends the old ICE's
> register space.
> 
> Reviewed-by: Om Prakash Singh <quic_omprsing@quicinc.com>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> Co-developed-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 01ac3769ffa6..5986a33ddd8b 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -2595,7 +2595,7 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>   		ice: crypto@1d88000 {
>   			compatible = "qcom,sm8650-inline-crypto-engine",
>   				     "qcom,inline-crypto-engine";
> -			reg = <0 0x01d88000 0 0x8000>;
> +			reg = <0 0x01d88000 0 0x18000>;
>   
>   			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>   		};
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH v7 1/2] arm64: dts: qcom: sm8650: extend the register range for UFS ICE

[Konrad Dybcio]

On 1.10.2024 10:35 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> The Inline Crypto Engine (ICE) for UFS/EMMC supports the Hardware Key
> Manager (HWKM) to securely manage storage keys. Enable using this
> hardware on sm8650.
> 
> This requires us to increase the register range: HWKM is an additional
> piece of hardware sitting alongside ICE, and extends the old ICE's
> register space.
> 
> Reviewed-by: Om Prakash Singh <quic_omprsing@quicinc.com>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> Co-developed-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v7 1/2] arm64: dts: qcom: sm8650: extend the register range for UFS ICE

[Bjorn Andersson]

On Tue, Oct 01, 2024 at 10:35:30AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> The Inline Crypto Engine (ICE) for UFS/EMMC supports the Hardware Key
> Manager (HWKM) to securely manage storage keys. Enable using this
> hardware on sm8650.
> 
> This requires us to increase the register range: HWKM is an additional
> piece of hardware sitting alongside ICE, and extends the old ICE's
> register space.
> 

This commit message doesn't follow what Neil requested in v5:

https://lore.kernel.org/lkml/109b1e46-f46f-4636-87d5-66266e04ccff@linaro.org/

> Reviewed-by: Om Prakash Singh <quic_omprsing@quicinc.com>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>

I unfortunately can't find where Neil provided this.

Is this tag referring to this patch having been tested together with the
driver changes, so he's saying that HWKM works fine. Or is he saying
that the old feature set still works after the growth of the register
region (i.e. what he requested in v5)?

Regards,
Bjorn

> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> Co-developed-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 01ac3769ffa6..5986a33ddd8b 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -2595,7 +2595,7 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>  		ice: crypto@1d88000 {
>  			compatible = "qcom,sm8650-inline-crypto-engine",
>  				     "qcom,inline-crypto-engine";
> -			reg = <0 0x01d88000 0 0x8000>;
> +			reg = <0 0x01d88000 0 0x18000>;
>  
>  			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>  		};
> 
> -- 
> 2.43.0
> 
> 

Re: [PATCH v7 1/2] arm64: dts: qcom: sm8650: extend the register range for UFS ICE

[Bartosz Golaszewski]

On Fri, Oct 4, 2024 at 4:06 PM Bjorn Andersson
<quic_bjorande@quicinc.com> wrote:
>
> On Tue, Oct 01, 2024 at 10:35:30AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > The Inline Crypto Engine (ICE) for UFS/EMMC supports the Hardware Key
> > Manager (HWKM) to securely manage storage keys. Enable using this
> > hardware on sm8650.
> >
> > This requires us to increase the register range: HWKM is an additional
> > piece of hardware sitting alongside ICE, and extends the old ICE's
> > register space.
> >
>
> This commit message doesn't follow what Neil requested in v5:
>
> https://lore.kernel.org/lkml/109b1e46-f46f-4636-87d5-66266e04ccff@linaro.org/
>

Because we have dropped the new property two versions ago as per this
series' cover letter.

> > Reviewed-by: Om Prakash Singh <quic_omprsing@quicinc.com>
> > Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
>
> I unfortunately can't find where Neil provided this.
>
> Is this tag referring to this patch having been tested together with the
> driver changes, so he's saying that HWKM works fine. Or is he saying
> that the old feature set still works after the growth of the register
> region (i.e. what he requested in v5)?
>

I think Neil tested the full functionality of HWKM on sm8650 as per
Gaurav's instructions. I did the same as well.

Bart

Re: [PATCH v7 1/2] arm64: dts: qcom: sm8650: extend the register range for UFS ICE

[Neil Armstrong]

On 04/10/2024 16:15, Bartosz Golaszewski wrote:
> On Fri, Oct 4, 2024 at 4:06 PM Bjorn Andersson
> <quic_bjorande@quicinc.com> wrote:
>>
>> On Tue, Oct 01, 2024 at 10:35:30AM +0200, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> The Inline Crypto Engine (ICE) for UFS/EMMC supports the Hardware Key
>>> Manager (HWKM) to securely manage storage keys. Enable using this
>>> hardware on sm8650.
>>>
>>> This requires us to increase the register range: HWKM is an additional
>>> piece of hardware sitting alongside ICE, and extends the old ICE's
>>> register space.
>>>
>>
>> This commit message doesn't follow what Neil requested in v5:
>>
>> https://lore.kernel.org/lkml/109b1e46-f46f-4636-87d5-66266e04ccff@linaro.org/
>>
> 
> Because we have dropped the new property two versions ago as per this
> series' cover letter.

The patch now is fine for me, the comment was applicable to v5

> 
>>> Reviewed-by: Om Prakash Singh <quic_omprsing@quicinc.com>
>>> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
>>
>> I unfortunately can't find where Neil provided this.
>>
>> Is this tag referring to this patch having been tested together with the
>> driver changes, so he's saying that HWKM works fine. Or is he saying
>> that the old feature set still works after the growth of the register
>> region (i.e. what he requested in v5)?
>>
> 
> I think Neil tested the full functionality of HWKM on sm8650 as per
> Gaurav's instructions. I did the same as well.

Exact, I can re-test if necessary, but I trust Bartosz here.

Neil

> 
> Bart

[PATCH v7 2/2] arm64: dts: qcom: sm8550: extend the register range for UFS ICE

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The Inline Crypto Engine (ICE) for UFS/EMMC supports the Hardware Key
Manager (HWKM) to securely manage storage keys. Enable using this
hardware on sm8550.

This requires us to increase the register range: HWKM is an additional
piece of hardware sitting alongside ICE, and extends the old ICE's
register space.

NOTE: Although wrapped keys cannot be independently generated and
tested on this platform using generate, prepare and import key calls,
there are non-kernel paths to create wrapped keys, and still use the
kernel to program them into ICE. Hence, enabling wrapped key support
on sm8550 too.

Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
Co-developed-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8550.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index 9dc0ee3eb98f..93c8aa32e411 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -2076,7 +2076,8 @@ opp-300000000 {
 		ice: crypto@1d88000 {
 			compatible = "qcom,sm8550-inline-crypto-engine",
 				     "qcom,inline-crypto-engine";
-			reg = <0 0x01d88000 0 0x8000>;
+			reg = <0 0x01d88000 0 0x18000>;
+
 			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
 		};
 

-- 
2.43.0

Re: [PATCH v7 2/2] arm64: dts: qcom: sm8550: extend the register range for UFS ICE

[neil.armstrong]

On 01/10/2024 10:35, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> The Inline Crypto Engine (ICE) for UFS/EMMC supports the Hardware Key
> Manager (HWKM) to securely manage storage keys. Enable using this
> hardware on sm8550.
> 
> This requires us to increase the register range: HWKM is an additional
> piece of hardware sitting alongside ICE, and extends the old ICE's
> register space.
> 
> NOTE: Although wrapped keys cannot be independently generated and
> tested on this platform using generate, prepare and import key calls,
> there are non-kernel paths to create wrapped keys, and still use the
> kernel to program them into ICE. Hence, enabling wrapped key support
> on sm8550 too.
> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> Co-developed-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm8550.dtsi | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> index 9dc0ee3eb98f..93c8aa32e411 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> @@ -2076,7 +2076,8 @@ opp-300000000 {
>   		ice: crypto@1d88000 {
>   			compatible = "qcom,sm8550-inline-crypto-engine",
>   				     "qcom,inline-crypto-engine";
> -			reg = <0 0x01d88000 0 0x8000>;
> +			reg = <0 0x01d88000 0 0x18000>;
> +
>   			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>   		};
>   
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH v7 2/2] arm64: dts: qcom: sm8550: extend the register range for UFS ICE

[Konrad Dybcio]

On 1.10.2024 10:35 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> The Inline Crypto Engine (ICE) for UFS/EMMC supports the Hardware Key
> Manager (HWKM) to securely manage storage keys. Enable using this
> hardware on sm8550.
> 
> This requires us to increase the register range: HWKM is an additional
> piece of hardware sitting alongside ICE, and extends the old ICE's
> register space.
> 
> NOTE: Although wrapped keys cannot be independently generated and
> tested on this platform using generate, prepare and import key calls,
> there are non-kernel paths to create wrapped keys, and still use the
> kernel to program them into ICE. Hence, enabling wrapped key support
> on sm8550 too.
> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> Co-developed-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v7 0/2] arm64: dts: qcom: extend the register range for ICE on sm8[56]50

[Bjorn Andersson]

On Tue, Oct 01, 2024 at 10:35:29AM +0200, Bartosz Golaszewski wrote:
> The following changes extend the register range for ICE IPs on sm8550
> and sm8650 in order to cover the registers used for wrapped key support
> on these platforms.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Changes in v7:
> - bring the ICE register range up to its full size of 0x18000
> - Link to v6: https://lore.kernel.org/r/20240906-wrapped-keys-dts-v6-0-3f0287cf167e@linaro.org
> 
> Changes in v6:
> - split out the DT changes into a separate series

Bartosz, this strategy of "let's split things such that the maintainers
can't see the full picture" is just BS. It needs to stop.

Now you will argue that these patches stands on their own, and that
might be a valid case, but the argumentation you're making in the commit
message clearly ties them to the code changes you're making somewhere
else.

Regards,
Bjorn

> - remove the new DT property from the series
> - rework commit messages
> Link to v5: https://lore.kernel.org/lkml/20240617005825.1443206-1-quic_gaurkash@quicinc.com/
> 
> ---
> Bartosz Golaszewski (2):
>       arm64: dts: qcom: sm8650: extend the register range for UFS ICE
>       arm64: dts: qcom: sm8550: extend the register range for UFS ICE
> 
>  arch/arm64/boot/dts/qcom/sm8550.dtsi | 3 ++-
>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> ---
> base-commit: 77df9e4bb2224d8ffbddec04c333a9d7965dad6c
> change-id: 20240906-wrapped-keys-dts-b733dac51d01
> 
> Best regards,
> -- 
> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 

Re: [PATCH v7 0/2] arm64: dts: qcom: extend the register range for ICE on sm8[56]50

[Bartosz Golaszewski]

On Fri, Oct 4, 2024 at 3:56 PM Bjorn Andersson
<quic_bjorande@quicinc.com> wrote:
>
> On Tue, Oct 01, 2024 at 10:35:29AM +0200, Bartosz Golaszewski wrote:
> > The following changes extend the register range for ICE IPs on sm8550
> > and sm8650 in order to cover the registers used for wrapped key support
> > on these platforms.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Changes in v7:
> > - bring the ICE register range up to its full size of 0x18000
> > - Link to v6: https://lore.kernel.org/r/20240906-wrapped-keys-dts-v6-0-3f0287cf167e@linaro.org
> >
> > Changes in v6:
> > - split out the DT changes into a separate series
>
> Bartosz, this strategy of "let's split things such that the maintainers
> can't see the full picture" is just BS. It needs to stop.
>

You're exaggerating, I'm not doing anything like this. You're still
Cc'ed on the single big series containing the code changes for wrapped
keys. The full picture is over there.

> Now you will argue that these patches stands on their own, and that

Yes, that's precisely what I'm going to say, because it's true.
0x18000 is the true register size (as per QCom docs) for ICE on
sm8[56]50 and sa8775p too and that alone warrants this change. If we
can get the DTS changes out of the way of wrapped keys, then that's
just a bonus.

> might be a valid case, but the argumentation you're making in the commit
> message clearly ties them to the code changes you're making somewhere
> else.
>

Are you referring to "(...) registers used for wrapped key support on
these platforms"? I could argue that it just says what the additional
registers are used for but I can drop the mention of this from the
message and just say "0x18000 is the true register range for ICE on
sm8650" if that works for you.

Bart

Re: [PATCH v7 0/2] arm64: dts: qcom: extend the register range for ICE on sm8[56]50

[Bjorn Andersson]

On Fri, Oct 04, 2024 at 04:13:15PM GMT, Bartosz Golaszewski wrote:
> On Fri, Oct 4, 2024 at 3:56 PM Bjorn Andersson
> <quic_bjorande@quicinc.com> wrote:
> >
> > On Tue, Oct 01, 2024 at 10:35:29AM +0200, Bartosz Golaszewski wrote:
> > > The following changes extend the register range for ICE IPs on sm8550
> > > and sm8650 in order to cover the registers used for wrapped key support
> > > on these platforms.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Changes in v7:
> > > - bring the ICE register range up to its full size of 0x18000
> > > - Link to v6: https://lore.kernel.org/r/20240906-wrapped-keys-dts-v6-0-3f0287cf167e@linaro.org
> > >
> > > Changes in v6:
> > > - split out the DT changes into a separate series
> >
> > Bartosz, this strategy of "let's split things such that the maintainers
> > can't see the full picture" is just BS. It needs to stop.
> >
> 
> You're exaggerating, I'm not doing anything like this. You're still
> Cc'ed on the single big series containing the code changes for wrapped
> keys. The full picture is over there.
> 
> > Now you will argue that these patches stands on their own, and that
> 
> Yes, that's precisely what I'm going to say, because it's true.

Good, I fully agree with you here.

> 0x18000 is the true register size (as per QCom docs) for ICE on
> sm8[56]50 and sa8775p too and that alone warrants this change. If we
> can get the DTS changes out of the way of wrapped keys, then that's
> just a bonus.
> 
> > might be a valid case, but the argumentation you're making in the commit
> > message clearly ties them to the code changes you're making somewhere
> > else.
> >
> 
> Are you referring to "(...) registers used for wrapped key support on
> these platforms"? I could argue that it just says what the additional
> registers are used for but I can drop the mention of this from the
> message and just say "0x18000 is the true register range for ICE on
> sm8650" if that works for you.
> 

Please update the commit messages of the two patches to not say that
they are enabling HWKM on the platforms, but that you're expanding the
mmio region to cover the whole IP-block. I think it makes sense to
mention that this will allow us to enable HWKM.

Thanks,
Bjorn