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

[Gokul Sriram P <quic_gokulsri@quicinc.com>]

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@quicinc.com>
>>
>> Add new binding document for hexagon based WCSS secure PIL remoteproc.
>> IPQ5332, IPQ9574 follows secure PIL remoteproc.
>>
>> Signed-off-by: Manikanta Mylavarapu<quic_mmanikan@quicinc.com>
>> Signed-off-by: Gokul Sriram Palanisamy<quic_gokulsri@quicinc.com>
>> ---
>>   .../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@quicinc.com>
>> +
>> +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
>