[PATCH v2 0/5] i2c: designware: Improve support of multi-messages transfer

[PATCH v2 0/5] i2c: designware: Improve support of multi-messages transfer

[Benoît Monin]

Add support for the I2C_M_STOP flag to the .xfer() function of the
designware driver. This allows grouping multiple accesses in a single
call and changing the target address after a STOP flag. This is achieved
by splitting i2c_dw_xfer() in two functions. The core logic handling
the transaction is now in __i2c_dw_xfer_unlocked(), while i2c_dw_xfer()
loops over the messages to search for the I2C_M_STOP flag and calls
__i2c_dw_xfer_unlocked().

Handle controllers that lack the ability to emit a RESTART when two
consecutive messages have the same address and direction by aborting
transfers that contain such a sequence of messages. For those controllers,
we also check that we do not get any unwanted STOP caused by a Tx FIFO
underrun, as they lack the ability to hold the clock during a transaction.

The I2C controllers found in the EyeQ6Lplus and EyeQ7H SoCs from Mobileye
lack such capability, so a compatible string is added because this cannot
be detected at runtime.

This patch series also brings two cleanups. One in i2c_dw_read() optimizes
the read of the message flags. And the other sorts the compatible strings
alphabetically in dw_i2c_of_match[].

Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
Changes in v2:
- Add a single compatible for mobileye i2c controllers based in
  DesignWare IP.
- Handle the I2C_M_STOP flag instead of emiting a STOP on target address
  change.
- Abort transfer when it requires a RESTART on controller that cannot
  emit them.
- Detect FIFO underrun instead of disabling threaded interrupt on
  PREEMPT_RT kernel.
- Sort the compatible entries in a separate patch.
- Add a cleanup patch on flag reading in i2c_dw_read().
- Link to v1: https://lore.kernel.org/r/20251017-i2c-dw-v1-0-7b85b71c7a87@bootlin.com

---
Benoît Monin (5):
      dt-bindings: i2c: dw: Add Mobileye I2C controllers
      i2c: designware: Optimize flag reading in i2c_dw_read()
      i2c: designware: Sort compatible strings in alphabetical order
      i2c: designware: Implement I2C_M_STOP support
      i2c: designware: Support of controller with IC_EMPTYFIFO_HOLD_MASTER disabled

 .../bindings/i2c/snps,designware-i2c.yaml          |   1 +
 drivers/i2c/busses/i2c-designware-core.h           |   1 +
 drivers/i2c/busses/i2c-designware-master.c         | 159 ++++++++++++++++-----
 drivers/i2c/busses/i2c-designware-platdrv.c        |   5 +-
 4 files changed, 128 insertions(+), 38 deletions(-)
---
base-commit: dcb6fa37fd7bc9c3d2b066329b0d27dedf8becaa
change-id: 20251014-i2c-dw-da315f758296

Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 1/5] dt-bindings: i2c: dw: Add Mobileye I2C controllers

[Benoît Monin]

Add compatible string for the I2C controllers present in Mobileye
Eyeq6Lplus SoC. The same controllers are also present in the EyeQ7H.

Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
 Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
index d904191bb0c6..bc84631f28d1 100644
--- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
@@ -36,6 +36,7 @@ properties:
         const: baikal,bt1-sys-i2c
       - items:
           - enum:
+              - mobileye,eyeq6lplus-i2c
               - mscc,ocelot-i2c
               - sophgo,sg2044-i2c
               - thead,th1520-i2c

-- 
2.51.1

[PATCH v2 2/5] i2c: designware: Optimize flag reading in i2c_dw_read()

[Benoît Monin]

Optimize the i2c_dw_read() function by reading the message flags only
once per message, rather than for every byte.

