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

[PATCH v2] 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 Wolfram,

I have postponed this patch because I was looking for better solution,
but it seems that this patch together with 'disable fifo-almost-empty...'
patch solves the issue of arbitration lost completely.
The patch has been adjusted according to your comment.

Regards
Andrzej

v2:
- replaced switch with if clause,
- commit message fits 75 chars limit.

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

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 00e81e3..cbd93ce 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,12 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 			i2c->state = -ETIMEDOUT;
 			goto stop;
 		}
+
+		if ((trans_status & HSI2C_MASTER_ST_MASK) == 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 v2] i2c: exynos5: fix arbitration lost handling

[Andi Shyti]

Hi Andrzej,

On Wed, Feb 22, 2017 at 12:04:34PM +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>

you forgot:

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

in any case I tested it again on tm2e with next-20170223.

Thanks,
Andi

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

[Andrzej Hajda]

On 23.02.2017 07:29, Andi Shyti wrote:
> Hi Andrzej,
>
> On Wed, Feb 22, 2017 at 12:04:34PM +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>
> you forgot:
>
> Tested-by: Andi Shyti <andi.shyti@samsung.com>
> Reviewed-by: Andi Shyti <andi.shyti@samsung.com>

Oops, my fault.
>
> in any case I tested it again on tm2e with next-20170223.

Thanks again for testing.

Regards
Andrzej

>
> Thanks,
> Andi
> --
> 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 v2] i2c: exynos5: fix arbitration lost handling

[Wolfram Sang]

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

On Wed, Feb 22, 2017 at 12:04:34PM +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>

Applied to for-next (will be in this merge window), thanks!

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

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

[Javier Martinez Canillas]

Hello Andrzej,

On 02/22/2017 08:04 AM, 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>
> ---

This patch causes regressions on Exynos5 boards (at least I noticed it in
Exynos5800 Peach Pi board). I see transmission timeouts when accessing I2C
device registers, i.e:

$ cat /sys/class/rtc/rtc0/time
[   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
[   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
cat: /sys/class/rtc/rtc0/time: Invalid argument

Doing a partial revert of $SUBJECT (reading I2C_TRANS_STATUS register when
it was before) "fixes" the problem [0].

I've read the Exynos5800 and Exynos5433 SoC manuals and couldn't figure out
what's causing it. I first thought that the solution was to make I2C core
to retry when the TRANSFER_DONE_AUTO field isn't set [1] but that only makes
the issue less frequent, it still fails. I also tried to increment the retry
count [2] but it also didn't help.

Any ideas about what could be happening here?

[0]:
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index cbd93ce0661f..736a82472101 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -457,7 +457,6 @@ 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) {
@@ -482,11 +481,13 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 			goto stop;
 		}
 
+		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
 		if ((trans_status & HSI2C_MASTER_ST_MASK) == 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;

[1]:
>From 3c60d3a0356563b33b739d309a4f64c003d83053 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javier@osg.samsung.com>
Date: Mon, 6 Mar 2017 19:35:44 -0300
Subject: [PATCH] i2c: exynos5: Make transaction to retry if isn't successfully completed

After commit 7999eecb7e56 ("i2c: exynos5: fix arbitration lost handling"),
some I2C transactions are failing because the TRANSFER_DONE_AUTO field is
not set in the I2C_TRANS_STATUS register so the i2c->status value is left
to -EINVAL causing the i2c->msg_complete completion to never be signaled.

For example, when reading the time of an I2C rtc on an Exynos5800 machine:

$ cat /sys/class/rtc/rtc0/time
[   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
[   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
cat: /sys/class/rtc/rtc0/time: Invalid argument

To avoid this, set i2c->state to -EAGAIN if TRANSFER_DONE_AUTO is not set to
allow the I2C core to retry if the transaction wasn't successfully completed.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
 drivers/i2c/busses/i2c-exynos5.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index cbd93ce0661f..d294fb61520c 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -506,6 +506,10 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 		} else if (trans_status & HSI2C_TRANS_DONE) {
 			i2c->trans_done = 1;
 			i2c->state = 0;
+		} else {
+			dev_dbg(i2c->dev, "Transaction was not completed\n");
+			i2c->state = -EAGAIN;
+			goto stop;
 		}
 	}
 
-- 
2.9.3

[2]
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index d294fb61520c..2b0ee9040188 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -774,7 +774,7 @@ static int exynos5_i2c_probe(struct platform_device *pdev)
        strlcpy(i2c->adap.name, "exynos5-i2c", sizeof(i2c->adap.name));
        i2c->adap.owner   = THIS_MODULE;
        i2c->adap.algo    = &exynos5_i2c_algorithm;
-       i2c->adap.retries = 3;
+       i2c->adap.retries = 5;
 
        i2c->dev = &pdev->dev;
        i2c->clk = devm_clk_get(&pdev->dev, "hsi2c");

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

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

[Andrzej Hajda]

On 08.03.2017 21:05, Javier Martinez Canillas wrote:
> Hello Andrzej,
>
> On 02/22/2017 08:04 AM, 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>
>> ---
> This patch causes regressions on Exynos5 boards (at least I noticed it in
> Exynos5800 Peach Pi board). I see transmission timeouts when accessing I2C
> device registers, i.e:
>
> $ cat /sys/class/rtc/rtc0/time
> [   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
> [   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
> cat: /sys/class/rtc/rtc0/time: Invalid argument

Hmm, at 12e10000 Exynos5 has hsi2c_9, and on this bus I have found only
infineon,slb9645tt TPM module. At least on mainline kernel.
What kernel do you use? Any additional changes to kernel?
Do you observe it on mainline kernel?

Regarding the issue, how often it happens?
Please verify that this is not just coincidence.

>
> Doing a partial revert of $SUBJECT (reading I2C_TRANS_STATUS register when
> it was before) "fixes" the problem [0].

That look crazy, the only difference for non-Exynos7 variant (which is
in Exynos5 boards) is that with your change HSI2C_TRANS_STATUS is read
only when HSI2C_INT_I2C happens, am I right?
Anyway I have realized that in case of Exynos5 the device HSI2C driver
works with is named "Universal Serial Interface" and has slightly
different set of registers than HSI2C device present in Exynos52[56]0,
but I do not know if it matters.

If [0] really fixes the issue I think you can create real patch and send
for testing, but it looks very suspicious.

Regards
Andrzej

>
> I've read the Exynos5800 and Exynos5433 SoC manuals and couldn't figure out
> what's causing it. I first thought that the solution was to make I2C core
> to retry when the TRANSFER_DONE_AUTO field isn't set [1] but that only makes
> the issue less frequent, it still fails. I also tried to increment the retry
> count [2] but it also didn't help.
>
> Any ideas about what could be happening here?
>
> [0]:
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> index cbd93ce0661f..736a82472101 100644
> --- a/drivers/i2c/busses/i2c-exynos5.c
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -457,7 +457,6 @@ 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) {
> @@ -482,11 +481,13 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>  			goto stop;
>  		}
>  
> +		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
>  		if ((trans_status & HSI2C_MASTER_ST_MASK) == 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;
>
> [1]:
> >From 3c60d3a0356563b33b739d309a4f64c003d83053 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javier@osg.samsung.com>
> Date: Mon, 6 Mar 2017 19:35:44 -0300
> Subject: [PATCH] i2c: exynos5: Make transaction to retry if isn't successfully completed
>
> After commit 7999eecb7e56 ("i2c: exynos5: fix arbitration lost handling"),
> some I2C transactions are failing because the TRANSFER_DONE_AUTO field is
> not set in the I2C_TRANS_STATUS register so the i2c->status value is left
> to -EINVAL causing the i2c->msg_complete completion to never be signaled.
>
> For example, when reading the time of an I2C rtc on an Exynos5800 machine:
>
> $ cat /sys/class/rtc/rtc0/time
> [   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
> [   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
> cat: /sys/class/rtc/rtc0/time: Invalid argument
>
> To avoid this, set i2c->state to -EAGAIN if TRANSFER_DONE_AUTO is not set to
> allow the I2C core to retry if the transaction wasn't successfully completed.
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>  drivers/i2c/busses/i2c-exynos5.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> index cbd93ce0661f..d294fb61520c 100644
> --- a/drivers/i2c/busses/i2c-exynos5.c
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -506,6 +506,10 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>  		} else if (trans_status & HSI2C_TRANS_DONE) {
>  			i2c->trans_done = 1;
>  			i2c->state = 0;
> +		} else {
> +			dev_dbg(i2c->dev, "Transaction was not completed\n");
> +			i2c->state = -EAGAIN;
> +			goto stop;
>  		}
>  	}
>  

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

[Andrzej Hajda]

Hi again,

On 09.03.2017 08:51, Andrzej Hajda wrote:
> On 08.03.2017 21:05, Javier Martinez Canillas wrote:
>> Hello Andrzej,
>>
>> On 02/22/2017 08:04 AM, 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>
>>> ---
>> This patch causes regressions on Exynos5 boards (at least I noticed it in
>> Exynos5800 Peach Pi board). I see transmission timeouts when accessing I2C
>> device registers, i.e:
>>
>> $ cat /sys/class/rtc/rtc0/time
>> [   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
>> [   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
>> cat: /sys/class/rtc/rtc0/time: Invalid argument
> Hmm, at 12e10000 Exynos5 has hsi2c_9, and on this bus I have found only
> infineon,slb9645tt TPM module. At least on mainline kernel.
> What kernel do you use? Any additional changes to kernel?
> Do you observe it on mainline kernel?
>
> Regarding the issue, how often it happens?
> Please verify that this is not just coincidence.
>
>> Doing a partial revert of $SUBJECT (reading I2C_TRANS_STATUS register when
>> it was before) "fixes" the problem [0].
> That look crazy, the only difference for non-Exynos7 variant (which is
> in Exynos5 boards) is that with your change HSI2C_TRANS_STATUS is read
> only when HSI2C_INT_I2C happens, am I right?
> Anyway I have realized that in case of Exynos5 the device HSI2C driver
> works with is named "Universal Serial Interface" and has slightly
> different set of registers than HSI2C device present in Exynos52[56]0,
> but I do not know if it matters.
>
> If [0] really fixes the issue I think you can create real patch and send
> for testing, but it looks very suspicious.

Datasheet for Exynos5260 states clearly that TRANSFER_DONE_AUTO bit is
cleared automatically after reading TRANS_STATUS register, so reading
this register has side effect and in case of Exynos5 should be done only
in 'if (int_status & HSI2C_INT_I2C)' clause. In case of Exynos7 (ie
Exynos5433 :) ) reading TRANS_STATUS should not have side effects.
Do you want to prepare fix patch from [0]?

Regards
Andrzej

>
> Regards
> Andrzej
>
>> I've read the Exynos5800 and Exynos5433 SoC manuals and couldn't figure out
>> what's causing it. I first thought that the solution was to make I2C core
>> to retry when the TRANSFER_DONE_AUTO field isn't set [1] but that only makes
>> the issue less frequent, it still fails. I also tried to increment the retry
>> count [2] but it also didn't help.
>>
>> Any ideas about what could be happening here?
>>
>> [0]:
>> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
>> index cbd93ce0661f..736a82472101 100644
>> --- a/drivers/i2c/busses/i2c-exynos5.c
>> +++ b/drivers/i2c/busses/i2c-exynos5.c
>> @@ -457,7 +457,6 @@ 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) {
>> @@ -482,11 +481,13 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>>  			goto stop;
>>  		}
>>  
>> +		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
>>  		if ((trans_status & HSI2C_MASTER_ST_MASK) == 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;
>>
>> [1]:
>> >From 3c60d3a0356563b33b739d309a4f64c003d83053 Mon Sep 17 00:00:00 2001
>> From: Javier Martinez Canillas <javier@osg.samsung.com>
>> Date: Mon, 6 Mar 2017 19:35:44 -0300
>> Subject: [PATCH] i2c: exynos5: Make transaction to retry if isn't successfully completed
>>
>> After commit 7999eecb7e56 ("i2c: exynos5: fix arbitration lost handling"),
>> some I2C transactions are failing because the TRANSFER_DONE_AUTO field is
>> not set in the I2C_TRANS_STATUS register so the i2c->status value is left
>> to -EINVAL causing the i2c->msg_complete completion to never be signaled.
>>
>> For example, when reading the time of an I2C rtc on an Exynos5800 machine:
>>
>> $ cat /sys/class/rtc/rtc0/time
>> [   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
>> [   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
>> cat: /sys/class/rtc/rtc0/time: Invalid argument
>>
>> To avoid this, set i2c->state to -EAGAIN if TRANSFER_DONE_AUTO is not set to
>> allow the I2C core to retry if the transaction wasn't successfully completed.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>>  drivers/i2c/busses/i2c-exynos5.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
>> index cbd93ce0661f..d294fb61520c 100644
>> --- a/drivers/i2c/busses/i2c-exynos5.c
>> +++ b/drivers/i2c/busses/i2c-exynos5.c
>> @@ -506,6 +506,10 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>>  		} else if (trans_status & HSI2C_TRANS_DONE) {
>>  			i2c->trans_done = 1;
>>  			i2c->state = 0;
>> +		} else {
>> +			dev_dbg(i2c->dev, "Transaction was not completed\n");
>> +			i2c->state = -EAGAIN;
>> +			goto stop;
>>  		}
>>  	}
>>  
>

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

[Javier Martinez Canillas]

Hello Andrzej,

On 03/09/2017 08:03 AM, Andrzej Hajda wrote:
> Hi again,
> 
> On 09.03.2017 08:51, Andrzej Hajda wrote:
>> On 08.03.2017 21:05, Javier Martinez Canillas wrote:
>>> Hello Andrzej,
>>>
>>> On 02/22/2017 08:04 AM, 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>
>>>> ---
>>> This patch causes regressions on Exynos5 boards (at least I noticed it in
>>> Exynos5800 Peach Pi board). I see transmission timeouts when accessing I2C
>>> device registers, i.e:
>>>
>>> $ cat /sys/class/rtc/rtc0/time
>>> [   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
>>> [   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
>>> cat: /sys/class/rtc/rtc0/time: Invalid argument
>> Hmm, at 12e10000 Exynos5 has hsi2c_9, and on this bus I have found only
>> infineon,slb9645tt TPM module. At least on mainline kernel.
>> What kernel do you use? Any additional changes to kernel?
>> Do you observe it on mainline kernel?
>>
>> Regarding the issue, how often it happens?
>> Please verify that this is not just coincidence.
>>
>>> Doing a partial revert of $SUBJECT (reading I2C_TRANS_STATUS register when
>>> it was before) "fixes" the problem [0].
>> That look crazy, the only difference for non-Exynos7 variant (which is
>> in Exynos5 boards) is that with your change HSI2C_TRANS_STATUS is read
>> only when HSI2C_INT_I2C happens, am I right?
>> Anyway I have realized that in case of Exynos5 the device HSI2C driver
>> works with is named "Universal Serial Interface" and has slightly
>> different set of registers than HSI2C device present in Exynos52[56]0,
>> but I do not know if it matters.
>>
>> If [0] really fixes the issue I think you can create real patch and send
>> for testing, but it looks very suspicious.
> 
> Datasheet for Exynos5260 states clearly that TRANSFER_DONE_AUTO bit is
> cleared automatically after reading TRANS_STATUS register, so reading
> this register has side effect and in case of Exynos5 should be done only

Yes, I found that in the Exynos5422 SoC manual as well. But still it wasn't
clear to me since AFAICT the logic should be the same. In other words, this
is what should happen (added comments to make more clear):

static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
{
...
	int_status = readl(i2c->regs + HSI2C_INT_STATUS);
	/* INT_I2C is set in int_status if interrupt occurred */
	writel(int_status, i2c->regs + HSI2C_INT_STATUS);
	trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
	/*
	 * TRANSFER_DONE_AUTO bit will be cleared when HSI2C_TRANS_STATUS
	 * is read but is set in trans_status if transaction succeeded. 
	 */

	/* handle interrupt related to the transfer status */
	if (i2c->variant->hw == HSI2C_EXYNOS7) {
...
	} else if (int_status & HSI2C_INT_I2C) {
	       /*
	        * Both HSI2C_INT_I2C and HSI2C_TRANS_DONE should be set
	        * but the latter isn't. trans_status & HSI2C_TRANS_DONE == 0.
		*/
		} else if (trans_status & HSI2C_TRANS_DONE) {
			i2c->trans_done = 1;
			i2c->state = 0;
		}
	}
...
 stop:
	/*
	 * Since i2c->trans_done is 0, the msg_complete completion won't be
	 * signaled and so the wait_for_completion_timeout() will timeout.
	 */
	if ((i2c->trans_done && (i2c->msg->len == i2c->msg_ptr)) ||
	    (i2c->state < 0)) {
		writel(0, i2c->regs + HSI2C_INT_ENABLE);
		exynos5_i2c_clr_pend_irq(i2c);
		complete(&i2c->msg_complete);
	}

	spin_unlock(&i2c->lock);

	return IRQ_HANDLED;
}

So I do understand that it has side effects but I don't see how this can
change the driver's logic since the state is already stored in variables.

But probably I'm missing something obvious...

> in 'if (int_status & HSI2C_INT_I2C)' clause. In case of Exynos7 (ie
> Exynos5433 :) ) reading TRANS_STATUS should not have side effects.
> Do you want to prepare fix patch from [0]?
> 

Sure, would something like the following work for you?

>From cba9f00ee7c419cee5ff980b583d92092d3ceaac Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javier@osg.samsung.com>
Date: Thu, 9 Mar 2017 10:06:21 -0300
Subject: [PATCH] i2c: exynos5: Avoid transaction timeouts due TRANSFER_DONE_AUTO not set

After commit 7999eecb7e56 ("i2c: exynos5: fix arbitration lost handling"),
some I2C transactions are failing because the TRANSFER_DONE_AUTO field is
not set in the I2C_TRANS_STATUS register so the i2c->status value is left
to -EINVAL causing the i2c->msg_complete completion to never be signaled.

For example, when reading the time of an I2C rtc on an Exynos5800 machine:

$ cat /sys/class/rtc/rtc0/time
[   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
[   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
cat: /sys/class/rtc/rtc0/time: Invalid argument

The Exynos5422 manual states clearly that most I2C_TRANS_STATUS reg bits
(including TRANSFER_DONE_AUTO) are cleared after the register is read. So
reading has side effects and should only be done if HSI2C_INT_I2C was set.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
 drivers/i2c/busses/i2c-exynos5.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index cbd93ce0661f..736a82472101 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -457,7 +457,6 @@ 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) {
@@ -482,11 +481,13 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 			goto stop;
 		}
 
+		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
 		if ((trans_status & HSI2C_MASTER_ST_MASK) == 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.9.3
 
Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

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

[Andrzej Hajda]

On 09.03.2017 14:13, Javier Martinez Canillas wrote:
> Hello Andrzej,
>
> On 03/09/2017 08:03 AM, Andrzej Hajda wrote:
>> Hi again,
>>
>> On 09.03.2017 08:51, Andrzej Hajda wrote:
>>> On 08.03.2017 21:05, Javier Martinez Canillas wrote:
>>>> Hello Andrzej,
>>>>
>>>> On 02/22/2017 08:04 AM, 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>
>>>>> ---
>>>> This patch causes regressions on Exynos5 boards (at least I noticed it in
>>>> Exynos5800 Peach Pi board). I see transmission timeouts when accessing I2C
>>>> device registers, i.e:
>>>>
>>>> $ cat /sys/class/rtc/rtc0/time
>>>> [   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
>>>> [   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
>>>> cat: /sys/class/rtc/rtc0/time: Invalid argument
>>> Hmm, at 12e10000 Exynos5 has hsi2c_9, and on this bus I have found only
>>> infineon,slb9645tt TPM module. At least on mainline kernel.
>>> What kernel do you use? Any additional changes to kernel?
>>> Do you observe it on mainline kernel?
>>>
>>> Regarding the issue, how often it happens?
>>> Please verify that this is not just coincidence.
>>>
>>>> Doing a partial revert of $SUBJECT (reading I2C_TRANS_STATUS register when
>>>> it was before) "fixes" the problem [0].
>>> That look crazy, the only difference for non-Exynos7 variant (which is
>>> in Exynos5 boards) is that with your change HSI2C_TRANS_STATUS is read
>>> only when HSI2C_INT_I2C happens, am I right?
>>> Anyway I have realized that in case of Exynos5 the device HSI2C driver
>>> works with is named "Universal Serial Interface" and has slightly
>>> different set of registers than HSI2C device present in Exynos52[56]0,
>>> but I do not know if it matters.
>>>
>>> If [0] really fixes the issue I think you can create real patch and send
>>> for testing, but it looks very suspicious.
>> Datasheet for Exynos5260 states clearly that TRANSFER_DONE_AUTO bit is
>> cleared automatically after reading TRANS_STATUS register, so reading
>> this register has side effect and in case of Exynos5 should be done only
> Yes, I found that in the Exynos5422 SoC manual as well. But still it wasn't
> clear to me since AFAICT the logic should be the same. In other words, this
> is what should happen (added comments to make more clear):
>
> static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
> {
> ...
> 	int_status = readl(i2c->regs + HSI2C_INT_STATUS);
> 	/* INT_I2C is set in int_status if interrupt occurred */
> 	writel(int_status, i2c->regs + HSI2C_INT_STATUS);
> 	trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
> 	/*
> 	 * TRANSFER_DONE_AUTO bit will be cleared when HSI2C_TRANS_STATUS
> 	 * is read but is set in trans_status if transaction succeeded. 
> 	 */
>
> 	/* handle interrupt related to the transfer status */
> 	if (i2c->variant->hw == HSI2C_EXYNOS7) {
> ...
> 	} else if (int_status & HSI2C_INT_I2C) {
> 	       /*
> 	        * Both HSI2C_INT_I2C and HSI2C_TRANS_DONE should be set
> 	        * but the latter isn't. trans_status & HSI2C_TRANS_DONE == 0.
> 		*/
> 		} else if (trans_status & HSI2C_TRANS_DONE) {
> 			i2c->trans_done = 1;
> 			i2c->state = 0;
> 		}
> 	}
> ...
>  stop:
> 	/*
> 	 * Since i2c->trans_done is 0, the msg_complete completion won't be
> 	 * signaled and so the wait_for_completion_timeout() will timeout.
> 	 */
> 	if ((i2c->trans_done && (i2c->msg->len == i2c->msg_ptr)) ||
> 	    (i2c->state < 0)) {
> 		writel(0, i2c->regs + HSI2C_INT_ENABLE);
> 		exynos5_i2c_clr_pend_irq(i2c);
> 		complete(&i2c->msg_complete);
> 	}
>
> 	spin_unlock(&i2c->lock);
>
> 	return IRQ_HANDLED;
> }
>
> So I do understand that it has side effects but I don't see how this can
> change the driver's logic since the state is already stored in variables.
>
> But probably I'm missing something obvious...

As this is rx timeout lets look at rx path only:
When last byt arrives probably at least three things are performed:
1. HSI2C_INT_RX_ALMOSTFULL irq,
2. HSI2C_TRANS_DONE bit is set.
3. HSI2C_INT_I2C irq,

It is not clear in which order it is done, 1-2-3 is quite probable, and
since our read of affected registers is not atomic from IP point of
view, it is possible following sequence:
a) ip signals HSI2C_INT_RX_ALMOSTFULL,
b) irq handler is called for HSI2C_INT_RX_ALMOSTFULL,
c) driver clears HSI2C_INT_STATUS,
c) ip sets HSI2C_TRANS_DONE, signals HSI2C_INT_I2C,
e) driver reads TRANS_STATUS, and resets HSI2C_TRANS_DONE
f) irq handler is called for HSI2C_INT_I2C, but HSI2C_TRANS_DONE bit is
already cleared.

If you like to experiment you can try to move reading TRANS_STATUS
before reading INT_STATUS, and check if that changes anything, or event
something more crazy:
    trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
    int_status = readl(i2c->regs + HSI2C_INT_STATUS);
    writel(int_status, i2c->regs + HSI2C_INT_STATUS);
    trans_status |= readl(i2c->regs + HSI2C_TRANS_STATUS);

but this is not material for patch :)

Your initial proposition [0] is the most suitable solution.

Regards
Andrzej

>
>> in 'if (int_status & HSI2C_INT_I2C)' clause. In case of Exynos7 (ie
>> Exynos5433 :) ) reading TRANS_STATUS should not have side effects.
>> Do you want to prepare fix patch from [0]?
>>
> Sure, would something like the following work for you?
>
> >From cba9f00ee7c419cee5ff980b583d92092d3ceaac Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javier@osg.samsung.com>
> Date: Thu, 9 Mar 2017 10:06:21 -0300
> Subject: [PATCH] i2c: exynos5: Avoid transaction timeouts due TRANSFER_DONE_AUTO not set
>
> After commit 7999eecb7e56 ("i2c: exynos5: fix arbitration lost handling"),
> some I2C transactions are failing because the TRANSFER_DONE_AUTO field is
> not set in the I2C_TRANS_STATUS register so the i2c->status value is left
> to -EINVAL causing the i2c->msg_complete completion to never be signaled.
>
> For example, when reading the time of an I2C rtc on an Exynos5800 machine:
>
> $ cat /sys/class/rtc/rtc0/time
> [   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
> [   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
> cat: /sys/class/rtc/rtc0/time: Invalid argument
>
> The Exynos5422 manual states clearly that most I2C_TRANS_STATUS reg bits
> (including TRANSFER_DONE_AUTO) are cleared after the register is read. So
> reading has side effects and should only be done if HSI2C_INT_I2C was set.
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>  drivers/i2c/busses/i2c-exynos5.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> index cbd93ce0661f..736a82472101 100644
> --- a/drivers/i2c/busses/i2c-exynos5.c
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -457,7 +457,6 @@ 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) {
> @@ -482,11 +481,13 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>  			goto stop;
>  		}
>  
> +		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
>  		if ((trans_status & HSI2C_MASTER_ST_MASK) == 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;

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

[Andrzej Hajda]

On 09.03.2017 14:13, Javier Martinez Canillas wrote:
> (...)
> Sure, would something like the following work for you?
>
> >From cba9f00ee7c419cee5ff980b583d92092d3ceaac Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javier@osg.samsung.com>
> Date: Thu, 9 Mar 2017 10:06:21 -0300
> Subject: [PATCH] i2c: exynos5: Avoid transaction timeouts due TRANSFER_DONE_AUTO not set
>
> After commit 7999eecb7e56 ("i2c: exynos5: fix arbitration lost handling"),
> some I2C transactions are failing because the TRANSFER_DONE_AUTO field is
> not set in the I2C_TRANS_STATUS register so the i2c->status value is left
> to -EINVAL causing the i2c->msg_complete completion to never be signaled.
>
> For example, when reading the time of an I2C rtc on an Exynos5800 machine:
>
> $ cat /sys/class/rtc/rtc0/time
> [   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
> [   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
> cat: /sys/class/rtc/rtc0/time: Invalid argument
>
> The Exynos5422 manual states clearly that most I2C_TRANS_STATUS reg bits
> (including TRANSFER_DONE_AUTO) are cleared after the register is read. So
> reading has side effects and should only be done if HSI2C_INT_I2C was set.
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

Looks OK, you can add Fixes tag and my Reviewed-by: Andrzej Hajda
<a.hajda@samsung.com>.

Regards
Andrzej


> ---
>  drivers/i2c/busses/i2c-exynos5.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> index cbd93ce0661f..736a82472101 100644
> --- a/drivers/i2c/busses/i2c-exynos5.c
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -457,7 +457,6 @@ 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) {
> @@ -482,11 +481,13 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>  			goto stop;
>  		}
>  
> +		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
>  		if ((trans_status & HSI2C_MASTER_ST_MASK) == 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;

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

[Javier Martinez Canillas]

Hello Andrzej,

On 03/09/2017 10:49 AM, Andrzej Hajda wrote:
> On 09.03.2017 14:13, Javier Martinez Canillas wrote:
>> Hello Andrzej,
>>
>> On 03/09/2017 08:03 AM, Andrzej Hajda wrote:
>>> Hi again,
>>>
>>> On 09.03.2017 08:51, Andrzej Hajda wrote:
>>>> On 08.03.2017 21:05, Javier Martinez Canillas wrote:
>>>>> Hello Andrzej,
>>>>>
>>>>> On 02/22/2017 08:04 AM, 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>
>>>>>> ---
>>>>> This patch causes regressions on Exynos5 boards (at least I noticed it in
>>>>> Exynos5800 Peach Pi board). I see transmission timeouts when accessing I2C
>>>>> device registers, i.e:
>>>>>
>>>>> $ cat /sys/class/rtc/rtc0/time
>>>>> [   25.924594] exynos5-hsi2c 12e10000.i2c: rx timeout
>>>>> [   65.028365] max77686-rtc max77802-rtc: Fail to read time reg(-22)
>>>>> cat: /sys/class/rtc/rtc0/time: Invalid argument
>>>> Hmm, at 12e10000 Exynos5 has hsi2c_9, and on this bus I have found only
>>>> infineon,slb9645tt TPM module. At least on mainline kernel.
>>>> What kernel do you use? Any additional changes to kernel?
>>>> Do you observe it on mainline kernel?
>>>>
>>>> Regarding the issue, how often it happens?
>>>> Please verify that this is not just coincidence.
>>>>
>>>>> Doing a partial revert of $SUBJECT (reading I2C_TRANS_STATUS register when
>>>>> it was before) "fixes" the problem [0].
>>>> That look crazy, the only difference for non-Exynos7 variant (which is
>>>> in Exynos5 boards) is that with your change HSI2C_TRANS_STATUS is read
>>>> only when HSI2C_INT_I2C happens, am I right?
>>>> Anyway I have realized that in case of Exynos5 the device HSI2C driver
>>>> works with is named "Universal Serial Interface" and has slightly
>>>> different set of registers than HSI2C device present in Exynos52[56]0,
>>>> but I do not know if it matters.
>>>>
>>>> If [0] really fixes the issue I think you can create real patch and send
>>>> for testing, but it looks very suspicious.
>>> Datasheet for Exynos5260 states clearly that TRANSFER_DONE_AUTO bit is
>>> cleared automatically after reading TRANS_STATUS register, so reading
>>> this register has side effect and in case of Exynos5 should be done only
>> Yes, I found that in the Exynos5422 SoC manual as well. But still it wasn't
>> clear to me since AFAICT the logic should be the same. In other words, this
>> is what should happen (added comments to make more clear):
>>
>> static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>> {
>> ...
>> 	int_status = readl(i2c->regs + HSI2C_INT_STATUS);
>> 	/* INT_I2C is set in int_status if interrupt occurred */
>> 	writel(int_status, i2c->regs + HSI2C_INT_STATUS);
>> 	trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
>> 	/*
>> 	 * TRANSFER_DONE_AUTO bit will be cleared when HSI2C_TRANS_STATUS
>> 	 * is read but is set in trans_status if transaction succeeded. 
>> 	 */
>>
>> 	/* handle interrupt related to the transfer status */
>> 	if (i2c->variant->hw == HSI2C_EXYNOS7) {
>> ...
>> 	} else if (int_status & HSI2C_INT_I2C) {
>> 	       /*
>> 	        * Both HSI2C_INT_I2C and HSI2C_TRANS_DONE should be set
>> 	        * but the latter isn't. trans_status & HSI2C_TRANS_DONE == 0.
>> 		*/
>> 		} else if (trans_status & HSI2C_TRANS_DONE) {
>> 			i2c->trans_done = 1;
>> 			i2c->state = 0;
>> 		}
>> 	}
>> ...
>>  stop:
>> 	/*
>> 	 * Since i2c->trans_done is 0, the msg_complete completion won't be
>> 	 * signaled and so the wait_for_completion_timeout() will timeout.
>> 	 */
>> 	if ((i2c->trans_done && (i2c->msg->len == i2c->msg_ptr)) ||
>> 	    (i2c->state < 0)) {
>> 		writel(0, i2c->regs + HSI2C_INT_ENABLE);
>> 		exynos5_i2c_clr_pend_irq(i2c);
>> 		complete(&i2c->msg_complete);
>> 	}
>>
>> 	spin_unlock(&i2c->lock);
>>
>> 	return IRQ_HANDLED;
>> }
>>
>> So I do understand that it has side effects but I don't see how this can
>> change the driver's logic since the state is already stored in variables.
>>
>> But probably I'm missing something obvious...
> 
> As this is rx timeout lets look at rx path only:
> When last byt arrives probably at least three things are performed:
> 1. HSI2C_INT_RX_ALMOSTFULL irq,
> 2. HSI2C_TRANS_DONE bit is set.
> 3. HSI2C_INT_I2C irq,
> 
> It is not clear in which order it is done, 1-2-3 is quite probable, and
> since our read of affected registers is not atomic from IP point of
> view, it is possible following sequence:
> a) ip signals HSI2C_INT_RX_ALMOSTFULL,
> b) irq handler is called for HSI2C_INT_RX_ALMOSTFULL,
> c) driver clears HSI2C_INT_STATUS,
> c) ip sets HSI2C_TRANS_DONE, signals HSI2C_INT_I2C,
> e) driver reads TRANS_STATUS, and resets HSI2C_TRANS_DONE
> f) irq handler is called for HSI2C_INT_I2C, but HSI2C_TRANS_DONE bit is
> already cleared.
> 

Right, this is a good explanation. Thanks a lot for taking the time to write it.

> If you like to experiment you can try to move reading TRANS_STATUS
> before reading INT_STATUS, and check if that changes anything, or event
> something more crazy:
>     trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
>     int_status = readl(i2c->regs + HSI2C_INT_STATUS);
>     writel(int_status, i2c->regs + HSI2C_INT_STATUS);
>     trans_status |= readl(i2c->regs + HSI2C_TRANS_STATUS);
> 
> but this is not material for patch :)
> 
> Your initial proposition [0] is the most suitable solution.
>

Great, I'll add the fixes and your reviewed-by tags and post it as a patch.

> Regards
> Andrzej
> 
 
Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

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

[Javier Martinez Canillas]

Hello Andrzej,

On 03/09/2017 10:59 AM, Javier Martinez Canillas wrote:
>>
>> Your initial proposition [0] is the most suitable solution.
>>
> 
> Great, I'll add the fixes and your reviewed-by tags and post it as a patch.
> 

Ups, it seems I forgot to cc you when posting the patch, sorry about that...

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1349049.html

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America