[PATCH 0/3] i2c: imx: Fix handling of arbitration loss

[PATCH 0/3] i2c: imx: Fix handling of arbitration loss

[Christian Eggers]

On my (noisy) system, I2C arbitration losses happen quite often. In it's
current implementation, the IAL flag is partly handled, but has a
number of shortcomings:

1. The driver runs unnecessarily in a timeout when waiting for an
interrupt.

2. The driver performs 500 ms busy-waiting without any value.

3. Arbitration loss errors may be reported one transfer later than they
occured.

Best regards
Christian

[PATCH 0/3] i2c: imx: Fix handling of arbitration loss

[Christian Eggers]

On my (noisy) system, I2C arbitration losses happen quite often. In it's
current implementation, the IAL flag is partly handled, but has a
number of shortcomings:

1. The driver runs unnecessarily in a timeout when waiting for an
interrupt.

2. The driver performs 500 ms busy-waiting without any value.

3. Arbitration loss errors may be reported one transfer later than they
occured.

Best regards
Christian



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Christian Eggers]

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 with 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 0ab5381aa012..d8b2e632dd10 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -425,6 +425,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;
+			temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
 			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
 			return -EAGAIN;
 		}
-- 
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

[PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Christian Eggers]

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 with 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 0ab5381aa012..d8b2e632dd10 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -425,6 +425,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;
+			temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
 			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
 			return -EAGAIN;
 		}
-- 
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

Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Uwe Kleine-König]

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

Hello,

On Thu, Sep 17, 2020 at 02:20:27PM +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 with 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 0ab5381aa012..d8b2e632dd10 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -425,6 +425,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;
> +			temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
>  			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
>  			return -EAGAIN;

This looks strange. First the flag is cleared and then it is (in some
cases) set again.

If I2SR_IIF is set in temp you ack this irq without handling it. (Which
might happen if atomic is set and irqs are off?!)

I see this idiom is used in a few more places in the driver already, I
didn't check but these might have the same problem maybe?

Best regards
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 --]

Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1549 bytes --]

Hello,

On Thu, Sep 17, 2020 at 02:20:27PM +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 with 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 0ab5381aa012..d8b2e632dd10 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -425,6 +425,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;
> +			temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
>  			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
>  			return -EAGAIN;

This looks strange. First the flag is cleared and then it is (in some
cases) set again.

If I2SR_IIF is set in temp you ack this irq without handling it. (Which
might happen if atomic is set and irqs are off?!)

I see this idiom is used in a few more places in the driver already, I
didn't check but these might have the same problem maybe?

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Christian Eggers]

Hello Uwe,

On Thursday, 17 September 2020, 16:02:35 CEST, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Sep 17, 2020 at 02:20:27PM +0200, Christian Eggers wrote:
> ...
> >  		/* check for arbitration lost */
> >  		if (temp & I2SR_IAL) {
> >  			temp &= ~I2SR_IAL;
> > +			temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
> >  			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> >  			return -EAGAIN;
> ...

> This looks strange. First the flag is cleared and then it is (in some
> cases) set again.
i.MX controllers require writing a 0 to clear these bits. Vybrid controllers
need writing a 1 for the same.

> If I2SR_IIF is set in temp you ack this irq without handling it. (Which
> might happen if atomic is set and irqs are off?!)
This patch is only about using the correct processor specific value for 
acknowledging an IRQ... But I think that returning EAGAIN (which aborts the
transfer) should be handling enough. At the next transfer, the controller will
be set back to master mode.

> I see this idiom is used in a few more places in the driver already, I
> didn't check but these might have the same problem maybe?

Best regards
Christian

Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Christian Eggers]

Hello Uwe,

On Thursday, 17 September 2020, 16:02:35 CEST, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Sep 17, 2020 at 02:20:27PM +0200, Christian Eggers wrote:
> ...
> >  		/* check for arbitration lost */
> >  		if (temp & I2SR_IAL) {
> >  			temp &= ~I2SR_IAL;
> > +			temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
> >  			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> >  			return -EAGAIN;
> ...

> This looks strange. First the flag is cleared and then it is (in some
> cases) set again.
i.MX controllers require writing a 0 to clear these bits. Vybrid controllers
need writing a 1 for the same.

> If I2SR_IIF is set in temp you ack this irq without handling it. (Which
> might happen if atomic is set and irqs are off?!)
This patch is only about using the correct processor specific value for 
acknowledging an IRQ... But I think that returning EAGAIN (which aborts the
transfer) should be handling enough. At the next transfer, the controller will
be set back to master mode.

> I see this idiom is used in a few more places in the driver already, I
> didn't check but these might have the same problem maybe?

Best regards
Christian




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Uwe Kleine-König]

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

