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

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

Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
V7->V8:
- Clk rate fixed to 100KHz
- Check SCL line to see if it is LOW due to some faults
- removed last use of unlikely() in earlier patch
- Enhanced comment over skip_sda_polling

V6->V7:
- Lots of review comments from Wolfram incorporated
- get[put]_gpio updated to more generic [un]prepare_recovery with return type as
  void.
- prototypes of generic recovery routines are exposed to controller driver and
  they are now required to pass recover_bus()
- gpio flags removed from recovery info
- default clock rate is 100 KHz if not passed by controller driver
- clk count is 9, fixed.

V5->V6:
- Removed sda_gpio_flags
- Make scl_gpio_flags as GPIOF_OPEN_DRAIN | GPIOF_OUT_INIT_HIGH by default
- update bri->set_scl and bri->get_sda for gpio recovery case in i2c core
- Guaranteed to generate 9 falling-rising edges for bus recovery

V4->V5:
- section name corrected to 3.1.16
- merged gpio and non-gpio recovery routines to remove code redundancy
- Changed types of gpio and gpio-flags to unsigned and unsigned long
- Checking return value of get_gpio() now
- using DIV_ROUND_UP for calculating delay, to get more correct value

 drivers/i2c/i2c-core.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    |  50 +++++++++++++++++
 2 files changed, 198 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a7edf98..9b6a1a6 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -27,7 +27,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>
