[PATCH V3 0/2] I2C: Add bus recovery infrastructure

[PATCH V3 0/2] I2C: Add bus recovery infrastructure

[Viresh Kumar]

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

Changes since V2:
- gpio flags are now passed from controller drivers
- added support for sda line polling
- Aligned i2c-designware driver with generic recovery support

Viresh Kumar (2):
  i2c/adapter: Add bus recovery infrastructure
  i2c/designware: Provide optional i2c bus recovery function

 drivers/i2c/busses/i2c-designware-core.c    |    7 ++-
 drivers/i2c/busses/i2c-designware-platdrv.c |   24 +++++
 drivers/i2c/i2c-core.c                      |  125 +++++++++++++++++++++++++++
 include/linux/i2c.h                         |   52 +++++++++++
 include/linux/i2c/i2c-designware.h          |   49 +++++++++++
 5 files changed, 256 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/i2c/i2c-designware.h

-- 
1.7.8.110.g4cb5d

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

[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.

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 <viresh.kumar-qxv4g6HH51o@public.gmane.org>
---
 drivers/i2c/i2c-core.c |  125 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    |   52 ++++++++++++++++++++
 2 files changed, 177 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e9c1893..5cd5f83 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -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>
@@ -103,6 +105,88 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 #define i2c_device_uevent	NULL
 #endif	/* CONFIG_HOTPLUG */
 
+/* i2c bus recovery routines */
+static int i2c_gpio_recover_bus(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	unsigned long delay = 1000000;
+	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);
+		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;
+}
+
 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,
+						"!get_sda. skip sda polling\n");
+					bri->skip_sda_polling = true;
+				}
+
+				bri->recover_bus = i2c_scl_recover_bus;
+				dev_info(&adap->dev,
+					"registered for scl bus recovery\n");
+			} else {
+				dev_warn(&adap->dev,
+					"doesn't have valid recovery type\n");
+			}
+		}
+	}
+
+exit_recovery:
 	/* create pre-declared device nodes */
 	if (adap->nr < __i2c_first_dynamic_bus_num)
 		i2c_scan_static_board_info(adap);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 8e25a91..1310d1a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -365,6 +365,55 @@ struct i2c_algorithm {
 	u32 (*functionality) (struct i2c_adapter *);
 };
 
+/**
+ * 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.
+ * @is_gpio_recovery: true, select gpio type else scl type. Only required if
+ *	recover_bus == NULL.
+ * @clock_rate_khz: clock rate of dummy clock in khz. Required for both gpio and
+ *	scl type recovery.
+ * @clock_cnt: count of max clocks to be generated. Required for both gpio and
+ *	scl type recovery.
+ * @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.
+ * @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.
+ */
+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 {
 
 	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)
 
-- 
1.7.8.110.g4cb5d

[PATCH V3 2/2] i2c/designware: Provide i2c bus recovery support

[Viresh Kumar]

Add bus recovery support for designware_i2c controller. It uses generic gpio
based i2c_gpio_recover_bus() routine.

Signed-off-by: Vincenzo Frascino <vincenzo.frascino-qxv4g6HH51o@public.gmane.org>
Signed-off-by: Shiraz Hashim <shiraz.hashim-qxv4g6HH51o@public.gmane.org>
Signed-off-by: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-core.c    |    7 +++-
 drivers/i2c/busses/i2c-designware-platdrv.c |   24 +++++++++++++
 include/linux/i2c/i2c-designware.h          |   49 +++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/i2c/i2c-designware.h

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index df87992..db70fcb 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -525,7 +525,12 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
 	if (ret == 0) {
 		dev_err(dev->dev, "controller timed out\n");
-		i2c_dw_init(dev);
+		if (adap->bus_recovery_info &&
+				adap->bus_recovery_info->recover_bus) {
+			dev_dbg(dev->dev, "try i2c bus recovery\n");
+			adap->bus_recovery_info->recover_bus(adap);
+		}
+
 		ret = -ETIMEDOUT;
 		goto done;
 	} else if (ret < 0)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index f08d024..76bf108 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -29,6 +29,7 @@
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/i2c/i2c-designware.h>
 #include <linux/clk.h>
 #include <linux/errno.h>
 #include <linux/sched.h>