On Thu, Sep 17, 2020 at 04:13:50PM +0200, Christian Eggers wrote:
> Hello Uwe,
> 
> On Thursday, 17 September 2020, 16:02:35 CEST, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Thu, Sep 17, 2020 at 02:20:27PM +0200, Christian Eggers wrote:
> > ...
> > >             /* check for arbitration lost */
> > >             if (temp & I2SR_IAL) {
> > >                     temp &= ~I2SR_IAL;
> > > +                   temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
> > >                     imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> > >                     return -EAGAIN;
> > ...
> 
> > This looks strange. First the flag is cleared and then it is (in some
> > cases) set again.
> i.MX controllers require writing a 0 to clear these bits. Vybrid controllers
> need writing a 1 for the same.

Yes, I understood that.

> > If I2SR_IIF is set in temp you ack this irq without handling it. (Which
> > might happen if atomic is set and irqs are off?!)
> This patch is only about using the correct processor specific value for
> acknowledging an IRQ... But I think that returning EAGAIN (which aborts the
> transfer) should be handling enough. At the next transfer, the controller will
> be set back to master mode.

Either you didn't understand what I meant, or I don't understand why you
consider your patch right anyhow. So I try to explain in other and more
words.

IMHO the intention here (and also what happens on i.MX) is that exactly
the AL interrupt pending bit should be cleared and the IF irq is
supposed to be untouched.

Given there are only two irq flags in the I2SR register (which is called
IBSR on Vybrid) the status quo (i.e. without your patch) is:

  On i.MX IAL is cleared
  On Vybrid IIF (which is called IBIF there) is cleared.

With your patch we get:

  On i.MX IAL is cleared
  On Vybrid both IIF (aka IBIF) and IAL (aka IBAL) are cleared.

To get it right for both SoC types you have to do (e.g.):

	temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ I2SR_IAL;

(and in i2c_imx_isr() the same using I2SR_IIF instead of I2SR_IAL
because there currently IAL might be cleared by mistake on Vybrid).

I considered creating a patch, but as I don't have a Vybrid on my desk
and on i.MX there is no change, I let you do this.

Best regards
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 --]

Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2480 bytes --]

On Thu, Sep 17, 2020 at 04:13:50PM +0200, Christian Eggers wrote:
> Hello Uwe,
> 
> On Thursday, 17 September 2020, 16:02:35 CEST, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Thu, Sep 17, 2020 at 02:20:27PM +0200, Christian Eggers wrote:
> > ...
> > >             /* check for arbitration lost */
> > >             if (temp & I2SR_IAL) {
> > >                     temp &= ~I2SR_IAL;
> > > +                   temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
> > >                     imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> > >                     return -EAGAIN;
> > ...
> 
> > This looks strange. First the flag is cleared and then it is (in some
> > cases) set again.
> i.MX controllers require writing a 0 to clear these bits. Vybrid controllers
> need writing a 1 for the same.

Yes, I understood that.

> > If I2SR_IIF is set in temp you ack this irq without handling it. (Which
> > might happen if atomic is set and irqs are off?!)
> This patch is only about using the correct processor specific value for
> acknowledging an IRQ... But I think that returning EAGAIN (which aborts the
> transfer) should be handling enough. At the next transfer, the controller will
> be set back to master mode.

Either you didn't understand what I meant, or I don't understand why you
consider your patch right anyhow. So I try to explain in other and more
words.

IMHO the intention here (and also what happens on i.MX) is that exactly
the AL interrupt pending bit should be cleared and the IF irq is
supposed to be untouched.

Given there are only two irq flags in the I2SR register (which is called
IBSR on Vybrid) the status quo (i.e. without your patch) is:

  On i.MX IAL is cleared
  On Vybrid IIF (which is called IBIF there) is cleared.

With your patch we get:

  On i.MX IAL is cleared
  On Vybrid both IIF (aka IBIF) and IAL (aka IBAL) are cleared.

To get it right for both SoC types you have to do (e.g.):

	temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ I2SR_IAL;

(and in i2c_imx_isr() the same using I2SR_IIF instead of I2SR_IAL
because there currently IAL might be cleared by mistake on Vybrid).

I considered creating a patch, but as I don't have a Vybrid on my desk
and on i.MX there is no change, I let you do this.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Christian Eggers]

