Re: [v3] cpuidle: add riscv cpuidle driver

[Rafael J. Wysocki]

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?

> +}
> +
> +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);
> --