Re: 回复：[PATCH] cpuidle: add riscv cpuidle driver

[fuyao <fuyao@allwinnertech.com>]

On Mon, Sep 14, 2020 at 08:58:02PM +0800, 刘邵华BTD wrote:
> Hi Daniel,
> > > 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 <liush@allwinnertech.com>
> > > ---
> > 
> > [ ... ]
> > 
> > >  
> > >  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 <asm/cpuidle.h>
> > > +
> > > +void cpu_do_idle(void)
> > > +{
> > > + __asm__ __volatile__ ("wfi");
> > > +
> > 
> > extra line
> > 
> > > +}
> 
> > As for the next deeper states should end up with the cpu_do_idle
> > function, isn't there an extra operation with the wfi() like flushing
> > the l1 cache?
> 
> Data cache consistency is mainly ensured by hardware in riscv, and there is no 
> implementation of flushing data cache in kernel. Before wfi(),add an memory 
> barrier operation - mb(). Is this feasible? 
> 
>   //arch/riscv/include/asm/barrier.h
> 
>   17 #define RISCV_FENCE(p, s) \
>   18         __asm__ __volatile__ ("fence " #p "," #s : : : "memory")
>   19 
>   20 /* These barriers need to enforce ordering on both devices or memory. */                                                                                                            
>   21 #define mb()            RISCV_FENCE(iorw,iorw) 
> 
> 
> After modification, the codes is as follows.
> 
> 81 @@ -0,0 +1,8 @@
> 82 +// SPDX-License-Identifier: GPL-2.0
> 83 +#include <asm/cpuidle.h>
> 84 +
> 85 +void cpu_do_idle(void)
> 86 +{
> 87 +       mb();
take a sceptical attibute with it , anyway mb() shout comment it with why
> 88 +       __asm__ __volatile__ ("wfi");
> 89 +
> 90 +}
> 
> > > 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
> > > +        help
> > > +          Select this option to enable generic cpuidle driver for RISCV.
> > > +   Now only support C0 State.
> > 
> > Identation
> 
> I'll fix it. Thank you!

-- 
<http://www.allwinnertech.com> Allwinnertech Technology