Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpu/nrf53: add I2C and SPI support #19798

Merged
merged 7 commits into from
Jul 13, 2023

Conversation

dylad
Copy link
Member

@dylad dylad commented Jul 5, 2023

Contribution description

This PR provides support for nRF53 SPI and I2C.
It also moves common structs from each nRF CPU folder to cpu/nrf5x_common to avoid duplication.
Moreover, since nRF9160 and nRF5340 have shared IRQ for UART/SPI/I2C. Both this families now use a common file to register and manage these interrupts. Note that nRF9160 have different name for its interrupts than nRF5340 but they have the same purpose.

Testing procedure

Since some structs were moved around, I think this PR should be carefully tested against nRF52, nRF53 and nRF9160 to avoid any issues.
On nRF5340DK-APP, SPI can be tested with its onboard SPI flash.

Issues/PRs references

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration labels Jul 5, 2023
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 5, 2023
@riot-ci
Copy link

riot-ci commented Jul 6, 2023

Murdock results

✔️ PASSED

883d138 pkg: don't align nrf53 features on nrf51/nrf52

Success Failures Total Runtime
6936 0 6936 10m:48s

Artifacts

@dylad dylad force-pushed the cpu/nrf53/add_spi_i2c_support branch from d715883 to e846df8 Compare July 7, 2023 07:48

#define BOARD_QSPI_PIN_CS GPIO_PIN(0, 18)
#define BOARD_QSPI_PIN_WP GPIO_PIN(0, 15)
#define BOARD_QSPI_PIN_HOLD GPIO_PIN(0, 16)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doccheck wants this documented 😕

@benpicco
Copy link
Contributor

benpicco commented Jul 7, 2023

I can confirm that SPI and I2C are still working fine on nRF52(840) - please squash directly

@dylad dylad force-pushed the cpu/nrf53/add_spi_i2c_support branch from e846df8 to 36fffe2 Compare July 8, 2023 15:10
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doccheck is still not happy - the /** @} */ on nrf5340dk-app/include/periph_conf.h:115 has no matching partner

cpu/nrf5x_common/include/periph_cpu_common.h Outdated Show resolved Hide resolved
@dylad dylad force-pushed the cpu/nrf53/add_spi_i2c_support branch from 36fffe2 to 02854cb Compare July 10, 2023 14:06
@dylad
Copy link
Member Author

dylad commented Jul 10, 2023

Rebased.
@benpicco I'll your last comments a bit later today and re-run some tests before.

@dylad dylad force-pushed the cpu/nrf53/add_spi_i2c_support branch 3 times, most recently from 8484fc1 to f1c3476 Compare July 11, 2023 08:40
@dylad
Copy link
Member Author

dylad commented Jul 11, 2023

I've retried I2C with bme680 sensor on a custom board:


> Help: Press s to start test, r to print it is ready
START
main(): This is RIOT! (Version: 2023.07-devel-827-geb5be-cpu/nrf53/add_spi_i2c_support)
Initialize BME680 sensor 0 ... OK
[bme680]: dev=0, T = 27.45 degC, P = 100670 Pa, H = 45.124 %, G = 3356293 ohms
+-----------------------------------------+
[bme680]: dev=0, T = 27.45 degC, P = 100670 Pa, H = 44.985 %, G = 2196721 ohms
+-----------------------------------------+
[bme680]: dev=0, T = 27.45 degC, P = 100672 Pa, H = 44.895 %, G = 1618357 ohms
+-----------------------------------------+
[bme680]: dev=0, T = 27.44 degC, P = 100674 Pa, H = 44.859 %, G = 1290944 ohms
+-----------------------------------------+
[bme680]: dev=0, T = 27.43 degC, P = 100670 Pa, H = 44.841 %, G = 1067729 ohms
+-----------------------------------------+
[bme680]: dev=0, T = 27.42 degC, P = 100672 Pa, H = 44.832 %, G = 909708 ohms
+-----------------------------------------+
[bme680]: dev=0, T = 27.41 degC, P = 100672 Pa, H = 44.861 %, G = 784084 ohms
+-----------------------------------------+
[bme680]: dev=0, T = 27.40 degC, P = 100672 Pa, H = 44.904 %, G = 686493 ohms
+-----------------------------------------+
[bme680]: dev=0, T = 27.39 degC, P = 100670 Pa, H = 44.949 %, G = 609591 ohms

Also tested tests/sys/vfs_default on nrf5340dk-app and it was working fine.

@dylad
Copy link
Member Author

dylad commented Jul 11, 2023

@benpicco could you confirm that I didn't break anything again on nRF52 please ?

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nRF52 is still working fine

@benpicco
Copy link
Contributor

bors merge

bors bot added a commit that referenced this pull request Jul 11, 2023
19777: cpu/avr8_common: Prepare for rework ISR r=benpicco a=nandojve

### Contribution description

This prepares for rework how ISR is handled for AVR-8 platform. It is not expected changes on the behavior but tests on other boards were welcome to avoid regressions.

#### Improvements
 * Split UART state from ISR states. Now it is necessary two variables and GPIORx registers are automatically selected when available.
 * UART states now supports up to 8 UARTs.
 * Added AVR8_ISR macro do clean-up and hide internals related to ISR processing. This allows changes on ISR without any other changes on drivers.

### Testing procedure

Tests were conducted using atmega328p-xplained-mini and atxmega-a1u-xpro and the zigduino board was only built. The example thread_duel was used to test regressions.

19798: cpu/nrf53: add I2C and SPI support r=benpicco a=dylad

### Contribution description

