linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/3] i2c: imx: Fix handling of arbitration loss
@ 2020-10-07  8:45 Christian Eggers
  2020-10-07  8:45 ` [PATCH v5 1/3] i2c: imx: Fix reset of I2SR_IAL flag Christian Eggers
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Christian Eggers @ 2020-10-07  8:45 UTC (permalink / raw)
  To: Oleksij Rempel, Shawn Guo, Sascha Hauer, Fabio Estevam,
	Uwe Kleine-König, David Laight
  Cc: Pengutronix Kernel Team, NXP Linux Team, linux-i2c,
	linux-arm-kernel, linux-kernel, Christian Eggers,
	Krzysztof Kozlowski

Changes in v2:
---------------
- Don't accidently clear additional status flags on Vybrid
  (reported by Uwe Kleine-Koenig)

Changes in v3:
---------------
- dedicated function for clearing an irq

Changes in v4:
---------------
- Extend comment (W1C vs. W0C)

Changes in v5:
---------------
- Added missing "Tested-By" tags.

Best regards
Christian



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v5 1/3] i2c: imx: Fix reset of I2SR_IAL flag
  2020-10-07  8:45 [PATCH v5 0/3] i2c: imx: Fix handling of arbitration loss Christian Eggers
@ 2020-10-07  8:45 ` Christian Eggers
  2020-10-08  9:56   ` Wolfram Sang
  2020-10-09  7:11   ` Oleksij Rempel
  2020-10-07  8:45 ` [PATCH v5 2/3] i2c: imx: Check for I2SR_IAL after every byte Christian Eggers
  2020-10-07  8:45 ` [PATCH v5 3/3] i2c: imx: Don't generate STOP condition if arbitration has been lost Christian Eggers
  2 siblings, 2 replies; 9+ messages in thread
From: Christian Eggers @ 2020-10-07  8:45 UTC (permalink / raw)
  To: Oleksij Rempel, Shawn Guo, Sascha Hauer, Fabio Estevam,
	Uwe Kleine-König, David Laight
  Cc: Pengutronix Kernel Team, NXP Linux Team, linux-i2c,
	linux-arm-kernel, linux-kernel, Christian Eggers,
	Krzysztof Kozlowski, stable

According to the "VFxxx Controller Reference Manual" (and the comment
block starting at line 97), Vybrid requires writing a one for clearing
an interrupt flag. Syncing the method for clearing I2SR_IIF in
i2c_imx_isr().

Signed-off-by: Christian Eggers <ceggers@arri.de>
Fixes: 4b775022f6fd ("i2c: imx: add struct to hold more configurable quirks")
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: stable@vger.kernel.org
---
 drivers/i2c/busses/i2c-imx.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 0ab5381aa012..cbdcab73a055 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -412,6 +412,19 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
 	dma->chan_using = NULL;
 }
 
+static void i2c_imx_clear_irq(struct imx_i2c_struct *i2c_imx, unsigned int bits)
+{
+	unsigned int temp;
+
+	/*
+	 * i2sr_clr_opcode is the value to clear all interrupts. Here we want to
+	 * clear only <bits>, so we write ~i2sr_clr_opcode with just <bits>
+	 * toggled. This is required because i.MX needs W1C and Vybrid uses W0C.
+	 */
+	temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ bits;
+	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
+}
+
 static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool atomic)
 {
 	unsigned long orig_jiffies = jiffies;
@@ -424,8 +437,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
 
 		/* check for arbitration lost */
 		if (temp & I2SR_IAL) {
-			temp &= ~I2SR_IAL;
-			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
+			i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
 			return -EAGAIN;
 		}
 
@@ -623,9 +635,7 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 	if (temp & I2SR_IIF) {
 		/* save status register */
 		i2c_imx->i2csr = temp;
-		temp &= ~I2SR_IIF;
-		temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
-		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
+		i2c_imx_clear_irq(i2c_imx, I2SR_IIF);
 		wake_up(&i2c_imx->queue);
 		return IRQ_HANDLED;
 	}
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v5 2/3] i2c: imx: Check for I2SR_IAL after every byte
  2020-10-07  8:45 [PATCH v5 0/3] i2c: imx: Fix handling of arbitration loss Christian Eggers
  2020-10-07  8:45 ` [PATCH v5 1/3] i2c: imx: Fix reset of I2SR_IAL flag Christian Eggers
@ 2020-10-07  8:45 ` Christian Eggers
  2020-10-09  7:53   ` Oleksij Rempel
  2020-10-07  8:45 ` [PATCH v5 3/3] i2c: imx: Don't generate STOP condition if arbitration has been lost Christian Eggers
  2 siblings, 1 reply; 9+ messages in thread
