When i2c_dw_xfer() is called with more than one message, it sets the
target address according to the first message. If any of the following
messages have a different target address, the transfer finishes with
an error.
Instead, if the next message has a different target address, wait until
all previous messages are sent and the STOP condition is detected. This
will complete the current part of the transfer. The next part is then
handled by looping in i2c_dw_xfer(), calling i2c_dw_xfer_init() and
i2c_dw_wait_transfer() until all messages of the transfer have been
processed, or an error is detected.
The RESTART bit is now set after the first message of each part of the
transfer, instead of just after the very first message of the whole
transfer.
For each address change, i2c_dw_xfer_init() is called, which takes care
of disabling the adapter before changing the target address register,
then re-enabling it. Given that we cannot know the value of the
I2C_DYNAMIC_TAR_UPDATE parameter, this is the only sure way to change
the target address.
Based on the work of Dmitry Guzman <dmitry.guzman@mobileye.com>
Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
drivers/i2c/busses/i2c-designware-master.c | 58 ++++++++++++++++--------------
1 file changed, 31 insertions(+), 27 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index c7a72c28786c2..f9a180b145da8 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -436,6 +436,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
u8 *buf = dev->tx_buf;
bool need_restart = false;
unsigned int flr;
+ int first_idx = dev->msg_write_idx;
intr_mask = DW_IC_INTR_MASTER_MASK;
@@ -446,11 +447,11 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
* If target address has changed, we need to
* reprogram the target address in the I2C
* adapter when we are done with this transfer.
+ * This can be done after STOP_DET IRQ flag is raised.
+ * So, disable "TX FIFO empty" interrupt.
*/
if (msgs[dev->msg_write_idx].addr != addr) {
- dev_err(dev->dev,
- "%s: invalid target address\n", __func__);
- dev->msg_err = -EINVAL;
+ intr_mask &= ~DW_IC_INTR_TX_EMPTY;
break;
}
@@ -465,7 +466,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
* set restart bit between messages.
*/
if ((dev->master_cfg & DW_IC_CON_RESTART_EN) &&
- (dev->msg_write_idx > 0))
+ (dev->msg_write_idx > first_idx))
need_restart = true;
}
@@ -822,7 +823,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
break;
}
- reinit_completion(&dev->cmd_complete);
dev->msgs = msgs;
dev->msgs_num = num;
dev->cmd_err = 0;
@@ -841,18 +841,33 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
if (ret < 0)
goto done;
- /* Start the transfers */
- i2c_dw_xfer_init(dev);
+ do {
+ reinit_completion(&dev->cmd_complete);
- /* Wait for tx to complete */
- ret = i2c_dw_wait_transfer(dev);
- if (ret) {
- dev_err(dev->dev, "controller timed out\n");
- /* i2c_dw_init_master() implicitly disables the adapter */
- i2c_recover_bus(&dev->adapter);
- i2c_dw_init_master(dev);
- goto done;
- }
+ /* Start the transfers */
+ i2c_dw_xfer_init(dev);
+
+ /* Wait for tx to complete */
+ ret = i2c_dw_wait_transfer(dev);
+ if (ret) {
+ dev_err(dev->dev, "controller timed out\n");
+ /* i2c_dw_init_master() implicitly disables the adapter */
+ i2c_recover_bus(&dev->adapter);
+ i2c_dw_init_master(dev);
+ goto done;
+ }
+
+ if (dev->msg_err) {
+ ret = dev->msg_err;
+ goto done;
+ }
+
+ /* We have an error */
+ if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
+ ret = i2c_dw_handle_tx_abort(dev);
+ goto done;
+ }
+ } while (dev->msg_write_idx < num);
/*
* This happens rarely (~1:500) and is hard to reproduce. Debug trace
@@ -874,23 +889,12 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
*/
__i2c_dw_disable_nowait(dev);
- if (dev->msg_err) {
- ret = dev->msg_err;
- goto done;
- }
-
/* No error */
if (likely(!dev->cmd_err && !dev->status)) {
ret = num;
goto done;
}
- /* We have an error */
- if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
- ret = i2c_dw_handle_tx_abort(dev);
- goto done;
- }
-
if (dev->status)
dev_err(dev->dev,
"transfer terminated early - interrupt latency too high?\n");
--
2.51.0
Hi Benoît,
On 17/10/2025 16:59, Benoît Monin wrote:
> When i2c_dw_xfer() is called with more than one message, it sets the
> target address according to the first message. If any of the following
> messages have a different target address, the transfer finishes with
> an error.
>
> Instead, if the next message has a different target address, wait until
> all previous messages are sent and the STOP condition is detected. This
> will complete the current part of the transfer. The next part is then
> handled by looping in i2c_dw_xfer(), calling i2c_dw_xfer_init() and
> i2c_dw_wait_transfer() until all messages of the transfer have been
> processed, or an error is detected.
>
> The RESTART bit is now set after the first message of each part of the
> transfer, instead of just after the very first message of the whole
> transfer.
>
> For each address change, i2c_dw_xfer_init() is called, which takes care
> of disabling the adapter before changing the target address register,
> then re-enabling it. Given that we cannot know the value of the
> I2C_DYNAMIC_TAR_UPDATE parameter, this is the only sure way to change
> the target address.
I have the problem described here:
https://lore.kernel.org/linux-i2c/ee6afdd7-3117-43cd-831f-e0ec5ee46f46@kernel.org/
And it looks like this patch is intended to solve that problem (one transaction
with two writes to different target addresses).
I tried this patch, but it doesn't work. Instead I get a time out:
[ 111.695238] i2c_designware 1f00074000.i2c: controller timed out
Is it indeed meant to solve the problem I have or is it addressing another
issue?
I'm happy to help test patches.
Regards,
Hans
>
> Based on the work of Dmitry Guzman <dmitry.guzman@mobileye.com>
>
> Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
> ---
> drivers/i2c/busses/i2c-designware-master.c | 58 ++++++++++++++++--------------
> 1 file changed, 31 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index c7a72c28786c2..f9a180b145da8 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -436,6 +436,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> u8 *buf = dev->tx_buf;
> bool need_restart = false;
> unsigned int flr;
> + int first_idx = dev->msg_write_idx;
>
> intr_mask = DW_IC_INTR_MASTER_MASK;
>
> @@ -446,11 +447,11 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> * If target address has changed, we need to
> * reprogram the target address in the I2C
> * adapter when we are done with this transfer.
> + * This can be done after STOP_DET IRQ flag is raised.
> + * So, disable "TX FIFO empty" interrupt.
> */
> if (msgs[dev->msg_write_idx].addr != addr) {
> - dev_err(dev->dev,
> - "%s: invalid target address\n", __func__);
> - dev->msg_err = -EINVAL;
> + intr_mask &= ~DW_IC_INTR_TX_EMPTY;
> break;
> }
>
> @@ -465,7 +466,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> * set restart bit between messages.
> */
> if ((dev->master_cfg & DW_IC_CON_RESTART_EN) &&
> - (dev->msg_write_idx > 0))
> + (dev->msg_write_idx > first_idx))
> need_restart = true;
> }
>
> @@ -822,7 +823,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> break;
> }
>
> - reinit_completion(&dev->cmd_complete);
> dev->msgs = msgs;
> dev->msgs_num = num;
> dev->cmd_err = 0;
> @@ -841,18 +841,33 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> if (ret < 0)
> goto done;
>
> - /* Start the transfers */
> - i2c_dw_xfer_init(dev);
> + do {
> + reinit_completion(&dev->cmd_complete);
>
> - /* Wait for tx to complete */
> - ret = i2c_dw_wait_transfer(dev);
> - if (ret) {
> - dev_err(dev->dev, "controller timed out\n");
> - /* i2c_dw_init_master() implicitly disables the adapter */
> - i2c_recover_bus(&dev->adapter);
> - i2c_dw_init_master(dev);
> - goto done;
> - }
> + /* Start the transfers */
> + i2c_dw_xfer_init(dev);
> +
> + /* Wait for tx to complete */
> + ret = i2c_dw_wait_transfer(dev);
> + if (ret) {
> + dev_err(dev->dev, "controller timed out\n");
> + /* i2c_dw_init_master() implicitly disables the adapter */
> + i2c_recover_bus(&dev->adapter);
> + i2c_dw_init_master(dev);
> + goto done;
> + }
> +
> + if (dev->msg_err) {
> + ret = dev->msg_err;
> + goto done;
> + }
> +
> + /* We have an error */
> + if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
> + ret = i2c_dw_handle_tx_abort(dev);
> + goto done;
> + }
> + } while (dev->msg_write_idx < num);
>
> /*
> * This happens rarely (~1:500) and is hard to reproduce. Debug trace
> @@ -874,23 +889,12 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> */
> __i2c_dw_disable_nowait(dev);
>
> - if (dev->msg_err) {
> - ret = dev->msg_err;
> - goto done;
> - }
> -
> /* No error */
> if (likely(!dev->cmd_err && !dev->status)) {
> ret = num;
> goto done;
> }
>
> - /* We have an error */
> - if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
> - ret = i2c_dw_handle_tx_abort(dev);
> - goto done;
> - }
> -
> if (dev->status)
> dev_err(dev->dev,
> "transfer terminated early - interrupt latency too high?\n");
>
Hello Hans, On Monday, 20 October 2025 at 11:38:38 CEST, Hans Verkuil wrote: > Hi Benoît, > > On 17/10/2025 16:59, Benoît Monin wrote: > > When i2c_dw_xfer() is called with more than one message, it sets the > > target address according to the first message. If any of the following > > messages have a different target address, the transfer finishes with > > an error. > > > > Instead, if the next message has a different target address, wait until > > all previous messages are sent and the STOP condition is detected. This > > will complete the current part of the transfer. The next part is then > > handled by looping in i2c_dw_xfer(), calling i2c_dw_xfer_init() and > > i2c_dw_wait_transfer() until all messages of the transfer have been > > processed, or an error is detected. > > > > The RESTART bit is now set after the first message of each part of the > > transfer, instead of just after the very first message of the whole > > transfer. > > > > For each address change, i2c_dw_xfer_init() is called, which takes care > > of disabling the adapter before changing the target address register, > > then re-enabling it. Given that we cannot know the value of the > > I2C_DYNAMIC_TAR_UPDATE parameter, this is the only sure way to change > > the target address. > > I have the problem described here: > > https://lore.kernel.org/linux-i2c/ee6afdd7-3117-43cd-831f-e0ec5ee46f46@kernel.org/ > > And it looks like this patch is intended to solve that problem (one transaction > with two writes to different target addresses). > > I tried this patch, but it doesn't work. Instead I get a time out: > > [ 111.695238] i2c_designware 1f00074000.i2c: controller timed out > > Is it indeed meant to solve the problem I have or is it addressing another > issue? > For your particular case, that will not help reaching the other segments as we wait for a STOP before changing the target address. So, it should not fail but do a write to segment 0 in your eeprom. > I'm happy to help test patches. > Can you enable debug in i2c-designware-master to see which transaction is failing? Best regards, -- Benoît Monin, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
> For your particular case, that will not help reaching the other segments as > we wait for a STOP before changing the target address. So, it should not > fail but do a write to segment 0 in your eeprom. So, this patch replaces a repeated start with a stop + start combination? Please don't do this. It will give users a false impression that proper repeated start is supported. Honestly reporting that the HW does not support is the better option, so the user can decide what to do then.
On Monday, 20 October 2025 at 21:52:01 CEST, Wolfram Sang wrote: > > > For your particular case, that will not help reaching the other segments as > > we wait for a STOP before changing the target address. So, it should not > > fail but do a write to segment 0 in your eeprom. > > So, this patch replaces a repeated start with a stop + start > combination? Please don't do this. It will give users a false impression > that proper repeated start is supported. Honestly reporting that the HW > does not support is the better option, so the user can decide what to do > then. > This patch replaces a -EINVAL in the middle of the transfer by a STOP-then-START, but you are right, the expectation is to have a single STOP at the end of a combined transfer. I somehow overlooked that part. Maybe I could add support for the I2C_M_STOP flag instead? Or does an adapter has to support all the protocol mangling if flagged with I2C_FUNC_PROTOCOL_MANGLING? That would still allow to group multiple accesses to device that support a STOP in a transaction when done via i2c_dev I2C_RDWR ioctl, in a single-master configuration. Best regards, -- Benoît Monin, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Benoît, > This patch replaces a -EINVAL in the middle of the transfer by a > STOP-then-START, but you are right, the expectation is to have a single > STOP at the end of a combined transfer. I somehow overlooked that part. Yes. Sadly, the designware controllers are broken in that regard, it seems :( > Maybe I could add support for the I2C_M_STOP flag instead? Or does an > adapter has to support all the protocol mangling if flagged with > I2C_FUNC_PROTOCOL_MANGLING? It doesn't. It is a known weakness of I2C_FUNC_PROTOCOL_MANGLING. Maybe we could extrapolate it like we did for I2C_FUNC_NOSTART? Just an idea, I haven't really looked into it. > That would still allow to group multiple accesses to device that support a > STOP in a transaction when done via i2c_dev I2C_RDWR ioctl, in a single-master > configuration. I'd think such transfers are super rare but if you still want to implement it, please do. Note that some devices will still not work because they reset their state machine on STOP. See Hans' example. Happy hacking, Wolfram
© 2016 - 2026 Red Hat, Inc.