Replace timer unit with RISC-V CLINT #91
Conversation
| enable_d = enable_q; | ||
|
|
||
| // Reset count when timer expires | ||
| if (Autoreset && expired_o) |
There was a problem hiding this comment.
I think Autoreset should be a software-setable register, not a parameter.
| end | ||
|
|
||
| // Output assignment | ||
| assign expired_o = (compare_q != 32'h0 && compare_q <= count_q) ? 1'b1 : 1'b0; |
There was a problem hiding this comment.
I am not sure if I like that expired_o may trigger during configuration of the timer itself without it being enabled.
I would consider using something like ... ? enable_q : 1'b0.
|
|
||
| // Output assignment | ||
| assign expired_o = (compare_q != 32'h0 && compare_q <= count_q) ? 1'b1 : 1'b0; | ||
| assign overflow_o = (count_q == 32'hffff_ffff) ? 1'b1 : 1'b0; |
There was a problem hiding this comment.
I would consider using something like ... ? enable_q : 1'b0.
Should there be a sticky overflow register as well?
There was a problem hiding this comment.
A sticky option would make sense depending on the type of interrupt handling.
| set_interrupt_enable(1, IRQ_TIMER); | ||
| set_global_irq_enable(1); |
There was a problem hiding this comment.
I am not a big fan of enabling interrupts in functions without the explicit knowledge of users. I think its better to 'clean up' at the end.
|
Apart from the comments in the code there are two more questions: |
This PR replaces the timer unit module with an OBI-compliant implementation of the RISC-V Core Local Interrupt Controller (CLINT). The idea is to have a cleaner, more understandable implementation and a RISC-V compliant software stack to manage interrupts. In addition, a configurable number of simpler OBI timer units (e.g. adapted from PULP's APB timer) can be added in parallel and connected to custom external interrupt lines. Finally, this is intended to simplify the documentation of interrupt handling in Croc.
Checklist:
helloworldtesttimer_unit