| Message ID | 1416477788-5544-5-git-send-email-grygorii.strashko@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Uwe Kleine-König |
| Headers | show |
On Thu, Nov 20, 2014 at 12:03:07PM +0200, 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 if scl_pin is provided in Platform data at least. s/Platform/platform/ > > Because the controller must be held in reset while doing so, the s/Because/As/ > 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> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > drivers/i2c/busses/i2c-davinci.c | 76 ++++++++++++++++++---------------------- > 1 file changed, 35 insertions(+), 41 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c > index 2cef115..db2a2cd 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) > { > @@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev) > return 0; > } > > +/* This routine does i2c bus recovery as specified in the > + * i2c protocol Rev. 03 section 3.16 titled "Bus clear" > + */ This comment is wrong. The actual bus clear is implemented by i2c_generic_gpio_recovery. Also while touching this comment, convert it to the usual format with /* on its own line. (The file in question has already both types of comment, so consistency is not a reason to keep it as is.) Even though I remember that I reviewed this bus recovery patch (that resulted in 5f9296ba21b3) back then, I don't remember why it was split in prepare + recover + unprepare. But that is unrelated to this patch. > +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap) > +{ > + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap); > + > + dev_err(dev->dev, "initiating i2c bus recovery\n"); > + /* 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 = { I'd call this only davinci_i2c_recovery_info. > + .recover_bus = i2c_generic_gpio_recovery, > + .prepare_recovery = davinci_i2c_prepare_recovery, > + .unprepare_recovery = davinci_i2c_unprepare_recovery, > +}; new line here please > /* > * Waiting for bus not busy > */ > @@ -286,8 +276,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 +365,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; > } > @@ -717,6 +705,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) { Just another general comment about the driver that doesn't influence the correctness of this patch: The i2c-davinci driver is quite quick to reset the bus. I wonder how often this reset triggers. Is the bus in question less "stable" than others? Best regards Uwe
Hi Uwe, On 11/21/2014 09:07 PM, Uwe Kleine-König wrote: > On Thu, Nov 20, 2014 at 12:03:07PM +0200, 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 if scl_pin is provided in Platform data at least. > s/Platform/platform/ > >> >> Because the controller must be held in reset while doing so, the > s/Because/As/ > >> 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> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> drivers/i2c/busses/i2c-davinci.c | 76 ++++++++++++++++++---------------------- >> 1 file changed, 35 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c >> index 2cef115..db2a2cd 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) >> { >> @@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev) >> return 0; >> } >> >> +/* This routine does i2c bus recovery as specified in the >> + * i2c protocol Rev. 03 section 3.16 titled "Bus clear" >> + */ > This comment is wrong. The actual bus clear is implemented by > i2c_generic_gpio_recovery. Also while touching this comment, convert it > to the usual format with /* on its own line. (The file in question has > already both types of comment, so consistency is not a reason to keep it > as is.) It has been just copy-pasted, but ok. I'll change this comment as following: /* * This routine does i2c bus recovery by using i2c_generic_gpio_recovery * which is provided by I2C Bus recovery infrastructure. */ Is it ok? > > Even though I remember that I reviewed this bus recovery patch (that > resulted in 5f9296ba21b3) back then, I don't remember why it was split > in prepare + recover + unprepare. But that is unrelated to this patch. > >> +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap) >> +{ >> + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap); >> + >> + dev_err(dev->dev, "initiating i2c bus recovery\n"); >> + /* 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 = { > I'd call this only davinci_i2c_recovery_info. No. Pls, see next patch. > >> + .recover_bus = i2c_generic_gpio_recovery, >> + .prepare_recovery = davinci_i2c_prepare_recovery, >> + .unprepare_recovery = davinci_i2c_unprepare_recovery, >> +}; > new line here please :) Ok. Fixed in next patch. > >> /* >> * Waiting for bus not busy >> */ >> @@ -286,8 +276,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 +365,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; >> } >> @@ -717,6 +705,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) { > Just another general comment about the driver that doesn't influence the > correctness of this patch: The i2c-davinci driver is quite quick to > reset the bus. I wonder how often this reset triggers. Is the bus in > question less "stable" than others? In comparison to ..? :) regards, -grygorii -- 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
Hello Grygorii, On Fri, Nov 21, 2014 at 09:33:22PM +0200, Grygorii Strashko wrote: > On 11/21/2014 09:07 PM, Uwe Kleine-König wrote: > > On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote: > > Just another general comment about the driver that doesn't influence the > > correctness of this patch: The i2c-davinci driver is quite quick to > > reset the bus. I wonder how often this reset triggers. Is the bus in > > question less "stable" than others? > > In comparison to ..? :) In comparison to other bus drivers in other SoCs. I know this might be hard to answer. I just wonder where the reason for this has to be located. Strange hardware? Software bug? Or is this SoC just operating with strange slaves more often than others? Best regards Uwe
Hi Uwe, On 11/23/2014 10:36 PM, Uwe Kleine-König wrote: > On Fri, Nov 21, 2014 at 09:33:22PM +0200, Grygorii Strashko wrote: >> On 11/21/2014 09:07 PM, Uwe Kleine-König wrote: >>> On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote: >>> Just another general comment about the driver that doesn't influence the >>> correctness of this patch: The i2c-davinci driver is quite quick to >>> reset the bus. I wonder how often this reset triggers. Is the bus in >>> question less "stable" than others? >> >> In comparison to ..? :) > In comparison to other bus drivers in other SoCs. I know this might be > hard to answer. I just wonder where the reason for this has to be > located. Strange hardware? Software bug? Or is this SoC just operating > with strange slaves more often than others? Davinci driver does reset in two cases: - when I2C transaction isn't completed due to timeout (no irq received) - when BB is detected both cases are reasonable, because in 1st case HW state is undefined in 2d case - Davinci I2C supports only master mode and if BB detected we need perform some recovery procedure. Also, this patch doesn't introduce functional changes - it's just code reworking intended to reuse I2C bus recovery infrastructure i2c-omap.c - OMAP I2C driver does mostly the same now. i2c-tegra.c - seems, It will do reset even frequently. i2c-imx.c - if understand right, it will reinitialize I2C controller before each transfer, because it enables/disables I2C clocks. ... So, what i can say here is just "In comparison to ..?" :) regards, -grygorii -- 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
Hi Grygorii, On Mon, Nov 24, 2014 at 03:26:10PM +0200, Grygorii Strashko wrote: > On 11/23/2014 10:36 PM, Uwe Kleine-König wrote: > > On Fri, Nov 21, 2014 at 09:33:22PM +0200, Grygorii Strashko wrote: > >> On 11/21/2014 09:07 PM, Uwe Kleine-König wrote: > >>> On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote: > >>> Just another general comment about the driver that doesn't influence the > >>> correctness of this patch: The i2c-davinci driver is quite quick to > >>> reset the bus. I wonder how often this reset triggers. Is the bus in > >>> question less "stable" than others? > >> > >> In comparison to ..? :) > > In comparison to other bus drivers in other SoCs. I know this might be > > hard to answer. I just wonder where the reason for this has to be > > located. Strange hardware? Software bug? Or is this SoC just operating > > with strange slaves more often than others? > > Davinci driver does reset in two cases: > - when I2C transaction isn't completed due to timeout (no irq received) > - when BB is detected > both cases are reasonable, because in 1st case HW state is undefined > in 2d case - Davinci I2C supports only master mode and if BB detected > we need perform some recovery procedure. note that for multi-master scenarios bus recovery might be bad. So that has not necessarily something to do with "master mode only". > Also, this patch doesn't introduce functional changes - it's just code > reworking intended to reuse I2C bus recovery infrastructure Yeah this is fine, and my concern shouldn't stop your patch from getting in as it is an improvement for sure. Still I only had to handle BB once in my linux lifetime, and that was for a different reason. (Machine doing massive i2c transfers and so a client was often holding the bus busy when the watchdog resetted the cpu.) So I just wondered how often the need for recovery arises. Best regards Uwe
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index 2cef115..db2a2cd 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) { @@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev) return 0; } +/* This routine does i2c bus recovery as specified in the + * i2c protocol Rev. 03 section 3.16 titled "Bus clear" + */ +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap) +{ + struct davinci_i2c_dev *dev = i2c_get_adapdata(adap); + + dev_err(dev->dev, "initiating i2c bus recovery\n"); + /* 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 */ @@ -286,8 +276,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 +365,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; } @@ -717,6 +705,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 if scl_pin is provided in Platform data at least. Because 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> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/i2c/busses/i2c-davinci.c | 76 ++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 41 deletions(-)