[PATCH v3 0/2] Add support for proxy interconnect bandwidth votes

[PATCH v3 0/2] Add support for proxy interconnect bandwidth votes

[Sibi Sankar]

Add proxy interconnect bandwidth votes during modem bootup on SC7280 SoCs.

V3:
 * Re-ordered clock list, fixed pdc_sync typo [Rob/Matthias]

V2:
 * Dropped patch 3 from version 1 [Sub with Bjorn's patch]
 * Add YAML support [Krzysztof]
 * Drop interconnect names [Bjorn]

Sibi Sankar (2):
  arm64: dts: qcom: sc7280: Add proxy interconnect requirements for
    modem
  dt-bindings: remoteproc: qcom: Add SC7280 MSS bindings

 .../bindings/remoteproc/qcom,sc7280-mss-pil.yaml   | 261 +++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi |   1 +
 2 files changed, 262 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml

-- 
2.7.4

[PATCH v3 1/2] arm64: dts: qcom: sc7280: Add proxy interconnect requirements for modem

[Sibi Sankar]

Add interconnects that are required to be proxy voted upon during modem
bootup on SC7280 SoCs.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
index 9f4a9c263c35..91aad86cc708 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
@@ -88,6 +88,7 @@
 	status = "okay";
 	compatible = "qcom,sc7280-mss-pil";
 	iommus = <&apps_smmu 0x124 0x0>, <&apps_smmu 0x488 0x7>;
+	interconnects = <&mc_virt MASTER_LLCC 0 &mc_virt SLAVE_EBI1 0>;
 	memory-region = <&mba_mem>, <&mpss_mem>;
 };
 
-- 
2.7.4

Re: [PATCH v3 1/2] arm64: dts: qcom: sc7280: Add proxy interconnect requirements for modem

[Stephen Boyd]

Quoting Sibi Sankar (2022-05-11 01:19:21)
> Add interconnects that are required to be proxy voted upon during modem
> bootup on SC7280 SoCs.
>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH v3 2/2] dt-bindings: remoteproc: qcom: Add SC7280 MSS bindings

[Sibi Sankar]

Add MSS PIL loading bindings for SC7280 SoCs.

Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

v3:
 * Re-ordered clock list, fixed pdc_sync typo [Rob/Matthias]

 .../bindings/remoteproc/qcom,sc7280-mss-pil.yaml   | 261 +++++++++++++++++++++
 1 file changed, 261 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
new file mode 100644
index 000000000000..2f95bfd7b3eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
@@ -0,0 +1,261 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/qcom,sc7280-mss-pil.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SC7280 MSS Peripheral Image Loader
+
+maintainers:
+  - Sibi Sankar <quic_sibis@quicinc.com>
+
+description:
+  This document defines the binding for a component that loads and boots firmware
+  on the Qualcomm Technology Inc. SC7280 Modem Hexagon Core.
+
+properties:
+  compatible:
+    enum:
+      - qcom,sc7280-mss-pil
+
+  reg:
+    items:
+      - description: MSS QDSP6 registers
+      - description: RMB registers
+
+  reg-names:
+    items:
+      - const: qdsp6
+      - const: rmb
+
+  iommus:
+    items:
+      - description: MSA Stream 1
+      - description: MSA Stream 2
+
+  interconnects:
+    items:
+      - description: Path leading to system memory
+
+  interrupts:
+    items:
+      - description: Watchdog interrupt
+      - description: Fatal interrupt
+      - description: Ready interrupt
+      - description: Handover interrupt
+      - description: Stop acknowledge interrupt
+      - description: Shutdown acknowledge interrupt
+
+  interrupt-names:
+    items:
+      - const: wdog
+      - const: fatal
+      - const: ready
+      - const: handover
+      - const: stop-ack
+      - const: shutdown-ack
+
+  clocks:
+    items:
+      - description: GCC MSS IFACE clock
+      - description: GCC MSS OFFLINE clock
+      - description: GCC MSS SNOC_AXI clock
+      - description: RPMH PKA clock
+      - description: RPMH XO clock
+
+  clock-names:
+    items:
+      - const: iface
+      - const: offline
+      - const: snoc_axi
+      - const: pka
+      - const: xo
+
+  power-domains:
+    items:
+      - description: CX power domain
+      - description: MSS power domain
+
+  power-domain-names:
+    items:
+      - const: cx
+      - const: mss
+
+  resets:
+    items:
+      - description: AOSS restart
+      - description: PDC reset
+
+  reset-names:
+    items:
+      - const: mss_restart
+      - const: pdc_reset
+
+  memory-region:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: Phandle reference to the reserved-memory for the MBA region followed
+                 by the modem region.
+
+  firmware-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      The name of the firmware which should be loaded for this remote
+      processor.
+
+  qcom,halt-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Phandle reference to a syscon representing TCSR followed by the
+      four offsets within syscon for q6, modem, nc and vq6 halt registers.
+
+  qcom,ext-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Two phandle references to syscons representing TCSR_REG and TCSR register
+      space followed by the two offsets within the syscon to force_clk_en/rscc_disable
+      and axim1_clk_off/crypto_clk_off registers respectively.
+
+  qcom,qaccept-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Phandle reference to a syscon representing TCSR followed by the
+      three offsets within syscon for mdm, cx and axi qaccept registers.
+
+  qcom,qmp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Reference to the AOSS side-channel message RAM.
+
+  qcom,smem-states:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: States used by the AP to signal the Hexagon core
+    items:
+      - description: Stop the modem
+
+  qcom,smem-state-names:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: The names of the state bits used for SMP2P output
+    const: stop
+
+  glink-edge:
+    type: object
+    description: |
+      Qualcomm G-Link subnode which represents communication edge, channels
+      and devices related to the DSP.
+
+    properties:
+      interrupts:
+        items:
+          - description: IRQ from MSS to GLINK
+
+      mboxes:
+        items:
+          - description: Mailbox for communication between APPS and MSS
+
+      label:
+        description: The names of the state bits used for SMP2P output
+        items:
+          - const: modem
+
+      qcom,remote-pid:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: ID of the shared memory used by GLINK for communication with MSS
+
+    required:
+      - interrupts
+      - mboxes
+      - label
+      - qcom,remote-pid
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - iommus
+  - interconnects
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+  - power-domains
+  - power-domain-names
+  - resets
+  - reset-names
+  - qcom,halt-regs
+  - qcom,ext-regs
+  - qcom,qaccept-regs
+  - memory-region
+  - qcom,qmp
+  - qcom,smem-states
+  - qcom,smem-state-names
+  - glink-edge
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
+    #include <dt-bindings/clock/qcom,rpmh.h>
+    #include <dt-bindings/interconnect/qcom,sc7280.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/mailbox/qcom-ipcc.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
+    #include <dt-bindings/reset/qcom,sdm845-aoss.h>
+    #include <dt-bindings/reset/qcom,sdm845-pdc.h>
+
+    remoteproc_mpss: remoteproc@4080000 {
+        compatible = "qcom,sc7280-mss-pil";
+        reg = <0x04080000 0x10000>, <0x04180000 0x48>;
+        reg-names = "qdsp6", "rmb";
+
+        iommus = <&apps_smmu 0x124 0x0>, <&apps_smmu 0x488 0x7>;
+
+        interconnects = <&mc_virt MASTER_LLCC 0 &mc_virt SLAVE_EBI1 0>;
+
+        interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>,
+                              <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
+                              <&modem_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
+                              <&modem_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
+                              <&modem_smp2p_in 3 IRQ_TYPE_EDGE_RISING>,
+                              <&modem_smp2p_in 7 IRQ_TYPE_EDGE_RISING>;
+
+        interrupt-names = "wdog", "fatal", "ready", "handover",
+                          "stop-ack", "shutdown-ack";
+
+        clocks = <&gcc GCC_MSS_CFG_AHB_CLK>,
+                 <&gcc GCC_MSS_OFFLINE_AXI_CLK>,
+                 <&gcc GCC_MSS_SNOC_AXI_CLK>,
+                 <&rpmhcc RPMH_PKA_CLK>,
+                 <&rpmhcc RPMH_CXO_CLK>;
+        clock-names = "iface", "offline", "snoc_axi", "pka", "xo";
+
+        power-domains = <&rpmhpd SC7280_CX>,
+                        <&rpmhpd SC7280_MSS>;
+        power-domain-names = "cx", "mss";
+
+        memory-region = <&mba_mem>, <&mpss_mem>;
+
+        qcom,qmp = <&aoss_qmp>;
+
+        qcom,smem-states = <&modem_smp2p_out 0>;
+        qcom,smem-state-names = "stop";
+
+        resets = <&aoss_reset AOSS_CC_MSS_RESTART>,
+                 <&pdc_reset PDC_MODEM_SYNC_RESET>;
+        reset-names = "mss_restart", "pdc_reset";
+
+        qcom,halt-regs = <&tcsr_mutex 0x23000 0x25000 0x28000 0x33000>;
+        qcom,ext-regs = <&tcsr 0x10000 0x10004 &tcsr_mutex 0x26004 0x26008>;
+        qcom,qaccept-regs = <&tcsr_mutex 0x23030 0x23040 0x23020>;
+
+        glink-edge {
+            interrupts-extended = <&ipcc IPCC_CLIENT_MPSS
+                                   IPCC_MPROC_SIGNAL_GLINK_QMP
+                                   IRQ_TYPE_EDGE_RISING>;
+            mboxes = <&ipcc IPCC_CLIENT_MPSS
+                      IPCC_MPROC_SIGNAL_GLINK_QMP>;
+            label = "modem";
+            qcom,remote-pid = <1>;
+        };
+    };
-- 
2.7.4

Re: [PATCH v3 2/2] dt-bindings: remoteproc: qcom: Add SC7280 MSS bindings

[Matthias Kaehlcke]

Hi Sibi,

On Wed, May 11, 2022 at 01:49:22PM +0530, Sibi Sankar wrote:
> Add MSS PIL loading bindings for SC7280 SoCs.
> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>

There is already a binding for 'qcom,sc7280-mss-pil' in
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt. Shouldn't
the entries from that file be deleted?

> 
> v3:
>  * Re-ordered clock list, fixed pdc_sync typo [Rob/Matthias]
> 
>  .../bindings/remoteproc/qcom,sc7280-mss-pil.yaml   | 261 +++++++++++++++++++++
>  1 file changed, 261 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
> new file mode 100644
> index 000000000000..2f95bfd7b3eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
> @@ -0,0 +1,261 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,sc7280-mss-pil.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SC7280 MSS Peripheral Image Loader
> +
> +maintainers:
> +  - Sibi Sankar <quic_sibis@quicinc.com>
> +
> +description:
> +  This document defines the binding for a component that loads and boots firmware
> +  on the Qualcomm Technology Inc. SC7280 Modem Hexagon Core.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sc7280-mss-pil
> +
> +  reg:
> +    items:
> +      - description: MSS QDSP6 registers
> +      - description: RMB registers
> +
> +  reg-names:
> +    items:
> +      - const: qdsp6
> +      - const: rmb
> +
> +  iommus:
> +    items:
> +      - description: MSA Stream 1
> +      - description: MSA Stream 2
> +
> +  interconnects:
> +    items:
> +      - description: Path leading to system memory
> +
> +  interrupts:
> +    items:
> +      - description: Watchdog interrupt
> +      - description: Fatal interrupt
> +      - description: Ready interrupt
> +      - description: Handover interrupt
> +      - description: Stop acknowledge interrupt
> +      - description: Shutdown acknowledge interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: wdog
> +      - const: fatal
> +      - const: ready
> +      - const: handover
> +      - const: stop-ack
> +      - const: shutdown-ack


The existing binding (qcom,q6v5.txt) also has:

- interrupts-extended:
        Usage: required
	Value type: <prop-encoded-array>
	Definition: reference to the interrupts that match interrupt-names

That's covered implicitly by 'interrupts' I suppose?

> +
> +  clocks:
> +    items:
> +      - description: GCC MSS IFACE clock
> +      - description: GCC MSS OFFLINE clock
> +      - description: GCC MSS SNOC_AXI clock
> +      - description: RPMH PKA clock
> +      - description: RPMH XO clock
> +
> +  clock-names:
> +    items:
> +      - const: iface
> +      - const: offline
> +      - const: snoc_axi
> +      - const: pka
> +      - const: xo
> +
> +  power-domains:
> +    items:
> +      - description: CX power domain
> +      - description: MSS power domain
> +
> +  power-domain-names:
> +    items:
> +      - const: cx
> +      - const: mss
> +
> +  resets:
> +    items:
> +      - description: AOSS restart
> +      - description: PDC reset
> +
> +  reset-names:
> +    items:
> +      - const: mss_restart
> +      - const: pdc_reset
> +
> +  memory-region:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: Phandle reference to the reserved-memory for the MBA region followed
> +                 by the modem region.
> +
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      The name of the firmware which should be loaded for this remote
> +      processor.
> +
> +  qcom,halt-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Phandle reference to a syscon representing TCSR followed by the
> +      four offsets within syscon for q6, modem, nc and vq6 halt registers.
> +
> +  qcom,ext-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Two phandle references to syscons representing TCSR_REG and TCSR register
> +      space followed by the two offsets within the syscon to force_clk_en/rscc_disable
> +      and axim1_clk_off/crypto_clk_off registers respectively.
> +
> +  qcom,qaccept-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Phandle reference to a syscon representing TCSR followed by the
> +      three offsets within syscon for mdm, cx and axi qaccept registers.
> +
> +  qcom,qmp:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Reference to the AOSS side-channel message RAM.
> +
> +  qcom,smem-states:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: States used by the AP to signal the Hexagon core
> +    items:
> +      - description: Stop the modem
> +
> +  qcom,smem-state-names:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: The names of the state bits used for SMP2P output
> +    const: stop
> +
> +  glink-edge:
> +    type: object
> +    description: |
> +      Qualcomm G-Link subnode which represents communication edge, channels
> +      and devices related to the DSP.
> +
> +    properties:
> +      interrupts:
> +        items:
> +          - description: IRQ from MSS to GLINK
> +
> +      mboxes:
> +        items:
> +          - description: Mailbox for communication between APPS and MSS
> +
> +      label:
> +        description: The names of the state bits used for SMP2P output
> +        items:
> +          - const: modem
> +
> +      qcom,remote-pid:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: ID of the shared memory used by GLINK for communication with MSS
> +
> +    required:
> +      - interrupts
> +      - mboxes
> +      - label
> +      - qcom,remote-pid
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - iommus
> +  - interconnects
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - power-domain-names
> +  - resets
> +  - reset-names
> +  - qcom,halt-regs
> +  - qcom,ext-regs
> +  - qcom,qaccept-regs
> +  - memory-region
> +  - qcom,qmp

'qcom,qmp' is marked as 'optional' in qcom,q6v5.txt

Re: [PATCH v3 2/2] dt-bindings: remoteproc: qcom: Add SC7280 MSS bindings

[Sibi Sankar]

Hey Matthias,

