ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT

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 <[email protected]>

---

arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0

index b0a8428..3b443199 100644

arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0

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

Hi Paul,

Quoted 1 line · Paul Parsons

Some MFP configurations specify MFP_LPM_KEEP_OUTPUT to

preserve the gpio output

Quoted 3 lines

value (HIGH or LOW) during sleep mode. For example:

? ???GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

Unfortunately MFP_LPM_KEEP_OUTPUT makes no special

provision for setting the

Quoted 1 line

sleep mode gpio direction, unlike MFP_LPM_DRIVE_HIGH

and MFP_LPM_DRIVE_LOW.

Quoted 3 lines

Consequently MFP configurations of the form:

? ???GPIO<n>_GPIO |

MFP_LPM_KEEP_OUTPUT,

Quoted 4 lines

will set the sleep mode gpio direction to INPUT.

This patch forces the sleep mode gpio direction to

OUTPUT in cases where

Quoted 1 line

MFP_LPM_KEEP_OUTPUT was specified. This brings

MFP_LPM_KEEP_OUTPUT into line

Review

Quoted 6 lines

with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.

Signed-off-by Paul Parsons <[email protected]>

---

(quoted diff) +0 -0

b/arch/arm/mach-pxa/mfp-pxa2xx.c

arch/arm/mach-pxa/mfp-pxa2xx.c static int __mfp_config_gpio(unsigned

Quoted 4 lines

index b0a8428..3b443199 100644

arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0

@@ -96,6 +96,9 @@ static int __mfp_config_gpio(unsigned

gpio, unsigned long c)

Quoted 6 lines

arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0

 ? ??? ??? 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>
?

Hello Igor,

Quoted 40 lines · Igor Grinberg

Hi Paul,

Quoted 1 line · Paul Parsons

Some MFP configurations specify MFP_LPM_KEEP_OUTPUT to

preserve the gpio output

Quoted 3 lines

value (HIGH or LOW) during sleep mode. For example:

     GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

Unfortunately MFP_LPM_KEEP_OUTPUT makes no special

provision for setting the

Quoted 1 line

sleep mode gpio direction, unlike MFP_LPM_DRIVE_HIGH

and MFP_LPM_DRIVE_LOW.

Quoted 3 lines

Consequently MFP configurations of the form:

     GPIO<n>_GPIO |

MFP_LPM_KEEP_OUTPUT,

Quoted 4 lines

will set the sleep mode gpio direction to INPUT.

This patch forces the sleep mode gpio direction to

OUTPUT in cases where

Quoted 1 line

MFP_LPM_KEEP_OUTPUT was specified. This brings

MFP_LPM_KEEP_OUTPUT into line

Review

Quoted 6 lines

with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.

Signed-off-by Paul Parsons <[email protected]>

---

(quoted diff) +0 -0

b/arch/arm/mach-pxa/mfp-pxa2xx.c

arch/arm/mach-pxa/mfp-pxa2xx.c static int __mfp_config_gpio(unsigned

Quoted 4 lines

index b0a8428..3b443199 100644

arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0

@@ -96,6 +96,9 @@ static int __mfp_config_gpio(unsigned

gpio, unsigned long c)

Quoted 3 lines

arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0

          break;
      }
 

Quoted 6 lines

arch/arm/mach-pxa/mfp-pxa2xx.c static int __mfp_config_gpio(unsigned gpio, unsigned long c) · line 101

Quoted 3 lines

arch/arm/mach-pxa/mfp-pxa2xx.c +3 -0

+    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) &&

%

Quoted 4 lines

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().

Quoted 6 lines

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 at the point the GPDR registers are forced.

Quoted 4 lines

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 */

%

Quoted 3 lines · Paul Parsons

Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg <[email protected]>

Quoted 4 lines · wrote:

Quoted 4 lines

Hi Paul,

Quoted 1 line · Paul Parsons

Some MFP configurations specify

MFP_LPM_KEEP_OUTPUT to

Quoted 2 lines

Quoted 2 lines

preserve the gpio output

Quoted 1 line

value (HIGH or LOW) during sleep mode. For

example:

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

? ? ? GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

Unfortunately MFP_LPM_KEEP_OUTPUT makes no

special

Quoted 2 lines

Quoted 2 lines

provision for setting the

Quoted 1 line

sleep mode gpio direction, unlike

MFP_LPM_DRIVE_HIGH

Quoted 7 lines

Quoted 7 lines

and MFP_LPM_DRIVE_LOW.

Quoted 3 lines

Consequently MFP configurations of the form:

? ? ? GPIO<n>_GPIO |

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

will set the sleep mode gpio direction to

INPUT.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This patch forces the sleep mode gpio direction

to

Quoted 8 lines

Quoted 8 lines

OUTPUT in cases where

Quoted 1 line

MFP_LPM_KEEP_OUTPUT was specified. This brings

MFP_LPM_KEEP_OUTPUT into line

Quoted 5 lines

with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.

Signed-off-by Paul Parsons <[email protected]>

---

Quoted 6 lines

Quoted 6 lines

Review

Quoted 1 line

(quoted diff) +0 -0

b/arch/arm/mach-pxa/mfp-pxa2xx.c

arch/arm/mach-pxa/mfp-pxa2xx.c static int

Quoted 4 lines

index b0a8428..3b443199 100644

arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0

@@ -96,6 +96,9 @@ static int

__mfp_config_gpio(unsigned

Quoted 2 lines

Quoted 2 lines

gpio, unsigned long c)

Quoted 1 line

? ? ? ?

???break;

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

? ? ???}
???
+? ? if (c &

MFP_LPM_KEEP_OUTPUT)

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

+? ? ? ? is_out = 1;
+

MFP_LPM_KEEP_OUTPUT does not meant to be used to

setup the

Quoted 3 lines

Quoted 1 line

mfp config,

As far as I can see, MFP_LPM_KEEP_OUTPUT is *only* used

to setup the

Quoted 3 lines

mfp config:

% xzcat linux-3.4-rc1.tar.xz | fgrep -a

MFP_LPM_KEEP_OUTPUT

Quoted 1 line

??? GPIO13_GPIO |

MFP_LPM_KEEP_OUTPUT,??? /*
CORGI_GPIO_LED_ORANGE */

Quoted 1 line

??? GPIO38_GPIO |

MFP_LPM_KEEP_OUTPUT,??? /* CORGI_GPIO_CHRG_ON
*/

Quoted 1 line

??? GPIO43_GPIO |

MFP_LPM_KEEP_OUTPUT,??? /*
CORGI_GPIO_CHRG_UKN */

Quoted 1 line

#define MFP_LPM_KEEP_OUTPUT??? (0x1

<< 25)

Quoted 1 line

??? /* set corresponding PGSR bit of

those marked MFP_LPM_KEEP_OUTPUT */

Quoted 1 line

??? ??? if

((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT) &&

Quoted 1 line

%

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

Quoted 1 line

Quoted 1 line

but for pins that have been configured for output

by

Quoted 4 lines

Quoted 2 lines

gpio_direction_out().
(Hint: *_KEEP_*).

MFP_LPM_KEEP_OUTPUT only takes effect when the system

enters sleep mode,

Quoted 1 line

at which point the GPIO directions (GPDR registers) are

forced from the

Quoted 1 line

values privately stored in gpdr_lpm[].

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

Quoted 1 line

As far as I can tell MFP_LPM_KEEP_OUTPUT has no direct

connection to

Quoted 1 line

gpio_direction_output().

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

Quoted 2 lines

Quoted 2 lines

Also, I don't think this is a good idea, because

the patch

Quoted 2 lines

Quoted 2 lines

allows a pin
be configured as output, but does not forces a

value

Quoted 3 lines

Quoted 2 lines

(high/low)
and this is not safe.

Quoted 1 line

A value does not need to be provided until the system

enters sleep mode,

Quoted 1 line

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.

Quoted 1 line

? ? 353??? static int

pxa2xx_mfp_suspend(void)

Quoted 2 lines

? ? 354??? {
? ? 355??? ???

int i;

Quoted 2 lines

? ? 356???
? ? 357??? ???

/* set corresponding PGSR bit of those marked
MFP_LPM_KEEP_OUTPUT */

Quoted 1 line

? ? 358??? ???

for (i = 0; i < pxa_last_gpio; i++) {

Quoted 1 line

? ? 359??? ???

??? if ((gpio_desc[i].config &
MFP_LPM_KEEP_OUTPUT) &&

Quoted 1 line

? ? 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).

Quoted 1 line

? ? 361??? ???

??? ??? if (GPLR(i) &
GPIO_bit(i))

Quoted 1 line

? ? 362??? ???

??? ??? ???
PGSR(gpio_to_bank(i)) |= GPIO_bit(i);

Quoted 1 line

? ? 363??? ???

??? ??? else

Quoted 1 line

? ? 364??? ???

??? ??? ???
PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);

Quoted 1 line

? ? 365??? ???

??? }

Quoted 1 line

? ? 366??? ???

}

Quoted 2 lines

? ? 367???
? ? 368??? ???

for (i = 0; i <= gpio_to_bank(pxa_last_gpio); i++) {

Quoted 3 lines

? ? 369???
? ? ? ???...
? ? 375??? ???

??? GPDR(i * 32) = gpdr_lpm[i];

Quoted 1 line

? ? 376??? ???

}

Quoted 1 line

? ? 377??? ???

return 0;

Quoted 3 lines

? ? 378??? }

However looking at pxa2xx_mfp_suspend() more closely it

is true that the

Quoted 1 line

GPDR registers are set before the PGSR values take

effect (when the

Quoted 2 lines

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_.

Quoted 1 line

So the presumption must be that the GPIOs configured as

DRIVE_HIGH,

Quoted 1 line

DRIVER_LOW and now KEEP_OUTPUT already have a valid

direction and value

Quoted 1 line

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!

Quoted 5 lines

Quoted 3 lines

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

Quoted 4 lines

LOW during sleep mode:

% fgrep MFP_LPM_KEEP_OUTPUT arch/arm/mach-pxa/hx4700.c ??? GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT,??? /* BQ24022_nCHARGE_EN
*/

Quoted 1 line

??? GPIO96_GPIO |

MFP_LPM_KEEP_OUTPUT,??? /* BQ24022_ISET2 */

Quoted 1 line

%

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?

Hello Igor,

Quoted 86 lines · Igor Grinberg

Quoted 3 lines · Paul Parsons

Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg <[email protected]>

Quoted 4 lines · wrote:

Quoted 4 lines

Hi Paul,

Quoted 1 line · Paul Parsons

Some MFP configurations specify

MFP_LPM_KEEP_OUTPUT to

Quoted 2 lines

Quoted 2 lines

preserve the gpio output

Quoted 1 line

value (HIGH or LOW) during sleep mode. For

example:

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

      GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

Unfortunately MFP_LPM_KEEP_OUTPUT makes no

special

Quoted 2 lines

Quoted 2 lines

provision for setting the

Quoted 1 line

sleep mode gpio direction, unlike

MFP_LPM_DRIVE_HIGH

Quoted 7 lines

Quoted 7 lines

and MFP_LPM_DRIVE_LOW.

Quoted 3 lines

Consequently MFP configurations of the form:

      GPIO<n>_GPIO |

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

will set the sleep mode gpio direction to

INPUT.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This patch forces the sleep mode gpio direction

to

Quoted 8 lines

Quoted 8 lines

OUTPUT in cases where

Quoted 1 line

MFP_LPM_KEEP_OUTPUT was specified. This brings

MFP_LPM_KEEP_OUTPUT into line

Quoted 5 lines

with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.

Signed-off-by Paul Parsons <[email protected]>

---

Quoted 6 lines

Quoted 6 lines

Review

Quoted 1 line

(quoted diff) +0 -0

b/arch/arm/mach-pxa/mfp-pxa2xx.c

arch/arm/mach-pxa/mfp-pxa2xx.c static int

Quoted 4 lines

index b0a8428..3b443199 100644

arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0

@@ -96,6 +96,9 @@ static int

__mfp_config_gpio(unsigned

Quoted 2 lines

Quoted 2 lines

gpio, unsigned long c)

Quoted 1 line

break;

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

       }

+ if (c &

MFP_LPM_KEEP_OUTPUT)

Quoted 4 lines

Quoted 4 lines

arch/arm/mach-pxa/mfp-pxa2xx.c static int __mfp_config_gpio(unsigned gpio, unsigned long c) · line 101

Quoted 2 lines

arch/arm/mach-pxa/mfp-pxa2xx.c +2 -0

+        is_out = 1;
+

MFP_LPM_KEEP_OUTPUT does not meant to be used to

setup the

Quoted 3 lines

Quoted 1 line

mfp config,

As far as I can see, MFP_LPM_KEEP_OUTPUT is *only* used

to setup the

Quoted 3 lines

mfp config:

% xzcat linux-3.4-rc1.tar.xz | fgrep -a

MFP_LPM_KEEP_OUTPUT

Quoted 1 line

    GPIO13_GPIO |

MFP_LPM_KEEP_OUTPUT, /*
CORGI_GPIO_LED_ORANGE */

Quoted 1 line

    GPIO38_GPIO |

MFP_LPM_KEEP_OUTPUT, /* CORGI_GPIO_CHRG_ON
*/

Quoted 1 line

    GPIO43_GPIO |

MFP_LPM_KEEP_OUTPUT, /*
CORGI_GPIO_CHRG_UKN */

Quoted 1 line

#define MFP_LPM_KEEP_OUTPUT (0x1

<< 25)

Quoted 1 line

    /* set corresponding PGSR bit of

those marked MFP_LPM_KEEP_OUTPUT */

Quoted 1 line

if

((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT) &&

Quoted 1 line

%

Quoted 2 lines

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.

Quoted 10 lines

Quoted 2 lines

Quoted 2 lines

but for pins that have been configured for output

by

Quoted 4 lines

Quoted 2 lines

gpio_direction_out().
(Hint: *_KEEP_*).

MFP_LPM_KEEP_OUTPUT only takes effect when the system

enters sleep mode,

Quoted 1 line

at which point the GPIO directions (GPDR registers) are

forced from the

Quoted 5 lines

Quoted 1 line

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...

Quoted 17 lines

Quoted 2 lines

As far as I can tell MFP_LPM_KEEP_OUTPUT has no direct

connection to

Quoted 1 line

gpio_direction_output().

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

Quoted 2 lines

Quoted 2 lines

Also, I don't think this is a good idea, because

the patch

Quoted 2 lines

Quoted 2 lines

allows a pin
be configured as output, but does not forces a

value

Quoted 3 lines

Quoted 2 lines

(high/low)
and this is not safe.

Quoted 7 lines

Quoted 1 line

A value does not need to be provided until the system

enters sleep mode,

Quoted 1 line

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...

Quoted 13 lines

Quoted 1 line

353 static int

pxa2xx_mfp_suspend(void)

Quoted 2 lines

    354    {
    355       

int i;

Quoted 2 lines

    356    
    357       

/* set corresponding PGSR bit of those marked
MFP_LPM_KEEP_OUTPUT */

Quoted 1 line

    358       

for (i = 0; i < pxa_last_gpio; i++) {

Quoted 1 line

    359       

    if ((gpio_desc[i].config &

Quoted 8 lines

MFP_LPM_KEEP_OUTPUT) &&

Quoted 1 line

    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.

Quoted 33 lines

Quoted 2 lines

    361       

        if (GPLR(i) &

GPIO_bit(i))

Quoted 1 line

    362       

PGSR(gpio_to_bank(i)) |= GPIO_bit(i);

Quoted 1 line

    363       

else

Quoted 1 line

    364       

PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);

Quoted 1 line

    365       

    }

Quoted 1 line

    366       

}

Quoted 2 lines

    367    
    368       

for (i = 0; i <= gpio_to_bank(pxa_last_gpio); i++) {

Quoted 3 lines

    369    
         ...
    375       

    GPDR(i * 32) = gpdr_lpm[i];

Quoted 1 line

    376       

}

Quoted 1 line

    377       

return 0;

Quoted 3 lines

    378    }

However looking at pxa2xx_mfp_suspend() more closely it