@@ -104,6 +106,122 @@ 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 */
+#define RECOVERY_CLK_CNT	9
+#define DEFAULT_CLK_RATE	100	/* KHz */
+/* 10^6/KHz for delay in ns */
+unsigned long clk_delay = DIV_ROUND_UP(1000000, DEFAULT_CLK_RATE * 2);
+
+static int get_sda_gpio_value(struct i2c_adapter *adap)
+{
+	return gpio_get_value(adap->bus_recovery_info->sda_gpio);
+}
+
+static int get_scl_gpio_value(struct i2c_adapter *adap)
+{
+	return gpio_get_value(adap->bus_recovery_info->scl_gpio);
+}
+
+static void set_scl_gpio_value(struct i2c_adapter *adap, int val)
+{
+	gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
+}
+
+static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	struct device *dev = &adap->dev;
+	int ret = 0;
+
+	ret = gpio_request_one(bri->scl_gpio, GPIOF_OPEN_DRAIN |
+			GPIOF_OUT_INIT_HIGH, "i2c-scl");
+	if (ret) {
+		dev_warn(dev, "gpio request fail: %d\n", bri->scl_gpio);
+		return ret;
+	}
+
+	if (!bri->skip_sda_polling) {
+		if (gpio_request_one(bri->sda_gpio, GPIOF_IN, "i2c-sda")) {
+			/* work without sda polling */
+			dev_warn(dev, "can't get sda: %d. Skip sda polling\n",
+					bri->sda_gpio);
+			bri->skip_sda_polling = true;
+		}
+	}
+
+	return ret;
+}
+
+static void i2c_put_gpios_for_recovery(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+
+	if (!bri->skip_sda_polling)
+		gpio_free(bri->sda_gpio);
+
+	gpio_free(bri->scl_gpio);
+}
+
+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);
+
+	/* SCL shouldn't be low here */
+	if (!bri->get_scl(adap)) {
+		dev_err(&adap->dev, "SCL is stuck Low, exiting recovery.\n");
+		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)
+			/* 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");
+				goto unprepare;
+			}
+
+			if (bri->get_sda(adap))
+				break;
+	}
+
+unprepare:
+	if (bri->unprepare_recovery)
+		bri->unprepare_recovery(bri);
+
+	return 0;
+}
+
+int i2c_generic_scl_recovery(struct i2c_adapter *adap)
+{
+	adap->bus_recovery_info->set_scl(adap, 1);
+	return i2c_generic_recovery(adap);
+}
+
+int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
+{
+	int ret;
+
+	ret = i2c_get_gpios_for_recovery(adap);
+	if (ret)
+		return ret;
+
+	ret = i2c_generic_recovery(adap);
+	i2c_put_gpios_for_recovery(adap);
+
+	return ret;
+}
+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -896,6 +1014,36 @@ 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_err(&adap->dev, "Invalid recover_bus(), skip recovery \n");
+			adap->bus_recovery_info = NULL;
+			goto exit_recovery;
+		}
+
+		/* GPIO recovery */
+		if (bri->recover_bus == i2c_generic_gpio_recovery) {
+			bri->get_scl = get_scl_gpio_value;
+			bri->set_scl = set_scl_gpio_value;
+
+			if (!bri->skip_sda_polling)
+				bri->get_sda = get_sda_gpio_value;
+		} 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;
+			}
+		} 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 800de22..165282f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -368,6 +368,54 @@ 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
+ *	i2c_generic_scl_recovery() or i2c_generic_gpio_recovery().
+ * @skip_sda_polling: if true, bus recovery will not poll sda line to check if
+ *	it became high or not. Platforms/controllers which don't have
+ *	configuration registers to control sda line must set this. Only required
+ *	if recover_bus == NULL.
+ * @get_sda: This gets current value of sda line. For GPIO recovery
+ *	get_sda_gpio_value() is used here otherwise controller specific routine
+ *	must be passed.
+ * @get_scl: This gets current value of scl line. For GPIO recovery
+ *	get_scl_gpio_value() is used here otherwise controller specific routine
+ *	must be passed.
+ * @set_scl: This sets/clears scl line. For GPIO recovery set_scl_gpio_value()
+ *	is used here otherwise controller specific routine must be passed.
+ * @prepare_recovery: This will be called before starting recovery. Platform may
+ *	configure padmux here for sda/scl line or something else they want.
+ * @unprepare_recovery: This will be called after completing recovery. Platform
+ *	may configure padmux here for sda/scl line or something else they want.
+ * @scl_gpio: gpio number of the scl line. Only required for GPIO recovery.
+ * @sda_gpio: gpio number of the sda line. Only required for GPIO recovery.
+ */
+struct i2c_bus_recovery_info {
+	int (*recover_bus)(struct i2c_adapter *);
+	bool skip_sda_polling;
+
+	/*
+	 * Fn pointers for recovery, will point either to:
+	 * - set_scl_gpio_value and get_[scl]sda_gpio_value for gpio recovery
+	 * - Controller specific routines, otherwise
+	 */
+	int (*get_sda)(struct i2c_adapter *);
+	int (*get_scl)(struct i2c_adapter *);
+	void (*set_scl)(struct i2c_adapter *, int val);
+
+	void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
+	void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
+
+	/* gpio recovery */
+	unsigned scl_gpio;
+	unsigned sda_gpio;
+};
+
+/* Generic recovery routines */
+int i2c_generic_gpio_recovery(struct i2c_adapter *adap);
+int i2c_generic_scl_recovery(struct i2c_adapter *adap);
+
 /*
  * i2c_adapter is the structure used to identify a physical i2c bus along
  * with the access algorithms necessary to access it.
@@ -391,6 +439,8 @@ struct i2c_adapter {
 
 	struct mutex userspace_clients_lock;
 	struct list_head userspace_clients;
+
+	struct i2c_bus_recovery_info *bus_recovery_info;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH V8 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. Platforms need to pass struct
i2c_bus_recovery_info as platform data designware I2C controller.

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
V7->V8:
- Removed setting clock_rate_khz.

 drivers/i2c/busses/i2c-designware-core.c    |  6 +++++-
 drivers/i2c/busses/i2c-designware-platdrv.c | 17 +++++++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index cbba7db..24feaaf 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -538,7 +538,11 @@ 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) {
+			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 0506fef..8c44eb9 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -55,6 +55,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_bus_recovery_info *recovery_info;
 	int irq, r;
 
 	/* NOTE: driver uses the static register mapping */
@@ -141,17 +142,27 @@ 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 */
+	recovery_info = dev_get_platdata(&pdev->dev);
+	if (recovery_info) {
+		recovery_info->recover_bus = i2c_generic_gpio_recovery;
+		adap->bus_recovery_info = recovery_info;
+	} else {
+		adap->bus_recovery_info = NULL;
+	}
+
 	adap->nr = pdev->id;
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
 		dev_err(&pdev->dev, "failure adding adapter\n");
-		goto err_free_irq;
+		goto err_free_recovery_info;
 	}
 	of_i2c_register_devices(adap);
 
 	return 0;
 
-err_free_irq:
+err_free_recovery_info:
+	kfree(recovery_info);
 	free_irq(dev->irq, dev);
 err_iounmap:
 	iounmap(dev->base);
@@ -174,6 +185,8 @@ static int __devexit dw_i2c_remove(struct platform_device *pdev)
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 	struct resource *mem;
 
