| Message ID | 1417448047-15236-5-git-send-email-grygorii.strashko@ti.com |
|---|---|
| State | New |
| Headers | show |
Hello Grygorii, On 01/12/14 16:34, Grygorii Strashko wrote: > This patch converts Davinci I2C driver to use I2C bus recovery > infrastructure, introduced by commit 5f9296ba21b3 ("i2c: Add > bus recovery infrastructure"). > > The i2c_bus_recovery_info is configured for Davinci I2C adapter > only in case scl_pin is provided in platform data. > > As the controller must be held in reset while doing so, the > recovery routine must re-init the controller. Since this was already > being done after each call to i2c_recover_bus, move those calls into > the recovery_prepare/unprepare routines and as well. > > CC: Sekhar Nori <nsekhar@ti.com> > CC: Kevin Hilman <khilman@deeprootsystems.com> > CC: Santosh Shilimkar <ssantosh@kernel.org> > CC: Murali Karicheri <m-karicheri2@ti.com> > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > drivers/i2c/busses/i2c-davinci.c | 77 +++++++++++++++++++--------------------- > 1 file changed, 36 insertions(+), 41 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c > index 17e1203..00aed63 100644 > --- a/drivers/i2c/busses/i2c-davinci.c > +++ b/drivers/i2c/busses/i2c-davinci.c > @@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg) > return readw_relaxed(i2c_dev->base + reg); > } > > -/* Generate a pulse on the i2c clock pin. */ > -static void davinci_i2c_clock_pulse(unsigned int scl_pin) > -{ > - u16 i; > - > - if (scl_pin) { > - /* Send high and low on the SCL line */ > - for (i = 0; i < 9; i++) { > - gpio_set_value(scl_pin, 0); > - udelay(20); > - gpio_set_value(scl_pin, 1); > - udelay(20); > - } > - } > -} > - > -/* This routine does i2c bus recovery as specified in the > - * i2c protocol Rev. 03 section 3.16 titled "Bus clear" > - */ > -static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev) > -{ > - u32 flag = 0; > - struct davinci_i2c_platform_data *pdata = dev->pdata; > - > - dev_err(dev->dev, "initiating i2c bus recovery\n"); > - /* Send NACK to the slave */ > - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG); > - flag |= DAVINCI_I2C_MDR_NACK; > - /* write the data into mode register */ > - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag); > - davinci_i2c_clock_pulse(pdata->scl_pin); > - /* Send STOP */ > - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG); > - flag |= DAVINCI_I2C_MDR_STP; > - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag); > -} > - > static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev, > int val) > { > @@ -267,6 +230,34 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev) > } > > /* > + * This routine does i2c bus recovery by using i2c_generic_gpio_recovery > + * which is provided by I2C Bus recovery infrastructure. > + */ > +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap) > +{ > + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap); > + > + /* Disable interrupts */ > + davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0); I suppose, you don't need to disable IRQs if you reset the controller as the very next action. > + > + /* put I2C into reset */ > + davinci_i2c_reset_ctrl(dev, 0); > +} > + > +static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap) > +{ > + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap); > + > + i2c_davinci_init(dev); > +} > + > +static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = { > + .recover_bus = i2c_generic_gpio_recovery, > + .prepare_recovery = davinci_i2c_prepare_recovery, > + .unprepare_recovery = davinci_i2c_unprepare_recovery, > +}; > + > +/* > * Waiting for bus not busy > */ > static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev, > @@ -286,8 +277,7 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev, > return -ETIMEDOUT; > } else { > to_cnt = 0; > - davinci_i2c_recover_bus(dev); > - i2c_davinci_init(dev); > + i2c_recover_bus(&dev->adapter); > } > } > if (allow_sleep) > @@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop) > dev->adapter.timeout); > if (r == 0) { > dev_err(dev->dev, "controller timed out\n"); > - davinci_i2c_recover_bus(dev); > - i2c_davinci_init(dev); > + i2c_recover_bus(adap); > dev->buf_len = 0; > return -ETIMEDOUT; > } > @@ -721,6 +710,12 @@ static int davinci_i2c_probe(struct platform_device *pdev) > adap->timeout = DAVINCI_I2C_TIMEOUT; > adap->dev.of_node = pdev->dev.of_node; > > + if (dev->pdata->scl_pin) { > + adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info; > + adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin; > + adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin; > + } > + > adap->nr = pdev->id; > r = i2c_add_numbered_adapter(adap); > if (r) { >
Hi, so, the bus recovery patches look fine to me in general. It is only this one question left which I always had with bus recovery. Maybe you guys can join me thinking about it. > @@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop) > dev->adapter.timeout); > if (r == 0) { > dev_err(dev->dev, "controller timed out\n"); > - davinci_i2c_recover_bus(dev); > - i2c_davinci_init(dev); > + i2c_recover_bus(adap); > dev->buf_len = 0; > return -ETIMEDOUT; The I2C specs say in 3.1.16 that the recovery procedure should be used when SDA is stuck low. So, I do wonder if we should apply the recovery after a timeout. Stuck SDA might be one reason for timeout, but there may be others... Thanks, Wolfram
Hi, On 03/18/2015 10:31 PM, Wolfram Sang wrote: > Hi, > > so, the bus recovery patches look fine to me in general. > > It is only this one question left which I always had with bus recovery. > Maybe you guys can join me thinking about it. Ok. Thanks and sorry for delayed reply - missed your e-mail :( I'll resend them next week. > >> @@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop) >> dev->adapter.timeout); >> if (r == 0) { >> dev_err(dev->dev, "controller timed out\n"); >> - davinci_i2c_recover_bus(dev); >> - i2c_davinci_init(dev); >> + i2c_recover_bus(adap); >> dev->buf_len = 0; >> return -ETIMEDOUT; > > The I2C specs say in 3.1.16 that the recovery procedure should be used > when SDA is stuck low. So, I do wonder if we should apply the recovery > after a timeout. Stuck SDA might be one reason for timeout, but there > may be others... This is ancient code. And regarding your question - Might be it would be reasonable to add call of i2c_davinci_wait_bus_not_busy() at the end of i2c_davinci_xfer()? This way we will wait for Bus Free before performing recovery. Of course, i2c_davinci_wait_bus_not_busy() has to be fixed first as proposed by Alexander Sverdlin here: https://patchwork.ozlabs.org/patch/448994/.
> > The I2C specs say in 3.1.16 that the recovery procedure should be used > > when SDA is stuck low. So, I do wonder if we should apply the recovery > > after a timeout. Stuck SDA might be one reason for timeout, but there > > may be others... > > This is ancient code. And regarding your question - > Might be it would be reasonable to add call of > i2c_davinci_wait_bus_not_busy() at the end of i2c_davinci_xfer()? > This way we will wait for Bus Free before performing recovery. That might be an improvement, but the generic question still remains: Is a timeout a reason for recovery? SDA stuck low is one reason for a timeout. I have problems making up my mind here between being pragmatic and being in accordance with the specs. > Of course, i2c_davinci_wait_bus_not_busy() has to be fixed first > as proposed by Alexander Sverdlin here: > https://patchwork.ozlabs.org/patch/448994/. Okay, good that you said it. So I'll give his patch series priority over this one.
On 04/03/2015 11:18 PM, Wolfram Sang wrote: > >>> The I2C specs say in 3.1.16 that the recovery procedure should be used >>> when SDA is stuck low. So, I do wonder if we should apply the recovery >>> after a timeout. Stuck SDA might be one reason for timeout, but there >>> may be others... >> >> This is ancient code. And regarding your question - >> Might be it would be reasonable to add call of >> i2c_davinci_wait_bus_not_busy() at the end of i2c_davinci_xfer()? >> This way we will wait for Bus Free before performing recovery. > > That might be an improvement, but the generic question still remains: > Is a timeout a reason for recovery? SDA stuck low is one reason for a > timeout. I have problems making up my mind here between being pragmatic > and being in accordance with the specs. The timeout here means there were no responses from I2C controller within some reasonable time period (default - 1 sec). Which in turn means that Bus/HW state is "unknown" and init&recovery seems reasonable here, but yes - "init&recovery" could be optimized more, but, in my opinion, only as subsequent patches. Actually, i2c_generic_recovery() will just exit if SDA is high already. > >> Of course, i2c_davinci_wait_bus_not_busy() has to be fixed first >> as proposed by Alexander Sverdlin here: >> https://patchwork.ozlabs.org/patch/448994/. > > Okay, good that you said it. So I'll give his patch series priority over > this one. Sorry, but this series already mises few merge windows and it has a lot of revied-by and tested-by, so could we proceed please? Re-based & re-sent http://www.spinics.net/lists/arm-kernel/msg410810.html
> >> Of course, i2c_davinci_wait_bus_not_busy() has to be fixed first > >> as proposed by Alexander Sverdlin here: > >> https://patchwork.ozlabs.org/patch/448994/. > > > > Okay, good that you said it. So I'll give his patch series priority over > > this one. > > > Sorry, but this series already mises few merge windows and it has a lot > of revied-by and tested-by, so could we proceed please? > > Re-based & re-sent http://www.spinics.net/lists/arm-kernel/msg410810.html ??? Didn't you say above that Alexaders's patch is needed first?
On 04/06/2015 07:09 PM, Wolfram Sang wrote: > >>>> Of course, i2c_davinci_wait_bus_not_busy() has to be fixed first >>>> as proposed by Alexander Sverdlin here: >>>> https://patchwork.ozlabs.org/patch/448994/. >>> >>> Okay, good that you said it. So I'll give his patch series priority over >>> this one. >> >> >> Sorry, but this series already mises few merge windows and it has a lot >> of revied-by and tested-by, so could we proceed please? >> >> Re-based & re-sent http://www.spinics.net/lists/arm-kernel/msg410810.html > > ??? Didn't you say above that Alexaders's patch is needed first? > Sorry for misunderstanding. I said that if We'd like to continue and optimize more recovery path then yes - Alexaders's patch will be needed (patch 2 from his series [PATCH 2/3] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy(), which, in turn need to be rebased as the first one in his series and re-send). And in my opinion all such improvements could be done by subsequent patches.
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index 17e1203..00aed63 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg) return readw_relaxed(i2c_dev->base + reg); } -/* Generate a pulse on the i2c clock pin. */ -static void davinci_i2c_clock_pulse(unsigned int scl_pin) -{ - u16 i; - - if (scl_pin) { - /* Send high and low on the SCL line */ - for (i = 0; i < 9; i++) { - gpio_set_value(scl_pin, 0); - udelay(20); - gpio_set_value(scl_pin, 1); - udelay(20); - } - } -} - -/* This routine does i2c bus recovery as specified in the - * i2c protocol Rev. 03 section 3.16 titled "Bus clear" - */ -static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev) -{ - u32 flag = 0; - struct davinci_i2c_platform_data *pdata = dev->pdata; - - dev_err(dev->dev, "initiating i2c bus recovery\n"); - /* Send NACK to the slave */ - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG); - flag |= DAVINCI_I2C_MDR_NACK; - /* write the data into mode register */ - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag); - davinci_i2c_clock_pulse(pdata->scl_pin); - /* Send STOP */ - flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG); - flag |= DAVINCI_I2C_MDR_STP; - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag); -} - static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev, int val) { @@ -267,6 +230,34 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev) } /* + * This routine does i2c bus recovery by using i2c_generic_gpio_recovery + * which is provided by I2C Bus recovery infrastructure. + */ +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap) +{ + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap); + + /* Disable interrupts */ + davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0); + + /* put I2C into reset */ + davinci_i2c_reset_ctrl(dev, 0); +} + +static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap) +{ + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap); + + i2c_davinci_init(dev); +} + +static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = { + .recover_bus = i2c_generic_gpio_recovery, + .prepare_recovery = davinci_i2c_prepare_recovery, + .unprepare_recovery = davinci_i2c_unprepare_recovery, +}; + +/* * Waiting for bus not busy */ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev, @@ -286,8 +277,7 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev, return -ETIMEDOUT; } else { to_cnt = 0; - davinci_i2c_recover_bus(dev); - i2c_davinci_init(dev); + i2c_recover_bus(&dev->adapter); } } if (allow_sleep) @@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop) dev->adapter.timeout); if (r == 0) { dev_err(dev->dev, "controller timed out\n"); - davinci_i2c_recover_bus(dev); - i2c_davinci_init(dev); + i2c_recover_bus(adap); dev->buf_len = 0; return -ETIMEDOUT; } @@ -721,6 +710,12 @@ static int davinci_i2c_probe(struct platform_device *pdev) adap->timeout = DAVINCI_I2C_TIMEOUT; adap->dev.of_node = pdev->dev.of_node; + if (dev->pdata->scl_pin) { + adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info; + adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin; + adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin; + } + adap->nr = pdev->id; r = i2c_add_numbered_adapter(adap); if (r) {
This patch converts Davinci I2C driver to use I2C bus recovery infrastructure, introduced by commit 5f9296ba21b3 ("i2c: Add bus recovery infrastructure"). The i2c_bus_recovery_info is configured for Davinci I2C adapter only in case scl_pin is provided in platform data. As the controller must be held in reset while doing so, the recovery routine must re-init the controller. Since this was already being done after each call to i2c_recover_bus, move those calls into the recovery_prepare/unprepare routines and as well. CC: Sekhar Nori <nsekhar@ti.com> CC: Kevin Hilman <khilman@deeprootsystems.com> CC: Santosh Shilimkar <ssantosh@kernel.org> CC: Murali Karicheri <m-karicheri2@ti.com> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/i2c/busses/i2c-davinci.c | 77 +++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 41 deletions(-)