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

[Andrzej Hajda <a.hajda@samsung.com>]

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