is true that the

Quoted 1 line

GPDR registers are set before the PGSR values take

effect (when the

Quoted 8 lines

Quoted 2 lines

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.

Quoted 5 lines

Quoted 2 lines

So the presumption must be that the GPIOs configured as

DRIVE_HIGH,

Quoted 1 line

DRIVER_LOW and now KEEP_OUTPUT already have a valid

direction and value

Quoted 8 lines

Quoted 1 line

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.

Quoted 17 lines

Quoted 6 lines

Quoted 4 lines

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

Quoted 4 lines

LOW during sleep mode:

% fgrep MFP_LPM_KEEP_OUTPUT arch/arm/mach-pxa/hx4700.c

    GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT, /* BQ24022_nCHARGE_EN
*/

Quoted 1 line

    GPIO96_GPIO |

MFP_LPM_KEEP_OUTPUT, /* BQ24022_ISET2 */

Quoted 1 line

%

Quoted 4 lines

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.

Hi Paul,

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

Quoted 3 lines · Paul Parsons

Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg <[email protected]>

Quoted 5 lines · wrote:

Quoted 5 lines

Quoted 3 lines · Paul Parsons

Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg <[email protected]>

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines · wrote:

Quoted 4 lines

Hi Paul,

Quoted 1 line · Paul Parsons

Some MFP configurations specify

MFP_LPM_KEEP_OUTPUT to

Quoted 2 lines

Quoted 2 lines

preserve the gpio output

Quoted 1 line

value (HIGH or LOW) during sleep mode.

For

Quoted 3 lines

Quoted 3 lines

example:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

? ?

???GPIO72_GPIO |

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

Unfortunately MFP_LPM_KEEP_OUTPUT makes

no

Quoted 6 lines

Quoted 6 lines

special

Quoted 2 lines

Quoted 2 lines

provision for setting the

Quoted 1 line

sleep mode gpio direction, unlike

MFP_LPM_DRIVE_HIGH

Quoted 2 lines

Quoted 2 lines

and MFP_LPM_DRIVE_LOW.

Quoted 1 line

Consequently MFP configurations of the

form:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

? ?

???GPIO<n>_GPIO |

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

will set the sleep mode gpio direction

to

Quoted 3 lines

Quoted 3 lines

INPUT.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This patch forces the sleep mode gpio

direction

Quoted 3 lines

Quoted 3 lines

to

Quoted 2 lines

Quoted 2 lines

OUTPUT in cases where

Quoted 1 line

MFP_LPM_KEEP_OUTPUT was specified. This

brings

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

MFP_LPM_KEEP_OUTPUT into line

Quoted 1 line

with MFP_LPM_DRIVE_HIGH and

MFP_LPM_DRIVE_LOW.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Signed-off-by Paul Parsons <[email protected]>

---

diff --git

a/arch/arm/mach-pxa/mfp-pxa2xx.c

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

b/arch/arm/mach-pxa/mfp-pxa2xx.c

arch/arm/mach-pxa/mfp-pxa2xx.c static int

Quoted 4 lines

index b0a8428..3b443199 100644

arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0

@@ -96,6 +96,9 @@ static int

__mfp_config_gpio(unsigned

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

gpio, unsigned long c)

Quoted 1 line

? ? ? ?

? ? break;

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

? ? ? ? }
? ?
+? ? if (c &

MFP_LPM_KEEP_OUTPUT)

Quoted 1 line

Quoted 1 line

Quoted 1 line

+? ? ? ? is_out =

1;

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

+

MFP_LPM_KEEP_OUTPUT does not meant to be

used to

Quoted 4 lines

Quoted 4 lines

setup the

Quoted 3 lines

Quoted 1 line

mfp config,

As far as I can see, MFP_LPM_KEEP_OUTPUT is

*only* used

Quoted 10 lines

Quoted 10 lines

to setup the

Quoted 3 lines

mfp config:

% xzcat linux-3.4-rc1.tar.xz | fgrep -a

MFP_LPM_KEEP_OUTPUT

Quoted 1 line

? ???GPIO13_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /*
CORGI_GPIO_LED_ORANGE */

Quoted 1 line

? ???GPIO38_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /*

CORGI_GPIO_CHRG_ON

Quoted 7 lines

Quoted 7 lines

*/

Quoted 1 line

? ???GPIO43_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /*
CORGI_GPIO_CHRG_UKN */

Quoted 1 line

#define MFP_LPM_KEEP_OUTPUT? ? (0x1

<< 25)

Quoted 1 line

? ???/* set corresponding

PGSR bit of

Quoted 3 lines

Quoted 3 lines

those marked MFP_LPM_KEEP_OUTPUT */

Quoted 1 line

? ? ? ???if

