Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4 0/3] i2c: imx: Fix handling of arbitration loss
@ 2020-10-06 16:08 Christian Eggers
  2020-10-06 16:08 ` [PATCH v4 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-06 16:08 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

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

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



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

* [PATCH v4 1/3] i2c: imx: Fix reset of I2SR_IAL flag
  2020-10-06 16:08 [PATCH v4 0/3] i2c: imx: Fix handling of arbitration loss Christian Eggers
@ 2020-10-06 16:08 ` Christian Eggers
  2020-10-06 18:46   ` Uwe Kleine-König
  2020-10-07  7:50   ` Krzysztof Kozlowski
  2020-10-06 16:08 ` [PATCH v4 2/3] i2c: imx: Check for I2SR_IAL after every byte Christian Eggers
  2020-10-06 16:08 ` [PATCH v4 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-06 16:08 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, 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>
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	[flat|nested] 9+ messages in thread

* [PATCH v4 2/3] i2c: imx: Check for I2SR_IAL after every byte
  2020-10-06 16:08 [PATCH v4 0/3] i2c: imx: Fix handling of arbitration loss Christian Eggers
  2020-10-06 16:08 ` [PATCH v4 1/3] i2c: imx: Fix reset of I2SR_IAL flag Christian Eggers
@ 2020-10-06 16:08 ` Christian Eggers
  2020-10-07  7:51   ` Krzysztof Kozlowski
  2020-10-06 16:08 ` [PATCH v4 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-06 16:08 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, 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>
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	[flat|nested] 9+ messages in thread

* [PATCH v4 3/3] i2c: imx: Don't generate STOP condition if arbitration has been lost
  2020-10-06 16:08 [PATCH v4 0/3] i2c: imx: Fix handling of arbitration loss Christian Eggers
  2020-10-06 16:08 ` [PATCH v4 1/3] i2c: imx: Fix reset of I2SR_IAL flag Christian Eggers
  2020-10-06 16:08 ` [PATCH v4 2/3] i2c: imx: Check for I2SR_IAL after every byte Christian Eggers
@ 2020-10-06 16:08 ` Christian Eggers
  2 siblings, 0 replies; 9+ messages in thread
From: Christian Eggers @ 2020-10-06 16:08 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, 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>
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	[flat|nested] 9+ messages in thread

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


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

On Tue, Oct 06, 2020 at 06:08:12PM +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>
> Cc: stable@vger.kernel.org

Fixes: 4b775022f6fd ("i2c: imx: add struct to hold more configurable quirks")
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards and thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

* Re: [PATCH v4 1/3] i2c: imx: Fix reset of I2SR_IAL flag
  2020-10-06 16:08 ` [PATCH v4 1/3] i2c: imx: Fix reset of I2SR_IAL flag Christian Eggers
  2020-10-06 18:46   ` Uwe Kleine-König
@ 2020-10-07  7:50   ` Krzysztof Kozlowski
  2020-10-07  8:17     ` Christian Eggers
  1 sibling, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-07  7:50 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,
	stable

On Tue, 6 Oct 2020 at 18:10, Christian Eggers <ceggers@arri.de> 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>
> Cc: stable@vger.kernel.org
> ---
>  drivers/i2c/busses/i2c-imx.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)

I replied to your v2 with testing, so what happened with all my tested tags?

Best regards,
Krzysztof

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

* Re: [PATCH v4 2/3] i2c: imx: Check for I2SR_IAL after every byte
  2020-10-06 16:08 ` [PATCH v4 2/3] i2c: imx: Check for I2SR_IAL after every byte Christian Eggers
@ 2020-10-07  7:51   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-07  7:51 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,
	stable

On Tue, 6 Oct 2020 at 18:11, Christian Eggers <ceggers@arri.de> 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>
> Cc: stable@vger.kernel.org

Where is the tested-by tag from me?

Best regards,
Krzysztof

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

* Re: [PATCH v4 1/3] i2c: imx: Fix reset of I2SR_IAL flag
  2020-10-07  7:50   ` Krzysztof Kozlowski
@ 2020-10-07  8:17     ` Christian Eggers
  2020-10-07  8:27       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 9+ messages in thread
From: Christian Eggers @ 2020-10-07  8:17 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  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,
	stable

On Wednesday, 7 October 2020, 09:50:23 CEST, Krzysztof Kozlowski wrote:
> I replied to your v2 with testing, so what happened with all my tested tags?

I am quite new to the kernel development process. Seems that I should 
integrate all "Tested-by" tags into following version of my patches.

In which cases shall the tested tags be kept and in which cases they become 
invalid?

Best regards
Christian




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

* Re: [PATCH v4 1/3] i2c: imx: Fix reset of I2SR_IAL flag
  2020-10-07  8:17     ` Christian Eggers
@ 2020-10-07  8:27       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 9+ messages in thread
From: Krzysztof Kozlowski @ 2020-10-07  8:27 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,
	stable

On Wed, 7 Oct 2020 at 10:17, Christian Eggers <ceggers@arri.de> wrote:
>
> On Wednesday, 7 October 2020, 09:50:23 CEST, Krzysztof Kozlowski wrote:
> > I replied to your v2 with testing, so what happened with all my tested tags?
>
> I am quite new to the kernel development process. Seems that I should
> integrate all "Tested-by" tags into following version of my patches.
>
> In which cases shall the tested tags be kept and in which cases they become
> invalid?

https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L584

Your v3 touched only one patch, so all tags for all other patches
should be added and preserved. If the patch changed significantly that
review or testing is not appropriate, you could remove someone's tag
but then you should ask for testing again. And you did not send it for
testing.

Your v4 only extended a comment which does not affect testing. All
tags, review, ack and tested by should be added/preserved.

Otherwise you ask for testing (or reviewing) and then do not credit
this person. Neither maintainers know that patches were tested.

Best regards,
Krzysztof

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

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 16:08 [PATCH v4 0/3] i2c: imx: Fix handling of arbitration loss Christian Eggers
2020-10-06 16:08 ` [PATCH v4 1/3] i2c: imx: Fix reset of I2SR_IAL flag Christian Eggers
2020-10-06 18:46   ` Uwe Kleine-König
2020-10-07  7:50   ` Krzysztof Kozlowski
2020-10-07  8:17     ` Christian Eggers
2020-10-07  8:27       ` Krzysztof Kozlowski
2020-10-06 16:08 ` [PATCH v4 2/3] i2c: imx: Check for I2SR_IAL after every byte Christian Eggers
2020-10-07  7:51   ` Krzysztof Kozlowski
2020-10-06 16:08 ` [PATCH v4 3/3] i2c: imx: Don't generate STOP condition if arbitration has been lost Christian Eggers

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git