Message-ID: <a4e0bc9b-1482-49ac-8454-39edeaf3b676@kernel.org>
Date: Wed, 17 Dec 2025 08:55:47 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: "Alex G." <mr.nuke.me@...il.com>
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 17/12/2025 06:01, Alex G. wrote:
>> 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 

I don't think you looked then. get_maintainers gives you clear answer.

> myself, but you know a lot about these bindings. Is it okay if I list 
> you as the maintainer of this binding, Krzysztof?

No. I am not the maintainer of this SoC.

> 
>>> +
>>> +  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?

Instead of asking the same question again, which would give you the same
answer "No, you only need interrupts" please rather think on the rest of
the answer - look at other bindings.


..


>>> +  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 

No, it does not.

Look at these files even - they have maxItems. Where do you see maxItems
here? So how this can be done the same way ("match")?

> qcom,wcnss.yaml. What constraints are you expecting here?

So you take the latest binding, e.g. pas-common for all new platforms.
Or qcom,qcs404-cdsp-pil.yaml. You need maxItems here and list of items
for the names.



> 
>>> +    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?

In previous cases you did not look at other binding, so I am questioning
now everything.

> 
>>
>>> +  - 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?

Disallow the clocks. See example-schema.

> 
>> 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.

I don't understand what example has anything to do with testing. We
speak here ONLY about this binding. I do not review other code. This
patch should have the example, but if you cannot come with one, because
it does not exist in any existing sources, then you should just say
that. You have entire commit msg to explain every unusual thing. And
testing something on a device is not a reason, because you do not test
the binding on a device.


Best regards,
Krzysztof

Powered by blists - more mailing lists