Re: [v3] cpuidle: add riscv cpuidle driver

From: Palmer Dabbelt
Date: Fri Nov 20 2020 - 21:35:35 EST

On Thu, 12 Nov 2020 08:28:09 PST (-0800), rafael@xxxxxxxxxx wrote:
On Fri, Sep 25, 2020 at 5:46 AM liush <liush@xxxxxxxxxxxxxxxxx> wrote:

This patch adds a simple cpuidle driver for RISC-V systems using
the WFI state. Other states will be supported in the future.

Reported-by: kernel test robot <lkp@xxxxxxxxx>

This isn't needed in a patch adding a new driver.

Signed-off-by: liush <liush@xxxxxxxxxxxxxxxxx>
---
Changes in v3:
- fix the issue reported by kernel test robot
"drivers/cpuidle/cpuidle-riscv.c:22:12: warning: no previous prototype
for 'riscv_low_level_suspend_enter' [-Wmissing-prototypes]"
Changes in v2:
- call "mb()" before run "WFI" in cpu_do_idle
- modify commit description
- place "select CPU_IDLE" in alphabetical order
- replace "__asm__ __volatile__ ("wfi")" with "wait_for_interrupt()"
- delete "cpuidle.c",move "cpu_do_idle()" to cpuidle.h
- modify "arch_cpu_idle", "cpu_do_idle" can be called by
"arch_cpu_idle"
- fix space/tab issues
- modify riscv_low_level_suspend_enter to __weak mode

arch/riscv/Kconfig | 7 +++++
arch/riscv/include/asm/cpuidle.h | 16 ++++++++++++
arch/riscv/kernel/process.c | 3 ++-
drivers/cpuidle/Kconfig | 5 ++++
drivers/cpuidle/Kconfig.riscv | 11 ++++++++
drivers/cpuidle/Makefile | 4 +++
drivers/cpuidle/cpuidle-riscv.c | 55 ++++++++++++++++++++++++++++++++++++++++
7 files changed, 100 insertions(+), 1 deletion(-)
create mode 100644 arch/riscv/include/asm/cpuidle.h
create mode 100644 drivers/cpuidle/Kconfig.riscv
create mode 100644 drivers/cpuidle/cpuidle-riscv.c

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index df18372..799bf86 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -33,6 +33,7 @@ config RISCV
select ARCH_WANT_HUGE_PMD_SHARE if 64BIT
select CLONE_BACKWARDS
select COMMON_CLK
+ select CPU_IDLE
select EDAC_SUPPORT
select GENERIC_ARCH_TOPOLOGY if SMP
select GENERIC_ATOMIC64 if !64BIT
@@ -407,6 +408,12 @@ config BUILTIN_DTB
depends on RISCV_M_MODE
depends on OF

+menu "CPU Power Management"
+
+source "drivers/cpuidle/Kconfig"
+
+endmenu
+
menu "Power management options"

source "kernel/power/Kconfig"
diff --git a/arch/riscv/include/asm/cpuidle.h b/arch/riscv/include/asm/cpuidle.h
new file mode 100644
index 00000000..599b810
--- /dev/null
+++ b/arch/riscv/include/asm/cpuidle.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __RISCV_CPUIDLE_H
+#define __RISCV_CPUIDLE_H
+
+static inline void cpu_do_idle(void)
+{
+ /*
+ * Add mb() here to ensure that all
+ * IO/MEM access are completed prior
+ * to enter WFI.
+ */
+ mb();

Either the comment isn't precise enough, or this may not work as expected.

The memory barrier prevents memory accesses occurring earlier in the
code flow from being reordered (by the processor or by the compiler)
after the function call below, but is this really needed? That is,
can they be reordered anyway? If so, then why?

+ wait_for_interrupt();
+}
+
+#endif
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 2b97c49..5431aaa 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -21,6 +21,7 @@
#include <asm/string.h>
#include <asm/switch_to.h>
#include <asm/thread_info.h>
+#include <asm/cpuidle.h>

register unsigned long gp_in_global __asm__("gp");

@@ -35,7 +36,7 @@ extern asmlinkage void ret_from_kernel_thread(void);

void arch_cpu_idle(void)
{
- wait_for_interrupt();
+ cpu_do_idle();
local_irq_enable();
}

diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index c0aeedd..f6be0fd 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -62,6 +62,11 @@ depends on PPC
source "drivers/cpuidle/Kconfig.powerpc"
endmenu

