[PATCH 0/1] OMAP: I2C: Add mpu wake up latency constraint

[PATCH 0/1] OMAP: I2C: Add mpu wake up latency constraint

[Kalle Jokiniemi]

Sending this patch that fixes a problem where I2C transfers get stalled
because MPU can hit off mode while i2c is waiting for transfer
completion.

There are some changes on how clkrate is passed to i2c-omap bus
driver, as I needed to pass also the constraint setting function.
So review comments are most welcome.

Applies on top of pm-branch. Just build tested for
omap3_pm_defconfig. Could not get linux-omap to boot
on my rx-51, so it's only tested with our internal tree.

- Kalle

[PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Kalle Jokiniemi]

While waiting for completion of the i2c transfer, the
MPU could hit OFF mode and cause several msecs of
delay that made i2c transfers fail more often. The
extra delays and subsequent re-trys cause i2c clocks
to be active more often. This has also an negative
effect on power consumption.

Added a constraint that allows MPU to wake up in few
hundred usecs, which is roughly the average i2c wait
period.

The constraint function is passed as platform data from
plat-omap/i2c.c and applied only on OMAP34XX devices.

Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
---
 arch/arm/plat-omap/i2c.c      |   49 +++++++++++++++++++++++++++++++----------
 drivers/i2c/busses/i2c-omap.c |   17 +++++++++++---
 include/linux/i2c-omap.h      |    9 +++++++
 3 files changed, 59 insertions(+), 16 deletions(-)
 create mode 100644 include/linux/i2c-omap.h

diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
index 8b84839..d43d0ad 100644
--- a/arch/arm/plat-omap/i2c.c
+++ b/arch/arm/plat-omap/i2c.c
@@ -26,8 +26,10 @@
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 #include <linux/i2c.h>
+#include <linux/i2c-omap.h>
 #include <mach/irqs.h>
 #include <mach/mux.h>
+#include <mach/omap-pm.h>
 
 #define OMAP_I2C_SIZE		0x3f
 #define OMAP1_I2C_BASE		0xfffb3800
@@ -69,14 +71,14 @@ static struct resource i2c_resources[][2] = {
 		},					\
 	}
 
-static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
+static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
 static struct platform_device omap_i2c_devices[] = {
-	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
+	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
 #if	defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
-	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
+	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
 #endif
 #if	defined(CONFIG_ARCH_OMAP34XX)
-	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
+	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
 #endif
 };
 
@@ -100,6 +102,25 @@ static const int omap34xx_pins[][2] = {};
 
 #define OMAP_I2C_CMDLINE_SETUP	(BIT(31))
 
+/**
+ * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
+ * @dev:	i2c bus device pointer
+ * @set:	set or clear constraints
+ *
+ * When waiting for completion of a i2c transfer, we need to set a wake up
+ * latency constraint for the MPU. This is to ensure quick enough wakeup
+ * from idle, when transfer completes.
+ */
+#ifdef CONFIG_ARCH_OMAP34XX
+static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
+{
+	omap_pm_set_max_mpu_wakeup_lat(dev, set ? 500 : -1);
+}
+#else
+static inline void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
+{; }
+#endif
+
 static void __init omap_i2c_mux_pins(int bus)
 {
 	int scl, sda;
@@ -180,8 +201,8 @@ static int __init omap_i2c_bus_setup(char *str)
 	get_options(str, 3, ints);
 	if (ints[0] < 2 || ints[1] < 1 || ints[1] > ports)
 		return 0;
-	i2c_rate[ints[1] - 1] = ints[2];
-	i2c_rate[ints[1] - 1] |= OMAP_I2C_CMDLINE_SETUP;
+	i2c_pdata[ints[1] - 1].clkrate = ints[2];
+	i2c_pdata[ints[1] - 1].clkrate |= OMAP_I2C_CMDLINE_SETUP;
 
 	return 1;
 }
