| Message ID | 1432818224-17070-6-git-send-email-vaibhav.hiremath@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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
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);