arm64: dts: qcom: sdm845: Add Q6V5 MSS node
diff mbox series

Message ID 20181127085828.17908-1-sibis@codeaurora.org
State New
Headers show
Series
  • arm64: dts: qcom: sdm845: Add Q6V5 MSS node
Related show

Commit Message

Sibi Sankar Nov. 27, 2018, 8:58 a.m. UTC
This patch adds Q6V5 MSS remoteproc node for SDM845 SoCs.

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

The remoteproc mss node depends on the following bindings:
https://patchwork.kernel.org/patch/10490559/ - rpmhp dt bindings
https://patchwork.kernel.org/patch/10678301/ - AOP QMP dt bindings
https://patchwork.kernel.org/patch/10691215/ - mss power-domain dt bindings
https://patchwork.kernel.org/patch/10691213/ - shutdown-ack dt bindings

It also 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 | 63 ++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Comments

Doug Anderson Dec. 13, 2018, 10:17 p.m. UTC | #1
Hi,

On Tue, Nov 27, 2018 at 12:58 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>
> ---
>
> The remoteproc mss node depends on the following bindings:
> https://patchwork.kernel.org/patch/10490559/ - rpmhp dt bindings

This is an older version of the patch.  Now at v7 at
<https://patchwork.kernel.org/patch/10725801/>


