I2C: Add bus recovery infrastructure

Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c protocol Rev. 03 section 3.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.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/i2c/i2c-core.c |  125 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    |   52 ++++++++++++++++++++
 2 files changed, 177 insertions(+), 0 deletions(-)

drivers/i2c/i2c-core.c +2 -0

index e9c1893..5cd5f83 100644
@@ -26,7 +26,9 @@
 
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/delay.h>
 #include <linux/errno.h>
+#include <linux/gpio.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/init.h>

drivers/i2c/i2c-core.c +66 -0

+	int i, ret, val = 1;
+
+	if (bri->get_gpio)
+		bri->get_gpio(bri->scl_gpio);
+
+	ret = gpio_request_one(bri->scl_gpio, bri->scl_gpio_flags, "i2c-scl");
+	if (ret < 0) {
+		dev_warn(&adap->dev, "gpio request fail: %d\n", bri->scl_gpio);
+		return ret;
+	}
+
+	if (!bri->skip_sda_polling) {
+		if (bri->get_gpio)
+			bri->get_gpio(bri->sda_gpio);
+
+		ret = gpio_request_one(bri->sda_gpio, bri->sda_gpio_flags,
+				"i2c-sda");
+		if (ret < 0) {
+			/* work without sda polling */
+			dev_warn(&adap->dev,
+				"sda_gpio request fail: %d. Skip sda polling\n",
+				bri->scl_gpio);
+			bri->skip_sda_polling = true;
+			if (bri->put_gpio)
+				bri->put_gpio(bri->sda_gpio);
+		}
+	}
+
+	delay /= bri->clock_rate_khz * 2;
+
+	for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
+		ndelay(delay);
+		gpio_set_value(bri->scl_gpio, val);
+
+		/* break if sda got high, check only when scl line is high */
+		if (!bri->skip_sda_polling && val)
+			if (gpio_get_value(bri->sda_gpio))
+				break;
+	}
+
+	gpio_free(bri->scl_gpio);
+
+	if (bri->put_gpio)
+		bri->put_gpio(bri->scl_gpio);
+
+	if (!bri->skip_sda_polling) {
+		gpio_free(bri->sda_gpio);
+
+		if (bri->put_gpio)
+			bri->put_gpio(bri->sda_gpio);
+	}
+
+	return 0;
+}
+
+static int i2c_scl_recover_bus(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	int i, val = 0;
+	unsigned long delay = 1000000;
+
+	delay /= bri->clock_rate_khz * 2;
+
+	for (i = 0; i < bri->clock_cnt * 2; i++,
+			val = !val) {
+		bri->set_scl(adap, val);

drivers/i2c/i2c-core.c +27 -0

+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);

@@ -861,6 +945,47 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 			 "Failed to create compatibility class link\n");
 #endif
 
