[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

Some MFP configurations specify MFP_LPM_KEEP_OUTPUT to preserve the gpio output
value (HIGH or LOW) during sleep mode. For example:

    GPIO72_GPIO | MFP_LPM_KEEP_OUTPUT,

Unfortunately MFP_LPM_KEEP_OUTPUT makes no special provision for setting the
sleep mode gpio direction, unlike MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.
Consequently MFP configurations of the form:

    GPIO<n>_GPIO | MFP_LPM_KEEP_OUTPUT,

will set the sleep mode gpio direction to INPUT.

This patch forces the sleep mode gpio direction to OUTPUT in cases where
MFP_LPM_KEEP_OUTPUT was specified. This brings MFP_LPM_KEEP_OUTPUT into line
with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.

Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
---

diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c b/arch/arm/mach-pxa/mfp-pxa2xx.c
index b0a8428..3b443199 100644
--- a/arch/arm/mach-pxa/mfp-pxa2xx.c
+++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
@@ -96,6 +96,9 @@ static int __mfp_config_gpio(unsigned gpio, unsigned long c)
 		break;
 	}
 
+	if (c & MFP_LPM_KEEP_OUTPUT)
+		is_out = 1;
+
 	if (is_out ^ gpio_desc[gpio].dir_inverted)
 		gpdr_lpm[bank] |= mask;
 	else

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Igor Grinberg]

Hi Paul,

On 03/31/12 15:20, Paul Parsons wrote:
> Some MFP configurations specify MFP_LPM_KEEP_OUTPUT to preserve the gpio output
> value (HIGH or LOW) during sleep mode. For example:
> 
>     GPIO72_GPIO | MFP_LPM_KEEP_OUTPUT,
> 
> Unfortunately MFP_LPM_KEEP_OUTPUT makes no special provision for setting the
> sleep mode gpio direction, unlike MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.
> Consequently MFP configurations of the form:
> 
>     GPIO<n>_GPIO | MFP_LPM_KEEP_OUTPUT,
> 
> will set the sleep mode gpio direction to INPUT.
> 
> This patch forces the sleep mode gpio direction to OUTPUT in cases where
> MFP_LPM_KEEP_OUTPUT was specified. This brings MFP_LPM_KEEP_OUTPUT into line
> with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.
> 
> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
> ---
> 
> diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c b/arch/arm/mach-pxa/mfp-pxa2xx.c
> index b0a8428..3b443199 100644
> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
> @@ -96,6 +96,9 @@ static int __mfp_config_gpio(unsigned gpio, unsigned long c)
>  		break;
>  	}
>  
> +	if (c & MFP_LPM_KEEP_OUTPUT)
> +		is_out = 1;
> +

MFP_LPM_KEEP_OUTPUT does not meant to be used to setup the mfp config,
but for pins that have been configured for output by gpio_direction_out().
(Hint: *_KEEP_*).

Also, I don't think this is a good idea, because the patch allows a pin
be configured as output, but does not forces a value (high/low)
and this is not safe.

Why can't you use:
GPIO<n>_GPIO | MFP_LPM_DRIVE_<level>
?

>  	if (is_out ^ gpio_desc[gpio].dir_inverted)
>  		gpdr_lpm[bank] |= mask;
>  	else

-- 
Regards,
Igor.

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Paul,
> 
> On 03/31/12 15:20, Paul Parsons wrote:
> > Some MFP configurations specify MFP_LPM_KEEP_OUTPUT to
> preserve the gpio output
> > value (HIGH or LOW) during sleep mode. For example:
> > 
> >? ???GPIO72_GPIO |
> MFP_LPM_KEEP_OUTPUT,
> > 
> > Unfortunately MFP_LPM_KEEP_OUTPUT makes no special
> provision for setting the
> > sleep mode gpio direction, unlike MFP_LPM_DRIVE_HIGH
> and MFP_LPM_DRIVE_LOW.
> > Consequently MFP configurations of the form:
> > 
> >? ???GPIO<n>_GPIO |
> MFP_LPM_KEEP_OUTPUT,
> > 
> > will set the sleep mode gpio direction to INPUT.
> > 
> > This patch forces the sleep mode gpio direction to
> OUTPUT in cases where
> > MFP_LPM_KEEP_OUTPUT was specified. This brings
> MFP_LPM_KEEP_OUTPUT into line
> > with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.
> > 
> > Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
> > ---
> > 
> > diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c
> b/arch/arm/mach-pxa/mfp-pxa2xx.c
> > index b0a8428..3b443199 100644
> > --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
> > +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
> > @@ -96,6 +96,9 @@ static int __mfp_config_gpio(unsigned
> gpio, unsigned long c)
> >? ??? ??? break;
> >? ??? }
> >? 
> > +??? if (c & MFP_LPM_KEEP_OUTPUT)
> > +??? ??? is_out = 1;
> > +
> 
> MFP_LPM_KEEP_OUTPUT does not meant to be used to setup the
> mfp config,

As far as I can see, MFP_LPM_KEEP_OUTPUT is *only* used to setup the
mfp config:

% xzcat linux-3.4-rc1.tar.xz | fgrep -a MFP_LPM_KEEP_OUTPUT
	GPIO13_GPIO | MFP_LPM_KEEP_OUTPUT,	/* CORGI_GPIO_LED_ORANGE */
	GPIO38_GPIO | MFP_LPM_KEEP_OUTPUT,	/* CORGI_GPIO_CHRG_ON */
	GPIO43_GPIO | MFP_LPM_KEEP_OUTPUT,	/* CORGI_GPIO_CHRG_UKN */
#define MFP_LPM_KEEP_OUTPUT	(0x1 << 25)
	/* set corresponding PGSR bit of those marked MFP_LPM_KEEP_OUTPUT */
		if ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT) &&
% 

> but for pins that have been configured for output by
> gpio_direction_out().
> (Hint: *_KEEP_*).

MFP_LPM_KEEP_OUTPUT only takes effect when the system enters sleep mode,
at which point the GPIO directions (GPDR registers) are forced from the
values privately stored in gpdr_lpm[].
As far as I can tell MFP_LPM_KEEP_OUTPUT has no direct connection to
gpio_direction_output().

> Also, I don't think this is a good idea, because the patch
> allows a pin
> be configured as output, but does not forces a value
> (high/low)
> and this is not safe.

A value does not need to be provided until the system enters sleep mode,
at which point the value is provided via the PGSR registers.

   353	static int pxa2xx_mfp_suspend(void)
   354	{
   355		int i;
   356	
   357		/* set corresponding PGSR bit of those marked MFP_LPM_KEEP_OUTPUT */
   358		for (i = 0; i < pxa_last_gpio; i++) {
   359			if ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT) &&
   360			    (GPDR(i) & GPIO_bit(i))) {
   361				if (GPLR(i) & GPIO_bit(i))
   362					PGSR(gpio_to_bank(i)) |= GPIO_bit(i);
   363				else
   364					PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);
   365			}
   366		}
   367	
   368		for (i = 0; i <= gpio_to_bank(pxa_last_gpio); i++) {
   369	
        ...
   375			GPDR(i * 32) = gpdr_lpm[i];
   376		}
   377		return 0;
   378	}

However looking at pxa2xx_mfp_suspend() more closely it is true that the
GPDR registers are set before the PGSR values take effect (when the
processor enters sleep mode).
That is no different from MFP_LPM_DRIVE_HIGH or MFP_LPM_DRIVE_LOW though.
So the presumption must be that the GPIOs configured as DRIVE_HIGH,
DRIVER_LOW and now KEEP_OUTPUT already have a valid direction and value
set@the point the GPDR registers are forced.

> Why can't you use:
> GPIO<n>_GPIO | MFP_LPM_DRIVE_<level>
> ?

The hx4700 has a couple of charging GPIOs which we want to keep HIGH or
LOW during sleep mode:

% fgrep MFP_LPM_KEEP_OUTPUT arch/arm/mach-pxa/hx4700.c
	GPIO72_GPIO | MFP_LPM_KEEP_OUTPUT,	/* BQ24022_nCHARGE_EN */
	GPIO96_GPIO | MFP_LPM_KEEP_OUTPUT,	/* BQ24022_ISET2 */
% 

Regards,
Paul

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Igor Grinberg]

On 04/01/12 13:56, Paul Parsons wrote:
> Hello Igor,
> 
> --- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> Hi Paul,
>>
>> On 03/31/12 15:20, Paul Parsons wrote:
>>> Some MFP configurations specify MFP_LPM_KEEP_OUTPUT to
>> preserve the gpio output
>>> value (HIGH or LOW) during sleep mode. For example:
>>>
>>>      GPIO72_GPIO |
>> MFP_LPM_KEEP_OUTPUT,
>>>
>>> Unfortunately MFP_LPM_KEEP_OUTPUT makes no special
>> provision for setting the
>>> sleep mode gpio direction, unlike MFP_LPM_DRIVE_HIGH
>> and MFP_LPM_DRIVE_LOW.
>>> Consequently MFP configurations of the form:
>>>
>>>      GPIO<n>_GPIO |
>> MFP_LPM_KEEP_OUTPUT,
>>>
>>> will set the sleep mode gpio direction to INPUT.
>>>
>>> This patch forces the sleep mode gpio direction to
>> OUTPUT in cases where
>>> MFP_LPM_KEEP_OUTPUT was specified. This brings
>> MFP_LPM_KEEP_OUTPUT into line
>>> with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.
>>>
>>> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
>>> ---
>>>
>>> diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c
>> b/arch/arm/mach-pxa/mfp-pxa2xx.c
>>> index b0a8428..3b443199 100644
>>> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
>>> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
>>> @@ -96,6 +96,9 @@ static int __mfp_config_gpio(unsigned
>> gpio, unsigned long c)
>>>           break;
>>>       }
>>>   
>>> +    if (c & MFP_LPM_KEEP_OUTPUT)
>>> +        is_out = 1;
>>> +
>>
>> MFP_LPM_KEEP_OUTPUT does not meant to be used to setup the
>> mfp config,
> 
> As far as I can see, MFP_LPM_KEEP_OUTPUT is *only* used to setup the
> mfp config:
> 
> % xzcat linux-3.4-rc1.tar.xz | fgrep -a MFP_LPM_KEEP_OUTPUT
> 	GPIO13_GPIO | MFP_LPM_KEEP_OUTPUT,	/* CORGI_GPIO_LED_ORANGE */
> 	GPIO38_GPIO | MFP_LPM_KEEP_OUTPUT,	/* CORGI_GPIO_CHRG_ON */
> 	GPIO43_GPIO | MFP_LPM_KEEP_OUTPUT,	/* CORGI_GPIO_CHRG_UKN */
> #define MFP_LPM_KEEP_OUTPUT	(0x1 << 25)
> 	/* set corresponding PGSR bit of those marked MFP_LPM_KEEP_OUTPUT */
> 		if ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT) &&
> % 

How can you be certain that you don't break corgi, with the proposed patch?

> 
>> but for pins that have been configured for output by
>> gpio_direction_out().
>> (Hint: *_KEEP_*).
> 
> MFP_LPM_KEEP_OUTPUT only takes effect when the system enters sleep mode,
> at which point the GPIO directions (GPDR registers) are forced from the
> values privately stored in gpdr_lpm[].

Correct, but with your patch, the direction will be forced to output,
but the value will be undefined...

> As far as I can tell MFP_LPM_KEEP_OUTPUT has no direct connection to
> gpio_direction_output().

Well, it has... in the pxa2xx_mfp_suspend() function you've pasted below.

> 
>> Also, I don't think this is a good idea, because the patch
>> allows a pin
>> be configured as output, but does not forces a value
>> (high/low)
>> and this is not safe.
> 
> A value does not need to be provided until the system enters sleep mode,
> at which point the value is provided via the PGSR registers.

Yes, but you don't force the PGSR to be set...
and that is the problem.

> 
>    353	static int pxa2xx_mfp_suspend(void)
>    354	{
>    355		int i;
>    356	
>    357		/* set corresponding PGSR bit of those marked MFP_LPM_KEEP_OUTPUT */
>    358		for (i = 0; i < pxa_last_gpio; i++) {
>    359			if ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT) &&
>    360			    (GPDR(i) & GPIO_bit(i))) {

Look at the check above...
the PGSR only gets set for the GPIOs configured to output
(presumably by gpio_direction_output() function).

>    361				if (GPLR(i) & GPIO_bit(i))
>    362					PGSR(gpio_to_bank(i)) |= GPIO_bit(i);
>    363				else
>    364					PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);
>    365			}
>    366		}
>    367	
>    368		for (i = 0; i <= gpio_to_bank(pxa_last_gpio); i++) {
>    369	
>         ...
>    375			GPDR(i * 32) = gpdr_lpm[i];
>    376		}
>    377		return 0;
>    378	}
> 
> However looking at pxa2xx_mfp_suspend() more closely it is true that the
> GPDR registers are set before the PGSR values take effect (when the
> processor enters sleep mode).
> That is no different from MFP_LPM_DRIVE_HIGH or MFP_LPM_DRIVE_LOW though.

Well, it is different, because the in case of MFP_LPM_DRIVE_*,
the PGSR is set in the __mfp_config_gpio() function and has a _known value_.

> So the presumption must be that the GPIOs configured as DRIVE_HIGH,
> DRIVER_LOW and now KEEP_OUTPUT already have a valid direction and value
> set at the point the GPDR registers are forced.

Yes. That is exactly my point here.
MFP_LPM_DRIVE_* has the value determined and set.
MFP_LPM_KEEP_OUTPUT does not, but with your patch it will effectively
force the GPIO to be output in sleep mode, but with no value set!

> 
>> Why can't you use:
>> GPIO<n>_GPIO | MFP_LPM_DRIVE_<level>
>> ?
> 
> The hx4700 has a couple of charging GPIOs which we want to keep HIGH or
> LOW during sleep mode:
> 
> % fgrep MFP_LPM_KEEP_OUTPUT arch/arm/mach-pxa/hx4700.c
> 	GPIO72_GPIO | MFP_LPM_KEEP_OUTPUT,	/* BQ24022_nCHARGE_EN */
> 	GPIO96_GPIO | MFP_LPM_KEEP_OUTPUT,	/* BQ24022_ISET2 */
> % 

I still, can't understand, why MFP_LPM_DRIVE_* does not do the job for you...
What's the real problem with using MFP_LPM_DRIVE_*?
Can't you specify it for those GPIO72/96?


-- 
Regards,
Igor.

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
> On 04/01/12 13:56, Paul Parsons
> wrote:
> > Hello Igor,
> > 
> > --- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il>
> wrote:
> >> Hi Paul,
> >>
> >> On 03/31/12 15:20, Paul Parsons wrote:
> >>> Some MFP configurations specify
> MFP_LPM_KEEP_OUTPUT to
> >> preserve the gpio output
> >>> value (HIGH or LOW) during sleep mode. For
> example:
> >>>
> >>>? ? ? GPIO72_GPIO |
> >> MFP_LPM_KEEP_OUTPUT,
> >>>
> >>> Unfortunately MFP_LPM_KEEP_OUTPUT makes no
> special
> >> provision for setting the
> >>> sleep mode gpio direction, unlike
> MFP_LPM_DRIVE_HIGH
> >> and MFP_LPM_DRIVE_LOW.
> >>> Consequently MFP configurations of the form:
> >>>
> >>>? ? ? GPIO<n>_GPIO |
> >> MFP_LPM_KEEP_OUTPUT,
> >>>
> >>> will set the sleep mode gpio direction to
> INPUT.
> >>>
> >>> This patch forces the sleep mode gpio direction
> to
> >> OUTPUT in cases where
> >>> MFP_LPM_KEEP_OUTPUT was specified. This brings
> >> MFP_LPM_KEEP_OUTPUT into line
> >>> with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.
> >>>
> >>> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
> >>> ---
> >>>
> >>> diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c
> >> b/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>> index b0a8428..3b443199 100644
> >>> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>> @@ -96,6 +96,9 @@ static int
> __mfp_config_gpio(unsigned
> >> gpio, unsigned long c)
> >>>? ? ? ?
> ???break;
> >>>? ? ???}
> >>>???
> >>> +? ? if (c &
> MFP_LPM_KEEP_OUTPUT)
> >>> +? ? ? ? is_out = 1;
> >>> +
> >>
> >> MFP_LPM_KEEP_OUTPUT does not meant to be used to
> setup the
> >> mfp config,
> > 
> > As far as I can see, MFP_LPM_KEEP_OUTPUT is *only* used
> to setup the
> > mfp config:
> > 
> > % xzcat linux-3.4-rc1.tar.xz | fgrep -a
> MFP_LPM_KEEP_OUTPUT
> > ??? GPIO13_GPIO |
> MFP_LPM_KEEP_OUTPUT,??? /*
> CORGI_GPIO_LED_ORANGE */
> > ??? GPIO38_GPIO |
> MFP_LPM_KEEP_OUTPUT,??? /* CORGI_GPIO_CHRG_ON
> */
> > ??? GPIO43_GPIO |
> MFP_LPM_KEEP_OUTPUT,??? /*
> CORGI_GPIO_CHRG_UKN */
> > #define MFP_LPM_KEEP_OUTPUT??? (0x1
> << 25)
> > ??? /* set corresponding PGSR bit of
> those marked MFP_LPM_KEEP_OUTPUT */
> > ??? ??? if
> ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT) &&
> > % 
> 
> How can you be certain that you don't break corgi, with the
> proposed patch?

On corgi, the three GPIOs configured as MFP_LPM_KEEP_OUTPUT:
    CORGI_GPIO_LED_ORANGE
    CORGI_GPIO_CHRG_ON
    CORGI_GPIO_CHRG_UKN
are all used as outputs.

> >> but for pins that have been configured for output
> by
> >> gpio_direction_out().
> >> (Hint: *_KEEP_*).
> > 
> > MFP_LPM_KEEP_OUTPUT only takes effect when the system
> enters sleep mode,
> > at which point the GPIO directions (GPDR registers) are
> forced from the
> > values privately stored in gpdr_lpm[].
> 
> Correct, but with your patch, the direction will be forced
> to output,
> but the value will be undefined...

The values will be defined in pxa2xx_mfp_suspend() when it sets the PGSR
registers. Or at least they will be on corgi and hx4700, but see below...

> > As far as I can tell MFP_LPM_KEEP_OUTPUT has no direct
> connection to
> > gpio_direction_output().
> 
> Well, it has... in the pxa2xx_mfp_suspend() function you've
> pasted below.
> 
> > 
> >> Also, I don't think this is a good idea, because
> the patch
> >> allows a pin
> >> be configured as output, but does not forces a
> value
> >> (high/low)
> >> and this is not safe.
> > 
> > A value does not need to be provided until the system
> enters sleep mode,
> > at which point the value is provided via the PGSR
> registers.
> 
> Yes, but you don't force the PGSR to be set...
> and that is the problem.

OK, see below...

> >? ? 353??? static int
> pxa2xx_mfp_suspend(void)
> >? ? 354??? {
> >? ? 355??? ???
> int i;
> >? ? 356??? 
> >? ? 357??? ???
> /* set corresponding PGSR bit of those marked
> MFP_LPM_KEEP_OUTPUT */
> >? ? 358??? ???
> for (i = 0; i < pxa_last_gpio; i++) {
> >? ? 359??? ???
> ??? if ((gpio_desc[i].config &
> MFP_LPM_KEEP_OUTPUT) &&
> >? ? 360??? ???
> ??? ? ? (GPDR(i) &
> GPIO_bit(i))) {
> 
> Look at the check above...
> the PGSR only gets set for the GPIOs configured to output
> (presumably by gpio_direction_output() function).

OK, I understand your objection here.

My feeling is that the (GPDR(i) & GPIO_bit(i)) test is unwanted because
the current state of GPDR (via gpio_direction_output()) does not apply in
sleep mode, yet for some reason it is being allowed to prevent the
setting of PGSR in the case of GPIO inputs.

Consequently the (GPDR(i) & GPIO_bit(i)) test should be removed.

> >? ? 361??? ???
> ??? ??? if (GPLR(i) &
> GPIO_bit(i))
> >? ? 362??? ???
> ??? ??? ???
> PGSR(gpio_to_bank(i)) |= GPIO_bit(i);
> >? ? 363??? ???
> ??? ??? else
> >? ? 364??? ???
> ??? ??? ???
> PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);
> >? ? 365??? ???
> ??? }
> >? ? 366??? ???
> }
> >? ? 367??? 
> >? ? 368??? ???
> for (i = 0; i <= gpio_to_bank(pxa_last_gpio); i++) {
> >? ? 369??? 
> >? ? ? ???...
> >? ? 375??? ???
> ??? GPDR(i * 32) = gpdr_lpm[i];
> >? ? 376??? ???
> }
> >? ? 377??? ???
> return 0;
> >? ? 378??? }
> > 
> > However looking at pxa2xx_mfp_suspend() more closely it
> is true that the
> > GPDR registers are set before the PGSR values take
> effect (when the
> > processor enters sleep mode).
> > That is no different from MFP_LPM_DRIVE_HIGH or
> MFP_LPM_DRIVE_LOW though.
> 
> Well, it is different, because the in case of
> MFP_LPM_DRIVE_*,
> the PGSR is set in the __mfp_config_gpio() function and has
> a _known value_.

KEEP_OUTPUT does the same where GPIOs are configured as outputs, as is
the case with corgi and hx4700.
Your valid objection is for cases where GPIOs are configured as inputs.
So I presume that removing the (GPDR(i) & GPIO_bit(i)) test will satisfy
your objection.

> > So the presumption must be that the GPIOs configured as
> DRIVE_HIGH,
> > DRIVER_LOW and now KEEP_OUTPUT already have a valid
> direction and value
> > set at the point the GPDR registers are forced.
> 
> Yes. That is exactly my point here.
> MFP_LPM_DRIVE_* has the value determined and set.
> MFP_LPM_KEEP_OUTPUT does not, but with your patch it will
> effectively
> force the GPIO to be output in sleep mode, but with no value
> set!

Only in cases where the GPIO is used as an input in normal mode but
configured to be an output in sleep mode.
Removing the (GPDR(i) & GPIO_bit(i)) test addresses that case.

> >> Why can't you use:
> >> GPIO<n>_GPIO | MFP_LPM_DRIVE_<level>
> >> ?
> > 
> > The hx4700 has a couple of charging GPIOs which we want
> to keep HIGH or
> > LOW during sleep mode:
> > 
> > % fgrep MFP_LPM_KEEP_OUTPUT arch/arm/mach-pxa/hx4700.c
> > ??? GPIO72_GPIO |
> MFP_LPM_KEEP_OUTPUT,??? /* BQ24022_nCHARGE_EN
> */
> > ??? GPIO96_GPIO |
> MFP_LPM_KEEP_OUTPUT,??? /* BQ24022_ISET2 */
> > % 
> 
> I still, can't understand, why MFP_LPM_DRIVE_* does not do
> the job for you...
> What's the real problem with using MFP_LPM_DRIVE_*?
> Can't you specify it for those GPIO72/96?

The feeling was that if the unit was charging in normal mode then it
should remain charging in sleep mode, and if it wasn't then it should
remain not charging.
Actually that's an interim solution until the bootloader can take control
of sleep mode charging, but we're not there yet.

Regards,
Paul

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Igor Grinberg]

Hi Paul,

Can you, please, configure your mailer to _not_ wrap the text?
It is rally hard to read it after a few replies...

On 04/01/12 20:56, Paul Parsons wrote:
> Hello Igor,
> 
> --- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> On 04/01/12 13:56, Paul Parsons
>> wrote:
>>> Hello Igor,
>>>
>>> --- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il>
>> wrote:
>>>> Hi Paul,
>>>>
>>>> On 03/31/12 15:20, Paul Parsons wrote:
>>>>> Some MFP configurations specify
>> MFP_LPM_KEEP_OUTPUT to
>>>> preserve the gpio output
>>>>> value (HIGH or LOW) during sleep mode. For
>> example:
>>>>>
>>>>>       GPIO72_GPIO |
>>>> MFP_LPM_KEEP_OUTPUT,
>>>>>
>>>>> Unfortunately MFP_LPM_KEEP_OUTPUT makes no
>> special
>>>> provision for setting the
>>>>> sleep mode gpio direction, unlike
>> MFP_LPM_DRIVE_HIGH
>>>> and MFP_LPM_DRIVE_LOW.
>>>>> Consequently MFP configurations of the form:
>>>>>
>>>>>       GPIO<n>_GPIO |
>>>> MFP_LPM_KEEP_OUTPUT,
>>>>>
>>>>> will set the sleep mode gpio direction to
>> INPUT.
>>>>>
>>>>> This patch forces the sleep mode gpio direction
>> to
>>>> OUTPUT in cases where
>>>>> MFP_LPM_KEEP_OUTPUT was specified. This brings
>>>> MFP_LPM_KEEP_OUTPUT into line
>>>>> with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.
>>>>>
>>>>> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
>>>>> ---
>>>>>
>>>>> diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c
>>>> b/arch/arm/mach-pxa/mfp-pxa2xx.c
>>>>> index b0a8428..3b443199 100644
>>>>> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
>>>>> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
>>>>> @@ -96,6 +96,9 @@ static int
>> __mfp_config_gpio(unsigned
>>>> gpio, unsigned long c)
>>>>>        
>>    break;
>>>>>        }
>>>>>    
>>>>> +    if (c &
>> MFP_LPM_KEEP_OUTPUT)
>>>>> +        is_out = 1;
>>>>> +
>>>>
>>>> MFP_LPM_KEEP_OUTPUT does not meant to be used to
>> setup the
>>>> mfp config,
>>>
>>> As far as I can see, MFP_LPM_KEEP_OUTPUT is *only* used
>> to setup the
>>> mfp config:
>>>
>>> % xzcat linux-3.4-rc1.tar.xz | fgrep -a
>> MFP_LPM_KEEP_OUTPUT
>>>     GPIO13_GPIO |
>> MFP_LPM_KEEP_OUTPUT,    /*
>> CORGI_GPIO_LED_ORANGE */
>>>     GPIO38_GPIO |
>> MFP_LPM_KEEP_OUTPUT,    /* CORGI_GPIO_CHRG_ON
>> */
>>>     GPIO43_GPIO |
>> MFP_LPM_KEEP_OUTPUT,    /*
>> CORGI_GPIO_CHRG_UKN */
>>> #define MFP_LPM_KEEP_OUTPUT    (0x1
>> << 25)
>>>     /* set corresponding PGSR bit of
>> those marked MFP_LPM_KEEP_OUTPUT */
>>>         if
>> ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT) &&
>>> % 
>>
>> How can you be certain that you don't break corgi, with the
>> proposed patch?
> 
> On corgi, the three GPIOs configured as MFP_LPM_KEEP_OUTPUT:
>     CORGI_GPIO_LED_ORANGE
>     CORGI_GPIO_CHRG_ON
>     CORGI_GPIO_CHRG_UKN
> are all used as outputs.

Have you verified it, because currently MFP_LPM_KEEP_OUTPUT
does not do a direction configuration... and IMO, it should not!

> 
>>>> but for pins that have been configured for output
>> by
>>>> gpio_direction_out().
>>>> (Hint: *_KEEP_*).
>>>
>>> MFP_LPM_KEEP_OUTPUT only takes effect when the system
>> enters sleep mode,
>>> at which point the GPIO directions (GPDR registers) are
>> forced from the
>>> values privately stored in gpdr_lpm[].
>>
>> Correct, but with your patch, the direction will be forced
>> to output,
>> but the value will be undefined...
> 
> The values will be defined in pxa2xx_mfp_suspend() when it sets the PGSR
> registers. Or at least they will be on corgi and hx4700, but see below...

No, they will not, because the direction can be input...

> 
>>> As far as I can tell MFP_LPM_KEEP_OUTPUT has no direct
>> connection to
>>> gpio_direction_output().
>>
>> Well, it has... in the pxa2xx_mfp_suspend() function you've
>> pasted below.
>>
>>>
>>>> Also, I don't think this is a good idea, because
>> the patch
>>>> allows a pin
>>>> be configured as output, but does not forces a
>> value
>>>> (high/low)
>>>> and this is not safe.
>>>
>>> A value does not need to be provided until the system
>> enters sleep mode,
>>> at which point the value is provided via the PGSR
>> registers.
>>
>> Yes, but you don't force the PGSR to be set...
>> and that is the problem.
> 
> OK, see below...
> 
>>>     353    static int
>> pxa2xx_mfp_suspend(void)
>>>     354    {
>>>     355       
>> int i;
>>>     356    
>>>     357       
>> /* set corresponding PGSR bit of those marked
>> MFP_LPM_KEEP_OUTPUT */
>>>     358       
>> for (i = 0; i < pxa_last_gpio; i++) {
>>>     359       
>>     if ((gpio_desc[i].config &
>> MFP_LPM_KEEP_OUTPUT) &&
>>>     360       
>>         (GPDR(i) &
>> GPIO_bit(i))) {
>>
>> Look at the check above...
>> the PGSR only gets set for the GPIOs configured to output
>> (presumably by gpio_direction_output() function).
> 
> OK, I understand your objection here.
> 
> My feeling is that the (GPDR(i) & GPIO_bit(i)) test is unwanted because
> the current state of GPDR (via gpio_direction_output()) does not apply in
> sleep mode,

It gets copied to PGSR in the pxa2xx_mfp_suspend() and therefore it does apply!
That is exactly the meaning of MFP_LPM_KEEP_OUTPUT:
_If_ the pin is output, then _keep_ it output with the _runtime_ value.

> yet for some reason it is being allowed to prevent the
> setting of PGSR in the case of GPIO inputs.

Because, the PGSR is valid only for GPIO outputs!
and has nothing to do with inputs.

> 
> Consequently the (GPDR(i) & GPIO_bit(i)) test should be removed.

No, absolutely not!
This will change the meaning of the MFP_LPM_KEEP_OUTPUT symbol!
I have a feeling that you are trying to solve a problem that does not exist!

> 
>>>     361       
>>         if (GPLR(i) &
>> GPIO_bit(i))
>>>     362       
>>            
>> PGSR(gpio_to_bank(i)) |= GPIO_bit(i);
>>>     363       
>>         else
>>>     364       
>>            
>> PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);
>>>     365       
>>     }
>>>     366       
>> }
>>>     367    
>>>     368       
>> for (i = 0; i <= gpio_to_bank(pxa_last_gpio); i++) {
>>>     369    
>>>          ...
>>>     375       
>>     GPDR(i * 32) = gpdr_lpm[i];
>>>     376       
>> }
>>>     377       
>> return 0;
>>>     378    }
>>>
>>> However looking at pxa2xx_mfp_suspend() more closely it
>> is true that the
>>> GPDR registers are set before the PGSR values take
>> effect (when the
>>> processor enters sleep mode).
>>> That is no different from MFP_LPM_DRIVE_HIGH or
>> MFP_LPM_DRIVE_LOW though.
>>
>> Well, it is different, because the in case of
>> MFP_LPM_DRIVE_*,
>> the PGSR is set in the __mfp_config_gpio() function and has
>> a _known value_.
> 
> KEEP_OUTPUT does the same where GPIOs are configured as outputs, as is
> the case with corgi and hx4700.
> Your valid objection is for cases where GPIOs are configured as inputs.
> So I presume that removing the (GPDR(i) & GPIO_bit(i)) test will satisfy
> your objection.

again, no, because it will change the meaning of the MFP_LPM_KEEP_OUTPUT symbol!

> 
>>> So the presumption must be that the GPIOs configured as
>> DRIVE_HIGH,
>>> DRIVER_LOW and now KEEP_OUTPUT already have a valid
>> direction and value
>>> set at the point the GPDR registers are forced.
>>
>> Yes. That is exactly my point here.
>> MFP_LPM_DRIVE_* has the value determined and set.
>> MFP_LPM_KEEP_OUTPUT does not, but with your patch it will
>> effectively
>> force the GPIO to be output in sleep mode, but with no value
>> set!
> 
> Only in cases where the GPIO is used as an input in normal mode but
> configured to be an output in sleep mode.
> Removing the (GPDR(i) & GPIO_bit(i)) test addresses that case.

No, because, if the GPIO is input, you will configure it for output
and set some arbitrary value that is seen on the input!
This is bad!

> 
>>>> Why can't you use:
>>>> GPIO<n>_GPIO | MFP_LPM_DRIVE_<level>
>>>> ?
>>>
>>> The hx4700 has a couple of charging GPIOs which we want
>> to keep HIGH or
>>> LOW during sleep mode:
>>>
>>> % fgrep MFP_LPM_KEEP_OUTPUT arch/arm/mach-pxa/hx4700.c
>>>     GPIO72_GPIO |
>> MFP_LPM_KEEP_OUTPUT,    /* BQ24022_nCHARGE_EN
>> */
>>>     GPIO96_GPIO |
>> MFP_LPM_KEEP_OUTPUT,    /* BQ24022_ISET2 */
>>> % 
>>
>> I still, can't understand, why MFP_LPM_DRIVE_* does not do
>> the job for you...
>> What's the real problem with using MFP_LPM_DRIVE_*?
>> Can't you specify it for those GPIO72/96?
> 
> The feeling was that if the unit was charging in normal mode then it
> should remain charging in sleep mode, and if it wasn't then it should
> remain not charging.

Ok, finally we are talking about a real problem...
I see...
IMO, the best solution will be to add a generic GPIOx_GPIO_OUT
(as already proposed by Haojian) definition (where x is the
required GPIO number)for your used GPIOs and use it in MFP config
structure for hx4700 along with MFP_LPM_KEEP_OUTPUT.


Another solution would be (though a bit hackish):
GPIOx_GPIO | MFP_LPM_DRIVE_LOW | MFP_LPM_KEEP_OUTPUT

The above should work for you as you would expect - will set the GPIO
to be output low by default, then you will reconfigure it to the
appropriate level from your charger driver and the new level will be used
by the existing logic in pxa2xx_mfp_suspend().

> Actually that's an interim solution until the bootloader can take control
> of sleep mode charging, but we're not there yet.

still you don't want the turn on/off of the charger in the suspend sequence,
so IMO, it must also be fixed in Linux.
But, please, don't change the meaning of MFP_LPM_KEEP_OUTPUT!
Because, currently it is safe to use it also with inputs
and we don't want to break this assumption.

-- 
Regards,
Igor.

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

Hello Igor,

--- On Mon, 2/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Paul,
> 
> Can you, please, configure your mailer to _not_ wrap the
> text?
> It is rally hard to read it after a few replies...
> 
> On 04/01/12 20:56, Paul Parsons wrote:
> > Hello Igor,
> > 
> > --- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il>
> wrote:
> >> On 04/01/12 13:56, Paul Parsons
> >> wrote:
> >>> Hello Igor,
> >>>
> >>> --- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il>
> >> wrote:
> >>>> Hi Paul,
> >>>>
> >>>> On 03/31/12 15:20, Paul Parsons wrote:
> >>>>> Some MFP configurations specify
> >> MFP_LPM_KEEP_OUTPUT to
> >>>> preserve the gpio output
> >>>>> value (HIGH or LOW) during sleep mode.
> For
> >> example:
> >>>>>
> >>>>>? ?
> ???GPIO72_GPIO |
> >>>> MFP_LPM_KEEP_OUTPUT,
> >>>>>
> >>>>> Unfortunately MFP_LPM_KEEP_OUTPUT makes
> no
> >> special
> >>>> provision for setting the
> >>>>> sleep mode gpio direction, unlike
> >> MFP_LPM_DRIVE_HIGH
> >>>> and MFP_LPM_DRIVE_LOW.
> >>>>> Consequently MFP configurations of the
> form:
> >>>>>
> >>>>>? ?
> ???GPIO<n>_GPIO |
> >>>> MFP_LPM_KEEP_OUTPUT,
> >>>>>
> >>>>> will set the sleep mode gpio direction
> to
> >> INPUT.
> >>>>>
> >>>>> This patch forces the sleep mode gpio
> direction
> >> to
> >>>> OUTPUT in cases where
> >>>>> MFP_LPM_KEEP_OUTPUT was specified. This
> brings
> >>>> MFP_LPM_KEEP_OUTPUT into line
> >>>>> with MFP_LPM_DRIVE_HIGH and
> MFP_LPM_DRIVE_LOW.
> >>>>>
> >>>>> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
> >>>>> ---
> >>>>>
> >>>>> diff --git
> a/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>> b/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>>> index b0a8428..3b443199 100644
> >>>>> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>>> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>>> @@ -96,6 +96,9 @@ static int
> >> __mfp_config_gpio(unsigned
> >>>> gpio, unsigned long c)
> >>>>>? ? ? ? 
> >>? ? break;
> >>>>>? ? ? ? }
> >>>>>? ? 
> >>>>> +? ? if (c &
> >> MFP_LPM_KEEP_OUTPUT)
> >>>>> +? ? ? ? is_out =
> 1;
> >>>>> +
> >>>>
> >>>> MFP_LPM_KEEP_OUTPUT does not meant to be
> used to
> >> setup the
> >>>> mfp config,
> >>>
> >>> As far as I can see, MFP_LPM_KEEP_OUTPUT is
> *only* used
> >> to setup the
> >>> mfp config:
> >>>
> >>> % xzcat linux-3.4-rc1.tar.xz | fgrep -a
> >> MFP_LPM_KEEP_OUTPUT
> >>>? ???GPIO13_GPIO |
> >> MFP_LPM_KEEP_OUTPUT,? ? /*
> >> CORGI_GPIO_LED_ORANGE */
> >>>? ???GPIO38_GPIO |
> >> MFP_LPM_KEEP_OUTPUT,? ? /*
> CORGI_GPIO_CHRG_ON
> >> */
> >>>? ???GPIO43_GPIO |
> >> MFP_LPM_KEEP_OUTPUT,? ? /*
> >> CORGI_GPIO_CHRG_UKN */
> >>> #define MFP_LPM_KEEP_OUTPUT? ? (0x1
> >> << 25)
> >>>? ???/* set corresponding
> PGSR bit of
> >> those marked MFP_LPM_KEEP_OUTPUT */
> >>>? ? ? ???if
> >> ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT)
> &&
> >>> % 
> >>
> >> How can you be certain that you don't break corgi,
> with the
> >> proposed patch?
> > 
> > On corgi, the three GPIOs configured as
> MFP_LPM_KEEP_OUTPUT:
> >? ???CORGI_GPIO_LED_ORANGE
> >? ???CORGI_GPIO_CHRG_ON
> >? ???CORGI_GPIO_CHRG_UKN
> > are all used as outputs.
> 
> Have you verified it, because currently MFP_LPM_KEEP_OUTPUT
> does not do a direction configuration... and IMO, it should
> not!

Yes, all 3 GPIOs are configured as outputs:
CORGI_GPIO_LED_ORANGE in drivers/leds/leds-gpio.c
CORGI_GPIO_CHRG_ON in arch/arm/mach-pxa/corgi_pm.c
CORGI_GPIO_CHRG_UKN in arch/arm/mach-pxa/corgi_pm.c

> 
> > 
> >>>> but for pins that have been configured for
> output
> >> by
> >>>> gpio_direction_out().
> >>>> (Hint: *_KEEP_*).
> >>>
> >>> MFP_LPM_KEEP_OUTPUT only takes effect when the
> system
> >> enters sleep mode,
> >>> at which point the GPIO directions (GPDR
> registers) are
> >> forced from the
> >>> values privately stored in gpdr_lpm[].
> >>
> >> Correct, but with your patch, the direction will be
> forced
> >> to output,
> >> but the value will be undefined...
> > 
> > The values will be defined in pxa2xx_mfp_suspend() when
> it sets the PGSR
> > registers. Or at least they will be on corgi and
> hx4700, but see below...
> 
> No, they will not, because the direction can be input...
> 
> > 
> >>> As far as I can tell MFP_LPM_KEEP_OUTPUT has no
> direct
> >> connection to
> >>> gpio_direction_output().
> >>
> >> Well, it has... in the pxa2xx_mfp_suspend()
> function you've
> >> pasted below.
> >>
> >>>
> >>>> Also, I don't think this is a good idea,
> because
> >> the patch
> >>>> allows a pin
> >>>> be configured as output, but does not
> forces a
> >> value
> >>>> (high/low)
> >>>> and this is not safe.
> >>>
> >>> A value does not need to be provided until the
> system
> >> enters sleep mode,
> >>> at which point the value is provided via the
> PGSR
> >> registers.
> >>
> >> Yes, but you don't force the PGSR to be set...
> >> and that is the problem.
> > 
> > OK, see below...
> > 
> >>>? ???353? ? static
> int
> >> pxa2xx_mfp_suspend(void)
> >>>? ???354? ? {
> >>>? ???355? ?
> ???
> >> int i;
> >>>? ???356? ? 
> >>>? ???357? ?
> ???
> >> /* set corresponding PGSR bit of those marked
> >> MFP_LPM_KEEP_OUTPUT */
> >>>? ???358? ?
> ???
> >> for (i = 0; i < pxa_last_gpio; i++) {
> >>>? ???359? ?
> ???
> >>? ???if ((gpio_desc[i].config
> &
> >> MFP_LPM_KEEP_OUTPUT) &&
> >>>? ???360? ?
> ???
> >>? ? ? ???(GPDR(i)
> &
> >> GPIO_bit(i))) {
> >>
> >> Look at the check above...
> >> the PGSR only gets set for the GPIOs configured to
> output
> >> (presumably by gpio_direction_output() function).
> > 
> > OK, I understand your objection here.
> > 
> > My feeling is that the (GPDR(i) & GPIO_bit(i)) test
> is unwanted because
> > the current state of GPDR (via gpio_direction_output())
> does not apply in
> > sleep mode,
> 
> It gets copied to PGSR in the pxa2xx_mfp_suspend() and
> therefore it does apply!
> That is exactly the meaning of MFP_LPM_KEEP_OUTPUT:
> _If_ the pin is output, then _keep_ it output with the
> _runtime_ value.

OK, I see, your interpretation is that MFP_LPM_KEEP_OUTPUT is optional:
it should not apply if the pin is an input at the time
pxa2xx_mfp_suspend() is called.
My interpretation was that MFP_LPM_KEEP_OUTPUT is mandatory: it always
applies regardless of the current pin direction, exactly like
MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.

If your interpretation is correct then DRIVE_HIGH and DRIVE_LOW will
likewise need to check the state of GPDR at the time
pxa2xx_mfp_suspend() is called.
If my interpretation is correct then having KEEP_OUTPUT ignore the state
of GPDR in pxa2xx_mfp_suspend() is the correct thing to do.

Do you agree that KEEP_OUTPUT, DRIVE_HIGH and DRIVE_LOW should all be
consistent in being either optional or mandatory sleep mode
configurations?

> 
> > yet for some reason it is being allowed to prevent the
> > setting of PGSR in the case of GPIO inputs.
> 
> Because, the PGSR is valid only for GPIO outputs!
> and has nothing to do with inputs.

I know, it makes no sense to specify KEEP_OUTPUT, DRIVE_HIGH or DRIVE_LOW
for GPIO inputs.
But since DRIVE_HIGH or DRIVE_LOW currently don't prevent that, the
presumption must be that GPIOs configured as DRIVE_HIGH or DRIVE_LOW are
being used as outputs. Likewise for KEEP_OUTPUT?

> 
> > 
> > Consequently the (GPDR(i) & GPIO_bit(i)) test
> should be removed.
> 
> No, absolutely not!
> This will change the meaning of the MFP_LPM_KEEP_OUTPUT
> symbol!

Only if you interpret KEEP_OUTPUT as an optional sleep mode configuration.

> I have a feeling that you are trying to solve a problem that
> does not exist!

The problem is that MFP configurations of the form:
    GPIO<n>_GPIO | MFP_LPM_KEEP_OUTPUT,
will always set the sleep mode gpio direction to INPUT.
That is the case with corgi and would also be the case with hx4700.
That is the problem which needs to be solved.

> 
> > 
> >>>? ???361? ?
> ???
> >>? ? ? ???if (GPLR(i)
> &
> >> GPIO_bit(i))
> >>>? ???362? ?
> ???
> >>? ? ? ? ? ? 
> >> PGSR(gpio_to_bank(i)) |= GPIO_bit(i);
> >>>? ???363? ?
> ???
> >>? ? ? ???else
> >>>? ???364? ?
> ???
> >>? ? ? ? ? ? 
> >> PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);
> >>>? ???365? ?
> ???
> >>? ???}
> >>>? ???366? ?
> ???
> >> }
> >>>? ???367? ? 
> >>>? ???368? ?
> ???
> >> for (i = 0; i <= gpio_to_bank(pxa_last_gpio);
> i++) {
> >>>? ???369? ? 
> >>>? ? ? ? ? ...
> >>>? ???375? ?
> ???
> >>? ???GPDR(i * 32) =
> gpdr_lpm[i];
> >>>? ???376? ?
> ???
> >> }
> >>>? ???377? ?
> ???
> >> return 0;
> >>>? ???378? ? }
> >>>
> >>> However looking at pxa2xx_mfp_suspend() more
> closely it
> >> is true that the
> >>> GPDR registers are set before the PGSR values
> take
> >> effect (when the
> >>> processor enters sleep mode).
> >>> That is no different from MFP_LPM_DRIVE_HIGH
> or
> >> MFP_LPM_DRIVE_LOW though.
> >>
> >> Well, it is different, because the in case of
> >> MFP_LPM_DRIVE_*,
> >> the PGSR is set in the __mfp_config_gpio() function
> and has
> >> a _known value_.
> > 
> > KEEP_OUTPUT does the same where GPIOs are configured as
> outputs, as is
> > the case with corgi and hx4700.
> > Your valid objection is for cases where GPIOs are
> configured as inputs.
> > So I presume that removing the (GPDR(i) &
> GPIO_bit(i)) test will satisfy
> > your objection.
> 
> again, no, because it will change the meaning of the
> MFP_LPM_KEEP_OUTPUT symbol!
> 
> > 
> >>> So the presumption must be that the GPIOs
> configured as
> >> DRIVE_HIGH,
> >>> DRIVER_LOW and now KEEP_OUTPUT already have a
> valid
> >> direction and value
> >>> set at the point the GPDR registers are
> forced.
> >>
> >> Yes. That is exactly my point here.
> >> MFP_LPM_DRIVE_* has the value determined and set.
> >> MFP_LPM_KEEP_OUTPUT does not, but with your patch
> it will
> >> effectively
> >> force the GPIO to be output in sleep mode, but with
> no value
> >> set!
> > 
> > Only in cases where the GPIO is used as an input in
> normal mode but
> > configured to be an output in sleep mode.
> > Removing the (GPDR(i) & GPIO_bit(i)) test addresses
> that case.
> 
> No, because, if the GPIO is input, you will configure it for
> output
> and set some arbitrary value that is seen on the input!
> This is bad!
> 
> > 
> >>>> Why can't you use:
> >>>> GPIO<n>_GPIO |
> MFP_LPM_DRIVE_<level>
> >>>> ?
> >>>
> >>> The hx4700 has a couple of charging GPIOs which
> we want
> >> to keep HIGH or
> >>> LOW during sleep mode:
> >>>
> >>> % fgrep MFP_LPM_KEEP_OUTPUT
> arch/arm/mach-pxa/hx4700.c
> >>>? ???GPIO72_GPIO |
> >> MFP_LPM_KEEP_OUTPUT,? ? /*
> BQ24022_nCHARGE_EN
> >> */
> >>>? ???GPIO96_GPIO |
> >> MFP_LPM_KEEP_OUTPUT,? ? /* BQ24022_ISET2
> */
> >>> % 
> >>
> >> I still, can't understand, why MFP_LPM_DRIVE_* does
> not do
> >> the job for you...
> >> What's the real problem with using
> MFP_LPM_DRIVE_*?
> >> Can't you specify it for those GPIO72/96?
> > 
> > The feeling was that if the unit was charging in normal
> mode then it
> > should remain charging in sleep mode, and if it wasn't
> then it should
> > remain not charging.
> 
> Ok, finally we are talking about a real problem...
> I see...
> IMO, the best solution will be to add a generic
> GPIOx_GPIO_OUT
> (as already proposed by Haojian) definition (where x is the
> required GPIO number)for your used GPIOs and use it in MFP
> config
> structure for hx4700 along with MFP_LPM_KEEP_OUTPUT.
> 
> 
> Another solution would be (though a bit hackish):
> GPIOx_GPIO | MFP_LPM_DRIVE_LOW | MFP_LPM_KEEP_OUTPUT
> 
> The above should work for you as you would expect - will set
> the GPIO
> to be output low by default, then you will reconfigure it to
> the
> appropriate level from your charger driver and the new level
> will be used
> by the existing logic in pxa2xx_mfp_suspend().

Those solutions are fair enough.
Can we first answer the question of whether KEEP_OUTPUT, DRIVE_HIGH and
DRIVE_LOW are optional or mandatory sleep mode configurations. That may
determine the way forward.

> 
> > Actually that's an interim solution until the
> bootloader can take control
> > of sleep mode charging, but we're not there yet.
> 
> still you don't want the turn on/off of the charger in the
> suspend sequence,
> so IMO, it must also be fixed in Linux.
> But, please, don't change the meaning of
> MFP_LPM_KEEP_OUTPUT!
> Because, currently it is safe to use it also with inputs
> and we don't want to break this assumption.

Regards,
Paul

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Igor Grinberg]

On 04/02/12 14:30, Paul Parsons wrote:
> Hello Igor,
> 
> --- On Mon, 2/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> Hi Paul,
>>
>> Can you, please, configure your mailer to _not_ wrap the
>> text?
>> It is rally hard to read it after a few replies...
>>
>> On 04/01/12 20:56, Paul Parsons wrote:
>>> Hello Igor,
>>>
>>> --- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il>
>> wrote:
>>>> On 04/01/12 13:56, Paul Parsons
>>>> wrote:
>>>>> Hello Igor,
>>>>>
>>>>> --- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il>
>>>> wrote:
>>>>>> Hi Paul,
>>>>>>
>>>>>> On 03/31/12 15:20, Paul Parsons wrote:
>>>>>>> Some MFP configurations specify
>>>> MFP_LPM_KEEP_OUTPUT to
>>>>>> preserve the gpio output
>>>>>>> value (HIGH or LOW) during sleep mode.
>> For
>>>> example:
>>>>>>>
>>>>>>>    
>>    GPIO72_GPIO |
>>>>>> MFP_LPM_KEEP_OUTPUT,
>>>>>>>
>>>>>>> Unfortunately MFP_LPM_KEEP_OUTPUT makes
>> no
>>>> special
>>>>>> provision for setting the
>>>>>>> sleep mode gpio direction, unlike
>>>> MFP_LPM_DRIVE_HIGH
>>>>>> and MFP_LPM_DRIVE_LOW.
>>>>>>> Consequently MFP configurations of the
>> form:
>>>>>>>
>>>>>>>    
>>    GPIO<n>_GPIO |
>>>>>> MFP_LPM_KEEP_OUTPUT,
>>>>>>>
>>>>>>> will set the sleep mode gpio direction
>> to
>>>> INPUT.
>>>>>>>
>>>>>>> This patch forces the sleep mode gpio
>> direction
>>>> to
>>>>>> OUTPUT in cases where
>>>>>>> MFP_LPM_KEEP_OUTPUT was specified. This
>> brings
>>>>>> MFP_LPM_KEEP_OUTPUT into line
>>>>>>> with MFP_LPM_DRIVE_HIGH and
>> MFP_LPM_DRIVE_LOW.
>>>>>>>
>>>>>>> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
>>>>>>> ---
>>>>>>>
>>>>>>> diff --git
>> a/arch/arm/mach-pxa/mfp-pxa2xx.c
>>>>>> b/arch/arm/mach-pxa/mfp-pxa2xx.c
>>>>>>> index b0a8428..3b443199 100644
>>>>>>> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
>>>>>>> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
>>>>>>> @@ -96,6 +96,9 @@ static int
>>>> __mfp_config_gpio(unsigned
>>>>>> gpio, unsigned long c)
>>>>>>>         
>>>>     break;
>>>>>>>         }
>>>>>>>     
>>>>>>> +    if (c &
>>>> MFP_LPM_KEEP_OUTPUT)
>>>>>>> +        is_out =
>> 1;
>>>>>>> +
>>>>>>
>>>>>> MFP_LPM_KEEP_OUTPUT does not meant to be
>> used to
>>>> setup the
>>>>>> mfp config,
>>>>>
>>>>> As far as I can see, MFP_LPM_KEEP_OUTPUT is
>> *only* used
>>>> to setup the
>>>>> mfp config:
>>>>>
>>>>> % xzcat linux-3.4-rc1.tar.xz | fgrep -a
>>>> MFP_LPM_KEEP_OUTPUT
>>>>>      GPIO13_GPIO |
>>>> MFP_LPM_KEEP_OUTPUT,    /*
>>>> CORGI_GPIO_LED_ORANGE */
>>>>>      GPIO38_GPIO |
>>>> MFP_LPM_KEEP_OUTPUT,    /*
>> CORGI_GPIO_CHRG_ON
>>>> */
>>>>>      GPIO43_GPIO |
>>>> MFP_LPM_KEEP_OUTPUT,    /*
>>>> CORGI_GPIO_CHRG_UKN */
>>>>> #define MFP_LPM_KEEP_OUTPUT    (0x1
>>>> << 25)
>>>>>      /* set corresponding
>> PGSR bit of
>>>> those marked MFP_LPM_KEEP_OUTPUT */
>>>>>          if
>>>> ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT)
>> &&
>>>>> % 
>>>>
>>>> How can you be certain that you don't break corgi,
>> with the
>>>> proposed patch?
>>>
>>> On corgi, the three GPIOs configured as
>> MFP_LPM_KEEP_OUTPUT:
>>>      CORGI_GPIO_LED_ORANGE
>>>      CORGI_GPIO_CHRG_ON
>>>      CORGI_GPIO_CHRG_UKN
>>> are all used as outputs.
>>
>> Have you verified it, because currently MFP_LPM_KEEP_OUTPUT
>> does not do a direction configuration... and IMO, it should
>> not!
> 
> Yes, all 3 GPIOs are configured as outputs:
> CORGI_GPIO_LED_ORANGE in drivers/leds/leds-gpio.c
> CORGI_GPIO_CHRG_ON in arch/arm/mach-pxa/corgi_pm.c
> CORGI_GPIO_CHRG_UKN in arch/arm/mach-pxa/corgi_pm.c

Good, then they will be kept output also in the suspend mode,
as for current implementation.

> 
>>
>>>
>>>>>> but for pins that have been configured for
>> output
>>>> by
>>>>>> gpio_direction_out().
>>>>>> (Hint: *_KEEP_*).
>>>>>
>>>>> MFP_LPM_KEEP_OUTPUT only takes effect when the
>> system
>>>> enters sleep mode,
>>>>> at which point the GPIO directions (GPDR
>> registers) are
>>>> forced from the
>>>>> values privately stored in gpdr_lpm[].
>>>>
>>>> Correct, but with your patch, the direction will be
>> forced
>>>> to output,
>>>> but the value will be undefined...
>>>
>>> The values will be defined in pxa2xx_mfp_suspend() when
>> it sets the PGSR
>>> registers. Or at least they will be on corgi and
>> hx4700, but see below...
>>
>> No, they will not, because the direction can be input...
>>
>>>
>>>>> As far as I can tell MFP_LPM_KEEP_OUTPUT has no
>> direct
>>>> connection to
>>>>> gpio_direction_output().
>>>>
>>>> Well, it has... in the pxa2xx_mfp_suspend()
>> function you've
>>>> pasted below.
>>>>
>>>>>
>>>>>> Also, I don't think this is a good idea,
>> because
>>>> the patch
>>>>>> allows a pin
>>>>>> be configured as output, but does not
>> forces a
>>>> value
>>>>>> (high/low)
>>>>>> and this is not safe.
>>>>>
>>>>> A value does not need to be provided until the
>> system
>>>> enters sleep mode,
>>>>> at which point the value is provided via the
>> PGSR
>>>> registers.
>>>>
>>>> Yes, but you don't force the PGSR to be set...
>>>> and that is the problem.
>>>
>>> OK, see below...
>>>
>>>>>      353    static
>> int
>>>> pxa2xx_mfp_suspend(void)
>>>>>      354    {
>>>>>      355   
>>    
>>>> int i;
>>>>>      356    
>>>>>      357   
>>    
>>>> /* set corresponding PGSR bit of those marked
>>>> MFP_LPM_KEEP_OUTPUT */
>>>>>      358   
>>    
>>>> for (i = 0; i < pxa_last_gpio; i++) {
>>>>>      359   
>>    
>>>>      if ((gpio_desc[i].config
>> &
>>>> MFP_LPM_KEEP_OUTPUT) &&
>>>>>      360   
>>    
>>>>          (GPDR(i)
>> &
>>>> GPIO_bit(i))) {
>>>>
>>>> Look at the check above...
>>>> the PGSR only gets set for the GPIOs configured to
>> output
>>>> (presumably by gpio_direction_output() function).
>>>
>>> OK, I understand your objection here.
>>>
>>> My feeling is that the (GPDR(i) & GPIO_bit(i)) test
>> is unwanted because
>>> the current state of GPDR (via gpio_direction_output())
>> does not apply in
>>> sleep mode,
>>
>> It gets copied to PGSR in the pxa2xx_mfp_suspend() and
>> therefore it does apply!
>> That is exactly the meaning of MFP_LPM_KEEP_OUTPUT:
>> _If_ the pin is output, then _keep_ it output with the
>> _runtime_ value.
> 
> OK, I see, your interpretation is that MFP_LPM_KEEP_OUTPUT is optional:
> it should not apply if the pin is an input at the time
> pxa2xx_mfp_suspend() is called.
> My interpretation was that MFP_LPM_KEEP_OUTPUT is mandatory: it always
> applies regardless of the current pin direction, exactly like
> MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.
> 
> If your interpretation is correct then DRIVE_HIGH and DRIVE_LOW will
> likewise need to check the state of GPDR at the time
> pxa2xx_mfp_suspend() is called.
> If my interpretation is correct then having KEEP_OUTPUT ignore the state
> of GPDR in pxa2xx_mfp_suspend() is the correct thing to do.
> 
> Do you agree that KEEP_OUTPUT, DRIVE_HIGH and DRIVE_LOW should all be
> consistent in being either optional or mandatory sleep mode
> configurations?

No, according to current implementation (and IMO, it is _not_ a bug):
DRIVE_HIGH and DRIVE_LOW are mandatory,
KEEP_OUTPUT is optional.

> 
>>
>>> yet for some reason it is being allowed to prevent the
>>> setting of PGSR in the case of GPIO inputs.
>>
>> Because, the PGSR is valid only for GPIO outputs!
>> and has nothing to do with inputs.
> 
> I know, it makes no sense to specify KEEP_OUTPUT, DRIVE_HIGH or DRIVE_LOW
> for GPIO inputs.

That's the point, that it does make sense for certain cases,
when you want the GPIO to be bidirectional, but output in suspend.
This allows certain flexibility.

> But since DRIVE_HIGH or DRIVE_LOW currently don't prevent that,

Don't prevent what? DRIVE_HIGH and DRIVE_LOW allow you to do what
ever you want with the GPIO in runtime, but have it strictly configured
for the suspend.

> the
> presumption must be that GPIOs configured as DRIVE_HIGH or DRIVE_LOW are
> being used as outputs. Likewise for KEEP_OUTPUT?

KEEP_OUTPUT only affects the GPIOs configured for output in runtime
and let you keep the last output value.

> 
>>
>>>
>>> Consequently the (GPDR(i) & GPIO_bit(i)) test
>> should be removed.
>>
>> No, absolutely not!
>> This will change the meaning of the MFP_LPM_KEEP_OUTPUT
>> symbol!
> 
> Only if you interpret KEEP_OUTPUT as an optional sleep mode configuration.

That what the current implementation does and IMO, it is the right thing.

> 
>> I have a feeling that you are trying to solve a problem that
>> does not exist!
> 
> The problem is that MFP configurations of the form:
>     GPIO<n>_GPIO | MFP_LPM_KEEP_OUTPUT,
> will always set the sleep mode gpio direction to INPUT.
> That is the case with corgi and would also be the case with hx4700.
> That is the problem which needs to be solved.

Yes, it is just there is no h/w MFP in PXA2xx and all the stuff done in s/w.
That is the reason it is a bit mess.
IMO, the cleanest solution would be to have GPIO<n>_GPIO_OUT for each
GPIO that needs to be configured as output instead of breaking the
already fragile MFP logic.

> 
>>
>>>
>>>>>      361   
>>    
>>>>          if (GPLR(i)
>> &
>>>> GPIO_bit(i))
>>>>>      362   
>>    
>>>>             
>>>> PGSR(gpio_to_bank(i)) |= GPIO_bit(i);
>>>>>      363   
>>    
>>>>          else
>>>>>      364   
>>    
>>>>             
>>>> PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);
>>>>>      365   
>>    
>>>>      }
>>>>>      366   
>>    
>>>> }
>>>>>      367    
>>>>>      368   
>>    
>>>> for (i = 0; i <= gpio_to_bank(pxa_last_gpio);
>> i++) {
>>>>>      369    
>>>>>           ...
>>>>>      375   
>>    
>>>>      GPDR(i * 32) =
>> gpdr_lpm[i];
>>>>>      376   
>>    
>>>> }
>>>>>      377   
>>    
>>>> return 0;
>>>>>      378    }
>>>>>
>>>>> However looking at pxa2xx_mfp_suspend() more
>> closely it
>>>> is true that the
>>>>> GPDR registers are set before the PGSR values
>> take
>>>> effect (when the
>>>>> processor enters sleep mode).
>>>>> That is no different from MFP_LPM_DRIVE_HIGH
>> or
>>>> MFP_LPM_DRIVE_LOW though.
>>>>
>>>> Well, it is different, because the in case of
>>>> MFP_LPM_DRIVE_*,
>>>> the PGSR is set in the __mfp_config_gpio() function
>> and has
>>>> a _known value_.
>>>
>>> KEEP_OUTPUT does the same where GPIOs are configured as
>> outputs, as is
>>> the case with corgi and hx4700.
>>> Your valid objection is for cases where GPIOs are
>> configured as inputs.
>>> So I presume that removing the (GPDR(i) &
>> GPIO_bit(i)) test will satisfy
>>> your objection.
>>
>> again, no, because it will change the meaning of the
>> MFP_LPM_KEEP_OUTPUT symbol!
>>
>>>
>>>>> So the presumption must be that the GPIOs
>> configured as
>>>> DRIVE_HIGH,
>>>>> DRIVER_LOW and now KEEP_OUTPUT already have a
>> valid
>>>> direction and value
>>>>> set at the point the GPDR registers are
>> forced.
>>>>
>>>> Yes. That is exactly my point here.
>>>> MFP_LPM_DRIVE_* has the value determined and set.
>>>> MFP_LPM_KEEP_OUTPUT does not, but with your patch
>> it will
>>>> effectively
>>>> force the GPIO to be output in sleep mode, but with
>> no value
>>>> set!
>>>
>>> Only in cases where the GPIO is used as an input in
>> normal mode but
>>> configured to be an output in sleep mode.
>>> Removing the (GPDR(i) & GPIO_bit(i)) test addresses
>> that case.
>>
>> No, because, if the GPIO is input, you will configure it for
>> output
>> and set some arbitrary value that is seen on the input!
>> This is bad!
>>
>>>
>>>>>> Why can't you use:
>>>>>> GPIO<n>_GPIO |
>> MFP_LPM_DRIVE_<level>
>>>>>> ?
>>>>>
>>>>> The hx4700 has a couple of charging GPIOs which
>> we want
>>>> to keep HIGH or
>>>>> LOW during sleep mode:
>>>>>
>>>>> % fgrep MFP_LPM_KEEP_OUTPUT
>> arch/arm/mach-pxa/hx4700.c
>>>>>      GPIO72_GPIO |
>>>> MFP_LPM_KEEP_OUTPUT,    /*
>> BQ24022_nCHARGE_EN
>>>> */
>>>>>      GPIO96_GPIO |
>>>> MFP_LPM_KEEP_OUTPUT,    /* BQ24022_ISET2
>> */
>>>>> % 
>>>>
>>>> I still, can't understand, why MFP_LPM_DRIVE_* does
>> not do
>>>> the job for you...
>>>> What's the real problem with using
>> MFP_LPM_DRIVE_*?
>>>> Can't you specify it for those GPIO72/96?
>>>
>>> The feeling was that if the unit was charging in normal
>> mode then it
>>> should remain charging in sleep mode, and if it wasn't
>> then it should
>>> remain not charging.
>>
>> Ok, finally we are talking about a real problem...
>> I see...
>> IMO, the best solution will be to add a generic
>> GPIOx_GPIO_OUT
>> (as already proposed by Haojian) definition (where x is the
>> required GPIO number)for your used GPIOs and use it in MFP
>> config
>> structure for hx4700 along with MFP_LPM_KEEP_OUTPUT.
>>
>>
>> Another solution would be (though a bit hackish):
>> GPIOx_GPIO | MFP_LPM_DRIVE_LOW | MFP_LPM_KEEP_OUTPUT
>>
>> The above should work for you as you would expect - will set
>> the GPIO
>> to be output low by default, then you will reconfigure it to
>> the
>> appropriate level from your charger driver and the new level
>> will be used
>> by the existing logic in pxa2xx_mfp_suspend().
> 
> Those solutions are fair enough.
> Can we first answer the question of whether KEEP_OUTPUT, DRIVE_HIGH and
> DRIVE_LOW are optional or mandatory sleep mode configurations. That may
> determine the way forward.

