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

Commit Message

Edwin Chiu Feb. 21, 2022, 7:26 a.m. UTC

Create cpuidle driver for sunplus sp7021 chip

Signed-off-by: Edwin Chiu <[e-mail]>
---
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

Sudeep Holla Feb. 21, 2022, 10:51 a.m. UTC | #1

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 <[e-mail]>
> ---
> 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:	[e-mail]
>  S:	Maintained
>  F:	drivers/net/ethernet/dlink/sundance.c
>  
> +SUNPLUS CPUIDLE DRIVER
> +M:	Edwin Chiu <[e-mail]>
> +S:	Maintained
> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> +F:	include/linux/platform_data/cpuidle-sunplus.h
> +
>  SUPERH
>  M:	Yoshinori Sato <[e-mail]>
>  M:	Rich Felker <[e-mail]>
> 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) ?

Krzysztof Kozlowski Feb. 21, 2022, 4:47 p.m. UTC | #2

On 21/02/2022 08:26, Edwin Chiu wrote:
> Create cpuidle driver for sunplus sp7021 chip
> 
> Signed-off-by: Edwin Chiu <[e-mail]>
> ---
> 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:	[e-mail]
>  S:	Maintained
>  F:	drivers/net/ethernet/dlink/sundance.c
>  
> +SUNPLUS CPUIDLE DRIVER
> +M:	Edwin Chiu <[e-mail]>
> +S:	Maintained
> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> +F:	include/linux/platform_data/cpuidle-sunplus.h
> +
>  SUPERH
>  M:	Yoshinori Sato <[e-mail]>
>  M:	Rich Felker <[e-mail]>
> 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

Edwin Chiu 邱垂峰 March 1, 2022, 9:18 a.m. UTC | #3

> -----Original Message-----
> From: Sudeep Holla <[e-mail]>
> Sent: Monday, February 21, 2022 6:52 PM
> To: Edwin Chiu <[e-mail]>
> Cc: Edwin Chiu 邱垂峰 <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Sudeep Holla <[e-mail]>;
> [e-mail]; [e-mail]
> 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 <[e-mail]>
> > ---
> > 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:	[e-mail]
> >  S:	Maintained
> >  F:	drivers/net/ethernet/dlink/sundance.c
> >
> > +SUNPLUS CPUIDLE DRIVER
> > +M:	Edwin Chiu <[e-mail]>
> > +S:	Maintained
> > +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> > +F:	include/linux/platform_data/cpuidle-sunplus.h
> > +
> >  SUPERH
> >  M:	Yoshinori Sato <[e-mail]>
> >  M:	Rich Felker <[e-mail]>
> > 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: [phone] ext.2590
[e-mail]
300 新竹科學園區創新一路19號

Edwin Chiu 邱垂峰 March 1, 2022, 9:30 a.m. UTC | #4

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Tuesday, February 22, 2022 12:48 AM
> To: Edwin Chiu <[e-mail]>; Edwin Chiu 邱垂峰 <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> 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 <[e-mail]>
> > ---
> > 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:	[e-mail]
> >  S:	Maintained
> >  F:	drivers/net/ethernet/dlink/sundance.c
> >
> > +SUNPLUS CPUIDLE DRIVER
> > +M:	Edwin Chiu <[e-mail]>
> > +S:	Maintained
> > +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> > +F:	include/linux/platform_data/cpuidle-sunplus.h
> > +
> >  SUPERH
> >  M:	Yoshinori Sato <[e-mail]>
> >  M:	Rich Felker <[e-mail]>
> > 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: [phone] ext.2590
[e-mail]
300 新竹科學園區創新一路19號

Krzysztof Kozlowski March 1, 2022, 11:33 a.m. UTC | #5

On 01/03/2022 10:30, Edwin Chiu 邱垂峰 wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Tuesday, February 22, 2022 12:48 AM
>> To: Edwin Chiu <[e-mail]>; Edwin Chiu 邱垂峰 <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> 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 <[e-mail]>
>>> ---
>>> 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:	[e-mail]
>>>  S:	Maintained
>>>  F:	drivers/net/ethernet/dlink/sundance.c
>>>
>>> +SUNPLUS CPUIDLE DRIVER
>>> +M:	Edwin Chiu <[e-mail]>
>>> +S:	Maintained
>>> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
>>> +F:	include/linux/platform_data/cpuidle-sunplus.h
>>> +
>>>  SUPERH
>>>  M:	Yoshinori Sato <[e-mail]>
>>>  M:	Rich Felker <[e-mail]>
>>> 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 | #6

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

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Tuesday, March 1, 2022 7:34 PM
> To: Edwin Chiu 邱垂峰 <[e-mail]>; Edwin Chiu <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> 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 <[e-mail]>
> >> Sent: Tuesday, February 22, 2022 12:48 AM
> >> To: Edwin Chiu <[e-mail]>; Edwin Chiu 邱垂峰
> >> <[e-mail]>;
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> 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 <[e-mail]>
> >>> ---
> >>> 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:	[e-mail]
> >>>  S:	Maintained
> >>>  F:	drivers/net/ethernet/dlink/sundance.c
> >>>
> >>> +SUNPLUS CPUIDLE DRIVER
> >>> +M:	Edwin Chiu <[e-mail]>
> >>> +S:	Maintained
> >>> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> >>> +F:	include/linux/platform_data/cpuidle-sunplus.h
> >>> +
> >>>  SUPERH
> >>>  M:	Yoshinori Sato <[e-mail]>
> >>>  M:	Rich Felker <[e-mail]>
> >>> 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: [phone] ext.2590
[e-mail]
300 新竹科學園區創新一路19號

Krzysztof Kozlowski March 3, 2022, 9:34 a.m. UTC | #8

On 03/03/2022 10:01, Edwin Chiu 邱垂峰 wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Tuesday, March 1, 2022 7:34 PM
>> To: Edwin Chiu 邱垂峰 <[e-mail]>; Edwin Chiu <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> 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 <[e-mail]>
>>>> Sent: Tuesday, February 22, 2022 12:48 AM
>>>> To: Edwin Chiu <[e-mail]>; Edwin Chiu 邱垂峰
>>>> <[e-mail]>;
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]
>>>> 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 <[e-mail]>
>>>>> ---
>>>>> 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:	[e-mail]
>>>>>  S:	Maintained
>>>>>  F:	drivers/net/ethernet/dlink/sundance.c
>>>>>
>>>>> +SUNPLUS CPUIDLE DRIVER
>>>>> +M:	Edwin Chiu <[e-mail]>
>>>>> +S:	Maintained
>>>>> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
>>>>> +F:	include/linux/platform_data/cpuidle-sunplus.h
>>>>> +
>>>>>  SUPERH
>>>>>  M:	Yoshinori Sato <[e-mail]>
>>>>>  M:	Rich Felker <[e-mail]>
>>>>> 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.

Best regards,
Krzysztof

Sudeep Holla March 3, 2022, 10:02 a.m. UTC | #9

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 <[e-mail]>
> >> Sent: Tuesday, March 1, 2022 7:34 PM
> >> To: Edwin Chiu 邱垂峰 <[e-mail]>; Edwin Chiu <[e-mail]>;
> >> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> 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 <[e-mail]>
> >>>> Sent: Tuesday, February 22, 2022 12:48 AM
> >>>> To: Edwin Chiu <[e-mail]>; Edwin Chiu 邱垂峰
> >>>> <[e-mail]>;
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]
> >>>> 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 <[e-mail]>
> >>>>> ---
> >>>>> 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:	[e-mail]
> >>>>>  S:	Maintained
> >>>>>  F:	drivers/net/ethernet/dlink/sundance.c
> >>>>>
> >>>>> +SUNPLUS CPUIDLE DRIVER
> >>>>> +M:	Edwin Chiu <[e-mail]>
> >>>>> +S:	Maintained
> >>>>> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> >>>>> +F:	include/linux/platform_data/cpuidle-sunplus.h
> >>>>> +
> >>>>>  SUPERH
> >>>>>  M:	Yoshinori Sato <[e-mail]>
> >>>>>  M:	Rich Felker <[e-mail]>
> >>>>> 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

