cpu/nrf9160: add twi and spi support by dylad · Pull Request #16814 · RIOT-OS/RIOT

dylad · 2021-09-05T20:26:41Z

Contribution description

This PR adds support for I2C and SPI for nRF9160 MCUs.
Since these IPs are the same as nRF52. I moved SPI and I2C code to nrf5x_common (I did not touch nRF51 drivers). And I also recreate a SPI/I2C irq handler to match the behaviour of nrf52 platform.

It would be good to ensure nothing is broken on nRF52 platform with this PR but I don't have the hardware with me for now.

Testing procedure

on nRF52 based board and nRF9160dk, use tests/periph_i2c and tests/periph_spi to test.

I provide some output from my custom nRF9160 based board:

tests/driver_hts221:

2021-09-05 22:14:24,606 # main(): This is RIOT! (Version: 2021.04-devel-2410-gc333e-pr/cpu/nrf9160_add_twi_and_spi_support)
2021-09-05 22:14:24,607 # Init HTS221 on I2C_DEV(0)
2021-09-05 22:14:24,664 # H: 63.0%, T: 37.0°C
2021-09-05 22:14:26,704 # H: 62.7%, T: 29.1°C
2021-09-05 22:14:28,743 # H: 63.4%, T: 29.1°C
2021-09-05 22:14:30,782 # H: 63.3%, T: 29.2°C
2021-09-05 22:14:32,820 # H: 62.9%, T: 29.2°C
2021-09-05 22:14:34,860 # H: 62.6%, T: 29.2°C
2021-09-05 22:14:36,899 # H: 62.2%, T: 29.3°C
2021-09-05 22:14:38,938 # H: 61.9%, T: 29.3°C
2021-09-05 22:14:40,976 # H: 61.6%, T: 29.4°C
2021-09-05 22:14:43,016 # H: 61.2%, T: 29.4°C
2021-09-05 22:14:45,054 # H: 60.9%, T: 29.3°C

tests/mtd_raw:

2021-09-05 22:13:34,852 # reboot
2021-09-05 22:13:34,859 # main(): This is RIOT! (Version: 2021.04-devel-2408-ga599-pr/cpu/nrf9160_add_twi_and_spi_support)
2021-09-05 22:13:34,860 # Manual MTD test
2021-09-05 22:13:34,862 # init MTD_0… OK (16 MiB)
> test 0
2021-09-05 22:13:37,203 # test 0
2021-09-05 22:13:37,204 # [START]
2021-09-05 22:13:37,423 # [SUCCESS]

Issues/PRs references

None.

boards/nrf9160dk/include/periph_conf.h

cpu/nrf5x_common/periph/Makefile

cpu/nrf9160/spi_twi_irq.c

boards/nrf9160dk/include/periph_conf.h

benpicco

Looks good, please squash!

benpicco · 2021-09-06T15:42:53Z

I think it would make sense to merge cpu/nrf52 and cpu/nrf9160.
Isn't the nrf9160 just a nrf52 with a LTE modem attached?

dylad · 2021-09-06T16:23:31Z

Not really. nRF9160 is a cortex-m33 but they share the same peripherals with a few difference.

dylad · 2021-09-06T16:28:00Z

I would like someone to check if this PR didn't not break something in nRF52 SPI and I2C before merging if possible.
I don't have the required hardware with me.

benpicco · 2021-09-06T16:51:28Z

Sure, let me run tests/mtd_raw (for SPI flash) with USEMODULE += i2c_scan on nrf52840dk:

> 2021-09-06 18:49:40,590 # main(): This is RIOT! (Version: 2021.10-devel-548-ge52b5-HEAD)
2021-09-06 18:49:40,591 # Manual MTD test
2021-09-06 18:49:40,593 # init MTD_0… OK (8 MiB)
> info
2021-09-06 18:50:12,875 # info
2021-09-06 18:50:12,876 # mtd devices: 1
2021-09-06 18:50:12,877 #  -=[ MTD_0 ]=-
2021-09-06 18:50:12,878 # sectors: 2048
2021-09-06 18:50:12,880 # pages per sector: 16
2021-09-06 18:50:12,882 # page size: 256
2021-09-06 18:50:12,883 # total: 8 MiB
> test 0
2021-09-06 18:52:55,650 # test 0
2021-09-06 18:52:55,650 # [START]
2021-09-06 18:52:55,907 # [SUCCESS]

