Message-ID: <4bbc7303-759f-4e00-a397-d233a013f620@gmail.com>
Date: Wed, 17 Dec 2025 16:22:42 -0600
From: 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/17/25 1:55 AM, Krzysztof Kozlowski wrote:
> 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.

get_maintainers on qcom,q6v5.txt gives five generic subsystem 
maintainers, and you are on of them! I'll take something more specific, 
but just defaulting to get_maintainers is not helpful here.

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

Thank you for confirming you do not wish to be listed as the maintainer 
here.

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

No. It's a clarifying question with additional context. I think I should 
be expected to ask them when I still have doubts.
>>>> +  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.

Okay. I wasn't sure if that's the appropriate solution when QCS404 and 
IPQ8074 take a different number of smem-states.
>>
>>>> +    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.I resent the accusation. I looked at several other bindings to see the 
best way to write this one in a manner that also works with "make 
dt_binding_check". I have done my homework, and think it is unproductive 
to accuse me of not doing it because I did not do it to your standards 
or liking.

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

Okay. Makes sense (clocks: false).

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

I will mention this discrepant in the commit message.

> Best regards,
> Krzysztof

Powered by blists - more mailing lists