+	kfree(dev->adapter.bus_recovery_info);
+
 	platform_set_drvdata(pdev, NULL);
 	i2c_del_adapter(&dev->adapter);
 	put_device(&pdev->dev);
-- 
1.7.12.rc2.18.g61b472e

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

[Wolfram Sang]

On Mon, Dec 03, 2012 at 08:24:05AM +0530, Viresh Kumar wrote:
> Add bus recovery support for designware_i2c controller. It uses generic gpio
> based i2c_gpio_recover_bus() routine. Platforms need to pass struct
> i2c_bus_recovery_info as platform data designware I2C controller.
> 
> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> V7->V8:
> - Removed setting clock_rate_khz.
> 
>  drivers/i2c/busses/i2c-designware-core.c    |  6 +++++-
>  drivers/i2c/busses/i2c-designware-platdrv.c | 17 +++++++++++++++--
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index cbba7db..24feaaf 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -538,7 +538,11 @@ 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) {
> +			dev_dbg(dev->dev, "try i2c bus recovery\n");
> +			adap->bus_recovery_info->recover_bus(adap);
> +		}
> +

I think we need something like i2c_recover_bus in the core which does
the above and also returns the return code from recover_bus. If there is
no recover_bus it should return EOPNOTSUPP.

Then the driver can do

	ret = i2c_recover_bus(adap);
	if (ret < 0)
		i2c_dw_init();

If not calling i2c_dw_init, you will probably cause a regression.

>  		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 0506fef..8c44eb9 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -55,6 +55,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_bus_recovery_info *recovery_info;
>  	int irq, r;
>  
>  	/* NOTE: driver uses the static register mapping */
> @@ -141,17 +142,27 @@ 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 */
> +	recovery_info = dev_get_platdata(&pdev->dev);
> +	if (recovery_info) {
> +		recovery_info->recover_bus = i2c_generic_gpio_recovery;
> +		adap->bus_recovery_info = recovery_info;
> +	} else {
> +		adap->bus_recovery_info = NULL;
> +	}
> +

Please post the platform patch next time, too. Then I can get a better
understanding...

>  	adap->nr = pdev->id;
>  	r = i2c_add_numbered_adapter(adap);
>  	if (r) {
>  		dev_err(&pdev->dev, "failure adding adapter\n");
> -		goto err_free_irq;
> +		goto err_free_recovery_info;
>  	}
>  	of_i2c_register_devices(adap);
>  
>  	return 0;
>  
> -err_free_irq:
> +err_free_recovery_info:
> +	kfree(recovery_info);

... because I am wondering about the kfree here.

>  	free_irq(dev->irq, dev);
>  err_iounmap:
>  	iounmap(dev->base);
> @@ -174,6 +185,8 @@ static int __devexit dw_i2c_remove(struct platform_device *pdev)
>  	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
>  	struct resource *mem;
>  
> +	kfree(dev->adapter.bus_recovery_info);
> +
>  	platform_set_drvdata(pdev, NULL);
>  	i2c_del_adapter(&dev->adapter);
>  	put_device(&pdev->dev);
> -- 
> 1.7.12.rc2.18.g61b472e
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Viresh Kumar]

On 24 January 2013 12:54, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Mon, Dec 03, 2012 at 08:24:05AM +0530, Viresh Kumar wrote:
>> @@ -538,7 +538,11 @@ 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) {
>> +                     dev_dbg(dev->dev, "try i2c bus recovery\n");
>> +                     adap->bus_recovery_info->recover_bus(adap);
>> +             }
>> +
>
> I think we need something like i2c_recover_bus in the core which does
> the above and also returns the return code from recover_bus. If there is
> no recover_bus it should return EOPNOTSUPP.
>
> Then the driver can do
>
>         ret = i2c_recover_bus(adap);
>         if (ret < 0)
>                 i2c_dw_init();
>
> If not calling i2c_dw_init, you will probably cause a regression.

Fair enough.

>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>> +err_free_recovery_info:
>> +     kfree(recovery_info);

Leftover of earlier versions, my mistake :(

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

[Viresh Kumar]

On 3 December 2012 08:24, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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.
>
> Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> V7->V8:
> - Clk rate fixed to 100KHz
> - Check SCL line to see if it is LOW due to some faults
> - removed last use of unlikely() in earlier patch
> - Enhanced comment over skip_sda_polling

As merge window is shifted for few more days, i am trying another
time to get this in 3.8 :)

