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

[Chunyan Zhang]

On 一,  2月 20, 2017 at 10:47:56上午 +, Sudeep Holla wrote:
> 
> 
> On 20/02/17 09:37, Chunyan Zhang wrote:
> > Hi Sudeep,
> > 
> > On 五,  2月 17, 2017 at 10:28:00上午 +, Sudeep Holla wrote:
> >>
> >>
> >> On 17/02/17 07:28, Chunyan Zhang wrote:
> >>> Hi Sudeep,
> >>>
> >>> On 二,  2月 14, 2017 at 04:44:53下午 +, Sudeep Holla wrote:
>  On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
>   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 = <0x>;
> 
>  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.

Thank you Sudeep, I will have my colleague to study further according to
the direction you provided here.

Thanks for your comments,
Chunyan

> 
> -- 
> Regards,
> Sudeep

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

[Chunyan Zhang]

On 一,  2月 20, 2017 at 10:47:56上午 +, Sudeep Holla wrote:
> 
> 
> On 20/02/17 09:37, Chunyan Zhang wrote:
> > Hi Sudeep,
> > 
> > On 五,  2月 17, 2017 at 10:28:00上午 +, Sudeep Holla wrote:
> >>
> >>
> >> On 17/02/17 07:28, Chunyan Zhang wrote:
> >>> Hi Sudeep,
> >>>
> >>> On 二,  2月 14, 2017 at 04:44:53下午 +, Sudeep Holla wrote:
>  On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
>   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 = <0x>;
> 
>  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.

Thank you Sudeep, I will have my colleague to study further according to
the direction you provided here.

Thanks for your comments,
Chunyan

> 
> -- 
> Regards,
> Sudeep

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

[Sudeep Holla]

On 20/02/17 09:37, Chunyan Zhang wrote:
> Hi Sudeep,
> 
> On 五,  2月 17, 2017 at 10:28:00上午 +, Sudeep Holla wrote:
>>
>>
>> On 17/02/17 07:28, Chunyan Zhang wrote:
>>> Hi Sudeep,
>>>
>>> On 二,  2月 14, 2017 at 04:44:53下午 +, Sudeep Holla wrote:
 On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
  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 = <0x>;

 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

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

[Sudeep Holla]

On 20/02/17 09:37, Chunyan Zhang wrote:
> Hi Sudeep,
> 
> On 五,  2月 17, 2017 at 10:28:00上午 +, Sudeep Holla wrote:
>>
>>
>> On 17/02/17 07:28, Chunyan Zhang wrote:
>>> Hi Sudeep,
>>>
>>> On 二,  2月 14, 2017 at 04:44:53下午 +, Sudeep Holla wrote:
 On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
  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 = <0x>;

 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

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

[Chunyan Zhang]

Hi Sudeep,

On 五,  2月 17, 2017 at 10:28:00上午 +, Sudeep Holla wrote:
> 
> 
> On 17/02/17 07:28, Chunyan Zhang wrote:
> > Hi Sudeep,
> > 
> > On 二,  2月 14, 2017 at 04:44:53下午 +, Sudeep Holla wrote:
> >> On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
> >>  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 = <0x>;
> >>
> >> 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

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

[Chunyan Zhang]

Hi Sudeep,

On 五,  2月 17, 2017 at 10:28:00上午 +, Sudeep Holla wrote:
> 
> 
> On 17/02/17 07:28, Chunyan Zhang wrote:
> > Hi Sudeep,
> > 
> > On 二,  2月 14, 2017 at 04:44:53下午 +, Sudeep Holla wrote:
> >> On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
> >>  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 = <0x>;
> >>
> >> 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

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

[Sudeep Holla]

On 17/02/17 07:28, Chunyan Zhang wrote:
> Hi Sudeep,
> 
> On 二,  2月 14, 2017 at 04:44:53下午 +, Sudeep Holla wrote:
>> On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
>>  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 = <0x>;
>>
>> 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".

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

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

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

[Sudeep Holla]

On 17/02/17 07:28, Chunyan Zhang wrote:
> Hi Sudeep,
> 
> On 二,  2月 14, 2017 at 04:44:53下午 +, Sudeep Holla wrote:
>> On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
>>  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 = <0x>;
>>
>> 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".

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

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

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

[Chunyan Zhang]

Hi Sudeep,

On 二,  2月 14, 2017 at 04:44:53下午 +, Sudeep Holla wrote:
> On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
>  wrote:
> > From: Orson Zhai 
> >
> > SC9860G is a 8 cores of A53 SoC with 4G LTE support SoC from Spreadtrum.
> >
> > According to regular hierarchy of sprd dts, whale2.dtsi contains SoC
> > peripherals IP nodes, sc9860.dtsi contains stuff related to ARM core stuff
> > and sp9860g dts is for the board level.
> >
> > Signed-off-by: Orson Zhai 
> > Signed-off-by: Chunyan Zhang 
> > ---
> >  arch/arm64/boot/dts/sprd/Makefile |   3 +-
> >  arch/arm64/boot/dts/sprd/sc9860.dtsi  | 534 
> > ++
> >  arch/arm64/boot/dts/sprd/sp9860g-1h10.dts |  58 
> >  arch/arm64/boot/dts/sprd/whale2.dtsi  |  66 
> >  4 files changed, 660 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/arm64/boot/dts/sprd/sc9860.dtsi
> >  create mode 100644 arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> >  create mode 100644 arch/arm64/boot/dts/sprd/whale2.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/sprd/Makefile 
> > b/arch/arm64/boot/dts/sprd/Makefile
> > index b658c5e..f0535e6 100644
> > --- a/arch/arm64/boot/dts/sprd/Makefile
> > +++ b/arch/arm64/boot/dts/sprd/Makefile
> > @@ -1,4 +1,5 @@
> > -dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb
> > +dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb \
> > +   sp9860g-1h10.dtb
> >
> >  always := $(dtb-y)
> >  subdir-y   := $(dts-dirs)
> > diff --git a/arch/arm64/boot/dts/sprd/sc9860.dtsi 
> > b/arch/arm64/boot/dts/sprd/sc9860.dtsi
> > new file mode 100644
> > index 000..604a8c9
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/sprd/sc9860.dtsi
> > @@ -0,0 +1,534 @@
> 
> [...]
> 
> > +   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 = <0x>;
> 
> 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
was definitely not elegant, the author indeed intendly didn't want CPU entered 
this
state, I will remove this node therefore.
 
> Can you get me the dump of:
> grep "" /sys/devices/system/cpu/cpu*/cpuidle/state*/{time,usage}
> 

FYI: https://www.irccloud.com/pastebin/XyEMLzfq/

Thanks,
Chunyan

> IIUC, you might have seen boot issue without this values and workaround
> the issue with such high values ? If so please drop this state.
> 
> > +   entry-latency-us = <1500>;
> > +   exit-latency-us = <1500>;
> > +   min-residency-us = <0x>;
> > +   local-timer-stop;
> > +   arm,psci-suspend-param = <0x01010005>;
> > +   };
> > +   };
> >
> 
> --
> Regards,
> Sudeep

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

[Chunyan Zhang]

Hi Sudeep,

On 二,  2月 14, 2017 at 04:44:53下午 +, Sudeep Holla wrote:
> On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
>  wrote:
> > From: Orson Zhai 
> >
> > SC9860G is a 8 cores of A53 SoC with 4G LTE support SoC from Spreadtrum.
> >
> > According to regular hierarchy of sprd dts, whale2.dtsi contains SoC
> > peripherals IP nodes, sc9860.dtsi contains stuff related to ARM core stuff
> > and sp9860g dts is for the board level.
> >
> > Signed-off-by: Orson Zhai 
> > Signed-off-by: Chunyan Zhang 
> > ---
> >  arch/arm64/boot/dts/sprd/Makefile |   3 +-
> >  arch/arm64/boot/dts/sprd/sc9860.dtsi  | 534 
> > ++
> >  arch/arm64/boot/dts/sprd/sp9860g-1h10.dts |  58 
> >  arch/arm64/boot/dts/sprd/whale2.dtsi  |  66 
> >  4 files changed, 660 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/arm64/boot/dts/sprd/sc9860.dtsi
> >  create mode 100644 arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> >  create mode 100644 arch/arm64/boot/dts/sprd/whale2.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/sprd/Makefile 
> > b/arch/arm64/boot/dts/sprd/Makefile
> > index b658c5e..f0535e6 100644
> > --- a/arch/arm64/boot/dts/sprd/Makefile
> > +++ b/arch/arm64/boot/dts/sprd/Makefile
> > @@ -1,4 +1,5 @@
> > -dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb
> > +dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb \
> > +   sp9860g-1h10.dtb
> >
> >  always := $(dtb-y)
> >  subdir-y   := $(dts-dirs)
> > diff --git a/arch/arm64/boot/dts/sprd/sc9860.dtsi 
> > b/arch/arm64/boot/dts/sprd/sc9860.dtsi
> > new file mode 100644
> > index 000..604a8c9
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/sprd/sc9860.dtsi
> > @@ -0,0 +1,534 @@
> 
> [...]
> 
> > +   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 = <0x>;
> 
> 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
was definitely not elegant, the author indeed intendly didn't want CPU entered 
this
state, I will remove this node therefore.
 
> Can you get me the dump of:
> grep "" /sys/devices/system/cpu/cpu*/cpuidle/state*/{time,usage}
> 

FYI: https://www.irccloud.com/pastebin/XyEMLzfq/

Thanks,
Chunyan

> IIUC, you might have seen boot issue without this values and workaround
> the issue with such high values ? If so please drop this state.
> 
> > +   entry-latency-us = <1500>;
> > +   exit-latency-us = <1500>;
> > +   min-residency-us = <0x>;
> > +   local-timer-stop;
> > +   arm,psci-suspend-param = <0x01010005>;
> > +   };
> > +   };
> >
> 
> --
> Regards,
> Sudeep

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

[Chunyan Zhang]

On 二,  2月 14, 2017 at 03:57:43下午 +, Mark Rutland wrote:
> Hi,
> 
> On Tue, Feb 14, 2017 at 05:19:08PM +0800, Chunyan Zhang wrote:
> 
> > +   timer {
> > +   compatible = "arm,armv8-timer";
> > +   interrupts =  > +| IRQ_TYPE_LEVEL_LOW)>,
> > + > +| IRQ_TYPE_LEVEL_LOW)>,
> > + > +| IRQ_TYPE_LEVEL_LOW)>,
> > + > +| IRQ_TYPE_LEVEL_LOW)>;
> > +   clock-frequency = <2600>;
> > +   };
> 
> Your FW should program CNTFRQ on all CPUs, and this clock-frequency
> property should not be necessary. Please remove it.
>

Will remove it.
 
> > +
> > +   pmu {
> > +   compatible = "arm,armv8-pmuv3";
> 
> Please list "arm,cortex-a53-pmu" first.
> 
> > +   interrupts = ,
> > +   ,
> > +   ,
> > +   ,
> > +   ,
> > +   ,
> > +   ,
> > +   ;
> > +   };
> 
> As you are using SPIs, please add an interrupt-affinity property, per
> Documentation/devicetree/bindings/arm/pmu.txt.

Added, would appear in the next revision.

> 
> > diff --git a/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts 
> > b/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> > new file mode 100644
> > index 000..59390fc
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> > @@ -0,0 +1,58 @@
> > +/*
> > + * Spreadtrum SP9860g board DTS file
> > + *
> > + * Copyright (C) 2017, Spreadtrum Communications Inc.
> > + *
> > + * This file is licensed under a dual GPLv2 or X11 license.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "sc9860.dtsi"
> > +
> > +/ {
> > +   model = "Spreadtrum SP9860G 3GFHD Board";
> > +
> > +   compatible = "sprd,sc9860", "sprd,sp9860g-1h10";
> > +
> > +   sprd,sc-id = <9860 1 0x2>;
> 
> What is this? I can't see any documentation or user of this property in
> mainline.
> 

I will remove this property in the next iteration, actually this is for 
Spreadtrum's dtb tools only, I just forgot to remove it before sending this 
patch out.

Thanks,
Chunyan

> 
> Thanks,
> Mark.

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

[Chunyan Zhang]

On 二,  2月 14, 2017 at 03:57:43下午 +, Mark Rutland wrote:
> Hi,
> 
> On Tue, Feb 14, 2017 at 05:19:08PM +0800, Chunyan Zhang wrote:
> 
> > +   timer {
> > +   compatible = "arm,armv8-timer";
> > +   interrupts =  > +| IRQ_TYPE_LEVEL_LOW)>,
> > + > +| IRQ_TYPE_LEVEL_LOW)>,
> > + > +| IRQ_TYPE_LEVEL_LOW)>,
> > + > +| IRQ_TYPE_LEVEL_LOW)>;
> > +   clock-frequency = <2600>;
> > +   };
> 
> Your FW should program CNTFRQ on all CPUs, and this clock-frequency
> property should not be necessary. Please remove it.
>

Will remove it.
 
> > +
> > +   pmu {
> > +   compatible = "arm,armv8-pmuv3";
> 
> Please list "arm,cortex-a53-pmu" first.
> 
> > +   interrupts = ,
> > +   ,
> > +   ,
> > +   ,
> > +   ,
> > +   ,
> > +   ,
> > +   ;
> > +   };
> 
> As you are using SPIs, please add an interrupt-affinity property, per
> Documentation/devicetree/bindings/arm/pmu.txt.

Added, would appear in the next revision.

> 
> > diff --git a/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts 
> > b/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> > new file mode 100644
> > index 000..59390fc
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> > @@ -0,0 +1,58 @@
> > +/*
> > + * Spreadtrum SP9860g board DTS file
> > + *
> > + * Copyright (C) 2017, Spreadtrum Communications Inc.
> > + *
> > + * This file is licensed under a dual GPLv2 or X11 license.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "sc9860.dtsi"
> > +
> > +/ {
> > +   model = "Spreadtrum SP9860G 3GFHD Board";
> > +
> > +   compatible = "sprd,sc9860", "sprd,sp9860g-1h10";
> > +
> > +   sprd,sc-id = <9860 1 0x2>;
> 
> What is this? I can't see any documentation or user of this property in
> mainline.
> 

I will remove this property in the next iteration, actually this is for 
Spreadtrum's dtb tools only, I just forgot to remove it before sending this 
patch out.

Thanks,
Chunyan

> 
> Thanks,
> Mark.

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

[Sudeep Holla]

On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
 wrote:
> From: Orson Zhai 
>
> SC9860G is a 8 cores of A53 SoC with 4G LTE support SoC from Spreadtrum.
>
> According to regular hierarchy of sprd dts, whale2.dtsi contains SoC
> peripherals IP nodes, sc9860.dtsi contains stuff related to ARM core stuff
> and sp9860g dts is for the board level.
>
> Signed-off-by: Orson Zhai 
> Signed-off-by: Chunyan Zhang 
> ---
>  arch/arm64/boot/dts/sprd/Makefile |   3 +-
>  arch/arm64/boot/dts/sprd/sc9860.dtsi  | 534 
> ++
>  arch/arm64/boot/dts/sprd/sp9860g-1h10.dts |  58 
>  arch/arm64/boot/dts/sprd/whale2.dtsi  |  66 
>  4 files changed, 660 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/boot/dts/sprd/sc9860.dtsi
>  create mode 100644 arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
>  create mode 100644 arch/arm64/boot/dts/sprd/whale2.dtsi
>
> diff --git a/arch/arm64/boot/dts/sprd/Makefile 
> b/arch/arm64/boot/dts/sprd/Makefile
> index b658c5e..f0535e6 100644
> --- a/arch/arm64/boot/dts/sprd/Makefile
> +++ b/arch/arm64/boot/dts/sprd/Makefile
> @@ -1,4 +1,5 @@
> -dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb
> +dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb \
> +   sp9860g-1h10.dtb
>
>  always := $(dtb-y)
>  subdir-y   := $(dts-dirs)
> diff --git a/arch/arm64/boot/dts/sprd/sc9860.dtsi 
> b/arch/arm64/boot/dts/sprd/sc9860.dtsi
> new file mode 100644
> index 000..604a8c9
> --- /dev/null
> +++ b/arch/arm64/boot/dts/sprd/sc9860.dtsi
> @@ -0,0 +1,534 @@

[...]

> +   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 = <0x>;

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.

Can you get me the dump of:
grep "" /sys/devices/system/cpu/cpu*/cpuidle/state*/{time,usage}

