Re: [2/3] i2c: exynos5: implement bus recovery functionality

From: Andrzej Hajda <a.hajda@samsung.com>
To: Wolfram Sang <wsa@the-dreams.de>
Cc: Andi Shyti <andi.shyti@samsung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	"open list:I2C SUBSYSTEM" <linux-i2c@vger.kernel.org>,
	"moderated list:ARM/SAMSUNG EXYNOS ARM ARCHITECTURES"
	<linux-samsung-soc@vger.kernel.org>
Subject: Re: [2/3] i2c: exynos5: implement bus recovery functionality
Date: Tue, 16 Jan 2018 09:35:45 +0100	[thread overview]
Message-ID: <8d4c93bf-c27d-7a64-fb4e-3957760ff9b7@samsung.com> (raw)
In-Reply-To: <20180115205250.p5mbn3ib2paj53pv@ninjato>

On 15.01.2018 21:52, Wolfram Sang wrote:
> Hi,
>
> On Thu, Nov 30, 2017 at 03:30:06PM +0100, Andrzej Hajda wrote:
>> In some circumstances I2C clients can be in abnormal state, to allow
>> recover them from it I2C master can pulse SCL line until SDA line
>> goes up and then generate STOP condition. Exynos driver does not have
>> possibility to manipulate I2C lines directly, but it can provide similar
>> functionality using manual commands. Replacing I2C adapter reset with
> Really? READ_DATA looks like 8 clock pulses, but you need 9.

This is why I have used word 'similar' :) Unfortunately chip does not
provide method to send exactly 9 pulses.
But according to [1] it is the worst case scenario, usually less than 9
pulses is enough.
Another solution I see is to READ_DATA twice, this way we will have 16
pulses.

[1]:
http://www.analog.com/media/en/technical-documentation/application-notes/54305147357414AN686_0.pdf
>
>> bus recovery significantly incereases I2C bus robustness in case of timeouts.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>
>
>> @@ -642,7 +666,7 @@ static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
>>  		ret = exynos5_i2c_wait_bus_idle(i2c);
>>  
>>  	if (ret < 0) {
>> -		exynos5_i2c_reset(i2c);
>> +		exynos5_i2c_recovery(i2c);
> Bus recovery is only defined for stalled busses, i.e. when SDA is stuck
> low. It is not a suitable method to deal with general timeouts.

You are right, I though I do not have precise info that SDA is stuck
low, but I guess status HSI2C_MASTER_ST_LOSE indicates is, I will check it.

>
>
>> +int exynos5_i2c_recover_bus(struct i2c_adapter *adap)
>> +{
>> +	struct exynos5_i2c *i2c = adap->algo_data;
>> +
>> +	i2c_lock_adapter(adap);
>> +	clk_enable(i2c->clk);
>> +	exynos5_i2c_recovery(i2c);
>> +	clk_disable(i2c->clk);
>> +	i2c_unlock_adapter(adap);
> Why do you need to lock? Aren't you protected by the call to master_xfer
> which then detected a problem?

If the problem is detected in master_xfer driver uses internal function
exynos5_i2c_recovery,
but function exynos5_i2c_recover_bus is called by i2c_recover_bus, and
the latter can be called from any context,
this is why I have added locking.

Regards
Andrzej
>
> Regards,
>
>    Wolfram
>