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/nrf5x_common: implement periph/gpio_ll{,_irq} #17980

Merged
merged 1 commit into from
Apr 26, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented Apr 22, 2022

Contribution description

Implements gpio_ll and gpio_ll_irq for NRF5x.

Testing procedure

make BOARD=nrf52840dk -C tests/periph_gpio_ll
make BOARD=nrf51dk -C tests/periph_gpio_ll

Both are passing locally for me

Issues/PRs references

Depends on and includes: #16787

@github-actions github-actions bot added Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Apr 22, 2022
@maribu maribu added Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first and removed Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: build system Area: Build system Area: Kconfig Area: Kconfig integration labels Apr 22, 2022
@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 22, 2022
@github-actions github-actions bot added Area: Kconfig Area: Kconfig integration Platform: ARM Platform: This PR/issue effects ARM-based platforms and removed Area: drivers Area: Device drivers labels Apr 22, 2022
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 22, 2022
cpu/nrf5x_common/include/gpio_ll_arch.h Outdated Show resolved Hide resolved
cpu/nrf5x_common/periph/gpio_ll_irq.c Outdated Show resolved Hide resolved
@maribu
Copy link
Member Author

maribu commented Apr 22, 2022

Output of the benchmark on the nRF52840-DK:

Help: Press s to start test, r to print it is ready
START
main(): This is RIOT! (Version: 2022.07-devel-163-gea5f8-gpio_ll/nrf5x)

Benchmarking GPIO APIs
======================

estimating loop overhead for compensation
-----------------------------------------
2345 us for 50000 iterations

periph/gpio: Using 2x gpio_set() and 2x gpio_clear()
---------------------------------------------------
50000 iterations took 51563 us (53908 us uncompensated)
Two square waves pins at       969687 Hz (      927506 Hz uncompensated)
~66 CPU cycles per square wave period (~69 cycles uncompensated)
:'-(

periph/gpio_ll: Using gpio_ll_set() and gpio_ll_clear()
-------------------------------------------------------
50000 iterations took 1562 us (3907 us uncompensated)
Two square waves pins at     32010243 Hz (    12797542 Hz uncompensated)
~2 CPU cycles per square wave period (~5 cycles uncompensated)
:-D

periph/gpio: Using 4x gpio_toggle()
-----------------------------------
50000 iterations took 56250 us (58595 us uncompensated)
Two square waves pins at       888888 Hz (      853315 Hz uncompensated)
~72 CPU cycles per square wave period (~75 cycles uncompensated)
:'-(

periph/gpio_ll: Using 2x gpio_ll_toggle()
-----------------------------------------
50000 iterations took 14063 us (16408 us uncompensated)
Two square waves pins at      3555429 Hz (     3047294 Hz uncompensated)
~18 CPU cycles per square wave period (~21 cycles uncompensated)
:'-(

periph/gpio: Using 4x gpio_write()
----------------------------------
50000 iterations took 56250 us (58595 us uncompensated)
Two square waves pins at       888888 Hz (      853315 Hz uncompensated)
~72 CPU cycles per square wave period (~75 cycles uncompensated)
:'-(

periph/gpio_ll: Using 2x gpio_ll_write()
----------------------------------------
50000 iterations took 1562 us (3907 us uncompensated)
Two square waves pins at     32010243 Hz (    12797542 Hz uncompensated)
~2 CPU cycles per square wave period (~5 cycles uncompensated)
:-D


TEST SUCCEEDED
{ "threads": [{ "name": "main", "stack_size": 1536, "stack_used": 440 }]}

So gpio_ll_set(), gpio_ll_clear(), and gpio_ll_write() perform optimal (one CPU cycle per call). gpio_ll_toggle() is suboptimal, as there is no hardware acceleration for it (or I couldn't find it in the datasheet). 9 CPU cycles per access is a bit unpleasant, but a read-modify-write sequence by hand with IRQ disabling is expected to be non-optimal.

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

Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
@maribu
Copy link
Member Author

maribu commented Apr 23, 2022

The nRF9x has a different layout of the memory mapped I/O area, but the start address remaimed the same. There is value in using the start adress of the are father than e.g. the lowest GPIO register, as the least significant bytes are all zeros, allowing to construct the adress via a load immediate instruction (or st least I believe so). I opted for just creating a define for that by hand.

With that, now all is green.

Comment on lines +126 to +136
static inline void * gpio_port_unpack_addr(gpio_port_t port)
{
/* NRF5X_IO_AREA_START is the start of the memory mapped I/O area. Both data
* and flash are mapped before it. So if it is an I/O address, it
* cannot be a packed data address and (hopefully) is a GPIO port */
if (port >= NRF5X_IO_AREA_START) {
return NULL;
}

return (void *)port;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@benpicco are you fine with this? (Note that NRF5X_IO_AREA_START is defined in this header, so it will be needed to be maintained by us. But I don't expect the I/O memory area to be mapped differently for future nRF MCUs that can share the same GPIO driver.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that will work

@benpicco benpicco merged commit b6b1468 into RIOT-OS:master Apr 26, 2022
@maribu maribu deleted the gpio_ll/nrf5x branch June 2, 2022 19:39
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration 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 Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants