From patchwork Tue Sep 15 12:59:23 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: liush X-Patchwork-Id: 11776489 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 3A8AD159A for ; Tue, 15 Sep 2020 13:00:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 114A82076C for ; Tue, 15 Sep 2020 13:00:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726470AbgIONAY (ORCPT ); Tue, 15 Sep 2020 09:00:24 -0400 Received: from smtp2207-205.mail.aliyun.com ([121.197.207.205]:39363 "EHLO smtp2207-205.mail.aliyun.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726183AbgIOM7e (ORCPT ); Tue, 15 Sep 2020 08:59:34 -0400 X-Alimail-AntiSpam: AC=CONTINUE;BC=0.07386647|-1;CH=green;DM=|CONTINUE|false|;DS=CONTINUE|ham_system_inform|0.0476423-0.000196116-0.952162;FP=0|0|0|0|0|-1|-1|-1;HT=e01l10422;MF=liush@allwinnertech.com;NM=1;PH=DW;RN=14;RT=14;SR=0;TI=W4_5948689_DEFAULT_0A9326FA_1600174289947_o7001c2340k; Received: from WS-web (liush@allwinnertech.com[W4_5948689_DEFAULT_0A9326FA_1600174289947_o7001c2340k]) by ay29a011140100197.et135 at Tue, 15 Sep 2020 20:59:23 +0800 Date: Tue, 15 Sep 2020 20:59:23 +0800 From: " =?utf-8?b?5YiY6YK15Y2OQlRE?= " To: "Palmer Dabbelt" Cc: "Paul Walmsley" , "aou" , "rjw" , "daniel.lezcano" , "Anup Patel" , "Atish Patra" , "Damien Le Moal" , "wangkefeng.wang" , "kernel" , "zong.li" , "linux-riscv" , "linux-kernel" , "linux-pm" Reply-To: " =?utf-8?b?5YiY6YK15Y2OQlRE?= " Message-ID: <83f5615c-0ab0-4e1e-b083-b5558ee222b9.liush@allwinnertech.com> Subject: =?utf-8?b?5Zue5aSN77yaW1BBVENIXSBjcHVpZGxlOiBhZGQgcmlzY3YgY3B1aWRs?= =?utf-8?b?ZSBkcml2ZXI=?= X-Mailer: [Alimail-Mailagent][W4_5948689][DEFAULT][Firefox] MIME-Version: 1.0 References: <1600048323-2964-1-git-send-email-liush@allwinnertech.com>, x-aliyun-mail-creator: W4_5948689_DEFAULT_IHJTW96aWxsYS81LjAgKFgxMTsgVWJ1bnR1OyBMaW51eCB4ODZfNjQ7IHJ2OjgwLjApIEdlY2tvLzIwMTAwMTAxIEZpcmVmb3gvODAuMA==jQ In-Reply-To: Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org Hi Palmer, > > This patch adds a cpuidle driver for systems based RISCV architecture. > > This patch supports state WFI. Other states will be supported in the > > future. > > > > Signed-off-by: liush > > --- > > arch/riscv/Kconfig | 7 +++++ > > arch/riscv/include/asm/cpuidle.h | 7 +++++ > > arch/riscv/kernel/Makefile | 1 + > > arch/riscv/kernel/cpuidle.c | 8 ++++++ > > drivers/cpuidle/Kconfig | 5 ++++ > > drivers/cpuidle/Kconfig.riscv | 11 ++++++++ > > drivers/cpuidle/Makefile | 4 +++ > > drivers/cpuidle/cpuidle-riscv.c | 55 ++++++++++++++++++++++++++++++++++++++++ > > 8 files changed, 98 insertions(+) > > create mode 100644 arch/riscv/include/asm/cpuidle.h > > create mode 100644 arch/riscv/kernel/cpuidle.c > > 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..c7ddb9d 100644 > > --- a/arch/riscv/Kconfig > > +++ b/arch/riscv/Kconfig > > @@ -86,6 +86,7 @@ config RISCV > > select SPARSE_IRQ > > select SYSCTL_EXCEPTION_TRACE > > select THREAD_INFO_IN_TASK > > + select CPU_IDLE > > > > config ARCH_MMAP_RND_BITS_MIN > > default 18 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..2599d2f > > --- /dev/null > > +++ b/arch/riscv/include/asm/cpuidle.h > > @@ -0,0 +1,7 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef __RISCV_CPUIDLE_H > > +#define __RISCV_CPUIDLE_H > > + > +extern void cpu_do_idle(void); > > + > > +#endif > > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile > > index dc93710..396ba9c 100644 > > --- a/arch/riscv/kernel/Makefile > > +++ b/arch/riscv/kernel/Makefile > > @@ -29,6 +29,7 @@ obj-y += riscv_ksyms.o > > obj-y += stacktrace.o > > obj-y += cacheinfo.o > > obj-y += patch.o > > +obj-y += cpuidle.o > Presumably we want this to be a Kconfig option, if only to avoid excess size on > the smaller systems? Thank you for your suggestions. I am considering following Anup's suggestion - delete cpuidle.c and implement cpu_do_idle in cpuidle.h. > > obj-$(CONFIG_MMU) += vdso.o vdso/ > > > > obj-$(CONFIG_RISCV_M_MODE) += traps_misaligned.o > > diff --git a/arch/riscv/kernel/cpuidle.c b/arch/riscv/kernel/cpuidle.c > > new file mode 100644 > > index 00000000..a3289e7 > > --- /dev/null > > +++ b/arch/riscv/kernel/cpuidle.c > > @@ -0,0 +1,8 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +#include > > + > > +void cpu_do_idle(void) > > +{ > > + __asm__ __volatile__ ("wfi"); > > + > We have wait_for_interrupt() that does this already, but it's one line so it > doesn't really matter. Either way, there's an extra newline here. Thanks, i'll modify it > Additionally, we have arch_cpu_idle() which is calling cpu_do_idle() on other > platforms. Presumably we should be doing something similar, under the > assumption that we will eventually have cpu_do_idle() hook into CPU idle > drivers here? Based on your comments, I run a test and found that arch_cpu_idle could be executed normally without enabling the idle driver.Part of the code of the experiment: +++ b/arch/riscv/kernel/process.c @@ -21,6 +21,7 @@ #include #include #include +#include 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..e86d36b > > --- /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 > Looks like there's some space/tab issues here. IIRC checkpatch will catch this > sort of thing. yes, i'll modify it > > + 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define MAX_IDLE_STATES 1 > > + > > +/* TODO: Implement deeper idle states */ > > +static int riscv_low_level_suspend_enter(int state) > > +{ > > + return 0; > As far as I can tell, this driver just doesn't do anything? Assuming that's > the case, it's probably best to just drop everything but cpu_do_idle() until we > have something to exercise this. Although, the driver in this case is not necessary, it is essential when work in the macro CPU_PM_CPU_IDLE_ENTER_PARAM.And it could also be used for supporting other deeper states in the future. So I think it would be better to keep riscv_low_level_suspend_enter but change it to __weak function. > > +} > > + > > +/* 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); > > +} > > + > > +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); --- /dev/null +++ b/arch/riscv/include/asm/cpuidle.h @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __RISCV_CPUIDLE_H +#define __RISCV_CPUIDLE_H + +static inline void cpu_do_idle(void) +{ + mb(); + wait_for_interrupt(); +} + +#endif