| Message ID | 1678164097-13247-1-git-send-email-quic_mmanikan@quicinc.com |
|---|---|
| Headers | show |
| Series | Add multipd remoteproc support | expand |
On 3/7/2023 6:53 PM, Rob Herring wrote: > > On Tue, 07 Mar 2023 10:11:27 +0530, 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> >> --- >> .../bindings/remoteproc/qcom,multipd-pil.yaml | 282 ++++++++++++++++++ >> 1 file changed, 282 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml >> > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.example.dts:22:18: fatal error: dt-bindings/clock/qcom,gcc-ipq5018.h: No such file or directory > 22 | #include <dt-bindings/clock/qcom,gcc-ipq5018.h> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > compilation terminated. > make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.example.dtb] Error 1 > make[1]: *** Waiting for unfinished jobs.... > make: *** [Makefile:1512: dt_binding_check] Error 2 > > doc reference errors (make refcheckdocs): > > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1678164097-13247-2-git-send-email-quic_mmanikan@quicinc.com > > The base for the series is generally the latest rc1. A different dependency > should be noted in *this* patch. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit after running the above command yourself. Note > that DT_SCHEMA_FILES can be set to your schema file to speed up checking > your schema. However, it must be unset to test all examples with your schema. > I mentioned dependency link (https://lore.kernel.org/linux-arm-msm/20220621161126.15883-1-quic_srichara@quicinc.com/) in cover page patch because it's required for entire series. I will add dependency link's and raise new patchset. Thanks & Regards, Manikanta.
On 03/05/2023 12:59, Manikanta Mylavarapu wrote: > > > On 3/7/2023 6:53 PM, Rob Herring wrote: >> >> On Tue, 07 Mar 2023 10:11:27 +0530, 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> >>> --- >>> .../bindings/remoteproc/qcom,multipd-pil.yaml | 282 ++++++++++++++++++ >>> 1 file changed, 282 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml >>> >> >> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' >> on your patch (DT_CHECKER_FLAGS is new in v5.13): >> >> yamllint warnings/errors: >> >> dtschema/dtc warnings/errors: >> Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.example.dts:22:18: fatal error: dt-bindings/clock/qcom,gcc-ipq5018.h: No such file or directory >> 22 | #include <dt-bindings/clock/qcom,gcc-ipq5018.h> >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> compilation terminated. >> make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.example.dtb] Error 1 >> make[1]: *** Waiting for unfinished jobs.... >> make: *** [Makefile:1512: dt_binding_check] Error 2 >> >> doc reference errors (make refcheckdocs): >> >> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1678164097-13247-2-git-send-email-quic_mmanikan@quicinc.com >> >> The base for the series is generally the latest rc1. A different dependency >> should be noted in *this* patch. >> >> If you already ran 'make dt_binding_check' and didn't see the above >> error(s), then make sure 'yamllint' is installed and dt-schema is up to >> date: >> >> pip3 install dtschema --upgrade >> >> Please check and re-submit after running the above command yourself. Note >> that DT_SCHEMA_FILES can be set to your schema file to speed up checking >> your schema. However, it must be unset to test all examples with your schema. >> > > I mentioned dependency link > (https://lore.kernel.org/linux-arm-msm/20220621161126.15883-1-quic_srichara@quicinc.com/) > in cover page patch because it's required for entire series. I will add > dependency link's and raise new patchset. Is the dependency merged for v6.4-rc1? Looks not, so this means the patch cannot be merged for next three months. Why do you need any dependency here in this binding? Best regards, Krzysztof
On 3/7/2023 8:47 PM, Krzysztof Kozlowski wrote: > On 07/03/2023 05:41, 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> >> --- >> .../bindings/remoteproc/qcom,multipd-pil.yaml | 282 ++++++++++++++++++ >> 1 file changed, 282 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..b788607f5abd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml >> @@ -0,0 +1,282 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-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. > > What is a "pd"? > 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. Here we have two pd's called root and user pd. We can correlate Root pd as root and user pd as user in linux. Root pd has more privileges than user pd. From remoteproc driver perspective, root pd corresponds to QDSP6 processor bring up and user pd corresponds to Wifi radio (WCSS) bring up. >> + >> +properties: >> + compatible: >> + enum: >> + - qcom,ipq5018-q6-mpd >> + - qcom,ipq9574-q6-mpd >> + >> + '#address-cells': true >> + >> + '#size-cells': true > > Why do you need both? > > If really needed, these should be const. > It's not required. I am going to remove it. >> + >> + 'ranges': true >> + > > Same question - why do you need it? > It's not required. I am going to remove it. >> + reg: >> + maxItems: 1 >> + >> + interrupts-extended: > > Instead interrupts > Sure. I will use '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: >> + minItems: 25 >> + maxItems: 25 > > Drop both and instead describe the items. Anyway minItems are not needed > here. > Sure. I will drop min & max items and describe clocks. >> + >> + clock-names: >> + minItems: 25 >> + maxItems: 25 > > Drop both and instead list the names. > Sure. I will drop. >> + >> + assigned-clocks: >> + minItems: 13 >> + maxItems: 13 > > Drop, they do not have to be mentioned in the binding. If you think they > need to, then why? > >> + >> + assigned-clock-rates: >> + minItems: 13 >> + maxItems: 13 > > Ditto > >> + >> + qcom,smem-states: >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + description: States used by the AP to signal the remoteprocessor >> + items: >> + - description: Shutdown Q6 >> + - description: Stop Q6 >> + >> + qcom,smem-state-names: >> + description: >> + Names of the states used by the AP to signal the remoteprocessor >> + items: >> + - const: shutdown >> + - const: stop >> + >> + memory-region: >> + items: >> + - description: Q6 pd reserved region >> + >> + glink-edge: >> + $ref: /schemas/remoteproc/qcom,glink-edge.yaml# > > unevaluatedProperties: false > Sure, will add. >> + description: >> + Qualcomm G-Link subnode which represents communication edge, channels >> + and devices related to the Modem. >> + >> +patternProperties: >> + "^remoteproc_pd1|remoteproc_pd2|remoteproc_pd3": > > No, underscores are not allowed. Also, what is pd? > Sure, will remove underscores. >> + 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" > > Drop quotes Sure, will remove it. > >> + >> + interrupts-extended: > > Same as before > Sure, will use 'interrupts'. >> + 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 remoteprocessor >> + 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 remoteprocessor > > remote processor > I will update. >> + items: >> + - const: shutdown >> + - const: stop >> + - const: spawn > > This is confusing. Why your children have the same properties as parent? > Here both parent & child considered as remote processor. So once they powered up/power down/crashed, they used to do some handshaking with APPS processor. So interrupts are common between parent i.e root pd and child i.e user pd >> + >> + required: >> + - compatible >> + >> + additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + - interrupts-extended > > interrupts > Sure. I will use 'interrupts' instead of interrupts-extended >> + - interrupt-names >> + - qcom,smem-states >> + - qcom,smem-state-names >> + - memory-region >> + >> +additionalProperties: false >> + >> +allOf: >> + - if: >> + properties: >> + compatible: >> + enum: >> + - qcom,ipq9574-q6-mpd >> + then: >> + properties: >> + assigned-clocks: >> + items: >> + - description: Phandle, clock specifier of GCC_ANOC_WCSS_AXI_M_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_AHB_S_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_ECAHB_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_ACMT_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_AXI_M_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AXIM_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AXIM2_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AHB_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AHB_S_CLK >> + - description: Phandle, clock specifier of GCC_Q6SS_BOOT_CLK >> + - description: Phandle, clock specifier of GCC_MEM_NOC_Q6_AXI_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_Q6_TBU_CLK >> + - description: Phandle, clock specifier of GCC_SYS_NOC_WCSS_AHB_CLK > > Eh, so here they are. But Why? Do you expect different clocks for > others? If so, where are they? > > Anyway, drop useless "Phandle, clock specifier of". Clocks cannot be > anything else than phandle and a clock specifier. Instead of using some > cryptic ACRONYM_OR_SOME_CLK, describe them. Just like we do for other > bindings. You have plenty of good examples, so please start from them. > > >> + assigned-clock-rates: >> + items: >> + - description: Must be 266666667 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 266666667 HZ >> + - description: Must be 533000000 HZ >> + - description: Must be 342857143 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 342857143 HZ >> + - description: Must be 533000000 HZ >> + - description: Must be 533000000 HZ >> + - description: Must be 133333333 HZ > > ??? > > If these are fixed, why this is in DT? DT is for variable and > non-discoverable pieces and you do not have here anything variable, but > fixed. > >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/clock/qcom,gcc-ipq5018.h> >> + #include <dt-bindings/reset/qcom,gcc-ipq5018.h> > > Use 4 spaces for example indentation. > Sure, will use 4 spaces. >> + >> + q6v5_wcss: remoteproc@cd00000 { >> + compatible = "qcom,ipq5018-q6-mpd"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + reg = <0x0cd00000 0x4040>; >> + interrupts-extended = <&intc GIC_SPI 291 IRQ_TYPE_EDGE_RISING>, >> + <&wcss_smp2p_in 0 0>, > > Wrong alignment of indentation > Sure, will update alignment. >> + <&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>; >> + }; >> + >> + q6_wcss_pd1: remoteproc_pd1 { >> + compatible = "qcom,ipq5018-wcss-ahb-mpd"; >> + 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"; >> + }; >> + >> + q6_wcss_pd2: remoteproc_pd2 { >> + compatible = "qcom,ipq5018-wcss-pcie-mpd"; >> + 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"; >> + status = "okay"; > > Drop statuses from the example. > Sure, will drop status property. > > Best regards, > Krzysztof > Thanks & Regards, Manikanta.
On 3/7/2023 7:56 PM, Rob Herring wrote: > On Tue, Mar 07, 2023 at 10:11:27AM +0530, 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> >> --- >> .../bindings/remoteproc/qcom,multipd-pil.yaml | 282 ++++++++++++++++++ >> 1 file changed, 282 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..b788607f5abd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml >> @@ -0,0 +1,282 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-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. > > What is PD? I don't see it defined anywhere. > 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. Here we have two pd's called root and user pd. We can correlate Root pd as root and user pd as user in linux. Root pd has more privileges than user pd. From remoteproc driver perspective, root pd corresponds to QDSP6 processor bring up and user pd corresponds to Wifi radio (WCSS) bring up. I will try to add this info in cover page. >> + >> +properties: >> + compatible: >> + enum: >> + - qcom,ipq5018-q6-mpd >> + - qcom,ipq9574-q6-mpd >> + >> + '#address-cells': true > > Need to define the size. > >> + >> + '#size-cells': true > > ditto > It's not required. I am going to remove it. >> + >> + 'ranges': true > > Don't need quotes > It's not required. I am going to remove it. >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts-extended: > > Just 'interrupts'. Both forms are always supported. > Sure, will use '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: >> + minItems: 25 >> + maxItems: 25 > > You need to list out what the clocks are. > Sure. I will do. >> + >> + clock-names: >> + minItems: 25 >> + maxItems: 25 >> + >> + assigned-clocks: > > You can drop this. Implicitly supported. > >> + minItems: 13 >> + maxItems: 13 >> + >> + assigned-clock-rates: >> + minItems: 13 >> + maxItems: 13 >> + >> + qcom,smem-states: >> + $ref: /schemas/types.yaml#/definitions/phandle-array > > I believe this already has a type. It should be defined in a common > schema if not already and then included in this schema. > >> + description: States used by the AP to signal the remoteprocessor >> + items: >> + - description: Shutdown Q6 >> + - description: Stop Q6 >> + >> + qcom,smem-state-names: >> + description: >> + Names of the states used by the AP to signal the remoteprocessor >> + 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: >> + "^remoteproc_pd1|remoteproc_pd2|remoteproc_pd3": >> + 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" > > Don't need quotes. > I will remove it. >> + - "qcom,ipq9574-wcss-ahb-mpd" >> + - "qcom,ipq5018-wcss-pcie-mpd" >> + >> + interrupts-extended: > > Just interrupts > I will use 'interrupts' >> + 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 remoteprocessor >> + 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 remoteprocessor >> + items: >> + - const: shutdown >> + - const: stop >> + - const: spawn >> + >> + required: >> + - compatible >> + >> + additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + - interrupts-extended >> + - interrupt-names >> + - qcom,smem-states >> + - qcom,smem-state-names >> + - memory-region >> + >> +additionalProperties: false >> + >> +allOf: >> + - if: >> + properties: >> + compatible: >> + enum: >> + - qcom,ipq9574-q6-mpd >> + then: >> + properties: >> + assigned-clocks: > > Don't need to define assigned-clocks > >> + items: >> + - description: Phandle, clock specifier of GCC_ANOC_WCSS_AXI_M_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_AHB_S_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_ECAHB_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_ACMT_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_AXI_M_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AXIM_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AXIM2_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AHB_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AHB_S_CLK >> + - description: Phandle, clock specifier of GCC_Q6SS_BOOT_CLK >> + - description: Phandle, clock specifier of GCC_MEM_NOC_Q6_AXI_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_Q6_TBU_CLK >> + - description: Phandle, clock specifier of GCC_SYS_NOC_WCSS_AHB_CLK >> + assigned-clock-rates: >> + items: >> + - description: Must be 266666667 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 266666667 HZ >> + - description: Must be 533000000 HZ >> + - description: Must be 342857143 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 342857143 HZ >> + - description: Must be 533000000 HZ >> + - description: Must be 533000000 HZ >> + - description: Must be 133333333 HZ >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/clock/qcom,gcc-ipq5018.h> >> + #include <dt-bindings/reset/qcom,gcc-ipq5018.h> >> + >> + q6v5_wcss: remoteproc@cd00000 { >> + compatible = "qcom,ipq5018-q6-mpd"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + reg = <0x0cd00000 0x4040>; >> + 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>; >> + }; >> + >> + q6_wcss_pd1: remoteproc_pd1 { >> + compatible = "qcom,ipq5018-wcss-ahb-mpd"; >> + 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"; >> + }; >> + >> + q6_wcss_pd2: remoteproc_pd2 { >> + compatible = "qcom,ipq5018-wcss-pcie-mpd"; >> + 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"; >> + status = "okay"; > > Don't need status in examples. > I will remove status property. Thanks & Regards, Manikanta. >> + }; >> + >> + q6_wcss_pd3: remoteproc_pd3 { >> + compatible = "qcom,ipq5018-wcss-pcie-mpd"; >> + 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"; >> + status = "okay"; >> + }; >> + }; >> -- >> 2.34.1 >>
On 3/7/2023 8:49 PM, Krzysztof Kozlowski wrote: > On 07/03/2023 05:41, Manikanta Mylavarapu wrote: >> Add support for the QDSP6 gcc clock control used on IPQ9574 >> based devices. This would allow mpd remoteproc driver to control >> the required gcc clocks to bring the subsystem out of reset. >> >> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> >> --- >> include/dt-bindings/clock/qcom,ipq9574-gcc.h | 159 ++++++++++--------- >> 1 file changed, 83 insertions(+), 76 deletions(-) >> >> diff --git a/include/dt-bindings/clock/qcom,ipq9574-gcc.h b/include/dt-bindings/clock/qcom,ipq9574-gcc.h >> index c89e96d568c6..8bd6350ecd56 100644 >> --- a/include/dt-bindings/clock/qcom,ipq9574-gcc.h >> +++ b/include/dt-bindings/clock/qcom,ipq9574-gcc.h >> @@ -138,80 +138,87 @@ >> #define WCSS_AHB_CLK_SRC 129 >> #define GCC_Q6_AHB_CLK 130 >> #define GCC_Q6_AHB_S_CLK 131 >> -#define GCC_WCSS_ECAHB_CLK 132 >> -#define GCC_WCSS_ACMT_CLK 133 > > That's an ABI break, if file was accepted. Or a very weird change > anyway, if it wasn't (why adding entry and immediately changing it?). > > Best regards, > Krzysztof > I will add new macros at the end instead in middle. Thanks & Regards, Manikanta.
On 3/7/2023 8:47 PM, Krzysztof Kozlowski wrote: > On 07/03/2023 05:41, 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> >> --- >> .../bindings/remoteproc/qcom,multipd-pil.yaml | 282 ++++++++++++++++++ >> 1 file changed, 282 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..b788607f5abd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml >> @@ -0,0 +1,282 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-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. > > What is a "pd"? > >> + >> +properties: >> + compatible: >> + enum: >> + - qcom,ipq5018-q6-mpd >> + - qcom,ipq9574-q6-mpd >> + >> + '#address-cells': true >> + >> + '#size-cells': true > > Why do you need both? > > If really needed, these should be const. > >> + >> + 'ranges': true >> + > > Same question - why do you need it? > >> + reg: >> + maxItems: 1 >> + >> + interrupts-extended: > > Instead 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: >> + minItems: 25 >> + maxItems: 25 > > Drop both and instead describe the items. Anyway minItems are not needed > here. > >> + >> + clock-names: >> + minItems: 25 >> + maxItems: 25 > > Drop both and instead list the names. > >> + >> + assigned-clocks: >> + minItems: 13 >> + maxItems: 13 > > Drop, they do not have to be mentioned in the binding. If you think they > need to, then why? > >> + >> + assigned-clock-rates: >> + minItems: 13 >> + maxItems: 13 > > Ditto > Clocks in multipd architecture will be handled by QDSP6 firmware. So i am going to remove clock handling. >> + >> + qcom,smem-states: >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + description: States used by the AP to signal the remoteprocessor >> + items: >> + - description: Shutdown Q6 >> + - description: Stop Q6 >> + >> + qcom,smem-state-names: >> + description: >> + Names of the states used by the AP to signal the remoteprocessor >> + items: >> + - const: shutdown >> + - const: stop >> + >> + memory-region: >> + items: >> + - description: Q6 pd reserved region >> + >> + glink-edge: >> + $ref: /schemas/remoteproc/qcom,glink-edge.yaml# > > unevaluatedProperties: false > >> + description: >> + Qualcomm G-Link subnode which represents communication edge, channels >> + and devices related to the Modem. >> + >> +patternProperties: >> + "^remoteproc_pd1|remoteproc_pd2|remoteproc_pd3": > > No, underscores are not allowed. Also, what is pd? > >> + 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" > > Drop quotes > >> + >> + interrupts-extended: > > Same as before > >> + 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 remoteprocessor >> + 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 remoteprocessor > > remote processor > >> + items: >> + - const: shutdown >> + - const: stop >> + - const: spawn > > This is confusing. Why your children have the same properties as parent? > >> + >> + required: >> + - compatible >> + >> + additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + - interrupts-extended > > interrupts > >> + - interrupt-names >> + - qcom,smem-states >> + - qcom,smem-state-names >> + - memory-region >> + >> +additionalProperties: false >> + >> +allOf: >> + - if: >> + properties: >> + compatible: >> + enum: >> + - qcom,ipq9574-q6-mpd >> + then: >> + properties: >> + assigned-clocks: >> + items: >> + - description: Phandle, clock specifier of GCC_ANOC_WCSS_AXI_M_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_AHB_S_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_ECAHB_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_ACMT_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_AXI_M_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AXIM_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AXIM2_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AHB_CLK >> + - description: Phandle, clock specifier of GCC_Q6_AHB_S_CLK >> + - description: Phandle, clock specifier of GCC_Q6SS_BOOT_CLK >> + - description: Phandle, clock specifier of GCC_MEM_NOC_Q6_AXI_CLK >> + - description: Phandle, clock specifier of GCC_WCSS_Q6_TBU_CLK >> + - description: Phandle, clock specifier of GCC_SYS_NOC_WCSS_AHB_CLK > > Eh, so here they are. But Why? Do you expect different clocks for > others? If so, where are they? > > Anyway, drop useless "Phandle, clock specifier of". Clocks cannot be > anything else than phandle and a clock specifier. Instead of using some > cryptic ACRONYM_OR_SOME_CLK, describe them. Just like we do for other > bindings. You have plenty of good examples, so please start from them. > > Clocks in multipd architecture will be handled by QDSP6 firmware. So i am going to remove clock handling. >> + assigned-clock-rates: >> + items: >> + - description: Must be 266666667 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 266666667 HZ >> + - description: Must be 533000000 HZ >> + - description: Must be 342857143 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 133333333 HZ >> + - description: Must be 342857143 HZ >> + - description: Must be 533000000 HZ >> + - description: Must be 533000000 HZ >> + - description: Must be 133333333 HZ > > ??? > > If these are fixed, why this is in DT? DT is for variable and > non-discoverable pieces and you do not have here anything variable, but > fixed. > Clocks in multipd architecture will be handled by QDSP6 firmware. So i am going to remove clock handling. Thanks & Regards, Manikanta. >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/clock/qcom,gcc-ipq5018.h> >> + #include <dt-bindings/reset/qcom,gcc-ipq5018.h> > > Use 4 spaces for example indentation. > >> + >> + q6v5_wcss: remoteproc@cd00000 { >> + compatible = "qcom,ipq5018-q6-mpd"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + reg = <0x0cd00000 0x4040>; >> + interrupts-extended = <&intc GIC_SPI 291 IRQ_TYPE_EDGE_RISING>, >> + <&wcss_smp2p_in 0 0>, > > Wrong alignment of indentation > >> + <&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>; >> + }; >> + >> + q6_wcss_pd1: remoteproc_pd1 { >> + compatible = "qcom,ipq5018-wcss-ahb-mpd"; >> + 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"; >> + }; >> + >> + q6_wcss_pd2: remoteproc_pd2 { >> + compatible = "qcom,ipq5018-wcss-pcie-mpd"; >> + 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"; >> + status = "okay"; > > Drop statuses from the example. > > > Best regards, > Krzysztof >
On 3/7/2023 11:37 AM, Kathiravan T wrote: > > On 3/7/2023 10:11 AM, Manikanta Mylavarapu wrote: >> Enable IPQ5018 APCS IPC support by adding the compatible. >> >> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> >> --- >> drivers/mailbox/qcom-apcs-ipc-mailbox.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c >> b/drivers/mailbox/qcom-apcs-ipc-mailbox.c >> index 6bbf87c6d60b..0b873c76fd7e 100644 >> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c >> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c >> @@ -141,6 +141,7 @@ static int qcom_apcs_ipc_remove(struct >> platform_device *pdev) >> /* .data is the offset of the ipc register within the global block */ >> static const struct of_device_id qcom_apcs_ipc_of_match[] = { >> + { .compatible = "qcom,ipq5018-apcs-apps-global", .data = >> &ipq6018_apcs_data }, > > With the bindings updated, you can drop this patch. > > Thanks, Kathiravan T. > Sure, I will drop this patch. Thanks & Regards, Manikanta.
On 5/8/2023 7:59 PM, Manikanta Mylavarapu wrote: > > > On 3/7/2023 8:49 PM, Krzysztof Kozlowski wrote: >> On 07/03/2023 05:41, Manikanta Mylavarapu wrote: >>> Add support for the QDSP6 gcc clock control used on IPQ9574 >>> based devices. This would allow mpd remoteproc driver to control >>> the required gcc clocks to bring the subsystem out of reset. >>> >>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> >>> --- >>> include/dt-bindings/clock/qcom,ipq9574-gcc.h | 159 ++++++++++--------- >>> 1 file changed, 83 insertions(+), 76 deletions(-) >>> >>> diff --git a/include/dt-bindings/clock/qcom,ipq9574-gcc.h >>> b/include/dt-bindings/clock/qcom,ipq9574-gcc.h >>> index c89e96d568c6..8bd6350ecd56 100644 >>> --- a/include/dt-bindings/clock/qcom,ipq9574-gcc.h >>> +++ b/include/dt-bindings/clock/qcom,ipq9574-gcc.h >>> @@ -138,80 +138,87 @@ >>> #define WCSS_AHB_CLK_SRC 129 >>> #define GCC_Q6_AHB_CLK 130 >>> #define GCC_Q6_AHB_S_CLK 131 >>> -#define GCC_WCSS_ECAHB_CLK 132 >>> -#define GCC_WCSS_ACMT_CLK 133 >> >> That's an ABI break, if file was accepted. Or a very weird change >> anyway, if it wasn't (why adding entry and immediately changing it?). >> >> Best regards, >> Krzysztof >> > > I will add new macros at the end instead in middle. > > Thanks & Regards, > Manikanta. Clocks in multipd architecture will be handled by QDSP6 firmware. So i am going to remove clock macros and drop this patch. Thanks & Regards, Manikanta.
On 3/7/2023 7:45 PM, Kathiravan T wrote: > > On 3/7/2023 10:11 AM, Manikanta Mylavarapu wrote: >> Add initial device tree support for the MP03.5-C1 board. >> >> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/Makefile | 1 + >> .../arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts | 64 +++++++++++++++++++ >> 2 files changed, 65 insertions(+) >> create mode 100644 arch/arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts >> >> diff --git a/arch/arm64/boot/dts/qcom/Makefile >> b/arch/arm64/boot/dts/qcom/Makefile >> index b77a95e97a56..10d1eafe57e4 100644 >> --- a/arch/arm64/boot/dts/qcom/Makefile >> +++ b/arch/arm64/boot/dts/qcom/Makefile >> @@ -4,6 +4,7 @@ dtb-$(CONFIG_ARCH_QCOM) += >> apq8094-sony-xperia-kitakami-karin_windy.dtb >> dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb >> dtb-$(CONFIG_ARCH_QCOM) += apq8096-ifc6640.dtb >> dtb-$(CONFIG_ARCH_QCOM) += ipq5018-mp03.1-c2.dtb >> +dtb-$(CONFIG_ARCH_QCOM) += ipq5018-mp03.5-c1.dtb >> dtb-$(CONFIG_ARCH_QCOM) += ipq6018-cp01-c1.dtb >> dtb-$(CONFIG_ARCH_QCOM) += ipq8074-hk01.dtb >> dtb-$(CONFIG_ARCH_QCOM) += ipq8074-hk10-c1.dtb >> diff --git a/arch/arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts >> b/arch/arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts >> new file mode 100644 >> index 000000000000..51ddd7367ac6 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts >> @@ -0,0 +1,64 @@ >> +// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause >> +/* >> + * IPQ5018 CP01 board device tree source >> + * >> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. >> + */ >> + >> +/dts-v1/; >> + >> +#include "ipq5018.dtsi" >> + >> +/ { >> + model = "Qualcomm Technologies, Inc. IPQ5018/AP-MP03.5-C1"; >> + compatible = "qcom,ipq5018-mp03.5-c1", "qcom,ipq5018"; >> + >> + aliases { >> + serial0 = &blsp1_uart1; >> + }; >> + >> + chosen { >> + bootargs = "console=ttyMSM0,115200,n8 rw init=/init swiotlb=1 >> coherent_pool=2M"; > > > Is the bootargs required? > > No, i will remove it. >> + stdout-path = "serial0:115200n8"; >> + }; >> +}; >> + >> +&tlmm { >> + blsp0_uart_pins: uart_pins { > > > node name should be uart-state {. Didn't dtbs_check complain it or am I > missing something? > > Yeah, it should be. I will correct it. >> + pins = "gpio20", "gpio21"; >> + function = "blsp0_uart0"; >> + bias-disable; >> + }; >> +}; >> + >> +&blsp1_uart1 { >> + pinctrl-0 = <&blsp0_uart_pins>; >> + pinctrl-names = "default"; >> + status = "ok"; > > > s/ok/okay/ > > I will update. >> +}; >> + >> +&q6v5_wcss { >> + q6_wcss_pd1: remoteproc_pd1 { >> + interrupts-extended = <&wcss_smp2p_in 8 0>, >> + <&wcss_smp2p_in 9 0>, >> + <&wcss_smp2p_in 12 0>, >> + <&wcss_smp2p_in 11 0>; > > > please align all these entries > > I will do it. >> + 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"; >> + }; > > > leave a blank line > > Sure, I will do it. >> + q6_wcss_pd2: remoteproc_pd2 { >> + status = "okay"; >> + }; >> + >> + q6_wcss_pd3: remoteproc_pd3 { >> + status = "okay"; >> + }; >> +};
On 3/7/2023 9:12 PM, Krzysztof Kozlowski wrote: > On 07/03/2023 05:41, Manikanta Mylavarapu wrote: >> Add initial device tree support for the MP03.5-C1 board. >> >> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/Makefile | 1 + >> .../arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts | 64 +++++++++++++++++++ >> 2 files changed, 65 insertions(+) >> create mode 100644 arch/arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts >> >> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile >> index b77a95e97a56..10d1eafe57e4 100644 >> --- a/arch/arm64/boot/dts/qcom/Makefile >> +++ b/arch/arm64/boot/dts/qcom/Makefile >> @@ -4,6 +4,7 @@ dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb >> dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb >> dtb-$(CONFIG_ARCH_QCOM) += apq8096-ifc6640.dtb >> dtb-$(CONFIG_ARCH_QCOM) += ipq5018-mp03.1-c2.dtb >> +dtb-$(CONFIG_ARCH_QCOM) += ipq5018-mp03.5-c1.dtb >> dtb-$(CONFIG_ARCH_QCOM) += ipq6018-cp01-c1.dtb >> dtb-$(CONFIG_ARCH_QCOM) += ipq8074-hk01.dtb >> dtb-$(CONFIG_ARCH_QCOM) += ipq8074-hk10-c1.dtb >> diff --git a/arch/arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts b/arch/arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts >> new file mode 100644 >> index 000000000000..51ddd7367ac6 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts >> @@ -0,0 +1,64 @@ >> +// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause >> +/* >> + * IPQ5018 CP01 board device tree source >> + * >> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. >> + */ >> + >> +/dts-v1/; >> + >> +#include "ipq5018.dtsi" >> + >> +/ { >> + model = "Qualcomm Technologies, Inc. IPQ5018/AP-MP03.5-C1"; >> + compatible = "qcom,ipq5018-mp03.5-c1", "qcom,ipq5018"; >> + >> + aliases { >> + serial0 = &blsp1_uart1; >> + }; >> + >> + chosen { >> + bootargs = "console=ttyMSM0,115200,n8 rw init=/init swiotlb=1 coherent_pool=2M"; > > Not a common DT property. Drop. > Sure, i will drop it. >> + stdout-path = "serial0:115200n8"; >> + }; >> +}; >> + >> +&tlmm { >> + blsp0_uart_pins: uart_pins { > > Does not look like you tested the DTS against bindings. Please run `make > dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst > for instructions). > > I'll stop the review. Please first run tests. > > I will verify DTS against bindings. Thanks & Regards, Manikanta. > > Best regards, > Krzysztof >
On 5/3/2023 9:57 PM, Krzysztof Kozlowski wrote: > On 03/05/2023 12:59, Manikanta Mylavarapu wrote: >> >> >> On 3/7/2023 6:53 PM, Rob Herring wrote: >>> >>> On Tue, 07 Mar 2023 10:11:27 +0530, 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> >>>> --- >>>> .../bindings/remoteproc/qcom,multipd-pil.yaml | 282 ++++++++++++++++++ >>>> 1 file changed, 282 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml >>>> >>> >>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' >>> on your patch (DT_CHECKER_FLAGS is new in v5.13): >>> >>> yamllint warnings/errors: >>> >>> dtschema/dtc warnings/errors: >>> Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.example.dts:22:18: fatal error: dt-bindings/clock/qcom,gcc-ipq5018.h: No such file or directory >>> 22 | #include <dt-bindings/clock/qcom,gcc-ipq5018.h> >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> compilation terminated. >>> make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.example.dtb] Error 1 >>> make[1]: *** Waiting for unfinished jobs.... >>> make: *** [Makefile:1512: dt_binding_check] Error 2 >>> >>> doc reference errors (make refcheckdocs): >>> >>> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1678164097-13247-2-git-send-email-quic_mmanikan@quicinc.com >>> >>> The base for the series is generally the latest rc1. A different dependency >>> should be noted in *this* patch. >>> >>> If you already ran 'make dt_binding_check' and didn't see the above >>> error(s), then make sure 'yamllint' is installed and dt-schema is up to >>> date: >>> >>> pip3 install dtschema --upgrade >>> >>> Please check and re-submit after running the above command yourself. Note >>> that DT_SCHEMA_FILES can be set to your schema file to speed up checking >>> your schema. However, it must be unset to test all examples with your schema. >>> >> >> I mentioned dependency link >> (https://lore.kernel.org/linux-arm-msm/20220621161126.15883-1-quic_srichara@quicinc.com/) >> in cover page patch because it's required for entire series. I will add >> dependency link's and raise new patchset. > > Is the dependency merged for v6.4-rc1? Looks not, so this means the > patch cannot be merged for next three months. > > Why do you need any dependency here in this binding? > > Best regards, > Krzysztof > Since i removed gcc clocks from DTS nodes, dt-bindings are not required. I will remove it. Thanks & Regards, Manikanta.
This patch is based on the IPQ5018, IPQ9574 baseport patches available here https://lore.kernel.org/linux-arm-msm/20220621161126.15883-1-quic_srichara@quicinc.com/ https://lore.kernel.org/linux-arm-msm/20230217142030.16012-1-quic_devipriy@quicinc.com/ APSS brings Q6 out of reset and then Q6 brings WCSS block (wifi radio's) out of reset. --------------- --> |WiFi 2G radio| | -------------- | -------- ------- | | APSS | ---> |QDSP6| -----| --------- ------- | | | | -------------- --> |WiFi 5G radio| -------------- Problem here is if any radio crashes, subsequently other radio also should crash because Q6 crashed. Let's say 2G radio crashed, Q6 should pass this info to APSS. Only Q6 processor interrupts registered with APSS. Obviously Q6 should crash and raise fatal interrupt to APSS. Due to this 5G radio also crashed. But no issue in 5G radio, because of 2G radio crash 5G radio also impacted. In multi pd model, this problem is resolved. Here WCSS functionality (WiFi radio's) moved out from Q6 root pd to a separate user pd. Due to this, radio's independently pass their status info to APPS with out crashing Q6. So other radio's won't be impacted. --------- |WiFi | --> |2G radio| | --------- ------ Start Q6 ------- | | | ------------------> | | | | | Start WCSS PD1 (2G) | | | |APSS| ----------------------->|QDSP6|-----| | | Start WCSS PD1 (5G) | | | | ----------------------->| |-----| ------ ------- | | | ----------- |-->|WiFi | |5G radio | ----------- According to linux terminology, here consider Q6 as root i.e it provide all services, WCSS (wifi radio's) as user i.e it uses services provided by root. Since Q6 root & WCSS user pd's able to communicate with APSS individually, multipd remoteproc driver registers each PD with rproc framework. Here clients (Wifi host drivers) intrested on WCSS PD rproc, so multipd driver start's root pd in the context of WCSS user pd rproc start. Similarly on down path, root pd will be stopped after wcss user pd stopped. Here WCSS(user) PD is dependent on Q6(root) PD, so first q6 pd should be up before wcss pd. After wcss pd goes down, q6 pd should be turned off. rproc->ops->start(userpd_rproc) { /* Boot root pd rproc */ rproc_boot(upd_dev->parent); --- /* user pd rproc start sequence */ --- --- } With this way we ensure that root pd brought up before userpd. rproc->ops->stop(userpd_rproc) { --- --- /* user pd rproc stop sequence */ --- --- /* Shutdown root pd rproc */ rproc_shutdown(upd_dev->parent); } After userpd rproc stops, root pd rproc will be stopped. IPQ5018, IPQ9574 supports multipd remoteproc driver. Manikanta Mylavarapu (11): dt-bindings: remoteproc: qcom: Add support for multipd model dt-bindings: mailbox: qcom: Add IPQ5018 APCS compatible dt-bindings: scm: Add compatible for IPQ5018 dt-bindings: arm: qcom: Add ipq5018-mp03.5-c1 dt-bindings: clock: qcom: gcc-ipq9574: Add Q6 gcc clock control clk: qcom: IPQ9574: Add q6/wcss clocks mailbox: qcom-apcs-ipc: Add IPQ5018 APCS IPC support remoteproc: qcom: Add Hexagon based multipd rproc driver arm64: dtsi: qcom: ipq5018: enable nodes required for multipd arm64: dts: qcom: ipq5018: Add MP03.5-c1 board support arm64: dtsi: qcom: ipq9574: Add nodes to bring up multipd .../devicetree/bindings/arm/qcom.yaml | 1 + .../bindings/firmware/qcom,scm.yaml | 1 + .../mailbox/qcom,apcs-kpss-global.yaml | 2 + .../bindings/remoteproc/qcom,multipd-pil.yaml | 282 ++++++++ arch/arm64/boot/dts/qcom/Makefile | 1 + .../arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts | 64 ++ arch/arm64/boot/dts/qcom/ipq5018.dtsi | 130 ++++ arch/arm64/boot/dts/qcom/ipq9574.dtsi | 145 ++++ drivers/clk/qcom/gcc-ipq9574.c | 119 ++++ drivers/firmware/qcom_scm.c | 114 +++ drivers/firmware/qcom_scm.h | 6 + drivers/mailbox/qcom-apcs-ipc-mailbox.c | 1 + drivers/remoteproc/Kconfig | 20 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/qcom_common.c | 23 + drivers/remoteproc/qcom_common.h | 1 + drivers/remoteproc/qcom_q6v5.c | 41 +- drivers/remoteproc/qcom_q6v5.h | 15 +- drivers/remoteproc/qcom_q6v5_adsp.c | 5 +- drivers/remoteproc/qcom_q6v5_mpd.c | 668 ++++++++++++++++++ drivers/remoteproc/qcom_q6v5_mss.c | 4 +- drivers/remoteproc/qcom_q6v5_pas.c | 3 +- drivers/soc/qcom/mdt_loader.c | 314 ++++++++ include/dt-bindings/clock/qcom,ipq9574-gcc.h | 159 +++-- include/linux/firmware/qcom/qcom_scm.h | 3 + include/linux/soc/qcom/mdt_loader.h | 19 + 26 files changed, 2057 insertions(+), 85 deletions(-) create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,multipd-pil.yaml create mode 100644 arch/arm64/boot/dts/qcom/ipq5018-mp03.5-c1.dts create mode 100644 drivers/remoteproc/qcom_q6v5_mpd.c -- 2.34.1