[PATCH v2 1/7] dt-bindings: soc: qcom: Add remote-pid binding for GLINK SMEM

[PATCH v2 1/7] dt-bindings: soc: qcom: Add remote-pid binding for GLINK SMEM

[Sibi Sankar]

Add missing qcom,remote-pid dt binding required for GLINK SMEM
which specifies the remote endpoint of the GLINK edge.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
index 0b8cc533ca83..59ae603ba520 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
@@ -21,6 +21,11 @@ edge.
 	Definition: should specify the IRQ used by the remote processor to
 		    signal this processor about communication related events
 
+- qcom,remote-pid:
+	Usage: required for glink-smem
+	Value type: <u32>
+	Definition: specifies the identfier of the remote endpoint of this edge
+
 - qcom,rpm-msg-ram:
 	Usage: required for glink-rpm
 	Value type: <prop-encoded-array>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/7] dt-bindings: remoteproc: qcom: Add clock bindings for Q6V5

[Sibi Sankar]

Add missing clock bindings for Q6V5 MSS on SDM845 SoCs.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 .../devicetree/bindings/remoteproc/qcom,q6v5.txt       | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 9ff5b0309417..780adc043b37 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -39,13 +39,17 @@ on the Qualcomm Hexagon core.
 - clocks:
 	Usage: required
 	Value type: <phandle>
-	Definition: reference to the iface, bus and mem clocks to be held on
-		    behalf of the booting of the Hexagon core
+	Definition: reference to the list of 4 clocks for the modem sub-system
+		    reference to the list of 8 clocks for the modem sub-system
+		    on SDM845 SoCs
 
 - clock-names:
 	Usage: required
 	Value type: <stringlist>
-	Definition: must be "iface", "bus", "mem"
+	Definition: must be "iface", "bus", "mem", "xo" for the modem sub-system
+		    must be "iface", "bus", "mem", "gpll0_mss", "snoc_axi",
+		    "mnoc_axi", "prng", "xo" for the modem sub-system on SDM845
+		    SoCs
 
 - resets:
 	Usage: required
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/7] dt-bindings: remoteproc: qcom: Fixup regulator dependencies

[Sibi Sankar]

Fixup regulator supply dependencies for Q6V5 MSS on MSM996 SoCs.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 780adc043b37..98894e6ad456 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -76,7 +76,9 @@ on the Qualcomm Hexagon core.
 	Usage: required
 	Value type: <phandle>
 	Definition: reference to the regulators to be held on behalf of the
-		    booting of the Hexagon core
+		    booting of the Hexagon core on MSM8916 SoCs
+		    reference to the pll-supply regulator to be held on behalf
+		    of the booting of the Hexagon core on MSM8996 SoCs
 
 - qcom,smem-states:
 	Usage: required
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 4/7] dt-bindings: remoteproc: qcom: Add power-domain bindings for Q6V5

[Sibi Sankar]

Add power-domain bindings for Q6V5 MSS on MSM8996 and SDM845 SoCs.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---

v2:
  * Add load_state power-domain
  * List cx and mx power-domains for MSM8996

 .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 98894e6ad456..50695cd86397 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -80,6 +80,22 @@ on the Qualcomm Hexagon core.
 		    reference to the pll-supply regulator to be held on behalf
 		    of the booting of the Hexagon core on MSM8996 SoCs
 
+- power-domains:
+	Usage: required
+	Value type: <phandle>
+	Definition: reference to the list of 2 power-domains for the modem
+		    sub-system on MSM8996 SoCs
+		    reference to the list of 4 power-domains for the modem
+		    sub-system on SDM845 SoCs
+
+- power-domain-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "cx", "mx" for the modem sub-system on MSM8996
+		    SoCs
+		    must be "cx", "mx", "mss", "load_state" for the modem
+		    sub-system on SDM845 SoCs
+
 - qcom,smem-states:
 	Usage: required
 	Value type: <phandle>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 5/7] arm64: dts: qcom: sdm845: Add Q6V5 MSS node

[Sibi Sankar]

This patch adds Q6V5 MSS remoteproc node for SDM845 SoCs.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---

v2:
  * Fixed style changes
  * Added missing clocks in the dt-bindings
  * Split mss remoteproc node into a number of patches

This patch depends on the mpss and mba memory regions and pdc reset node.
https://patchwork.kernel.org/patch/10662089/
https://patchwork.kernel.org/patch/10657325/

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 52 ++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 5da9fa1feb8a..33ff8668828f 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1366,6 +1366,58 @@
 			};
 		};
 
