diff mbox series

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

Message ID 20230815140030.1068590-1-robimarko@gmail.com
State Superseded
Headers show
Series [v2,1/5] dt-bindings: firmware: qcom,scm: Document SDI disable | expand

Commit Message

Robert Marko Aug. 15, 2023, 1:59 p.m. UTC
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(+)

Comments

Krzysztof Kozlowski Aug. 16, 2023, 6:15 a.m. UTC | #1
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
Rob Herring (Arm) Aug. 21, 2023, 7:31 p.m. UTC | #2
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
Brian Norris Aug. 21, 2023, 7:44 p.m. UTC | #3
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
Robert Marko Aug. 21, 2023, 8:39 p.m. UTC | #4
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
diff mbox series

Patch

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: