drivers: i2c: i2c_stm32_v2: stability improvements #89501

erian747 · 2025-05-05T20:26:25Z

Due to various reported stability issues with the STM32 I2C V2 driver in interrupt mode, the controller part is rewritten in an attempt to closely follow documentation found in reference manual and errata sheets.

See #70077

A side effect should also be increased performance

This PR replaces the old draft at #82940

erwango

First of all, thanks @erian747 for the work on this driver and the time you're taking to share your improvements.

Before starting the review, a quick note from my part:
while I do believe this definitely brings important improvements to the current driver, we should also make sure this doesn't bring regression to others (not a surprise to you I guess).
Then, despite the issues you faced, this driver is used w/o trouble in number of applications, which tells that driver behavior depends on the use cases and also that it's difficult to reproduce all conditions in test environment.
So, on the hypothesis that everything can't be verified by testing, we should take special care in the review.

In order to make this PR progress at a reasonable speed, here are few things that will help us I the review
Is there a possibility to split this PR into smaller chunks that would be easier to swallow in reviews ?
If not, further splitting the PR in more single purpose commits would greatly help in the review process. You made efforts on that already but I had a quick look and it's still not easy to track changes. I know by experience that this is a tedious task, but this is really efficient (this usually also helps to find bugs).
Finally, would you be able to provide more indications on the tests that were made on this change, like the targets used, kind of tests (Zephyr tests/drivers ones for instance), ... ?

Last preliminary note on the change:
Drop of LL usage, even though they are justified, imply potential issue in further maintenance, as we lose the thin hw portability it brings.
This concern depends on IPs, some are more or less stable across series. Fortunately, I2C seems relatively stable in that regard, but yet...
So when possible, let's try to minimize removal of LL.

erian747 · 2025-05-18T07:58:09Z

Thanks for the comment
Regarding "regression to other" i completely agree, especially the "target" mode probably needs some testing and adjustments
DMA and non interrupt mode should ofcourse also be tested (even if i "believe" i haven't made any regression there)

What i have tested is master mode with irq on real hardware: STM32F071 and STM32H747 platforms with various types of sensors ranging from small messages with single byte data to messages with up to 1536 bytes and it seems solid
Would be glad if someone using target mode could run a few test after i have made a few changes i already now know about should be done

Regarding splitting commits in to smaller chunks, i understand what you mean but the complicated commit "Rework interrupt handler" which required deep insight into how the I2C IP works im not sure it helps splitting it, because the changes are tightly coupled together. What i know would really be helpful during a review is a specification in form of a state diagram, activity diagram etc describing how things suppose to work, i will look into that...

For LL usage, i saw there is a function LL_I2C_HandleTransfer() that can help removing a few direct register writes without having any impact on code, and also some writes to CR1 can be replaced by LL functions

I will update the PR as soon as i have time

erwango · 2025-05-23T09:48:26Z

Heads up: You'll probably be (badly) hit by #87731...

erian747 · 2025-05-23T11:31:45Z

Thanks for mentioning it, seems like its mostly renaming done in i2c_ll_stm32_v2.c so shouldn't be hard to manage

etienne-lms

The changes look sane to me.
Some minor comments.

Since there are now quite common few common functions between the irq and non-irq implementation, I wonder if it would help to have 3 source files. Well, maybe in a later change.

@erwango, with this change, there are a few raw manipulations of the I2C registers content (interrupt bits, CR1/CR2 fields loading, ...). I'm pretty fine with that but maybe you prefer we stick at most to the LL API? Since we're in a STM32 I2C v2 controller driver, all related SoCs have common fields/macros naming in the LL. It seems reliable to me (from a maintenance perspective).

drivers/i2c/i2c_ll_stm32_v2.c

erian747 · 2025-06-26T19:22:33Z

The changes look sane to me. Some minor comments.

Since there are now quite common few common functions between the irq and non-irq implementation, I wonder if it would help to have 3 source files. Well, maybe in a later change.

@erwango, with this change, there are a few raw manipulations of the I2C registers content (interrupt bits, CR1/CR2 fields loading, ...). I'm pretty fine with that but maybe you prefer we stick at most to the LL API? Since we're in a STM32 I2C v2 controller driver, all related SoCs have common fields/macros naming in the LL. It seems reliable to me (from a maintenance perspective).

I also think it would be a good idea to split up the file, right now it is a bit to big to be readable
But as you say, probably better to make a separate small PR for that

erian747 · 2025-06-26T19:24:05Z

I think most of the comments have been addressed in most recent push

erian747 · 2025-06-26T22:10:50Z

Very quick and ugly sketch

Good thing when drawing thigs like this is that i realized the condition marked with red ??? probably could/should never happen
But easy to get lost so im going to think a bit more about it before modifying the code

etienne-lms

~~More~~ Most of the review comments below are related to coding style issues like
s/if (foo & mask)/if ((foo & mask) != 0U)/.

Commit "drivers: i2c_stm32_v2: Remove obsolete functions" claims to only remove obsolete functions but it does more. I think it could only do that, the other change could be move in other related commits, already present in this commit series.

That said, those changes look good to me and address some corner cases as they claim to.

drivers/i2c/i2c_ll_stm32_v2.c

etienne-lms

Commit "drivers: i2c_stm32_v2: DMA fixup" fixes an regression introduced by a former commit in the series. It could be better all commits are consistent and add no regression (build and run) when possible. That helps a lot when digging for a regression using git bisect or like.
I would suggest to add insert a preparatory commit at the beginning of your patch series to introduce dma_xfert_start() and dma_finish() local helper functions and have msg_init() calling them.

drivers/i2c/i2c_ll_stm32_v2.c

erian747 · 2025-07-03T06:30:29Z

Was a bit hard to move DMA changes to an initial commit, lots of conflicts with commits following
but i did give it a go, and atleast the resulting code after the final commit in the patch sequence matches with how it was before
so total behavior of this PR should be unaffected by latest push

erian747 · 2025-07-07T06:54:03Z

New push again
Fix for DMA transfers when message size is > 255 bytes
I also extended the refactoring in commit "Refactor end of master transfer" as the stm32_i2c_irq_xfer function became quite long
So i moved the second part of the function into a new function "stm32_i2c_irq_msg_finish"

sonarqubecloud · 2025-07-12T06:06:46Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

zhang-wenchao · 2025-09-15T15:37:14Z

Is there any problem with this not being updated?

erian747 · 2025-09-15T16:41:59Z

I am waiting for approval from reviewers, noting more planned for me to change

erwango · 2025-09-17T09:38:05Z

drivers/i2c/i2c_ll_stm32_v2.c

 	struct i2c_stm32_data *data = dev->data;
 	const struct i2c_stm32_config *cfg = dev->config;
 	I2C_TypeDef *i2c = cfg->i2c;


Nitpick: those declarations should go at the beginning of the new block

drivers/i2c/i2c_ll_stm32_v2.c

etienne-lms

Some minor comments. I'm still looking at the overall changes.

etienne-lms · 2025-07-15T15:44:16Z

drivers/i2c/i2c_ll_stm32_v2.c

+	}
+}
+
+static inline void dma_finish(const struct device *dev, struct i2c_msg *msg)


