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:

quoted

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

quoted

[+ Lorenzo]

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

quoted

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 <redacted>
---
 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.

quoted

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).

quoted

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