diff mbox series

[v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

Message ID 1628e048220f066204b8ac27f3cedf7f3cc02963.1645427180.git.edwinchiu0505tw@gmail.com
State New
Headers show
Series [v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 | expand

Commit Message

Edwin Chiu Feb. 21, 2022, 7:26 a.m. UTC 
Create cpuidle driver for sunplus sp7021 chip

Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com>
---
Changes in v3
 - Rearrangement #include sequence
 - Change remark style to /*~*/
 - Align author email address to same as sob
 - Optimal code
Changes in v4
 - According Rob Herringrobh's comment
   There is no need for this binding.
   Just wanting a different driver is not a reason
   for a duplicate schema.
   So remove yaml file and submit driver again.
Changes in v5
 - According Krzysztof's comment
   You either use appropriate compatible in DT
   or add your compatible to cpuidle-arm.
   Even if this did not work, then the solution is to
   use common parts, not to duplicate entire driver.
   According Sudeep's comment
   In short NACK for any dedicated driver for this platform,
   use the generic cpuidle-arm driver with appropriate platform hooks
   Create cpuidle-sunplus.c in arch/arm/mach-sunplus/
   for hook generic cpuidle-arm driver

 MAINTAINERS                                   |  6 ++
 arch/arm/mach-sunplus/cpuidle-sunplus.c       | 88 +++++++++++++++++
 include/linux/platform_data/cpuidle-sunplus.h | 12 ++++
 3 files changed, 106 insertions(+)
 create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c
 create mode 100644 include/linux/platform_data/cpuidle-sunplus.h

Comments

Edwin Chiu 邱垂峰 March 1, 2022, 9:18 a.m. UTC | #1 
> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: Monday, February 21, 2022 6:52 PM
> To: Edwin Chiu <edwinchiu0505tw@gmail.com>
> Cc: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; robh+dt@kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; rafael@kernel.org; Sudeep Holla <sudeep.holla@arm.com>;
> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org
> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
> 
> On Mon, Feb 21, 2022 at 03:26:18PM +0800, Edwin Chiu wrote:
> > Create cpuidle driver for sunplus sp7021 chip
> >
> > Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com>
> > ---
> > Changes in v3
> >  - Rearrangement #include sequence
> >  - Change remark style to /*~*/
> >  - Align author email address to same as sob
> >  - Optimal code
> > Changes in v4
> >  - According Rob Herringrobh's comment
> >    There is no need for this binding.
> >    Just wanting a different driver is not a reason
> >    for a duplicate schema.
> >    So remove yaml file and submit driver again.
> > Changes in v5
> >  - According Krzysztof's comment
> >    You either use appropriate compatible in DT
> >    or add your compatible to cpuidle-arm.
> >    Even if this did not work, then the solution is to
> >    use common parts, not to duplicate entire driver.
> >    According Sudeep's comment
> >    In short NACK for any dedicated driver for this platform,
> >    use the generic cpuidle-arm driver with appropriate platform hooks
> >    Create cpuidle-sunplus.c in arch/arm/mach-sunplus/
> >    for hook generic cpuidle-arm driver
> >
> >  MAINTAINERS                                   |  6 ++
> >  arch/arm/mach-sunplus/cpuidle-sunplus.c       | 88 +++++++++++++++++
> >  include/linux/platform_data/cpuidle-sunplus.h | 12 ++++
> >  3 files changed, 106 insertions(+)
> >  create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c
> >  create mode 100644 include/linux/platform_data/cpuidle-sunplus.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18252,6 +18252,12 @@ L:	netdev@vger.kernel.org
> >  S:	Maintained
> >  F:	drivers/net/ethernet/dlink/sundance.c
> >
> > +SUNPLUS CPUIDLE DRIVER
> > +M:	Edwin Chiu <edwinchiu0505tw@gmail.com>
> > +S:	Maintained
> > +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> > +F:	include/linux/platform_data/cpuidle-sunplus.h
> > +
> >  SUPERH
> >  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
> >  M:	Rich Felker <dalias@libc.org>
> > diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > b/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > new file mode 100644
> > index 0000000..e9d9738
> > --- /dev/null
> > +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > @@ -0,0 +1,88 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * SP7021 cpu idle Driver.
> > + * Copyright (C) Sunplus Tech / Tibbo Tech.
> > + */
> > +#define pr_fmt(fmt) "CPUidle arm: " fmt
> > +
> > +#include <linux/cpuidle.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_data/cpuidle-sunplus.h>
> > +
> > +#include <asm/cpuidle.h>
> > +
> > +typedef int (*idle_fn)(void);
> > +
> > +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops);
> > +
> > +static int sp7021_cpuidle_enter(unsigned long index) {
> > +	return __this_cpu_read(sp7021_idle_ops)[index]();
> > +}
> > +static int sp7021_cpu_spc(void)
> > +{
> > +	cpu_v7_do_idle();   /* idle to WFI */
> > +	return 0;
> > +}
> 
> You really don't need a cpuidle driver to just WFI for any states.
> Add the driver when you have something non WFI in the suspend function.
> 
> > +static const struct of_device_id sp7021_idle_state_match[] = {
> > +	{ .compatible = "arm,idle-state", .data = sp7021_cpu_spc },
> > +	{ },
> > +};
> 
> This is better than adding new driver like you did in previous version.
> 
> I did a quick check but couldn't figure out. How do cpus get switched ON or OFF on this platform(for
> example during CPU hotplug) ?
> 
> --
> Regards,
> Sudeep


In this patch, I just want to submit cpuidle function.
So there have no cpu hotplug function now.



邱垂峰 EdwinChiu
智能運算專案
T: +886-3-5786005 ext.2590
edwin.chiu@sunplus.com
300 新竹科學園區創新一路19號
Edwin Chiu 邱垂峰 March 1, 2022, 9:30 a.m. UTC | #2 
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Tuesday, February 22, 2022 12:48 AM
> To: Edwin Chiu <edwinchiu0505tw@gmail.com>; Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>;
> robh+dt@kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org;
> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org
> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
> 
> On 21/02/2022 08:26, Edwin Chiu wrote:
> > Create cpuidle driver for sunplus sp7021 chip
> >
> > Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com>
> > ---
> > Changes in v3
> >  - Rearrangement #include sequence
> >  - Change remark style to /*~*/
> >  - Align author email address to same as sob
> >  - Optimal code
> > Changes in v4
> >  - According Rob Herringrobh's comment
> >    There is no need for this binding.
> >    Just wanting a different driver is not a reason
> >    for a duplicate schema.
> >    So remove yaml file and submit driver again.
> > Changes in v5
> >  - According Krzysztof's comment
> >    You either use appropriate compatible in DT
> >    or add your compatible to cpuidle-arm.
> >    Even if this did not work, then the solution is to
> >    use common parts, not to duplicate entire driver.
> >    According Sudeep's comment
> >    In short NACK for any dedicated driver for this platform,
> >    use the generic cpuidle-arm driver with appropriate platform hooks
> >    Create cpuidle-sunplus.c in arch/arm/mach-sunplus/
> >    for hook generic cpuidle-arm driver
> >
> >  MAINTAINERS                                   |  6 ++
> >  arch/arm/mach-sunplus/cpuidle-sunplus.c       | 88 +++++++++++++++++
> >  include/linux/platform_data/cpuidle-sunplus.h | 12 ++++
> >  3 files changed, 106 insertions(+)
> >  create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c
> >  create mode 100644 include/linux/platform_data/cpuidle-sunplus.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -18252,6 +18252,12 @@ L:	netdev@vger.kernel.org
> >  S:	Maintained
> >  F:	drivers/net/ethernet/dlink/sundance.c
> >
> > +SUNPLUS CPUIDLE DRIVER
> > +M:	Edwin Chiu <edwinchiu0505tw@gmail.com>
> > +S:	Maintained
> > +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> > +F:	include/linux/platform_data/cpuidle-sunplus.h
> > +
> >  SUPERH
> >  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
> >  M:	Rich Felker <dalias@libc.org>
> > diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > b/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > new file mode 100644
> > index 0000000..e9d9738
> > --- /dev/null
> > +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > @@ -0,0 +1,88 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * SP7021 cpu idle Driver.
> > + * Copyright (C) Sunplus Tech / Tibbo Tech.
> > + */
> > +#define pr_fmt(fmt) "CPUidle arm: " fmt
> > +
> > +#include <linux/cpuidle.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_data/cpuidle-sunplus.h>
> > +
> > +#include <asm/cpuidle.h>
> > +
> > +typedef int (*idle_fn)(void);
> > +
> > +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops);
> > +
> > +static int sp7021_cpuidle_enter(unsigned long index) {
> > +	return __this_cpu_read(sp7021_idle_ops)[index]();
> > +}
> > +static int sp7021_cpu_spc(void)
> > +{
> > +	cpu_v7_do_idle();   /* idle to WFI */
> > +	return 0;
> > +}
> > +static const struct of_device_id sp7021_idle_state_match[] = {
> > +	{ .compatible = "arm,idle-state", .data = sp7021_cpu_spc },
> > +	{ },
> > +};
> 
> This is confusing. You want to have two drivers to bind to the same compatible? As I wrote in the
> previous messages, you should simply use arm,idle-state just like few other architectures.
> 
> 
> Best regards,
> Krzysztof