@Paul/Wolfram: Any more comments ?

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

[Paul Carpenter]

Viresh Kumar wrote:
> On 3 December 2012 08:24, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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.
>>
...
> @Paul/Wolfram: Any more comments ?

Nothing more from me. I will keep quiet now :-)


-- 
Paul Carpenter          | paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org
<http://www.pcserviceselectronics.co.uk/>    PC Services
<http://www.pcserviceselectronics.co.uk/fonts/> Timing Diagram Font
<http://www.gnuh8.org.uk/>  GNU H8 - compiler & Renesas H8/H8S/H8 Tiny
<http://www.badweb.org.uk/> For those web sites you hate

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

[Viresh Kumar]

On 6 December 2012 21:28, Paul Carpenter
<paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org> wrote:
> Nothing more from me. I will keep quiet now :-)

As you have invested some effort in reviewing it, you want to give your
Reviewed-by?

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

[Paul Carpenter]

Viresh Kumar wrote:
> On 3 December 2012 08:24, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>> V7->V8:
>> - Clk rate fixed to 100KHz
>> - Check SCL line to see if it is LOW due to some faults
>> - removed last use of unlikely() in earlier patch
>> - Enhanced comment over skip_sda_polling
> 
> As merge window is shifted for few more days, i am trying another
> time to get this in 3.8 :)
> 
> @Paul/Wolfram: Any more comments ?

OK by me

Reviewed-by: Paul Carpenter <paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org>

-- 
Paul Carpenter          | paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org
<http://www.pcserviceselectronics.co.uk/>    PC Services
<http://www.pcserviceselectronics.co.uk/fonts/> Timing Diagram Font
<http://www.gnuh8.org.uk/>  GNU H8 - compiler & Renesas H8/H8S/H8 Tiny
<http://www.badweb.org.uk/> For those web sites you hate

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

[Viresh Kumar]

On 7 December 2012 02:00, Paul Carpenter
<paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org> wrote:
> OK by me
>
> Reviewed-by: Paul Carpenter <paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org>

Are you taking it for 3.8 now?

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

[Wolfram Sang]

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

On Tue, Dec 11, 2012 at 10:22:37AM +0530, Viresh Kumar wrote:
> On 7 December 2012 02:00, Paul Carpenter
> <paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org> wrote:
> > OK by me
> >
> > Reviewed-by: Paul Carpenter <paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org>
> 
> Are you taking it for 3.8 now?

Since I missed the things Paul luckily spotted, I think it might make
sense to actually use the framework myself to get me a better feeling
that I have not missed anything else. So, I am going to add support for
another I2C master controller to your framework and see what will happen
when I actually use it. It will be probably the mxs controller, I
collected the hardware for that already...

-- 
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 V8 1/2] i2c/adapter: Add bus recovery infrastructure

[Paul Carpenter]

Wolfram Sang wrote:
> On Tue, Dec 11, 2012 at 10:22:37AM +0530, Viresh Kumar wrote:
>> On 7 December 2012 02:00, Paul Carpenter
>> <paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org> wrote:
>>> OK by me
>>>
>>> Reviewed-by: Paul Carpenter <paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org>
>> Are you taking it for 3.8 now?
> 
> Since I missed the things Paul luckily spotted, I think it might make
> sense to actually use the framework myself to get me a better feeling
> that I have not missed anything else. So, I am going to add support for
> another I2C master controller to your framework and see what will happen
> when I actually use it. It will be probably the mxs controller, I
> collected the hardware for that already...

Mine was straight code review, not had a spare 5 minutes to do anything
else, time of year and customer panics, let alone having the builders
in. If anyone has had that they will know the nightmare I mean.

-- 
Paul Carpenter          | paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org
<http://www.pcserviceselectronics.co.uk/>    PC Services
<http://www.pcserviceselectronics.co.uk/fonts/> Timing Diagram Font
<http://www.gnuh8.org.uk/>  GNU H8 - compiler & Renesas H8/H8S/H8 Tiny
<http://www.badweb.org.uk/> For those web sites you hate

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

[Viresh Kumar]

On 19 December 2012 05:30, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> Since I missed the things Paul luckily spotted, I think it might make
> sense to actually use the framework myself to get me a better feeling
> that I have not missed anything else. So, I am going to add support for
> another I2C master controller to your framework and see what will happen
> when I actually use it. It will be probably the mxs controller, I
> collected the hardware for that already...

