Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's SP9860G
From: Chunyan Zhang
Date: Mon Feb 20 2017 - 04:41:25 EST
Hi Sudeep,
On ä, 2æ 17, 2017 at 10:28:00äå +0000, Sudeep Holla wrote:
>
>
> On 17/02/17 07:28, Chunyan Zhang wrote:
> > Hi Sudeep,
> >
> > On ä, 2æ 14, 2017 at 04:44:53äå +0000, Sudeep Holla wrote:
> >> On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
> >> <chunyan.zhang@xxxxxxxxxxxxxx> wrote:
>
> [..]
>
> >>
> >>> + idle-states{
> >>> + entry-method = "arm,psci";
> >>> +
> >>> + CORE_PD: core_pd {
> >>> + compatible = "arm,idle-state";
> >>> + entry-latency-us = <1000>;
> >>> + exit-latency-us = <700>;
> >>> + min-residency-us = <2500>;
> >>> + local-timer-stop;
> >>> + arm,psci-suspend-param = <0x00010002>;
> >>> + };
> >>> +
> >>> + CLUSTER_PD: cluster_pd {
> >>> + compatible = "arm,idle-state";
> >>> + entry-latency-us = <1000>;
> >>> + exit-latency-us = <1000>;
> >>> + min-residency-us = <3000>;
> >>> + local-timer-stop;
> >>> + arm,psci-suspend-param = <0x01010003>;
> >>> + };
> >>> +
> >>> + DEEP_SLEEP: deep_sleep {
> >>> + compatible = "arm,idle-state";
> >>> + wakeup-latency-us = <0xffffffff>;
> >>
> >> A value > 4294 seconds(i.e >1 hour) seems suspicious.
> >> Are you working around the firmware issue with high latency value so
> >> that it's never entered ? Why not remove advertising the state from DT.
> >>
> >
> > Haved checked with related colleagues, this node 'deep_sleep' was not for working
> > around any firmware issue, but was a trick utilization of idle subsystem, and that
>
> Really ? Any latency greater few milliseconds are sounds useless. I
> still don't understand what you mean by "trick utilization of idle
> subsystem".
>
Sorry for confused expression, I meant it was not a right way to utilize idle mechanism
and shouldn't be upstreamed.
> > was definitely not elegant, the author indeed intendly didn't want CPU entered this
> > state, I will remove this node therefore.
>
> It's quick and dirty "HACK* to retain and advertise the state but
> ensure it's never entered and obstruct the boot. It's not a trick to
> exploit any idle subsystem utilization.
>
Right, actually deep_sleep was for 'suspend' (forces idleness upon the OS until a wake-up event
resumes the OS from suspend), for example when users press power key on mobile phone to turn off
the screen. So the author implemented 'suspend' using cpu_psci_ops::cpu_suspend I figure that this
way is not correct, I will remove this state from DT.
I would appreciate any suggestion for how to implement this kind of function properly.
Thanks,
Chunyan
> >
> >> Can you get me the dump of:
> >> grep "" /sys/devices/system/cpu/cpu*/cpuidle/state*/{time,usage}
> >>
> >
> > FYI: https://www.irccloud.com/pastebin/XyEMLzfq/
> >
>
> As expected, state3(deep_sleep) is never entered.
>
> --
> Regards,
> Sudeep