@@ -195,9 +216,11 @@ static int __init omap_register_i2c_bus_cmdline(void)
 {
 	int i, err = 0;
 
-	for (i = 0; i < ARRAY_SIZE(i2c_rate); i++)
-		if (i2c_rate[i] & OMAP_I2C_CMDLINE_SETUP) {
-			i2c_rate[i] &= ~OMAP_I2C_CMDLINE_SETUP;
+	for (i = 0; i < ARRAY_SIZE(i2c_pdata); i++)
+		if (i2c_pdata[i].clkrate & OMAP_I2C_CMDLINE_SETUP) {
+			i2c_pdata[i].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
+			i2c_pdata[i].set_mpu_wkup_lat =
+						omap_i2c_set_wfc_mpu_wkup_lat;
 			err = omap_i2c_add_bus(i + 1);
 			if (err)
 				goto out;
@@ -231,9 +254,11 @@ int __init omap_register_i2c_bus(int bus_id, u32 clkrate,
 			return err;
 	}
 
-	if (!i2c_rate[bus_id - 1])
-		i2c_rate[bus_id - 1] = clkrate;
-	i2c_rate[bus_id - 1] &= ~OMAP_I2C_CMDLINE_SETUP;
+	if (!i2c_pdata[bus_id - 1].clkrate)
+		i2c_pdata[bus_id - 1].clkrate = clkrate;
+
+	i2c_pdata[bus_id - 1].set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
+	i2c_pdata[bus_id - 1].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
 
 	return omap_i2c_add_bus(bus_id);
 }
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 75bf3ad..da6d276 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -37,6 +37,7 @@
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/i2c-omap.h>
 
 /* I2C controller revisions */
 #define OMAP_I2C_REV_2			0x20
@@ -165,6 +166,8 @@ struct omap_i2c_dev {
 	struct clk		*fclk;		/* Functional clock */
 	struct completion	cmd_complete;
 	struct resource		*ioarea;
+	void			(*set_mpu_wkup_lat)(struct device *dev,
+						    int set);
 	u32			speed;		/* Speed of bus in Khz */
 	u16			cmd_err;
 	u8			*buf;
@@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	 * REVISIT: We should abort the transfer on signals, but the bus goes
 	 * into arbitration and we're currently unable to recover from it.
 	 */
+	dev->set_mpu_wkup_lat(dev->dev, 1);
 	r = wait_for_completion_timeout(&dev->cmd_complete,
 					OMAP_I2C_TIMEOUT);
+	dev->set_mpu_wkup_lat(dev->dev, 0);
 	dev->buf_len = 0;
 	if (r < 0)
 		return r;
@@ -844,6 +849,7 @@ omap_i2c_probe(struct platform_device *pdev)
 	struct omap_i2c_dev	*dev;
 	struct i2c_adapter	*adap;
 	struct resource		*mem, *irq, *ioarea;
+	struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
 	irq_handler_t isr;
 	int r;
 	u32 speed = 0;
@@ -873,10 +879,13 @@ omap_i2c_probe(struct platform_device *pdev)
 		goto err_release_region;
 	}
 
-	if (pdev->dev.platform_data != NULL)
-		speed = *(u32 *)pdev->dev.platform_data;
-	else
-		speed = 100;	/* Defualt speed */
+	if (pdata != NULL) {
+		speed = pdata->clkrate;
+		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
+	} else {
+		speed = 100;	/* Default speed */
+		dev->set_mpu_wkup_lat = NULL;
+	}
 
 	dev->speed = speed;
 	dev->idle = 1;
diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
new file mode 100644
index 0000000..1362fba
--- /dev/null
+++ b/include/linux/i2c-omap.h
@@ -0,0 +1,9 @@
+#ifndef __I2C_OMAP_H__
+#define __I2C_OMAP_H__
+
+struct omap_i2c_bus_platform_data {
+	u32		clkrate;
+	void		(*set_mpu_wkup_lat)(struct device *dev, int set);
+};
+
+#endif
-- 
1.5.4.3

Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Kevin Hilman]

Kalle Jokiniemi <kalle.jokiniemi@digia.com> writes:

> While waiting for completion of the i2c transfer, the
> MPU could hit OFF mode and cause several msecs of
> delay that made i2c transfers fail more often. The
> extra delays and subsequent re-trys cause i2c clocks
> to be active more often. This has also an negative
> effect on power consumption.
>
> Added a constraint that allows MPU to wake up in few
> hundred usecs, which is roughly the average i2c wait
> period.
>
> The constraint function is passed as platform data from
> plat-omap/i2c.c and applied only on OMAP34XX devices.

Seems like the latency value should also be (optionally) passed in
pdata so this can be experimented with per-platform.

Other than that looks ok to me.

Do we have an official OMAP I2C maintainer who should signoff on this?

Kevin


> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
> ---
>  arch/arm/plat-omap/i2c.c      |   49 +++++++++++++++++++++++++++++++----------
>  drivers/i2c/busses/i2c-omap.c |   17 +++++++++++---
>  include/linux/i2c-omap.h      |    9 +++++++
>  3 files changed, 59 insertions(+), 16 deletions(-)
>  create mode 100644 include/linux/i2c-omap.h
>
> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> index 8b84839..d43d0ad 100644
> --- a/arch/arm/plat-omap/i2c.c
> +++ b/arch/arm/plat-omap/i2c.c
> @@ -26,8 +26,10 @@
>  #include <linux/kernel.h>
>  #include <linux/platform_device.h>
>  #include <linux/i2c.h>
> +#include <linux/i2c-omap.h>
>  #include <mach/irqs.h>
>  #include <mach/mux.h>
> +#include <mach/omap-pm.h>
>  
>  #define OMAP_I2C_SIZE		0x3f
>  #define OMAP1_I2C_BASE		0xfffb3800
> @@ -69,14 +71,14 @@ static struct resource i2c_resources[][2] = {
>  		},					\
>  	}
>  
> -static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> +static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
>  static struct platform_device omap_i2c_devices[] = {
> -	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> +	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
>  #if	defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> -	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> +	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
>  #endif
>  #if	defined(CONFIG_ARCH_OMAP34XX)
> -	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> +	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
>  #endif
>  };
>  
> @@ -100,6 +102,25 @@ static const int omap34xx_pins[][2] = {};
>  
>  #define OMAP_I2C_CMDLINE_SETUP	(BIT(31))
>  
> +/**
> + * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
> + * @dev:	i2c bus device pointer
> + * @set:	set or clear constraints
> + *
> + * When waiting for completion of a i2c transfer, we need to set a wake up
> + * latency constraint for the MPU. This is to ensure quick enough wakeup
> + * from idle, when transfer completes.
> + */
> +#ifdef CONFIG_ARCH_OMAP34XX
> +static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> +{
> +	omap_pm_set_max_mpu_wakeup_lat(dev, set ? 500 : -1);
> +}
> +#else
> +static inline void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> +{; }
> +#endif
> +
>  static void __init omap_i2c_mux_pins(int bus)
>  {
>  	int scl, sda;
> @@ -180,8 +201,8 @@ static int __init omap_i2c_bus_setup(char *str)
>  	get_options(str, 3, ints);
>  	if (ints[0] < 2 || ints[1] < 1 || ints[1] > ports)
>  		return 0;
> -	i2c_rate[ints[1] - 1] = ints[2];
> -	i2c_rate[ints[1] - 1] |= OMAP_I2C_CMDLINE_SETUP;
> +	i2c_pdata[ints[1] - 1].clkrate = ints[2];
> +	i2c_pdata[ints[1] - 1].clkrate |= OMAP_I2C_CMDLINE_SETUP;
>  
>  	return 1;
>  }
> @@ -195,9 +216,11 @@ static int __init omap_register_i2c_bus_cmdline(void)
>  {
>  	int i, err = 0;
>  
> -	for (i = 0; i < ARRAY_SIZE(i2c_rate); i++)
> -		if (i2c_rate[i] & OMAP_I2C_CMDLINE_SETUP) {
> -			i2c_rate[i] &= ~OMAP_I2C_CMDLINE_SETUP;
> +	for (i = 0; i < ARRAY_SIZE(i2c_pdata); i++)
> +		if (i2c_pdata[i].clkrate & OMAP_I2C_CMDLINE_SETUP) {
> +			i2c_pdata[i].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
> +			i2c_pdata[i].set_mpu_wkup_lat =
> +						omap_i2c_set_wfc_mpu_wkup_lat;
>  			err = omap_i2c_add_bus(i + 1);
>  			if (err)
>  				goto out;
> @@ -231,9 +254,11 @@ int __init omap_register_i2c_bus(int bus_id, u32 clkrate,
>  			return err;
>  	}
>  
> -	if (!i2c_rate[bus_id - 1])
> -		i2c_rate[bus_id - 1] = clkrate;
> -	i2c_rate[bus_id - 1] &= ~OMAP_I2C_CMDLINE_SETUP;
> +	if (!i2c_pdata[bus_id - 1].clkrate)
> +		i2c_pdata[bus_id - 1].clkrate = clkrate;
> +
> +	i2c_pdata[bus_id - 1].set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
> +	i2c_pdata[bus_id - 1].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
>  
>  	return omap_i2c_add_bus(bus_id);
>  }
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 75bf3ad..da6d276 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -37,6 +37,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/i2c-omap.h>
>  
>  /* I2C controller revisions */
>  #define OMAP_I2C_REV_2			0x20
> @@ -165,6 +166,8 @@ struct omap_i2c_dev {
>  	struct clk		*fclk;		/* Functional clock */
>  	struct completion	cmd_complete;
>  	struct resource		*ioarea;
> +	void			(*set_mpu_wkup_lat)(struct device *dev,
> +						    int set);
>  	u32			speed;		/* Speed of bus in Khz */
>  	u16			cmd_err;
>  	u8			*buf;
> @@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>  	 * REVISIT: We should abort the transfer on signals, but the bus goes
>  	 * into arbitration and we're currently unable to recover from it.
>  	 */
> +	dev->set_mpu_wkup_lat(dev->dev, 1);
>  	r = wait_for_completion_timeout(&dev->cmd_complete,
>  					OMAP_I2C_TIMEOUT);
> +	dev->set_mpu_wkup_lat(dev->dev, 0);
>  	dev->buf_len = 0;
>  	if (r < 0)
>  		return r;
> @@ -844,6 +849,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  	struct omap_i2c_dev	*dev;
>  	struct i2c_adapter	*adap;
>  	struct resource		*mem, *irq, *ioarea;
> +	struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
>  	irq_handler_t isr;
>  	int r;
>  	u32 speed = 0;
> @@ -873,10 +879,13 @@ omap_i2c_probe(struct platform_device *pdev)
>  		goto err_release_region;
>  	}
>  
> -	if (pdev->dev.platform_data != NULL)
> -		speed = *(u32 *)pdev->dev.platform_data;
> -	else
> -		speed = 100;	/* Defualt speed */
> +	if (pdata != NULL) {
> +		speed = pdata->clkrate;
> +		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> +	} else {
> +		speed = 100;	/* Default speed */
> +		dev->set_mpu_wkup_lat = NULL;
> +	}
>  
>  	dev->speed = speed;
>  	dev->idle = 1;
> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> new file mode 100644
> index 0000000..1362fba
> --- /dev/null
> +++ b/include/linux/i2c-omap.h
> @@ -0,0 +1,9 @@
> +#ifndef __I2C_OMAP_H__
> +#define __I2C_OMAP_H__
> +
> +struct omap_i2c_bus_platform_data {
> +	u32		clkrate;
> +	void		(*set_mpu_wkup_lat)(struct device *dev, int set);
> +};
> +
> +#endif
> -- 
> 1.5.4.3

Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Jarkko Nikula]

Hi Kalle

Few minor comments below.

On Thu, 17 Sep 2009 19:28:43 +0300
Kalle Jokiniemi <kalle.jokiniemi@digia.com> wrote:

> -static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> +static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
>  static struct platform_device omap_i2c_devices[] = {
> -	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> +	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
>  #if	defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> -	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> +	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
>  #endif
>  #if	defined(CONFIG_ARCH_OMAP34XX)
> -	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> +	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
>  #endif
>  };
This is good idea. IRCC, e.g. the I2C_SCL t_high and t_low timing values
can be board dependent values so passing just the i2c_rate as a
platform_data seems to be not enough in the future.

> @@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>  	 * REVISIT: We should abort the transfer on signals, but the bus goes
>  	 * into arbitration and we're currently unable to recover from it.
>  	 */
> +	dev->set_mpu_wkup_lat(dev->dev, 1);
>  	r = wait_for_completion_timeout(&dev->cmd_complete,
>  					OMAP_I2C_TIMEOUT);
> +	dev->set_mpu_wkup_lat(dev->dev, 0);

These lines here and lines below in omap_i2c_probe looks suspicious. I
would rather allow callback to be NULL (and then you would not need
empty function in arch/arm/plat-omap/i2c.c) and just test it here
before executing. Looks much more safe code then.

> +	if (pdata != NULL) {
> +		speed = pdata->clkrate;
> +		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> +	} else {
> +		speed = 100;	/* Default speed */
> +		dev->set_mpu_wkup_lat = NULL;
> +	}


-- 
Jarkko

Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Kalle Jokiniemi]

