* [PATCH 0/2] i2c: omap: cleanups @ 2012-10-25 10:23 Shubhrajyoti D 2012-10-25 10:23 ` [PATCH 1/2] i2c: omap: re-factor omap_i2c_init function Shubhrajyoti D 2012-10-25 10:23 ` [PATCH 2/2] i2c: omap: make reset a seperate function Shubhrajyoti D 0 siblings, 2 replies; 6+ messages in thread From: Shubhrajyoti D @ 2012-10-25 10:23 UTC (permalink / raw) To: linux-omap Cc: linux-i2c, linux-arm-kernel, ben-linux, tony, w.sang, Shubhrajyoti D Applies on Felipe's series http://www.spinics.net/lists/linux-omap/msg79995.html Tested with Terro sys fix + Felipe's stop sched_clock() during suspend on omap3630 beagle both idle and suspend. Functional testing on omap4sdp. Shubhrajyoti D (2): i2c: omap: re-factor omap_i2c_init function i2c: omap: make reset a seperate function drivers/i2c/busses/i2c-omap.c | 97 +++++++++++++++++++++-------------------- 1 files changed, 49 insertions(+), 48 deletions(-) -- 1.7.5.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] i2c: omap: re-factor omap_i2c_init function 2012-10-25 10:23 [PATCH 0/2] i2c: omap: cleanups Shubhrajyoti D @ 2012-10-25 10:23 ` Shubhrajyoti D 2012-10-25 10:52 ` Felipe Balbi 2012-10-25 10:23 ` [PATCH 2/2] i2c: omap: make reset a seperate function Shubhrajyoti D 1 sibling, 1 reply; 6+ messages in thread From: Shubhrajyoti D @ 2012-10-25 10:23 UTC (permalink / raw) To: linux-omap Cc: linux-i2c, linux-arm-kernel, ben-linux, tony, w.sang, Shubhrajyoti D re-factor omap_i2c_init() so that we can re-use it for resume. While at it also remove the bufstate variable as we write it in omap_i2c_resize_fifo for every transfer. Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> --- drivers/i2c/busses/i2c-omap.c | 71 ++++++++++++++++++---------------------- 1 files changed, 32 insertions(+), 39 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 5e5cefb..3d400b1 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -209,7 +209,6 @@ struct omap_i2c_dev { u16 pscstate; u16 scllstate; u16 sclhstate; - u16 bufstate; u16 syscstate; u16 westate; u16 errata; @@ -285,9 +284,31 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg) } } +static void __omap_i2c_init(struct omap_i2c_dev *dev) +{ + + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); + /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */ + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate); + + /* SCL low and high time values */ + omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate); + omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate); + if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) + omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate); + /* Take the I2C module out of reset: */ + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); + /* + * Don't write to this register if the IE state is 0 as it can + * cause deadlock. + */ + if (dev->iestate) + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate); +} + static int omap_i2c_init(struct omap_i2c_dev *dev) { - u16 psc = 0, scll = 0, sclh = 0, buf = 0; + u16 psc = 0, scll = 0, sclh = 0; u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0; unsigned long fclk_rate = 12000000; unsigned long timeout; @@ -337,11 +358,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev) * REVISIT: Some wkup sources might not be needed. */ dev->westate = OMAP_I2C_WE_ALL; - omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, - dev->westate); } } - omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); if (dev->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) { /* @@ -426,28 +444,18 @@ static int omap_i2c_init(struct omap_i2c_dev *dev) sclh = fclk_rate / (dev->speed * 2) - 7 + psc; } - /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */ - omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, psc); - - /* SCL low and high time values */ - omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll); - omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh); - - /* Take the I2C module out of reset: */ - omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); - /* Enable interrupts */ dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY | OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL) | ((dev->fifo_size) ? (OMAP_I2C_IE_RDR | OMAP_I2C_IE_XDR) : 0); - omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate); - if (dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) { - dev->pscstate = psc; - dev->scllstate = scll; - dev->sclhstate = sclh; - dev->bufstate = buf; - } + + dev->pscstate = psc; + dev->scllstate = scll; + dev->sclhstate = sclh; + + __omap_i2c_init(dev); + return 0; } @@ -1268,23 +1276,8 @@ static int omap_i2c_runtime_resume(struct device *dev) { struct omap_i2c_dev *_dev = dev_get_drvdata(dev); - if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) { - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0); - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate); - omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, _dev->scllstate); - omap_i2c_write_reg(_dev, OMAP_I2C_SCLH_REG, _dev->sclhstate); - omap_i2c_write_reg(_dev, OMAP_I2C_BUF_REG, _dev->bufstate); - omap_i2c_write_reg(_dev, OMAP_I2C_SYSC_REG, _dev->syscstate); - omap_i2c_write_reg(_dev, OMAP_I2C_WE_REG, _dev->westate); - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); - } - - /* - * Don't write to this register if the IE state is 0 as it can - * cause deadlock. - */ - if (_dev->iestate) - omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev->iestate); + if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) + __omap_i2c_init(_dev); return 0; } -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] i2c: omap: re-factor omap_i2c_init function 2012-10-25 10:23 ` [PATCH 1/2] i2c: omap: re-factor omap_i2c_init function Shubhrajyoti D @ 2012-10-25 10:52 ` Felipe Balbi 0 siblings, 0 replies; 6+ messages in thread From: Felipe Balbi @ 2012-10-25 10:52 UTC (permalink / raw) To: Shubhrajyoti D Cc: linux-omap, linux-i2c, linux-arm-kernel, ben-linux, tony, w.sang [-- Attachment #1: Type: text/plain, Size: 2749 bytes --] Hi, (a small top-post here, don't forget to keep the patch version in the subject, I think this is v3 already, so next patch should be v4) On Thu, Oct 25, 2012 at 03:53:05PM +0530, Shubhrajyoti D wrote: > re-factor omap_i2c_init() so that we can re-use it for resume. > While at it also remove the bufstate variable as we write it > in omap_i2c_resize_fifo for every transfer. > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 71 ++++++++++++++++++---------------------- > 1 files changed, 32 insertions(+), 39 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 5e5cefb..3d400b1 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -209,7 +209,6 @@ struct omap_i2c_dev { > u16 pscstate; > u16 scllstate; > u16 sclhstate; > - u16 bufstate; > u16 syscstate; > u16 westate; > u16 errata; > @@ -285,9 +284,31 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg) > } > } > > +static void __omap_i2c_init(struct omap_i2c_dev *dev) > +{ > + > + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); > + /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */ > + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate); > + > + /* SCL low and high time values */ > + omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate); > + omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate); > + if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) > + omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate); > + /* Take the I2C module out of reset: */ > + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); > + /* > + * Don't write to this register if the IE state is 0 as it can > + * cause deadlock. > + */ > + if (dev->iestate) > + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate); you still miss the extra blank lines here. Try something like below: omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); /* Setup clock prescaler to obtain approx 12MHz I2C module clock: */ omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate); /* SCL low and high time values */ omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate); omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate); if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate); /* Take the I2C module out of reset: */ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); /* * Don't write to this register if the IE state is 0 as it can * cause deadlock. */ if (dev->iestate) omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate); -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] i2c: omap: make reset a seperate function 2012-10-25 10:23 [PATCH 0/2] i2c: omap: cleanups Shubhrajyoti D 2012-10-25 10:23 ` [PATCH 1/2] i2c: omap: re-factor omap_i2c_init function Shubhrajyoti D @ 2012-10-25 10:23 ` Shubhrajyoti D [not found] ` <1351160586-12306-3-git-send-email-shubhrajyoti-l0cyMroinI0@public.gmane.org> 1 sibling, 1 reply; 6+ messages in thread From: Shubhrajyoti D @ 2012-10-25 10:23 UTC (permalink / raw) To: linux-omap Cc: linux-i2c, linux-arm-kernel, ben-linux, tony, w.sang, Shubhrajyoti D Implement reset as a seperate function. This will enable us to make sure that we don't do the calculation again on every transfer. Also at probe the reset is not added as the hwmod is doing that for us. Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> --- drivers/i2c/busses/i2c-omap.c | 28 ++++++++++++++++++---------- 1 files changed, 18 insertions(+), 10 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 3d400b1..5a87ff9 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -306,15 +306,9 @@ static void __omap_i2c_init(struct omap_i2c_dev *dev) omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate); } -static int omap_i2c_init(struct omap_i2c_dev *dev) +static int omap_i2c_reset(struct omap_i2c_dev *dev) { - u16 psc = 0, scll = 0, sclh = 0; - u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0; - unsigned long fclk_rate = 12000000; unsigned long timeout; - unsigned long internal_clk = 0; - struct clk *fclk; - if (dev->rev >= OMAP_I2C_OMAP1_REV_2) { /* Disable I2C controller before soft reset */ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, @@ -360,6 +354,17 @@ static int omap_i2c_init(struct omap_i2c_dev *dev) dev->westate = OMAP_I2C_WE_ALL; } } + return 0; +} + +static int omap_i2c_init(struct omap_i2c_dev *dev) +{ + u16 psc = 0, scll = 0, sclh = 0; + u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0; + unsigned long fclk_rate = 12000000; + unsigned long internal_clk = 0; + struct clk *fclk; + if (dev->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) { /* @@ -592,7 +597,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, if (timeout == 0) { dev_err(dev->dev, "controller timed out\n"); ret = -ETIMEDOUT; - omap_i2c_init(dev); + omap_i2c_reset(dev); + __omap_i2c_init(dev); goto out; } @@ -603,7 +609,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, || (dev->cmd_err & OMAP_I2C_STAT_ROVR) || (dev->cmd_err & OMAP_I2C_STAT_XUDF)) { ret = -EIO; - omap_i2c_init(dev); + omap_i2c_reset(dev); + __omap_i2c_init(dev); goto out; } @@ -621,7 +628,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, return 0; ret = -EREMOTEIO; - omap_i2c_init(dev); + omap_i2c_reset(dev); + __omap_i2c_init(dev); } out: -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
[parent not found: <1351160586-12306-3-git-send-email-shubhrajyoti-l0cyMroinI0@public.gmane.org>]
* Re: [PATCH 2/2] i2c: omap: make reset a seperate function [not found] ` <1351160586-12306-3-git-send-email-shubhrajyoti-l0cyMroinI0@public.gmane.org> @ 2012-10-25 10:55 ` Felipe Balbi [not found] ` <20121025105516.GH21217-S8G//mZuvNWo5Im9Ml3/Zg@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Felipe Balbi @ 2012-10-25 10:55 UTC (permalink / raw) To: Shubhrajyoti D Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, ben-linux-elnMNo+KYs3YtjvyW6yDsg, tony-4v6yS6AI5VpBDgjK7y7TUQ, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ, Benoit Cousson [-- Attachment #1: Type: text/plain, Size: 1675 bytes --] Hi, On Thu, Oct 25, 2012 at 03:53:06PM +0530, Shubhrajyoti D wrote: > Implement reset as a seperate function. > This will enable us to make sure that we don't do the > calculation again on every transfer. > Also at probe the reset is not added as the hwmod is doing that > for us. since you're touching registers which supposedly only hwmod should touch, you ought to Cc Benoit to make sure he knows what's you're doing here. I'm adding him to Cc > @@ -592,7 +597,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, > if (timeout == 0) { > dev_err(dev->dev, "controller timed out\n"); > ret = -ETIMEDOUT; > - omap_i2c_init(dev); > + omap_i2c_reset(dev); > + __omap_i2c_init(dev); > goto out; > } > > @@ -603,7 +609,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, > || (dev->cmd_err & OMAP_I2C_STAT_ROVR) > || (dev->cmd_err & OMAP_I2C_STAT_XUDF)) { > ret = -EIO; > - omap_i2c_init(dev); > + omap_i2c_reset(dev); > + __omap_i2c_init(dev); > goto out; > } > > @@ -621,7 +628,8 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, > return 0; > > ret = -EREMOTEIO; > - omap_i2c_init(dev); > + omap_i2c_reset(dev); > + __omap_i2c_init(dev); eventually we need to try to forcefully trigger these errors above (nack, overflow, underflow and arbitration lost) and try to make sure if actually need to reset the controller all the time. I find it really odd that we're always resetting the IP in every error condition without actually trying to figure out what's wrong with the driver (if there is something wrong with the driver, of course). -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20121025105516.GH21217-S8G//mZuvNWo5Im9Ml3/Zg@public.gmane.org>]
* Re: [PATCH 2/2] i2c: omap: make reset a seperate function [not found] ` <20121025105516.GH21217-S8G//mZuvNWo5Im9Ml3/Zg@public.gmane.org> @ 2012-10-25 13:47 ` Shubhrajyoti 0 siblings, 0 replies; 6+ messages in thread From: Shubhrajyoti @ 2012-10-25 13:47 UTC (permalink / raw) To: balbi-l0cyMroinI0 Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, ben-linux-elnMNo+KYs3YtjvyW6yDsg, tony-4v6yS6AI5VpBDgjK7y7TUQ, w.sang-bIcnvbaLZ9MEGnE8C9+IrQ, Benoit Cousson On Thursday 25 October 2012 04:25 PM, Felipe Balbi wrote: > overflow, underflow these are data errors personally feel may be removed. > and arbitration lost) will investigate. > and try to make sure if > actually need to reset the controller all the time. I find it really odd > that we're always resetting the IP in every error condition without > actually trying to figure out what's wrong with the driver (if there is > something wrong with the driver, of course). ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-25 13:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-25 10:23 [PATCH 0/2] i2c: omap: cleanups Shubhrajyoti D
2012-10-25 10:23 ` [PATCH 1/2] i2c: omap: re-factor omap_i2c_init function Shubhrajyoti D
2012-10-25 10:52 ` Felipe Balbi
2012-10-25 10:23 ` [PATCH 2/2] i2c: omap: make reset a seperate function Shubhrajyoti D
[not found] ` <1351160586-12306-3-git-send-email-shubhrajyoti-l0cyMroinI0@public.gmane.org>
2012-10-25 10:55 ` Felipe Balbi
[not found] ` <20121025105516.GH21217-S8G//mZuvNWo5Im9Ml3/Zg@public.gmane.org>
2012-10-25 13:47 ` Shubhrajyoti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).