Nothing could be better than that :)

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

[Viresh Kumar]

On 20 December 2012 14:47, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 19 December 2012 05:30, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>> Since I missed the things Paul luckily spotted, I think it might make
>> sense to actually use the framework myself to get me a better feeling
>> that I have not missed anything else. So, I am going to add support for
>> another I2C master controller to your framework and see what will happen
>> when I actually use it. It will be probably the mxs controller, I
>> collected the hardware for that already...
>
> Nothing could be better than that :)

Hi Wolfram,

HNY '13.

You got some chance to test this stuff ?

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

[Viresh Kumar]

On 7 January 2013 16:02, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 20 December 2012 14:47, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On 19 December 2012 05:30, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>>> Since I missed the things Paul luckily spotted, I think it might make
>>> sense to actually use the framework myself to get me a better feeling
>>> that I have not missed anything else. So, I am going to add support for
>>> another I2C master controller to your framework and see what will happen
>>> when I actually use it. It will be probably the mxs controller, I
>>> collected the hardware for that already...
>>
>> Nothing could be better than that :)
>
> Hi Wolfram,
>
> HNY '13.
>
> You got some chance to test this stuff ?

Ping!!

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

[Wolfram Sang]

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?

> 
> Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> V7->V8:
> - Clk rate fixed to 100KHz
> - Check SCL line to see if it is LOW due to some faults
> - removed last use of unlikely() in earlier patch
> - Enhanced comment over skip_sda_polling

I am still not sure why we need it. I prefixed comments which sketch a
way to get rid of it with an '*' at the beginning of the line.

> 
> V6->V7:
> - Lots of review comments from Wolfram incorporated
> - get[put]_gpio updated to more generic [un]prepare_recovery with return type as
>   void.
> - prototypes of generic recovery routines are exposed to controller driver and
>   they are now required to pass recover_bus()
> - gpio flags removed from recovery info
> - default clock rate is 100 KHz if not passed by controller driver
> - clk count is 9, fixed.
> 
> V5->V6:
> - Removed sda_gpio_flags
> - Make scl_gpio_flags as GPIOF_OPEN_DRAIN | GPIOF_OUT_INIT_HIGH by default
> - update bri->set_scl and bri->get_sda for gpio recovery case in i2c core
> - Guaranteed to generate 9 falling-rising edges for bus recovery
> 
> V4->V5:
> - section name corrected to 3.1.16
> - merged gpio and non-gpio recovery routines to remove code redundancy
> - Changed types of gpio and gpio-flags to unsigned and unsigned long
> - Checking return value of get_gpio() now
> - using DIV_ROUND_UP for calculating delay, to get more correct value
> 
>  drivers/i2c/i2c-core.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h    |  50 +++++++++++++++++
>  2 files changed, 198 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index a7edf98..9b6a1a6 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -27,7 +27,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>
> @@ -104,6 +106,122 @@ 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 */
> +#define RECOVERY_CLK_CNT	9
> +#define DEFAULT_CLK_RATE	100	/* KHz */
> +/* 10^6/KHz for delay in ns */
> +unsigned long clk_delay = DIV_ROUND_UP(1000000, DEFAULT_CLK_RATE * 2);

no global variable. should go into the function needing it.