I attached a AT24C EEPROM to the I2C bus

> i2c_scan 0
2021-09-06 18:50:15,627 # i2c_scan 0
2021-09-06 18:50:15,630 # Scanning I2C device 0...
2021-09-06 18:50:15,636 # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2021-09-06 18:50:15,639 #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2021-09-06 18:50:15,643 # 0x00 R R R R R R R R R R R R R R - -
2021-09-06 18:50:15,648 # 0x10 - - - - - - - - - - - - - - - -
2021-09-06 18:50:15,653 # 0x20 - - - - - - - - - - - - - - - -
2021-09-06 18:50:15,659 # 0x30 - - - - - - - - - - - - - - - -
2021-09-06 18:50:15,665 # 0x40 - - - - - - - - - - - - - - - -
2021-09-06 18:50:15,670 # 0x50 X - - - - - - - - - - - - - - -
2021-09-06 18:50:15,676 # 0x60 - - - - - - - - - - - - - - - -
2021-09-06 18:50:15,680 # 0x70 - - - - - - - - R R R R R R R R

still works!

dylad · 2021-09-06T18:22:40Z

Squashed, thanks for testing on nRF52 platform !

benpicco · 2021-09-06T20:15:32Z

hm looks like you need something like

--- a/pkg/nimble/Makefile.dep
+++ b/pkg/nimble/Makefile.dep
@@ -42,7 +42,7 @@ endif
 # nimble controller dependencies
 ifneq (,$(filter nimble_controller,$(USEMODULE)))
   USEMODULE += nimble_transport_ram
-  ifneq (,$(filter nrf5%,$(CPU_FAM)))
+  ifneq (,$(filter nrf9160 nrf5%,$(CPU_FAM)))
     USEMODULE += nimble_drivers_nrf5x
   endif
 endif

fjmolinas · 2021-09-07T06:56:51Z

hm looks like you need something like

--- a/pkg/nimble/Makefile.dep
+++ b/pkg/nimble/Makefile.dep
@@ -42,7 +42,7 @@ endif
 # nimble controller dependencies
 ifneq (,$(filter nimble_controller,$(USEMODULE)))
   USEMODULE += nimble_transport_ram
-  ifneq (,$(filter nrf5%,$(CPU_FAM)))
+  ifneq (,$(filter nrf9160 nrf5%,$(CPU_FAM)))
     USEMODULE += nimble_drivers_nrf5x
   endif
 endif

Hmm no you would need more than that, this is based on the fact that mynewt-nimble needs to initialize nrf5 timers see https://github.com/apache/mynewt-nimble/blob/master/porting/nimble/src/hal_timer.c, and so we do the same when using mynewt-core for this. But AFAIKT in nimble this is only for nrf52 so no guarantees this works for nrf9*. Please apply this patch instead:

diff --git a/pkg/nimble/Makefile.dep b/pkg/nimble/Makefile.dep
index 07d4caf282..45e8d47ec8 100644
--- a/pkg/nimble/Makefile.dep
+++ b/pkg/nimble/Makefile.dep
@@ -18,7 +18,9 @@ USEMODULE += nimble_porting_nimble
 ifneq (,$(filter mynewt-core,$(USEPKG)))
   USEMODULE += mynewt-core_os
   USEMODULE += mynewt-core_util
-  USEMODULE += mynewt-core_nrf5x_hal
+  ifneq (,$(filter nrf5%,$(CPU)))
+    USEMODULE += mynewt-core_nrf5x_hal
+  endif
 else
   # uwb-% requires mynewt-core so is incompatible with nimble_npl_riot
   ifeq (,$(filter uwb%,$(USEPKG)))