From: Christian Eggers @ 2020-10-07  8:45 UTC (permalink / raw)
  To: Oleksij Rempel, Shawn Guo, Sascha Hauer, Fabio Estevam,
	Uwe Kleine-König, David Laight
  Cc: Pengutronix Kernel Team, NXP Linux Team, linux-i2c,
	linux-arm-kernel, linux-kernel, Christian Eggers,
	Krzysztof Kozlowski, stable

Arbitration Lost (IAL) can happen after every single byte transfer. If
arbitration is lost, the I2C hardware will autonomously switch from
master mode to slave. If a transfer is not aborted in this state,
consecutive transfers will not be executed by the hardware and will
timeout.

Signed-off-by: Christian Eggers <ceggers@arri.de>
Tested (not extensively) on Vybrid VF500 (Toradex VF50):
Tested-by: Krzysztof Kozlowski <krzk@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/i2c/busses/i2c-imx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index cbdcab73a055..63575af41c09 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -490,6 +490,16 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool atomic)
 		dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
 		return -ETIMEDOUT;
 	}
+
+	/* check for arbitration lost */
+	if (i2c_imx->i2csr & I2SR_IAL) {
+		dev_dbg(&i2c_imx->adapter.dev, "<%s> Arbitration lost\n", __func__);
+		i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
+
+		i2c_imx->i2csr = 0;
+		return -EAGAIN;
+	}
+
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n", __func__);
 	i2c_imx->i2csr = 0;
 	return 0;
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v5 3/3] i2c: imx: Don't generate STOP condition if arbitration has been lost
  2020-10-07  8:45 [PATCH v5 0/3] i2c: imx: Fix handling of arbitration loss Christian Eggers
  2020-10-07  8:45 ` [PATCH v5 1/3] i2c: imx: Fix reset of I2SR_IAL flag Christian Eggers
  2020-10-07  8:45 ` [PATCH v5 2/3] i2c: imx: Check for I2SR_IAL after every byte Christian Eggers
@ 2020-10-07  8:45 ` Christian Eggers
  2020-10-09  8:14   ` Oleksij Rempel
  2 siblings, 1 reply; 9+ messages in thread
From: Christian Eggers @ 2020-10-07  8:45 UTC (permalink / raw)
  To: Oleksij Rempel, Shawn Guo, Sascha Hauer, Fabio Estevam,
	Uwe Kleine-König, David Laight
  Cc: Pengutronix Kernel Team, NXP Linux Team, linux-i2c,
	linux-arm-kernel, linux-kernel, Christian Eggers,
	Krzysztof Kozlowski, stable

If arbitration is lost, the master automatically changes to slave mode.
I2SR_IBB may or may not be reset by hardware. Raising a STOP condition
by resetting I2CR_MSTA has no effect and will not clear I2SR_IBB.

So calling i2c_imx_bus_busy() is not required and would busy-wait until
timeout.

Signed-off-by: Christian Eggers <ceggers@arri.de>
Tested (not extensively) on Vybrid VF500 (Toradex VF50):
Tested-by: Krzysztof Kozlowski <krzk@kernel.org>
Cc: stable@vger.kernel.org # Requires trivial backporting, simple remove
                           # the 3rd argument from the calls to
                           # i2c_imx_bus_busy().
---
 drivers/i2c/busses/i2c-imx.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 63575af41c09..5d8a79319b2b 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -615,6 +615,8 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx, bool atomic)
 		/* Stop I2C transaction */
 		dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
 		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+		if (!(temp & I2CR_MSTA))
+			i2c_imx->stopped = 1;
 		temp &= ~(I2CR_MSTA | I2CR_MTX);
 		if (i2c_imx->dma)
 			temp &= ~I2CR_DMAEN;
@@ -778,9 +780,12 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 		 */
 		dev_dbg(dev, "<%s> clear MSTA\n", __func__);
 		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);
