Messages in this thread | | | Date | Tue, 9 May 2023 22:16:36 +0530 | Subject | Re: [PATCH 01/11] dt-bindings: remoteproc: qcom: Add support for multipd model | From | Manikanta Mylavarapu <> |
| |
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 >
| |