Add cpuidle driver for Sunplus SP7021

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.

 MAINTAINERS                                   |   8 ++
 drivers/cpuidle/Kconfig.arm                   |   7 ++
 drivers/cpuidle/Makefile                      |   1 +
 drivers/cpuidle/cpuidle-sunplus.c             | 167 ++++++++++++++++++++++++++
 include/linux/platform_data/cpuidle-sunplus.h |  19 +++
 5 files changed, 202 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-sunplus.c
 create mode 100644 include/linux/platform_data/cpuidle-sunplus.h

MAINTAINERS +0 -0

index e0dca8f..a938a58 100644

MAINTAINERS +3 -0

+F:	drivers/cpuidle/cpuidle-sunplus.c
+F:	include/linux/platform_data/cpuidle-sunplus.h
+
 SUPERH
 M:	Yoshinori Sato <[email protected]>
 M:	Rich Felker <[email protected]>

drivers/cpuidle/Kconfig.arm +7 -0

index 15d6c46..fd921c5 100644
@@ -118,3 +118,10 @@ config ARM_QCOM_SPM_CPUIDLE
 	  The Subsystem Power Manager (SPM) controls low power modes for the
 	  CPU and L2 cores. It interface with various system drivers to put
 	  the cores in low power modes.
+
+config ARM_SUNPLUS_CPUIDLE
+	bool "CPU Idle Driver For SUNPLUS SoCs"
+	depends on !ARM64
+	select DT_IDLE_STATES
+	help
+	  Select this to enable cpuidle on SP7021 processors.

drivers/cpuidle/Makefile +1 -0

index 26bbc5e..0a020d1 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
 obj-$(CONFIG_ARM_PSCI_CPUIDLE_DOMAIN)	+= cpuidle-psci-domain.o
 obj-$(CONFIG_ARM_TEGRA_CPUIDLE)		+= cpuidle-tegra.o
 obj-$(CONFIG_ARM_QCOM_SPM_CPUIDLE)	+= cpuidle-qcom-spm.o
+obj-$(CONFIG_ARM_SUNPLUS_CPUIDLE)		+= cpuidle-sunplus.o
 
 ###############################################################################
 # MIPS drivers

drivers/cpuidle/cpuidle-sunplus.c new file +25 -0