> https://patchwork.kernel.org/patch/10678301/ - AOP QMP dt bindings
> https://patchwork.kernel.org/patch/10691215/ - mss power-domain dt bindings
> https://patchwork.kernel.org/patch/10691213/ - shutdown-ack dt bindings
>
> It also 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 | 63 ++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 58870273dbc9..df16ee464872 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1095,6 +1095,69 @@
>                         };
>                 };
>
> +               remoteproc@4080000 {
> +                       compatible = "qcom,sdm845-mss-pil";
> +                       reg = <0x04080000 0x408>, <0x04180000 0x48>;

s/0x04080000/0x4080000 to appease the DT folks.


> +                       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>,
> +                               <&modem_smp2p_in 7 IRQ_TYPE_EDGE_RISING>;
> +
> +                       interrupt-names = "wdog", "fatal", "ready",
> +                                         "handover", "stop-ack",
> +                                         "shutdown-ack";

nit: maybe remove blank line between "interrupts-extended" and
"interrupt-names".  Nice to keep -names close to the things they're
naming.


> +                       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";

Bindings list clock-names as "iface", "bus", "mem".  You have "iface",
"bus", "mem", "gpll0_mss", "snoc_axi", "mnoc_axi", "prng", "xo".  It
looks like these extra clocks were added in commit 231f67d1fb2f
("remoteproc: q6v5: Add support for mss remoteproc on SDM845") but you
forgot to update the bindings.  Looking in that patch I also see an
"axis2" which you seem to be missing.  Do you need it?


> +                       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>;
> +
> +                       power-domains = <&aoss_qmp_pd AOSS_QMP_LS_MODEM>,
> +                                       <&rpmhpd SDM845_CX>,
> +                                       <&rpmhpd SDM845_MX>,
> +                                       <&rpmhpd SDM845_MSS>;
> +                       power-domain-names = "aop", "cx", "mx", "mss";
> +
> +                       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>;

The "qcom,remote-pid" property isn't documented for "glink-edge".  Do
you need it?  ...if you do, please send a patch to update the
bindings.


-Doug
Bjorn Andersson Dec. 14, 2018, 4:52 a.m. UTC | #2
On Thu 13 Dec 14:17 PST 2018, Doug Anderson wrote:
> On Tue, Nov 27, 2018 at 12:58 AM Sibi Sankar <sibis@codeaurora.org> wrote:
[..]
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index 58870273dbc9..df16ee464872 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -1095,6 +1095,69 @@
> >                         };
> >                 };
> >
> > +               remoteproc@4080000 {
> > +                       compatible = "qcom,sdm845-mss-pil";
> > +                       reg = <0x04080000 0x408>, <0x04180000 0x48>;
> 
> s/0x04080000/0x4080000 to appease the DT folks.
> 

Andy requests this to be padded to 8 digits, and I've come to really
appreciate this as it makes sorting much easier.

But perhaps there's a verdict on this?

Regards,
Bjorn
Doug Anderson Dec. 14, 2018, 4:18 p.m. UTC | #3
Hi,

On Thu, Dec 13, 2018 at 8:52 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Thu 13 Dec 14:17 PST 2018, Doug Anderson wrote:
> > On Tue, Nov 27, 2018 at 12:58 AM Sibi Sankar <sibis@codeaurora.org> wrote:
> [..]
> > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > index 58870273dbc9..df16ee464872 100644
> > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > > @@ -1095,6 +1095,69 @@
> > >                         };
> > >                 };
> > >
> > > +               remoteproc@4080000 {
> > > +                       compatible = "qcom,sdm845-mss-pil";
> > > +                       reg = <0x04080000 0x408>, <0x04180000 0x48>;
> >
> > s/0x04080000/0x4080000 to appease the DT folks.
> >
>
> Andy requests this to be padded to 8 digits, and I've come to really
> appreciate this as it makes sorting much easier.
>
> But perhaps there's a verdict on this?

Hrm.  First I've heard of that.  ...and all of the other addresses in
this file aren't padded to 8 digits.  Ugh.  I could submit a patch to
fix them all (I actually like them padded too) but given the current
number of outstanding patches against sdm845.dtsi it's just going to
cause lots of merge conflicts?

I thought it was general DT practice to always omit leading zeros but
I just searched and it appears that policy only applies to unit
addresses.  Specifically I note that in
<https://lkml.kernel.org/r/20180111060004.9333-1-bjorn.andersson@linaro.org>
Rob H asked you to remove the leading zero from the unit address but
_not_ the "reg".  So I guess padding the reg to 8 digits is OK.

tl;dr: Sure, keep the padding the 8 digits here and eventually we can
fix-up the other nodes when there's not so much churn to sdm845.dtsi
(or maybe Andy can do it himself?)  Sound like a plan?


-Doug
Sibi Sankar Dec. 14, 2018, 4:29 p.m. UTC | #4
Hi Doug,
Thanks for the review!

On 2018-12-14 03:47, Doug Anderson wrote:
> Hi,
> 
> On Tue, Nov 27, 2018 at 12:58 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>
>> ---
>> 
>> The remoteproc mss node depends on the following bindings:
>> https://patchwork.kernel.org/patch/10490559/ - rpmhp dt bindings
> 
> This is an older version of the patch.  Now at v7 at
> <https://patchwork.kernel.org/patch/10725801/>
> 
> 
>> https://patchwork.kernel.org/patch/10678301/ - AOP QMP dt bindings
>> https://patchwork.kernel.org/patch/10691215/ - mss power-domain dt 
>> bindings
>> https://patchwork.kernel.org/patch/10691213/ - shutdown-ack dt 
>> bindings
>> 
>> It also 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 | 63 
>> ++++++++++++++++++++++++++++
>>  1 file changed, 63 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
>> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 58870273dbc9..df16ee464872 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -1095,6 +1095,69 @@
>>                         };
>>                 };
>> 
>> +               remoteproc@4080000 {
>> +                       compatible = "qcom,sdm845-mss-pil";
>> +                       reg = <0x04080000 0x408>, <0x04180000 0x48>;
> 
> s/0x04080000/0x4080000 to appease the DT folks.
> 

as you said in the other thread will leave the
padding untouched

> 
>> +                       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>,
>> +                               <&modem_smp2p_in 7 
>> IRQ_TYPE_EDGE_RISING>;
>> +
>> +                       interrupt-names = "wdog", "fatal", "ready",
>> +                                         "handover", "stop-ack",
>> +                                         "shutdown-ack";
> 
> nit: maybe remove blank line between "interrupts-extended" and
> "interrupt-names".  Nice to keep -names close to the things they're
> naming.
> 

sure will do that.. I guess I'll have to remove
the blank line in clock-names as well

> 
>> +                       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";
> 
> Bindings list clock-names as "iface", "bus", "mem".  You have "iface",
> "bus", "mem", "gpll0_mss", "snoc_axi", "mnoc_axi", "prng", "xo".  It
> looks like these extra clocks were added in commit 231f67d1fb2f
> ("remoteproc: q6v5: Add support for mss remoteproc on SDM845") but you
> forgot to update the bindings.  Looking in that patch I also see an
> "axis2" which you seem to be missing.  Do you need it?
> 

yes missed adding them..will add them in the next respin

> 
>> +                       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>;
>> +
>> +                       power-domains = <&aoss_qmp_pd 
>> AOSS_QMP_LS_MODEM>,
>> +                                       <&rpmhpd SDM845_CX>,
>> +                                       <&rpmhpd SDM845_MX>,
>> +                                       <&rpmhpd SDM845_MSS>;
>> +                       power-domain-names = "aop", "cx", "mx", "mss";
>> +
>> +                       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>;
> 
> The "qcom,remote-pid" property isn't documented for "glink-edge".  Do
> you need it?  ...if you do, please send a patch to update the
> bindings.
> 

yes glink seems to be missing the remote-pid..
I will add that as well in v2

> 
> -Doug

Patch
diff mbox series

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 58870273dbc9..df16ee464872 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1095,6 +1095,69 @@ 
 			};
 		};
 
+		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>,
+				<&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_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>;
+
+			power-domains = <&aoss_qmp_pd AOSS_QMP_LS_MODEM>,
+					<&rpmhpd SDM845_CX>,
+					<&rpmhpd SDM845_MX>,
+					<&rpmhpd SDM845_MSS>;
+			power-domain-names = "aop", "cx", "mx", "mss";
+
+			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>;