[riot-notifications] [RIOT-OS/RIOT] boards/nucleo-wl55jc : Initial support (#16255)

Francisco notifications at github.com
Wed Apr 7 17:06:16 CEST 2021


@fjmolinas commented on this pull request.

Some comments. I also noticed that the clock configuration for the `stm32wl` is not added to `Kconfig.clk`, do you think that can be added?

> @@ -236,7 +238,7 @@ int gpio_init_int(gpio_t pin, gpio_mode_t mode, gpio_flank_t flank,
     isr_ctx[pin_num].arg = arg;
 
     /* enable clock of the SYSCFG module for EXTI configuration */
-#if !defined(CPU_FAM_STM32WB) && !defined(CPU_FAM_STM32MP1)
+#if !defined(CPU_FAM_STM32WB) && !defined(CPU_FAM_STM32MP1) && !defined(CPU_FAM_STM32WL)

```suggestion
#if !defined(CPU_FAM_STM32WB) && !defined(CPU_FAM_STM32MP1) && \
    !defined(CPU_FAM_STM32WL)
```

> @@ -49,7 +49,7 @@
 #elif defined(CPU_FAM_STM32L4) || defined(CPU_FAM_STM32G4) || \
       defined(CPU_FAM_STM32L5)
 #define PM_STOP_CONFIG  (PWR_CR1_LPMS_STOP1)
-#elif defined(CPU_FAM_STM32WB) || defined(CPU_FAM_STM32G0)
+#elif defined(CPU_FAM_STM32WB) || defined(CPU_FAM_STM32G0) || defined(CPU_FAM_STM32WL)

```suggestion
#elif defined(CPU_FAM_STM32WB) || defined(CPU_FAM_STM32G0) || \
      defined(CPU_FAM_STM32WL)
```

> @@ -71,7 +71,7 @@
 #elif defined(CPU_FAM_STM32L4) || defined(CPU_FAM_STM32G4) || \
       defined(CPU_FAM_STM32L5)
 #define PM_STANDBY_CONFIG   (PWR_CR1_LPMS_STANDBY)
-#elif defined(CPU_FAM_STM32WB) || defined(CPU_FAM_STM32G0)
+#elif defined(CPU_FAM_STM32WB) || defined(CPU_FAM_STM32G0) || defined(CPU_FAM_STM32WL)

```suggestion
#elif defined(CPU_FAM_STM32WB) || defined(CPU_FAM_STM32G0) || \
      defined(CPU_FAM_STM32WL)
```

> @@ -8,7 +8,7 @@
 #  - STM32_PINCOUNT: R (64)
 #  - STM32_ROMSIZE: G (1024K)
 CPU_MODEL_UPPERCASE = $(call uppercase,$(CPU_MODEL))
-STM32_INFO     := $(shell echo $(CPU_MODEL_UPPERCASE) | sed -E -e 's/^STM32(F|L|W|G|MP)([0-7]|B)([A-Z0-9])([0-9])(.)(.)?(_A)?/\1 \2 \2\3\4 \3 \4 \5 \6 \7/')
+STM32_INFO := $(shell echo $(CPU_MODEL_UPPERCASE) | sed -E -e 's/^STM32(F|L|W|G|MP)([0-7]|B|L)([A-Z0-9])([0-9])(.)(.)?(_A)?/\1 \2 \2\3\4 \3 \4 \5 \6 \7/')

```suggestion
STM32_INFO     := $(shell echo $(CPU_MODEL_UPPERCASE) | sed -E -e 's/^STM32(F|L|W|G|MP)([0-7]|B|L)([A-Z0-9])([0-9])(.)(.)?(_A)?/\1 \2 \2\3\4 \3 \4 \5 \6 \7/')
```

I don't see why change the alignment,

> +#if defined(CPU_FAM_STM32WL)
+        RCC->CR |= (RCC_CR_HSEBYPPWR);
+#endif

Can you explain?

> @@ -0,0 +1,3 @@
+FEATURES_REQUIRED += periph_lpuart

Why required? I would remove this.

> +
+## Flashing the device
+
+The ST Nucleo-wl55jc board includes an on-board ST-LINK V2 programmer. The
+easiest way to program the board is to use OpenOCD. Once you have installed
+OpenOCD (look [here](https://github.com/RIOT-OS/RIOT/wiki/OpenOCD) for
+installation instructions), you can flash the board simply by typing
+
+```
+make BOARD=nucleo-wl55jc flash
+```
+and debug via GDB by simply typing
+```
+make BOARD=nucleo-wl55jc debug
+```
+

```suggestion
```

> +#define LED1_PORT           GPIOB
+#define LED1_PIN            GPIO_PIN(PORT_B, 9)
+#define LED1_MASK           (1 << 9)
+#define LED1_ON             (LED0_PORT->BSRR = LED1_MASK)
+#define LED1_OFF            (LED0_PORT->BSRR = (LED1_MASK << 16))
+#define LED1_TOGGLE         (LED0_PORT->ODR  ^= LED1_MASK)
+
+#define LED2_PORT           GPIOB
+#define LED2_PIN            GPIO_PIN(PORT_B, 11)
+#define LED2_MASK           (1 << 11)
+#define LED2_ON             (LED0_PORT->BSRR = LED2_MASK)
+#define LED2_OFF            (LED0_PORT->BSRR = (LED2_MASK << 16))
+#define LED2_TOGGLE         (LED0_PORT->ODR  ^= LED2_MASK)
+/** @} */
+
+/* p-nucleo-wb55 always use LED0, as there is no dual use of its pin */

```suggestion
/* nucleo-wl55jc always use LED0, as there is no dual use of its pin */
```

> +/**
+ * @name    Timer configuration
+ * @{
+ */
+static const timer_conf_t timer_config[] = {
+    {
+        .dev      = TIM2,
+        .max      = 0xffffffff,
+        .rcc_mask = RCC_APB1ENR1_TIM2EN,
+        .bus      = APB1,
+        .irqn     = TIM2_IRQn
+    }
+};
+
+#define TIMER_0_ISR         isr_tim2
+
+#define TIMER_NUMOF         ARRAY_SIZE(timer_config)
+/** @} */

```suggestion
 #include "cfg_timer_tim2.h"
/** @} */
```

> +#define CONFIG_CLOCK_APB1_DIV           1
+#define CONFIG_CLOCK_APB2_DIV           1

If `CONFIG` maybe add `ifndef` guards?

> @@ -10,8 +10,8 @@ else ifneq (,$(filter $(CPU_FAM),f0 f1 f3))
   SRC += stmclk_f0f1f3.c
 else ifneq (,$(filter $(CPU_FAM),l0 l1))
   SRC += stmclk_l0l1.c
-else ifneq (,$(filter $(CPU_FAM),l4 wb))
-  SRC += stmclk_l4wb.c
+else ifneq (,$(filter $(CPU_FAM),l4 wb wl))
+  SRC += stmclk_l4wbwl.c

I would rename to `stmcli_l4wx`

> +/* Add specific clock configuration (HSE, LSE) for this board here */
+#ifndef CONFIG_BOARD_HAS_LSE
+#define CONFIG_BOARD_HAS_LSE            1
+#endif
+
+/* This board provides a 32MHz HSE oscillator  */
+#ifndef CONFIG_BOARD_HAS_HSE
+#define CONFIG_BOARD_HAS_HSE            1
+#endif
+
+#define CLOCK_HSE                       MHZ(32)
+
+/* VCO output frequency ((PLL input clock frequency / PLLM ) x PLLN ) must be
+   between 96 and 344 MHz. PLLN can have values <=127 & >=6 */
+/* use a core clock of 48MHz and run APBx buses at the same speed */
+#define CONFIG_CLOCK_PLL_N              18

The `CPU_FAM_STM32WB` defines some things like `CLOCK_CORECLOCK_MAX` in `cpu/stm32/include/clk/l4l5wb/cfg_clock_default.h`, can you adapt and/or adapt the equivalent? Also since you are using that file already could you maybe move this definition there? That was its already `infdef` guarded.

-- 
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/16255#pullrequestreview-630104648
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210407/460d40cb/attachment-0001.htm>


More information about the notifications mailing list