cpu/rpx0xx: add periph timer #16627
cpu/rpx0xx: add periph timer #16627
Conversation
fabian18
requested review from
cgundogan,
jia200x and
leandrolanzieri
as code owners
github-actions
bot
added
Area: boards
cpu/rpx0xx/periph/timer.c
Outdated
| */ | ||
|
|
||
| /** | ||
| * @ingroup cpu_rp2040 |
There was a problem hiding this comment.
| * @ingroup cpu_rp2040 | |
| * @ingroup cpu_rpx0xx |
cpu/rpx0xx/periph/timer.c
Outdated
| } | ||
| /* The timer must run at 1000000 Hz (µs precision) | ||
| because the number of cycles per µs is shared with the watchdog. | ||
| The reference clock (clk_ref) is devided by WATCHDOG->TICK.bits.CYCLES |
There was a problem hiding this comment.
| The reference clock (clk_ref) is devided by WATCHDOG->TICK.bits.CYCLES | |
| The reference clock (clk_ref) is divided by WATCHDOG->TICK.bits.CYCLES |
cpu/rpx0xx/include/periph_cpu.h
Outdated
| * @brief Configuration type of a timer channel | ||
| */ | ||
| typedef struct { | ||
| IRQn_Type irqn; /** timer channel interrupt number */ |
There was a problem hiding this comment.
| IRQn_Type irqn; /** timer channel interrupt number */ | |
| IRQn_Type irqn; /**< timer channel interrupt number */ |
cpu/rpx0xx/include/periph_cpu.h
Outdated
| TIMER_Type *dev; /** pointer to timer base address */ | ||
| const timer_channel_conf_t *ch; /** pointer to timer channel configuration */ | ||
| uint8_t chn; /** number of timer channels */ |
There was a problem hiding this comment.
| TIMER_Type *dev; /** pointer to timer base address */ | |
| const timer_channel_conf_t *ch; /** pointer to timer channel configuration */ | |
| uint8_t chn; /** number of timer channels */ | |
| TIMER_Type *dev; /**< pointer to timer base address */ | |
| const timer_channel_conf_t *ch; /**< pointer to timer channel configuration */ | |
| uint8_t chn; /**< number of timer channels */ |
cpu/rpx0xx/periph/timer.c
Outdated
| do { | ||
| time_h = tmp; | ||
| time_l = TIMER->TIMERAWL; | ||
| } while ((tmp = TIMER->TIMERAWH) != time_h); |
There was a problem hiding this comment.
If I understand the datasheet correctly, it would be better to read from registers TIMELR and TIMEHR. To my understanding a read from TIMELR causes the raw time value from TIMERAWH to be copied into TIMEHR simultaneously while reading the raw least significant time word. Hence, a subsequent read from TIMEHR will contain the value of TIMERAWH at the time TIMELR was read, so that the looping is not needed.
However, an those reads should be wrapped into irq_disable() and irq_restore(), as an interrupting thread also reading the time could still mess with it.
There was a problem hiding this comment.
The SDK does not do it that way because of the second core. But since we are only running on core 0, we could certainly just read TIMELR and then TIMEHR.
cpu/rpx0xx/periph/timer.c
Outdated
| unsigned state = irq_disable(); | ||
| _timer_disable_periodic(dev, channel); | ||
| /* an alarm interrupt matches on the lower 32 bit of the 64 bit timer counter */ | ||
| uint64_t target = DEV(dev)->TIMERAWL + timeout; | ||
| *ALARM(dev, channel) = (uint32_t)target; | ||
| irq_restore(state); |
There was a problem hiding this comment.
Couldn't it happen that on a value of timeout = 0 this has to value for almost a full period, if the clock tick happens between line 147 and 148?
There was a problem hiding this comment.
I tried to force the timer interrupt to trigger if timeout == 0.
if (!timeout) {
io_reg_atomic_set(&DEV(dev)->INTF.reg, 1U << channel);
}But that did not work. When the line executes gdb showed:
(gdb)
target not halted
target rp2040.cpu was not halted when step was requested
CMSIS-DAP command mismatch. Expected 0x5 received 0x0
and the program freezed.
Is it ok to execute the callback immediately?
if (!timeout) {
if (_timer_ctx_cb[dev]) {
_timer_ctx_cb[dev](_timer_ctx_arg[dev], channel);
}
}
|
Care to rebase and squash? |
fabian18
force-pushed
the
cpu_rp2040_periph_timer
branch
from
825aecb
to
c3fa593
Compare
github-actions
bot
removed
Area: doc
benpicco
added
the
CI: ready for build
|
CI is not happy yet |
fabian18
requested review from
aabadie,
fjmolinas,
MichelRottleuthner and
smlng
as code owners
This error disappears without |
It complains that #define GPIO_PIN(port, pin) (pin)A fix could be: #define GPIO_PIN(port, pin) ((((port) & 0)) | (pin)) |
This could result in the compiler optimizing out the store, as it would be unaware that the store has side effects. The issue is that the C++ compiler is interpreting the C code as C++ code. I think something like this could work for both C and C++: diff --git a/cpu/rpx0xx/include/periph_cpu.h b/cpu/rpx0xx/include/periph_cpu.h
index 1f0ea78122..87803d12e0 100644
--- a/cpu/rpx0xx/include/periph_cpu.h
+++ b/cpu/rpx0xx/include/periph_cpu.h
@@ -416,7 +416,7 @@ static inline volatile uint32_t * gpio_pad_register(uint8_t pin)
*/
static inline void gpio_set_pad_config(uint8_t pin, gpio_pad_ctrl_t config)
{
- *(volatile gpio_pad_ctrl_t*)gpio_pad_register(pin) = config;
+ *(volatile gpio_pad_ctrl_t*)gpio_pad_register(pin) = (volatile gpio_pad_ctrl_t)config;
}
/** |
C++ keeps being picky about it. static inline void gpio_set_pad_config(uint8_t pin, volatile gpio_pad_ctrl_t config)
{
*(volatile gpio_pad_ctrl_t *)gpio_pad_register(pin) = config;
}and static inline void gpio_set_pad_config(uint8_t pin, gpio_pad_ctrl_t config)
{
*(volatile gpio_pad_ctrl_t *)gpio_pad_register(pin) = *(volatile gpio_pad_ctrl_t *)&config;
}work out. static inline void gpio_set_pad_config(uint8_t pin, gpio_pad_ctrl_t config)
{
uint32_t *c = (uint32_t *)&config;
*gpio_pad_register(pin) = *c;
}I guess because |
benpicco
added
CI: ready for build
|
Kconfig still needs an update |
|
May I rebase and squash? |
fabian18
force-pushed
the
cpu_rp2040_periph_timer
branch
from
60f0b0c
to
ee535fa
Compare
|
Is there something missing here? |
Only an ACK. I will give the tests another spin later today and then I can provide an ACK, assuming the tests still pass. |
There was a problem hiding this comment.
ACK. One variable naming suggestion. Please squash right in, if you take it.
$ make BOARD=rpi-pico -C tests/periph_timer flash test:
READY
s
START
main(): This is RIOT! (Version: 2021.10-devel-254-gee535f-fabian)
Test for peripheral TIMERs
Available timers: 1
Testing TIMER_0:
TIMER_0: initialization successful
TIMER_0: stopped
TIMER_0: set channel 0 to 5000
TIMER_0: set channel 1 to 10000
TIMER_0: set channel 2 to 15000
TIMER_0: set channel 3 to 20000
TIMER_0: starting
TIMER_0: channel 0 fired at SW count 59908 - init: 59908
TIMER_0: channel 1 fired at SW count 119659 - diff: 59751
TIMER_0: channel 2 fired at SW count 179561 - diff: 59902
TIMER_0: channel 3 fired at SW count 239467 - diff: 59906
TEST SUCCEEDED
$ make BOARD=rpi-pico -C tests/periph_timer_periodic flash test
READY
s
START
main(): This is RIOT! (Version: 2021.10-devel-254-gee535f-fabian)
Running Timer 0 at 1000000 Hz.
One counter cycle is 25000 ticks or 25 ms
Will print 'tick' every cycle.
TEST START
[0] tick
[0] tick
[0] tick
[0] tick
[0] tick
[0] tick
[0] tick
[0] tick
[0] tick
[0] tick
[0] tick
[0] tick
Cycles:
channel 0 = 12 [OK]
TEST SUCCEEDED
$ make BOARD=rpi-pico -C tests/periph_timer_short_relative_set flash test
READY
s
START
main(): This is RIOT! (Version: 2021.10-devel-254-gee535f-fabian)
Test for peripheral TIMER short timer_set()
This test tries timer_set() with decreasing intervals down to 0.
You should see lines like 'interval <n> ok', followed by a success message.
On failure, this test prints an error message.
testing periph_timer 0, freq 1000000
interval 99 ok
interval 98 ok
interval 97 ok
interval 96 ok
interval 95 ok
interval 94 ok
interval 93 ok
interval 92 ok
interval 91 ok
interval 90 ok
interval 89 ok
interval 88 ok
interval 87 ok
interval 86 ok
interval 85 ok
interval 84 ok
interval 83 ok
interval 82 ok
interval 81 ok
interval 80 ok
interval 79 ok
interval 78 ok
interval 77 ok
interval 76 ok
interval 75 ok
interval 74 ok
interval 73 ok
interval 72 ok
interval 71 ok
interval 70 ok
interval 69 ok
interval 68 ok
interval 67 ok
interval 66 ok
interval 65 ok
interval 64 ok
interval 63 ok
interval 62 ok
interval 61 ok
interval 60 ok
interval 59 ok
interval 58 ok
interval 57 ok
interval 56 ok
interval 55 ok
interval 54 ok
interval 53 ok
interval 52 ok
interval 51 ok
interval 50 ok
interval 49 ok
interval 48 ok
interval 47 ok
interval 46 ok
interval 45 ok
interval 44 ok
interval 43 ok
interval 42 ok
interval 41 ok
interval 40 ok
interval 39 ok
interval 38 ok
interval 37 ok
interval 36 ok
interval 35 ok
interval 34 ok
interval 33 ok
interval 32 ok
interval 31 ok
interval 30 ok
interval 29 ok
interval 28 ok
interval 27 ok
interval 26 ok
interval 25 ok
interval 24 ok
interval 23 ok
interval 22 ok
interval 21 ok
interval 20 ok
interval 19 ok
interval 18 ok
interval 17 ok
interval 16 ok
interval 15 ok
interval 14 ok
interval 13 ok
interval 12 ok
interval 11 ok
interval 10 ok
interval 9 ok
interval 8 ok
interval 7 ok
interval 6 ok
interval 5 ok
interval 4 ok
interval 3 ok
interval 2 ok
interval 1 ok
interval 0 ok
TEST SUCCEEDED
I ran the last test 33 times as this one has a stochastic element with 100% success rate.
cpu/rpx0xx/include/periph_cpu.h
Outdated
| typedef struct { | ||
| TIMER_Type *dev; /**< pointer to timer base address */ | ||
| const timer_channel_conf_t *ch; /**< pointer to timer channel configuration */ | ||
| uint8_t chn; /**< number of timer channels */ |
There was a problem hiding this comment.
| uint8_t chn; /**< number of timer channels */ | |
| uint8_t ch_numof; /**< number of timer channels */ |
The postfix numof is commonly used within RIOT's code base, maybe use it here as well for consistency?
fabian18
force-pushed
the
cpu_rp2040_periph_timer
branch
from
ee535fa
to
258681f
Compare
|
I have added your naming suggestion and rebased to the master branch |
Contribution description
This PR contributes a
periph_timerdriver implementation for the RP2040 MCU.Testing procedure
Issues/PRs references
Depends on #16609