| Message ID | 20240715151824.90033-1-eichest@gmail.com |
|---|---|
| Headers | show |
| Series | i2c: imx: prevent rescheduling in non-dma mode | expand |
On Mon, Jul 15, 2024 at 05:17:51PM +0200, Stefan Eichenberger wrote: > From: Stefan Eichenberger <stefan.eichenberger@toradex.com> > > According to the reference manual it is only necessary to poll for bus I supposed it should be "According to i2c spec", > busy and arbitration lost in multi master mode. This helps to avoid > rescheduling while the i2c bus is busy and avoids SMBus devices to > timeout. > Reviewed-by: Frank Li <Frank.Li@nxp.com> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com> > --- > drivers/i2c/busses/i2c-imx.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 3842e527116b7..1add946e3bc20 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -216,6 +216,8 @@ struct imx_i2c_struct { > struct i2c_client *slave; > enum i2c_slave_event last_slave_event; > > + bool multi_master; > + > /* For checking slave events. */ > spinlock_t slave_lock; > struct hrtimer slave_timer; > @@ -481,6 +483,9 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a > unsigned long orig_jiffies = jiffies; > unsigned int temp; > > + if (!i2c_imx->multi_master) > + return 0; > + > while (1) { > temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR); > > @@ -540,8 +545,8 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool atomic) > return -ETIMEDOUT; > } > > - /* check for arbitration lost */ > - if (i2c_imx->i2csr & I2SR_IAL) { > + /* In multi-master mode check for arbitration lost */ > + if (i2c_imx->multi_master && (i2c_imx->i2csr & I2SR_IAL)) { > dev_dbg(&i2c_imx->adapter.dev, "<%s> Arbitration lost\n", __func__); > i2c_imx_clear_irq(i2c_imx, I2SR_IAL); > > @@ -1468,6 +1473,8 @@ static int i2c_imx_probe(struct platform_device *pdev) > goto rpm_disable; > } > > + i2c_imx->multi_master = of_property_read_bool(pdev->dev.of_node, "multi-master"); > + > /* Set up clock divider */ > i2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ; > ret = of_property_read_u32(pdev->dev.of_node, > -- > 2.43.0 >
On Mon, Jul 15, 2024 at 05:17:53PM +0200, Stefan Eichenberger wrote: > From: Stefan Eichenberger <stefan.eichenberger@toradex.com> > > 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@toradex.com> > --- > 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 >
Hi Frank, On Mon, Jul 15, 2024 at 01:03:26PM -0400, Frank Li wrote: > > +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. > I think there is not FIFO on the i.MX I2C controller, or do I miss something? In the i.MX 8M Plus reference manual they write for example: > In Master Receive mode, reading the data register allows a read to occur > and initiates the next byte to be received. I will test the changes with readb_relaxed() and writeb_relaxed() and see what happens, thanks a lot for the hint. Regards, Stefan