On Wed, 2009-09-30 at 19:36 +0300, Kevin Hilman wrote:
> Kalle Jokiniemi <kalle.jokiniemi@digia.com> writes:
> 
> > While waiting for completion of the i2c transfer, the
> > MPU could hit OFF mode and cause several msecs of
> > delay that made i2c transfers fail more often. The
> > extra delays and subsequent re-trys cause i2c clocks
> > to be active more often. This has also an negative
> > effect on power consumption.
> >
> > Added a constraint that allows MPU to wake up in few
> > hundred usecs, which is roughly the average i2c wait
> > period.
> >
> > The constraint function is passed as platform data from
> > plat-omap/i2c.c and applied only on OMAP34XX devices.
> 
> Seems like the latency value should also be (optionally) passed in
> pdata so this can be experimented with per-platform.

Well, it kind of is already, since we pass the function that sets the
latency from platform code. And that function has the latency
hard-coded. My though was to use little #ifdeffery in defining the
function (and latency) for say omap3, and omap4 platforms.

If #ifdeffery is out of the question, we could define
omap3_i2c_set_mpu.. and omap4_i2c_set_mpu.. functions and pass one in
init according to some "if (cpu_is_omapxyz)" statement. I think omap1&2
don't need any latency constraints (and cannot use them as there is no
pm-layer implementation for those).

What do you think?

- Kalle

> 
> Other than that looks ok to me.
> 
> Do we have an official OMAP I2C maintainer who should signoff on this?
> 
> Kevin
> 
> 
> > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
> > ---
> >  arch/arm/plat-omap/i2c.c      |   49 +++++++++++++++++++++++++++++++----------
> >  drivers/i2c/busses/i2c-omap.c |   17 +++++++++++---
> >  include/linux/i2c-omap.h      |    9 +++++++
> >  3 files changed, 59 insertions(+), 16 deletions(-)
> >  create mode 100644 include/linux/i2c-omap.h
> >
> > diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> > index 8b84839..d43d0ad 100644
> > --- a/arch/arm/plat-omap/i2c.c
> > +++ b/arch/arm/plat-omap/i2c.c
> > @@ -26,8 +26,10 @@
> >  #include <linux/kernel.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/i2c.h>
> > +#include <linux/i2c-omap.h>
> >  #include <mach/irqs.h>
> >  #include <mach/mux.h>
> > +#include <mach/omap-pm.h>
> >  
> >  #define OMAP_I2C_SIZE		0x3f
> >  #define OMAP1_I2C_BASE		0xfffb3800
> > @@ -69,14 +71,14 @@ static struct resource i2c_resources[][2] = {
> >  		},					\
> >  	}
> >  
> > -static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> > +static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
> >  static struct platform_device omap_i2c_devices[] = {
> > -	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> > +	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
> >  #if	defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> > -	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> > +	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
> >  #endif
> >  #if	defined(CONFIG_ARCH_OMAP34XX)
> > -	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> > +	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
> >  #endif
> >  };
> >  
> > @@ -100,6 +102,25 @@ static const int omap34xx_pins[][2] = {};
> >  
> >  #define OMAP_I2C_CMDLINE_SETUP	(BIT(31))
> >  
> > +/**
> > + * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
> > + * @dev:	i2c bus device pointer
> > + * @set:	set or clear constraints
> > + *
> > + * When waiting for completion of a i2c transfer, we need to set a wake up
> > + * latency constraint for the MPU. This is to ensure quick enough wakeup
> > + * from idle, when transfer completes.
> > + */
> > +#ifdef CONFIG_ARCH_OMAP34XX
> > +static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> > +{
> > +	omap_pm_set_max_mpu_wakeup_lat(dev, set ? 500 : -1);
> > +}
> > +#else
> > +static inline void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> > +{; }
> > +#endif
> > +
> >  static void __init omap_i2c_mux_pins(int bus)
> >  {
> >  	int scl, sda;
> > @@ -180,8 +201,8 @@ static int __init omap_i2c_bus_setup(char *str)
> >  	get_options(str, 3, ints);
> >  	if (ints[0] < 2 || ints[1] < 1 || ints[1] > ports)
> >  		return 0;
> > -	i2c_rate[ints[1] - 1] = ints[2];
> > -	i2c_rate[ints[1] - 1] |= OMAP_I2C_CMDLINE_SETUP;
> > +	i2c_pdata[ints[1] - 1].clkrate = ints[2];
> > +	i2c_pdata[ints[1] - 1].clkrate |= OMAP_I2C_CMDLINE_SETUP;
> >  
> >  	return 1;
> >  }
> > @@ -195,9 +216,11 @@ static int __init omap_register_i2c_bus_cmdline(void)
> >  {
> >  	int i, err = 0;
> >  
> > -	for (i = 0; i < ARRAY_SIZE(i2c_rate); i++)
> > -		if (i2c_rate[i] & OMAP_I2C_CMDLINE_SETUP) {
> > -			i2c_rate[i] &= ~OMAP_I2C_CMDLINE_SETUP;
> > +	for (i = 0; i < ARRAY_SIZE(i2c_pdata); i++)
> > +		if (i2c_pdata[i].clkrate & OMAP_I2C_CMDLINE_SETUP) {
> > +			i2c_pdata[i].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
> > +			i2c_pdata[i].set_mpu_wkup_lat =
> > +						omap_i2c_set_wfc_mpu_wkup_lat;
> >  			err = omap_i2c_add_bus(i + 1);
> >  			if (err)
> >  				goto out;
> > @@ -231,9 +254,11 @@ int __init omap_register_i2c_bus(int bus_id, u32 clkrate,
> >  			return err;
> >  	}
> >  
> > -	if (!i2c_rate[bus_id - 1])
> > -		i2c_rate[bus_id - 1] = clkrate;
> > -	i2c_rate[bus_id - 1] &= ~OMAP_I2C_CMDLINE_SETUP;
> > +	if (!i2c_pdata[bus_id - 1].clkrate)
> > +		i2c_pdata[bus_id - 1].clkrate = clkrate;
> > +
> > +	i2c_pdata[bus_id - 1].set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
> > +	i2c_pdata[bus_id - 1].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
> >  
> >  	return omap_i2c_add_bus(bus_id);
> >  }
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index 75bf3ad..da6d276 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -37,6 +37,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> > +#include <linux/i2c-omap.h>
> >  
> >  /* I2C controller revisions */
> >  #define OMAP_I2C_REV_2			0x20
> > @@ -165,6 +166,8 @@ struct omap_i2c_dev {
> >  	struct clk		*fclk;		/* Functional clock */
> >  	struct completion	cmd_complete;
> >  	struct resource		*ioarea;
> > +	void			(*set_mpu_wkup_lat)(struct device *dev,
> > +						    int set);
> >  	u32			speed;		/* Speed of bus in Khz */
> >  	u16			cmd_err;
> >  	u8			*buf;
> > @@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> >  	 * REVISIT: We should abort the transfer on signals, but the bus goes
> >  	 * into arbitration and we're currently unable to recover from it.
> >  	 */
> > +	dev->set_mpu_wkup_lat(dev->dev, 1);
> >  	r = wait_for_completion_timeout(&dev->cmd_complete,
> >  					OMAP_I2C_TIMEOUT);
> > +	dev->set_mpu_wkup_lat(dev->dev, 0);
> >  	dev->buf_len = 0;
> >  	if (r < 0)
> >  		return r;
> > @@ -844,6 +849,7 @@ omap_i2c_probe(struct platform_device *pdev)
> >  	struct omap_i2c_dev	*dev;
> >  	struct i2c_adapter	*adap;
> >  	struct resource		*mem, *irq, *ioarea;
> > +	struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
> >  	irq_handler_t isr;
> >  	int r;
> >  	u32 speed = 0;
> > @@ -873,10 +879,13 @@ omap_i2c_probe(struct platform_device *pdev)
> >  		goto err_release_region;
> >  	}
> >  
> > -	if (pdev->dev.platform_data != NULL)
> > -		speed = *(u32 *)pdev->dev.platform_data;
> > -	else
> > -		speed = 100;	/* Defualt speed */
> > +	if (pdata != NULL) {
> > +		speed = pdata->clkrate;
> > +		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> > +	} else {
> > +		speed = 100;	/* Default speed */
> > +		dev->set_mpu_wkup_lat = NULL;
> > +	}
> >  
> >  	dev->speed = speed;
> >  	dev->idle = 1;
> > diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> > new file mode 100644
> > index 0000000..1362fba
> > --- /dev/null
> > +++ b/include/linux/i2c-omap.h
> > @@ -0,0 +1,9 @@
> > +#ifndef __I2C_OMAP_H__
> > +#define __I2C_OMAP_H__
> > +
> > +struct omap_i2c_bus_platform_data {
> > +	u32		clkrate;
> > +	void		(*set_mpu_wkup_lat)(struct device *dev, int set);
> > +};
> > +
> > +#endif
> > -- 
> > 1.5.4.3

Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Kalle Jokiniemi]

On Thu, 2009-10-01 at 09:10 +0300, Jarkko Nikula wrote:
> Hi Kalle
> 
> Few minor comments below.
> 
> On Thu, 17 Sep 2009 19:28:43 +0300
> Kalle Jokiniemi <kalle.jokiniemi@digia.com> wrote:
> 
> > -static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> > +static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
> >  static struct platform_device omap_i2c_devices[] = {
> > -	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> > +	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
> >  #if	defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> > -	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> > +	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
> >  #endif
> >  #if	defined(CONFIG_ARCH_OMAP34XX)
> > -	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> > +	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
> >  #endif
> >  };
> This is good idea. IRCC, e.g. the I2C_SCL t_high and t_low timing values
> can be board dependent values so passing just the i2c_rate as a
> platform_data seems to be not enough in the future.

I gather that this t_high/t_low passing change can be added later ;)


> 
> > @@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> >  	 * REVISIT: We should abort the transfer on signals, but the bus goes
> >  	 * into arbitration and we're currently unable to recover from it.
> >  	 */
> > +	dev->set_mpu_wkup_lat(dev->dev, 1);
> >  	r = wait_for_completion_timeout(&dev->cmd_complete,
> >  					OMAP_I2C_TIMEOUT);
> > +	dev->set_mpu_wkup_lat(dev->dev, 0);
> 
> These lines here and lines below in omap_i2c_probe looks suspicious. I
> would rather allow callback to be NULL (and then you would not need
> empty function in arch/arm/plat-omap/i2c.c) and just test it here
> before executing. Looks much more safe code then.

Yes, this needs to be fixed. Otherwise we could get null pointer calls.
Nice catch.

- Kalle

> 
> > +	if (pdata != NULL) {
> > +		speed = pdata->clkrate;
> > +		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> > +	} else {
> > +		speed = 100;	/* Default speed */
> > +		dev->set_mpu_wkup_lat = NULL;
> > +	}
> 
> 

Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Aaro Koskinen]

Hello,

Kalle Jokiniemi wrote:
> On Wed, 2009-09-30 at 19:36 +0300, Kevin Hilman wrote:
>> Seems like the latency value should also be (optionally) passed in
>> pdata so this can be experimented with per-platform.
> 
> Well, it kind of is already, since we pass the function that sets the
> latency from platform code. And that function has the latency
> hard-coded.

Paul Walmsley suggested earlier that the latency value should be
calculated from bus specific parameters:

http://www.mail-archive.com/linux-omap@vger.kernel.org/msg12358.html

A.

Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Kevin Hilman]

Kalle Jokiniemi <kalle.jokiniemi@digia.com> writes:

> On Wed, 2009-09-30 at 19:36 +0300, Kevin Hilman wrote:
>> Kalle Jokiniemi <kalle.jokiniemi@digia.com> writes:
>> 
>> > While waiting for completion of the i2c transfer, the
>> > MPU could hit OFF mode and cause several msecs of
>> > delay that made i2c transfers fail more often. The
>> > extra delays and subsequent re-trys cause i2c clocks
>> > to be active more often. This has also an negative
>> > effect on power consumption.
>> >
>> > Added a constraint that allows MPU to wake up in few
>> > hundred usecs, which is roughly the average i2c wait
>> > period.
>> >
>> > The constraint function is passed as platform data from
>> > plat-omap/i2c.c and applied only on OMAP34XX devices.
>> 
>> Seems like the latency value should also be (optionally) passed in
>> pdata so this can be experimented with per-platform.
>
> Well, it kind of is already, since we pass the function that sets the
> latency from platform code. And that function has the latency
> hard-coded. 

I see it now, I initially thought that hard-coded value was in the
driver, not in plat-omap/i2c.c.

> My though was to use little #ifdeffery in defining the
> function (and latency) for say omap3, and omap4 platforms.
>
> If #ifdeffery is out of the question, we could define
> omap3_i2c_set_mpu.. and omap4_i2c_set_mpu.. functions and pass one in
> init according to some "if (cpu_is_omapxyz)" statement. I think omap1&2
> don't need any latency constraints (and cannot use them as there is no
> pm-layer implementation for those).
>
> What do you think?

I recommend setting the functions at runtime using cpu_is* and
allowing the functino to be NULL on platforms that don't need it.
Combined with the NULL check proposed by Jarkko that should work well.

Kevin

Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Kalle Jokiniemi]

On Thu, 2009-10-01 at 14:41 +0300, Aaro Koskinen wrote:
> Hello,
> 
> Kalle Jokiniemi wrote:
> > On Wed, 2009-09-30 at 19:36 +0300, Kevin Hilman wrote:
> >> Seems like the latency value should also be (optionally) passed in
> >> pdata so this can be experimented with per-platform.
> > 
> > Well, it kind of is already, since we pass the function that sets the
> > latency from platform code. And that function has the latency
> > hard-coded.
> 
> Paul Walmsley suggested earlier that the latency value should be
> calculated from bus specific parameters:
> 
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg12358.html

Yes, this is a good idea in theory, but the reality of wake-up latencies
kind-a kill this one. Wake-up from even C1 (MPU INA, CORE ON) takes
~130us on fastest OPP. And when you add 70us of sleep transition into
that, you get 200us at minimum.

So what it would require to actually get latencies that Paul calculated,
is a new C-state that bypasses the idle loop completely. This would
essentially keep cpu in busy loop while waiting for interrupt (or i2c
completion in this case). In pm-sense, it seems unwise.

For us, the 500us constraint seems to work quite nicely. It removes the
problems we had with i2c transfers timing out with off mode, and
restores average transfer times (from clk_enable to clk_disable) to few
hundred us (that were observed with retention).

But it's open software, people are free to experiment :)

- Kalle

> 
> A.

RE: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Pandita, Vikram]

Jokiniemi

>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kalle
>Jokiniemi
>Sent: Thursday, September 17, 2009 11:29 AM
>To: khilman@deeprootsystems.com
>Cc: jhnikula@gmail.com; linux-omap@vger.kernel.org; Kalle Jokiniemi
>Subject: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
>
>While waiting for completion of the i2c transfer, the
>MPU could hit OFF mode and cause several msecs of
>delay that made i2c transfers fail more often. The

How many bytes i2c read were you doing, that failed?

What OPP are you in when you observe these failures? 
VDD1:OPP?

>extra delays and subsequent re-trys cause i2c clocks
>to be active more often. This has also an negative
>effect on power consumption.
>
>Added a constraint that allows MPU to wake up in few
>hundred usecs, which is roughly the average i2c wait
>period.

How did you arrive at the number 500us for the wakeup latency? 

On Zoom2 with Synaptic touch screen controller on I2C-2,
at vdd1:opp1 we need to put a wakeup latency of 400us else we see data corruption.

Our case is doing a write of 1 byte followed by read of 16 bytes.

>
>The constraint function is passed as platform data from
>plat-omap/i2c.c and applied only on OMAP34XX devices.
>
>Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
>---
> arch/arm/plat-omap/i2c.c      |   49 +++++++++++++++++++++++++++++++----------
> drivers/i2c/busses/i2c-omap.c |   17 +++++++++++---
> include/linux/i2c-omap.h      |    9 +++++++
> 3 files changed, 59 insertions(+), 16 deletions(-)
> create mode 100644 include/linux/i2c-omap.h
>
>diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
>index 8b84839..d43d0ad 100644
>--- a/arch/arm/plat-omap/i2c.c
>+++ b/arch/arm/plat-omap/i2c.c
>@@ -26,8 +26,10 @@
> #include <linux/kernel.h>
> #include <linux/platform_device.h>
> #include <linux/i2c.h>
>+#include <linux/i2c-omap.h>
> #include <mach/irqs.h>
> #include <mach/mux.h>
>+#include <mach/omap-pm.h>
>
> #define OMAP_I2C_SIZE		0x3f
> #define OMAP1_I2C_BASE		0xfffb3800
>@@ -69,14 +71,14 @@ static struct resource i2c_resources[][2] = {
> 		},					\
> 	}
>
>-static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
>+static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
> static struct platform_device omap_i2c_devices[] = {
>-	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
>+	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
> #if	defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
>-	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
>+	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
> #endif
> #if	defined(CONFIG_ARCH_OMAP34XX)
>-	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
>+	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
> #endif
> };
>
>@@ -100,6 +102,25 @@ static const int omap34xx_pins[][2] = {};
>
> #define OMAP_I2C_CMDLINE_SETUP	(BIT(31))
>
>+/**
>+ * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
>+ * @dev:	i2c bus device pointer
>+ * @set:	set or clear constraints
>+ *
>+ * When waiting for completion of a i2c transfer, we need to set a wake up
>+ * latency constraint for the MPU. This is to ensure quick enough wakeup
>+ * from idle, when transfer completes.
>+ */
>+#ifdef CONFIG_ARCH_OMAP34XX
>+static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
>+{
>+	omap_pm_set_max_mpu_wakeup_lat(dev, set ? 500 : -1);
>+}
>+#else
>+static inline void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
>+{; }
>+#endif
>+
> static void __init omap_i2c_mux_pins(int bus)
> {
> 	int scl, sda;
>@@ -180,8 +201,8 @@ static int __init omap_i2c_bus_setup(char *str)
> 	get_options(str, 3, ints);
> 	if (ints[0] < 2 || ints[1] < 1 || ints[1] > ports)
> 		return 0;
>-	i2c_rate[ints[1] - 1] = ints[2];
>-	i2c_rate[ints[1] - 1] |= OMAP_I2C_CMDLINE_SETUP;
>+	i2c_pdata[ints[1] - 1].clkrate = ints[2];
>+	i2c_pdata[ints[1] - 1].clkrate |= OMAP_I2C_CMDLINE_SETUP;
>
> 	return 1;
> }
>@@ -195,9 +216,11 @@ static int __init omap_register_i2c_bus_cmdline(void)
> {
> 	int i, err = 0;
>
>-	for (i = 0; i < ARRAY_SIZE(i2c_rate); i++)
>-		if (i2c_rate[i] & OMAP_I2C_CMDLINE_SETUP) {
>-			i2c_rate[i] &= ~OMAP_I2C_CMDLINE_SETUP;
>+	for (i = 0; i < ARRAY_SIZE(i2c_pdata); i++)
>+		if (i2c_pdata[i].clkrate & OMAP_I2C_CMDLINE_SETUP) {
>+			i2c_pdata[i].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
>+			i2c_pdata[i].set_mpu_wkup_lat =
>+						omap_i2c_set_wfc_mpu_wkup_lat;
> 			err = omap_i2c_add_bus(i + 1);
> 			if (err)
> 				goto out;
>@@ -231,9 +254,11 @@ int __init omap_register_i2c_bus(int bus_id, u32 clkrate,
> 			return err;
> 	}
>
>-	if (!i2c_rate[bus_id - 1])
>-		i2c_rate[bus_id - 1] = clkrate;
>-	i2c_rate[bus_id - 1] &= ~OMAP_I2C_CMDLINE_SETUP;
>+	if (!i2c_pdata[bus_id - 1].clkrate)
>+		i2c_pdata[bus_id - 1].clkrate = clkrate;
>+
>+	i2c_pdata[bus_id - 1].set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
>+	i2c_pdata[bus_id - 1].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
>
> 	return omap_i2c_add_bus(bus_id);
> }
>diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>index 75bf3ad..da6d276 100644
>--- a/drivers/i2c/busses/i2c-omap.c
>+++ b/drivers/i2c/busses/i2c-omap.c
>@@ -37,6 +37,7 @@
> #include <linux/platform_device.h>
> #include <linux/clk.h>
> #include <linux/io.h>
>+#include <linux/i2c-omap.h>
>
> /* I2C controller revisions */
> #define OMAP_I2C_REV_2			0x20
>@@ -165,6 +166,8 @@ struct omap_i2c_dev {
> 	struct clk		*fclk;		/* Functional clock */
> 	struct completion	cmd_complete;
> 	struct resource		*ioarea;
>+	void			(*set_mpu_wkup_lat)(struct device *dev,
>+						    int set);
> 	u32			speed;		/* Speed of bus in Khz */
> 	u16			cmd_err;
> 	u8			*buf;
>@@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> 	 * REVISIT: We should abort the transfer on signals, but the bus goes
> 	 * into arbitration and we're currently unable to recover from it.
> 	 */
>+	dev->set_mpu_wkup_lat(dev->dev, 1);
> 	r = wait_for_completion_timeout(&dev->cmd_complete,
> 					OMAP_I2C_TIMEOUT);
>+	dev->set_mpu_wkup_lat(dev->dev, 0);
> 	dev->buf_len = 0;
> 	if (r < 0)
> 		return r;
>@@ -844,6 +849,7 @@ omap_i2c_probe(struct platform_device *pdev)
> 	struct omap_i2c_dev	*dev;
> 	struct i2c_adapter	*adap;
> 	struct resource		*mem, *irq, *ioarea;
>+	struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
> 	irq_handler_t isr;
> 	int r;
> 	u32 speed = 0;
>@@ -873,10 +879,13 @@ omap_i2c_probe(struct platform_device *pdev)
> 		goto err_release_region;
> 	}
>
>-	if (pdev->dev.platform_data != NULL)
>-		speed = *(u32 *)pdev->dev.platform_data;
>-	else
>-		speed = 100;	/* Defualt speed */
>+	if (pdata != NULL) {
>+		speed = pdata->clkrate;
>+		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
>+	} else {
>+		speed = 100;	/* Default speed */
>+		dev->set_mpu_wkup_lat = NULL;
>+	}
>
> 	dev->speed = speed;
> 	dev->idle = 1;
>diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
>new file mode 100644
>index 0000000..1362fba
>--- /dev/null
>+++ b/include/linux/i2c-omap.h
>@@ -0,0 +1,9 @@
>+#ifndef __I2C_OMAP_H__
>+#define __I2C_OMAP_H__
>+
>+struct omap_i2c_bus_platform_data {
>+	u32		clkrate;
>+	void		(*set_mpu_wkup_lat)(struct device *dev, int set);
>+};
>+
>+#endif
>--
>1.5.4.3
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Kalle Jokiniemi]

Hi,

On Mon, 2009-10-05 at 20:08 +0300, Pandita, Vikram wrote:
> Jokiniemi
> 
> >-----Original Message-----
> >From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kalle
> >Jokiniemi
> >Sent: Thursday, September 17, 2009 11:29 AM
> >To: khilman@deeprootsystems.com
> >Cc: jhnikula@gmail.com; linux-omap@vger.kernel.org; Kalle Jokiniemi
> >Subject: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
> >
> >While waiting for completion of the i2c transfer, the
> >MPU could hit OFF mode and cause several msecs of
> >delay that made i2c transfers fail more often. The
> 
> How many bytes i2c read were you doing, that failed?

This I don't know. I was monitoring average transfer times from
clk_enable to clk_disable in i2c-omap.c and I noticed the transfers
timed out to 1000ms several times. Hence the conclusion that something
went wrong (this is what I was referring to with "fail more often").


> 
> What OPP are you in when you observe these failures? 
> VDD1:OPP?

I had on-demand governor in use, but device was mostly idling, so OPP2
probably.


> 
> >extra delays and subsequent re-trys cause i2c clocks
> >to be active more often. This has also an negative
> >effect on power consumption.
> >
> >Added a constraint that allows MPU to wake up in few
> >hundred usecs, which is roughly the average i2c wait
> >period.
> 
> How did you arrive at the number 500us for the wakeup latency?

This was about the average time that I observed i2c-transfers to be
(from clk_enable to clk_disable), when off mode was not enabled.

> 
> On Zoom2 with Synaptic touch screen controller on I2C-2,
> at vdd1:opp1 we need to put a wakeup latency of 400us else we see data corruption.

Hmm, I guess you are using new cpu idle latency numbers on zoom2, since
on current pm-branch code 500us==400us==C3 C-state. But I'll repost with
400us, it might make a difference once those C-state latency numbers are
updated in pm-branch.

- Kalle

> 
> Our case is doing a write of 1 byte followed by read of 16 bytes.
> 
> >
> >The constraint function is passed as platform data from
> >plat-omap/i2c.c and applied only on OMAP34XX devices.
> >
> >Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
> >---
> > arch/arm/plat-omap/i2c.c      |   49 +++++++++++++++++++++++++++++++----------
> > drivers/i2c/busses/i2c-omap.c |   17 +++++++++++---
> > include/linux/i2c-omap.h      |    9 +++++++
> > 3 files changed, 59 insertions(+), 16 deletions(-)
> > create mode 100644 include/linux/i2c-omap.h
> >
> >diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> >index 8b84839..d43d0ad 100644
> >--- a/arch/arm/plat-omap/i2c.c
> >+++ b/arch/arm/plat-omap/i2c.c
> >@@ -26,8 +26,10 @@
> > #include <linux/kernel.h>
> > #include <linux/platform_device.h>
> > #include <linux/i2c.h>
> >+#include <linux/i2c-omap.h>
> > #include <mach/irqs.h>
> > #include <mach/mux.h>
> >+#include <mach/omap-pm.h>
> >
> > #define OMAP_I2C_SIZE		0x3f
> > #define OMAP1_I2C_BASE		0xfffb3800
> >@@ -69,14 +71,14 @@ static struct resource i2c_resources[][2] = {
> > 		},					\
> > 	}
> >
> >-static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> >+static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
> > static struct platform_device omap_i2c_devices[] = {
> >-	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> >+	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
> > #if	defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> >-	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> >+	I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
> > #endif
> > #if	defined(CONFIG_ARCH_OMAP34XX)
> >-	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> >+	I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
> > #endif
> > };
> >
> >@@ -100,6 +102,25 @@ static const int omap34xx_pins[][2] = {};
> >
> > #define OMAP_I2C_CMDLINE_SETUP	(BIT(31))
> >
> >+/**
> >+ * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
> >+ * @dev:	i2c bus device pointer
> >+ * @set:	set or clear constraints
> >+ *
> >+ * When waiting for completion of a i2c transfer, we need to set a wake up
> >+ * latency constraint for the MPU. This is to ensure quick enough wakeup
> >+ * from idle, when transfer completes.
> >+ */
> >+#ifdef CONFIG_ARCH_OMAP34XX
> >+static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> >+{
> >+	omap_pm_set_max_mpu_wakeup_lat(dev, set ? 500 : -1);
> >+}
> >+#else
> >+static inline void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> >+{; }
> >+#endif
> >+
> > static void __init omap_i2c_mux_pins(int bus)
> > {
> > 	int scl, sda;
> >@@ -180,8 +201,8 @@ static int __init omap_i2c_bus_setup(char *str)
> > 	get_options(str, 3, ints);
> > 	if (ints[0] < 2 || ints[1] < 1 || ints[1] > ports)
> > 		return 0;
> >-	i2c_rate[ints[1] - 1] = ints[2];
> >-	i2c_rate[ints[1] - 1] |= OMAP_I2C_CMDLINE_SETUP;
> >+	i2c_pdata[ints[1] - 1].clkrate = ints[2];
> >+	i2c_pdata[ints[1] - 1].clkrate |= OMAP_I2C_CMDLINE_SETUP;
> >
> > 	return 1;
> > }
> >@@ -195,9 +216,11 @@ static int __init omap_register_i2c_bus_cmdline(void)
> > {
> > 	int i, err = 0;
> >
> >-	for (i = 0; i < ARRAY_SIZE(i2c_rate); i++)
> >-		if (i2c_rate[i] & OMAP_I2C_CMDLINE_SETUP) {
> >-			i2c_rate[i] &= ~OMAP_I2C_CMDLINE_SETUP;
> >+	for (i = 0; i < ARRAY_SIZE(i2c_pdata); i++)
> >+		if (i2c_pdata[i].clkrate & OMAP_I2C_CMDLINE_SETUP) {
> >+			i2c_pdata[i].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
> >+			i2c_pdata[i].set_mpu_wkup_lat =
> >+						omap_i2c_set_wfc_mpu_wkup_lat;
> > 			err = omap_i2c_add_bus(i + 1);
> > 			if (err)
> > 				goto out;
> >@@ -231,9 +254,11 @@ int __init omap_register_i2c_bus(int bus_id, u32 clkrate,
> > 			return err;
> > 	}
> >
> >-	if (!i2c_rate[bus_id - 1])
> >-		i2c_rate[bus_id - 1] = clkrate;
> >-	i2c_rate[bus_id - 1] &= ~OMAP_I2C_CMDLINE_SETUP;
> >+	if (!i2c_pdata[bus_id - 1].clkrate)
> >+		i2c_pdata[bus_id - 1].clkrate = clkrate;
> >+
> >+	i2c_pdata[bus_id - 1].set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
> >+	i2c_pdata[bus_id - 1].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
> >
> > 	return omap_i2c_add_bus(bus_id);
> > }
> >diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> >index 75bf3ad..da6d276 100644
> >--- a/drivers/i2c/busses/i2c-omap.c
> >+++ b/drivers/i2c/busses/i2c-omap.c
> >@@ -37,6 +37,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/clk.h>
> > #include <linux/io.h>
> >+#include <linux/i2c-omap.h>
> >
> > /* I2C controller revisions */
> > #define OMAP_I2C_REV_2			0x20
> >@@ -165,6 +166,8 @@ struct omap_i2c_dev {
> > 	struct clk		*fclk;		/* Functional clock */
> > 	struct completion	cmd_complete;
> > 	struct resource		*ioarea;
> >+	void			(*set_mpu_wkup_lat)(struct device *dev,
> >+						    int set);
> > 	u32			speed;		/* Speed of bus in Khz */
> > 	u16			cmd_err;
> > 	u8			*buf;
> >@@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > 	 * REVISIT: We should abort the transfer on signals, but the bus goes
> > 	 * into arbitration and we're currently unable to recover from it.
> > 	 */
> >+	dev->set_mpu_wkup_lat(dev->dev, 1);
> > 	r = wait_for_completion_timeout(&dev->cmd_complete,
> > 					OMAP_I2C_TIMEOUT);
> >+	dev->set_mpu_wkup_lat(dev->dev, 0);
> > 	dev->buf_len = 0;
> > 	if (r < 0)
> > 		return r;
> >@@ -844,6 +849,7 @@ omap_i2c_probe(struct platform_device *pdev)
> > 	struct omap_i2c_dev	*dev;
> > 	struct i2c_adapter	*adap;
> > 	struct resource		*mem, *irq, *ioarea;
> >+	struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
> > 	irq_handler_t isr;
> > 	int r;
> > 	u32 speed = 0;
> >@@ -873,10 +879,13 @@ omap_i2c_probe(struct platform_device *pdev)
> > 		goto err_release_region;
> > 	}
> >
> >-	if (pdev->dev.platform_data != NULL)
> >-		speed = *(u32 *)pdev->dev.platform_data;
> >-	else
> >-		speed = 100;	/* Defualt speed */
> >+	if (pdata != NULL) {
> >+		speed = pdata->clkrate;
> >+		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> >+	} else {
> >+		speed = 100;	/* Default speed */
> >+		dev->set_mpu_wkup_lat = NULL;
> >+	}
> >
> > 	dev->speed = speed;
> > 	dev->idle = 1;
> >diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> >new file mode 100644
> >index 0000000..1362fba
> >--- /dev/null
> >+++ b/include/linux/i2c-omap.h
> >@@ -0,0 +1,9 @@
> >+#ifndef __I2C_OMAP_H__
> >+#define __I2C_OMAP_H__
> >+
> >+struct omap_i2c_bus_platform_data {
> >+	u32		clkrate;
> >+	void		(*set_mpu_wkup_lat)(struct device *dev, int set);
> >+};
> >+
> >+#endif
> >--
> >1.5.4.3
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Nishanth Menon]

Kalle Jokiniemi said the following on 10/07/2009 05:10 AM:
>>> extra delays and subsequent re-trys cause i2c clocks
>>> to be active more often. This has also an negative
>>> effect on power consumption.
>>>
>>> Added a constraint that allows MPU to wake up in few
>>> hundred usecs, which is roughly the average i2c wait
>>> period.
>>>       
>> How did you arrive at the number 500us for the wakeup latency?
>>     
>
> This was about the average time that I observed i2c-transfers to be
> (from clk_enable to clk_disable), when off mode was not enabled.
>   
just a quick 2cents: I understand that the 500uSec might need to be
board dependent - here is why:
one theory we have regarding the timeout delay having to be modified on
zoom2 was that if the delay was 500uSec, the touch screen controller
timesout and pulls of the data in the middle of transfer.
Regards,
Nishanth Menon

RE: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Sonasath, Moiz]

Hello Jokiniemi,

> -----Original Message-----
> From: Kalle Jokiniemi [mailto:kalle.jokiniemi@digia.com]
> Sent: Wednesday, October 07, 2009 10:01 AM
> To: Nishanth Menon
> Cc: Pandita, Vikram; khilman@deeprootsystems.com; Sonasath, Moiz;
> jhnikula@gmail.com; linux-omap@vger.kernel.org; jouni.hogander
> Subject: Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in
> i2c
> 
> On Wed, 2009-10-07 at 13:49 +0300, Nishanth Menon wrote:
> > Kalle Jokiniemi said the following on 10/07/2009 05:10 AM:
> > >>> extra delays and subsequent re-trys cause i2c clocks
> > >>> to be active more often. This has also an negative
> > >>> effect on power consumption.
> > >>>
> > >>> Added a constraint that allows MPU to wake up in few
> > >>> hundred usecs, which is roughly the average i2c wait
> > >>> period.
> > >>>
> > >> How did you arrive at the number 500us for the wakeup latency?
> > >>
> > >
> > > This was about the average time that I observed i2c-transfers to be
> > > (from clk_enable to clk_disable), when off mode was not enabled.
> > >
> > just a quick 2cents: I understand that the 500uSec might need to be
> > board dependent - here is why:
> > one theory we have regarding the timeout delay having to be modified on
> > zoom2 was that if the delay was 500uSec, the touch screen controller
> > timesout and pulls of the data in the middle of transfer.
> 
> You have a good point there. I think this will require changing the way
> of passing the bus-specific platform data: currently bus-id and bus-rate
> are passed as function parameters of omap_register_i2c_bus. IMO that
> would need to change to use just one omap_i2c_bus_platform_data
> structure that would contain these and the mpu wake up constraint.
> 
> The other option is to add another parameter to omap_register_i2c_bus,
> but that does not seem as extensible to me. It's much easier to add even
> another platform parameter to a struct, than adding a new function
> parameter. With struct you can always use some known good value in case
> the parameter is not defined, but in function parameter we need to
> always modify all the board files.
> 
> Either way, it will be a time consuming task to go through all those
> board files, since they currently use those function parameters. If I
> find that time, I'll do it.
> 
> BTW, those cpu idle C-state latencies and thresholds could also be
> board-specific. Reasons: we already set board specific voltage and
> oscillator setup times and we have different idle/active consumptions
> (due to differences in peripherals / pad configs etc.), which effect
> threshold values. Kevin, take note, maybe something to add to "TODO" in
> elinux wiki ;)

