diff mbox series

[v2,7/9] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states

Message ID 49cf5d94beb9af9ef4e78d4c52f3b0ad20b7c63f.1558430617.git.amit.kucheria@linaro.org
State New
Headers show
Series None | expand

Commit Message

Amit Kucheria May 21, 2019, 9:35 a.m. UTC 
Add device bindings for cpuidle states for cpu devices.

Cc: Marc Gonzalez <marc.w.gonzalez@free.fr>
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

---
 arch/arm64/boot/dts/qcom/msm8998.dtsi | 50 +++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

-- 
2.17.1

Comments

Marc Gonzalez May 21, 2019, 4:10 p.m. UTC | #1 
On 21/05/2019 14:03, Marc Gonzalez wrote:

> the system starts to boot, hangs a few seconds, then silently reboots


Using extremely high-tech debugging tools (i.e. spraying printk left and right)
I traced this one down to:

psci_cpu_suspend_enter: 435
psci_cpu_suspend: 171
psci_cpu_suspend: __invoke_psci_fn_smc c4000001
__invoke_psci_fn_smc: id=c4000001 3 0 0
/*** we never return from arm_smccc_smc() ***/


The following dmesg log caught my eye, and might be relevant:

ARM_SMCCC_ARCH_WORKAROUND_1 missing from firmware


If I revert your patch, psci_cpu_suspend_enter() is never called,
so we don't tickle the arm_smccc_smc() monster.

Could it be that my FW doesn't support PSCI?

Regards.
Jeffrey Hugo Oct. 1, 2019, 2:21 p.m. UTC | #2 
On Mon, Sep 30, 2019 at 4:44 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
>

> Can you try removing just the *SLEEP_1 states from the cpu-idle-states

> property? I want to understand if this is triggered just by the deeper

> C-state.


Still seeing the issue with just the SLEEP_0 states.  For reference,
Bjorn suggested adding kpti=no to the command line, which also
appeared to have no effect on the issue.

>

> On Tue, Oct 1, 2019 at 3:50 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:

> >

> > Amit, the merged version of the below change causes a boot failure

> > (nasty hang, sometimes with RCU stalls) on the msm8998 laptops.  Oddly

> > enough, it seems to be resolved if I remove the cpu-idle-states

> > property from one of the cpu nodes.

> >

> > I see no issues with the msm8998 MTP.

> >

> > Do you have any suggestions on how we might debug this?

> >

> > On Tue, May 21, 2019 at 3:38 AM Amit Kucheria <amit.kucheria@linaro.org> wrote:

> > >

> > > Add device bindings for cpuidle states for cpu devices.

> > >

> > > Cc: Marc Gonzalez <marc.w.gonzalez@free.fr>

> > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

> > > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> > > ---

> > >  arch/arm64/boot/dts/qcom/msm8998.dtsi | 50 +++++++++++++++++++++++++++

> > >  1 file changed, 50 insertions(+)

> > >

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

> > > index 3fd0769fe648..54810980fcf9 100644

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

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

> > > @@ -78,6 +78,7 @@

> > >                         compatible = "arm,armv8";

> > >                         reg = <0x0 0x0>;

> > >                         enable-method = "psci";

> > > +                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;

> > >                         efficiency = <1024>;

> > >                         next-level-cache = <&L2_0>;