The patch v5 implemented according your comment.
Used common part of arm,idle-state.
Create new enable-method for cpuidle.ops function.
It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible.

What do you mean " simply use arm,idle-state just like few other architectures "?


邱垂峰 EdwinChiu
智能運算專案
T: +886-3-5786005 ext.2590
edwin.chiu@sunplus.com
300 新竹科學園區創新一路19號
Krzysztof Kozlowski March 1, 2022, 11:33 a.m. UTC | #3 
On 01/03/2022 10:30, Edwin Chiu 邱垂峰 wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Tuesday, February 22, 2022 12:48 AM
>> To: Edwin Chiu <edwinchiu0505tw@gmail.com>; Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>;
>> robh+dt@kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org;
>> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org
>> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
>>
>> On 21/02/2022 08:26, Edwin Chiu wrote:
>>> Create cpuidle driver for sunplus sp7021 chip
>>>
>>> Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com>
>>> ---
>>> Changes in v3
>>>  - Rearrangement #include sequence
>>>  - Change remark style to /*~*/
>>>  - Align author email address to same as sob
>>>  - Optimal code
>>> Changes in v4
>>>  - According Rob Herringrobh's comment
>>>    There is no need for this binding.
>>>    Just wanting a different driver is not a reason
>>>    for a duplicate schema.
>>>    So remove yaml file and submit driver again.
>>> Changes in v5
>>>  - According Krzysztof's comment
>>>    You either use appropriate compatible in DT
>>>    or add your compatible to cpuidle-arm.
>>>    Even if this did not work, then the solution is to
>>>    use common parts, not to duplicate entire driver.
>>>    According Sudeep's comment
>>>    In short NACK for any dedicated driver for this platform,
>>>    use the generic cpuidle-arm driver with appropriate platform hooks
>>>    Create cpuidle-sunplus.c in arch/arm/mach-sunplus/
>>>    for hook generic cpuidle-arm driver
>>>
>>>  MAINTAINERS                                   |  6 ++
>>>  arch/arm/mach-sunplus/cpuidle-sunplus.c       | 88 +++++++++++++++++
>>>  include/linux/platform_data/cpuidle-sunplus.h | 12 ++++
>>>  3 files changed, 106 insertions(+)
>>>  create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c
>>>  create mode 100644 include/linux/platform_data/cpuidle-sunplus.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -18252,6 +18252,12 @@ L:	netdev@vger.kernel.org
>>>  S:	Maintained
>>>  F:	drivers/net/ethernet/dlink/sundance.c
>>>
>>> +SUNPLUS CPUIDLE DRIVER
>>> +M:	Edwin Chiu <edwinchiu0505tw@gmail.com>
>>> +S:	Maintained
>>> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
>>> +F:	include/linux/platform_data/cpuidle-sunplus.h
>>> +
>>>  SUPERH
>>>  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
>>>  M:	Rich Felker <dalias@libc.org>
>>> diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c
>>> b/arch/arm/mach-sunplus/cpuidle-sunplus.c
>>> new file mode 100644
>>> index 0000000..e9d9738
>>> --- /dev/null
>>> +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c
>>> @@ -0,0 +1,88 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * SP7021 cpu idle Driver.
>>> + * Copyright (C) Sunplus Tech / Tibbo Tech.
>>> + */
>>> +#define pr_fmt(fmt) "CPUidle arm: " fmt
>>> +
>>> +#include <linux/cpuidle.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_data/cpuidle-sunplus.h>
>>> +
>>> +#include <asm/cpuidle.h>
>>> +
>>> +typedef int (*idle_fn)(void);
>>> +
>>> +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops);
>>> +
>>> +static int sp7021_cpuidle_enter(unsigned long index) {
>>> +	return __this_cpu_read(sp7021_idle_ops)[index]();
>>> +}
>>> +static int sp7021_cpu_spc(void)
>>> +{
>>> +	cpu_v7_do_idle();   /* idle to WFI */
>>> +	return 0;
>>> +}
>>> +static const struct of_device_id sp7021_idle_state_match[] = {
>>> +	{ .compatible = "arm,idle-state", .data = sp7021_cpu_spc },
>>> +	{ },
>>> +};
>>
>> This is confusing. You want to have two drivers to bind to the same compatible? As I wrote in the
>> previous messages, you should simply use arm,idle-state just like few other architectures.
>>
>>
>> Best regards,
>> Krzysztof
> 
> 
> The patch v5 implemented according your comment.
> Used common part of arm,idle-state.
> Create new enable-method for cpuidle.ops function.
> It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible.
> 
> What do you mean " simply use arm,idle-state just like few other architectures "?
>