@@ -45,6 +46,11 @@ static struct i2c_algorithm i2c_dw_algo = {
 	.master_xfer	= i2c_dw_xfer,
 	.functionality	= i2c_dw_func,
 };
+
+static struct i2c_bus_recovery_info dw_recovery_info = {
+	.is_gpio_recovery = true,
+};
+
 static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 {
 	return clk_get_rate(dev->clk)/1000;
@@ -55,6 +61,7 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	struct dw_i2c_dev *dev;
 	struct i2c_adapter *adap;
 	struct resource *mem, *ioarea;
+	struct i2c_dw_pdata *pdata;
 	int irq, r;
 
 	/* NOTE: driver uses the static register mapping */
@@ -141,6 +148,23 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	adap->dev.parent = &pdev->dev;
 	adap->dev.of_node = pdev->dev.of_node;
 
+	/* Bus recovery support */
+	pdata = dev_get_platdata(&pdev->dev);
+	if (pdata) {
+		dw_recovery_info.get_gpio = pdata->get_gpio;
+		dw_recovery_info.put_gpio = pdata->put_gpio;
+		dw_recovery_info.scl_gpio = pdata->scl_gpio;
+		dw_recovery_info.scl_gpio_flags = pdata->scl_gpio_flags;
+		dw_recovery_info.clock_rate_khz = clk_get_rate(dev->clk) / 1000;
+
+		if (!pdata->skip_sda_polling) {
+			dw_recovery_info.sda_gpio = pdata->sda_gpio;
+			dw_recovery_info.sda_gpio_flags = pdata->sda_gpio_flags;
+		}
+
+		adap->bus_recovery_info = &dw_recovery_info;
+	}
+
 	adap->nr = pdev->id;
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
diff --git a/include/linux/i2c/i2c-designware.h b/include/linux/i2c/i2c-designware.h
new file mode 100644
index 0000000..341bd4c
--- /dev/null
+++ b/include/linux/i2c/i2c-designware.h
@@ -0,0 +1,49 @@
+/*
+ * Synopsys DesignWare I2C adapter driver's platform data
+ *
+ * Copyright (C) 2012 ST Microelectronics.
+ * Author: Vincenzo Frascino <vincenzo.frascino-qxv4g6HH51o@public.gmane.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#ifndef I2C_DESIGNWARE_H
+#define I2C_DESIGNWARE_H
+
+#include <linux/platform_device.h>
+
+/*
+ * struct i2c_dw_pdata - Designware I2c platform data
+ * @scl_gpio: gpio number of the scl line.
+ * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
+ *	GPIOF_OUT_INIT_LOW.
+ * @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
+ * @skip_sda_polling: if true, bus recovery will not poll sda line to check if
+ *	it became high or not. Below are required only if this is false.
+ * @sda_gpio: gpio number of the sda line.
+ * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
+ *	GPIOF_OUT_INIT_LOW.
+ */
+struct i2c_dw_pdata {
+	int scl_gpio;
+	int scl_gpio_flags;
+	int (*get_gpio)(unsigned gpio);
+	int (*put_gpio)(unsigned gpio);
+
+	/* sda polling specific */
+	bool skip_sda_polling;
+	int sda_gpio;
+	int sda_gpio_flags;
+};
+
+#endif /* I2C_DESIGNWARE_H */
-- 
1.7.8.110.g4cb5d

Re: [PATCH V3 0/2] I2C: Add bus recovery infrastructure

[Viresh Kumar]

On 3/2/2012 11:53 AM, Viresh KUMAR wrote:
> 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
> 

Hi,

Did somebody applied these patches? Or any more comments?

--
viresh

Re: [PATCH V3 0/2] I2C: Add bus recovery infrastructure

[Viresh Kumar]

On 3/2/2012 11:53 AM, Viresh KUMAR wrote:
> 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 :(

-- 
viresh

Re: [PATCH V3 0/2] I2C: Add bus recovery infrastructure

[Rajeev kumar]

Hello Wolfram,

On 4/17/2012 2:35 PM, Viresh KUMAR wrote:
> On 3/2/2012 11:53 AM, Viresh KUMAR wrote:
>> 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 :(
>

Tested By: Rajeev Kumar <rajeev-dlh.kumar-qxv4g6HH51o@public.gmane.org>

Rgds
Rajeev

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 10372 bytes --]

On Fri, Mar 02, 2012 at 11:53:42AM +0530, Viresh Kumar wrote:
> 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 <viresh.kumar-qxv4g6HH51o@public.gmane.org>

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.

Linus, can you have a look wearing your pinmux/pinctrl hat? This
functionality may need to switch SDA/SCL pins to GPIOs so they can be
manually clocked. I'd think this is a prime example for pinmux, but am
unsure if we should rely on it only. Is it enforced for active
platforms?

> ---
>  drivers/i2c/i2c-core.c |  125 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h    |   52 ++++++++++++++++++++
>  2 files changed, 177 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index e9c1893..5cd5f83 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -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>
> @@ -103,6 +105,88 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
>  #define i2c_device_uevent	NULL
>  #endif	/* CONFIG_HOTPLUG */
>  
> +/* 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'.

> +	unsigned long delay = 1000000;

udelay?

> +	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);
> +		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()

> +
>  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,
> +						"!get_sda. skip sda polling\n");
> +					bri->skip_sda_polling = true;
> +				}
> +
> +				bri->recover_bus = i2c_scl_recover_bus;
> +				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.

> +			}
> +		}
> +	}
> +
> +exit_recovery:
>  	/* create pre-declared device nodes */
>  	if (adap->nr < __i2c_first_dynamic_bus_num)
>  		i2c_scan_static_board_info(adap);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 8e25a91..1310d1a 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -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.

> +/**
> + * 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?

> + * @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)?

> + * @clock_rate_khz: clock rate of dummy clock in khz. Required for both gpio and
> + *	scl type recovery.

Hrmm, current bus speed should really be in i2c_adapter somewhen. Not
your problem, though (unless you want it to be :)) Jean, what do you
think?

> + * @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)?

> + * @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.

> + * @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.

> + */
> +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 {
>  
>  	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.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[viresh kumar]

On Mon, Apr 23, 2012 at 6:26 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Fri, Mar 02, 2012 at 11:53:42AM +0530, Viresh Kumar wrote:
> Finally, a review \o/

:)

>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> +/* 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'.

Sure.

>> +     unsigned long delay = 1000000;
>
> udelay?

No. It's ndelay. Will rename it.

>> +     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);
>> +             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()

gpio calls can't be done at init. As we have to request and free them
again and again. So would be required to keep two calls.

Not sure, if i understand your point here.

>>  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,
>> +                                             "!get_sda. skip sda polling\n");
>> +                                     bri->skip_sda_polling = true;
>> +                             }
>> +
>> +                             bri->recover_bus = i2c_scl_recover_bus;
>> +                             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.

Hmm. Actually this is a error case. User wanted to have recovery infra,
but didn't mention the method of recovery: via gpio or sda/scl. Don't
know if we could make it dev_err() instead.

>> +                     }
>> +             }
>> +     }
>> +
>> +exit_recovery:
>>       /* create pre-declared device nodes */
>>       if (adap->nr < __i2c_first_dynamic_bus_num)
>>               i2c_scan_static_board_info(adap);
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 8e25a91..1310d1a 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -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.

Sure.

>> +/**
>> + * 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?

Can be done with get_sda(), but have doubt for GPIO. How to check if sda polling
is required in GPIO case. Obviously by checking its value. Zero is a
valid value.
And -1 doesn't look good.

>> + * @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)?

Can be done based on value of set_scl, but not on GPIO because of earlier
mentioned reasons.

>> + * @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)?

Don't know. Somebody, probably Tegra guys, asked for it during earlier
versions of this
patch. IIRC, they needed 10.

>> + * @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.

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

>> + * @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.

Ok.

>> + */
>> +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 {
>>
>>       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.

Sounds good. Based on, if timeout is returned by controller or not.

--
Viresh

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

[Linus Walleij]

On Mon, Apr 23, 2012 at 3:55 PM, viresh kumar <viresh.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Mon, Apr 23, 2012 at 6:26 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:

>>> + * @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.
>
> 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.

Yours,
Linus Walleij

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

[Jean Delvare]

Hi Wolfram,

On Mon, 23 Apr 2012 14:56:31 +0200, Wolfram Sang wrote:
> On Fri, Mar 02, 2012 at 11:53:42AM +0530, Viresh Kumar wrote:
> > 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 <viresh.kumar-qxv4g6HH51o@public.gmane.org>
> 
> 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.

I'm afraid I won't have any time for this in a near future. If/when you
are happy with the code, feel free to merge it.

Thanks,
-- 
Jean Delvare

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

[Stephen Warren]

On 04/24/2012 06:08 AM, Linus Walleij wrote:
> On Mon, Apr 23, 2012 at 3:55 PM, viresh kumar <viresh.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Mon, Apr 23, 2012 at 6:26 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> 
>>>> + * @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.
>>
>> 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?

Yes: I don't think pinctrl alone is the correct way here.

The reason here is there's no guarantee that pinctrl is able to interact
with GPIOs in any way.

On some systems, there may be dedicated pins for (at least some) GPIOs,
so there may be no pinctrl driver that affects them (that'd cover the
I2C-over-GPIO-bit-banging case).

We've defined that gpio_direction_input/output() must set up any pinmux
as required to enable usage of those pins as GPIOs. However, we have not
defined that the pinctrl driver must expose a (pinctrl, not C) function
that allows configuring a pin/group as GPIOs. Hence, again, there may be
no way for pinctrl to switch pins into GPIO mode even when they could be
used as GPIOs.

In particular, on Tegra, GPIO vs. pinctrl is controlled by the GPIO HW
and driver, not the pinctrl HW or driver, and so the Tegra pinctrl
driver doesn't expose any GPIO function. Rather, the GPIO driver
programs the relevant HW bits inside gpio_direction_input/output(), and
simply notifies the pinctrl core that it has, rather than having the
pinctrl driver program any HW.

That said, other HW might have multiple GPIOs that can be routed to a
single pin, and might allow configuration of which via pinctrl.

I guess what we need to do is have I2C drivers provide the following
information to the I2C core:

a) GPIO ID for SCL and SDA
b) Optional pinctrl state for GPIO-based recovery, and the pinctrl state
to return to afterwards

The I2C core would have to do something like:

if (state_bitbang)
    pinctrl_select_state(state_bitbang)
gpio_request(gpio_scl)
gpio_request(gpio_sda)
// do recovery
gpio_free(gpio_sda)
gpio_free(gpio_scl)
if (state_bitbang_done)
    pinctrl_select_state(state_bitbang_done)

Noting that perhaps state_bitbang_done could be changed dynamically by
the driver for internal reasons, and hence shouldn't be cached by the
I2C core, but rather read from the I2C driver struct each time it's used.

> 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?).

Finally, in order to cover any other special cases, I guess I2C drivers
need to have a top-level entry-point to perform bus recovery, so they
can do any HW programming of the I2C controller they need, then call
into the I2C core recovery utility code I outlined above. That should
allow complete flexibility.

> 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.