Edwin Chiu 邱垂峰 March 4, 2022, 11:24 a.m. UTC | #10

> -----Original Message-----
> From: Sudeep Holla <[e-mail]>
> Sent: Thursday, March 3, 2022 6:03 PM
> To: Krzysztof Kozlowski <[e-mail]>
> Cc: Edwin Chiu 邱垂峰 <[e-mail]>; Edwin Chiu <[e-mail]>;
> Sudeep Holla <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]
> 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 <[e-mail]>
> > >> Sent: Tuesday, March 1, 2022 7:34 PM
> > >> To: Edwin Chiu 邱垂峰 <[e-mail]>; Edwin Chiu
> > >> <[e-mail]>;
> > >> [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]
> > >> 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 <[e-mail]>
> > >>>> Sent: Tuesday, February 22, 2022 12:48 AM
> > >>>> To: Edwin Chiu <[e-mail]>; Edwin Chiu 邱垂峰
> > >>>> <[e-mail]>;
> > >>>> [e-mail]; [e-mail];
> > >>>> [e-mail]; [e-mail];
> > >>>> [e-mail]; [e-mail]
> > >>>> 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 <[e-mail]>
> > >>>>> ---
> > >>>>> 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:	[e-mail]
> > >>>>>  S:	Maintained
> > >>>>>  F:	drivers/net/ethernet/dlink/sundance.c
> > >>>>>
> > >>>>> +SUNPLUS CPUIDLE DRIVER
> > >>>>> +M:	Edwin Chiu <[e-mail]>
> > >>>>> +S:	Maintained
> > >>>>> +F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
> > >>>>> +F:	include/linux/platform_data/cpuidle-sunplus.h
> > >>>>> +
> > >>>>>  SUPERH
> > >>>>>  M:	Yoshinori Sato <[e-mail]>
> > >>>>>  M:	Rich Felker <[e-mail]>
> > >>>>> 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: [phone] ext.2590
[e-mail]
300 新竹科學園區創新一路19號

Sudeep Holla March 4, 2022, 2:37 p.m. UTC | #11

On Fri, Mar 04, 2022 at 11:24:32AM +0000, Edwin Chiu 邱垂峰 wrote:
> 
> Thanks your advice.
> Look like key point still only WFI function when cpuidle.

Indeed.

> As I explain before, only enable generic ARM cpuidle driver is not work.

Why do you think it is a must. Most arch(including arm) has default
arch_cpu_idle handler that will be called if no cpuidle driver is active.
It does execute the default WFI, so you don't need a driver to achieve
the same.

> It need enable-method code to assign cpuidle_ops functions.

Correct, but you may not need that driver to be active at all. That is the
main point of these discussions. Sorry if that was not mentioned explicitly
earlier.

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

So you want to add custom low power mode support in future, so add the driver
when that is ready. The platform must do WFI even now without the driver
you are adding. Have you checked that ?

> What does it mean for "please document the chosen "sunplus,sc-smp" as bot
> cpu idle and hotplug methods" ?

I meant if you are adding any custom SMP+Idle mentods you need to add the
compatible to [1] or [2] based on what is more appropriate.

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

Yes that covers the description of idle states but not the entry method.
There are 2 separate things. You need both "arm,idle-state" and
"sunplus,sc-smp" or "psci" whichever you decide to implement on your
platform. If there is no implementation yet, it is strongly suggested
to go for "psci" unless you have reasons not to. Please add that info
when you submit the custom support, I will check on that again when
you post. But for now you don't need anything.

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index e0dca8f..5c96428 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18252,6 +18252,12 @@  L:	[e-mail]
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS CPUIDLE DRIVER
+M:	Edwin Chiu <[e-mail]>
+S:	Maintained
+F:	arch/arm/mach-sunplus/cpuidle-sunplus.c
+F:	include/linux/platform_data/cpuidle-sunplus.h
+
 SUPERH
 M:	Yoshinori Sato <[e-mail]>
 M:	Rich Felker <[e-mail]>
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 <[e-mail]>");
+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