+		remoteproc@4080000 {
+			compatible = "qcom,sdm845-mss-pil";
+			reg = <0x04080000 0x408>, <0x04180000 0x48>;
+			reg-names = "qdsp6", "rmb";
+
+			interrupts-extended =
+				<&intc GIC_SPI 266 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>;
+			interrupt-names = "wdog", "fatal", "ready",
+					  "handover", "stop-ack";
+
+			clocks = <&gcc GCC_MSS_CFG_AHB_CLK>,
+				 <&gcc GCC_MSS_Q6_MEMNOC_AXI_CLK>,
+				 <&gcc GCC_BOOT_ROM_AHB_CLK>,
+				 <&gcc GCC_MSS_GPLL0_DIV_CLK_SRC>,
+				 <&gcc GCC_MSS_SNOC_AXI_CLK>,
+				 <&gcc GCC_MSS_MFAB_AXIS_CLK>,
+				 <&gcc GCC_PRNG_AHB_CLK>,
+				 <&rpmhcc RPMH_CXO_CLK>;
+			clock-names = "iface", "bus", "mem", "gpll0_mss",
+				      "snoc_axi", "mnoc_axi", "prng", "xo";
+
+			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_regs
+						0x23000 0x25000 0x24000>;
+
+			mba {
+				memory-region = <&mba_region>;
+			};
+
+			mpss {
+				memory-region = <&mpss_region>;
+			};
+
+			glink-edge {
+				interrupts = <GIC_SPI 449 IRQ_TYPE_EDGE_RISING>;
+				label = "modem";
+				qcom,remote-pid = <1>;
+				mboxes = <&apss_shared 12>;
+				mbox-names = "mpss_smem";
+			};
+		};
+
 		usb_1_hsphy: phy@88e2000 {
 			compatible = "qcom,sdm845-qusb2-phy";
 			reg = <0x88e2000 0x400>;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 6/7] arm64: dts: qcom: sdm845: Add power-domain for Q6V5 MSS node

[Sibi Sankar]

Add power-domains cx, mx, mss and load_state for Q6V5 MSS node.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---

This patch depends on the following bindings:
https://patchwork.kernel.org/patch/10725801/ - rpmhpd dt bindings
https://patchwork.kernel.org/patch/10725793/ - rpmhpd dt node
https://patchwork.kernel.org/patch/10678301/ - AOP QMP dt bindings

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 33ff8668828f..56f5f55db9e2 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1401,6 +1401,12 @@
 			qcom,halt-regs = <&tcsr_mutex_regs
 						0x23000 0x25000 0x24000>;
 
+			power-domains = <&aoss_qmp_pd AOSS_QMP_LS_MODEM>,
+					<&rpmhpd SDM845_CX>,
+					<&rpmhpd SDM845_MX>,
+					<&rpmhpd SDM845_MSS>;
+			power-domain-names = "load_state", "cx", "mx", "mss";
+
 			mba {
 				memory-region = <&mba_region>;
 			};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 7/7] arm64: dts: qcom: sdm845: Add shutdown-ack for Q6V5 MSS node

[Sibi Sankar]

Add shutdown-ack smp2p interrupt for Q6V5 MSS node.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---

This patch depends on:
https://patchwork.kernel.org/patch/10691213/ - shutdown-ack dt bindings

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 56f5f55db9e2..db17216a5bce 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1376,9 +1376,11 @@
 				<&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 3 IRQ_TYPE_EDGE_RISING>,
+				<&modem_smp2p_in 7 IRQ_TYPE_EDGE_RISING>;
 			interrupt-names = "wdog", "fatal", "ready",
-					  "handover", "stop-ack";
+					  "handover", "stop-ack",
+					  "shutdown-ack";
 
 			clocks = <&gcc GCC_MSS_CFG_AHB_CLK>,
 				 <&gcc GCC_MSS_Q6_MEMNOC_AXI_CLK>,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 1/7] dt-bindings: soc: qcom: Add remote-pid binding for GLINK SMEM

[Doug Anderson]

Hi,

On Mon, Dec 17, 2018 at 2:07 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Add missing qcom,remote-pid dt binding required for GLINK SMEM
> which specifies the remote endpoint of the GLINK edge.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 5 +++++
>  1 file changed, 5 insertions(+)

Fixes: 2b41d6c8e696 ("dt-bindings: soc: qcom: Extend GLINK to cover SMEM")


> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> index 0b8cc533ca83..59ae603ba520 100644
> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
> @@ -21,6 +21,11 @@ edge.
>         Definition: should specify the IRQ used by the remote processor to
>                     signal this processor about communication related events
>
> +- qcom,remote-pid:
> +       Usage: required for glink-smem
> +       Value type: <u32>
> +       Definition: specifies the identfier of the remote endpoint of this edge

s/identfier/identifier/


Other than the typo this seems right to me.  Feel free to add my
Reviewed-by tag when that's fixed.


-Doug

Re: [PATCH v2 2/7] dt-bindings: remoteproc: qcom: Add clock bindings for Q6V5

[Doug Anderson]

Hi,

On Mon, Dec 17, 2018 at 2:07 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Add missing clock bindings for Q6V5 MSS on SDM845 SoCs.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt       | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

Fixes: 9f058fa2efb1 ("remoteproc: qcom: Add support for mss remoteproc
on msm8996")
Fixes: fb22022ff63d ("dt-bindings: remoteproc: Add Q6v5 Modem PIL
binding for SDM845")

...it probably doesn't matter too much but if we wanted to be really
careful we could split into two patches, one for the msm8996 and one
for sdm845.  I don't think people care that much about stable
backports of bindings though (someone can feel free to correct me)...


> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 9ff5b0309417..780adc043b37 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -39,13 +39,17 @@ on the Qualcomm Hexagon core.
>  - clocks:
>         Usage: required
>         Value type: <phandle>
> -       Definition: reference to the iface, bus and mem clocks to be held on
> -                   behalf of the booting of the Hexagon core
> +       Definition: reference to the list of 4 clocks for the modem sub-system
> +                   reference to the list of 8 clocks for the modem sub-system
> +                   on SDM845 SoCs

The above is confusing because you don't list the SoCs that are
supposed to use the 4 clocks.  How about instead:

Definition: reference to the clocks that match clock-names


>  - clock-names:
>         Usage: required
>         Value type: <stringlist>
> -       Definition: must be "iface", "bus", "mem"
> +       Definition: must be "iface", "bus", "mem", "xo" for the modem sub-system
> +                   must be "iface", "bus", "mem", "gpll0_mss", "snoc_axi",
> +                   "mnoc_axi", "prng", "xo" for the modem sub-system on SDM845
> +                   SoCs

Same here where it's confusing.  ...but also, it it correct?  As far
as I can tell you're missing msm8996.  It's better to just be explicit
and list each one, ideally without all the prose.

Definition: The clocks needed depend on the compatible string:

qcom,sdm845-mss-pil: "xo", "prng", "iface", "snoc_axi", "bus", "mem",
"gpll0_mss", "mnoc_axi"
qcom,msm8996-mss-pil: "xo", "pnoc", "iface", "bus", "mem", "gpll0_mss_clk"
qcom,msm8974-mss-pil: "xo", "iface", "bus", "mem"
qcom,msm8916-mss-pil: "xo", "iface", "bus", "mem"
qcom,q6v5-pil: "xo", "iface", "bus", "mem"

...as far as I can tell this binding is supposed to account for
"qcom,ipq8074-wcss-pil" too but it seems that one doesn't have
clock-names.


-Doug

Re: [PATCH v2 3/7] dt-bindings: remoteproc: qcom: Fixup regulator dependencies

[Doug Anderson]

Hi,

On Mon, Dec 17, 2018 at 2:08 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Fixup regulator supply dependencies for Q6V5 MSS on MSM996 SoCs.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 780adc043b37..98894e6ad456 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -76,7 +76,9 @@ on the Qualcomm Hexagon core.
>         Usage: required
>         Value type: <phandle>
>         Definition: reference to the regulators to be held on behalf of the
> -                   booting of the Hexagon core
> +                   booting of the Hexagon core on MSM8916 SoCs
> +                   reference to the pll-supply regulator to be held on behalf
> +                   of the booting of the Hexagon core on MSM8996 SoCs

The prose gets in the way and doesn't add anything.  I also don't
understand what you're saying for msm8996.  You're saying that
"pll-supply" is required there but none of the others?  That doesn't
seem to be true in the code I have in front of me, but maybe I'm
missing some patch.  For me, I'd write:

For the compatible strings below the following supplies are required:
  "qcom,q6v5-pil"
  "qcom,msm8916-mss-pil",
  "qcom,msm8974-mss-pil"
- cx-supply:
- mss-supply:
- mx-supply:
- pll-supply:
Usage: required
Value type: <phandle>
Definition: reference to the regulators to be held on behalf of the
    booting of the Hexagon core


...and if msm8996 actually needs "pll-supply", you could add in...

For the compatible strings below the following supplies are required:
  "qcom,msm8996-mss-pil"
- pll-supply:
Usage: required
Value type: <phandle>
Definition: reference to the regulators to be held on behalf of the
    booting of the Hexagon core

Re: [PATCH v2 4/7] dt-bindings: remoteproc: qcom: Add power-domain bindings for Q6V5

[Doug Anderson]

Hi,

On Mon, Dec 17, 2018 at 2:08 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Add power-domain bindings for Q6V5 MSS on MSM8996 and SDM845 SoCs.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>
> v2:
>   * Add load_state power-domain
>   * List cx and mx power-domains for MSM8996
>
>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 98894e6ad456..50695cd86397 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -80,6 +80,22 @@ on the Qualcomm Hexagon core.
>                     reference to the pll-supply regulator to be held on behalf
>                     of the booting of the Hexagon core on MSM8996 SoCs
>
> +- power-domains:
> +       Usage: required
> +       Value type: <phandle>
> +       Definition: reference to the list of 2 power-domains for the modem
> +                   sub-system on MSM8996 SoCs

This is truly required for msm8996 SoCs?  The code I'm looking at
doesn't try to get these power domains for 8996 so presumably you're
breaking backward compatibility with old device tree files by making
this required now.  I don't personally know how widespread msm8996
usage is w/ upstream, so I'd let Bjorn comment on whether he thinks
this is OK.

As with the other patches in this series, I personally prefer less
prose and more lists / tables of exactly what is required for which
compatible string.


> +                   reference to the list of 4 power-domains for the modem
> +                   sub-system on SDM845 SoCs
> +
> +- power-domain-names:
> +       Usage: required
> +       Value type: <stringlist>
> +       Definition: must be "cx", "mx" for the modem sub-system on MSM8996
> +                   SoCs
> +                   must be "cx", "mx", "mss", "load_state" for the modem
> +                   sub-system on SDM845 SoCs

I haven't see a patch for using "load_state".  Can you point at it?  I
guess this was "aop" in your last version?


-Doug

Re: [PATCH v2 5/7] arm64: dts: qcom: sdm845: Add Q6V5 MSS node

[Doug Anderson]

Hi,

On Mon, Dec 17, 2018 at 2:08 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> This patch adds Q6V5 MSS remoteproc node for SDM845 SoCs.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>
> v2:
>   * Fixed style changes
>   * Added missing clocks in the dt-bindings
>   * Split mss remoteproc node into a number of patches

I know there was some off-list suggestion to split this into a number
of patches, but to actually make that useful to anyone we'd actually
need to _also_ post up patches to make the driver probe / work without
these power domains.  ...and as per other discussions it's kinda
"lucky" that it happens to work without them and Bjorn wasn't
supportive of making this optional.

So I'd actually fold patch 6 into patch 5 and focus on getting the
"aoss_qmp_pd" landed sooner rather than later.


Keeping the "shutdown-ack" as a separate patch makes sense though
since the bindings currently list that as "optional" and I guess
things work OK w/out it.


Once patch #6 is folded into patch #5 feel free to add my Reviewed-by tag.

Re: [PATCH v2 6/7] arm64: dts: qcom: sdm845: Add power-domain for Q6V5 MSS node

[Doug Anderson]

Hi,

On Mon, Dec 17, 2018 at 2:08 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Add power-domains cx, mx, mss and load_state for Q6V5 MSS node.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>
> This patch depends on the following bindings:
> https://patchwork.kernel.org/patch/10725801/ - rpmhpd dt bindings
> https://patchwork.kernel.org/patch/10725793/ - rpmhpd dt node
> https://patchwork.kernel.org/patch/10678301/ - AOP QMP dt bindings
>
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)

As per my comments on patch #5, I think this patch (AKA patch #6)
should be folded in there.


> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 33ff8668828f..56f5f55db9e2 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1401,6 +1401,12 @@
>                         qcom,halt-regs = <&tcsr_mutex_regs
>                                                 0x23000 0x25000 0x24000>;
>
> +                       power-domains = <&aoss_qmp_pd AOSS_QMP_LS_MODEM>,
> +                                       <&rpmhpd SDM845_CX>,
> +                                       <&rpmhpd SDM845_MX>,
> +                                       <&rpmhpd SDM845_MSS>;
> +                       power-domain-names = "load_state", "cx", "mx", "mss";

I guess you changed this to "load_state" from "aop" before?  Is there
code that actually uses this?


-Doug

-Doug

Re: [PATCH v2 7/7] arm64: dts: qcom: sdm845: Add shutdown-ack for Q6V5 MSS node

[Doug Anderson]

Hi,

On Mon, Dec 17, 2018 at 2:08 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Add shutdown-ack smp2p interrupt for Q6V5 MSS node.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>
> This patch depends on:
> https://patchwork.kernel.org/patch/10691213/ - shutdown-ack dt bindings
>
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v2 1/7] dt-bindings: soc: qcom: Add remote-pid binding for GLINK SMEM