I don't think inline helps a lot here. I would remove it and rely on compiler and project directives to get the expected optimized embedded code.
That's said, it not a blocking issue.

etienne-lms · 2025-09-17T09:49:49Z

drivers/i2c/i2c_ll_stm32_v2.c

+		if (data->current.len != 0U) {
+			/* Configure TX DMA */
+			data->dma_blk_cfg.source_address =
+				(uint32_t)data->current.buf;


Suggested change

(uint32_t)data->current.buf;

data->dma_blk_cfg.source_address = (uint32_t)data->current.buf;

etienne-lms · 2025-09-17T09:55:37Z

drivers/i2c/i2c_ll_stm32_v2.c

+	if ((msg->flags & I2C_MSG_READ) != 0U) {
+		dma_stop(cfg->rx_dma.dev_dma, cfg->rx_dma.dma_channel);
+		LL_I2C_DisableDMAReq_RX(cfg->i2c);
+#if defined(CONFIG_DCACHE)


You can remove this guard. It's already handled in stm32_cache.h.

etienne-lms · 2025-09-17T09:57:41Z

drivers/i2c/i2c_ll_stm32_v2.c

+		LL_I2C_DisableDMAReq_RX(cfg->i2c);
+#if defined(CONFIG_DCACHE)
+		if (!stm32_buf_in_nocache((uintptr_t)msg->buf, msg->len)) {
+			sys_cache_data_invd_range((void *)msg->buf, msg->len);


Despite many calls to sys_cache_data_invd_range() do cast the 1st pointer arg to (void *), it is not needed.

etienne-lms · 2025-09-17T10:03:57Z

drivers/i2c/i2c_ll_stm32_v2.c

-	}
+		data->current.len -= xfer_len;
+		data->current.buf += xfer_len;
+#endif


Please add an empty line below.

etienne-lms · 2025-09-17T10:19:30Z

drivers/i2c/i2c_ll_stm32_v2.c

 		goto end;
 	}
-
+	/*


Could you preserve the empty line above.

By the way, to conform with Zephyr multi-line inline comment coding style, prefer below (and elsewhere):

- /* - * Blablabla... + /* Blablabla... * blablabla */

This is not a block comment.

Empty line fixed and multi-line comment at this location

etienne-lms · 2025-09-17T10:21:31Z

drivers/i2c/i2c_ll_stm32_v2.c

+#ifdef CONFIG_I2C_STM32_V2_DMA
+	/* Stop DMA and invalidate cache if needed */
+	dma_finish(dev, msg);
+#endif


An empty line below is welcome.

etienne-lms · 2025-09-17T10:23:34Z

drivers/i2c/i2c_ll_stm32_v2.c

+		data->master_active = false;
+	}
+	/* Don't disable I2C if a slave is attached */
+	keep_enabled |= data->slave_attached;


