| Message ID | 1509590430-11968-5-git-send-email-preid@electromag.com.au |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: designware: add i2c gpio recovery option | expand |
On Thu, 2017-11-02 at 10:40 +0800, Phil Reid wrote: > From: Tim Sander <tim@krieglstein.org> > > This patch contains much input from Phil Reid and has been tested > on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the > SCL and SDA GPIO's. > > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Shame on me, I totally forgot one important comment on all this. How do we switch pinctrl back to the native function? Is it guaranteed by pinctrl framework and all underneath drivers?
On 7/11/2017 00:09, Andy Shevchenko wrote: > On Thu, 2017-11-02 at 10:40 +0800, Phil Reid wrote: >> From: Tim Sander <tim@krieglstein.org> >> >> This patch contains much input from Phil Reid and has been tested >> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the >> SCL and SDA GPIO's. >> >> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Shame on me, I totally forgot one important comment on all this. > > How do we switch pinctrl back to the native function? Is it guaranteed > by pinctrl framework and all underneath drivers? > > G'day Andy, That is a good question. I don't have an understanding of how the pinctrl framework works with respect to requesting gpios. My device (Intel / Altera Cyclone V SOC) doesn't have a pinctrl for the i2c / gpio mux as yet. It's all setup by the bootloader and they don't expect you to change it. I'm using two separate GPIO's "wired" to the i2c bus via the SOC FPGA for the recovery. Tim was doing the same.
Hi Sorry for beeing so silent, i have been swept to completly different area so this thing was swapped out. Thanks Phil for your work on this! Am Dienstag, 7. November 2017, 09:02:50 CET schrieb Phil Reid: > On 7/11/2017 00:09, Andy Shevchenko wrote: > > On Thu, 2017-11-02 at 10:40 +0800, Phil Reid wrote: > >> From: Tim Sander <tim@krieglstein.org> > >> > >> This patch contains much input from Phil Reid and has been tested > >> on Intel/Altera Cyclone V SOC Hardware with Altera GPIO's for the > >> SCL and SDA GPIO's. > >> > >> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Shame on me, I totally forgot one important comment on all this. > > > > How do we switch pinctrl back to the native function? Is it guaranteed > > by pinctrl framework and all underneath drivers? > That is a good question. I don't have an understanding of how the pinctrl > framework works with respect to requesting gpios. > My device (Intel / Altera Cyclone V SOC) doesn't have a pinctrl for the i2c > / gpio mux as yet. According to the documentation from Intel/Altera it is not allowed to change the pinmux while running. My guess is that they are using a shift chain, so the output values of all pins in the chain are not stable. I think they have been lazy and just used the io config for fpgas with an jtag controller and connected the shift chain for pinconfig to this controller. So unfortunatly it is not possible to switch single pins on the run without interfering with other pins. > It's all setup by the bootloader and they don't expect > you to change it. I'm using two separate GPIO's "wired" to the i2c bus via > the SOC FPGA for the recovery. Tim was doing the same. Yes, i think thats the only way. But it is annoying that the i2c controllers of 201x have no way to recover from such bus errors >:-(. Best regards Tim
On Wed, 2017-11-08 at 09:29 +0100, Tim Sander wrote: > > > How do we switch pinctrl back to the native function? Is it > > > guaranteed > > > by pinctrl framework and all underneath drivers? > > That is a good question. I don't have an understanding of how the > > pinctrl > > framework works with respect to requesting gpios. > > My device (Intel / Altera Cyclone V SOC) doesn't have a pinctrl for > > the i2c > > / gpio mux as yet. > > According to the documentation from Intel/Altera it is not allowed to > change > the pinmux while running. The driver is used on many platforms and requesting GPIO function while running _is_ a pinmux change. > My guess is that they are using a shift chain, so > the output values of all pins in the chain are not stable. I think > they have > been lazy and just used the io config for fpgas with an jtag > controller and > connected the shift chain for pinconfig to this controller. So > unfortunatly it > is not possible to switch single pins on the run without interfering > with > other pins. ...for this certain SoC. > > It's all setup by the bootloader and they don't expect > > you to change it. I'm using two separate GPIO's "wired" to the i2c > > bus via > > the SOC FPGA for the recovery. Tim was doing the same. > > Yes, i think thats the only way. But it is annoying that the i2c > controllers > of 201x have no way to recover from such bus errors >:-(. Yep, it's pity, and we need to cope somehow with it. So, my understanding of the current case is that either this change goes in only for certain SoC(s), or waiting until there is a guarantee from pinctrl subsystem to return function to native back when recovery finished. My personal vote is for latter.
On 8/11/2017 17:29, Andy Shevchenko wrote: > On Wed, 2017-11-08 at 09:29 +0100, Tim Sander wrote: > >>>> How do we switch pinctrl back to the native function? Is it >>>> guaranteed >>>> by pinctrl framework and all underneath drivers? >>> That is a good question. I don't have an understanding of how the >>> pinctrl >>> framework works with respect to requesting gpios. >>> My device (Intel / Altera Cyclone V SOC) doesn't have a pinctrl for >>> the i2c >>> / gpio mux as yet. >> >> According to the documentation from Intel/Altera it is not allowed to >> change >> the pinmux while running. > > The driver is used on many platforms and requesting GPIO function while > running _is_ a pinmux change. > >> My guess is that they are using a shift chain, so >> the output values of all pins in the chain are not stable. I think >> they have >> been lazy and just used the io config for fpgas with an jtag >> controller and >> connected the shift chain for pinconfig to this controller. So >> unfortunatly it >> is not possible to switch single pins on the run without interfering >> with >> other pins. > > ...for this certain SoC. > >>> It's all setup by the bootloader and they don't expect >>> you to change it. I'm using two separate GPIO's "wired" to the i2c >>> bus via >>> the SOC FPGA for the recovery. Tim was doing the same. >> >> Yes, i think thats the only way. But it is annoying that the i2c >> controllers >> of 201x have no way to recover from such bus errors >:-(. > > Yep, it's pity, and we need to cope somehow with it. > > So, my understanding of the current case is that either this change goes in only for certain SoC(s), or waiting until there is a guarantee from pinctrl subsystem to return function to native back when recovery finished. > > My personal vote is for latter. > I'm not sure my change set will affect anything.. Current drivers using the gpio functions are requesting the gpio using the old interface. If releasing the gpio doesn't restore the original functionality than nothing changes with this series. For the designware driver, the recovery code wont be called unless the gpio's are specified via the device tree. So the pinmux config shouldn't get changed. It'd be nice to have the code in mainline even if most soc's can't use it as yet.
On Fri, 2017-11-10 at 14:55 +0800, Phil Reid wrote: > On 8/11/2017 17:29, Andy Shevchenko wrote: > > On Wed, 2017-11-08 at 09:29 +0100, Tim Sander wrote: > I'm not sure my change set will affect anything.. > Current drivers using the gpio functions are requesting the gpio using > the old interface. > If releasing the gpio doesn't restore the original functionality than > nothing changes with this series. > > For the designware driver, the recovery code wont be called unless the > gpio's are specified via the device tree. > So the pinmux config shouldn't get changed. > > It'd be nice to have the code in mainline even if most soc's can't use > it as yet. I'm fine as long as it doesn't break existing users of DW I2C.
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 3d684c6..6b82809 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -230,7 +230,11 @@ int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev) while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) { if (timeout <= 0) { dev_warn(dev->dev, "timeout waiting for bus ready\n"); - return -ETIMEDOUT; + i2c_recover_bus(&dev->adapter); + + if (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) + return -ETIMEDOUT; + return 0; } timeout--; usleep_range(1000, 1100); diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 33c6c8f..d58a336 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -286,6 +286,7 @@ struct dw_i2c_dev { void (*disable_int)(struct dw_i2c_dev *dev); int (*init)(struct dw_i2c_dev *dev); int mode; + struct i2c_bus_recovery_info rinfo; }; #define ACCESS_SWAP 0x00000001 diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 418c233..ae69188 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -25,11 +25,13 @@ #include <linux/err.h> #include <linux/errno.h> #include <linux/export.h> +#include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/module.h> #include <linux/pm_runtime.h> +#include <linux/reset.h> #include "i2c-designware-core.h" @@ -443,6 +445,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) if (!wait_for_completion_timeout(&dev->cmd_complete, adap->timeout)) { dev_err(dev->dev, "controller timed out\n"); /* i2c_dw_init implicitly disables the adapter */ + i2c_recover_bus(&dev->adapter); i2c_dw_init_master(dev); ret = -ETIMEDOUT; goto done; @@ -613,6 +616,56 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id) return IRQ_HANDLED; } +static void i2c_dw_prepare_recovery(struct i2c_adapter *adap) +{ + struct dw_i2c_dev *dev = i2c_get_adapdata(adap); + + i2c_dw_disable(dev); + reset_control_assert(dev->rst); + i2c_dw_prepare_clk(dev, false); +} + +static void i2c_dw_unprepare_recovery(struct i2c_adapter *adap) +{ + struct dw_i2c_dev *dev = i2c_get_adapdata(adap); + + i2c_dw_prepare_clk(dev, true); + reset_control_deassert(dev->rst); + i2c_dw_init_master(dev); +} + +static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) +{ + struct i2c_bus_recovery_info *rinfo = &dev->rinfo; + struct i2c_adapter *adap = &dev->adapter; + struct gpio_desc *gpio; + int r; + + gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH); + if (IS_ERR(gpio)) { + r = PTR_ERR(gpio); + if (r == -ENOENT) + return 0; + return r; + } + rinfo->scl_gpiod = gpio; + + gpio = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN); + if (IS_ERR(gpio)) + return PTR_ERR(gpio); + rinfo->sda_gpiod = gpio; + + rinfo->recover_bus = i2c_generic_scl_recovery; + rinfo->prepare_recovery = i2c_dw_prepare_recovery; + rinfo->unprepare_recovery = i2c_dw_unprepare_recovery; + adap->bus_recovery_info = rinfo; + + dev_info(dev->dev, "running with gpio recovery mode! scl%s", + rinfo->sda_gpiod ? ",sda" : ""); + + return 0; +} + int i2c_dw_probe(struct dw_i2c_dev *dev) { struct i2c_adapter *adap = &dev->adapter; @@ -652,6 +705,10 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) return ret; } + ret = i2c_dw_init_recovery_info(dev); + if (ret) + return ret; + /* * Increment PM usage count during adapter registration in order to * avoid possible spurious runtime suspend when adapter device is