Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 11 Aug 2021 11:37:39 -0600
From:   Rob Herring <robh@kernel.org>
To:     Rakesh Pillai <pillair@codeaurora.org>
Cc:     agross@kernel.org, bjorn.andersson@linaro.org,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, sibis@codeaurora.org,
        sboyd@kernel.org, mpubbise@codeaurora.org
Subject: Re: [PATCH v2 2/3] dt-bindings: remoteproc: qcom: Add SC7280 WPSS
 support
Message-ID: <YRQK4xgxTAD2ALME@robh.at.kernel.org>
References: <1628618483-664-1-git-send-email-pillair@codeaurora.org>
 <1628618483-664-3-git-send-email-pillair@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1628618483-664-3-git-send-email-pillair@codeaurora.org>
Precedence: bulk

On Tue, Aug 10, 2021 at 11:31:22PM +0530, Rakesh Pillai wrote:
> Add WPSS PIL loading support for SC7280 SoCs.
> 
> Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>
> ---
>  .../bindings/remoteproc/qcom,hexagon-v56.yaml      | 79 ++++++++++++++++++++--
>  1 file changed, 74 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,hexagon-v56.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,hexagon-v56.yaml
> index 5f3558e..9ae433c6 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:
> @@ -26,25 +27,30 @@ properties:
>  
>    interrupts-extended:
>      minItems: 5
> -    maxItems: 5
> +    maxItems: 6
>  
>    interrupt-names:
>      minItems: 5
> -    maxItems: 5
> +    maxItems: 6
>  
>    clocks:
> -    minItems: 7
> +    minItems: 3

Patch 1 shows 3 clocks are valid...

>      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:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
> +
> +  power-domain-names:
> +    minItems: 1
> +    maxItems: 2
>  
>    resets:
>      minItems: 1
> @@ -92,6 +98,7 @@ required:
>    - clocks
>    - clock-names
>    - power-domains
> +  - power-domain-names

It's not backwards compatible to add a required property.

>    - qcom,halt-regs
>    - memory-region
>    - qcom,smem-states
> @@ -129,6 +136,31 @@ allOf:
>          compatible:
>            contains:
>              enum:
> +              - qcom,sc7280-wpss-pil
> +    then:
> +      properties:
> +        interrupts-extended:
> +          items:
> +            - description: Watchdog interrupt
> +            - description: Fatal interrupt
> +            - description: Ready interrupt
> +            - description: Handover interrupt
> +            - description: Stop acknowledge interrupt
> +            - description: Shutdown acknowledge interrupt
> +        interrupt-names:
> +          items:
> +            - const: wdog
> +            - const: fatal
> +            - const: ready
> +            - const: handover
> +            - const: stop-ack
> +            - const: shutdown-ack

The items lists can go in the main section with 'minItems: 5' and just 
'minItems: 6' here.

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
>                - qcom,sdm845-adsp-pil
>      then:
>        properties:
> @@ -211,6 +243,26 @@ allOf:
>          power-domains:
>            items:
>              - description: CX power domain
> +        power-domain-names:
> +          items:
> +            - const: cx
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sc7280-wpss-pil
> +    then:
> +      properties:
> +        power-domains:
> +          items:
> +            - description: CX power domain
> +            - description: MX power domain
> +        power-domain-names:
> +          items:
> +            - const: cx
> +            - const: mx

As 'cx' is always the first entry, the 'items' lists here can be moved 
to the main section and you just need minItems or maxItems in the 
if/then.

>  
>    - if:
>        properties:
> @@ -244,6 +296,23 @@ 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
> +
>  examples:
>    - |
>      #include <dt-bindings/interrupt-controller/arm-gic.h>
> -- 
> 2.7.4
> 
>