[3/5] arm64: dts: msm8916: Add spc compat tag

Message ID 1463634020-17252-4-git-send-email-andy.gross@linaro.org
State New
Headers show

Commit Message

Andy Gross May 19, 2016, 5 a.m.
This patch adds the qcom,idle-state-spc compatible to the SPC idle
state.  This compatible indicates that the state is one which supports
freeze.

Signed-off-by: Andy Gross <andy.gross@linaro.org>

---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andy Gross May 19, 2016, 8:16 p.m. | #1
On 19 May 2016 at 14:52, Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote:
> Hi Andy,

>

> On 05/19/2016 08:00 AM, Andy Gross wrote:

>> This patch adds the qcom,idle-state-spc compatible to the SPC idle

>> state.  This compatible indicates that the state is one which supports

>> freeze.

>>

>> Signed-off-by: Andy Gross <andy.gross@linaro.org>

>> ---

>>  arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-

>>  1 file changed, 1 insertion(+), 1 deletion(-)

>>

>> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi

>> index 208af00..032e411 100644

>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi

>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi

>> @@ -104,7 +104,7 @@

>>

>>               idle-states {

>

> Is this change depends on some another patchset, I cannot find

> idle-states dt node on mainline nor linux-next ?


The cover letter gives a link to the patchset that contains the basis
for the changes.  You can also check out my posted branch which merges
in a part of those changes.

The link to the specific patch is:
http://www.spinics.net/lists/linux-arm-msm/msg19675.html

or you can look at this:
https://git.linaro.org/people/andy.gross/linux-pm.git/shortlog/refs/heads/for-pramod
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland June 10, 2016, 3:48 p.m. | #2
[+ Lorenzo]

On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> This patch adds the qcom,idle-state-spc compatible to the SPC idle

> state.  This compatible indicates that the state is one which supports

> freeze.

> 

> Signed-off-by: Andy Gross <andy.gross@linaro.org>

> ---

>  arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi

> index 208af00..032e411 100644

> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi

> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi

> @@ -104,7 +104,7 @@

>  

>  		idle-states {

>  			CPU_SPC: spc {

> -				compatible = "arm,idle-state";

> +				compatible = "qcom,idle-state-spc", "arm,idle-state";

>  				arm,psci-suspend-param = <0x40000002>;

>  				entry-latency-us = <130>;

>  				exit-latency-us = <150>;


This looks suspicious.

This is a PSCI idle state, and we have a PSCI driver driven by the
generic ARM cpuidle driver.

Why do we need a qcom-specific compatible here?

Surely we should be able to use the idle code in a generic fashion to
driver suspend-to-idle?

Thank,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland June 10, 2016, 4:25 p.m. | #3
On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:

> > [+ Lorenzo]

> > 

> > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:

> > > This patch adds the qcom,idle-state-spc compatible to the SPC idle

> > > state.  This compatible indicates that the state is one which supports

> > > freeze.

> > > 

> > > Signed-off-by: Andy Gross <andy.gross@linaro.org>

> > > ---

> > >  arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-

> > >  1 file changed, 1 insertion(+), 1 deletion(-)

> > > 

> > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > > index 208af00..032e411 100644

> > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > > @@ -104,7 +104,7 @@

> > >  

> > >  		idle-states {

> > >  			CPU_SPC: spc {

> > > -				compatible = "arm,idle-state";

> > > +				compatible = "qcom,idle-state-spc", "arm,idle-state";

> > >  				arm,psci-suspend-param = <0x40000002>;

> > >  				entry-latency-us = <130>;

> > >  				exit-latency-us = <150>;

> > 

> > This looks suspicious.

> > 

> > This is a PSCI idle state, and we have a PSCI driver driven by the

> > generic ARM cpuidle driver.

> > 

> > Why do we need a qcom-specific compatible here?

> > 

> > Surely we should be able to use the idle code in a generic fashion to

> > driver suspend-to-idle?

> 

> We need a way to identify specific idle states that support suspend-to-idle.  In

> addition, when we have identified the states, we may have to configure the

> enter_freeze() function.


Could you elaborate on what you mean by a state supporting
suspend-to-idle? It was my understanding that any idle state should
function for suspend-to-idle (and the choice of state is potentially
subjective).

> I chose to do this outside of the arm cpuidle driver because I didn't want to

> add any more DT information aside from the compatible, and I needed a separate

> place for the Qualcomm specific suspend code. 


Which suspend code is Qualcomm-specific? There shouldn't be anything on
the PSCI side, so I can only imagine that device management is left.
What am I missing?

> With the compatible, this makes

> my 32 and 64 bit processor suspend code identical, as we have our own cpuidle

> driver for the 32 bit procs.

> 

> An alternative would be to add some facilities to communicate this to the arm

> cpuidle driver and configure the enter_freeze() function at some later point.


I would prefer that suspend-to-idle using PSCI occurred via the generic
PSCI and cpuidle code. I am happy to extend that code if there is
something lacking, and I would prefer to not have platform-specific
hooks.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Gross June 10, 2016, 4:47 p.m. | #4
On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:

> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:

> > > [+ Lorenzo]

> > > 

> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:

> > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle

> > > > state.  This compatible indicates that the state is one which supports

> > > > freeze.

> > > > 

> > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>

> > > > ---

> > > >  arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-

> > > >  1 file changed, 1 insertion(+), 1 deletion(-)

> > > > 

> > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > > > index 208af00..032e411 100644

> > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > > > @@ -104,7 +104,7 @@

> > > >  

> > > >  		idle-states {

> > > >  			CPU_SPC: spc {

> > > > -				compatible = "arm,idle-state";

> > > > +				compatible = "qcom,idle-state-spc", "arm,idle-state";

> > > >  				arm,psci-suspend-param = <0x40000002>;

> > > >  				entry-latency-us = <130>;

> > > >  				exit-latency-us = <150>;

> > > 

> > > This looks suspicious.

> > > 

> > > This is a PSCI idle state, and we have a PSCI driver driven by the

> > > generic ARM cpuidle driver.

> > > 

> > > Why do we need a qcom-specific compatible here?

> > > 

> > > Surely we should be able to use the idle code in a generic fashion to

> > > driver suspend-to-idle?

> > 

> > We need a way to identify specific idle states that support suspend-to-idle.  In

> > addition, when we have identified the states, we may have to configure the

> > enter_freeze() function.

> 

> Could you elaborate on what you mean by a state supporting

> suspend-to-idle? It was my understanding that any idle state should

> function for suspend-to-idle (and the choice of state is potentially

> subjective).


when you freeze the system, cpuidle will try to find the deepest state which
supports freeze (by checking if a enter_freeze() exists).  If it does exist,
then the tick is frozen and the enter_freeze is called as each cpu goes idle.

If no enter_freeze exists, the system will suspend, but will continue to have
interrupts occur due to the tick and other bookkeeping stuff.

> 

> > I chose to do this outside of the arm cpuidle driver because I didn't want to

> > add any more DT information aside from the compatible, and I needed a separate

> > place for the Qualcomm specific suspend code. 

> 

> Which suspend code is Qualcomm-specific? There shouldn't be anything on

> the PSCI side, so I can only imagine that device management is left.

> What am I missing?


Qualcomm won't be supporting the psci suspend feature and will be doing their
own thing.  As such, they need their own suspend ops.  In addition, we have
platforms that don't use psci (32 bit).

> > With the compatible, this makes

> > my 32 and 64 bit processor suspend code identical, as we have our own cpuidle

> > driver for the 32 bit procs.

> > 

> > An alternative would be to add some facilities to communicate this to the arm

> > cpuidle driver and configure the enter_freeze() function at some later point.

> 

> I would prefer that suspend-to-idle using PSCI occurred via the generic

> PSCI and cpuidle code. I am happy to extend that code if there is

> something lacking, and I would prefer to not have platform-specific

> hooks.


Right, I briefly looked at that in the beginning and decided against it.  But if
we can at least add some way to identify freezeable idle states and configure
the freeze function, that'd cover all the requirements.

Regards,

Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Gross June 10, 2016, 5:27 p.m. | #5
On Fri, Jun 10, 2016 at 06:06:56PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Jun 10, 2016 at 11:52:48AM -0500, Andy Gross wrote:

> 

> [...]

> 

> > > (1) enter_freeze() hooks are not strictly necessary to enable

> > >     suspend-to-idle (they are if we want the tick to be frozen

> > >     on suspend-to-idle, which is different)

> > 

> > I'd think that you'd want the tick frozen.  Even if you are going to

> > just call the deepest freezable idle state in your freeze_function,

> > you don't want to keep getting woken up as this costs some power usage

> 

> As I said, that's a separate issue from these bindings.


I kind of see that coupled with the determination that a idle state supports
freeze.  Or are you wanting to have something else that decides whether or not
to configure the enter_freeze?


> > > (2) If I understand your code correctly you have to set the suspend

> > >     ops hook to make sure suspend-to-idle is enabled. This is a core

> > >     code issue rather than anything else, given that suspend-to-idle

> > >     (hey it is based on CPUidle !) does NOT rely on suspend ops to

> > >     function.

> > 

> > It only requires having suspend_ops and a valid function.  Otherwise

> > you can never suspend and never exercise the freeze portion of the

> > cpuidle code.

> 

> As I said, *currently* you have to call suspend_set_ops() to set

> up the pm_states label for PM_SUSPEND_FREEZE, suspend_ops are

> irrelevant to make suspend-to-idle work and as I said that's

> related to core code rather than platform specific suspend ops.

> 

> We should solve issues, not work around them.


I wholeheartedly agree.  I was just being more precise in my answer.

> 

> > > So the gist is: as far as I am concerned you do not need any of this

> > > code to enable (yes you need PSCI idle states but no

> > > qcom,idle-state-spc compatible string whatsoever) suspend-to-idle

> > > on ARM64 on top of PSCI, let me know what I am missing.

> > 

> > If we had the facilities in the arm cpuidle driver then for 64 bit

> > processors, I wouldn't have to do anything except provision my

> > suspend_ops + valid function.  For 32 bit, we actually already use

> > this compat tag and I just have to add code in the spm driver (qcom

> > cpuidle) to init the suspend_ops.

> 

> For 64-bit you do not have to have add any facility.

> 

> 1) we should change core code to make PM_SUSPEND_FREEZE independent

>    of suspend_set_ops()


So then, if cpuidle is active and you have idle states supporting freeze, then
you can always implicitly freeze the system.  This would infer then that the
system will always indicate it can do freeze.  I am good with that.

For now, we can get away with just implementing the changes you are suggesting.

> 2) we should define which idle states are freezable (99% of them are

>    minus coupled idle states), through generic bindings


This would be in the form of a DT property?  And given this property then we'd
just assign a enter_freeze() function that calls the enter() for that state.  Or
would we have something else that we need to key off of to decide to configure
the enter_freeze?

Regards,

Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lina Iyer June 10, 2016, 9:16 p.m. | #6
On Fri, Jun 10 2016 at 10:47 -0600, Andy Gross wrote:
>On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:

>> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:

>> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:

>> > > [+ Lorenzo]

>> > >

>> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:

<...>
>>

>> > I chose to do this outside of the arm cpuidle driver because I didn't want to

>> > add any more DT information aside from the compatible, and I needed a separate

>> > place for the Qualcomm specific suspend code.

>>

>> Which suspend code is Qualcomm-specific? There shouldn't be anything on

>> the PSCI side, so I can only imagine that device management is left.

>> What am I missing?

>


>Qualcomm won't be supporting the psci suspend feature and will be doing their

>own thing.  As such, they need their own suspend ops.  In addition, we have

>platforms that don't use psci (32 bit).

>

Andy, which PSCI suspend feature are you referring to, here? On 8916, we
do support CPU_SUSPEND in PSCI.

You don't want to call SYSTEM_SUSPEND, as it requires the other CPUs to
be CPU_OFF (hotplugged off) before the last core calls this PSCI
function.

Lina
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Gross June 10, 2016, 9:52 p.m. | #7
On 10 June 2016 at 16:16, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Fri, Jun 10 2016 at 10:47 -0600, Andy Gross wrote:

>>

>> On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:

>>>

>>> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:

>>> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:

>>> > > [+ Lorenzo]

>>> > >

>>> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:

>

> <...>

>>>

>>>

>>> > I chose to do this outside of the arm cpuidle driver because I didn't

>>> > want to

>>> > add any more DT information aside from the compatible, and I needed a

>>> > separate

>>> > place for the Qualcomm specific suspend code.

>>>

>>> Which suspend code is Qualcomm-specific? There shouldn't be anything on

>>> the PSCI side, so I can only imagine that device management is left.

>>> What am I missing?

>>

>>

>

>> Qualcomm won't be supporting the psci suspend feature and will be doing

>> their

>> own thing.  As such, they need their own suspend ops.  In addition, we

>> have

>> platforms that don't use psci (32 bit).

>>

> Andy, which PSCI suspend feature are you referring to, here? On 8916, we

> do support CPU_SUSPEND in PSCI.

>

> You don't want to call SYSTEM_SUSPEND, as it requires the other CPUs to

> be CPU_OFF (hotplugged off) before the last core calls this PSCI

> function.


I was referring to the SYSTEM_SUSPEND psci feature.  It was my
understanding that wouldn't be supported on the Qualcomm platforms.

Regards,

Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland June 13, 2016, 11 a.m. | #8
On Fri, Jun 10, 2016 at 11:47:21AM -0500, Andy Gross wrote:
> On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:

> > On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:

> > > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:

> > > > [+ Lorenzo]

> > > > 

> > > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:

> > > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle

> > > > > state.  This compatible indicates that the state is one which supports

> > > > > freeze.

> > > > > 

> > > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>

> > > > > ---

> > > > >  arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-

> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)

> > > > > 

> > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > > > > index 208af00..032e411 100644

> > > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > > > > @@ -104,7 +104,7 @@

> > > > >  

> > > > >  		idle-states {

> > > > >  			CPU_SPC: spc {

> > > > > -				compatible = "arm,idle-state";

> > > > > +				compatible = "qcom,idle-state-spc", "arm,idle-state";

> > > > >  				arm,psci-suspend-param = <0x40000002>;

> > > > >  				entry-latency-us = <130>;

> > > > >  				exit-latency-us = <150>;

> > > > 

> > > > This looks suspicious.

> > > > 

> > > > This is a PSCI idle state, and we have a PSCI driver driven by the

> > > > generic ARM cpuidle driver.

> > > > 

> > > > Why do we need a qcom-specific compatible here?

> > > > 

> > > > Surely we should be able to use the idle code in a generic fashion to

> > > > driver suspend-to-idle?

> > > 

> > > We need a way to identify specific idle states that support suspend-to-idle.  In

> > > addition, when we have identified the states, we may have to configure the

> > > enter_freeze() function.

> > 

> > Could you elaborate on what you mean by a state supporting

> > suspend-to-idle? It was my understanding that any idle state should

> > function for suspend-to-idle (and the choice of state is potentially

> > subjective).

> 

> when you freeze the system, cpuidle will try to find the deepest state which

> supports freeze (by checking if a enter_freeze() exists).  If it does exist,

> then the tick is frozen and the enter_freeze is called as each cpu goes idle.


Per Lorenzo's replies in another thread, it sounds like this is a
generic issue, and not specific to Qualcomm. My understanding is that
the only issue is coupled idle states, and further, that issue is really
an implementation detail within Linux w.r.t. IRQ management.

So it sounds like we need to rework things to be robust in the case of
coupled idle states, and we can wire up enter_freeze for all states
generically.

If there is some peroblem with making things robust, I assume we can
identify coupled idle states today in some generic manner. I don't
currently see the need for any DT binding.

Lorenzo, does the above make sense to you?

> > > I chose to do this outside of the arm cpuidle driver because I didn't want to

> > > add any more DT information aside from the compatible, and I needed a separate

> > > place for the Qualcomm specific suspend code. 

> > 

> > Which suspend code is Qualcomm-specific? There shouldn't be anything on

> > the PSCI side, so I can only imagine that device management is left.

> > What am I missing?

> 

> Qualcomm won't be supporting the psci suspend feature and will be doing their

> own thing.  As such, they need their own suspend ops.  In addition, we have

> platforms that don't use psci (32 bit).


That's orthogonal to suspend-to-idle, which can be handled in a generic
fashion. It's also orthogonal to 32-bit !PSCI platforms, as my
complaint was regarding the use of this with PSCI.

> > > With the compatible, this makes

> > > my 32 and 64 bit processor suspend code identical, as we have our own cpuidle

> > > driver for the 32 bit procs.

> > > 

> > > An alternative would be to add some facilities to communicate this to the arm

> > > cpuidle driver and configure the enter_freeze() function at some later point.

> > 

> > I would prefer that suspend-to-idle using PSCI occurred via the generic

> > PSCI and cpuidle code. I am happy to extend that code if there is

> > something lacking, and I would prefer to not have platform-specific

> > hooks.

> 

> Right, I briefly looked at that in the beginning and decided against it.  But if

> we can at least add some way to identify freezeable idle states and configure

> the freeze function, that'd cover all the requirements.


Ok, per the above I believe that is covered.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 208af00..032e411 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -104,7 +104,7 @@ 
 
 		idle-states {
 			CPU_SPC: spc {
-				compatible = "arm,idle-state";
+				compatible = "qcom,idle-state-spc", "arm,idle-state";
 				arm,psci-suspend-param = <0x40000002>;
 				entry-latency-us = <130>;
 				exit-latency-us = <150>;