Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il> wrote:

On 04/01/12 13:56, Paul Parsons
wrote:

quoted

Hello Igor,

--- On Sun, 1/4/12, Igor Grinberg <grinberg@compulab.co.il>

wrote:

quoted

quoted

Hi Paul,

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

quoted

Some MFP configurations specify

MFP_LPM_KEEP_OUTPUT to

quoted

quoted

preserve the gpio output

quoted

value (HIGH or LOW) during sleep mode. For

example:

quoted

quoted

quoted

? ? ? GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT,

quoted

Unfortunately MFP_LPM_KEEP_OUTPUT makes no

special

quoted

quoted

provision for setting the

quoted

sleep mode gpio direction, unlike

MFP_LPM_DRIVE_HIGH

quoted

quoted

and MFP_LPM_DRIVE_LOW.

quoted

Consequently MFP configurations of the form:

? ? ? GPIO<n>_GPIO |

MFP_LPM_KEEP_OUTPUT,

quoted

will set the sleep mode gpio direction to

INPUT.

quoted

quoted

quoted

This patch forces the sleep mode gpio direction

to

quoted

quoted

OUTPUT in cases where

quoted

MFP_LPM_KEEP_OUTPUT was specified. This brings

MFP_LPM_KEEP_OUTPUT into line

quoted

with MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.

Signed-off-by: Paul Parsons <redacted>
---

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

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

quoted

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

quoted

quoted

gpio, unsigned long c)

quoted

? ? ? ?

???break;

quoted

quoted

quoted

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

MFP_LPM_KEEP_OUTPUT)

quoted

quoted

quoted

+? ? ? ? is_out = 1;
+

MFP_LPM_KEEP_OUTPUT does not meant to be used to

setup the

quoted

quoted

mfp config,

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

to setup the

quoted

mfp config:

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

MFP_LPM_KEEP_OUTPUT

quoted

??? GPIO13_GPIO |

MFP_LPM_KEEP_OUTPUT,??? /*
CORGI_GPIO_LED_ORANGE */

quoted

??? GPIO38_GPIO |

MFP_LPM_KEEP_OUTPUT,??? /* CORGI_GPIO_CHRG_ON
*/

quoted

??? GPIO43_GPIO |

MFP_LPM_KEEP_OUTPUT,??? /*
CORGI_GPIO_CHRG_UKN */

quoted

#define MFP_LPM_KEEP_OUTPUT??? (0x1

<< 25)

quoted

??? /* set corresponding PGSR bit of

those marked MFP_LPM_KEEP_OUTPUT */

quoted

??? ??? if

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

quoted

% 

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

quoted

but for pins that have been configured for output

by

quoted

quoted

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

MFP_LPM_KEEP_OUTPUT only takes effect when the system

enters sleep mode,

quoted

at which point the GPIO directions (GPDR registers) are

forced from the

quoted

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

As far as I can tell MFP_LPM_KEEP_OUTPUT has no direct

connection to

quoted

gpio_direction_output().

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

quoted

quoted

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

the patch

quoted

quoted

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

value

quoted

quoted

(high/low)
and this is not safe.

A value does not need to be provided until the system

enters sleep mode,

quoted

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

? ? 353??? static int

pxa2xx_mfp_suspend(void)

quoted

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

int i;

quoted

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

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

quoted

? ? 358??? ???

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

quoted

? ? 359??? ???

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

quoted

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

? ? 361??? ???

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

quoted

? ? 362??? ???

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

quoted

? ? 363??? ???

??? ??? else

quoted

? ? 364??? ???

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

quoted

? ? 365??? ???

??? }

quoted

? ? 366??? ???

}

quoted

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

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

quoted

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

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

quoted

? ? 376??? ???

}

quoted

? ? 377??? ???

return 0;

quoted

? ? 378??? }

However looking at pxa2xx_mfp_suspend() more closely it

is true that the

quoted

GPDR registers are set before the PGSR values take

effect (when the

quoted

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

So the presumption must be that the GPIOs configured as

DRIVE_HIGH,

quoted

DRIVER_LOW and now KEEP_OUTPUT already have a valid

direction and value

quoted

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

quoted

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

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

??? GPIO96_GPIO |

MFP_LPM_KEEP_OUTPUT,??? /* BQ24022_ISET2 */

quoted

% 

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