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

nrf52: Implement EasyDMA-based SPI peripheral implemenation #14057

Merged
merged 16 commits into from
May 18, 2020

Conversation

bergzand
Copy link
Member

Contribution description

This PR splits the common nrf5x SPI peripheral into the old (,and for the nrf52 deprecated peripheral,) nRF51 driver and a new EasyDMA-based driver for the nRF52. The new driver is significantly faster than the old one as the bytes can be send back-to-back without overhead from the CPU.

I decided to split the nrf5x common peripheral driver, it became too much of an ifdef hell to keep them merged.

The peripheral driver requires a GPIO interrupt on the nrf52832 due to errata 58. The proposed driver uses an adapted form of the workaround presented by Nordic. This anomaly also exists on engineering sample A of the nrf52840. I decided not to support that to shave off some additional flash usage and runtime checks. This might be an issue on the nrf52840-pdk boards.

Testing procedure

Compilation should verify that all nrf52 boards have been adapted, the board config requires modification from NRF_SPI# to NRF_SPIM#. Maybe @MrKevinWeiss can give this a try on the RobotFW tests to verify the runtime behaviour.

Issues/PRs references

None

@bergzand bergzand added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 11, 2020
@bergzand bergzand force-pushed the pr/nrf52/dma_spi branch 3 times, most recently from b520aca to 48d72ef Compare May 11, 2020 22:05
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.

Looking good - some small comments.

cpu/nrf52/periph/spi.c Outdated Show resolved Hide resolved
cpu/nrf52/periph/spi.c Show resolved Hide resolved
cpu/nrf52/periph/spi.c Outdated Show resolved Hide resolved
cpu/nrf52/spi_twi_irq.c Outdated Show resolved Hide resolved
@bergzand
Copy link
Member Author

@benpicco I've modified the code to minimize the number of ifdefs and let the compiler handle removing any resulting dead code.

cpu/nrf52/spi_twi_irq.c Outdated Show resolved Hide resolved
cpu/nrf52/spi_twi_irq.c Outdated Show resolved Hide resolved
@bergzand bergzand removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 12, 2020
@bergzand bergzand force-pushed the pr/nrf52/dma_spi branch from ed6e7f5 to 4f2db36 Compare May 12, 2020 13:30
@bergzand
Copy link
Member Author

Rebased on top of #14061 (and now waiting on that PR)

@bergzand bergzand added the State: waiting for other PR State: The PR requires another PR to be merged first label May 12, 2020
@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label May 12, 2020
@bergzand bergzand force-pushed the pr/nrf52/dma_spi branch from 1b5c543 to ce8c795 Compare May 12, 2020 17:17
@bergzand
Copy link
Member Author

Rebased!

@bergzand bergzand added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 12, 2020
cpu/nrf52/periph/spi.c Outdated Show resolved Hide resolved
cpu/nrf52/periph/spi.c Outdated Show resolved Hide resolved
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.

Looks good to me - just some minor nits.
You can squash directly.

cpu/nrf52/include/periph_cpu.h Outdated Show resolved Hide resolved
cpu/nrf52/periph/spi.c Outdated Show resolved Hide resolved
cpu/nrf52/periph/spi.c Show resolved Hide resolved
cpu/nrf52/periph/spi.c Outdated Show resolved Hide resolved
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.

Please squash!

@bergzand bergzand force-pushed the pr/nrf52/dma_spi branch from 4b6920e to d0047dd Compare May 18, 2020 17:08
@bergzand
Copy link
Member Author

Squashed!

@bergzand
Copy link
Member Author

Ho, That rebase is not how it should be, give me a second for a next attempt

@bergzand bergzand force-pushed the pr/nrf52/dma_spi branch from d0047dd to e76ee57 Compare May 18, 2020 17:18
@bergzand
Copy link
Member Author

Should be good now, at least the content of the commits now matches their description

@benpicco benpicco merged commit 9f707bf into RIOT-OS:master May 18, 2020
@bergzand
Copy link
Member Author

Thanks for the review @benpicco!

@bergzand bergzand deleted the pr/nrf52/dma_spi branch May 18, 2020 18:17
@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants