Message-ID: <f38764d7-9d7d-47f4-a099-b6ac6b12be6e@gmail.com>
Date: Tue, 16 Dec 2025 23:01:54 -0600
From: "Alex G." <mr.nuke.me@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 devicetree@...r.kernel.org, Bjorn Andersson <andersson@...nel.org>,
 Mathieu Poirier <mathieu.poirier@...aro.org>, konradybcio@...nel.org,
 linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 1/3] dt-bindings: remoteproc: qcom,ipq8074-wcss-pil:
 convert to DT schema

On 12/15/25 11:53 PM, Krzysztof Kozlowski wrote:
> On Tue, Dec 09, 2025 at 06:37:23PM -0600, Alexandru Gagniuc wrote:
>> Convert the QCS404 and IPQ WCSS Peripheral Image Loader bindings to DT
>> schema. The text bindngs incorrectly implied that IPQ8074 needs only
>> one qcom,smem-states entry. This is only true for QCS404. IPQ8074
>> requires both "stop" and "shutdown".
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@...il.com>
> 
> Don't add fake addresses to CC. I could not respond to this email
> because of that!

Okay.

>> ---
>>   .../remoteproc/qcom,ipq9574-wcss-pil.yaml     | 167 ++++++++++++++++++
>>   .../bindings/remoteproc/qcom,q6v5.txt         | 102 -----------
>>   2 files changed, 167 insertions(+), 102 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,ipq9574-wcss-pil.yaml
>>   delete mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,ipq9574-wcss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,ipq9574-wcss-pil.yaml
>> new file mode 100644
>> index 0000000000000..d28f42661d084
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,ipq9574-wcss-pil.yaml
> 
> Filename based on the compatible, so for example:
> qcom,ipq8074-wcss-pil.yaml
Okay.
>> @@ -0,0 +1,167 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/qcom,ipq9574-wcss-pil.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm IPQ WCSS Peripheral Image Loader
>> +
>> +maintainers:
>> +  - Placeholder Maintainer <placeholder@...nel.org>
> 
> This must be a real person. Fallback is your SoC maintainer.

I can't find an official maintainer for IPQ8074 or IPQ9574. I could list 
myself, but you know a lot about these bindings. Is it okay if I list 
you as the maintainer of this binding, Krzysztof?

>> +
>> +  reg-names:
>> +    items:
>> +      - const: qdsp6
>> +      - const: rmb
>> +
>> +  interrupts-extended:
> 
> No, you only need interrupts. Please look at other bindings - how they
> write this.

I thought I needed interrupts-extended if the interrupts use more than 
one interrupt controller. Is that not the case?

>> +    minItems: 5
> 
> Drop
> 
>> +    maxItems: 5
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: wdog
>> +      - const: fatal
>> +      - const: ready
>> +      - const: handover
>> +      - const: stop-ack
>> +
>> +  resets:
>> +    minItems: 3
> 
> Drop
I will drop all the items you identified as excessive.>
>> +    maxItems: 3
>> +
>> +  reset-names:
>> +    items:
>> +      - const: wcss_aon_reset
>> +      - const: wcss_reset
>> +      - const: wcss_q6_reset
>> +
>> +  clocks:
>> +    minItems: 10
>> +    maxItems: 13
> 
> Why is this flexible? Wasn't in the old binding and nothing in the
> commit msg explained a change in the binding.

I was thinking ahead to the next patch in the series that adds IPQ9574 
binding. It makes more sense to keep it at 10 fot this patch, like you 
suggest.

>> +
>> +  clock-names:
>> +    minItems: 10
>> +    maxItems: 13
>> +
>> +  cx-supply:
>> +    description:
>> +      reference to the regulators used for the booting of the Hexagon core
>> +
>> +  memory-region:
>> +    description: Reference to wcss reserved-memory region
> 
> Drop description. Missing maxItems, please look at other bindings. Don't
> write your own style, but look how we already wrote remoteproc bindings
> (the latest).

Is "maxItems: 1" the correct thing to add here?
>> +
>> +  qcom,halt-regs:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      A phandle reference to a syscon representing TCSR followed by the three
>> +      offsets within syscon for q6, wcss and nc halt registers.
>> +    items:
>> +      - items:
>> +          - description: phandle to TCSR_MUTEX registers
>> +          - description: offset to the Q6 halt register
>> +          - description: offset to the wcss halt register
>> +          - description: offset to the nc halt register
>> +
>> +  qcom,smem-states:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> That's incomplete - missing constraints. Are you sure you wrote this
> code the same way we already did for other devices?

I am not sure. It seems to match qcom,qcs404-cdsp-pil.yaml or 
qcom,wcnss.yaml. What constraints are you expecting here?

>> +    description: States used by the AP to signal the remote processor
>> +
>> +  qcom,smem-state-names:
>> +    description:
>> +      Names of the states used by the AP to signal the remote processor
>> +
>> +  glink-edge:
>> +    $ref: /schemas/remoteproc/qcom,glink-edge.yaml#
>> +    description:
>> +      Qualcomm G-Link subnode which represents communication edge, channels
>> +      and devices related to the Modem.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - interrupts-extended
>> +  - interrupt-names
>> +  - memory-region
>> +  - qcom,halt-regs
>> +  - qcom,smem-states
>> +  - qcom,smem-state-names
>> +
>> +allOf:
> 
> Seems you do not reference other schemas. I am going to repeat myself
> for 10th time: are you sure you followed other devices?

It's the sixth time, but I see your point. Comparing to 
qcom,qcs404-cdsp-pil.yaml or qcom,wcnss.yaml, I can't see what's 
missing. What do I need here?

> 
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq8074-wcss-pil
>> +    then:
>> +      properties:
>> +        qcom,smem-states:
>> +          items:
>> +            - description: Shutdown Q6
>> +            - description: Stop Q6
>> +        qcom,smem-state-names:
>> +          items:
>> +            - const: shutdown
>> +            - const: stop
> 
> Missing clocks

The text binding that this replaces implies no clocks for IPQ8074. What 
would you like me to add instead?

> Missing blank line
> 
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,qcs404-wcss-pil
>> +    then:
>> +      properties:
>> +        qcom,smem-states:
>> +          maxItems: 1
>> +        qcom,smem-state-names:
>> +          items:
>> +            - const: stop
> 
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,qcs404-wcss-pil
>> +    then:
>> +      properties:
>> +        clocks:
>> +          minItems: 10
>> +          maxItems: 10
>> +        clock-names:
>> +          items:
>> +            - const: xo
>> +            - const: gcc_abhs_cbcr
>> +            - const: gcc_axim_cbcr
>> +            - const: lcc_ahbfabric_cbc
>> +            - const: tcsr_lcc_cbc
>> +            - const: lcc_abhs_cbc
>> +            - const: lcc_tcm_slave_cbc
>> +            - const: lcc_abhm_cbc
>> +            - const: lcc_axim_cbc
>> +            - const: lcc_bcr_sleep
> 
> All this goes to previous if.

Okay

>> +      required:
>> +        - clocks
>> +        - clock-names
>> +        - cx-supply
>> +
>> +additionalProperties: false
> 
> Missing example.

I plan to add the example in the next patch in the series that adds 
IPQ9547 binding. I don't have the resources to test IPQ8074 or QCS404, 
and I want to be sure that the example I add is tested.

Alex

Powered by blists - more mailing lists