Re: [PATCH v2 01/11] clocksource: timer-riscv: select CSR from device tree

[Antony Pavlov <antonynpavlov@gmail.com>]

On Fri, 7 May 2021 13:34:30 +0200
Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:

> Hello Antony,
> 
> On 07.05.21 00:08, Antony Pavlov wrote:
> > barebox timer-riscv driver supports one of user counters:
> > 
> >   * 'cycle', counter for RDCYCLE instruction (CSR 0xc00);
> >   * 'time', timer for RDTIME instruction (CSR 0xc01).
> > 
> > At the moment in S-mode timer-riscv uses the 'cycle' counter,
> > and in M-mode timer-riscv uses the 'time' timer.
> 
> Other way round, right? cycle for M-Mode, time for S-Mode?

Yes, you are right, my bad.

> > 
> > Alas picorv32 CPU core supports only the 'cycle' counter.
> > VexRiscV CPU core supports only the 'time' timer.
> > 
> > This patch makes it possible to use the 'time' timer
> > for VexRiscV CPU in M-mode.
> 
> Is that allowed by the ISA? To provide time, but not cycle?
> Can VexRiscV boot Linux? If so, how does Linux handle lack
> of this CSR?

Here is an unobvious answer for all these questions at once.

The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
Document Version 20190608-Priv-MSU-Ratified states that

<<
   Attempts to access a non-existent CSR raise an illegal instruction exception
>>

So you can even realize in hardware very few CSR for exception handling (mstatus, mcause, mtvec, mie, mepc)
and try to emulate all other CSR in software !

As a result exception handler for VexRiscv emulates access to 'cycle' CSR (RDCYCLE pseudo-op)
by reading the 'time' CSR !  
Please see https://github.com/SpinalHDL/VexRiscv/blob/master/src/main/c/emulator/src/main.c#L239

opensbi do near the same ! Please see https://github.com/riscv/opensbi/blob/master/lib/sbi/sbi_emulate_csr.c#L64

Can VexRiscV boot Linux? Yes, it can boot linux!
Please see this demo: https://asciinema.org/a/7fLu84ytgtVd3rjPm7Li3GD4r
In this demo barebox loads emulator.bin into the RAM.
This emulator.bin realizes SBI for linux kernel and emulates all missed features
necessary for running linux.


>  
> > Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> > ---
> >  arch/riscv/cpu/time.c             |  7 +++++++
> >  arch/riscv/dts/erizo.dtsi         |  2 ++
> >  arch/riscv/include/asm/timer.h    |  1 +
> >  drivers/clocksource/timer-riscv.c | 19 ++++++++-----------
> >  4 files changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/riscv/cpu/time.c b/arch/riscv/cpu/time.c
> > index 39bb6a5112..59c8ca61d6 100644
> > --- a/arch/riscv/cpu/time.c
> > +++ b/arch/riscv/cpu/time.c
> > @@ -18,6 +18,7 @@
> >  #include <asm/timer.h>
> >  
> >  unsigned long riscv_timebase;
> > +unsigned long riscv_use_csr_cycle;
> >  
> >  int timer_init(void)
> >  {
> > @@ -32,6 +33,12 @@ int timer_init(void)
> >  
> >  	riscv_timebase = prop;
> >  
> > +	if (of_property_read_bool(cpu, "csr-cycle")) {
> > +		riscv_use_csr_cycle = 1;
> > +	} else {
> > +		riscv_use_csr_cycle = 0;
> > +	}
> > +
> 
> Any reason this couldn't happen in driver probe?

Because we already have riscv_timebase parse routine outside of driver probe.
I tryed to avoid code duplication.
Why we couldn't parse riscv_timebase in driver probe?

> I'd also prefer another name, e.g. barebox,use-csr-cycle ?

Yes, barebox,use-csr-cycle is a better name.


> >  	of_platform_populate(cpu, NULL, NULL);
> >  
> >  	return 0;
> > diff --git a/arch/riscv/dts/erizo.dtsi b/arch/riscv/dts/erizo.dtsi
> > index 228711bd69..b3ccf281f2 100644
> > --- a/arch/riscv/dts/erizo.dtsi
> > +++ b/arch/riscv/dts/erizo.dtsi
> > @@ -22,6 +22,8 @@
> >  
> >  		timebase-frequency = <24000000>;
> >  
> > +		csr-cycle;
> > +
> >  		cpu@0 {
> >  			device_type = "cpu";
> >  			compatible = "cliffordwolf,picorv32", "riscv";
> > diff --git a/arch/riscv/include/asm/timer.h b/arch/riscv/include/asm/timer.h
> > index 1f78ef4c00..555b3f5989 100644
> > --- a/arch/riscv/include/asm/timer.h
> > +++ b/arch/riscv/include/asm/timer.h
> > @@ -5,5 +5,6 @@
> >  
> >  int timer_init(void);
> >  extern unsigned long riscv_timebase;
> > +extern unsigned long riscv_use_csr_cycle;
> >  
> >  #endif /* _ASM_RISCV_DELAY_H */
> > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> > index ef67cff475..c0deed40eb 100644
> > --- a/drivers/clocksource/timer-riscv.c
> > +++ b/drivers/clocksource/timer-riscv.c
> > @@ -13,7 +13,7 @@
> >  #include <asm/timer.h>
> >  #include <asm/csr.h>
> >  
> > -static u64 notrace riscv_timer_get_count_sbi(void)
> > +static u64 notrace riscv_timer_get_count_time(void)
> >  {
> >  	__maybe_unused u32 hi, lo;
> >  
> > @@ -28,7 +28,7 @@ static u64 notrace riscv_timer_get_count_sbi(void)
> >  	return ((u64)hi << 32) | lo;
> >  }
> >  
> > -static u64 notrace riscv_timer_get_count_rdcycle(void)
> > +static u64 notrace riscv_timer_get_count_cycle(void)
> >  {
> >  	__maybe_unused u32 hi, lo;
> >  
> > @@ -43,16 +43,7 @@ static u64 notrace riscv_timer_get_count_rdcycle(void)
> >  	return ((u64)hi << 32) | lo;
> >  }
> >  
> > -static u64 notrace riscv_timer_get_count(void)
> > -{
> > -	if (IS_ENABLED(CONFIG_RISCV_SBI))
> > -		return riscv_timer_get_count_sbi();
> > -	else
> > -		return riscv_timer_get_count_rdcycle();
> > -}
> > -
> >  static struct clocksource riscv_clocksource = {
> > -	.read		= riscv_timer_get_count,
> >  	.mask		= CLOCKSOURCE_MASK(64),
> >  	.priority	= 100,
> >  };
> > @@ -61,6 +52,12 @@ static int riscv_timer_init(struct device_d* dev)
> >  {
> >  	dev_info(dev, "running at %lu Hz\n", riscv_timebase);
> >  
> > +	if (riscv_use_csr_cycle) {
> > +		riscv_clocksource.read = riscv_timer_get_count_cycle;
> > +	} else {
> > +		riscv_clocksource.read = riscv_timer_get_count_time;
> > +	}
> > +
> >  	riscv_clocksource.mult = clocksource_hz2mult(riscv_timebase, riscv_clocksource.shift);
> >  
> >  	return init_clock(&riscv_clocksource);
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |


-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox