[PATCH 0/3] PM: Misc latency fixes

[PATCH 0/3] PM: Misc latency fixes

[Kalle Jokiniemi]

Hello,
Here are some fruits from digging out the latency sources
of our idle loop. The main latency source was powerdomain
state counter updating at beginning and end of the idle
loop. Also PER previous state reading strangely seemed to
cause some latency with significance. Could not find any
TRM or errata comment to why this is, though.

The I2C mpu wakeup latency constraint patch has been updated
to calculate latencies at boot from clkrate and fifo size.
This was included in this set, since it benefits from the
reduced latency of the other patches.

Patches tested on linux-omap/pm and rx-51.

Kalle Jokiniemi (3):
      OMAP3: Only update pm-counters when needed
      PM: Skip PER previous state register read
      OMAP: I2C: Add mpu wake up latency constraint in i2c

 arch/arm/mach-omap2/pm-debug.c |   51 ++++++++++++++++++++++++++++++++++++-
 arch/arm/mach-omap2/pm.h       |    2 +
 arch/arm/mach-omap2/pm34xx.c   |   31 ++++++++++++++--------
 arch/arm/plat-omap/i2c.c       |   54 +++++++++++++++++++++++++++++++---------
 drivers/i2c/busses/i2c-omap.c  |   25 +++++++++++++++---
 include/linux/i2c-omap.h       |    9 ++++++
 6 files changed, 143 insertions(+), 29 deletions(-)

[PATCH 1/3] OMAP3: Only update pm-counters when needed

[Kalle Jokiniemi]

From: Kalle Jokiniemi <ext-kalle.jokiniemi@nokia.com>

The biggest source of latency in idle loop (omap_sram_idle
function) comes from updating the state counters for each
power domain. The two purposes of these counters are to
provide debug data in debugfs, and to keep track of context
losses occurring during idle transitions (off mode counters).

I created new debugfs interface "enable_count" for
enabling the "count" interface, which exposes the debug
part of these counters. The counters are not updated
anymore for CORE ON idle transitions, when the count
interface is disabled. For deeper CORE states, counters
are still updated to preserve context loss tracking.

This change decreases C1/C2 state latency over 100us at
OPP2.

Signed-off-by: Kalle Jokiniemi <ext-kalle.jokiniemi@nokia.com>
---
 arch/arm/mach-omap2/pm-debug.c |   51 ++++++++++++++++++++++++++++++++++++++-
 arch/arm/mach-omap2/pm.h       |    2 +
 arch/arm/mach-omap2/pm34xx.c   |   12 +++++----
 3 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
index 5aac64f..76c2696 100644
--- a/arch/arm/mach-omap2/pm-debug.c
+++ b/arch/arm/mach-omap2/pm-debug.c
@@ -37,6 +37,9 @@
 #include "prm-regbits-34xx.h"
 
 int omap2_pm_debug;
+static int pm_dbg_count_active;
+static struct dentry *de_pm_debug_count;
+static struct dentry *de_pm_debug;
 
 #define DUMP_PRM_MOD_REG(mod, reg)    \
 	regs[reg_count].name = #mod "." #reg; \
@@ -327,6 +330,11 @@ int pm_dbg_regset_save(int reg_set)
 	return 0;
 }
 
+int pm_dbg_count_is_active(void)
+{
+	return pm_dbg_count_active;
+}
+
 static const char pwrdm_state_names[][4] = {
 	"OFF",
 	"RET",
@@ -460,6 +468,40 @@ static const struct file_operations debug_reg_fops = {
 	.release        = single_release,
 };
 
+static int pm_dbg_count_enable_get(void *data, u64 *val)
+{
+	*val = pm_dbg_count_active;
+	return 0;
+}
+
+static int pm_dbg_count_enable_set(void *data, u64 val)
+{
+	if (val > 1) {
+		printk(KERN_ERR "Invalid value! 1 to enable, 0 to disable\n");
+		return -EINVAL;
+	}
+
+	if (val == 1 && !pm_dbg_count_active) {
+		de_pm_debug_count = debugfs_create_file("count", S_IRUGO,
+			de_pm_debug, (void *)DEBUG_FILE_COUNTERS, &debug_fops);
+
+		if (de_pm_debug_count == NULL) {
+			printk(KERN_ERR "Error: could not create debugfs "
+					"entry\n");
+			return -ENOMEM;
+		}
+		pm_dbg_count_active = 1;
+	} else if (val == 0 && pm_dbg_count_active) {
+		debugfs_remove(de_pm_debug_count);
+		de_pm_debug_count = NULL;
+		pm_dbg_count_active = 0;
+	}
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(enable_count_fops, pm_dbg_count_enable_get,
+				pm_dbg_count_enable_set, "%llu\n");
+
 int pm_dbg_regset_init(int reg_set)
 {
 	char name[2];
@@ -576,12 +618,17 @@ static int __init pm_dbg_init(void)
 		return -ENODEV;
 	}
 
+	pm_dbg_count_active = 0;
+
 	d = debugfs_create_dir("pm_debug", NULL);
 	if (IS_ERR(d))
 		return PTR_ERR(d);
+	de_pm_debug = d;
+
+	(void) debugfs_create_file("enable_count", S_IRUGO |
+				S_IWUGO, d, &pm_dbg_count_active,
+				&enable_count_fops);
 
-	(void) debugfs_create_file("count", S_IRUGO,
-		d, (void *)DEBUG_FILE_COUNTERS, &debug_fops);
 	(void) debugfs_create_file("time", S_IRUGO,
 		d, (void *)DEBUG_FILE_TIMERS, &debug_fops);
 
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index f8d11a2..3f0202b 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -63,12 +63,14 @@ extern int omap2_pm_debug;
 extern void pm_dbg_update_time(struct powerdomain *pwrdm, int prev);
 extern int pm_dbg_regset_save(int reg_set);
 extern int pm_dbg_regset_init(int reg_set);
+extern int pm_dbg_count_is_active(void);
 #else
 #define omap2_pm_dump(mode, resume, us)		do {} while (0);
 #define omap2_pm_debug				0
 #define pm_dbg_update_time(pwrdm, prev) do {} while (0);
 #define pm_dbg_regset_save(reg_set) do {} while (0);
 #define pm_dbg_regset_init(reg_set) do {} while (0);
+#define pm_dbg_count_is_active()		0
 #endif /* CONFIG_PM_DEBUG */
 
 extern void omap24xx_idle_loop_suspend(void);
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 01260ec..237c819 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -378,15 +378,17 @@ void omap_sram_idle(void)
 		return;
 	}
 
-	pwrdm_pre_transition();
+	per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
+	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
+
+	if (pm_dbg_count_is_active() || (core_next_state != PWRDM_POWER_ON))
+		pwrdm_pre_transition();
 
 	/* NEON control */
 	if (pwrdm_read_pwrst(neon_pwrdm) == PWRDM_POWER_ON)
 		pwrdm_set_next_pwrst(neon_pwrdm, mpu_next_state);
 
 	/* PER */
-	per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
-	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
 	if (per_next_state < PWRDM_POWER_ON) {
 		omap_uart_prepare_idle(2);
 		omap2_gpio_prepare_for_idle(per_next_state);
@@ -505,8 +507,8 @@ void omap_sram_idle(void)
 		omap3_disable_io_chain();
 	}
 
-
-	pwrdm_post_transition();
+	if (pm_dbg_count_is_active() || (core_next_state != PWRDM_POWER_ON))
+		pwrdm_post_transition();
 
 	omap2_clkdm_allow_idle(mpu_pwrdm->pwrdm_clkdms[0]);
 }
-- 
1.5.4.3

[PATCH 2/3] PM: Skip PER previous state register read

[Kalle Jokiniemi]

According to measurements, reading the previous state of PER
domain after wfi takes ~11us on OPP2.

