Re: [PATCH] riscv: andesv5: Prefer using the generic RISC-V timer driver in S-mode

[Yu-Chien Peter Lin <peterlin@andestech.com>]

Hi Samuel,

On Wed, Sep 27, 2023 at 04:32:30PM -0500, Samuel Holland wrote:
> On 9/27/23 02:25, Yu Chien Peter Lin wrote:
> > The Andes PLMT driver directly accesses the mtime MMIO region,
> > indicating its intended use in the M-mode boot stage. However,
> > since U-Boot proper (S-mode) also uses the PLMT driver, we need
> > to specifically mark the region as readable through PMPCFGx (or
> > S/U-mode read-only shared data region for Smepmp) in OpenSBI.
> > 
> > Granting permission for this case doesn't make sense. Instead,
> > we should use the generic RISC-V timer driver to read the mtime
> > through the TIME CSR.  Therefore, we add SPL_ANDES_PLMT_TIMER
> > config, the PLMT driver will be compiled only against M-mode
> > U-Boot or U-Boot SPL.
> > 
> > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com>
> > ---
> >  arch/riscv/cpu/andesv5/Kconfig | 3 ++-
> >  drivers/timer/Kconfig          | 9 ++++++++-
> >  drivers/timer/Makefile         | 2 +-
> >  3 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/riscv/cpu/andesv5/Kconfig b/arch/riscv/cpu/andesv5/Kconfig
> > index 82bb5a2a53..eba576af2f 100644
> > --- a/arch/riscv/cpu/andesv5/Kconfig
> > +++ b/arch/riscv/cpu/andesv5/Kconfig
> > @@ -4,8 +4,9 @@ config RISCV_NDS
> >  	imply CPU
> >  	imply CPU_RISCV
> >  	imply RISCV_TIMER if (RISCV_SMODE || SPL_RISCV_SMODE)
> > +	imply ANDES_PLMT_TIMER if RISCV_MMODE
> > +	imply SPL_ANDES_PLMT_TIMER if SPL_RISCV_MMODE
> 
> You don't need the "if RISCV_MMODE" condition since the imply statement
> will be ignored if the dependency is not met. Either way:
> 
> Reviewed-by: Samuel Holland <samuel@sholland.org>

Thanks for pointing this out! Will drop the condition.

Regards,
Peter Lin

> >  	imply ANDES_PLICSW if (RISCV_MMODE || SPL_RISCV_MMODE)
> > -	imply ANDES_PLMT_TIMER if (RISCV_MMODE || SPL_RISCV_MMODE)
> >  	imply V5L2_CACHE
> >  	imply SPL_CPU
> >  	imply SPL_OPENSBI
> > diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> > index 915b2af160..157298a941 100644
> > --- a/drivers/timer/Kconfig
> > +++ b/drivers/timer/Kconfig
> > @@ -59,7 +59,14 @@ config ALTERA_TIMER
> >  
> >  config ANDES_PLMT_TIMER
> >  	bool
> > -	depends on RISCV_MMODE || SPL_RISCV_MMODE
> > +	depends on RISCV_MMODE
> > +	help
> > +	  The Andes PLMT block holds memory-mapped mtime register
> > +	  associated with timer tick.
> > +
> > +config SPL_ANDES_PLMT_TIMER
> > +	bool
> > +	depends on SPL_RISCV_MMODE
> >  	help
> >  	  The Andes PLMT block holds memory-mapped mtime register
> >  	  associated with timer tick.
> > diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
> > index 1ca74805fd..1f5c16fdf3 100644
> > --- a/drivers/timer/Makefile
> > +++ b/drivers/timer/Makefile
> > @@ -4,7 +4,7 @@
> >  
> >  obj-y += timer-uclass.o
> >  obj-$(CONFIG_ALTERA_TIMER)	+= altera_timer.o
> > -obj-$(CONFIG_ANDES_PLMT_TIMER) += andes_plmt_timer.o
> > +obj-$(CONFIG_$(SPL_)ANDES_PLMT_TIMER) += andes_plmt_timer.o
> >  obj-$(CONFIG_ARC_TIMER)	+= arc_timer.o
> >  obj-$(CONFIG_ARM_TWD_TIMER)	+= arm_twd_timer.o
> >  obj-$(CONFIG_AST_TIMER)	+= ast_timer.o
>