i2c: stm32-i2c-(v1/v2) don't handle i2c_burst_write like expected

[dwagenk]

Discussion for this issue started in #4429

with samples/drivers/i2c_fujitsu_fram adapted to use I2C_1 as I2C_DEV or with a sample app using i2c_burst_write16 function there's the following issue.

Expected behaviour (S = start, P = stop):
S - SLAVE_ADDR|W - FRAM_ADDR_HIGH - FRAM_ADDR_LOW - BYTE_0 - [...] BYTE_n - P
There should be no STOP or REPEATED start condition between the FRAM address and the data.

Behaviour of stm32-i2c drivers:
S - SLAVE_ADDR|W - FRAM_ADDR_HIGH - FRAM_ADDR_LOW - P -
S - SLAVE_ADDR|W - BYTE_0 - [...] BYTE_n - P

[dwagenk]

just for reference: mcux i2c driver seems to be unable to handle messages that lack the I2C_MSG_STOP flag all together. Tried to get some reference on how other boards handle these cases with the frdm_kw41z board I have.

[ydamigos]

@dwagenk I started working on the issue on my personal repository (https://github.com/ydamigos/zephyr/tree/stm32-i2c). Now the stm32 I2C driver uses the I2C API flags, but there are still some loose ends to tie up to make the API's i2c_burst_write functions to work properly.

[dwagenk]

@ydamigos I just pushed a version to fix this problem in v2 driver to my personal repo. Functionality is tested in non-interrupt mode on stm32f3_disco and works good.
See https://github.com/DWagenk/zephyr/commit/f274f36c425fbbada98931e3ff02bf7074b94412 or https://github.com/DWagenk/zephyr/tree/stm32_i2c_adapt_driver_v2_to_match_API
Lets compare and see which approach we want to take.

Since I don't like reusing the flags with different meaning I pass flags_next_msg into the functions, so both are available:
this messages flags msg->flags
next messages flags flags_next_message
The rest of the code looks pretty straightforward this way.

[dwagenk]

functionality is tested in interrupt mode as well: works good, I just don't like the while loops in msg_done when we could use interrupts, but that's nothing to change here/now.

[ydamigos]

@dwagenk I agree with you and ydamigos@76ffd76 just uses the I2C API flags. The only trick the code does is to set the I2C_MSG_RESTART flag on the first message in order to create the start condition. When the message doesn't have a I2C_MSG_STOP and it is a I2C_MSG_WRITE it enables the RELOAD mode.
I tested the non-interrupt mode for v1/v2 and it worked. Interrupt mode works on v1 (I want to test it a littler more) and I didn't have the time to check it on v2.

[dwagenk]

@ydamigos I looked into your code and the trick with the I2C_MSG_RESTART flag for first message looks good to me.
I don't know if we get by, without having the knowledge inside the driver, whether next message starts with a restart condition or without one.
If we don't make any assumptions based on message direction inside the driver (like your decision to enable reload mode in i2c_ll_stm32_v2.c line 34 is based on), the API can be used as flexible as in the example code attached, glueing messages together, without having to prepare a single buffer in the app.
For v2 driver this worked with my fork. Even Messages with 300+ bytes and no start/stop/restart condition in between, as long as every single message was less than 255 bytes long.

example.c.txt

[ydamigos]

If we don't make any assumptions based on message direction inside the driver (like your decision to enable reload mode in i2c_ll_stm32_v2.c line 34 is based on)

@dwagenk I didn't mean to check message direction, just if it has a I2C_MSG_STOP flag. I must have mixed something during a rebase. I will check it a little bit more to see if we could avoid the knowledge, whether next message starts with a restart condition or without one. We should use the same i2c_ll_stm32.c file without ifdefs regarding stm32 I2C version.

[dwagenk]

@ydamigos ydamigos

We should use the same i2c_ll_stm32.c file without ifdefs regarding stm32 I2C version.

yes, definitely, I just started on v2 and didn't wan't to break v1 before working on it, so I left the old version in place for v1 driver.

I will check it a little bit more to see if we could avoid the knowledge, whether next message starts with a restart condition or without one.

Since this problem should be not only relevant for stm32 we could think of reserving 1 bit (or 2 if we can think of yet another use case where it might be needed) of i2c_msg->flags for driver internal use in general. Would be a bigger change, but might be worth it.
Then we could define I2C_MSG_NEXT_IS_RESTART in i2c_ll_stm32.h and avoid passing around additional flag-bytes.

[ydamigos]

@dwagenk I merged both approaches in the following ydamigos@7a112a0 keeping us both as authors. Unfortunately, I didn't find the time to check it on hardware. If it is ok with you, let's work on this commit as a base.

[dwagenk]

@ydamigos I tested v2 driver with stm32f3_disco board and adapted sample i2c_fujitsu_fram file https://gist.github.com/DWagenk/8260d1958e114f4104656190c121fbcf which also contains a long write, that is composed of multiple messages that need to be concatenated by the driver.

• non-interrupt mode works
• interrupt mode only works when I revert the changes in stm32_i2c_event_isr, so this needs some attention.

[dwagenk]

interrupt mode works now, see ydamigos@7a112a0#commitcomment-25177689

[ydamigos]

@dwagenk Thanks for checking it. I hope I will find time to check it with stm32 I2C v1 hardware before creating a PR.

[ydamigos]

PR #4572 was merged.