cpu/stm32: unify i2c drivers and apply new API by aabadie · Pull Request #9202 · RIOT-OS/RIOT

aabadie · 2018-05-25T14:55:32Z

Contribution description

This PR unifies the I2C peripheral driver implementation for STM32 F0/F3/F7/L0/L4 families, since they are very similar and adapt the related board configurations.
This PR also changes the way i2c peripheral configurations are provided by the boards. Now each board must define a nice array of structs, like for other peripherals.

For F0, F7 and L4, I added a default configuration for the nucleo-f070rb, nucleo-f722ze, and b-l475e-iot01a boards.

I tested it on b-l072z-lrwan1 and b-l475e-iot01a with a hts221 sensor and it works. I couldn't really test the F3 driver, since the driver seems broken, even in master. I couldn't find a solution for this. Same for F0.

For F3, I also had to update the f303xe cmsis file, and also found an issue in the periph_common init. Will open a PR for this.

Issues/PRs references

#6577

vincent-d · 2018-05-25T15:27:35Z

boards/b-l072z-lrwan1/include/periph_conf.h

 #define I2C_NUMOF           I2C_0_EN
 #define I2C_IRQ_PRIO        1
 #define I2C_APBCLK          (CLOCK_APB1)
+#define I2C_SPEED           I2C_SPEED_NORMAL


I think it would be great to take advantage of the refactoring to get rid of this "old-style" define way and use an array of struct as it's done for other periph.

vincent-d · 2018-05-25T15:28:45Z

cpu/stm32_common/include/periph_cpu_common.h

+    gpio_mode_t pin_mode;   /**< with or without pull resistor */
+    gpio_af_t af;           /**< I2C alternate function value */
+    uint8_t ev_irqn;        /**< event IRQ */
+} i2c_conf_t;


Good to have this, but it seems unused, isn't it?

vincent-d · 2018-05-25T15:29:15Z

cpu/stm32_common/include/periph_cpu_common.h

+    gpio_t scl;             /**< scl pin number */
+    gpio_t sda;             /**< sda pin number */
+    gpio_mode_t pin_mode;   /**< with or without pull resistor */
+    gpio_af_t af;           /**< I2C alternate function value */


Don't you need an af per pin (e.g. one for sda, and one for scl)?

aabadie · 2018-05-26T21:53:00Z

Current status of this PR:

L0: works, tested on b-l072z-lrwan
L4: works, tested on b-l475e-iot01a
F7: works, tested on nucleo144-f722
F0 and F3 are broken. For both, the application hangs before sending the start condition (more precisely, in the function that does that, while configuring the registers). I couldn't find a reason why this happens and gdb didn't help either.

aabadie · 2018-05-28T07:34:04Z

F0 and F3 are broken

Just found the problem ! Now both are working, will push a fix. In fact, the I2C clock source has to be turned on in one RCC register (CFGR3).

aabadie · 2018-05-28T08:15:58Z

This PR is ready for review again.

Actual status: tested nucleo-f070/f303/l073/f722, b-l072z-lrwan1 and b-l475e-iot01a boards with success.
So now I2C works for all STM32 F0/F3/F7/L0/L4 cpu families.

aabadie · 2018-05-28T08:31:37Z

There's a problem with Travis: this PR conflicts with current master (because of the DMA changes in the STM32 periph_common header file).
What should we do in this case ? Rebase new_i2c_if on top of master (with or without --preserve-merges option) ? @miri64 did you have the same issue with the netif2 feature branch ? (how was this solved ?) Sorry, lots of questions.

vincent-d · 2018-05-28T10:26:09Z

Quickly tested with nucleo-f091 (see aabadie#7). It seems OK so far.

vincent-d

A first batch of comments, I didn't go through everything yet.

vincent-d · 2018-05-28T10:36:16Z

cpu/stm32_common/periph/i2c.c

+ */
+
+/**
+ * @ingroup     cpu_stm32l0


cpu_stm32_common

vincent-d · 2018-05-28T10:38:43Z

cpu/stm32_common/periph/i2c.c

+
+#define I2C_IRQ_PRIO    (1)
+
+#define CLEAR_FLAG(dev)            dev->ICR |= I2C_ICR_NACKCF   | \


we usually avoid macro, I would suggest to use a constant to only define the value,
i.e. #define CLEAR_FLAG (I2C_ICR_NACKCF | I2C_ICR_ARLOCF | I2C_ICR_BERRCF)
and then use it dev->ISR |= CLEAR_FLAG

Another method which will clear all w1c flags is

dev->ISR = dev->ISR;

vincent-d · 2018-05-28T10:41:19Z

cpu/stm32_common/periph/i2c.c

+
+#if defined(CPU_FAM_STM32F0) || defined(CPU_FAM_STM32F3)
+    /* Clear I2CSW bit */
+    RCC->CFGR3 &= ~(i2c_config[dev].rcc_sw_mask);


I think this is not the expected behavior, you should clear with RCC_CFGR3_I2C1SW

This is what is done. But the macro used for setting the clock after is not correct. I'll update this but that will required another field in the configuration struct.

It works only when rcc_sw_mask is 1. This line will write 0, and the next one 1. If rcc_sw_mask is 0, it will leave the field as-is (so if this was the reset value it works, but then this line is useless).

vincent-d · 2018-05-28T10:42:22Z

cpu/stm32_common/periph/i2c.c

+
+    DEBUG("[i2c] init: configuring pins\n");
+    /* configure pins */
+    gpio_init(i2c_config[dev].scl_pin, GPIO_OD_PU);


Not sure if GPIO_OD_PU is the best here, same for sda. Why not using GPIO_OUT?

vincent-d · 2018-05-28T10:45:35Z

cpu/stm32_common/periph/i2c.c

+    return length;
+}
+
+void i2c_poweron(i2c_t dev)


i2c_poweron and i2c_poweroff are not in the API anymore

vincent-d · 2018-05-28T11:40:39Z

cpu/stm32_common/periph/i2c.c

+    periph_clk_dis(i2c_config[dev].bus, i2c_config[dev].rcc_mask);
+}
+
+static inline void _start(I2C_TypeDef *i2c, uint8_t address,


Even though 10-bit address is not yet implemented, I think it would be better to use uint16_t for address. Maybe pass the flags to be able to enable this feature later on.

vincent-d · 2018-05-28T11:55:55Z

cpu/stm32_common/include/cpu_conf_stm32_common.h

+
+static const i2c_timing_param_t timing_params[] = {
+#if defined(CPU_FAM_STM32F0)
+    [ I2C_SPEED_NORMAL ]    = {


These timings are valid for an input clock wt 48MHz (same for L4 and F7, btw).

The clock requirements should be stated clearly somewhere (alongside the i2c_conf_t definition I guess).

vincent-d · 2018-05-28T11:57:28Z

cpu/stm32_common/periph/i2c.c

+    DEBUG("[i2c] init: configuring device\n");
+    /* set the timing register value from predefined values */
+    i2c_timing_param_t tp = timing_params[i2c_config[dev].speed];
+    uint32_t timing = (( (uint32_t)tp.presc << 28) |


You might use the constants from the vendor header for the shift values

aabadie · 2018-05-28T12:38:27Z

Thanks for reviewing @vincent-d, I'll update the PR asap

aabadie · 2018-05-28T14:59:13Z

@vincent-d, I think I addressed your comments. Regarding the pin configuration, even not a good reason, it kept what was set for f3 and l0. Looking at the reference manual didn't help.

vincent-d

Still some small questions/remarks. Looks pretty good otherwise.

vincent-d · 2018-05-28T15:12:05Z

cpu/stm32_common/include/cpu_conf_stm32_common.h

+        .sdadel = 0x0,      /* t_SDADEL = 0ns   */
+        .scldel = 0x1,      /* t_SCLDEL = 250ns */
+    }
+#elif defined(CPU_FAM_STM32L4) || defined(CPU_FAM_STM32F7)


this could be factored in the CPU_FAM_STM32F0 block

I'm also wondering if we should not group by frequency instead of by family. But this could be done later on if we think it makes more sense.

vincent-d · 2018-05-28T15:15:37Z

cpu/stm32_common/periph/i2c.c

+    i2c->CR2 |= I2C_CR2_STOP;
+}
+
+static inline void irq_handler(i2c_t dev)


It seems no interrupt is enable, is this function really needed or is this more for a future use?

One interrupt is enabled here: https://github.com/RIOT-OS/RIOT/pull/9202/files#diff-8703624eb1944c61de7e7320bf02f6e3R80

On certain families (F3, L4, F7) there are 2 per I2C (ev/er), I only took the er for the moment.

vincent-d · 2018-05-28T15:17:17Z

Regarding the pin configuration, even not a good reason, it kept what was set for f3 and l0.

I'd say let's start with that and fix later if this is an issue. My only concern is when an external pull-up is already there.

vincent-d

Found a couple of small other issues, hopefully the last ones!

vincent-d · 2018-05-28T17:22:16Z

cpu/stm32_common/periph/i2c.c

+    while (!(i2c->CR2 & I2C_CR2_START) && tick--) {}
+}
+
+static inline int _read(I2C_TypeDef *i2c, uint8_t *data, int length)


length should be size_t

vincent-d · 2018-05-28T17:22:29Z

cpu/stm32_common/periph/i2c.c

+
+    uint16_t tick = TICK_TIMEOUT;
+
+    for (int i = 0; i < length; i++) {


vincent-d · 2018-05-28T17:23:03Z

cpu/stm32_common/periph/i2c.c

+    for (int i = 0; i < length; i++) {
+        /* wait for transfer to finish */
+        DEBUG("[i2c] read: Waiting for DR to be full\n");
+        while (!(i2c->ISR & I2C_ISR_RXNE) && tick--) {


tick should be reset to TICK_TIMEOUT before

vincent-d · 2018-05-28T17:23:21Z

cpu/stm32_common/periph/i2c.c

+    return 0;
+}
+
+static inline int _write(I2C_TypeDef *i2c, const uint8_t *data, int length)


vincent-d · 2018-05-28T17:23:34Z

cpu/stm32_common/periph/i2c.c

+
+    uint16_t tick = TICK_TIMEOUT;
+
+    for (int i = 0; i < length; i++) {


vincent-d · 2018-05-28T17:23:47Z

cpu/stm32_common/periph/i2c.c

+    for (int i = 0; i < length; i++) {
+        /* wait for ack */
+        DEBUG("[i2c] write: Waiting for ACK\n");
+        while (!(i2c->ISR & I2C_ISR_TXIS) && tick--)  {


tick should be reset to TICK_TIMEOUT before

aabadie · 2018-05-28T20:15:40Z

@vincent-d, comments addressed!

- i2c_1 is built for f0, f3, f7, l0 and l4 - i2c_2 is built for f4

aabadie · 2018-06-13T11:22:44Z

The rebase issue is fixed, some commits were not at the right place...

aabadie · 2018-06-13T11:55:39Z

Comments addressed @dylad

dylad · 2018-06-13T12:12:06Z

@aabadie Travis reported an error with cppcheck.
Squashed directly so we can merge this one.

aabadie · 2018-06-13T12:19:44Z

@dylad, finally fixed.

dylad · 2018-06-13T12:19:49Z

Great job @aabadie
Here we go !

dylad · 2018-06-13T12:26:02Z

@aabadie Some of your files now introduced merge conflict with master again. We will need to fix it somehow later

cpu/stm32: unify i2c drivers and apply new API

aabadie added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: drivers Area: Device drivers TF: I2C Marks issues and PRs related to the work of the I²C rework task force labels May 25, 2018

haukepetersen mentioned this pull request May 25, 2018

periph/i2c: tracking the remodeling #6577

Closed

47 tasks

vincent-d reviewed May 25, 2018

View reviewed changes

aabadie changed the title ~~Unify stm32 L0 and F3 I2C implementation and apply new API~~ Unify STM32 F0/F3/L0 i2c driver and apply new API May 26, 2018

aabadie changed the title ~~Unify STM32 F0/F3/L0 i2c driver and apply new API~~ Unify STM32 F0/F3/L0/L4 i2c driver and apply new API May 26, 2018

aabadie changed the title ~~Unify STM32 F0/F3/L0/L4 i2c driver and apply new API~~ Unify STM32 F0/F3/F7/L0/L4 i2c driver and apply new API May 26, 2018

This was referenced May 27, 2018

drivers/periph_common: init i2c only if module is loaded #9204

Merged

Track support for ST b-l475e-iot01a board #7585

Open

aabadie force-pushed the new_i2c_stm32l0_f3 branch from d9e446a to e68abff Compare May 27, 2018 14:01

aabadie mentioned this pull request May 28, 2018

cpu: efm32: adapt to new I2C interface #9208

Merged

aabadie force-pushed the new_i2c_stm32l0_f3 branch 3 times, most recently from 9f3c503 to d94ec5b Compare May 28, 2018 08:15

aabadie force-pushed the new_i2c_stm32l0_f3 branch from d94ec5b to 45c2b8f Compare May 28, 2018 08:18

vincent-d requested changes May 28, 2018

View reviewed changes

vincent-d reviewed May 28, 2018

View reviewed changes

aabadie mentioned this pull request May 29, 2018

drivers/periph_i2c: i2c_init should return void #9219

Merged

aabadie added 8 commits June 13, 2018 13:12

cpu/stm32{f0,f3,f7,l0,l4}: unify i2c driver and use new API

5449e0d

boards/*: adapt stm32l0 based board to new I2C api

60b7a46

boards: adapt stm32f3 based boards to new I2C api

18ac593

boards/nucleo-f070: add i2c configuration for i2c

c6d345c

boards/nucleo144-f303: remove useless I2C_NUMOF define

7431d63

boards/b-l475e-iot01a: provide i2c configuration

c053f91

boards/nucleo-f722ze provide i2c configuration

9660183

cpu/stm32_common: split common i2c periph in 2

9549b6f

- i2c_1 is built for f0, f3, f7, l0 and l4 - i2c_2 is built for f4

aabadie force-pushed the new_i2c_stm32l0_f3 branch from 5275fa9 to d81ce15 Compare June 13, 2018 11:12

aabadie and others added 6 commits June 13, 2018 13:18

boards/nucleo-f207zg: use new style for i2c config

fd14e54

boards*: adapt stm32f4 based board with new i2c conf style

ac099e3

boards/nucleo-l152re: use new style for i2c config

c89ad07

cpu/stm32_common: adapt i2c driver to stm32f1

aef852e

boards*: adapt f1 based boards to new i2c api

0b11486

cpu/stm32_common: add timeouts in stm32f4 i2c driver

241d702

aabadie force-pushed the new_i2c_stm32l0_f3 branch from d81ce15 to 4aa44dc Compare June 13, 2018 11:19

cpu/stm32_common: update return values to error codes

fafec43

aabadie force-pushed the new_i2c_stm32l0_f3 branch from 8209dfb to fafec43 Compare June 13, 2018 12:15

dylad merged commit 7c021da into RIOT-OS:new_i2c_if Jun 13, 2018

aabadie deleted the new_i2c_stm32l0_f3 branch June 21, 2018 09:31

basilfx pushed a commit to basilfx/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9202 from aabadie/new_i2c_stm32l0_f3

a70df98

cpu/stm32: unify i2c drivers and apply new API

dylad added a commit to dylad/RIOT that referenced this pull request Jul 10, 2018

Merge pull request RIOT-OS#9202 from aabadie/new_i2c_stm32l0_f3

ecf4be1

cpu/stm32: unify i2c drivers and apply new API

dylad added a commit that referenced this pull request Jul 11, 2018

Merge pull request #9202 from aabadie/new_i2c_stm32l0_f3

2b3d36b

cpu/stm32: unify i2c drivers and apply new API

aabadie added this to the Release 2018.10 milestone Nov 5, 2018


		#define I2C_IRQ_PRIO (1)

		#define CLEAR_FLAG(dev) dev->ICR \|= I2C_ICR_NACKCF \| \


		uint16_t tick = TICK_TIMEOUT;

		for (int i = 0; i < length; i++) {

Conversation

aabadie commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

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!

aabadie commented May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented May 28, 2018

Uh oh!

aabadie commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincent-d commented May 28, 2018

Uh oh!

vincent-d left a comment

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!

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!

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!

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!

aabadie commented May 28, 2018

Uh oh!

aabadie commented May 28, 2018

Uh oh!

vincent-d left a comment

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!

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!

vincent-d commented May 28, 2018

Uh oh!

vincent-d left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented May 25, 2018 •

edited

Loading

aabadie commented May 26, 2018 •

edited

Loading

aabadie commented May 28, 2018 •

edited

Loading

aabadie commented May 28, 2018 •

edited

Loading