Sometime back, Paul Walmsley mentioned the calculation for coming up with platform dependent wake-up latency to avoid any receiver overrun problems, the link to that is here:
http://www.pwsan.com/omap/i2c-wakeup-latency.html

Based on this, we should calculate the wake-up latency in the i2c-omap.c file using the following equation:

Parameter to pass to MPU Wakeup latency = [(FIFO Depth)bytes - (FIFO THRSH)bytes]/ [I2C byte rate]

Where, I2C byte rate = [(dev->speed)/8] bytes/sec

This should solve our purpose for any platform.

Using this expression in Zoom2 scenario, with

FIFO depth = 8 Bytes
FIFO THRSH = 4 Bytes
I2C byte rate = 100K / 8 = 12500 Bytes/sec
(For I2C Bus2 running at 100KHz/sec on which we have the touch screen controller)

Parameter to pass to MPU Wakeup latency = 320 usec

Which, when applied overcomes the touch screen problem.

The reason why even 400 usec solved the issue was because the exit latency for C3 state was 410 usec and so with 400 usec in place, the system will only go to C2 state with exit latency of 18 usec.

Following link gives the full discussion that happened on some issue of spurious IRQ's and has a good explanation about this wake-up latency issue by Paul:

http://www.mail-archive.com/linux-omap@vger.kernel.org/msg11973.html




> > - Kalle
> 
> > Regards,
> > Nishanth Menon
> 

Regards
Moiz Sonasath

RE: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Woodruff, Richard]

> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Kalle Jokiniemi
> Sent: Friday, October 02, 2009 5:59 AM


> Yes, this is a good idea in theory, but the reality of wake-up latencies
> kind-a kill this one. Wake-up from even C1 (MPU INA, CORE ON) takes
> ~130us on fastest OPP. And when you add 70us of sleep transition into
> that, you get 200us at minimum.

These values feel a bit on the high side.  Have you measured since tweaking your DPLL M/N values?

Are you measuring just across WFI or adding in some part of the idle code path?

> For us, the 500us constraint seems to work quite nicely. It removes the
> problems we had with i2c transfers timing out with off mode, and
> restores average transfer times (from clk_enable to clk_disable) to few
> hundred us (that were observed with retention).

Some of the historic I2C timeout issues were from the wakeup sources not being programmed properly.

Your constraint in my understanding is more about saving power.  Today cpuidle doesn't predict interrupt events very well.  As such the huge timeout used with i2c will never gate an off mode attempt.  You will loose in context save and restore with out constraint.

I floated some idea a while back on pm list to try and do something really simple with irqs.  ... take a timestamp on last irq, when cpuilde goes to sleep if current time since last irq is too near then choose safe sleep state.  Comments I got at that time was some didn't like extra overhead on irq path.  However, I don't know I agree with that.

Regards,
Richard W.

RE: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Kalle Jokiniemi]

On Wed, 2009-10-07 at 21:52 +0300, Woodruff, Richard wrote:
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Kalle Jokiniemi
> > Sent: Friday, October 02, 2009 5:59 AM
> 
> 
> > Yes, this is a good idea in theory, but the reality of wake-up latencies
> > kind-a kill this one. Wake-up from even C1 (MPU INA, CORE ON) takes
> > ~130us on fastest OPP. And when you add 70us of sleep transition into
> > that, you get 200us at minimum.
> 
> These values feel a bit on the high side.  Have you measured since tweaking your DPLL M/N values?
> 
> Are you measuring just across WFI or adding in some part of the idle code path?

For the numbers I mentioned, the path from start of omap_sram_idle to
end of omap_sram_idle was included. And one can't really not include it,
since it is run with interrupts disabled.

> 
> > For us, the 500us constraint seems to work quite nicely. It removes the
> > problems we had with i2c transfers timing out with off mode, and
> > restores average transfer times (from clk_enable to clk_disable) to few
> > hundred us (that were observed with retention).
> 
> Some of the historic I2C timeout issues were from the wakeup sources
> not being programmed properly.

There could be such issues, I have not investigated for other reasons
causing the time-outs. 


> 
> Your constraint in my understanding is more about saving power.  

True.

> Today cpuidle doesn't predict interrupt events very well.  As such the
> huge timeout used with i2c will never gate an off mode attempt.  You
> will loose in context save and restore with out constraint.

True, that is why the constraint is needed on latency basis.

- Kalle

> 
> I floated some idea a while back on pm list to try and do something
> really simple with irqs.  ... take a timestamp on last irq, when
> cpuilde goes to sleep if current time since last irq is too near then
> choose safe sleep state.  Comments I got at that time was some didn't
> like extra overhead on irq path.  However, I don't know I agree with
> that.
> 
> Regards,
> Richard W.