diff mbox series

[3/5] i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in xiic_process()

Message ID 20200613150751.114595-3-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
The __xiic_start_xfer() manipulates the interrupt flags, xiic_wakeup()
may result in return from xiic_xfer() early. Defer both to the end of
the xiic_process() interrupt thread, so that they are executed after
all the other interrupt bits handling completed and once it completely
safe to perform changes to the interrupt bits in the hardware.

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 | 37 ++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

Comments

Raviteja Narayanam June 26, 2020, 12:13 p.m. UTC | #1
> -----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 3/5] i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in
> xiic_process()
> 
> The __xiic_start_xfer() manipulates the interrupt flags, xiic_wakeup() may
> result in return from xiic_xfer() early. Defer both to the end of the
> xiic_process() interrupt thread, so that they are executed after all the other
> interrupt bits handling completed and once it completely safe to perform
> changes to the interrupt bits in the hardware.
> 
> 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 | 37 ++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
> 6db71c0fb6583..87eca9d46afd9 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -373,6 +373,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  	struct xiic_i2c *i2c = dev_id;
>  	u32 pend, isr, ier;
>  	u32 clr = 0;
> +	int xfer_more = 0;
> +	int wakeup_req = 0;
> +	int wakeup_code = 0;
> 
>  	/* Get the interrupt Status from the IPIF. There is no clearing of
>  	 * interrupts in the IPIF. Interrupts must be cleared at the source.
> @@ -409,10 +412,14 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  		 */
>  		xiic_reinit(i2c);
> 
> -		if (i2c->rx_msg)
> -			xiic_wakeup(i2c, STATE_ERROR);
> -		if (i2c->tx_msg)
> -			xiic_wakeup(i2c, STATE_ERROR);
> +		if (i2c->rx_msg) {
> +			wakeup_req = 1;
> +			wakeup_code = STATE_ERROR;
> +		}
> +		if (i2c->tx_msg) {
> +			wakeup_req = 1;
> +			wakeup_code = STATE_ERROR;
> +		}
>  	}
>  	if (pend & XIIC_INTR_RX_FULL_MASK) {
>  		/* Receive register/FIFO is full */
> @@ -446,8 +453,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  				i2c->tx_msg++;
>  				dev_dbg(i2c->adap.dev.parent,
>  					"%s will start next...\n", __func__);
> -
> -				__xiic_start_xfer(i2c);
> +				xfer_more = 1;
>  			}
>  		}
>  	}
> @@ -461,11 +467,13 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  		if (!i2c->tx_msg)
>  			goto out;
> 
> -		if ((i2c->nmsgs == 1) && !i2c->rx_msg &&
> -			xiic_tx_space(i2c) == 0)
> -			xiic_wakeup(i2c, STATE_DONE);
> +		wakeup_req = 1;
> +
> +		if (i2c->nmsgs == 1 && !i2c->rx_msg &&
> +		    xiic_tx_space(i2c) == 0)
> +			wakeup_code = STATE_DONE;
>  		else
> -			xiic_wakeup(i2c, STATE_ERROR);
> +			wakeup_code = STATE_ERROR;
>  	}
>  	if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK))
> {
>  		/* Transmit register/FIFO is empty or ½ empty */ @@ -489,7
> +497,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  			if (i2c->nmsgs > 1) {
>  				i2c->nmsgs--;
>  				i2c->tx_msg++;
> -				__xiic_start_xfer(i2c);
> +				xfer_more = 1;
>  			} else {
>  				xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
> 
> @@ -507,6 +515,13 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
>  	dev_dbg(i2c->adap.dev.parent, "%s clr: 0x%x\n", __func__, clr);
> 
>  	xiic_setreg32(i2c, XIIC_IISR_OFFSET, clr);
> +	if (xfer_more)
> +		__xiic_start_xfer(i2c);
> +	if (wakeup_req)
> +		xiic_wakeup(i2c, wakeup_code);
> +
> +	WARN_ON(xfer_more && wakeup_req);
> +
>  	mutex_unlock(&i2c->lock);
>  	return IRQ_HANDLED;
>  }

This is tested and working fine.

Raviteja N
Raviteja Narayanam July 8, 2020, 3:23 p.m. UTC | #2
Tested-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

Thanks
Raviteja N

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Raviteja Narayanam
> Sent: Friday, June 26, 2020 5:44 PM
> To: Marek Vasut <marex@denx.de>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: RE: [PATCH 3/5] i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in
> xiic_process()
> 
> 
> 
> > -----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 3/5] i2c: xiic: Defer xiic_wakeup() and
> > __xiic_start_xfer() in
> > xiic_process()
> >
> > The __xiic_start_xfer() manipulates the interrupt flags, xiic_wakeup()
> > may result in return from xiic_xfer() early. Defer both to the end of
> > the
> > xiic_process() interrupt thread, so that they are executed after all
> > the other interrupt bits handling completed and once it completely
> > safe to perform changes to the interrupt bits in the hardware.
> >
> > 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 | 37
> > ++++++++++++++++++++++++-----------
> >  1 file changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-xiic.c
> > b/drivers/i2c/busses/i2c-xiic.c index
> > 6db71c0fb6583..87eca9d46afd9 100644
> > --- a/drivers/i2c/busses/i2c-xiic.c
> > +++ b/drivers/i2c/busses/i2c-xiic.c
> > @@ -373,6 +373,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
> >  	struct xiic_i2c *i2c = dev_id;
> >  	u32 pend, isr, ier;
> >  	u32 clr = 0;
> > +	int xfer_more = 0;
> > +	int wakeup_req = 0;
> > +	int wakeup_code = 0;
> >
> >  	/* Get the interrupt Status from the IPIF. There is no clearing of
> >  	 * interrupts in the IPIF. Interrupts must be cleared at the source.
> > @@ -409,10 +412,14 @@ static irqreturn_t xiic_process(int irq, void
> *dev_id)
> >  		 */
> >  		xiic_reinit(i2c);
> >
> > -		if (i2c->rx_msg)
> > -			xiic_wakeup(i2c, STATE_ERROR);
> > -		if (i2c->tx_msg)
> > -			xiic_wakeup(i2c, STATE_ERROR);
> > +		if (i2c->rx_msg) {
> > +			wakeup_req = 1;
> > +			wakeup_code = STATE_ERROR;
> > +		}
> > +		if (i2c->tx_msg) {
> > +			wakeup_req = 1;
> > +			wakeup_code = STATE_ERROR;
> > +		}
> >  	}
> >  	if (pend & XIIC_INTR_RX_FULL_MASK) {
> >  		/* Receive register/FIFO is full */ @@ -446,8 +453,7 @@ static
> > irqreturn_t xiic_process(int irq, void *dev_id)
> >  				i2c->tx_msg++;
> >  				dev_dbg(i2c->adap.dev.parent,
> >  					"%s will start next...\n", __func__);
> > -
> > -				__xiic_start_xfer(i2c);
> > +				xfer_more = 1;
> >  			}
> >  		}
> >  	}
> > @@ -461,11 +467,13 @@ static irqreturn_t xiic_process(int irq, void
> *dev_id)
> >  		if (!i2c->tx_msg)
> >  			goto out;
> >
> > -		if ((i2c->nmsgs == 1) && !i2c->rx_msg &&
> > -			xiic_tx_space(i2c) == 0)
> > -			xiic_wakeup(i2c, STATE_DONE);
> > +		wakeup_req = 1;
> > +
> > +		if (i2c->nmsgs == 1 && !i2c->rx_msg &&
> > +		    xiic_tx_space(i2c) == 0)
> > +			wakeup_code = STATE_DONE;
> >  		else
> > -			xiic_wakeup(i2c, STATE_ERROR);
> > +			wakeup_code = STATE_ERROR;
> >  	}
> >  	if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK))
> {
> >  		/* Transmit register/FIFO is empty or ½ empty */ @@ -489,7
> > +497,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
> >  			if (i2c->nmsgs > 1) {
> >  				i2c->nmsgs--;
> >  				i2c->tx_msg++;
> > -				__xiic_start_xfer(i2c);
> > +				xfer_more = 1;
> >  			} else {
> >  				xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
> >
> > @@ -507,6 +515,13 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
> >  	dev_dbg(i2c->adap.dev.parent, "%s clr: 0x%x\n", __func__, clr);
> >
> >  	xiic_setreg32(i2c, XIIC_IISR_OFFSET, clr);
> > +	if (xfer_more)
> > +		__xiic_start_xfer(i2c);
> > +	if (wakeup_req)
> > +		xiic_wakeup(i2c, wakeup_code);
> > +
> > +	WARN_ON(xfer_more && wakeup_req);
> > +
> >  	mutex_unlock(&i2c->lock);
> >  	return IRQ_HANDLED;
> >  }
> 
> This is tested and working fine.
> 
> Raviteja N
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 6db71c0fb6583..87eca9d46afd9 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -373,6 +373,9 @@  static irqreturn_t xiic_process(int irq, void *dev_id)
 	struct xiic_i2c *i2c = dev_id;
 	u32 pend, isr, ier;
 	u32 clr = 0;
+	int xfer_more = 0;
+	int wakeup_req = 0;
+	int wakeup_code = 0;
 
 	/* Get the interrupt Status from the IPIF. There is no clearing of
 	 * interrupts in the IPIF. Interrupts must be cleared at the source.
@@ -409,10 +412,14 @@  static irqreturn_t xiic_process(int irq, void *dev_id)
 		 */
 		xiic_reinit(i2c);
 
-		if (i2c->rx_msg)
-			xiic_wakeup(i2c, STATE_ERROR);
-		if (i2c->tx_msg)
-			xiic_wakeup(i2c, STATE_ERROR);
+		if (i2c->rx_msg) {
+			wakeup_req = 1;
+			wakeup_code = STATE_ERROR;
+		}
+		if (i2c->tx_msg) {
+			wakeup_req = 1;
+			wakeup_code = STATE_ERROR;
+		}
 	}
 	if (pend & XIIC_INTR_RX_FULL_MASK) {
 		/* Receive register/FIFO is full */
@@ -446,8 +453,7 @@  static irqreturn_t xiic_process(int irq, void *dev_id)
 				i2c->tx_msg++;
 				dev_dbg(i2c->adap.dev.parent,
 					"%s will start next...\n", __func__);
-
-				__xiic_start_xfer(i2c);
+				xfer_more = 1;
 			}
 		}
 	}