+menu "RISCV CPU Idle Drivers"
+depends on RISCV
+source "drivers/cpuidle/Kconfig.riscv"
+endmenu
+
config HALTPOLL_CPUIDLE
tristate "Halt poll cpuidle driver"
depends on X86 && KVM_GUEST
diff --git a/drivers/cpuidle/Kconfig.riscv b/drivers/cpuidle/Kconfig.riscv
new file mode 100644
index 00000000..7bec059
--- /dev/null
+++ b/drivers/cpuidle/Kconfig.riscv
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# RISCV CPU Idle drivers
+#
+config RISCV_CPUIDLE
+ bool "Generic RISCV CPU idle Driver"
+ select DT_IDLE_STATES
+ select CPU_IDLE_MULTIPLE_DRIVERS
+ help
+ Select this option to enable generic cpuidle driver for RISCV.
+ Now only support C0 State.
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 26bbc5e..4c83c4e 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -34,3 +34,7 @@ obj-$(CONFIG_MIPS_CPS_CPUIDLE) += cpuidle-cps.o
# POWERPC drivers
obj-$(CONFIG_PSERIES_CPUIDLE) += cpuidle-pseries.o
obj-$(CONFIG_POWERNV_CPUIDLE) += cpuidle-powernv.o
+
+###############################################################################
+# RISCV drivers
+obj-$(CONFIG_RISCV_CPUIDLE) += cpuidle-riscv.o
diff --git a/drivers/cpuidle/cpuidle-riscv.c b/drivers/cpuidle/cpuidle-riscv.c
new file mode 100644
index 00000000..5dddcfa
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-riscv.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RISC-V CPU idle driver.
+ *
+ * Copyright (C) 2020-2022 Allwinner Ltd
+ *
+ * Based on code - driver/cpuidle/cpuidle-at91.c
+ *
+ */
+#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_device.h>
+#include <asm/cpuidle.h>
+
+#define MAX_IDLE_STATES 1
+
+/* TODO: Implement deeper idle states */
+static int riscv_low_level_suspend_enter(int state)
+{
+ return 0;
+}
+
+/* Actual code that puts the SoC in different idle states */
+static int riscv_enter_idle(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ return CPU_PM_CPU_IDLE_ENTER_PARAM(riscv_low_level_suspend_enter,
+ index, 0);

I'm not sure why this is needed at all.

Because there is only one idle state, idx in __CPU_PM_CPU_IDLE_ENTER()
will always be 0, so it'll always call cpu_do_idle(), so can't it be
invoked directly from here?

And since the arch_cpu_idle() is also WFI, why is the full-blown
cpuidle driver needed at this point?

IIRC that was essentially the same feedback as I had on some earlier version of
this. The ISA defines WFI and a handful of pause mechanisms, and while I'd be
happy to take a driver that selects between those I don't really see a reason
to unless there's some hardware that benefits from it. I would definitely buy
the argument that those existing standard mechanisms are insufficient to build
a realistic chip, but without any concrete users it's very hard to reason about
any code -- that's true for standard extensions, but doubly so for anything
else. In this case requiring an in-tree user may be a bit pedantic, as there's
really only one way to go about this sort of thing, but it's the generally
accepted approach in Linux and without an in-tree user it's very hard to
maintain the code.

I know it can be a headache to keep stuff like this out of tree, and while
we've accepted some stuff with only out of tree users I don't want to make that
a general policy. Specifically I'm thinking of some helper functions for the
hypervisor extension that aren't properly used, but I consider that a bit of a
special case -- that's a standard RISC-V extension, and the ratification
process is just so glacially paced that it seems silly to make a bunch more
work for everyone when it comes to some simple refactoring.

In this case I don't really see such a concrete use case. I suppose a driver
could be constructed for the WFM/pause type stuff, but I don't really see those
(at least as currently defined) being interesting for the Linux use case.
While obviously it'd be best to have any other idle scheme as a standard RISC-V
extension, I understand that is a long process and my guess would be that
(assuming the RISC-V stuff ever get taken seriously) we have a bunch of
non-standard schemes that arrive before an official standard shows up. While I
don't really see any reason to do anything differently for an arbitrary idle
driver, it's impossible to reason about that sort of thing without some user of
the code.

So essentially: I'd be happy to take this if something used it, but without a
user I don't really see how I can.

Sorry!


+}
+
+static struct cpuidle_driver riscv_idle_driver = {
+ .name = "riscv_idle",
+ .owner = THIS_MODULE,
+ .states[0] = {
+ .enter = riscv_enter_idle,
+ .exit_latency = 1,
+ .target_residency = 1,
+ .name = "WFI",
+ .desc = "RISCV WFI",
+ },
+ .state_count = MAX_IDLE_STATES,
+};
+
+static int __init riscv_cpuidle_init(void)
+{
+ return cpuidle_register(&riscv_idle_driver, NULL);
+}
+
+device_initcall(riscv_cpuidle_init);
--