If IC_EMPTYFIFO_HOLD_MASTER_EN parameter is 0, "Stop" and "Repeated
Start" bits in command register doesn't 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
consequent write messages 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
To fix it, for the controllers that behave this way, if the next message
to the same slave device has the same direction as the previous one, it
is sent to the controller only after the previous message is sent and
STOP_DET IRQ flag is raised by the controller.
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 entries of Mobileye SoCs needing the work-around and
sort the entries alphabetically.
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.
In a PREEMPT-RT kernel, interrupt handlers are by default executed in
thread and may be interrupted, which can lead to breaking an I2C message
by inserting an unwanted STOP.
To ensure proper operation on realtime kernel, use IRQF_NO_THREAD flag
when requesting IRQ.
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-core.h | 1 +
drivers/i2c/busses/i2c-designware-master.c | 45 +++++++++++++++++++++--------
drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++--
3 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 347843b4f5dd7..a31a8698e511a 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 f9a180b145da8..e5af0439ec832 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -443,18 +443,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
u32 flags = msgs[dev->msg_write_idx].flags;
- /*
- * 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) {
- intr_mask &= ~DW_IC_INTR_TX_EMPTY;
- break;
- }
-
if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
/* new i2c_msg */
buf = msgs[dev->msg_write_idx].buf;
@@ -470,6 +458,25 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
need_restart = true;
}
+ /*
+ * 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.
+ * Also force a stop-then-start between two messages
+ * in the same direction if we need to restart on a
+ * adapter that does not handle restart.
+ */
+ if (msgs[dev->msg_write_idx].addr != addr ||
+ ((need_restart &&
+ dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
+ ((msgs[dev->msg_write_idx].flags & I2C_M_RD) ==
+ (msgs[dev->msg_write_idx - 1].flags & I2C_M_RD))))) {
+ intr_mask &= ~DW_IC_INTR_TX_EMPTY;
+ break;
+ }
+
regmap_read(dev->map, DW_IC_TXFLR, &flr);
tx_limit = dev->tx_fifo_depth - flr;
@@ -1062,6 +1069,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
+ * during filling the FIFO buffer and possible data lost.
+ */
+ 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 34d881572351c..5e459175dcdb2 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -345,9 +345,11 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
}
static const struct of_device_id dw_i2c_of_match[] = {
- { .compatible = "snps,designware-i2c", },
- { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
{ .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
+ { .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
+ { .compatible = "mobileye,eyeq7h-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
+ { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
+ { .compatible = "snps,designware-i2c", },
{}
};
MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
--
2.51.0
Hi,
On Fri, Oct 17, 2025 at 04:59:34PM +0200, Benoît Monin wrote:
> If IC_EMPTYFIFO_HOLD_MASTER_EN parameter is 0, "Stop" and "Repeated
> Start" bits in command register doesn't 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
> consequent write messages 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
>
> To fix it, for the controllers that behave this way, if the next message
> to the same slave device has the same direction as the previous one, it
> is sent to the controller only after the previous message is sent and
> STOP_DET IRQ flag is raised by the controller.
>
> 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 entries of Mobileye SoCs needing the work-around and
> sort the entries alphabetically.
>
> 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.
>
> In a PREEMPT-RT kernel, interrupt handlers are by default executed in
> thread and may be interrupted, which can lead to breaking an I2C message
> by inserting an unwanted STOP.
>
> To ensure proper operation on realtime kernel, use IRQF_NO_THREAD flag
> when requesting IRQ.
But even with that, it is still possible that something else is running
with local interrupt disabled so this may still happen, although likelyhood
is smaller.
You could use ACCESS_POLLING but that too can be preempted.
Perhaps best is to fail the transfer and let the caller retry?
> 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-core.h | 1 +
> drivers/i2c/busses/i2c-designware-master.c | 45 +++++++++++++++++++++--------
> drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++--
> 3 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 347843b4f5dd7..a31a8698e511a 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 f9a180b145da8..e5af0439ec832 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -443,18 +443,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
> u32 flags = msgs[dev->msg_write_idx].flags;
>
> - /*
> - * 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) {
> - intr_mask &= ~DW_IC_INTR_TX_EMPTY;
> - break;
> - }
> -
> if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> /* new i2c_msg */
> buf = msgs[dev->msg_write_idx].buf;
> @@ -470,6 +458,25 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> need_restart = true;
> }
>
> + /*
> + * 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.
> + * Also force a stop-then-start between two messages
> + * in the same direction if we need to restart on a
> + * adapter that does not handle restart.
> + */
> + if (msgs[dev->msg_write_idx].addr != addr ||
> + ((need_restart &&
> + dev->flags & NO_EMPTYFIFO_HOLD_MASTER &&
> + ((msgs[dev->msg_write_idx].flags & I2C_M_RD) ==
> + (msgs[dev->msg_write_idx - 1].flags & I2C_M_RD))))) {
> + intr_mask &= ~DW_IC_INTR_TX_EMPTY;
> + break;
This, if we want to add it even, needs a helper function. The above is
really hard to parse ;-)
> + }
> +
> regmap_read(dev->map, DW_IC_TXFLR, &flr);
> tx_limit = dev->tx_fifo_depth - flr;
>
> @@ -1062,6 +1069,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
> + * during filling the FIFO buffer and possible data lost.
> + */
> + 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 34d881572351c..5e459175dcdb2 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -345,9 +345,11 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
> }
>
> static const struct of_device_id dw_i2c_of_match[] = {
> - { .compatible = "snps,designware-i2c", },
> - { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> { .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
> + { .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> + { .compatible = "mobileye,eyeq7h-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> + { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> + { .compatible = "snps,designware-i2c", },
> {}
> };
> MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
>
> --
> 2.51.0
On Fri, Oct 17, 2025 at 04:59:34PM +0200, Benoît Monin wrote:
> If IC_EMPTYFIFO_HOLD_MASTER_EN parameter is 0, "Stop" and "Repeated
> Start" bits in command register doesn't 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
> consequent write messages 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
>
> To fix it, for the controllers that behave this way, if the next message
> to the same slave device has the same direction as the previous one, it
> is sent to the controller only after the previous message is sent and
> STOP_DET IRQ flag is raised by the controller.
>
> 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 entries of Mobileye SoCs needing the work-around and
> sort the entries alphabetically.
>
> 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.
>
> In a PREEMPT-RT kernel, interrupt handlers are by default executed in
> thread and may be interrupted, which can lead to breaking an I2C message
> by inserting an unwanted STOP.
>
> To ensure proper operation on realtime kernel, use IRQF_NO_THREAD flag
> when requesting IRQ.
> Based on the work of Dmitry Guzman <dmitry.guzman@mobileye.com>
You may also use a tag "Originally-by".
...
> static const struct of_device_id dw_i2c_of_match[] = {
> - { .compatible = "snps,designware-i2c", },
> - { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> { .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
> + { .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> + { .compatible = "mobileye,eyeq7h-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> + { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> + { .compatible = "snps,designware-i2c", },
Sorting can be moved to a separate change (and while at that the inner trailing
comma can be dropped).
> {}
> };
--
With Best Regards,
Andy Shevchenko
On 17/10/2025 16:59, Benoît Monin wrote:
>
> static const struct of_device_id dw_i2c_of_match[] = {
> - { .compatible = "snps,designware-i2c", },
> - { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> { .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
> + { .compatible = "mobileye,eyeq6lplus-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
> + { .compatible = "mobileye,eyeq7h-i2c", .data = (void *)NO_EMPTYFIFO_HOLD_MASTER },
That's not necessary. Don't create duplicated entries, express
compatibility.
Best regards,
Krzysztof
© 2016 - 2026 Red Hat, Inc.