| Message ID | 1504073857-122449-5-git-send-email-preid@electromag.com.au |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i2c: designware: add i2c gpio recovery option | expand |
On Wed, 2017-08-30 at 14:17 +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. I am still a little unsure about the recover > in the timeout case (i2c-designware-core.c:770) as i could not > test this codepath. > - if (abort_source & DW_IC_TX_ARB_LOST) > + if (abort_source & DW_IC_TX_ARB_LOST) { > + i2c_recover_bus(&dev->adapter); > return -EAGAIN; > - else if (abort_source & DW_IC_TX_ABRT_GCALL_READ) > + } else if (abort_source & DW_IC_TX_ABRT_GCALL_READ) else is redundant. > return -EINVAL; /* wrong msgs[] data */ > else Ditto. > return -EIO; > +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) || (r == -ENOENT)) Copy'n'paste typo? > + 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, > + "adapter: %s running with gpio recovery mode! scl:%i > sda:%i\n", > + adap->name, !!rinfo->scl_gpiod, !!rinfo->sda_gpiod); Instead of doing numbers, better just to list available descriptors, e.g. ...("... %s scl\n", rinfo->sda_gpiod ? "sda,"); No need to explain that scl doesn't need any check here. And I'm not sure why do you need adap->name here. Can you show an example of output from your test platform? > + if (!ret) > + ret = i2c_dw_init_recovery_info(dev); Better to if (ret) return ret; return i2c...(); > + > return ret;
On 08/30/2017 09:17 AM, 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. I am still a little unsure about the recover > in the timeout case (i2c-designware-core.c:770) as i could not > test this codepath. > > Signed-off-by: Tim Sander <tim@krieglstein.org> > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- > drivers/i2c/busses/i2c-designware-common.c | 11 ++++-- > drivers/i2c/busses/i2c-designware-core.h | 1 + > drivers/i2c/busses/i2c-designware-master.c | 57 ++++++++++++++++++++++++++++++ > 3 files changed, 66 insertions(+), 3 deletions(-) > While taking into account Andy's comments please modify the last sentence in the above commit log - i2c-designware-core.c doesn't exist anymore. Maybe better is to have the uncertainty documented as a "REVISIT:" comment in the code etc. > @@ -254,9 +258,10 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev) > for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources)) > dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]); > > - if (abort_source & DW_IC_TX_ARB_LOST) > + if (abort_source & DW_IC_TX_ARB_LOST) { > + i2c_recover_bus(&dev->adapter); Are you sure about doing recovery for arbitration lost case? To me it seems wrong to do it if another master is accessing the bus.
Thanks for the review. On 28/09/2017 18:58, Andy Shevchenko wrote: > On Wed, 2017-08-30 at 14:17 +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. I am still a little unsure about the recover >> in the timeout case (i2c-designware-core.c:770) as i could not >> test this codepath. > > >> - if (abort_source & DW_IC_TX_ARB_LOST) >> + if (abort_source & DW_IC_TX_ARB_LOST) { >> + i2c_recover_bus(&dev->adapter); >> return -EAGAIN; > >> - else if (abort_source & DW_IC_TX_ABRT_GCALL_READ) >> + } else if (abort_source & DW_IC_TX_ABRT_GCALL_READ) > > else is redundant. > >> return -EINVAL; /* wrong msgs[] data */ >> else > > Ditto. Yep. > >> return -EIO; > >> +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) || (r == -ENOENT)) > > Copy'n'paste typo? Yep. > >> + 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, >> + "adapter: %s running with gpio recovery mode! scl:%i >> sda:%i\n", >> + adap->name, !!rinfo->scl_gpiod, !!rinfo->sda_gpiod); > > Instead of doing numbers, better just to list available descriptors, > e.g. > > ...("... %s scl\n", rinfo->sda_gpiod ? "sda,"); Ok. > > No need to explain that scl doesn't need any check here. > > And I'm not sure why do you need adap->name here. Can you show an > example of output from your test platform? Good question. can't see a need. > >> + if (!ret) >> + ret = i2c_dw_init_recovery_info(dev); > > Better to > if (ret) > return ret; > > return i2c...(); > >> + >> return ret; >
On 28/09/2017 21:21, Jarkko Nikula wrote: > On 08/30/2017 09:17 AM, 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. I am still a little unsure about the recover >> in the timeout case (i2c-designware-core.c:770) as i could not >> test this codepath. >> >> Signed-off-by: Tim Sander <tim@krieglstein.org> >> Signed-off-by: Phil Reid <preid@electromag.com.au> >> --- >> drivers/i2c/busses/i2c-designware-common.c | 11 ++++-- >> drivers/i2c/busses/i2c-designware-core.h | 1 + >> drivers/i2c/busses/i2c-designware-master.c | 57 ++++++++++++++++++++++++++++++ >> 3 files changed, 66 insertions(+), 3 deletions(-) >> > While taking into account Andy's comments please modify the last sentence in the above commit log - i2c-designware-core.c doesn't exist anymore. Maybe better is > to have the uncertainty documented as a "REVISIT:" comment in the code etc. > >> @@ -254,9 +258,10 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev) >> for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources)) >> dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]); >> - if (abort_source & DW_IC_TX_ARB_LOST) >> + if (abort_source & DW_IC_TX_ARB_LOST) { >> + i2c_recover_bus(&dev->adapter); > > Are you sure about doing recovery for arbitration lost case? To me it seems wrong to do it if another master is accessing the bus. > yes I think your right.
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 3d684c6..e3120987 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); @@ -254,9 +258,10 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev) for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources)) dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]); - if (abort_source & DW_IC_TX_ARB_LOST) + if (abort_source & DW_IC_TX_ARB_LOST) { + i2c_recover_bus(&dev->adapter); return -EAGAIN; - else if (abort_source & DW_IC_TX_ABRT_GCALL_READ) + } else if (abort_source & DW_IC_TX_ABRT_GCALL_READ) return -EINVAL; /* wrong msgs[] data */ else return -EIO; diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index fef44b5..8707c76 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -284,6 +284,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..07e34fe 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,57 @@ 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) || (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, + "adapter: %s running with gpio recovery mode! scl:%i sda:%i\n", + adap->name, !!rinfo->scl_gpiod, !!rinfo->sda_gpiod); + + return 0; +} + int i2c_dw_probe(struct dw_i2c_dev *dev) { struct i2c_adapter *adap = &dev->adapter; @@ -664,6 +718,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) dev_err(dev->dev, "failure adding adapter: %d\n", ret); pm_runtime_put_noidle(dev->dev); + if (!ret) + ret = i2c_dw_init_recovery_info(dev); + return ret; } EXPORT_SYMBOL_GPL(i2c_dw_probe);