Hi Uwe,

On Friday, 25 September 2020, 10:11:01 CEST, Uwe Kleine-König wrote:
> On Thu, Sep 17, 2020 at 04:13:50PM +0200, Christian Eggers wrote:
> > On Thursday, 17 September 2020, 16:02:35 CEST, Uwe Kleine-König wrote:
> > > On Thu, Sep 17, 2020 at 02:20:27PM +0200, Christian Eggers wrote:
> > > ...
> > > 
> > > >             /* check for arbitration lost */
> > > >             if (temp & I2SR_IAL) {
> > > >             
> > > >                     temp &= ~I2SR_IAL;
> > > > 
> > > > +                   temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
> > > > 
> > > >                     imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> > > >                     return -EAGAIN;
> > > 
> > > ...
> > > 
> > > This looks strange. First the flag is cleared and then it is (in some
> > > cases) set again.
> > 
> > i.MX controllers require writing a 0 to clear these bits. Vybrid
> > controllers need writing a 1 for the same.
> 
> Yes, I understood that.
> 
> > > If I2SR_IIF is set in temp you ack this irq without handling it. (Which
> > > might happen if atomic is set and irqs are off?!)
> > 
> > This patch is only about using the correct processor specific value for
> > acknowledging an IRQ... But I think that returning EAGAIN (which aborts
> > the
> > transfer) should be handling enough. At the next transfer, the controller
> > will be set back to master mode.
> 
> Either you didn't understand what I meant, or I don't understand why you
> consider your patch right anyhow. 
Probably both.

> So I try to explain in other and more words.
> 
> IMHO the intention here (and also what happens on i.MX) is that exactly
> the AL interrupt pending bit should be cleared and the IF irq is
> supposed to be untouched.
Yes.

> Given there are only two irq flags in the I2SR register (which is called
> IBSR on Vybrid) ...

Vybrid:
=======
+-------+-----+------+-----+------+-----+-----+------+------+
| BIT   |  7  |  6   |  5  |  4   |  3  |  2  |  1   |  0   |
+-------+-----+------+-----+------+-----+-----+------+------+
| READ  | TCF | IAAS | IBB | IBAL |  0  | SRW | IBIF | RXAK |
+-------+-----+------+-----+------+-----+-----+------+------+
| WRITE |  -  |  -   |  -  | W1C  |  -  |  -  | W1C  |  -   |
+-------+-----+------+-----+-^^^--+-----+-----+-^^^--+------+

i.MX:
=======
+-------+-----+------+-----+------+-----+-----+------+------+
| BIT   |  7  |  6   |  5  |  4   |  3  |  2  |  1   |  0   |
+-------+-----+------+-----+------+-----+-----+------+------+
| READ  | ICF | IAAS | IBB | IAL  |  0  | SRW | IIF  | RXAK |
+-------+-----+------+-----+------+-----+-----+------+------+
| WRITE |  -  |  -   |  -  | W0C  |  -  |  -  | W0C  |  -   |
+-------+-----+------+-----+-^^^--+-----+-----+-^^^--+------+


> ... the status quo (i.e. without your patch) is:
> 
>   On i.MX IAL is cleared
Yes

>   On Vybrid IIF (which is called IBIF there) is cleared.
If IBIF is set, then it's cleared (probably by accident).
But in the "if (temp & I2SR_IAL)" condition, I focus on 
the IBAL flag, not IBIF.

> With your patch we get:
> 
>   On i.MX IAL is cleared
Yes

>   On Vybrid both IIF (aka IBIF) and IAL (aka IBAL) are cleared.
Agree. IBAL is cleared by intention, IBIF by accident (if set).
Do you see any problem if IBIF is also cleared?


> To get it right for both SoC types you have to do (e.g.):
> 
> 	temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ I2SR_IAL;
Sorry, even if this is correct, it looks hard to understand for me.

> (and in i2c_imx_isr() the same using I2SR_IIF instead of I2SR_IAL
> because there currently IAL might be cleared by mistake on Vybrid).
> 
> I considered creating a patch, but as I don't have a Vybrid on my desk
> and on i.MX there is no change, I let you do this.
I also don't own a Vybrid system. I consider my patch correct in terms of
clearing the IBAL flag (which was wrong before). Additional work may be
useful for NOT clearing the other status flag. I also would like to keep
this task for somebody who owns a Vybrid system. But the other patches
in this series fixes some more important problems, so maybe you could
give your acknowledge anyhow.

Best regards
Christian

Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Christian Eggers]

Hi Uwe,

On Friday, 25 September 2020, 10:11:01 CEST, Uwe Kleine-König wrote:
> On Thu, Sep 17, 2020 at 04:13:50PM +0200, Christian Eggers wrote:
> > On Thursday, 17 September 2020, 16:02:35 CEST, Uwe Kleine-König wrote:
> > > On Thu, Sep 17, 2020 at 02:20:27PM +0200, Christian Eggers wrote:
> > > ...
> > > 
> > > >             /* check for arbitration lost */
> > > >             if (temp & I2SR_IAL) {
> > > >             
> > > >                     temp &= ~I2SR_IAL;
> > > > 
> > > > +                   temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
> > > > 
> > > >                     imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> > > >                     return -EAGAIN;
> > > 
> > > ...
> > > 
> > > This looks strange. First the flag is cleared and then it is (in some
> > > cases) set again.
> > 
> > i.MX controllers require writing a 0 to clear these bits. Vybrid
> > controllers need writing a 1 for the same.
> 
> Yes, I understood that.
> 
> > > If I2SR_IIF is set in temp you ack this irq without handling it. (Which
> > > might happen if atomic is set and irqs are off?!)
> > 
> > This patch is only about using the correct processor specific value for
> > acknowledging an IRQ... But I think that returning EAGAIN (which aborts
> > the
> > transfer) should be handling enough. At the next transfer, the controller
> > will be set back to master mode.
> 
> Either you didn't understand what I meant, or I don't understand why you
> consider your patch right anyhow. 
Probably both.

> So I try to explain in other and more words.
> 
> IMHO the intention here (and also what happens on i.MX) is that exactly
> the AL interrupt pending bit should be cleared and the IF irq is
> supposed to be untouched.
Yes.

> Given there are only two irq flags in the I2SR register (which is called
> IBSR on Vybrid) ...

Vybrid:
=======
+-------+-----+------+-----+------+-----+-----+------+------+
| BIT   |  7  |  6   |  5  |  4   |  3  |  2  |  1   |  0   |
+-------+-----+------+-----+------+-----+-----+------+------+
| READ  | TCF | IAAS | IBB | IBAL |  0  | SRW | IBIF | RXAK |
+-------+-----+------+-----+------+-----+-----+------+------+
| WRITE |  -  |  -   |  -  | W1C  |  -  |  -  | W1C  |  -   |
+-------+-----+------+-----+-^^^--+-----+-----+-^^^--+------+

i.MX:
=======
+-------+-----+------+-----+------+-----+-----+------+------+
| BIT   |  7  |  6   |  5  |  4   |  3  |  2  |  1   |  0   |
+-------+-----+------+-----+------+-----+-----+------+------+
| READ  | ICF | IAAS | IBB | IAL  |  0  | SRW | IIF  | RXAK |
+-------+-----+------+-----+------+-----+-----+------+------+
| WRITE |  -  |  -   |  -  | W0C  |  -  |  -  | W0C  |  -   |
+-------+-----+------+-----+-^^^--+-----+-----+-^^^--+------+


> ... the status quo (i.e. without your patch) is:
> 
>   On i.MX IAL is cleared
Yes

>   On Vybrid IIF (which is called IBIF there) is cleared.
If IBIF is set, then it's cleared (probably by accident).
But in the "if (temp & I2SR_IAL)" condition, I focus on 
the IBAL flag, not IBIF.

> With your patch we get:
> 
>   On i.MX IAL is cleared
Yes

>   On Vybrid both IIF (aka IBIF) and IAL (aka IBAL) are cleared.
Agree. IBAL is cleared by intention, IBIF by accident (if set).
Do you see any problem if IBIF is also cleared?


> To get it right for both SoC types you have to do (e.g.):
> 
> 	temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ I2SR_IAL;
Sorry, even if this is correct, it looks hard to understand for me.

> (and in i2c_imx_isr() the same using I2SR_IIF instead of I2SR_IAL
> because there currently IAL might be cleared by mistake on Vybrid).
> 
> I considered creating a patch, but as I don't have a Vybrid on my desk
> and on i.MX there is no change, I let you do this.
I also don't own a Vybrid system. I consider my patch correct in terms of
clearing the IBAL flag (which was wrong before). Additional work may be
useful for NOT clearing the other status flag. I also would like to keep
this task for somebody who owns a Vybrid system. But the other patches
in this series fixes some more important problems, so maybe you could
give your acknowledge anyhow.

Best regards
Christian





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Uwe Kleine-König]

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