+	/* bus recovery specific initialization */
+	if (adap->bus_recovery_info) {
+		struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+
+		if (bri->recover_bus) {
+			dev_info(&adap->dev,
+				"registered for non-generic bus recovery\n");
+		} else {
+			/* Use generic recovery routines */
+			if (!bri->clock_rate_khz) {
+				dev_warn(&adap->dev,
+					"doesn't have valid recovery clock rate\n");
+				goto exit_recovery;
+			}
+
+			/* Most controller need 9 clocks at max */
+			if (!bri->clock_cnt)
+				bri->clock_cnt = 9;
+
+			if (bri->is_gpio_recovery) {
+				bri->recover_bus = i2c_gpio_recover_bus;
+				dev_info(&adap->dev,
+					"registered for gpio bus recovery\n");
+			} else if (bri->set_scl) {
+				if (!bri->skip_sda_polling && !bri->get_sda) {
+					dev_warn(&adap->dev,

drivers/i2c/i2c-core.c +5 -0

+			}
+		}
+	}
+
+exit_recovery:
 	/* create pre-declared device nodes */
 	if (adap->nr < __i2c_first_dynamic_bus_num)
 		i2c_scan_static_board_info(adap);

include/linux/i2c.h +0 -0

index 8e25a91..1310d1a 100644

include/linux/i2c.h +21 -0

+ */
+struct i2c_bus_recovery_info {
+	int (*recover_bus)(struct i2c_adapter *);
+	bool skip_sda_polling;
+	bool is_gpio_recovery;
+	u32 clock_rate_khz;
+	u8 clock_cnt;
+
+	/* scl/sda recovery */
+	void (*set_scl)(struct i2c_adapter *, int val);
+	int (*get_sda)(struct i2c_adapter *);
+
+	/* gpio recovery */
+	int (*get_gpio)(unsigned gpio);
+	int (*put_gpio)(unsigned gpio);
+	u32 scl_gpio;
+	u32 sda_gpio;
+	u32 scl_gpio_flags;
+	u32 sda_gpio_flags;
+};
+
 /*
  * i2c_adapter is the structure used to identify a physical i2c bus along
  * with the access algorithms necessary to access it.

On Fri, Mar 02, 2012 at 11:53:42AM +0530, Viresh Kumar wrote:
Finally, a review \o/

drivers/i2c/i2c-core.c static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env) · line 111

Quoted 5 lines

drivers/i2c/i2c-core.c +4 -0

+/* i2c bus recovery routines */
+static int i2c_gpio_recover_bus(struct i2c_adapter *adap)
+{
+     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;

I think something alike 'recov_info' is more readable than 'bri'.

Quoted 1 line

+     unsigned long delay = 1000000;

udelay?

Quoted 14 lines

+     return 0;

drivers/i2c/i2c-core.c +13 -0

+}
+
+static int i2c_scl_recover_bus(struct i2c_adapter *adap)
+{
+     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+     int i, val = 0;
+     unsigned long delay = 1000000;
+
+     delay /= bri->clock_rate_khz * 2;
+
+     for (i = 0; i < bri->clock_cnt * 2; i++,
+                     val = !val) {
+             bri->set_scl(adap, val);

drivers/i2c/i2c-core.c static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env) · line 188

Quoted 10 lines

drivers/i2c/i2c-core.c +10 -0

+             ndelay(delay);
+
+             /* break if sda got high, check only when scl line is high */
+             if (!bri->skip_sda_polling && val)
+                     if (bri->get_sda(adap))
+                             break;
+     }
+
+     return 0;
+}

We have two recover bus functions here. I think it would be better to have only one. set_scl and get_sda can be mapped to helper functions doing needed gpio-calls during init. Something like this pseudo-code:

       if (!bri->set_scl && we_have_a_scl_gpio)
               set_scl = i2c_set_scl_via_gpio()

Quoted 38 lines

 static int i2c_device_probe(struct device *dev)
 {
      struct i2c_client       *client = i2c_verify_client(dev);

drivers/i2c/i2c-core.c +31 -0

@@ -861,6 +945,47 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
                        "Failed to create compatibility class link\n");
  #endif
 
+     /* bus recovery specific initialization */
+     if (adap->bus_recovery_info) {
+             struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+
+             if (bri->recover_bus) {
+                     dev_info(&adap->dev,
+                             "registered for non-generic bus recovery\n");
+             } else {
+                     /* Use generic recovery routines */
+                     if (!bri->clock_rate_khz) {
+                             dev_warn(&adap->dev,
+                                     "doesn't have valid recovery clock rate\n");
+                             goto exit_recovery;
+                     }
+
+                     /* Most controller need 9 clocks at max */
+                     if (!bri->clock_cnt)
+                             bri->clock_cnt = 9;
+
+                     if (bri->is_gpio_recovery) {
+                             bri->recover_bus = i2c_gpio_recover_bus;
+                             dev_info(&adap->dev,
+                                     "registered for gpio bus recovery\n");
+                     } else if (bri->set_scl) {
+                             if (!bri->skip_sda_polling && !bri->get_sda) {
+                                     dev_warn(&adap->dev,
+                                             "!get_sda. skip sda polling\n");
+                                     bri->skip_sda_polling = true;
+                             }
+
+                             bri->recover_bus = i2c_scl_recover_bus;

drivers/i2c/i2c-core.c static int i2c_register_adapter(struct i2c_adapter *adap) · line 983

Quoted 5 lines

drivers/i2c/i2c-core.c +5 -0

+                             dev_info(&adap->dev,
+                                     "registered for scl bus recovery\n");
+                     } else {
+                             dev_warn(&adap->dev,
+                                     "doesn't have valid recovery type\n");

Printouts here should probably be dev_dbg (if not left out), most users won't care.

Quoted 12 lines

drivers/i2c/i2c-core.c +5 -0

+                     }
+             }
+     }
+
+exit_recovery:
       /* create pre-declared device nodes */
       if (adap->nr < __i2c_first_dynamic_bus_num)
               i2c_scan_static_board_info(adap);

include/linux/i2c.h +0 -0

index 8e25a91..1310d1a 100644

include/linux/i2c.h struct i2c_algorithm { · line 367

Quoted 4 lines

include/linux/i2c.h +0 -0

@@ -365,6 +365,55 @@ struct i2c_algorithm {
       u32 (*functionality) (struct i2c_adapter *);
  };
 

The description of the members is a good start. To be complete, I'd think we need some higher level description in Documentation/i2c, too, with an example how to set up.

include/linux/i2c.h struct i2c_algorithm { · line 373

Quoted 6 lines

include/linux/i2c.h +6 -0

+/**
+ * struct i2c_bus_recovery_info - I2c bus recovery information
+ * @recover_bus: Recover routine. Either pass driver's recover_bus() routine, or
+ *   pass it NULL to use generic ones, i.e. gpio or scl based.
+ * @skip_sda_polling: if true, bus recovery will not poll sda line to check if
+ *   it became high or not. Only required if recover_bus == NULL.

Assume this when there is no get_sda() and/or SDA GPIO  defined?

include/linux/i2c.h struct i2c_algorithm { · line 375

Quoted 2 lines

include/linux/i2c.h +2 -0

+ * @is_gpio_recovery: true, select gpio type else scl type. Only required if
+ *   recover_bus == NULL.

Assume this when there is no GPIO defined (and no recover_bus)?

include/linux/i2c.h struct i2c_algorithm { · line 379

Quoted 2 lines

include/linux/i2c.h +2 -0

+ * @clock_cnt: count of max clocks to be generated. Required for both gpio and
+ *   scl type recovery.

Why should that be different from 9 (as said in the docs)?

include/linux/i2c.h struct i2c_algorithm { · line 387

Quoted 8 lines

include/linux/i2c.h +8 -0

+ * @set_scl: controller specific scl configuration routine. Only required if
+ *   is_gpio_recovery == false
+ * @get_sda: controller specific sda read routine. Only required if
+ *   is_gpio_recovery == false and skip_sda_polling == false.
+ * @get_gpio: called before recover_bus() to get padmux configured for scl line.
+ *   as gpio. Only required if is_gpio_recovery == true.
+ * @put_gpio: called after recover_bus() to get padmux configured for scl line
+ *   as scl. Only required if is_gpio_recovery == true.

Can't we use the pinmux/pinctrl subsystem here? Not knowing too much about it, CCing Linus.

include/linux/i2c.h struct i2c_algorithm { · line 395

Quoted 8 lines

include/linux/i2c.h +8 -0

+ * @scl_gpio: gpio number of the scl line. Only required if is_gpio_recovery ==
+ *   true.
+ * @sda_gpio: gpio number of the sda line. Only required if is_gpio_recovery ==
+ *   true and skip_sda_polling == false.
+ * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
+ *   GPIOF_OUT_INIT_LOW.
+ * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
+ *   GPIOF_OUT_INIT_LOW.

For the last 4, I'd suggest to pass a struct gpio scl_sda_gpio_array[2] or something.

Quoted 26 lines

include/linux/i2c.h +21 -0

+ */
+struct i2c_bus_recovery_info {
+     int (*recover_bus)(struct i2c_adapter *);
+     bool skip_sda_polling;
+     bool is_gpio_recovery;
+     u32 clock_rate_khz;
+     u8 clock_cnt;
+
+     /* scl/sda recovery */
+     void (*set_scl)(struct i2c_adapter *, int val);
+     int (*get_sda)(struct i2c_adapter *);
+
+     /* gpio recovery */
+     int (*get_gpio)(unsigned gpio);
+     int (*put_gpio)(unsigned gpio);
+     u32 scl_gpio;
+     u32 sda_gpio;
+     u32 scl_gpio_flags;
+     u32 sda_gpio_flags;
+};
+
  /*
   * i2c_adapter is the structure used to identify a physical i2c bus along
   * with the access algorithms necessary to access it.

@@ -388,6 +437,9 @@ struct i2c_adapter {
 

include/linux/i2c.h struct i2c_adapter { · line 445

Quoted 8 lines

include/linux/i2c.h +3 -0

       struct mutex userspace_clients_lock;
       struct list_head userspace_clients;
+
+     /* Pass valid pointer if recovery infrastructure is required */
+     struct i2c_bus_recovery_info *bus_recovery_info;
  };
  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 

I'd also think calling the recover function should be in the core and not in all the drivers.

Quoted 9 lines · Wolfram Sang

Quoted 8 lines

+ * @set_scl: controller specific scl configuration routine. Only required if
+ *   is_gpio_recovery == false
+ * @get_sda: controller specific sda read routine. Only required if
+ *   is_gpio_recovery == false and skip_sda_polling == false.
+ * @get_gpio: called before recover_bus() to get padmux configured for scl line.
+ *   as gpio. Only required if is_gpio_recovery == true.
+ * @put_gpio: called after recover_bus() to get padmux configured for scl line
+ *   as scl. Only required if is_gpio_recovery == true.

Quoted 2 lines

Can't we use the pinmux/pinctrl subsystem here? Not knowing too much about it, CCing Linus.

Can be. But probably true only for architectures where pinctrl is defined. Linus?

Quoted 11 lines · viresh kumar

Quoted 9 lines · Wolfram Sang

Quoted 8 lines

+ * @set_scl: controller specific scl configuration routine. Only required if
+ * is_gpio_recovery == false
+ * @get_sda: controller specific sda read routine. Only required if
+ * is_gpio_recovery == false and skip_sda_polling == false.
+ * @get_gpio: called before recover_bus() to get padmux configured for scl line.
+ * as gpio. Only required if is_gpio_recovery == true.
+ * @put_gpio: called after recover_bus() to get padmux configured for scl line
+ * as scl. Only required if is_gpio_recovery == true.

Quoted 5 lines

Quoted 2 lines

Can't we use the pinmux/pinctrl subsystem here? Not knowing too much about it, CCing Linus.

Can be. But probably true only for architectures where pinctrl is defined. Linus?

We *could* do it by defining a standard state for the pinctrl handle in <linux/pinctrl/pinctrl-state.h> (pending in linux-next) like:

#define PINCTRL_STATE_GPIO "gpio"

Or so, then have the core grab that using the struct device * of this pin controller, returing it to PINCTRL_STATE_DEFAULT afterwards.

It seems a bit scary to do that in core code though, because we don't know which state we should return to, should it really be default state?

And what if we're using GPIO bit-banging, then we grab the pins into some state they're already in.

So to me it seems better to do this at driver level using these hooks so the driver is in full control of the pinctrl states.

Stephen, do you agree?

But:

Many platforms I've seen actually have the ability to mux their I2C pins into GPIO and bitbang them, sometimes this seems to be used to work around broken silicon for example. And it seems what is done here is simply some instance of bitbanging (am I right?).

I wonder if it would make sense to model this in the core, so any I2C driver can specify that it also supports bit-banging using GPIO pins A,B and then this can be used for anything, not just for this recovery.

So the above hooks should be possible to use to switch the entire driver over to bitbang mode.

Quoted 10 lines · Viresh Kumar

Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c protocol Rev. 03 section 3.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.

Quoted 5 lines

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.

Signed-off-by Viresh Kumar <[email protected]>

Finally, a review \o/

There are some comments regarding the code, but the most important thing is to get the interface right.

Jean, I don't think this is embedded only, so an additional view might be helpful here.

Hello,

This patchset adds i2c bus recovery infrastructure to i2c adapters as specified in the i2c protocol Rev. 03 section 3.16 titled "Bus clear".

http://www.nxp.com/documents/user_manual/UM10204.pdf

This patch was earlier part of a separate thread:
http://www.spinics.net/lists/linux-i2c/msg07267.html

This patchset adds i2c bus recovery infrastructure to i2c adapters as specified in the i2c protocol Rev. 03 section 3.16 titled "Bus clear".

http://www.nxp.com/documents/user_manual/UM10204.pdf

This patch was earlier part of a separate thread:
http://www.spinics.net/lists/linux-i2c/msg07267.html

Quoted 7 lines · Viresh KUMAR

This patchset adds i2c bus recovery infrastructure to i2c adapters as specified in the i2c protocol Rev. 03 section 3.16 titled "Bus clear".

http://www.nxp.com/documents/user_manual/UM10204.pdf

This patch was earlier part of a separate thread:
http://www.spinics.net/lists/linux-i2c/msg07267.html

Hi Wolfram,

I know that you have been very busy with other stuff, but Can you please apply this patchset if it looks fine to you?

It had been in queue for over 1.5 months now :(