((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT)

&&

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

%

How can you be certain that you don't break corgi,

with the

Quoted 3 lines

Quoted 1 line

proposed patch?

On corgi, the three GPIOs configured as

MFP_LPM_KEEP_OUTPUT:

Quoted 4 lines

? ???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!

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

but for pins that have been configured for

output

Quoted 5 lines

Quoted 5 lines

by

Quoted 4 lines

Quoted 2 lines

gpio_direction_out().
(Hint: *_KEEP_*).

MFP_LPM_KEEP_OUTPUT only takes effect when the

system

Quoted 2 lines

Quoted 2 lines

enters sleep mode,

Quoted 1 line

at which point the GPIO directions (GPDR

registers) are

Quoted 4 lines

Quoted 4 lines

forced from the

Quoted 1 line

values privately stored in gpdr_lpm[].

Correct, but with your patch, the direction will be

forced

Quoted 4 lines

Quoted 2 lines

to output,
but the value will be undefined...

The values will be defined in pxa2xx_mfp_suspend() when

it sets the PGSR

Quoted 1 line

registers. Or at least they will be on corgi and

hx4700, but see below...

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

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

As far as I can tell MFP_LPM_KEEP_OUTPUT has no

direct

Quoted 4 lines

Quoted 4 lines

connection to

Quoted 1 line

gpio_direction_output().

Well, it has... in the pxa2xx_mfp_suspend()

function you've

Quoted 4 lines

Quoted 4 lines

pasted below.

Quoted 2 lines

Quoted 2 lines

Also, I don't think this is a good idea,

because

Quoted 3 lines

Quoted 3 lines

the patch

Quoted 2 lines

Quoted 2 lines

allows a pin
be configured as output, but does not

forces a

Quoted 5 lines

Quoted 5 lines

value

Quoted 4 lines

Quoted 2 lines

(high/low)
and this is not safe.

A value does not need to be provided until the

system

Quoted 2 lines

Quoted 2 lines

enters sleep mode,

Quoted 1 line

at which point the value is provided via the

PGSR

Quoted 8 lines

Quoted 4 lines

registers.

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

OK, see below...

Quoted 1 line

Quoted 1 line

? ???353? ? static

int

Quoted 3 lines

Quoted 3 lines

pxa2xx_mfp_suspend(void)

Quoted 2 lines

? ???354? ? {
? ???355? ?

???

Quoted 3 lines

Quoted 3 lines

int i;

Quoted 2 lines

? ???356? ?
? ???357? ?

???

Quoted 3 lines

Quoted 3 lines

/* set corresponding PGSR bit of those marked
MFP_LPM_KEEP_OUTPUT */

Quoted 1 line

? ???358? ?

???

Quoted 2 lines

Quoted 2 lines

for (i = 0; i < pxa_last_gpio; i++) {

Quoted 1 line

? ???359? ?

???

Quoted 1 line

Quoted 1 line

? ???if ((gpio_desc[i].config

&

Quoted 2 lines

Quoted 2 lines

MFP_LPM_KEEP_OUTPUT) &&

Quoted 1 line

? ???360? ?

???

Quoted 1 line

Quoted 1 line

? ? ? ???(GPDR(i)

&

Quoted 4 lines

Quoted 4 lines

GPIO_bit(i))) {

Look at the check above...
the PGSR only gets set for the GPIOs configured to

output

Quoted 5 lines

Quoted 1 line

(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

Quoted 1 line

the current state of GPDR (via gpio_direction_output())

does not apply in

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 3 lines

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!

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

? ???361? ?

???

Quoted 1 line

Quoted 1 line

? ? ? ???if (GPLR(i)

&

Quoted 2 lines

Quoted 2 lines

GPIO_bit(i))

Quoted 1 line

? ???362? ?

???

Quoted 3 lines

Quoted 3 lines

? ? ? ? ? ?
PGSR(gpio_to_bank(i)) |= GPIO_bit(i);

Quoted 1 line

? ???363? ?

???

Quoted 2 lines

Quoted 2 lines

? ? ? ???else

Quoted 1 line

? ???364? ?

???

Quoted 3 lines

Quoted 3 lines

? ? ? ? ? ?
PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);

Quoted 1 line

? ???365? ?

???

Quoted 2 lines

Quoted 2 lines

? ???}

Quoted 1 line

? ???366? ?

???

Quoted 3 lines

Quoted 3 lines

}

Quoted 2 lines

? ???367? ?
? ???368? ?

???

Quoted 1 line

Quoted 1 line

for (i = 0; i <= gpio_to_bank(pxa_last_gpio);

i++) {

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

? ???369? ?
? ? ? ? ? ...
? ???375? ?

???

Quoted 1 line

Quoted 1 line

? ???GPDR(i * 32) =

gpdr_lpm[i];

Quoted 1 line

Quoted 1 line

Quoted 1 line

? ???376? ?

???

Quoted 2 lines

Quoted 2 lines

}

Quoted 1 line

? ???377? ?

???

Quoted 4 lines

Quoted 4 lines

return 0;

Quoted 3 lines

? ???378? ? }

However looking at pxa2xx_mfp_suspend() more

closely it

Quoted 2 lines

Quoted 2 lines

is true that the

Quoted 1 line

GPDR registers are set before the PGSR values

take

Quoted 3 lines

Quoted 3 lines

effect (when the

Quoted 2 lines

processor enters sleep mode).
That is no different from MFP_LPM_DRIVE_HIGH

or

Quoted 5 lines

Quoted 5 lines

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

Quoted 3 lines

Quoted 1 line

a _known value_.

KEEP_OUTPUT does the same where GPIOs are configured as

outputs, as is

Quoted 2 lines

the case with corgi and hx4700.
Your valid objection is for cases where GPIOs are

configured as inputs.

Quoted 1 line

So I presume that removing the (GPDR(i) &

GPIO_bit(i)) test will satisfy

Quoted 1 line

your objection.

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

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

So the presumption must be that the GPIOs

configured as

Quoted 2 lines

Quoted 2 lines

DRIVE_HIGH,

Quoted 1 line

DRIVER_LOW and now KEEP_OUTPUT already have a

valid

Quoted 2 lines

Quoted 2 lines

direction and value

Quoted 1 line

set at the point the GPDR registers are

forced.

Quoted 4 lines

Quoted 4 lines

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

Quoted 2 lines

Quoted 2 lines

effectively
force the GPIO to be output in sleep mode, but with

no value

Quoted 3 lines

Quoted 1 line

set!

Only in cases where the GPIO is used as an input in

normal mode but

Quoted 2 lines

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!

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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

MFP_LPM_DRIVE_<level>

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

?

The hx4700 has a couple of charging GPIOs which

we want

Quoted 4 lines

Quoted 4 lines

to keep HIGH or

Quoted 3 lines

LOW during sleep mode:

% fgrep MFP_LPM_KEEP_OUTPUT

arch/arm/mach-pxa/hx4700.c

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

? ???GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /*

BQ24022_nCHARGE_EN

Quoted 3 lines

Quoted 3 lines

*/

Quoted 1 line

? ???GPIO96_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /* BQ24022_ISET2

*/

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

%

I still, can't understand, why MFP_LPM_DRIVE_* does

not do

Quoted 2 lines

Quoted 2 lines

the job for you...
What's the real problem with using

MFP_LPM_DRIVE_*?

Quoted 3 lines

Quoted 1 line

Can't you specify it for those GPIO72/96?

The feeling was that if the unit was charging in normal

mode then it

Quoted 1 line

should remain charging in sleep mode, and if it wasn't

then it should

Quoted 1 line

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().

Quoted 2 lines

Actually that's an interim solution until the

bootloader can take control

Quoted 1 line

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.

Hello Igor,

Quoted 124 lines · 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...

Quoted 3 lines · Paul Parsons

Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg <[email protected]>

Quoted 5 lines · wrote:

Quoted 5 lines

Quoted 3 lines · Paul Parsons

Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg <[email protected]>

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines · wrote:

Quoted 4 lines

Hi Paul,

Quoted 1 line · Paul Parsons

Some MFP configurations specify

MFP_LPM_KEEP_OUTPUT to

Quoted 2 lines

Quoted 2 lines

preserve the gpio output

Quoted 1 line

value (HIGH or LOW) during sleep mode.

For

Quoted 3 lines

Quoted 3 lines

example:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

GPIO72_GPIO |

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

Unfortunately MFP_LPM_KEEP_OUTPUT makes

no

Quoted 6 lines

Quoted 6 lines

special

Quoted 2 lines

Quoted 2 lines

provision for setting the

Quoted 1 line

sleep mode gpio direction, unlike

MFP_LPM_DRIVE_HIGH

Quoted 2 lines

Quoted 2 lines

and MFP_LPM_DRIVE_LOW.

Quoted 1 line

Consequently MFP configurations of the

form:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

GPIO<n>_GPIO |

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

will set the sleep mode gpio direction

to

Quoted 3 lines

Quoted 3 lines

INPUT.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This patch forces the sleep mode gpio

direction

Quoted 3 lines

Quoted 3 lines

to

Quoted 2 lines

Quoted 2 lines

OUTPUT in cases where

Quoted 1 line

MFP_LPM_KEEP_OUTPUT was specified. This

brings

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

MFP_LPM_KEEP_OUTPUT into line

Quoted 1 line

with MFP_LPM_DRIVE_HIGH and

MFP_LPM_DRIVE_LOW.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Signed-off-by Paul Parsons <[email protected]>

---

diff --git

a/arch/arm/mach-pxa/mfp-pxa2xx.c

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

b/arch/arm/mach-pxa/mfp-pxa2xx.c

arch/arm/mach-pxa/mfp-pxa2xx.c static int

Quoted 4 lines

index b0a8428..3b443199 100644

arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0

@@ -96,6 +96,9 @@ static int

__mfp_config_gpio(unsigned

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

gpio, unsigned long c)

Quoted 1 line

    break;

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

        }

+ if (c &

MFP_LPM_KEEP_OUTPUT)

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ is_out =

1;

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

+

MFP_LPM_KEEP_OUTPUT does not meant to be

used to

Quoted 4 lines

Quoted 4 lines

setup the

Quoted 3 lines

Quoted 1 line

mfp config,

As far as I can see, MFP_LPM_KEEP_OUTPUT is

*only* used

Quoted 10 lines

Quoted 10 lines

to setup the

Quoted 3 lines

mfp config:

% xzcat linux-3.4-rc1.tar.xz | fgrep -a

MFP_LPM_KEEP_OUTPUT

Quoted 1 line

     GPIO13_GPIO |

MFP_LPM_KEEP_OUTPUT, /*
CORGI_GPIO_LED_ORANGE */

Quoted 1 line

     GPIO38_GPIO |

MFP_LPM_KEEP_OUTPUT, /*

CORGI_GPIO_CHRG_ON

Quoted 7 lines

Quoted 7 lines

*/

Quoted 1 line

     GPIO43_GPIO |

MFP_LPM_KEEP_OUTPUT, /*
CORGI_GPIO_CHRG_UKN */

Quoted 1 line

#define MFP_LPM_KEEP_OUTPUT (0x1

<< 25)

Quoted 1 line

     /* set corresponding

PGSR bit of

Quoted 3 lines

Quoted 3 lines

those marked MFP_LPM_KEEP_OUTPUT */

Quoted 1 line

if