[Sibi Sankar]

Hi Doug,
Thanks for the review :)

On 2018-12-18 05:29, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 17, 2018 at 2:07 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> Add missing qcom,remote-pid dt binding required for GLINK SMEM
>> which specifies the remote endpoint of the GLINK edge.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 5 +++++
>>  1 file changed, 5 insertions(+)
> 
> Fixes: 2b41d6c8e696 ("dt-bindings: soc: qcom: Extend GLINK to cover 
> SMEM")
> 
> 
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt 
>> b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
>> index 0b8cc533ca83..59ae603ba520 100644
>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt
>> @@ -21,6 +21,11 @@ edge.
>>         Definition: should specify the IRQ used by the remote 
>> processor to
>>                     signal this processor about communication related 
>> events
>> 
>> +- qcom,remote-pid:
>> +       Usage: required for glink-smem
>> +       Value type: <u32>
>> +       Definition: specifies the identfier of the remote endpoint of 
>> this edge
> 
> s/identfier/identifier/
> 

missed this, will correct it in v3.

> 
> Other than the typo this seems right to me.  Feel free to add my
> Reviewed-by tag when that's fixed.
> 
> 
> -Doug

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 2/7] dt-bindings: remoteproc: qcom: Add clock bindings for Q6V5

[Sibi Sankar]

