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

[Javier Martinez Canillas <javier@osg.samsung.com>]

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