> +
> +static int get_sda_gpio_value(struct i2c_adapter *adap)
> +{
> +	return gpio_get_value(adap->bus_recovery_info->sda_gpio);
> +}
> +
> +static int get_scl_gpio_value(struct i2c_adapter *adap)
> +{
> +	return gpio_get_value(adap->bus_recovery_info->scl_gpio);
> +}
> +
> +static void set_scl_gpio_value(struct i2c_adapter *adap, int val)
> +{
> +	gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
> +}
> +
> +static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +	struct device *dev = &adap->dev;
> +	int ret = 0;
> +
> +	ret = gpio_request_one(bri->scl_gpio, GPIOF_OPEN_DRAIN |
> +			GPIOF_OUT_INIT_HIGH, "i2c-scl");
> +	if (ret) {
> +		dev_warn(dev, "gpio request fail: %d\n", bri->scl_gpio);
> +		return ret;
> +	}
> +
> +	if (!bri->skip_sda_polling) {

* if (is_valid_gpio(bri->sda_gpio)) ...

> +		if (gpio_request_one(bri->sda_gpio, GPIOF_IN, "i2c-sda")) {
> +			/* work without sda polling */
> +			dev_warn(dev, "can't get sda: %d. Skip sda polling\n",
> +					bri->sda_gpio);
> +			bri->skip_sda_polling = true;

* Instead of above line:
	bri->get_sda = get_sda_gpio_value;

> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void i2c_put_gpios_for_recovery(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +
> +	if (!bri->skip_sda_polling)

* if (is_valid_gpio(bri->sda_gpio)) ...

> +		gpio_free(bri->sda_gpio);
> +
> +	gpio_free(bri->scl_gpio);
> +}
> +
> +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?

> +				goto unprepare;
> +			}
> +
> +			if (bri->get_sda(adap))
> +				break;

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

> +	}
> +
> +unprepare:
> +	if (bri->unprepare_recovery)
> +		bri->unprepare_recovery(bri);
> +
> +	return 0;
> +}
> +
> +int i2c_generic_scl_recovery(struct i2c_adapter *adap)
> +{
> +	adap->bus_recovery_info->set_scl(adap, 1);
> +	return i2c_generic_recovery(adap);
> +}
> +
> +int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
> +{
> +	int ret;
> +
> +	ret = i2c_get_gpios_for_recovery(adap);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_generic_recovery(adap);
> +	i2c_put_gpios_for_recovery(adap);
> +
> +	return ret;
> +}
> +
>  static int i2c_device_probe(struct device *dev)
>  {
>  	struct i2c_client	*client = i2c_verify_client(dev);
> @@ -896,6 +1014,36 @@ 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_err(&adap->dev, "Invalid recover_bus(), skip recovery \n");
> +			adap->bus_recovery_info = NULL;
> +			goto exit_recovery;
> +		}
> +
> +		/* GPIO recovery */
> +		if (bri->recover_bus == i2c_generic_gpio_recovery) {
> +			bri->get_scl = get_scl_gpio_value;
> +			bri->set_scl = set_scl_gpio_value;
> +
> +			if (!bri->skip_sda_polling)
> +				bri->get_sda = get_sda_gpio_value;

* was moved upwards

> +		} else if (bri->set_scl) {
> +			if (!bri->skip_sda_polling && !bri->get_sda) {

* if (!bri->get_sda) ...

> +				dev_warn(&adap->dev,
> +						"!get_sda. skip sda polling\n");

That message needs improvement ("!get_sda").

> +				bri->skip_sda_polling = true;
> +			}
> +		} 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 800de22..165282f 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -368,6 +368,54 @@ struct i2c_algorithm {
>  	u32 (*functionality) (struct i2c_adapter *);
>  };
>  
> +/**
> + * struct i2c_bus_recovery_info - I2c bus recovery information

I2C

> + * @recover_bus: Recover routine. Either pass driver's recover_bus() routine, or
> + *	i2c_generic_scl_recovery() or i2c_generic_gpio_recovery().
> + * @skip_sda_polling: if true, bus recovery will not poll sda line to check if
> + *	it became high or not. Platforms/controllers which don't have
> + *	configuration registers to control sda line must set this. Only required
> + *	if recover_bus == NULL.

Last sentence is moot, because recover_bus is not allowed to be NULL.

* or can be removed

> + * @get_sda: This gets current value of sda line. For GPIO recovery
> + *	get_sda_gpio_value() is used here otherwise controller specific routine
> + *	must be passed.

* "@get_sda: This gets current value of sda line. For generic SCL recovery,
optional. For generic GPIO recovery, used internally if SDA GPIO is
set."

This block could be moved below get/set_scl.

> + * @get_scl: This gets current value of scl line. For GPIO recovery
> + *	get_scl_gpio_value() is used here otherwise controller specific routine
> + *	must be passed.

"@get_scl: This gets current value of scl line. For generic SCL
recovery, mandatory. For generic GPIO recovery, used internally."

> + * @set_scl: This sets/clears scl line. For GPIO recovery set_scl_gpio_value()
> + *	is used here otherwise controller specific routine must be passed.

Like above.

> + * @prepare_recovery: This will be called before starting recovery. Platform may
> + *	configure padmux here for sda/scl line or something else they want.
> + * @unprepare_recovery: This will be called after completing recovery. Platform
> + *	may configure padmux here for sda/scl line or something else they want.
> + * @scl_gpio: gpio number of the scl line. Only required for GPIO recovery.
> + * @sda_gpio: gpio number of the sda line. Only required for GPIO recovery.
> + */
> +struct i2c_bus_recovery_info {
> +	int (*recover_bus)(struct i2c_adapter *);
> +	bool skip_sda_polling;
> +
> +	/*
> +	 * Fn pointers for recovery, will point either to:
> +	 * - set_scl_gpio_value and get_[scl]sda_gpio_value for gpio recovery
> +	 * - Controller specific routines, otherwise
> +	 */

This comment can be dropped, I think.

> +	int (*get_sda)(struct i2c_adapter *);
> +	int (*get_scl)(struct i2c_adapter *);
> +	void (*set_scl)(struct i2c_adapter *, int val);
> +
> +	void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
> +	void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
> +
> +	/* gpio recovery */
> +	unsigned scl_gpio;
> +	unsigned sda_gpio;
> +};
> +
> +/* Generic recovery routines */
> +int i2c_generic_gpio_recovery(struct i2c_adapter *adap);
> +int i2c_generic_scl_recovery(struct i2c_adapter *adap);
> +
>  /*
>   * i2c_adapter is the structure used to identify a physical i2c bus along
>   * with the access algorithms necessary to access it.
> @@ -391,6 +439,8 @@ struct i2c_adapter {
>  
>  	struct mutex userspace_clients_lock;
>  	struct list_head userspace_clients;
> +
> +	struct i2c_bus_recovery_info *bus_recovery_info;
>  };
>  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)

Thanks,

   Wolfram

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

[Viresh Kumar]

On 24 January 2013 12:54, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> Hi Viresh,

Hi Wolfram

> I think we are getting close.

Wow!!

> On Mon, Dec 03, 2012 at 08:24:04AM +0530, Viresh Kumar wrote:
>> 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?

@Uwe/Paul: ??

>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> +/* 10^6/KHz for delay in ns */
>> +unsigned long clk_delay = DIV_ROUND_UP(1000000, DEFAULT_CLK_RATE * 2);
>
> no global variable. should go into the function needing it.