Hi Doug,
Thanks for the review :)

On 2018-12-18 05:29, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 17, 2018 at 2:07 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> Add missing clock bindings for Q6V5 MSS on SDM845 SoCs.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt       | 10 
>> +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> Fixes: 9f058fa2efb1 ("remoteproc: qcom: Add support for mss remoteproc
> on msm8996")
> Fixes: fb22022ff63d ("dt-bindings: remoteproc: Add Q6v5 Modem PIL
> binding for SDM845")
> 
> ...it probably doesn't matter too much but if we wanted to be really
> careful we could split into two patches, one for the msm8996 and one
> for sdm845.  I don't think people care that much about stable
> backports of bindings though (someone can feel free to correct me)...
> 

I did think of splitting this up but it doesn't
actually fix 9f058fa2efb1 yet. I noticed a few missing
clocks for mss on 8996 when I did a diff with the
corresponding CAF tree. Hence couldn't add bindings for
it. Will add them once I validate mss on 8996 with the
necessary changes.

> 
>> diff --git 
>> a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt 
>> b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 9ff5b0309417..780adc043b37 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> @@ -39,13 +39,17 @@ on the Qualcomm Hexagon core.
>>  - clocks:
>>         Usage: required
>>         Value type: <phandle>
>> -       Definition: reference to the iface, bus and mem clocks to be 
>> held on
>> -                   behalf of the booting of the Hexagon core
>> +       Definition: reference to the list of 4 clocks for the modem 
>> sub-system
>> +                   reference to the list of 8 clocks for the modem 
>> sub-system
>> +                   on SDM845 SoCs
> 
> The above is confusing because you don't list the SoCs that are
> supposed to use the 4 clocks.  How about instead:
> 
> Definition: reference to the clocks that match clock-names
> 

AFAIK, only the exceptions are captured. I am fine
with both, I'll wait for Bjorn/Rob's preference.

> 
>>  - clock-names:
>>         Usage: required
>>         Value type: <stringlist>
>> -       Definition: must be "iface", "bus", "mem"
>> +       Definition: must be "iface", "bus", "mem", "xo" for the modem 
>> sub-system
>> +                   must be "iface", "bus", "mem", "gpll0_mss", 
>> "snoc_axi",
>> +                   "mnoc_axi", "prng", "xo" for the modem sub-system 
>> on SDM845
>> +                   SoCs
> 
> Same here where it's confusing.  ...but also, it it correct?  As far
> as I can tell you're missing msm8996.  It's better to just be explicit
> and list each one, ideally without all the prose.
> 
> Definition: The clocks needed depend on the compatible string:
> 

ditto

> qcom,sdm845-mss-pil: "xo", "prng", "iface", "snoc_axi", "bus", "mem",
> "gpll0_mss", "mnoc_axi"
> qcom,msm8996-mss-pil: "xo", "pnoc", "iface", "bus", "mem", 
> "gpll0_mss_clk"

ditto

> qcom,msm8974-mss-pil: "xo", "iface", "bus", "mem"
> qcom,msm8916-mss-pil: "xo", "iface", "bus", "mem"
> qcom,q6v5-pil: "xo", "iface", "bus", "mem"
> 
> ...as far as I can tell this binding is supposed to account for
> "qcom,ipq8074-wcss-pil" too but it seems that one doesn't have
> clock-names.
> 

Yeah the lack of clocks have to be documented
for ipq8074-wcss-pil.. will do it in v3

> 
> -Doug

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 4/7] dt-bindings: remoteproc: qcom: Add power-domain bindings for Q6V5

[Sibi Sankar]

Hi Doug,
Thanks for the review :)

