Re: [PATCH] MFD: TWL: add power off functionality

Re: [PATCH] MFD: TWL: add power off functionality

[Igor Grinberg]

Hi Neil,

On 12/03/11 03:35, NeilBrown wrote:
> On Sun, 27 Nov 2011 11:42:17 +0200 Igor Grinberg <grinberg@compulab.co.il>
> wrote:
> 
>> ping!
> 
> pong ...
> 
> 
> Hi,
>  I've been trying this patch out on my GTA04 with 3.2-rc4 and it doesn't
>  work :-(

Probably, v3.2-rc4 is not the best to try things out...
This patch is based on v3.1, can you try v3.1, so at least we can
check the that the patch itself has no problems?
Also, CC'ing linux-omap.

> 
>  As soon as it tries to touch the i2c controller to send the power-down
>  message I get:
> 
> [   96.130920] Unhandled fault: external abort on non-linefetch (0x1028) at 0xfa070008
> [   96.138885] Internal error: : 1028 [#1] PREEMPT
> [   96.143585] Modules linked in: bluetooth ipv6 g_ether hso rfkill
> [   96.149841] CPU: 0    Not tainted  (3.2.0-rc4+ #145)
> [   96.155029] PC is at omap_i2c_wait_for_bb+0xc4/0x100
> [   96.160186] LR is at omap_i2c_wait_for_bb+0xac/0x100
> [   96.165344] pc : [<c02e84d4>]    lr : [<c02e84bc>]    psr: 60000013
> [   96.165344] sp : d4a1ddc0  ip : d4a1dd20  fp : 00000002
> [   96.177246] r10: 00000002  r9 : c0d1f2c8  r8 : dbda9c00
> [   96.182678] r7 : c05d2a88  r6 : dbda9c00  r5 : ffff9aa8  r4 : c0d1f458
> [   96.189453] r3 : 00000008  r2 : 00000002  r1 : fa070000  r0 : 00000001
> [   96.196228] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
> [   96.203643] Control: 10c5387d  Table: 8bdc8019  DAC: 00000015
> [   96.209625] Process poweroff (pid: 722, stack limit = 0xd4a1c2f0)
> 
> The call trace is 
> [   96.373413] [<c02e84d4>] (omap_i2c_wait_for_bb+0xc4/0x100) from [<c02e8aa8>] (omap_i2c_xfer+0x34/0x4fc)
> [   96.383178] [<c02e8aa8>] (omap_i2c_xfer+0x34/0x4fc) from [<c02e6978>] (i2c_transfer+0xac/0x130)
> [   96.392211] [<c02e6978>] (i2c_transfer+0xac/0x130) from [<c0259d20>] (twl_i2c_read+0xd8/0x12c)
> [   96.401153] [<c0259d20>] (twl_i2c_read+0xd8/0x12c) from [<c025ba0c>] (twl4030_power_off+0x34/0x124)
> [   96.410583] [<c025ba0c>] (twl4030_power_off+0x34/0x124) from [<c000f130>] (machine_power_off+0x1c/0x28)
> [   96.420349] [<c000f130>] (machine_power_off+0x1c/0x28) from [<c004ad94>] (sys_reboot+0x124/0x1e0)
> [   96.429565] [<c004ad94>] (sys_reboot+0x124/0x1e0) from [<c000e780>] (ret_fast_syscall+0x0/0x3c)

Have you modified the patch?
Because, there is no twl_i2c_read() call in twl4030_power_off() function...
Are there any other changes we need to be aware of?

> 
> 
> It has accessed this same address 0xfa070008 multiple times during normally
> running, but here at shutdown it gets an external abort.
> Presumably something is being turned of earlier in the shutdown sequence so
> that i2c is no longer available, but I have no idea what.

What distro are you using?
Does it do any kernel related sub systems power games?

> 
> Do you have any idea what might be being disabled/turned-off/unmapped/
> cleared/whatever that could cause this?

No idea currently, I need to examine the changes between v3.1 and v3.2-rc4.

> 
> I see:
> [   96.029968] twl 1-0048: shutdown
> [   96.038604] i2c i2c-1: shutdown
> 
> amongst the messages, but as far as I can tell there is no actual shutdown
> method for these to call so they don't do anything.
> 
> Ideas?

I haven't seen those messages in my v3.1.
I will try to look at this, but it will take time
as I'm on something else right now.

-- 
Regards,
Igor.

Re: [PATCH] MFD: TWL: add power off functionality

[NeilBrown]

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

On Sun, 04 Dec 2011 11:56:44 +0200 Igor Grinberg <grinberg@compulab.co.il>
wrote:

> Hi Neil,
> 
> On 12/03/11 03:35, NeilBrown wrote:
> > On Sun, 27 Nov 2011 11:42:17 +0200 Igor Grinberg <grinberg@compulab.co.il>
> > wrote:
> > 
> >> ping!
> > 
> > pong ...
> > 
> > 
> > Hi,
> >  I've been trying this patch out on my GTA04 with 3.2-rc4 and it doesn't
> >  work :-(
> 
> Probably, v3.2-rc4 is not the best to try things out...
> This patch is based on v3.1, can you try v3.1, so at least we can
> check the that the patch itself has no problems?
> Also, CC'ing linux-omap.

I think I'll be able to give 3.1 a try - I'll let you know.


> 
> > 
> >  As soon as it tries to touch the i2c controller to send the power-down
> >  message I get:
> > 
> > [   96.130920] Unhandled fault: external abort on non-linefetch (0x1028) at 0xfa070008
> > [   96.138885] Internal error: : 1028 [#1] PREEMPT
> > [   96.143585] Modules linked in: bluetooth ipv6 g_ether hso rfkill
> > [   96.149841] CPU: 0    Not tainted  (3.2.0-rc4+ #145)
> > [   96.155029] PC is at omap_i2c_wait_for_bb+0xc4/0x100
> > [   96.160186] LR is at omap_i2c_wait_for_bb+0xac/0x100
> > [   96.165344] pc : [<c02e84d4>]    lr : [<c02e84bc>]    psr: 60000013
> > [   96.165344] sp : d4a1ddc0  ip : d4a1dd20  fp : 00000002
> > [   96.177246] r10: 00000002  r9 : c0d1f2c8  r8 : dbda9c00
> > [   96.182678] r7 : c05d2a88  r6 : dbda9c00  r5 : ffff9aa8  r4 : c0d1f458
> > [   96.189453] r3 : 00000008  r2 : 00000002  r1 : fa070000  r0 : 00000001
> > [   96.196228] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
> > [   96.203643] Control: 10c5387d  Table: 8bdc8019  DAC: 00000015
> > [   96.209625] Process poweroff (pid: 722, stack limit = 0xd4a1c2f0)
> > 
> > The call trace is 
> > [   96.373413] [<c02e84d4>] (omap_i2c_wait_for_bb+0xc4/0x100) from [<c02e8aa8>] (omap_i2c_xfer+0x34/0x4fc)
> > [   96.383178] [<c02e8aa8>] (omap_i2c_xfer+0x34/0x4fc) from [<c02e6978>] (i2c_transfer+0xac/0x130)
> > [   96.392211] [<c02e6978>] (i2c_transfer+0xac/0x130) from [<c0259d20>] (twl_i2c_read+0xd8/0x12c)
> > [   96.401153] [<c0259d20>] (twl_i2c_read+0xd8/0x12c) from [<c025ba0c>] (twl4030_power_off+0x34/0x124)
> > [   96.410583] [<c025ba0c>] (twl4030_power_off+0x34/0x124) from [<c000f130>] (machine_power_off+0x1c/0x28)
> > [   96.420349] [<c000f130>] (machine_power_off+0x1c/0x28) from [<c004ad94>] (sys_reboot+0x124/0x1e0)
> > [   96.429565] [<c004ad94>] (sys_reboot+0x124/0x1e0) from [<c000e780>] (ret_fast_syscall+0x0/0x3c)
> 
> Have you modified the patch?
> Because, there is no twl_i2c_read() call in twl4030_power_off() function...
> Are there any other changes we need to be aware of?

Yes I did, but with the unaltered patch I still got the crash in
omap_i2c_wait_for_bb.  I was just seeing if reading would work when writing
didn't.


> 
> > 
> > 
> > It has accessed this same address 0xfa070008 multiple times during normally
> > running, but here at shutdown it gets an external abort.
> > Presumably something is being turned of earlier in the shutdown sequence so
> > that i2c is no longer available, but I have no idea what.
> 
> What distro are you using?

Debian

> Does it do any kernel related sub systems power games?

I don't think so, no.


> 
> > 
> > Do you have any idea what might be being disabled/turned-off/unmapped/
> > cleared/whatever that could cause this?
> 
> No idea currently, I need to examine the changes between v3.1 and v3.2-rc4.
> 
> > 
> > I see:
> > [   96.029968] twl 1-0048: shutdown
> > [   96.038604] i2c i2c-1: shutdown
> > 
> > amongst the messages, but as far as I can tell there is no actual shutdown
> > method for these to call so they don't do anything.
> > 
> > Ideas?
> 
> I haven't seen those messages in my v3.1.
> I will try to look at this, but it will take time
> as I'm on something else right now.
> 

These messages are generated by having CONFIG_DEBUG_DRIVER defined.
It's just showing that device_shutdown() had tried to shut these things down.


Thanks,
NeilBrown

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

Re: [PATCH] MFD: TWL: add power off functionality

[NeilBrown]

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

On Sun, 4 Dec 2011 21:58:59 +1100 NeilBrown <neilb@suse.de> wrote:

> On Sun, 04 Dec 2011 11:56:44 +0200 Igor Grinberg <grinberg@compulab.co.il>
> wrote:
> 
> > Hi Neil,
> > 
> > On 12/03/11 03:35, NeilBrown wrote:
> > > On Sun, 27 Nov 2011 11:42:17 +0200 Igor Grinberg <grinberg@compulab.co.il>
> > > wrote:
> > > 
> > >> ping!
> > > 
> > > pong ...
> > > 
> > > 
> > > Hi,
> > >  I've been trying this patch out on my GTA04 with 3.2-rc4 and it doesn't
> > >  work :-(
> > 
> > Probably, v3.2-rc4 is not the best to try things out...
> > This patch is based on v3.1, can you try v3.1, so at least we can
> > check the that the patch itself has no problems?
> > Also, CC'ing linux-omap.
> 
> I think I'll be able to give 3.1 a try - I'll let you know.
> 

Yes, works fine with 3.1


I've managed to find the problem.

commit af8db1508f2c9f3b6e633e2d2d906c6557c617f9
Author: Peter Chen <peter.chen@freescale.com>
Date:   Tue Nov 15 21:52:29 2011 +0100

    PM / driver core: disable device's runtime PM during shutdown
 
....

@@ -1742,6 +1743,8 @@ void device_shutdown(void)
                 */
                list_del_init(&dev->kobj.entry);
                spin_unlock(&devices_kset->list_lock);
+               /* Disable all device's runtime power management */
+               pm_runtime_disable(dev);
 
                if (dev->bus && dev->bus->shutdown) {
                        dev_dbg(dev, "shutdown\n");



Removing this call allows power-off to work.

It seems that omap_i2c.1 is normally in runtime suspend.
omap_i2c_xfer wakes it up, performs the xfer, then puts it back to sleep.

So this pm_runtime_disable is called while the device is asleep, so it stays
asleep.  omap_i2c_xfer cannot wake it up and so cannot xfer anything.

I'll start a new thread including the people responsible for that patch.

Thanks for your time,
NeilBrown

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

Re: [PATCH] MFD: TWL: add power off functionality

[Igor Grinberg]

On 12/04/11 23:43, NeilBrown wrote:
> On Sun, 4 Dec 2011 21:58:59 +1100 NeilBrown <neilb@suse.de> wrote:
> 
>> On Sun, 04 Dec 2011 11:56:44 +0200 Igor Grinberg <grinberg@compulab.co.il>
>> wrote:
>>
>>> Hi Neil,
>>>
>>> On 12/03/11 03:35, NeilBrown wrote:
>>>> On Sun, 27 Nov 2011 11:42:17 +0200 Igor Grinberg <grinberg@compulab.co.il>
>>>> wrote:
>>>>
>>>>> ping!
>>>>
>>>> pong ...
>>>>
>>>>
>>>> Hi,
>>>>  I've been trying this patch out on my GTA04 with 3.2-rc4 and it doesn't
>>>>  work :-(
>>>
>>> Probably, v3.2-rc4 is not the best to try things out...
>>> This patch is based on v3.1, can you try v3.1, so at least we can
>>> check the that the patch itself has no problems?
>>> Also, CC'ing linux-omap.
>>
>> I think I'll be able to give 3.1 a try - I'll let you know.
>>
> 
> Yes, works fine with 3.1

Good to hear!

> 
> 
> I've managed to find the problem.
> 
> commit af8db1508f2c9f3b6e633e2d2d906c6557c617f9
> Author: Peter Chen <peter.chen@freescale.com>
> Date:   Tue Nov 15 21:52:29 2011 +0100
> 
>     PM / driver core: disable device's runtime PM during shutdown
>  
> ....
> 
> @@ -1742,6 +1743,8 @@ void device_shutdown(void)
>                  */
>                 list_del_init(&dev->kobj.entry);
>                 spin_unlock(&devices_kset->list_lock);
> +               /* Disable all device's runtime power management */
> +               pm_runtime_disable(dev);
>  
>                 if (dev->bus && dev->bus->shutdown) {
>                         dev_dbg(dev, "shutdown\n");
> 
> 
> 
> Removing this call allows power-off to work.
> 
> It seems that omap_i2c.1 is normally in runtime suspend.
> omap_i2c_xfer wakes it up, performs the xfer, then puts it back to sleep.
> 
> So this pm_runtime_disable is called while the device is asleep, so it stays
> asleep.  omap_i2c_xfer cannot wake it up and so cannot xfer anything.

Good job on that investigation.
There were several discussions runtime pm related at the kernel summit,
but I failed to see the impact of that on the issue...

> 
> I'll start a new thread including the people responsible for that patch.

Hopefully, it will get us a solution.


-- 
Regards,
Igor.