Hello Christian,

On Fri, Oct 02, 2020 at 10:01:30AM +0200, Christian Eggers wrote:
> On Friday, 25 September 2020, 10:11:01 CEST, Uwe Kleine-König wrote:
> > On Thu, Sep 17, 2020 at 04:13:50PM +0200, Christian Eggers wrote:
> > IMHO the intention here (and also what happens on i.MX) is that exactly
> > the AL interrupt pending bit should be cleared and the IF irq is
> > supposed to be untouched.
> 
> Yes.
> 
> > Given there are only two irq flags in the I2SR register (which is called
> > IBSR on Vybrid) ...
> 
> Vybrid:
> =======
> +-------+-----+------+-----+------+-----+-----+------+------+
> | BIT   |  7  |  6   |  5  |  4   |  3  |  2  |  1   |  0   |
> +-------+-----+------+-----+------+-----+-----+------+------+
> | READ  | TCF | IAAS | IBB | IBAL |  0  | SRW | IBIF | RXAK |
> +-------+-----+------+-----+------+-----+-----+------+------+
> | WRITE |  -  |  -   |  -  | W1C  |  -  |  -  | W1C  |  -   |
> +-------+-----+------+-----+-^^^--+-----+-----+-^^^--+------+
> 
> i.MX:
> =======
> +-------+-----+------+-----+------+-----+-----+------+------+
> | BIT   |  7  |  6   |  5  |  4   |  3  |  2  |  1   |  0   |
> +-------+-----+------+-----+------+-----+-----+------+------+
> | READ  | ICF | IAAS | IBB | IAL  |  0  | SRW | IIF  | RXAK |
> +-------+-----+------+-----+------+-----+-----+------+------+
> | WRITE |  -  |  -   |  -  | W0C  |  -  |  -  | W0C  |  -   |
> +-------+-----+------+-----+-^^^--+-----+-----+-^^^--+------+
> 
> 
> > ... the status quo (i.e. without your patch) is:
> >
> >   On i.MX IAL is cleared
> 
> Yes
> 
> >   On Vybrid IIF (which is called IBIF there) is cleared.
> If IBIF is set, then it's cleared (probably by accident).
> But in the "if (temp & I2SR_IAL)" condition, I focus on
> the IBAL flag, not IBIF.
> 
> > With your patch we get:
> >
> >   On i.MX IAL is cleared
> 
> Yes
> 
> >   On Vybrid both IIF (aka IBIF) and IAL (aka IBAL) are cleared.
> Agree. IBAL is cleared by intention, IBIF by accident (if set).
> Do you see any problem if IBIF is also cleared?

Yes. If there is a real problem I'm not sure, but it's enough of an
issue that there are possible side effects on Vybrid. I refuse to think
about real problems given that it is so easy to make it right.

> > To get it right for both SoC types you have to do (e.g.):
> >
> >       temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ I2SR_IAL;
> Sorry, even if this is correct, it looks hard to understand for me.

Maybe a comment would be appropriate, something like:

	/*
	 * i2sr_clr_opcode is the value to clear all interrupts. Here we
	 * want to clear no irq but I2SR_IAL, so we write
	 * ~i2sr_clr_opcode with just the I2SR_IAL bit toggled.
	 */

Maybe put this comment together with the code in a new function to have
it only once.

