diff mbox series

[v3,2/3] dt-bindings: remoteproc: qcom: Add SC7280 WPSS support

Message ID 1631811353-503-3-git-send-email-pillair@codeaurora.org
State Superseded
Headers show
Series Add support for sc7280 WPSS PIL loading | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success
robh/dtbs-check success

Commit Message

Rakesh Pillai Sept. 16, 2021, 4:55 p.m. UTC
Add WPSS PIL loading support for SC7280 SoCs.

Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>
---
 .../bindings/remoteproc/qcom,hexagon-v56.yaml      | 88 +++++++++++++++++++++-
 1 file changed, 86 insertions(+), 2 deletions(-)

Comments

Stephen Boyd Sept. 17, 2021, 6:25 a.m. UTC | #1
Quoting Rakesh Pillai (2021-09-16 09:55:52)
> @@ -78,6 +84,10 @@ properties:
>        Phandle reference to a syscon representing TCSR followed by the
>        three offsets within syscon for q6, modem and nc halt registers.
>
> +  qcom,qmp:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Reference to the AOSS side-channel message RAM.
> +
>    qcom,smem-states:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      description: States used by the AP to signal the Hexagon core
> @@ -117,6 +127,33 @@ allOf:
>          compatible:
>            contains:
>              enum:
> +              - qcom,sc7280-wpss-pil
> +    then:

Honestly I find this if/else to be a huge tangle. Why not split the
binding so that each compatible is a different file? Then it is easier
to read and see what properties to set.

> +      properties:
> +        interrupts-extended:
> +          maxItems: 6
> +          items:
> +            - description: Watchdog interrupt
> +            - description: Fatal interrupt
> +            - description: Ready interrupt
Rakesh Pillai Sept. 17, 2021, 10:26 a.m. UTC | #2
> -----Original Message-----
> From: Stephen Boyd <swboyd@chromium.org>
> Sent: Friday, September 17, 2021 11:56 AM
> To: Rakesh Pillai <pillair@codeaurora.org>; agross@kernel.org;
> bjorn.andersson@linaro.org; mathieu.poirier@linaro.org; ohad@wizery.com;
> p.zabel@pengutronix.de; robh+dt@kernel.org
> Cc: linux-arm-msm@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; sibis@codeaurora.org; mpubbise@codeaurora.org;
> kuabhs@chromium.org
> Subject: Re: [PATCH v3 2/3] dt-bindings: remoteproc: qcom: Add SC7280
> WPSS support
> 
> Quoting Rakesh Pillai (2021-09-16 09:55:52)
> > @@ -78,6 +84,10 @@ properties:
> >        Phandle reference to a syscon representing TCSR followed by the
> >        three offsets within syscon for q6, modem and nc halt registers.
> >
> > +  qcom,qmp:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Reference to the AOSS side-channel message RAM.
> > +
> >    qcom,smem-states:
> >      $ref: /schemas/types.yaml#/definitions/phandle-array
> >      description: States used by the AP to signal the Hexagon core @@
> > -117,6 +127,33 @@ allOf:
> >          compatible:
> >            contains:
> >              enum:
> > +              - qcom,sc7280-wpss-pil
> > +    then:
> 
> Honestly I find this if/else to be a huge tangle. Why not split the binding so
> that each compatible is a different file? Then it is easier to read and see what
> properties to set.

Hi Stephen,
I will create a separate dt-bindings yaml file for sc7280-wpss-pil, which will avoid all such if-else conditions.

> 
> > +      properties:
> > +        interrupts-extended:
> > +          maxItems: 6
> > +          items:
> > +            - description: Watchdog interrupt
> > +            - description: Fatal interrupt
> > +            - description: Ready interrupt
Bjorn Andersson Sept. 21, 2021, 11:37 p.m. UTC | #3
On Thu 16 Sep 23:25 PDT 2021, Stephen Boyd wrote:

> Quoting Rakesh Pillai (2021-09-16 09:55:52)
> > @@ -78,6 +84,10 @@ properties:
> >        Phandle reference to a syscon representing TCSR followed by the
> >        three offsets within syscon for q6, modem and nc halt registers.
> >
> > +  qcom,qmp:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Reference to the AOSS side-channel message RAM.
> > +
> >    qcom,smem-states:
> >      $ref: /schemas/types.yaml#/definitions/phandle-array
> >      description: States used by the AP to signal the Hexagon core
> > @@ -117,6 +127,33 @@ allOf:
> >          compatible:
> >            contains:
> >              enum:
> > +              - qcom,sc7280-wpss-pil
> > +    then:
> 
> Honestly I find this if/else to be a huge tangle. Why not split the
> binding so that each compatible is a different file? Then it is easier
> to read and see what properties to set.
> 

Further more, the way we express the non-PAS properties in the PAS node
in the dtsi and then switch the compatible in the non-PAS devices means
that we're causing validation errors.

So we should explode this binding to get rid of the conditionals and to
describe the "superset" of the PAS and non-PAS compatibles, for
platforms where this is applicable.

Regards,
Bjorn

