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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Antony,

On 07.05.21 19:41, Antony Pavlov wrote:
> On Fri, 7 May 2021 13:34:30 +0200
> Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>> 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.

Thanks for clearing this up :)

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

Because riscv_timebase is also used for CLINT clocksource, but this one
here would be exclusive to riscv timer, so it should be read there.

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

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

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