On 5/11/22 10:29 PM, Matthias Kaehlcke wrote:
> Hi Sibi,
> 
> On Wed, May 11, 2022 at 01:49:22PM +0530, Sibi Sankar wrote:
>> Add MSS PIL loading bindings for SC7280 SoCs.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> 
> There is already a binding for 'qcom,sc7280-mss-pil' in
> Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt. Shouldn't
> the entries from that file be deleted?
> 
>>
>> v3:
>>   * Re-ordered clock list, fixed pdc_sync typo [Rob/Matthias]
>>
>>   .../bindings/remoteproc/qcom,sc7280-mss-pil.yaml   | 261 +++++++++++++++++++++
>>   1 file changed, 261 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
>> new file mode 100644
>> index 000000000000..2f95bfd7b3eb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
>> @@ -0,0 +1,261 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/qcom,sc7280-mss-pil.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm SC7280 MSS Peripheral Image Loader
>> +
>> +maintainers:
>> +  - Sibi Sankar <quic_sibis@quicinc.com>
>> +
>> +description:
>> +  This document defines the binding for a component that loads and boots firmware
>> +  on the Qualcomm Technology Inc. SC7280 Modem Hexagon Core.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,sc7280-mss-pil
>> +
>> +  reg:
>> +    items:
>> +      - description: MSS QDSP6 registers
>> +      - description: RMB registers
>> +
>> +  reg-names:
>> +    items:
>> +      - const: qdsp6
>> +      - const: rmb
>> +
>> +  iommus:
>> +    items:
>> +      - description: MSA Stream 1
>> +      - description: MSA Stream 2
>> +
>> +  interconnects:
>> +    items:
>> +      - description: Path leading to system memory
>> +
>> +  interrupts:
>> +    items:
>> +      - description: Watchdog interrupt
>> +      - description: Fatal interrupt
>> +      - description: Ready interrupt
>> +      - description: Handover interrupt
>> +      - description: Stop acknowledge interrupt
>> +      - description: Shutdown acknowledge interrupt
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: wdog
>> +      - const: fatal
>> +      - const: ready
>> +      - const: handover
>> +      - const: stop-ack
>> +      - const: shutdown-ack
> 
> 
> The existing binding (qcom,q6v5.txt) also has:
> 
> - interrupts-extended:
>          Usage: required
> 	Value type: <prop-encoded-array>
> 	Definition: reference to the interrupts that match interrupt-names
> 
> That's covered implicitly by 'interrupts' I suppose?


Yeah ^^ was discussed before during the sc7280 wpss patch series. Rob
said the tooling handles both the same way.

https://lore.kernel.org/lkml/CAL_Jsq+khyhbwJ5-GPZ5ZGkY4nX_obq4t92Z0V6sZH3Oyj4Fow@mail.gmail.com/


> 
>> +
>> +  clocks:
>> +    items:
>> +      - description: GCC MSS IFACE clock
>> +      - description: GCC MSS OFFLINE clock
>> +      - description: GCC MSS SNOC_AXI clock
>> +      - description: RPMH PKA clock
>> +      - description: RPMH XO clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: iface
>> +      - const: offline
>> +      - const: snoc_axi
>> +      - const: pka
>> +      - const: xo
>> +
>> +  power-domains:
>> +    items:
>> +      - description: CX power domain
>> +      - description: MSS power domain
>> +
>> +  power-domain-names:
>> +    items:
>> +      - const: cx
>> +      - const: mss
>> +
>> +  resets:
>> +    items:
>> +      - description: AOSS restart
>> +      - description: PDC reset
>> +
>> +  reset-names:
>> +    items:
>> +      - const: mss_restart
>> +      - const: pdc_reset
>> +
>> +  memory-region:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: Phandle reference to the reserved-memory for the MBA region followed
>> +                 by the modem region.
>> +
>> +  firmware-name:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description:
>> +      The name of the firmware which should be loaded for this remote
>> +      processor.
>> +
>> +  qcom,halt-regs:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      Phandle reference to a syscon representing TCSR followed by the
>> +      four offsets within syscon for q6, modem, nc and vq6 halt registers.
>> +
>> +  qcom,ext-regs:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      Two phandle references to syscons representing TCSR_REG and TCSR register
>> +      space followed by the two offsets within the syscon to force_clk_en/rscc_disable
>> +      and axim1_clk_off/crypto_clk_off registers respectively.
>> +
>> +  qcom,qaccept-regs:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      Phandle reference to a syscon representing TCSR followed by the
>> +      three offsets within syscon for mdm, cx and axi qaccept registers.
>> +
>> +  qcom,qmp:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: Reference to the AOSS side-channel message RAM.
>> +
>> +  qcom,smem-states:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: States used by the AP to signal the Hexagon core
>> +    items:
>> +      - description: Stop the modem
>> +
>> +  qcom,smem-state-names:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description: The names of the state bits used for SMP2P output
>> +    const: stop
>> +
>> +  glink-edge:
>> +    type: object
>> +    description: |
>> +      Qualcomm G-Link subnode which represents communication edge, channels
>> +      and devices related to the DSP.
>> +
>> +    properties:
>> +      interrupts:
>> +        items:
>> +          - description: IRQ from MSS to GLINK
>> +
>> +      mboxes:
>> +        items:
>> +          - description: Mailbox for communication between APPS and MSS
>> +
>> +      label:
>> +        description: The names of the state bits used for SMP2P output
>> +        items:
>> +          - const: modem
>> +
>> +      qcom,remote-pid:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        description: ID of the shared memory used by GLINK for communication with MSS
>> +
>> +    required:
>> +      - interrupts
>> +      - mboxes
>> +      - label
>> +      - qcom,remote-pid
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - iommus
>> +  - interconnects
>> +  - interrupts
>> +  - interrupt-names
>> +  - clocks
>> +  - clock-names
>> +  - power-domains
>> +  - power-domain-names
>> +  - resets
>> +  - reset-names
>> +  - qcom,halt-regs
>> +  - qcom,ext-regs
>> +  - qcom,qaccept-regs
>> +  - memory-region
>> +  - qcom,qmp
> 
> 'qcom,qmp' is marked as 'optional' in qcom,q6v5.txt

Yeah even though we were forced to mark/implement it as optional in the
original bindings file/driver (since it was a single bindings file
covering all the SoCs), it is functionally required for sc7280 mss to
reach xo-shutdown.

> 

-Sibi

Re: [PATCH v3 2/2] dt-bindings: remoteproc: qcom: Add SC7280 MSS bindings

[Krzysztof Kozlowski]

On 11/05/2022 10:19, Sibi Sankar wrote:
> Add MSS PIL loading bindings for SC7280 SoCs.

Why not converting existing bindings? The compatible is already there,
so you duplicated its binding.

> 
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> ---
> 
> v3:
>  * Re-ordered clock list, fixed pdc_sync typo [Rob/Matthias]
> 
>  .../bindings/remoteproc/qcom,sc7280-mss-pil.yaml   | 261 +++++++++++++++++++++
>  1 file changed, 261 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
> new file mode 100644
> index 000000000000..2f95bfd7b3eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
> @@ -0,0 +1,261 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,sc7280-mss-pil.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SC7280 MSS Peripheral Image Loader
> +
> +maintainers:
> +  - Sibi Sankar <quic_sibis@quicinc.com>
> +
> +description:
> +  This document defines the binding for a component that loads and boots firmware
> +  on the Qualcomm Technology Inc. SC7280 Modem Hexagon Core.

s/This document defines the binding for//
Instead describe the hardware.


Anyway, similar patch was already sent:
https://lore.kernel.org/all/20220511161602.117772-7-sireeshkodali1@gmail.com/
Except its several issues, it is much more complete and specific.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sc7280-mss-pil
> +
> +  reg:
> +    items:
> +      - description: MSS QDSP6 registers
> +      - description: RMB registers
> +
> +  reg-names:
> +    items:
> +      - const: qdsp6
> +      - const: rmb
> +
> +  iommus:
> +    items:
> +      - description: MSA Stream 1
> +      - description: MSA Stream 2
> +
> +  interconnects:
> +    items:
> +      - description: Path leading to system memory
> +
> +  interrupts:
> +    items:
> +      - description: Watchdog interrupt
> +      - description: Fatal interrupt
> +      - description: Ready interrupt
> +      - description: Handover interrupt
> +      - description: Stop acknowledge interrupt
> +      - description: Shutdown acknowledge interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: wdog
> +      - const: fatal
> +      - const: ready
> +      - const: handover
> +      - const: stop-ack
> +      - const: shutdown-ack
> +
> +  clocks:
> +    items:
> +      - description: GCC MSS IFACE clock
> +      - description: GCC MSS OFFLINE clock
> +      - description: GCC MSS SNOC_AXI clock
> +      - description: RPMH PKA clock
> +      - description: RPMH XO clock
> +
> +  clock-names:
> +    items:
> +      - const: iface
> +      - const: offline
> +      - const: snoc_axi
> +      - const: pka
> +      - const: xo
> +
> +  power-domains:
> +    items:
> +      - description: CX power domain
> +      - description: MSS power domain
> +
> +  power-domain-names:
> +    items:
> +      - const: cx
> +      - const: mss
> +
> +  resets:
> +    items:
> +      - description: AOSS restart
> +      - description: PDC reset
> +
> +  reset-names:
> +    items:
> +      - const: mss_restart
> +      - const: pdc_reset
> +
> +  memory-region:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array

This should be defined by core schema and ref should not be needed.

> +    description: Phandle reference to the reserved-memory for the MBA region followed
> +                 by the modem region.

maxItems

> +
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      The name of the firmware which should be loaded for this remote
> +      processor.
> +
> +  qcom,halt-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Phandle reference to a syscon representing TCSR followed by the
> +      four offsets within syscon for q6, modem, nc and vq6 halt registers.
> +
> +  qcom,ext-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Two phandle references to syscons representing TCSR_REG and TCSR register
> +      space followed by the two offsets within the syscon to force_clk_en/rscc_disable
> +      and axim1_clk_off/crypto_clk_off registers respectively.
> +
> +  qcom,qaccept-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Phandle reference to a syscon representing TCSR followed by the
> +      three offsets within syscon for mdm, cx and axi qaccept registers.
> +
> +  qcom,qmp:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Reference to the AOSS side-channel message RAM.
> +
> +  qcom,smem-states:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: States used by the AP to signal the Hexagon core
> +    items:
> +      - description: Stop the modem
> +
> +  qcom,smem-state-names:
> +    $ref: /schemas/types.yaml#/definitions/string

For some reason you decided to make the same mistakes as the
https://lore.kernel.org/all/20220511161602.117772-7-sireeshkodali1@gmail.com/

even though all other bindings with this property looks correct.

Please, re-use existing bindings, do not reinvent things in incorrect way.

I'll stop the review, you need to align first.

What is weird, your v2 was before Sireesh's patch, and you both made the
same mistakes which do not exist in current bindings.

All comments from his set apply here. It seems that his patchset came
after yours and copied stuff from your bindings, so yours would be FIFO,
if you made proper binding conversion.

Best regards,
Krzysztof

Re: [PATCH v3 2/2] dt-bindings: remoteproc: qcom: Add SC7280 MSS bindings

[Sibi Sankar]

Hey Krzysztof,

Thanks for taking time to review the patch series.

On 5/11/22 11:40 PM, Krzysztof Kozlowski wrote:
> On 11/05/2022 10:19, Sibi Sankar wrote:
>> Add MSS PIL loading bindings for SC7280 SoCs.
> 
> Why not converting existing bindings? The compatible is already there,
> so you duplicated its binding.

I'll make sure that all references to the sc7280 mss gets deleted from
the main binding doc in the next-respin.


https://lore.kernel.org/lkml/CAE-0n51KBYjZvwGNy06_okmEWjEfRLQO54CYaY6-JnbBk6kOhA@mail.gmail.com/

https://lore.kernel.org/lkml/YUps1JfGtf6JdbCx@ripper/

Bjorn/Stephen gave the above comments when the wpss bindings was in the 
process of being merged. It was agreed that a single big clunky binding
with a lot of if/else would be confusing and Bjorn wanted a separate
file for it specifically because it overrides a pas compatible. SC7280 
mss satisfies both the requirements.

> 
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>
>> v3:
>>   * Re-ordered clock list, fixed pdc_sync typo [Rob/Matthias]
>>
>>   .../bindings/remoteproc/qcom,sc7280-mss-pil.yaml   | 261 +++++++++++++++++++++
>>   1 file changed, 261 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
>> new file mode 100644
>> index 000000000000..2f95bfd7b3eb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-mss-pil.yaml
>> @@ -0,0 +1,261 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/qcom,sc7280-mss-pil.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm SC7280 MSS Peripheral Image Loader
>> +
>> +maintainers:
>> +  - Sibi Sankar <quic_sibis@quicinc.com>
>> +
>> +description:
>> +  This document defines the binding for a component that loads and boots firmware
>> +  on the Qualcomm Technology Inc. SC7280 Modem Hexagon Core.
> 
> s/This document defines the binding for//
> Instead describe the hardware.

ack

> 
> 
> Anyway, similar patch was already sent:
> https://lore.kernel.org/all/20220511161602.117772-7-sireeshkodali1@gmail.com/
> Except its several issues, it is much more complete and specific.

same reason as detailed above.

> 
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,sc7280-mss-pil
>> +
>> +  reg:
>> +    items:
>> +      - description: MSS QDSP6 registers
>> +      - description: RMB registers
>> +
>> +  reg-names:
>> +    items:
>> +      - const: qdsp6
>> +      - const: rmb
>> +
>> +  iommus:
>> +    items:
>> +      - description: MSA Stream 1
>> +      - description: MSA Stream 2
>> +
>> +  interconnects:
>> +    items:
>> +      - description: Path leading to system memory
>> +
>> +  interrupts:
>> +    items:
>> +      - description: Watchdog interrupt
>> +      - description: Fatal interrupt
>> +      - description: Ready interrupt
>> +      - description: Handover interrupt
>> +      - description: Stop acknowledge interrupt
>> +      - description: Shutdown acknowledge interrupt
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: wdog
>> +      - const: fatal
>> +      - const: ready
>> +      - const: handover
>> +      - const: stop-ack
>> +      - const: shutdown-ack
>> +
>> +  clocks:
>> +    items:
>> +      - description: GCC MSS IFACE clock
>> +      - description: GCC MSS OFFLINE clock
>> +      - description: GCC MSS SNOC_AXI clock
>> +      - description: RPMH PKA clock
>> +      - description: RPMH XO clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: iface
>> +      - const: offline
>> +      - const: snoc_axi
>> +      - const: pka
>> +      - const: xo
>> +
>> +  power-domains:
>> +    items:
>> +      - description: CX power domain
>> +      - description: MSS power domain
>> +
>> +  power-domain-names:
>> +    items:
>> +      - const: cx
>> +      - const: mss
>> +
>> +  resets:
>> +    items:
>> +      - description: AOSS restart
>> +      - description: PDC reset
>> +
>> +  reset-names:
>> +    items:
>> +      - const: mss_restart
>> +      - const: pdc_reset
>> +
>> +  memory-region:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> This should be defined by core schema and ref should not be needed.
> 
>> +    description: Phandle reference to the reserved-memory for the MBA region followed
>> +                 by the modem region.
> 
> maxItems

ack

> 
>> +
>> +  firmware-name:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description:
>> +      The name of the firmware which should be loaded for this remote
>> +      processor.
>> +
>> +  qcom,halt-regs:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      Phandle reference to a syscon representing TCSR followed by the
>> +      four offsets within syscon for q6, modem, nc and vq6 halt registers.
>> +
>> +  qcom,ext-regs:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      Two phandle references to syscons representing TCSR_REG and TCSR register
>> +      space followed by the two offsets within the syscon to force_clk_en/rscc_disable
>> +      and axim1_clk_off/crypto_clk_off registers respectively.
>> +
>> +  qcom,qaccept-regs:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      Phandle reference to a syscon representing TCSR followed by the
>> +      three offsets within syscon for mdm, cx and axi qaccept registers.
>> +
>> +  qcom,qmp:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: Reference to the AOSS side-channel message RAM.
>> +
>> +  qcom,smem-states:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: States used by the AP to signal the Hexagon core
>> +    items:
>> +      - description: Stop the modem
>> +
>> +  qcom,smem-state-names:
>> +    $ref: /schemas/types.yaml#/definitions/string
> 
> For some reason you decided to make the same mistakes as the
> https://lore.kernel.org/all/20220511161602.117772-7-sireeshkodali1@gmail.com/
> 
> even though all other bindings with this property looks correct.
> 
> Please, re-use existing bindings, do not reinvent things in incorrect way.
> 
> I'll stop the review, you need to align first.
> 
> What is weird, your v2 was before Sireesh's patch, and you both made the
> same mistakes which do not exist in current bindings.

I guess he used the qcom,sc7280-wpss-pil.yaml for reference as well lol.
Sure I'll allign with him and who gets to post what.

> 
> All comments from his set apply here. It seems that his patchset came
> after yours and copied stuff from your bindings, so yours would be FIFO,
> if you made proper binding conversion.
> 
> Best regards,
> Krzysztof
>