| Message ID | 20230521222852.5740-2-quic_mmanikan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2,01/13] dt-bindings: remoteproc: qcom: Add support for multipd model | expand |
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@quicinc.com> > --- > 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@kernel.org> > + - Mathieu Poirier <mathieu.poirier@linaro.org> > + > +description: > + Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd > + remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC. Pd means protection > + domain. 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 > + > + interrupts-extended: > + items: > + - description: Watchdog interrupt > + - description: Fatal interrupt > + - description: Ready interrupt > + - description: Handover interrupt > + - description: Stop acknowledge interrupt interrupts instead. The same in required:. I replied also to your comment on your comment. :) Best regards, Krzysztof
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@quicinc.com> > --- > 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@kernel.org> > + - Mathieu Poirier <mathieu.poirier@linaro.org> > + > +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. > + > + 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). > + > + 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? Best regards, Krzysztof
On 5/30/2023 4:16 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@quicinc.com> >> --- >> 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@kernel.org> >> + - Mathieu Poirier <mathieu.poirier@linaro.org> >> + >> +description: >> + Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd >> + remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC. Pd means protection >> + domain. 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 >> + >> + interrupts-extended: >> + items: >> + - description: Watchdog interrupt >> + - description: Fatal interrupt >> + - description: Ready interrupt >> + - description: Handover interrupt >> + - description: Stop acknowledge interrupt > > > interrupts instead. The same in required:. I replied also to your > comment on your comment. :) > > Best regards, > Krzysztof > I will use interrupts instead of interrupts-extended in dt-bindings. Thanks & Regards, Manikanta.
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@quicinc.com> >> --- >> 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@kernel.org> >> + - Mathieu Poirier <mathieu.poirier@linaro.org> >> + >> +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 >
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@quicinc.com> >> --- >> 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@kernel.org> >> + - Mathieu Poirier <mathieu.poirier@linaro.org> >> + >> +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 >
On 05/06/2023 14:02, Manikanta Mylavarapu wrote: >>> + 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. So it is now in email, but not in the code... > >>> + >>> + 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. It's the same device - WCSS - right? We do not create multiple nodes and compatibles for the same devices. Bus suffixes are almost never parts of compatibles. >> >> Drop. >> >>> + >>> +unevaluatedProperties: false >> >> This changed... why? >> >> > 'unevaluatedProperties' is similar to 'additionalProperties' except > that it recognize properties declared in subschemas as well. You don't have to explain me what are unevaluatedProperties or additionalProperties. Let's assume that I know them. What you should explain is why you changed it. Where is the reference to other schema? Best regards, Krzysztof
On 6/6/2023 11:44 AM, Krzysztof Kozlowski wrote: > On 05/06/2023 14:02, Manikanta Mylavarapu wrote: >>>> + 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. > > So it is now in email, but not in the code... >> I will add this info, corresponding block diagram in driver code. >>>> + >>>> + 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. > > It's the same device - WCSS - right? We do not create multiple nodes and > compatibles for the same devices. Bus suffixes are almost never parts of > compatibles. > > No it's not the same device. WCSS on inside IPQ5018 and WCSS attached via pcie to IPQ5018. Here QDSP6 managing both WCSS's. So for better clarity i will use attached SOC ID in compatible. Below are the new compatible's. - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio - qcom,ipq9574-wcss-mpd //IPQ9574 internal radio - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio >>> >>> Drop. >>> >>>> + >>>> +unevaluatedProperties: false >>> >>> This changed... why? >>> >>> >> 'unevaluatedProperties' is similar to 'additionalProperties' except >> that it recognize properties declared in subschemas as well. > > You don't have to explain me what are unevaluatedProperties or > additionalProperties. Let's assume that I know them. What you should > explain is why you changed it. Where is the reference to other schema? > I will go with previously used 'additionalProperties' itself and add 'unevaluatedProperties' in glink-edge. Thanks & Regards, Manikanta. > > Best regards, > Krzysztof >
Manikanta Mylavarapu <quic_mmanikan@quicinc.com> writes: >>>>> + >>>>> + 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. >> >> It's the same device - WCSS - right? We do not create multiple nodes and >> compatibles for the same devices. Bus suffixes are almost never parts of >> compatibles. > > > No it's not the same device. WCSS on inside IPQ5018 and WCSS attached > via pcie to IPQ5018. Here QDSP6 managing both WCSS's. > > So for better clarity i will use attached SOC ID in compatible. > Below are the new compatible's. > > - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio > - qcom,ipq9574-wcss-mpd //IPQ9574 internal radio > - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio What mandates that there's just one QCN6122 device attached to PCI? Assuming fixed PCI configurations like that makes me worried.
On 6/6/2023 7:19 PM, Kalle Valo wrote: > Manikanta Mylavarapu <quic_mmanikan@quicinc.com> writes: > >>>>>> + >>>>>> + 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. >>> >>> It's the same device - WCSS - right? We do not create multiple nodes and >>> compatibles for the same devices. Bus suffixes are almost never parts of >>> compatibles. >> >> >> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached >> via pcie to IPQ5018. Here QDSP6 managing both WCSS's. >> >> So for better clarity i will use attached SOC ID in compatible. >> Below are the new compatible's. >> >> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio >> - qcom,ipq9574-wcss-mpd //IPQ9574 internal radio >> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio > > What mandates that there's just one QCN6122 device attached to PCI? > Assuming fixed PCI configurations like that makes me worried. > IPQ5018 always has one internal radio, attached pcie radio's depends on no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's number of pcie devices controlled by QDSP6. Thanks & Regards, Manikanta.
On 07/06/2023 10:10, Manikanta Mylavarapu wrote: > > > On 6/6/2023 7:19 PM, Kalle Valo wrote: >> Manikanta Mylavarapu <quic_mmanikan@quicinc.com> writes: >> >>>>>>> + >>>>>>> + 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. >>>> >>>> It's the same device - WCSS - right? We do not create multiple nodes and >>>> compatibles for the same devices. Bus suffixes are almost never parts of >>>> compatibles. >>> >>> >>> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached >>> via pcie to IPQ5018. Here QDSP6 managing both WCSS's. >>> >>> So for better clarity i will use attached SOC ID in compatible. >>> Below are the new compatible's. >>> >>> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio >>> - qcom,ipq9574-wcss-mpd //IPQ9574 internal radio >>> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio >> >> What mandates that there's just one QCN6122 device attached to PCI? >> Assuming fixed PCI configurations like that makes me worried. >> > > IPQ5018 always has one internal radio, attached pcie radio's depends on > no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two > qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's > number of pcie devices controlled by QDSP6. So this is hot-pluggable (or at least board-pluggable), then should not be a part of static DTS. Some concepts of virtual-processes is anyway far away from hardware description, thus does not fit into DTS. Adding now to the equation PCIe with variable number of such processes, brings us even further. This is not a DT property. Remember - DT describes hardware. Best regards, Krzysztof
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@quicinc.com> >> --- >> 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@kernel.org> >> + - Mathieu Poirier <mathieu.poirier@linaro.org> >> + >> +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. > >> + >> + 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). > >> + >> + 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... > I will remove names from bindings and use firmware path as per https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/ath11k/IPQ5018/hw1.0 >> + >> + - 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. > I will drop. Thanks & Regards, Manikanta. >> + >> +unevaluatedProperties: false > > This changed... why? > > > Best regards, > Krzysztof >
On 6/7/2023 1:57 PM, Krzysztof Kozlowski wrote: > On 07/06/2023 10:10, Manikanta Mylavarapu wrote: >> >> >> On 6/6/2023 7:19 PM, Kalle Valo wrote: >>> Manikanta Mylavarapu <quic_mmanikan@quicinc.com> writes: >>> >>>>>>>> + >>>>>>>> + 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. >>>>> >>>>> It's the same device - WCSS - right? We do not create multiple nodes and >>>>> compatibles for the same devices. Bus suffixes are almost never parts of >>>>> compatibles. >>>> >>>> >>>> No it's not the same device. WCSS on inside IPQ5018 and WCSS attached >>>> via pcie to IPQ5018. Here QDSP6 managing both WCSS's. >>>> >>>> So for better clarity i will use attached SOC ID in compatible. >>>> Below are the new compatible's. >>>> >>>> - qcom,ipq5018-wcss-mpd //IPQ5018 internal radio >>>> - qcom,ipq9574-wcss-mpd //IPQ9574 internal radio >>>> - qcom,qcn6122-wcss-mpd //IPQ5018 attached radio >>> >>> What mandates that there's just one QCN6122 device attached to PCI? >>> Assuming fixed PCI configurations like that makes me worried. >>> >> >> IPQ5018 always has one internal radio, attached pcie radio's depends on >> no of pcie ports. IPQ5018 has 2 pcie ports, so it supports max two >> qcn6122 devices. One compatible (qcom,qcn6122-wcss-mpd) itself support's >> number of pcie devices controlled by QDSP6. > > So this is hot-pluggable (or at least board-pluggable), then should not > be a part of static DTS. > > Some concepts of virtual-processes is anyway far away from hardware > description, thus does not fit into DTS. Adding now to the equation PCIe > with variable number of such processes, brings us even further. > > This is not a DT property. Remember - DT describes hardware. > > Best regards, > Krzysztof > In the multipd architecture based Socs, There is one Q6 DSP which runs the OS/kernel and there are one or more instances of WCSS radios (It can be either internal or pcie attached). These WCSS cores are controlled by the Q6 (Q6 DSP brings wcss radios out of reset/ shuts it down etc). Q6 forms the 'root Protection domain' and the wcss radios are termed as the 'user protection domain'. The compatible's that is being added here are to manage the 'root domain' and 'user domain'. Not sure if using the words 'pcie'/'ahb' made it confusing. So, 'qcom,ipq5018-q6-mpd' and 'qcom,ipq5018-wcss-mpd'. There will be multiple instances of 'qcom,ipq5018-wcss-mpd' in DT based on number of wcss radios connected on that board and only one instance of 'qcom,ipq5018-q6-mpd'. Is this approach ok ? Thanks & Regards, Manikanta.
On 21/06/2023 13:39, Manikanta Mylavarapu wrote: >>> on number of wcss radios connected on that board and only one instance >>> of 'qcom,ipq5018-q6-mpd'. >>> >> >> I don't understand why the user protection domains need a specific >> compatible. Why do they need compatible at all? >> >> Not mentioning that amount of your domains on Q6 is actually fixed per >> SoC and probably should not be in DT at all. >> > root domain is fixed per soc (One Q6 DSP, one per soc) > user domain(s) are variable (based on number of wcss radios attached) > > The sequence to initialize, bring up, tear down the Q6 and wcss radios > are completely different. So in order to differentiate them, we will > need different compatibles. So this is a new rproc driver/architecture > which has a parent/child topology (Q6 DSP -> Master/parent controls > the WCSS (child)). I understand you need different properties, but I don't see yet the benefit of creating here artificial compatibles. Look at your ipq9574 DTSI change - it does not use even ipq9574 compatibles for 66% of its children. Maybe you should have there just property describing type of device or bringup? Given SoC cannot come with different amount of children (PD) and different amount of radios. You even fix the firmware name, so boards/customers cannot use anything else. It's fixed and the only variable element here is disabling some of the blocks per board, if they do not have physical interface (e.g. radio). You even hard-code the number of PD by using "pd-[123]", without unit address, so you do not expect it will grow. Unless you want to say that these are devices? But your binding does not really suggest it... Best regards, Krzysztof
On 6/24/2023 12:49 PM, Krzysztof Kozlowski wrote: > On 21/06/2023 13:39, Manikanta Mylavarapu wrote: >>>> on number of wcss radios connected on that board and only one instance >>>> of 'qcom,ipq5018-q6-mpd'. >>>> >>> >>> I don't understand why the user protection domains need a specific >>> compatible. Why do they need compatible at all? >>> >>> Not mentioning that amount of your domains on Q6 is actually fixed per >>> SoC and probably should not be in DT at all. >>> >> root domain is fixed per soc (One Q6 DSP, one per soc) >> user domain(s) are variable (based on number of wcss radios attached) >> >> The sequence to initialize, bring up, tear down the Q6 and wcss radios >> are completely different. So in order to differentiate them, we will >> need different compatibles. So this is a new rproc driver/architecture >> which has a parent/child topology (Q6 DSP -> Master/parent controls >> the WCSS (child)). > > I understand you need different properties, but I don't see yet the > benefit of creating here artificial compatibles. Look at your ipq9574 > DTSI change - it does not use even ipq9574 compatibles for 66% of its > children. > > Maybe you should have there just property describing type of device or > bringup? > Yeah i got your point. Indeed the requirement here is to have device specific compatibles, so will have just two compatible one for Q6-MPD and another for WCSS-MPD device's "qcom,q6-mpd" --> For Q6-MPD device "qcom,wcss-mpd" --> For WCSS-MPD device Is this approach fine ? > Given SoC cannot come with different amount of children (PD) and > different amount of radios. You even fix the firmware name, so > boards/customers cannot use anything else. It's fixed and the only > variable element here is disabling some of the blocks per board, if they > do not have physical interface (e.g. radio). > > You even hard-code the number of PD by using "pd-[123]", without unit > address, so you do not expect it will grow. > > Unless you want to say that these are devices? But your binding does not > really suggest it... > > > Yes, as i mentioned above the requirement is to have device specific bindings. We will remove "PD-X" from soc dtsi and add it in board dts file. So soc dts would have "Q6-MPD" master node & board dts have "WCSS-MPD" child nodes based on number of radio's connected on board. Is this fine ? Thanks & Regards, Manikanta.
On 30/06/2023 09:12, Manikanta Mylavarapu wrote: > > > On 6/24/2023 12:49 PM, Krzysztof Kozlowski wrote: >> On 21/06/2023 13:39, Manikanta Mylavarapu wrote: >>>>> on number of wcss radios connected on that board and only one instance >>>>> of 'qcom,ipq5018-q6-mpd'. >>>>> >>>> >>>> I don't understand why the user protection domains need a specific >>>> compatible. Why do they need compatible at all? >>>> >>>> Not mentioning that amount of your domains on Q6 is actually fixed per >>>> SoC and probably should not be in DT at all. >>>> >>> root domain is fixed per soc (One Q6 DSP, one per soc) >>> user domain(s) are variable (based on number of wcss radios attached) >>> >>> The sequence to initialize, bring up, tear down the Q6 and wcss radios >>> are completely different. So in order to differentiate them, we will >>> need different compatibles. So this is a new rproc driver/architecture >>> which has a parent/child topology (Q6 DSP -> Master/parent controls >>> the WCSS (child)). >> >> I understand you need different properties, but I don't see yet the >> benefit of creating here artificial compatibles. Look at your ipq9574 >> DTSI change - it does not use even ipq9574 compatibles for 66% of its >> children. >> >> Maybe you should have there just property describing type of device or >> bringup? >> > > Yeah i got your point. Indeed the requirement here is to > have device specific compatibles, so will have just two > compatible one for Q6-MPD and another for WCSS-MPD device's > > > "qcom,q6-mpd" --> For Q6-MPD device > "qcom,wcss-mpd" --> For WCSS-MPD device > > Is this approach fine ? Can you fix your reply style, so it is like on every mailing list? Some weird indentation does no help to read it. I was proposing to drop compatibles entirely. Making compatibles generic is not solving any of my concerns. I don't understand what do you want to achieve here and very limited description of the hardware in the binding does not help. > >> Given SoC cannot come with different amount of children (PD) and >> different amount of radios. You even fix the firmware name, so >> boards/customers cannot use anything else. It's fixed and the only >> variable element here is disabling some of the blocks per board, if they >> do not have physical interface (e.g. radio). >> >> You even hard-code the number of PD by using "pd-[123]", without unit >> address, so you do not expect it will grow. >> >> Unless you want to say that these are devices? But your binding does not >> really suggest it... >> >> >> Yes, as i mentioned above the requirement is to have device What requirement? You did not describe anything. Binding describes hardware, not your requirements. Binding said nothing about devices. > specific bindings. We will remove "PD-X" from soc dtsi and > add it in board dts file. Why? How is it related to the bindings? What does it solve? Instead of doing some changes you should explain why. > > So soc dts would have "Q6-MPD" master node & board dts have > "WCSS-MPD" child nodes based on number of radio's connected > on board. > > Is this fine ? > Why? Best regards, Krzysztof
On 7/1/2023 4:21 PM, Krzysztof Kozlowski wrote: > On 30/06/2023 09:12, Manikanta Mylavarapu wrote: >> >> >> On 6/24/2023 12:49 PM, Krzysztof Kozlowski wrote: >>> On 21/06/2023 13:39, Manikanta Mylavarapu wrote: >>>>>> on number of wcss radios connected on that board and only one instance >>>>>> of 'qcom,ipq5018-q6-mpd'. >>>>>> >>>>> >>>>> I don't understand why the user protection domains need a specific >>>>> compatible. Why do they need compatible at all? >>>>> >>>>> Not mentioning that amount of your domains on Q6 is actually fixed per >>>>> SoC and probably should not be in DT at all. >>>>> >>>> root domain is fixed per soc (One Q6 DSP, one per soc) >>>> user domain(s) are variable (based on number of wcss radios attached) >>>> >>>> The sequence to initialize, bring up, tear down the Q6 and wcss radios >>>> are completely different. So in order to differentiate them, we will >>>> need different compatibles. So this is a new rproc driver/architecture >>>> which has a parent/child topology (Q6 DSP -> Master/parent controls >>>> the WCSS (child)). >>> >>> I understand you need different properties, but I don't see yet the >>> benefit of creating here artificial compatibles. Look at your ipq9574 >>> DTSI change - it does not use even ipq9574 compatibles for 66% of its >>> children. >>> >>> Maybe you should have there just property describing type of device or >>> bringup? >>> >> >> Yeah i got your point. Indeed the requirement here is to >> have device specific compatibles, so will have just two >> compatible one for Q6-MPD and another for WCSS-MPD device's >> >> >> "qcom,q6-mpd" --> For Q6-MPD device >> "qcom,wcss-mpd" --> For WCSS-MPD device >> >> Is this approach fine ? > > Can you fix your reply style, so it is like on every mailing list? Some > weird indentation does no help to read it. > Sure, i will change my reply style and don't use any indentation. Sorry for inconvenience. > I was proposing to drop compatibles entirely. Making compatibles generic > is not solving any of my concerns. > > I don't understand what do you want to achieve here and very limited > description of the hardware in the binding does not help. > Sure, i remove user pd compatibles. In root pd probe itself, user pd remoteproc's are taken care. Also I updated diagram in cover page, it gives enough information. >> >>> Given SoC cannot come with different amount of children (PD) and >>> different amount of radios. You even fix the firmware name, so >>> boards/customers cannot use anything else. It's fixed and the only >>> variable element here is disabling some of the blocks per board, if they >>> do not have physical interface (e.g. radio). >>> >>> You even hard-code the number of PD by using "pd-[123]", without unit >>> address, so you do not expect it will grow. >>> >>> Unless you want to say that these are devices? But your binding does not >>> really suggest it... >>> >>> >>> Yes, as i mentioned above the requirement is to have device > > What requirement? You did not describe anything. Binding describes > hardware, not your requirements. > > Binding said nothing about devices. > Yeah got your point. So we removed userpd compatibles from dt-bindings. >> specific bindings. We will remove "PD-X" from soc dtsi and >> add it in board dts file. > > Why? How is it related to the bindings? What does it solve? Instead of > doing some changes you should explain why. > >> >> So soc dts would have "Q6-MPD" master node & board dts have >> "WCSS-MPD" child nodes based on number of radio's connected >> on board. >> >> Is this fine ? >> > > Why? > "PD-X" controls user pd WCSS bring up. WCSS blocks may vary based on number of radio's connected on board but QDSP6 is always present. So i will keep Q6 node in soc dtsi file and move 'PD-X' node to board dts file. Thanks & Regards, Manikanta. > Best regards, > Krzysztof >
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@kernel.org> + - Mathieu Poirier <mathieu.poirier@linaro.org> + +description: + Multipd Peripheral Image Loader loads firmware and boots Q6 pd, WCSS pd + remoteproc's on the Qualcomm IPQ5018, IPQ9574 SoC. Pd means protection + domain. 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 + + 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. + + properties: + compatible: + enum: + - qcom,ipq5018-wcss-ahb-mpd + - qcom,ipq9574-wcss-ahb-mpd + - qcom,ipq5018-wcss-pcie-mpd + + firmware-name: + $ref: /schemas/types.yaml#/definitions/string-array + items: + - description: Firmware name of the Hexagon core + + interrupts-extended: + items: + - description: Fatal interrupt + - description: Ready interrupt + - description: Spawn acknowledge interrupt + - description: Stop acknowledge interrupt + + 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 + + - if: + properties: + compatible: + contains: + enum: + - qcom,ipq9574-q6-mpd + then: + properties: + firmware-name: + items: + - const: IPQ9574/q6_fw.mdt + - const: IPQ9574/m3_fw.mdt + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + q6v5_wcss: remoteproc@cd00000 { + compatible = "qcom,ipq5018-q6-mpd"; + reg = <0x0cd00000 0x4040>; + firmware-name = "IPQ5018/q6_fw.mdt", + "IPQ5018/m3_fw.mdt", + "qcn6122/m3_fw.mdt"; + interrupts-extended = <&intc GIC_SPI 291 IRQ_TYPE_EDGE_RISING>, + <&wcss_smp2p_in 0 0>, + <&wcss_smp2p_in 1 0>, + <&wcss_smp2p_in 2 0>, + <&wcss_smp2p_in 3 0>; + interrupt-names = "wdog", + "fatal", + "ready", + "handover", + "stop-ack"; + + qcom,smem-states = <&wcss_smp2p_out 0>, + <&wcss_smp2p_out 1>; + qcom,smem-state-names = "shutdown", + "stop"; + + memory-region = <&q6_region>; + + glink-edge { + interrupts = <GIC_SPI 179 IRQ_TYPE_EDGE_RISING>; + label = "rtr"; + qcom,remote-pid = <1>; + mboxes = <&apcs_glb 8>; + }; + + pd-1 { + compatible = "qcom,ipq5018-wcss-ahb-mpd"; + firmware-name = "IPQ5018/q6_fw.mdt"; + interrupts-extended = <&wcss_smp2p_in 8 0>, + <&wcss_smp2p_in 9 0>, + <&wcss_smp2p_in 12 0>, + <&wcss_smp2p_in 11 0>; + interrupt-names = "fatal", + "ready", + "spawn-ack", + "stop-ack"; + qcom,smem-states = <&wcss_smp2p_out 8>, + <&wcss_smp2p_out 9>, + <&wcss_smp2p_out 10>; + qcom,smem-state-names = "shutdown", + "stop", + "spawn"; + }; + + pd-2 { + compatible = "qcom,ipq5018-wcss-pcie-mpd"; + firmware-name = "IPQ5018/q6_fw.mdt"; + interrupts-extended = <&wcss_smp2p_in 16 0>, + <&wcss_smp2p_in 17 0>, + <&wcss_smp2p_in 20 0>, + <&wcss_smp2p_in 19 0>; + interrupt-names = "fatal", + "ready", + "spawn-ack", + "stop-ack"; + + qcom,smem-states = <&wcss_smp2p_out 16>, + <&wcss_smp2p_out 17>, + <&wcss_smp2p_out 18>; + qcom,smem-state-names = "shutdown", + "stop", + "spawn"; + }; + + pd-3 { + compatible = "qcom,ipq5018-wcss-pcie-mpd"; + firmware-name = "IPQ5018/q6_fw.mdt"; + interrupts-extended = <&wcss_smp2p_in 24 0>, + <&wcss_smp2p_in 25 0>, + <&wcss_smp2p_in 28 0>, + <&wcss_smp2p_in 27 0>; + interrupt-names = "fatal", + "ready", + "spawn-ack", + "stop-ack"; + + qcom,smem-states = <&wcss_smp2p_out 24>, + <&wcss_smp2p_out 25>, + <&wcss_smp2p_out 26>; + qcom,smem-state-names = "shutdown", + "stop", + "spawn"; + }; + };
Add new binding document for multipd model remoteproc. IPQ5018, IPQ9574 follows multipd model. Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> --- 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 -- 2.17.1