diff mbox series

[5/5] i2c: xiic: Only ever transfer single message

Message ID 20200613150751.114595-5-marex@denx.de
State New
Headers show
Series [1/5] i2c: xiic: Fix broken locking on tx_msg | expand

Commit Message

Marek Vasut June 13, 2020, 3:07 p.m. UTC
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(-)

Comments

Wolfram Sang June 13, 2020, 7:33 p.m. UTC | #1
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
Marek Vasut June 13, 2020, 7:37 p.m. UTC | #2
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.
Wolfram Sang June 13, 2020, 7:42 p.m. UTC | #3
> >> 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.
Raviteja Narayanam June 26, 2020, 12:14 p.m. UTC | #4
> -----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
Michal Simek July 13, 2020, 8:37 a.m. UTC | #5
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 mbox series

Patch

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)