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/sam0_common/periph_gpio_ll: fix gpio_get_port() and gpio_ll_query_conf() #20999

Merged

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 18, 2024

Contribution description

gpio_get_port()

It turns out that the legacy GPIO API and GPIO LL may disagree on what the GPIO base address is: GPIO LL will use the IOBUS as base address no matter what, the legacy GPIO API will use the APB as base address unless periph_gpio_fast_read is used.

If the APIs disagree, we need to do impedance matching.

gpio_ll_query_conf()

For the other MCUs, we take the input register state instead of the output register state when the pin is configured as input. Let's do the same here, as this is a lot more useful and intuitive.

Testing procedure

gpio_query_conf(gpio_get_port(GPIO_PIN(PA, 1)), gpio_get_pin_num(GPIO_PIN(PA, 1)); should no longer run into a blown assertion.

Issues/PRs references

None

It turns out that the legacy GPIO API and GPIO LL may disagree on what
the GPIO base address is: GPIO LL will use the IOBUS as base address
no matter what, the legacy GPIO API will use the APB as base address
unless `periph_gpio_fast_read` is used.

If the APIs disagree, we need to do impedance matching.
@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch labels Nov 18, 2024
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Nov 18, 2024
@benpicco
Copy link
Contributor

benpicco commented Nov 18, 2024

GPIO LL will use the IOBUS as base address no matter what, the legacy GPIO API will use the APB as base address unless periph_gpio_fast_read is used.

Wouldn't it be less trouble if GPIO LL behaved the same wrt the use of periph_gpio_fast_read?

@maribu
Copy link
Member Author

maribu commented Nov 18, 2024

Well, there is no reason to ever write slow. Only for reading there is a power penalty to pay for fast access. That is why GPIO LL is always using the IOBUS for output (if there is an IOBUS mapping), and APB for read only when periph_gpio_ll_fast_read is not used.

The end game is also to get rid of periph_gpio implementations and implement that API on top of GPIO LL, once all MCUs are supported. At that point, we won't need to worry about periph_gpio's design decisions.

For the other MCUs, we take the input register state instead of the
output register state when the pin is configured as input. Let's do
the same here, as this is a lot more useful and intuitive.
@maribu maribu changed the title cpu/sam0_common/periph_gpio_ll: fix gpio_get_port() cpu/sam0_common/periph_gpio_ll: fix gpio_get_port() and gpio_query_conf() Nov 18, 2024
@maribu maribu changed the title cpu/sam0_common/periph_gpio_ll: fix gpio_get_port() and gpio_query_conf() cpu/sam0_common/periph_gpio_ll: fix gpio_get_port() and gpio_ll_query_conf() Nov 18, 2024
@riot-ci
Copy link

riot-ci commented Nov 18, 2024

Murdock results

✔️ PASSED

0222b8c cpu/sam0_common/periph_gpio_ll: fix gpio_query_conf()

Success Failures Total Runtime
10249 0 10249 16m:47s

Artifacts

@maribu maribu added this pull request to the merge queue Nov 18, 2024
Merged via the queue into RIOT-OS:master with commit 387f970 Nov 18, 2024
26 checks passed
@maribu
Copy link
Member Author

maribu commented Nov 19, 2024

Backport provided in #21008

@maribu maribu deleted the cpu/sam0_common/gpio_ll/fix-gpio_get_port branch November 19, 2024 09:05
@maribu
Copy link
Member Author

maribu commented Nov 19, 2024

Thx :)

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 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 Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants