| Message ID | 20240715151824.90033-4-eichest@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: imx: prevent rescheduling in non-dma mode | expand |
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
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); +} + +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;