[RFC] i2c-omap: Don't wait needlessly

[RFC] i2c-omap: Don't wait needlessly

[Sakari Ailus]

When the I2C controller is initialised, software reset is performed to
the controller. I've never seen that taking more than 1 ns while the
msleep call causes excessive delays in I2C transfers.

Tested on OMAP 3.

Signed-off-by: Sakari Ailus <sakari.ailus@nokia.com>
---
 drivers/i2c/busses/i2c-omap.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index b2b0e2f..b19b851 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -40,6 +40,8 @@
 
 /* timeout waiting for the controller to respond */
 #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))
+/* no more busyloop in ns */
+#define OMAP_I2C_MINOR_TIMEOUT		10
 
 #define OMAP_I2C_REV_REG		0x00
 #define OMAP_I2C_IE_REG			0x04
@@ -255,6 +257,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	unsigned long internal_clk = 0;
 
 	if (!dev->rev1) {
+		int delay = 0;
+
 		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, OMAP_I2C_SYSC_SRST);
 		/* For some reason we need to set the EN bit before the
 		 * reset done bit gets set. */
@@ -262,6 +266,14 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 		while (!(omap_i2c_read_reg(dev, OMAP_I2C_SYSS_REG) &
 			 OMAP_I2C_SYSS_RDONE)) {
+			if (delay < OMAP_I2C_MINOR_TIMEOUT) {
+				ndelay(1);
+				delay++;
+				continue;
+			} else if (delay == OMAP_I2C_MINOR_TIMEOUT) {
+				dev_warn(dev->dev, "minor timeout not enough");
+				delay++;
+			}
 			if (time_after(jiffies, timeout)) {
 				dev_warn(dev->dev, "timeout waiting "
 						"for controller reset\n");
-- 
1.5.0.6

Re: [RFC] i2c-omap: Don't wait needlessly

[Jarkko Nikula]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

On Tue, 21 Oct 2008 19:19:08 +0300
"ext Sakari Ailus" <sakari.ailus@nokia.com> wrote:

>  /* timeout waiting for the controller to respond */
>  #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))
> +/* no more busyloop in ns */
> +#define OMAP_I2C_MINOR_TIMEOUT		10
>  
>  #define OMAP_I2C_REV_REG		0x00
>  #define OMAP_I2C_IE_REG			0x04
> @@ -255,6 +257,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  	unsigned long internal_clk = 0;
>  
>  	if (!dev->rev1) {
> +		int delay = 0;
> +
>  		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, OMAP_I2C_SYSC_SRST);
>  		/* For some reason we need to set the EN bit before the
>  		 * reset done bit gets set. */
> @@ -262,6 +266,14 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>  		while (!(omap_i2c_read_reg(dev, OMAP_I2C_SYSS_REG) &
>  			 OMAP_I2C_SYSS_RDONE)) {
> +			if (delay < OMAP_I2C_MINOR_TIMEOUT) {
> +				ndelay(1);
> +				delay++;
> +				continue;
> +			} else if (delay == OMAP_I2C_MINOR_TIMEOUT) {
> +				dev_warn(dev->dev, "minor timeout not enough");
> +				delay++;
> +			}
>  			if (time_after(jiffies, timeout)) {
>  				dev_warn(dev->dev, "timeout waiting "
>  						"for controller reset\n");
>
I would rather, if there is no need for such a long delay like
OMAP_I2C_TIMEOUT, remove that time_after and msleep(1) stuff and
just loop few iterations with udelay(1). Zero thinked & tested diff
attached.

I would say that ndelay(1) just doesn't look relevant to < 1 GHz
cpus :-)


Jarkko

[-- Attachment #2: hack.diff --]
[-- Type: text/x-diff, Size: 766 bytes --]

--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -248,16 +248,16 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, OMAP_I2C_SYSC_SRST);
 		/* For some reason we need to set the EN bit before the
 		 * reset done bit gets set. */
-		timeout = jiffies + OMAP_I2C_TIMEOUT;
+		timeout = 10;
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 		while (!(omap_i2c_read_reg(dev, OMAP_I2C_SYSS_REG) &
 			 OMAP_I2C_SYSS_RDONE)) {
-			if (time_after(jiffies, timeout)) {
+			udelay(1);
+			if (--timeout) {
 				dev_warn(dev->dev, "timeout waiting "
 						"for controller reset\n");
 				return -ETIMEDOUT;
 			}
-			msleep(1);
 		}
 	}
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);

Re: [RFC] i2c-omap: Don't wait needlessly

[Sakari Ailus]

Jarkko Nikula wrote:
> I would rather, if there is no need for such a long delay like
> OMAP_I2C_TIMEOUT, remove that time_after and msleep(1) stuff and
> just loop few iterations with udelay(1). Zero thinked & tested diff
> attached.

I just though of allowing the reset to take longer as I have no idea how 
long it could take, let alone other versions of OMAP.

> I would say that ndelay(1) just doesn't look relevant to < 1 GHz
> cpus :-)

Good point. On ARM ndelay(1) seems to be equal to udelay(1) at the moment.

I actually just removed the ndelay(1) and again, "delay" won't get past 
1 if I print it after the loop.

It'd be nice to know how it works on other OMAP versions before making 
such changes. :-)

-- 
Sakari Ailus
sakari.ailus@nokia.com

Re: [RFC] i2c-omap: Don't wait needlessly

[Tony Lindgren]

* Sakari Ailus <sakari.ailus@nokia.com> [081027 09:22]:
> Jarkko Nikula wrote:
>> I would rather, if there is no need for such a long delay like
>> OMAP_I2C_TIMEOUT, remove that time_after and msleep(1) stuff and
>> just loop few iterations with udelay(1). Zero thinked & tested diff
>> attached.
>
> I just though of allowing the reset to take longer as I have no idea how  
> long it could take, let alone other versions of OMAP.
>
>> I would say that ndelay(1) just doesn't look relevant to < 1 GHz
>> cpus :-)
>
> Good point. On ARM ndelay(1) seems to be equal to udelay(1) at the moment.
>
> I actually just removed the ndelay(1) and again, "delay" won't get past  
> 1 if I print it after the loop.
>
> It'd be nice to know how it works on other OMAP versions before making  
> such changes. :-)

Let me know if you come up with a refreshed patch for this, ignoring
for now.

Tony

Re: [RFC] i2c-omap: Don't wait needlessly

[Sakari Ailus]

ext Tony Lindgren wrote:
> * Sakari Ailus <sakari.ailus@nokia.com> [081027 09:22]:
>> Jarkko Nikula wrote:
>>> I would rather, if there is no need for such a long delay like
>>> OMAP_I2C_TIMEOUT, remove that time_after and msleep(1) stuff and
>>> just loop few iterations with udelay(1). Zero thinked & tested diff
>>> attached.
>> I just though of allowing the reset to take longer as I have no idea how  
>> long it could take, let alone other versions of OMAP.
>>
>>> I would say that ndelay(1) just doesn't look relevant to < 1 GHz
>>> cpus :-)
>> Good point. On ARM ndelay(1) seems to be equal to udelay(1) at the moment.
>>
>> I actually just removed the ndelay(1) and again, "delay" won't get past  
>> 1 if I print it after the loop.
>>
>> It'd be nice to know how it works on other OMAP versions before making  
>> such changes. :-)
> 
> Let me know if you come up with a refreshed patch for this, ignoring
> for now.

I think the need for this patch has largely gone away as i2c-omap 
appears not to do msleep() anymore for every second transfer or so.

-- 
Sakari Ailus
sakari.ailus@nokia.com