| Message ID | 20200613150751.114595-5-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/5] i2c: xiic: Fix broken locking on tx_msg | expand |
Hi Marek, On Sat, Jun 13, 2020 at 05:07:51PM +0200, Marek Vasut wrote: > Transferring multiple messages via XIIC suffers from strange interaction > between the interrupt status/enable register flags. These flags are being > reused in the hardware to indicate different things for read and write > transfer, and doing multiple transactions becomes horribly complex. Just > send a single transaction and reload the controller with another message > once the transaction is done in the interrupt handler thread. Do we still get a repeated start between messages of a transfer? Or will it be a STOP/START combination? Happy hacking, Wolfram
On 6/13/20 9:33 PM, Wolfram Sang wrote: > Hi Marek, Hi, > On Sat, Jun 13, 2020 at 05:07:51PM +0200, Marek Vasut wrote: >> Transferring multiple messages via XIIC suffers from strange interaction >> between the interrupt status/enable register flags. These flags are being >> reused in the hardware to indicate different things for read and write >> transfer, and doing multiple transactions becomes horribly complex. Just >> send a single transaction and reload the controller with another message >> once the transaction is done in the interrupt handler thread. > > Do we still get a repeated start between messages of a transfer? Or will > it be a STOP/START combination? Repeated start.
> >> Transferring multiple messages via XIIC suffers from strange interaction > >> between the interrupt status/enable register flags. These flags are being > >> reused in the hardware to indicate different things for read and write > >> transfer, and doing multiple transactions becomes horribly complex. Just > >> send a single transaction and reload the controller with another message > >> once the transaction is done in the interrupt handler thread. > > > > Do we still get a repeated start between messages of a transfer? Or will > > it be a STOP/START combination? > > Repeated start. Good. That was my high level question. I'll leave the rest of the review then for Michal.
> -----Original Message----- > From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On > Behalf Of Marek Vasut > Sent: Saturday, June 13, 2020 8:38 PM > To: linux-i2c@vger.kernel.org > Cc: Marek Vasut <marex@denx.de>; Michal Simek <michals@xilinx.com>; > Shubhrajyoti Datta <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org> > Subject: [PATCH 5/5] i2c: xiic: Only ever transfer single message > > Transferring multiple messages via XIIC suffers from strange interaction > between the interrupt status/enable register flags. These flags are being reused > in the hardware to indicate different things for read and write transfer, and > doing multiple transactions becomes horribly complex. Just send a single > transaction and reload the controller with another message once the > transaction is done in the interrupt handler thread. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Michal Simek <michal.simek@xilinx.com> > Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > Cc: Wolfram Sang <wsa@kernel.org> > --- > drivers/i2c/busses/i2c-xiic.c | 43 ++++++++--------------------------- > 1 file changed, 10 insertions(+), 33 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index > e4c3427b2f3f5..fad0b84a273d1 100644 > --- a/drivers/i2c/busses/i2c-xiic.c > +++ b/drivers/i2c/busses/i2c-xiic.c > @@ -595,8 +595,6 @@ static void xiic_start_send(struct xiic_i2c *i2c) { > struct i2c_msg *msg = i2c->tx_msg; > > - xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK); > - > dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, len: %d", > __func__, msg, msg->len); > dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n", @@ > -614,11 +612,13 @@ static void xiic_start_send(struct xiic_i2c *i2c) > xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); > } > > - xiic_fill_tx_fifo(i2c); > - > /* Clear any pending Tx empty, Tx Error and then enable them. */ > xiic_irq_clr_en(i2c, XIIC_INTR_TX_EMPTY_MASK | > XIIC_INTR_TX_ERROR_MASK | > - XIIC_INTR_BNB_MASK); > + XIIC_INTR_BNB_MASK | > + ((i2c->nmsgs > 1 || xiic_tx_space(i2c)) ? > + XIIC_INTR_TX_HALF_MASK : 0)); > + > + xiic_fill_tx_fifo(i2c); > } > > static void __xiic_start_xfer(struct xiic_i2c *i2c) @@ -634,35 +634,12 @@ > static void __xiic_start_xfer(struct xiic_i2c *i2c) Can remove the definition of unused variable ("first"). > i2c->rx_pos = 0; > i2c->tx_pos = 0; > i2c->state = STATE_START; > - while ((fifo_space >= 2) && (first || (i2c->nmsgs > 1))) { > - if (!first) { > - i2c->nmsgs--; > - i2c->tx_msg++; > - i2c->tx_pos = 0; > - } else > - first = 0; > - > - if (i2c->tx_msg->flags & I2C_M_RD) { > - /* we dont date putting several reads in the FIFO */ > - xiic_start_recv(i2c); > - return; > - } else { > - xiic_start_send(i2c); > - if (xiic_tx_space(i2c) != 0) { > - /* the message could not be completely sent > */ > - break; > - } > - } > - > - fifo_space = xiic_tx_fifo_space(i2c); > + if (i2c->tx_msg->flags & I2C_M_RD) { > + /* we dont date putting several reads in the FIFO */ > + xiic_start_recv(i2c); > + } else { > + xiic_start_send(i2c); > } > - > - /* there are more messages or the current one could not be completely > - * put into the FIFO, also enable the half empty interrupt > - */ > - if (i2c->nmsgs > 1 || xiic_tx_space(i2c)) > - xiic_irq_clr_en(i2c, XIIC_INTR_TX_HALF_MASK); > - > } > > static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num) This patch is much needed to simplify the logic. Tested, working fine. Raviteja N
On 13. 06. 20 21:42, Wolfram Sang wrote: > >>>> Transferring multiple messages via XIIC suffers from strange interaction >>>> between the interrupt status/enable register flags. These flags are being >>>> reused in the hardware to indicate different things for read and write >>>> transfer, and doing multiple transactions becomes horribly complex. Just >>>> send a single transaction and reload the controller with another message >>>> once the transaction is done in the interrupt handler thread. >>> >>> Do we still get a repeated start between messages of a transfer? Or will >>> it be a STOP/START combination? >> >> Repeated start. > > Good. That was my high level question. I'll leave the rest of the review > then for Michal. > I have read all reviews in this thread and there should be update on 4/5 and discussion around 2/5 is not done. That's why please send 1/3/4/5 as v2 and 2/5 separately to deal with this separately. Thanks, Michal
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index e4c3427b2f3f5..fad0b84a273d1 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -595,8 +595,6 @@ static void xiic_start_send(struct xiic_i2c *i2c) { struct i2c_msg *msg = i2c->tx_msg; - xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK); - dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, len: %d", __func__, msg, msg->len); dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n", @@ -614,11 +612,13 @@ static void xiic_start_send(struct xiic_i2c *i2c) xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); } - xiic_fill_tx_fifo(i2c); - /* Clear any pending Tx empty, Tx Error and then enable them. */ xiic_irq_clr_en(i2c, XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_ERROR_MASK | - XIIC_INTR_BNB_MASK); + XIIC_INTR_BNB_MASK | + ((i2c->nmsgs > 1 || xiic_tx_space(i2c)) ? + XIIC_INTR_TX_HALF_MASK : 0)); + + xiic_fill_tx_fifo(i2c); } static void __xiic_start_xfer(struct xiic_i2c *i2c) @@ -634,35 +634,12 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c) i2c->rx_pos = 0; i2c->tx_pos = 0; i2c->state = STATE_START; - while ((fifo_space >= 2) && (first || (i2c->nmsgs > 1))) { - if (!first) { - i2c->nmsgs--; - i2c->tx_msg++; - i2c->tx_pos = 0; - } else - first = 0; - - if (i2c->tx_msg->flags & I2C_M_RD) { - /* we dont date putting several reads in the FIFO */ - xiic_start_recv(i2c); - return; - } else { - xiic_start_send(i2c); - if (xiic_tx_space(i2c) != 0) { - /* the message could not be completely sent */ - break; - } - } - - fifo_space = xiic_tx_fifo_space(i2c); + if (i2c->tx_msg->flags & I2C_M_RD) { + /* we dont date putting several reads in the FIFO */ + xiic_start_recv(i2c); + } else { + xiic_start_send(i2c); } - - /* there are more messages or the current one could not be completely - * put into the FIFO, also enable the half empty interrupt - */ - if (i2c->nmsgs > 1 || xiic_tx_space(i2c)) - xiic_irq_clr_en(i2c, XIIC_INTR_TX_HALF_MASK); - } static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
Transferring multiple messages via XIIC suffers from strange interaction between the interrupt status/enable register flags. These flags are being reused in the hardware to indicate different things for read and write transfer, and doing multiple transactions becomes horribly complex. Just send a single transaction and reload the controller with another message once the transaction is done in the interrupt handler thread. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Michal Simek <michal.simek@xilinx.com> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> Cc: Wolfram Sang <wsa@kernel.org> --- drivers/i2c/busses/i2c-xiic.c | 43 ++++++++--------------------------- 1 file changed, 10 insertions(+), 33 deletions(-)