linux-kernel - Re: [PATCH RFC 1/3] dt-bindings: remoteproc: qcom,ipq8074-wcss-pil: convert to DT schema

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20251216-notorious-omniscient-frog-caceaf@quoll>
Date: Tue, 16 Dec 2025 06:53:04 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Alexandru Gagniuc <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 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!

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

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

> +
> +description:
> +  The IPQ WCSS peripheral image loader is used to load firmware on the Qualcomm
> +  Q6 processor that exposes WiFi-6 devices to the OS via the AHB bus. It is
> +  generally used by ath11k to start up the wireless firmware.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq8074-wcss-pil
> +      - qcom,qcs404-wcss-pil
> +
> +  reg:
> +    minItems: 2

Drop

> +    maxItems: 2
> +    description:
> +      The base address and size of the QDSP6, and RMB register blocks

Drop description. Look at other bindings how this is written.

> +
> +  reg-names:
> +    items:
> +      - const: qdsp6
> +      - const: rmb
> +
> +  interrupts-extended:

No, you only need interrupts. Please look at other bindings - how they
write this.

> +    minItems: 5

Drop

> +    maxItems: 5
> +
> +  interrupt-names:
> +    items:
> +      - const: wdog
> +      - const: fatal
> +      - const: ready
> +      - const: handover
> +      - const: stop-ack
> +
> +  resets:
> +    minItems: 3

Drop

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

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

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

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

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

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.

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

Missing example.


Best regards,
Krzysztof