I kept it to make sure we don't do this calculation every time... I would like
to keep it for better performance..

>> +static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
>> +{
>> +     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +     struct device *dev = &adap->dev;
>> +     int ret = 0;
>> +
>> +     ret = gpio_request_one(bri->scl_gpio, GPIOF_OPEN_DRAIN |
>> +                     GPIOF_OUT_INIT_HIGH, "i2c-scl");
>> +     if (ret) {
>> +             dev_warn(dev, "gpio request fail: %d\n", bri->scl_gpio);
>> +             return ret;
>> +     }
>> +
>> +     if (!bri->skip_sda_polling) {
>
> * if (is_valid_gpio(bri->sda_gpio)) ...

Hmm.. looks pretty good and correct. Will buy this one :)

>> +             if (gpio_request_one(bri->sda_gpio, GPIOF_IN, "i2c-sda")) {
>> +                     /* work without sda polling */
>> +                     dev_warn(dev, "can't get sda: %d. Skip sda polling\n",
>> +                                     bri->sda_gpio);
>> +                     bri->skip_sda_polling = true;
>
> * Instead of above line:
>         bri->get_sda = get_sda_gpio_value;

Hmm.. couldn't get this one...
So, what i understood is, because we don't have skip_sda_polling now, we
have to choose some other way to say, we don't support sda.. so we can
mark get_sda as NULL. right?

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

i2c_generic_recovery() is an local routine called from both scl and
gpio recovery
routines. So, keeping a single copy of prepare/unprepare calls makes more sense.

The other point about getting gpios first and then calling prepare..
I don't think that would be right thing to do. Suppose we call prepare
first, which would
update padmux on the board. At this time gpio may be in output mode and my burn
our boards. So, its better to get gpios in desired modes and then change muxing.

>> +     /* SCL shouldn't be low here */
>> +     if (!bri->get_scl(adap)) {
>> +             dev_err(&adap->dev, "SCL is stuck Low, exiting recovery.\n");
>
> returning -EBUSY?

Yes. Will recheck on errors returned from the routine.

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

yes.

>> +                             goto unprepare;
>> +                     }
>> +
>> +                     if (bri->get_sda(adap))
>> +                             break;
>
> 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

Good one.

>> +                             dev_warn(&adap->dev,
>> +                                             "!get_sda. skip sda polling\n");
>
> That message needs improvement ("!get_sda").

+1

All other comments are accepted too.. Hopefully it may get in 3.9 :)

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

[Wolfram Sang]

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


> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >> +/* 10^6/KHz for delay in ns */
> >> +unsigned long clk_delay = DIV_ROUND_UP(1000000, DEFAULT_CLK_RATE * 2);
> >
> > no global variable. should go into the function needing it.
> 
> I kept it to make sure we don't do this calculation every time... I would like
> to keep it for better performance..

Please move it. It is a constant anyhow, and globals are best avoided.

> >> +                     /* work without sda polling */
> >> +                     dev_warn(dev, "can't get sda: %d. Skip sda polling\n",
> >> +                                     bri->sda_gpio);
> >> +                     bri->skip_sda_polling = true;
> >
> > * Instead of above line:
> >         bri->get_sda = get_sda_gpio_value;
> 
> Hmm.. couldn't get this one...
> So, what i understood is, because we don't have skip_sda_polling now, we
> have to choose some other way to say, we don't support sda.. so we can
> mark get_sda as NULL. right?

Yup.

> The other point about getting gpios first and then calling prepare..
> I don't think that would be right thing to do. Suppose we call prepare
> first, which would
> update padmux on the board. At this time gpio may be in output mode and my burn
> our boards. So, its better to get gpios in desired modes and then change muxing.

OK.

Now onto V9...

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 V8 1/2] i2c/adapter: Add bus recovery infrastructure

