Re: [PATCH v8 2/6] dt-bindings: remoteproc: qcom: document hexagon based WCSS secure PIL

[Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>]

On Sat, Dec 20, 2025 at 09:39:54AM +0100, Krzysztof Kozlowski wrote:
> On Fri, Dec 19, 2025 at 08:40:06AM +0530, Varadarajan Narayanan wrote:
> > From: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> >
> > Add new binding document for hexagon based WCSS secure PIL remoteproc.
> > IPQ5018, IPQ5332 and IPQ9574 follow secure PIL remoteproc.
> >
> > Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> > Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
> > Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> > [ Dropped ipq5424 support ]
> > Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>
> > ---
> > v8: Dropped Krzysztof's 'Reviewed-by' as the bindings file has changed significantly
> >     Drop ipq5424 support
> >     Update example to ipq9574 instead of ipq5424
> >     Change 'mboxes' description
>
> I do not see any "significant" differences in the binding. You ONLY
> dropped one compatible (no problem, why would we care?) and renamed one
> description in mboxs. No other changes, nothing, so I do not understand
> what was the significant difference here.
>
> Dropping reviews at v8 is really unexpected and to me it feels like you
> rewrite everything, which should not happen at v8.

Sorry about this. Was not sure if the changes introduced in v6
(https://lore.kernel.org/all/20251208-ipq5018-wifi-v6-2-d0ce2facaa5f@outlook.com/#t)
had your approval. Hence wanted to seek your approval once again.

> > ---
> >  .../remoteproc/qcom,wcss-sec-pil.yaml         | 172 ++++++++++++++++++
> >  1 file changed, 172 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> > new file mode 100644
> > index 000000000000..0fe04e0a4ca5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
>
>
> Well, since you ask for re-review and I really cannot find the
> difference, then let's start nitpicking from scratch:
>
> Please use one of the compatibles, e.g. the oldest device, as filename.
>
> > @@ -0,0 +1,172 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/remoteproc/qcom,wcss-sec-pil.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm WCSS Secure Peripheral Image Loader
> > +
> > +maintainers:
> > +  - Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> > +
> > +description:
> > +  Wireless Connectivity Subsystem (WCSS) Secure Peripheral Image Loader loads
> > +  firmware and power up QDSP6 remoteproc on the Qualcomm IPQ series SoC.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - qcom,ipq5018-wcss-sec-pil
> > +      - qcom,ipq5332-wcss-sec-pil
> > +      - qcom,ipq9574-wcss-sec-pil
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  firmware-name:
> > +    maxItems: 1
> > +    description: Firmware name for the Hexagon core
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Watchdog interrupt
> > +      - description: Fatal interrupt
> > +      - description: Ready interrupt
> > +      - description: Handover interrupt
> > +      - description: Stop acknowledge interrupt
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: wdog
> > +      - const: fatal
> > +      - const: ready
> > +      - const: handover
> > +      - const: stop-ack
> > +
> > +  clocks:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
>
> What is this? How did it ever appear here? Sorry, but this is not a
> syntax present anywhere.
>
> > +
> > +  clock-names:
> > +    $ref: /schemas/types.yaml#/definitions/string-array
>
> Neither this. Look, I have never reviewed something like this:
>
> https://lore.kernel.org/all/20240829134021.1452711-2-quic_gokulsri@quicinc.com/
>
> NAK, that's not a binding style at all. Please do not invent completely
> different style.
>
> What's weird, this change did not happen at v8, so you still kept my
> review tag even after inventing this weird code.

These were introduced in v6 https://lore.kernel.org/all/20251208-ipq5018-wifi-v6-2-d0ce2facaa5f@outlook.com/#t by George, not me.
This is why I felt the changes were 'significant' and dropped your reviewed-by tag.
Will restore them to the way you had approved earlier and post a new version.

Once I restore the above to your approved version, can I include your reviewed-by?
Or, shall I wait for your fresh approval of v9. Please advice.

Thanks
Varada

> > +  mboxes:
> > +    items:
> > +      - description: TMECom mailbox driver
> > +
> > +  qcom,smem-states:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    description: States used by the AP to signal the remote processor
> > +    items:
> > +      - description: Stop Q6
> > +      - description: Shutdown Q6
> > +
> > +  qcom,smem-state-names:
> > +    description:
> > +      Names of the states used by the AP to signal the remote processor
> > +    items:
> > +      - const: stop
> > +      - const: shutdown
> > +
> > +  memory-region:
> > +    items:
> > +      - description: Q6 reserved region
> > +
> > +  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.
> > +    unevaluatedProperties: false
>
> Best regards,
> Krzysztof
>