> > (and in i2c_imx_isr() the same using I2SR_IIF instead of I2SR_IAL
> > because there currently IAL might be cleared by mistake on Vybrid).
> >
> > I considered creating a patch, but as I don't have a Vybrid on my desk
> > and on i.MX there is no change, I let you do this.
> I also don't own a Vybrid system. I consider my patch correct in terms of
> clearing the IBAL flag (which was wrong before). Additional work may be
> useful for NOT clearing the other status flag. I also would like to keep
> this task for somebody who owns a Vybrid system. But the other patches
> in this series fixes some more important problems, so maybe you could
> give your acknowledge anyhow.

No, please don't replace one bug found by another (now) known bug. Still
more given that the newly introduced bug is much harder to trigger and
debug.

Best regards
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 --]

Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3856 bytes --]

Hello Christian,

On Fri, Oct 02, 2020 at 10:01:30AM +0200, Christian Eggers wrote:
> On Friday, 25 September 2020, 10:11:01 CEST, Uwe Kleine-König wrote:
> > On Thu, Sep 17, 2020 at 04:13:50PM +0200, Christian Eggers wrote:
> > IMHO the intention here (and also what happens on i.MX) is that exactly
> > the AL interrupt pending bit should be cleared and the IF irq is
> > supposed to be untouched.
> 
> Yes.
> 
> > Given there are only two irq flags in the I2SR register (which is called
> > IBSR on Vybrid) ...
> 
> Vybrid:
> =======
> +-------+-----+------+-----+------+-----+-----+------+------+
> | BIT   |  7  |  6   |  5  |  4   |  3  |  2  |  1   |  0   |
> +-------+-----+------+-----+------+-----+-----+------+------+
> | READ  | TCF | IAAS | IBB | IBAL |  0  | SRW | IBIF | RXAK |
> +-------+-----+------+-----+------+-----+-----+------+------+
> | WRITE |  -  |  -   |  -  | W1C  |  -  |  -  | W1C  |  -   |
> +-------+-----+------+-----+-^^^--+-----+-----+-^^^--+------+
> 
> i.MX:
> =======
> +-------+-----+------+-----+------+-----+-----+------+------+
> | BIT   |  7  |  6   |  5  |  4   |  3  |  2  |  1   |  0   |
> +-------+-----+------+-----+------+-----+-----+------+------+
> | READ  | ICF | IAAS | IBB | IAL  |  0  | SRW | IIF  | RXAK |
> +-------+-----+------+-----+------+-----+-----+------+------+
> | WRITE |  -  |  -   |  -  | W0C  |  -  |  -  | W0C  |  -   |
> +-------+-----+------+-----+-^^^--+-----+-----+-^^^--+------+
> 
> 
> > ... the status quo (i.e. without your patch) is:
> >
> >   On i.MX IAL is cleared
> 
> Yes
> 
> >   On Vybrid IIF (which is called IBIF there) is cleared.
> If IBIF is set, then it's cleared (probably by accident).
> But in the "if (temp & I2SR_IAL)" condition, I focus on
> the IBAL flag, not IBIF.
> 
> > With your patch we get:
> >
> >   On i.MX IAL is cleared
> 
> Yes
> 
> >   On Vybrid both IIF (aka IBIF) and IAL (aka IBAL) are cleared.
> Agree. IBAL is cleared by intention, IBIF by accident (if set).
> Do you see any problem if IBIF is also cleared?

Yes. If there is a real problem I'm not sure, but it's enough of an
issue that there are possible side effects on Vybrid. I refuse to think
about real problems given that it is so easy to make it right.

> > To get it right for both SoC types you have to do (e.g.):
> >
> >       temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ I2SR_IAL;
> Sorry, even if this is correct, it looks hard to understand for me.

Maybe a comment would be appropriate, something like:

	/*
	 * i2sr_clr_opcode is the value to clear all interrupts. Here we
	 * want to clear no irq but I2SR_IAL, so we write
	 * ~i2sr_clr_opcode with just the I2SR_IAL bit toggled.
	 */

Maybe put this comment together with the code in a new function to have
it only once.

