[riot-notifications] [RIOT-OS/RIOT] cpu/rpx0xx: add periph timer (#16627)

Marian Buschsieweke notifications at github.com
Fri Jul 9 12:08:23 CEST 2021


@maribu commented on this pull request.

Looks very good. Some comments inline.

> @@ -0,0 +1,241 @@
+/*
+ * Copyright (C) 2021 Otto-von-Guericke Universität Magdeburg
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup     cpu_rp2040

```suggestion
 * @ingroup     cpu_rpx0xx
```

> +            _timer_reset(dev);
+        }
+        /* rearm */
+        *ALARM(dev, channel) = *ALARM(dev, channel);
+    }
+    cortexm_isr_end();
+}
+
+int timer_init(tim_t dev, uint32_t freq, timer_cb_t cb, void *arg)
+{
+    if (dev >= TIMER_NUMOF) {
+        return -ENODEV;
+    }
+    /* 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

```suggestion
       The reference clock (clk_ref) is divided by WATCHDOG->TICK.bits.CYCLES
```

> +    UART0_Type *dev;    /**< Base address of the I/O registers of the device */
+    gpio_t rx_pin;      /**< GPIO pin to use for RX */
+    gpio_t tx_pin;      /**< GPIO pin to use for TX */
+    IRQn_Type irqn;     /**< IRQ number of the UART interface */
+} uart_conf_t;
+
+/**
+ * @brief   Prevent shared timer functions from being used
+ */
+#define PERIPH_TIMER_PROVIDES_SET
+
+/**
+ * @brief   Configuration type of a timer channel
+ */
+typedef struct {
+    IRQn_Type irqn;             /** timer channel interrupt number */

```suggestion
    IRQn_Type irqn;             /**< timer channel interrupt number */
```

> +    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 */

```suggestion
    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 */
```

> +    do {
+        time_h = tmp;
+        time_l = TIMER->TIMERAWL;
+    } while ((tmp = TIMER->TIMERAWH) != time_h);

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. 

> +    DEV(dev)->TIMELW = 0; /* always write timelw before timehw */
+    DEV(dev)->TIMEHW = 0; /* writes do not get copied to time until timehw is written */

I think this should be wrapped into `irq_disable()` and `irq_restore()`, as another thread might interrupt after the first right and before the second and set the current time to a different value.

To my understanding a write to TIMELW writes into a temporary 32 bit register. And a write to TIMEHW results in both time words being updated simultaneously, the most significant word from was is written to TIMEHW and the least significant word from what was stored in the temporary register during most recent write to TIMELW.

E.g. if a second thread would set the time to 0xffffffffffffffff after one thread already perform the `DEV(dev)->TIMELW = 0;`, the temporary register would still be set to `0xffffffff` when thread calling `_timer_reset()` resumes. The `DEV(dev)->TIMEHW = 0;` would cause the time then to be set to `0x00000000ffffffff`.

> +    if (_timer_is_periodic(dev, channel)) {
+        if (_timer_reset_on_match(dev, channel)) {
+            _timer_reset(dev);
+        }
+        /* rearm */
+        *ALARM(dev, channel) = *ALARM(dev, channel);
+    }

Shouldn't this be done first in the ISR?

On most MCUs, the reset on match would be done by the hardware right after the alarm value was hit, so that e.g. a periodic timer with reset on match at 10 ms would result in an IRQ precisely every 10 ms. Since the timer of the RP2040 is not able to do this in hardware, we should emulate the reset as closely as possible to the hardware behavior. Doing the reset right at the beginning of the ISR should increase the odds that the reset is happening before the timer increments after the match.

Alternatively, one could do something like this:
1. Busy wait until the timer ticks
2. Set the timer to the value of microseconds that have passed since the alarm to compensate for the ISR duration

But this could be a bit tricky to implement correctly.

> +    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);

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?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16627#pullrequestreview-702828848
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210709/b5a4ca16/attachment.htm>


More information about the notifications mailing list