cpu/kinetis: reworked GPIO driver implementation by haukepetersen · Pull Request #4830 · RIOT-OS/RIOT

haukepetersen · 2016-02-16T11:07:08Z

NOTE: this is untested code and so far only compiles for the mulle board

alternative 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.

jnohlgard · 2016-02-16T12:13:49Z

cpu/kinetis_common/include/periph_cpu.h

 /**
 * @brief   Define a CPU specific GPIO pin generator macro
 */
-#define GPIO_PIN(port, pin)          ((port << GPIO_PORT_SHIFT) | pin)


What is the benefit of not staying on byte boundaries? Are you trying to save bits?

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.

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?

jnohlgard · 2016-02-16T13:58:16Z

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?

jnohlgard · 2016-02-16T13:59:51Z

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..

haukepetersen · 2016-02-18T16:44:42Z

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 test/periph_gpio application and test a number of pins. If they work the code is alright, if not I need to fix stuff. The testing is essentially the same than testing a new GPIO implementation.

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.

haukepetersen · 2016-02-18T17:09:04Z

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 mulle next.

jnohlgard · 2016-02-18T20:23:33Z

Ok, I will review this PR sometime during the weekend. Ping me on Monday if I forget.

haukepetersen · 2016-02-19T08:33:10Z

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?

jnohlgard · 2016-02-19T09:32:29Z

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.

jnohlgard · 2016-02-19T09:34:43Z

cpu/kinetis_common/periph/gpio.c

+#define GPIO_PORTS_NUMOF    (7)
+
+/**
+ * @brief We allow for 7 (4-bit - 1) concurrent external interrupts to be set


(3 bit - 1)

2**4 = 16, 2**3 = 8

ups, beginners mistake :-)

though 7 does not break anything, as long as the number is <= 8.

haukepetersen · 2016-02-19T16:38:19Z

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.

jnohlgard · 2016-02-19T17:40:47Z

Nice, I like simplifications

jnohlgard · 2016-02-19T19:18:02Z

cpu/kinetis_common/periph/gpio.c

-    mutex_unlock(&int_config_lock);

-    return ret;
+static inline PORT_Type *port(gpio_t pin)


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)

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...

jnohlgard · 2016-02-19T20:48:19Z

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 (gpio_init_af()), and that the GPIO registers are untouched when the AF is not GPIO_AF_GPIO. The GPIO registers are only needed to configure the pin when using it as GPIO, if configuring for e.g. I2C you only need to touch the PORT registers.

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.

jnohlgard · 2016-02-19T21:02:50Z

cpu/kinetis_common/periph/gpio.c

+/**
+ * @brief   Allocation of memory for each independent interrupt slot
+ */
+static isr_ctx_t isr_ctx[CTX_NUMOF] = {{0}};


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...

Yapp, you are right, actually wrote a little test application earlier today to make sure... Thanks for this, @kaspar030! :-)

haukepetersen · 2016-02-20T17:11:39Z

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?

jnohlgard · 2016-02-20T20:41:34Z

@haukepetersen

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.

jnohlgard · 2016-02-20T20:42:49Z

I like the reworked gpio_init_port()! It will make periph driver pin initialization dead simple.

jnohlgard · 2016-02-20T20:59:18Z

cpu/kinetis_common/periph/gpio.c

+
+    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;


Use the ISFR register instead to avoid having to do a RMW cycle:

port->ISFR = (1 << i);

jnohlgard · 2016-02-20T21:16:12Z

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 gpio_init_af), but I guess setting pins as interrupt sources and then reconfiguring them to another mode while interrupts are still coming in is mostly an academic case with no danger to real world applications and can be deferred to the GPIO thread safety issue.

haukepetersen · 2016-02-21T13:28:37Z

I have a feeling there is a possible race condition between enabled pin IRQs and changing the configuration to disable interrupts

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

haukepetersen · 2016-02-21T13:32:41Z

using ISFR reg to clear interrupt flag in ISR now

jnohlgard · 2016-02-23T10:14:17Z

cpu/kinetis_common/periph/gpio.c

+        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);


As a safeguard, add a check/assert for cb != NULL

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...

OK, agreed.

jnohlgard · 2016-02-23T11:32:13Z

@haukepetersen do you have access to the pba-d-01-kw2x and frdm-k64f boards for testing?