IIUC, you might have seen boot issue without this values and workaround
the issue with such high values ? If so please drop this state.

> +   entry-latency-us = <1500>;
> +   exit-latency-us = <1500>;
> +   min-residency-us = <0x>;
> +   local-timer-stop;
> +   arm,psci-suspend-param = <0x01010005>;
> +   };
> +   };
>

--
Regards,
Sudeep

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

[Sudeep Holla]

On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
 wrote:
> From: Orson Zhai 
>
> SC9860G is a 8 cores of A53 SoC with 4G LTE support SoC from Spreadtrum.
>
> According to regular hierarchy of sprd dts, whale2.dtsi contains SoC
> peripherals IP nodes, sc9860.dtsi contains stuff related to ARM core stuff
> and sp9860g dts is for the board level.
>
> Signed-off-by: Orson Zhai 
> Signed-off-by: Chunyan Zhang 
> ---
>  arch/arm64/boot/dts/sprd/Makefile |   3 +-
>  arch/arm64/boot/dts/sprd/sc9860.dtsi  | 534 
> ++
>  arch/arm64/boot/dts/sprd/sp9860g-1h10.dts |  58 
>  arch/arm64/boot/dts/sprd/whale2.dtsi  |  66 
>  4 files changed, 660 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/boot/dts/sprd/sc9860.dtsi
>  create mode 100644 arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
>  create mode 100644 arch/arm64/boot/dts/sprd/whale2.dtsi
>
> diff --git a/arch/arm64/boot/dts/sprd/Makefile 
> b/arch/arm64/boot/dts/sprd/Makefile
> index b658c5e..f0535e6 100644
> --- a/arch/arm64/boot/dts/sprd/Makefile
> +++ b/arch/arm64/boot/dts/sprd/Makefile
> @@ -1,4 +1,5 @@
> -dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb
> +dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb \
> +   sp9860g-1h10.dtb
>
>  always := $(dtb-y)
>  subdir-y   := $(dts-dirs)
> diff --git a/arch/arm64/boot/dts/sprd/sc9860.dtsi 
> b/arch/arm64/boot/dts/sprd/sc9860.dtsi
> new file mode 100644
> index 000..604a8c9
> --- /dev/null
> +++ b/arch/arm64/boot/dts/sprd/sc9860.dtsi
> @@ -0,0 +1,534 @@

[...]

> +   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 = <0x>;

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.

Can you get me the dump of:
grep "" /sys/devices/system/cpu/cpu*/cpuidle/state*/{time,usage}

IIUC, you might have seen boot issue without this values and workaround
the issue with such high values ? If so please drop this state.

> +   entry-latency-us = <1500>;
> +   exit-latency-us = <1500>;
> +   min-residency-us = <0x>;
> +   local-timer-stop;
> +   arm,psci-suspend-param = <0x01010005>;
> +   };
> +   };
>

--
Regards,
Sudeep

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

[Mark Rutland]

Hi,

On Tue, Feb 14, 2017 at 05:19:08PM +0800, Chunyan Zhang wrote:

> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts =  +  | IRQ_TYPE_LEVEL_LOW)>,
> +   +  | IRQ_TYPE_LEVEL_LOW)>,
> +   +  | IRQ_TYPE_LEVEL_LOW)>,
> +   +  | IRQ_TYPE_LEVEL_LOW)>;
> + clock-frequency = <2600>;
> + };

Your FW should program CNTFRQ on all CPUs, and this clock-frequency
property should not be necessary. Please remove it.

> +
> + pmu {
> + compatible = "arm,armv8-pmuv3";

Please list "arm,cortex-a53-pmu" first.

> + interrupts = ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ;
> + };

As you are using SPIs, please add an interrupt-affinity property, per
Documentation/devicetree/bindings/arm/pmu.txt.