@@ -461,11 +467,13 @@  static irqreturn_t xiic_process(int irq, void *dev_id)
 		if (!i2c->tx_msg)
 			goto out;
 
-		if ((i2c->nmsgs == 1) && !i2c->rx_msg &&
-			xiic_tx_space(i2c) == 0)
-			xiic_wakeup(i2c, STATE_DONE);
+		wakeup_req = 1;
+
+		if (i2c->nmsgs == 1 && !i2c->rx_msg &&
+		    xiic_tx_space(i2c) == 0)
+			wakeup_code = STATE_DONE;
 		else
-			xiic_wakeup(i2c, STATE_ERROR);
+			wakeup_code = STATE_ERROR;
 	}
 	if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK)) {
 		/* Transmit register/FIFO is empty or ½ empty */
@@ -489,7 +497,7 @@  static irqreturn_t xiic_process(int irq, void *dev_id)
 			if (i2c->nmsgs > 1) {
 				i2c->nmsgs--;
 				i2c->tx_msg++;
-				__xiic_start_xfer(i2c);
+				xfer_more = 1;
 			} else {
 				xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
 
@@ -507,6 +515,13 @@  static irqreturn_t xiic_process(int irq, void *dev_id)
 	dev_dbg(i2c->adap.dev.parent, "%s clr: 0x%x\n", __func__, clr);
 
 	xiic_setreg32(i2c, XIIC_IISR_OFFSET, clr);
+	if (xfer_more)
+		__xiic_start_xfer(i2c);
+	if (wakeup_req)
+		xiic_wakeup(i2c, wakeup_code);
+
+	WARN_ON(xfer_more && wakeup_req);
+
 	mutex_unlock(&i2c->lock);
 	return IRQ_HANDLED;
 }