Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1584507

Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's SP9860G

Show all headers | View raw

On 20/02/17 09:37, Chunyan Zhang wrote:
> 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@spreadtrum.com> 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.
> 

No problem.

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

OK.

> I would appreciate any suggestion for how to implement this kind of
> function properly.


For the 'suspend' functionality you have described above, all you need
is the firmware to implement PSCI SYSTEM_SUSPEND API in the firmware.
The kernel psci driver detects the presence of the same and registers
the suspend ops automatically. You need not add anything in the code or
DT for the same.

-- 
Regards,
Sudeep

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/5] Add Spreadtrum SP9860G support Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-02-14 10:50 +0100
  [PATCH 5/5] serial: sprd: adjust TIMEOUT to a big value Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-02-14 10:50 +0100
  [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's SP9860G Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-02-14 10:50 +0100
    Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's SP9860G Sudeep Holla <sudeep.holla@arm.com> - 2017-02-14 17:50 +0100
      Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's  SP9860G Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-02-17 08:40 +0100
        Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's  SP9860G Sudeep Holla <sudeep.holla@arm.com> - 2017-02-17 11:30 +0100
          Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's  SP9860G Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-02-20 10:50 +0100
            Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's  SP9860G Sudeep Holla <sudeep.holla@arm.com> - 2017-02-20 11:50 +0100
              Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's  SP9860G Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-02-20 14:00 +0100
    Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's  SP9860G Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-02-16 09:10 +0100
  [PATCH 4/5] sprd_serial: switch comptible string to sc-uart Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-02-14 10:50 +0100
    Re: [PATCH 4/5] sprd_serial: switch comptible string to sc-uart Arnd Bergmann <arnd@arndb.de> - 2017-02-16 14:40 +0100
      Re: [PATCH 4/5] sprd_serial: switch comptible string to sc-uart Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-02-17 08:40 +0100
  [PATCH 2/5] Documentation: sprd: Add bindings for SP9860G Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-02-14 10:50 +0100

csiph-web