The message index is only modified by the outer loop, so reading the
flags in the inner loop was always getting the same value.

Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
 drivers/i2c/busses/i2c-designware-master.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 41e9b5ecad20..ec4fc2708d03 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -586,11 +586,12 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 	unsigned int rx_valid;
 
 	for (; dev->msg_read_idx < dev->msgs_num; dev->msg_read_idx++) {
+		u32 flags = msgs[dev->msg_read_idx].flags;
 		unsigned int tmp;
 		u32 len;
 		u8 *buf;
 
-		if (!(msgs[dev->msg_read_idx].flags & I2C_M_RD))
+		if (!(flags & I2C_M_RD))
 			continue;
 
 		if (!(dev->status & STATUS_READ_IN_PROGRESS)) {
@@ -604,8 +605,6 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 		regmap_read(dev->map, DW_IC_RXFLR, &rx_valid);
 
 		for (; len > 0 && rx_valid > 0; len--, rx_valid--) {
-			u32 flags = msgs[dev->msg_read_idx].flags;
-
 			regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
 			tmp &= DW_IC_DATA_CMD_DAT;
 			/* Ensure length byte is a valid value */

-- 
2.51.1

[PATCH v2 3/5] i2c: designware: Sort compatible strings in alphabetical order

[Benoît Monin]

Reorder the of_device_id structures so that they are in alphabetical
order. Also drop the unneeded inner trailing comma in the
"snps,designware-i2c" struct.

Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 34d881572351..d7d764f7554d 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -345,9 +345,9 @@ 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 = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
+	{ .compatible = "snps,designware-i2c" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, dw_i2c_of_match);

-- 
2.51.1

[PATCH v2 4/5] i2c: designware: Implement I2C_M_STOP support

[Benoît Monin]

Add the support of the I2C_M_STOP flag in i2c_msg by splitting
i2c_dw_xfer() in two: __i2c_dw_xfer_unlocked() for the core transfer logic
and i2c_dw_xfer() for handling the high-level transaction management.

In detail __i2c_dw_xfer_unlocked() starts a transaction and wait for its
completion, either with a STOP on the bus or an error. i2c_dw_xfer()
loops over the messages to search for the I2C_M_STOP flag and calls
__i2c_dw_xfer_unlocked() for each part of the messages up to a STOP or
the end of the messages array.

i2c_dw_xfer() holds the device lock while calling __i2c_dw_xfer_unlocked(),
this allows 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.

Also, now that we have a lookup of the messages in i2c_dw_xfer() prior
to each transaction, we use it to make sure the messages are valid for
the transaction. We check that the target address does not change before
starting the transaction instead of aborting the transfer while it is
happening, as it was done in i2c_dw_xfer_msg(). The target address can
only be changed after an I2C_M_STOP flag, thus a STOP on the i2c bus.

The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
functionalities supported by the adapter, except for the AMD NAVI i2c
controller which uses its own xfer() function and is left untouched.

Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
---
 drivers/i2c/busses/i2c-designware-master.c | 103 ++++++++++++++++++++---------
 1 file changed, 70 insertions(+), 33 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index ec4fc2708d03..da1963d25def 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -431,7 +431,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 	struct i2c_msg *msgs = dev->msgs;
 	u32 intr_mask;
 	int tx_limit, rx_limit;
-	u32 addr = msgs[dev->msg_write_idx].addr;
 	u32 buf_len = dev->tx_buf_len;
 	u8 *buf = dev->tx_buf;
 	bool need_restart = false;
@@ -442,18 +441,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.
-		 */
-		if (msgs[dev->msg_write_idx].addr != addr) {
-			dev_err(dev->dev,
-				"%s: invalid target address\n", __func__);
-			dev->msg_err = -EINVAL;
-			break;
-		}
-
 		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
 			/* new i2c_msg */
 			buf = msgs[dev->msg_write_idx].buf;
@@ -801,26 +788,15 @@ static int i2c_dw_wait_transfer(struct dw_i2c_dev *dev)
 }
 
 /*
- * Prepare controller for a transaction and call i2c_dw_xfer_msg.
+ * Prepare controller for a transaction, start the transfer of the msgs
+ * and wait for completion.
+ * Caller must hold the device lock.
  */
 static int
-i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+__i2c_dw_xfer_unlocked(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
 {
-	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	int ret;
 
-	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
-
-	pm_runtime_get_sync(dev->dev);
-
-	switch (dev->flags & MODEL_MASK) {
-	case MODEL_AMD_NAVI_GPU:
-		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
-		goto done_nolock;
-	default:
-		break;
-	}
-
 	reinit_completion(&dev->cmd_complete);
 	dev->msgs = msgs;
 	dev->msgs_num = num;
@@ -832,10 +808,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	dev->abort_source = 0;
 	dev->rx_outstanding = 0;
 
-	ret = i2c_dw_acquire_lock(dev);
-	if (ret)
-		goto done_nolock;
-
 	ret = i2c_dw_wait_bus_not_busy(dev);
 	if (ret < 0)
 		goto done;
@@ -896,13 +868,75 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	ret = -EIO;
 
+done:
+	return ret;
+}
+
+static int
+i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+	struct i2c_msg *msg;
+	int ret, cnt;
+
+	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+
+	pm_runtime_get_sync(dev->dev);
+
+	switch (dev->flags & MODEL_MASK) {
+	case MODEL_AMD_NAVI_GPU:
+		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
+		goto done_nolock;
+	default:
+		break;
+	}
+
+	ret = i2c_dw_acquire_lock(dev);
+	if (ret)
+		goto done_nolock;
+
+	/*
+	 * If the I2C_M_STOP is present in some the messages,
+	 * we do one transaction for each part up to the STOP.
+	 */
+	for (msg = msgs; msg < msgs + num; msg += cnt) {
+		u16 addr = msg->addr;
+
+		/*
+		 * Count the messages in a transaction, up to a STOP
+		 * or the end of the msgs.
+		 */
+		for (cnt = 1; ; cnt++) {
+			/*
+			 * We cannot change the target address during
+			 * a transaction, so make sure the address stays
+			 * the same.
+			 */
+			if (msg[cnt - 1].addr != addr) {
+				dev_err(dev->dev, "invalid target address\n");
+				ret = -EINVAL;
+				goto done;
+			}
+
+			if ((msg[cnt - 1].flags & I2C_M_STOP) ||
+			    (msg + cnt == msgs + num))
+				break;
+		}
+
+		ret = __i2c_dw_xfer_unlocked(dev, msg, cnt);
+		if (ret < 0)
+			goto done;
+	}
+
 done:
 	i2c_dw_release_lock(dev);
 
 done_nolock:
 	pm_runtime_put_autosuspend(dev->dev);
 
-	return ret;
+	if (ret < 0)
+		return ret;
+	return num;
 }
 
 static const struct i2c_algorithm i2c_dw_algo = {
@@ -920,6 +954,9 @@ void i2c_dw_configure_master(struct dw_i2c_dev *dev)
 
 	dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
 
+	if ((dev->flags & MODEL_MASK) != MODEL_AMD_NAVI_GPU)
+		dev->functionality |= I2C_FUNC_PROTOCOL_MANGLING;
+
 	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
 			  DW_IC_CON_RESTART_EN;
 

-- 
2.51.1

[PATCH v2 5/5] i2c: designware: Support of controller with IC_EMPTYFIFO_HOLD_MASTER disabled

[Benoît Monin]

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

Re: [PATCH v2 2/5] i2c: designware: Optimize flag reading in i2c_dw_read()

[Andy Shevchenko]

On Fri, Oct 31, 2025 at 03:35:40PM +0100, Benoît Monin wrote:
> Optimize the i2c_dw_read() function by reading the message flags only
> once per message, rather than for every byte.
> 
> The message index is only modified by the outer loop, so reading the
> flags in the inner loop was always getting the same value.

Does it affect the binary (compiled) file?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/5] i2c: designware: Sort compatible strings in alphabetical order

[Andy Shevchenko]

On Fri, Oct 31, 2025 at 03:35:41PM +0100, Benoît Monin wrote:
> Reorder the of_device_id structures so that they are in alphabetical
> order. Also drop the unneeded inner trailing comma in the
> "snps,designware-i2c" struct.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] i2c: designware: Implement I2C_M_STOP support

[Andy Shevchenko]

On Fri, Oct 31, 2025 at 03:35:42PM +0100, Benoît Monin wrote:
> Add the support of the I2C_M_STOP flag in i2c_msg by splitting
> i2c_dw_xfer() in two: __i2c_dw_xfer_unlocked() for the core transfer logic
> and i2c_dw_xfer() for handling the high-level transaction management.
> 
> In detail __i2c_dw_xfer_unlocked() starts a transaction and wait for its
> completion, either with a STOP on the bus or an error. i2c_dw_xfer()
> loops over the messages to search for the I2C_M_STOP flag and calls
> __i2c_dw_xfer_unlocked() for each part of the messages up to a STOP or
> the end of the messages array.
> 
> i2c_dw_xfer() holds the device lock while calling __i2c_dw_xfer_unlocked(),
> this allows 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.
> 
> Also, now that we have a lookup of the messages in i2c_dw_xfer() prior
> to each transaction, we use it to make sure the messages are valid for
> the transaction. We check that the target address does not change before
> starting the transaction instead of aborting the transfer while it is
> happening, as it was done in i2c_dw_xfer_msg(). The target address can
> only be changed after an I2C_M_STOP flag, thus a STOP on the i2c bus.
> 
> The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
> functionalities supported by the adapter, except for the AMD NAVI i2c
> controller which uses its own xfer() function and is left untouched.

...

>  /*
> - * Prepare controller for a transaction and call i2c_dw_xfer_msg.
> + * Prepare controller for a transaction, start the transfer of the msgs
> + * and wait for completion.
> + * Caller must hold the device lock.

Comment is good, but having a lockdep annotation in addition to is even better!

>   */

...

> +done:
> +	return ret;

Drop now unneeded label, return directly.

...

> +static int
> +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +	struct i2c_msg *msg;
> +	int ret, cnt;
> +
> +	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);