Removed this unneccessary latency from cases where we know
PER power domain did not try to enter off mode.

Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
---
 arch/arm/mach-omap2/pm34xx.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 237c819..b70ea19 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -489,12 +489,19 @@ void omap_sram_idle(void)
 
 	/* PER */
 	if (per_next_state < PWRDM_POWER_ON) {
-		per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
-		if (per_prev_state == PWRDM_POWER_OFF) {
-			omap3_per_restore_context();
-			omap3_gpio_restore_pad_context(0);
-		} else if (per_next_state == PWRDM_POWER_OFF)
-			omap3_gpio_restore_pad_context(1);
+		if (per_next_state == PWRDM_POWER_OFF) {
+			/*
+			 * Reading the prev-state takes long time (11us@OPP2),
+			 * only do it, if we really tried to put PER in OFF
+			 */
+			per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
+			if (per_prev_state == PWRDM_POWER_OFF) {
+				omap3_per_restore_context();
+				omap3_gpio_restore_pad_context(0);
+			} else if (per_next_state == PWRDM_POWER_OFF) {
+				omap3_gpio_restore_pad_context(1);
+			}
+		}
 		omap2_gpio_resume_after_idle();
 		omap_uart_resume_idle(2);
 		if (per_state_modified)
-- 
1.5.4.3

[PATCH V4 3/3] 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.

Created a mechanism for passing and using the
constraint setting function in driver code. The used
mpu wake up latency constraints are now set individually
per bus, and they are calculated based on clock rate
and fifo size.

Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley,
and Nishanth Menon for tuning out the details of
this patch.

Cc: Moiz Sonasath <m-sonasath@ti.com>
Cc: Jarkko Nikula <jhnikula@gmail.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Nishanth Menon <nm@ti.com>
Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
---
 arch/arm/plat-omap/i2c.c      |   54 +++++++++++++++++++++++++++++++---------
 drivers/i2c/busses/i2c-omap.c |   25 ++++++++++++++++---
 include/linux/i2c-omap.h      |    9 +++++++
 3 files changed, 72 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..3c122cd 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,31 @@ static const int omap34xx_pins[][2] = {};
 
 #define OMAP_I2C_CMDLINE_SETUP	(BIT(31))
 
+#ifdef CONFIG_ARCH_OMAP34XX
+/*
+ * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
+ * @dev:	i2c bus device pointer
+ * @val:	latency constraint to set, -1 to disable constraint
+ *
+ * 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.
+ */
+static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int val)
+{
+	omap_pm_set_max_mpu_wakeup_lat(dev, val);
+}
+#endif
+
+static void __init omap_set_i2c_constraint_func(
+				struct omap_i2c_bus_platform_data *pd)
+{
+	if (cpu_is_omap34xx())
+		pd->set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
+	else
+		pd->set_mpu_wkup_lat = NULL;
+}
+
 static void __init omap_i2c_mux_pins(int bus)
 {
 	int scl, sda;
@@ -180,8 +207,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 +222,10 @@ 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;
+			omap_set_i2c_constraint_func(&i2c_pdata[i]);
 			err = omap_i2c_add_bus(i + 1);
 			if (err)
 				goto out;
@@ -231,9 +259,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;
+
+	omap_set_i2c_constraint_func(&i2c_pdata[bus_id - 1]);
+	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..bbea8a0 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,9 @@ struct omap_i2c_dev {
 	struct clk		*fclk;		/* Functional clock */
 	struct completion	cmd_complete;
 	struct resource		*ioarea;
+	u32			latency;	/* maximum mpu wkup latency */
+	void			(*set_mpu_wkup_lat)(struct device *dev,
+						    int latency);
 	u32			speed;		/* Speed of bus in Khz */
 	u16			cmd_err;
 	u8			*buf;
@@ -526,8 +530,12 @@ 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.
 	 */
+	if (dev->set_mpu_wkup_lat != NULL)
+		dev->set_mpu_wkup_lat(dev->dev, dev->latency);
 	r = wait_for_completion_timeout(&dev->cmd_complete,
 					OMAP_I2C_TIMEOUT);
+	if (dev->set_mpu_wkup_lat != NULL)
+		dev->set_mpu_wkup_lat(dev->dev, -1);
 	dev->buf_len = 0;
 	if (r < 0)
 		return r;
@@ -844,6 +852,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 +882,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;
@@ -911,6 +923,11 @@ omap_i2c_probe(struct platform_device *pdev)
 		 */
 		dev->fifo_size = (dev->fifo_size / 2);
 		dev->b_hw = 1; /* Enable hardware fixes */
+
+		/* calculate wakeup latency constraint for MPU */
+		if (dev->set_mpu_wkup_lat != NULL)
+			dev->latency = (1000000 * dev->fifo_size) /
+				       (1000 * speed / 8);
 	}
 
 	/* reset ASAP, clearing any IRQs */
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 V4 3/3] 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 21, 2009 6:51 AM
> To: khilman@deeprootsystems.com
> Cc: linux-omap@vger.kernel.org; Kalle Jokiniemi; Sonasath, Moiz; Jarkko
> Nikula; Paul Walmsley; Menon, Nishanth
> Subject: [PATCH V4 3/3] 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
> extra delays and subsequent re-trys cause i2c clocks
> to be active more often. This has also an negative
> effect on power consumption.
> 
> Created a mechanism for passing and using the
> constraint setting function in driver code. The used
> mpu wake up latency constraints are now set individually
> per bus, and they are calculated based on clock rate
> and fifo size.
> 
> Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley,
> and Nishanth Menon for tuning out the details of
> this patch.
> 
> Cc: Moiz Sonasath <m-sonasath@ti.com>
> Cc: Jarkko Nikula <jhnikula@gmail.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Nishanth Menon <nm@ti.com>
> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
> ---
> 


>  	dev->speed = speed;
>  	dev->idle = 1;
> @@ -911,6 +923,11 @@ omap_i2c_probe(struct platform_device *pdev)
>  		 */
>  		dev->fifo_size = (dev->fifo_size / 2);
>  		dev->b_hw = 1; /* Enable hardware fixes */
> +
> +		/* calculate wakeup latency constraint for MPU */
> +		if (dev->set_mpu_wkup_lat != NULL)
> +			dev->latency = (1000000 * dev->fifo_size) /
> +				       (1000 * speed / 8);
>  	}

IMHO, here instead of using 'dev->fifo_size' for calculating the 
'dev->latency', we need to use:

1. For RX case, to avoid Reciver overrun:
	if (msg->flags & I2C_M_RD)
		Use [(FIFO Depth)bytes - (FIFO THRSH)bytes] in calculating 
		dev->latency

Because conceptually, RDR/RRDY interrupts are generated when RTRSH is reached, so we want the MPU to be wake up within the time it takes to fill the FIFO from RTRSH to FIFO Depth (FIFO full).  

2. For TX case, to avoid Transmitter Underflow:
	if (!(msg->flags & I2C_M_RD))
		Use (FIFO THRSH)bytes in calculating dev->latency

Because conceptually, XDR/XRDY interrupts are generated when XTRSH is reached, so we want the MPU to be wake up within the time it takes to drain the FIFO from XTRSH to Zero (FIFO empty).  

Using, dev->fifo_size instead, works in the present code because we have a RTRSH/XTRSH = dev->fifo_size/2 = 4 bytes
And therefore,
(FIFO Depth)bytes - (FIFO THRSH)bytes
= 8 - 4 = 4 bytes

But, to make it more generic in future and to make it independent of any changes in the RTRSH/XTRSH values or FIFO depths in future, we should use a generic code here.  

> 
>  	/* reset ASAP, clearing any IRQs */
> 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

Regards
Moiz Sonasath

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

[Kalle Jokiniemi]

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

OK, let's try this once more, since my mail did not seem to go to
linux-omap.

Sorry for the spam.

See my comments below:

On Fri, 2009-10-23 at 18:53 +0300, Sonasath, Moiz wrote:
> Hello Jokiniemi!
> 
> > -----Original Message-----
> > From: Kalle Jokiniemi [mailto:kalle.jokiniemi@digia.com]
> > Sent: Wednesday, October 21, 2009 6:51 AM
> > To: khilman@deeprootsystems.com
> > Cc: linux-omap@vger.kernel.org; Kalle Jokiniemi; Sonasath, Moiz;
> Jarkko
> > Nikula; Paul Walmsley; Menon, Nishanth
> > Subject: [PATCH V4 3/3] 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
> > extra delays and subsequent re-trys cause i2c clocks
> > to be active more often. This has also an negative
> > effect on power consumption.
> > 
> > Created a mechanism for passing and using the
> > constraint setting function in driver code. The used
> > mpu wake up latency constraints are now set individually
> > per bus, and they are calculated based on clock rate
> > and fifo size.
> > 
> > Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley,
> > and Nishanth Menon for tuning out the details of
> > this patch.
> > 
> > Cc: Moiz Sonasath <m-sonasath@ti.com>
> > Cc: Jarkko Nikula <jhnikula@gmail.com>
> > Cc: Paul Walmsley <paul@pwsan.com>
> > Cc: Nishanth Menon <nm@ti.com>
> > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
> > ---
> > 
> 
> 
> >  	dev->speed = speed;
> >  	dev->idle = 1;
> > @@ -911,6 +923,11 @@ omap_i2c_probe(struct platform_device *pdev)
> >  		 */
> >  		dev->fifo_size = (dev->fifo_size / 2);
> >  		dev->b_hw = 1; /* Enable hardware fixes */
> > +
> > +		/* calculate wakeup latency constraint for MPU */
> > +		if (dev->set_mpu_wkup_lat != NULL)
> > +			dev->latency = (1000000 * dev->fifo_size) /
> > +				       (1000 * speed / 8);
> >  	}
> 
> IMHO, here instead of using 'dev->fifo_size' for calculating the 
> 'dev->latency', we need to use:
> 
> 1. For RX case, to avoid Reciver overrun:
> 	if (msg->flags & I2C_M_RD)
> 		Use [(FIFO Depth)bytes - (FIFO THRSH)bytes] in calculating 
> 		dev->latency
> 
> Because conceptually, RDR/RRDY interrupts are generated when RTRSH is
> reached, so we want the MPU to be wake up within the time it takes to
> fill the FIFO from RTRSH to FIFO Depth (FIFO full).  
> 
> 2. For TX case, to avoid Transmitter Underflow:
> 	if (!(msg->flags & I2C_M_RD))
> 		Use (FIFO THRSH)bytes in calculating dev->latency
> 
> Because conceptually, XDR/XRDY interrupts are generated when XTRSH is
> reached, so we want the MPU to be wake up within the time it takes to
> drain the FIFO from XTRSH to Zero (FIFO empty).  
> 
> Using, dev->fifo_size instead, works in the present code because we
> have a RTRSH/XTRSH = dev->fifo_size/2 = 4 bytes
> And therefore,
> (FIFO Depth)bytes - (FIFO THRSH)bytes
> = 8 - 4 = 4 bytes
> 
> But, to make it more generic in future and to make it independent of
> any changes in the RTRSH/XTRSH values or FIFO depths in future, we
> should use a generic code here.

Well, I don't completely agree with the necessity of preparing for
different rx/tx thresholds. For this to make sense, the i2c-omap driver
should first separate in it's code the use of rx and tx thresholds. If
someone is planning to do that, he/she should anyway update the usage of
fifo_size throughout the code, including the wake up latency setting.

Anyways, attached a patch that separates the mpu wake up latencies for
rx and tx. In case that is needed. Though I'm not for it, since it adds
unneeded complexity.

- Kalle

> 
> > 
> >  	/* reset ASAP, clearing any IRQs */
> > 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
> 
> Regards
> Moiz Sonasath
> 

[-- Attachment #2: 0001-OMAP-I2C-Add-mpu-wake-up-latency-constraint-in-i2c.patch --]
[-- Type: application/mbox, Size: 8549 bytes --]

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

[kalle.jokiniemi]

Kevin,

Could you please take this patch in? Currently i2c is not working very well with cpuidle and off mode, and the patch is needed.

I would go with V4 of the patch.

- Kalle

>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org 
>[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of ext 
>Kalle Jokiniemi
>Sent: 29. lokakuuta 2009 10:55
>To: Sonasath, Moiz
>Cc: khilman@deeprootsystems.com; linux-omap@vger.kernel.org
>Subject: RE: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency 
>constraint in i2c
>
>OK, let's try this once more, since my mail did not seem to go 
>to linux-omap.
>
>Sorry for the spam.
>
>See my comments below:
>
>On Fri, 2009-10-23 at 18:53 +0300, Sonasath, Moiz wrote:
>> Hello Jokiniemi!
>> 
>> > -----Original Message-----
>> > From: Kalle Jokiniemi [mailto:kalle.jokiniemi@digia.com]
>> > Sent: Wednesday, October 21, 2009 6:51 AM
>> > To: khilman@deeprootsystems.com
>> > Cc: linux-omap@vger.kernel.org; Kalle Jokiniemi; Sonasath, Moiz;
>> Jarkko
>> > Nikula; Paul Walmsley; Menon, Nishanth
>> > Subject: [PATCH V4 3/3] 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 extra delays and subsequent re-trys cause i2c 
>> > clocks to be active more often. This has also an negative 
>effect on 
>> > power consumption.
>> > 
>> > Created a mechanism for passing and using the constraint setting 
>> > function in driver code. The used mpu wake up latency constraints 
>> > are now set individually per bus, and they are calculated based on 
>> > clock rate and fifo size.
>> > 
>> > Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley, and 
>Nishanth 
>> > Menon for tuning out the details of this patch.
>> > 
>> > Cc: Moiz Sonasath <m-sonasath@ti.com>
>> > Cc: Jarkko Nikula <jhnikula@gmail.com>
>> > Cc: Paul Walmsley <paul@pwsan.com>
>> > Cc: Nishanth Menon <nm@ti.com>
>> > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
>> > ---
>> > 
>> 
>> 
>> >  	dev->speed = speed;
>> >  	dev->idle = 1;
>> > @@ -911,6 +923,11 @@ omap_i2c_probe(struct platform_device *pdev)
>> >  		 */
>> >  		dev->fifo_size = (dev->fifo_size / 2);
>> >  		dev->b_hw = 1; /* Enable hardware fixes */
>> > +
>> > +		/* calculate wakeup latency constraint for MPU */
>> > +		if (dev->set_mpu_wkup_lat != NULL)
>> > +			dev->latency = (1000000 * dev->fifo_size) /
>> > +				       (1000 * speed / 8);
>> >  	}
>> 
>> IMHO, here instead of using 'dev->fifo_size' for calculating the 
>> 'dev->latency', we need to use:
>> 
>> 1. For RX case, to avoid Reciver overrun:
>> 	if (msg->flags & I2C_M_RD)
>> 		Use [(FIFO Depth)bytes - (FIFO THRSH)bytes] in 
>calculating 
>> 		dev->latency
>> 
>> Because conceptually, RDR/RRDY interrupts are generated when 
>RTRSH is 
>> reached, so we want the MPU to be wake up within the time it 
>takes to 
>> fill the FIFO from RTRSH to FIFO Depth (FIFO full).
>> 
>> 2. For TX case, to avoid Transmitter Underflow:
>> 	if (!(msg->flags & I2C_M_RD))
>> 		Use (FIFO THRSH)bytes in calculating dev->latency
>> 
>> Because conceptually, XDR/XRDY interrupts are generated when 
>XTRSH is 
>> reached, so we want the MPU to be wake up within the time it 
>takes to 
>> drain the FIFO from XTRSH to Zero (FIFO empty).
>> 
>> Using, dev->fifo_size instead, works in the present code because we 
>> have a RTRSH/XTRSH = dev->fifo_size/2 = 4 bytes And therefore, (FIFO 
>> Depth)bytes - (FIFO THRSH)bytes = 8 - 4 = 4 bytes
>> 
>> But, to make it more generic in future and to make it independent of 
>> any changes in the RTRSH/XTRSH values or FIFO depths in future, we 
>> should use a generic code here.
>
>Well, I don't completely agree with the necessity of preparing 
>for different rx/tx thresholds. For this to make sense, the 
>i2c-omap driver should first separate in it's code the use of 
>rx and tx thresholds. If someone is planning to do that, 
>he/she should anyway update the usage of fifo_size throughout 
>the code, including the wake up latency setting.
>
>Anyways, attached a patch that separates the mpu wake up 
>latencies for rx and tx. In case that is needed. Though I'm 
>not for it, since it adds unneeded complexity.
>
>- Kalle
>
>> 
>> > 
>> >  	/* reset ASAP, clearing any IRQs */ 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
>> 
>> Regards
>> Moiz Sonasath
>> 
>

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

[Kevin Hilman]

<kalle.jokiniemi@nokia.com> writes:

> Could you please take this patch in? Currently i2c is not working
> very well with cpuidle and off mode, and the patch is needed.
>
> I would go with V4 of the patch.

Kalle,

Last I saw with this thread, it didn't look like there was resolution.
If things are resolved, this shouldn't go upstream via PM branch, since
it doesn't seem to have any dependencies on the PM branch.

It should go upstream either via the I2C list, or via Tony.

If it goes via i2c list, I can add it to PM branch while waiting for
it to merge.

Kevin


>>-----Original Message-----
>>From: linux-omap-owner@vger.kernel.org 
>>[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of ext 
>>Kalle Jokiniemi
>>Sent: 29. lokakuuta 2009 10:55
>>To: Sonasath, Moiz
>>Cc: khilman@deeprootsystems.com; linux-omap@vger.kernel.org
>>Subject: RE: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency 
>>constraint in i2c
>>
>>OK, let's try this once more, since my mail did not seem to go 
>>to linux-omap.
>>
>>Sorry for the spam.
>>
>>See my comments below:
>>
>>On Fri, 2009-10-23 at 18:53 +0300, Sonasath, Moiz wrote:
>>> Hello Jokiniemi!
>>> 
>>> > -----Original Message-----
>>> > From: Kalle Jokiniemi [mailto:kalle.jokiniemi@digia.com]
>>> > Sent: Wednesday, October 21, 2009 6:51 AM
>>> > To: khilman@deeprootsystems.com
>>> > Cc: linux-omap@vger.kernel.org; Kalle Jokiniemi; Sonasath, Moiz;
>>> Jarkko
>>> > Nikula; Paul Walmsley; Menon, Nishanth
>>> > Subject: [PATCH V4 3/3] 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 extra delays and subsequent re-trys cause i2c 
>>> > clocks to be active more often. This has also an negative 
>>effect on 
>>> > power consumption.
>>> > 
>>> > Created a mechanism for passing and using the constraint setting 
>>> > function in driver code. The used mpu wake up latency constraints 
>>> > are now set individually per bus, and they are calculated based on 
>>> > clock rate and fifo size.
>>> > 
>>> > Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley, and 
>>Nishanth 
>>> > Menon for tuning out the details of this patch.
>>> > 
>>> > Cc: Moiz Sonasath <m-sonasath@ti.com>
>>> > Cc: Jarkko Nikula <jhnikula@gmail.com>
>>> > Cc: Paul Walmsley <paul@pwsan.com>
>>> > Cc: Nishanth Menon <nm@ti.com>
>>> > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
>>> > ---
>>> > 
>>> 
>>> 
>>> >  	dev->speed = speed;
>>> >  	dev->idle = 1;
>>> > @@ -911,6 +923,11 @@ omap_i2c_probe(struct platform_device *pdev)
>>> >  		 */
>>> >  		dev->fifo_size = (dev->fifo_size / 2);
>>> >  		dev->b_hw = 1; /* Enable hardware fixes */
>>> > +
>>> > +		/* calculate wakeup latency constraint for MPU */
>>> > +		if (dev->set_mpu_wkup_lat != NULL)
>>> > +			dev->latency = (1000000 * dev->fifo_size) /
>>> > +				       (1000 * speed / 8);
>>> >  	}
>>> 
>>> IMHO, here instead of using 'dev->fifo_size' for calculating the 
>>> 'dev->latency', we need to use:
>>> 
>>> 1. For RX case, to avoid Reciver overrun:
>>> 	if (msg->flags & I2C_M_RD)
>>> 		Use [(FIFO Depth)bytes - (FIFO THRSH)bytes] in 
>>calculating 
>>> 		dev->latency
>>> 
>>> Because conceptually, RDR/RRDY interrupts are generated when 
>>RTRSH is 
>>> reached, so we want the MPU to be wake up within the time it 
>>takes to 
>>> fill the FIFO from RTRSH to FIFO Depth (FIFO full).
>>> 
>>> 2. For TX case, to avoid Transmitter Underflow:
>>> 	if (!(msg->flags & I2C_M_RD))
>>> 		Use (FIFO THRSH)bytes in calculating dev->latency
>>> 
>>> Because conceptually, XDR/XRDY interrupts are generated when 
>>XTRSH is 
>>> reached, so we want the MPU to be wake up within the time it 
>>takes to 
>>> drain the FIFO from XTRSH to Zero (FIFO empty).
>>> 
>>> Using, dev->fifo_size instead, works in the present code because we 
>>> have a RTRSH/XTRSH = dev->fifo_size/2 = 4 bytes And therefore, (FIFO 
>>> Depth)bytes - (FIFO THRSH)bytes = 8 - 4 = 4 bytes
>>> 
>>> But, to make it more generic in future and to make it independent of 
>>> any changes in the RTRSH/XTRSH values or FIFO depths in future, we 
>>> should use a generic code here.
>>
>>Well, I don't completely agree with the necessity of preparing 
>>for different rx/tx thresholds. For this to make sense, the 
>>i2c-omap driver should first separate in it's code the use of 
>>rx and tx thresholds. If someone is planning to do that, 
>>he/she should anyway update the usage of fifo_size throughout 
>>the code, including the wake up latency setting.
>>
>>Anyways, attached a patch that separates the mpu wake up 
>>latencies for rx and tx. In case that is needed. Though I'm 
>>not for it, since it adds unneeded complexity.
>>
>>- Kalle
>>
>>> 
>>> > 
>>> >  	/* reset ASAP, clearing any IRQs */ 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
>>> 
>>> Regards
>>> Moiz Sonasath
>>> 
>>

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

[kalle.jokiniemi]

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

> -----Original Message-----
> From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: 20. marraskuuta 2009 18:28
> To: Jokiniemi Kalle (Nokia-D/Tampere); Tony Lindgren
> Cc: linux-omap@vger.kernel.org; m-sonasath@ti.com; Hogander 
> Jouni (Nokia-D/Tampere)
> Subject: Re: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up 
> latency constraint in i2c
> 
> <kalle.jokiniemi@nokia.com> writes:
> 
> > Could you please take this patch in? Currently i2c is not 
> working very 
> > well with cpuidle and off mode, and the patch is needed.
> >
> > I would go with V4 of the patch.
> 
> Kalle,
> 
> Last I saw with this thread, it didn't look like there was resolution.

OK. Let's resolve it then :)

Moiz, could you please comment on my last reply in the old thread:

--------- (sent on 29th of October) ----------------

> >>Well, I don't completely agree with the necessity of preparing for 
> >>different rx/tx thresholds. For this to make sense, the i2c-omap 
> >>driver should first separate in it's code the use of rx and tx 
> >>thresholds. If someone is planning to do that, he/she should anyway 
> >>update the usage of fifo_size throughout the code, 
> including the wake 
> >>up latency setting.
> >>
> >>Anyways, attached a patch that separates the mpu wake up 
> latencies for 
> >>rx and tx. In case that is needed. Though I'm not for it, since it 
> >>adds unneeded complexity.
> >>
--------- (sent on 29th of October) ----------------

Attached also the alternative patch I was referring to.


> If things are resolved, this shouldn't go upstream via PM 
> branch, since it doesn't seem to have any dependencies on the 
> PM branch.

Ah, the pm interfaces are in mainline now.


> 
> It should go upstream either via the I2C list, or via Tony.
> 
> If it goes via i2c list, I can add it to PM branch while 
> waiting for it to merge.

Tony, would you take this? If not, what is the address for i2c mailing list, and who maintains i2c?

- Kalle

> 
> Kevin
> 
> 
> >>-----Original Message-----
> >>From: linux-omap-owner@vger.kernel.org 
> >>[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of ext Kalle 
> >>Jokiniemi
> >>Sent: 29. lokakuuta 2009 10:55
> >>To: Sonasath, Moiz
> >>Cc: khilman@deeprootsystems.com; linux-omap@vger.kernel.org
> >>Subject: RE: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency 
> >>constraint in i2c
> >>
> >>OK, let's try this once more, since my mail did not seem to go to 
> >>linux-omap.
> >>
> >>Sorry for the spam.
> >>
> >>See my comments below:
> >>
> >>On Fri, 2009-10-23 at 18:53 +0300, Sonasath, Moiz wrote:
> >>> Hello Jokiniemi!
> >>> 
> >>> > -----Original Message-----
> >>> > From: Kalle Jokiniemi [mailto:kalle.jokiniemi@digia.com]
> >>> > Sent: Wednesday, October 21, 2009 6:51 AM
> >>> > To: khilman@deeprootsystems.com
> >>> > Cc: linux-omap@vger.kernel.org; Kalle Jokiniemi; Sonasath, Moiz;
> >>> Jarkko
> >>> > Nikula; Paul Walmsley; Menon, Nishanth
> >>> > Subject: [PATCH V4 3/3] 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 extra delays and subsequent 
> re-trys cause i2c 
> >>> > clocks to be active more often. This has also an negative
> >>effect on
> >>> > power consumption.
> >>> > 
> >>> > Created a mechanism for passing and using the 
> constraint setting 
> >>> > function in driver code. The used mpu wake up latency 
> constraints 
> >>> > are now set individually per bus, and they are 
> calculated based on 
> >>> > clock rate and fifo size.
> >>> > 
> >>> > Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley, and
> >>Nishanth
> >>> > Menon for tuning out the details of this patch.
> >>> > 
> >>> > Cc: Moiz Sonasath <m-sonasath@ti.com>
> >>> > Cc: Jarkko Nikula <jhnikula@gmail.com>
> >>> > Cc: Paul Walmsley <paul@pwsan.com>
> >>> > Cc: Nishanth Menon <nm@ti.com>
> >>> > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
> >>> > ---
> >>> > 
> >>> 
> >>> 
> >>> >  	dev->speed = speed;
> >>> >  	dev->idle = 1;
> >>> > @@ -911,6 +923,11 @@ omap_i2c_probe(struct 
> platform_device *pdev)
> >>> >  		 */
> >>> >  		dev->fifo_size = (dev->fifo_size / 2);
> >>> >  		dev->b_hw = 1; /* Enable hardware fixes */
> >>> > +
> >>> > +		/* calculate wakeup latency constraint 
> for MPU */
> >>> > +		if (dev->set_mpu_wkup_lat != NULL)
> >>> > +			dev->latency = (1000000 * 
> dev->fifo_size) /
> >>> > +				       (1000 * speed / 8);
> >>> >  	}
> >>> 
> >>> IMHO, here instead of using 'dev->fifo_size' for calculating the 
> >>> 'dev->latency', we need to use:
> >>> 
> >>> 1. For RX case, to avoid Reciver overrun:
> >>> 	if (msg->flags & I2C_M_RD)
> >>> 		Use [(FIFO Depth)bytes - (FIFO THRSH)bytes] in
> >>calculating
> >>> 		dev->latency
> >>> 
> >>> Because conceptually, RDR/RRDY interrupts are generated when
> >>RTRSH is
> >>> reached, so we want the MPU to be wake up within the time it
> >>takes to
> >>> fill the FIFO from RTRSH to FIFO Depth (FIFO full).
> >>> 
> >>> 2. For TX case, to avoid Transmitter Underflow:
> >>> 	if (!(msg->flags & I2C_M_RD))
> >>> 		Use (FIFO THRSH)bytes in calculating dev->latency
> >>> 
> >>> Because conceptually, XDR/XRDY interrupts are generated when
> >>XTRSH is
> >>> reached, so we want the MPU to be wake up within the time it
> >>takes to
> >>> drain the FIFO from XTRSH to Zero (FIFO empty).
> >>> 
> >>> Using, dev->fifo_size instead, works in the present code 
> because we 
> >>> have a RTRSH/XTRSH = dev->fifo_size/2 = 4 bytes And 
> therefore, (FIFO 
> >>> Depth)bytes - (FIFO THRSH)bytes = 8 - 4 = 4 bytes
> >>> 
> >>> But, to make it more generic in future and to make it 
> independent of 
> >>> any changes in the RTRSH/XTRSH values or FIFO depths in 
> future, we 
> >>> should use a generic code here.
> >>
> >>Well, I don't completely agree with the necessity of preparing for 
> >>different rx/tx thresholds. For this to make sense, the i2c-omap 
> >>driver should first separate in it's code the use of rx and tx 
> >>thresholds. If someone is planning to do that, he/she should anyway 
> >>update the usage of fifo_size throughout the code, 
> including the wake 
> >>up latency setting.
> >>
> >>Anyways, attached a patch that separates the mpu wake up 
> latencies for 
> >>rx and tx. In case that is needed. Though I'm not for it, since it 
> >>adds unneeded complexity.
> >>
> >>- Kalle
> >>
> >>> 
> >>> > 
> >>> >  	/* reset ASAP, clearing any IRQs */ 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
> >>> 
> >>> Regards
> >>> Moiz Sonasath
> >>> 
> >>
> 

[-- Attachment #2: 0001-OMAP-I2C-Add-mpu-wake-up-latency-constraint-in-i2c.patch --]
[-- Type: application/octet-stream, Size: 8549 bytes --]

From e207b7cddf9e874adb150f506557af200634a650 Mon Sep 17 00:00:00 2001
From: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
Date: Tue, 27 Oct 2009 13:59:19 +0200
Subject: [PATCH] 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
extra delays and subsequent re-trys cause i2c clocks
to be active more often. This has also an negative
effect on power consumption.

Created a mechanism for passing and using the
constraint setting function in driver code. The used
mpu wake up latency constraints are now set individually
per bus, and they are calculated based on clock rate
and fifo size for rx and tx transfers.

Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley,
and Nishanth Menon for tuning out the details of
this patch.

Cc: Moiz Sonasath <m-sonasath@ti.com>
Cc: Jarkko Nikula <jhnikula@gmail.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Nishanth Menon <nm@ti.com>
Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
---
 arch/arm/plat-omap/i2c.c      |   54 +++++++++++++++++++++++++++++++---------
 drivers/i2c/busses/i2c-omap.c |   38 ++++++++++++++++++++++++----
 include/linux/i2c-omap.h      |    9 +++++++
 3 files changed, 83 insertions(+), 18 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..3c122cd 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,31 @@ static const int omap34xx_pins[][2] = {};
 
 #define OMAP_I2C_CMDLINE_SETUP	(BIT(31))
 
+#ifdef CONFIG_ARCH_OMAP34XX
+/*
+ * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
+ * @dev:	i2c bus device pointer
+ * @val:	latency constraint to set, -1 to disable constraint
+ *
+ * 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.
+ */
+static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int val)
+{
+	omap_pm_set_max_mpu_wakeup_lat(dev, val);
+}
+#endif
+
+static void __init omap_set_i2c_constraint_func(
+				struct omap_i2c_bus_platform_data *pd)
+{
+	if (cpu_is_omap34xx())
+		pd->set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
+	else
+		pd->set_mpu_wkup_lat = NULL;
+}
+
 static void __init omap_i2c_mux_pins(int bus)
 {
 	int scl, sda;
@@ -180,8 +207,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 +222,10 @@ 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;
+			omap_set_i2c_constraint_func(&i2c_pdata[i]);
 			err = omap_i2c_add_bus(i + 1);
 			if (err)
 				goto out;
@@ -231,9 +259,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;
+
+	omap_set_i2c_constraint_func(&i2c_pdata[bus_id - 1]);
+	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..e17c079 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,10 @@ struct omap_i2c_dev {
 	struct clk		*fclk;		/* Functional clock */
 	struct completion	cmd_complete;
 	struct resource		*ioarea;
+	u32			tx_latency;	/* tx max mpu wkup latency */
+	u32			rx_latency;	/* rx max mpu wkup latency */
+	void			(*set_mpu_wkup_lat)(struct device *dev,
+						    int latency);
 	u32			speed;		/* Speed of bus in Khz */
 	u16			cmd_err;
 	u8			*buf;
@@ -526,8 +531,16 @@ 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.
 	 */
+	if (dev->set_mpu_wkup_lat != NULL) {
+		if (msg->flags & I2C_M_RD)
+			dev->set_mpu_wkup_lat(dev->dev, dev->rx_latency);
+		else
+			dev->set_mpu_wkup_lat(dev->dev, dev->tx_latency);
+	}
 	r = wait_for_completion_timeout(&dev->cmd_complete,
 					OMAP_I2C_TIMEOUT);
+	if (dev->set_mpu_wkup_lat != NULL)
+		dev->set_mpu_wkup_lat(dev->dev, -1);
 	dev->buf_len = 0;
 	if (r < 0)
 		return r;
@@ -844,9 +857,11 @@ 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;
+	u16 fifo_full_size;
 
 	/* NOTE: driver uses the static register mapping */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -873,10 +888,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;
@@ -902,17 +920,25 @@ omap_i2c_probe(struct platform_device *pdev)
 
 		/* Set up the fifo size - Get total size */
 		s = (omap_i2c_read_reg(dev, OMAP_I2C_BUFSTAT_REG) >> 14) & 0x3;
-		dev->fifo_size = 0x8 << s;
+		fifo_full_size = 0x8 << s;
 
 		/*
 		 * Set up notification threshold as half the total available
 		 * size. This is to ensure that we can handle the status on int
 		 * call back latencies.
 		 */
-		dev->fifo_size = (dev->fifo_size / 2);
+		dev->fifo_size = (fifo_full_size / 2);
 		dev->b_hw = 1; /* Enable hardware fixes */
 	}
 
+	/* calculate rx and tx wakeup latency constraints for MPU */
+	if (dev->set_mpu_wkup_lat != NULL) {
+		dev->tx_latency = (dev->fifo_size * 1000000) /
+				  (1000 * dev->speed / 8);
+		dev->rx_latency = ((fifo_full_size - dev->fifo_size) *
+				  1000000) / (1000 * dev->speed / 8);
+	}
+
 	/* reset ASAP, clearing any IRQs */
 	omap_i2c_init(dev);
 
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 V4 3/3] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Sonasath, Moiz]

> -----Original Message-----
> From: Kalle Jokiniemi [mailto:kalle.jokiniemi@digia.com]
> Sent: Tuesday, October 27, 2009 7:02 AM
> To: Sonasath, Moiz
> Cc: khilman@deeprootsystems.com; linux-omap@vger.kernel.org; Jarkko
> Nikula; Paul Walmsley; Menon, Nishanth; Pandita, Vikram; jouni.hogander
> Subject: RE: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency constraint
> in i2c
> 
> On Fri, 2009-10-23 at 18:53 +0300, Sonasath, Moiz wrote:
> > Hello Jokiniemi!
> >
> > > -----Original Message-----
> > > From: Kalle Jokiniemi [mailto:kalle.jokiniemi@digia.com]
> > > Sent: Wednesday, October 21, 2009 6:51 AM
> > > To: khilman@deeprootsystems.com
> > > Cc: linux-omap@vger.kernel.org; Kalle Jokiniemi; Sonasath, Moiz;
> > Jarkko
> > > Nikula; Paul Walmsley; Menon, Nishanth
> > > Subject: [PATCH V4 3/3] 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
> > > extra delays and subsequent re-trys cause i2c clocks
> > > to be active more often. This has also an negative
> > > effect on power consumption.
> > >
> > > Created a mechanism for passing and using the
> > > constraint setting function in driver code. The used
> > > mpu wake up latency constraints are now set individually
> > > per bus, and they are calculated based on clock rate
> > > and fifo size.
> > >
> > > Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley,
> > > and Nishanth Menon for tuning out the details of
> > > this patch.
> > >
> > > Cc: Moiz Sonasath <m-sonasath@ti.com>
> > > Cc: Jarkko Nikula <jhnikula@gmail.com>
> > > Cc: Paul Walmsley <paul@pwsan.com>
> > > Cc: Nishanth Menon <nm@ti.com>
> > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
> > > ---
> > >
> >
> >
> > >  	dev->speed = speed;
> > >  	dev->idle = 1;
> > > @@ -911,6 +923,11 @@ omap_i2c_probe(struct platform_device *pdev)
> > >  		 */
> > >  		dev->fifo_size = (dev->fifo_size / 2);
> > >  		dev->b_hw = 1; /* Enable hardware fixes */
> > > +
> > > +		/* calculate wakeup latency constraint for MPU */
> > > +		if (dev->set_mpu_wkup_lat != NULL)
> > > +			dev->latency = (1000000 * dev->fifo_size) /
> > > +				       (1000 * speed / 8);
> > >  	}
> >
> > IMHO, here instead of using 'dev->fifo_size' for calculating the
> > 'dev->latency', we need to use:
> >
> > 1. For RX case, to avoid Reciver overrun:
> > 	if (msg->flags & I2C_M_RD)
> > 		Use [(FIFO Depth)bytes - (FIFO THRSH)bytes] in calculating
> > 		dev->latency
> >
> > Because conceptually, RDR/RRDY interrupts are generated when RTRSH is
> > reached, so we want the MPU to be wake up within the time it takes to
> > fill the FIFO from RTRSH to FIFO Depth (FIFO full).
> >
> > 2. For TX case, to avoid Transmitter Underflow:
> > 	if (!(msg->flags & I2C_M_RD))
> > 		Use (FIFO THRSH)bytes in calculating dev->latency
> >
> > Because conceptually, XDR/XRDY interrupts are generated when XTRSH is
> > reached, so we want the MPU to be wake up within the time it takes to
> > drain the FIFO from XTRSH to Zero (FIFO empty).
> >
> > Using, dev->fifo_size instead, works in the present code because we
> > have a RTRSH/XTRSH = dev->fifo_size/2 = 4 bytes
> > And therefore,
> > (FIFO Depth)bytes - (FIFO THRSH)bytes
> > = 8 - 4 = 4 bytes
> >
> > But, to make it more generic in future and to make it independent of
> > any changes in the RTRSH/XTRSH values or FIFO depths in future, we
> > should use a generic code here.
> 
> Well, I don't completely agree with the necessity of preparing for
> different rx/tx thresholds. For this to make sense, the i2c-omap driver
> should first separate in it's code the use of rx and tx thresholds. If
> someone is planning to do that, he/she should anyway update the usage of
> fifo_size throughout the code, including the wake up latency setting.
> 
> Anyways, attached a patch that separates the mpu wake up latencies for
> rx and tx. In case that is needed. Though I'm not for it, since it adds
> unneeded complexity.
> 

