[riot-notifications] [RIOT-OS/RIOT] drivers/periph/ptp_clock (#15074)

benpicco notifications at github.com
Wed Nov 18 00:07:58 CET 2020


@benpicco commented on this pull request.

Looks good, but I'm a bit confused about `PTPTTHR` being a 32 bit register, yet you store `seconds` in a `uint64_t` - why go through all the trouble with 64 bit arithmetic if the hardware is going to throw them all away? 

> +#include "periph_cpu.h"
+#include "timex.h"
+
+#define ENABLE_DEBUG        (0)
+#include "debug.h"
+
+/* Workaround for typos in vendor files; drop when fixed upstream */
+#ifndef ETH_PTPTSCR_TSSSR
+#define ETH_PTPTSCR_TSSSR ETH_PTPTSSR_TSSSR
+#endif
+#ifndef ETH_PTPTSCR_TSSARFE
+#define ETH_PTPTSCR_TSSARFE ETH_PTPTSSR_TSSARFE
+#endif
+
+/* The PTP uses the same clock domain as the CPU */
+#define HCLK                CLOCK_CORECLOCK

Is this another alias for `CLOCK_AHB`?

> +#endif
+#ifndef ETH_PTPTSCR_TSSARFE
+#define ETH_PTPTSCR_TSSARFE ETH_PTPTSSR_TSSARFE
+#endif
+
+/* The PTP uses the same clock domain as the CPU */
+#define HCLK                CLOCK_CORECLOCK
+
+/* PTPSSIR is the number of nanoseconds to add onto the sub-second register
+ * (the one counting the nanoseconds part of the timestamp with the
+ * configuration we chose here). It is therefore the resolution of the clock
+ * in nanoseconds. (Note that the accuracy is expected to be way worse than
+ * the resolution.)
+ */
+#ifndef STM32_PTPSSIR
+#if CLOCK_CORECLOCK > 200000000

```suggestion
#if CLOCK_CORECLOCK > MHZ(200) 
```

> +#endif
+
+/* The PTP uses the same clock domain as the CPU */
+#define HCLK                CLOCK_CORECLOCK
+
+/* PTPSSIR is the number of nanoseconds to add onto the sub-second register
+ * (the one counting the nanoseconds part of the timestamp with the
+ * configuration we chose here). It is therefore the resolution of the clock
+ * in nanoseconds. (Note that the accuracy is expected to be way worse than
+ * the resolution.)
+ */
+#ifndef STM32_PTPSSIR
+#if CLOCK_CORECLOCK > 200000000
+/* Go for 10 ns resolution on CPU clocked higher than 200 MHz */
+#define STM32_PTPSSIR       (10LLU)
+#elif CLOCK_CORECLOCK > 100000000

```suggestion
#elif CLOCK_CORECLOCK > MHZ(100)
```

> +/**
+ * @brief   Return the result of x / y, scientifically rounded
+ * @param   x       Number to divide
+ * @param   y       @p x should be divided by this
+ * @return  x/y, scientifically rounded
+ * @pre     Both @p x and @p y are compile time constant integers and the
+ *          expressions are evaluated without side-effects
+ */
+#define ROUNDED_DIV(x, y)   (((x) + ((y) / 2)) / (y))
+
+static const uint32_t ptpssir = STM32_PTPSSIR;
+static const uint32_t ptptsar = ROUNDED_DIV(NS_PER_SEC * (1ULL << 32), HCLK * STM32_PTPSSIR);
+
+void ptp_init(void)
+{
+    stm32_eth_common_init();

Maybe add a comment that the `eth` part will not call this if the PTP clock is used.
Are we sure `ptp_init()` is always called before `stm32_eth_init()`? 

> +        timestamp->seconds = ETH->PTPTSHR;
+        timestamp->nanoseconds = ETH->PTPTSLR;
+    } while (timestamp->seconds != ETH->PTPTSHR);
+
+    /* TODO: Most significant bit of ETH->PTPTSLR is the sign bit of the time
+     * stamp. Because the seconds register is unsigned, an overflow is not
+     * expected before year 2106. It is not clear from the data sheet, how the
+     * time stamp is to be interpreted when the negative bit is set. For now,
+     * we just ignore this potential source of problems. */
+    irq_restore(irq_state);
+}
+
+#if IS_USED(MODULE_PERIPH_PTP_TIMER)
+void ptp_timer_clear(void)
+{
+    /* TODO: Use atomic utils, once merged */

it's merged now, right? 

> +    assert(target);
+     DEBUG("[periph_ptp] Set timer: %" PRIu32 ".%" PRIu32 "\n",

whitespace ;) 

> +        ETH->PTPTSCR &= ~ETH_PTPTSCR_TSITE;
+        irq_restore(state);
+    }
+}
+
+void ptp_timer_set_absolute(const ptp_timestamp_t *target)
+{
+    assert(target);
+     DEBUG("[periph_ptp] Set timer: %" PRIu32 ".%" PRIu32 "\n",
+           (uint32_t)target->seconds, target->nanoseconds);
+    unsigned state = irq_disable();
+    /* Mask PTP timer IRQ first, so that an interrupt is not triggered
+     * too early. (The target time is not set atomically.) */
+    ETH->MACIMR |= ETH_MACIMR_TSTIM;
+    /* Set target time */
+    ETH->PTPTTHR = target->seconds;

`PTPTTHR` is only 32 bit, right? 

-- 
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/15074#pullrequestreview-532878286
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201117/e3535b8c/attachment-0001.htm>


More information about the notifications mailing list