diff --git a/pkg/uwb-core/Makefile.dep b/pkg/uwb-core/Makefile.dep
index b4f0c0355a..fd59855a7a 100644
--- a/pkg/uwb-core/Makefile.dep
+++ b/pkg/uwb-core/Makefile.dep
@@ -25,7 +25,7 @@ endif
 ifneq (,$(filter uwb-core_dpl,$(USEMODULE)))
   USEPKG += mynewt-core
   USEMODULE += mynewt-core_os
-  ifneq (,$(filter nrf%,$(CPU)))
+  ifneq (,$(filter nrf5%,$(CPU)))
     USEMODULE += mynewt-core_nrf5x_hal
   endif
 endif

dylad · 2021-09-07T06:59:52Z

Wait wait wait, why should nimble even tries to compile on nRF9160 ? It doesn't have a bluetooth radio !

fjmolinas · 2021-09-07T07:04:37Z

Wait wait wait, why should nimble even tries to compile on nRF9160 ? It doesn't have a bluetooth radio !

Then its unrelated, it just appplies to uwb-core, you can apply the patch there only.

dylad · 2021-09-07T07:07:52Z

you can apply the patch there only.

Sadly, this is not enough. I've got new errors then.
I'll look further later today.

fjmolinas · 2021-09-07T07:12:08Z

you can apply the patch there only.

Sadly, this is not enough. I've got new errors then.
I'll look further later today.

I'll find the fix, this is a bad assumption on my side with the pkg. This should do:

diff --git a/pkg/mynewt-core/contrib/Makefile b/pkg/mynewt-core/contrib/Makefile
index 0bb65a903f..c16f097dad 100644
--- a/pkg/mynewt-core/contrib/Makefile
+++ b/pkg/mynewt-core/contrib/Makefile
@@ -3,7 +3,7 @@ MODULE = mynewt-core
 # exclude submodule sources from *.c wildcard source selection
 SRC := $(filter-out nrf5x_isr.c cputime.c,$(wildcard *.c))
 
-ifneq (,$(filter nrf%,$(CPU)))
+ifneq (,$(filter nrf5%,$(CPU)))
   SRC += nrf5x_isr.c
 else
   SRC += cputime.c
diff --git a/pkg/mynewt-core/mynewt-core_os.mk b/pkg/mynewt-core/mynewt-core_os.mk
index 8077bdeabe..6bfbccbb1b 100644
--- a/pkg/mynewt-core/mynewt-core_os.mk
+++ b/pkg/mynewt-core/mynewt-core_os.mk
@@ -8,7 +8,7 @@ SRC := \
   os_cputime_pwr2.c \
   #
 
-ifneq (,$(filter nrf%,$(CPU)))
+ifneq (,$(filter nrf5%,$(CPU)))
   SRC += os_cputime.c
 endif
 
diff --git a/pkg/uwb-core/Makefile.dep b/pkg/uwb-core/Makefile.dep
index b4f0c0355a..fd59855a7a 100644
--- a/pkg/uwb-core/Makefile.dep
+++ b/pkg/uwb-core/Makefile.dep
@@ -25,7 +25,7 @@ endif
 ifneq (,$(filter uwb-core_dpl,$(USEMODULE)))
   USEPKG += mynewt-core
   USEMODULE += mynewt-core_os
-  ifneq (,$(filter nrf%,$(CPU)))
+  ifneq (,$(filter nrf5%,$(CPU)))
     USEMODULE += mynewt-core_nrf5x_hal
   endif
 endif

dylad · 2021-09-07T07:15:51Z

@fjmolinas Your last patch did the trick ! Thanks for your help :)
Should we apply this patch as a separate commit in this PR or open another as this is unrelated ?

fjmolinas · 2021-09-07T07:19:22Z

@fjmolinas Your last patch did the trick ! Thanks for your help :)
Should we apply this patch as a separate commit in this PR or open another as this is unrelated ?

Separete commit is fine IMO

As nRF9160 doesn't have a bluetooth radio, we should not pull the same dependencies as nRF52, thus ensure these dependencies are only added for nrf5x families

dylad · 2021-09-08T07:31:23Z

any last words for this one ?