((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT)

&&

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

%

How can you be certain that you don't break corgi,

with the

Quoted 3 lines

Quoted 1 line

proposed patch?

On corgi, the three GPIOs configured as

MFP_LPM_KEEP_OUTPUT:

Quoted 4 lines

CORGI_GPIO_LED_ORANGE
CORGI_GPIO_CHRG_ON
CORGI_GPIO_CHRG_UKN
are all used as outputs.

Quoted 3 lines

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

Quoted 98 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

but for pins that have been configured for

output

Quoted 5 lines

Quoted 5 lines

by

Quoted 4 lines

Quoted 2 lines

gpio_direction_out().
(Hint: *_KEEP_*).

MFP_LPM_KEEP_OUTPUT only takes effect when the

system

Quoted 2 lines

Quoted 2 lines

enters sleep mode,

Quoted 1 line

at which point the GPIO directions (GPDR

registers) are

Quoted 4 lines

Quoted 4 lines

forced from the

Quoted 1 line

values privately stored in gpdr_lpm[].

Correct, but with your patch, the direction will be

forced

Quoted 4 lines

Quoted 2 lines

to output,
but the value will be undefined...

The values will be defined in pxa2xx_mfp_suspend() when

it sets the PGSR

Quoted 1 line

registers. Or at least they will be on corgi and

hx4700, but see below...

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

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

As far as I can tell MFP_LPM_KEEP_OUTPUT has no

direct

Quoted 4 lines

Quoted 4 lines

connection to

Quoted 1 line

gpio_direction_output().

Well, it has... in the pxa2xx_mfp_suspend()

function you've

Quoted 4 lines

Quoted 4 lines

pasted below.

Quoted 2 lines

Quoted 2 lines

Also, I don't think this is a good idea,

because

Quoted 3 lines

Quoted 3 lines

the patch

Quoted 2 lines

Quoted 2 lines

allows a pin
be configured as output, but does not

forces a

Quoted 5 lines

Quoted 5 lines

value

Quoted 4 lines

Quoted 2 lines

(high/low)
and this is not safe.

A value does not need to be provided until the

system

Quoted 2 lines

Quoted 2 lines

enters sleep mode,

Quoted 1 line

at which point the value is provided via the

PGSR

Quoted 8 lines

Quoted 4 lines

registers.

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

OK, see below...

Quoted 1 line

Quoted 1 line

353 static

int

Quoted 8 lines

Quoted 7 lines

pxa2xx_mfp_suspend(void)

Quoted 2 lines

     354    {
     355   

int i;

Quoted 2 lines

     356    
     357   

Quoted 8 lines

Quoted 8 lines

/* set corresponding PGSR bit of those marked
MFP_LPM_KEEP_OUTPUT */

Quoted 1 line

     358   

for (i = 0; i < pxa_last_gpio; i++) {

Quoted 1 line

     359   

if ((gpio_desc[i].config

&

Quoted 4 lines

Quoted 4 lines

MFP_LPM_KEEP_OUTPUT) &&

Quoted 1 line

     360   

         (GPDR(i)

&

Quoted 4 lines

Quoted 4 lines

GPIO_bit(i))) {

Look at the check above...
the PGSR only gets set for the GPIOs configured to

output

Quoted 5 lines

Quoted 1 line

(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

Quoted 1 line

the current state of GPDR (via gpio_direction_output())

Quoted 8 lines

does not apply in

Quoted 1 line

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?

Quoted 7 lines

Quoted 4 lines

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?

Quoted 9 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 151 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

     361   

         if (GPLR(i)

&

Quoted 14 lines

Quoted 13 lines

GPIO_bit(i))

Quoted 1 line

     362   

PGSR(gpio_to_bank(i)) |= GPIO_bit(i);

Quoted 1 line

     363   

else

Quoted 1 line

     364   

PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);

Quoted 1 line

     365   

Quoted 8 lines

Quoted 8 lines

     }

Quoted 1 line

     366   

}

Quoted 2 lines

     367    
     368   

for (i = 0; i <= gpio_to_bank(pxa_last_gpio);

i++) {

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

     369    
          ...
     375   

     GPDR(i * 32) =

gpdr_lpm[i];

Quoted 9 lines

Quoted 9 lines

Quoted 1 line

     376   

}

Quoted 1 line

     377   

return 0;

Quoted 3 lines

     378    }

However looking at pxa2xx_mfp_suspend() more

closely it

Quoted 2 lines

Quoted 2 lines

is true that the

Quoted 1 line

GPDR registers are set before the PGSR values

take

Quoted 3 lines

Quoted 3 lines

effect (when the

Quoted 2 lines

processor enters sleep mode).
That is no different from MFP_LPM_DRIVE_HIGH

or

Quoted 5 lines

Quoted 5 lines

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

Quoted 3 lines

Quoted 1 line

a _known value_.

KEEP_OUTPUT does the same where GPIOs are configured as

outputs, as is

Quoted 2 lines

the case with corgi and hx4700.
Your valid objection is for cases where GPIOs are

configured as inputs.

Quoted 1 line

So I presume that removing the (GPDR(i) &

GPIO_bit(i)) test will satisfy

Quoted 1 line

your objection.

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

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

So the presumption must be that the GPIOs

configured as

Quoted 2 lines

Quoted 2 lines

DRIVE_HIGH,

Quoted 1 line

DRIVER_LOW and now KEEP_OUTPUT already have a

valid

Quoted 2 lines

Quoted 2 lines

direction and value

Quoted 1 line

set at the point the GPDR registers are

forced.

Quoted 4 lines

Quoted 4 lines

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

Quoted 2 lines

Quoted 2 lines

effectively
force the GPIO to be output in sleep mode, but with

no value

Quoted 3 lines

Quoted 1 line

set!

Only in cases where the GPIO is used as an input in

normal mode but

Quoted 2 lines

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!

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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

MFP_LPM_DRIVE_<level>

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

?

The hx4700 has a couple of charging GPIOs which

we want

Quoted 4 lines

Quoted 4 lines

to keep HIGH or

Quoted 3 lines

LOW during sleep mode:

% fgrep MFP_LPM_KEEP_OUTPUT

arch/arm/mach-pxa/hx4700.c

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

     GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT, /*

BQ24022_nCHARGE_EN

Quoted 3 lines

Quoted 3 lines

*/

Quoted 1 line

     GPIO96_GPIO |

MFP_LPM_KEEP_OUTPUT, /* BQ24022_ISET2

*/

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

%

I still, can't understand, why MFP_LPM_DRIVE_* does

not do

Quoted 2 lines

Quoted 2 lines

the job for you...
What's the real problem with using

MFP_LPM_DRIVE_*?

Quoted 3 lines

Quoted 1 line

Can't you specify it for those GPIO72/96?

The feeling was that if the unit was charging in normal

mode then it

Quoted 1 line

should remain charging in sleep mode, and if it wasn't

then it should

Quoted 1 line

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

Quoted 7 lines

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.

From git log:

Quoted 6 lines

Quoted 3 lines

Actually that's an interim solution until the

bootloader can take control

Quoted 1 line

of sleep mode charging, but we're not there yet.

Quoted 7 lines

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

Quoted 3 lines · Paul Parsons

Hello Igor,

--- On Mon, 2/4/12, Igor Grinberg <[email protected]>

Quoted 3 lines · wrote:

Quoted 3 lines

Hi Paul,

Can you, please, configure your mailer to _not_

wrap the

Quoted 6 lines

Quoted 6 lines

text?
It is rally hard to read it after a few replies...

Quoted 2 lines · Paul Parsons

Hello Igor,

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

--- On Sun, 1/4/12, Igor Grinberg <[email protected]>

Quoted 5 lines · wrote:

Quoted 5 lines

Quoted 3 lines · Paul Parsons

Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg

<[email protected]>

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines · wrote:

Quoted 3 lines

Hi Paul,

On 03/31/12 15:20, Paul Parsons

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Some MFP configurations

specify

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

MFP_LPM_KEEP_OUTPUT to

Quoted 2 lines

Quoted 2 lines

preserve the gpio output

Quoted 1 line

value (HIGH or LOW) during

sleep mode.

Quoted 8 lines

Quoted 8 lines

For

Quoted 3 lines

Quoted 3 lines

example:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

? ?

? ? GPIO72_GPIO |

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

Unfortunately

MFP_LPM_KEEP_OUTPUT makes

Quoted 4 lines

Quoted 4 lines

no

Quoted 3 lines

Quoted 3 lines

special

Quoted 2 lines

Quoted 2 lines

provision for setting the

Quoted 1 line

sleep mode gpio direction,

unlike

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

MFP_LPM_DRIVE_HIGH

Quoted 2 lines

Quoted 2 lines

and MFP_LPM_DRIVE_LOW.

Quoted 1 line

Consequently MFP configurations

of the

Quoted 7 lines

Quoted 7 lines

form:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

? ?

? ? GPIO<n>_GPIO |

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

MFP_LPM_KEEP_OUTPUT,

Quoted 2 lines

will set the sleep mode gpio

direction

Quoted 4 lines

Quoted 4 lines

to

Quoted 3 lines

Quoted 3 lines

INPUT.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This patch forces the sleep

mode gpio

Quoted 4 lines

Quoted 4 lines

direction

Quoted 3 lines

Quoted 3 lines

to

Quoted 2 lines

Quoted 2 lines

OUTPUT in cases where

Quoted 1 line

MFP_LPM_KEEP_OUTPUT was

specified. This

Quoted 6 lines

Quoted 6 lines

brings

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

MFP_LPM_KEEP_OUTPUT into line

Quoted 1 line

with MFP_LPM_DRIVE_HIGH and

MFP_LPM_DRIVE_LOW.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Signed-off-by Paul Parsons

<[email protected]>

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

---

diff --git

a/arch/arm/mach-pxa/mfp-pxa2xx.c

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

b/arch/arm/mach-pxa/mfp-pxa2xx.c

Quoted 2 lines

index b0a8428..3b443199 100644
---

a/arch/arm/mach-pxa/mfp-pxa2xx.c

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+++

b/arch/arm/mach-pxa/mfp-pxa2xx.c

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

Quoted 1 line

Review static int

Quoted 1 line

(quoted diff) +0 -0

@@ -96,6 +96,9 @@ static int

__mfp_config_gpio(unsigned

Quoted 2 lines

Quoted 2 lines

gpio, unsigned long c)

Quoted 1 line

? ? ?

???

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

? ???break;

Quoted 1 line

Quoted 1 line

Quoted 1 line

? ? ?

???}

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

? ???
+? ? if (c &

MFP_LPM_KEEP_OUTPUT)

Quoted 1 line

Quoted 1 line

Quoted 1 line

+? ? ? ?

is_out =

Quoted 4 lines

Quoted 4 lines

1;

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

+

MFP_LPM_KEEP_OUTPUT does not meant

to be

Quoted 5 lines

Quoted 5 lines

used to

Quoted 4 lines

Quoted 4 lines

setup the

Quoted 3 lines

Quoted 1 line

mfp config,

As far as I can see,

MFP_LPM_KEEP_OUTPUT is

Quoted 5 lines

Quoted 5 lines

*only* used

Quoted 4 lines

Quoted 4 lines

to setup the

Quoted 3 lines

mfp config:

% xzcat linux-3.4-rc1.tar.xz | fgrep

-a

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

MFP_LPM_KEEP_OUTPUT

Quoted 1 line

? ? ? GPIO13_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /* CORGI_GPIO_LED_ORANGE */

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

? ? ? GPIO38_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /*

CORGI_GPIO_CHRG_ON

Quoted 5 lines

Quoted 5 lines

*/

Quoted 1 line

? ? ? GPIO43_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /* CORGI_GPIO_CHRG_UKN */

Quoted 1 line

#define MFP_LPM_KEEP_OUTPUT?

? (0x1

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

<< 25)

Quoted 1 line

? ? ? /* set

corresponding

Quoted 4 lines

Quoted 4 lines

PGSR bit of

Quoted 3 lines

Quoted 3 lines

those marked MFP_LPM_KEEP_OUTPUT */

Quoted 1 line

? ? ? ? ? if