I mean, do it similarly (by using arm,idle-state and other related
properties) to for example ti,am4372/ti,am3352.

Best regards,
Krzysztof
Sudeep Holla March 1, 2022, 12:23 p.m. UTC | #4 
On Tue, Mar 01, 2022 at 09:18:31AM +0000, Edwin Chiu 邱垂峰 wrote:
> > 
> > You really don't need a cpuidle driver to just WFI for any states.
> > Add the driver when you have something non WFI in the suspend function.
> >

This is still valid and you haven't responded to this.

> > > +static const struct of_device_id sp7021_idle_state_match[] = {
> > > +	{ .compatible = "arm,idle-state", .data = sp7021_cpu_spc },
> > > +	{ },
> > > +};
> > 
> > This is better than adding new driver like you did in previous version.
> > 
> > I did a quick check but couldn't figure out. How do cpus get switched ON
> > or OFF on this platform(for example during CPU hotplug) ?.
> > 
> 
> In this patch, I just want to submit cpuidle function.
> So there have no cpu hotplug function now.


You need to document the binding now for both idle and hotplug. You
can't mix and match. You can either use PSCI or custom "sunplus,sc-smp"
for both cpu on/off and suspend. So you must document it now even if you
don't plan to support hotplug now. And when you add later, you must use
the same method.