Well for the DRIVE_HIGH and DRIVE_LOW, the names speak for them selfs -
MultiFunctionalPin_LowPowerManager_DRIVE_<HIGH|LOW> and there is no
questions arise.
For the MFP_LPM_KEEP_OUTPUT - is PXA2xx specific, apparently it is
confusing and probably has to have a comment explaining the meaning.

>From git log:
---------------cut---------------------
commit 1106143d7ab43ba07678c88c85417df219354ae8
Author: Eric Miao <eric.y.miao@gmail.com>
Date:   Mon Jan 11 21:25:15 2010 +0800

    [ARM] pxa: add MFP_LPM_KEEP_OUTPUT flag to pin config
    
    Some pins are expected to keep their last level during suspend, and
    introduce MFP_LPM_KEEP_OUTPUT for this.
    
    Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
---------------cut---------------------

The key words above are: "keep their last level".

> 
>>
>>> Actually that's an interim solution until the
>> bootloader can take control
>>> of sleep mode charging, but we're not there yet.
>>
>> still you don't want the turn on/off of the charger in the
>> suspend sequence,
>> so IMO, it must also be fixed in Linux.
>> But, please, don't change the meaning of
>> MFP_LPM_KEEP_OUTPUT!
>> Because, currently it is safe to use it also with inputs
>> and we don't want to break this assumption.
> 
> Regards,
> Paul
> 

-- 
Regards,
Igor.

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

Hello Igor,

--- On Mon, 2/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
> On 04/02/12 14:30, Paul Parsons
> wrote:
> > Hello Igor,
> > 
> > --- On Mon, 2/4/12, Igor Grinberg <grinberg@compulab.co.il>
> wrote:
> >> Hi Paul,
> >>
> >> Can you, please, configure your mailer to _not_
> wrap the
> >> text?
> >> It is rally hard to read it after a few replies...
> >>
> >> On 04/01/12 20:56, Paul Parsons wrote:
> >>> Hello Igor,
> >>>
> >>> --- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il>
> >> wrote:
> >>>> On 04/01/12 13:56, Paul Parsons
> >>>> wrote:
> >>>>> Hello Igor,
> >>>>>
> >>>>> --- On Sun, 1/4/12, Igor Grinberg
> <grinberg@compulab.co.il>
> >>>> wrote:
> >>>>>> Hi Paul,
> >>>>>>
> >>>>>> On 03/31/12 15:20, Paul Parsons
> wrote:
> >>>>>>> Some MFP configurations
> specify
> >>>> MFP_LPM_KEEP_OUTPUT to
> >>>>>> preserve the gpio output
> >>>>>>> value (HIGH or LOW) during
> sleep mode.
> >> For
> >>>> example:
> >>>>>>>
> >>>>>>>? ? 
> >>? ? GPIO72_GPIO |
> >>>>>> MFP_LPM_KEEP_OUTPUT,
> >>>>>>>
> >>>>>>> Unfortunately
> MFP_LPM_KEEP_OUTPUT makes
> >> no
> >>>> special
> >>>>>> provision for setting the
> >>>>>>> sleep mode gpio direction,
> unlike
> >>>> MFP_LPM_DRIVE_HIGH
> >>>>>> and MFP_LPM_DRIVE_LOW.
> >>>>>>> Consequently MFP configurations
> of the
> >> form:
> >>>>>>>
> >>>>>>>? ? 
> >>? ? GPIO<n>_GPIO |
> >>>>>> MFP_LPM_KEEP_OUTPUT,
> >>>>>>>
> >>>>>>> will set the sleep mode gpio
> direction
> >> to
> >>>> INPUT.
> >>>>>>>
> >>>>>>> This patch forces the sleep
> mode gpio
> >> direction
> >>>> to
> >>>>>> OUTPUT in cases where
> >>>>>>> MFP_LPM_KEEP_OUTPUT was
> specified. This
> >> brings
> >>>>>> MFP_LPM_KEEP_OUTPUT into line
> >>>>>>> with MFP_LPM_DRIVE_HIGH and
> >> MFP_LPM_DRIVE_LOW.
> >>>>>>>
> >>>>>>> Signed-off-by: Paul Parsons
> <lost.distance@yahoo.com>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> diff --git
> >> a/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>>>> b/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>>>>> index b0a8428..3b443199 100644
> >>>>>>> ---
> a/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>>>>> +++
> b/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>>>>> @@ -96,6 +96,9 @@ static int
> >>>> __mfp_config_gpio(unsigned
> >>>>>> gpio, unsigned long c)
> >>>>>>>? ? ?
> ???
> >>>>? ???break;
> >>>>>>>? ? ?
> ???}
> >>>>>>>? ???
> >>>>>>> +? ? if (c &
> >>>> MFP_LPM_KEEP_OUTPUT)
> >>>>>>> +? ? ? ?
> is_out =
> >> 1;
> >>>>>>> +
> >>>>>>
> >>>>>> MFP_LPM_KEEP_OUTPUT does not meant
> to be
> >> used to
> >>>> setup the
> >>>>>> mfp config,
> >>>>>
> >>>>> As far as I can see,
> MFP_LPM_KEEP_OUTPUT is
> >> *only* used
> >>>> to setup the
> >>>>> mfp config:
> >>>>>
> >>>>> % xzcat linux-3.4-rc1.tar.xz | fgrep
> -a
> >>>> MFP_LPM_KEEP_OUTPUT
> >>>>>? ? ? GPIO13_GPIO |
> >>>> MFP_LPM_KEEP_OUTPUT,? ? /*
> >>>> CORGI_GPIO_LED_ORANGE */
> >>>>>? ? ? GPIO38_GPIO |
> >>>> MFP_LPM_KEEP_OUTPUT,? ? /*
> >> CORGI_GPIO_CHRG_ON
> >>>> */
> >>>>>? ? ? GPIO43_GPIO |
> >>>> MFP_LPM_KEEP_OUTPUT,? ? /*
> >>>> CORGI_GPIO_CHRG_UKN */
> >>>>> #define MFP_LPM_KEEP_OUTPUT?
> ? (0x1
> >>>> << 25)
> >>>>>? ? ? /* set
> corresponding
> >> PGSR bit of
> >>>> those marked MFP_LPM_KEEP_OUTPUT */
> >>>>>? ? ? ? ? if
> >>>> ((gpio_desc[i].config &
> MFP_LPM_KEEP_OUTPUT)
> >> &&
> >>>>> % 
> >>>>
> >>>> How can you be certain that you don't break
> corgi,
> >> with the
> >>>> proposed patch?
> >>>
> >>> On corgi, the three GPIOs configured as
> >> MFP_LPM_KEEP_OUTPUT:
> >>>? ? ? CORGI_GPIO_LED_ORANGE
> >>>? ? ? CORGI_GPIO_CHRG_ON
> >>>? ? ? CORGI_GPIO_CHRG_UKN
> >>> are all used as outputs.
> >>
> >> Have you verified it, because currently
> MFP_LPM_KEEP_OUTPUT
> >> does not do a direction configuration... and IMO,
> it should
> >> not!
> > 
> > Yes, all 3 GPIOs are configured as outputs:
> > CORGI_GPIO_LED_ORANGE in drivers/leds/leds-gpio.c
> > CORGI_GPIO_CHRG_ON in arch/arm/mach-pxa/corgi_pm.c
> > CORGI_GPIO_CHRG_UKN in arch/arm/mach-pxa/corgi_pm.c
> 
> Good, then they will be kept output also in the suspend
> mode,
> as for current implementation.
> 
> > 
> >>
> >>>
> >>>>>> but for pins that have been
> configured for
> >> output
> >>>> by
> >>>>>> gpio_direction_out().
> >>>>>> (Hint: *_KEEP_*).
> >>>>>
> >>>>> MFP_LPM_KEEP_OUTPUT only takes effect
> when the
> >> system
> >>>> enters sleep mode,
> >>>>> at which point the GPIO directions
> (GPDR
> >> registers) are
> >>>> forced from the
> >>>>> values privately stored in gpdr_lpm[].
> >>>>
> >>>> Correct, but with your patch, the direction
> will be
> >> forced
> >>>> to output,
> >>>> but the value will be undefined...
> >>>
> >>> The values will be defined in
> pxa2xx_mfp_suspend() when
> >> it sets the PGSR
> >>> registers. Or at least they will be on corgi
> and
> >> hx4700, but see below...
> >>
> >> No, they will not, because the direction can be
> input...
> >>
> >>>
> >>>>> As far as I can tell
> MFP_LPM_KEEP_OUTPUT has no
> >> direct
> >>>> connection to
> >>>>> gpio_direction_output().
> >>>>
> >>>> Well, it has... in the
> pxa2xx_mfp_suspend()
> >> function you've
> >>>> pasted below.
> >>>>
> >>>>>
> >>>>>> Also, I don't think this is a good
> idea,
> >> because
> >>>> the patch
> >>>>>> allows a pin
> >>>>>> be configured as output, but does
> not
> >> forces a
> >>>> value
> >>>>>> (high/low)
> >>>>>> and this is not safe.
> >>>>>
> >>>>> A value does not need to be provided
> until the
> >> system
> >>>> enters sleep mode,
> >>>>> at which point the value is provided
> via the
> >> PGSR
> >>>> registers.
> >>>>
> >>>> Yes, but you don't force the PGSR to be
> set...
> >>>> and that is the problem.
> >>>
> >>> OK, see below...
> >>>
> >>>>>? ? ? 353? ?
> static
> >> int
> >>>> pxa2xx_mfp_suspend(void)
> >>>>>? ? ? 354? ? {
> >>>>>? ? ?
> 355???
> >>? ? 
> >>>> int i;
> >>>>>? ? ? 356? ? 
> >>>>>? ? ?
> 357???
> >>? ? 
> >>>> /* set corresponding PGSR bit of those
> marked
> >>>> MFP_LPM_KEEP_OUTPUT */
> >>>>>? ? ?
> 358???
> >>? ? 
> >>>> for (i = 0; i < pxa_last_gpio; i++) {
> >>>>>? ? ?
> 359???
> >>? ? 
> >>>>? ? ? if
> ((gpio_desc[i].config
> >> &
> >>>> MFP_LPM_KEEP_OUTPUT) &&
> >>>>>? ? ?
> 360???
> >>? ? 
> >>>>? ? ? ? ? (GPDR(i)
> >> &
> >>>> GPIO_bit(i))) {
> >>>>
> >>>> Look at the check above...
> >>>> the PGSR only gets set for the GPIOs
> configured to
> >> output
> >>>> (presumably by gpio_direction_output()
> function).
> >>>
> >>> OK, I understand your objection here.
> >>>
> >>> My feeling is that the (GPDR(i) &
> GPIO_bit(i)) test
> >> is unwanted because
> >>> the current state of GPDR (via
> gpio_direction_output())
> >> does not apply in
> >>> sleep mode,
> >>
> >> It gets copied to PGSR in the pxa2xx_mfp_suspend()
> and
> >> therefore it does apply!
> >> That is exactly the meaning of
> MFP_LPM_KEEP_OUTPUT:
> >> _If_ the pin is output, then _keep_ it output with
> the
> >> _runtime_ value.
> > 
> > OK, I see, your interpretation is that
> MFP_LPM_KEEP_OUTPUT is optional:
> > it should not apply if the pin is an input at the time
> > pxa2xx_mfp_suspend() is called.
> > My interpretation was that MFP_LPM_KEEP_OUTPUT is
> mandatory: it always
> > applies regardless of the current pin direction,
> exactly like
> > MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.
> > 
> > If your interpretation is correct then DRIVE_HIGH and
> DRIVE_LOW will
> > likewise need to check the state of GPDR at the time
> > pxa2xx_mfp_suspend() is called.
> > If my interpretation is correct then having KEEP_OUTPUT
> ignore the state
> > of GPDR in pxa2xx_mfp_suspend() is the correct thing to
> do.
> > 
> > Do you agree that KEEP_OUTPUT, DRIVE_HIGH and DRIVE_LOW
> should all be
> > consistent in being either optional or mandatory sleep
> mode
> > configurations?
> 
> No, according to current implementation (and IMO, it is
> _not_ a bug):
> DRIVE_HIGH and DRIVE_LOW are mandatory,
> KEEP_OUTPUT is optional.
> 
> > 
> >>
> >>> yet for some reason it is being allowed to
> prevent the
> >>> setting of PGSR in the case of GPIO inputs.
> >>
> >> Because, the PGSR is valid only for GPIO outputs!
> >> and has nothing to do with inputs.
> > 
> > I know, it makes no sense to specify KEEP_OUTPUT,
> DRIVE_HIGH or DRIVE_LOW
> > for GPIO inputs.
> 
> That's the point, that it does make sense for certain
> cases,
> when you want the GPIO to be bidirectional, but output in
> suspend.
> This allows certain flexibility.
> 
> > But since DRIVE_HIGH or DRIVE_LOW currently don't
> prevent that,
> 
> Don't prevent what?

Don't prevent a GPIO input being changed to an output when entering
suspend().

By the way, there is still the issue of DRIVE_HIGH and DRIVE_LOW
forcing the GPIO direction to output in pxa2xx_mfp_suspend()
*before* the PGSR values take effect. Surely a bug?

> DRIVE_HIGH and DRIVE_LOW allow you to do
> what
> ever you want with the GPIO in runtime, but have it strictly
> configured
> for the suspend.
> 
> > the
> > presumption must be that GPIOs configured as DRIVE_HIGH
> or DRIVE_LOW are
> > being used as outputs. Likewise for KEEP_OUTPUT?
> 
> KEEP_OUTPUT only affects the GPIOs configured for output in
> runtime
> and let you keep the last output value.
> 
> > 
> >>
> >>>
> >>> Consequently the (GPDR(i) & GPIO_bit(i))
> test
> >> should be removed.
> >>
> >> No, absolutely not!
> >> This will change the meaning of the
> MFP_LPM_KEEP_OUTPUT
> >> symbol!
> > 
> > Only if you interpret KEEP_OUTPUT as an optional sleep
> mode configuration.
> 
> That what the current implementation does and IMO, it is the
> right thing.
> 
> > 
> >> I have a feeling that you are trying to solve a
> problem that
> >> does not exist!
> > 
> > The problem is that MFP configurations of the form:
> >? ???GPIO<n>_GPIO |
> MFP_LPM_KEEP_OUTPUT,
> > will always set the sleep mode gpio direction to
> INPUT.
> > That is the case with corgi and would also be the case
> with hx4700.
> > That is the problem which needs to be solved.
> 
> Yes, it is just there is no h/w MFP in PXA2xx and all the
> stuff done in s/w.
> That is the reason it is a bit mess.

It's more than a mess: it's broken, per the current corgi configuration.

> IMO, the cleanest solution would be to have
> GPIO<n>_GPIO_OUT for each
> GPIO that needs to be configured as output instead of
> breaking the
> already fragile MFP logic.

And similarly fix the corgi configuration.

> 
> > 
> >>
> >>>
> >>>>>? ? ?
> 361???
> >>? ? 
> >>>>? ? ? ? ? if
> (GPLR(i)
> >> &
> >>>> GPIO_bit(i))
> >>>>>? ? ?
> 362???
> >>? ? 
> >>>>? ? ? ? ?
> ???
> >>>> PGSR(gpio_to_bank(i)) |= GPIO_bit(i);
> >>>>>? ? ?
> 363???
> >>? ? 
> >>>>? ? ? ? ? else
> >>>>>? ? ?
> 364???
> >>? ? 
> >>>>? ? ? ? ?
> ???
> >>>> PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);
> >>>>>? ? ?
> 365???
> >>? ? 
> >>>>? ? ? }
> >>>>>? ? ?
> 366???
> >>? ? 
> >>>> }
> >>>>>? ? ? 367? ? 
> >>>>>? ? ?
> 368???
> >>? ? 
> >>>> for (i = 0; i <=
> gpio_to_bank(pxa_last_gpio);
> >> i++) {
> >>>>>? ? ? 369? ? 
> >>>>>? ? ? ?
> ???...
> >>>>>? ? ?
> 375???
> >>? ? 
> >>>>? ? ? GPDR(i * 32) =
> >> gpdr_lpm[i];
> >>>>>? ? ?
> 376???
> >>? ? 
> >>>> }
> >>>>>? ? ?
> 377???
> >>? ? 
> >>>> return 0;
> >>>>>? ? ? 378? ? }
> >>>>>
> >>>>> However looking at pxa2xx_mfp_suspend()
> more
> >> closely it
> >>>> is true that the
> >>>>> GPDR registers are set before the PGSR
> values
> >> take
> >>>> effect (when the
> >>>>> processor enters sleep mode).
> >>>>> That is no different from
> MFP_LPM_DRIVE_HIGH
> >> or
> >>>> MFP_LPM_DRIVE_LOW though.
> >>>>
> >>>> Well, it is different, because the in case
> of
> >>>> MFP_LPM_DRIVE_*,
> >>>> the PGSR is set in the __mfp_config_gpio()
> function
> >> and has
> >>>> a _known value_.
> >>>
> >>> KEEP_OUTPUT does the same where GPIOs are
> configured as
> >> outputs, as is
> >>> the case with corgi and hx4700.
> >>> Your valid objection is for cases where GPIOs
> are
> >> configured as inputs.
> >>> So I presume that removing the (GPDR(i) &
> >> GPIO_bit(i)) test will satisfy
> >>> your objection.
> >>
> >> again, no, because it will change the meaning of
> the
> >> MFP_LPM_KEEP_OUTPUT symbol!
> >>
> >>>
> >>>>> So the presumption must be that the
> GPIOs
> >> configured as
> >>>> DRIVE_HIGH,
> >>>>> DRIVER_LOW and now KEEP_OUTPUT already
> have a
> >> valid
> >>>> direction and value
> >>>>> set at the point the GPDR registers
> are
> >> forced.
> >>>>
> >>>> Yes. That is exactly my point here.
> >>>> MFP_LPM_DRIVE_* has the value determined
> and set.
> >>>> MFP_LPM_KEEP_OUTPUT does not, but with your
> patch
> >> it will
> >>>> effectively
> >>>> force the GPIO to be output in sleep mode,
> but with
> >> no value
> >>>> set!
> >>>
> >>> Only in cases where the GPIO is used as an
> input in
> >> normal mode but
> >>> configured to be an output in sleep mode.
> >>> Removing the (GPDR(i) & GPIO_bit(i)) test
> addresses
> >> that case.
> >>
> >> No, because, if the GPIO is input, you will
> configure it for
> >> output
> >> and set some arbitrary value that is seen on the
> input!
> >> This is bad!
> >>
> >>>
> >>>>>> Why can't you use:
> >>>>>> GPIO<n>_GPIO |
> >> MFP_LPM_DRIVE_<level>
> >>>>>> ?
> >>>>>
> >>>>> The hx4700 has a couple of charging
> GPIOs which
> >> we want
> >>>> to keep HIGH or
> >>>>> LOW during sleep mode:
> >>>>>
> >>>>> % fgrep MFP_LPM_KEEP_OUTPUT
> >> arch/arm/mach-pxa/hx4700.c
> >>>>>? ? ? GPIO72_GPIO |
> >>>> MFP_LPM_KEEP_OUTPUT,? ? /*
> >> BQ24022_nCHARGE_EN
> >>>> */
> >>>>>? ? ? GPIO96_GPIO |
> >>>> MFP_LPM_KEEP_OUTPUT,? ? /*
> BQ24022_ISET2
> >> */
> >>>>> % 
> >>>>
> >>>> I still, can't understand, why
> MFP_LPM_DRIVE_* does
> >> not do
> >>>> the job for you...
> >>>> What's the real problem with using
> >> MFP_LPM_DRIVE_*?
> >>>> Can't you specify it for those GPIO72/96?
> >>>
> >>> The feeling was that if the unit was charging
> in normal
> >> mode then it
> >>> should remain charging in sleep mode, and if it
> wasn't
> >> then it should
> >>> remain not charging.
> >>
> >> Ok, finally we are talking about a real problem...
> >> I see...
> >> IMO, the best solution will be to add a generic
> >> GPIOx_GPIO_OUT
> >> (as already proposed by Haojian) definition (where
> x is the
> >> required GPIO number)for your used GPIOs and use it
> in MFP
> >> config
> >> structure for hx4700 along with
> MFP_LPM_KEEP_OUTPUT.
> >>
> >>
> >> Another solution would be (though a bit hackish):
> >> GPIOx_GPIO | MFP_LPM_DRIVE_LOW |
> MFP_LPM_KEEP_OUTPUT
> >>
> >> The above should work for you as you would expect -
> will set
> >> the GPIO
> >> to be output low by default, then you will
> reconfigure it to
> >> the
> >> appropriate level from your charger driver and the
> new level
> >> will be used
> >> by the existing logic in pxa2xx_mfp_suspend().
> > 
> > Those solutions are fair enough.
> > Can we first answer the question of whether
> KEEP_OUTPUT, DRIVE_HIGH and
> > DRIVE_LOW are optional or mandatory sleep mode
> configurations. That may
> > determine the way forward.
> 
> Well for the DRIVE_HIGH and DRIVE_LOW, the names speak for
> them selfs -
> MultiFunctionalPin_LowPowerManager_DRIVE_<HIGH|LOW>
> and there is no
> questions arise.
> For the MFP_LPM_KEEP_OUTPUT - is PXA2xx specific, apparently
> it is
> confusing and probably has to have a comment explaining the
> meaning.
> 
> From git log:
> ---------------cut---------------------
> commit 1106143d7ab43ba07678c88c85417df219354ae8
> Author: Eric Miao <eric.y.miao@gmail.com>
> Date:???Mon Jan 11 21:25:15 2010 +0800
> 
> ? ? [ARM] pxa: add MFP_LPM_KEEP_OUTPUT flag to pin
> config
> ? ? 
> ? ? Some pins are expected to keep their last
> level during suspend, and
> ? ? introduce MFP_LPM_KEEP_OUTPUT for this.
> ? ? 
> ? ? Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
> ---------------cut---------------------
> 
> The key words above are: "keep their last level".

Which is exactly what would have happened with my patch, since all
cases of KEEP_OUTPUT were indeed being applied to outputs only.

> 
> > 
> >>
> >>> Actually that's an interim solution until the
> >> bootloader can take control
> >>> of sleep mode charging, but we're not there
> yet.
> >>
> >> still you don't want the turn on/off of the charger
> in the
> >> suspend sequence,
> >> so IMO, it must also be fixed in Linux.
> >> But, please, don't change the meaning of
> >> MFP_LPM_KEEP_OUTPUT!
> >> Because, currently it is safe to use it also with
> inputs
> >> and we don't want to break this assumption.

Regards,
Paul

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Haojian Zhuang]

>> > But since DRIVE_HIGH or DRIVE_LOW currently don't
>> prevent that,
>>
>> Don't prevent what?
>
> Don't prevent a GPIO input being changed to an output when entering
> suspend().
>
PGSRx register allow for selecting the output state (the driven value)
of each GPIO pin when the processor enters and while it is in sleep
mode.

Each bit of PGSRx register is named as SS (Sleep State of GPIO<n>).

SS '0' -- If GPIO is programmed as an output, the pin is driven and held
low during the transition to and while in sleep mode.

SS '1' -- If GPIO is programmed as an output, the pin is driven and held
high during the transition to and while in sleep mode.

So pin must be configured to GPIO output first before entering sleep mode.

Now the issue comes. If you use the pin as input, why do you need to change
it into output mode in run time?

> By the way, there is still the issue of DRIVE_HIGH and DRIVE_LOW
> forcing the GPIO direction to output in pxa2xx_mfp_suspend()
> *before* the PGSR values take effect. Surely a bug?
>
It's evil to change pin from input mode to output mode if it's in active mode.

Actually we only need to configure control pins to peripherals on pxa27x. It's
a little different from pxa3xx. For pxa3xx, we can even configure input pin
in low power mode. PXA3xx has standby mode that internal device keeps
its state. PXA27x doesn't have this feature. If we want to save power while
PXA27x is suspend, the peripheral should be shutdown too.

Since we need to shutdown peripheral, control pin should always connect to
 reset or chip select signal of peripheral, we can disable the peripheral. So
we needn't to configure low power mode of those input pins.

Thanks
Haojian

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

Hello Haojian,

--- On Mon, 2/4/12, Haojian Zhuang <haojian.zhuang@gmail.com> wrote:
> >> > But since DRIVE_HIGH or
> DRIVE_LOW currently don't
> >> prevent that,
> >>
> >> Don't prevent what?
> >
> > Don't prevent a GPIO input being changed to an output
> when entering
> > suspend().
> >
> PGSRx register allow for selecting the output state (the
> driven value)
> of each GPIO pin when the processor enters and while it is
> in sleep
> mode.
> 
> Each bit of PGSRx register is named as SS (Sleep State of
> GPIO<n>).
> 
> SS '0' -- If GPIO is programmed as an output, the pin is
> driven and held
> low during the transition to and while in sleep mode.
> 
> SS '1' -- If GPIO is programmed as an output, the pin is
> driven and held
> high during the transition to and while in sleep mode.
> 
> So pin must be configured to GPIO output first before
> entering sleep mode.
> 
> Now the issue comes. If you use the pin as input, why do you
> need to change
> it into output mode in run time?

I cannot think of a reason why you would change an input to an output.

But the possibility that the direction could be changed from input
to output has been cited as a reason why KEEP_OUTPUT cannot force
the gpio direction to output.

It is relatively easy to verify that KEEP_OUTPUT does not change the
direction from input to output, because currently only 2 platforms
use KEEP_OUTPUT, namely corgi and hx4700.

It is less easy to verify that DRIVE_HIGH and DRIVE_LOW do not change
the direction from input to output, because many more platforms use
them; a cursory check suggests that up to 16 platforms use them.

> 
> > By the way, there is still the issue of DRIVE_HIGH and
> DRIVE_LOW
> > forcing the GPIO direction to output in
> pxa2xx_mfp_suspend()
> > *before* the PGSR values take effect. Surely a bug?
> >
> It's evil to change pin from input mode to output mode if
> it's in active mode.

Agreed.

So pxa2xx_mfp_suspend() contains a potential bug, unless someone can
verify that DRIVE_HIGH and DRIVE_LOW never change the direction from
input to output.

> 
> Actually we only need to configure control pins to
> peripherals on pxa27x. It's
> a little different from pxa3xx. For pxa3xx, we can even
> configure input pin
> in low power mode. PXA3xx has standby mode that internal
> device keeps
> its state. PXA27x doesn't have this feature. If we want to
> save power while
> PXA27x is suspend, the peripheral should be shutdown too.
> 
> Since we need to shutdown peripheral, control pin should
> always connect to
>  reset or chip select signal of peripheral, we can disable
> the peripheral. So
> we needn't to configure low power mode of those input pins.

Regards,
Paul

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Igor Grinberg]

On 04/02/12 16:33, Paul Parsons wrote:
> Hello Igor,
> 

[...] Remove the unreadable stuff...

>>>>> On corgi, the three GPIOs configured as
>>>> MFP_LPM_KEEP_OUTPUT:
>>>>>       CORGI_GPIO_LED_ORANGE
>>>>>       CORGI_GPIO_CHRG_ON
>>>>>       CORGI_GPIO_CHRG_UKN
>>>>> are all used as outputs.
>>>>
>>>> Have you verified it, because currently
>> MFP_LPM_KEEP_OUTPUT
>>>> does not do a direction configuration... and IMO,
>> it should
>>>> not!
>>>
>>> Yes, all 3 GPIOs are configured as outputs:
>>> CORGI_GPIO_LED_ORANGE in drivers/leds/leds-gpio.c
>>> CORGI_GPIO_CHRG_ON in arch/arm/mach-pxa/corgi_pm.c
>>> CORGI_GPIO_CHRG_UKN in arch/arm/mach-pxa/corgi_pm.c
>>
>> Good, then they will be kept output also in the suspend
>> mode,
>> as for current implementation.

Now I can see that it will not... because of the below line:
GPDR(i * 32) = gpdr_lpm[i];

in the pxa2xx_mfp_suspend() function.

[...] remove unreadable stuff...

>>>
>>> Do you agree that KEEP_OUTPUT, DRIVE_HIGH and DRIVE_LOW
>> should all be
>>> consistent in being either optional or mandatory sleep
>> mode
>>> configurations?
>>
>> No, according to current implementation (and IMO, it is
>> _not_ a bug):
>> DRIVE_HIGH and DRIVE_LOW are mandatory,
>> KEEP_OUTPUT is optional.
>>
>>>
>>>>
>>>>> yet for some reason it is being allowed to
>> prevent the
>>>>> setting of PGSR in the case of GPIO inputs.
>>>>
>>>> Because, the PGSR is valid only for GPIO outputs!
>>>> and has nothing to do with inputs.
>>>
>>> I know, it makes no sense to specify KEEP_OUTPUT,
>> DRIVE_HIGH or DRIVE_LOW
>>> for GPIO inputs.
>>
>> That's the point, that it does make sense for certain
>> cases,
>> when you want the GPIO to be bidirectional, but output in
>> suspend.
>> This allows certain flexibility.
>>
>>> But since DRIVE_HIGH or DRIVE_LOW currently don't
>> prevent that,
>>
>> Don't prevent what?
> 
> Don't prevent a GPIO input being changed to an output when entering
> suspend().

Why should they prevent that? In case of bidirectional GPIOs it is
perfectly fine.

> 
> By the way, there is still the issue of DRIVE_HIGH and DRIVE_LOW
> forcing the GPIO direction to output in pxa2xx_mfp_suspend()
> *before* the PGSR values take effect. Surely a bug?

No, the order is not important as the final setting is done by the h/w
during the suspend transition.

> 
>> DRIVE_HIGH and DRIVE_LOW allow you to do
>> what
>> ever you want with the GPIO in runtime, but have it strictly
>> configured
>> for the suspend.
>>
>>> the
>>> presumption must be that GPIOs configured as DRIVE_HIGH
>> or DRIVE_LOW are
>>> being used as outputs. Likewise for KEEP_OUTPUT?
>>
>> KEEP_OUTPUT only affects the GPIOs configured for output in
>> runtime
>> and let you keep the last output value.
>>
>>>
>>>>
>>>>>
>>>>> Consequently the (GPDR(i) & GPIO_bit(i))
>> test
>>>> should be removed.
>>>>
>>>> No, absolutely not!
>>>> This will change the meaning of the
>> MFP_LPM_KEEP_OUTPUT
>>>> symbol!
>>>
>>> Only if you interpret KEEP_OUTPUT as an optional sleep
>> mode configuration.
>>
>> That what the current implementation does and IMO, it is the
>> right thing.
>>
>>>
>>>> I have a feeling that you are trying to solve a
>> problem that
>>>> does not exist!
>>>
>>> The problem is that MFP configurations of the form:
>>>      GPIO<n>_GPIO |
>> MFP_LPM_KEEP_OUTPUT,
>>> will always set the sleep mode gpio direction to
>> INPUT.
>>> That is the case with corgi and would also be the case
>> with hx4700.
>>> That is the problem which needs to be solved.
>>
>> Yes, it is just there is no h/w MFP in PXA2xx and all the
>> stuff done in s/w.
>> That is the reason it is a bit mess.
> 
> It's more than a mess: it's broken, per the current corgi configuration.

Ok. Indeed it looks broken even with my meaning of KEEP_OUTPUT,
because of the line that sets all GPDRs to gpdr_lpm values in
pxa2xx_mfp_suspend() function.

> 
>> IMO, the cleanest solution would be to have
>> GPIO<n>_GPIO_OUT for each
>> GPIO that needs to be configured as output instead of
>> breaking the
>> already fragile MFP logic.
> 
> And similarly fix the corgi configuration.

Please, check the attached patch.
It should not change the meaning of KEEP_OUTPUT, but fix the bug.

[...] remove unreadable stuff...

>>>>> The feeling was that if the unit was charging
>> in normal
>>>> mode then it
>>>>> should remain charging in sleep mode, and if it
>> wasn't
>>>> then it should
>>>>> remain not charging.
>>>>
>>>> Ok, finally we are talking about a real problem...
>>>> I see...
>>>> IMO, the best solution will be to add a generic
>>>> GPIOx_GPIO_OUT
>>>> (as already proposed by Haojian) definition (where
>> x is the
>>>> required GPIO number)for your used GPIOs and use it
>> in MFP
>>>> config
>>>> structure for hx4700 along with
>> MFP_LPM_KEEP_OUTPUT.
>>>>
>>>>
>>>> Another solution would be (though a bit hackish):
>>>> GPIOx_GPIO | MFP_LPM_DRIVE_LOW |
>> MFP_LPM_KEEP_OUTPUT
>>>>
>>>> The above should work for you as you would expect -
>> will set
>>>> the GPIO
>>>> to be output low by default, then you will
>> reconfigure it to
>>>> the
>>>> appropriate level from your charger driver and the
>> new level
>>>> will be used
>>>> by the existing logic in pxa2xx_mfp_suspend().
>>>
>>> Those solutions are fair enough.
>>> Can we first answer the question of whether
>> KEEP_OUTPUT, DRIVE_HIGH and
>>> DRIVE_LOW are optional or mandatory sleep mode
>> configurations. That may
>>> determine the way forward.
>>
>> Well for the DRIVE_HIGH and DRIVE_LOW, the names speak for
>> them selfs -
>> MultiFunctionalPin_LowPowerManager_DRIVE_<HIGH|LOW>
>> and there is no
>> questions arise.
>> For the MFP_LPM_KEEP_OUTPUT - is PXA2xx specific, apparently
>> it is
>> confusing and probably has to have a comment explaining the
>> meaning.
>>
>> From git log:
>> ---------------cut---------------------
>> commit 1106143d7ab43ba07678c88c85417df219354ae8
>> Author: Eric Miao <eric.y.miao@gmail.com>
>> Date:   Mon Jan 11 21:25:15 2010 +0800
>>
>>     [ARM] pxa: add MFP_LPM_KEEP_OUTPUT flag to pin
>> config
>>     
>>     Some pins are expected to keep their last
>> level during suspend, and
>>     introduce MFP_LPM_KEEP_OUTPUT for this.
>>     
>>     Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
>> ---------------cut---------------------
>>
>> The key words above are: "keep their last level".
> 
> Which is exactly what would have happened with my patch, since all
> cases of KEEP_OUTPUT were indeed being applied to outputs only.

No, your patch will change also the inputs to outputs.
Because "last level" does not apply to input pins.


-- 
Regards,
Igor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pxa2xx-mfp.patch
Type: text/x-patch
Size: 545 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120403/b1cfe5ee/attachment.bin>

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Igor Grinberg]

On 04/02/12 18:34, Paul Parsons wrote:
> Hello Haojian,
> 
> --- On Mon, 2/4/12, Haojian Zhuang <haojian.zhuang@gmail.com> wrote:
>>>>> But since DRIVE_HIGH or
>> DRIVE_LOW currently don't
>>>> prevent that,
>>>>
>>>> Don't prevent what?
>>>
>>> Don't prevent a GPIO input being changed to an output
>> when entering
>>> suspend().
>>>
>> PGSRx register allow for selecting the output state (the
>> driven value)
>> of each GPIO pin when the processor enters and while it is
>> in sleep
>> mode.
>>
>> Each bit of PGSRx register is named as SS (Sleep State of
>> GPIO<n>).
>>
>> SS '0' -- If GPIO is programmed as an output, the pin is
>> driven and held
>> low during the transition to and while in sleep mode.
>>
>> SS '1' -- If GPIO is programmed as an output, the pin is
>> driven and held
>> high during the transition to and while in sleep mode.
>>
>> So pin must be configured to GPIO output first before
>> entering sleep mode.
>>
>> Now the issue comes. If you use the pin as input, why do you
>> need to change
>> it into output mode in run time?
> 
> I cannot think of a reason why you would change an input to an output.

One reason, I can think of is bidirectional pins:
the direction can change several times during runtime, but
peripheral may require the pin to be in some defined state in suspend.

> 
> But the possibility that the direction could be changed from input
> to output has been cited as a reason why KEEP_OUTPUT cannot force
> the gpio direction to output.

Indeed, and that's why your patch is wrong - it does exactly that,
it enables the possibility to change the input to output and drive an
undefined level.

> 
> It is relatively easy to verify that KEEP_OUTPUT does not change the
> direction from input to output, because currently only 2 platforms
> use KEEP_OUTPUT, namely corgi and hx4700.

Again, with your patch it will make the change possible.

> 
> It is less easy to verify that DRIVE_HIGH and DRIVE_LOW do not change
> the direction from input to output, because many more platforms use
> them; a cursory check suggests that up to 16 platforms use them.

DRIVE_HIGH and DRIVE_LOW are used across several _different_ SoCs
and care must be taken while changing something related to them.

The DRIVE_HIGH and DRIVE_LOW in current implementation _can_ change
the inputs to outputs, but IMO it is the board support code
responsibility to verify that the right thing is done.

> 
>>
>>> By the way, there is still the issue of DRIVE_HIGH and
>> DRIVE_LOW
>>> forcing the GPIO direction to output in
>> pxa2xx_mfp_suspend()
>>> *before* the PGSR values take effect. Surely a bug?
>>>
>> It's evil to change pin from input mode to output mode if
>> it's in active mode.

Unless, it is peripheral requirement.

> 
> Agreed.
> 
> So pxa2xx_mfp_suspend() contains a potential bug, unless someone can
> verify that DRIVE_HIGH and DRIVE_LOW never change the direction from
> input to output.

They do, but I don't consider it a bug, but a flexibility.
When you specify DRIVE_HIGH or DRIVE_LOW in the board pin config,
you must know what you are doing and be responsible.


-- 
Regards,
Igor.

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

Hello Igor,

--- On Tue, 3/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
> On 04/02/12 16:33, Paul Parsons
> wrote:
> > Hello Igor,
> > 
> 
> [...] Remove the unreadable stuff...
> 
> >>>>> On corgi, the three GPIOs configured
> as
> >>>> MFP_LPM_KEEP_OUTPUT:
> >>>>>? ?
> ???CORGI_GPIO_LED_ORANGE
> >>>>>? ?
> ???CORGI_GPIO_CHRG_ON
> >>>>>? ?
> ???CORGI_GPIO_CHRG_UKN
> >>>>> are all used as outputs.
> >>>>
> >>>> Have you verified it, because currently
> >> MFP_LPM_KEEP_OUTPUT
> >>>> does not do a direction configuration...
> and IMO,
> >> it should
> >>>> not!
> >>>
> >>> Yes, all 3 GPIOs are configured as outputs:
> >>> CORGI_GPIO_LED_ORANGE in
> drivers/leds/leds-gpio.c
> >>> CORGI_GPIO_CHRG_ON in
> arch/arm/mach-pxa/corgi_pm.c
> >>> CORGI_GPIO_CHRG_UKN in
> arch/arm/mach-pxa/corgi_pm.c
> >>
> >> Good, then they will be kept output also in the
> suspend
> >> mode,
> >> as for current implementation.
> 
> Now I can see that it will not... because of the below
> line:
> GPDR(i * 32) = gpdr_lpm[i];
> 
> in the pxa2xx_mfp_suspend() function.#

Which is precisely the issue this patch addresses, namely to force
the gpdr_lpm[] direction to output for KEEP_OUTPUT configurations.

> 
> [...] remove unreadable stuff...
> 
> >>>
> >>> Do you agree that KEEP_OUTPUT, DRIVE_HIGH and
> DRIVE_LOW
> >> should all be
> >>> consistent in being either optional or
> mandatory sleep
> >> mode
> >>> configurations?
> >>
> >> No, according to current implementation (and IMO,
> it is
> >> _not_ a bug):
> >> DRIVE_HIGH and DRIVE_LOW are mandatory,
> >> KEEP_OUTPUT is optional.
> >>
> >>>
> >>>>
> >>>>> yet for some reason it is being allowed
> to
> >> prevent the
> >>>>> setting of PGSR in the case of GPIO
> inputs.
> >>>>
> >>>> Because, the PGSR is valid only for GPIO
> outputs!
> >>>> and has nothing to do with inputs.
> >>>
> >>> I know, it makes no sense to specify
> KEEP_OUTPUT,
> >> DRIVE_HIGH or DRIVE_LOW
> >>> for GPIO inputs.
> >>
> >> That's the point, that it does make sense for
> certain
> >> cases,
> >> when you want the GPIO to be bidirectional, but
> output in
> >> suspend.
> >> This allows certain flexibility.
> >>
> >>> But since DRIVE_HIGH or DRIVE_LOW currently
> don't
> >> prevent that,
> >>
> >> Don't prevent what?
> > 
> > Don't prevent a GPIO input being changed to an output
> when entering
> > suspend().
> 
> Why should they prevent that? In case of bidirectional GPIOs
> it is
> perfectly fine.

I didn't say they should prevent that. I said they don't prevent
that, thereby allowing the direction of an input gpio to be changed
to output before the PGSR value is applied.

> 
> > 
> > By the way, there is still the issue of DRIVE_HIGH and
> DRIVE_LOW
> > forcing the GPIO direction to output in
> pxa2xx_mfp_suspend()
> > *before* the PGSR values take effect. Surely a bug?
> 
> No, the order is not important as the final setting is done
> by the h/w
> during the suspend transition.

No, the GPIO direction is changed by software, not hardware.
Hardware suspend does not change the GPIO direction, it only
drives the GPIO values from the PGSR registers.

Thus there is a definite window between the direction being changed
by software and the PGSR value being driven by hardware.

Within that window, the output values are indeterminate, at least
as far as the suspend software is concerned.

So unless valid output values have already been programmed onto
those input GPIOs beforehand, this direction/PGSR window is a bug.

> 
> > 
> >> DRIVE_HIGH and DRIVE_LOW allow you to do
> >> what
> >> ever you want with the GPIO in runtime, but have it
> strictly
> >> configured
> >> for the suspend.
> >>
> >>> the
> >>> presumption must be that GPIOs configured as
> DRIVE_HIGH
> >> or DRIVE_LOW are
> >>> being used as outputs. Likewise for
> KEEP_OUTPUT?
> >>
> >> KEEP_OUTPUT only affects the GPIOs configured for
> output in
> >> runtime
> >> and let you keep the last output value.
> >>
> >>>
> >>>>
> >>>>>
> >>>>> Consequently the (GPDR(i) &
> GPIO_bit(i))
> >> test
> >>>> should be removed.
> >>>>
> >>>> No, absolutely not!
> >>>> This will change the meaning of the
> >> MFP_LPM_KEEP_OUTPUT
> >>>> symbol!
> >>>
> >>> Only if you interpret KEEP_OUTPUT as an
> optional sleep
> >> mode configuration.
> >>
> >> That what the current implementation does and IMO,
> it is the
> >> right thing.
> >>
> >>>
> >>>> I have a feeling that you are trying to
> solve a
> >> problem that
> >>>> does not exist!
> >>>
> >>> The problem is that MFP configurations of the
> form:
> >>>? ? ? GPIO<n>_GPIO |
> >> MFP_LPM_KEEP_OUTPUT,
> >>> will always set the sleep mode gpio direction
> to
> >> INPUT.
> >>> That is the case with corgi and would also be
> the case
> >> with hx4700.
> >>> That is the problem which needs to be solved.
> >>
> >> Yes, it is just there is no h/w MFP in PXA2xx and
> all the
> >> stuff done in s/w.
> >> That is the reason it is a bit mess.
> > 
> > It's more than a mess: it's broken, per the current
> corgi configuration.
> 
> Ok. Indeed it looks broken even with my meaning of
> KEEP_OUTPUT,
> because of the line that sets all GPDRs to gpdr_lpm values
> in
> pxa2xx_mfp_suspend() function.
> 
> > 
> >> IMO, the cleanest solution would be to have
> >> GPIO<n>_GPIO_OUT for each
> >> GPIO that needs to be configured as output instead
> of
> >> breaking the
> >> already fragile MFP logic.
> > 
> > And similarly fix the corgi configuration.
> 
> Please, check the attached patch.
> It should not change the meaning of KEEP_OUTPUT, but fix the
> bug.

Yes, that fixes the KEEP_OUTPUT bug.

It would be beneficial if someone could formally document the precise
meaning of KEEP_OUTPUT.

The direction/PGSR window bug is still present though.

I'm starting to think it would be better for pxa2xx_mfp_suspend()
to not touch GPDR at all, ever. Thus outputs would remain outputs,
inputs would remain inputs. That would solve the latter bug and
simplify PXA2xx MFP, but at the cost of making DRIVE_HIGH/DRIVE_LOW
conditional like KEEP_OUTPUT.

It would be beneficial if someone could formally document the precise
meaning of DRIVE_HIGH/DRIVE_LOW.

> 
> [...] remove unreadable stuff...
> 
> >>>>> The feeling was that if the unit was
> charging
> >> in normal
> >>>> mode then it
> >>>>> should remain charging in sleep mode,
> and if it
> >> wasn't
> >>>> then it should
> >>>>> remain not charging.
> >>>>
> >>>> Ok, finally we are talking about a real
> problem...
> >>>> I see...
> >>>> IMO, the best solution will be to add a
> generic
> >>>> GPIOx_GPIO_OUT
> >>>> (as already proposed by Haojian) definition
> (where
> >> x is the
> >>>> required GPIO number)for your used GPIOs
> and use it
> >> in MFP
> >>>> config
> >>>> structure for hx4700 along with
> >> MFP_LPM_KEEP_OUTPUT.
> >>>>
> >>>>
> >>>> Another solution would be (though a bit
> hackish):
> >>>> GPIOx_GPIO | MFP_LPM_DRIVE_LOW |
> >> MFP_LPM_KEEP_OUTPUT
> >>>>
> >>>> The above should work for you as you would
> expect -
> >> will set
> >>>> the GPIO
> >>>> to be output low by default, then you will
> >> reconfigure it to
> >>>> the
> >>>> appropriate level from your charger driver
> and the
> >> new level
> >>>> will be used
> >>>> by the existing logic in
> pxa2xx_mfp_suspend().
> >>>
> >>> Those solutions are fair enough.
> >>> Can we first answer the question of whether
> >> KEEP_OUTPUT, DRIVE_HIGH and
> >>> DRIVE_LOW are optional or mandatory sleep mode
> >> configurations. That may
> >>> determine the way forward.
> >>
> >> Well for the DRIVE_HIGH and DRIVE_LOW, the names
> speak for
> >> them selfs -
> >>
> MultiFunctionalPin_LowPowerManager_DRIVE_<HIGH|LOW>
> >> and there is no
> >> questions arise.
> >> For the MFP_LPM_KEEP_OUTPUT - is PXA2xx specific,
> apparently
> >> it is
> >> confusing and probably has to have a comment
> explaining the
> >> meaning.
> >>
> >> From git log:
> >> ---------------cut---------------------
> >> commit 1106143d7ab43ba07678c88c85417df219354ae8
> >> Author: Eric Miao <eric.y.miao@gmail.com>
> >> Date:???Mon Jan 11 21:25:15 2010
> +0800
> >>
> >>? ???[ARM] pxa: add
> MFP_LPM_KEEP_OUTPUT flag to pin
> >> config
> >>? ???
> >>? ???Some pins are expected to
> keep their last
> >> level during suspend, and
> >>? ???introduce
> MFP_LPM_KEEP_OUTPUT for this.
> >>? ???
> >>? ???Signed-off-by: Eric Miao
> <eric.y.miao@gmail.com>
> >> ---------------cut---------------------
> >>
> >> The key words above are: "keep their last level".
> > 
> > Which is exactly what would have happened with my
> patch, since all
> > cases of KEEP_OUTPUT were indeed being applied to
> outputs only.
> 
> No, your patch will change also the inputs to outputs.
> Because "last level" does not apply to input pins.

My point was that KEEP_OUTPUT configurations are only applied to
GPIOs used as outputs. So no directions would be changed in practice.

Regards,
Paul

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Igor Grinberg]

On 04/03/12 15:30, Paul Parsons wrote:
> Hello Igor,

[...]

>> Now I can see that it will not... because of the below
>> line:
>> GPDR(i * 32) = gpdr_lpm[i];
>>
>> in the pxa2xx_mfp_suspend() function.#
> 
> Which is precisely the issue this patch addresses, namely to force
> the gpdr_lpm[] direction to output for KEEP_OUTPUT configurations.

Yes, but it changes the meaning of KEEP_OUTPUT and that is not good.

[...]

>>> Don't prevent a GPIO input being changed to an output
>> when entering
>>> suspend().
>>
>> Why should they prevent that? In case of bidirectional GPIOs
>> it is
>> perfectly fine.
> 
> I didn't say they should prevent that. I said they don't prevent
> that, thereby allowing the direction of an input gpio to be changed
> to output before the PGSR value is applied.

Ok, I see.

> 
>>
>>>
>>> By the way, there is still the issue of DRIVE_HIGH and
>> DRIVE_LOW
>>> forcing the GPIO direction to output in
>> pxa2xx_mfp_suspend()
>>> *before* the PGSR values take effect. Surely a bug?
>>
>> No, the order is not important as the final setting is done
>> by the h/w
>> during the suspend transition.
> 
> No, the GPIO direction is changed by software, not hardware.
> Hardware suspend does not change the GPIO direction, it only
> drives the GPIO values from the PGSR registers.

Yes, that is correct.

> 
> Thus there is a definite window between the direction being changed
> by software and the PGSR value being driven by hardware.

Right, this is the cost of lack of the h/w MFP support :-(

> 
> Within that window, the output values are indeterminate, at least
> as far as the suspend software is concerned.
> 
> So unless valid output values have already been programmed onto
> those input GPIOs beforehand, this direction/PGSR window is a bug.

Yep, sounds correct.

[...]

>> Please, check the attached patch.
>> It should not change the meaning of KEEP_OUTPUT, but fix the
>> bug.
> 
> Yes, that fixes the KEEP_OUTPUT bug.

Can this be considered as Tested-by for the formal patch?

> 
> It would be beneficial if someone could formally document the precise
> meaning of KEEP_OUTPUT.

I will try to do it along with the formal patch submission.

> 
> The direction/PGSR window bug is still present though.

Yet another patch "closing windows" attached,
care to check/test it please?

> 
> I'm starting to think it would be better for pxa2xx_mfp_suspend()
> to not touch GPDR at all, ever. Thus outputs would remain outputs,
> inputs would remain inputs. That would solve the latter bug and
> simplify PXA2xx MFP, but at the cost of making DRIVE_HIGH/DRIVE_LOW
> conditional like KEEP_OUTPUT.

I don't think it is a good idea, because then DRIVE_HIGH/DRIVE_LOW will
have different meaning between the platforms (SoCs).
How about the attached patch to fix this "windows" thing for PXA2xx?

> 
> It would be beneficial if someone could formally document the precise
> meaning of DRIVE_HIGH/DRIVE_LOW.

I can try doing it along with the formal version of the attached patch,
if we will consider it as a solution.

[...]

>> No, your patch will change also the inputs to outputs.
>> Because "last level" does not apply to input pins.
> 
> My point was that KEEP_OUTPUT configurations are only applied to
> GPIOs used as outputs. So no directions would be changed in practice.

I failed to understand that from your previous posts.
Nevertheless, the patch in the previous email fixes exactly this.

-- 
Regards,
Igor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pxa2xx-mfp2.patch
Type: text/x-patch
Size: 763 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120403/03d40497/attachment.bin>

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Igor Grinberg]

Hmmm....
If we change the GPSR and GPCR, the old levels should be reserved, right?
Attached...


-- 
Regards,
Igor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pxa2xx-mfp2.patch
Type: text/x-patch
Size: 1444 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120403/c3e05cd2/attachment.bin>

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

--- On Tue, 3/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hmmm....
> If we change the GPSR and GPCR, the old levels should be
> reserved, right?
> Attached...

Yes, that sounds right: if resume() restores the old directions then
it makes sense for it to restore the old levels first. Otherwise it
encounters the same problem as suspend().

On that very subject, I notice that __mfp_config_gpio() also sets
directions without first setting levels.

Problem is, it is not obvious what levels it should set.
Perhaps it should likewise set the levels to the PGSR values, on the
tenuous presumption that any hardware should already be in low power
mode at the time __mfp_config_gpio() is called.
But then again maybe not. This is more tricky.

I'll give the patches a try before the end of the day.

Regards,
Paul

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

Hello Igor,

--- On Tue, 3/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Please, check the attached patch.
> It should not change the meaning of KEEP_OUTPUT, but fix the
> bug.

Yes, your patch - pxa2xx-mfp.patch - does fix the KEEP_OUTPUT bug.

Regards,
Paul

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

Hello Igor,

--- On Tue, 3/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hmmm....
> If we change the GPSR and GPCR, the old levels should be
> reserved, right?
> Attached...

Your patch - pxa2xx-mfp2.patch - doesn't introduce any problems, but
I never had a failure case that could be used to prove the patch fixes
the direction/PGSR window bug. In theory the patch *should* fix it.

As for restoring the GPIO levels via GPSR and GPCR, that does seem
to work. Certainly the regular GPIO (AF0) outputs have their levels
restored OK.

Regards,
Paul

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Igor Grinberg]

Hi Paul,

On 04/04/12 02:55, Paul Parsons wrote:
> Hello Igor,
> 
> --- On Tue, 3/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> Please, check the attached patch.
>> It should not change the meaning of KEEP_OUTPUT, but fix the
>> bug.
> 
> Yes, your patch - pxa2xx-mfp.patch - does fix the KEEP_OUTPUT bug.

Looking at it one more time, we setup the GPDR with values from gpdr_lpm,
and afterwards we setup them once again to update for KEEP_CONFIG.
This can introduce a kind of spike on the GPIO (depending on how fast
the system runs).
So, fixed patch attached. Can you please test it once more and I will
submit it properly?


-- 
Regards,
Igor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pxa2xx-mfp.patch
Type: text/x-patch
Size: 817 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120405/abeb7b52/attachment.bin>

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Paul Parsons]

Hello Igor,

--- On Thu, 5/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
> >> Please, check the attached patch.
> >> It should not change the meaning of KEEP_OUTPUT,
> but fix the
> >> bug.
> > 
> > Yes, your patch - pxa2xx-mfp.patch - does fix the
> KEEP_OUTPUT bug.
> 
> Looking at it one more time, we setup the GPDR with values
> from gpdr_lpm,
> and afterwards we setup them once again to update for
> KEEP_CONFIG.
> This can introduce a kind of spike on the GPIO (depending on
> how fast
> the system runs).
> So, fixed patch attached. Can you please test it once more
> and I will
> submit it properly?

Yes, I've tested it and this new patch fixes the KEEP_OUTPUT bug.

Regards,
Paul

[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

[Igor Grinberg]

Hi Paul,

Sorry for a delay, I've been a bit busy with work...

On 04/03/12 19:26, Paul Parsons wrote:
> --- On Tue, 3/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> Hmmm....
>> If we change the GPSR and GPCR, the old levels should be
>> reserved, right?
>> Attached...
> 
> Yes, that sounds right: if resume() restores the old directions then
> it makes sense for it to restore the old levels first. Otherwise it
> encounters the same problem as suspend().
> 
> On that very subject, I notice that __mfp_config_gpio() also sets
> directions without first setting levels.

Hmmm... this is not good, but...

> 
> Problem is, it is not obvious what levels it should set.
> Perhaps it should likewise set the levels to the PGSR values, on the
> tenuous presumption that any hardware should already be in low power
> mode at the time __mfp_config_gpio() is called.
> But then again maybe not. This is more tricky.

Well, it looks like most of the cases the __mfp_config_gpio() is called
during init time and the levels are setup only later and PGSR still has
its reset values.
This leaves the direction outputs without a defined level, but having
the MFP_CFG_IN() as the default setting is the right thing and probably
assures the "safeness" of the __mfp_config_gpio().
Also, that is exactly why we should not use the MFP_CFG_OUT() directly.

I agree with you that probably the __mfp_config_gpio() function
should not touch the GPDR, but changing this has affect on all PXA2xx
boards, so I would *not* touch that until there is a real issue.

> 
> I'll give the patches a try before the end of the day.

Thanks


-- 
Regards,
Igor.