> diff --git a/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts 
> b/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> new file mode 100644
> index 000..59390fc
> --- /dev/null
> +++ b/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> @@ -0,0 +1,58 @@
> +/*
> + * Spreadtrum SP9860g board DTS file
> + *
> + * Copyright (C) 2017, Spreadtrum Communications Inc.
> + *
> + * This file is licensed under a dual GPLv2 or X11 license.
> + */
> +
> +/dts-v1/;
> +
> +#include "sc9860.dtsi"
> +
> +/ {
> + model = "Spreadtrum SP9860G 3GFHD Board";
> +
> + compatible = "sprd,sc9860", "sprd,sp9860g-1h10";
> +
> + sprd,sc-id = <9860 1 0x2>;

What is this? I can't see any documentation or user of this property in
mainline.


Thanks,
Mark.

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

[Mark Rutland]

Hi,

On Tue, Feb 14, 2017 at 05:19:08PM +0800, Chunyan Zhang wrote:

> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts =  +  | IRQ_TYPE_LEVEL_LOW)>,
> +   +  | IRQ_TYPE_LEVEL_LOW)>,
> +   +  | IRQ_TYPE_LEVEL_LOW)>,
> +   +  | IRQ_TYPE_LEVEL_LOW)>;
> + clock-frequency = <2600>;
> + };

Your FW should program CNTFRQ on all CPUs, and this clock-frequency
property should not be necessary. Please remove it.

> +
> + pmu {
> + compatible = "arm,armv8-pmuv3";

Please list "arm,cortex-a53-pmu" first.

> + interrupts = ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ;
> + };

As you are using SPIs, please add an interrupt-affinity property, per
Documentation/devicetree/bindings/arm/pmu.txt.

> diff --git a/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts 
> b/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> new file mode 100644
> index 000..59390fc
> --- /dev/null
> +++ b/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> @@ -0,0 +1,58 @@
> +/*
> + * Spreadtrum SP9860g board DTS file
> + *
> + * Copyright (C) 2017, Spreadtrum Communications Inc.
> + *
> + * This file is licensed under a dual GPLv2 or X11 license.
> + */
> +
> +/dts-v1/;
> +
> +#include "sc9860.dtsi"
> +
> +/ {
> + model = "Spreadtrum SP9860G 3GFHD Board";
> +
> + compatible = "sprd,sc9860", "sprd,sp9860g-1h10";
> +
> + sprd,sc-id = <9860 1 0x2>;

What is this? I can't see any documentation or user of this property in
mainline.


Thanks,
Mark.

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

[Chunyan Zhang]

From: Orson Zhai 

SC9860G is a 8 cores of A53 SoC with 4G LTE support SoC from Spreadtrum.

According to regular hierarchy of sprd dts, whale2.dtsi contains SoC
peripherals IP nodes, sc9860.dtsi contains stuff related to ARM core stuff
and sp9860g dts is for the board level.

Signed-off-by: Orson Zhai 
Signed-off-by: Chunyan Zhang 
---
 arch/arm64/boot/dts/sprd/Makefile |   3 +-
 arch/arm64/boot/dts/sprd/sc9860.dtsi  | 534 ++
 arch/arm64/boot/dts/sprd/sp9860g-1h10.dts |  58 
 arch/arm64/boot/dts/sprd/whale2.dtsi  |  66 
 4 files changed, 660 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/boot/dts/sprd/sc9860.dtsi
 create mode 100644 arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
 create mode 100644 arch/arm64/boot/dts/sprd/whale2.dtsi

diff --git a/arch/arm64/boot/dts/sprd/Makefile 
b/arch/arm64/boot/dts/sprd/Makefile
index b658c5e..f0535e6 100644
--- a/arch/arm64/boot/dts/sprd/Makefile
+++ b/arch/arm64/boot/dts/sprd/Makefile
@@ -1,4 +1,5 @@
-dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb
+dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb \
+   sp9860g-1h10.dtb
 
 always := $(dtb-y)
 subdir-y   := $(dts-dirs)
diff --git a/arch/arm64/boot/dts/sprd/sc9860.dtsi 
b/arch/arm64/boot/dts/sprd/sc9860.dtsi
new file mode 100644
index 000..604a8c9
--- /dev/null
+++ b/arch/arm64/boot/dts/sprd/sc9860.dtsi
@@ -0,0 +1,534 @@
+/*
+ * Spreadtrum SP9860 SoC DTS file
+ *
+ * Copyright (C) 2016, Spreadtrum Communications Inc.
+ *
+ * This file is licensed under a dual GPLv2 or X11 license.
+ */
+
+#include 
+#include "whale2.dtsi"
+
+/ {
+   cpus {
+   #address-cells = <2>;
+   #size-cells = <0>;
+
+   cpu-map {
+   cluster0 {
+   core0 {
+   cpu = <>;
+   };
+   core1 {
+   cpu = <>;
+   };
+   core2 {
+   cpu = <>;
+   };
+   core3 {
+   cpu = <>;
+   };
+   };
+
+   cluster1 {
+   core0 {
+   cpu = <>;
+   };
+   core1 {
+   cpu = <>;
+   };
+   core2 {
+   cpu = <>;
+   };
+   core3 {
+   cpu = <>;
+   };
+   };
+   };
+
+   CPU0: cpu@53 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x53>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU1: cpu@530001 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x530001>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU2: cpu@530002 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x530002>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU3: cpu@530003 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x530003>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU4: cpu@530100 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x530100>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU5: cpu@530101 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x530101>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU6: cpu@530102 {
+   device_type = 

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

[Chunyan Zhang]

From: Orson Zhai 

SC9860G is a 8 cores of A53 SoC with 4G LTE support SoC from Spreadtrum.

According to regular hierarchy of sprd dts, whale2.dtsi contains SoC
peripherals IP nodes, sc9860.dtsi contains stuff related to ARM core stuff
and sp9860g dts is for the board level.

Signed-off-by: Orson Zhai 
Signed-off-by: Chunyan Zhang 
---
 arch/arm64/boot/dts/sprd/Makefile |   3 +-
 arch/arm64/boot/dts/sprd/sc9860.dtsi  | 534 ++
 arch/arm64/boot/dts/sprd/sp9860g-1h10.dts |  58 
 arch/arm64/boot/dts/sprd/whale2.dtsi  |  66 
 4 files changed, 660 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/boot/dts/sprd/sc9860.dtsi
 create mode 100644 arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
 create mode 100644 arch/arm64/boot/dts/sprd/whale2.dtsi

diff --git a/arch/arm64/boot/dts/sprd/Makefile 
b/arch/arm64/boot/dts/sprd/Makefile
index b658c5e..f0535e6 100644
--- a/arch/arm64/boot/dts/sprd/Makefile
+++ b/arch/arm64/boot/dts/sprd/Makefile
@@ -1,4 +1,5 @@
-dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb
+dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb \
+   sp9860g-1h10.dtb
 
 always := $(dtb-y)
 subdir-y   := $(dts-dirs)
diff --git a/arch/arm64/boot/dts/sprd/sc9860.dtsi 
b/arch/arm64/boot/dts/sprd/sc9860.dtsi
new file mode 100644
index 000..604a8c9
--- /dev/null
+++ b/arch/arm64/boot/dts/sprd/sc9860.dtsi
@@ -0,0 +1,534 @@
+/*
+ * Spreadtrum SP9860 SoC DTS file
+ *
+ * Copyright (C) 2016, Spreadtrum Communications Inc.
+ *
+ * This file is licensed under a dual GPLv2 or X11 license.
+ */
+
+#include 
+#include "whale2.dtsi"
+
+/ {
+   cpus {
+   #address-cells = <2>;
+   #size-cells = <0>;
+
+   cpu-map {
+   cluster0 {
+   core0 {
+   cpu = <>;
+   };
+   core1 {
+   cpu = <>;
+   };
+   core2 {
+   cpu = <>;
+   };
+   core3 {
+   cpu = <>;
+   };
+   };
+
+   cluster1 {
+   core0 {
+   cpu = <>;
+   };
+   core1 {
+   cpu = <>;
+   };
+   core2 {
+   cpu = <>;
+   };
+   core3 {
+   cpu = <>;
+   };
+   };
+   };
+
+   CPU0: cpu@53 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x53>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU1: cpu@530001 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x530001>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU2: cpu@530002 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x530002>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU3: cpu@530003 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x530003>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU4: cpu@530100 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x530100>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU5: cpu@530101 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+   reg = <0x0 0x530101>;
+   enable-method = "psci";
+   cpu-idle-states = <_PD _PD _SLEEP>;
+   };
+
+   CPU6: cpu@530102 {
+   device_type = "cpu";
+   compatible = "arm,cortex-a53", "arm,armv8";
+