On 2018-12-18 05:31, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 17, 2018 at 2:08 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> Add power-domain bindings for Q6V5 MSS on MSM8996 and SDM845 SoCs.
>> 
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>> 
>> v2:
>>   * Add load_state power-domain
>>   * List cx and mx power-domains for MSM8996
>> 
>>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 16 
>> ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt 
>> b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 98894e6ad456..50695cd86397 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> @@ -80,6 +80,22 @@ on the Qualcomm Hexagon core.
>>                     reference to the pll-supply regulator to be held 
>> on behalf
>>                     of the booting of the Hexagon core on MSM8996 SoCs
>> 
>> +- power-domains:
>> +       Usage: required
>> +       Value type: <phandle>
>> +       Definition: reference to the list of 2 power-domains for the 
>> modem
>> +                   sub-system on MSM8996 SoCs
> 
> This is truly required for msm8996 SoCs?  The code I'm looking at
> doesn't try to get these power domains for 8996 so presumably you're
> breaking backward compatibility with old device tree files by making
> this required now.  I don't personally know how widespread msm8996
> usage is w/ upstream, so I'd let Bjorn comment on whether he thinks
> this is OK.
> 

This is one of the reasons why the dt node
for mss on 8996 has not been posted/merged upstream.
Hence backward compatibility is not broken yet
in mainline :) .. However it will break on official
linaro integration releases (old dt + new kernel)

> As with the other patches in this series, I personally prefer less
> prose and more lists / tables of exactly what is required for which
> compatible string.
> 
> 
>> +                   reference to the list of 4 power-domains for the 
>> modem
>> +                   sub-system on SDM845 SoCs
>> +
>> +- power-domain-names:
>> +       Usage: required
>> +       Value type: <stringlist>
>> +       Definition: must be "cx", "mx" for the modem sub-system on 
>> MSM8996
>> +                   SoCs
>> +                   must be "cx", "mx", "mss", "load_state" for the 
>> modem
>> +                   sub-system on SDM845 SoCs
> 
> I haven't see a patch for using "load_state".  Can you point at it?  I
> guess this was "aop" in your last version?
> 

using load_state was Bjorn's suggestion and seemed
more appropriate than aop

> 
> -Doug

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 5/7] arm64: dts: qcom: sdm845: Add Q6V5 MSS node

[Sibi Sankar]

Hi Doug,
Thanks for the review :)

On 2018-12-18 05:32, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 17, 2018 at 2:08 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> This patch adds Q6V5 MSS remoteproc node for SDM845 SoCs.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>> 
>> v2:
>>   * Fixed style changes
>>   * Added missing clocks in the dt-bindings
>>   * Split mss remoteproc node into a number of patches
> 
> I know there was some off-list suggestion to split this into a number
> of patches, but to actually make that useful to anyone we'd actually
> need to _also_ post up patches to make the driver probe / work without
> these power domains.  ...and as per other discussions it's kinda
> "lucky" that it happens to work without them and Bjorn wasn't
> supportive of making this optional.
> 
> So I'd actually fold patch 6 into patch 5 and focus on getting the
> "aoss_qmp_pd" landed sooner rather than later.
> 

I'll fold them in v3

> 
> Keeping the "shutdown-ack" as a separate patch makes sense though
> since the bindings currently list that as "optional" and I guess
> things work OK w/out it.
> 
> 
> Once patch #6 is folded into patch #5 feel free to add my Reviewed-by 
> tag.

okay

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 6/7] arm64: dts: qcom: sdm845: Add power-domain for Q6V5 MSS node

[Sibi Sankar]

Hi Doug,
Thanks for the review :)

