Update Timer HAL to use new autogen interface#385
Update Timer HAL to use new autogen interface#385ziuziakowska wants to merge 1 commit intolowRISC:mainfrom
Conversation
ziuziakowska
force-pushed
the
hal-autogen-rework-timer
branch
2 times, most recently
from
5d8fcc3 to
7706761
Compare
ziuziakowska
force-pushed
the
hal-autogen-rework-timer
branch
2 times, most recently
from
a01fd9a to
501d512
Compare
| return DEV_READ(timer + TIMER_V_LOWER0_REG); | ||
| /* the rv_timer is effectively a level-triggered interrupt, so to | ||
| * clear it we can schedule an interrupt infinitely far away... */ | ||
| timer_compare_write(timer, UINT64_MAX); |
There was a problem hiding this comment.
If I'm reading the doc correctly, you also need to clear the INTR_STATE register: https://opentitan.org/book/hw/ip/rv_timer/doc/programmers_guide.html#interrupt-handling
There was a problem hiding this comment.
Me and @AlexJones0 looked at this and we deduced that writing INTR_STATE will only clear the interrupt until the comparison between time and time compare is checked again and the interrupt re-asserted. See also this line, though I think it wouldn't hurt to also "clear" the interrupt by writing INTR_STATE just in case the interrupt bit doesn't fall immediately after writing.
There was a problem hiding this comment.
I think that you need to do both - writing to INTR_STATE is required to clear the interrupt, but it will be immediately re-asserted if you haven't configured the timer so the comparison is no longer true.
Even if you make the change here with timer_compare_write, my understanding is that you still then need the INTR_STATE write. Essentially, for "Event" type interrupts, the prim_intr_hw latches incoming interrupt events which must be reset via the INTR_STATE write. The timer comparison change is first needed to stop another event immediately coming in however.
| } | ||
|
|
||
| uint64_t timer_get_value(timer_t timer) | ||
| void timer_enable(timer_t timer) |
There was a problem hiding this comment.
Nit. I would prefer timer_set_enabled(timer_t timer, bool enable)
| } | ||
|
|
||
| void timer_enable_interrupt(timer_t timer) | ||
| uint64_t timer_value_read_us(timer_t timer) |
There was a problem hiding this comment.
| uint64_t timer_value_read_us(timer_t timer) | |
| uint64_t timer_value_read(timer_t timer) |
This function can't garantee the value unit. If a test change the cfg0 reg, this function name would be inconsistent.
There was a problem hiding this comment.
That is true, but in practice I think we should have convenience functionality like this in the library instead of relying on everyone to calculate times themselves which could be much more error prone. All usages of the timer so far have used one us per tick so this was meant to simplify that for future tests.
I could add documentation to those functions in particular to notify users that if they are initialising the timer outside of timer_init then these functions shouldn't be used.
|
|
||
| typedef void *timer_t; | ||
| /* interrupts */ | ||
| bool timer_interrupt_enable_read(timer_t timer); |
There was a problem hiding this comment.
No need to change in this PR, just a heads up, in OT these functions are auto generated, because all IPs follow the same spec.
Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>
ziuziakowska
force-pushed
the
hal-autogen-rework-timer
branch
from
501d512 to
2edd6f4
Compare
3 participants
Split off from #299.