Re: [PATCH v1 3/3] i2c: imx: prevent rescheduling in non dma mode

[Frank Li]

On Mon, Jul 15, 2024 at 05:17:53PM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@xxxxxxxxxxx>
> 
> We are experiencing a problem with the i.MX I2C controller when
> communicating with SMBus devices. We are seeing devices time-out because
> the time between sending/receiving two bytes is too long, and the SMBus
> device returns to the idle state. This happens because the i.MX I2C
> controller sends and receives byte by byte. When a byte is sent or
> received, we get an interrupt and can send or receive the next byte.
> 
> The current implementation sends a byte and then waits for an event
> generated by the interrupt subroutine. After the event is received, the
> next byte is sent and we wait again. This waiting allows the scheduler
> to reschedule other tasks, with the disadvantage that we may not send
> the next byte for a long time because the send task is not immediately
> scheduled. For example, if the rescheduling takes more than 25ms, this
> can cause SMBus devices to timeout and communication to fail.
> 
> This patch changes the behavior so that we do not reschedule the
> send/receive task, but instead send or receive the next byte in the
> interrupt subroutine. This prevents rescheduling and drastically reduces
> the time between sending/receiving bytes. The cost in the interrupt
> subroutine is relatively small, we check what state we are in and then
> send/receive the next byte. Before we had to call wake_up, which is even
> less expensive. However, we also had to do some scheduling, which
> increased the overall cost compared to the new solution. The wake_up
> function to wake up the send/receive task is now only called when an
> error occurs or when the transfer is complete.
> 
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@xxxxxxxxxxx>
> ---
>  drivers/i2c/busses/i2c-imx.c | 257 ++++++++++++++++++++++++++++++++---
>  1 file changed, 235 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index e242166cb6638..ac21c2001596e 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -197,6 +197,17 @@ struct imx_i2c_dma {
>  	enum dma_data_direction dma_data_dir;
>  };
>  
> +enum imx_i2c_state {
> +	IMX_I2C_STATE_DONE,
> +	IMX_I2C_STATE_FAILED,
> +	IMX_I2C_STATE_WRITE,
> +	IMX_I2C_STATE_DMA,
> +	IMX_I2C_STATE_READ,
> +	IMX_I2C_STATE_READ_CONTINUE,
> +	IMX_I2C_STATE_READ_BLOCK_DATA,
> +	IMX_I2C_STATE_READ_BLOCK_DATA_LEN,
> +};
> +
>  struct imx_i2c_struct {
>  	struct i2c_adapter	adapter;
>  	struct clk		*clk;
> @@ -216,6 +227,12 @@ struct imx_i2c_struct {
>  	struct i2c_client	*slave;
>  	enum i2c_slave_event last_slave_event;
>  
> +	struct i2c_msg		*msg;
> +	unsigned int		msg_buf_idx;
> +	int			isr_result;
> +	bool			is_lastmsg;
> +	enum imx_i2c_state	state;
> +
>  	bool			multi_master;
>  
>  	/* For checking slave events. */
> @@ -908,11 +925,150 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
>  	return ret;
>  }
>  
> +static inline int i2c_imx_isr_acked(struct imx_i2c_struct *i2c_imx)
> +{
> +	i2c_imx->isr_result = 0;
> +
> +	if (imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) & I2SR_RXAK) {
> +		i2c_imx->state = IMX_I2C_STATE_FAILED;
> +		i2c_imx->isr_result = -ENXIO;
> +		wake_up(&i2c_imx->queue);
> +	}
> +
> +	return i2c_imx->isr_result;
> +}
> +
> +static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
> +{
> +	int result;
> +
> +	result = i2c_imx_isr_acked(i2c_imx);
> +	if (result)
> +		return result;
> +
> +	if (i2c_imx->msg->len == i2c_imx->msg_buf_idx)
> +		return 0;
> +
> +	imx_i2c_write_reg(i2c_imx->msg->buf[i2c_imx->msg_buf_idx++], i2c_imx, IMX_I2C_I2DR);
> +
> +	return 1;
> +}
> +
> +static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
> +{
> +	int result;
> +	unsigned int temp;
> +
> +	result = i2c_imx_isr_acked(i2c_imx);
> +	if (result)
> +		return result;
> +
> +	/* setup bus to read data */
> +	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	temp &= ~I2CR_MTX;
> +	if (i2c_imx->msg->len - 1)
> +		temp &= ~I2CR_TXAK;
> +
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
> +
> +	return 0;
> +}
> +
> +static inline void i2c_imx_isr_read_continue(struct imx_i2c_struct *i2c_imx)
> +{
> +	unsigned int temp;
> +
> +	if ((i2c_imx->msg->len - 1) == i2c_imx->msg_buf_idx) {
> +		if (i2c_imx->is_lastmsg) {
> +			/*
> +			 * It must generate STOP before read I2DR to prevent
> +			 * controller from generating another clock cycle
> +			 */
> +			temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +			if (!(temp & I2CR_MSTA))
> +				i2c_imx->stopped =  1;
> +			temp &= ~(I2CR_MSTA | I2CR_MTX);
> +			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +		} else {
> +			/*
> +			 * For i2c master receiver repeat restart operation like:
> +			 * read -> repeat MSTA -> read/write
> +			 * The controller must set MTX before read the last byte in
> +			 * the first read operation, otherwise the first read cost
> +			 * one extra clock cycle.
> +			 */
> +			temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +			temp |= I2CR_MTX;
> +			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +		}
> +	} else if (i2c_imx->msg_buf_idx == (i2c_imx->msg->len - 2)) {
> +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +		temp |= I2CR_TXAK;
> +		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +	}
> +
> +	i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);

