-
Notifications
You must be signed in to change notification settings - Fork 2k
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/kinetis: reworked GPIO driver implementation #4830
Conversation
* @brief Define a CPU specific GPIO pin generator macro | ||
*/ | ||
#define GPIO_PIN(port, pin) ((port << GPIO_PORT_SHIFT) | pin) | ||
#define GPIO_PIN(x, y) (((x + 1) << 12) | (x << 6) | y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the benefit of not staying on byte boundaries? Are you trying to save bits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple: by encoding the pins like this, it is very easy to generage the base registers for PORTx
and GPIOx
. Just look at the port()
and `gpio()' getter functions to see the beauty of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now, but I think we need at least an explanation inside a code comment.
However, do we really need to have both x and x+1 inside the pin value? Would it not make more sense to just do + 1
inside the port function below?
I know this is WIP but I don't like the general look of the code, there is too much bit-fiddling and confusing multiplications with array indexes etc. Can we do this in steps? I'll refactor the PORT PR #4813 to merge port.c into gpio.c and change the name from port_init to gpio_init_af and then we can start looking at refactoring the whole GPIO driver? |
What I mean is that I'm sure there are some good thoughts in this PR, but there are too many changes at once to be able to follow and review.. |
I disagree. Especially for an GPIO driver the testing is very simple ans straight forward. Just run the In general, the used coding style is proved itself for many other boards and is pretty much the most efficient option we have at the moment. |
99b432e
to
a099845
Compare
Now not WIP anymore, successfully tested with the user button and LED on the phytec board for now. Needs to be tested for the FRDM and the |
Ok, I will review this PR sometime during the weekend. Ping me on Monday if
I forget.
|
I thought about what you said earlier and had an idea: how about I cut the interrupt channel lookup part out of the driver and put it in it's own (shared) module? For this I can then write unit-tests and it also can easily be re-used in other GPIO drivers. Does this sound better to you? |
Good idea, other GPIO drivers will likely benefit from the same kind of interrupt handler pool to reduce memory bloat when few GPIOs are configured as interrupt sources. |
#define GPIO_PORTS_NUMOF (7) | ||
|
||
/** | ||
* @brief We allow for 7 (4-bit - 1) concurrent external interrupts to be set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(3 bit - 1)
2**4 = 16,
2**3 = 8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ups, beginners mistake :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though 7 does not break anything, as long as the number is <= 8.
058e3a7
to
7aff094
Compare
Did one fix that got lost on the way, forgot to clear an interrupt channel again if a pin is redefined from interrupt input to simple input or output. While at it, I found a nice simplification that does not need the 'isr_map' to be initialized with ones anymore, also allowing us now to have 16 interrupts configured concurrently. |
Nice, I like simplifications |
|
||
return ret; | ||
static inline PORT_Type *port(gpio_t pin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and all other static
functions: Could you add a prefix underscore to the name to follow the conventions used in other modules (e.g. gnrc, xtimer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, I would rather see the other modules to not do it - it is not really a convention and the underscore style is IMHO very ugly...
As I understand this PR, this is how a periph driver should configure its pins: gpio_init(my_pin, GPIO_DIR_OUT, GPIO_NOPULL);
gpio_init_af(my_pin, GPIO_AF3); I would like it to be so that periph drivers only need to make one call ( I think there needs to be a mutex or lock of some kind when messing with the ctx variables, since it's a RMW cycle. |
/** | ||
* @brief Allocation of memory for each independent interrupt slot | ||
*/ | ||
static isr_ctx_t isr_ctx[CTX_NUMOF] = {{0}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
misleading initializer syntax, AFAIK this only specifies the value of the first element of the array, all other elements will be zero-filled, even if the {0}
is changed to something else...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yapp, you are right, actually wrote a little test application earlier today to make sure... Thanks for this, @kaspar030! :-)
About the AF configuration for other peripherals: You have a good point, this can made simpler for the Kinetis - how about 77c1635? The way to use this in other periph drivers is now very simple: // simple usage
gpio_init_port(pin, GPIO_AF_3);
or
// af 6, open-drain with pull-up
gpio_init_port(pin,( GPIO_AF_6 | GPIO_PCR_OD | GPIO_PCR_PU)); The driver is now also prepared for simple adaption of #4862... Regarding the thread safety: another very good point - which affects almost all out current GPIO implementations! -> I would tend to put this in a separate issue for now and find a global solution and apply that to all periph (GPIO) drivers once decided. Would that be ok with you? |
I agree with making it a separate issue. After reading through #4862 I have realized that all GPIO drivers are thread unsafe in varying degrees. |
I like the reworked |
|
||
for (int i = 0; i < 32; i++) { | ||
if ((status & (1 << i)) && (port->PCR[i] & PORT_PCR_IRQC_MASK)) { | ||
port->PCR[i] |= PORT_PCR_ISF_MASK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the ISFR register instead to avoid having to do a RMW cycle:
port->ISFR = (1 << i);
I have a feeling there is a possible race condition between enabled pin IRQs and changing the configuration to disable interrupts (even with the safeguard inside |
I think this should be fine, l211 to l220 should make sure, that the context for an interrupt channel does actually exist until the channel is disabled. But the race conditions while initializing pins are pretty scary, though not much (or no) code until now seemed to have been effected by this?! Anyway I opened an issue for this: #4866 |
using ISFR reg to clear interrupt flag in ISR now |
if ((status & (1 << i)) && (port->PCR[i] & PORT_PCR_IRQC_MASK)) { | ||
port->ISFR = (1 << i); | ||
int ctx = get_ctx(port_num, i); | ||
isr_ctx[ctx].cb(isr_ctx[ctx].arg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a safeguard, add a check/assert for cb != NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided actively against this to keep the ISR routine as short as possible. As the ctx
is only modified in this driver, the chance of it accidentally being set to NULL
is zero so I think it is save here to go without the check. The ISR is slow enough already...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, agreed.
@haukepetersen do you have access to the pba-d-01-kw2x and frdm-k64f boards for testing? |
Yes I do, will test again this afternoon |
Sorry, just noticed that my pba boards are all in Nuremberg at the Embedded world, so testing them will have to wait until Friday. @cgundogan, @PeterKietzmann: Or might you be so nice and give this PR a test on the |
tested successfully for the |
also tested successfully for the |
OK to squash |
ACK, please squash |
b859db8
to
34f9dee
Compare
rebased and squashed |
cpu/kinetis: reworked GPIO driver implementation
Awesome! @gebart: thanks for your help and patience! |
NOTE: this is untested code and so far only compiles for the
mulle
boardalternative to #4813
Compared to master, this PR saves ~200 byte ROM and ~200 byte RAM while offering slightly more functionality (->
gpio_init_af
)I think there is no need for an explicit
port
module, all GPIO pin/port related functions can be put into this one implementation file, while still offering the AF pin configuration to be used by other peripheral modules.