((gpio_desc[i].config &

MFP_LPM_KEEP_OUTPUT)

Quoted 4 lines

Quoted 4 lines

&&

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

%

How can you be certain that you don't break

corgi,

Quoted 11 lines

Quoted 11 lines

with the

Quoted 3 lines

Quoted 1 line

proposed patch?

On corgi, the three GPIOs configured as

MFP_LPM_KEEP_OUTPUT:

Quoted 4 lines

? ? ? 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

Quoted 1 line

Quoted 1 line

does not do a direction configuration... and IMO,

it should

Quoted 6 lines

Quoted 1 line

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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

but for pins that have been

configured for

Quoted 6 lines

Quoted 6 lines

output

Quoted 5 lines

Quoted 5 lines

by

Quoted 4 lines

Quoted 2 lines

gpio_direction_out().
(Hint: *_KEEP_*).

MFP_LPM_KEEP_OUTPUT only takes effect

when the

Quoted 3 lines

Quoted 3 lines

system

Quoted 2 lines

Quoted 2 lines

enters sleep mode,

Quoted 1 line

at which point the GPIO directions

(GPDR

Quoted 5 lines

Quoted 5 lines

registers) are

Quoted 4 lines

Quoted 4 lines

forced from the

Quoted 1 line

values privately stored in gpdr_lpm[].

Correct, but with your patch, the direction

will be

Quoted 5 lines

Quoted 5 lines

forced

Quoted 4 lines

Quoted 2 lines

to output,
but the value will be undefined...

The values will be defined in

pxa2xx_mfp_suspend() when

Quoted 2 lines

Quoted 2 lines

it sets the PGSR

Quoted 1 line

registers. Or at least they will be on corgi

and

Quoted 3 lines

Quoted 3 lines

hx4700, but see below...

No, they will not, because the direction can be

input...

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

As far as I can tell

MFP_LPM_KEEP_OUTPUT has no

Quoted 5 lines

Quoted 5 lines

direct

Quoted 4 lines

Quoted 4 lines

connection to

Quoted 1 line

gpio_direction_output().

Well, it has... in the

pxa2xx_mfp_suspend()

Quoted 5 lines

Quoted 5 lines

function you've

Quoted 4 lines

Quoted 4 lines

pasted below.

Quoted 2 lines

Quoted 2 lines

Also, I don't think this is a good

idea,

Quoted 4 lines

Quoted 4 lines

because

Quoted 3 lines

Quoted 3 lines

the patch

Quoted 2 lines

Quoted 2 lines

allows a pin
be configured as output, but does

not

Quoted 6 lines

Quoted 6 lines

forces a

Quoted 5 lines

Quoted 5 lines

value

Quoted 4 lines

Quoted 2 lines

(high/low)
and this is not safe.

A value does not need to be provided

until the

Quoted 3 lines

Quoted 3 lines

system

Quoted 2 lines

Quoted 2 lines

enters sleep mode,

Quoted 1 line

at which point the value is provided

via the

Quoted 4 lines

Quoted 4 lines

PGSR

Quoted 3 lines

Quoted 3 lines

registers.

Yes, but you don't force the PGSR to be

set...

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

and that is the problem.

OK, see below...

Quoted 1 line

Quoted 1 line

? ? ? 353? ?

static

Quoted 4 lines

Quoted 4 lines

int

Quoted 3 lines

Quoted 3 lines

pxa2xx_mfp_suspend(void)

Quoted 2 lines

? ? ? 354? ? {
? ? ?

355???

Quoted 4 lines

Quoted 4 lines

? ?

Quoted 3 lines

Quoted 3 lines

int i;

Quoted 2 lines

? ? ? 356? ?
? ? ?

357???

Quoted 2 lines

Quoted 2 lines

? ?

Quoted 1 line

Quoted 1 line

/* set corresponding PGSR bit of those

marked

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

MFP_LPM_KEEP_OUTPUT */

Quoted 1 line

? ? ?

358???

Quoted 3 lines

Quoted 3 lines

? ?

Quoted 2 lines

Quoted 2 lines

for (i = 0; i < pxa_last_gpio; i++) {

Quoted 1 line

? ? ?

359???

Quoted 2 lines

Quoted 2 lines

? ?

Quoted 1 line

Quoted 1 line

? ? ? if

((gpio_desc[i].config

Quoted 3 lines

Quoted 3 lines

&

Quoted 2 lines

Quoted 2 lines

MFP_LPM_KEEP_OUTPUT) &&

Quoted 1 line

? ? ?

360???

Quoted 7 lines

Quoted 7 lines

? ?

Quoted 1 line

Quoted 1 line

? ? ? ? ? (GPDR(i)

&

Quoted 4 lines

Quoted 4 lines

GPIO_bit(i))) {

Look at the check above...
the PGSR only gets set for the GPIOs

configured to

Quoted 2 lines

Quoted 2 lines

output

Quoted 1 line

Quoted 1 line

(presumably by gpio_direction_output()

function).

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

OK, I understand your objection here.

My feeling is that the (GPDR(i) &

GPIO_bit(i)) test

Quoted 2 lines

Quoted 2 lines

is unwanted because

Quoted 1 line

the current state of GPDR (via

gpio_direction_output())

Quoted 4 lines

Quoted 4 lines

does not apply in

Quoted 1 line

sleep mode,

It gets copied to PGSR in the pxa2xx_mfp_suspend()

and

Quoted 2 lines

Quoted 2 lines

therefore it does apply!
That is exactly the meaning of

MFP_LPM_KEEP_OUTPUT:

Quoted 1 line

Quoted 1 line

_If_ the pin is output, then _keep_ it output with

the

Quoted 3 lines

Quoted 1 line

_runtime_ value.

OK, I see, your interpretation is that

MFP_LPM_KEEP_OUTPUT is optional:

Quoted 3 lines

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

Quoted 1 line

applies regardless of the current pin direction,

exactly like

Quoted 3 lines

MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.

If your interpretation is correct then DRIVE_HIGH and

DRIVE_LOW will

Quoted 3 lines

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

Quoted 1 line

of GPDR in pxa2xx_mfp_suspend() is the correct thing to

do.

Quoted 2 lines

Do you agree that KEEP_OUTPUT, DRIVE_HIGH and DRIVE_LOW

should all be

Quoted 1 line

consistent in being either optional or mandatory sleep

mode

Quoted 1 line

configurations?

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

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

yet for some reason it is being allowed to

prevent the

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 2 lines

the
presumption must be that GPIOs configured as DRIVE_HIGH

or DRIVE_LOW are

Quoted 1 line

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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Consequently the (GPDR(i) & GPIO_bit(i))

test

Quoted 4 lines

Quoted 4 lines

should be removed.

No, absolutely not!
This will change the meaning of the

MFP_LPM_KEEP_OUTPUT

Quoted 3 lines

Quoted 1 line

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.

Quoted 2 lines

Quoted 2 lines

I have a feeling that you are trying to solve a

problem that

Quoted 4 lines

Quoted 1 line

does not exist!

The problem is that MFP configurations of the form:
? ???GPIO<n>_GPIO |

MFP_LPM_KEEP_OUTPUT,

Quoted 1 line

will always set the sleep mode gpio direction to

INPUT.

Quoted 1 line

That is the case with corgi and would also be the case

with hx4700.

Quoted 1 line

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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

? ? ?

361???

Quoted 2 lines

Quoted 2 lines

? ?

Quoted 1 line

Quoted 1 line

? ? ? ? ? if

(GPLR(i)

Quoted 3 lines

Quoted 3 lines

&

Quoted 2 lines

Quoted 2 lines

GPIO_bit(i))

Quoted 1 line

? ? ?

362???

Quoted 2 lines

Quoted 2 lines

? ?

Quoted 1 line

Quoted 1 line

? ? ? ? ?

???

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

PGSR(gpio_to_bank(i)) |= GPIO_bit(i);

Quoted 1 line

? ? ?

363???

Quoted 3 lines

Quoted 3 lines

? ?

Quoted 2 lines

Quoted 2 lines

? ? ? ? ? else

Quoted 1 line

? ? ?

364???

Quoted 2 lines

Quoted 2 lines

? ?

Quoted 1 line

Quoted 1 line

? ? ? ? ?

???

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);

Quoted 1 line

? ? ?

365???

Quoted 3 lines

Quoted 3 lines

? ?

Quoted 2 lines

Quoted 2 lines

? ? ? }

Quoted 1 line

? ? ?

366???

Quoted 4 lines

Quoted 4 lines

? ?

Quoted 3 lines

Quoted 3 lines

}

Quoted 2 lines

? ? ? 367? ?
? ? ?

368???

Quoted 2 lines

Quoted 2 lines

? ?

Quoted 1 line

Quoted 1 line

for (i = 0; i <=

gpio_to_bank(pxa_last_gpio);

Quoted 3 lines

Quoted 3 lines

i++) {

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

? ? ? 369? ?
? ? ? ?

???...

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

? ? ?

375???

Quoted 4 lines

Quoted 4 lines

? ?

Quoted 1 line

Quoted 1 line

? ? ? GPDR(i * 32) =

gpdr_lpm[i];

Quoted 1 line

Quoted 1 line

Quoted 1 line

? ? ?

376???

Quoted 3 lines

Quoted 3 lines

? ?

Quoted 2 lines

Quoted 2 lines

}

Quoted 1 line

? ? ?

377???

Quoted 5 lines

Quoted 5 lines

? ?

Quoted 4 lines

Quoted 4 lines

return 0;

Quoted 3 lines

? ? ? 378? ? }

However looking at pxa2xx_mfp_suspend()

more

Quoted 3 lines

Quoted 3 lines

closely it

Quoted 2 lines

Quoted 2 lines

is true that the

Quoted 1 line

GPDR registers are set before the PGSR

values

Quoted 4 lines

Quoted 4 lines

take

Quoted 3 lines

Quoted 3 lines

effect (when the

Quoted 2 lines

processor enters sleep mode).
That is no different from

MFP_LPM_DRIVE_HIGH

Quoted 4 lines

Quoted 4 lines

or

Quoted 3 lines

Quoted 3 lines

MFP_LPM_DRIVE_LOW though.

Well, it is different, because the in case

of

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

MFP_LPM_DRIVE_*,
the PGSR is set in the __mfp_config_gpio()

function

Quoted 4 lines

Quoted 4 lines

and has

Quoted 3 lines

Quoted 1 line

a _known value_.

KEEP_OUTPUT does the same where GPIOs are

configured as

Quoted 3 lines

Quoted 3 lines

outputs, as is

Quoted 2 lines

the case with corgi and hx4700.
Your valid objection is for cases where GPIOs

are

Quoted 6 lines

Quoted 6 lines

configured as inputs.

Quoted 1 line

So I presume that removing the (GPDR(i) &

GPIO_bit(i)) test will satisfy

Quoted 1 line

your objection.

again, no, because it will change the meaning of

the

Quoted 4 lines

Quoted 4 lines

MFP_LPM_KEEP_OUTPUT symbol!

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

So the presumption must be that the

GPIOs

Quoted 3 lines

Quoted 3 lines

configured as

Quoted 2 lines

Quoted 2 lines

DRIVE_HIGH,

Quoted 1 line

DRIVER_LOW and now KEEP_OUTPUT already

have a

Quoted 3 lines

Quoted 3 lines

valid

Quoted 2 lines

Quoted 2 lines

direction and value

Quoted 1 line

set at the point the GPDR registers

are

Quoted 4 lines

Quoted 4 lines

forced.

Quoted 3 lines

Quoted 3 lines

Yes. That is exactly my point here. MFP_LPM_DRIVE_* has the value determined

and set.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

MFP_LPM_KEEP_OUTPUT does not, but with your

patch

Quoted 3 lines

Quoted 3 lines

it will

Quoted 2 lines

Quoted 2 lines

effectively
force the GPIO to be output in sleep mode,

but with

Quoted 4 lines

Quoted 4 lines

no value

Quoted 3 lines

Quoted 1 line

set!

Only in cases where the GPIO is used as an

input in

Quoted 3 lines

Quoted 3 lines

normal mode but

Quoted 2 lines

configured to be an output in sleep mode.
Removing the (GPDR(i) & GPIO_bit(i)) test

addresses

Quoted 3 lines

Quoted 3 lines

that case.

No, because, if the GPIO is input, you will

configure it for

