* Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-09-17 16:28 ` [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c Kalle Jokiniemi
@ 2009-09-30 16:36 ` Kevin Hilman
2009-10-01 7:44 ` Kalle Jokiniemi
2009-10-01 6:10 ` Jarkko Nikula
2009-10-05 17:08 ` Pandita, Vikram
2 siblings, 1 reply; 15+ messages in thread
From: Kevin Hilman @ 2009-09-30 16:36 UTC (permalink / raw)
To: Kalle Jokiniemi; +Cc: jhnikula, linux-omap
Kalle Jokiniemi <kalle.jokiniemi@digia.com> writes:
> While waiting for completion of the i2c transfer, the
> MPU could hit OFF mode and cause several msecs of
> delay that made i2c transfers fail more often. The
> extra delays and subsequent re-trys cause i2c clocks
> to be active more often. This has also an negative
> effect on power consumption.
>
> Added a constraint that allows MPU to wake up in few
> hundred usecs, which is roughly the average i2c wait
> period.
>
> The constraint function is passed as platform data from
> plat-omap/i2c.c and applied only on OMAP34XX devices.
Seems like the latency value should also be (optionally) passed in
pdata so this can be experimented with per-platform.
Other than that looks ok to me.
Do we have an official OMAP I2C maintainer who should signoff on this?
Kevin
> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
> ---
> arch/arm/plat-omap/i2c.c | 49 +++++++++++++++++++++++++++++++----------
> drivers/i2c/busses/i2c-omap.c | 17 +++++++++++---
> include/linux/i2c-omap.h | 9 +++++++
> 3 files changed, 59 insertions(+), 16 deletions(-)
> create mode 100644 include/linux/i2c-omap.h
>
> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> index 8b84839..d43d0ad 100644
> --- a/arch/arm/plat-omap/i2c.c
> +++ b/arch/arm/plat-omap/i2c.c
> @@ -26,8 +26,10 @@
> #include <linux/kernel.h>
> #include <linux/platform_device.h>
> #include <linux/i2c.h>
> +#include <linux/i2c-omap.h>
> #include <mach/irqs.h>
> #include <mach/mux.h>
> +#include <mach/omap-pm.h>
>
> #define OMAP_I2C_SIZE 0x3f
> #define OMAP1_I2C_BASE 0xfffb3800
> @@ -69,14 +71,14 @@ static struct resource i2c_resources[][2] = {
> }, \
> }
>
> -static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> +static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
> static struct platform_device omap_i2c_devices[] = {
> - I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> + I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
> #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> - I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> + I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
> #endif
> #if defined(CONFIG_ARCH_OMAP34XX)
> - I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> + I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
> #endif
> };
>
> @@ -100,6 +102,25 @@ static const int omap34xx_pins[][2] = {};
>
> #define OMAP_I2C_CMDLINE_SETUP (BIT(31))
>
> +/**
> + * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
> + * @dev: i2c bus device pointer
> + * @set: set or clear constraints
> + *
> + * When waiting for completion of a i2c transfer, we need to set a wake up
> + * latency constraint for the MPU. This is to ensure quick enough wakeup
> + * from idle, when transfer completes.
> + */
> +#ifdef CONFIG_ARCH_OMAP34XX
> +static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> +{
> + omap_pm_set_max_mpu_wakeup_lat(dev, set ? 500 : -1);
> +}
> +#else
> +static inline void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> +{; }
> +#endif
> +
> static void __init omap_i2c_mux_pins(int bus)
> {
> int scl, sda;
> @@ -180,8 +201,8 @@ static int __init omap_i2c_bus_setup(char *str)
> get_options(str, 3, ints);
> if (ints[0] < 2 || ints[1] < 1 || ints[1] > ports)
> return 0;
> - i2c_rate[ints[1] - 1] = ints[2];
> - i2c_rate[ints[1] - 1] |= OMAP_I2C_CMDLINE_SETUP;
> + i2c_pdata[ints[1] - 1].clkrate = ints[2];
> + i2c_pdata[ints[1] - 1].clkrate |= OMAP_I2C_CMDLINE_SETUP;
>
> return 1;
> }
> @@ -195,9 +216,11 @@ static int __init omap_register_i2c_bus_cmdline(void)
> {
> int i, err = 0;
>
> - for (i = 0; i < ARRAY_SIZE(i2c_rate); i++)
> - if (i2c_rate[i] & OMAP_I2C_CMDLINE_SETUP) {
> - i2c_rate[i] &= ~OMAP_I2C_CMDLINE_SETUP;
> + for (i = 0; i < ARRAY_SIZE(i2c_pdata); i++)
> + if (i2c_pdata[i].clkrate & OMAP_I2C_CMDLINE_SETUP) {
> + i2c_pdata[i].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
> + i2c_pdata[i].set_mpu_wkup_lat =
> + omap_i2c_set_wfc_mpu_wkup_lat;
> err = omap_i2c_add_bus(i + 1);
> if (err)
> goto out;
> @@ -231,9 +254,11 @@ int __init omap_register_i2c_bus(int bus_id, u32 clkrate,
> return err;
> }
>
> - if (!i2c_rate[bus_id - 1])
> - i2c_rate[bus_id - 1] = clkrate;
> - i2c_rate[bus_id - 1] &= ~OMAP_I2C_CMDLINE_SETUP;
> + if (!i2c_pdata[bus_id - 1].clkrate)
> + i2c_pdata[bus_id - 1].clkrate = clkrate;
> +
> + i2c_pdata[bus_id - 1].set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
> + i2c_pdata[bus_id - 1].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
>
> return omap_i2c_add_bus(bus_id);
> }
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 75bf3ad..da6d276 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -37,6 +37,7 @@
> #include <linux/platform_device.h>
> #include <linux/clk.h>
> #include <linux/io.h>
> +#include <linux/i2c-omap.h>
>
> /* I2C controller revisions */
> #define OMAP_I2C_REV_2 0x20
> @@ -165,6 +166,8 @@ struct omap_i2c_dev {
> struct clk *fclk; /* Functional clock */
> struct completion cmd_complete;
> struct resource *ioarea;
> + void (*set_mpu_wkup_lat)(struct device *dev,
> + int set);
> u32 speed; /* Speed of bus in Khz */
> u16 cmd_err;
> u8 *buf;
> @@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> * REVISIT: We should abort the transfer on signals, but the bus goes
> * into arbitration and we're currently unable to recover from it.
> */
> + dev->set_mpu_wkup_lat(dev->dev, 1);
> r = wait_for_completion_timeout(&dev->cmd_complete,
> OMAP_I2C_TIMEOUT);
> + dev->set_mpu_wkup_lat(dev->dev, 0);
> dev->buf_len = 0;
> if (r < 0)
> return r;
> @@ -844,6 +849,7 @@ omap_i2c_probe(struct platform_device *pdev)
> struct omap_i2c_dev *dev;
> struct i2c_adapter *adap;
> struct resource *mem, *irq, *ioarea;
> + struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
> irq_handler_t isr;
> int r;
> u32 speed = 0;
> @@ -873,10 +879,13 @@ omap_i2c_probe(struct platform_device *pdev)
> goto err_release_region;
> }
>
> - if (pdev->dev.platform_data != NULL)
> - speed = *(u32 *)pdev->dev.platform_data;
> - else
> - speed = 100; /* Defualt speed */
> + if (pdata != NULL) {
> + speed = pdata->clkrate;
> + dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> + } else {
> + speed = 100; /* Default speed */
> + dev->set_mpu_wkup_lat = NULL;
> + }
>
> dev->speed = speed;
> dev->idle = 1;
> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> new file mode 100644
> index 0000000..1362fba
> --- /dev/null
> +++ b/include/linux/i2c-omap.h
> @@ -0,0 +1,9 @@
> +#ifndef __I2C_OMAP_H__
> +#define __I2C_OMAP_H__
> +
> +struct omap_i2c_bus_platform_data {
> + u32 clkrate;
> + void (*set_mpu_wkup_lat)(struct device *dev, int set);
> +};
> +
> +#endif
> --
> 1.5.4.3
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-09-30 16:36 ` Kevin Hilman
@ 2009-10-01 7:44 ` Kalle Jokiniemi
2009-10-01 11:41 ` Aaro Koskinen
2009-10-01 14:58 ` Kevin Hilman
0 siblings, 2 replies; 15+ messages in thread
From: Kalle Jokiniemi @ 2009-10-01 7:44 UTC (permalink / raw)
To: Kevin Hilman; +Cc: jhnikula@gmail.com, linux-omap@vger.kernel.org
On Wed, 2009-09-30 at 19:36 +0300, Kevin Hilman wrote:
> Kalle Jokiniemi <kalle.jokiniemi@digia.com> writes:
>
> > While waiting for completion of the i2c transfer, the
> > MPU could hit OFF mode and cause several msecs of
> > delay that made i2c transfers fail more often. The
> > extra delays and subsequent re-trys cause i2c clocks
> > to be active more often. This has also an negative
> > effect on power consumption.
> >
> > Added a constraint that allows MPU to wake up in few
> > hundred usecs, which is roughly the average i2c wait
> > period.
> >
> > The constraint function is passed as platform data from
> > plat-omap/i2c.c and applied only on OMAP34XX devices.
>
> Seems like the latency value should also be (optionally) passed in
> pdata so this can be experimented with per-platform.
Well, it kind of is already, since we pass the function that sets the
latency from platform code. And that function has the latency
hard-coded. My though was to use little #ifdeffery in defining the
function (and latency) for say omap3, and omap4 platforms.
If #ifdeffery is out of the question, we could define
omap3_i2c_set_mpu.. and omap4_i2c_set_mpu.. functions and pass one in
init according to some "if (cpu_is_omapxyz)" statement. I think omap1&2
don't need any latency constraints (and cannot use them as there is no
pm-layer implementation for those).
What do you think?
- Kalle
>
> Other than that looks ok to me.
>
> Do we have an official OMAP I2C maintainer who should signoff on this?
>
> Kevin
>
>
> > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
> > ---
> > arch/arm/plat-omap/i2c.c | 49 +++++++++++++++++++++++++++++++----------
> > drivers/i2c/busses/i2c-omap.c | 17 +++++++++++---
> > include/linux/i2c-omap.h | 9 +++++++
> > 3 files changed, 59 insertions(+), 16 deletions(-)
> > create mode 100644 include/linux/i2c-omap.h
> >
> > diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> > index 8b84839..d43d0ad 100644
> > --- a/arch/arm/plat-omap/i2c.c
> > +++ b/arch/arm/plat-omap/i2c.c
> > @@ -26,8 +26,10 @@
> > #include <linux/kernel.h>
> > #include <linux/platform_device.h>
> > #include <linux/i2c.h>
> > +#include <linux/i2c-omap.h>
> > #include <mach/irqs.h>
> > #include <mach/mux.h>
> > +#include <mach/omap-pm.h>
> >
> > #define OMAP_I2C_SIZE 0x3f
> > #define OMAP1_I2C_BASE 0xfffb3800
> > @@ -69,14 +71,14 @@ static struct resource i2c_resources[][2] = {
> > }, \
> > }
> >
> > -static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> > +static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
> > static struct platform_device omap_i2c_devices[] = {
> > - I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> > + I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
> > #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> > - I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> > + I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
> > #endif
> > #if defined(CONFIG_ARCH_OMAP34XX)
> > - I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> > + I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
> > #endif
> > };
> >
> > @@ -100,6 +102,25 @@ static const int omap34xx_pins[][2] = {};
> >
> > #define OMAP_I2C_CMDLINE_SETUP (BIT(31))
> >
> > +/**
> > + * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
> > + * @dev: i2c bus device pointer
> > + * @set: set or clear constraints
> > + *
> > + * When waiting for completion of a i2c transfer, we need to set a wake up
> > + * latency constraint for the MPU. This is to ensure quick enough wakeup
> > + * from idle, when transfer completes.
> > + */
> > +#ifdef CONFIG_ARCH_OMAP34XX
> > +static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> > +{
> > + omap_pm_set_max_mpu_wakeup_lat(dev, set ? 500 : -1);
> > +}
> > +#else
> > +static inline void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> > +{; }
> > +#endif
> > +
> > static void __init omap_i2c_mux_pins(int bus)
> > {
> > int scl, sda;
> > @@ -180,8 +201,8 @@ static int __init omap_i2c_bus_setup(char *str)
> > get_options(str, 3, ints);
> > if (ints[0] < 2 || ints[1] < 1 || ints[1] > ports)
> > return 0;
> > - i2c_rate[ints[1] - 1] = ints[2];
> > - i2c_rate[ints[1] - 1] |= OMAP_I2C_CMDLINE_SETUP;
> > + i2c_pdata[ints[1] - 1].clkrate = ints[2];
> > + i2c_pdata[ints[1] - 1].clkrate |= OMAP_I2C_CMDLINE_SETUP;
> >
> > return 1;
> > }
> > @@ -195,9 +216,11 @@ static int __init omap_register_i2c_bus_cmdline(void)
> > {
> > int i, err = 0;
> >
> > - for (i = 0; i < ARRAY_SIZE(i2c_rate); i++)
> > - if (i2c_rate[i] & OMAP_I2C_CMDLINE_SETUP) {
> > - i2c_rate[i] &= ~OMAP_I2C_CMDLINE_SETUP;
> > + for (i = 0; i < ARRAY_SIZE(i2c_pdata); i++)
> > + if (i2c_pdata[i].clkrate & OMAP_I2C_CMDLINE_SETUP) {
> > + i2c_pdata[i].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
> > + i2c_pdata[i].set_mpu_wkup_lat =
> > + omap_i2c_set_wfc_mpu_wkup_lat;
> > err = omap_i2c_add_bus(i + 1);
> > if (err)
> > goto out;
> > @@ -231,9 +254,11 @@ int __init omap_register_i2c_bus(int bus_id, u32 clkrate,
> > return err;
> > }
> >
> > - if (!i2c_rate[bus_id - 1])
> > - i2c_rate[bus_id - 1] = clkrate;
> > - i2c_rate[bus_id - 1] &= ~OMAP_I2C_CMDLINE_SETUP;
> > + if (!i2c_pdata[bus_id - 1].clkrate)
> > + i2c_pdata[bus_id - 1].clkrate = clkrate;
> > +
> > + i2c_pdata[bus_id - 1].set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
> > + i2c_pdata[bus_id - 1].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
> >
> > return omap_i2c_add_bus(bus_id);
> > }
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index 75bf3ad..da6d276 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -37,6 +37,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/clk.h>
> > #include <linux/io.h>
> > +#include <linux/i2c-omap.h>
> >
> > /* I2C controller revisions */
> > #define OMAP_I2C_REV_2 0x20
> > @@ -165,6 +166,8 @@ struct omap_i2c_dev {
> > struct clk *fclk; /* Functional clock */
> > struct completion cmd_complete;
> > struct resource *ioarea;
> > + void (*set_mpu_wkup_lat)(struct device *dev,
> > + int set);
> > u32 speed; /* Speed of bus in Khz */
> > u16 cmd_err;
> > u8 *buf;
> > @@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > * REVISIT: We should abort the transfer on signals, but the bus goes
> > * into arbitration and we're currently unable to recover from it.
> > */
> > + dev->set_mpu_wkup_lat(dev->dev, 1);
> > r = wait_for_completion_timeout(&dev->cmd_complete,
> > OMAP_I2C_TIMEOUT);
> > + dev->set_mpu_wkup_lat(dev->dev, 0);
> > dev->buf_len = 0;
> > if (r < 0)
> > return r;
> > @@ -844,6 +849,7 @@ omap_i2c_probe(struct platform_device *pdev)
> > struct omap_i2c_dev *dev;
> > struct i2c_adapter *adap;
> > struct resource *mem, *irq, *ioarea;
> > + struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
> > irq_handler_t isr;
> > int r;
> > u32 speed = 0;
> > @@ -873,10 +879,13 @@ omap_i2c_probe(struct platform_device *pdev)
> > goto err_release_region;
> > }
> >
> > - if (pdev->dev.platform_data != NULL)
> > - speed = *(u32 *)pdev->dev.platform_data;
> > - else
> > - speed = 100; /* Defualt speed */
> > + if (pdata != NULL) {
> > + speed = pdata->clkrate;
> > + dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> > + } else {
> > + speed = 100; /* Default speed */
> > + dev->set_mpu_wkup_lat = NULL;
> > + }
> >
> > dev->speed = speed;
> > dev->idle = 1;
> > diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> > new file mode 100644
> > index 0000000..1362fba
> > --- /dev/null
> > +++ b/include/linux/i2c-omap.h
> > @@ -0,0 +1,9 @@
> > +#ifndef __I2C_OMAP_H__
> > +#define __I2C_OMAP_H__
> > +
> > +struct omap_i2c_bus_platform_data {
> > + u32 clkrate;
> > + void (*set_mpu_wkup_lat)(struct device *dev, int set);
> > +};
> > +
> > +#endif
> > --
> > 1.5.4.3
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-10-01 7:44 ` Kalle Jokiniemi
@ 2009-10-01 11:41 ` Aaro Koskinen
2009-10-02 10:59 ` Kalle Jokiniemi
2009-10-01 14:58 ` Kevin Hilman
1 sibling, 1 reply; 15+ messages in thread
From: Aaro Koskinen @ 2009-10-01 11:41 UTC (permalink / raw)
To: ext Kalle Jokiniemi
Cc: Kevin Hilman, jhnikula@gmail.com, linux-omap@vger.kernel.org
Hello,
Kalle Jokiniemi wrote:
> On Wed, 2009-09-30 at 19:36 +0300, Kevin Hilman wrote:
>> Seems like the latency value should also be (optionally) passed in
>> pdata so this can be experimented with per-platform.
>
> Well, it kind of is already, since we pass the function that sets the
> latency from platform code. And that function has the latency
> hard-coded.
Paul Walmsley suggested earlier that the latency value should be
calculated from bus specific parameters:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg12358.html
A.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-10-01 11:41 ` Aaro Koskinen
@ 2009-10-02 10:59 ` Kalle Jokiniemi
2009-10-07 18:52 ` Woodruff, Richard
0 siblings, 1 reply; 15+ messages in thread
From: Kalle Jokiniemi @ 2009-10-02 10:59 UTC (permalink / raw)
To: Aaro Koskinen
Cc: Kevin Hilman, jhnikula@gmail.com, linux-omap@vger.kernel.org
On Thu, 2009-10-01 at 14:41 +0300, Aaro Koskinen wrote:
> Hello,
>
> Kalle Jokiniemi wrote:
> > On Wed, 2009-09-30 at 19:36 +0300, Kevin Hilman wrote:
> >> Seems like the latency value should also be (optionally) passed in
> >> pdata so this can be experimented with per-platform.
> >
> > Well, it kind of is already, since we pass the function that sets the
> > latency from platform code. And that function has the latency
> > hard-coded.
>
> Paul Walmsley suggested earlier that the latency value should be
> calculated from bus specific parameters:
>
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg12358.html
Yes, this is a good idea in theory, but the reality of wake-up latencies
kind-a kill this one. Wake-up from even C1 (MPU INA, CORE ON) takes
~130us on fastest OPP. And when you add 70us of sleep transition into
that, you get 200us at minimum.
So what it would require to actually get latencies that Paul calculated,
is a new C-state that bypasses the idle loop completely. This would
essentially keep cpu in busy loop while waiting for interrupt (or i2c
completion in this case). In pm-sense, it seems unwise.
For us, the 500us constraint seems to work quite nicely. It removes the
problems we had with i2c transfers timing out with off mode, and
restores average transfer times (from clk_enable to clk_disable) to few
hundred us (that were observed with retention).
But it's open software, people are free to experiment :)
- Kalle
>
> A.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-10-02 10:59 ` Kalle Jokiniemi
@ 2009-10-07 18:52 ` Woodruff, Richard
2009-10-09 4:50 ` Kalle Jokiniemi
0 siblings, 1 reply; 15+ messages in thread
From: Woodruff, Richard @ 2009-10-07 18:52 UTC (permalink / raw)
To: Kalle Jokiniemi, Aaro Koskinen
Cc: Kevin Hilman, jhnikula@gmail.com, linux-omap@vger.kernel.org
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Kalle Jokiniemi
> Sent: Friday, October 02, 2009 5:59 AM
> Yes, this is a good idea in theory, but the reality of wake-up latencies
> kind-a kill this one. Wake-up from even C1 (MPU INA, CORE ON) takes
> ~130us on fastest OPP. And when you add 70us of sleep transition into
> that, you get 200us at minimum.
These values feel a bit on the high side. Have you measured since tweaking your DPLL M/N values?
Are you measuring just across WFI or adding in some part of the idle code path?
> For us, the 500us constraint seems to work quite nicely. It removes the
> problems we had with i2c transfers timing out with off mode, and
> restores average transfer times (from clk_enable to clk_disable) to few
> hundred us (that were observed with retention).
Some of the historic I2C timeout issues were from the wakeup sources not being programmed properly.
Your constraint in my understanding is more about saving power. Today cpuidle doesn't predict interrupt events very well. As such the huge timeout used with i2c will never gate an off mode attempt. You will loose in context save and restore with out constraint.
I floated some idea a while back on pm list to try and do something really simple with irqs. ... take a timestamp on last irq, when cpuilde goes to sleep if current time since last irq is too near then choose safe sleep state. Comments I got at that time was some didn't like extra overhead on irq path. However, I don't know I agree with that.
Regards,
Richard W.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-10-07 18:52 ` Woodruff, Richard
@ 2009-10-09 4:50 ` Kalle Jokiniemi
0 siblings, 0 replies; 15+ messages in thread
From: Kalle Jokiniemi @ 2009-10-09 4:50 UTC (permalink / raw)
To: Woodruff, Richard
Cc: Aaro Koskinen, Kevin Hilman, jhnikula@gmail.com,
linux-omap@vger.kernel.org
On Wed, 2009-10-07 at 21:52 +0300, Woodruff, Richard wrote:
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Kalle Jokiniemi
> > Sent: Friday, October 02, 2009 5:59 AM
>
>
> > Yes, this is a good idea in theory, but the reality of wake-up latencies
> > kind-a kill this one. Wake-up from even C1 (MPU INA, CORE ON) takes
> > ~130us on fastest OPP. And when you add 70us of sleep transition into
> > that, you get 200us at minimum.
>
> These values feel a bit on the high side. Have you measured since tweaking your DPLL M/N values?
>
> Are you measuring just across WFI or adding in some part of the idle code path?
For the numbers I mentioned, the path from start of omap_sram_idle to
end of omap_sram_idle was included. And one can't really not include it,
since it is run with interrupts disabled.
>
> > For us, the 500us constraint seems to work quite nicely. It removes the
> > problems we had with i2c transfers timing out with off mode, and
> > restores average transfer times (from clk_enable to clk_disable) to few
> > hundred us (that were observed with retention).
>
> Some of the historic I2C timeout issues were from the wakeup sources
> not being programmed properly.
There could be such issues, I have not investigated for other reasons
causing the time-outs.
>
> Your constraint in my understanding is more about saving power.
True.
> Today cpuidle doesn't predict interrupt events very well. As such the
> huge timeout used with i2c will never gate an off mode attempt. You
> will loose in context save and restore with out constraint.
True, that is why the constraint is needed on latency basis.
- Kalle
>
> I floated some idea a while back on pm list to try and do something
> really simple with irqs. ... take a timestamp on last irq, when
> cpuilde goes to sleep if current time since last irq is too near then
> choose safe sleep state. Comments I got at that time was some didn't
> like extra overhead on irq path. However, I don't know I agree with
> that.
>
> Regards,
> Richard W.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-10-01 7:44 ` Kalle Jokiniemi
2009-10-01 11:41 ` Aaro Koskinen
@ 2009-10-01 14:58 ` Kevin Hilman
1 sibling, 0 replies; 15+ messages in thread
From: Kevin Hilman @ 2009-10-01 14:58 UTC (permalink / raw)
To: Kalle Jokiniemi; +Cc: jhnikula@gmail.com, linux-omap@vger.kernel.org
Kalle Jokiniemi <kalle.jokiniemi@digia.com> writes:
> On Wed, 2009-09-30 at 19:36 +0300, Kevin Hilman wrote:
>> Kalle Jokiniemi <kalle.jokiniemi@digia.com> writes:
>>
>> > While waiting for completion of the i2c transfer, the
>> > MPU could hit OFF mode and cause several msecs of
>> > delay that made i2c transfers fail more often. The
>> > extra delays and subsequent re-trys cause i2c clocks
>> > to be active more often. This has also an negative
>> > effect on power consumption.
>> >
>> > Added a constraint that allows MPU to wake up in few
>> > hundred usecs, which is roughly the average i2c wait
>> > period.
>> >
>> > The constraint function is passed as platform data from
>> > plat-omap/i2c.c and applied only on OMAP34XX devices.
>>
>> Seems like the latency value should also be (optionally) passed in
>> pdata so this can be experimented with per-platform.
>
> Well, it kind of is already, since we pass the function that sets the
> latency from platform code. And that function has the latency
> hard-coded.
I see it now, I initially thought that hard-coded value was in the
driver, not in plat-omap/i2c.c.
> My though was to use little #ifdeffery in defining the
> function (and latency) for say omap3, and omap4 platforms.
>
> If #ifdeffery is out of the question, we could define
> omap3_i2c_set_mpu.. and omap4_i2c_set_mpu.. functions and pass one in
> init according to some "if (cpu_is_omapxyz)" statement. I think omap1&2
> don't need any latency constraints (and cannot use them as there is no
> pm-layer implementation for those).
>
> What do you think?
I recommend setting the functions at runtime using cpu_is* and
allowing the functino to be NULL on platforms that don't need it.
Combined with the NULL check proposed by Jarkko that should work well.
Kevin
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-09-17 16:28 ` [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c Kalle Jokiniemi
2009-09-30 16:36 ` Kevin Hilman
@ 2009-10-01 6:10 ` Jarkko Nikula
2009-10-01 7:56 ` Kalle Jokiniemi
2009-10-05 17:08 ` Pandita, Vikram
2 siblings, 1 reply; 15+ messages in thread
From: Jarkko Nikula @ 2009-10-01 6:10 UTC (permalink / raw)
To: Kalle Jokiniemi; +Cc: khilman, linux-omap
Hi Kalle
Few minor comments below.
On Thu, 17 Sep 2009 19:28:43 +0300
Kalle Jokiniemi <kalle.jokiniemi@digia.com> wrote:
> -static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> +static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
> static struct platform_device omap_i2c_devices[] = {
> - I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> + I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
> #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> - I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> + I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
> #endif
> #if defined(CONFIG_ARCH_OMAP34XX)
> - I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> + I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
> #endif
> };
This is good idea. IRCC, e.g. the I2C_SCL t_high and t_low timing values
can be board dependent values so passing just the i2c_rate as a
platform_data seems to be not enough in the future.
> @@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> * REVISIT: We should abort the transfer on signals, but the bus goes
> * into arbitration and we're currently unable to recover from it.
> */
> + dev->set_mpu_wkup_lat(dev->dev, 1);
> r = wait_for_completion_timeout(&dev->cmd_complete,
> OMAP_I2C_TIMEOUT);
> + dev->set_mpu_wkup_lat(dev->dev, 0);
These lines here and lines below in omap_i2c_probe looks suspicious. I
would rather allow callback to be NULL (and then you would not need
empty function in arch/arm/plat-omap/i2c.c) and just test it here
before executing. Looks much more safe code then.
> + if (pdata != NULL) {
> + speed = pdata->clkrate;
> + dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> + } else {
> + speed = 100; /* Default speed */
> + dev->set_mpu_wkup_lat = NULL;
> + }
--
Jarkko
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-10-01 6:10 ` Jarkko Nikula
@ 2009-10-01 7:56 ` Kalle Jokiniemi
0 siblings, 0 replies; 15+ messages in thread
From: Kalle Jokiniemi @ 2009-10-01 7:56 UTC (permalink / raw)
To: Jarkko Nikula; +Cc: khilman@deeprootsystems.com, linux-omap@vger.kernel.org
On Thu, 2009-10-01 at 09:10 +0300, Jarkko Nikula wrote:
> Hi Kalle
>
> Few minor comments below.
>
> On Thu, 17 Sep 2009 19:28:43 +0300
> Kalle Jokiniemi <kalle.jokiniemi@digia.com> wrote:
>
> > -static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> > +static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
> > static struct platform_device omap_i2c_devices[] = {
> > - I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> > + I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
> > #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> > - I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> > + I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
> > #endif
> > #if defined(CONFIG_ARCH_OMAP34XX)
> > - I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> > + I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
> > #endif
> > };
> This is good idea. IRCC, e.g. the I2C_SCL t_high and t_low timing values
> can be board dependent values so passing just the i2c_rate as a
> platform_data seems to be not enough in the future.
I gather that this t_high/t_low passing change can be added later ;)
>
> > @@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > * REVISIT: We should abort the transfer on signals, but the bus goes
> > * into arbitration and we're currently unable to recover from it.
> > */
> > + dev->set_mpu_wkup_lat(dev->dev, 1);
> > r = wait_for_completion_timeout(&dev->cmd_complete,
> > OMAP_I2C_TIMEOUT);
> > + dev->set_mpu_wkup_lat(dev->dev, 0);
>
> These lines here and lines below in omap_i2c_probe looks suspicious. I
> would rather allow callback to be NULL (and then you would not need
> empty function in arch/arm/plat-omap/i2c.c) and just test it here
> before executing. Looks much more safe code then.
Yes, this needs to be fixed. Otherwise we could get null pointer calls.
Nice catch.
- Kalle
>
> > + if (pdata != NULL) {
> > + speed = pdata->clkrate;
> > + dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> > + } else {
> > + speed = 100; /* Default speed */
> > + dev->set_mpu_wkup_lat = NULL;
> > + }
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-09-17 16:28 ` [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c Kalle Jokiniemi
2009-09-30 16:36 ` Kevin Hilman
2009-10-01 6:10 ` Jarkko Nikula
@ 2009-10-05 17:08 ` Pandita, Vikram
2009-10-07 10:10 ` Kalle Jokiniemi
2 siblings, 1 reply; 15+ messages in thread
From: Pandita, Vikram @ 2009-10-05 17:08 UTC (permalink / raw)
To: Kalle Jokiniemi, khilman@deeprootsystems.com, Sonasath, Moiz
Cc: jhnikula@gmail.com, linux-omap@vger.kernel.org
Jokiniemi
>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kalle
>Jokiniemi
>Sent: Thursday, September 17, 2009 11:29 AM
>To: khilman@deeprootsystems.com
>Cc: jhnikula@gmail.com; linux-omap@vger.kernel.org; Kalle Jokiniemi
>Subject: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
>
>While waiting for completion of the i2c transfer, the
>MPU could hit OFF mode and cause several msecs of
>delay that made i2c transfers fail more often. The
How many bytes i2c read were you doing, that failed?
What OPP are you in when you observe these failures?
VDD1:OPP?
>extra delays and subsequent re-trys cause i2c clocks
>to be active more often. This has also an negative
>effect on power consumption.
>
>Added a constraint that allows MPU to wake up in few
>hundred usecs, which is roughly the average i2c wait
>period.
How did you arrive at the number 500us for the wakeup latency?
On Zoom2 with Synaptic touch screen controller on I2C-2,
at vdd1:opp1 we need to put a wakeup latency of 400us else we see data corruption.
Our case is doing a write of 1 byte followed by read of 16 bytes.
>
>The constraint function is passed as platform data from
>plat-omap/i2c.c and applied only on OMAP34XX devices.
>
>Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
>---
> arch/arm/plat-omap/i2c.c | 49 +++++++++++++++++++++++++++++++----------
> drivers/i2c/busses/i2c-omap.c | 17 +++++++++++---
> include/linux/i2c-omap.h | 9 +++++++
> 3 files changed, 59 insertions(+), 16 deletions(-)
> create mode 100644 include/linux/i2c-omap.h
>
>diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
>index 8b84839..d43d0ad 100644
>--- a/arch/arm/plat-omap/i2c.c
>+++ b/arch/arm/plat-omap/i2c.c
>@@ -26,8 +26,10 @@
> #include <linux/kernel.h>
> #include <linux/platform_device.h>
> #include <linux/i2c.h>
>+#include <linux/i2c-omap.h>
> #include <mach/irqs.h>
> #include <mach/mux.h>
>+#include <mach/omap-pm.h>
>
> #define OMAP_I2C_SIZE 0x3f
> #define OMAP1_I2C_BASE 0xfffb3800
>@@ -69,14 +71,14 @@ static struct resource i2c_resources[][2] = {
> }, \
> }
>
>-static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
>+static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
> static struct platform_device omap_i2c_devices[] = {
>- I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
>+ I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
> #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
>- I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
>+ I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
> #endif
> #if defined(CONFIG_ARCH_OMAP34XX)
>- I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
>+ I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
> #endif
> };
>
>@@ -100,6 +102,25 @@ static const int omap34xx_pins[][2] = {};
>
> #define OMAP_I2C_CMDLINE_SETUP (BIT(31))
>
>+/**
>+ * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
>+ * @dev: i2c bus device pointer
>+ * @set: set or clear constraints
>+ *
>+ * When waiting for completion of a i2c transfer, we need to set a wake up
>+ * latency constraint for the MPU. This is to ensure quick enough wakeup
>+ * from idle, when transfer completes.
>+ */
>+#ifdef CONFIG_ARCH_OMAP34XX
>+static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
>+{
>+ omap_pm_set_max_mpu_wakeup_lat(dev, set ? 500 : -1);
>+}
>+#else
>+static inline void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
>+{; }
>+#endif
>+
> static void __init omap_i2c_mux_pins(int bus)
> {
> int scl, sda;
>@@ -180,8 +201,8 @@ static int __init omap_i2c_bus_setup(char *str)
> get_options(str, 3, ints);
> if (ints[0] < 2 || ints[1] < 1 || ints[1] > ports)
> return 0;
>- i2c_rate[ints[1] - 1] = ints[2];
>- i2c_rate[ints[1] - 1] |= OMAP_I2C_CMDLINE_SETUP;
>+ i2c_pdata[ints[1] - 1].clkrate = ints[2];
>+ i2c_pdata[ints[1] - 1].clkrate |= OMAP_I2C_CMDLINE_SETUP;
>
> return 1;
> }
>@@ -195,9 +216,11 @@ static int __init omap_register_i2c_bus_cmdline(void)
> {
> int i, err = 0;
>
>- for (i = 0; i < ARRAY_SIZE(i2c_rate); i++)
>- if (i2c_rate[i] & OMAP_I2C_CMDLINE_SETUP) {
>- i2c_rate[i] &= ~OMAP_I2C_CMDLINE_SETUP;
>+ for (i = 0; i < ARRAY_SIZE(i2c_pdata); i++)
>+ if (i2c_pdata[i].clkrate & OMAP_I2C_CMDLINE_SETUP) {
>+ i2c_pdata[i].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
>+ i2c_pdata[i].set_mpu_wkup_lat =
>+ omap_i2c_set_wfc_mpu_wkup_lat;
> err = omap_i2c_add_bus(i + 1);
> if (err)
> goto out;
>@@ -231,9 +254,11 @@ int __init omap_register_i2c_bus(int bus_id, u32 clkrate,
> return err;
> }
>
>- if (!i2c_rate[bus_id - 1])
>- i2c_rate[bus_id - 1] = clkrate;
>- i2c_rate[bus_id - 1] &= ~OMAP_I2C_CMDLINE_SETUP;
>+ if (!i2c_pdata[bus_id - 1].clkrate)
>+ i2c_pdata[bus_id - 1].clkrate = clkrate;
>+
>+ i2c_pdata[bus_id - 1].set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
>+ i2c_pdata[bus_id - 1].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
>
> return omap_i2c_add_bus(bus_id);
> }
>diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>index 75bf3ad..da6d276 100644
>--- a/drivers/i2c/busses/i2c-omap.c
>+++ b/drivers/i2c/busses/i2c-omap.c
>@@ -37,6 +37,7 @@
> #include <linux/platform_device.h>
> #include <linux/clk.h>
> #include <linux/io.h>
>+#include <linux/i2c-omap.h>
>
> /* I2C controller revisions */
> #define OMAP_I2C_REV_2 0x20
>@@ -165,6 +166,8 @@ struct omap_i2c_dev {
> struct clk *fclk; /* Functional clock */
> struct completion cmd_complete;
> struct resource *ioarea;
>+ void (*set_mpu_wkup_lat)(struct device *dev,
>+ int set);
> u32 speed; /* Speed of bus in Khz */
> u16 cmd_err;
> u8 *buf;
>@@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> * REVISIT: We should abort the transfer on signals, but the bus goes
> * into arbitration and we're currently unable to recover from it.
> */
>+ dev->set_mpu_wkup_lat(dev->dev, 1);
> r = wait_for_completion_timeout(&dev->cmd_complete,
> OMAP_I2C_TIMEOUT);
>+ dev->set_mpu_wkup_lat(dev->dev, 0);
> dev->buf_len = 0;
> if (r < 0)
> return r;
>@@ -844,6 +849,7 @@ omap_i2c_probe(struct platform_device *pdev)
> struct omap_i2c_dev *dev;
> struct i2c_adapter *adap;
> struct resource *mem, *irq, *ioarea;
>+ struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
> irq_handler_t isr;
> int r;
> u32 speed = 0;
>@@ -873,10 +879,13 @@ omap_i2c_probe(struct platform_device *pdev)
> goto err_release_region;
> }
>
>- if (pdev->dev.platform_data != NULL)
>- speed = *(u32 *)pdev->dev.platform_data;
>- else
>- speed = 100; /* Defualt speed */
>+ if (pdata != NULL) {
>+ speed = pdata->clkrate;
>+ dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
>+ } else {
>+ speed = 100; /* Default speed */
>+ dev->set_mpu_wkup_lat = NULL;
>+ }
>
> dev->speed = speed;
> dev->idle = 1;
>diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
>new file mode 100644
>index 0000000..1362fba
>--- /dev/null
>+++ b/include/linux/i2c-omap.h
>@@ -0,0 +1,9 @@
>+#ifndef __I2C_OMAP_H__
>+#define __I2C_OMAP_H__
>+
>+struct omap_i2c_bus_platform_data {
>+ u32 clkrate;
>+ void (*set_mpu_wkup_lat)(struct device *dev, int set);
>+};
>+
>+#endif
>--
>1.5.4.3
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-10-05 17:08 ` Pandita, Vikram
@ 2009-10-07 10:10 ` Kalle Jokiniemi
2009-10-07 10:49 ` Nishanth Menon
0 siblings, 1 reply; 15+ messages in thread
From: Kalle Jokiniemi @ 2009-10-07 10:10 UTC (permalink / raw)
To: Pandita, Vikram
Cc: khilman@deeprootsystems.com, Sonasath, Moiz, jhnikula@gmail.com,
linux-omap@vger.kernel.org
Hi,
On Mon, 2009-10-05 at 20:08 +0300, Pandita, Vikram wrote:
> Jokiniemi
>
> >-----Original Message-----
> >From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kalle
> >Jokiniemi
> >Sent: Thursday, September 17, 2009 11:29 AM
> >To: khilman@deeprootsystems.com
> >Cc: jhnikula@gmail.com; linux-omap@vger.kernel.org; Kalle Jokiniemi
> >Subject: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
> >
> >While waiting for completion of the i2c transfer, the
> >MPU could hit OFF mode and cause several msecs of
> >delay that made i2c transfers fail more often. The
>
> How many bytes i2c read were you doing, that failed?
This I don't know. I was monitoring average transfer times from
clk_enable to clk_disable in i2c-omap.c and I noticed the transfers
timed out to 1000ms several times. Hence the conclusion that something
went wrong (this is what I was referring to with "fail more often").
>
> What OPP are you in when you observe these failures?
> VDD1:OPP?
I had on-demand governor in use, but device was mostly idling, so OPP2
probably.
>
> >extra delays and subsequent re-trys cause i2c clocks
> >to be active more often. This has also an negative
> >effect on power consumption.
> >
> >Added a constraint that allows MPU to wake up in few
> >hundred usecs, which is roughly the average i2c wait
> >period.
>
> How did you arrive at the number 500us for the wakeup latency?
This was about the average time that I observed i2c-transfers to be
(from clk_enable to clk_disable), when off mode was not enabled.
>
> On Zoom2 with Synaptic touch screen controller on I2C-2,
> at vdd1:opp1 we need to put a wakeup latency of 400us else we see data corruption.
Hmm, I guess you are using new cpu idle latency numbers on zoom2, since
on current pm-branch code 500us==400us==C3 C-state. But I'll repost with
400us, it might make a difference once those C-state latency numbers are
updated in pm-branch.
- Kalle
>
> Our case is doing a write of 1 byte followed by read of 16 bytes.
>
> >
> >The constraint function is passed as platform data from
> >plat-omap/i2c.c and applied only on OMAP34XX devices.
> >
> >Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
> >---
> > arch/arm/plat-omap/i2c.c | 49 +++++++++++++++++++++++++++++++----------
> > drivers/i2c/busses/i2c-omap.c | 17 +++++++++++---
> > include/linux/i2c-omap.h | 9 +++++++
> > 3 files changed, 59 insertions(+), 16 deletions(-)
> > create mode 100644 include/linux/i2c-omap.h
> >
> >diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> >index 8b84839..d43d0ad 100644
> >--- a/arch/arm/plat-omap/i2c.c
> >+++ b/arch/arm/plat-omap/i2c.c
> >@@ -26,8 +26,10 @@
> > #include <linux/kernel.h>
> > #include <linux/platform_device.h>
> > #include <linux/i2c.h>
> >+#include <linux/i2c-omap.h>
> > #include <mach/irqs.h>
> > #include <mach/mux.h>
> >+#include <mach/omap-pm.h>
> >
> > #define OMAP_I2C_SIZE 0x3f
> > #define OMAP1_I2C_BASE 0xfffb3800
> >@@ -69,14 +71,14 @@ static struct resource i2c_resources[][2] = {
> > }, \
> > }
> >
> >-static u32 i2c_rate[ARRAY_SIZE(i2c_resources)];
> >+static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)];
> > static struct platform_device omap_i2c_devices[] = {
> >- I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]),
> >+ I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
> > #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX)
> >- I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]),
> >+ I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]),
> > #endif
> > #if defined(CONFIG_ARCH_OMAP34XX)
> >- I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]),
> >+ I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]),
> > #endif
> > };
> >
> >@@ -100,6 +102,25 @@ static const int omap34xx_pins[][2] = {};
> >
> > #define OMAP_I2C_CMDLINE_SETUP (BIT(31))
> >
> >+/**
> >+ * omap_i2c_set_wfc_mpu_wkup_lat - sets mpu wake up constraint
> >+ * @dev: i2c bus device pointer
> >+ * @set: set or clear constraints
> >+ *
> >+ * When waiting for completion of a i2c transfer, we need to set a wake up
> >+ * latency constraint for the MPU. This is to ensure quick enough wakeup
> >+ * from idle, when transfer completes.
> >+ */
> >+#ifdef CONFIG_ARCH_OMAP34XX
> >+static void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> >+{
> >+ omap_pm_set_max_mpu_wakeup_lat(dev, set ? 500 : -1);
> >+}
> >+#else
> >+static inline void omap_i2c_set_wfc_mpu_wkup_lat(struct device *dev, int set)
> >+{; }
> >+#endif
> >+
> > static void __init omap_i2c_mux_pins(int bus)
> > {
> > int scl, sda;
> >@@ -180,8 +201,8 @@ static int __init omap_i2c_bus_setup(char *str)
> > get_options(str, 3, ints);
> > if (ints[0] < 2 || ints[1] < 1 || ints[1] > ports)
> > return 0;
> >- i2c_rate[ints[1] - 1] = ints[2];
> >- i2c_rate[ints[1] - 1] |= OMAP_I2C_CMDLINE_SETUP;
> >+ i2c_pdata[ints[1] - 1].clkrate = ints[2];
> >+ i2c_pdata[ints[1] - 1].clkrate |= OMAP_I2C_CMDLINE_SETUP;
> >
> > return 1;
> > }
> >@@ -195,9 +216,11 @@ static int __init omap_register_i2c_bus_cmdline(void)
> > {
> > int i, err = 0;
> >
> >- for (i = 0; i < ARRAY_SIZE(i2c_rate); i++)
> >- if (i2c_rate[i] & OMAP_I2C_CMDLINE_SETUP) {
> >- i2c_rate[i] &= ~OMAP_I2C_CMDLINE_SETUP;
> >+ for (i = 0; i < ARRAY_SIZE(i2c_pdata); i++)
> >+ if (i2c_pdata[i].clkrate & OMAP_I2C_CMDLINE_SETUP) {
> >+ i2c_pdata[i].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
> >+ i2c_pdata[i].set_mpu_wkup_lat =
> >+ omap_i2c_set_wfc_mpu_wkup_lat;
> > err = omap_i2c_add_bus(i + 1);
> > if (err)
> > goto out;
> >@@ -231,9 +254,11 @@ int __init omap_register_i2c_bus(int bus_id, u32 clkrate,
> > return err;
> > }
> >
> >- if (!i2c_rate[bus_id - 1])
> >- i2c_rate[bus_id - 1] = clkrate;
> >- i2c_rate[bus_id - 1] &= ~OMAP_I2C_CMDLINE_SETUP;
> >+ if (!i2c_pdata[bus_id - 1].clkrate)
> >+ i2c_pdata[bus_id - 1].clkrate = clkrate;
> >+
> >+ i2c_pdata[bus_id - 1].set_mpu_wkup_lat = omap_i2c_set_wfc_mpu_wkup_lat;
> >+ i2c_pdata[bus_id - 1].clkrate &= ~OMAP_I2C_CMDLINE_SETUP;
> >
> > return omap_i2c_add_bus(bus_id);
> > }
> >diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> >index 75bf3ad..da6d276 100644
> >--- a/drivers/i2c/busses/i2c-omap.c
> >+++ b/drivers/i2c/busses/i2c-omap.c
> >@@ -37,6 +37,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/clk.h>
> > #include <linux/io.h>
> >+#include <linux/i2c-omap.h>
> >
> > /* I2C controller revisions */
> > #define OMAP_I2C_REV_2 0x20
> >@@ -165,6 +166,8 @@ struct omap_i2c_dev {
> > struct clk *fclk; /* Functional clock */
> > struct completion cmd_complete;
> > struct resource *ioarea;
> >+ void (*set_mpu_wkup_lat)(struct device *dev,
> >+ int set);
> > u32 speed; /* Speed of bus in Khz */
> > u16 cmd_err;
> > u8 *buf;
> >@@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> > * REVISIT: We should abort the transfer on signals, but the bus goes
> > * into arbitration and we're currently unable to recover from it.
> > */
> >+ dev->set_mpu_wkup_lat(dev->dev, 1);
> > r = wait_for_completion_timeout(&dev->cmd_complete,
> > OMAP_I2C_TIMEOUT);
> >+ dev->set_mpu_wkup_lat(dev->dev, 0);
> > dev->buf_len = 0;
> > if (r < 0)
> > return r;
> >@@ -844,6 +849,7 @@ omap_i2c_probe(struct platform_device *pdev)
> > struct omap_i2c_dev *dev;
> > struct i2c_adapter *adap;
> > struct resource *mem, *irq, *ioarea;
> >+ struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
> > irq_handler_t isr;
> > int r;
> > u32 speed = 0;
> >@@ -873,10 +879,13 @@ omap_i2c_probe(struct platform_device *pdev)
> > goto err_release_region;
> > }
> >
> >- if (pdev->dev.platform_data != NULL)
> >- speed = *(u32 *)pdev->dev.platform_data;
> >- else
> >- speed = 100; /* Defualt speed */
> >+ if (pdata != NULL) {
> >+ speed = pdata->clkrate;
> >+ dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> >+ } else {
> >+ speed = 100; /* Default speed */
> >+ dev->set_mpu_wkup_lat = NULL;
> >+ }
> >
> > dev->speed = speed;
> > dev->idle = 1;
> >diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> >new file mode 100644
> >index 0000000..1362fba
> >--- /dev/null
> >+++ b/include/linux/i2c-omap.h
> >@@ -0,0 +1,9 @@
> >+#ifndef __I2C_OMAP_H__
> >+#define __I2C_OMAP_H__
> >+
> >+struct omap_i2c_bus_platform_data {
> >+ u32 clkrate;
> >+ void (*set_mpu_wkup_lat)(struct device *dev, int set);
> >+};
> >+
> >+#endif
> >--
> >1.5.4.3
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
2009-10-07 10:10 ` Kalle Jokiniemi
@ 2009-10-07 10:49 ` Nishanth Menon
[not found] ` <1254927643.22468.315.camel@ubuntu>
0 siblings, 1 reply; 15+ messages in thread
From: Nishanth Menon @ 2009-10-07 10:49 UTC (permalink / raw)
To: Kalle Jokiniemi
Cc: Pandita, Vikram, khilman@deeprootsystems.com, Sonasath, Moiz,
jhnikula@gmail.com, linux-omap@vger.kernel.org
Kalle Jokiniemi said the following on 10/07/2009 05:10 AM:
>>> extra delays and subsequent re-trys cause i2c clocks
>>> to be active more often. This has also an negative
>>> effect on power consumption.
>>>
>>> Added a constraint that allows MPU to wake up in few
>>> hundred usecs, which is roughly the average i2c wait
>>> period.
>>>
>> How did you arrive at the number 500us for the wakeup latency?
>>
>
> This was about the average time that I observed i2c-transfers to be
> (from clk_enable to clk_disable), when off mode was not enabled.
>
just a quick 2cents: I understand that the 500uSec might need to be
board dependent - here is why:
one theory we have regarding the timeout delay having to be modified on
zoom2 was that if the delay was 500uSec, the touch screen controller
timesout and pulls of the data in the middle of transfer.
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 15+ messages in thread