pussuw commented on pull request #5782: URL: https://github.com/apache/incubator-nuttx/pull/5782#issuecomment-1080247601
> @pussuw @jlaitine sorry, I may mix use OpenSBI and RISCV SBI in the discussion, so let me clarify what I mean the interface is the spec documented here: https://github.com/riscv-non-isa/riscv-sbi-doc/releases/download/v1.0.0/riscv-sbi.pdf OpenSBI is just an implementation. Supporting RISCV SBI doesn't mean we must integrate OpenSBI in M mode NuttX, we can write an implementation from scratch just like others: > Implementation ID Name > 0 Berkeley Boot Loader (BBL) > 1 OpenSBI > 2 Xvisor > 3 KVM > 4 RustSBI > 5 Diosix > 6 Coffer > > Of course, We don't need implement the full set at this time. The only needed function is: > > ``` > Chapter 6. Timer Extension (EID #0x54494D45 "TIME") > This replaces legacy timer extension (EID #0x00). It follows the new calling convention defined in > v0.2. > 6.1. Function: Set Timer (FID #0) > struct sbiret sbi_set_timer(uint64_t stime_value) > Programs the clock for next event after stime_value time. stime_value is in absolute time. This > function must clear the pending timer interrupt bit as well. > If the supervisor wishes to clear the timer interrupt without scheduling the next timer event, it can > either request a timer interrupt infinitely far into the future (i.e., (uint64_t)-1), or it can instead mask > the timer interrupt by clearing sie.STIE CSR bit. > ``` > > sbi_set_timer is more flexible than riscv_sbi_ack_timer since we can't implement tickless mode on top of riscv_sbi_ack_timer, but the tickless mode is very important for all battery powered device. That is why I recommend to follow the spec to avoid the rigid and limited api like riscv_sbi_ack_timer. I can of course implement the setter, but > @pussuw @jlaitine sorry, I may mix use OpenSBI and RISCV SBI in the discussion, so let me clarify what I mean the interface is the spec documented here: https://github.com/riscv-non-isa/riscv-sbi-doc/releases/download/v1.0.0/riscv-sbi.pdf OpenSBI is just an implementation. Supporting RISCV SBI doesn't mean we must integrate OpenSBI in M mode NuttX, we can write an implementation from scratch just like others: > Implementation ID Name > 0 Berkeley Boot Loader (BBL) > 1 OpenSBI > 2 Xvisor > 3 KVM > 4 RustSBI > 5 Diosix > 6 Coffer > > Of course, We don't need implement the full set at this time. The only needed function is: > > ``` > Chapter 6. Timer Extension (EID #0x54494D45 "TIME") > This replaces legacy timer extension (EID #0x00). It follows the new calling convention defined in > v0.2. > 6.1. Function: Set Timer (FID #0) > struct sbiret sbi_set_timer(uint64_t stime_value) > Programs the clock for next event after stime_value time. stime_value is in absolute time. This > function must clear the pending timer interrupt bit as well. > If the supervisor wishes to clear the timer interrupt without scheduling the next timer event, it can > either request a timer interrupt infinitely far into the future (i.e., (uint64_t)-1), or it can instead mask > the timer interrupt by clearing sie.STIE CSR bit. > ``` > > sbi_set_timer is more flexible than riscv_sbi_ack_timer since we can't implement tickless mode on top of riscv_sbi_ack_timer, but the tickless mode is very important for all battery powered device. That is why I recommend to follow the spec to avoid the rigid and limited api like riscv_sbi_ack_timer. Yes I'm aware of this spec. and the implementation of riscv_sbi_ack_timer. There is one big issue with the spec. IMO, it does not provide a way to read the timer (mtime) value or the value of mtimecmp. To create an accurate and safe to use ticker both are needed. The initial value of mtimecmp can be anything, typically it is not reset upon warm reset and retains its old value. This is problematic as if mtime >= mtimecmp, the timer will keep interrupting, as it is a level detect interrupt. So, without knowing the value of both, it is impossible for the supervisor to set a value that is in the future. I have no idea how this is supposed to work. All I can say is that it will become extremely easy to softlock the system into running the timer ISR over and over. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
