Re: [PATCH V8 1/2] i2c/adapter: Add bus recovery infrastructure

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

On Thu, Jan 24, 2013 at 08:24:45AM +0100, Wolfram Sang wrote:
> Hi Viresh,
> 
> I think we are getting close.
> 
> On Mon, Dec 03, 2012 at 08:24:04AM +0530, Viresh Kumar wrote:
> > Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c
> > protocol Rev. 03 section 3.1.16 titled "Bus clear".
> > 
> > http://www.nxp.com/documents/user_manual/UM10204.pdf
> > 
> > Sometimes during operation i2c bus hangs and we need to give dummy clocks to
> > slave device to start the transfer again. Now we may have capability in the bus
> > controller to generate these clocks or platform may have gpio pins which can be
> > toggled to generate dummy clocks. This patch supports both.
> > 
> > This patch also adds in generic bus recovery routines gpio or scl line based
> > which can be used by bus controller. In addition controller driver may provide
> > its own version of the bus recovery routine.
> > 
> > This doesn't support multi-master recovery for now.
> 
> Assuming that recover_bus is not called on BUS_BUSY but on TIMEOUTs,,
> this should work?
How do you differentiate these two? You're machine boots and sees sda
being low. How long should it wait for action on sda or scl until it can
diagnose a timeout?

If the current code is operated on a multi-master bus and the bus is
busy various things can happen. At least without sda polling the current
transaction could be modified and completed. Not sure if that can happen
with sda polling.

Some more comments below.

> > +static int i2c_generic_recovery(struct i2c_adapter *adap)
> > +{
> > +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> > +	int i, val = 0;
> > +
> > +	if (bri->prepare_recovery)
> > +		bri->prepare_recovery(bri);
> 
> prepare_recovery and unprepare should be in i2c_generic_scl_recovery and
> i2c_generic_gpio_recovery since they are probably needed to turn SDA/SCL
> into GPIOs and back. We want that before the gpio_get/put routines.
> 
> > +
> > +	/* SCL shouldn't be low here */
> > +	if (!bri->get_scl(adap)) {
> > +		dev_err(&adap->dev, "SCL is stuck Low, exiting recovery.\n");
> 
> returning -EBUSY?
> 
> > +		goto unprepare;
> > +	}
> > +
> > +	/*
> > +	 * By this time SCL is high, as we need to give 9 falling-rising edges
> > +	 */
> > +	for (i = 0; i < RECOVERY_CLK_CNT * 2; i++, val = !val) {
> > +		bri->set_scl(adap, val);
> > +		ndelay(clk_delay);
> > +
> > +		/* break if sda got high, check only when scl line is high */
> > +		if (!bri->skip_sda_polling && val)
> 
> * if (val && bri->get_sda)
> 
> > +			/* Check SCL again to see fault condition */
> > +			if (!bri->get_scl(adap)) {
> > +				dev_err(&adap->dev, "SCL is stuck Low during recovery, exiting recovery.\n");
> 
> returning -EBUSY?
> This scl check should not depend on skip_sda_polling, or?
Well right. But note this might also just be a slave doing clock
streching.

> > +				goto unprepare;
> > +			}
> > +
> > +			if (bri->get_sda(adap))
> > +				break;
Indention suggests that you want this in the body of the if (val &&
bri->get_sda). Unfortunately the C-Compiler won't notice without braces.

> Checking SDA should be done before setting SCL? That would
> 
> a) let us escape early if SDA got HIGH magically somehow until we enter
>    the for loop for the first time
> b) breaking out of the for loop after the last pulse is moot
You mean:

	val = 0;

	for (i = 0; i < RECOVERY_CLK_CNT * 2; i++, val = !val) {
		if (!val && !bri->get_scl(adap))
				scl stuck low (or wait a bit to sort out clock streching)
		if (bri->get_sda && bri->get_sda(adap))
			break;

		bri->set_scl(adap, val);
		ndelay(clk_delay);
	}

Looks ok I think. This would also get rid of the seperate scl check
before the loop.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |