i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle

Commit Message

Kevin Hilman Oct. 12, 2012, 6:40 p.m. UTC

From: Kevin Hilman <[e-mail]>

Currently, runtime PM is used to keep the device enabled only during
active transfers and for a configurable runtime PM autosuspend timout
after an xfer.

In addition to idling the device, driver's ->runtime_suspend() method
currently disables device interrupts when idle.  However, on some SoCs
(notably OMAP4+), the I2C hardware may shared with other coprocessors.
This means that the MPU will still recieve interrupts if a coprocessor
is using the I2C device.  To avoid this, also disable interrupts at
the MPU INTC when idling the device in ->runtime_suspend() (and
re-enable them in ->runtime_resume().)  This part based on an original
patch from Shubhrajyoti Datta.  NOTE: for proper sharing the I2C with
a coprocessor, this driver still needs hwspinlock support added.

This change is also meant to address an issue reported by Kalle
Jokiniemi where I2C bus interrupt may be enabled before an I2C device
interrupt handler (e.g. just after noirq resume phase) causing an
interrupt flood on the I2C bus interrupt before the device interrupt
is enabled (e.g. interrupts coming from devices on I2C connected PMIC
before the PMIC chained hanlder is enabled.)  This problem is addresed
by ensuring that the I2C bus interrupt left disabled until an I2C xfer
is requested.

Cc: Kalle Jokiniemi <[e-mail]>
Cc: Grygorii Strashko <[e-mail]>
Cc: Shubhrajyoti Datta <[e-mail]>,
Cc: Huzefa Kankroliwala <[e-mail]>
Cc: Nishanth Menon <[e-mail]>
Signed-off-by: Kevin Hilman <[e-mail]>
---
 drivers/i2c/busses/i2c-omap.c |    3 +++
 1 file changed, 3 insertions(+)

Comments

Shubhrajyoti Datta Oct. 12, 2012, 7:30 p.m. UTC | #1

On Sat, Oct 13, 2012 at 12:10 AM, Kevin Hilman
<[e-mail]> wrote:
> From: Kevin Hilman <[e-mail]>
>
> Currently, runtime PM is used to keep the device enabled only during
> active transfers and for a configurable runtime PM autosuspend timout
> after an xfer.
>
> In addition to idling the device, driver's ->runtime_suspend() method
> currently disables device interrupts when idle.  However, on some SoCs
> (notably OMAP4+), the I2C hardware may shared with other coprocessors.
> This means that the MPU will still recieve interrupts if a coprocessor
> is using the I2C device.  To avoid this, also disable interrupts at
> the MPU INTC when idling the device in ->runtime_suspend() (and
> re-enable them in ->runtime_resume().)  This part based on an original
> patch from Shubhrajyoti Datta.  NOTE: for proper sharing the I2C with
> a coprocessor, this driver still needs hwspinlock support added.
>
> This change is also meant to address an issue reported by Kalle
> Jokiniemi where I2C bus interrupt may be enabled before an I2C device
> interrupt handler (e.g. just after noirq resume phase) causing an
> interrupt flood on the I2C bus interrupt before the device interrupt
> is enabled (e.g. interrupts coming from devices on I2C connected PMIC
> before the PMIC chained hanlder is enabled.)  This problem is addresed
> by ensuring that the I2C bus interrupt left disabled until an I2C xfer
> is requested.

Looks good to me.
Will wait for Kalle though.

