[PATCH] OMAP/I2C - Fix timeout problem during suspend.

[PATCH] OMAP/I2C - Fix timeout problem during suspend.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]



On a board with OMAP3 processor and TWL4030 Power management,
we need to talk to the TWL4030 during late suspend but cannot
because the I2C interrupt is disabled (as late suspend disables
interrupt).

e.g. I get messages like:

[   62.161102] musb-omap2430 musb-omap2430: LATE power domain suspend
[   63.167205] omap_i2c omap_i2c.1: controller timed out
[   63.183044] twl: i2c_read failed to transfer all messages
[   64.182861] omap_i2c omap_i2c.1: controller timed out
[   64.198455] twl: i2c_write failed to transfer all messages
[   65.198455] omap_i2c omap_i2c.1: controller timed out
[   65.203765] twl: i2c_write failed to transfer all messages

The stack shows omap2430_runtime_suspend calling twl4030_set_suspend
which tries to power-down the USB PHY (twl4030_phy_suspend ->
twl4030_phy_power -> __twl4030_phy_power which as a nice WARN_ON
that helps).

Then we get the same in resume:

[   69.603912] musb-omap2430 musb-omap2430: EARLY power domain resume
[   70.610473] omap_i2c omap_i2c.1: controller timed out
[   70.626129] twl: i2c_write failed to transfer all messages
etc.

So don't disable interrupts for I2C.

Signed-off-by: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index fa23faa..23e4c65 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1066,7 +1066,7 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	isr = (dev->rev < OMAP_I2C_OMAP1_REV_2) ? omap_i2c_omap1_isr :
 								   omap_i2c_isr;
-	r = request_irq(dev->irq, isr, 0, pdev->name, dev);
+	r = request_irq(dev->irq, isr, IRQF_NO_SUSPEND, pdev->name, dev);
 
 	if (r) {
 		dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP/I2C - Fix timeout problem during suspend.

[Kevin Hilman]

+Felipe

NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org> writes:

> On a board with OMAP3 processor and TWL4030 Power management,
> we need to talk to the TWL4030 during late suspend but cannot
> because the I2C interrupt is disabled (as late suspend disables
> interrupt).

I'm not convinced this is the right solution to this problem.

IMO, this problem is caused by the MUSB driver being broken for
suspend/resume.  I've reported this problem (and an RFC/PATCH)
already[1], but I don't think the driver has been fixed.

Can you try my patch[1] to see if it fixes your problem as well?

Kevin

[1] http://marc.info/?l=linux-omap&m=132252827112721&w=2

Re: [PATCH] OMAP/I2C - Fix timeout problem during suspend.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

On Wed, 04 Jan 2012 14:19:48 -0800 Kevin Hilman <khilman-l0cyMroinI0@public.gmane.org> wrote:

> +Felipe
> 
> NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org> writes:
> 
> > On a board with OMAP3 processor and TWL4030 Power management,
> > we need to talk to the TWL4030 during late suspend but cannot
> > because the I2C interrupt is disabled (as late suspend disables
> > interrupt).
> 
> I'm not convinced this is the right solution to this problem.
> 
> IMO, this problem is caused by the MUSB driver being broken for
> suspend/resume.  I've reported this problem (and an RFC/PATCH)
> already[1], but I don't think the driver has been fixed.
> 
> Can you try my patch[1] to see if it fixes your problem as well?
> 
> Kevin
> 
> [1] http://marc.info/?l=linux-omap&m=132252827112721&w=2
> 

Yes... and no.

I reverted my patch to confirm that the timeout messages come back which
they did.
I then applied your patch and the suspend was nice and smooth again with no
timeouts.  So that is good.

However immediately after I wake it up I get:

[  109.193054] Powerdomain (core_pwrdm) didn't enter target state 1
[  109.199310] Could not enter target state in pm_suspend

whereas with my patch in place I get:

[  123.666046] Successfully put all powerdomains to target state

Following this hint I looked into current draw.  With my patch I get a
suspend-time current draw of 60-80mA (which is still too high..).
With your patch in place I get 120mA or more (about 4 tests, definitely at
least 30mA difference).  I measure this by checking the 'charge_now' reported
by the bq27000 in the battery.

This is without the usb cable plugged in.  With usb plugged in your version
has the positive effect that charging continues while in suspend while with
mine it doesn't.  But I don't think that justifies the extra current drain :-)

So I'll be sticking with my patch for now.

My next problem I need to resolve relates to i2c and the charger as well.
When entering suspend the various twl4030 interrupts are disabled by not
masked.  This means they can still fire but are not handled until after
suspend.  This effectively blocks other interrupts from the twl4030 that I
actually want (like the RTC alarm).
I think there are at least 3 or 4 bugs in here making it rather hard to sort
out.  However I think I will want to mask interrupt sources when they are
disabled.  To mask the interrupts I need to talk to the twl4030 over i2c.
And this means that I need the i2c interrupt to still be working.

So I feel that my patch might be more generally useful.  However I confess
that there is a lot here that I don't completely understand, and I might have
a different opinion tomorrow.

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP/I2C - Fix timeout problem during suspend.

[Kevin Hilman]

NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org> writes:

> On Wed, 04 Jan 2012 14:19:48 -0800 Kevin Hilman <khilman-l0cyMroinI0@public.gmane.org> wrote:
>
>> +Felipe
>> 
>> NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org> writes:
>> 
>> > On a board with OMAP3 processor and TWL4030 Power management,
>> > we need to talk to the TWL4030 during late suspend but cannot
>> > because the I2C interrupt is disabled (as late suspend disables
>> > interrupt).
>> 
>> I'm not convinced this is the right solution to this problem.
>> 
>> IMO, this problem is caused by the MUSB driver being broken for
>> suspend/resume.  I've reported this problem (and an RFC/PATCH)
>> already[1], but I don't think the driver has been fixed.
>> 
>> Can you try my patch[1] to see if it fixes your problem as well?
>> 
>> Kevin
>> 
>> [1] http://marc.info/?l=linux-omap&m=132252827112721&w=2
>> 
>
> Yes... and no.
>
> I reverted my patch to confirm that the timeout messages come back which
> they did.
> I then applied your patch and the suspend was nice and smooth again with no
> timeouts.  So that is good.
>
> However immediately after I wake it up I get:
>
> [  109.193054] Powerdomain (core_pwrdm) didn't enter target state 1
> [  109.199310] Could not enter target state in pm_suspend
>
> whereas with my patch in place I get:
>
> [  123.666046] Successfully put all powerdomains to target state
>
> Following this hint I looked into current draw.  With my patch I get a
> suspend-time current draw of 60-80mA (which is still too high..).
> With your patch in place I get 120mA or more (about 4 tests, definitely at
> least 30mA difference).  I measure this by checking the 'charge_now' reported
> by the bq27000 in the battery.
>
> This is without the usb cable plugged in.  

Strange... without the usb cable plugged in, runtime PM should kick in
and suspend the device long before system suspend happens so that there
shouldn't be anything for the MUSB driver to do during system suspend.

> With usb plugged in your version
> has the positive effect that charging continues while in suspend while with
> mine it doesn't.  But I don't think that justifies the extra current drain :-)

The reason I implemented it that way was because I understood that with
a cable plugged in, you cant reliabaly suspend (or resume) MUSB due to
HW bugs/limitations.  

Hopefully Felipe can chime in here to clarify.

> So I'll be sticking with my patch for now.
>
> My next problem I need to resolve relates to i2c and the charger as well.
> When entering suspend the various twl4030 interrupts are disabled by not
> masked.  This means they can still fire but are not handled until after
> suspend.  This effectively blocks other interrupts from the twl4030 that I
> actually want (like the RTC alarm).
> I think there are at least 3 or 4 bugs in here making it rather hard to sort
> out.  However I think I will want to mask interrupt sources when they are
> disabled.  To mask the interrupts I need to talk to the twl4030 over i2c.
> And this means that I need the i2c interrupt to still be working.
>
> So I feel that my patch might be more generally useful.  However I confess
> that there is a lot here that I don't completely understand, and I might have
> a different opinion tomorrow.

I think your patch is probably useful also, but would like to see the
MUSB driver issues better clarified and fixed as well.

Kevin