haukepetersen · 2016-02-24T11:32:47Z

Yes I do, will test again this afternoon

haukepetersen · 2016-02-24T17:27:53Z

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 pba board?

haukepetersen · 2016-02-24T18:05:16Z

tested successfully for the frdm-k64f

haukepetersen · 2016-02-26T14:51:51Z

also tested successfully for the pba-d-01-kw2x

jnohlgard · 2016-02-27T22:50:09Z

OK to squash

jnohlgard · 2016-02-28T12:24:32Z

ACK, please squash

haukepetersen · 2016-02-29T10:09:03Z

rebased and squashed

cpu/kinetis: reworked GPIO driver implementation

haukepetersen · 2016-02-29T15:48:23Z

Awesome! @gebart: thanks for your help and patience!

haukepetersen assigned jnohlgard Feb 16, 2016

haukepetersen mentioned this pull request Feb 16, 2016

kinetis: Abstraction for PORT module #4813

Closed

jnohlgard reviewed Feb 16, 2016
View reviewed changes

haukepetersen force-pushed the opt_kinetis_gpio branch from 99b432e to a099845 Compare February 18, 2016 17:07

haukepetersen removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Feb 18, 2016

jnohlgard added this to the Release 2016.03 milestone Feb 19, 2016

jnohlgard reviewed Feb 19, 2016
View reviewed changes

haukepetersen force-pushed the opt_kinetis_gpio branch 2 times, most recently from 058e3a7 to 7aff094 Compare February 19, 2016 16:20

jnohlgard mentioned this pull request Feb 19, 2016

enhance gpio periphal driver #4472

Closed

jnohlgard reviewed Feb 19, 2016
View reviewed changes

haukepetersen mentioned this pull request Feb 20, 2016

periph/gpio: merged dir and pushpull parameters #4862

Merged

jnohlgard reviewed Feb 20, 2016
View reviewed changes

haukepetersen mentioned this pull request Feb 21, 2016

periph: GPIO drivers are not thread safe #4866

Open

haukepetersen mentioned this pull request Feb 21, 2016

drivers/periph: reworked the ADC driver #4430

Merged

jnohlgard reviewed Feb 23, 2016
View reviewed changes

jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 27, 2016

OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 27, 2016

cpu/kinetis_common: reworked GPIO driver

34f9dee

haukepetersen force-pushed the opt_kinetis_gpio branch from b859db8 to 34f9dee Compare February 29, 2016 10:08

haukepetersen removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 29, 2016

OlegHahm added a commit that referenced this pull request Feb 29, 2016

Merge pull request #4830 from haukepetersen/opt_kinetis_gpio

c8403e4

cpu/kinetis: reworked GPIO driver implementation

OlegHahm merged commit c8403e4 into RIOT-OS:master Feb 29, 2016

haukepetersen deleted the opt_kinetis_gpio branch February 29, 2016 15:48

jnohlgard mentioned this pull request Mar 1, 2016

kinetis: Add DMA support to UART driver #4931

Closed

Conversation

haukepetersen commented Feb 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Feb 16, 2016

Uh oh!

jnohlgard commented Feb 16, 2016

Uh oh!

haukepetersen commented Feb 18, 2016

Uh oh!

haukepetersen commented Feb 18, 2016

Uh oh!

jnohlgard commented Feb 18, 2016 via email

Uh oh!

haukepetersen commented Feb 19, 2016

Uh oh!

jnohlgard commented Feb 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Feb 19, 2016

Uh oh!

jnohlgard commented Feb 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Feb 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Feb 20, 2016

Uh oh!

jnohlgard commented Feb 20, 2016

Uh oh!

jnohlgard commented Feb 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Feb 20, 2016

Uh oh!

haukepetersen commented Feb 21, 2016

Uh oh!

haukepetersen commented Feb 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Feb 23, 2016

Uh oh!

haukepetersen commented Feb 24, 2016

Uh oh!

haukepetersen commented Feb 24, 2016

Uh oh!

haukepetersen commented Feb 24, 2016

Uh oh!

haukepetersen commented Feb 26, 2016

Uh oh!

jnohlgard commented Feb 27, 2016

Uh oh!

jnohlgard commented Feb 28, 2016

Uh oh!

haukepetersen commented Feb 29, 2016

Uh oh!

haukepetersen commented Feb 29, 2016