Yes Kalle, you are right! Not having different RX/TX wake-up latencies will absolutely work fine with the in-place code as we have both the RX/TX thresholds same. But, I think in future we might have to play around with different RX/TX thresholds and so from a conceptually right and generic point of view it makes sense to incur the cost of the added complexity.

The patch V4 looks perfect to me :)

- Moiz
 
> - Kalle
> 
> >
> > >
> > >  	/* reset ASAP, clearing any IRQs */
> > > 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
> >
> > Regards
> > Moiz Sonasath
> >

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

[kalle.jokiniemi]

Hi,


> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of ext 
> Sonasath, Moiz
> Sent: 23. marraskuuta 2009 18:10
> To: Kalle Jokiniemi
> Cc: khilman@deeprootsystems.com; linux-omap@vger.kernel.org; 
> Jarkko Nikula; Paul Walmsley; Menon, Nishanth; Pandita, 
> Vikram; Hogander Jouni (Nokia-D/Tampere)
> Subject: RE: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up 
> latency constraint in i2c
> 

<<snip-snip>>

> > 
> > Well, I don't completely agree with the necessity of preparing for 
> > different rx/tx thresholds. For this to make sense, the i2c-omap 
> > driver should first separate in it's code the use of rx and tx 
> > thresholds. If someone is planning to do that, he/she should anyway 
> > update the usage of fifo_size throughout the code, 
> including the wake up latency setting.
> > 
> > Anyways, attached a patch that separates the mpu wake up 
> latencies for 
> > rx and tx. In case that is needed. Though I'm not for it, since it 
> > adds unneeded complexity.
> > 
> 
> Yes Kalle, you are right! Not having different RX/TX wake-up 
> latencies will absolutely work fine with the in-place code as 
> we have both the RX/TX thresholds same.

Great :)

> But, I think in 
> future we might have to play around with different RX/TX 
> thresholds and so from a conceptually right and generic point 
> of view it makes sense to incur the cost of the added complexity.

I agree. Once someone differentiates the thresholds in the driver, it very much makes sense to also calculate different mpu wake up latencies for RX and TX.

> 
> The patch V4 looks perfect to me :)

Great :)

So, where do we push it?

- Kalle


> 
> - Moiz
>  
> > - Kalle
> > 
> > >
> > > >
> > > >  	/* reset ASAP, clearing any IRQs */ 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
> > >
> > > Regards
> > > Moiz Sonasath
> > >
> --
> 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 V4 3/3] OMAP: I2C: Add mpu wake up latency constraint in i2c

[Kevin Hilman]

<kalle.jokiniemi@nokia.com> writes:

>> The patch V4 looks perfect to me :)
>
> Great :)
>
> So, where do we push it?

I suggest to Ben Dooks (i2c maintainer for embedded platforms) and to linux-i2c list.
Also Cc linux-omap.  


>From MAINTAINERS:

I2C SUBSYSTEM
M:	"Jean Delvare (PC drivers, core)" <khali@linux-fr.org>
M:	"Ben Dooks (embedded platforms)" <ben-linux@fluff.org>
L:	linux-i2c@vger.kernel.org
W:	http://i2c.wiki.kernel.org/
T:	quilt kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/
S:	Maintained
F:	Documentation/i2c/
F:	drivers/i2c/
F:	include/linux/i2c.h
F:	include/linux/i2c-dev.h
F:	include/linux/i2c-id.h


Also, FYI... in recent kernels there is a scripts/get_maintainer.pl
script.  If you run a patch through that script, it will spit out a
list of folks to send the patch to.

Kevin

Re: [PATCH 2/3] PM: Skip PER previous state register read

[Kevin Hilman]

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

> According to measurements, reading the previous state of PER
> domain after wfi takes ~11us on OPP2.
>
> Removed this unneccessary latency from cases where we know
> PER power domain did not try to enter off mode.
>
> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>

Looks good, pulling into PM branch.

But still amazed at such a *huge* latency to read that register.

Kevin

> ---
>  arch/arm/mach-omap2/pm34xx.c |   19 +++++++++++++------
>  1 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 237c819..b70ea19 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -489,12 +489,19 @@ void omap_sram_idle(void)
>  
>  	/* PER */
>  	if (per_next_state < PWRDM_POWER_ON) {
> -		per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
> -		if (per_prev_state == PWRDM_POWER_OFF) {
> -			omap3_per_restore_context();
> -			omap3_gpio_restore_pad_context(0);
> -		} else if (per_next_state == PWRDM_POWER_OFF)
> -			omap3_gpio_restore_pad_context(1);
> +		if (per_next_state == PWRDM_POWER_OFF) {
> +			/*
> +			 * Reading the prev-state takes long time (11us@OPP2),
> +			 * only do it, if we really tried to put PER in OFF
> +			 */
> +			per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
> +			if (per_prev_state == PWRDM_POWER_OFF) {
> +				omap3_per_restore_context();
> +				omap3_gpio_restore_pad_context(0);
> +			} else if (per_next_state == PWRDM_POWER_OFF) {
> +				omap3_gpio_restore_pad_context(1);
> +			}
> +		}
>  		omap2_gpio_resume_after_idle();
>  		omap_uart_resume_idle(2);
>  		if (per_state_modified)
> -- 
> 1.5.4.3

RE: [PATCH 2/3] PM: Skip PER previous state register read

[kalle.jokiniemi]

Hi Kevin, 

>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org 
>[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of ext Kevin Hilman
>Sent: 30. lokakuuta 2009 18:32
>To: Kalle Jokiniemi
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCH 2/3] PM: Skip PER previous state register read
>
>Kalle Jokiniemi <kalle.jokiniemi@digia.com> writes:
>
>> According to measurements, reading the previous state of PER domain 
>> after wfi takes ~11us on OPP2.
>>
>> Removed this unneccessary latency from cases where we know PER power 
>> domain did not try to enter off mode.
>>
>> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
>
>Looks good, pulling into PM branch.
>
>But still amazed at such a *huge* latency to read that register.

We talked about this with Peter, and he suggested the reason might
be that the register is in WKUP domain, which runs on slow clock.
Checking from TRM, it is mentioned that at least in low-power mode,
the 32k clock is used in PRM. Could not figure out what is used in
normal mode, though. 

Probably the same reason makes the debug counter updates so slow.

- Kalle


>
>Kevin
>
>> ---
>>  arch/arm/mach-omap2/pm34xx.c |   19 +++++++++++++------
>>  1 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c 
>> b/arch/arm/mach-omap2/pm34xx.c index 237c819..b70ea19 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -489,12 +489,19 @@ void omap_sram_idle(void)
>>  
>>  	/* PER */
>>  	if (per_next_state < PWRDM_POWER_ON) {
>> -		per_prev_state = pwrdm_read_prev_pwrst(per_pwrdm);
>> -		if (per_prev_state == PWRDM_POWER_OFF) {
>> -			omap3_per_restore_context();
>> -			omap3_gpio_restore_pad_context(0);
>> -		} else if (per_next_state == PWRDM_POWER_OFF)
>> -			omap3_gpio_restore_pad_context(1);
>> +		if (per_next_state == PWRDM_POWER_OFF) {
>> +			/*
>> +			 * Reading the prev-state takes long 
>time (11us@OPP2),
>> +			 * only do it, if we really tried to 
>put PER in OFF
>> +			 */
>> +			per_prev_state = 
>pwrdm_read_prev_pwrst(per_pwrdm);
>> +			if (per_prev_state == PWRDM_POWER_OFF) {
>> +				omap3_per_restore_context();
>> +				omap3_gpio_restore_pad_context(0);
>> +			} else if (per_next_state == PWRDM_POWER_OFF) {
>> +				omap3_gpio_restore_pad_context(1);
>> +			}
>> +		}
>>  		omap2_gpio_resume_after_idle();
>>  		omap_uart_resume_idle(2);
>>  		if (per_state_modified)
>> --
>> 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/3] OMAP3: Only update pm-counters when needed

[Kevin Hilman]

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

> From: Kalle Jokiniemi <ext-kalle.jokiniemi@nokia.com>
>
> The biggest source of latency in idle loop (omap_sram_idle
> function) comes from updating the state counters for each
> power domain. The two purposes of these counters are to
> provide debug data in debugfs, and to keep track of context
> losses occurring during idle transitions (off mode counters).
>
> I created new debugfs interface "enable_count" for
> enabling the "count" interface, which exposes the debug
> part of these counters. The counters are not updated
> anymore for CORE ON idle transitions, when the count
> interface is disabled. For deeper CORE states, counters
> are still updated to preserve context loss tracking.
>
> This change decreases C1/C2 state latency over 100us at
> OPP2.
>
> Signed-off-by: Kalle Jokiniemi <ext-kalle.jokiniemi@nokia.com>

I'm not opposed to this patch in principle, but I wonder if we might
be able to get rid of most of this delay with some optimizations in
the powerdomain code.

I'm guessing it's the reads from the PRCM that are causing such a
delay since otherwise, that should be a pretty fast codepath.  In this
case there are reads of PWRSTST and reads and writes of PREPWRSTST for
each powerdomain.

Paul has mentioned a few times the idea of having some powerdomain
enhancements that cache a copy of some of these registers so they
don't have to always be read/written from the PRCM, although I'm not
sure without digging some more if that makes sense for the PWRST and
PREPWRST registers.

I think we could also be a bit smarter about doing the transition
counters for every powerdomain.  During most of the transitions, we
don't actually expect a transition for many of the powerdomains
because a transition hasn't been programmed.  Maybe we can skip the
PRM reads in those cases.

Anyways, some comments on the actual patch below...


> ---
>  arch/arm/mach-omap2/pm-debug.c |   51 ++++++++++++++++++++++++++++++++++++++-
>  arch/arm/mach-omap2/pm.h       |    2 +
>  arch/arm/mach-omap2/pm34xx.c   |   12 +++++----
>  3 files changed, 58 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> index 5aac64f..76c2696 100644
> --- a/arch/arm/mach-omap2/pm-debug.c
> +++ b/arch/arm/mach-omap2/pm-debug.c
> @@ -37,6 +37,9 @@
>  #include "prm-regbits-34xx.h"
>  
>  int omap2_pm_debug;
> +static int pm_dbg_count_active;
> +static struct dentry *de_pm_debug_count;
> +static struct dentry *de_pm_debug;
>  
>  #define DUMP_PRM_MOD_REG(mod, reg)    \
>  	regs[reg_count].name = #mod "." #reg; \
> @@ -327,6 +330,11 @@ int pm_dbg_regset_save(int reg_set)
>  	return 0;
>  }
>  
> +int pm_dbg_count_is_active(void)
> +{
> +	return pm_dbg_count_active;
> +}
> +
>  static const char pwrdm_state_names[][4] = {
>  	"OFF",
>  	"RET",
> @@ -460,6 +468,40 @@ static const struct file_operations debug_reg_fops = {
>  	.release        = single_release,
>  };
>  
> +static int pm_dbg_count_enable_get(void *data, u64 *val)
> +{
> +	*val = pm_dbg_count_active;
> +	return 0;
> +}
> +
> +static int pm_dbg_count_enable_set(void *data, u64 val)
> +{
> +	if (val > 1) {
> +		printk(KERN_ERR "Invalid value! 1 to enable, 0 to disable\n");
> +		return -EINVAL;
> +	}
> +
> +	if (val == 1 && !pm_dbg_count_active) {
> +		de_pm_debug_count = debugfs_create_file("count", S_IRUGO,
> +			de_pm_debug, (void *)DEBUG_FILE_COUNTERS, &debug_fops);
> +
> +		if (de_pm_debug_count == NULL) {
> +			printk(KERN_ERR "Error: could not create debugfs "
> +					"entry\n");
> +			return -ENOMEM;
> +		}
> +		pm_dbg_count_active = 1;
> +	} else if (val == 0 && pm_dbg_count_active) {
> +		debugfs_remove(de_pm_debug_count);
> +		de_pm_debug_count = NULL;
> +		pm_dbg_count_active = 0;
> +	}
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(enable_count_fops, pm_dbg_count_enable_get,
> +				pm_dbg_count_enable_set, "%llu\n");
> +
>  int pm_dbg_regset_init(int reg_set)
>  {
>  	char name[2];
> @@ -576,12 +618,17 @@ static int __init pm_dbg_init(void)
>  		return -ENODEV;
>  	}
>  
> +	pm_dbg_count_active = 0;
> +
>  	d = debugfs_create_dir("pm_debug", NULL);
>  	if (IS_ERR(d))
>  		return PTR_ERR(d);
> +	de_pm_debug = d;
> +
> +	(void) debugfs_create_file("enable_count", S_IRUGO |
> +				S_IWUGO, d, &pm_dbg_count_active,
> +				&enable_count_fops);
>  
> -	(void) debugfs_create_file("count", S_IRUGO,
> -		d, (void *)DEBUG_FILE_COUNTERS, &debug_fops);
>  	(void) debugfs_create_file("time", S_IRUGO,
>  		d, (void *)DEBUG_FILE_TIMERS, &debug_fops);
>  
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index f8d11a2..3f0202b 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -63,12 +63,14 @@ extern int omap2_pm_debug;
>  extern void pm_dbg_update_time(struct powerdomain *pwrdm, int prev);
>  extern int pm_dbg_regset_save(int reg_set);
>  extern int pm_dbg_regset_init(int reg_set);
> +extern int pm_dbg_count_is_active(void);
>  #else
>  #define omap2_pm_dump(mode, resume, us)		do {} while (0);
>  #define omap2_pm_debug				0
>  #define pm_dbg_update_time(pwrdm, prev) do {} while (0);
>  #define pm_dbg_regset_save(reg_set) do {} while (0);
>  #define pm_dbg_regset_init(reg_set) do {} while (0);
> +#define pm_dbg_count_is_active()		0
>  #endif /* CONFIG_PM_DEBUG */
>  
>  extern void omap24xx_idle_loop_suspend(void);
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 01260ec..237c819 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -378,15 +378,17 @@ void omap_sram_idle(void)
>  		return;
>  	}
>  
> -	pwrdm_pre_transition();
> +	per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
> +	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
> +
> +	if (pm_dbg_count_is_active() || (core_next_state != PWRDM_POWER_ON))
> +		pwrdm_pre_transition();

I'm getting concerned abou tall the special case checking etc. that
we're doing inside omap_sram_idle().  It's starting to feel a bit
cluttered.

I think I'd rather see this checking moved into the powerdomain code
itself.

>  	/* NEON control */
>  	if (pwrdm_read_pwrst(neon_pwrdm) == PWRDM_POWER_ON)
>  		pwrdm_set_next_pwrst(neon_pwrdm, mpu_next_state);
>  
>  	/* PER */
> -	per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
> -	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
>  	if (per_next_state < PWRDM_POWER_ON) {
>  		omap_uart_prepare_idle(2);
>  		omap2_gpio_prepare_for_idle(per_next_state);
> @@ -505,8 +507,8 @@ void omap_sram_idle(void)
>  		omap3_disable_io_chain();
>  	}
>  
> -
> -	pwrdm_post_transition();
> +	if (pm_dbg_count_is_active() || (core_next_state != PWRDM_POWER_ON))
> +		pwrdm_post_transition();
>  
>  	omap2_clkdm_allow_idle(mpu_pwrdm->pwrdm_clkdms[0]);
>  }
> -- 
> 1.5.4.3

Re: [PATCH 1/3] OMAP3: Only update pm-counters when needed

[Kalle Jokiniemi]

Hi Kevin,

On Fri, 2009-10-30 at 01:07 +0200, Kevin Hilman wrote:
> Kalle Jokiniemi <kalle.jokiniemi@digia.com> writes:
> 
> > From: Kalle Jokiniemi <ext-kalle.jokiniemi@nokia.com>
> >
> > The biggest source of latency in idle loop (omap_sram_idle
> > function) comes from updating the state counters for each
> > power domain. The two purposes of these counters are to
> > provide debug data in debugfs, and to keep track of context
> > losses occurring during idle transitions (off mode counters).
> >
> > I created new debugfs interface "enable_count" for
> > enabling the "count" interface, which exposes the debug
> > part of these counters. The counters are not updated
> > anymore for CORE ON idle transitions, when the count
> > interface is disabled. For deeper CORE states, counters
> > are still updated to preserve context loss tracking.
> >
> > This change decreases C1/C2 state latency over 100us at
> > OPP2.
> >
> > Signed-off-by: Kalle Jokiniemi <ext-kalle.jokiniemi@nokia.com>
> 
> I'm not opposed to this patch in principle, but I wonder if we might
> be able to get rid of most of this delay with some optimizations in
> the powerdomain code.

Well, I had similar thoughts, but opted just disabling what I could when
not using the debugfs. It seems like valid approach, since we do not
want unused debug functionality to hinder performance.

With more thinkwork and these discussions, I'm sure we can get the
powerdomain code to perform much better, in addition to my patch.

> 
> I'm guessing it's the reads from the PRCM that are causing such a
> delay since otherwise, that should be a pretty fast codepath.  In this
> case there are reads of PWRSTST and reads and writes of PREPWRSTST for
> each powerdomain.

Yes, I wonder why they are so slow? I noticed the same in patch 2/3 of
this set: reading the PREPWRSTST_PER seems to take over 10us at OPP2.
That's a huge delay for a simple register access IMO. I even tried
removing all the fancy bit-toggling and use a direct read with a
bitmask, but that did not have effect, still over 10us.


> 
> Paul has mentioned a few times the idea of having some powerdomain
> enhancements that cache a copy of some of these registers so they
> don't have to always be read/written from the PRCM, although I'm not
> sure without digging some more if that makes sense for the PWRST and
> PREPWRST registers.

If I can make it today, I'll check how long a PWRST reg read takes. I
know the PREPWRST takes long. Reading the next state (PWRSTCTRL) is fast
according to my earlier measurements.

