Add cpuidle driver for Sunplus SP7021
Subsystems (2)
PATCH v5 1/1 Add cpuidle driver for Sunplus SP7021
This is a patch series for cpuidle driver for Sunplus SP7021 SoC.
Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD card and etc.) into a single chip. It is designed for industrial control.
Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html
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
Edwin Chiu (1):
cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
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
Signature · 1 line
2.7.4
PATCH v5 1/1 cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Create cpuidle driver for sunplus sp7021 chip
---
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
MAINTAINERS +6 -0
@@ -18252,6 +18252,12 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <[email protected]> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <[email protected]> M: Rich Felker <[email protected]>
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +88 -0
88 lines not on this page · raw message
include/linux/platform_data/cpuidle-sunplus.h new file +12 -0
@@ -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
Signature · 1 line
2.7.4
Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Quoted 73 lines · Edwin Chiu
Create cpuidle driver for sunplus sp7021 chip
Signed-off-by Edwin Chiu <[email protected]>---
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 Createcpuidle-sunplus.cinarch/arm/mach-sunplus/
for hook generic cpuidle-arm driverMAINTAINERS | 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.hMAINTAINERS +6
-0@@ -18252,6 +18252,12 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <[email protected]> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <[email protected]> M: Rich Felker <[email protected]>arch/arm/mach-sunplus/cpuidle-sunplus.c new file +16
-0@@ -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);
arch/arm/mach-sunplus/cpuidle-sunplus.c line 26
Quoted 10 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +10
-0+ +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.
arch/arm/mach-sunplus/cpuidle-sunplus.c line 30
Quoted 4 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +4
-0+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) ?
Signature · 2 lines
Regards, Sudeep
RE: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Quoted 104 lines
-----Original Message-----
From: Sudeep Holla <[email protected]>
Sent: Monday, February 21, 2022 6:52 PM
To: Edwin Chiu <[email protected]>
Cc: Edwin Chiu 邱垂峰 <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; Sudeep Holla <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 72 lines · Edwin Chiu
Create cpuidle driver for sunplus sp7021 chip
Signed-off-by Edwin Chiu <[email protected]>---
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 Createcpuidle-sunplus.cinarch/arm/mach-sunplus/
for hook generic cpuidle-arm driverMAINTAINERS | 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.hMAINTAINERS +6
-0@@ -18252,6 +18252,12 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <[email protected]> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <[email protected]> M: Rich Felker <[email protected]>arch/arm/mach-sunplus/cpuidle-sunplus.c new file +15
-0@@ -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 ); +arch/arm/mach-sunplus/cpuidle-sunplus.c line 25
Quoted 10 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +10
-0+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.
arch/arm/mach-sunplus/cpuidle-sunplus.c line 30
Quoted 4 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +4
-0+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.
Quoted 6 lines
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
[email protected]
300 新竹科學園區創新一路19號
Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Quoted 4 lines · Edwin Chiu 邱垂峰
Quoted 4 lines
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.
Quoted 7 lines
Quoted 7 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c line 30
Quoted 4 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +4
-0+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.
Quoted 6 lines
Quoted 3 lines
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.
Signature · 2 lines
Regards, Sudeep
Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
In reply to Edwin ChiuQuoted 77 lines · Edwin Chiu
Create cpuidle driver for sunplus sp7021 chip
Signed-off-by Edwin Chiu <[email protected]>---
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 Createcpuidle-sunplus.cinarch/arm/mach-sunplus/
for hook generic cpuidle-arm driverMAINTAINERS | 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.hMAINTAINERS +6
-0@@ -18252,6 +18252,12 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <[email protected]> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <[email protected]> M: Rich Felker <[email protected]>arch/arm/mach-sunplus/cpuidle-sunplus.c new file +20
-0@@ -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]();
arch/arm/mach-sunplus/cpuidle-sunplus.c line 30
Quoted 10 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +10
-0+} +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
RE: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Quoted 102 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, February 22, 2022 12:48 AM
To: Edwin Chiu <[email protected]>; Edwin Chiu 邱垂峰 <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 76 lines · Edwin Chiu
Create cpuidle driver for sunplus sp7021 chip
Signed-off-by Edwin Chiu <[email protected]>---
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 Createcpuidle-sunplus.cinarch/arm/mach-sunplus/
for hook generic cpuidle-arm driverMAINTAINERS | 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.hMAINTAINERS +6
-0@@ -18252,6 +18252,12 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <[email protected]> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <[email protected]> M: Rich Felker <[email protected]>arch/arm/mach-sunplus/cpuidle-sunplus.c new file +19
-0@@ -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]();arch/arm/mach-sunplus/cpuidle-sunplus.c line 29
Quoted 10 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +10
-0+} +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.
Quoted 2 lines
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
[email protected]
300 新竹科學園區創新一路19號
Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Quoted 106 lines · Edwin Chiu 邱垂峰
Quoted 102 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, February 22, 2022 12:48 AM
To: Edwin Chiu <[email protected]>; Edwin Chiu 邱垂峰 <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 76 lines · Edwin Chiu
Create cpuidle driver for sunplus sp7021 chip
Signed-off-by Edwin Chiu <[email protected]>---
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 Createcpuidle-sunplus.cinarch/arm/mach-sunplus/
for hook generic cpuidle-arm driverMAINTAINERS | 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.hMAINTAINERS +6
-0@@ -18252,6 +18252,12 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <[email protected]> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <[email protected]> M: Rich Felker <[email protected]>arch/arm/mach-sunplus/cpuidle-sunplus.c new file +19
-0@@ -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]();arch/arm/mach-sunplus/cpuidle-sunplus.c line 29
Quoted 10 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +10
-0+} +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.
Quoted 2 lines
Best regards,
Krzysztof
Quoted 7 lines
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
RE: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Quoted 126 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, March 1, 2022 7:34 PM
To: Edwin Chiu 邱垂峰 <[email protected]>; Edwin Chiu <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 95 lines · Edwin Chiu 邱垂峰
Quoted 95 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, February 22, 2022 12:48 AM
To: Edwin Chiu <[email protected]>; Edwin Chiu 邱垂峰 <[email protected]>;
[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 81 lines · Edwin Chiu
Create cpuidle driver for sunplus sp7021 chip
Signed-off-by Edwin Chiu <[email protected]>---
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 Createcpuidle-sunplus.cinarch/arm/mach-sunplus/
for hook generic cpuidle-arm driverMAINTAINERS | 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.hMAINTAINERS +6
-0@@ -18252,6 +18252,12 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <[email protected]> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <[email protected]> M: Rich Felker <[email protected]>arch/arm/mach-sunplus/cpuidle-sunplus.c new file +24
-0@@ -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 ;Quoted 8 lines
Quoted 8 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c line 30
Quoted 5 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +5
-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 fewother architectures.
Quoted 11 lines
Quoted 4 lines
Best regards,
KrzysztofThe 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.
Quoted 8 lines
Quoted 2 lines
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
am3352amx3_idle_init(~) assign idle_states[i].wfi_flags = states[i].wfi_flags; amx3_idle_enter(~) call idle_fn(idle_state->wfi_flags)
sunplus-sp7021sp7021_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
[email protected]
300 新竹科學園區創新一路19號
Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Quoted 145 lines · Edwin Chiu 邱垂峰
Quoted 126 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, March 1, 2022 7:34 PM
To: Edwin Chiu 邱垂峰 <[email protected]>; Edwin Chiu <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 95 lines · Edwin Chiu 邱垂峰
Quoted 95 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, February 22, 2022 12:48 AM
To: Edwin Chiu <[email protected]>; Edwin Chiu 邱垂峰 <[email protected]>;
[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 81 lines · Edwin Chiu
Create cpuidle driver for sunplus sp7021 chip
Signed-off-by Edwin Chiu <[email protected]>---
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 Createcpuidle-sunplus.cinarch/arm/mach-sunplus/
for hook generic cpuidle-arm driverMAINTAINERS | 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.hMAINTAINERS +6
-0@@ -18252,6 +18252,12 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <[email protected]> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <[email protected]> M: Rich Felker <[email protected]>arch/arm/mach-sunplus/cpuidle-sunplus.c new file +24
-0@@ -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 ;Quoted 8 lines
Quoted 8 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c line 30
Quoted 5 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +5
-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 fewother architectures.
Quoted 11 lines
Quoted 4 lines
Best regards,
KrzysztofThe 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.Quoted 8 lines
Quoted 2 lines
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,
KrzysztofThe am3352 cpuidle code structure is very similar to ours.
Used enable-method = "ti,am3352" and compatible = "arm,idle-state" inam33xx.dtsiUsedCPUIDLE_METHOD_OF_DECLARE(pm33xx_idle, "ti,am3352", &amx3_cpuidle_ops)inpm33xx-core.cThe difference are
am3352amx3_idle_init(~)assign idle_states[i].wfi_flags = states[i].wfi_flags;amx3_idle_enter(~)callidle_fn(idle_state->wfi_flags)
Quoted 7 lines
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_fnfunction 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
Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Quoted 150 lines · Krzysztof Kozlowski
Quoted 149 lines · Edwin Chiu 邱垂峰
Quoted 126 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, March 1, 2022 7:34 PM
To: Edwin Chiu 邱垂峰 <[email protected]>; Edwin Chiu <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 95 lines · Edwin Chiu 邱垂峰
Quoted 95 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, February 22, 2022 12:48 AM
To: Edwin Chiu <[email protected]>; Edwin Chiu 邱垂峰 <[email protected]>;
[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 81 lines · Edwin Chiu
Create cpuidle driver for sunplus sp7021 chip
Signed-off-by Edwin Chiu <[email protected]>---
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 Createcpuidle-sunplus.cinarch/arm/mach-sunplus/
for hook generic cpuidle-arm driverMAINTAINERS | 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.hMAINTAINERS +6
-0@@ -18252,6 +18252,12 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <[email protected]> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <[email protected]> M: Rich Felker <[email protected]>arch/arm/mach-sunplus/cpuidle-sunplus.c new file +24
-0@@ -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 ;Quoted 8 lines
Quoted 8 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c line 30
Quoted 5 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +5
-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 fewother architectures.
Quoted 11 lines
Quoted 4 lines
Best regards,
KrzysztofThe 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.Quoted 8 lines
Quoted 2 lines
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,
KrzysztofThe am3352 cpuidle code structure is very similar to ours.
Used enable-method = "ti,am3352" and compatible = "arm,idle-state" inam33xx.dtsiUsedCPUIDLE_METHOD_OF_DECLARE(pm33xx_idle, "ti,am3352", &amx3_cpuidle_ops)inpm33xx-core.cThe difference are
am3352amx3_idle_init(~)assign idle_states[i].wfi_flags = states[i].wfi_flags;amx3_idle_enter(~)callidle_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]();
Quoted 9 lines
Quoted 3 lines
I don't think am3352 cpuidle code architecture simpler than ours. The
idle_fnfunction 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
RE: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Quoted 188 lines
-----Original Message-----
From: Sudeep Holla <[email protected]>
Sent: Thursday, March 3, 2022 6:03 PM
To: Krzysztof Kozlowski <[email protected]>
Cc: Edwin Chiu 邱垂峰 <[email protected]>; Edwin Chiu <[email protected]>; Sudeep Holla <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 158 lines · Krzysztof Kozlowski
Quoted 157 lines · Edwin Chiu 邱垂峰
Quoted 131 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, March 1, 2022 7:34 PM
To: Edwin Chiu 邱垂峰 <[email protected]>; Edwin Chiu <[email protected]>;
[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 97 lines · Edwin Chiu 邱垂峰
Quoted 97 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, February 22, 2022 12:48 AM
To: Edwin Chiu <[email protected]>; Edwin Chiu 邱垂峰 <[email protected]>;
[email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021Quoted 83 lines · Edwin Chiu
Create cpuidle driver for sunplus sp7021 chip
Signed-off-by Edwin Chiu <[email protected]>---
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 Createcpuidle-sunplus.cinarch/arm/mach-sunplus/
for hook generic cpuidle-arm driverMAINTAINERS | 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.hMAINTAINERS index e0dca8f..5c96428 renamed from MAINTAINERS +0
-0100644
MAINTAINERS +6
-0@@ -18252,6 +18252,12 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <[email protected]> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <[email protected]> M: Rich Felker <[email protected]>arch/arm/mach-sunplus/cpuidle-sunplus.c new file +24
-0@@ -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 ; +}Quoted 8 lines
Quoted 8 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c line 28
Quoted 4 lines
arch/arm/mach-sunplus/cpuidle-sunplus.c new file +4
-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.
Quoted 11 lines
Quoted 4 lines
Best regards,
KrzysztofThe 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.Quoted 8 lines
Quoted 2 lines
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,
KrzysztofThe am3352 cpuidle code structure is very similar to ours. Used enable-method = "ti,am3352" and compatible = "arm,idle-state" in
am33xx.dtsiUsedCPUIDLE_METHOD_OF_DECLARE(pm33xx_idle, "ti,am3352", &amx3_cpuidle_ops)inpm33xx-core.cThe difference are
am3352amx3_idle_init(~)assign idle_states[i].wfi_flags = states[i].wfi_flags;amx3_idle_enter(~)callidle_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]();Quoted 9 lines
Quoted 3 lines
I don't think am3352 cpuidle code architecture simpler than ours. The
idle_fnfunction 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.
Quoted 3 lines
--
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
[email protected]
300 新竹科學園區創新一路19號
Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021
Quoted 3 lines · Edwin Chiu 邱垂峰
Thanks your advice.
Look like key point still only WFI function when cpuidle.
Indeed.
Quoted 1 line
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.
Quoted 1 line
It need enable-method code to assign
cpuidle_opsfunctions.
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.
Quoted 5 lines
"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.ccode 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 ?
Quoted 2 lines
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.
Quoted 2 lines
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.
Signature · 5 lines
Regards, Sudeep [1] Documentation/devicetree/bindings/arm/cpu-enable-method/ [2] Documentation/devicetree/bindings/arm/cpus.yaml