[05/12] i2c: pxa: Add bus reset functionality

Message ID 1432818224-17070-6-git-send-email-vaibhav.hiremath@linaro.org
State New
Headers show

Commit Message

Vaibhav Hiremath May 28, 2015, 1:03 p.m.
From: Rob Herring <robh@kernel.org>

Since there is some problematic i2c slave devices on some
platforms such as dkb (sometimes), it will drop down sda
and make i2c bus hang, at that time, it need to config
scl/sda into gpio to simulate "stop" sequence to recover
i2c bus, so add this interface.

Signed-off-by: Leilei Shang <shangll@marvell.com>
Signed-off-by: Rob Herring <robh@kernel.org>
[vaibhav.hiremath@linaro.org: Updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

Comments

Vaibhav Hiremath May 29, 2015, 3:40 p.m. | #1
On Friday 29 May 2015 07:29 PM, Rob Herring wrote:
> On Thu, May 28, 2015 at 8:03 AM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> wrote:
>> From: Rob Herring <robh@kernel.org>
>
> This probably should still be Leilei, but...
>

Ok, Since I am taking forward from your sign-off, did not change it.
Anyway will fix in next version.

>> Since there is some problematic i2c slave devices on some
>> platforms such as dkb (sometimes), it will drop down sda
>> and make i2c bus hang, at that time, it need to config
>> scl/sda into gpio to simulate "stop" sequence to recover
>> i2c bus, so add this interface.
>>
>> Signed-off-by: Leilei Shang <shangll@marvell.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> [vaibhav.hiremath@linaro.org: Updated Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-pxa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 90 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> index 8ca5552..eb09071 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -37,6 +37,8 @@
>>   #include <linux/slab.h>
>>   #include <linux/io.h>
>>   #include <linux/i2c/pxa-i2c.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/pinctrl/consumer.h>
>>
>>   #include <asm/irq.h>
>>
>> @@ -177,6 +179,9 @@ struct pxa_i2c {
>>          bool                    highmode_enter;
>>          unsigned int            ilcr;
>>          unsigned int            iwcr;
>> +       struct pinctrl          *pinctrl;
>> +       struct pinctrl_state    *pin_i2c;
>> +       struct pinctrl_state    *pin_gpio;
>>   };
>>
>>   #define _IBMR(i2c)     ((i2c)->reg_ibmr)
>> @@ -269,6 +274,62 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
>>
>>   #define show_state(i2c) i2c_pxa_show_state(i2c, __LINE__, __func__)
>>
>> +static void i2c_bus_reset(struct pxa_i2c *i2c)
>
> There's a generic mechanism in i2c_generic_gpio_recovery we should use
> here. It appears to be similar, but not exactly the same. The pinctrl
> part should probably be done by gpio driver.
>


Good point.

As you mentioned, they are not exactly same.

But we can achieve exactly what we wanted using prepare & unprepare
callbacks.
Generate stop signal in unprepare() callback should suffice our need.


But I am not quite sure about pinctrl handling by gpio driver.
I was tracing the calls from gpio_request_one()anf gpio_free()
but it seems they are not bringing back pins to default state.

correct me if I am wrong.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 2, 2015, 1:12 p.m. | #2
On Thu, May 28, 2015 at 3:03 PM, Vaibhav Hiremath
<vaibhav.hiremath@linaro.org> wrote:

> From: Rob Herring <robh@kernel.org>
>
> Since there is some problematic i2c slave devices on some
> platforms such as dkb (sometimes), it will drop down sda
> and make i2c bus hang, at that time, it need to config
> scl/sda into gpio to simulate "stop" sequence to recover
> i2c bus, so add this interface.
>
> Signed-off-by: Leilei Shang <shangll@marvell.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> [vaibhav.hiremath@linaro.org: Updated Changelog]
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>

Double signed-off?

(...)
+#include <linux/of_gpio.h>

You should use <linux/gpio/consumer.h> and then use
GPIO descriptors instead.

> @@ -177,6 +179,9 @@ struct pxa_i2c {
>         bool                    highmode_enter;
>         unsigned int            ilcr;
>         unsigned int            iwcr;
> +       struct pinctrl          *pinctrl;
> +       struct pinctrl_state    *pin_i2c;
> +       struct pinctrl_state    *pin_gpio;

Yup that's the right way. I see this is the "default"
state for pin_i2c.

> +static void i2c_bus_reset(struct pxa_i2c *i2c)
> +{
> +       int ret, ccnt, pins_scl, pins_sda;

Use GPIO descriptors.

struct gpio_desc *scl, *sda;

> +       struct device *dev = i2c->adap.dev.parent;
> +       struct device_node *np = dev->of_node;
> +
> +       if (!i2c->pinctrl) {
> +               dev_warn(dev, "could not do i2c bus reset\n");
> +               return;
> +       }
> +
> +       ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_gpio);
> +       if (ret) {
> +               dev_err(dev, "could not set gpio pins\n");
> +               return;
> +       }

Exactly like that yes.

> +       pins_scl = of_get_named_gpio(np, "i2c-gpio", 0);
> +       if (!gpio_is_valid(pins_scl)) {
> +               dev_err(dev, "i2c scl gpio not set\n");
> +               goto err_out;
> +       }
> +       pins_sda = of_get_named_gpio(np, "i2c-gpio", 1);
> +       if (!gpio_is_valid(pins_sda)) {
> +               dev_err(dev, "i2c sda gpio not set\n");
> +               goto err_out;
> +       }

I would suggest just using two GPIOs in the node relying
on index order. With GPIO descriptors:

scl = gpiod_get_index(dev, "i2c-gpio", 0, GPIOD_ASIS);
sda = gpiod_get_index(dev, "i2c-gpio", 1, GPIOD_ASIS);

Then use gpiod* accessors below and...

> +
> +       gpio_request(pins_scl, NULL);
> +       gpio_request(pins_sda, NULL);
> +
> +       gpio_direction_input(pins_sda);
> +       for (ccnt = 20; ccnt; ccnt--) {
> +               gpio_direction_output(pins_scl, ccnt & 0x01);
> +               udelay(5);
> +       }
> +       gpio_direction_output(pins_scl, 0);
> +       udelay(5);
> +       gpio_direction_output(pins_sda, 0);
> +       udelay(5);
> +       /* stop signal */
> +       gpio_direction_output(pins_scl, 1);
> +       udelay(5);
> +       gpio_direction_output(pins_sda, 1);
> +       udelay(5);
> +
> +       gpio_free(pins_scl);
> +       gpio_free(pins_sda);

gpiod_put(scl);
gpiod_put(sda);

> +err_out:
> +       ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_i2c);
> +       if (ret)
> +               dev_err(dev, "could not set default(i2c) pins\n");
> +       return;

Nice.

Overall it looks like a real nice structured workaround using
the API exactly as intended, just need to catch up with
using GPIO descriptors.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vaibhav Hiremath June 2, 2015, 4:40 p.m. | #3
On Tuesday 02 June 2015 06:42 PM, Linus Walleij wrote:
> On Thu, May 28, 2015 at 3:03 PM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> wrote:
>
>> From: Rob Herring <robh@kernel.org>
>>
>> Since there is some problematic i2c slave devices on some
>> platforms such as dkb (sometimes), it will drop down sda
>> and make i2c bus hang, at that time, it need to config
>> scl/sda into gpio to simulate "stop" sequence to recover
>> i2c bus, so add this interface.
>>
>> Signed-off-by: Leilei Shang <shangll@marvell.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> [vaibhav.hiremath@linaro.org: Updated Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Double signed-off?
>
> (...)
> +#include <linux/of_gpio.h>
>
> You should use <linux/gpio/consumer.h> and then use
> GPIO descriptors instead.
>
>> @@ -177,6 +179,9 @@ struct pxa_i2c {
>>          bool                    highmode_enter;
>>          unsigned int            ilcr;
>>          unsigned int            iwcr;
>> +       struct pinctrl          *pinctrl;
>> +       struct pinctrl_state    *pin_i2c;
>> +       struct pinctrl_state    *pin_gpio;
>
> Yup that's the right way. I see this is the "default"
> state for pin_i2c.
>
>> +static void i2c_bus_reset(struct pxa_i2c *i2c)
>> +{
>> +       int ret, ccnt, pins_scl, pins_sda;
>
> Use GPIO descriptors.
>
> struct gpio_desc *scl, *sda;
>
>> +       struct device *dev = i2c->adap.dev.parent;
>> +       struct device_node *np = dev->of_node;
>> +
>> +       if (!i2c->pinctrl) {
>> +               dev_warn(dev, "could not do i2c bus reset\n");
>> +               return;
>> +       }
>> +
>> +       ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_gpio);
>> +       if (ret) {
>> +               dev_err(dev, "could not set gpio pins\n");
>> +               return;
>> +       }
>
> Exactly like that yes.
>
>> +       pins_scl = of_get_named_gpio(np, "i2c-gpio", 0);
>> +       if (!gpio_is_valid(pins_scl)) {
>> +               dev_err(dev, "i2c scl gpio not set\n");
>> +               goto err_out;
>> +       }
>> +       pins_sda = of_get_named_gpio(np, "i2c-gpio", 1);
>> +       if (!gpio_is_valid(pins_sda)) {
>> +               dev_err(dev, "i2c sda gpio not set\n");
>> +               goto err_out;
>> +       }
>
> I would suggest just using two GPIOs in the node relying
> on index order. With GPIO descriptors:
>
> scl = gpiod_get_index(dev, "i2c-gpio", 0, GPIOD_ASIS);
> sda = gpiod_get_index(dev, "i2c-gpio", 1, GPIOD_ASIS);
>
> Then use gpiod* accessors below and...
>
>> +
>> +       gpio_request(pins_scl, NULL);
>> +       gpio_request(pins_sda, NULL);
>> +
>> +       gpio_direction_input(pins_sda);
>> +       for (ccnt = 20; ccnt; ccnt--) {
>> +               gpio_direction_output(pins_scl, ccnt & 0x01);
>> +               udelay(5);
>> +       }
>> +       gpio_direction_output(pins_scl, 0);
>> +       udelay(5);
>> +       gpio_direction_output(pins_sda, 0);
>> +       udelay(5);
>> +       /* stop signal */
>> +       gpio_direction_output(pins_scl, 1);
>> +       udelay(5);
>> +       gpio_direction_output(pins_sda, 1);
>> +       udelay(5);
>> +
>> +       gpio_free(pins_scl);
>> +       gpio_free(pins_sda);
>
> gpiod_put(scl);
> gpiod_put(sda);
>
>> +err_out:
>> +       ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_i2c);
>> +       if (ret)
>> +               dev_err(dev, "could not set default(i2c) pins\n");
>> +       return;
>
> Nice.
>
> Overall it looks like a real nice structured workaround using
> the API exactly as intended, just need to catch up with
> using GPIO descriptors.
>


Thanks Linus,

I will work on this and submit V2 shortly.

Thanks,
Vaibhav

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vaibhav Hiremath June 2, 2015, 5:40 p.m. | #4
On Tuesday 02 June 2015 11:03 PM, Wolfram Sang wrote:
>
>> Since there is some problematic i2c slave devices on some
>> platforms such as dkb (sometimes), it will drop down sda
>> and make i2c bus hang, at that time, it need to config
>> scl/sda into gpio to simulate "stop" sequence to recover
>> i2c bus, so add this interface.
>
> Please note that the i2c core has a bus recovery infrastructure.
> Search i2c.h for struct i2c_bus_recovery_info.
>


Yeah,
I assume you are referring to "i2c_generic_gpio_recovery", right?

Rob had suggested this, and will be using it in my next version.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vaibhav Hiremath June 2, 2015, 5:57 p.m. | #5
On Tuesday 02 June 2015 11:18 PM, Wolfram Sang wrote:
>> I assume you are referring to "i2c_generic_gpio_recovery", right?
>>
>> Rob had suggested this, and will be using it in my next version.
>
> Great, thanks!
>


can you please also review the RFC which I submitted few days back?

http://www.spinics.net/lists/linux-i2c/msg19719.html


And also,

http://www.spinics.net/lists/arm-kernel/msg422034.html

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vaibhav Hiremath June 2, 2015, 6:06 p.m. | #6
On Tuesday 02 June 2015 11:32 PM, Wolfram Sang wrote:
>
>> can you please also review the RFC which I submitted few days back?
>
> They are added to the queue and will be reviewed when the time comes. I
> won't make any promises.
>


Ok, take your time.
Just wanted to make sure that they don't get buried under all email
traffic.

Anyway, thanks for your review.


Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vaibhav Hiremath June 2, 2015, 6:46 p.m. | #7
On Tuesday 02 June 2015 11:54 PM, Wolfram Sang wrote:
> Can you resend the hwlock patches? I use patchwork for tracking and
> those were sent when the server had a little downtime. Thanks!
>


Just resent it. please check.

Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vaibhav Hiremath June 3, 2015, 7:16 p.m. | #8
On Tuesday 02 June 2015 06:42 PM, Linus Walleij wrote:
> On Thu, May 28, 2015 at 3:03 PM, Vaibhav Hiremath
> <vaibhav.hiremath@linaro.org> wrote:
>
>> From: Rob Herring <robh@kernel.org>
>>
>> Since there is some problematic i2c slave devices on some
>> platforms such as dkb (sometimes), it will drop down sda
>> and make i2c bus hang, at that time, it need to config
>> scl/sda into gpio to simulate "stop" sequence to recover
>> i2c bus, so add this interface.
>>
>> Signed-off-by: Leilei Shang <shangll@marvell.com>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> [vaibhav.hiremath@linaro.org: Updated Changelog]
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>
> Double signed-off?
>

Thanks for your review.

Based on review comments on the list, i will have to change this patch
to use generic gpio reset implementation available in i2c-core.

Having said that, I am still sure whether GPIO lib (request/free)
would handle pinctrl configuration.

Thanks,
vaibhav
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 8ca5552..eb09071 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -37,6 +37,8 @@ 
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/i2c/pxa-i2c.h>
+#include <linux/of_gpio.h>
+#include <linux/pinctrl/consumer.h>
 
 #include <asm/irq.h>
 
@@ -177,6 +179,9 @@  struct pxa_i2c {
 	bool			highmode_enter;
 	unsigned int		ilcr;
 	unsigned int		iwcr;
+	struct pinctrl		*pinctrl;
+	struct pinctrl_state	*pin_i2c;
+	struct pinctrl_state	*pin_gpio;
 };
 
 #define _IBMR(i2c)	((i2c)->reg_ibmr)
@@ -269,6 +274,62 @@  static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
 
 #define show_state(i2c) i2c_pxa_show_state(i2c, __LINE__, __func__)
 
+static void i2c_bus_reset(struct pxa_i2c *i2c)
+{
+	int ret, ccnt, pins_scl, pins_sda;
+	struct device *dev = i2c->adap.dev.parent;
+	struct device_node *np = dev->of_node;
+
+	if (!i2c->pinctrl) {
+		dev_warn(dev, "could not do i2c bus reset\n");
+		return;
+	}
+
+	ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_gpio);
+	if (ret) {
+		dev_err(dev, "could not set gpio pins\n");
+		return;
+	}
+
+	pins_scl = of_get_named_gpio(np, "i2c-gpio", 0);
+	if (!gpio_is_valid(pins_scl)) {
+		dev_err(dev, "i2c scl gpio not set\n");
+		goto err_out;
+	}
+	pins_sda = of_get_named_gpio(np, "i2c-gpio", 1);
+	if (!gpio_is_valid(pins_sda)) {
+		dev_err(dev, "i2c sda gpio not set\n");
+		goto err_out;
+	}
+
+	gpio_request(pins_scl, NULL);
+	gpio_request(pins_sda, NULL);
+
+	gpio_direction_input(pins_sda);
+	for (ccnt = 20; ccnt; ccnt--) {
+		gpio_direction_output(pins_scl, ccnt & 0x01);
+		udelay(5);
+	}
+	gpio_direction_output(pins_scl, 0);
+	udelay(5);
+	gpio_direction_output(pins_sda, 0);
+	udelay(5);
+	/* stop signal */
+	gpio_direction_output(pins_scl, 1);
+	udelay(5);
+	gpio_direction_output(pins_sda, 1);
+	udelay(5);
+
+	gpio_free(pins_scl);
+	gpio_free(pins_sda);
+
+err_out:
+	ret = pinctrl_select_state(i2c->pinctrl, i2c->pin_i2c);
+	if (ret)
+		dev_err(dev, "could not set default(i2c) pins\n");
+	return;
+}
+
 static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
 {
 	unsigned int i;
@@ -281,6 +342,11 @@  static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
 	for (i = 0; i < i2c->irqlogidx; i++)
 		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
 	printk("\n");
+	if (strcmp(why, "exhausted retries") != 0) {
+		i2c_bus_reset(i2c);
+		/* reset i2c contorler when it's fail */
+		i2c_pxa_reset(i2c);
+	}
 }
 
 #else /* ifdef DEBUG */
@@ -1301,6 +1367,30 @@  static int i2c_pxa_probe(struct platform_device *dev)
 
 	platform_set_drvdata(dev, i2c);
 
+	i2c->pinctrl = devm_pinctrl_get(&dev->dev);
+	if (IS_ERR(i2c->pinctrl)) {
+		i2c->pinctrl = NULL;
+		dev_warn(&dev->dev, "could not get pinctrl\n");
+	} else {
+		i2c->pin_i2c = pinctrl_lookup_state(i2c->pinctrl, "default");
+		if (IS_ERR(i2c->pin_i2c)) {
+			dev_err(&dev->dev, "could not get default(i2c) pinstate\n");
+			ret = IS_ERR(i2c->pin_i2c);
+		}
+
+		i2c->pin_gpio = pinctrl_lookup_state(i2c->pinctrl, "gpio");
+		if (IS_ERR(i2c->pin_gpio)) {
+			dev_err(&dev->dev, "could not get gpio pinstate\n");
+			ret = IS_ERR(i2c->pin_gpio);
+		}
+
+		if (ret) {
+			i2c->pin_i2c = NULL;
+			i2c->pin_gpio = NULL;
+			i2c->pinctrl = NULL;
+			ret = 0;
+		}
+	}
 #ifdef CONFIG_I2C_PXA_SLAVE
 	printk(KERN_INFO "I2C: %s: PXA I2C adapter, slave address %d\n",
 	       dev_name(&i2c->adap.dev), i2c->slave_addr);