Re: [PATCH 03/12] i2c: pxa: Add reset operation when i2c bus busy

[Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Saturday 30 May 2015 01:09 AM, Robert Jarzmik wrote:
> Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
>
>> From: "Jett.Zhou" <jtzhou-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>>
>> According to some test in emei_dkb, we found some i2c slave device
>> (eg. camera sensor ov2659 power up) introduce noise on sda, so detect
>> i2c controller busy, and assert reset to i2c controller to recover as
>> early as possible to avoid more latency on the entire i2c transaction.
>>
>> Signed-off-by: Jett.Zhou <jtzhou-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>> [vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org: Removed reduction in timeout value, as I
>> do not have goot explanation for it. Logically it is not required.
>> And also Updated changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   drivers/i2c/busses/i2c-pxa.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> index d4c798a..a76c901 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -314,6 +314,10 @@ static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c)
>>   {
>>   	int timeout = DEF_TIMEOUT;
>>
>> +	if (readl(_ISR(i2c)) & (ISR_IBB | ISR_UB))
>> +		i2c_pxa_reset(i2c);
>
> The pxa27x manual states in the Developer Manual, chapter 9.4.13 "Reset
> Conditions" :
>              Software must ensure that (1) the I 2 C unit is not busy before it
>              asserts a reset
>
> Given that, I don't agree with this patch.


Hmmm,

Just saw pxa27x manual, and you are right.
In that case I am not sure how to address the issue mentioned in the
changelog.

On the other side,

Check for ISR_IBB, should be ok, as, as per spec it says,

"Set when the TWSI bus is busy but the SoC TWSI is not
involved in the transaction."

Also,
I believe we should be ok, as the first thing we do in i2c_pxa_reset()
is abort the current transaction and then assert reset. Isn't it?

> Moreover, reseting unconditionaly the
> i2c bus on each busy state on a write transaction for one single corner case is
> not something that has my agreement. A quirk might overcome my reluctance.
>

Any condition check you can possibly think of for asserting reset???

Having said that,
Somewhere I do agree with you that we need to further debug on noise
issue in HW rather hacking software. :)

I am ok to drop this patch, do further testing and revisit again if
issue persist.

Thanks,
Vaibhav