* [PATCH v5] Add cpuidle driver for Sunplus SP7021 @ 2022-02-21 7:26 Edwin Chiu 2022-02-21 7:26 ` [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 Edwin Chiu 0 siblings, 1 reply; 13+ messages in thread From: Edwin Chiu @ 2022-02-21 7:26 UTC (permalink / raw) To: edwin.chiu, robh+dt, devicetree, linux-kernel, rafael, daniel.lezcano, linux-pm Cc: Edwin Chiu 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 -- 2.7.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-02-21 7:26 [PATCH v5] Add cpuidle driver for Sunplus SP7021 Edwin Chiu @ 2022-02-21 7:26 ` Edwin Chiu 2022-02-21 10:51 ` Sudeep Holla 2022-02-21 16:47 ` Krzysztof Kozlowski 0 siblings, 2 replies; 13+ messages in thread From: Edwin Chiu @ 2022-02-21 7:26 UTC (permalink / raw) To: edwin.chiu, robh+dt, devicetree, linux-kernel, rafael, daniel.lezcano, linux-pm Cc: Edwin Chiu Create cpuidle driver for sunplus sp7021 chip Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com> --- Changes in v3 - Rearrangement #include sequence - Change remark style to /*~*/ - Align author email address to same as sob - Optimal code Changes in v4 - According Rob Herringrobh's comment There is no need for this binding. Just wanting a different driver is not a reason for a duplicate schema. So remove yaml file and submit driver again. Changes in v5 - According Krzysztof's comment You either use appropriate compatible in DT or add your compatible to cpuidle-arm. Even if this did not work, then the solution is to use common parts, not to duplicate entire driver. According Sudeep's comment In short NACK for any dedicated driver for this platform, use the generic cpuidle-arm driver with appropriate platform hooks Create cpuidle-sunplus.c in arch/arm/mach-sunplus/ for hook generic cpuidle-arm driver MAINTAINERS | 6 ++ arch/arm/mach-sunplus/cpuidle-sunplus.c | 88 +++++++++++++++++ include/linux/platform_data/cpuidle-sunplus.h | 12 ++++ 3 files changed, 106 insertions(+) create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c create mode 100644 include/linux/platform_data/cpuidle-sunplus.h diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18252,6 +18252,12 @@ L: netdev@vger.kernel.org S: Maintained F: drivers/net/ethernet/dlink/sundance.c +SUNPLUS CPUIDLE DRIVER +M: Edwin Chiu <edwinchiu0505tw@gmail.com> +S: Maintained +F: arch/arm/mach-sunplus/cpuidle-sunplus.c +F: include/linux/platform_data/cpuidle-sunplus.h + SUPERH M: Yoshinori Sato <ysato@users.sourceforge.jp> M: Rich Felker <dalias@libc.org> diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c b/arch/arm/mach-sunplus/cpuidle-sunplus.c new file mode 100644 index 0000000..e9d9738 --- /dev/null +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * SP7021 cpu idle Driver. + * Copyright (C) Sunplus Tech / Tibbo Tech. + */ +#define pr_fmt(fmt) "CPUidle arm: " fmt + +#include <linux/cpuidle.h> +#include <linux/of_device.h> +#include <linux/platform_data/cpuidle-sunplus.h> + +#include <asm/cpuidle.h> + +typedef int (*idle_fn)(void); + +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops); + +static int sp7021_cpuidle_enter(unsigned long index) +{ + return __this_cpu_read(sp7021_idle_ops)[index](); +} +static int sp7021_cpu_spc(void) +{ + cpu_v7_do_idle(); /* idle to WFI */ + return 0; +} +static const struct of_device_id sp7021_idle_state_match[] = { + { .compatible = "arm,idle-state", .data = sp7021_cpu_spc }, + { }, +}; +static int sp7021_cpuidle_init(struct device_node *cpu_node, int cpu) +{ + const struct of_device_id *match_id; + struct device_node *state_node; + int i; + int state_count = 1; + idle_fn idle_fns[CPUIDLE_STATE_MAX]; + idle_fn *fns; + + for (i = 0; ; i++) { + state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i); + if (!state_node) + break; + + if (!of_device_is_available(state_node)) + continue; + + if (i == CPUIDLE_STATE_MAX) { + pr_warn("%s: cpuidle states reached max possible\n", + __func__); + break; + } + + match_id = of_match_node(sp7021_idle_state_match, state_node); + if (!match_id) + return -ENODEV; + + idle_fns[state_count] = match_id->data; + + state_count++; + } + + if (state_count == 1) + goto check_sp; + + fns = devm_kcalloc(get_cpu_device(cpu), state_count, sizeof(*fns), + GFP_KERNEL); + if (!fns) + return -ENOMEM; + + for (i = 1; i < state_count; i++) + fns[i] = idle_fns[i]; + + per_cpu(sp7021_idle_ops, cpu) = fns; + +check_sp: + return 0; +} +static const struct cpuidle_ops sp7021_cpuidle_ops __initconst = { + .suspend = sp7021_cpuidle_enter, + .init = sp7021_cpuidle_init, +}; +CPUIDLE_METHOD_OF_DECLARE(sc_smp, "sunplus,sc-smp", &sp7021_cpuidle_ops); + +MODULE_AUTHOR("Edwin Chiu <edwinchiu0505tw@gmail.com>"); +MODULE_DESCRIPTION("Sunplus sp7021 cpuidle driver"); +MODULE_LICENSE("GPL"); + diff --git a/include/linux/platform_data/cpuidle-sunplus.h b/include/linux/platform_data/cpuidle-sunplus.h new file mode 100644 index 0000000..12e98e5 --- /dev/null +++ b/include/linux/platform_data/cpuidle-sunplus.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * SP7021 cpu idle Driver. + * Copyright (C) Sunplus Tech / Tibbo Tech. + */ + +#ifndef __CPUIDLE_SP7021_H +#define __CPUIDLE_SP7021_H + +extern int cpu_v7_do_idle(void); + +#endif -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-02-21 7:26 ` [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 Edwin Chiu @ 2022-02-21 10:51 ` Sudeep Holla 2022-03-01 9:18 ` Edwin Chiu 邱垂峰 2022-02-21 16:47 ` Krzysztof Kozlowski 1 sibling, 1 reply; 13+ messages in thread From: Sudeep Holla @ 2022-02-21 10:51 UTC (permalink / raw) To: Edwin Chiu Cc: edwin.chiu, robh+dt, devicetree, linux-kernel, rafael, Sudeep Holla, daniel.lezcano, linux-pm On Mon, Feb 21, 2022 at 03:26:18PM +0800, Edwin Chiu wrote: > Create cpuidle driver for sunplus sp7021 chip > > Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com> > --- > Changes in v3 > - Rearrangement #include sequence > - Change remark style to /*~*/ > - Align author email address to same as sob > - Optimal code > Changes in v4 > - According Rob Herringrobh's comment > There is no need for this binding. > Just wanting a different driver is not a reason > for a duplicate schema. > So remove yaml file and submit driver again. > Changes in v5 > - According Krzysztof's comment > You either use appropriate compatible in DT > or add your compatible to cpuidle-arm. > Even if this did not work, then the solution is to > use common parts, not to duplicate entire driver. > According Sudeep's comment > In short NACK for any dedicated driver for this platform, > use the generic cpuidle-arm driver with appropriate platform hooks > Create cpuidle-sunplus.c in arch/arm/mach-sunplus/ > for hook generic cpuidle-arm driver > > MAINTAINERS | 6 ++ > arch/arm/mach-sunplus/cpuidle-sunplus.c | 88 +++++++++++++++++ > include/linux/platform_data/cpuidle-sunplus.h | 12 ++++ > 3 files changed, 106 insertions(+) > create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c > create mode 100644 include/linux/platform_data/cpuidle-sunplus.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index e0dca8f..5c96428 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -18252,6 +18252,12 @@ L: netdev@vger.kernel.org > S: Maintained > F: drivers/net/ethernet/dlink/sundance.c > > +SUNPLUS CPUIDLE DRIVER > +M: Edwin Chiu <edwinchiu0505tw@gmail.com> > +S: Maintained > +F: arch/arm/mach-sunplus/cpuidle-sunplus.c > +F: include/linux/platform_data/cpuidle-sunplus.h > + > SUPERH > M: Yoshinori Sato <ysato@users.sourceforge.jp> > M: Rich Felker <dalias@libc.org> > diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c b/arch/arm/mach-sunplus/cpuidle-sunplus.c > new file mode 100644 > index 0000000..e9d9738 > --- /dev/null > +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c > @@ -0,0 +1,88 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * SP7021 cpu idle Driver. > + * Copyright (C) Sunplus Tech / Tibbo Tech. > + */ > +#define pr_fmt(fmt) "CPUidle arm: " fmt > + > +#include <linux/cpuidle.h> > +#include <linux/of_device.h> > +#include <linux/platform_data/cpuidle-sunplus.h> > + > +#include <asm/cpuidle.h> > + > +typedef int (*idle_fn)(void); > + > +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops); > + > +static int sp7021_cpuidle_enter(unsigned long index) > +{ > + return __this_cpu_read(sp7021_idle_ops)[index](); > +} > +static int sp7021_cpu_spc(void) > +{ > + cpu_v7_do_idle(); /* idle to WFI */ > + return 0; > +} You really don't need a cpuidle driver to just WFI for any states. Add the driver when you have something non WFI in the suspend function. > +static const struct of_device_id sp7021_idle_state_match[] = { > + { .compatible = "arm,idle-state", .data = sp7021_cpu_spc }, > + { }, > +}; This is better than adding new driver like you did in previous version. I did a quick check but couldn't figure out. How do cpus get switched ON or OFF on this platform(for example during CPU hotplug) ? -- Regards, Sudeep ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-02-21 10:51 ` Sudeep Holla @ 2022-03-01 9:18 ` Edwin Chiu 邱垂峰 2022-03-01 12:23 ` Sudeep Holla 0 siblings, 1 reply; 13+ messages in thread From: Edwin Chiu 邱垂峰 @ 2022-03-01 9:18 UTC (permalink / raw) To: Sudeep Holla, Edwin Chiu Cc: robh+dt@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, daniel.lezcano@linaro.org, linux-pm@vger.kernel.org > -----Original Message----- > From: Sudeep Holla <sudeep.holla@arm.com> > Sent: Monday, February 21, 2022 6:52 PM > To: Edwin Chiu <edwinchiu0505tw@gmail.com> > Cc: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; robh+dt@kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; rafael@kernel.org; Sudeep Holla <sudeep.holla@arm.com>; > daniel.lezcano@linaro.org; linux-pm@vger.kernel.org > Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 > > On Mon, Feb 21, 2022 at 03:26:18PM +0800, Edwin Chiu wrote: > > Create cpuidle driver for sunplus sp7021 chip > > > > Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com> > > --- > > Changes in v3 > > - Rearrangement #include sequence > > - Change remark style to /*~*/ > > - Align author email address to same as sob > > - Optimal code > > Changes in v4 > > - According Rob Herringrobh's comment > > There is no need for this binding. > > Just wanting a different driver is not a reason > > for a duplicate schema. > > So remove yaml file and submit driver again. > > Changes in v5 > > - According Krzysztof's comment > > You either use appropriate compatible in DT > > or add your compatible to cpuidle-arm. > > Even if this did not work, then the solution is to > > use common parts, not to duplicate entire driver. > > According Sudeep's comment > > In short NACK for any dedicated driver for this platform, > > use the generic cpuidle-arm driver with appropriate platform hooks > > Create cpuidle-sunplus.c in arch/arm/mach-sunplus/ > > for hook generic cpuidle-arm driver > > > > MAINTAINERS | 6 ++ > > arch/arm/mach-sunplus/cpuidle-sunplus.c | 88 +++++++++++++++++ > > include/linux/platform_data/cpuidle-sunplus.h | 12 ++++ > > 3 files changed, 106 insertions(+) > > create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c > > create mode 100644 include/linux/platform_data/cpuidle-sunplus.h > > > > diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -18252,6 +18252,12 @@ L: netdev@vger.kernel.org > > S: Maintained > > F: drivers/net/ethernet/dlink/sundance.c > > > > +SUNPLUS CPUIDLE DRIVER > > +M: Edwin Chiu <edwinchiu0505tw@gmail.com> > > +S: Maintained > > +F: arch/arm/mach-sunplus/cpuidle-sunplus.c > > +F: include/linux/platform_data/cpuidle-sunplus.h > > + > > SUPERH > > M: Yoshinori Sato <ysato@users.sourceforge.jp> > > M: Rich Felker <dalias@libc.org> > > diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c > > b/arch/arm/mach-sunplus/cpuidle-sunplus.c > > new file mode 100644 > > index 0000000..e9d9738 > > --- /dev/null > > +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c > > @@ -0,0 +1,88 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * SP7021 cpu idle Driver. > > + * Copyright (C) Sunplus Tech / Tibbo Tech. > > + */ > > +#define pr_fmt(fmt) "CPUidle arm: " fmt > > + > > +#include <linux/cpuidle.h> > > +#include <linux/of_device.h> > > +#include <linux/platform_data/cpuidle-sunplus.h> > > + > > +#include <asm/cpuidle.h> > > + > > +typedef int (*idle_fn)(void); > > + > > +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops); > > + > > +static int sp7021_cpuidle_enter(unsigned long index) { > > + return __this_cpu_read(sp7021_idle_ops)[index](); > > +} > > +static int sp7021_cpu_spc(void) > > +{ > > + cpu_v7_do_idle(); /* idle to WFI */ > > + return 0; > > +} > > You really don't need a cpuidle driver to just WFI for any states. > Add the driver when you have something non WFI in the suspend function. > > > +static const struct of_device_id sp7021_idle_state_match[] = { > > + { .compatible = "arm,idle-state", .data = sp7021_cpu_spc }, > > + { }, > > +}; > > This is better than adding new driver like you did in previous version. > > I did a quick check but couldn't figure out. How do cpus get switched ON or OFF on this platform(for > example during CPU hotplug) ? > > -- > Regards, > Sudeep In this patch, I just want to submit cpuidle function. So there have no cpu hotplug function now. 邱垂峰 EdwinChiu 智能運算專案 T: +886-3-5786005 ext.2590 edwin.chiu@sunplus.com 300 新竹科學園區創新一路19號 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-03-01 9:18 ` Edwin Chiu 邱垂峰 @ 2022-03-01 12:23 ` Sudeep Holla 0 siblings, 0 replies; 13+ messages in thread From: Sudeep Holla @ 2022-03-01 12:23 UTC (permalink / raw) To: Edwin Chiu 邱垂峰 Cc: Edwin Chiu, Sudeep Holla, robh+dt@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, daniel.lezcano@linaro.org, linux-pm@vger.kernel.org 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. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-02-21 7:26 ` [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 Edwin Chiu 2022-02-21 10:51 ` Sudeep Holla @ 2022-02-21 16:47 ` Krzysztof Kozlowski 2022-03-01 9:30 ` Edwin Chiu 邱垂峰 1 sibling, 1 reply; 13+ messages in thread From: Krzysztof Kozlowski @ 2022-02-21 16:47 UTC (permalink / raw) To: Edwin Chiu, edwin.chiu, robh+dt, devicetree, linux-kernel, rafael, daniel.lezcano, linux-pm On 21/02/2022 08:26, Edwin Chiu wrote: > Create cpuidle driver for sunplus sp7021 chip > > Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com> > --- > Changes in v3 > - Rearrangement #include sequence > - Change remark style to /*~*/ > - Align author email address to same as sob > - Optimal code > Changes in v4 > - According Rob Herringrobh's comment > There is no need for this binding. > Just wanting a different driver is not a reason > for a duplicate schema. > So remove yaml file and submit driver again. > Changes in v5 > - According Krzysztof's comment > You either use appropriate compatible in DT > or add your compatible to cpuidle-arm. > Even if this did not work, then the solution is to > use common parts, not to duplicate entire driver. > According Sudeep's comment > In short NACK for any dedicated driver for this platform, > use the generic cpuidle-arm driver with appropriate platform hooks > Create cpuidle-sunplus.c in arch/arm/mach-sunplus/ > for hook generic cpuidle-arm driver > > MAINTAINERS | 6 ++ > arch/arm/mach-sunplus/cpuidle-sunplus.c | 88 +++++++++++++++++ > include/linux/platform_data/cpuidle-sunplus.h | 12 ++++ > 3 files changed, 106 insertions(+) > create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c > create mode 100644 include/linux/platform_data/cpuidle-sunplus.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index e0dca8f..5c96428 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -18252,6 +18252,12 @@ L: netdev@vger.kernel.org > S: Maintained > F: drivers/net/ethernet/dlink/sundance.c > > +SUNPLUS CPUIDLE DRIVER > +M: Edwin Chiu <edwinchiu0505tw@gmail.com> > +S: Maintained > +F: arch/arm/mach-sunplus/cpuidle-sunplus.c > +F: include/linux/platform_data/cpuidle-sunplus.h > + > SUPERH > M: Yoshinori Sato <ysato@users.sourceforge.jp> > M: Rich Felker <dalias@libc.org> > diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c b/arch/arm/mach-sunplus/cpuidle-sunplus.c > new file mode 100644 > index 0000000..e9d9738 > --- /dev/null > +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c > @@ -0,0 +1,88 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * SP7021 cpu idle Driver. > + * Copyright (C) Sunplus Tech / Tibbo Tech. > + */ > +#define pr_fmt(fmt) "CPUidle arm: " fmt > + > +#include <linux/cpuidle.h> > +#include <linux/of_device.h> > +#include <linux/platform_data/cpuidle-sunplus.h> > + > +#include <asm/cpuidle.h> > + > +typedef int (*idle_fn)(void); > + > +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops); > + > +static int sp7021_cpuidle_enter(unsigned long index) > +{ > + return __this_cpu_read(sp7021_idle_ops)[index](); > +} > +static int sp7021_cpu_spc(void) > +{ > + cpu_v7_do_idle(); /* idle to WFI */ > + return 0; > +} > +static const struct of_device_id sp7021_idle_state_match[] = { > + { .compatible = "arm,idle-state", .data = sp7021_cpu_spc }, > + { }, > +}; This is confusing. You want to have two drivers to bind to the same compatible? As I wrote in the previous messages, you should simply use arm,idle-state just like few other architectures. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-02-21 16:47 ` Krzysztof Kozlowski @ 2022-03-01 9:30 ` Edwin Chiu 邱垂峰 2022-03-01 11:33 ` Krzysztof Kozlowski 0 siblings, 1 reply; 13+ messages in thread From: Edwin Chiu 邱垂峰 @ 2022-03-01 9:30 UTC (permalink / raw) To: Krzysztof Kozlowski, Edwin Chiu, robh+dt@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, daniel.lezcano@linaro.org, linux-pm@vger.kernel.org > -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: Tuesday, February 22, 2022 12:48 AM > To: Edwin Chiu <edwinchiu0505tw@gmail.com>; Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; > robh+dt@kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org; > daniel.lezcano@linaro.org; linux-pm@vger.kernel.org > Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 > > On 21/02/2022 08:26, Edwin Chiu wrote: > > Create cpuidle driver for sunplus sp7021 chip > > > > Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com> > > --- > > Changes in v3 > > - Rearrangement #include sequence > > - Change remark style to /*~*/ > > - Align author email address to same as sob > > - Optimal code > > Changes in v4 > > - According Rob Herringrobh's comment > > There is no need for this binding. > > Just wanting a different driver is not a reason > > for a duplicate schema. > > So remove yaml file and submit driver again. > > Changes in v5 > > - According Krzysztof's comment > > You either use appropriate compatible in DT > > or add your compatible to cpuidle-arm. > > Even if this did not work, then the solution is to > > use common parts, not to duplicate entire driver. > > According Sudeep's comment > > In short NACK for any dedicated driver for this platform, > > use the generic cpuidle-arm driver with appropriate platform hooks > > Create cpuidle-sunplus.c in arch/arm/mach-sunplus/ > > for hook generic cpuidle-arm driver > > > > MAINTAINERS | 6 ++ > > arch/arm/mach-sunplus/cpuidle-sunplus.c | 88 +++++++++++++++++ > > include/linux/platform_data/cpuidle-sunplus.h | 12 ++++ > > 3 files changed, 106 insertions(+) > > create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c > > create mode 100644 include/linux/platform_data/cpuidle-sunplus.h > > > > diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -18252,6 +18252,12 @@ L: netdev@vger.kernel.org > > S: Maintained > > F: drivers/net/ethernet/dlink/sundance.c > > > > +SUNPLUS CPUIDLE DRIVER > > +M: Edwin Chiu <edwinchiu0505tw@gmail.com> > > +S: Maintained > > +F: arch/arm/mach-sunplus/cpuidle-sunplus.c > > +F: include/linux/platform_data/cpuidle-sunplus.h > > + > > SUPERH > > M: Yoshinori Sato <ysato@users.sourceforge.jp> > > M: Rich Felker <dalias@libc.org> > > diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c > > b/arch/arm/mach-sunplus/cpuidle-sunplus.c > > new file mode 100644 > > index 0000000..e9d9738 > > --- /dev/null > > +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c > > @@ -0,0 +1,88 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * SP7021 cpu idle Driver. > > + * Copyright (C) Sunplus Tech / Tibbo Tech. > > + */ > > +#define pr_fmt(fmt) "CPUidle arm: " fmt > > + > > +#include <linux/cpuidle.h> > > +#include <linux/of_device.h> > > +#include <linux/platform_data/cpuidle-sunplus.h> > > + > > +#include <asm/cpuidle.h> > > + > > +typedef int (*idle_fn)(void); > > + > > +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops); > > + > > +static int sp7021_cpuidle_enter(unsigned long index) { > > + return __this_cpu_read(sp7021_idle_ops)[index](); > > +} > > +static int sp7021_cpu_spc(void) > > +{ > > + cpu_v7_do_idle(); /* idle to WFI */ > > + return 0; > > +} > > +static const struct of_device_id sp7021_idle_state_match[] = { > > + { .compatible = "arm,idle-state", .data = sp7021_cpu_spc }, > > + { }, > > +}; > > This is confusing. You want to have two drivers to bind to the same compatible? As I wrote in the > previous messages, you should simply use arm,idle-state just like few other architectures. > > > Best regards, > Krzysztof The patch v5 implemented according your comment. Used common part of arm,idle-state. Create new enable-method for cpuidle.ops function. It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible. What do you mean " simply use arm,idle-state just like few other architectures "? 邱垂峰 EdwinChiu 智能運算專案 T: +886-3-5786005 ext.2590 edwin.chiu@sunplus.com 300 新竹科學園區創新一路19號 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-03-01 9:30 ` Edwin Chiu 邱垂峰 @ 2022-03-01 11:33 ` Krzysztof Kozlowski 2022-03-03 9:01 ` Edwin Chiu 邱垂峰 0 siblings, 1 reply; 13+ messages in thread From: Krzysztof Kozlowski @ 2022-03-01 11:33 UTC (permalink / raw) To: Edwin Chiu 邱垂峰, Edwin Chiu, robh+dt@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, daniel.lezcano@linaro.org, linux-pm@vger.kernel.org On 01/03/2022 10:30, Edwin Chiu 邱垂峰 wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzk@kernel.org> >> Sent: Tuesday, February 22, 2022 12:48 AM >> To: Edwin Chiu <edwinchiu0505tw@gmail.com>; Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; >> robh+dt@kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org; >> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org >> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 >> >> On 21/02/2022 08:26, Edwin Chiu wrote: >>> Create cpuidle driver for sunplus sp7021 chip >>> >>> Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com> >>> --- >>> Changes in v3 >>> - Rearrangement #include sequence >>> - Change remark style to /*~*/ >>> - Align author email address to same as sob >>> - Optimal code >>> Changes in v4 >>> - According Rob Herringrobh's comment >>> There is no need for this binding. >>> Just wanting a different driver is not a reason >>> for a duplicate schema. >>> So remove yaml file and submit driver again. >>> Changes in v5 >>> - According Krzysztof's comment >>> You either use appropriate compatible in DT >>> or add your compatible to cpuidle-arm. >>> Even if this did not work, then the solution is to >>> use common parts, not to duplicate entire driver. >>> According Sudeep's comment >>> In short NACK for any dedicated driver for this platform, >>> use the generic cpuidle-arm driver with appropriate platform hooks >>> Create cpuidle-sunplus.c in arch/arm/mach-sunplus/ >>> for hook generic cpuidle-arm driver >>> >>> MAINTAINERS | 6 ++ >>> arch/arm/mach-sunplus/cpuidle-sunplus.c | 88 +++++++++++++++++ >>> include/linux/platform_data/cpuidle-sunplus.h | 12 ++++ >>> 3 files changed, 106 insertions(+) >>> create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c >>> create mode 100644 include/linux/platform_data/cpuidle-sunplus.h >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -18252,6 +18252,12 @@ L: netdev@vger.kernel.org >>> S: Maintained >>> F: drivers/net/ethernet/dlink/sundance.c >>> >>> +SUNPLUS CPUIDLE DRIVER >>> +M: Edwin Chiu <edwinchiu0505tw@gmail.com> >>> +S: Maintained >>> +F: arch/arm/mach-sunplus/cpuidle-sunplus.c >>> +F: include/linux/platform_data/cpuidle-sunplus.h >>> + >>> SUPERH >>> M: Yoshinori Sato <ysato@users.sourceforge.jp> >>> M: Rich Felker <dalias@libc.org> >>> diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c >>> b/arch/arm/mach-sunplus/cpuidle-sunplus.c >>> new file mode 100644 >>> index 0000000..e9d9738 >>> --- /dev/null >>> +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c >>> @@ -0,0 +1,88 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +/* >>> + * SP7021 cpu idle Driver. >>> + * Copyright (C) Sunplus Tech / Tibbo Tech. >>> + */ >>> +#define pr_fmt(fmt) "CPUidle arm: " fmt >>> + >>> +#include <linux/cpuidle.h> >>> +#include <linux/of_device.h> >>> +#include <linux/platform_data/cpuidle-sunplus.h> >>> + >>> +#include <asm/cpuidle.h> >>> + >>> +typedef int (*idle_fn)(void); >>> + >>> +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops); >>> + >>> +static int sp7021_cpuidle_enter(unsigned long index) { >>> + return __this_cpu_read(sp7021_idle_ops)[index](); >>> +} >>> +static int sp7021_cpu_spc(void) >>> +{ >>> + cpu_v7_do_idle(); /* idle to WFI */ >>> + return 0; >>> +} >>> +static const struct of_device_id sp7021_idle_state_match[] = { >>> + { .compatible = "arm,idle-state", .data = sp7021_cpu_spc }, >>> + { }, >>> +}; >> >> This is confusing. You want to have two drivers to bind to the same compatible? As I wrote in the >> previous messages, you should simply use arm,idle-state just like few other architectures. >> >> >> Best regards, >> Krzysztof > > > The patch v5 implemented according your comment. > Used common part of arm,idle-state. > Create new enable-method for cpuidle.ops function. > It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible. > > What do you mean " simply use arm,idle-state just like few other architectures "? > I mean, do it similarly (by using arm,idle-state and other related properties) to for example ti,am4372/ti,am3352. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-03-01 11:33 ` Krzysztof Kozlowski @ 2022-03-03 9:01 ` Edwin Chiu 邱垂峰 2022-03-03 9:34 ` Krzysztof Kozlowski 0 siblings, 1 reply; 13+ messages in thread From: Edwin Chiu 邱垂峰 @ 2022-03-03 9:01 UTC (permalink / raw) To: Krzysztof Kozlowski, Edwin Chiu, robh+dt@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, daniel.lezcano@linaro.org, linux-pm@vger.kernel.org > -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: Tuesday, March 1, 2022 7:34 PM > To: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; Edwin Chiu <edwinchiu0505tw@gmail.com>; > robh+dt@kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org; > daniel.lezcano@linaro.org; linux-pm@vger.kernel.org > Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 > > On 01/03/2022 10:30, Edwin Chiu 邱垂峰 wrote: > > > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzk@kernel.org> > >> Sent: Tuesday, February 22, 2022 12:48 AM > >> To: Edwin Chiu <edwinchiu0505tw@gmail.com>; Edwin Chiu 邱垂峰 > >> <edwin.chiu@sunplus.com>; > >> robh+dt@kernel.org; devicetree@vger.kernel.org; > >> robh+linux-kernel@vger.kernel.org; rafael@kernel.org; > >> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org > >> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for > >> sunplus sp7021 > >> > >> On 21/02/2022 08:26, Edwin Chiu wrote: > >>> Create cpuidle driver for sunplus sp7021 chip > >>> > >>> Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com> > >>> --- > >>> Changes in v3 > >>> - Rearrangement #include sequence > >>> - Change remark style to /*~*/ > >>> - Align author email address to same as sob > >>> - Optimal code > >>> Changes in v4 > >>> - According Rob Herringrobh's comment > >>> There is no need for this binding. > >>> Just wanting a different driver is not a reason > >>> for a duplicate schema. > >>> So remove yaml file and submit driver again. > >>> Changes in v5 > >>> - According Krzysztof's comment > >>> You either use appropriate compatible in DT > >>> or add your compatible to cpuidle-arm. > >>> Even if this did not work, then the solution is to > >>> use common parts, not to duplicate entire driver. > >>> According Sudeep's comment > >>> In short NACK for any dedicated driver for this platform, > >>> use the generic cpuidle-arm driver with appropriate platform hooks > >>> Create cpuidle-sunplus.c in arch/arm/mach-sunplus/ > >>> for hook generic cpuidle-arm driver > >>> > >>> MAINTAINERS | 6 ++ > >>> arch/arm/mach-sunplus/cpuidle-sunplus.c | 88 +++++++++++++++++ > >>> include/linux/platform_data/cpuidle-sunplus.h | 12 ++++ > >>> 3 files changed, 106 insertions(+) > >>> create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c > >>> create mode 100644 include/linux/platform_data/cpuidle-sunplus.h > >>> > >>> diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644 > >>> --- a/MAINTAINERS > >>> +++ b/MAINTAINERS > >>> @@ -18252,6 +18252,12 @@ L: netdev@vger.kernel.org > >>> S: Maintained > >>> F: drivers/net/ethernet/dlink/sundance.c > >>> > >>> +SUNPLUS CPUIDLE DRIVER > >>> +M: Edwin Chiu <edwinchiu0505tw@gmail.com> > >>> +S: Maintained > >>> +F: arch/arm/mach-sunplus/cpuidle-sunplus.c > >>> +F: include/linux/platform_data/cpuidle-sunplus.h > >>> + > >>> SUPERH > >>> M: Yoshinori Sato <ysato@users.sourceforge.jp> > >>> M: Rich Felker <dalias@libc.org> > >>> diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c > >>> b/arch/arm/mach-sunplus/cpuidle-sunplus.c > >>> new file mode 100644 > >>> index 0000000..e9d9738 > >>> --- /dev/null > >>> +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c > >>> @@ -0,0 +1,88 @@ > >>> +// SPDX-License-Identifier: GPL-2.0-only > >>> +/* > >>> + * SP7021 cpu idle Driver. > >>> + * Copyright (C) Sunplus Tech / Tibbo Tech. > >>> + */ > >>> +#define pr_fmt(fmt) "CPUidle arm: " fmt > >>> + > >>> +#include <linux/cpuidle.h> > >>> +#include <linux/of_device.h> > >>> +#include <linux/platform_data/cpuidle-sunplus.h> > >>> + > >>> +#include <asm/cpuidle.h> > >>> + > >>> +typedef int (*idle_fn)(void); > >>> + > >>> +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops); > >>> + > >>> +static int sp7021_cpuidle_enter(unsigned long index) { > >>> + return __this_cpu_read(sp7021_idle_ops)[index](); > >>> +} > >>> +static int sp7021_cpu_spc(void) > >>> +{ > >>> + cpu_v7_do_idle(); /* idle to WFI */ > >>> + return 0; > >>> +} > >>> +static const struct of_device_id sp7021_idle_state_match[] = { > >>> + { .compatible = "arm,idle-state", .data = sp7021_cpu_spc }, > >>> + { }, > >>> +}; > >> > >> This is confusing. You want to have two drivers to bind to the same > >> compatible? As I wrote in the previous messages, you should simply use arm,idle-state just like few > other architectures. > >> > >> > >> Best regards, > >> Krzysztof > > > > > > The patch v5 implemented according your comment. > > Used common part of arm,idle-state. > > Create new enable-method for cpuidle.ops function. > > It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible. > > > > What do you mean " simply use arm,idle-state just like few other architectures "? > > > > I mean, do it similarly (by using arm,idle-state and other related > properties) to for example ti,am4372/ti,am3352. > > Best regards, > Krzysztof The am3352 cpuidle code structure is very similar to ours. Used enable-method = "ti,am3352" and compatible = "arm,idle-state" in am33xx.dtsi Used CPUIDLE_METHOD_OF_DECLARE(pm33xx_idle, "ti,am3352", &amx3_cpuidle_ops) in pm33xx-core.c The difference are am3352 amx3_idle_init(~) assign idle_states[i].wfi_flags = states[i].wfi_flags; amx3_idle_enter(~) call idle_fn(idle_state->wfi_flags) sunplus-sp7021 sp7021_cpuidle_init(~) assign fns[i] = idle_fns[i]; sp7021_cpuidle_enter(~) call __this_cpu_read(sp7021_idle_ops)[index](); I don't think am3352 cpuidle code architecture simpler than ours. The idle_fn function need more complex method to be assign. How do you think? 邱垂峰 EdwinChiu 智能運算專案 T: +886-3-5786005 ext.2590 edwin.chiu@sunplus.com 300 新竹科學園區創新一路19號 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-03-03 9:01 ` Edwin Chiu 邱垂峰 @ 2022-03-03 9:34 ` Krzysztof Kozlowski 2022-03-03 10:02 ` Sudeep Holla 0 siblings, 1 reply; 13+ messages in thread From: Krzysztof Kozlowski @ 2022-03-03 9:34 UTC (permalink / raw) To: Edwin Chiu 邱垂峰, Edwin Chiu, robh+dt@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, daniel.lezcano@linaro.org, linux-pm@vger.kernel.org On 03/03/2022 10:01, Edwin Chiu 邱垂峰 wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzk@kernel.org> >> Sent: Tuesday, March 1, 2022 7:34 PM >> To: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; Edwin Chiu <edwinchiu0505tw@gmail.com>; >> robh+dt@kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org; >> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org >> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 >> >> On 01/03/2022 10:30, Edwin Chiu 邱垂峰 wrote: >>> >>> >>>> -----Original Message----- >>>> From: Krzysztof Kozlowski <krzk@kernel.org> >>>> Sent: Tuesday, February 22, 2022 12:48 AM >>>> To: Edwin Chiu <edwinchiu0505tw@gmail.com>; Edwin Chiu 邱垂峰 >>>> <edwin.chiu@sunplus.com>; >>>> robh+dt@kernel.org; devicetree@vger.kernel.org; >>>> robh+linux-kernel@vger.kernel.org; rafael@kernel.org; >>>> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org >>>> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for >>>> sunplus sp7021 >>>> >>>> On 21/02/2022 08:26, Edwin Chiu wrote: >>>>> Create cpuidle driver for sunplus sp7021 chip >>>>> >>>>> Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com> >>>>> --- >>>>> Changes in v3 >>>>> - Rearrangement #include sequence >>>>> - Change remark style to /*~*/ >>>>> - Align author email address to same as sob >>>>> - Optimal code >>>>> Changes in v4 >>>>> - According Rob Herringrobh's comment >>>>> There is no need for this binding. >>>>> Just wanting a different driver is not a reason >>>>> for a duplicate schema. >>>>> So remove yaml file and submit driver again. >>>>> Changes in v5 >>>>> - According Krzysztof's comment >>>>> You either use appropriate compatible in DT >>>>> or add your compatible to cpuidle-arm. >>>>> Even if this did not work, then the solution is to >>>>> use common parts, not to duplicate entire driver. >>>>> According Sudeep's comment >>>>> In short NACK for any dedicated driver for this platform, >>>>> use the generic cpuidle-arm driver with appropriate platform hooks >>>>> Create cpuidle-sunplus.c in arch/arm/mach-sunplus/ >>>>> for hook generic cpuidle-arm driver >>>>> >>>>> MAINTAINERS | 6 ++ >>>>> arch/arm/mach-sunplus/cpuidle-sunplus.c | 88 +++++++++++++++++ >>>>> include/linux/platform_data/cpuidle-sunplus.h | 12 ++++ >>>>> 3 files changed, 106 insertions(+) >>>>> create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c >>>>> create mode 100644 include/linux/platform_data/cpuidle-sunplus.h >>>>> >>>>> diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644 >>>>> --- a/MAINTAINERS >>>>> +++ b/MAINTAINERS >>>>> @@ -18252,6 +18252,12 @@ L: netdev@vger.kernel.org >>>>> S: Maintained >>>>> F: drivers/net/ethernet/dlink/sundance.c >>>>> >>>>> +SUNPLUS CPUIDLE DRIVER >>>>> +M: Edwin Chiu <edwinchiu0505tw@gmail.com> >>>>> +S: Maintained >>>>> +F: arch/arm/mach-sunplus/cpuidle-sunplus.c >>>>> +F: include/linux/platform_data/cpuidle-sunplus.h >>>>> + >>>>> SUPERH >>>>> M: Yoshinori Sato <ysato@users.sourceforge.jp> >>>>> M: Rich Felker <dalias@libc.org> >>>>> diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c >>>>> b/arch/arm/mach-sunplus/cpuidle-sunplus.c >>>>> new file mode 100644 >>>>> index 0000000..e9d9738 >>>>> --- /dev/null >>>>> +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c >>>>> @@ -0,0 +1,88 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0-only >>>>> +/* >>>>> + * SP7021 cpu idle Driver. >>>>> + * Copyright (C) Sunplus Tech / Tibbo Tech. >>>>> + */ >>>>> +#define pr_fmt(fmt) "CPUidle arm: " fmt >>>>> + >>>>> +#include <linux/cpuidle.h> >>>>> +#include <linux/of_device.h> >>>>> +#include <linux/platform_data/cpuidle-sunplus.h> >>>>> + >>>>> +#include <asm/cpuidle.h> >>>>> + >>>>> +typedef int (*idle_fn)(void); >>>>> + >>>>> +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops); >>>>> + >>>>> +static int sp7021_cpuidle_enter(unsigned long index) { >>>>> + return __this_cpu_read(sp7021_idle_ops)[index](); >>>>> +} >>>>> +static int sp7021_cpu_spc(void) >>>>> +{ >>>>> + cpu_v7_do_idle(); /* idle to WFI */ >>>>> + return 0; >>>>> +} >>>>> +static const struct of_device_id sp7021_idle_state_match[] = { >>>>> + { .compatible = "arm,idle-state", .data = sp7021_cpu_spc }, >>>>> + { }, >>>>> +}; >>>> >>>> This is confusing. You want to have two drivers to bind to the same >>>> compatible? As I wrote in the previous messages, you should simply use arm,idle-state just like few >> other architectures. >>>> >>>> >>>> Best regards, >>>> Krzysztof >>> >>> >>> The patch v5 implemented according your comment. >>> Used common part of arm,idle-state. >>> Create new enable-method for cpuidle.ops function. >>> It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible. >>> >>> What do you mean " simply use arm,idle-state just like few other architectures "? >>> >> >> I mean, do it similarly (by using arm,idle-state and other related >> properties) to for example ti,am4372/ti,am3352. >> >> Best regards, >> Krzysztof > > > The am3352 cpuidle code structure is very similar to ours. > Used enable-method = "ti,am3352" and compatible = "arm,idle-state" in am33xx.dtsi > Used CPUIDLE_METHOD_OF_DECLARE(pm33xx_idle, "ti,am3352", &amx3_cpuidle_ops) in pm33xx-core.c > > The difference are > am3352 > amx3_idle_init(~) assign idle_states[i].wfi_flags = states[i].wfi_flags; > amx3_idle_enter(~) call idle_fn(idle_state->wfi_flags) > > sunplus-sp7021 > sp7021_cpuidle_init(~) assign fns[i] = idle_fns[i]; > sp7021_cpuidle_enter(~) call __this_cpu_read(sp7021_idle_ops)[index](); > > I don't think am3352 cpuidle code architecture simpler than ours. > The idle_fn function need more complex method to be assign. > How do you think? You duplicated a driver, entire pieces of code. This is not acceptable. Therefore it does not really make sense to discuss whether duplicated solution seems simpler or not... We won't accept duplicated code. Especially for WFI-only driver. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-03-03 9:34 ` Krzysztof Kozlowski @ 2022-03-03 10:02 ` Sudeep Holla 2022-03-04 11:24 ` Edwin Chiu 邱垂峰 0 siblings, 1 reply; 13+ messages in thread From: Sudeep Holla @ 2022-03-03 10:02 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Edwin Chiu 邱垂峰, Edwin Chiu, Sudeep Holla, robh+dt@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, daniel.lezcano@linaro.org, linux-pm@vger.kernel.org On Thu, Mar 03, 2022 at 10:34:31AM +0100, Krzysztof Kozlowski wrote: > On 03/03/2022 10:01, Edwin Chiu 邱垂峰 wrote: > > > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzk@kernel.org> > >> Sent: Tuesday, March 1, 2022 7:34 PM > >> To: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; Edwin Chiu <edwinchiu0505tw@gmail.com>; > >> robh+dt@kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; rafael@kernel.org; > >> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org > >> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 > >> > >> On 01/03/2022 10:30, Edwin Chiu 邱垂峰 wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: Krzysztof Kozlowski <krzk@kernel.org> > >>>> Sent: Tuesday, February 22, 2022 12:48 AM > >>>> To: Edwin Chiu <edwinchiu0505tw@gmail.com>; Edwin Chiu 邱垂峰 > >>>> <edwin.chiu@sunplus.com>; > >>>> robh+dt@kernel.org; devicetree@vger.kernel.org; > >>>> robh+linux-kernel@vger.kernel.org; rafael@kernel.org; > >>>> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org > >>>> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for > >>>> sunplus sp7021 > >>>> > >>>> On 21/02/2022 08:26, Edwin Chiu wrote: > >>>>> Create cpuidle driver for sunplus sp7021 chip > >>>>> > >>>>> Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com> > >>>>> --- > >>>>> Changes in v3 > >>>>> - Rearrangement #include sequence > >>>>> - Change remark style to /*~*/ > >>>>> - Align author email address to same as sob > >>>>> - Optimal code > >>>>> Changes in v4 > >>>>> - According Rob Herringrobh's comment > >>>>> There is no need for this binding. > >>>>> Just wanting a different driver is not a reason > >>>>> for a duplicate schema. > >>>>> So remove yaml file and submit driver again. > >>>>> Changes in v5 > >>>>> - According Krzysztof's comment > >>>>> You either use appropriate compatible in DT > >>>>> or add your compatible to cpuidle-arm. > >>>>> Even if this did not work, then the solution is to > >>>>> use common parts, not to duplicate entire driver. > >>>>> According Sudeep's comment > >>>>> In short NACK for any dedicated driver for this platform, > >>>>> use the generic cpuidle-arm driver with appropriate platform hooks > >>>>> Create cpuidle-sunplus.c in arch/arm/mach-sunplus/ > >>>>> for hook generic cpuidle-arm driver > >>>>> > >>>>> MAINTAINERS | 6 ++ > >>>>> arch/arm/mach-sunplus/cpuidle-sunplus.c | 88 +++++++++++++++++ > >>>>> include/linux/platform_data/cpuidle-sunplus.h | 12 ++++ > >>>>> 3 files changed, 106 insertions(+) > >>>>> create mode 100644 arch/arm/mach-sunplus/cpuidle-sunplus.c > >>>>> create mode 100644 include/linux/platform_data/cpuidle-sunplus.h > >>>>> > >>>>> diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 100644 > >>>>> --- a/MAINTAINERS > >>>>> +++ b/MAINTAINERS > >>>>> @@ -18252,6 +18252,12 @@ L: netdev@vger.kernel.org > >>>>> S: Maintained > >>>>> F: drivers/net/ethernet/dlink/sundance.c > >>>>> > >>>>> +SUNPLUS CPUIDLE DRIVER > >>>>> +M: Edwin Chiu <edwinchiu0505tw@gmail.com> > >>>>> +S: Maintained > >>>>> +F: arch/arm/mach-sunplus/cpuidle-sunplus.c > >>>>> +F: include/linux/platform_data/cpuidle-sunplus.h > >>>>> + > >>>>> SUPERH > >>>>> M: Yoshinori Sato <ysato@users.sourceforge.jp> > >>>>> M: Rich Felker <dalias@libc.org> > >>>>> diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c > >>>>> b/arch/arm/mach-sunplus/cpuidle-sunplus.c > >>>>> new file mode 100644 > >>>>> index 0000000..e9d9738 > >>>>> --- /dev/null > >>>>> +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c > >>>>> @@ -0,0 +1,88 @@ > >>>>> +// SPDX-License-Identifier: GPL-2.0-only > >>>>> +/* > >>>>> + * SP7021 cpu idle Driver. > >>>>> + * Copyright (C) Sunplus Tech / Tibbo Tech. > >>>>> + */ > >>>>> +#define pr_fmt(fmt) "CPUidle arm: " fmt > >>>>> + > >>>>> +#include <linux/cpuidle.h> > >>>>> +#include <linux/of_device.h> > >>>>> +#include <linux/platform_data/cpuidle-sunplus.h> > >>>>> + > >>>>> +#include <asm/cpuidle.h> > >>>>> + > >>>>> +typedef int (*idle_fn)(void); > >>>>> + > >>>>> +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops); > >>>>> + > >>>>> +static int sp7021_cpuidle_enter(unsigned long index) { > >>>>> + return __this_cpu_read(sp7021_idle_ops)[index](); > >>>>> +} > >>>>> +static int sp7021_cpu_spc(void) > >>>>> +{ > >>>>> + cpu_v7_do_idle(); /* idle to WFI */ > >>>>> + return 0; > >>>>> +} > >>>>> +static const struct of_device_id sp7021_idle_state_match[] = { > >>>>> + { .compatible = "arm,idle-state", .data = sp7021_cpu_spc }, > >>>>> + { }, > >>>>> +}; > >>>> > >>>> This is confusing. You want to have two drivers to bind to the same > >>>> compatible? As I wrote in the previous messages, you should simply use arm,idle-state just like few > >> other architectures. > >>>> > >>>> > >>>> Best regards, > >>>> Krzysztof > >>> > >>> > >>> The patch v5 implemented according your comment. > >>> Used common part of arm,idle-state. > >>> Create new enable-method for cpuidle.ops function. > >>> It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible. > >>> > >>> What do you mean " simply use arm,idle-state just like few other architectures "? > >>> > >> > >> I mean, do it similarly (by using arm,idle-state and other related > >> properties) to for example ti,am4372/ti,am3352. > >> > >> Best regards, > >> Krzysztof > > > > > > The am3352 cpuidle code structure is very similar to ours. > > Used enable-method = "ti,am3352" and compatible = "arm,idle-state" in am33xx.dtsi > > Used CPUIDLE_METHOD_OF_DECLARE(pm33xx_idle, "ti,am3352", &amx3_cpuidle_ops) in pm33xx-core.c > > > > The difference are > > am3352 > > amx3_idle_init(~) assign idle_states[i].wfi_flags = states[i].wfi_flags; > > amx3_idle_enter(~) call idle_fn(idle_state->wfi_flags) > > > > sunplus-sp7021 > > sp7021_cpuidle_init(~) assign fns[i] = idle_fns[i]; > > sp7021_cpuidle_enter(~) call __this_cpu_read(sp7021_idle_ops)[index](); > > > > I don't think am3352 cpuidle code architecture simpler than ours. > > The idle_fn function need more complex method to be assign. > > How do you think? > > You duplicated a driver, entire pieces of code. This is not acceptable. > Therefore it does not really make sense to discuss whether duplicated > solution seems simpler or not... We won't accept duplicated code. > Especially for WFI-only driver. > +1 for above comment. In addition, the reference platform am33xx* doesn't seem to support hotplug (may be I am missing to see but quick grep gave no results) and their idle is definitely not just WFI. So what I asked is that please document the chosen "sunplus,sc-smp" as bot cpu idle and hotplug methods and when you support non WFI states, we can revisit this. Also you must stick to this hotplug method whenever you decided to support it. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-03-03 10:02 ` Sudeep Holla @ 2022-03-04 11:24 ` Edwin Chiu 邱垂峰 2022-03-04 14:37 ` Sudeep Holla 0 siblings, 1 reply; 13+ messages in thread From: Edwin Chiu 邱垂峰 @ 2022-03-04 11:24 UTC (permalink / raw) To: Sudeep Holla, Krzysztof Kozlowski Cc: Edwin Chiu, robh+dt@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, daniel.lezcano@linaro.org, linux-pm@vger.kernel.org > -----Original Message----- > From: Sudeep Holla <sudeep.holla@arm.com> > Sent: Thursday, March 3, 2022 6:03 PM > To: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; Edwin Chiu <edwinchiu0505tw@gmail.com>; > Sudeep Holla <sudeep.holla@arm.com>; robh+dt@kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; rafael@kernel.org; daniel.lezcano@linaro.org; linux-pm@vger.kernel.org > Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 > > On Thu, Mar 03, 2022 at 10:34:31AM +0100, Krzysztof Kozlowski wrote: > > On 03/03/2022 10:01, Edwin Chiu 邱垂峰 wrote: > > > > > > > > >> -----Original Message----- > > >> From: Krzysztof Kozlowski <krzk@kernel.org> > > >> Sent: Tuesday, March 1, 2022 7:34 PM > > >> To: Edwin Chiu 邱垂峰 <edwin.chiu@sunplus.com>; Edwin Chiu > > >> <edwinchiu0505tw@gmail.com>; > > >> robh+dt@kernel.org; devicetree@vger.kernel.org; > > >> robh+linux-kernel@vger.kernel.org; rafael@kernel.org; > > >> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org > > >> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for > > >> sunplus sp7021 > > >> > > >> On 01/03/2022 10:30, Edwin Chiu 邱垂峰 wrote: > > >>> > > >>> > > >>>> -----Original Message----- > > >>>> From: Krzysztof Kozlowski <krzk@kernel.org> > > >>>> Sent: Tuesday, February 22, 2022 12:48 AM > > >>>> To: Edwin Chiu <edwinchiu0505tw@gmail.com>; Edwin Chiu 邱垂峰 > > >>>> <edwin.chiu@sunplus.com>; > > >>>> robh+dt@kernel.org; devicetree@vger.kernel.org; > > >>>> robh+linux-kernel@vger.kernel.org; rafael@kernel.org; > > >>>> daniel.lezcano@linaro.org; linux-pm@vger.kernel.org > > >>>> Subject: Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver > > >>>> for sunplus sp7021 > > >>>> > > >>>> On 21/02/2022 08:26, Edwin Chiu wrote: > > >>>>> Create cpuidle driver for sunplus sp7021 chip > > >>>>> > > >>>>> Signed-off-by: Edwin Chiu <edwinchiu0505tw@gmail.com> > > >>>>> --- > > >>>>> Changes in v3 > > >>>>> - Rearrangement #include sequence > > >>>>> - Change remark style to /*~*/ > > >>>>> - Align author email address to same as sob > > >>>>> - Optimal code > > >>>>> Changes in v4 > > >>>>> - According Rob Herringrobh's comment > > >>>>> There is no need for this binding. > > >>>>> Just wanting a different driver is not a reason > > >>>>> for a duplicate schema. > > >>>>> So remove yaml file and submit driver again. > > >>>>> Changes in v5 > > >>>>> - According Krzysztof's comment > > >>>>> You either use appropriate compatible in DT > > >>>>> or add your compatible to cpuidle-arm. > > >>>>> Even if this did not work, then the solution is to > > >>>>> use common parts, not to duplicate entire driver. > > >>>>> According Sudeep's comment > > >>>>> In short NACK for any dedicated driver for this platform, > > >>>>> use the generic cpuidle-arm driver with appropriate platform hooks > > >>>>> Create cpuidle-sunplus.c in arch/arm/mach-sunplus/ > > >>>>> for hook generic cpuidle-arm driver > > >>>>> > > >>>>> MAINTAINERS | 6 ++ > > >>>>> arch/arm/mach-sunplus/cpuidle-sunplus.c | 88 +++++++++++++++++ > > >>>>> include/linux/platform_data/cpuidle-sunplus.h | 12 ++++ > > >>>>> 3 files changed, 106 insertions(+) create mode 100644 > > >>>>> arch/arm/mach-sunplus/cpuidle-sunplus.c > > >>>>> create mode 100644 > > >>>>> include/linux/platform_data/cpuidle-sunplus.h > > >>>>> > > >>>>> diff --git a/MAINTAINERS b/MAINTAINERS index e0dca8f..5c96428 > > >>>>> 100644 > > >>>>> --- a/MAINTAINERS > > >>>>> +++ b/MAINTAINERS > > >>>>> @@ -18252,6 +18252,12 @@ L: netdev@vger.kernel.org > > >>>>> S: Maintained > > >>>>> F: drivers/net/ethernet/dlink/sundance.c > > >>>>> > > >>>>> +SUNPLUS CPUIDLE DRIVER > > >>>>> +M: Edwin Chiu <edwinchiu0505tw@gmail.com> > > >>>>> +S: Maintained > > >>>>> +F: arch/arm/mach-sunplus/cpuidle-sunplus.c > > >>>>> +F: include/linux/platform_data/cpuidle-sunplus.h > > >>>>> + > > >>>>> SUPERH > > >>>>> M: Yoshinori Sato <ysato@users.sourceforge.jp> > > >>>>> M: Rich Felker <dalias@libc.org> > > >>>>> diff --git a/arch/arm/mach-sunplus/cpuidle-sunplus.c > > >>>>> b/arch/arm/mach-sunplus/cpuidle-sunplus.c > > >>>>> new file mode 100644 > > >>>>> index 0000000..e9d9738 > > >>>>> --- /dev/null > > >>>>> +++ b/arch/arm/mach-sunplus/cpuidle-sunplus.c > > >>>>> @@ -0,0 +1,88 @@ > > >>>>> +// SPDX-License-Identifier: GPL-2.0-only > > >>>>> +/* > > >>>>> + * SP7021 cpu idle Driver. > > >>>>> + * Copyright (C) Sunplus Tech / Tibbo Tech. > > >>>>> + */ > > >>>>> +#define pr_fmt(fmt) "CPUidle arm: " fmt > > >>>>> + > > >>>>> +#include <linux/cpuidle.h> > > >>>>> +#include <linux/of_device.h> > > >>>>> +#include <linux/platform_data/cpuidle-sunplus.h> > > >>>>> + > > >>>>> +#include <asm/cpuidle.h> > > >>>>> + > > >>>>> +typedef int (*idle_fn)(void); > > >>>>> + > > >>>>> +static DEFINE_PER_CPU(idle_fn*, sp7021_idle_ops); > > >>>>> + > > >>>>> +static int sp7021_cpuidle_enter(unsigned long index) { > > >>>>> + return __this_cpu_read(sp7021_idle_ops)[index](); > > >>>>> +} > > >>>>> +static int sp7021_cpu_spc(void) { > > >>>>> + cpu_v7_do_idle(); /* idle to WFI */ > > >>>>> + return 0; > > >>>>> +} > > >>>>> +static const struct of_device_id sp7021_idle_state_match[] = { > > >>>>> + { .compatible = "arm,idle-state", .data = sp7021_cpu_spc }, > > >>>>> + { }, > > >>>>> +}; > > >>>> > > >>>> This is confusing. You want to have two drivers to bind to the > > >>>> same compatible? As I wrote in the previous messages, you should > > >>>> simply use arm,idle-state just like few > > >> other architectures. > > >>>> > > >>>> > > >>>> Best regards, > > >>>> Krzysztof > > >>> > > >>> > > >>> The patch v5 implemented according your comment. > > >>> Used common part of arm,idle-state. > > >>> Create new enable-method for cpuidle.ops function. > > >>> It only have arm cpuidle driver exist now, no two drivers to bind to the same compatible. > > >>> > > >>> What do you mean " simply use arm,idle-state just like few other architectures "? > > >>> > > >> > > >> I mean, do it similarly (by using arm,idle-state and other related > > >> properties) to for example ti,am4372/ti,am3352. > > >> > > >> Best regards, > > >> Krzysztof > > > > > > > > > The am3352 cpuidle code structure is very similar to ours. > > > Used enable-method = "ti,am3352" and compatible = "arm,idle-state" > > > in am33xx.dtsi Used CPUIDLE_METHOD_OF_DECLARE(pm33xx_idle, > > > "ti,am3352", &amx3_cpuidle_ops) in pm33xx-core.c > > > > > > The difference are > > > am3352 > > > amx3_idle_init(~) assign idle_states[i].wfi_flags = > > > states[i].wfi_flags; > > > amx3_idle_enter(~) call idle_fn(idle_state->wfi_flags) > > > > > > sunplus-sp7021 > > > sp7021_cpuidle_init(~) assign fns[i] = idle_fns[i]; > > > sp7021_cpuidle_enter(~) call > > > __this_cpu_read(sp7021_idle_ops)[index](); > > > > > > I don't think am3352 cpuidle code architecture simpler than ours. > > > The idle_fn function need more complex method to be assign. > > > How do you think? > > > > You duplicated a driver, entire pieces of code. This is not acceptable. > > Therefore it does not really make sense to discuss whether duplicated > > solution seems simpler or not... We won't accept duplicated code. > > Especially for WFI-only driver. > > > > +1 for above comment. > > In addition, the reference platform am33xx* doesn't seem to support hotplug (may be I am missing to > see but quick grep gave no results) and their idle is definitely not just WFI. So what I asked is that please > document the chosen "sunplus,sc-smp" as bot cpu idle and hotplug methods and when you support non > WFI states, we can revisit this. Also you must stick to this hotplug method whenever you decided to > support it. > > > -- > Regards, > Sudeep Thanks your advice. Look like key point still only WFI function when cpuidle. As I explain before, only enable generic ARM cpuidle driver is not work. It need enable-method code to assign cpuidle_ops functions. "psci" is one of enable-method, but there have problem in my side due to smc or secure code unsupported. So I create cpuidle-sunplus.c code with "sunplus,sc-smp" to let cpuidle code complete for our source code. With this structure, I can add more custom low power code in the future. What does it mean for "please document the chosen "sunplus,sc-smp" as bot cpu idle and hotplug methods" ? Does it mean "edit yaml file"? (Previously, I submit yaml file also, but Rob say I don't need submit when I use compatible="arm,idle-state") 邱垂峰 EdwinChiu 智能運算專案 T: +886-3-5786005 ext.2590 edwin.chiu@sunplus.com 300 新竹科學園區創新一路19號 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 2022-03-04 11:24 ` Edwin Chiu 邱垂峰 @ 2022-03-04 14:37 ` Sudeep Holla 0 siblings, 0 replies; 13+ messages in thread From: Sudeep Holla @ 2022-03-04 14:37 UTC (permalink / raw) To: Edwin Chiu 邱垂峰 Cc: Krzysztof Kozlowski, Sudeep Holla, Edwin Chiu, robh+dt@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, daniel.lezcano@linaro.org, linux-pm@vger.kernel.org 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. -- Regards, Sudeep [1] Documentation/devicetree/bindings/arm/cpu-enable-method/ [2] Documentation/devicetree/bindings/arm/cpus.yaml ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-03-04 14:37 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-02-21 7:26 [PATCH v5] Add cpuidle driver for Sunplus SP7021 Edwin Chiu 2022-02-21 7:26 ` [PATCH v5] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021 Edwin Chiu 2022-02-21 10:51 ` Sudeep Holla 2022-03-01 9:18 ` Edwin Chiu 邱垂峰 2022-03-01 12:23 ` Sudeep Holla 2022-02-21 16:47 ` Krzysztof Kozlowski 2022-03-01 9:30 ` Edwin Chiu 邱垂峰 2022-03-01 11:33 ` Krzysztof Kozlowski 2022-03-03 9:01 ` Edwin Chiu 邱垂峰 2022-03-03 9:34 ` Krzysztof Kozlowski 2022-03-03 10:02 ` Sudeep Holla 2022-03-04 11:24 ` Edwin Chiu 邱垂峰 2022-03-04 14:37 ` Sudeep Holla
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).