> 
> I think we could also be a bit smarter about doing the transition
> counters for every powerdomain.  During most of the transitions, we
> don't actually expect a transition for many of the powerdomains
> because a transition hasn't been programmed.  Maybe we can skip the
> PRM reads in those cases.

This is a good point. There's no sense in updating the statuses for
non-transitioning power domains (and spending tens of us waiting for
those updates).

> 
> Anyways, some comments on the actual patch below...
> 
> 
> > ---
> >  arch/arm/mach-omap2/pm-debug.c |   51 ++++++++++++++++++++++++++++++++++++++-
> >  arch/arm/mach-omap2/pm.h       |    2 +
> >  arch/arm/mach-omap2/pm34xx.c   |   12 +++++----
> >  3 files changed, 58 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> > index 5aac64f..76c2696 100644
> > --- a/arch/arm/mach-omap2/pm-debug.c
> > +++ b/arch/arm/mach-omap2/pm-debug.c
> > @@ -37,6 +37,9 @@
> >  #include "prm-regbits-34xx.h"
> >  
> >  int omap2_pm_debug;
> > +static int pm_dbg_count_active;
> > +static struct dentry *de_pm_debug_count;
> > +static struct dentry *de_pm_debug;
> >  
> >  #define DUMP_PRM_MOD_REG(mod, reg)    \
> >  	regs[reg_count].name = #mod "." #reg; \
> > @@ -327,6 +330,11 @@ int pm_dbg_regset_save(int reg_set)
> >  	return 0;
> >  }
> >  
> > +int pm_dbg_count_is_active(void)
> > +{
> > +	return pm_dbg_count_active;
> > +}
> > +
> >  static const char pwrdm_state_names[][4] = {
> >  	"OFF",
> >  	"RET",
> > @@ -460,6 +468,40 @@ static const struct file_operations debug_reg_fops = {
> >  	.release        = single_release,
> >  };
> >  
> > +static int pm_dbg_count_enable_get(void *data, u64 *val)
> > +{
> > +	*val = pm_dbg_count_active;
> > +	return 0;
> > +}
> > +
> > +static int pm_dbg_count_enable_set(void *data, u64 val)
> > +{
> > +	if (val > 1) {
> > +		printk(KERN_ERR "Invalid value! 1 to enable, 0 to disable\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (val == 1 && !pm_dbg_count_active) {
> > +		de_pm_debug_count = debugfs_create_file("count", S_IRUGO,
> > +			de_pm_debug, (void *)DEBUG_FILE_COUNTERS, &debug_fops);
> > +
> > +		if (de_pm_debug_count == NULL) {
> > +			printk(KERN_ERR "Error: could not create debugfs "
> > +					"entry\n");
> > +			return -ENOMEM;
> > +		}
> > +		pm_dbg_count_active = 1;
> > +	} else if (val == 0 && pm_dbg_count_active) {
> > +		debugfs_remove(de_pm_debug_count);
> > +		de_pm_debug_count = NULL;
> > +		pm_dbg_count_active = 0;
> > +	}
> > +	return 0;
> > +}
> > +
> > +DEFINE_SIMPLE_ATTRIBUTE(enable_count_fops, pm_dbg_count_enable_get,
> > +				pm_dbg_count_enable_set, "%llu\n");
> > +
> >  int pm_dbg_regset_init(int reg_set)
> >  {
> >  	char name[2];
> > @@ -576,12 +618,17 @@ static int __init pm_dbg_init(void)
> >  		return -ENODEV;
> >  	}
> >  
> > +	pm_dbg_count_active = 0;
> > +
> >  	d = debugfs_create_dir("pm_debug", NULL);
> >  	if (IS_ERR(d))
> >  		return PTR_ERR(d);
> > +	de_pm_debug = d;
> > +
> > +	(void) debugfs_create_file("enable_count", S_IRUGO |
> > +				S_IWUGO, d, &pm_dbg_count_active,
> > +				&enable_count_fops);
> >  
> > -	(void) debugfs_create_file("count", S_IRUGO,
> > -		d, (void *)DEBUG_FILE_COUNTERS, &debug_fops);
> >  	(void) debugfs_create_file("time", S_IRUGO,
> >  		d, (void *)DEBUG_FILE_TIMERS, &debug_fops);
> >  
> > diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> > index f8d11a2..3f0202b 100644
> > --- a/arch/arm/mach-omap2/pm.h
> > +++ b/arch/arm/mach-omap2/pm.h
> > @@ -63,12 +63,14 @@ extern int omap2_pm_debug;
> >  extern void pm_dbg_update_time(struct powerdomain *pwrdm, int prev);
> >  extern int pm_dbg_regset_save(int reg_set);
> >  extern int pm_dbg_regset_init(int reg_set);
> > +extern int pm_dbg_count_is_active(void);
> >  #else
> >  #define omap2_pm_dump(mode, resume, us)		do {} while (0);
> >  #define omap2_pm_debug				0
> >  #define pm_dbg_update_time(pwrdm, prev) do {} while (0);
> >  #define pm_dbg_regset_save(reg_set) do {} while (0);
> >  #define pm_dbg_regset_init(reg_set) do {} while (0);
> > +#define pm_dbg_count_is_active()		0
> >  #endif /* CONFIG_PM_DEBUG */
> >  
> >  extern void omap24xx_idle_loop_suspend(void);
> > diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> > index 01260ec..237c819 100644
> > --- a/arch/arm/mach-omap2/pm34xx.c
> > +++ b/arch/arm/mach-omap2/pm34xx.c
> > @@ -378,15 +378,17 @@ void omap_sram_idle(void)
> >  		return;
> >  	}
> >  
> > -	pwrdm_pre_transition();
> > +	per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
> > +	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
> > +
> > +	if (pm_dbg_count_is_active() || (core_next_state != PWRDM_POWER_ON))
> > +		pwrdm_pre_transition();
> 
> I'm getting concerned abou tall the special case checking etc. that
> we're doing inside omap_sram_idle().  It's starting to feel a bit
> cluttered.

I agree, but on the other hand, we do need these checks in one form or
other. I think we could at least make it more readable by moving for
example power-domain specific preparations/post-transition activities to
separate functions.

> 
> I think I'd rather see this checking moved into the powerdomain code
> itself.

You're right, it'll fit more naturally there.

Thanks for the good comments! I think were getting good discussion here.
It seems we need to refactor our idle loop and optimize powerdomain code
to offer better readability and performance.

I'll fix my patch and try to think some ideas on how to improve the
state transition updating.

- Kalle


> 
> >  	/* NEON control */
> >  	if (pwrdm_read_pwrst(neon_pwrdm) == PWRDM_POWER_ON)
> >  		pwrdm_set_next_pwrst(neon_pwrdm, mpu_next_state);
> >  
> >  	/* PER */
> > -	per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
> > -	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
> >  	if (per_next_state < PWRDM_POWER_ON) {
> >  		omap_uart_prepare_idle(2);
> >  		omap2_gpio_prepare_for_idle(per_next_state);
> > @@ -505,8 +507,8 @@ void omap_sram_idle(void)
> >  		omap3_disable_io_chain();
> >  	}
> >  
> > -
> > -	pwrdm_post_transition();
> > +	if (pm_dbg_count_is_active() || (core_next_state != PWRDM_POWER_ON))
> > +		pwrdm_post_transition();
> >  
> >  	omap2_clkdm_allow_idle(mpu_pwrdm->pwrdm_clkdms[0]);
> >  }
> > -- 
> > 1.5.4.3

Re: [PATCH 0/3] PM: Misc latency fixes

[Kalle Jokiniemi]

Hi Kevin,

On Wed, 2009-10-21 at 14:51 +0300, Kalle Jokiniemi wrote:
> Hello,
> Here are some fruits from digging out the latency sources
> of our idle loop. The main latency source was powerdomain
> state counter updating at beginning and end of the idle
> loop. Also PER previous state reading strangely seemed to
> cause some latency with significance. Could not find any
> TRM or errata comment to why this is, though.
> 
> The I2C mpu wakeup latency constraint patch has been updated
> to calculate latencies at boot from clkrate and fifo size.
> This was included in this set, since it benefits from the
> reduced latency of the other patches.
> 
> Patches tested on linux-omap/pm and rx-51.
> 
> Kalle Jokiniemi (3):
>       OMAP3: Only update pm-counters when needed
>       PM: Skip PER previous state register read

How about those latency optimization patches? Are you are you going to
take them in? I'd like to know if there is something to fix in those, so
I can finish it this week. Starting next week I'll be moving to other
responsibilities and might not be able to do additional changes for the
patches.

>       OMAP: I2C: Add mpu wake up latency constraint in i2c

This i2c patch can be pushed separately, in case you're waiting for that
thread to finish.

- Kalle

> 
>  arch/arm/mach-omap2/pm-debug.c |   51 ++++++++++++++++++++++++++++++++++++-
>  arch/arm/mach-omap2/pm.h       |    2 +
>  arch/arm/mach-omap2/pm34xx.c   |   31 ++++++++++++++--------
>  arch/arm/plat-omap/i2c.c       |   54 +++++++++++++++++++++++++++++++---------
>  drivers/i2c/busses/i2c-omap.c  |   25 +++++++++++++++---
>  include/linux/i2c-omap.h       |    9 ++++++
>  6 files changed, 143 insertions(+), 29 deletions(-)
> 
> 
> --
> 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