tests/periph_spi: printing and testing SPI clock rates drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904) drivers/periph_spi: add the `bus` parameter to spi_get_*() This was necessary for implementations where multiple devices can have different clock sources. This broke the macros SPI_CLK_* that were reverted to an enum. periph/spi: adapted to the new API Arbitrary speed support was added to all implementations where it was missing except kinetis. nrf52/periph/spi: moved in nfr5x_common (RIOT-OS#16814)

tests/periph_spi: printing and testing SPI clock rates drivers/periph_spi: change API of spi_acquire (from RIOT-OS#15904) drivers/periph_spi: add the `bus` parameter to spi_get_*() This was necessary for implementations where multiple devices can have different clock sources. This broke the macros SPI_CLK_* that were reverted to an enum. periph/spi: adapted to the new API Arbitrary speed support was added to all implementations where it was missing. nrf52/periph/spi: moved in nfr5x_common (RIOT-OS#16814)

dylad requested review from aabadie, cgundogan, haukepetersen, jia200x and leandrolanzieri as code owners September 5, 2021 20:26

github-actions bot added Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Sep 5, 2021

dylad force-pushed the pr/cpu/nrf9160_add_twi_and_spi_support branch from 393fcdf to 87c732d Compare September 5, 2021 20:38

benpicco reviewed Sep 6, 2021

View reviewed changes

boards/nrf9160dk/include/periph_conf.h Outdated Show resolved Hide resolved

boards/nrf9160dk/include/periph_conf.h Outdated Show resolved Hide resolved

cpu/nrf5x_common/periph/Makefile Show resolved Hide resolved

cpu/nrf9160/spi_twi_irq.c Outdated Show resolved Hide resolved

benpicco reviewed Sep 6, 2021

View reviewed changes

boards/nrf9160dk/include/periph_conf.h Outdated Show resolved Hide resolved

benpicco approved these changes Sep 6, 2021

View reviewed changes

dylad added 2 commits September 6, 2021 20:15

cpu/nrf9160: add TWI and SPI support

1beda0f

boards/nrf9160dk: add TWI and SPI configuration

0c60a2a

dylad force-pushed the pr/cpu/nrf9160_add_twi_and_spi_support branch from e52b537 to 0c60a2a Compare September 6, 2021 18:21

dylad added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 6, 2021

dylad added 2 commits September 7, 2021 09:27

pkg/mynewt-core: don't pull unwanted dependencies for nRF9160

ed655ab

As nRF9160 doesn't have a bluetooth radio, we should not pull the same dependencies as nRF52, thus ensure these dependencies are only added for nrf5x families

pkg/uwb-core: don't pull unwanted dependencies for nRF9160

633ddee

As nRF9160 doesn't have a bluetooth radio, we should not pull the same dependencies as nRF52, thus ensure these dependencies are only added for nrf5x families

github-actions bot added Area: build system Area: Build system Area: pkg Area: External package ports labels Sep 7, 2021

benpicco merged commit bcaaaec into RIOT-OS:master Sep 8, 2021

dylad deleted the pr/cpu/nrf9160_add_twi_and_spi_support branch September 8, 2021 17:50

hugueslarrive added a commit to hugueslarrive/RIOT that referenced this pull request Sep 11, 2021

nrf52/periph/spi: moved in nfr5x_common (RIOT-OS#16814)

54e555a

benpicco added this to the Release 2021.10 milestone Oct 20, 2021

Conversation

dylad commented Sep 5, 2021

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco commented Sep 6, 2021

Uh oh!

dylad commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylad commented Sep 6, 2021

Uh oh!

benpicco commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylad commented Sep 6, 2021

Uh oh!

benpicco commented Sep 6, 2021

Uh oh!

fjmolinas commented Sep 7, 2021

Uh oh!

dylad commented Sep 7, 2021

Uh oh!

fjmolinas commented Sep 7, 2021

Uh oh!

dylad commented Sep 7, 2021

Uh oh!

fjmolinas commented Sep 7, 2021

Uh oh!

dylad commented Sep 7, 2021

Uh oh!

fjmolinas commented Sep 7, 2021

Uh oh!

dylad commented Sep 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dylad commented Sep 6, 2021 •

edited

Loading

benpicco commented Sep 6, 2021 •

edited

Loading