-		i2c_imx_bus_busy(i2c_imx, 0, false);
+		if (!i2c_imx->stopped)
+			i2c_imx_bus_busy(i2c_imx, 0, false);
 	} else {
 		/*
 		 * For i2c master receiver repeat restart operation like:
@@ -905,9 +910,12 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
 				dev_dbg(&i2c_imx->adapter.dev,
 					"<%s> clear MSTA\n", __func__);
 				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);
-				i2c_imx_bus_busy(i2c_imx, 0, atomic);
+				if (!i2c_imx->stopped)
+					i2c_imx_bus_busy(i2c_imx, 0, atomic);
 			} else {
 				/*
 				 * For i2c master receiver repeat restart operation like:
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/3] i2c: imx: Fix reset of I2SR_IAL flag
  2020-10-07  8:45 ` [PATCH v5 1/3] i2c: imx: Fix reset of I2SR_IAL flag Christian Eggers
@ 2020-10-08  9:56   ` Wolfram Sang
  2020-10-09  7:11   ` Oleksij Rempel
  1 sibling, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2020-10-08  9:56 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Oleksij Rempel, Shawn Guo, Sascha Hauer, Fabio Estevam,
	Uwe Kleine-König, David Laight, Pengutronix Kernel Team,
	NXP Linux Team, linux-i2c, linux-arm-kernel, linux-kernel,
	Krzysztof Kozlowski, stable

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On Wed, Oct 07, 2020 at 10:45:22AM +0200, Christian Eggers wrote:
> According to the "VFxxx Controller Reference Manual" (and the comment
> block starting at line 97), Vybrid requires writing a one for clearing
> an interrupt flag. Syncing the method for clearing I2SR_IIF in
> i2c_imx_isr().
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Fixes: 4b775022f6fd ("i2c: imx: add struct to hold more configurable quirks")
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: stable@vger.kernel.org

Applied to for-current, thanks!

Waiting for review tags for patches 2 & 3.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/3] i2c: imx: Fix reset of I2SR_IAL flag
  2020-10-07  8:45 ` [PATCH v5 1/3] i2c: imx: Fix reset of I2SR_IAL flag Christian Eggers
  2020-10-08  9:56   ` Wolfram Sang
@ 2020-10-09  7:11   ` Oleksij Rempel
  2020-10-09  7:52     ` Oleksij Rempel
  1 sibling, 1 reply; 9+ messages in thread
From: Oleksij Rempel @ 2020-10-09  7:11 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Oleksij Rempel, Shawn Guo, Sascha Hauer, Fabio Estevam,
	Uwe Kleine-König, David Laight, linux-kernel,
	Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team,
	stable, linux-arm-kernel, linux-i2c

Hi Christian,

On Wed, Oct 07, 2020 at 10:45:22AM +0200, Christian Eggers wrote:
> According to the "VFxxx Controller Reference Manual" (and the comment
> block starting at line 97), Vybrid requires writing a one for clearing
> an interrupt flag. Syncing the method for clearing I2SR_IIF in
> i2c_imx_isr().
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Fixes: 4b775022f6fd ("i2c: imx: add struct to hold more configurable quirks")
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: stable@vger.kernel.org
> ---
>  drivers/i2c/busses/i2c-imx.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 0ab5381aa012..cbdcab73a055 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -412,6 +412,19 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
>  	dma->chan_using = NULL;
>  }
>  
> +static void i2c_imx_clear_irq(struct imx_i2c_struct *i2c_imx, unsigned int bits)
> +{
> +	unsigned int temp;
> +
> +	/*
> +	 * i2sr_clr_opcode is the value to clear all interrupts. Here we want to
> +	 * clear only <bits>, so we write ~i2sr_clr_opcode with just <bits>
> +	 * toggled. This is required because i.MX needs W1C and Vybrid uses W0C.
> +	 */

This comment need correction. The i.MX needs W0C and Vybrid uses W1C 

> +	temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ bits;
> +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> +}
> +
>  static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool atomic)
>  {
>  	unsigned long orig_jiffies = jiffies;
> @@ -424,8 +437,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
>  
>  		/* check for arbitration lost */
>  		if (temp & I2SR_IAL) {
> -			temp &= ~I2SR_IAL;
> -			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> +			i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
>  			return -EAGAIN;
>  		}
>  
> @@ -623,9 +635,7 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  	if (temp & I2SR_IIF) {
>  		/* save status register */
>  		i2c_imx->i2csr = temp;
> -		temp &= ~I2SR_IIF;
> -		temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
> -		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> +		i2c_imx_clear_irq(i2c_imx, I2SR_IIF);
>  		wake_up(&i2c_imx->queue);
>  		return IRQ_HANDLED;
>  	}
> -- 

Otherwise

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> 

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/3] i2c: imx: Fix reset of I2SR_IAL flag
  2020-10-09  7:11   ` Oleksij Rempel
@ 2020-10-09  7:52     ` Oleksij Rempel
  0 siblings, 0 replies; 9+ messages in thread