__func__ can be turned on or off at runtime (with Dynamic Debug help).

> +	pm_runtime_get_sync(dev->dev);

Can you switch to use ACQUIRE() ?

> +	switch (dev->flags & MODEL_MASK) {
> +	case MODEL_AMD_NAVI_GPU:
> +		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> +		goto done_nolock;
> +	default:
> +		break;
> +	}
> +
> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		goto done_nolock;
> +
> +	/*
> +	 * If the I2C_M_STOP is present in some the messages,
> +	 * we do one transaction for each part up to the STOP.
> +	 */
> +	for (msg = msgs; msg < msgs + num; msg += cnt) {
> +		u16 addr = msg->addr;
> +
> +		/*
> +		 * Count the messages in a transaction, up to a STOP
> +		 * or the end of the msgs.
> +		 */
> +		for (cnt = 1; ; cnt++) {
> +			/*
> +			 * We cannot change the target address during
> +			 * a transaction, so make sure the address stays
> +			 * the same.
> +			 */
> +			if (msg[cnt - 1].addr != addr) {
> +				dev_err(dev->dev, "invalid target address\n");
> +				ret = -EINVAL;
> +				goto done;
> +			}
> +
> +			if ((msg[cnt - 1].flags & I2C_M_STOP) ||
> +			    (msg + cnt == msgs + num))
> +				break;
> +		}
> +
> +		ret = __i2c_dw_xfer_unlocked(dev, msg, cnt);
> +		if (ret < 0)
> +			goto done;
> +	}
> +
>  done:
>  	i2c_dw_release_lock(dev);
>  
>  done_nolock:
>  	pm_runtime_put_autosuspend(dev->dev);
>  
> -	return ret;
> +	if (ret < 0)
> +		return ret;
> +	return num;
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] dt-bindings: i2c: dw: Add Mobileye I2C controllers

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

On Fri, Oct 31, 2025 at 03:35:39PM +0100, Benoît Monin wrote:
> Add compatible string for the I2C controllers present in Mobileye
> Eyeq6Lplus SoC. The same controllers are also present in the EyeQ7H.

Then where is the compatible for the q7h?

pw-bot: changes-requested

> 
> Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
> ---
>  Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> index d904191bb0c6..bc84631f28d1 100644
> --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> @@ -36,6 +36,7 @@ properties:
>          const: baikal,bt1-sys-i2c
>        - items:
>            - enum:
> +              - mobileye,eyeq6lplus-i2c
>                - mscc,ocelot-i2c
>                - sophgo,sg2044-i2c
>                - thead,th1520-i2c
> 
> -- 
> 2.51.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 4/5] i2c: designware: Implement I2C_M_STOP support

[Mika Westerberg]

On Fri, Oct 31, 2025 at 03:35:42PM +0100, Benoît Monin wrote:
> Add the support of the I2C_M_STOP flag in i2c_msg by splitting
> i2c_dw_xfer() in two: __i2c_dw_xfer_unlocked() for the core transfer logic
> and i2c_dw_xfer() for handling the high-level transaction management.
> 
> In detail __i2c_dw_xfer_unlocked() starts a transaction and wait for its
> completion, either with a STOP on the bus or an error. i2c_dw_xfer()
> loops over the messages to search for the I2C_M_STOP flag and calls
> __i2c_dw_xfer_unlocked() for each part of the messages up to a STOP or
> the end of the messages array.
> 
> i2c_dw_xfer() holds the device lock while calling __i2c_dw_xfer_unlocked(),
> this allows 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.
> 
> Also, now that we have a lookup of the messages in i2c_dw_xfer() prior
> to each transaction, we use it to make sure the messages are valid for
> the transaction. We check that the target address does not change before
> starting the transaction instead of aborting the transfer while it is
> happening, as it was done in i2c_dw_xfer_msg(). The target address can
> only be changed after an I2C_M_STOP flag, thus a STOP on the i2c bus.
> 
> The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
> functionalities supported by the adapter, except for the AMD NAVI i2c

I2C controller

> controller which uses its own xfer() function and is left untouched.
> 
> Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
> ---
>  drivers/i2c/busses/i2c-designware-master.c | 103 ++++++++++++++++++++---------
>  1 file changed, 70 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index ec4fc2708d03..da1963d25def 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -431,7 +431,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  	struct i2c_msg *msgs = dev->msgs;
>  	u32 intr_mask;
>  	int tx_limit, rx_limit;
> -	u32 addr = msgs[dev->msg_write_idx].addr;
>  	u32 buf_len = dev->tx_buf_len;
>  	u8 *buf = dev->tx_buf;
>  	bool need_restart = false;
> @@ -442,18 +441,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.
> -		 */
> -		if (msgs[dev->msg_write_idx].addr != addr) {
> -			dev_err(dev->dev,
> -				"%s: invalid target address\n", __func__);
> -			dev->msg_err = -EINVAL;
> -			break;
> -		}
> -
>  		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
>  			/* new i2c_msg */
>  			buf = msgs[dev->msg_write_idx].buf;
> @@ -801,26 +788,15 @@ static int i2c_dw_wait_transfer(struct dw_i2c_dev *dev)
>  }
>  
>  /*
> - * Prepare controller for a transaction and call i2c_dw_xfer_msg.
> + * Prepare controller for a transaction, start the transfer of the msgs
> + * and wait for completion.
> + * Caller must hold the device lock.
>   */
>  static int
> -i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +__i2c_dw_xfer_unlocked(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
>  {
> -	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
>  	int ret;
>  
> -	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> -
> -	pm_runtime_get_sync(dev->dev);
> -
> -	switch (dev->flags & MODEL_MASK) {
> -	case MODEL_AMD_NAVI_GPU:
> -		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> -		goto done_nolock;
> -	default:
> -		break;
> -	}
> -
>  	reinit_completion(&dev->cmd_complete);
>  	dev->msgs = msgs;
>  	dev->msgs_num = num;
> @@ -832,10 +808,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	dev->abort_source = 0;
>  	dev->rx_outstanding = 0;
>  
> -	ret = i2c_dw_acquire_lock(dev);
> -	if (ret)
> -		goto done_nolock;
> -
>  	ret = i2c_dw_wait_bus_not_busy(dev);
>  	if (ret < 0)
>  		goto done;
> @@ -896,13 +868,75 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  
>  	ret = -EIO;
>  
> +done:
> +	return ret;
> +}
> +
> +static int
> +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)

Typically if you have

i2c_dw_xfer()
i2c_dw_xfer_unlocked()

The only difference is that the former call the latter with lock held.
However, this is not the case here which is confusing.

I suggest move the lookup to be part of the _unlocked() variant.

While there, can we use size_t with the num parameter.

> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +	struct i2c_msg *msg;
> +	int ret, cnt;
> +
> +	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> +
> +	pm_runtime_get_sync(dev->dev);
> +
> +	switch (dev->flags & MODEL_MASK) {
> +	case MODEL_AMD_NAVI_GPU:
> +		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> +		goto done_nolock;

Not really problem of this patch but I'm sure there is cleaner way to deal
with this. I mean first of all we don't wan't to differentiate here what
model this is. Instead we can look for a "quirks" based on dev->flags.

Secondly goto inside switch is confusing too.

This can be better written like:

if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) {
	...
}

> +	default:
> +		break;
> +	}
> +
> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		goto done_nolock;
> +
> +	/*
> +	 * If the I2C_M_STOP is present in some the messages,
> +	 * we do one transaction for each part up to the STOP.
> +	 */
> +	for (msg = msgs; msg < msgs + num; msg += cnt) {
> +		u16 addr = msg->addr;
> +
> +		/*
> +		 * Count the messages in a transaction, up to a STOP
> +		 * or the end of the msgs.
> +		 */
> +		for (cnt = 1; ; cnt++) {
> +			/*
> +			 * We cannot change the target address during
> +			 * a transaction, so make sure the address stays
> +			 * the same.
> +			 */
> +			if (msg[cnt - 1].addr != addr) {
> +				dev_err(dev->dev, "invalid target address\n");
> +				ret = -EINVAL;
> +				goto done;
> +			}
> +
> +			if ((msg[cnt - 1].flags & I2C_M_STOP) ||
> +			    (msg + cnt == msgs + num))
> +				break;
> +		}
> +
> +		ret = __i2c_dw_xfer_unlocked(dev, msg, cnt);
> +		if (ret < 0)
> +			goto done;

This can be

	break;

> +	}
> +
>  done:
>  	i2c_dw_release_lock(dev);
>  
>  done_nolock:
>  	pm_runtime_put_autosuspend(dev->dev);
>  
> -	return ret;
> +	if (ret < 0)
> +		return ret;
> +	return num;
>  }
>  
>  static const struct i2c_algorithm i2c_dw_algo = {
> @@ -920,6 +954,9 @@ void i2c_dw_configure_master(struct dw_i2c_dev *dev)
>  
>  	dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
>  
> +	if ((dev->flags & MODEL_MASK) != MODEL_AMD_NAVI_GPU)
> +		dev->functionality |= I2C_FUNC_PROTOCOL_MANGLING;
> +
>  	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
>  			  DW_IC_CON_RESTART_EN;
>  
> 
> -- 
> 2.51.1

Re: [PATCH v2 5/5] i2c: designware: Support of controller with IC_EMPTYFIFO_HOLD_MASTER disabled

[Mika Westerberg]

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

Re: [PATCH v2 5/5] i2c: designware: Support of controller with IC_EMPTYFIFO_HOLD_MASTER disabled

[Andy Shevchenko]

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

Re: [PATCH v2 5/5] i2c: designware: Support of controller with IC_EMPTYFIFO_HOLD_MASTER disabled

[Mika Westerberg]

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 :)

Re: [PATCH v2 1/5] dt-bindings: i2c: dw: Add Mobileye I2C controllers

[Benoît Monin]

Hi Conor,

On Friday, 31 October 2025 at 15:58:49 CET, Conor Dooley wrote:
> On Fri, Oct 31, 2025 at 03:35:39PM +0100, Benoît Monin wrote:
> > Add compatible string for the I2C controllers present in Mobileye
> > Eyeq6Lplus SoC. The same controllers are also present in the EyeQ7H.
> 
> Then where is the compatible for the q7h?
> 
I had both entries in version 1 of the patch but Krzysztof told me to
avoid creating duplicated entries. Should I just drop the EyeQ7H
mention in the commit message?

> pw-bot: changes-requested
> 
> > 
> > Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
> > ---
> >  Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> > index d904191bb0c6..bc84631f28d1 100644
> > --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> > @@ -36,6 +36,7 @@ properties:
> >          const: baikal,bt1-sys-i2c
> >        - items:
> >            - enum:
> > +              - mobileye,eyeq6lplus-i2c
> >                - mscc,ocelot-i2c
> >                - sophgo,sg2044-i2c
> >                - thead,th1520-i2c
> > 
> 

Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 1/5] dt-bindings: i2c: dw: Add Mobileye I2C controllers

[Krzysztof Kozlowski]

On 06/11/2025 10:42, Benoît Monin wrote:
> Hi Conor,
> 
> On Friday, 31 October 2025 at 15:58:49 CET, Conor Dooley wrote:
>> On Fri, Oct 31, 2025 at 03:35:39PM +0100, Benoît Monin wrote:
>>> Add compatible string for the I2C controllers present in Mobileye
>>> Eyeq6Lplus SoC. The same controllers are also present in the EyeQ7H.
>>
>> Then where is the compatible for the q7h?
>>
> I had both entries in version 1 of the patch but Krzysztof told me to
> avoid creating duplicated entries. Should I just drop the EyeQ7H
> mention in the commit message?
> 

I did not ask to drop the compatible. I asked to express the
compatibility between two devices (see my talk for beginners about DTS
or just read writing bindings).

Best regards,
Krzysztof

Re: [PATCH v2 2/5] i2c: designware: Optimize flag reading in i2c_dw_read()

[Benoît Monin]

Hi Andy,

On Friday, 31 October 2025 at 15:48:50 CET, Andy Shevchenko wrote:
> On Fri, Oct 31, 2025 at 03:35:40PM +0100, Benoît Monin wrote:
> > Optimize the i2c_dw_read() function by reading the message flags only
> > once per message, rather than for every byte.
> > 
> > The message index is only modified by the outer loop, so reading the
> > flags in the inner loop was always getting the same value.
> 
> Does it affect the binary (compiled) file?
> 
Yes it does. On the mips64 system I am testing this on, built with gcc11,
i2c_dw_process_transfer() which inline i2c_dw_read() get 16 bytes shorter.
 
Looking at the disassembled code, the flags was read in the inner loop prior
to the patch.

Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/5] i2c: designware: Optimize flag reading in i2c_dw_read()

[Krzysztof Kozlowski]

On 31/10/2025 15:48, Andy Shevchenko wrote:
> On Fri, Oct 31, 2025 at 03:35:40PM +0100, Benoît Monin wrote:
>> Optimize the i2c_dw_read() function by reading the message flags only
>> once per message, rather than for every byte.
>>
>> The message index is only modified by the outer loop, so reading the
>> flags in the inner loop was always getting the same value.
> 
> Does it affect the binary (compiled) file?


It does not really matter that much, because new code is more readable -
'flags' depend on the outer (first) loop and they are used there, so
that's where variable should be defined.

Best regards,
Krzysztof

Re: [PATCH v2 2/5] i2c: designware: Optimize flag reading in i2c_dw_read()

[Andy Shevchenko]

On Thu, Nov 06, 2025 at 11:50:36AM +0100, Krzysztof Kozlowski wrote:
> On 31/10/2025 15:48, Andy Shevchenko wrote:
> > On Fri, Oct 31, 2025 at 03:35:40PM +0100, Benoît Monin wrote:
> >> Optimize the i2c_dw_read() function by reading the message flags only
> >> once per message, rather than for every byte.
> >>
> >> The message index is only modified by the outer loop, so reading the
> >> flags in the inner loop was always getting the same value.
> > 
> > Does it affect the binary (compiled) file?
> 
> It does not really matter that much, because new code is more readable -
> 'flags' depend on the outer (first) loop and they are used there, so
> that's where variable should be defined.

Did I oppose that?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] i2c: designware: Implement I2C_M_STOP support

[Benoît Monin]

Hi Mika,

On Monday, 3 November 2025 at 11:39:08 CET, Mika Westerberg wrote:
> On Fri, Oct 31, 2025 at 03:35:42PM +0100, Benoît Monin wrote:
> > Add the support of the I2C_M_STOP flag in i2c_msg by splitting
> > i2c_dw_xfer() in two: __i2c_dw_xfer_unlocked() for the core transfer logic
> > and i2c_dw_xfer() for handling the high-level transaction management.
> > 
> > In detail __i2c_dw_xfer_unlocked() starts a transaction and wait for its
> > completion, either with a STOP on the bus or an error. i2c_dw_xfer()
> > loops over the messages to search for the I2C_M_STOP flag and calls
> > __i2c_dw_xfer_unlocked() for each part of the messages up to a STOP or
> > the end of the messages array.
> > 
> > i2c_dw_xfer() holds the device lock while calling __i2c_dw_xfer_unlocked(),
> > this allows 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.
> > 
> > Also, now that we have a lookup of the messages in i2c_dw_xfer() prior
> > to each transaction, we use it to make sure the messages are valid for
> > the transaction. We check that the target address does not change before
> > starting the transaction instead of aborting the transfer while it is
> > happening, as it was done in i2c_dw_xfer_msg(). The target address can
> > only be changed after an I2C_M_STOP flag, thus a STOP on the i2c bus.
> > 
> > The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
> > functionalities supported by the adapter, except for the AMD NAVI i2c
> 
> I2C controller
> 
> > controller which uses its own xfer() function and is left untouched.
> > 
> > Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
> > ---
> >  drivers/i2c/busses/i2c-designware-master.c | 103 ++++++++++++++++++++---------
> >  1 file changed, 70 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> > index ec4fc2708d03..da1963d25def 100644
> > --- a/drivers/i2c/busses/i2c-designware-master.c
> > +++ b/drivers/i2c/busses/i2c-designware-master.c
> > @@ -431,7 +431,6 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
> >  	struct i2c_msg *msgs = dev->msgs;
> >  	u32 intr_mask;
> >  	int tx_limit, rx_limit;
> > -	u32 addr = msgs[dev->msg_write_idx].addr;
> >  	u32 buf_len = dev->tx_buf_len;
> >  	u8 *buf = dev->tx_buf;
> >  	bool need_restart = false;
> > @@ -442,18 +441,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.
> > -		 */
> > -		if (msgs[dev->msg_write_idx].addr != addr) {
> > -			dev_err(dev->dev,
> > -				"%s: invalid target address\n", __func__);
> > -			dev->msg_err = -EINVAL;
> > -			break;
> > -		}
> > -
> >  		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> >  			/* new i2c_msg */
> >  			buf = msgs[dev->msg_write_idx].buf;
> > @@ -801,26 +788,15 @@ static int i2c_dw_wait_transfer(struct dw_i2c_dev *dev)
> >  }
> >  
> >  /*
> > - * Prepare controller for a transaction and call i2c_dw_xfer_msg.
> > + * Prepare controller for a transaction, start the transfer of the msgs
> > + * and wait for completion.
> > + * Caller must hold the device lock.
> >   */
> >  static int
> > -i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > +__i2c_dw_xfer_unlocked(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
> >  {
> > -	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> >  	int ret;
> >  
> > -	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> > -
> > -	pm_runtime_get_sync(dev->dev);
> > -
> > -	switch (dev->flags & MODEL_MASK) {
> > -	case MODEL_AMD_NAVI_GPU:
> > -		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> > -		goto done_nolock;
> > -	default:
> > -		break;
> > -	}
> > -
> >  	reinit_completion(&dev->cmd_complete);
> >  	dev->msgs = msgs;
> >  	dev->msgs_num = num;
> > @@ -832,10 +808,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  	dev->abort_source = 0;
> >  	dev->rx_outstanding = 0;
> >  
> > -	ret = i2c_dw_acquire_lock(dev);
> > -	if (ret)
> > -		goto done_nolock;
> > -
> >  	ret = i2c_dw_wait_bus_not_busy(dev);
> >  	if (ret < 0)
> >  		goto done;
> > @@ -896,13 +868,75 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  
> >  	ret = -EIO;
> >  
> > +done:
> > +	return ret;
> > +}
> > +
> > +static int
> > +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> 
> Typically if you have
> 
> i2c_dw_xfer()
> i2c_dw_xfer_unlocked()
> 
> The only difference is that the former call the latter with lock held.
> However, this is not the case here which is confusing.
> 
> I suggest move the lookup to be part of the _unlocked() variant.
> 
> While there, can we use size_t with the num parameter.
> 
Maybe __i2c_dw_xfer_unlocked() is the wrong name, as it is meant to be
called potentially multiple times from i2c_dw_xfer(), depending on the
number of messages flagged with I2C_M_STOP. So it is not an unlocked
version of i2c_dw_xfer(). I'll try to find a better name.

> > +{
> > +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > +	struct i2c_msg *msg;
> > +	int ret, cnt;
> > +
> > +	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> > +
> > +	pm_runtime_get_sync(dev->dev);
> > +
> > +	switch (dev->flags & MODEL_MASK) {
> > +	case MODEL_AMD_NAVI_GPU:
> > +		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> > +		goto done_nolock;
> 
> Not really problem of this patch but I'm sure there is cleaner way to deal
> with this. I mean first of all we don't wan't to differentiate here what
> model this is. Instead we can look for a "quirks" based on dev->flags.
> 
> Secondly goto inside switch is confusing too.
> 
> This can be better written like:
> 
> if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) {
> 	...
> }
> 
If we also add the ACQUIRE() for pm_runtime suggested by Andy to
amd_i2c_dw_xfer_quirk(), we could then create a struct i2c_algorithm
dedicated to this hardware and set it in i2c_dw_probe_master(). This way
we can skip all this code, no need to check the model for every transfer.

> > +	default:
> > +		break;
> > +	}
> > +
> > +	ret = i2c_dw_acquire_lock(dev);
> > +	if (ret)
> > +		goto done_nolock;
> > +
> > +	/*
> > +	 * If the I2C_M_STOP is present in some the messages,
> > +	 * we do one transaction for each part up to the STOP.
> > +	 */
> > +	for (msg = msgs; msg < msgs + num; msg += cnt) {
> > +		u16 addr = msg->addr;
> > +
> > +		/*
> > +		 * Count the messages in a transaction, up to a STOP
> > +		 * or the end of the msgs.
> > +		 */
> > +		for (cnt = 1; ; cnt++) {
> > +			/*
> > +			 * We cannot change the target address during
> > +			 * a transaction, so make sure the address stays
> > +			 * the same.
> > +			 */
> > +			if (msg[cnt - 1].addr != addr) {
> > +				dev_err(dev->dev, "invalid target address\n");
> > +				ret = -EINVAL;
> > +				goto done;
> > +			}
> > +
> > +			if ((msg[cnt - 1].flags & I2C_M_STOP) ||
> > +			    (msg + cnt == msgs + num))
> > +				break;
> > +		}
> > +
> > +		ret = __i2c_dw_xfer_unlocked(dev, msg, cnt);
> > +		if (ret < 0)
> > +			goto done;
> 
> This can be
> 
> 	break;
> 
> > +	}
> > +
> >  done:
> >  	i2c_dw_release_lock(dev);
> >  
> >  done_nolock:
> >  	pm_runtime_put_autosuspend(dev->dev);
> >  
> > -	return ret;
> > +	if (ret < 0)
> > +		return ret;
> > +	return num;
> >  }
> >  
> >  static const struct i2c_algorithm i2c_dw_algo = {
> > @@ -920,6 +954,9 @@ void i2c_dw_configure_master(struct dw_i2c_dev *dev)
> >  
> >  	dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
> >  
> > +	if ((dev->flags & MODEL_MASK) != MODEL_AMD_NAVI_GPU)
> > +		dev->functionality |= I2C_FUNC_PROTOCOL_MANGLING;
> > +
> >  	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
> >  			  DW_IC_CON_RESTART_EN;
> >  
> > 
> 


Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 5/5] i2c: designware: Support of controller with IC_EMPTYFIFO_HOLD_MASTER disabled

[Benoît Monin]

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