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

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

[Yu Chien Peter Lin]

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
 	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
-- 
2.34.1

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

[Samuel Holland]

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>

>  	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

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

[Yu-Chien Peter Lin]

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
>