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_msg_is_valid(), 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
protect against FIFO underrun, two changes are done:

First we flag the interrupt with IRQF_NO_THREAD, to prevent it from
running in a thread on PREEMPT-RT kernel. This ensures that we are not
interrupted when filling the FIFO as it is very time-senstive. For
example, being preempted after writing a single byte in the FIFO with
a 1MHz bus gives us only 18µs before an underrun.

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 TX 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  | 32 +++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-platdrv.c |  1 +
 3 files changed, 34 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 2977c13c709c..3486a8c760e0 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -710,6 +710,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
@@ -896,6 +904,16 @@ i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t i
 		return false;
 	}
 
+	/*
+	 * Make sure we don't need explicit RESTART between two messages
+	 * in the same direction for controllers that cannot emit them.
+	 */
+	if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
+	    (msgs[idx - 1].flags & I2C_M_RD) == (msgs[idx].flags & I2C_M_RD)) {
+		dev_err(dev->dev, "cannot emit RESTART\n");
+		return false;
+	}
+
 	return true;
 }
 
@@ -1113,6 +1131,20 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
 		irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND;
 	}
 
+	/*
+	 * The first writing to TX FIFO buffer causes transmission start. If
+	 * IC_EMPTYFIFO_HOLD_MASTER_EN is not set, when TX FIFO gets empty, I2C
+	 * controller finishes the transaction. If writing to FIFO is
+	 * interrupted, FIFO can get empty and the transaction will be finished
+	 * prematurely. FIFO buffer is filled in IRQ handler, but in PREEMPT_RT
+	 * kernel IRQ handler by default is executed in thread that can be
+	 * preempted with another higher priority thread or an interrupt. So,
+	 * IRQF_NO_THREAD flag is required in order to prevent any preemption
+	 * when filling the FIFO.
+	 */
+	if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER)
+		irq_flags |= IRQF_NO_THREAD;
+
 	ret = i2c_dw_acquire_lock(dev);
 	if (ret)
 		return ret;
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 Wed, Nov 19, 2025 at 04:05:36PM +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_msg_is_valid(), 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
> protect against FIFO underrun, two changes are done:
> 
> First we flag the interrupt with IRQF_NO_THREAD, to prevent it from
> running in a thread on PREEMPT-RT kernel. This ensures that we are not
> interrupted when filling the FIFO as it is very time-senstive. For
> example, being preempted after writing a single byte in the FIFO with
> a 1MHz bus gives us only 18µs before an underrun.
> 
> 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 TX 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.

...

>  #define ACCESS_NO_IRQ_SUSPEND			BIT(1)
>  #define ARBITRATION_SEMAPHORE			BIT(2)
>  #define ACCESS_POLLING				BIT(3)
> +#define NO_EMPTYFIFO_HOLD_MASTER		BIT(4)

Can we name it

#define ACCESS_NO_EMPTYFIFO_HOLD_MASTER		BIT(4)

?

It will at least keep them under same namespace.

...

> i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t i

> +	/*
> +	 * Make sure we don't need explicit RESTART between two messages
> +	 * in the same direction for controllers that cannot emit them.
> +	 */
> +	if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
> +	    (msgs[idx - 1].flags & I2C_M_RD) == (msgs[idx].flags & I2C_M_RD)) {
> +		dev_err(dev->dev, "cannot emit RESTART\n");
> +		return false;
> +	}

Ah, Now I see the point of checking the idx first, but can we rather call it
with idx >= 1 to begin with?

>  	return true;
>  }

...

> +	/*
> +	 * The first writing to TX FIFO buffer causes transmission start. If
> +	 * IC_EMPTYFIFO_HOLD_MASTER_EN is not set, when TX FIFO gets empty, I2C
> +	 * controller finishes the transaction. If writing to FIFO is

Make the lines more equal by length.

	 * The first writing to TX FIFO buffer causes transmission start.
	 * If IC_EMPTYFIFO_HOLD_MASTER_EN is not set, when TX FIFO gets empty,
	 * I2C controller finishes the transaction. If writing to FIFO is

> +	 * interrupted, FIFO can get empty and the transaction will be finished
> +	 * prematurely. FIFO buffer is filled in IRQ handler, but in PREEMPT_RT
> +	 * kernel IRQ handler by default is executed in thread that can be

> +	 * preempted with another higher priority thread or an interrupt. So,
> +	 * IRQF_NO_THREAD flag is required in order to prevent any preemption
> +	 * when filling the FIFO.

Similarly

	 * preempted with another higher priority thread or an interrupt.
	 * So, IRQF_NO_THREAD flag is required in order to prevent any
	 * preemption when filling the FIFO.


Dunno if the middle part needs to be rewrapped, perhaps so...

> +	 */

...

>  	{ .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
> +	{ .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },

Are you expecting more with this? I would rather use a compatible matching
instead of the flag,

>  	{ .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
>  	{ .compatible = "snps,designware-i2c" },

-- 
With Best Regards,
Andy Shevchenko

Hi Andy,

Thanks for the reviews.
(ack on other comments)

On Wednesday, 19 November 2025 at 21:15:16 CET, Andy Shevchenko wrote:
> On Wed, Nov 19, 2025 at 04:05:36PM +0100, Benoît Monin wrote:
[...]

> > i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t i
> 
> > +	/*
> > +	 * Make sure we don't need explicit RESTART between two messages
> > +	 * in the same direction for controllers that cannot emit them.
> > +	 */
> > +	if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
> > +	    (msgs[idx - 1].flags & I2C_M_RD) == (msgs[idx].flags & I2C_M_RD)) {
> > +		dev_err(dev->dev, "cannot emit RESTART\n");
> > +		return false;
> > +	}
> 
> Ah, Now I see the point of checking the idx first, but can we rather call it
> with idx >= 1 to begin with?
> 
We would still have to check it when calling i2c_dw_msg_is_valid(), as the
first message after a STOP don't have any limitation. It is not just for
protecting against an out-of-bound access to msgs. The validity of a
message is in relation to the previous message in the same transaction.

I will change the comment to make this clearer.

> >  	return true;
> >  }
[...]

> >  	{ .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
> > +	{ .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> 
> Are you expecting more with this? I would rather use a compatible matching
> instead of the flag,
> 
The IC_EMPTYFIFO_HOLD_MASTER_EN parameter is part of the DesignWare IP, it
is not specific to Mobileye. Given that typical i2c accesses (single read,
single write, write-then-read) work on non-PREMPT_RT without this patch, I
suspect there are other controllers that lack the ability to hold the clock
when the FIFO is empty that could benefit from this flag.

> >  	{ .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

On Tue, Nov 25, 2025 at 11:45:35AM +0100, Benoît Monin wrote:
> On Wednesday, 19 November 2025 at 21:15:16 CET, Andy Shevchenko wrote:
> > On Wed, Nov 19, 2025 at 04:05:36PM +0100, Benoît Monin wrote:

[...]

> > > +	/*
> > > +	 * Make sure we don't need explicit RESTART between two messages
> > > +	 * in the same direction for controllers that cannot emit them.
> > > +	 */
> > > +	if (dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
> > > +	    (msgs[idx - 1].flags & I2C_M_RD) == (msgs[idx].flags & I2C_M_RD)) {
> > > +		dev_err(dev->dev, "cannot emit RESTART\n");
> > > +		return false;
> > > +	}
> > 
> > Ah, Now I see the point of checking the idx first, but can we rather call it
> > with idx >= 1 to begin with?
> > 
> We would still have to check it when calling i2c_dw_msg_is_valid(), as the
> first message after a STOP don't have any limitation. It is not just for
> protecting against an out-of-bound access to msgs. The validity of a
> message is in relation to the previous message in the same transaction.
> 
> I will change the comment to make this clearer.

OK!

> > >  	return true;

[...]

> > >  	{ .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
> > > +	{ .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> > 
> > Are you expecting more with this? I would rather use a compatible matching
> > instead of the flag,
> > 
> The IC_EMPTYFIFO_HOLD_MASTER_EN parameter is part of the DesignWare IP, it
> is not specific to Mobileye. Given that typical i2c accesses (single read,
> single write, write-then-read) work on non-PREMPT_RT without this patch, I
> suspect there are other controllers that lack the ability to hold the clock
> when the FIFO is empty that could benefit from this flag.

USB DWC3 driver uses properties for that. We really should go away from this
MODEL_*/FLAG_* legacy approach in the driver. Either compatible string or
a specific property would be better than inventing yet another no so scalable
flag.

> > >  	{ .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> > >  	{ .compatible = "snps,designware-i2c" },

-- 
With Best Regards,
Andy Shevchenko