Implementing bus recovery

Implementing bus recovery

[Simon Lindgren]

I would like to implement bus recovery support for the at91 driver, but 
there it is not clear how the core support is supposed to be used, and I 
could not find an existing adapter implementation using it.

First off, reading the old mailing list discussion here:
http://thread.gmane.org/gmane.linux.drivers.i2c/10225

It seems a pinctrl state should be used to switch the pins over to gpio 
mode. Now, I need a place to put this state switching. The 
i2c_bus_recovery_info struct looks like this:

struct i2c_bus_recovery_info {
	int (*recover_bus)(struct i2c_adapter *);

	int (*get_scl)(struct i2c_adapter *);
	void (*set_scl)(struct i2c_adapter *, int val);
	int (*get_sda)(struct i2c_adapter *);

	void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
	void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);

	/* gpio recovery */
	int scl_gpio;
	int sda_gpio;
};

There is no usable callback to do this, because {un,}prepare_recovery is 
only passed the bus recovery info and no driver data is reachable from that.

So next up, I thought I'd wrap i2c_generic_gpio_recovery and do the 
state switching there. But then I have to fill in the entire info struct 
by hand which definitely does not seem to be the intention given the 
special case in i2c_register_adapter. What am I missing?

Re: Implementing bus recovery

[Mark Roszko]

Prepare_recovery and unprepare_recovery are to configure the pad
multiplexer since thats platform specific. You don't care about the
adapter reference as you just need the scl_gpio and sda_gpio values to
know what pins to reconfigure.

In fact Atmel's pad mux functions look like this:

at91_set_A_periph(unsigned pin, int use_pullup)
at91_set_B_periph(unsigned pin, int use_pullup)
at91_set_C_periph(unsigned pin, int use_pullup)
.....


Here's the latest(or close) driver specific patch that slipped through
the cracks it seems?
http://www.spinics.net/lists/linux-i2c/msg11357.html


Bus_recovery itself you have to call in the at91 driver on a
controller timeout just like the patch does.
 if (ret == 0) {
                 dev_err(dev->dev, "controller timed out\n");
-                at91_init_twi_bus(dev);
 +                if (i2c_recover_bus(dev->adapter->bus_recovery_info) < 0)
 +                      at91_init_twi_bus(dev);

                 ret = -ETIMEDOUT;
                 goto error;
}

the call stack looks something like this

--at91_twi_xfer
----- i2c_recover_bus
--------- i2c_generic_gpio_recovery
-------------i2c_get_gpios_for_recovery  (requests gpios)
-------------i2c_generic_recovery
----------------prepare_recovery         (configure padmux to put gpio
on the pin)
----------------get_sda, get_scl          (get gpio value, generic
function by default is fine for atmel)
----------------set_scl                       (set scl value, generic
function by default is fine for atmel)
----------------unrepare_recovery       (configure padmux to put twi
back on the pin)
-------------i2c_put_gpios_for_recovery (frees gpios)


Somewhere in at91_twi_probe before the adapter is registered you also
have to setup the bus_recovery_info struct and set recover_bus to the
i2c_generic_gpio_recovery function similar to the patch linked.

Re: Implementing bus recovery

[Simon Lindgren]

Den 2014-08-23 02:41, Mark Roszko skrev:
> Prepare_recovery and unprepare_recovery are to configure the pad
> multiplexer since thats platform specific. You don't care about the
> adapter reference as you just need the scl_gpio and sda_gpio values to
> know what pins to reconfigure.
>
> In fact Atmel's pad mux functions look like this:
>
> at91_set_A_periph(unsigned pin, int use_pullup)
> at91_set_B_periph(unsigned pin, int use_pullup)
> at91_set_C_periph(unsigned pin, int use_pullup)
> .....
>
>
> Here's the latest(or close) driver specific patch that slipped through
> the cracks it seems?
> http://www.spinics.net/lists/linux-i2c/msg11357.html
>

We are using a sama5d3-based machine which is almost entirely dt based, 
so passing callbacks to the driver using the platform data seems like a 
bad fit. Hard coding these calls in the driver won't really work either 
(even for a single machine) since not all instances of the adapter exist 
on the same peripheral. Ie, one adapter is on peripheral A and the 
others are on peripheral B for this specific soc.

The way I assumed it would work was to declare a "recovery" pinmux state 
in the device tree and then switch to that during the recovery and then 
back to default once it's done. Could you expand a bit on this please?

>
> Bus_recovery itself you have to call in the at91 driver on a
> controller timeout just like the patch does.
>   if (ret == 0) {
>                   dev_err(dev->dev, "controller timed out\n");
> -                at91_init_twi_bus(dev);
>   +                if (i2c_recover_bus(dev->adapter->bus_recovery_info) < 0)
>   +                      at91_init_twi_bus(dev);
>
>                   ret = -ETIMEDOUT;
>                   goto error;
> }
>
> the call stack looks something like this
>
> --at91_twi_xfer
> ----- i2c_recover_bus
> --------- i2c_generic_gpio_recovery
> -------------i2c_get_gpios_for_recovery  (requests gpios)
> -------------i2c_generic_recovery
> ----------------prepare_recovery         (configure padmux to put gpio
> on the pin)
> ----------------get_sda, get_scl          (get gpio value, generic
> function by default is fine for atmel)
> ----------------set_scl                       (set scl value, generic
> function by default is fine for atmel)
> ----------------unrepare_recovery       (configure padmux to put twi
> back on the pin)
> -------------i2c_put_gpios_for_recovery (frees gpios)
>
>
> Somewhere in at91_twi_probe before the adapter is registered you also
> have to setup the bus_recovery_info struct and set recover_bus to the
> i2c_generic_gpio_recovery function similar to the patch linked.
>

That matches my previous understanding, thanks for the confirmation.

Re: Implementing bus recovery

[Mark Roszko]

>We are using a sama5d3-based machine which is almost entirely dt based, so passing callbacks to the driver using the platform data seems like a bad fit. >Hard coding these calls in the driver won't really work either (even for a single machine) since not all instances of the adapter exist on the same peripheral. >Ie, one adapter is on peripheral A and the others are on peripheral B for this specific soc.

 I forgot the SAMA5 doesn't keep the TWI peripherals on the same
pinmux positions. Even more variance with the other Atmel SoCs using
the same driver. Easiest fix would be to pass the i2c_adapter pointer
rather than i2c_bus_recovery_info which seems harmless and retains the
recovery info pointer internally. That way you can grab the at91
driver struct where you would have saved the pinmux before/after
config.

Btw, side thing, this may help you since you seem to care about your
SAMA5's i2c functioning.... if you are doing smbus block reads with
the SAMA5 you may want to pull this patch for yourself:
https://patchwork.ozlabs.org/patch/381843/. Fixes a kernel panic and
at91 TWI controller timeout.

Re: Implementing bus recovery

[Simon Lindgren]

(btw, I noticed my emails were badly formatted. Hopefully I have fixed
that now)

Den 2014-08-27 05:08, Mark Roszko skrev:
>> We are using a sama5d3-based machine which is almost entirely dt based, so passing callbacks to the driver using the platform data seems like a bad fit. >Hard coding these calls in the driver won't really work either (even for a single machine) since not all instances of the adapter exist on the same peripheral. >Ie, one adapter is on peripheral A and the others are on peripheral B for this specific soc.
> 
>  I forgot the SAMA5 doesn't keep the TWI peripherals on the same
> pinmux positions. Even more variance with the other Atmel SoCs using
> the same driver. Easiest fix would be to pass the i2c_adapter pointer
> rather than i2c_bus_recovery_info which seems harmless and retains the
> recovery info pointer internally. That way you can grab the at91
> driver struct where you would have saved the pinmux before/after
> config.
> 

Gave this a shot and it seems to work fine :-)

> Btw, side thing, this may help you since you seem to care about your
> SAMA5's i2c functioning.... if you are doing smbus block reads with
> the SAMA5 you may want to pull this patch for yourself:
> https://patchwork.ozlabs.org/patch/381843/. Fixes a kernel panic and
> at91 TWI controller timeout.
> 

Nice, thanks for the heads-up :-)