If IC_EMPTYFIFO_HOLD_MASTER_EN parameter is 0, "Stop" and "Repeated Start"
bits in command register does not exist, thus it is impossible to send
several consecutive write messages in a single hardware batch. The
existing implementation worked with such configuration incorrectly:
all consecutive write messages are joined into a single message without
any Start/Stop or Repeated Start conditions. For example, the following
command:
i2ctransfer -y 0 w1@0x55 0x00 w1@0x55 0x01
does the same as
i2ctransfer -y 0 w2@0x55 0x00 0x01
In i2c_dw_xfer(), we ensure that we do not have such sequence of messages
requiring a RESTART, aborting the transfer on controller that cannot
emit them explicitly.
This behavior is activated by compatible entries because the state of
the IC_EMPTYFIFO_HOLD_MASTER_EN parameter cannot be detected at runtime.
Add the compatible entry for Mobileye SoCs needing the workaround.
There is another possible problem with this controller configuration:
When the CPU is putting commands to the FIFO, this process must not be
interrupted because if FIFO buffer gets empty, the controller finishes
the I2C transaction and generates STOP condition on the bus.
If we continue writing the remainder of the message to the FIFO, the
controller will start emitting a new transaction with those data. This
turns a single a single message into multiple I2C transactions. To
ensure that we do not keep processing a message after a FIFO underrun,
checks are added in two places.
First in i2c_dw_xfer_msg() we check the raw interrupt status register to
see if a STOP condition was detected while filling the FIFO, and abort
if so. This can happen with threaded interrupt on a PREEMPT_RT kernel
if we are preempted during the processing of each bytes of the message.
Second in i2c_dw_process_transfer(), we abort if a STOP is detected
while a read or a write is in progress. This can occur when processing
a message larger than the FIFO. In that case the message is processed in
parts, and rely on the TW EMPTY interrupt to refill the FIFO when it gets
below a threshold. If servicing this interrupt is delayed for too long,
it can trigger a FIFO underrun, thus an unwanted STOP.
Originally-by: Dmitry Guzman <dmitry.guzman@mobileye.com>
Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
drivers/i2c/busses/i2c-designware-core.h | 1 +
drivers/i2c/busses/i2c-designware-master.c | 51 +++++++++++++++++++++++++++++
drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
3 files changed, 53 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 347843b4f5dd..a31a8698e511 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -311,6 +311,7 @@ struct dw_i2c_dev {
#define ACCESS_NO_IRQ_SUSPEND BIT(1)
#define ARBITRATION_SEMAPHORE BIT(2)
#define ACCESS_POLLING BIT(3)
+#define NO_EMPTYFIFO_HOLD_MASTER BIT(4)
#define MODEL_MSCC_OCELOT BIT(8)
#define MODEL_BAIKAL_BT1 BIT(9)
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index da1963d25def..329bb69485f4 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -463,6 +463,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
rx_limit = dev->rx_fifo_depth - flr;
while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
+ unsigned int raw_stat;
u32 cmd = 0;
/*
@@ -487,6 +488,21 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
need_restart = false;
}
+ /*
+ * With threaded interrupt on a PREEMPT-RT kernel, we may
+ * be interrupted while filling the FIFO. Abort the
+ * transfer in case of a FIFO underrun on controller that
+ * emits a STOP in that case.
+ */
+ if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER) {
+ regmap_read(dev->map, DW_IC_RAW_INTR_STAT,
+ &raw_stat);
+ if (raw_stat & DW_IC_INTR_STOP_DET) {
+ dev->msg_err = -EIO;
+ goto done;
+ }
+ }
+
if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
/* Avoid rx buffer overrun */
@@ -526,6 +542,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
dev->status &= ~STATUS_WRITE_IN_PROGRESS;
}
+done:
/*
* If i2c_msg index search is completed, we don't need TX_EMPTY
* interrupt any more.
@@ -706,6 +723,14 @@ static void i2c_dw_process_transfer(struct dw_i2c_dev *dev, unsigned int stat)
if (stat & DW_IC_INTR_TX_EMPTY)
i2c_dw_xfer_msg(dev);
+ /* Abort if we detect a STOP in the middle of a read or a write */
+ if ((stat & DW_IC_INTR_STOP_DET) &&
+ (dev->status & (STATUS_READ_IN_PROGRESS | STATUS_WRITE_IN_PROGRESS))) {
+ dev_err(dev->dev, "spurious STOP detected\n");
+ dev->rx_outstanding = 0;
+ dev->msg_err = -EIO;
+ }
+
/*
* No need to modify or disable the interrupt mask here.
* i2c_dw_xfer_msg() will take care of it according to
@@ -872,6 +897,21 @@ __i2c_dw_xfer_unlocked(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
return ret;
}
+/*
+ * Return true if the message needs an explicit RESTART before being sent.
+ * Without an explicit RESTART, two consecutive messages in the same direction
+ * will be merged into a single transfer.
+ * The adapter always emits a RESTART when the direction changes.
+ */
+static inline bool i2c_dw_msg_need_restart(struct i2c_msg msgs[], int idx)
+{
+ /* No need for a RESTART on the first message */
+ if (idx == 0)
+ return false;
+
+ return (msgs[idx - 1].flags & I2C_M_RD) == (msgs[idx].flags & I2C_M_RD);
+}
+
static int
i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
{
@@ -918,6 +958,17 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
goto done;
}
+ /*
+ * Make sure we don't need explicit RESTART for
+ * controllers that cannot emit them.
+ */
+ if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
+ i2c_dw_msg_need_restart(msg, cnt - 1)) {
+ dev_err(dev->dev, "cannot emit RESTART\n");
+ ret = -EINVAL;
+ goto done;
+ }
+
if ((msg[cnt - 1].flags & I2C_M_STOP) ||
(msg + cnt == msgs + num))
break;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d7d764f7554d..4aad3dc51fbc 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -346,6 +346,7 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
static const struct of_device_id dw_i2c_of_match[] = {
{ .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
+ { .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
{ .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
{ .compatible = "snps,designware-i2c" },
{}
--
2.51.1
On Fri, Oct 31, 2025 at 03:35:43PM +0100, Benoît Monin wrote:
> If IC_EMPTYFIFO_HOLD_MASTER_EN parameter is 0, "Stop" and "Repeated Start"
> bits in command register does not exist, thus it is impossible to send
> several consecutive write messages in a single hardware batch. The
> existing implementation worked with such configuration incorrectly:
> all consecutive write messages are joined into a single message without
> any Start/Stop or Repeated Start conditions. For example, the following
> command:
>
> i2ctransfer -y 0 w1@0x55 0x00 w1@0x55 0x01
>
> does the same as
>
> i2ctransfer -y 0 w2@0x55 0x00 0x01
>
> In i2c_dw_xfer(), we ensure that we do not have such sequence of messages
> requiring a RESTART, aborting the transfer on controller that cannot
> emit them explicitly.
>
> This behavior is activated by compatible entries because the state of
> the IC_EMPTYFIFO_HOLD_MASTER_EN parameter cannot be detected at runtime.
> Add the compatible entry for Mobileye SoCs needing the workaround.
>
> There is another possible problem with this controller configuration:
> When the CPU is putting commands to the FIFO, this process must not be
> interrupted because if FIFO buffer gets empty, the controller finishes
> the I2C transaction and generates STOP condition on the bus.
>
> If we continue writing the remainder of the message to the FIFO, the
> controller will start emitting a new transaction with those data. This
> turns a single a single message into multiple I2C transactions. To
> ensure that we do not keep processing a message after a FIFO underrun,
> checks are added in two places.
>
> First in i2c_dw_xfer_msg() we check the raw interrupt status register to
> see if a STOP condition was detected while filling the FIFO, and abort
> if so. This can happen with threaded interrupt on a PREEMPT_RT kernel
> if we are preempted during the processing of each bytes of the message.
>
> Second in i2c_dw_process_transfer(), we abort if a STOP is detected
> while a read or a write is in progress. This can occur when processing
> a message larger than the FIFO. In that case the message is processed in
> parts, and rely on the TW EMPTY interrupt to refill the FIFO when it gets
> below a threshold. If servicing this interrupt is delayed for too long,
> it can trigger a FIFO underrun, thus an unwanted STOP.
>
> Originally-by: Dmitry Guzman <dmitry.guzman@mobileye.com>
> Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
> ---
> drivers/i2c/busses/i2c-designware-core.h | 1 +
> drivers/i2c/busses/i2c-designware-master.c | 51 +++++++++++++++++++++++++++++
> drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
> 3 files changed, 53 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 347843b4f5dd..a31a8698e511 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -311,6 +311,7 @@ struct dw_i2c_dev {
> #define ACCESS_NO_IRQ_SUSPEND BIT(1)
> #define ARBITRATION_SEMAPHORE BIT(2)
> #define ACCESS_POLLING BIT(3)
> +#define NO_EMPTYFIFO_HOLD_MASTER BIT(4)
>
> #define MODEL_MSCC_OCELOT BIT(8)
> #define MODEL_BAIKAL_BT1 BIT(9)
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index da1963d25def..329bb69485f4 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -463,6 +463,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> rx_limit = dev->rx_fifo_depth - flr;
>
> while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
> + unsigned int raw_stat;
> u32 cmd = 0;
>
> /*
> @@ -487,6 +488,21 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> need_restart = false;
> }
>
> + /*
> + * With threaded interrupt on a PREEMPT-RT kernel, we may
> + * be interrupted while filling the FIFO. Abort the
> + * transfer in case of a FIFO underrun on controller that
> + * emits a STOP in that case.
> + */
> + if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER) {
> + regmap_read(dev->map, DW_IC_RAW_INTR_STAT,
> + &raw_stat);
> + if (raw_stat & DW_IC_INTR_STOP_DET) {
> + dev->msg_err = -EIO;
> + goto done;
> + }
> + }
> +
> if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
>
> /* Avoid rx buffer overrun */
> @@ -526,6 +542,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> dev->status &= ~STATUS_WRITE_IN_PROGRESS;
> }
>
> +done:
> /*
> * If i2c_msg index search is completed, we don't need TX_EMPTY
> * interrupt any more.
> @@ -706,6 +723,14 @@ static void i2c_dw_process_transfer(struct dw_i2c_dev *dev, unsigned int stat)
> if (stat & DW_IC_INTR_TX_EMPTY)
> i2c_dw_xfer_msg(dev);
>
> + /* Abort if we detect a STOP in the middle of a read or a write */
> + if ((stat & DW_IC_INTR_STOP_DET) &&
> + (dev->status & (STATUS_READ_IN_PROGRESS | STATUS_WRITE_IN_PROGRESS))) {
> + dev_err(dev->dev, "spurious STOP detected\n");
> + dev->rx_outstanding = 0;
> + dev->msg_err = -EIO;
> + }
> +
> /*
> * No need to modify or disable the interrupt mask here.
> * i2c_dw_xfer_msg() will take care of it according to
> @@ -872,6 +897,21 @@ __i2c_dw_xfer_unlocked(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
> return ret;
> }
>
> +/*
> + * Return true if the message needs an explicit RESTART before being sent.
> + * Without an explicit RESTART, two consecutive messages in the same direction
> + * will be merged into a single transfer.
> + * The adapter always emits a RESTART when the direction changes.
> + */
> +static inline bool i2c_dw_msg_need_restart(struct i2c_msg msgs[], int idx)
This can take const parameters.
> +{
Please move the dev->flags & NO_EMPTYFIFO_HOLD_MASTER here too.
> + /* No need for a RESTART on the first message */
> + if (idx == 0)
> + return false;
That's
if (!idx)
But why not pass the actual message instead of the index?
> +
> + return (msgs[idx - 1].flags & I2C_M_RD) == (msgs[idx].flags & I2C_M_RD);
You don't need the outer parens.
> +}
> +
> static int
> i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> {
> @@ -918,6 +958,17 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> goto done;
> }
>
> + /*
> + * Make sure we don't need explicit RESTART for
> + * controllers that cannot emit them.
> + */
> + if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
> + i2c_dw_msg_need_restart(msg, cnt - 1)) {
> + dev_err(dev->dev, "cannot emit RESTART\n");
> + ret = -EINVAL;
> + goto done;
> + }
> +
> if ((msg[cnt - 1].flags & I2C_M_STOP) ||
> (msg + cnt == msgs + num))
> break;
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d7d764f7554d..4aad3dc51fbc 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -346,6 +346,7 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
>
> static const struct of_device_id dw_i2c_of_match[] = {
> { .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
> + { .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> { .compatible = "snps,designware-i2c" },
> {}
>
> --
> 2.51.1
On Mon, Nov 03, 2025 at 11:43:30AM +0100, Mika Westerberg wrote: > On Fri, Oct 31, 2025 at 03:35:43PM +0100, Benoît Monin wrote: ... > > + return (msgs[idx - 1].flags & I2C_M_RD) == (msgs[idx].flags & I2C_M_RD); > > You don't need the outer parens. Priority of == is 10, while & is 11. -- With Best Regards, Andy Shevchenko
On Mon, Nov 03, 2025 at 03:20:52PM +0200, Andy Shevchenko wrote: > On Mon, Nov 03, 2025 at 11:43:30AM +0100, Mika Westerberg wrote: > > On Fri, Oct 31, 2025 at 03:35:43PM +0100, Benoît Monin wrote: > > ... > > > > + return (msgs[idx - 1].flags & I2C_M_RD) == (msgs[idx].flags & I2C_M_RD); > > > > You don't need the outer parens. > > Priority of == is 10, while & is 11. Indeed, agree. Then nevermind that comment :)
Hi Mika,
On Monday, 3 November 2025 at 11:43:30 CET, Mika Westerberg wrote:
> On Fri, Oct 31, 2025 at 03:35:43PM +0100, Benoît Monin wrote:
[...]
> > +/*
> > + * Return true if the message needs an explicit RESTART before being sent.
> > + * Without an explicit RESTART, two consecutive messages in the same direction
> > + * will be merged into a single transfer.
> > + * The adapter always emits a RESTART when the direction changes.
> > + */
> > +static inline bool i2c_dw_msg_need_restart(struct i2c_msg msgs[], int idx)
>
> This can take const parameters.
>
Agreed.
> > +{
>
> Please move the dev->flags & NO_EMPTYFIFO_HOLD_MASTER here too.
>
Agreed.
> > + /* No need for a RESTART on the first message */
> > + if (idx == 0)
> > + return false;
>
> That's
>
> if (!idx)
>
Ack.
> But why not pass the actual message instead of the index?
>
Because we need the direction of the previous message and if we are checking
the first message. Passing the array of messages and the index of the
message to check give us the info we need to perform the check.
> > +
> > + return (msgs[idx - 1].flags & I2C_M_RD) == (msgs[idx].flags & I2C_M_RD);
>
> You don't need the outer parens.
>
> > +}
> > +
> > static int
> > i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > {
> > @@ -918,6 +958,17 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > goto done;
> > }
> >
> > + /*
> > + * Make sure we don't need explicit RESTART for
> > + * controllers that cannot emit them.
> > + */
> > + if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
> > + i2c_dw_msg_need_restart(msg, cnt - 1)) {
> > + dev_err(dev->dev, "cannot emit RESTART\n");
> > + ret = -EINVAL;
> > + goto done;
> > + }
> > +
> > if ((msg[cnt - 1].flags & I2C_M_STOP) ||
> > (msg + cnt == msgs + num))
> > break;
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index d7d764f7554d..4aad3dc51fbc 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -346,6 +346,7 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
> >
> > static const struct of_device_id dw_i2c_of_match[] = {
> > { .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
> > + { .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> > { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> > { .compatible = "snps,designware-i2c" },
> > {}
> >
>
Best regards,
--
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
© 2016 - 2026 Red Hat, Inc.