[Viresh Kumar]

On 25 January 2013 14:20, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>> I kept it to make sure we don't do this calculation every time... I would like
>> to keep it for better performance..
>
> Please move it. It is a constant anyhow, and globals are best avoided.

I will move it. But i couldn't understand what you mean by "it is a constant
anyhow", because that was one of the reason for keeping it global.

> Now onto V9...

You will find the same there too :)

Anyway i need to do some minor cleanups requested by uwe, so v10 is
required.

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

[Wolfram Sang]

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

> Anyway i need to do some minor cleanups requested by uwe, so v10 is
> required.

Please wait until I finish my review for v9.

-- 
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 V8 1/2] i2c/adapter: Add bus recovery infrastructure

[Uwe Kleine-König]

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

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

[Viresh Kumar]

On 24 January 2013 16:24, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Thu, Jan 24, 2013 at 08:24:45AM +0100, Wolfram Sang wrote:
> Some more comments below.

Always welcomed :)

>> * 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");
>>
>> > +                           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.

Wow!! It was a blunder. Don't know how the braces got dropped.

My bad, but the new patchset (already posted), must have fixed it by
mistake :)

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

I have done something similar in V9..

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

[Wolfram Sang]

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


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

Timeout value. I consider I2C a static bus with no hotplugging. So,
either we have seen a START bit and know we are inside a transaction or
SDA is low because a slave is in an unknown state.

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

Which is a good reason to exit recovery.

Thanks,

   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 V8 1/2] i2c/adapter: Add bus recovery infrastructure

[Uwe Kleine-König]

Hey Wolfram,

On Fri, Jan 25, 2013 at 09:53:37AM +0100, Wolfram Sang wrote:
> > > 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?
> 
> Timeout value. I consider I2C a static bus with no hotplugging. So,
> either we have seen a START bit and know we are inside a transaction or
> SDA is low because a slave is in an unknown state.
This is probably true in at least 9 out of 10 cases today. I wonder if
that will stay as is. Up to you to ignore it. (I promise not to be
offended.)

> > > 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.
> 
> Which is a good reason to exit recovery.
Not necessarily. If a slave hangs and would need say 5 additional clocks
it still can stretch the 2nd clock cycle.

Best regards
Uwe

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

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

[Wolfram Sang]

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


> > Timeout value. I consider I2C a static bus with no hotplugging. So,
> > either we have seen a START bit and know we are inside a transaction or
> > SDA is low because a slave is in an unknown state.
> This is probably true in at least 9 out of 10 cases today. I wonder if
> that will stay as is. Up to you to ignore it. (I promise not to be
> offended.)

If that will be the case somewhen, bus recovery might be a bad idea in
general. Then again, such a setup is the one needing it most, probably
;) But hotplugging I2C is desperatly asking for problems.

> > > > 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.
> > 
> > Which is a good reason to exit recovery.
> Not necessarily. If a slave hangs and would need say 5 additional clocks
> it still can stretch the 2nd clock cycle.

While unlikely on the bit-level, still true. Need to think about it.

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

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