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
| } | ||
|
|
||
| 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)
sw/device/lib/hal/timer.c
Outdated
| } | ||
|
|
||
| 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.
There was a problem hiding this comment.
In Opentitan this higher level functions live in a library called testutils, I'm not against having these utilities in the hal, but there should also have functions to read the raw timer value, because tests would want to use it.
ziuziakowska
force-pushed
the
hal-autogen-rework-timer
branch
2 times, most recently
from
2edd6f4 to
0a08c8a
Compare
Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>
ziuziakowska
force-pushed
the
hal-autogen-rework-timer
branch
from
0a08c8a to
cbe2401
Compare
3 participants
Split off from #299.