new file mode 100644
index 0000000..0e7bf43
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014,2015, Linaro Ltd.
+ *
+ * SAW power controller driver
+ */
+#define pr_fmt(fmt) "CPUidle arm: " fmt
+
+#include <asm/suspend.h>
+#include <linux/cpu_cooling.h>
+#include <linux/cpuidle.h>
+#include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/platform_data/cpuidle-sunplus.h>
+
+#include "dt_idle_states.h"
+
+static int sp7021_wfi_finisher(unsigned long flags)
+{
+	cpu_v7_do_idle();   /* idle to WFI */

drivers/cpuidle/cpuidle-sunplus.c new file +14 -0

+		ret = cpu_suspend(idx, sp7021_wfi_finisher);
+	}
+	cpu_pm_exit();
+
+	return ret ? -1:idx;
+}
+
+static struct cpuidle_driver sp7021_idle_driver = {
+	.name = "sp7021_idle",
+	.owner = THIS_MODULE,
+	/*
+	 * State at index 0 is standby wfi and considered standard
+	 * on all ARM platforms. If in some platforms simple wfi
+	 * can't be used as "state 0", DT bindings must be implemented

drivers/cpuidle/cpuidle-sunplus.c new file +95 -0

+	}
+};
+
+static const struct of_device_id sp7021_idle_state_match[] = {
+	{ .compatible = "sunplus,sp7021-idle-state",
+		.data = sp7021_enter_idle_state },
+	{ },
+};
+
+/*
+ * sp7021_idle_init - Initializes sp7021 cpuidle driver
+ *
+ * Initializes sp7021 cpuidle driver for all CPUs, if any CPU fails
+ * to register cpuidle driver then rollback to cancel all CPUs
+ * registration.
+ */
+static int __init sp7021_idle_init(void)
+{
+	int cpu, ret;
+	struct cpuidle_driver *drv;
+	struct cpuidle_device *dev;
+
+	drv = kmemdup(&sp7021_idle_driver, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	drv->cpumask = (struct cpumask *)cpumask_of(cpu);
+	/*
+	 * Initialize idle states data, starting at index 1.  This
+	 * driver is DT only, if no DT idle states are detected (ret
+	 * == 0) let the driver initialization fail accordingly since
+	 * there is no reason to initialize the idle driver if only
+	 * wfi is supported.
+	 */
+	ret = dt_init_idle_driver(drv, sp7021_idle_state_match, 1);
+	if (ret <= 0)
+		return ret ? : -ENODEV;
+
+	ret = cpuidle_register_driver(drv);
+	if (ret) {
+		pr_err("Failed to register cpuidle driver\n");
+		return ret;
+	}
+
+	for_each_possible_cpu(cpu) {
+		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+		if (!dev) {
+			ret = -ENOMEM;
+			goto out_fail;
+		}
+		dev->cpu = cpu;
+
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("Failed to register cpuidle device for CPU %d\n", cpu);
+			kfree(dev);
+			goto out_fail;
+		}
+	}
+
+	return 0;
+
+out_fail:
+	while (--cpu >= 0) {
+		dev = per_cpu(cpuidle_devices, cpu);
+		cpuidle_unregister_device(dev);
+		kfree(dev);
+	}
+	cpuidle_unregister_driver(drv);
+
+	return ret;
+}
+
+static int __init idle_init(void)
+{
+	int ret;
+
+	if (of_machine_is_compatible("sunplus,sp7021-achip")) {
+		sp7021_idle_init();
+		ret = 0;
+	}	else
+		ret = -1;
+
+	if (ret) {
+		pr_err("failed to cpuidle init\n");
+		return ret;
+	}
+
+	return ret;
+}
+device_initcall(idle_init);
+
+MODULE_AUTHOR("Edwin Chiu <[email protected]>");
+MODULE_DESCRIPTION("Sunplus sp7021 cpuidle driver");
+MODULE_LICENSE("GPL");

include/linux/platform_data/cpuidle-sunplus.h new file +0 -0

new file mode 100644
index 0000000..bf87682

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, February 8, 2022 4:22 AM
To: Edwin Chiu <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Edwin Chiu 邱垂峰 <[email protected]>
Subject: Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

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

 MAINTAINERS                                   |   8 ++
 drivers/cpuidle/Kconfig.arm                   |   7 ++
 drivers/cpuidle/Makefile                      |   1 +
 drivers/cpuidle/cpuidle-sunplus.c             | 167 ++++++++++++++++++++++++++
 include/linux/platform_data/cpuidle-sunplus.h |  19 +++
 5 files changed, 202 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-sunplus.c  create mode

100644 include/linux/platform_data/cpuidle-sunplus.h

MAINTAINERS +0 -0

@@ -18252,6 +18252,14 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 

MAINTAINERS L: [email protected] · line 18259

Quoted 5 lines

MAINTAINERS +5 -0

+SUNPLUS CPUIDLE DRIVER
+M:	Edwin Chiu <[email protected]>
+S:	Maintained
+F:	drivers/cpuidle/Kconfig.arm
+F:	drivers/cpuidle/Makefile

This does not belong here.

Quoted 69 lines

MAINTAINERS +3 -0

+F:	drivers/cpuidle/cpuidle-sunplus.c
+F:	include/linux/platform_data/cpuidle-sunplus.h
+
 SUPERH
 M:	Yoshinori Sato <[email protected]>
 M:	Rich Felker <[email protected]>

drivers/cpuidle/Kconfig.arm +7 -0

index 15d6c46..fd921c5 100644
@@ -118,3 +118,10 @@ config ARM_QCOM_SPM_CPUIDLE
 	  The Subsystem Power Manager (SPM) controls low power modes for the
 	  CPU and L2 cores. It interface with various system drivers to put
 	  the cores in low power modes.
+
+config ARM_SUNPLUS_CPUIDLE
+	bool "CPU Idle Driver For SUNPLUS SoCs"
+	depends on !ARM64
+	select DT_IDLE_STATES
+	help
+	  Select this to enable cpuidle on SP7021 processors.

drivers/cpuidle/Makefile index renamed from drivers/cpuidle/Makefile +0 -0

26bbc5e..0a020d1 100644

drivers/cpuidle/Makefile +1 -0

@@ -25,6 +25,7 @@ obj-$(CONFIG_ARM_PSCI_CPUIDLE)		+= cpuidle-psci.o
 obj-$(CONFIG_ARM_PSCI_CPUIDLE_DOMAIN)	+= cpuidle-psci-domain.o
 obj-$(CONFIG_ARM_TEGRA_CPUIDLE)		+= cpuidle-tegra.o
 obj-$(CONFIG_ARM_QCOM_SPM_CPUIDLE)	+= cpuidle-qcom-spm.o
+obj-$(CONFIG_ARM_SUNPLUS_CPUIDLE)		+= cpuidle-sunplus.o
 
 
 ######################################################################

#########
# MIPS drivers

drivers/cpuidle/cpuidle-sunplus.c new file +27 -0

new file mode 100644
index 0000000..0e7bf43
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014,2015, Linaro Ltd.
+ *
+ * SAW power controller driver
+ */
+#define pr_fmt(fmt) "CPUidle arm: " fmt
+
+#include <asm/suspend.h>
+#include <linux/cpu_cooling.h>
+#include <linux/cpuidle.h>
+#include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/platform_data/cpuidle-sunplus.h>
+
+#include "dt_idle_states.h"
+
+static int sp7021_wfi_finisher(unsigned long flags) {
+	cpu_v7_do_idle();   /* idle to WFI */
+
+	return -1;
+}

drivers/cpuidle/cpuidle-sunplus.c line 35

Quoted 6 lines

drivers/cpuidle/cpuidle-sunplus.c new file +6 -0

+
+static int sp7021_enter_idle_state(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int idx) {
+	int ret;
+
+  /* if idx=0, call cpu_do_idle() */

Wrong indentation.

Quoted 7 lines

drivers/cpuidle/cpuidle-sunplus.c new file +7 -0

+	if (!idx) {
+		cpu_v7_do_idle();
+		return idx;
+	}
+
+	/* if idx>0, call cpu_suspend() */
+	ret = cpu_pm_enter();

drivers/cpuidle/cpuidle-sunplus.c line 48

Quoted 6 lines

drivers/cpuidle/cpuidle-sunplus.c new file +6 -0

+	if (!ret) {
+	/*
+	 * Pass idle state index to cpuidle_suspend which in turn
+	 * will call the CPU ops suspend protocol with idle index as a
+	 * parameter.
+	 */

The same.

Quoted 14 lines

drivers/cpuidle/cpuidle-sunplus.c new file +14 -0

+		ret = cpu_suspend(idx, sp7021_wfi_finisher);
+	}
+	cpu_pm_exit();
+
+	return ret ? -1:idx;
+}
+
+static struct cpuidle_driver sp7021_idle_driver = {
+	.name = "sp7021_idle",
+	.owner = THIS_MODULE,
+	/*
+	 * State at index 0 is standby wfi and considered standard
+	 * on all ARM platforms. If in some platforms simple wfi
+	 * can't be used as "state 0", DT bindings must be implemented

drivers/cpuidle/cpuidle-sunplus.c line 72

Quoted 10 lines

drivers/cpuidle/cpuidle-sunplus.c new file +10 -0

+	 * to work around this issue and allow installing a special
+	 * handler for idle state index 0.
+	 */
+	.states[0] = {
+		.enter                  = sp7021_enter_idle_state,
+		.exit_latency           = 1,
+		.target_residency       = 1,
+		.power_usage		= UINT_MAX,
+		.name                   = "WFI",
+		.desc                   = "ARM WFI",

I have impression that there is no point in having custom driver with WFI...

Still the main question from Daniel and Sudeep stays: why do you need this? You copied exactly cpuildle-arm driver, there is nothing different here. At least I could not spot differences. Maybe except that you use cpu_v7_do_idle explicitly.

Unfortunately I cannot understand the explanation here:
https://lore.kernel.org/all/[email protected]/
Why exactly cpuidle-arm does not work in your case?

Quoted 102 lines

drivers/cpuidle/cpuidle-sunplus.c new file +94 -0

+	}
+};
+
+static const struct of_device_id sp7021_idle_state_match[] = {
+	{ .compatible = "sunplus,sp7021-idle-state",
+		.data = sp7021_enter_idle_state },
+	{ },
+};
+
+/*
+ * sp7021_idle_init - Initializes sp7021 cpuidle driver
+ *
+ * Initializes sp7021 cpuidle driver for all CPUs, if any CPU fails
+ * to register cpuidle driver then rollback to cancel all CPUs
+ * registration.
+ */
+static int __init sp7021_idle_init(void) {
+	int cpu, ret;
+	struct cpuidle_driver *drv;
+	struct cpuidle_device *dev;
+
+	drv = kmemdup(&sp7021_idle_driver, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	drv->cpumask = (struct cpumask *)cpumask_of(cpu);
+	/*
+	 * Initialize idle states data, starting at index 1.  This
+	 * driver is DT only, if no DT idle states are detected (ret
+	 * == 0) let the driver initialization fail accordingly since
+	 * there is no reason to initialize the idle driver if only
+	 * wfi is supported.
+	 */
+	ret = dt_init_idle_driver(drv, sp7021_idle_state_match, 1);
+	if (ret <= 0)
+		return ret ? : -ENODEV;
+
+	ret = cpuidle_register_driver(drv);
+	if (ret) {
+		pr_err("Failed to register cpuidle driver\n");
+		return ret;
+	}
+
+	for_each_possible_cpu(cpu) {
+		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+		if (!dev) {
+			ret = -ENOMEM;
+			goto out_fail;
+		}
+		dev->cpu = cpu;
+
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("Failed to register cpuidle device for CPU %d\n", cpu);
+			kfree(dev);
+			goto out_fail;
+		}
+	}
+
+	return 0;
+
+out_fail:
+	while (--cpu >= 0) {
+		dev = per_cpu(cpuidle_devices, cpu);
+		cpuidle_unregister_device(dev);
+		kfree(dev);
+	}
+	cpuidle_unregister_driver(drv);
+
+	return ret;
+}
+
+static int __init idle_init(void)
+{
+	int ret;
+
+	if (of_machine_is_compatible("sunplus,sp7021-achip")) {
+		sp7021_idle_init();
+		ret = 0;
+	}	else
+		ret = -1;
+
+	if (ret) {
+		pr_err("failed to cpuidle init\n");
+		return ret;
+	}
+
+	return ret;
+}
+device_initcall(idle_init);
+
+MODULE_AUTHOR("Edwin Chiu <[email protected]>");
+MODULE_DESCRIPTION("Sunplus sp7021 cpuidle driver");
+MODULE_LICENSE("GPL");

include/linux/platform_data/cpuidle-sunplus.h new file +0 -0

new file mode 100644
index 0000000..bf87682
@@ -0,0 +1,19 @@

include/linux/platform_data/cpuidle-sunplus.h line 5

Quoted 6 lines

include/linux/platform_data/cpuidle-sunplus.h new file +6 -0

+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.

Do not duplicate SPDX.

include/linux/platform_data/cpuidle-sunplus.h line 14

Quoted 9 lines

include/linux/platform_data/cpuidle-sunplus.h new file +9 -0

+ */
+
+#ifndef __CPUIDLE_SP7021_H
+#define __CPUIDLE_SP7021_H
+
+struct cpuidle_sp7021_data {
+	int (*cpu23_powerdown)(void);
+	void (*pre_enter_aftr)(void);
+	void (*post_enter_aftr)(void);

Where are these used?

include/linux/platform_data/cpuidle-sunplus.h line 19

Quoted 4 lines

include/linux/platform_data/cpuidle-sunplus.h new file +4 -0

+};
+
+extern int cpu_v7_do_idle(void);> +
+#endif

Best regards,
Krzysztof

Hi Krzysztof:

Please see below answer.

Quoted 20 lines

Quoted 7 lines

drivers/cpuidle/cpuidle-sunplus.c new file +7 -0

+static struct cpuidle_driver sp7021_idle_driver = {
+	.name = "sp7021_idle",
+	.owner = THIS_MODULE,
+	/*
+	 * State at index 0 is standby wfi and considered standard
+	 * on all ARM platforms. If in some platforms simple wfi
+	 * can't be used as "state 0", DT bindings must be implemented

drivers/cpuidle/cpuidle-sunplus.c line 72

Quoted 10 lines

drivers/cpuidle/cpuidle-sunplus.c new file +10 -0

+	 * to work around this issue and allow installing a special
+	 * handler for idle state index 0.
+	 */
+	.states[0] = {
+		.enter                  = sp7021_enter_idle_state,
+		.exit_latency           = 1,
+		.target_residency       = 1,
+		.power_usage		= UINT_MAX,
+		.name                   = "WFI",
+		.desc                   = "ARM WFI",

I have impression that there is no point in having custom driver with WFI...

Quoted 8 lines

Still the main question from Daniel and Sudeep stays: why do you need this? You copied exactly cpuildle-arm driver, there is nothing different here. At least I could not spot differences. Maybe except that you use cpu_v7_do_idle explicitly.

Unfortunately I cannot understand the explanation here:
https://lore.kernel.org/all/[email protected]/
Why exactly cpuidle-arm does not work in your case?

Edwin=> I mean cpuidle-arm driver can't directly use with no modified.

       If someone want to use cpuidle-arm driver, below modification seems necessary.
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       Static int sp7021_cpuidle_suspend_enter(unsigned long index) {~}
       Static int __init sp7021_cpuidle_init(struct device_node *cpu_node, int cpu) {~}
       Static const struct cpuidle_ops sc_smp_ops __initconst = {
            .suspend = sp7021_cpuidle_suspend_enter,
            .init = sp7021_cpuidle_init,
       };
       CPUIDLE_METHOD_OF_DECLARE(sc_smp, "sunplus,sc-smp", &sc_smp_ops); //declare enable method
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       
       But change cpuilde-arm.c for sunplus driver seems not reasonable.
       That is why I want to submit cpuidle-sunplus.c
       Althought sunplus cpuidle only come in WFI, but it can complete the cpuidle framework.

Quoted 16 lines · Edwin Chiu 邱垂峰

Hi Krzysztof:

Please see below answer.

Quoted 12 lines

Quoted 12 lines

drivers/cpuidle/cpuidle-sunplus.c new file +12 -0

+static struct cpuidle_driver sp7021_idle_driver = {
+	.name = "sp7021_idle",
+	.owner = THIS_MODULE,
+	/*
+	 * State at index 0 is standby wfi and considered standard
+	 * on all ARM platforms. If in some platforms simple wfi
+	 * can't be used as "state 0", DT bindings must be implemented
+	 * to work around this issue and allow installing a special
+	 * handler for idle state index 0.
+	 */
+	.states[0] = {
+		.enter                  = sp7021_enter_idle_state,

Quoted 8 lines

Quoted 8 lines

drivers/cpuidle/cpuidle-sunplus.c line 72

Quoted 5 lines

drivers/cpuidle/cpuidle-sunplus.c new file +5 -0

+		.exit_latency           = 1,
+		.target_residency       = 1,
+		.power_usage		= UINT_MAX,
+		.name                   = "WFI",
+		.desc                   = "ARM WFI",

I have impression that there is no point in having custom driver with WFI...

Quoted 5 lines

Quoted 5 lines

Still the main question from Daniel and Sudeep stays: why do you need this? You copied exactly cpuildle-arm driver, there is nothing different here. At least I could not spot differences. Maybe except that you use cpu_v7_do_idle explicitly.

Quoted 4 lines

Quoted 4 lines

Unfortunately I cannot understand the explanation here:
https://lore.kernel.org/all/[email protected]/
Why exactly cpuidle-arm does not work in your case?

Quoted 12 lines

Edwin=> I mean cpuidle-arm driver can't directly use with no modified.

       If someone want to use cpuidle-arm driver, below modification seems necessary.
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       Static int sp7021_cpuidle_suspend_enter(unsigned long index) {~}
       Static int __init sp7021_cpuidle_init(struct device_node *cpu_node, int cpu) {~}
       Static const struct cpuidle_ops sc_smp_ops __initconst = {
            .suspend = sp7021_cpuidle_suspend_enter,
            .init = sp7021_cpuidle_init,
       };
       CPUIDLE_METHOD_OF_DECLARE(sc_smp, "sunplus,sc-smp", &sc_smp_ops); //declare enable method
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

-----Original Message-----
From: Sudeep Holla <[email protected]>
Sent: Monday, February 14, 2022 6:00 PM
To: Edwin Chiu 邱垂峰 <[email protected]>
Cc: Krzysztof Kozlowski <[email protected]>; Edwin Chiu <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

Quoted 17 lines · Krzysztof Kozlowski

Quoted 16 lines · Edwin Chiu 邱垂峰

Hi Krzysztof:

Please see below answer.

Quoted 12 lines

Quoted 12 lines

drivers/cpuidle/cpuidle-sunplus.c new file +12 -0

+static struct cpuidle_driver sp7021_idle_driver = {
+	.name = "sp7021_idle",
+	.owner = THIS_MODULE,
+	/*
+	 * State at index 0 is standby wfi and considered standard
+	 * on all ARM platforms. If in some platforms simple wfi
+	 * can't be used as "state 0", DT bindings must be implemented
+	 * to work around this issue and allow installing a special
+	 * handler for idle state index 0.
+	 */
+	.states[0] = {
+		.enter                  = sp7021_enter_idle_state,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/cpuidle/cpuidle-sunplus.c line 72

Quoted 5 lines

drivers/cpuidle/cpuidle-sunplus.c new file +5 -0

+		.exit_latency           = 1,
+		.target_residency       = 1,
+		.power_usage		= UINT_MAX,
+		.name                   = "WFI",
+		.desc                   = "ARM WFI",

I have impression that there is no point in having custom driver with WFI...

+1

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Still the main question from Daniel and Sudeep stays: why do you need this? You copied exactly cpuildle-arm driver, there is nothing different here. At least I could not spot differences. Maybe except that you use cpu_v7_do_idle explicitly.

Please comment or answer why you can't use standard driver.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Unfortunately I cannot understand the explanation here:
https://lore.kernel.org/all/0812c44f777d4026b79df2e3698294be@sphcmb
x02.sunplus.com.tw/ Why exactly cpuidle-arm does not work in your case?

Edwin=> I mean cpuidle-arm driver can't directly use with no modified.
If someone want to use cpuidle-arm driver, below modification seems necessary.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Quoted 7 lines

Quoted 7 lines

       Static int sp7021_cpuidle_suspend_enter(unsigned long index) {~}
       Static int __init sp7021_cpuidle_init(struct device_node *cpu_node, int cpu) {~}
       Static const struct cpuidle_ops sc_smp_ops __initconst = {
            .suspend = sp7021_cpuidle_suspend_enter,
            .init = sp7021_cpuidle_init,
       };
       CPUIDLE_METHOD_OF_DECLARE(sc_smp, "sunplus,sc-smp", &sc_smp_ops); //declare

enable method

Quoted 4 lines

Quoted 4 lines

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~

May be. It depends on what is your enable-method. I did a quick grep and could see any support for sunplus platform upstream. So I am not sure what is the cpu boot/enable method used. Is it PSCI or something custom. You should be using standard PSCI if it is relatively new platform or you have any other strong reasons to use custom method. If you are using custom method, then some changes like above is required but that will be in the platform port and not the core cpuidle driver/framework.

In short NACK for any dedicated driver for this platform, use the generic cpuidle-arm driver with appropriate platform hooks(like the above one only if you choose to use custom enable method and not standard PSCI)

--
Regards,
Sudeep

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 14, 2022 2:45 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 v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

Quoted 27 lines · Edwin Chiu 邱垂峰

Hi Krzysztof:

Please see below answer.

Quoted 23 lines

Quoted 7 lines

drivers/cpuidle/cpuidle-sunplus.c new file +7 -0

+static struct cpuidle_driver sp7021_idle_driver = {
+	.name = "sp7021_idle",
+	.owner = THIS_MODULE,
+	/*
+	 * State at index 0 is standby wfi and considered standard
+	 * on all ARM platforms. If in some platforms simple wfi
+	 * can't be used as "state 0", DT bindings must be implemented

drivers/cpuidle/cpuidle-sunplus.c line 72

Quoted 10 lines

drivers/cpuidle/cpuidle-sunplus.c new file +10 -0

+	 * to work around this issue and allow installing a special
+	 * handler for idle state index 0.
+	 */
+	.states[0] = {
+		.enter                  = sp7021_enter_idle_state,
+		.exit_latency           = 1,
+		.target_residency       = 1,
+		.power_usage		= UINT_MAX,
+		.name                   = "WFI",
+		.desc                   = "ARM WFI",

I have impression that there is no point in having custom driver with WFI...

Still the main question from Daniel and Sudeep stays: why do you need
this? You copied exactly cpuildle-arm driver, there is nothing
different here. At least I could not spot differences. Maybe except that you use cpu_v7_do_idle

explicitly.

Quoted 8 lines

Quoted 5 lines

Unfortunately I cannot understand the explanation here:
https://lore.kernel.org/all/0812c44f777d4026b79df2e3698294be@sphcmbx0
2.sunplus.com.tw/ Why exactly cpuidle-arm does not work in your case?

Edwin=> I mean cpuidle-arm driver can't directly use with no modified.
If someone want to use cpuidle-arm driver, below modification seems necessary.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Quoted 7 lines

       Static int sp7021_cpuidle_suspend_enter(unsigned long index) {~}
       Static int __init sp7021_cpuidle_init(struct device_node *cpu_node, int cpu) {~}
       Static const struct cpuidle_ops sc_smp_ops __initconst = {
            .suspend = sp7021_cpuidle_suspend_enter,
            .init = sp7021_cpuidle_init,
       };
       CPUIDLE_METHOD_OF_DECLARE(sc_smp, "sunplus,sc-smp", &sc_smp_ops); //declare

enable method

Quoted 7 lines

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~

But change cpuilde-arm.c for sunplus driver seems not reasonable.
That is why I want to submit cpuidle-sunplus.c
Althought sunplus cpuidle only come in WFI, but it can complete the cpuidle framework.

I don't think it is correct. You can use cpuidle-arm, because it is being always initialized with device_initcall(). 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. Duplicating is not acceptable.

Best regards,
Krzysztof

Quoted 71 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 14, 2022 2:45 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 v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

Quoted 27 lines · Edwin Chiu 邱垂峰

Hi Krzysztof:

Please see below answer.

Quoted 23 lines

Quoted 7 lines

drivers/cpuidle/cpuidle-sunplus.c new file +7 -0

+static struct cpuidle_driver sp7021_idle_driver = {
+	.name = "sp7021_idle",
+	.owner = THIS_MODULE,
+	/*
+	 * State at index 0 is standby wfi and considered standard
+	 * on all ARM platforms. If in some platforms simple wfi
+	 * can't be used as "state 0", DT bindings must be implemented

drivers/cpuidle/cpuidle-sunplus.c line 72

Quoted 10 lines

drivers/cpuidle/cpuidle-sunplus.c new file +10 -0

+	 * to work around this issue and allow installing a special
+	 * handler for idle state index 0.
+	 */
+	.states[0] = {
+		.enter                  = sp7021_enter_idle_state,
+		.exit_latency           = 1,
+		.target_residency       = 1,
+		.power_usage		= UINT_MAX,
+		.name                   = "WFI",
+		.desc                   = "ARM WFI",

I have impression that there is no point in having custom driver with WFI...

Still the main question from Daniel and Sudeep stays: why do you need
this? You copied exactly cpuildle-arm driver, there is nothing
different here. At least I could not spot differences. Maybe except that you use cpu_v7_do_idle

explicitly.

Quoted 8 lines

Quoted 5 lines

Unfortunately I cannot understand the explanation here:
https://lore.kernel.org/all/0812c44f777d4026b79df2e3698294be@sphcmbx0
2.sunplus.com.tw/ Why exactly cpuidle-arm does not work in your case?

Edwin=> I mean cpuidle-arm driver can't directly use with no modified.
If someone want to use cpuidle-arm driver, below modification seems necessary.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Quoted 7 lines

       Static int sp7021_cpuidle_suspend_enter(unsigned long index) {~}
       Static int __init sp7021_cpuidle_init(struct device_node *cpu_node, int cpu) {~}
       Static const struct cpuidle_ops sc_smp_ops __initconst = {
            .suspend = sp7021_cpuidle_suspend_enter,
            .init = sp7021_cpuidle_init,
       };
       CPUIDLE_METHOD_OF_DECLARE(sc_smp, "sunplus,sc-smp", &sc_smp_ops); //declare

enable method

Quoted 7 lines

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~

But change cpuilde-arm.c for sunplus driver seems not reasonable.
That is why I want to submit cpuidle-sunplus.c
Althought sunplus cpuidle only come in WFI, but it can complete the cpuidle framework.

I don't think it is correct. You can use cpuidle-arm, because it is being always initialized with device_initcall(). 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. Duplicating is not acceptable.

Quoted 2 lines

Best regards,
Krzysztof

I do used compatible = "arm,idle-state" in DT and enable generic arm cpuidle driver in menuconfig. But there have mount driver fail message due to no cpuidle_ops functions.
That is why I need added patch code to complete cpuidle driver.
According your comment, I will try to use common parts and hook some custom code, later.

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, February 18, 2022 4:31 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 v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

Quoted 69 lines · Edwin Chiu 邱垂峰

Quoted 69 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 14, 2022 2:45 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 v4] cpuidle: sunplus: Create cpuidle driver for sunplus sp7021

Quoted 28 lines · Edwin Chiu 邱垂峰

Hi Krzysztof:

Please see below answer.

Quoted 24 lines

Quoted 7 lines

drivers/cpuidle/cpuidle-sunplus.c new file +7 -0

+static struct cpuidle_driver sp7021_idle_driver = {
+	.name = "sp7021_idle",
+	.owner = THIS_MODULE,
+	/*
+	 * State at index 0 is standby wfi and considered standard
+	 * on all ARM platforms. If in some platforms simple wfi
+	 * can't be used as "state 0", DT bindings must be implemented

drivers/cpuidle/cpuidle-sunplus.c line 72

Quoted 10 lines

drivers/cpuidle/cpuidle-sunplus.c new file +10 -0

+	 * to work around this issue and allow installing a special
+	 * handler for idle state index 0.
+	 */
+	.states[0] = {
+		.enter                  = sp7021_enter_idle_state,
+		.exit_latency           = 1,
+		.target_residency       = 1,
+		.power_usage		= UINT_MAX,
+		.name                   = "WFI",
+		.desc                   = "ARM WFI",

I have impression that there is no point in having custom driver with WFI...

Still the main question from Daniel and Sudeep stays: why do you need this? You copied exactly cpuildle-arm driver, there is nothing different here. At least I could not spot differences. Maybe except that you use cpu_v7_do_idle

explicitly.

Quoted 9 lines

Quoted 6 lines

Unfortunately I cannot understand the explanation here:
https://lore.kernel.org/all/0812c44f777d4026b79df2e3698294be@sphcmb
x0 2.sunplus.com.tw/ Why exactly cpuidle-arm does not work in your case?

Edwin=> I mean cpuidle-arm driver can't directly use with no modified.
If someone want to use cpuidle-arm driver, below modification seems necessary.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~

Quoted 8 lines

       Static int sp7021_cpuidle_suspend_enter(unsigned long index) {~}
       Static int __init sp7021_cpuidle_init(struct device_node *cpu_node, int cpu) {~}
       Static const struct cpuidle_ops sc_smp_ops __initconst = {
            .suspend = sp7021_cpuidle_suspend_enter,
            .init = sp7021_cpuidle_init,
       };
       CPUIDLE_METHOD_OF_DECLARE(sc_smp, "sunplus,sc-smp",

&sc_smp_ops); //declare

enable method

Quoted 5 lines

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~
~~~~~~~~~~

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

But change cpuilde-arm.c for sunplus driver seems not reasonable.
That is why I want to submit cpuidle-sunplus.c
Althought sunplus cpuidle only come in WFI, but it can complete the cpuidle framework.

I don't think it is correct. You can use cpuidle-arm, because it is
being always initialized with device_initcall(). You either use appropriate compatible in DT or add

your compatible to cpuidle-arm.

Quoted 4 lines

Quoted 4 lines

Even if this did not work, then the solution is to use common parts, not to duplicate entire driver. Duplicating is not acceptable.

Quoted 8 lines

Quoted 2 lines

Best regards,
Krzysztof

I do used compatible = "arm,idle-state" in DT and enable generic arm cpuidle driver in menuconfig. But there have mount driver fail message due to no cpuidle_ops functions.
That is why I need added patch code to complete cpuidle driver.
According your comment, I will try to use common parts and hook some custom code, later.

I think I understood the motivation behind your driver. The cpuidle-arm requires enable-method property which usually uses for ARMv7 CPU_METHOD_OF_DECLARE(). You don't have CPU_METHOD_OF_DECLARE().

Now the question: why can't you define CPU_METHOD_OF_DECLARE() just like many other ARMv7 platforms do?

Best regards,
Krzysztof