[riot-notifications] [RIOT-OS/RIOT] cpu/nrf9160: add initial support for nRF9160DK board (#16650)

benpicco notifications at github.com
Sun Jul 18 20:36:28 CEST 2021


@benpicco commented on this pull request.

Looks good!

Did I get it right that `nrf5x_common/periph/uart.c` just works™ without further adoptions?
What about SPI and I2C? 

Btw, what's needed to make use of the LTE and GPS functionality? Are those relying on Blobs like SoftDevice?

> + * @author      Dylan Laduranty <dylan.laduranty at mesotic.com>
+ */
+
+#ifndef BOARD_H
+#define BOARD_H
+
+#include "cpu.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define CLOCK_HFCLK         (32U)           /* set to  0: internal RC oscillator
+                                             *        32: 32MHz crystal */
+#define CLOCK_LFCLK         (3)             /* set to  0: internal RC oscillator
+                                             *         3: High Accuracy oscillator */

is this an external oscillator? 

> +FEATURES_PROVIDED += periph_gpio
+FEATURES_PROVIDED += periph_gpio_irq
+FEATURES_PROVIDED += periph_timer

those are already provided by the CPU

>  FEATURES_PROVIDED += periph_uart_modecfg
-FEATURES_PROVIDED += periph_wdt periph_wdt_cb
+
+ifeq (,$(filter nrf9160 ,$(CPU_MODEL)))

```suggestion
ifneq (nrf9160,$(CPU_MODEL))
```

`CPU_MODEL` is not a list

> @@ -143,6 +153,7 @@ typedef struct {
  * @brief   Override SPI mode values
  * @{
  */
+#ifndef CPU_FAM_NRF9160

Is this necessary? 

> @@ -38,6 +38,15 @@
 #define PORT_BIT            (1 << 5)
 #define PIN_MASK            (0x1f)
 
+/* Compatibility wrapper defines for nRF9160 */
+#ifdef NRF_P0_S
+#define NRF_P0 NRF_P0_S
+#endif
+
+#ifdef NRF_GPIOTE0_S
+#define NRF_GPIOTE NRF_GPIOTE0_S
+#endif

```suggestion
#define GPIOTE_IRQn GPIOTE0_IRQn
#endif
```
saves us an `#ifdef` down the line

> +#ifdef CPU_FAM_NRF9160
+    NVIC_EnableIRQ(GPIOTE0_IRQn);
+#else

with the added compat define we can drop this 

> @@ -23,8 +23,14 @@
 
 #include "cpu.h"
 
+#ifdef NRF_POWER_S
+#define NRF_POWER NRF_POWER_S
+#endif
+
+/* TODO: implement proper pm_off for nRF9160 */

Is this even compiled if `periph_pm` is not advertised as present? 

> +ifeq ($(PROGRAMMER),jlink)
+  JLINK_DEVICE = NRF9160_XXAA
+endif

We can set `JLINK_DEVICE` unconditionally 

> +
+#include "vendor/nrf9160.h"
+#include "vendor/nrf9160_bitfields.h"
+#include "vendor/nrf9160_peripherals.h"
+
+#include "cpu_conf_common.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name    ARM Cortex-M specific CPU configuration
+ * @{
+ */
+#define CPU_DEFAULT_IRQ_PRIO            (2U)

all other CPUs set this to 1, I think this is a leftover from SoftDevice. 

> + * @author          Dylan Laduranty <dylan.laduranty at mesotic.com>
+ */
+
+#ifndef PERIPH_CPU_H
+#define PERIPH_CPU_H
+
+#include "periph_cpu_common.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief   System core clock speed, fixed to 64MHz for all NRF9160 CPUs
+ */
+#define CLOCK_CORECLOCK     (64000000U)

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

You may have to `#include macros/units.h` for this

> +
+#include "periph_cpu_common.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief   System core clock speed, fixed to 64MHz for all NRF9160 CPUs
+ */
+#define CLOCK_CORECLOCK     (64000000U)
+
+/**
+ * @name    Peripheral clock speed (fixed to 16MHz for nRF9160 based CPUs)
+ */
+#define PERIPH_CLOCK        (16000000U)

```suggestion
#define PERIPH_CLOCK        MHZ(16)
```

-- 
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/16650#pullrequestreview-709050321
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210718/7ae37881/attachment.htm>


More information about the notifications mailing list