Prefer avoid numerical OR operations on boolean variables.
if (data->slave_attached) { keep_enabled = true; }

etienne-lms · 2025-09-17T10:26:18Z

drivers/i2c/i2c_ll_stm32_v2.c

 	return stm32_i2c_irq_msg_finish(dev, msg);
+
+#ifdef CONFIG_I2C_STM32_V2_DMA
+dma_error:


Since this is called from only 1 place, would you be ok to move that code to line 807?

etienne-lms · 2025-09-17T14:20:32Z

drivers/i2c/i2c_ll_stm32_v2.c

+	if (!stm32_buf_in_nocache((uintptr_t)msg->buf, msg->len) &&
+		((msg->flags & I2C_MSG_RW_MASK) == I2C_MSG_WRITE)) {


Suggested change

if (!stm32_buf_in_nocache((uintptr_t)msg->buf, msg->len) &&

((msg->flags & I2C_MSG_RW_MASK) == I2C_MSG_WRITE)) {

if (!stm32_buf_in_nocache((uintptr_t)msg->buf, msg->len) &&

((msg->flags & I2C_MSG_RW_MASK) == I2C_MSG_WRITE)) {

To prepare for further patches and separation of interrupt/DMA modes from polled, this commit introduces two helpers for starting and stopping DMA transfers Signed-off-by: Erik Andersson <erian747@gmail.com>

- Rework transaction flow to better match reference manual and errata sheet for STM32 I2Cv2 peripheral - Handle message transactions larger than 255 bytes in isr - Combine stm32_i2c_msg_write and stm32_i2c_msg_read into one common function stm32_i2c_irq_xfer when using interrupts Signed-off-by: Erik Andersson <erian747@gmail.com>

stm32_i2c_msg_read and stm32_i2c_msg_write for interrupt mode is replaced by a common function: stm32_i2c_irq_xfer Signed-off-by: Erik Andersson <erian747@gmail.com>

Adapt DMA implementation to match changes introduced by new interrupt handler Signed-off-by: Erik Andersson <erian747@gmail.com>

Don't abort transaction on bus error in master mode according to errata sheet Signed-off-by: Erik Andersson <erian747@gmail.com>

The stm32_i2c_master_mode_end function can be replaced with a simple irq disable and giving to the device sync semaphore Signed-off-by: Erik Andersson <erian747@gmail.com>

If configuration of DMA is unsuccessful then abort transaction and return with an error code Signed-off-by: Erik Andersson <erian747@gmail.com>

sonarqubecloud · 2025-09-17T21:35:54Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions bot added area: I2C platform: STM32 ST Micro STM32 labels May 5, 2025

github-actions bot requested review from djiatsaf-st, erwango, FRASTM, gautierg-st, GeorgeCGV, marwaiehm-st, mathieuchopstm and teburd May 5, 2025 20:27

github-actions bot assigned erwango May 5, 2025

erwango reviewed May 16, 2025

View reviewed changes

FRASTM linked an issue Jun 9, 2025 that may be closed by this pull request

STM32 I2C v2 lockup with invalid data and read/write #70077

Open

erian747 force-pushed the stm32_i2c_v2_stability_improvements branch from 3bf5c8a to 8699f7e Compare June 25, 2025 20:15

github-actions bot requested a review from etienne-lms June 25, 2025 20:16

erian747 force-pushed the stm32_i2c_v2_stability_improvements branch from 4f76a7d to 8699f7e Compare June 25, 2025 20:20

etienne-lms reviewed Jun 26, 2025

View reviewed changes

erian747 force-pushed the stm32_i2c_v2_stability_improvements branch from 8699f7e to e1ee156 Compare June 26, 2025 19:08

erian747 force-pushed the stm32_i2c_v2_stability_improvements branch 3 times, most recently from 90b62af to 50d7ede Compare June 27, 2025 06:56

etienne-lms reviewed Jun 27, 2025

View reviewed changes

erian747 force-pushed the stm32_i2c_v2_stability_improvements branch from 50d7ede to 70aaf4a Compare June 28, 2025 04:22

etienne-lms reviewed Jun 30, 2025

View reviewed changes

drivers/i2c/i2c_ll_stm32_v2.c Outdated Show resolved Hide resolved

drivers/i2c/i2c_ll_stm32_v2.c Outdated Show resolved Hide resolved

drivers/i2c/i2c_ll_stm32_v2.c Outdated Show resolved Hide resolved

drivers/i2c/i2c_ll_stm32_v2.c Outdated Show resolved Hide resolved

erian747 force-pushed the stm32_i2c_v2_stability_improvements branch from 70aaf4a to a072e61 Compare July 3, 2025 06:20

mathieuchopstm added this to the v4.3.0 milestone Jul 3, 2025

erian747 force-pushed the stm32_i2c_v2_stability_improvements branch from a072e61 to 4640a24 Compare July 7, 2025 06:45

erian747 force-pushed the stm32_i2c_v2_stability_improvements branch 3 times, most recently from ab8c21d to 23b048b Compare July 12, 2025 05:27

erian747 requested a review from etienne-lms September 2, 2025 03:40

zhang-wenchao mentioned this pull request Sep 15, 2025

driver: stm32-i2c-v2: negative return from i2c_target_read_processed causes lockup. #93881

Open

1 task

erwango reviewed Sep 17, 2025

View reviewed changes

gautierg-st reviewed Sep 17, 2025

View reviewed changes

drivers/i2c/i2c_ll_stm32_v2.c Outdated Show resolved Hide resolved

drivers/i2c/i2c_ll_stm32_v2.c Outdated Show resolved Hide resolved

drivers/i2c/i2c_ll_stm32_v2.c Outdated Show resolved Hide resolved

etienne-lms reviewed Sep 17, 2025

View reviewed changes

drivers: i2c_stm32_v2: Add DMA helpers

2178302

To prepare for further patches and separation of interrupt/DMA modes from polled, this commit introduces two helpers for starting and stopping DMA transfers Signed-off-by: Erik Andersson <erian747@gmail.com>

erian747 force-pushed the stm32_i2c_v2_stability_improvements branch from 23b048b to c60e056 Compare September 17, 2025 20:55

erian747 added 6 commits September 17, 2025 23:02

drivers: i2c_stm32_v2: Remove obsolete functions

f5e1447

stm32_i2c_msg_read and stm32_i2c_msg_write for interrupt mode is replaced by a common function: stm32_i2c_irq_xfer Signed-off-by: Erik Andersson <erian747@gmail.com>

drivers: i2c_stm32_v2: DMA fixup

f47347f

Adapt DMA implementation to match changes introduced by new interrupt handler Signed-off-by: Erik Andersson <erian747@gmail.com>

drivers: i2c_stm32_v2: Bus error errata fix

01ef43a

Don't abort transaction on bus error in master mode according to errata sheet Signed-off-by: Erik Andersson <erian747@gmail.com>

drivers: i2c_stm32_v2: Refactor end of master transfer

f67c05e

The stm32_i2c_master_mode_end function can be replaced with a simple irq disable and giving to the device sync semaphore Signed-off-by: Erik Andersson <erian747@gmail.com>

drivers: i2c_stm32_v2: Abort on DMA error

3d24ec2

If configuration of DMA is unsuccessful then abort transaction and return with an error code Signed-off-by: Erik Andersson <erian747@gmail.com>

erian747 force-pushed the stm32_i2c_v2_stability_improvements branch from c60e056 to 3d24ec2 Compare September 17, 2025 21:09

	(uint32_t)data->current.buf;
	data->dma_blk_cfg.source_address = (uint32_t)data->current.buf;

		if (!stm32_buf_in_nocache((uintptr_t)msg->buf, msg->len) &&
		((msg->flags & I2C_MSG_RW_MASK) == I2C_MSG_WRITE)) {

drivers: i2c: i2c_stm32_v2: stability improvements #89501

Are you sure you want to change the base?

drivers: i2c: i2c_stm32_v2: stability improvements #89501

Conversation

erian747 commented May 5, 2025

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

erian747 commented May 18, 2025

Uh oh!

erwango commented May 23, 2025

Uh oh!

erian747 commented May 23, 2025

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erian747 commented Jun 26, 2025

Uh oh!

erian747 commented Jun 26, 2025

Uh oh!

erian747 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etienne-lms left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erian747 commented Jul 3, 2025

Uh oh!

erian747 commented Jul 7, 2025

Uh oh!

sonarqubecloud bot commented Jul 12, 2025

Quality Gate passed

Uh oh!

zhang-wenchao commented Sep 15, 2025

Uh oh!

erian747 commented Sep 15, 2025

Uh oh!

erwango Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

erian747 commented Jun 26, 2025 •

edited

Loading

etienne-lms left a comment •

edited

Loading

erwango Sep 17, 2025 •

edited

Loading