[PATCH v2 1/5] dt-bindings: firmware: qcom,scm: Document SDI disable

[PATCH v2 1/5] dt-bindings: firmware: qcom,scm: Document SDI disable

[Robert Marko]

IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
means that WDT being asserted or just trying to reboot will hang the board
in the debug mode and only pulling the power and repowering will help.
Some IPQ4019 boards like Google WiFI have it enabled as well.

So, lets add a boolean property to indicate that SDI should be disabled.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
index 4233ea839bfc..bf753192498a 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
@@ -89,6 +89,14 @@ properties:
       protocol to handle sleeping SCM calls.
     maxItems: 1
 
+  qcom,sdi-disable:
+    description:
+      Indicates that the SDI (Secure Debug Image) has been enabled by TZ
+      by default and it needs to be disabled.
+      If not disabled WDT assertion or reboot will cause the board to hang
+      in the debug mode.
+    type: boolean
+
   qcom,dload-mode:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     items:
-- 
2.41.0

[PATCH v2 2/5] firmware: qcom: scm: Add SDI disable support

[Robert Marko]

Some SoC-s like IPQ5018 require SDI(Secure Debug Image) to be disabled
before trying to reboot, otherwise board will just hang after reboot has
been issued via PSCI.

So, provide a call to SCM that allows disabling it.

Signed-off-by: Robert Marko <robimarko@gmail.com>
Acked-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/firmware/qcom_scm.c | 23 +++++++++++++++++++++++
 drivers/firmware/qcom_scm.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 06fe8aca870d..abb54df663ea 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -403,6 +403,29 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
 }
 EXPORT_SYMBOL_GPL(qcom_scm_set_remote_state);
 
+static int qcom_scm_disable_sdi(void)
+{
+	int ret;
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_BOOT,
+		.cmd = QCOM_SCM_BOOT_SDI_CONFIG,
+		.args[0] = 1, /* Disable watchdog debug */
+		.args[1] = 0, /* Disable SDI */
+		.arginfo = QCOM_SCM_ARGS(2),
+		.owner = ARM_SMCCC_OWNER_SIP,
+	};
+	struct qcom_scm_res res;
+
+	ret = qcom_scm_clk_enable();
+	if (ret)
+		return ret;
+	ret = qcom_scm_call(__scm->dev, &desc, &res);
+
+	qcom_scm_clk_disable();
+
+	return ret ? : res.result[0];
+}
+
 static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 {
 	struct qcom_scm_desc desc = {
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index e6e512bd57d1..7b68fa820495 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -80,6 +80,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
 #define QCOM_SCM_SVC_BOOT		0x01
 #define QCOM_SCM_BOOT_SET_ADDR		0x01
 #define QCOM_SCM_BOOT_TERMINATE_PC	0x02
+#define QCOM_SCM_BOOT_SDI_CONFIG	0x09
 #define QCOM_SCM_BOOT_SET_DLOAD_MODE	0x10
 #define QCOM_SCM_BOOT_SET_ADDR_MC	0x11
 #define QCOM_SCM_BOOT_SET_REMOTE_STATE	0x0a
-- 
2.41.0

Re: [PATCH v2 2/5] firmware: qcom: scm: Add SDI disable support

[Krzysztof Kozlowski]

On 15/08/2023 15:59, Robert Marko wrote:
> Some SoC-s like IPQ5018 require SDI(Secure Debug Image) to be disabled
> before trying to reboot, otherwise board will just hang after reboot has
> been issued via PSCI.
> 
> So, provide a call to SCM that allows disabling it.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> Acked-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/firmware/qcom_scm.c | 23 +++++++++++++++++++++++
>  drivers/firmware/qcom_scm.h |  1 +
>  2 files changed, 24 insertions(+)

This should be squashed with next patch. Adding local function without
user is useless.

Best regards,
Krzysztof

[PATCH v2 3/5] firmware: qcom_scm: disable SDI if required

[Robert Marko]

IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
means that WDT being asserted or just trying to reboot will hang the board
in the debug mode and only pulling the power and repowering will help.
Some IPQ4019 boards like Google WiFI have it enabled as well.

So, lets use the boolean DT property to disable SDI during SCM probe.
It is important to disable it as soon as possible as we might have a WDT
assertion at any time which would then leave the board in debug mode,
thus disabling it during SCM removal is not enough.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/firmware/qcom_scm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index abb54df663ea..71d886626233 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1491,6 +1491,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	if (download_mode)
 		qcom_scm_set_download_mode(true);
 
+	/*
+	 * Disable SDI if indicated by DT.
+	 */
+	if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-disable"))
+		qcom_scm_disable_sdi();
+
 	return 0;
 }
 
-- 
2.41.0

[PATCH v2 4/5] dt-bindings: firmware: qcom,scm: document IPQ5018 compatible

[Robert Marko]

It seems that IPQ5018 compatible was never documented in the bindings.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
index bf753192498a..212ad5f32408 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
@@ -24,6 +24,7 @@ properties:
           - qcom,scm-apq8064
           - qcom,scm-apq8084
           - qcom,scm-ipq4019
+          - qcom,scm-ipq5018
           - qcom,scm-ipq5332
           - qcom,scm-ipq6018
           - qcom,scm-ipq806x
-- 
2.41.0

Re: [PATCH v2 4/5] dt-bindings: firmware: qcom,scm: document IPQ5018 compatible

[Krzysztof Kozlowski]

On 15/08/2023 15:59, Robert Marko wrote:
> It seems that IPQ5018 compatible was never documented in the bindings.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 1 +

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 4/5] dt-bindings: firmware: qcom,scm: document IPQ5018 compatible

[Sricharan Ramabadhran]

On 8/16/2023 11:40 AM, Krzysztof Kozlowski wrote:
> On 15/08/2023 15:59, Robert Marko wrote:
>> It seems that IPQ5018 compatible was never documented in the bindings.
>>
>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>> ---
>>   Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 1 +
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 

  I earlier posted this here, [1]

  [1] https://www.spinics.net/lists/linux-mmc/msg77015.html


Regards,
  Sricharan

Re: [PATCH v2 4/5] dt-bindings: firmware: qcom,scm: document IPQ5018 compatible

[Krzysztof Kozlowski]

On 15/09/2023 14:23, Sricharan Ramabadhran wrote:
> 
> 
> On 8/16/2023 11:40 AM, Krzysztof Kozlowski wrote:
>> On 15/08/2023 15:59, Robert Marko wrote:
>>> It seems that IPQ5018 compatible was never documented in the bindings.
>>>
>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>>> ---
>>>   Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 1 +
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
> 
>   I earlier posted this here, [1]
> 
>   [1] https://www.spinics.net/lists/linux-mmc/msg77015.html

So it should be applied... although status said superseded. No clue,
please check with Bjorn.

https://patchwork.kernel.org/project/linux-arm-msm/list/?series=770464&state=*

Best regards,
Krzysztof

[PATCH v2 5/5] arm64: dts: qcom: ipq5018: indicate that SDI should be disabled

[Robert Marko]

Now that SCM has support for disabling SDI if indicated by DT, lets set
it for IPQ5018 as it has SDI enabled by default and it must be disabled.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 arch/arm64/boot/dts/qcom/ipq5018.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
index 9f13d2dcdfd5..3285c86824cf 100644
--- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
@@ -57,6 +57,7 @@ L2_0: l2-cache {
 	firmware {
 		scm {
 			compatible = "qcom,scm-ipq5018", "qcom,scm";
+			qcom,sdi-disable;
 		};
 	};
 
-- 
2.41.0

Re: [PATCH v2 1/5] dt-bindings: firmware: qcom,scm: Document SDI disable

[Krzysztof Kozlowski]

On 15/08/2023 15:59, Robert Marko wrote:
> IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> means that WDT being asserted or just trying to reboot will hang the board
> in the debug mode and only pulling the power and repowering will help.
> Some IPQ4019 boards like Google WiFI have it enabled as well.
> 
> So, lets add a boolean property to indicate that SDI should be disabled.
> 
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
>  Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> index 4233ea839bfc..bf753192498a 100644
> --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> @@ -89,6 +89,14 @@ properties:
>        protocol to handle sleeping SCM calls.
>      maxItems: 1
>  
> +  qcom,sdi-disable:

The property should describe rather current hardware/firmware state,
instead of expressing your intention for OS what to do. Therefore rather:
qcom,sdi-enabled
or
qcom,secure-debug-image


Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: firmware: qcom,scm: Document SDI disable

[Rob Herring]

On Wed, Aug 16, 2023 at 08:15:54AM +0200, Krzysztof Kozlowski wrote:
> On 15/08/2023 15:59, Robert Marko wrote:
> > IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> > means that WDT being asserted or just trying to reboot will hang the board
> > in the debug mode and only pulling the power and repowering will help.
> > Some IPQ4019 boards like Google WiFI have it enabled as well.
> > 
> > So, lets add a boolean property to indicate that SDI should be disabled.
> > 
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> > index 4233ea839bfc..bf753192498a 100644
> > --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> > @@ -89,6 +89,14 @@ properties:
> >        protocol to handle sleeping SCM calls.
> >      maxItems: 1
> >  
> > +  qcom,sdi-disable:
> 
> The property should describe rather current hardware/firmware state,
> instead of expressing your intention for OS what to do. Therefore rather:
> qcom,sdi-enabled
> or
> qcom,secure-debug-image

Why can't you just disable SDI unconditionally when going into debug 
mode? Is doing that when not enabled going to crash the system or 
something?

Rob

Re: [PATCH v2 1/5] dt-bindings: firmware: qcom,scm: Document SDI disable

[Robert Marko]

On Mon, 21 Aug 2023 at 21:31, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Aug 16, 2023 at 08:15:54AM +0200, Krzysztof Kozlowski wrote:
> > On 15/08/2023 15:59, Robert Marko wrote:
> > > IPQ5018 has SDI (Secure Debug Image) enabled by TZ by default, and that
> > > means that WDT being asserted or just trying to reboot will hang the board
> > > in the debug mode and only pulling the power and repowering will help.
> > > Some IPQ4019 boards like Google WiFI have it enabled as well.
> > >
> > > So, lets add a boolean property to indicate that SDI should be disabled.
> > >
> > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > ---
> > >  Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> > > index 4233ea839bfc..bf753192498a 100644
> > > --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> > > @@ -89,6 +89,14 @@ properties:
> > >        protocol to handle sleeping SCM calls.
> > >      maxItems: 1
> > >
> > > +  qcom,sdi-disable:
> >
> > The property should describe rather current hardware/firmware state,
> > instead of expressing your intention for OS what to do. Therefore rather:
> > qcom,sdi-enabled
> > or
> > qcom,secure-debug-image
>
> Why can't you just disable SDI unconditionally when going into debug
> mode? Is doing that when not enabled going to crash the system or
> something?

Because if not disabled you will enter the Secure Debug mode even on a
regular reboot and then you have to pull the power in order to boot again.
Even according to QCA docs they intended for the Linux to disable SDI as
TZ/QSEE will always enable it as part of booting.

Regards,
Robert
>
> Rob
>

Re: [PATCH v2 1/5] dt-bindings: firmware: qcom,scm: Document SDI disable

[Brian Norris]

On Mon, Aug 21, 2023 at 12:35 PM Robert Marko <robimarko@gmail.com> wrote:
> On Mon, 21 Aug 2023 at 21:31, Rob Herring <robh@kernel.org> wrote:
> > Why can't you just disable SDI unconditionally when going into debug
> > mode? Is doing that when not enabled going to crash the system or
> > something?

I asked the same, to resounding silence:

https://lore.kernel.org/all/20200721080054.2803881-1-computersforpeace@gmail.com/
https://lore.kernel.org/all/ZNlhSdh0qDMieTAS@localhost/

> Because if not disabled you will enter the Secure Debug mode even on a
> regular reboot and then you have to pull the power in order to boot again.
> Even according to QCA docs they intended for the Linux to disable SDI as
> TZ/QSEE will always enable it as part of booting.

NB: I've never read such docs. Presumably they're internal/private to
Qualcomm and/or its direct partners? I'd love to see them.

But, I think you (robinmarko) are not really answering the same
question that Rob (robh) is asking. Rob is asking why you ever *don't*
want to disable SDI. You're answering why we ever need to disable it
at all. I don't think the latter question is controversial.

FWIW, your description of those docs sounds like we should
unconditionally *disable* SDI (like my first RFC above), which would
answer Rob's question, and would agree with my RFC above :) And as a
bonus, no Device Tree change would be required.

Brian

Re: [PATCH v2 1/5] dt-bindings: firmware: qcom,scm: Document SDI disable

[Robert Marko]

On Mon, 21 Aug 2023 at 21:44, Brian Norris <computersforpeace@gmail.com> wrote:
>
> On Mon, Aug 21, 2023 at 12:35 PM Robert Marko <robimarko@gmail.com> wrote:
> > On Mon, 21 Aug 2023 at 21:31, Rob Herring <robh@kernel.org> wrote:
> > > Why can't you just disable SDI unconditionally when going into debug
> > > mode? Is doing that when not enabled going to crash the system or
> > > something?
>
> I asked the same, to resounding silence:
>
> https://lore.kernel.org/all/20200721080054.2803881-1-computersforpeace@gmail.com/
> https://lore.kernel.org/all/ZNlhSdh0qDMieTAS@localhost/
>
> > Because if not disabled you will enter the Secure Debug mode even on a
> > regular reboot and then you have to pull the power in order to boot again.
> > Even according to QCA docs they intended for the Linux to disable SDI as
> > TZ/QSEE will always enable it as part of booting.
>
> NB: I've never read such docs. Presumably they're internal/private to
> Qualcomm and/or its direct partners? I'd love to see them.

Sadly they are all behind the NDA.

>
> But, I think you (robinmarko) are not really answering the same
> question that Rob (robh) is asking. Rob is asking why you ever *don't*
> want to disable SDI. You're answering why we ever need to disable it
> at all. I don't think the latter question is controversial.

I understood his question differently, hence my answer.

>
> FWIW, your description of those docs sounds like we should
> unconditionally *disable* SDI (like my first RFC above), which would
> answer Rob's question, and would agree with my RFC above :) And as a
> bonus, no Device Tree change would be required.

Well, the thing is that I only have docs for some of the IPQ chips, and with
the insane variety of SoC-s that use SCM and TZ/QSEE but completely
different FW base or version something would break for sure so I would
prefer to opt-in if its really required as SDI was something that was until
IPQ5018 came along, always disabled by default, except for the weird
Google WiFI board.

Regards,
Robert
>
> Brian