> > +      properties:
> > +        interrupts-extended:
> > +          maxItems: 6
> > +          items:
> > +            - description: Watchdog interrupt
> > +            - description: Fatal interrupt
> > +            - description: Ready interrupt
Rakesh Pillai Sept. 22, 2021, 5:03 a.m. UTC | #4
> -----Original Message-----
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> Sent: Wednesday, September 22, 2021 5:08 AM
> To: Stephen Boyd <swboyd@chromium.org>
> Cc: Rakesh Pillai <pillair@codeaurora.org>; agross@kernel.org;
> mathieu.poirier@linaro.org; ohad@wizery.com; p.zabel@pengutronix.de;
> robh+dt@kernel.org; linux-arm-msm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> sibis@codeaurora.org; mpubbise@codeaurora.org; kuabhs@chromium.org
> Subject: Re: [PATCH v3 2/3] dt-bindings: remoteproc: qcom: Add SC7280
> WPSS support
> 
> On Thu 16 Sep 23:25 PDT 2021, Stephen Boyd wrote:
> 
> > Quoting Rakesh Pillai (2021-09-16 09:55:52)
> > > @@ -78,6 +84,10 @@ properties:
> > >        Phandle reference to a syscon representing TCSR followed by the
> > >        three offsets within syscon for q6, modem and nc halt
registers.
> > >
> > > +  qcom,qmp:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description: Reference to the AOSS side-channel message RAM.
> > > +
> > >    qcom,smem-states:
> > >      $ref: /schemas/types.yaml#/definitions/phandle-array
> > >      description: States used by the AP to signal the Hexagon core
> > > @@ -117,6 +127,33 @@ allOf:
> > >          compatible:
> > >            contains:
> > >              enum:
> > > +              - qcom,sc7280-wpss-pil
> > > +    then:
> >
> > Honestly I find this if/else to be a huge tangle. Why not split the
> > binding so that each compatible is a different file? Then it is easier
> > to read and see what properties to set.
> >
> 
> Further more, the way we express the non-PAS properties in the PAS node
> in the dtsi and then switch the compatible in the non-PAS devices means
that
> we're causing validation errors.
> 
> So we should explode this binding to get rid of the conditionals and to
> describe the "superset" of the PAS and non-PAS compatibles, for platforms
> where this is applicable.
> 
> Regards,
> Bjorn

Hi Bjorn,

I have posted v4 for this patchseries with wpss dt-bindings added as a
separate file.
Can you please check v4 ?

Thanks,
Rakesh Pillai.


> 
> > > +      properties:
> > > +        interrupts-extended:
> > > +          maxItems: 6
> > > +          items:
> > > +            - description: Watchdog interrupt
> > > +            - description: Fatal interrupt
> > > +            - description: Ready interrupt
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,hexagon-v56.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,hexagon-v56.yaml
index 051da43..5674602 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,hexagon-v56.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,hexagon-v56.yaml
@@ -17,6 +17,7 @@  properties:
   compatible:
     enum:
       - qcom,qcs404-cdsp-pil
+      - qcom,sc7280-wpss-pil
       - qcom,sdm845-adsp-pil
 
   reg:
@@ -43,14 +44,14 @@  properties:
       - const: stop-ack
 
   clocks:
-    minItems: 7
+    minItems: 3
     maxItems: 8
     description:
       List of phandles and clock specifier pairs for the Hexagon,
       per clock-names below.
 
   clock-names:
-    minItems: 7
+    minItems: 3
     maxItems: 8
 
   power-domains:
@@ -58,6 +59,11 @@  properties:
     items:
       - description: CX power domain
 
+  power-domain-names:
+    minItems: 1
+    items:
+      - const: cx
+
   resets:
     minItems: 1
     maxItems: 2
@@ -78,6 +84,10 @@  properties:
       Phandle reference to a syscon representing TCSR followed by the
       three offsets within syscon for q6, modem and nc halt registers.
 
+  qcom,qmp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Reference to the AOSS side-channel message RAM.
+
   qcom,smem-states:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     description: States used by the AP to signal the Hexagon core
@@ -117,6 +127,33 @@  allOf:
         compatible:
           contains:
             enum:
+              - qcom,sc7280-wpss-pil
+    then:
+      properties:
+        interrupts-extended:
+          maxItems: 6
+          items:
+            - description: Watchdog interrupt
+            - description: Fatal interrupt
+            - description: Ready interrupt
+            - description: Handover interrupt
+            - description: Stop acknowledge interrupt
+            - description: Shutdown acknowledge interrupt
+        interrupt-names:
+          maxItems: 6
+          items:
+            - const: wdog
+            - const: fatal
+            - const: ready
+            - const: handover
+            - const: stop-ack
+            - const: shutdown-ack
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
               - qcom,sdm845-adsp-pil
     then:
       properties:
@@ -192,6 +229,25 @@  allOf:
         compatible:
           contains:
             enum:
+              - qcom,sc7280-wpss-pil
+    then:
+      properties:
+        power-domains:
+          maxItems: 2
+          items:
+            - description: CX power domain
+            - description: MX power domain
+        power-domain-names:
+          maxItems: 2
+          items:
+            - const: cx
+            - const: mx
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
               - qcom,sdm845-adsp-pil
     then:
       properties:
@@ -219,6 +275,34 @@  allOf:
           items:
             - const: restart
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sc7280-wpss-pil
+    then:
+      properties:
+        resets:
+          items:
+            - description: AOSS restart
+            - description: PDC SYNC
+        reset-names:
+          items:
+            - const: restart
+            - const: pdc_sync
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sdm845-adsp-pil
+              - qcom,qcs404-cdsp-pil
+    then:
+      properties:
+        qcom,qmp: false
+
 examples:
   - |
     #include <dt-bindings/interrupt-controller/arm-gic.h>