On 2018-12-18 05:32, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 17, 2018 at 2:08 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> Add power-domains cx, mx, mss and load_state for Q6V5 MSS node.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>> 
>> This patch depends on the following bindings:
>> https://patchwork.kernel.org/patch/10725801/ - rpmhpd dt bindings
>> https://patchwork.kernel.org/patch/10725793/ - rpmhpd dt node
>> https://patchwork.kernel.org/patch/10678301/ - AOP QMP dt bindings
>> 
>>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 6 ++++++
>>  1 file changed, 6 insertions(+)
> 
> As per my comments on patch #5, I think this patch (AKA patch #6)
> should be folded in there.
> 

okay

> 
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
>> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 33ff8668828f..56f5f55db9e2 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -1401,6 +1401,12 @@
>>                         qcom,halt-regs = <&tcsr_mutex_regs
>>                                                 0x23000 0x25000 
>> 0x24000>;
>> 
>> +                       power-domains = <&aoss_qmp_pd 
>> AOSS_QMP_LS_MODEM>,
>> +                                       <&rpmhpd SDM845_CX>,
>> +                                       <&rpmhpd SDM845_MX>,
>> +                                       <&rpmhpd SDM845_MSS>;
>> +                       power-domain-names = "load_state", "cx", "mx", 
>> "mss";
> 
> I guess you changed this to "load_state" from "aop" before?  Is there
> code that actually uses this?
> 

Bjorn said he will be posting the patch
for handling power-domains for mss..

> 
> -Doug
> 
> -Doug

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 3/7] dt-bindings: remoteproc: qcom: Fixup regulator dependencies

[Sibi Sankar]

Hi Doug,
Thanks for the review :)

On 2018-12-18 05:30, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 17, 2018 at 2:08 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> Fixup regulator supply dependencies for Q6V5 MSS on MSM996 SoCs.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt 
>> b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 780adc043b37..98894e6ad456 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> @@ -76,7 +76,9 @@ on the Qualcomm Hexagon core.
>>         Usage: required
>>         Value type: <phandle>
>>         Definition: reference to the regulators to be held on behalf 
>> of the
>> -                   booting of the Hexagon core
>> +                   booting of the Hexagon core on MSM8916 SoCs
>> +                   reference to the pll-supply regulator to be held 
>> on behalf
>> +                   of the booting of the Hexagon core on MSM8996 SoCs
> 
> The prose gets in the way and doesn't add anything.  I also don't
> understand what you're saying for msm8996.  You're saying that
> "pll-supply" is required there but none of the others?  That doesn't
> seem to be true in the code I have in front of me, but maybe I'm
> missing some patch.  For me, I'd write:
> 

AFAIK, only the exceptions are captured. But your
suggestion seems more simple/complete. Perhaps I'll
replace SoCs instead of compatibles? Anyway
I'll wait for Bjorn/Rob's preference.

> For the compatible strings below the following supplies are required:
>   "qcom,q6v5-pil"
>   "qcom,msm8916-mss-pil",
>   "qcom,msm8974-mss-pil"
> - cx-supply:
> - mss-supply:
> - mx-supply:
> - pll-supply:
> Usage: required
> Value type: <phandle>
> Definition: reference to the regulators to be held on behalf of the
>     booting of the Hexagon core
> 
> 
> ...and if msm8996 actually needs "pll-supply", you could add in...
> 
> For the compatible strings below the following supplies are required:
>   "qcom,msm8996-mss-pil"
> - pll-supply:
> Usage: required
> Value type: <phandle>
> Definition: reference to the regulators to be held on behalf of the
>     booting of the Hexagon core

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 5/7] arm64: dts: qcom: sdm845: Add Q6V5 MSS node

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

Hi Sibi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on agross/for-next]
[also build test ERROR on v4.20-rc7 next-20181218]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sibi-Sankar/dt-bindings-soc-qcom-Add-remote-pid-binding-for-GLINK-SMEM/20181217-205522
base:   https://git.kernel.org/pub/scm/linux/kernel/git/agross/linux.git for-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> Error: arch/arm64/boot/dts/qcom/sdm845.dtsi:1380.18-19 syntax error
>> FATAL ERROR: Unable to parse input tree

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41299 bytes --]

Re: [PATCH v2 2/7] dt-bindings: remoteproc: qcom: Add clock bindings for Q6V5

[Doug Anderson]

Hi,