Why not use loop to read all data from FIFO? I think read_reg use readb(),
suggest change to readb_relaxed(). The similar case for writeb. dma_engine
will use writel() at least once when start DMA. it should be enough for
memory barrier. 

Because it move to irq handle, writex__relaxed() will help reduce some
register access time.

> +}
> +
> +static inline void i2c_imx_isr_read_block_data_len(struct imx_i2c_struct *i2c_imx)
> +{
> +	u8 len = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +
> +	if ((len == 0) || (len > I2C_SMBUS_BLOCK_MAX)) {
> +		i2c_imx->isr_result = -EPROTO;
> +		i2c_imx->state = IMX_I2C_STATE_FAILED;
> +		wake_up(&i2c_imx->queue);
> +	}
> +	i2c_imx->msg->len += len;
> +}
> +
>  static irqreturn_t i2c_imx_master_isr(struct imx_i2c_struct *i2c_imx, unsigned int status)
>  {
> -	/* save status register */
> -	i2c_imx->i2csr = status;
> -	wake_up(&i2c_imx->queue);
> +	switch (i2c_imx->state) {
> +	case IMX_I2C_STATE_DMA:
> +		i2c_imx->i2csr = status;
> +		wake_up(&i2c_imx->queue);
> +		break;
> +
> +	case IMX_I2C_STATE_READ:
> +		if (i2c_imx_isr_read(i2c_imx))
> +			break;
> +		i2c_imx->state = IMX_I2C_STATE_READ_CONTINUE;
> +		break;
> +
> +	case IMX_I2C_STATE_READ_CONTINUE:
> +		i2c_imx_isr_read_continue(i2c_imx);
> +		if (i2c_imx->msg_buf_idx == i2c_imx->msg->len) {
> +			i2c_imx->state = IMX_I2C_STATE_DONE;
> +			wake_up(&i2c_imx->queue);
> +		}
> +		break;
> +
> +	case IMX_I2C_STATE_READ_BLOCK_DATA:
> +		if (i2c_imx_isr_read(i2c_imx))
> +			break;
> +		i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA_LEN;
> +		break;
> +
> +	case IMX_I2C_STATE_READ_BLOCK_DATA_LEN:
> +		i2c_imx_isr_read_block_data_len(i2c_imx);
> +		i2c_imx->state = IMX_I2C_STATE_READ_CONTINUE;
> +		break;
> +
> +	case IMX_I2C_STATE_WRITE:
> +		if (i2c_imx_isr_write(i2c_imx))
> +			break;
> +		i2c_imx->state = IMX_I2C_STATE_DONE;
> +		wake_up(&i2c_imx->queue);
> +		break;
> +
> +	default:
> +		i2c_imx->i2csr = status;
> +		i2c_imx->state = IMX_I2C_STATE_FAILED;
> +		i2c_imx->isr_result = -EINVAL;
> +		wake_up(&i2c_imx->queue);
> +	}
>  
>  	return IRQ_HANDLED;
>  }
> @@ -959,6 +1115,8 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
>  	struct imx_i2c_dma *dma = i2c_imx->dma;
>  	struct device *dev = &i2c_imx->adapter.dev;
>  
> +	i2c_imx->state = IMX_I2C_STATE_DMA;
> +
>  	dma->chan_using = dma->chan_tx;
>  	dma->dma_transfer_dir = DMA_MEM_TO_DEV;
>  	dma->dma_data_dir = DMA_TO_DEVICE;
> @@ -1012,15 +1170,14 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
>  }
>  
>  static int i2c_imx_start_read(struct imx_i2c_struct *i2c_imx,
> -			       struct i2c_msg *msgs, bool atomic,
> -			       bool use_dma)
> +			       struct i2c_msg *msgs, bool use_dma)
>  {
>  	int result;
>  	unsigned int temp = 0;
>  
>  	/* write slave address */
>  	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
> -	result = i2c_imx_trx_complete(i2c_imx, atomic);
> +	result = i2c_imx_trx_complete(i2c_imx, !use_dma);
>  	if (result)
>  		return result;
>  	result = i2c_imx_acked(i2c_imx);
> @@ -1058,7 +1215,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
>  	struct imx_i2c_dma *dma = i2c_imx->dma;
>  	struct device *dev = &i2c_imx->adapter.dev;
>  
> -	result = i2c_imx_start_read(i2c_imx, msgs, false, true);
> +	i2c_imx->state = IMX_I2C_STATE_DMA;
> +
> +	result = i2c_imx_start_read(i2c_imx, msgs, true);
>  	if (result)
>  		return result;
>  
> @@ -1139,8 +1298,8 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
>  	return 0;
>  }
>  
> -static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
> -			 bool atomic)
> +static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
> +				struct i2c_msg *msgs)
>  {
>  	int i, result;
>  
> @@ -1149,7 +1308,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>  
>  	/* write slave address */
>  	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
> -	result = i2c_imx_trx_complete(i2c_imx, atomic);
> +	result = i2c_imx_trx_complete(i2c_imx, true);
>  	if (result)
>  		return result;
>  	result = i2c_imx_acked(i2c_imx);
> @@ -1163,7 +1322,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>  			"<%s> write byte: B%d=0x%X\n",
>  			__func__, i, msgs->buf[i]);
>  		imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
> -		result = i2c_imx_trx_complete(i2c_imx, atomic);
> +		result = i2c_imx_trx_complete(i2c_imx, true);
>  		if (result)
>  			return result;
>  		result = i2c_imx_acked(i2c_imx);
> @@ -1173,19 +1332,40 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>  	return 0;
>  }
>  
> -static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> +static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
>  {
> -	return i2c_imx_write(i2c_imx, msgs, true);
> +	dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
> +		__func__, i2c_8bit_addr_from_msg(msgs));
> +
> +	i2c_imx->state = IMX_I2C_STATE_WRITE;
> +	i2c_imx->msg = msgs;
> +	i2c_imx->msg_buf_idx = 0;
> +	/* write slave address and start transmission */
> +	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
> +	wait_event_timeout(i2c_imx->queue,
> +			   i2c_imx->state == IMX_I2C_STATE_DONE ||
> +			   i2c_imx->state == IMX_I2C_STATE_FAILED,
> +			   (msgs->len + 1)*HZ / 10);
> +	if (i2c_imx->state == IMX_I2C_STATE_FAILED) {
> +		dev_err(&i2c_imx->adapter.dev, "<%s> write failed with %d\n",
> +			__func__, i2c_imx->isr_result);
> +		return i2c_imx->isr_result;
> +	}
> +	if (i2c_imx->state != IMX_I2C_STATE_DONE) {
> +		dev_err(&i2c_imx->adapter.dev, "<%s> write timedout\n", __func__);
> +		return -ETIMEDOUT;
> +	}
> +	return 0;
>  }
>  
> -static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
> -			bool is_lastmsg, bool atomic)
> +static int i2c_imx_atomic_read(struct imx_i2c_struct *i2c_imx,
> +			       struct i2c_msg *msgs, bool is_lastmsg)
>  {
>  	int i, result;
>  	unsigned int temp;
>  	int block_data = msgs->flags & I2C_M_RECV_LEN;
>  
> -	result = i2c_imx_start_read(i2c_imx, msgs, atomic, false);
> +	result = i2c_imx_start_read(i2c_imx, msgs, false);
>  	if (result)
>  		return result;
>  
> @@ -1195,7 +1375,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>  	for (i = 0; i < msgs->len; i++) {
>  		u8 len = 0;
>  
> -		result = i2c_imx_trx_complete(i2c_imx, atomic);
> +		result = i2c_imx_trx_complete(i2c_imx, true);
>  		if (result)
>  			return result;
>  		/*
> @@ -1226,7 +1406,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>  				temp &= ~(I2CR_MSTA | I2CR_MTX);
>  				imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>  				if (!i2c_imx->stopped)
> -					i2c_imx_bus_busy(i2c_imx, 0, atomic);
> +					i2c_imx_bus_busy(i2c_imx, 0, true);
>  			} else {
>  				/*
>  				 * For i2c master receiver repeat restart operation like:
> @@ -1257,10 +1437,43 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>  	return 0;
>  }
>  
> -static int i2c_imx_atomic_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
> +static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>  			bool is_lastmsg)
>  {
> -	return i2c_imx_read(i2c_imx, msgs, is_lastmsg, true);
> +	int block_data = msgs->flags & I2C_M_RECV_LEN;
> +
> +	dev_dbg(&i2c_imx->adapter.dev,
> +		"<%s> write slave address: addr=0x%x\n",
> +		__func__, i2c_8bit_addr_from_msg(msgs));
> +
> +	i2c_imx->is_lastmsg = is_lastmsg;
> +
> +	if (block_data)
> +		i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA;
> +	else
> +		i2c_imx->state = IMX_I2C_STATE_READ;
> +	i2c_imx->msg = msgs;
> +	i2c_imx->msg_buf_idx = 0;
> +
> +	/* write slave address */
> +	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
> +	wait_event_timeout(i2c_imx->queue,
> +			   i2c_imx->state == IMX_I2C_STATE_DONE ||
> +			   i2c_imx->state == IMX_I2C_STATE_FAILED,
> +			   (msgs->len + 1)*HZ / 10);
> +	if (i2c_imx->state == IMX_I2C_STATE_FAILED) {
> +		dev_err(&i2c_imx->adapter.dev, "<%s> write failed with %d\n",
> +			__func__, i2c_imx->isr_result);
> +		return i2c_imx->isr_result;
> +	}
> +	if (i2c_imx->state != IMX_I2C_STATE_DONE) {
> +		dev_err(&i2c_imx->adapter.dev, "<%s> write timedout\n", __func__);
> +		return -ETIMEDOUT;
> +	}
> +	if (!i2c_imx->stopped)
> +		return i2c_imx_bus_busy(i2c_imx, 0, false);
> +
> +	return 0;
>  }
>  
>  static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
> @@ -1334,14 +1547,14 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>  			else if (use_dma && !block_data)
>  				result = i2c_imx_dma_read(i2c_imx, &msgs[i], is_lastmsg);
>  			else
> -				result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg, false);
> +				result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg);
>  		} else {
>  			if (atomic)
>  				result = i2c_imx_atomic_write(i2c_imx, &msgs[i]);
>  			else if (use_dma)
>  				result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
>  			else
> -				result = i2c_imx_write(i2c_imx, &msgs[i], false);
> +				result = i2c_imx_write(i2c_imx, &msgs[i]);
>  		}
>  		if (result)
>  			goto fail0;
> -- 
> 2.43.0
>