cpu/efm32: Minimal support for gpio_ll #18023
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrysn
requested review from
leandrolanzieri,
jia200x,
MrKevinWeiss and
basilfx
as code owners
github-actions
bot
added
Area: cpu
chrysn
added
Type: new feature
maribu
reviewed
Apr 27, 2022
There was a problem hiding this comment.
Looks good to me. Some comments inline.
drive and pull up strengths were not even defined to the same value (as AIU they should be for a device not supporting any), because they're still to be extended.
From a users point of view there is little difference in features missing due to missing implementation or lack of hardware support. IMO, this should be defined to a single numeric value until the feature is exposed by the driver.
github-actions
bot
added
Area: Kconfig
|
Added support for pull-up/-down resistors, now passing tests without warnings. Slew rates and pull-up strengths are collapsed because they are not supported in the hardware. Drive strengths are collapsed and documented not to be implemented right now (they're tricky: there is only one alternative strength possible per port) -- I'd leave these to whoever needs them. Interrupts are still not in, but I'd prefer to get this in with the current feature set, and add interrupts or drive strengths as they are needed. test output |
|
Now also tested on SLTB001a (after careful avoiding pins that are wired to any external pull-ups...). Test resultsbenchmark resultsMakefile.sltb001a... and earlier I missed placing this here: Makefile.stk3700 |
maribu
approved these changes
May 3, 2022
cpu/efm32/include/gpio_ll_arch.h
Outdated
| * are using an alternative drive strength, and refuse changing it if any are | ||
| * found. | ||
| * | ||
| * * There is an optional glitch suppression filter after the Schmitt tigger; |
There was a problem hiding this comment.
Suggested change
| * * There is an optional glitch suppression filter after the Schmitt tigger; | |
| * * There is an optional glitch suppression filter after the Schmitt trigger; |
There was a problem hiding this comment.
Thanks, fixed in squash. Christopher Robin and Eeyore will be sad, though.
chrysn
force-pushed
the
efm32-gpio-ll
branch
2 times, most recently
from
7fe3bb2
to
f86d17d
Compare
|
Build tests ran through, but I missed that a static test barked. I'm taking the liberty to set "skip build tests" on the upcoming insta-squashed fix that comes in for that; static checks will still run. |
Collapsing strengths as they'll stay unused for the time being.
chrysn
removed
the
CI: ready for build
chrysn
added
CI: skip compile test
|
IMO, this should be defined to a single numeric value until the
feature is exposed by the driver.
It's more a matter of "I don't expect this to be merged before I add
these anyway, so why bother writing code I'll remove in a few days"; if
you prefer this to be merged already in its short form, then yes I'll
collapse them.
|
|
Too late, this has been merged 5 months ago ;) |
|
Weird ... in which outgoing mail queue has this been sitting? GitHub doesn't show headers like bugs.debian.org does and I think would be best practice, so probably I'll never know...
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This is a minimal implementation of gpio_ll for EFM32; it is minimal in that
The choice of using indices for
gpio_port_tis certainly unconventional, and not using gpio_ll to its full potential. For (at least some of -- it's a diverse group) EFM32 chips it is necessary, though: On some families, there is one register block that has members likeP[n]->{DIN,DOUT}, but a separate blockP_SET[n]->DOUTfor setting (and a similar one for clearing) pins. As these blocks generally have different sizes, using&P[n]asgpio_port_twould require division and multiplication to get to the toggle blocks.Alternatives that were rejected (for now) are:
gpio_port_tthat contains all the relevant base pointers,DOUT_{SET,CLEAR,TOGGLE}registers in the main block.The third alternative can still be done, specializing the implementation for a subset of the EFM32 group. (I haven't tried to find out which of the 3 or more generations of EFM32 use which behavior).
Testing procedure
I've used an
stk3700kit; if it sits on a table in front of you, place two jumpers vertically on the right connectors, on the second and third pin pair closest to you.Benchmarks show 6 cycles per period -- not great, not terrible, and in the ball park of what I'd expect with the indexed peripherals:
I'd yet like to run these with a few other boards (stk3200, sltb001a), just didn't get around to it yet.
Notes that fit nowhere in particular
If you trace the code through the library you might see that it appears that toggle and set/clear operations sometimes fan out to interrupt enabling / disabling RMW cycles (
Bus_RegMasked{Set,Clear}).While that'd even be acceptable, as far as I can tell it doesn't happen, because the families that do not have set/clear registers tell you in their manuals to defer to the set/clear memory regions. Later families (after the memory regions were deprecated, I think along with the bitbanding memory regions) have dedicated set/clear registers again. Thus, the RMW code will not be reached.
Next steps
I'm asking for a high-level initial review on the whole thing; I'd probably add strengths etc. later to the extent this can be done without going into the details of subfamilies. (Although if others prefer to have the minimal form in first, I can do the defining-to-the-same-values, and this might get mergable more easily).
[edit: It's not as minimal as originally set out, see edited list above; any further additions like IRQs or drive strengths may be done in follow-up PRs on demand.]