Re: [PATCH V2 01/13] dt-bindings: remoteproc: qcom: Add support for multipd model

[Manikanta Mylavarapu]

On 5/30/2023 4:28 PM, Krzysztof Kozlowski wrote:
> On 22/05/2023 00:28, Manikanta Mylavarapu wrote:
> > Add new binding document for multipd model remoteproc.
> > IPQ5018, IPQ9574 follows multipd model.
> >
> > Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@xxxxxxxxxxx>
> > ---
> > Changes in V2:
> > 	- Fixed all comments and rebased for TOT.
> > 	- Changed to BSD-3-Clause based on internal open source team
> >            suggestion.
> > 	- Added firmware-name.
> >
> >   .../bindings/remoteproc/qcom,multipd-pil.yaml | 265 ++++++++++++++++++
> >   1 file changed, 265 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
> > new file mode 100644
> > index 000000000000..3257f27dc569
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml
> > @@ -0,0 +1,265 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/remoteproc/qcom,multipd-pil.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Multipd Secure Peripheral Image Loader
> > +
> > +maintainers:
> > +  - Bjorn Andersson <andersson@xxxxxxxxxx>
> > +  - Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > +
> > +description:
> > +  Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd
>
> ... boots Q6 Protection Domain (PD), WCSS PD ...
>
> > +  remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC.
>
> > Pd means protection
> > +  domain.
>
> so you can skip this sentence as it is explained already.
>
> > It's similar to process in Linux. Here QDSP6 processor runs each
> > +  wifi radio functionality on a separate process. One process can't access
> > +  other process resources, so this is termed as PD i.e protection domain.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - qcom,ipq5018-q6-mpd
> > +      - qcom,ipq9574-q6-mpd
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  firmware-name:
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> > +    description: Firmware name of the Hexagon core
>
> No need for ref and description. Instead maxItems.
>
> > +
> > +  interrupts-extended:
> > +    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
> > +
> > +  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
> > +
> > +  qcom,smem-state-names:
> > +    description:
> > +      Names of the states used by the AP to signal the remote processor
> > +    items:
> > +      - const: shutdown
> > +      - const: stop
> > +
> > +  memory-region:
> > +    items:
> > +      - description: Q6 pd 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.
> > +
> > +patternProperties:
> > +  "^pd-1|pd-2|pd-3":
> > +    type: object
> > +    description:
> > +      In Multipd model, WCSS pd depends on Q6 pd i.e Q6 pd should be up before
> > +      WCSS. It can be achieved by keeping wcss pd node as subnode of Q6
> > +      device node.
>
> That's not enough. Your description does not say what is this, why you
> have two protection domains for same compatible. What's more, it a bit
> deviates from hardware description.
WCSS means 'wireless connectivity sub system', in simple words it's a
wifi radio block.

IPQ5018 SOC has both internal (AHB) wifi radio/WCSS and external (PCIE)
wifi radio/WCSS. In Q6, Root protection domain will provide services to
both internal (AHB) and external (PCIE) wifi radio's protection domain.
So we have two protection domains for IPQ5018, one is for internal(AHB) 
and other is for external(PCIE) wifi radio.

> > +
> > +    properties:
> > +      compatible:
> > +        enum:
> > +          - qcom,ipq5018-wcss-ahb-mpd
> > +          - qcom,ipq9574-wcss-ahb-mpd
> > +          - qcom,ipq5018-wcss-pcie-mpd
>
> Keep rather alphabetical order (so both 5018 together).
>
> I also do not understand these at all. Why adding bus type to
> compatible? This rarely is allowed (unless it is PCIe controller within
> soc).
IPQ5018 SOC has in-built PCIE controller. Here QDSP6 will bring up
external(PCIE) and internal (AHB) wifi radio's. To separate AHB, PCIE 
radio's properties, i have added bus type to compatible.

> > +
> > +      firmware-name:
> > +        $ref: /schemas/types.yaml#/definitions/string-array
> > +        items:
> > +          - description: Firmware name of the Hexagon core
>
> same comments
>
> > +
> > +      interrupts-extended:
> > +        items:
> > +          - description: Fatal interrupt
> > +          - description: Ready interrupt
> > +          - description: Spawn acknowledge interrupt
> > +          - description: Stop acknowledge interrupt
>
> ditto
>
> > +
> > +      interrupt-names:
> > +        items:
> > +          - const: fatal
> > +          - const: ready
> > +          - const: spawn-ack
> > +          - const: stop-ack
> > +
> > +      qcom,smem-states:
> > +        $ref: /schemas/types.yaml#/definitions/phandle-array
> > +        description: States used by the AP to signal the remote processor
> > +        items:
> > +          - description: Shutdown WCSS pd
> > +          - description: Stop WCSS pd
> > +          - description: Spawn WCSS pd
> > +
> > +      qcom,smem-state-names:
> > +        description:
> > +          Names of the states used by the AP to signal the remote processor
> > +        items:
> > +          - const: shutdown
> > +          - const: stop
> > +          - const: spawn
> > +
> > +    required:
> > +      - compatible
> > +      - firmware-name
> > +      - interrupts-extended
> > +      - interrupt-names
> > +      - qcom,smem-states
> > +      - qcom,smem-state-names
> > +
> > +    unevaluatedProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - firmware-name
> > +  - reg
> > +  - interrupts-extended
> > +  - interrupt-names
> > +  - qcom,smem-states
> > +  - qcom,smem-state-names
> > +  - memory-region
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,ipq5018-q6-mpd
> > +    then:
> > +      properties:
> > +        firmware-name:
> > +          items:
> > +            - const: IPQ5018/q6_fw.mdt
> > +            - const: IPQ5018/m3_fw.mdt
> > +            - const: qcn6122/m3_fw.mdt
>
> No, names are not part of bindings. Also paths do not look correct. Open
> linux-firmware package and verify these are good...
>
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,ipq9574-q6-mpd
> > +    then:
> > +      properties:
> > +        firmware-name:
> > +          items:
> > +            - const: IPQ9574/q6_fw.mdt
> > +            - const: IPQ9574/m3_fw.mdt
>
> Drop.
>
> > +
> > +unevaluatedProperties: false
>
> This changed... why?
'unevaluatedProperties' is similar to 'additionalProperties' except
that it recognize properties declared in subschemas as well.

Thanks & Regards,
Manikanta.

> Best regards,
> Krzysztof