Messages in this thread | | | Date | Fri, 20 Nov 2020 18:34:40 -0800 (PST) | Subject | Re: [v3] cpuidle: add riscv cpuidle driver | From | Palmer Dabbelt <> |
| |
On Thu, 12 Nov 2020 08:28:09 PST (-0800), rafael@kernel.org wrote: > On Fri, Sep 25, 2020 at 5:46 AM liush <liush@allwinnertech.com> 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@intel.com> > > This isn't needed in a patch adding a new driver. > >> Signed-off-by: liush <liush@allwinnertech.com> >> --- >> 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); >> --
| |