This PR provides support for nRF53 SPI and I2C.
It also moves common structs from each nRF CPU folder to `cpu/nrf5x_common` to avoid duplication.
Moreover, since nRF9160 and nRF5340 have shared IRQ for UART/SPI/I2C. Both this families now use a common file to register and manage these interrupts. Note that nRF9160 have different name for its interrupts than nRF5340 but they have the same purpose.

### Testing procedure

Since some structs were moved around, I think this PR should be carefully tested against nRF52, nRF53 and nRF9160 to avoid any issues.
On nRF5340DK-APP, SPI can be tested with its onboard SPI flash.

### Issues/PRs references



Co-authored-by: Gerson Fernando Budke <[email protected]>
Co-authored-by: Dylan Laduranty <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 11, 2023

Build failed (retrying...):

@dylad
Copy link
Member Author

dylad commented Jul 11, 2023

Damn, I'll provide an I2C configuration for nrf5340dk-app...

@dylad dylad force-pushed the cpu/nrf53/add_spi_i2c_support branch from f1c3476 to e65f245 Compare July 11, 2023 12:43
@dylad dylad force-pushed the cpu/nrf53/add_spi_i2c_support branch from 1e32634 to fbb3b2c Compare July 11, 2023 21:43
@dylad
Copy link
Member Author

dylad commented Jul 12, 2023

bors try

bors bot added a commit that referenced this pull request Jul 12, 2023
@bors
Copy link
Contributor

bors bot commented Jul 12, 2023

try

Build failed:

@dylad dylad force-pushed the cpu/nrf53/add_spi_i2c_support branch from fbb3b2c to b050ad3 Compare July 12, 2023 19:52
@benpicco
Copy link
Contributor

bors merge

bors bot added a commit that referenced this pull request Jul 12, 2023
19798: cpu/nrf53: add I2C and SPI support r=benpicco a=dylad

### Contribution description

This PR provides support for nRF53 SPI and I2C.
It also moves common structs from each nRF CPU folder to `cpu/nrf5x_common` to avoid duplication.
Moreover, since nRF9160 and nRF5340 have shared IRQ for UART/SPI/I2C. Both this families now use a common file to register and manage these interrupts. Note that nRF9160 have different name for its interrupts than nRF5340 but they have the same purpose.

### Testing procedure

Since some structs were moved around, I think this PR should be carefully tested against nRF52, nRF53 and nRF9160 to avoid any issues.
On nRF5340DK-APP, SPI can be tested with its onboard SPI flash.

### Issues/PRs references



Co-authored-by: Dylan Laduranty <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 12, 2023

Build failed:

@benpicco
Copy link
Contributor

bors merge

bors bot added a commit that referenced this pull request Jul 13, 2023
19798: cpu/nrf53: add I2C and SPI support r=benpicco a=dylad

### Contribution description

This PR provides support for nRF53 SPI and I2C.
It also moves common structs from each nRF CPU folder to `cpu/nrf5x_common` to avoid duplication.
Moreover, since nRF9160 and nRF5340 have shared IRQ for UART/SPI/I2C. Both this families now use a common file to register and manage these interrupts. Note that nRF9160 have different name for its interrupts than nRF5340 but they have the same purpose.

### Testing procedure

Since some structs were moved around, I think this PR should be carefully tested against nRF52, nRF53 and nRF9160 to avoid any issues.
On nRF5340DK-APP, SPI can be tested with its onboard SPI flash.

### Issues/PRs references



Co-authored-by: Dylan Laduranty <[email protected]>
@dylad
Copy link
Member Author

dylad commented Jul 13, 2023

bors cancel

@bors
Copy link
Contributor

bors bot commented Jul 13, 2023

Canceled.

@dylad
Copy link
Member Author

dylad commented Jul 13, 2023

Unfortunately I can reproduce the error locally, this is not a false positive. I'll provide a fix and run a full compile test locally too.

@github-actions github-actions bot added Area: build system Area: Build system Area: pkg Area: External package ports Area: BLE Area: Bluetooth Low Energy support labels Jul 13, 2023
@dylad
Copy link
Member Author

dylad commented Jul 13, 2023

New commit added, I removed all nrf5% in pkg/ folder occurrence and I replace them by nrf51 nrf52 so nrf53 is opt out for now.

@dylad
Copy link
Member Author

dylad commented Jul 13, 2023

bors try

bors bot added a commit that referenced this pull request Jul 13, 2023
@@ -19,7 +19,8 @@ endif
ifneq (,$(filter uwb-core_dpl,$(USEMODULE)))
USEPKG += mynewt-core
USEMODULE += mynewt-core_os
ifneq (,$(filter nrf5%,$(CPU)))
# don't pull nrf53 into the list
ifneq (,$(filter nrf51 nrf52,$(CPU)))
USEMODULE += mynewt-core_nrf5x_hal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one also has support for nrf5340 and nrf91xx - but why does it use vendor specific code in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I never really used neither nimble nor mynewt.
I also don't understand why this didn't occur sooner...
I'll look at nimble support for nRF53 in RIOT at some point but let's focus on basic stuff first.

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Jul 13, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 0f50a8f into RIOT-OS:master Jul 13, 2023
@dylad
Copy link
Member Author

dylad commented Jul 13, 2023

Thanks @benpicco !

@dylad dylad deleted the cpu/nrf53/add_spi_i2c_support branch July 13, 2023 15:19
@bors
Copy link
Contributor

bors bot commented Jul 14, 2023

try

Timed out.

@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BLE Area: Bluetooth Low Energy support Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants