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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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