However, you still don't need this driver for just WFI, so please
explain why you think otherwise. Until then, you still won't get ACK
from my side.
Edwin Chiu 邱垂峰 March 3, 2022, 9:01 a.m. UTC | #5 
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Tuesday, March 1, 2022 7:34 PM
> To: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; Edwin Chiu <edwinchiu0505tw@gmail.com>;
> robh+dt@kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org;
> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org
> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
> 
> On 01/03/2022 10:30, Edwin Chiu 邱垂峰 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Tuesday, February 22, 2022 12:48 AM
> >> To: Edwin Chiu <edwinchiu0505tw@gmail.com>; Edwin Chiu 邱垂峰
> >> <edwin.chiu@sunplus.com>;
> >> robh+dt@kernel.org; devicetree@vger.kernel.org;
> >> robh+linux-kernel@vger.kernel.org; rafael@kernel.org;
> >> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org
> >> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for
> >> sunplus sp7021
> >>
> >> On 21/02/2022 08:26, Edwin Chiu wrote:
> >>> Create cpuidle driver for sunplus sp7021 chip
> >>>
> >>> Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com>
> >>> ---
> >>> Changes in v3
> >>>  - Rearrangement #include sequence
> >>>  - Change remark style to /*~*/
> >>>  - Align author email address to same as sob
> >>>  - Optimal code
> >>> Changes in v4
> >>>  - According Rob Herringrobh's comment
> >>>    There is no need for this binding.
> >>>    Just wanting a different driver is not a reason
> >>>    for a duplicate schema.
> >>>    So remove yaml file and submit driver again.
> >>> Changes in v5
> >>>  - According Krzysztof's comment
> >>>    You either use appropriate compatible in DT
> >>>    or add your compatible to cpuidle-arm.
> >>>    Even if this did not work, then the solution is to
> >>>    use common parts, not to duplicate entire driver.
> >>>    According Sudeep's comment
> >>>    In short NACK for any dedicated driver for this platform,
> >>>    use the generic cpuidle-arm driver with appropriate platform hooks
> >>>    Create cpuidle-sunplus.c in arch/arm/mach-sunplus/
> >>>    for hook generic cpuidle-arm driver
> >>>
> >>>  MAINTAINERS                                   |  6 ++
> >>>  arch/arm/mach-sunplus/cpuidle-sunplus.c       | 88 +++++++++++++++++
> >>>  include/linux/platform_data/cpuidle-sunplus.h | 12 ++++
> >>>  3 files changed, 106 insertions(+)
> >>>  create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c
> >>>  create mode 100644 include/linux/platform_data/cpuidle-sunplus.h
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -18252,6 +18252,12 @@ L:	netdev@vger.kernel.org
> >>>  S:	Maintained
> >>>  F:	drivers/net/ethernet/dlink/sundance.c
> >>>
> >>> +SUNPLUS CPUIDLE DRIVER
> >>> +M:	Edwin Chiu <edwinchiu0505tw@gmail.com>
> >>> +S:	Maintained
> >>> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> >>> +F:	include/linux/platform_data/cpuidle-sunplus.h
> >>> +
> >>>  SUPERH
> >>>  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
> >>>  M:	Rich Felker <dalias@libc.org>
> >>> diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c
> >>> b/arch/arm/mach-sunplus/cpuidle-sunplus.c
> >>> new file mode 100644
> >>> index 0000000..e9d9738
> >>> --- /dev/null
> >>> +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c
> >>> @@ -0,0 +1,88 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-only
> >>> +/*
> >>> + * SP7021 cpu idle Driver.
> >>> + * Copyright (C) Sunplus Tech / Tibbo Tech.
> >>> + */
> >>> +#define pr_fmt(fmt) "CPUidle arm: " fmt
> >>> +
> >>> +#include <linux/cpuidle.h>
> >>> +#include <linux/of_device.h>
> >>> +#include <linux/platform_data/cpuidle-sunplus.h>
> >>> +
> >>> +#include <asm/cpuidle.h>
> >>> +
> >>> +typedef int (*idle_fn)(void);
> >>> +
> >>> +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops);
> >>> +
> >>> +static int sp7021_cpuidle_enter(unsigned long index) {
> >>> +	return __this_cpu_read(sp7021_idle_ops)[index]();
> >>> +}
> >>> +static int sp7021_cpu_spc(void)
> >>> +{
> >>> +	cpu_v7_do_idle();   /* idle to WFI */
> >>> +	return 0;
> >>> +}
> >>> +static const struct of_device_id sp7021_idle_state_match[] = {
> >>> +	{ .compatible = "arm,idle-state", .data = sp7021_cpu_spc },
> >>> +	{ },
> >>> +};
> >>
> >> This is confusing. You want to have two drivers to bind to the same
> >> compatible? As I wrote in the previous messages, you should simply use arm,idle-state just like few
> other architectures.
> >>
> >>
> >> Best regards,
> >> Krzysztof
> >
> >
> > The patch v5 implemented according your comment.
> > Used common part of arm,idle-state.
> > Create new enable-method for cpuidle.ops function.
> > It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible.
> >
> > What do you mean " simply use arm,idle-state just like few other architectures "?
> >
> 
> I mean, do it similarly (by using arm,idle-state and other related
> properties) to for example ti,am4372/ti,am3352.
> 
> Best regards,
> Krzysztof


The am3352 cpuidle code structure is very similar to ours.				
Used enable-method = "ti,am3352" and compatible = "arm,idle-state" in am33xx.dtsi				
Used CPUIDLE_METHOD_OF_DECLARE(pm33xx_idle, "ti,am3352", &amx3_cpuidle_ops) in pm33xx-core.c				
				
The difference are				
am3352				
amx3_idle_init(~) assign idle_states[i].wfi_flags = states[i].wfi_flags;				
amx3_idle_enter(~) call idle_fn(idle_state->wfi_flags)				
				
sunplus-sp7021				
sp7021_cpuidle_init(~) assign fns[i] = idle_fns[i];				
sp7021_cpuidle_enter(~) call __this_cpu_read(sp7021_idle_ops)[index]();				

I don't think am3352 cpuidle code architecture simpler than ours.
The idle_fn function need more complex method to be assign.
How do you think?


邱垂峰 EdwinChiu
智能運算專案
T: +886-3-5786005 ext.2590
edwin.chiu@sunplus.com
300 新竹科學園區創新一路19號
Edwin Chiu 邱垂峰 March 4, 2022, 11:24 a.m. UTC | #6 
> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: Thursday, March 3, 2022 6:03 PM
> To: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; Edwin Chiu <edwinchiu0505tw@gmail.com>;
> Sudeep Holla <sudeep.holla@arm.com>; robh+dt@kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; rafael@kernel.org; daniel.lezcano@linaro.org; linux-pm@vger.kernel.org
> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
> 
> On Thu, Mar 03, 2022 at 10:34:31AM +0100, Krzysztof Kozlowski wrote:
> > On 03/03/2022 10:01, Edwin Chiu 邱垂峰 wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Krzysztof Kozlowski <krzk@kernel.org>
> > >> Sent: Tuesday, March 1, 2022 7:34 PM
> > >> To: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; Edwin Chiu
> > >> <edwinchiu0505tw@gmail.com>;
> > >> robh+dt@kernel.org; devicetree@vger.kernel.org;
> > >> robh+linux-kernel@vger.kernel.org; rafael@kernel.org;
> > >> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org
> > >> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for
> > >> sunplus sp7021
> > >>
> > >> On 01/03/2022 10:30, Edwin Chiu 邱垂峰 wrote:
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Krzysztof Kozlowski <krzk@kernel.org>
> > >>>> Sent: Tuesday, February 22, 2022 12:48 AM
> > >>>> To: Edwin Chiu <edwinchiu0505tw@gmail.com>; Edwin Chiu 邱垂峰
> > >>>> <edwin.chiu@sunplus.com>;
> > >>>> robh+dt@kernel.org; devicetree@vger.kernel.org;
> > >>>> robh+linux-kernel@vger.kernel.org; rafael@kernel.org;
> > >>>> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org
> > >>>> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver
> > >>>> for sunplus sp7021
> > >>>>
> > >>>> On 21/02/2022 08:26, Edwin Chiu wrote:
> > >>>>> Create cpuidle driver for sunplus sp7021 chip
> > >>>>>
> > >>>>> Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com>
> > >>>>> ---
> > >>>>> Changes in v3
> > >>>>>  - Rearrangement #include sequence
> > >>>>>  - Change remark style to /*~*/
> > >>>>>  - Align author email address to same as sob
> > >>>>>  - Optimal code
> > >>>>> Changes in v4
> > >>>>>  - According Rob Herringrobh's comment
> > >>>>>    There is no need for this binding.
> > >>>>>    Just wanting a different driver is not a reason
> > >>>>>    for a duplicate schema.
> > >>>>>    So remove yaml file and submit driver again.
> > >>>>> Changes in v5
> > >>>>>  - According Krzysztof's comment
> > >>>>>    You either use appropriate compatible in DT
> > >>>>>    or add your compatible to cpuidle-arm.
> > >>>>>    Even if this did not work, then the solution is to
> > >>>>>    use common parts, not to duplicate entire driver.
> > >>>>>    According Sudeep's comment
> > >>>>>    In short NACK for any dedicated driver for this platform,
> > >>>>>    use the generic cpuidle-arm driver with appropriate platform hooks
> > >>>>>    Create cpuidle-sunplus.c in arch/arm/mach-sunplus/
> > >>>>>    for hook generic cpuidle-arm driver
> > >>>>>
> > >>>>>  MAINTAINERS                                   |  6 ++
> > >>>>>  arch/arm/mach-sunplus/cpuidle-sunplus.c       | 88 +++++++++++++++++
> > >>>>>  include/linux/platform_data/cpuidle-sunplus.h | 12 ++++
> > >>>>>  3 files changed, 106 insertions(+)  create mode 100644
> > >>>>> arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>>  create mode 100644
> > >>>>> include/linux/platform_data/cpuidle-sunplus.h
> > >>>>>
> > >>>>> diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428
> > >>>>> 100644
> > >>>>> --- a/MAINTAINERS
> > >>>>> +++ b/MAINTAINERS
> > >>>>> @@ -18252,6 +18252,12 @@ L:	netdev@vger.kernel.org
> > >>>>>  S:	Maintained
> > >>>>>  F:	drivers/net/ethernet/dlink/sundance.c
> > >>>>>
> > >>>>> +SUNPLUS CPUIDLE DRIVER
> > >>>>> +M:	Edwin Chiu <edwinchiu0505tw@gmail.com>
> > >>>>> +S:	Maintained
> > >>>>> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>> +F:	include/linux/platform_data/cpuidle-sunplus.h
> > >>>>> +
> > >>>>>  SUPERH
> > >>>>>  M:	Yoshinori Sato <ysato@users.sourceforge.jp>
> > >>>>>  M:	Rich Felker <dalias@libc.org>
> > >>>>> diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>> b/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>> new file mode 100644
> > >>>>> index 0000000..e9d9738
> > >>>>> --- /dev/null
> > >>>>> +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>> @@ -0,0 +1,88 @@
> > >>>>> +// SPDX-License-Identifier: GPL-2.0-only
> > >>>>> +/*
> > >>>>> + * SP7021 cpu idle Driver.
> > >>>>> + * Copyright (C) Sunplus Tech / Tibbo Tech.
> > >>>>> + */
> > >>>>> +#define pr_fmt(fmt) "CPUidle arm: " fmt
> > >>>>> +
> > >>>>> +#include <linux/cpuidle.h>
> > >>>>> +#include <linux/of_device.h>
> > >>>>> +#include <linux/platform_data/cpuidle-sunplus.h>
> > >>>>> +
> > >>>>> +#include <asm/cpuidle.h>
> > >>>>> +
> > >>>>> +typedef int (*idle_fn)(void);
> > >>>>> +
> > >>>>> +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops);
> > >>>>> +
> > >>>>> +static int sp7021_cpuidle_enter(unsigned long index) {
> > >>>>> +	return __this_cpu_read(sp7021_idle_ops)[index]();
> > >>>>> +}
> > >>>>> +static int sp7021_cpu_spc(void) {
> > >>>>> +	cpu_v7_do_idle();   /* idle to WFI */
> > >>>>> +	return 0;
> > >>>>> +}
> > >>>>> +static const struct of_device_id sp7021_idle_state_match[] = {
> > >>>>> +	{ .compatible = "arm,idle-state", .data = sp7021_cpu_spc },
> > >>>>> +	{ },
> > >>>>> +};
> > >>>>
> > >>>> This is confusing. You want to have two drivers to bind to the
> > >>>> same compatible? As I wrote in the previous messages, you should
> > >>>> simply use arm,idle-state just like few
> > >> other architectures.
> > >>>>
> > >>>>
> > >>>> Best regards,
> > >>>> Krzysztof
> > >>>
> > >>>
> > >>> The patch v5 implemented according your comment.
> > >>> Used common part of arm,idle-state.
> > >>> Create new enable-method for cpuidle.ops function.
> > >>> It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible.
> > >>>
> > >>> What do you mean " simply use arm,idle-state just like few other architectures "?
> > >>>
> > >>
> > >> I mean, do it similarly (by using arm,idle-state and other related
> > >> properties) to for example ti,am4372/ti,am3352.
> > >>
> > >> Best regards,
> > >> Krzysztof
> > >
> > >
> > > The am3352 cpuidle code structure is very similar to ours.
> > > Used enable-method = "ti,am3352" and compatible = "arm,idle-state"
> > > in am33xx.dtsi Used CPUIDLE_METHOD_OF_DECLARE(pm33xx_idle,
> > > "ti,am3352", &amx3_cpuidle_ops) in pm33xx-core.c
> > >
> > > The difference are
> > > am3352
> > > amx3_idle_init(~) assign idle_states[i].wfi_flags =
> > > states[i].wfi_flags;
> > > amx3_idle_enter(~) call idle_fn(idle_state->wfi_flags)
> > >
> > > sunplus-sp7021
> > > sp7021_cpuidle_init(~) assign fns[i] = idle_fns[i];
> > > sp7021_cpuidle_enter(~) call
> > > __this_cpu_read(sp7021_idle_ops)[index]();
> > >
> > > I don't think am3352 cpuidle code architecture simpler than ours.
> > > The idle_fn function need more complex method to be assign.
> > > How do you think?
> >
> > You duplicated a driver, entire pieces of code. This is not acceptable.
> > Therefore it does not really make sense to discuss whether duplicated
> > solution seems simpler or not... We won't accept duplicated code.
> > Especially for WFI-only driver.
> >
> 
> +1 for above comment.
> 
> In addition, the reference platform am33xx* doesn't seem to support hotplug (may be I am missing to
> see but quick grep gave no results) and their idle is definitely not just WFI. So what I asked is that please
> document the chosen "sunplus,sc-smp" as bot cpu idle and hotplug methods and when you support non
> WFI states, we can revisit this. Also you must stick to this hotplug method whenever you decided to
> support it.
> 
> 
> --
> Regards,
> Sudeep


