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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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

I will replace 'PD' with 'protection domain' wherever applicable.

+  remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC.

Pd means protection
+  domain.

so you can skip this sentence as it is explained already.

Sure. I will skip.

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.

Ok. I will remove ref, description and add 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.

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

Sure, i will update.

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

+
+      firmware-name:
+        $ref: /schemas/types.yaml#/definitions/string-array
+        items:
+          - description: Firmware name of the Hexagon core

same comments

Ok. I will remove ref, description and add maxItems.

+
+      interrupts-extended:
+        items:
+          - description: Fatal interrupt
+          - description: Ready interrupt
+          - description: Spawn acknowledge interrupt
+          - description: Stop acknowledge interrupt

ditto

I will use 'interrupts' here.

Thanks & Regards,
Manikanta.

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


Best regards,
Krzysztof




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux