Re: [PATCH] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle
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-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
ke, 2012-10-17 kello 19:02 +0300, Felipe Balbi kirjoitti: Hi, On Thu, Oct 11, 2012 at 02:08:25PM -0700, Kevin Hilman wrote: Hi Kalle, Kalle Jokiniemi kalle.jokini...@jollamobile.com writes: The resume_noirq enables interrupts one-by-one starting from first one. Now if the wake up event for suspend came from i2c device, the i2c bus irq gets enabled before the threaded i2c device irq, causing a flood of i2c bus interrupts as the threaded irq that should clear the event is not enabled yet. Fixed the issue by adding suspend_noirq and resume_early functions that keep i2c bus interrupts disabled until resume_noirq has run completely. Issue was detected doing a wake up from autosleep with twl4030 power key on N9. Patch tested on N9. Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com This version looks good, thanks for the extra comments. Reviewed-by: Kevin Hilman khil...@ti.com Tested-by: Kevin Hilman khil...@ti.com Wolfram, This should also probably be Cc'd to stable since it affects earlier kernels as well. Thanks, just to make sure we're not fixing the wrong problem... does [1] help in any way ? Yes, I was fixing the wrong problem, this patch is obsolete. But the problem was in the TWL interrupt handling (PIH was run before SIH), not in i2c. See my other patch twl4030: Fix chained irq handling on resume from suspend [1] http://marc.info/?l=linux-omapm=135048839915719w=2 Could be related, though if I understood correctly, that runtime pm stuff gets run at noirq phase, so it probably does not help. - Kalle -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle
Hi, ma, 2012-10-15 kello 18:02 -0700, Tony Lindgren kirjoitti: * Kevin Hilman khil...@deeprootsystems.com [121015 10:32]: Kalle Jokiniemi kalle.jokini...@jollamobile.com 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-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] twl4030: Fix chained irq handling on resume from suspend
The irqs are enabled one-by-one in pm core resume_noirq phase. This leads to situation where the twl4030 primary interrupt handler (PIH) is enabled before the chained secondary handlers (SIH). As the PIH cannot clear the pending interrupt, and SIHs have not been enabled yet, a flood of interrupts hangs the device. Fixed the issue by setting the SIH irqs with IRQF_EARLY_RESUME flags, so they get enabled before the PIH. Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com --- drivers/mfd/twl4030-irq.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c index ad733d7..cdd1173 100644 --- a/drivers/mfd/twl4030-irq.c +++ b/drivers/mfd/twl4030-irq.c @@ -672,7 +672,8 @@ int twl4030_sih_setup(struct device *dev, int module, int irq_base) irq = sih_mod + twl4030_irq_base; irq_set_handler_data(irq, agent); agent-irq_name = kasprintf(GFP_KERNEL, twl4030_%s, sih-name); - status = request_threaded_irq(irq, NULL, handle_twl4030_sih, 0, + status = request_threaded_irq(irq, NULL, handle_twl4030_sih, + IRQF_EARLY_RESUME, agent-irq_name ?: sih-name, NULL); dev_info(dev, %s (irq %d) chaining IRQs %d..%d\n, sih-name, -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] twl4030: Fix chained irq handling on resume from suspend
Hi, ti, 2012-10-16 kello 17:59 +0300, Kalle Jokiniemi kirjoitti: The irqs are enabled one-by-one in pm core resume_noirq phase. This leads to situation where the twl4030 primary interrupt handler (PIH) is enabled before the chained secondary handlers (SIH). As the PIH cannot clear the pending interrupt, and SIHs have not been enabled yet, a flood of interrupts hangs the device. Fixed the issue by setting the SIH irqs with IRQF_EARLY_RESUME flags, so they get enabled before the PIH. Did it this way now, since the tl4030_irq is not really a driver that could do normal suspend / resume calls... and this is what the flag is for. Added Samuel to recipients, as get_maintainer reported him as maintainer. - Kalle Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com --- drivers/mfd/twl4030-irq.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c index ad733d7..cdd1173 100644 --- a/drivers/mfd/twl4030-irq.c +++ b/drivers/mfd/twl4030-irq.c @@ -672,7 +672,8 @@ int twl4030_sih_setup(struct device *dev, int module, int irq_base) irq = sih_mod + twl4030_irq_base; irq_set_handler_data(irq, agent); agent-irq_name = kasprintf(GFP_KERNEL, twl4030_%s, sih-name); - status = request_threaded_irq(irq, NULL, handle_twl4030_sih, 0, + status = request_threaded_irq(irq, NULL, handle_twl4030_sih, + IRQF_EARLY_RESUME, agent-irq_name ?: sih-name, NULL); dev_info(dev, %s (irq %d) chaining IRQs %d..%d\n, sih-name, -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Hi,
pe, 2012-10-12 kello 14:46 +, Strashko, Grygorii kirjoitti:
Hi Kevin,
yep, [1] is the same fix - thanks.
Hi Kalle,
i've applied these changes and PM runtime fix on top of
git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git
(omap2plus_defconfig)
Could you check it with your use case, pls? (just to be sure that idea is
right)
Odd, it's not working. I'll add some debug prints to see what happens
there.
- Kalle
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a0e49f6..cb09e20 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -586,6 +586,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
if (IS_ERR_VALUE(r))
goto out;
+ /* We have the bus, enable IRQ */
+ enable_irq(dev-irq);
+
r = omap_i2c_wait_for_bb(dev);
if (r 0)
goto out;
@@ -606,6 +609,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
r = num;
omap_i2c_wait_for_bb(dev);
+ disable_irq(dev-irq);
out:
pm_runtime_put(dev-dev);
return r;
@@ -1060,6 +1064,9 @@ omap_i2c_probe(struct platform_device *pdev)
omap_i2c_isr;
r = request_irq(dev-irq, isr, IRQF_NO_SUSPEND, pdev-name, dev);
+ /* We enable IRQ only when request for I2C from master */
+disable_irq(dev-irq);
+
if (r) {
dev_err(dev-dev, failure requesting irq %i\n, dev-irq);
goto err_unuse_clocks;
@@ -1182,7 +1189,23 @@ static int omap_i2c_runtime_resume(struct device *dev)
}
#endif /* CONFIG_PM_RUNTIME */
+static int omap_i2c_suspend(struct device *dev)
+{
+ int ret;
+
+ /*
+* Enable I2C device, so it will be accessible during
+* later stages of suspending when device Runtime PM is disabled.
+* I2C device will be turned off at noirq suspend stage.
+*/
+ ret = pm_runtime_resume(dev);
+ if (ret 0)
+ return ret;
+ return 0;
+}
+
static struct dev_pm_ops omap_i2c_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, NULL)
SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
omap_i2c_runtime_resume, NULL)
};
- Grygorii
From: Kevin Hilman [khil...@deeprootsystems.com]
Sent: Friday, October 12, 2012 5:34 PM
To: Strashko, Grygorii
Cc: Kalle Jokiniemi; linux-i2c@vger.kernel.org; w.s...@pengutronix.de;
ben-li...@fluff.org; t...@atomide.com; linux-o...@vger.kernel.org; Datta,
Shubhrajyoti; Kankroliwala, Huzefa
Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Strashko, Grygorii grygorii.stras...@ti.com writes:
Hi All,
Sorry, for the late reply.
+ CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4.
Hi Grygorii, thanks for reviewing. I was hoping you would have some
ideas here as this was sounding familiar to something you had
mentioned elsewhere.
Regarding this patch - from my point of view, it fixes corner case and not
an issue in general.
Let take a look on resume sequence:
- platform resume
- syscore resume
- resume_noirq
- enable IRQs - resume_device_irqs()
|- at this point IRQ handler will be invoked if IRQ state is
IRQS_PENDING.
|- so, the I2C device IRQ handler may be called at time when I2C
adapter IRQ is still disabled and, as result, the I2C device IRQ-handler
may fail. (I2C device and I2C adapter may use different physical IRQ lines)
- resume_late
|- enable I2C bus IRQ
Possibly, the better way is to enable/disable I2C bus IRQ when needed - in
our case in omap_i2c_xfer().
We use such approach in Android kernel 3.4
(http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e)
I agree, that should work and cover the cases where I2C is used by other
processors also. Shubhrajyoti already posted something similar[1] but
it needed some rework (comments from Russell and myself.)
Huzefa, Shubhrajyoti, who can rework this idea for the upstream and/or
follow up with the earlier patch[1]?
Wolfram, I guess for now lets hold off on $SUBJECT patch. Seems we can
come up with a broader solution. Thanks.
Kevin
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-October/124427.html
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle
Hi, la, 2012-10-13 kello 01:00 +0530, Shubhrajyoti Datta kirjoitti: On Sat, Oct 13, 2012 at 12:10 AM, Kevin Hilman khil...@deeprootsystems.com wrote: From: Kevin Hilman khil...@ti.com 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 kalle.jokini...@jollamobile.com Cc: Grygorii Strashko grygorii.stras...@ti.com Cc: Shubhrajyoti Datta shubhrajy...@ti.com, Cc: Huzefa Kankroliwala huzef...@ti.com Cc: Nishanth Menon n...@ti.com Signed-off-by: Kevin Hilman khil...@ti.com --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
ma, 2012-10-15 kello 15:41 +0530, Shubhrajyoti Datta kirjoitti: On Mon, Oct 15, 2012 at 2:46 PM, Kalle Jokiniemi kalle.jokini...@jollamobile.com wrote: ma, 2012-10-15 kello 09:21 +0300, Kalle Jokiniemi kirjoitti: Hi, pe, 2012-10-12 kello 14:46 +, Strashko, Grygorii kirjoitti: Hi Kevin, yep, [1] is the same fix - thanks. Hi Kalle, i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) Could you check it with your use case, pls? (just to be sure that idea is right) Odd, it's not working. I'll add some debug prints to see what happens there. Well, seems after enabling irq 23 in the resume_noirq, someone does i2c_xfer and there is consequent flood of i2c_xfers and interrupts. If there is continuous xfers, you could enable debug LL and see who is queuing the transfers. Not sure now how these IRQ numbers get mapped these days, my debug print says it's irq number 72 (UART1 from TRM) that is flooding (although it's printed from the i2c-omap isr function, so it's still I2C bus irq...). Can you do a cat /proc/interrupts Yes :) [root@localhost proc]# cat interrupts CPU0 20: 0 INTC gpmc 23: 2 INTC TWL4030-PIH 25: 0 INTC l3-debug-irq 26: 0 INTC l3-app-irq 28: 48157 INTC DMA 40: 0 INTC omap-iommu.0 52: 0 INTC dsp_wdt 53: 807807 INTC gp_timer 65: 0 INTC omap-sham 72: 5490 INTC omap_i2c 73: 85 INTC omap_i2c 77: 0 INTC omap_i2c 90: 1069 INTC OMAP UART2 102: 55940 INTC mmc0 179: 6142 GPIO omap2-onenand 306: 44666 PRCM pm_wkup 315: 4 PRCM hwmod_io, pm_io 338: 0 twl4030 twl4030_gpio 343: 2 twl4030 twl4030_power 346: 0 twl4030 twl4030_pwrbutton 348: 2 twl4030 twl4030_usb 349: 0 twl4030 rtc0 Hmm, I did not notice that PIH handler before, makes sense now that it triggers the flood (irq 23) as it is really the one that passes the interrupts to other handlers. - Kalle The irq enabling (in resume_noirq) is still slightly progressing after the flooding starts, but my watchdog kicks in before we get to the finish. Attached my debug prints patch and log. I used also no_console_suspend boot option. - Kalle -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Hi,
pe, 2012-10-12 kello 10:18 +, Strashko, Grygorii kirjoitti:
Hi All,
Sorry, for the late reply.
+ CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4.
Regarding this patch - from my point of view, it fixes corner case and not
an issue in general.
Let take a look on resume sequence:
- platform resume
- syscore resume
- resume_noirq
- enable IRQs - resume_device_irqs()
|- at this point IRQ handler will be invoked if IRQ state is
IRQS_PENDING.
|- so, the I2C device IRQ handler may be called at time when I2C
adapter IRQ is still disabled and, as result, the I2C device
IRQ-handler may fail. (I2C device and I2C adapter may use different
physical IRQ lines)
The use case is to wake up the the device from suspend via twl4030 power
key line. The twl4030 then will interrupt host via the i2c bus. And on
the host the i2c bus is then read by the twl4030-pwrkey threaded SW
interrupt (which is disabled at the point when i2c bus HW interrupt
occurs).
- resume_late
|- enable I2C bus IRQ
Possibly, the better way is to enable/disable I2C bus IRQ when needed - in
our case in omap_i2c_xfer().
We use such approach in Android kernel 3.4
(http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e)
Sounds like it could work... however I tested the patch above, but my
device now dies when waking it from suspend (autosleep, actually) with
power key :(
Have you guys considered reworking that patch for upstream? There seems
to be some spinlocking there that was not in linux-omap that I tested
on.
- Kalle
Grygorii
From: Kevin Hilman [khil...@deeprootsystems.com]
Sent: Friday, October 12, 2012 12:08 AM
To: Kalle Jokiniemi
Cc: linux-i2c@vger.kernel.org; w.s...@pengutronix.de; ben-li...@fluff.org;
t...@atomide.com; linux-o...@vger.kernel.org; Strashko, Grygorii; Datta,
Shubhrajyoti
Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Hi Kalle,
Kalle Jokiniemi kalle.jokini...@jollamobile.com writes:
The resume_noirq enables interrupts one-by-one starting from
first one. Now if the wake up event for suspend came from i2c
device, the i2c bus irq gets enabled before the threaded
i2c device irq, causing a flood of i2c bus interrupts as the
threaded irq that should clear the event is not enabled yet.
Fixed the issue by adding suspend_noirq and resume_early
functions that keep i2c bus interrupts disabled until
resume_noirq has run completely.
Issue was detected doing a wake up from autosleep with
twl4030 power key on N9. Patch tested on N9.
Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com
This version looks good, thanks for the extra comments.
Reviewed-by: Kevin Hilman khil...@ti.com
Tested-by: Kevin Hilman khil...@ti.com
Wolfram, This should also probably be Cc'd to stable since it affects
earlier kernels as well. Thanks,
Kevin
---
drivers/i2c/busses/i2c-omap.c | 34 ++
1 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a0e49f6..991341b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1132,6 +1132,36 @@ static int __devexit omap_i2c_remove(struct
platform_device *pdev)
}
#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
+static int omap_i2c_suspend_noirq(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /* Disabling irq here to balance the enable in resume_early */
+ disable_irq(_dev-irq);
+ return 0;
+}
+
+static int omap_i2c_resume_early(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /*
+ * The noirq_resume enables the interrupts one by one,
+ * this causes a interrupt flood if the SW irq actually reading
+ * event from i2c device is enabled only after i2c bus irq, as the
+ * irq that should clear the event is still disabled. We have to
+ * keep the bus irq disabled until all other irqs have been enabled.
+ */
+ enable_irq(_dev-irq);
+
+ return 0;
+}
+#endif
#ifdef CONFIG_PM_RUNTIME
static int omap_i2c_runtime_suspend(struct device *dev)
{
@@ -1183,6 +1213,10 @@ static int omap_i2c_runtime_resume(struct device
*dev)
#endif /* CONFIG_PM_RUNTIME */
static struct dev_pm_ops omap_i2c_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
+ .suspend_noirq = omap_i2c_suspend_noirq,
+ .resume_early = omap_i2c_resume_early,
+#endif
SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
omap_i2c_runtime_resume, NULL
[PATCH v2] ARM: OMAP: i2c: fix interrupt flood during resume
The resume_noirq enables interrupts one-by-one starting from
first one. Now if the wake up event for suspend came from i2c
device, the i2c bus irq gets enabled before the threaded
i2c device irq, causing a flood of i2c bus interrupts as the
threaded irq that should clear the event is not enabled yet.
Fixed the issue by adding suspend_late and resume_early
functions that keep i2c bus interrupts disabled until
resume_noirq has run completely.
Issue was detected doing a wake up from autosleep with
twl4030 power key on N9. Patch tested on N9.
Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com
---
drivers/i2c/busses/i2c-omap.c | 34 ++
1 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a0e49f6..991341b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1132,6 +1132,36 @@ static int __devexit omap_i2c_remove(struct
platform_device *pdev)
}
#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
+static int omap_i2c_suspend_noirq(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /* Disabling irq here to balance the enable in resume_early */
+ disable_irq(_dev-irq);
+ return 0;
+}
+
+static int omap_i2c_resume_early(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /*
+* The noirq_resume enables the interrupts one by one,
+* this causes a interrupt flood if the SW irq actually reading
+* event from i2c device is enabled only after i2c bus irq, as the
+* irq that should clear the event is still disabled. We have to
+* keep the bus irq disabled until all other irqs have been enabled.
+*/
+ enable_irq(_dev-irq);
+
+ return 0;
+}
+#endif
#ifdef CONFIG_PM_RUNTIME
static int omap_i2c_runtime_suspend(struct device *dev)
{
@@ -1183,6 +1213,10 @@ static int omap_i2c_runtime_resume(struct device *dev)
#endif /* CONFIG_PM_RUNTIME */
static struct dev_pm_ops omap_i2c_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
+ .suspend_noirq = omap_i2c_suspend_noirq,
+ .resume_early = omap_i2c_resume_early,
+#endif
SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
omap_i2c_runtime_resume, NULL)
};
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] ARM: OMAP: i2c: fix interrupt flood during resume
ke, 2012-10-10 kello 14:46 +0300, Kalle Jokiniemi kirjoitti:
The resume_noirq enables interrupts one-by-one starting from
first one. Now if the wake up event for suspend came from i2c
device, the i2c bus irq gets enabled before the threaded
i2c device irq, causing a flood of i2c bus interrupts as the
threaded irq that should clear the event is not enabled yet.
Fixed the issue by adding suspend_late and resume_early
functions that keep i2c bus interrupts disabled until
resume_noirq has run completely.
Issue was detected doing a wake up from autosleep with
twl4030 power key on N9. Patch tested on N9.
I did this now on top of latest linux-omap, should apply also to Jean's
staging tree. Let me know if something more is needed.
- Kalle
Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com
---
drivers/i2c/busses/i2c-omap.c | 34 ++
1 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a0e49f6..991341b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1132,6 +1132,36 @@ static int __devexit omap_i2c_remove(struct
platform_device *pdev)
}
#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
+static int omap_i2c_suspend_noirq(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /* Disabling irq here to balance the enable in resume_early */
+ disable_irq(_dev-irq);
+ return 0;
+}
+
+static int omap_i2c_resume_early(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /*
+ * The noirq_resume enables the interrupts one by one,
+ * this causes a interrupt flood if the SW irq actually reading
+ * event from i2c device is enabled only after i2c bus irq, as the
+ * irq that should clear the event is still disabled. We have to
+ * keep the bus irq disabled until all other irqs have been enabled.
+ */
+ enable_irq(_dev-irq);
+
+ return 0;
+}
+#endif
#ifdef CONFIG_PM_RUNTIME
static int omap_i2c_runtime_suspend(struct device *dev)
{
@@ -1183,6 +1213,10 @@ static int omap_i2c_runtime_resume(struct device *dev)
#endif /* CONFIG_PM_RUNTIME */
static struct dev_pm_ops omap_i2c_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
+ .suspend_noirq = omap_i2c_suspend_noirq,
+ .resume_early = omap_i2c_resume_early,
+#endif
SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
omap_i2c_runtime_resume, NULL)
};
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
The resume_noirq enables interrupts one-by-one starting from
first one. Now if the wake up event for suspend came from i2c
device, the i2c bus irq gets enabled before the threaded
i2c device irq, causing a flood of i2c bus interrupts as the
threaded irq that should clear the event is not enabled yet.
Fixed the issue by adding suspend_noirq and resume_early
functions that keep i2c bus interrupts disabled until
resume_noirq has run completely.
Issue was detected doing a wake up from autosleep with
twl4030 power key on N9. Patch tested on N9.
Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com
---
drivers/i2c/busses/i2c-omap.c | 34 ++
1 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a0e49f6..991341b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1132,6 +1132,36 @@ static int __devexit omap_i2c_remove(struct
platform_device *pdev)
}
#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
+static int omap_i2c_suspend_noirq(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /* Disabling irq here to balance the enable in resume_early */
+ disable_irq(_dev-irq);
+ return 0;
+}
+
+static int omap_i2c_resume_early(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /*
+* The noirq_resume enables the interrupts one by one,
+* this causes a interrupt flood if the SW irq actually reading
+* event from i2c device is enabled only after i2c bus irq, as the
+* irq that should clear the event is still disabled. We have to
+* keep the bus irq disabled until all other irqs have been enabled.
+*/
+ enable_irq(_dev-irq);
+
+ return 0;
+}
+#endif
#ifdef CONFIG_PM_RUNTIME
static int omap_i2c_runtime_suspend(struct device *dev)
{
@@ -1183,6 +1213,10 @@ static int omap_i2c_runtime_resume(struct device *dev)
#endif /* CONFIG_PM_RUNTIME */
static struct dev_pm_ops omap_i2c_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
+ .suspend_noirq = omap_i2c_suspend_noirq,
+ .resume_early = omap_i2c_resume_early,
+#endif
SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
omap_i2c_runtime_resume, NULL)
};
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: i2c-omap: fix interrupt flood during resume
The resume_noirq enables interrupts one-by-one starting from
first one. Now if the wake up event for suspend came from i2c
device, the i2c bus irq gets enabled before the threaded
i2c device irq, causing a flood of i2c bus interrupts as the
threaded irq that should clear the event is not enabled yet.
Fixed the issue by adding suspend_late and resume_early
functions that keep i2c bus interrupts disabled until
resume_noirq has run completely.
Issue was detected doing a wake up from autosleep with
twl4030 power key on N9. Patch tested on N9.
Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com
---
drivers/i2c/busses/i2c-omap.c | 37 +
1 files changed, 37 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 801df60..b77b0c2 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1158,6 +1158,35 @@ omap_i2c_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_SUSPEND
+static int omap_i2c_suspend_late(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /*
+* The noirq_resume enables the interrupts one by one,
+* this causes a interrupt flood if the SW irq actually reading
+* event from i2c device is enabled only after i2c bus irq, as the
+* irq that should clear the event is still disabled. We have to
+* disable the bus irq until all other irqs have been enabled.
+*/
+ disable_irq(_dev-irq);
+ return 0;
+}
+
+static int omap_i2c_resume_early(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ enable_irq(_dev-irq);
+
+ return 0;
+}
+#endif
#ifdef CONFIG_PM_RUNTIME
static int omap_i2c_runtime_suspend(struct device *dev)
{
@@ -1178,10 +1207,18 @@ static int omap_i2c_runtime_resume(struct device *dev)
return 0;
}
+#endif
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_PM_RUNTIME)
static struct dev_pm_ops omap_i2c_pm_ops = {
+#ifdef CONFIG_SUSPEND
+ .suspend_late = omap_i2c_suspend_late,
+ .resume_early = omap_i2c_resume_early,
+#endif
+#ifdef CONFIG_PM_RUNTIME
.runtime_suspend = omap_i2c_runtime_suspend,
.runtime_resume = omap_i2c_runtime_resume,
+#endif
};
#define OMAP_I2C_PM_OPS (omap_i2c_pm_ops)
#else
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] OMAP: I2C: Add mpu wake up latency constraint in i2c
From: ext Kalle Jokiniemi kalle.jokini...@digia.com
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-sonas...@ti.com
Cc: Jarkko Nikula jhnik...@gmail.com
Cc: Paul Walmsley p...@pwsan.com
Cc: Nishanth Menon n...@ti.com
Signed-off-by: Kalle Jokiniemi kalle.jokini...@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]),
#ifdefined(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
#ifdefined(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