>
> Cc: Kalle Jokiniemi <[e-mail]>
> Cc: Grygorii Strashko <[e-mail]>
> Cc: Shubhrajyoti Datta <[e-mail]>,
> Cc: Huzefa Kankroliwala <[e-mail]>
> Cc: Nishanth Menon <[e-mail]>
> Signed-off-by: Kevin Hilman <[e-mail]>
> ---
>  drivers/i2c/busses/i2c-omap.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index db31eae..e6413e8 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1255,6 +1255,7 @@ static int omap_i2c_runtime_suspend(struct device *dev)
>                 /* Flush posted write */
>                 omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
>         }
> +       disable_irq(_dev->irq);
>
>         return 0;
>  }
> @@ -1275,6 +1276,8 @@ static int omap_i2c_runtime_resume(struct device *dev)
>                 omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>         }
>
> +       enable_irq(_dev->irq);
> +
>         /*
>          * Don't write to this register if the IE state is 0 as it can
>          * cause deadlock.
> --
> 1.7.9.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kalle Jokiniemi Oct. 15, 2012, 6:25 a.m. UTC | #2

Hi,

la, 2012-10-13 kello 01:00 +0530, Shubhrajyoti Datta kirjoitti:
> On Sat, Oct 13, 2012 at 12:10 AM, Kevin Hilman
> <[e-mail]> wrote:
> > From: Kevin Hilman <[e-mail]>
> >
> > Currently, runtime PM is used to keep the device enabled only during
> > active transfers and for a configurable runtime PM autosuspend timout
> > after an xfer.
> >
> > In addition to idling the device, driver's ->runtime_suspend() method
> > currently disables device interrupts when idle.  However, on some SoCs
> > (notably OMAP4+), the I2C hardware may shared with other coprocessors.
> > This means that the MPU will still recieve interrupts if a coprocessor
> > is using the I2C device.  To avoid this, also disable interrupts at
> > the MPU INTC when idling the device in ->runtime_suspend() (and
> > re-enable them in ->runtime_resume().)  This part based on an original
> > patch from Shubhrajyoti Datta.  NOTE: for proper sharing the I2C with
> > a coprocessor, this driver still needs hwspinlock support added.
> >
> > This change is also meant to address an issue reported by Kalle
> > Jokiniemi where I2C bus interrupt may be enabled before an I2C device
> > interrupt handler (e.g. just after noirq resume phase) causing an

It is actually in middle of resume_noirq.

> > interrupt flood on the I2C bus interrupt before the device interrupt
> > is enabled (e.g. interrupts coming from devices on I2C connected PMIC
> > before the PMIC chained hanlder is enabled.)  This problem is addresed
> > by ensuring that the I2C bus interrupt left disabled until an I2C xfer
> > is requested.
> 
> Looks good to me.
> Will wait for Kalle though.

Does not work for me :(

As I said, the issue occurs for me when I enter static suspend (echo mem
> /sys/power/autosleep or /sys/power/state). I don't think doing this
just in runtime pm will fix my issue. Or do those handlers get run in
the normal suspend path as well?

- Kalle

> 
> >
> > Cc: Kalle Jokiniemi <[e-mail]>
> > Cc: Grygorii Strashko <[e-mail]>
> > Cc: Shubhrajyoti Datta <[e-mail]>,
> > Cc: Huzefa Kankroliwala <[e-mail]>
> > Cc: Nishanth Menon <[e-mail]>
> > Signed-off-by: Kevin Hilman <[e-mail]>
> > ---
> >  drivers/i2c/busses/i2c-omap.c |    3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index db31eae..e6413e8 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -1255,6 +1255,7 @@ static int omap_i2c_runtime_suspend(struct device *dev)
> >                 /* Flush posted write */
> >                 omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
> >         }
> > +       disable_irq(_dev->irq);
> >
> >         return 0;
> >  }
> > @@ -1275,6 +1276,8 @@ static int omap_i2c_runtime_resume(struct device *dev)
> >                 omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
> >         }
> >
> > +       enable_irq(_dev->irq);
> > +
> >         /*
> >          * Don't write to this register if the IE state is 0 as it can
> >          * cause deadlock.
> > --
> > 1.7.9.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> > the body of a message to [e-mail]
> > 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman Oct. 15, 2012, 5:31 p.m. UTC | #3

Kalle Jokiniemi <[e-mail]> writes:

> Hi,
>
> la, 2012-10-13 kello 01:00 +0530, Shubhrajyoti Datta kirjoitti:
>> On Sat, Oct 13, 2012 at 12:10 AM, Kevin Hilman
>> <[e-mail]> wrote:
>> > From: Kevin Hilman <[e-mail]>
>> >
>> > Currently, runtime PM is used to keep the device enabled only during
>> > active transfers and for a configurable runtime PM autosuspend timout
>> > after an xfer.
>> >
>> > In addition to idling the device, driver's ->runtime_suspend() method
>> > currently disables device interrupts when idle.  However, on some SoCs
>> > (notably OMAP4+), the I2C hardware may shared with other coprocessors.
>> > This means that the MPU will still recieve interrupts if a coprocessor
>> > is using the I2C device.  To avoid this, also disable interrupts at
>> > the MPU INTC when idling the device in ->runtime_suspend() (and
>> > re-enable them in ->runtime_resume().)  This part based on an original
>> > patch from Shubhrajyoti Datta.  NOTE: for proper sharing the I2C with
>> > a coprocessor, this driver still needs hwspinlock support added.
>> >
>> > This change is also meant to address an issue reported by Kalle
>> > Jokiniemi where I2C bus interrupt may be enabled before an I2C device
>> > interrupt handler (e.g. just after noirq resume phase) causing an
>
> It is actually in middle of resume_noirq.
>
>> > interrupt flood on the I2C bus interrupt before the device interrupt
>> > is enabled (e.g. interrupts coming from devices on I2C connected PMIC
>> > before the PMIC chained hanlder is enabled.)  This problem is addresed
>> > by ensuring that the I2C bus interrupt left disabled until an I2C xfer
>> > is requested.
>> 
>> Looks good to me.
>> Will wait for Kalle though.
>
> Does not work for me :(
>
> As I said, the issue occurs for me when I enter static suspend (echo mem
>> /sys/power/autosleep or /sys/power/state). I don't think doing this
> just in runtime pm will fix my issue. Or do those handlers get run in
> the normal suspend path as well?

If the I2C device is still active during the suspend path, these
handlers will get run by the PM domain code (in omap_device.)  However,
now that I think about it, the current omap_device PM domain code calls
these at the noirq level, not the late/early level, so it does not
address your original problem. :(

I suspect we'll need this and your original patch.


Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren Oct. 16, 2012, 1:02 a.m. UTC | #4

* Kevin Hilman <[e-mail]> [121015 10:32]:
> Kalle Jokiniemi <[e-mail]> writes:
> >
> > Does not work for me :(
> >
> > As I said, the issue occurs for me when I enter static suspend (echo mem
> >> /sys/power/autosleep or /sys/power/state). I don't think doing this
> > just in runtime pm will fix my issue. Or do those handlers get run in
> > the normal suspend path as well?
> 
> If the I2C device is still active during the suspend path, these
> handlers will get run by the PM domain code (in omap_device.)  However,
> now that I think about it, the current omap_device PM domain code calls
> these at the noirq level, not the late/early level, so it does not
> address your original problem. :(
> 
> I suspect we'll need this and your original patch.

Have you checked that this is not related to flushing the posted
write? If it is, reading back any register from the i2c controller
after writing to it ensures the write actually reaches the i2c
controller.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kalle Jokiniemi Oct. 16, 2012, 8:47 a.m. UTC | #5

Hi,

ma, 2012-10-15 kello 18:02 -0700, Tony Lindgren kirjoitti:
> * Kevin Hilman <[e-mail]> [121015 10:32]:
> > Kalle Jokiniemi <[e-mail]> writes:
> > >
> > > Does not work for me :(
> > >
> > > As I said, the issue occurs for me when I enter static suspend (echo mem
> > >> /sys/power/autosleep or /sys/power/state). I don't think doing this
> > > just in runtime pm will fix my issue. Or do those handlers get run in
> > > the normal suspend path as well?
> > 
> > If the I2C device is still active during the suspend path, these
> > handlers will get run by the PM domain code (in omap_device.)  However,
> > now that I think about it, the current omap_device PM domain code calls
> > these at the noirq level, not the late/early level, so it does not
> > address your original problem. :(
> > 
> > I suspect we'll need this and your original patch.
> 
> Have you checked that this is not related to flushing the posted
> write? If it is, reading back any register from the i2c controller
> after writing to it ensures the write actually reaches the i2c
> controller.

The twl4030-irq handler (handle_twl4030_pih) function just reads the PIH
irq status over the i2c and calls handle_nested_irq for each set module
(like USB, GPIO, etc) irq bit. This does not clear the interrupt
however, that is done in the nested interrupt, once it runs (by clearing
the actual interrupt inside the module).

I'm thinking now that it's actually this PIH handler that should be
postponed until resume_noirq has finished. I had another idea as well
yesterday to mark the secondary irq handlers with IRQF_EARLY_RESUME
flag. Though that did not work on the quick test I did. Anyway new patch
coming soon :)

- Kalle

> 
> Regards,
> 
> Tony


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wolfram Sang Nov. 1, 2012, 10:49 p.m. UTC | #6

Hi,

>  Anyway new patch coming soon :)

Was there one? I have skimmed a number of threads discussing spurious
interrupts or interrupt floods but AFAICS all discussions ended up in
trying another approach later or fixing the issue somewhere else than
I2C. Is this correct? Or is there a bugfix patch left for 3.7 that I
missed?

Thanks,

   Wolfram

Kalle Jokiniemi Nov. 2, 2012, 6:17 a.m. UTC | #7

to, 2012-11-01 kello 23:49 +0100, Wolfram Sang kirjoitti:
> Hi,
> 
> >  Anyway new patch coming soon :)
> 
> Was there one? I have skimmed a number of threads discussing spurious
> interrupts or interrupt floods but AFAICS all discussions ended up in
> trying another approach later or fixing the issue somewhere else than
> I2C. Is this correct? Or is there a bugfix patch left for 3.7 that I
> missed?

The problem was not actually in the i2c driver itself, the twl4030
Primary/Secondary interrupt handler irq wake up order was the problem.
The fix was this:
https://patchwork.kernel.org/patch/1601271/

Actually, Samuel, did you pick up my patch? I never got any response
after Kevin acked it.

- Kalle

> 
> Thanks,
> 
>    Wolfram
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..e6413e8 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1255,6 +1255,7 @@  static int omap_i2c_runtime_suspend(struct device *dev)
 		/* Flush posted write */
 		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
 	}
+	disable_irq(_dev->irq);
 
 	return 0;
 }
@@ -1275,6 +1276,8 @@  static int omap_i2c_runtime_resume(struct device *dev)
 		omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 	}
 
+	enable_irq(_dev->irq);
+
 	/*
 	 * Don't write to this register if the IE state is 0 as it can
 	 * cause deadlock.