On Mon, Dec 17, 2018 at 9:52 PM Sibi Sankar <sibis@codeaurora.org> wrote:
> >> @@ -39,13 +39,17 @@ on the Qualcomm Hexagon core.
> >>  - clocks:
> >>         Usage: required
> >>         Value type: <phandle>
> >> -       Definition: reference to the iface, bus and mem clocks to be
> >> held on
> >> -                   behalf of the booting of the Hexagon core
> >> +       Definition: reference to the list of 4 clocks for the modem
> >> sub-system
> >> +                   reference to the list of 8 clocks for the modem
> >> sub-system
> >> +                   on SDM845 SoCs
> >
> > The above is confusing because you don't list the SoCs that are
> > supposed to use the 4 clocks.  How about instead:
> >
> > Definition: reference to the clocks that match clock-names
> >
>
> AFAIK, only the exceptions are captured. I am fine
> with both, I'll wait for Bjorn/Rob's preference.

Sure, waiting for Bjron / Rob to chime in sounds good.  If you are
going to document just the exception, though, IMO at least reword it.
AKA:

reference to the list of 8 clocks for the modem sub-system on SDM845
SoCs; reference to the list of 4 clocks for the modem sub-system on
all other SoCs.

...the key being the "on all other" to make it clear.  ...but I still
like my original suggestion of using less prose and more lists, so
hopefully Bjorn / Rob are OK with that.


-Doug

Re: [PATCH v2 2/7] dt-bindings: remoteproc: qcom: Add clock bindings for Q6V5

[Rob Herring]

On Mon, Dec 17, 2018 at 03:37:19PM +0530, Sibi Sankar wrote:
> Add missing clock bindings for Q6V5 MSS on SDM845 SoCs.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt       | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 9ff5b0309417..780adc043b37 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -39,13 +39,17 @@ on the Qualcomm Hexagon core.
>  - clocks:
>  	Usage: required
>  	Value type: <phandle>
> -	Definition: reference to the iface, bus and mem clocks to be held on
> -		    behalf of the booting of the Hexagon core
> +	Definition: reference to the list of 4 clocks for the modem sub-system
> +		    reference to the list of 8 clocks for the modem sub-system
> +		    on SDM845 SoCs
>  
>  - clock-names:
>  	Usage: required
>  	Value type: <stringlist>
> -	Definition: must be "iface", "bus", "mem"
> +	Definition: must be "iface", "bus", "mem", "xo" for the modem sub-system
> +		    must be "iface", "bus", "mem", "gpll0_mss", "snoc_axi",
> +		    "mnoc_axi", "prng", "xo" for the modem sub-system on SDM845
> +		    SoCs

This seems to me a list of all clocks you need enabled, not what clocks 
actually go to the modem. Specifically, shouldn't the *noc_axi clocks be 
managed by the interconnect driver?

Rob

Re: [PATCH v2 2/7] dt-bindings: remoteproc: qcom: Add clock bindings for Q6V5

[Sibi Sankar]

Hi Rob,
Thanks for the review!

On 2018-12-18 22:57, Rob Herring wrote:
> On Mon, Dec 17, 2018 at 03:37:19PM +0530, Sibi Sankar wrote:
>> Add missing clock bindings for Q6V5 MSS on SDM845 SoCs.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt       | 10 
>> +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt 
>> b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 9ff5b0309417..780adc043b37 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> @@ -39,13 +39,17 @@ on the Qualcomm Hexagon core.
>>  - clocks:
>>  	Usage: required
>>  	Value type: <phandle>
>> -	Definition: reference to the iface, bus and mem clocks to be held on
>> -		    behalf of the booting of the Hexagon core
>> +	Definition: reference to the list of 4 clocks for the modem 
>> sub-system
>> +		    reference to the list of 8 clocks for the modem sub-system
>> +		    on SDM845 SoCs
>> 
>>  - clock-names:
>>  	Usage: required
>>  	Value type: <stringlist>
>> -	Definition: must be "iface", "bus", "mem"
>> +	Definition: must be "iface", "bus", "mem", "xo" for the modem 
>> sub-system
>> +		    must be "iface", "bus", "mem", "gpll0_mss", "snoc_axi",
>> +		    "mnoc_axi", "prng", "xo" for the modem sub-system on SDM845
>> +		    SoCs
> 
> This seems to me a list of all clocks you need enabled, not what clocks
> actually go to the modem. Specifically, shouldn't the *noc_axi clocks 
> be
> managed by the interconnect driver?

clocks = ...,
        <&gcc GCC_MSS_SNOC_AXI_CLK>,
        <&gcc GCC_MSS_MFAB_AXIS_CLK>,
          ...;
clock-names = ..., "snoc_axi", "mnoc_axi",...;

snoc_axi and mnoc_axi maps to above GCC clks and
both of them fall under the MSS functional group

> 
> Rob

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.