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

[Gokul Sriram P]

On 8/20/2024 4:50 PM, Krzysztof Kozlowski wrote:
> On Tue, Aug 20, 2024 at 02:25:14PM +0530, Gokul Sriram Palanisamy wrote:
> > From: Manikanta Mylavarapu<quic_mmanikan@xxxxxxxxxxx>
> >
> > Add new binding document for hexagon based WCSS secure PIL remoteproc.
> > IPQ5332, IPQ9574 follows secure PIL remoteproc.
> >
> > Signed-off-by: Manikanta Mylavarapu<quic_mmanikan@xxxxxxxxxxx>
> > Signed-off-by: Gokul Sriram Palanisamy<quic_gokulsri@xxxxxxxxxxx>
> > ---
> >   .../remoteproc/qcom,wcss-sec-pil.yaml         | 125 ++++++++++++++++++
> >   1 file changed, 125 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..c69401b6cec1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> > @@ -0,0 +1,125 @@
> > +# 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@xxxxxxxxxxx>
> > +
> > +description:
> > +  WCSS Secure Peripheral Image Loader loads firmware and power up QDSP6
> What is WCSS? Don't add random acronyms without any explanation.

WCSS/WCNSS - Wireless Connectivity subsystem.

will update.

> > +  remoteproc's on the Qualcomm IPQ9574, IPQ5332 SoC.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - qcom,ipq5332-wcss-sec-pil
> > +      - qcom,ipq9574-wcss-sec-pil
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  firmware-name:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description: Firmware name for the Hexagon core
> No, look how other bindings are doing it.
>
> It looks like you develop patches on some old kernel, so please start
> from scratch on newest kernel.
will update.
> > +
> > +  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:
> > +    items:
> > +      - description: IM SLEEP clock
> What is IM? Explain all acronyms.
>
> What is SLEEP?

IM_SLEEP_CLK - Internal Module sleep clock needed for Q6 reset.

SLEEP is not an acronym here.

> > +
> > +  clock-names:
> > +    items:
> > +      - const: im_sleep
> sleep? Are there different sleep clocks here?

We have different branches of sleep clk each enabled separately.

im_sleep is one of those branches that q6 uses.

> > +
> > +  qcom,smem-states:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    description: States used by the AP to signal the remote processor
> > +    items:
> > +      - description: Shutdown Q6
> > +      - description: Stop Q6
> > +
> Do not introduce other order. First stop, then shutdown.
will update
> > +  qcom,smem-state-names:
> > +    description:
> > +      Names of the states used by the AP to signal the remote processor
> > +    items:
> > +      - const: shutdown
> > +      - const: stop
> The same.
will update.
> > +
> > +  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
> > +
> > +required:
> > +  - compatible
> > +  - firmware-name
> > +  - reg
> > +  - interrupts
> > +  - interrupt-names
> > +  - qcom,smem-states
> > +  - qcom,smem-state-names
> > +  - memory-region
> Keep the same order as in properties.
ok.
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> > +    q6v5_wcss: remoteproc@d100000 {
> Drop unused label.

ok.

Regards,

Gokul

> > +      compatible = "qcom,ipq5332-wcss-sec-pil";
> > +      reg = <0xd100000 0x4040>;
> > +      firmware-name = "ath12k/IPQ5332/hw1.0/q6_fw0.mdt";
> > +      interrupts-extended = <&intc GIC_SPI 291 IRQ_TYPE_EDGE_RISING>,
> > +                            <&wcss_smp2p_in 0 IRQ_TYPE_NONE>,
> > +                            <&wcss_smp2p_in 1 IRQ_TYPE_NONE>,
> Best regards,
> Krzysztof