On Fri, Jun 10, 2016 at 05:31:53PM +0100, Lorenzo Pieralisi 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.

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

(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

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

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.

Regards,

Andy