From: Oleksij Rempel @ 2020-10-09  7:52 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Oleksij Rempel, Shawn Guo, Sascha Hauer, Fabio Estevam,
	Uwe Kleine-König, David Laight, linux-kernel,
	Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team,
	stable, linux-arm-kernel, linux-i2c

On Fri, Oct 09, 2020 at 09:11:32AM +0200, Oleksij Rempel wrote:
> Hi Christian,
> 
> On Wed, Oct 07, 2020 at 10:45:22AM +0200, Christian Eggers wrote:
> > According to the "VFxxx Controller Reference Manual" (and the comment
> > block starting at line 97), Vybrid requires writing a one for clearing
> > an interrupt flag. Syncing the method for clearing I2SR_IIF in
> > i2c_imx_isr().
> > 
> > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > Fixes: 4b775022f6fd ("i2c: imx: add struct to hold more configurable quirks")
> > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index 0ab5381aa012..cbdcab73a055 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -412,6 +412,19 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
> >  	dma->chan_using = NULL;
> >  }
> >  
> > +static void i2c_imx_clear_irq(struct imx_i2c_struct *i2c_imx, unsigned int bits)
> > +{
> > +	unsigned int temp;
> > +
> > +	/*
> > +	 * i2sr_clr_opcode is the value to clear all interrupts. Here we want to
> > +	 * clear only <bits>, so we write ~i2sr_clr_opcode with just <bits>
> > +	 * toggled. This is required because i.MX needs W1C and Vybrid uses W0C.
> > +	 */
> 
> This comment need correction. The i.MX needs W0C and Vybrid uses W1C 
> 
> > +	temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ bits;
> > +	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> > +}
> > +
> >  static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool atomic)
> >  {
> >  	unsigned long orig_jiffies = jiffies;
> > @@ -424,8 +437,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
> >  
> >  		/* check for arbitration lost */
> >  		if (temp & I2SR_IAL) {
> > -			temp &= ~I2SR_IAL;
> > -			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> > +			i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
> >  			return -EAGAIN;
> >  		}
> >  
> > @@ -623,9 +635,7 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> >  	if (temp & I2SR_IIF) {
> >  		/* save status register */
> >  		i2c_imx->i2csr = temp;
> > -		temp &= ~I2SR_IIF;
> > -		temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
> > -		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> > +		i2c_imx_clear_irq(i2c_imx, I2SR_IIF);
> >  		wake_up(&i2c_imx->queue);
> >  		return IRQ_HANDLED;
> >  	}
> > -- 
> 
> Otherwise
> 
> Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> 

By reviewing the second patch we found at least one more missing Vybrid
related case in the i2c_imx_trx_complete() function:
	imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);

	this is OK for iMX by broken on Vybrid.

Since you already fixing it in this patch for some of IMX_I2C_I2SR
writes, please do it here as well. This is the last unhandled case...:)

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 2/3] i2c: imx: Check for I2SR_IAL after every byte
  2020-10-07  8:45 ` [PATCH v5 2/3] i2c: imx: Check for I2SR_IAL after every byte Christian Eggers
@ 2020-10-09  7:53   ` Oleksij Rempel
  0 siblings, 0 replies; 9+ messages in thread
From: Oleksij Rempel @ 2020-10-09  7:53 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Oleksij Rempel, Shawn Guo, Sascha Hauer, Fabio Estevam,
	Uwe Kleine-König, David Laight, linux-kernel,
	Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team,
	stable, linux-arm-kernel, linux-i2c

On Wed, Oct 07, 2020 at 10:45:23AM +0200, Christian Eggers wrote:
> Arbitration Lost (IAL) can happen after every single byte transfer. If
> arbitration is lost, the I2C hardware will autonomously switch from
> master mode to slave. If a transfer is not aborted in this state,
> consecutive transfers will not be executed by the hardware and will
> timeout.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Tested (not extensively) on Vybrid VF500 (Toradex VF50):
> Tested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: stable@vger.kernel.org

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!

> ---
>  drivers/i2c/busses/i2c-imx.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index cbdcab73a055..63575af41c09 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -490,6 +490,16 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool atomic)
>  		dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
>  		return -ETIMEDOUT;
>  	}
> +
> +	/* check for arbitration lost */
> +	if (i2c_imx->i2csr & I2SR_IAL) {
> +		dev_dbg(&i2c_imx->adapter.dev, "<%s> Arbitration lost\n", __func__);
> +		i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
> +
> +		i2c_imx->i2csr = 0;
> +		return -EAGAIN;
> +	}
> +
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n", __func__);
>  	i2c_imx->i2csr = 0;
>  	return 0;
> -- 
> Christian Eggers
> Embedded software developer
> 
> Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
> Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
> Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
> Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
> Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 3/3] i2c: imx: Don't generate STOP condition if arbitration has been lost
  2020-10-07  8:45 ` [PATCH v5 3/3] i2c: imx: Don't generate STOP condition if arbitration has been lost Christian Eggers
@ 2020-10-09  8:14   ` Oleksij Rempel
  0 siblings, 0 replies; 9+ messages in thread
From: Oleksij Rempel @ 2020-10-09  8:14 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Oleksij Rempel, Shawn Guo, Sascha Hauer, Fabio Estevam,
	Uwe Kleine-König, David Laight, linux-kernel,
	Krzysztof Kozlowski, NXP Linux Team, Pengutronix Kernel Team,
	stable, linux-arm-kernel, linux-i2c

On Wed, Oct 07, 2020 at 10:45:24AM +0200, Christian Eggers wrote:
> If arbitration is lost, the master automatically changes to slave mode.
> I2SR_IBB may or may not be reset by hardware. Raising a STOP condition
> by resetting I2CR_MSTA has no effect and will not clear I2SR_IBB.
> 
> So calling i2c_imx_bus_busy() is not required and would busy-wait until
> timeout.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Tested (not extensively) on Vybrid VF500 (Toradex VF50):
> Tested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: stable@vger.kernel.org # Requires trivial backporting, simple remove
>                            # the 3rd argument from the calls to
>                            # i2c_imx_bus_busy().

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Thank you!


> ---
>  drivers/i2c/busses/i2c-imx.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 63575af41c09..5d8a79319b2b 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -615,6 +615,8 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx, bool atomic)
>  		/* Stop I2C transaction */
>  		dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>  		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +		if (!(temp & I2CR_MSTA))
> +			i2c_imx->stopped = 1;
>  		temp &= ~(I2CR_MSTA | I2CR_MTX);
>  		if (i2c_imx->dma)
>  			temp &= ~I2CR_DMAEN;
> @@ -778,9 +780,12 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
>  		 */
>  		dev_dbg(dev, "<%s> clear MSTA\n", __func__);
>  		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);
> -		i2c_imx_bus_busy(i2c_imx, 0, false);
> +		if (!i2c_imx->stopped)
> +			i2c_imx_bus_busy(i2c_imx, 0, false);
>  	} else {
>  		/*
>  		 * For i2c master receiver repeat restart operation like:
> @@ -905,9 +910,12 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>  				dev_dbg(&i2c_imx->adapter.dev,
>  					"<%s> clear MSTA\n", __func__);
>  				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);
> -				i2c_imx_bus_busy(i2c_imx, 0, atomic);
> +				if (!i2c_imx->stopped)
> +					i2c_imx_bus_busy(i2c_imx, 0, atomic);
>  			} else {
>  				/*
>  				 * For i2c master receiver repeat restart operation like:
> -- 
> Christian Eggers
> Embedded software developer
> 
> Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
> Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
> Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
> Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
> Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-10-09  8:14 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07  8:45 [PATCH v5 0/3] i2c: imx: Fix handling of arbitration loss Christian Eggers
2020-10-07  8:45 ` [PATCH v5 1/3] i2c: imx: Fix reset of I2SR_IAL flag Christian Eggers
2020-10-08  9:56   ` Wolfram Sang
2020-10-09  7:11   ` Oleksij Rempel
2020-10-09  7:52     ` Oleksij Rempel
2020-10-07  8:45 ` [PATCH v5 2/3] i2c: imx: Check for I2SR_IAL after every byte Christian Eggers
2020-10-09  7:53   ` Oleksij Rempel
2020-10-07  8:45 ` [PATCH v5 3/3] i2c: imx: Don't generate STOP condition if arbitration has been lost Christian Eggers
2020-10-09  8:14   ` Oleksij Rempel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).