[riot-notifications] [RIOT-OS/RIOT] Introduce ATxmega CPU and Boards (#15758)

benpicco notifications at github.com
Thu Jan 28 23:34:16 CET 2021


@benpicco commented on this pull request.

Looks good!

For GPIOs you can also use the `auto_test` command from `tests/periph_gpio`.
It takes two GPIOs that you have to connect with a jumper, then runs several configurations on them and tests if the results are as expected. 



> + */
+
+#ifndef BOARD_H
+#define BOARD_H
+
+#include "cpu.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief   Clock configuration
+ * @{
+ */
+#define CLOCK_CORECLOCK     (32000000ul)

You can also use 

```suggestion
#define CLOCK_CORECLOCK     MHZ(32)
```

from `macros/units.h`

> +#define TIMER_0_MASK        ()
+#define TIMER_0_FLAG        ()

Is this used anywhere?



> +#include "cpu_clock.h"
+#include "cpu.h"
+#include "irq.h"
+#include "periph/gpio.h"
+#include "periph/init.h"
+
+#ifndef CPU_ATXMEGA_CLK_SCALE_INIT
+#define CPU_ATXMEGA_CLK_SCALE_INIT    CPU_ATXMEGA_CLK_SCALE_DIV1
+#endif
+#ifndef CPU_ATXMEGA_BUS_SCALE_INIT
+#define CPU_ATXMEGA_BUS_SCALE_INIT    CPU_ATXMEGA_BUS_SCALE_DIV1_1
+#endif
+
+void led_init(void);
+
+void clk_init(void)

This should better be moved to the cpu directory and called from `cpu_init()`



> +    /*
+     * Previous instruction takes 3 clk cycles with -Os option
+     * we need another clk cycle before we can reuse it.
+     */
+    __asm__ __volatile__ ("nop");
+}
+
+void __attribute__((weak)) board_init(void)
+{
+    clk_init();
+
+    cpu_init();
+#ifdef LED_PORT
+    led_init();
+#endif
+    irq_enable();

let `cpu_init()` take care of that too

> + * @author      Hinnerk van Bruinehsen <h.v.bruinehsen at fu-berlin.de>
+ * @author      Laurent Navet <laurent.navet at gmail.com>
+ * @author      Hauke Petersen <hauke.petersen at fu-berlin.de>
+ * @author      Dimitri Nahm <dimitri.nahm at haw-hamburg.de>
+ * @author      Josua Arndt <jarndt at ias.rwth-aachen.de>
+ * @author      Steffen Robertz <steffen.robertz at rwth-aachen.de>
+ * @author      Matthew Blue <matthew.blue.neuro at gmail.com>
+ * @author      Francisco Acosta <francisco.acosta at inria.fr>
+ * @author      Marian Buschsieweke <marian.buschsieweke at ovgu.de>

That list is getting longer than the file itself :wink: 
No need to keep the entire history when it's just a bunch of defines. 

> + * @author      Stefan Pfeiffer <stefan.pfeiffer at fu-berlin.de>
+ * @author      Hauke Petersen <hauke.petersen at fu-berlin.de>
+ * @author      Hinnerk van Bruinehsen <h.v.bruinehsen at fu-berlin.de>
+ * @author      Kaspar Schleiser <kaspar at schleiser.de>
+ * @author      Josua Arndt <jarndt at ias.rwth-aachen.de>
+ * @author      Gerson Fernando Budke <nandojve at gmail.com>

Same here.
This is less about vanity and more about who's responsible for that family and can be bothered if something doesn't work. 

> +    for (uint8_t p = 0; p < 8; p++) {
+        if (pin_mask & (1 << p)) {

I guess there is only ever one bit set in `pin_mask`, right?

```suggestion
    uint8_t p = bitarithm_lsb(pin_mask);
```

> +}
+
+static inline void irq_handler(uint8_t port_num, uint8_t isr_vct_num)
+{
+    DEBUG("irq_handler port = 0x%02x, vct_num = %d \n", port_num, isr_vct_num);
+
+    if (isr_vct_num) {
+        port_num += PORT_MAX;
+    }
+
+    if (config_ctx[port_num].cb == NULL) {
+        DEBUG("WARNING! irq_handler without callback\n");
+        return;
+    }
+
+    avr8_enter_isr();

Should't we call this at the start of `irq_handler`?

> +
+    /* enable timer with calculated prescaler */
+    ctx[tim].dev->CTRLA = ctx[tim].mode;
+    DEBUG("timer.c: prescaler set to %d \n", ctx[tim].dev->CTRLA );
+
+    return 0;
+}
+
+int timer_set_absolute(tim_t tim, int channel, unsigned int value)
+{
+    if (channel >= CHANNELS) {
+        return -1;
+    }
+    DEBUG("timer_set_absolute channel %d to %u\n", channel, value );
+
+    // Compare or Capture Disable

```suggestion
    /* Compare or Capture Disable */
```

let's keep the comment style consistent :) 

> +    avr8_exit_isr();
+}
+#endif
+
+#ifdef TIMER_0
+ISR(TIMER_0_ISRA, ISR_BLOCK)
+{
+    _isr(0, 0);
+}
+ISR(TIMER_0_ISRB, ISR_BLOCK)
+{
+    _isr(0, 1);
+}
+ISR(TIMER_0_OVF, ISR_BLOCK)
+{
+    // _isr(0, 2);

missed that one?

> +    /* default 2% precision, required precision is at least 2% */
+    if (precision <= (10000 + BAUD_TOL)) {
+        return (precision - 10000);
+    }
+
+    /* Precision goal was not met calculate baudrate with uart frequency doubled */
+    precision = _xmega_bsel_bscale(&fcpu, &baud, 1, bsel, bscale );
+    *clk2x = 1;
+
+    return (precision - 10000);
+}
+
+static inline void _configure_pins(uart_t uart)
+{
+    /* configure RX pin */
+    if (uart_config[uart].rx_pin != GPIO_UNDEF) {

```suggestion
    if (gpio_is_valid(uart_config[uart].rx_pin)) {
```

in preparation for a new GPIO API

> +    /* Precision goal was not met calculate baudrate with uart frequency doubled */
+    precision = _xmega_bsel_bscale(&fcpu, &baud, 1, bsel, bscale );
+    *clk2x = 1;
+
+    return (precision - 10000);
+}
+
+static inline void _configure_pins(uart_t uart)
+{
+    /* configure RX pin */
+    if (uart_config[uart].rx_pin != GPIO_UNDEF) {
+        gpio_init(uart_config[uart].rx_pin, GPIO_IN);
+    }
+
+    /* configure TX pin */
+    if (uart_config[uart].tx_pin != GPIO_UNDEF) {

```suggestion
    if (gpio_is_valid(uart_config[uart].tx_pin) {
```

> @@ -263,108 +297,131 @@ void avr8_exit_isr(void)
 __attribute__((always_inline)) static inline void avr8_context_save(void)
 {
     __asm__ volatile (
-        "push __tmp_reg__                    \n\t"
-        "in   __tmp_reg__, __SREG__          \n\t"
-        "cli                                 \n\t"
-        "push __tmp_reg__                    \n\t"
-#if defined(RAMPZ) && !defined(__AVR_ATmega32U4__)

Do you know why ATmega32U4 was 'special' here?
Happy to get rid of that special-casing. 

-- 
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/15758#pullrequestreview-578770911
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210128/55855401/attachment.htm>


More information about the notifications mailing list