pxa:spitz_pm.c: commit b6eede11 breaks spitz resume under certain conditions.

pxa:spitz_pm.c: commit b6eede11 breaks spitz resume under certain conditions.

[Marko Katić]

Hi Eric,

========================================================
commit b6eede112673678f8a7a1680d6ad12245443829d
Author: Eric Miao <eric.y.miao@gmail.com>
Date:   Mon Jan 11 16:17:25 2010 +0800

    [ARM] pxa/spitz: use generic GPIO API and remove pxa_gpio_mode()

    REVISIT: change to GPIO18 is ugly, need to make sure whether that's
    really necessary - GPIO18_RDY as an VLIO input signal - we don't
    normally need to do such kind of trick during low power mode.

=========================================================

This GPIO18 change breaks spitz resume.
Spitz will fail to resume from STR under these conditions:

1) If the AC plug is powered and plugged in, and if you suspend the machine,
    the machine will not be able to resume if you remove the AC plug while the
   device is suspended.

2) If you connect the machine to a powered AC plug while the device is
suspended,
    the machine will not be able to resume.

If i comment out the call to pxa2xx_mfp_config in spitz_postsuspend(),
my machine will
properly resume under abovementioned conditions.

If i understand correctly, spitz_postsuspend () configures GPIO 18
to alternate mode 1 - RDY. However, I have no idea what RDY means or
what this GPIO is used for.

Eric can you please explain what's happening here?

pxa:spitz_pm.c: commit b6eede11 breaks spitz resume under certain conditions.

[Eric Miao]

On Fri, Sep 21, 2012 at 3:29 AM, Marko Kati? <dromede@gmail.com> wrote:
> Hi Eric,
>
> ========================================================
> commit b6eede112673678f8a7a1680d6ad12245443829d
> Author: Eric Miao <eric.y.miao@gmail.com>
> Date:   Mon Jan 11 16:17:25 2010 +0800
>
>     [ARM] pxa/spitz: use generic GPIO API and remove pxa_gpio_mode()
>
>     REVISIT: change to GPIO18 is ugly, need to make sure whether that's
>     really necessary - GPIO18_RDY as an VLIO input signal - we don't
>     normally need to do such kind of trick during low power mode.
>
> =========================================================
>
> This GPIO18 change breaks spitz resume.
> Spitz will fail to resume from STR under these conditions:
>
> 1) If the AC plug is powered and plugged in, and if you suspend the machine,
>     the machine will not be able to resume if you remove the AC plug while the
>    device is suspended.
>
> 2) If you connect the machine to a powered AC plug while the device is
> suspended,
>     the machine will not be able to resume.
>
> If i comment out the call to pxa2xx_mfp_config in spitz_postsuspend(),
> my machine will
> properly resume under abovementioned conditions.
>
> If i understand correctly, spitz_postsuspend () configures GPIO 18
> to alternate mode 1 - RDY. However, I have no idea what RDY means or
> what this GPIO is used for.
>
> Eric can you please explain what's happening here?

I believe that was derived from the legacy code, so if you think it's
a bug and can get a proper fix, just go with it. Note, however, if the
code affects poodle/corgi, you may take another look and make sure
it doesn't break the rest ones.

pxa:spitz_pm.c: commit b6eede11 breaks spitz resume under certain conditions.

[Marko Katić]

> I believe that was derived from the legacy code, so if you think it's
> a bug and can get a proper fix, just go with it. Note, however, if the
> code affects poodle/corgi, you may take another look and make sure
> it doesn't break the rest ones.

I think i found the actual problem here.

-       pxa_gpio_mode(GPIO18_RDY|GPIO_OUT | GPIO_DFLT_HIGH);
+       pxa2xx_mfp_config(&gpio18_config[0], 1);

These two lines don't do the same thing. The first one sets GPIO18 as
output, default high.
The second one sets GPIO18 to alternate function RDY. Similar thing
happens here:

-       pxa_gpio_mode(GPIO18_RDY_MD);
-       pxa_gpio_mode(10 | GPIO_IN);
+       pxa2xx_mfp_config(&gpio18_config[1], 1);

 This is probably due to the fact that
prior to pxa-regs.h breakup, GPIO18_RDY was defined as 18
and GPIO18_RDY_MD defined the RDY alternate function.

Basically, this bug is caused by a gpio naming mixup.

A simple fix would be:

 static unsigned long gpio18_config[] = {
-       GPIO18_RDY,
        GPIO18_GPIO,
+       GPIO18_RDY,
 };

I tested this and it works. If you think this is correct
i'll post a proper patch.

I also looked at the original sharp kernel sources.
Only tosa used the RDY signal for it's tc6393tx chip, other machines simply
configured gpio18 as output in their suspend routines.

pxa:spitz_pm.c: commit b6eede11 breaks spitz resume under certain conditions.

[Marko Katić]

> Almost there, but I guess we could do this better and less confusing by having
> another array, e.g. tosa_gpio18_config[], which is tosa specific, and only
> initialize that MFP setting in the tosa path.
>
>>
>> I also looked at the original sharp kernel sources.
>> Only tosa used the RDY signal for it's tc6393tx chip, other machines simply
>> configured gpio18 as output in their suspend routines.


Actually, tosa doesn't use sharpsl_pm. Tosa uses the pda-power framework.
I said that only tosa uses the RDY signal to point out that we
probably don't need
the mfp-config line in postsuspend. That being said, i still think
that the array ordering
fix is adequate. Maybe later we may remove the mfp-config line from
postsuspend when
we're absolutely sure it isn't necessary for devices that use spitz_pm.c.

pxa:spitz_pm.c: commit b6eede11 breaks spitz resume under certain conditions.

[Marko Katić]

On Thu, Oct 18, 2012 at 11:28 AM, Marko Kati? <dromede@gmail.com> wrote:
>> Almost there, but I guess we could do this better and less confusing by having
>> another array, e.g. tosa_gpio18_config[], which is tosa specific, and only
>> initialize that MFP setting in the tosa path.
>>
>>>
>>> I also looked at the original sharp kernel sources.
>>> Only tosa used the RDY signal for it's tc6393tx chip, other machines simply
>>> configured gpio18 as output in their suspend routines.
>
>
> Actually, tosa doesn't use sharpsl_pm. Tosa uses the pda-power framework.
> I said that only tosa uses the RDY signal to point out that we
> probably don't need
> the mfp-config line in postsuspend. That being said, i still think
> that the array ordering
> fix is adequate. Maybe later we may remove the mfp-config line from
> postsuspend when
> we're absolutely sure it isn't necessary for devices that use spitz_pm.c.

So Eric what do you think, is the simple gpio array reordering patch
an adequate fix for this bug?

pxa:spitz_pm.c: commit b6eede11 breaks spitz resume under certain conditions.

[Eric Miao]

On Fri, Oct 19, 2012 at 7:37 PM, Marko Kati? <dromede@gmail.com> wrote:
> On Thu, Oct 18, 2012 at 11:28 AM, Marko Kati? <dromede@gmail.com> wrote:
>>> Almost there, but I guess we could do this better and less confusing by having
>>> another array, e.g. tosa_gpio18_config[], which is tosa specific, and only
>>> initialize that MFP setting in the tosa path.
>>>
>>>>
>>>> I also looked at the original sharp kernel sources.
>>>> Only tosa used the RDY signal for it's tc6393tx chip, other machines simply
>>>> configured gpio18 as output in their suspend routines.
>>
>>
>> Actually, tosa doesn't use sharpsl_pm. Tosa uses the pda-power framework.
>> I said that only tosa uses the RDY signal to point out that we
>> probably don't need
>> the mfp-config line in postsuspend. That being said, i still think
>> that the array ordering
>> fix is adequate. Maybe later we may remove the mfp-config line from
>> postsuspend when
>> we're absolutely sure it isn't necessary for devices that use spitz_pm.c.
>
> So Eric what do you think, is the simple gpio array reordering patch
> an adequate fix for this bug?

Sorry for late reply. That simple reordering still looks a bit confusing
to me, i.e. the same pin firstly configured as GPIO then RDY. Do we
have a less confusing way to fix this?

pxa:spitz_pm.c: commit b6eede11 breaks spitz resume under certain conditions.

[Marko Katić]

On Tue, Oct 23, 2012 at 5:10 AM, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Fri, Oct 19, 2012 at 7:37 PM, Marko Kati? <dromede@gmail.com> wrote:
>> On Thu, Oct 18, 2012 at 11:28 AM, Marko Kati? <dromede@gmail.com> wrote:
>>>> Almost there, but I guess we could do this better and less confusing by having
>>>> another array, e.g. tosa_gpio18_config[], which is tosa specific, and only
>>>> initialize that MFP setting in the tosa path.
>>>>
>>>>>
>>>>> I also looked at the original sharp kernel sources.
>>>>> Only tosa used the RDY signal for it's tc6393tx chip, other machines simply
>>>>> configured gpio18 as output in their suspend routines.
>>>
>>>
>>> Actually, tosa doesn't use sharpsl_pm. Tosa uses the pda-power framework.
>>> I said that only tosa uses the RDY signal to point out that we
>>> probably don't need
>>> the mfp-config line in postsuspend. That being said, i still think
>>> that the array ordering
>>> fix is adequate. Maybe later we may remove the mfp-config line from
>>> postsuspend when
>>> we're absolutely sure it isn't necessary for devices that use spitz_pm.c.
>>
>> So Eric what do you think, is the simple gpio array reordering patch
>> an adequate fix for this bug?
>
> Sorry for late reply. That simple reordering still looks a bit confusing
> to me, i.e. the same pin firstly configured as GPIO then RDY. Do we
> have a less confusing way to fix this?

well, we could also remove the mfp-config line in postsuspend. I
suspect that it's only there because tosa was supposed to use
sharpsl_pm and it currently doesn't (and certainly never will). And as
i said, the original sharp kernels never did configure gpio 18 as RDY,
except for tosa.

pxa:spitz_pm.c: commit b6eede11 breaks spitz resume under certain conditions.

[Eric Miao]

On Tue, Oct 23, 2012 at 8:53 PM, Marko Kati? <dromede@gmail.com> wrote:
> On Tue, Oct 23, 2012 at 5:10 AM, Eric Miao <eric.y.miao@gmail.com> wrote:
>> On Fri, Oct 19, 2012 at 7:37 PM, Marko Kati? <dromede@gmail.com> wrote:
>>> On Thu, Oct 18, 2012 at 11:28 AM, Marko Kati? <dromede@gmail.com> wrote:
>>>>> Almost there, but I guess we could do this better and less confusing by having
>>>>> another array, e.g. tosa_gpio18_config[], which is tosa specific, and only
>>>>> initialize that MFP setting in the tosa path.
>>>>>
>>>>>>
>>>>>> I also looked at the original sharp kernel sources.
>>>>>> Only tosa used the RDY signal for it's tc6393tx chip, other machines simply
>>>>>> configured gpio18 as output in their suspend routines.
>>>>
>>>>
>>>> Actually, tosa doesn't use sharpsl_pm. Tosa uses the pda-power framework.
>>>> I said that only tosa uses the RDY signal to point out that we
>>>> probably don't need
>>>> the mfp-config line in postsuspend. That being said, i still think
>>>> that the array ordering
>>>> fix is adequate. Maybe later we may remove the mfp-config line from
>>>> postsuspend when
>>>> we're absolutely sure it isn't necessary for devices that use spitz_pm.c.
>>>
>>> So Eric what do you think, is the simple gpio array reordering patch
>>> an adequate fix for this bug?
>>
>> Sorry for late reply. That simple reordering still looks a bit confusing
>> to me, i.e. the same pin firstly configured as GPIO then RDY. Do we
>> have a less confusing way to fix this?
>
> well, we could also remove the mfp-config line in postsuspend. I
> suspect that it's only there because tosa was supposed to use
> sharpsl_pm and it currently doesn't (and certainly never will). And as
> i said, the original sharp kernels never did configure gpio 18 as RDY,
> except for tosa.

OK, please submit the patch for review.