Quoted 2 lines

Quoted 2 lines

output
and set some arbitrary value that is seen on the

input!

Quoted 9 lines

Quoted 9 lines

This is bad!

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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

MFP_LPM_DRIVE_<level>

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

?

The hx4700 has a couple of charging

GPIOs which

Quoted 4 lines

Quoted 4 lines

we want

Quoted 3 lines

Quoted 3 lines

to keep HIGH or

Quoted 2 lines

LOW during sleep mode:

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

% fgrep MFP_LPM_KEEP_OUTPUT

arch/arm/mach-pxa/hx4700.c

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

? ? ? GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /*

BQ24022_nCHARGE_EN

Quoted 3 lines

Quoted 3 lines

*/

Quoted 1 line

? ? ? GPIO96_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /*

BQ24022_ISET2

Quoted 4 lines

Quoted 4 lines

*/

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

%

I still, can't understand, why

MFP_LPM_DRIVE_* does

Quoted 7 lines

Quoted 7 lines

not do

Quoted 2 lines

Quoted 2 lines

the job for you...
What's the real problem with using

MFP_LPM_DRIVE_*?

Quoted 3 lines

Quoted 1 line

Can't you specify it for those GPIO72/96?

The feeling was that if the unit was charging

in normal

Quoted 2 lines

Quoted 2 lines

mode then it

Quoted 1 line

should remain charging in sleep mode, and if it

wasn't

Quoted 8 lines

Quoted 8 lines

then it should

Quoted 1 line

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

Quoted 1 line

Quoted 1 line

required GPIO number)for your used GPIOs and use it

in MFP

Quoted 2 lines

Quoted 2 lines

config
structure for hx4700 along with

MFP_LPM_KEEP_OUTPUT.

Quoted 4 lines

Quoted 4 lines

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

MFP_LPM_KEEP_OUTPUT

Quoted 2 lines

Quoted 2 lines

The above should work for you as you would expect -

will set

Quoted 2 lines

Quoted 2 lines

the GPIO
to be output low by default, then you will

reconfigure it to

Quoted 2 lines

Quoted 2 lines

the
appropriate level from your charger driver and the

new level

Quoted 5 lines

Quoted 2 lines

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

Quoted 1 line

DRIVE_LOW are optional or mandatory sleep mode

configurations. That may

Quoted 1 line

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 <[email protected]>
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 <[email protected]> ---------------cut---------------------

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

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Actually that's an interim solution until the

bootloader can take control

Quoted 1 line

of sleep mode charging, but we're not there

yet.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

still you don't want the turn on/off of the charger

in the

Quoted 5 lines

Quoted 5 lines

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

Quoted 1 line

Quoted 1 line

and we don't want to break this assumption.

Quoted 4 lines

Quoted 1 line

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?

Quoted 1 line

Quoted 1 line

Quoted 1 line

But since DRIVE_HIGH or

DRIVE_LOW currently don't

Quoted 5 lines

Quoted 3 lines

prevent that,

Don't prevent what?

Don't prevent a GPIO input being changed to an output

when entering

Quoted 2 lines

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?

Quoted 2 lines

By the way, there is still the issue of DRIVE_HIGH and

DRIVE_LOW

Quoted 1 line

forcing the GPIO direction to output in

pxa2xx_mfp_suspend()

Quoted 2 lines

*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.

Hello Haojian,

Quoted 27 lines · Haojian Zhuang

Quoted 1 line

Quoted 1 line

Quoted 1 line

But since DRIVE_HIGH or

DRIVE_LOW currently don't

Quoted 5 lines

Quoted 3 lines

prevent that,

Don't prevent what?

Don't prevent a GPIO input being changed to an output

when entering

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 10 lines

Quoted 3 lines

By the way, there is still the issue of DRIVE_HIGH and

DRIVE_LOW

Quoted 1 line

forcing the GPIO direction to output in

pxa2xx_mfp_suspend()

Quoted 2 lines

*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.

Hello Igor,

Quoted 13 lines

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

On corgi, the three GPIOs configured as

MFP_LPM_KEEP_OUTPUT:

Quoted 4 lines

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

Quoted 1 line

Quoted 1 line

does not do a direction configuration... and IMO,

it should

Quoted 2 lines

Quoted 1 line

not!

Quoted 8 lines

Quoted 4 lines

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.

Quoted 31 lines

Quoted 2 lines

Do you agree that KEEP_OUTPUT, DRIVE_HIGH and DRIVE_LOW

should all be

Quoted 1 line

consistent in being either optional or mandatory sleep

mode

Quoted 1 line

configurations?

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

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

yet for some reason it is being allowed to

prevent the

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 1 line

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.

Quoted 4 lines

Quoted 1 line

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?

Quoted 45 lines

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.

Quoted 2 lines

the
presumption must be that GPIOs configured as DRIVE_HIGH

or DRIVE_LOW are

Quoted 1 line

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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Consequently the (GPDR(i) & GPIO_bit(i))

test

Quoted 4 lines

Quoted 4 lines

should be removed.

No, absolutely not!
This will change the meaning of the

MFP_LPM_KEEP_OUTPUT

Quoted 3 lines

Quoted 1 line

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.

Quoted 2 lines

Quoted 2 lines

I have a feeling that you are trying to solve a

problem that

Quoted 4 lines

Quoted 1 line

does not exist!

The problem is that MFP configurations of the form:

     GPIO<n>_GPIO |

MFP_LPM_KEEP_OUTPUT,

Quoted 1 line

will always set the sleep mode gpio direction to

INPUT.

Quoted 1 line

That is the case with corgi and would also be the case

Quoted 6 lines

with hx4700.

Quoted 1 line

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.

Quoted 6 lines

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.

Quoted 66 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

The feeling was that if the unit was charging

in normal

Quoted 2 lines

Quoted 2 lines

mode then it

Quoted 1 line

should remain charging in sleep mode, and if it

wasn't

Quoted 8 lines

Quoted 8 lines

then it should

Quoted 1 line

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

Quoted 1 line

Quoted 1 line

required GPIO number)for your used GPIOs and use it

in MFP

Quoted 2 lines

Quoted 2 lines

config
structure for hx4700 along with

MFP_LPM_KEEP_OUTPUT.

Quoted 4 lines

Quoted 4 lines

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

MFP_LPM_KEEP_OUTPUT

Quoted 2 lines

Quoted 2 lines

The above should work for you as you would expect -

will set

Quoted 2 lines

Quoted 2 lines

the GPIO
to be output low by default, then you will

reconfigure it to

Quoted 2 lines

Quoted 2 lines

the
appropriate level from your charger driver and the

new level

Quoted 5 lines

Quoted 2 lines

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

Quoted 1 line

DRIVE_LOW are optional or mandatory sleep mode

configurations. That may

Quoted 1 line

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 <[email protected]>
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.

Quoted 4 lines

Signed-off-by Eric Miao <[email protected]>

---------------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.

Quoted 2 lines · Paul Parsons

Hello Igor,

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

On corgi, the three GPIOs configured

as

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

MFP_LPM_KEEP_OUTPUT:

Quoted 1 line

? ?

???CORGI_GPIO_LED_ORANGE

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

? ?

???CORGI_GPIO_CHRG_ON

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

? ?

???CORGI_GPIO_CHRG_UKN

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

are all used as outputs.

Have you verified it, because currently

MFP_LPM_KEEP_OUTPUT

Quoted 1 line

Quoted 1 line

does not do a direction configuration...

and IMO,

Quoted 5 lines

Quoted 5 lines

it should

Quoted 4 lines

Quoted 1 line

not!

Yes, all 3 GPIOs are configured as outputs:
CORGI_GPIO_LED_ORANGE in

drivers/leds/leds-gpio.c

Quoted 1 line

Quoted 1 line

Quoted 1 line

CORGI_GPIO_CHRG_ON in

arch/arm/mach-pxa/corgi_pm.c

Quoted 1 line

Quoted 1 line

Quoted 1 line

CORGI_GPIO_CHRG_UKN in

arch/arm/mach-pxa/corgi_pm.c

Quoted 2 lines

Quoted 2 lines

Good, then they will be kept output also in the

suspend