[PATCH] i2c: exynos5: fix arbitration lost handling

[PATCH] i2c: exynos5: fix arbitration lost handling

[Andrzej Hajda]

In case of arbitration lost adequate interrupt sometimes is not signaled. As
a result transfer timeouts and is not retried, as it should. To avoid such
cases code is added to check transaction status in case of every interrupt.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
Hi,

I am not I2C specialist, this patch is a result of debugging issues
with SiI8620 MHL driver on TM2 device. I guess the main problem is
with SiI8620 chip, but exynos5 i2c is also guilty - with i2c-gpio driver
it works correctly - there is only one arbitration lost at 1st transaction
(which is properly handled - transfer is repeated succesfully). In case
of exynos5 there are much more arbitration losts. Lowering bus speed
decreases frequency of losts but do not eliminate them.
Any help/hint, what could cause such issues?

I guess increasing adap.retries should also decrease fail probability,
currently for exynos5 it is set to 3.

Regards
Andrzej
---
 drivers/i2c/busses/i2c-exynos5.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index bea6071..a01c1ae 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -130,12 +130,32 @@
 /* I2C_TRANS_STATUS register bits */
 #define HSI2C_MASTER_BUSY			(1u << 17)
 #define HSI2C_SLAVE_BUSY			(1u << 16)
+
+/* I2C_TRANS_STATUS register bits for Exynos5 variant */
 #define HSI2C_TIMEOUT_AUTO			(1u << 4)
 #define HSI2C_NO_DEV				(1u << 3)
 #define HSI2C_NO_DEV_ACK			(1u << 2)
 #define HSI2C_TRANS_ABORT			(1u << 1)
 #define HSI2C_TRANS_DONE			(1u << 0)
 
+/* I2C_TRANS_STATUS register bits for Exynos7 variant */
+#define HSI2C_MASTER_ST_MASK			0xf
+#define HSI2C_MASTER_ST_IDLE			0x0
+#define HSI2C_MASTER_ST_START			0x1
+#define HSI2C_MASTER_ST_RESTART			0x2
+#define HSI2C_MASTER_ST_STOP			0x3
+#define HSI2C_MASTER_ST_MASTER_ID		0x4
+#define HSI2C_MASTER_ST_ADDR0			0x5
+#define HSI2C_MASTER_ST_ADDR1			0x6
+#define HSI2C_MASTER_ST_ADDR2			0x7
+#define HSI2C_MASTER_ST_ADDR_SR			0x8
+#define HSI2C_MASTER_ST_READ			0x9
+#define HSI2C_MASTER_ST_WRITE			0xa
+#define HSI2C_MASTER_ST_NO_ACK			0xb
+#define HSI2C_MASTER_ST_LOSE			0xc
+#define HSI2C_MASTER_ST_WAIT			0xd
+#define HSI2C_MASTER_ST_WAIT_CMD		0xe
+
 /* I2C_ADDR register bits */
 #define HSI2C_SLV_ADDR_SLV(x)			((x & 0x3ff) << 0)
 #define HSI2C_SLV_ADDR_MAS(x)			((x & 0x3ff) << 10)
@@ -437,6 +457,7 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 
 	int_status = readl(i2c->regs + HSI2C_INT_STATUS);
 	writel(int_status, i2c->regs + HSI2C_INT_STATUS);
+	trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
 
 	/* handle interrupt related to the transfer status */
 	if (i2c->variant->hw == HSI2C_EXYNOS7) {
@@ -460,8 +481,13 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 			i2c->state = -ETIMEDOUT;
 			goto stop;
 		}
+
+		switch (trans_status & HSI2C_MASTER_ST_MASK) {
+		case HSI2C_MASTER_ST_LOSE:
+			i2c->state = -EAGAIN;
+			goto stop;
+		}
 	} else if (int_status & HSI2C_INT_I2C) {
-		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
 		if (trans_status & HSI2C_NO_DEV_ACK) {
 			dev_dbg(i2c->dev, "No ACK from device\n");
 			i2c->state = -ENXIO;
-- 
2.7.4

Re: [PATCH] i2c: exynos5: fix arbitration lost handling

[Andi Shyti]

Hi Andrzej,

On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote:
> In case of arbitration lost adequate interrupt sometimes is not signaled. As
> a result transfer timeouts and is not retried, as it should. To avoid such
> cases code is added to check transaction status in case of every interrupt.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> Hi,
> 
> I am not I2C specialist, this patch is a result of debugging issues
> with SiI8620 MHL driver on TM2 device. I guess the main problem is
> with SiI8620 chip, but exynos5 i2c is also guilty - with i2c-gpio driver
> it works correctly - there is only one arbitration lost at 1st transaction
> (which is properly handled - transfer is repeated succesfully). In case
> of exynos5 there are much more arbitration losts. Lowering bus speed
> decreases frequency of losts but do not eliminate them.
> Any help/hint, what could cause such issues?

I'm having the same issue on TM2 and your patch fixes it. I'm
not sure, though, that's the right fix.

In any case I will look into it.

Andi

Re: [PATCH] i2c: exynos5: fix arbitration lost handling

[Wolfram Sang]

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

On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote:
> In case of arbitration lost adequate interrupt sometimes is not signaled. As
> a result transfer timeouts and is not retried, as it should. To avoid such
> cases code is added to check transaction status in case of every interrupt.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Looks good to me, but I'd like some review from one of the Samsung
people?

> ---
> Hi,
> 
> I am not I2C specialist, this patch is a result of debugging issues
> with SiI8620 MHL driver on TM2 device. I guess the main problem is
> with SiI8620 chip, but exynos5 i2c is also guilty - with i2c-gpio driver
> it works correctly - there is only one arbitration lost at 1st transaction
> (which is properly handled - transfer is repeated succesfully). In case
> of exynos5 there are much more arbitration losts. Lowering bus speed
> decreases frequency of losts but do not eliminate them.
> Any help/hint, what could cause such issues?
> 
> I guess increasing adap.retries should also decrease fail probability,
> currently for exynos5 it is set to 3.
> 
> Regards
> Andrzej
> ---
>  drivers/i2c/busses/i2c-exynos5.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> index bea6071..a01c1ae 100644
> --- a/drivers/i2c/busses/i2c-exynos5.c
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -130,12 +130,32 @@
>  /* I2C_TRANS_STATUS register bits */
>  #define HSI2C_MASTER_BUSY			(1u << 17)
>  #define HSI2C_SLAVE_BUSY			(1u << 16)
> +
> +/* I2C_TRANS_STATUS register bits for Exynos5 variant */
>  #define HSI2C_TIMEOUT_AUTO			(1u << 4)
>  #define HSI2C_NO_DEV				(1u << 3)
>  #define HSI2C_NO_DEV_ACK			(1u << 2)
>  #define HSI2C_TRANS_ABORT			(1u << 1)
>  #define HSI2C_TRANS_DONE			(1u << 0)
>  
> +/* I2C_TRANS_STATUS register bits for Exynos7 variant */
> +#define HSI2C_MASTER_ST_MASK			0xf
> +#define HSI2C_MASTER_ST_IDLE			0x0
> +#define HSI2C_MASTER_ST_START			0x1
> +#define HSI2C_MASTER_ST_RESTART			0x2
> +#define HSI2C_MASTER_ST_STOP			0x3
> +#define HSI2C_MASTER_ST_MASTER_ID		0x4
> +#define HSI2C_MASTER_ST_ADDR0			0x5
> +#define HSI2C_MASTER_ST_ADDR1			0x6
> +#define HSI2C_MASTER_ST_ADDR2			0x7
> +#define HSI2C_MASTER_ST_ADDR_SR			0x8
> +#define HSI2C_MASTER_ST_READ			0x9
> +#define HSI2C_MASTER_ST_WRITE			0xa
> +#define HSI2C_MASTER_ST_NO_ACK			0xb
> +#define HSI2C_MASTER_ST_LOSE			0xc
> +#define HSI2C_MASTER_ST_WAIT			0xd
> +#define HSI2C_MASTER_ST_WAIT_CMD		0xe
> +
>  /* I2C_ADDR register bits */
>  #define HSI2C_SLV_ADDR_SLV(x)			((x & 0x3ff) << 0)
>  #define HSI2C_SLV_ADDR_MAS(x)			((x & 0x3ff) << 10)
> @@ -437,6 +457,7 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>  
>  	int_status = readl(i2c->regs + HSI2C_INT_STATUS);
>  	writel(int_status, i2c->regs + HSI2C_INT_STATUS);
> +	trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
>  
>  	/* handle interrupt related to the transfer status */
>  	if (i2c->variant->hw == HSI2C_EXYNOS7) {
> @@ -460,8 +481,13 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>  			i2c->state = -ETIMEDOUT;
>  			goto stop;
>  		}
> +
> +		switch (trans_status & HSI2C_MASTER_ST_MASK) {
> +		case HSI2C_MASTER_ST_LOSE:
> +			i2c->state = -EAGAIN;
> +			goto stop;
> +		}
>  	} else if (int_status & HSI2C_INT_I2C) {
> -		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
>  		if (trans_status & HSI2C_NO_DEV_ACK) {
>  			dev_dbg(i2c->dev, "No ACK from device\n");
>  			i2c->state = -ENXIO;
> -- 
> 2.7.4
> 

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

Re: [PATCH] i2c: exynos5: fix arbitration lost handling

[Wolfram Sang]

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

On Wed, Jan 25, 2017 at 09:55:54PM +0100, Wolfram Sang wrote:
> On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote:
> > In case of arbitration lost adequate interrupt sometimes is not signaled. As
> > a result transfer timeouts and is not retried, as it should. To avoid such
> > cases code is added to check transaction status in case of every interrupt.
> > 
> > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> Looks good to me, but I'd like some review from one of the Samsung
> people?

Ooops, you are from Samsung as well :) Still, I'd like one more ack or
review from someone with the HW.

Thanks,

   Wolfram


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

Re: [PATCH] i2c: exynos5: fix arbitration lost handling

[Andi Shyti]

Hi,

> > > In case of arbitration lost adequate interrupt sometimes is not signaled. As
> > > a result transfer timeouts and is not retried, as it should. To avoid such
> > > cases code is added to check transaction status in case of every interrupt.
> > > 
> > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> > 
> > Looks good to me, but I'd like some review from one of the Samsung
> > people?
> 
> Ooops, you are from Samsung as well :) Still, I'd like one more ack or
> review from someone with the HW.

I am using this patch already:

Reviewed-by: Andi Shyti <andi.shyti@samsung.com>
Tested-by: Andi Shyti <andi.shyti@samsung.com>

Shouldn't we cc for the stable Kernel?

Andi

Re: [PATCH] i2c: exynos5: fix arbitration lost handling

[Wolfram Sang]

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

On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote:
> In case of arbitration lost adequate interrupt sometimes is not signaled. As
> a result transfer timeouts and is not retried, as it should. To avoid such
> cases code is added to check transaction status in case of every interrupt.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---

> +
> +		switch (trans_status & HSI2C_MASTER_ST_MASK) {
> +		case HSI2C_MASTER_ST_LOSE:
> +			i2c->state = -EAGAIN;
> +			goto stop;
> +		}

Why not using if() instead of switch() as in the rest of the driver?

And there is arbitration lost checking already with int_status &
HSI2C_INT_TRANS_ABORT. Any guess why it doesn't trigger?


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

Re: [PATCH] i2c: exynos5: fix arbitration lost handling

[Andrzej Hajda]

On 09.02.2017 17:27, Wolfram Sang wrote:
> On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote:
>> In case of arbitration lost adequate interrupt sometimes is not signaled. As
>> a result transfer timeouts and is not retried, as it should. To avoid such
>> cases code is added to check transaction status in case of every interrupt.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>> +
>> +		switch (trans_status & HSI2C_MASTER_ST_MASK) {
>> +		case HSI2C_MASTER_ST_LOSE:
>> +			i2c->state = -EAGAIN;
>> +			goto stop;
>> +		}
> Why not using if() instead of switch() as in the rest of the driver?

Following reasons (not serious ones):
1. With if() the line should be split anyway to keep 80 chars per line
limit, and the result looks less readable, so no gain:
        if ((trans_status & HSI2C_MASTER_ST_MASK) ==
             HSI2C_MASTER_ST_LOSE) {
            i2c->state = -EAGAIN;
            goto stop;
        }
2. It is ready to handle other status values as well, without repeating
long if() clause, in fact during tests I have added more values, but
finally they went out.
3. In the rest of the functions if() is used because code tests if some
bits are set, here we test if some bit field have specific value,
slightly different thing.

Of course I can change to if() if you prefer it.

> And there is arbitration lost checking already with int_status &
> HSI2C_INT_TRANS_ABORT. Any guess why it doesn't trigger?
>

Nope. This patch is just a result of comparing register values during
good and bad transfer.
I have looked for similar issues over the net, but without ultimate
answer, some hints are that master can check status of SDA line too early.
Anyway it works correctly with gpio bit-banging driver, it suggests
there could be something wrong in hsi2c logic.

Regards
Andrzej

Re: [PATCH] i2c: exynos5: fix arbitration lost handling

[Andrzej Hajda]

On 10.02.2017 08:39, Andrzej Hajda wrote:
> On 09.02.2017 17:27, Wolfram Sang wrote:
>> On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote:
>>> In case of arbitration lost adequate interrupt sometimes is not signaled. As
>>> a result transfer timeouts and is not retried, as it should. To avoid such
>>> cases code is added to check transaction status in case of every interrupt.
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>> +
>>> +		switch (trans_status & HSI2C_MASTER_ST_MASK) {
>>> +		case HSI2C_MASTER_ST_LOSE:
>>> +			i2c->state = -EAGAIN;
>>> +			goto stop;
>>> +		}
>> Why not using if() instead of switch() as in the rest of the driver?
> Following reasons (not serious ones):
> 1. With if() the line should be split anyway to keep 80 chars per line
> limit, and the result looks less readable, so no gain:
>         if ((trans_status & HSI2C_MASTER_ST_MASK) ==
>              HSI2C_MASTER_ST_LOSE) {
>             i2c->state = -EAGAIN;
>             goto stop;
>         }
> 2. It is ready to handle other status values as well, without repeating
> long if() clause, in fact during tests I have added more values, but
> finally they went out.
> 3. In the rest of the functions if() is used because code tests if some
> bits are set, here we test if some bit field have specific value,
> slightly different thing.
>
> Of course I can change to if() if you prefer it.
>
>> And there is arbitration lost checking already with int_status &
>> HSI2C_INT_TRANS_ABORT. Any guess why it doesn't trigger?
>>
> Nope. This patch is just a result of comparing register values during
> good and bad transfer.
> I have looked for similar issues over the net, but without ultimate
> answer, some hints are that master can check status of SDA line too early.
> Anyway it works correctly with gpio bit-banging driver, it suggests
> there could be something wrong in hsi2c logic.

I have just found in spec of newer version of the chip configuration bit
NO_ARB_FASTSDA. Citation:
> This bit deactivates the modified logic of the
> abnormal operation when the SCL is falling. Then,
> other I2C device quickly lowers the SDA line, and
> SDA has fallen before the first PCLK period
> elapses.
> 0 = Skip the master arbitration checking during the
> first PCLK period after SCL falls
> 1 = Restore original logic

I have no hardware with newer chip to really test if this solves the
issue, but I suppose it could, the only problem is that this bit is not
present in current chip version, Exynos5433.

Regards
Andrzej


>
> Regards
> Andrzej
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH] i2c: exynos5: fix arbitration lost handling

[Wolfram Sang]

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

On Fri, Feb 10, 2017 at 08:39:58AM +0100, Andrzej Hajda wrote:
> On 09.02.2017 17:27, Wolfram Sang wrote:
> > On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote:
> >> In case of arbitration lost adequate interrupt sometimes is not signaled. As
> >> a result transfer timeouts and is not retried, as it should. To avoid such
> >> cases code is added to check transaction status in case of every interrupt.
> >>
> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >> ---
> >> +
> >> +		switch (trans_status & HSI2C_MASTER_ST_MASK) {
> >> +		case HSI2C_MASTER_ST_LOSE:
> >> +			i2c->state = -EAGAIN;
> >> +			goto stop;
> >> +		}
> > Why not using if() instead of switch() as in the rest of the driver?
> 
> Following reasons (not serious ones):

I see. It is probably a taste thing, but a switch() with just one case
in it looks like something has been forgotten :) We can change it back
to switch() if ever more states need to be added.

Given that I am not super strict with the 80 char limit anyhow and it
will miss the limit by a few chars only, I'd prefer the if statement
actually.

Other than this, I am OK with applying the patch for 4.11. Can you
resend with the above change?

Thanks,

   Wolfram


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