Hi, On Mon, 2009-10-05 at 20:08 +0300, Pandita, Vikram wrote: > Jokiniemi > > >-----Original Message----- > >From: [email protected] > >[mailto:[email protected]] On Behalf Of Kalle > >Jokiniemi > >Sent: Thursday, September 17, 2009 11:29 AM > >To: [email protected] > >Cc: [email protected]; [email protected]; 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 <[email protected]> > >--- > > 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 [email protected] > >More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