Thanks your advice.
Look like key point still only WFI function when cpuidle.
As I explain before, only enable generic ARM cpuidle driver is not work.
It need enable-method code to assign cpuidle_ops functions.
"psci" is one of enable-method, but there have problem in my side due to smc or secure code unsupported.
So I create cpuidle-sunplus.c code with "sunplus,sc-smp" to let cpuidle code complete for our source code. 
With this structure, I can add more custom low power code in the future.

What does it mean for "please document the chosen "sunplus,sc-smp" as bot cpu idle and hotplug methods" ?
Does it mean "edit yaml file"? (Previously, I submit yaml file also, but Rob say I don't need submit when I use compatible="arm,idle-state")


邱垂峰 EdwinChiu
智能運算專案
T: +886-3-5786005 ext.2590
edwin.chiu@sunplus.com
300 新竹科學園區創新一路19號
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index e0dca8f..5c96428 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18252,6 +18252,12 @@  L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS CPUIDLE DRIVER
+M:	Edwin Chiu <edwinchiu0505tw@gmail.com>
+S:	Maintained
+F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
+F:	include/linux/platform_data/cpuidle-sunplus.h
+
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
 M:	Rich Felker <dalias@libc.org>
diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c b/arch/arm/mach-sunplus/cpuidle-sunplus.c
new file mode 100644
index 0000000..e9d9738
--- /dev/null
+++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c
@@ -0,0 +1,88 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SP7021 cpu idle Driver.
+ * Copyright (C) Sunplus Tech / Tibbo Tech.
+ */
+#define pr_fmt(fmt) "CPUidle arm: " fmt
+
+#include <linux/cpuidle.h>
+#include <linux/of_device.h>
+#include <linux/platform_data/cpuidle-sunplus.h>
+
+#include <asm/cpuidle.h>
+
+typedef int (*idle_fn)(void);
+
+static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops);
+
+static int sp7021_cpuidle_enter(unsigned long index)
+{
+	return __this_cpu_read(sp7021_idle_ops)[index]();
+}
+static int sp7021_cpu_spc(void)
+{
+	cpu_v7_do_idle();   /* idle to WFI */
+	return 0;
+}
+static const struct of_device_id sp7021_idle_state_match[] = {
+	{ .compatible = "arm,idle-state", .data = sp7021_cpu_spc },
+	{ },
+};
+static int sp7021_cpuidle_init(struct device_node *cpu_node, int cpu)
+{
+	const struct of_device_id *match_id;
+	struct device_node *state_node;
+	int i;
+	int state_count = 1;
+	idle_fn idle_fns[CPUIDLE_STATE_MAX];
+	idle_fn *fns;
+
+	for (i = 0; ; i++) {
+		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+		if (!state_node)
+			break;
+
+		if (!of_device_is_available(state_node))
+			continue;
+
+		if (i == CPUIDLE_STATE_MAX) {
+			pr_warn("%s: cpuidle states reached max possible\n",
+					__func__);
+			break;
+		}
+
+		match_id = of_match_node(sp7021_idle_state_match, state_node);
+		if (!match_id)
+			return -ENODEV;
+
+		idle_fns[state_count] = match_id->data;
+
+		state_count++;
+	}
+
+	if (state_count == 1)
+		goto check_sp;
+
+	fns = devm_kcalloc(get_cpu_device(cpu), state_count, sizeof(*fns),
+			GFP_KERNEL);
+	if (!fns)
+		return -ENOMEM;
+
+	for (i = 1; i < state_count; i++)
+		fns[i] = idle_fns[i];
+
+	per_cpu(sp7021_idle_ops, cpu) = fns;
+
+check_sp:
+	return 0;
+}
+static const struct cpuidle_ops sp7021_cpuidle_ops __initconst = {
+	.suspend = sp7021_cpuidle_enter,
+	.init = sp7021_cpuidle_init,
+};
+CPUIDLE_METHOD_OF_DECLARE(sc_smp, "sunplus,sc-smp", &sp7021_cpuidle_ops);
+
+MODULE_AUTHOR("Edwin Chiu <edwinchiu0505tw@gmail.com>");
+MODULE_DESCRIPTION("Sunplus sp7021 cpuidle driver");
+MODULE_LICENSE("GPL");
+
diff --git a/include/linux/platform_data/cpuidle-sunplus.h b/include/linux/platform_data/cpuidle-sunplus.h
new file mode 100644
index 0000000..12e98e5
--- /dev/null
+++ b/include/linux/platform_data/cpuidle-sunplus.h
@@ -0,0 +1,12 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * SP7021 cpu idle Driver.
+ * Copyright (C) Sunplus Tech / Tibbo Tech.
+ */
+
+#ifndef __CPUIDLE_SP7021_H
+#define __CPUIDLE_SP7021_H
+
+extern int cpu_v7_do_idle(void);
+
+#endif