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

[<kalle.jokiniemi@nokia.com>]

[-- 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