> > >                         L2_0: l2-cache {

> > > @@ -97,6 +98,7 @@

> > >                         compatible = "arm,armv8";

> > >                         reg = <0x0 0x1>;

> > >                         enable-method = "psci";

> > > +                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;

> > >                         efficiency = <1024>;

> > >                         next-level-cache = <&L2_0>;

> > >                         L1_I_1: l1-icache {

> > > @@ -112,6 +114,7 @@

> > >                         compatible = "arm,armv8";

> > >                         reg = <0x0 0x2>;

> > >                         enable-method = "psci";

> > > +                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;

> > >                         efficiency = <1024>;

> > >                         next-level-cache = <&L2_0>;

> > >                         L1_I_2: l1-icache {

> > > @@ -127,6 +130,7 @@

> > >                         compatible = "arm,armv8";

> > >                         reg = <0x0 0x3>;

> > >                         enable-method = "psci";

> > > +                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;

> > >                         efficiency = <1024>;

> > >                         next-level-cache = <&L2_0>;

> > >                         L1_I_3: l1-icache {

> > > @@ -142,6 +146,7 @@

> > >                         compatible = "arm,armv8";

> > >                         reg = <0x0 0x100>;

> > >                         enable-method = "psci";

> > > +                       cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;

> > >                         efficiency = <1536>;

> > >                         next-level-cache = <&L2_1>;

> > >                         L2_1: l2-cache {

> > > @@ -161,6 +166,7 @@

> > >                         compatible = "arm,armv8";

> > >                         reg = <0x0 0x101>;

> > >                         enable-method = "psci";

> > > +                       cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;

> > >                         efficiency = <1536>;

> > >                         next-level-cache = <&L2_1>;

> > >                         L1_I_101: l1-icache {

> > > @@ -176,6 +182,7 @@

> > >                         compatible = "arm,armv8";

> > >                         reg = <0x0 0x102>;

> > >                         enable-method = "psci";

> > > +                       cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;

> > >                         efficiency = <1536>;

> > >                         next-level-cache = <&L2_1>;

> > >                         L1_I_102: l1-icache {

> > > @@ -191,6 +198,7 @@

> > >                         compatible = "arm,armv8";

> > >                         reg = <0x0 0x103>;

> > >                         enable-method = "psci";

> > > +                       cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;

> > >                         efficiency = <1536>;

> > >                         next-level-cache = <&L2_1>;

> > >                         L1_I_103: l1-icache {

> > > @@ -238,6 +246,48 @@

> > >                                 };

> > >                         };

> > >                 };

> > > +

> > > +               idle-states {

> > > +                       entry-method = "psci";

> > > +

> > > +                       LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {

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

> > > +                               idle-state-name = "little-retention";

> > > +                               arm,psci-suspend-param = <0x00000002>;

> > > +                               entry-latency-us = <43>;

> > > +                               exit-latency-us = <86>;

> > > +                               min-residency-us = <200>;

> > > +                       };

> > > +

> > > +                       LITTLE_CPU_SLEEP_1: cpu-sleep-0-1 {

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

> > > +                               idle-state-name = "little-power-collapse";

> > > +                               arm,psci-suspend-param = <0x00000003>;

> > > +                               entry-latency-us = <100>;

> > > +                               exit-latency-us = <612>;

> > > +                               min-residency-us = <1000>;

> > > +                               local-timer-stop;

> > > +                       };

> > > +

> > > +                       BIG_CPU_SLEEP_0: cpu-sleep-1-0 {

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

> > > +                               idle-state-name = "big-retention";

> > > +                               arm,psci-suspend-param = <0x00000002>;

> > > +                               entry-latency-us = <41>;

> > > +                               exit-latency-us = <82>;

> > > +                               min-residency-us = <200>;

> > > +                       };

> > > +

> > > +                       BIG_CPU_SLEEP_1: cpu-sleep-1-1 {

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

> > > +                               idle-state-name = "big-power-collapse";

> > > +                               arm,psci-suspend-param = <0x00000003>;

> > > +                               entry-latency-us = <100>;

> > > +                               exit-latency-us = <525>;

> > > +                               min-residency-us = <1000>;

> > > +                               local-timer-stop;

> > > +                       };

> > > +               };

> > >         };

> > >

> > >         firmware {

> > > --

> > > 2.17.1

> > >
Amit Kucheria Oct. 4, 2019, 1:36 a.m. UTC | #3 
On Wed, Oct 2, 2019 at 11:48 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
>

> On Wed, Oct 2, 2019 at 3:27 AM Niklas Cassel <niklas.cassel@linaro.org> wrote:

> >

> > On Wed, Oct 02, 2019 at 11:19:50AM +0200, Niklas Cassel wrote:

> > > On Mon, Sep 30, 2019 at 04:20:15PM -0600, Jeffrey Hugo wrote:

> > > > Amit, the merged version of the below change causes a boot failure

> > > > (nasty hang, sometimes with RCU stalls) on the msm8998 laptops.  Oddly

> > > > enough, it seems to be resolved if I remove the cpu-idle-states

> > > > property from one of the cpu nodes.

> > > >

> > > > I see no issues with the msm8998 MTP.

> > >

> > > Hello Jeffrey, Amit,

> > >

> > > If the PSCI idle states work properly on the msm8998 devboard (MTP),

> > > but causes crashes on msm8998 laptops, the only logical change is

> > > that the PSCI firmware is different between the two devices.

> >

> > Since the msm8998 laptops boot using ACPI, perhaps these laptops

> > doesn't support PSCI/have any PSCI firmware at all.

>

> They have PSCI.  If there was no PSCI, I would expect the PSCI

> get_version request from Linux to fail, and all PSCI functionality to

> be disabled.

>

> However, your mention about ACPI sparked a thought.  ACPI describes

> the idle states, along with the PSCI info, in the ACPI0007 devices.

> Those exist on the laptops, and the info mostly correlates with Amit's

> patch (ACPI seems to be a bit more conservative about the latencies,

> and describes one additional deeper state).  However, upon a detailed

> analysis of the ACPI description, I did find something relevant - the

> retention state is not enabled.

>

> So, I hacked out the retention state from Amit's patch, and I did not

> observe a hang.  I used sysfs, and appeared able to validate that the

> power collapse state was being used successfully.


Interesting that the shallower sleep state was causing problems.
Usually, it is the deeper states that cause problems. So you plan to
override the idle states table in the board-specific DT?

Why does the platform even rely on DT? Shouldn't we use the ACPI tables instead?

> I'm guessing that something is weird with the laptops, where the CPUs

> can go into retention, but not come out, thus causing issues.

>

> I'll post a patch to fix up the laptops.  Thanks for all the help.
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 3fd0769fe648..54810980fcf9 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -78,6 +78,7 @@ 
 			compatible = "arm,armv8";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
 			efficiency = <1024>;
 			next-level-cache = <&L2_0>;
 			L2_0: l2-cache {
@@ -97,6 +98,7 @@ 
 			compatible = "arm,armv8";
 			reg = <0x0 0x1>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
 			efficiency = <1024>;
 			next-level-cache = <&L2_0>;
 			L1_I_1: l1-icache {
@@ -112,6 +114,7 @@ 
 			compatible = "arm,armv8";
 			reg = <0x0 0x2>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
 			efficiency = <1024>;
 			next-level-cache = <&L2_0>;
 			L1_I_2: l1-icache {
@@ -127,6 +130,7 @@ 
 			compatible = "arm,armv8";
 			reg = <0x0 0x3>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
 			efficiency = <1024>;
 			next-level-cache = <&L2_0>;
 			L1_I_3: l1-icache {
@@ -142,6 +146,7 @@ 
 			compatible = "arm,armv8";
 			reg = <0x0 0x100>;
 			enable-method = "psci";
+			cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
 			efficiency = <1536>;
 			next-level-cache = <&L2_1>;
 			L2_1: l2-cache {
@@ -161,6 +166,7 @@ 
 			compatible = "arm,armv8";
 			reg = <0x0 0x101>;
 			enable-method = "psci";
+			cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
 			efficiency = <1536>;
 			next-level-cache = <&L2_1>;
 			L1_I_101: l1-icache {
@@ -176,6 +182,7 @@ 
 			compatible = "arm,armv8";
 			reg = <0x0 0x102>;
 			enable-method = "psci";
+			cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
 			efficiency = <1536>;
 			next-level-cache = <&L2_1>;
 			L1_I_102: l1-icache {
@@ -191,6 +198,7 @@ 
 			compatible = "arm,armv8";
 			reg = <0x0 0x103>;
 			enable-method = "psci";
+			cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
 			efficiency = <1536>;
 			next-level-cache = <&L2_1>;
 			L1_I_103: l1-icache {
@@ -238,6 +246,48 @@ 
 				};
 			};
 		};
+
+		idle-states {
+			entry-method = "psci";
+
+			LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "little-retention";
+				arm,psci-suspend-param = <0x00000002>;
+				entry-latency-us = <43>;
+				exit-latency-us = <86>;
+				min-residency-us = <200>;
+			};
+
+			LITTLE_CPU_SLEEP_1: cpu-sleep-0-1 {
+				compatible = "arm,idle-state";
+				idle-state-name = "little-power-collapse";
+				arm,psci-suspend-param = <0x00000003>;
+				entry-latency-us = <100>;
+				exit-latency-us = <612>;
+				min-residency-us = <1000>;
+				local-timer-stop;
+			};
+
+			BIG_CPU_SLEEP_0: cpu-sleep-1-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "big-retention";
+				arm,psci-suspend-param = <0x00000002>;
+				entry-latency-us = <41>;
+				exit-latency-us = <82>;
+				min-residency-us = <200>;
+			};
+
+			BIG_CPU_SLEEP_1: cpu-sleep-1-1 {
+				compatible = "arm,idle-state";
+				idle-state-name = "big-power-collapse";
+				arm,psci-suspend-param = <0x00000003>;
+				entry-latency-us = <100>;
+				exit-latency-us = <525>;
+				min-residency-us = <1000>;
+				local-timer-stop;
+			};
+		};
 	};
 
 	firmware {