> > (and in i2c_imx_isr() the same using I2SR_IIF instead of I2SR_IAL
> > because there currently IAL might be cleared by mistake on Vybrid).
> >
> > I considered creating a patch, but as I don't have a Vybrid on my desk
> > and on i.MX there is no change, I let you do this.
> I also don't own a Vybrid system. I consider my patch correct in terms of
> clearing the IBAL flag (which was wrong before). Additional work may be
> useful for NOT clearing the other status flag. I also would like to keep
> this task for somebody who owns a Vybrid system. But the other patches
> in this series fixes some more important problems, so maybe you could
> give your acknowledge anyhow.

No, please don't replace one bug found by another (now) known bug. Still
more given that the newly introduced bug is much harder to trigger and
debug.

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] i2c: imx: Check for I2SR_IAL after every byte

[Christian Eggers]

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index d8b2e632dd10..9d9b668ec7f2 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -479,6 +479,20 @@ 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) {
+		unsigned int temp = i2c_imx->i2csr;
+
+		dev_dbg(&i2c_imx->adapter.dev, "<%s> Arbitration lost\n", __func__);
+		temp &= ~I2SR_IAL;
+		temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
+		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
+
+		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

[PATCH 2/3] i2c: imx: Check for I2SR_IAL after every byte

[Christian Eggers]

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index d8b2e632dd10..9d9b668ec7f2 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -479,6 +479,20 @@ 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) {
+		unsigned int temp = i2c_imx->i2csr;
+
+		dev_dbg(&i2c_imx->adapter.dev, "<%s> Arbitration lost\n", __func__);
+		temp &= ~I2SR_IAL;
+		temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IAL);
+		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
+
+		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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] i2c: imx: Don't generate STOP condition if arbitration has been lost

[Christian Eggers]

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 trival 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 9d9b668ec7f2..c52fe20d74c9 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -608,6 +608,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;
@@ -773,9 +775,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:
@@ -900,9 +905,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

[PATCH 3/3] i2c: imx: Don't generate STOP condition if arbitration has been lost

[Christian Eggers]

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 trival 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 9d9b668ec7f2..c52fe20d74c9 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -608,6 +608,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;
@@ -773,9 +775,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:
@@ -900,9 +905,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

Re: [PATCH 0/3] i2c: imx: Fix handling of arbitration loss

[Oleksij Rempel]

On Thu, Sep 17, 2020 at 02:20:26PM +0200, Christian Eggers wrote:
> On my (noisy) system, I2C arbitration losses happen quite often. In it's
> current implementation, the IAL flag is partly handled, but has a
> number of shortcomings:
> 
> 1. The driver runs unnecessarily in a timeout when waiting for an
> interrupt.
> 
> 2. The driver performs 500 ms busy-waiting without any value.
> 
> 3. Arbitration loss errors may be reported one transfer later than they
> occured.
> 
> Best regards
> Christian
> 

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

Thank you!

-- 
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 |

Re: [PATCH 0/3] i2c: imx: Fix handling of arbitration loss

[Oleksij Rempel]

On Thu, Sep 17, 2020 at 02:20:26PM +0200, Christian Eggers wrote:
> On my (noisy) system, I2C arbitration losses happen quite often. In it's
> current implementation, the IAL flag is partly handled, but has a
> number of shortcomings:
> 
> 1. The driver runs unnecessarily in a timeout when waiting for an
> interrupt.
> 
> 2. The driver performs 500 ms busy-waiting without any value.
> 
> 3. Arbitration loss errors may be reported one transfer later than they
> occured.
> 
> Best regards
> Christian
> 

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

Thank you!

-- 
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 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/3] i2c: imx: Fix handling of arbitration loss

[Christian Eggers]

On my (noisy) system, I2C arbitration losses happen quite often. In it's
current implementation, the IAL flags is partly handled, but has a
number of shortcomings:

1. The driver runs unnecessarily in a timeout when waiting for an
interrupt.

2. The driver performs 500 ms busy-waiting without any value.

3. Arbitration loss errors may be reported one transfer later than they
occured.

Best regards
Christian

[PATCH 0/3] i2c: imx: Fix handling of arbitration loss

[Christian Eggers]

On my (noisy) system, I2C arbitration losses happen quite often. In it's
current implementation, the IAL flags is partly handled, but has a
number of shortcomings:

1. The driver runs unnecessarily in a timeout when waiting for an
interrupt.

2. The driver performs 500 ms busy-waiting without any value.

3. Arbitration loss errors may be reported one transfer later than they
occured.

Best regards
Christian



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel