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

From: Paul Parsons <hidden>
Date: 2012-04-02 11:30:06

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:

quoted

Hello Igor,

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

wrote:

quoted

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

Hi Paul,

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

quoted

Some MFP configurations specify

MFP_LPM_KEEP_OUTPUT to

quoted

preserve the gpio output

quoted

value (HIGH or LOW) during sleep mode.

For

quoted

quoted
example:
quoted
quoted
quoted
? ?

???GPIO72_GPIO |

quoted

MFP_LPM_KEEP_OUTPUT,

quoted

Unfortunately MFP_LPM_KEEP_OUTPUT makes

no

quoted

special

quoted

provision for setting the

quoted

sleep mode gpio direction, unlike

MFP_LPM_DRIVE_HIGH

quoted

and MFP_LPM_DRIVE_LOW.

quoted

Consequently MFP configurations of the

form:

quoted

quoted
quoted
quoted
quoted
? ?

???GPIO<n>_GPIO |

quoted

MFP_LPM_KEEP_OUTPUT,

quoted

will set the sleep mode gpio direction

to

quoted

quoted
INPUT.
quoted
quoted
quoted
This patch forces the sleep mode gpio

direction

quoted

to

quoted

OUTPUT in cases where

quoted

MFP_LPM_KEEP_OUTPUT was specified. This

brings

quoted

MFP_LPM_KEEP_OUTPUT into line

quoted

with MFP_LPM_DRIVE_HIGH and

MFP_LPM_DRIVE_LOW.

quoted

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

diff --git

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

quoted

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

gpio, unsigned long c)

quoted

? ? ? ?

? ? break;

quoted

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

MFP_LPM_KEEP_OUTPUT)

quoted

quoted
quoted
+? ? ? ? is_out =

1;

quoted

quoted
quoted
quoted
quoted
+
MFP_LPM_KEEP_OUTPUT does not meant to be

used to

quoted

setup the

quoted

mfp config,

As far as I can see, MFP_LPM_KEEP_OUTPUT is

*only* used

quoted

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

*/

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

quoted

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

quoted

proposed patch?

On corgi, the three GPIOs configured as

MFP_LPM_KEEP_OUTPUT:

quoted

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

quoted

but for pins that have been configured for

output

quoted

by

quoted

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

MFP_LPM_KEEP_OUTPUT only takes effect when the

system

quoted

enters sleep mode,

quoted

at which point the GPIO directions (GPDR

registers) are

quoted

forced from the

quoted

values privately stored in gpdr_lpm[].

Correct, but with your patch, the direction will be

forced

quoted

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

The values will be defined in pxa2xx_mfp_suspend() when

it sets the PGSR

quoted

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

As far as I can tell MFP_LPM_KEEP_OUTPUT has no

direct

quoted

connection to

quoted

gpio_direction_output().

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

function you've

quoted

pasted below.

quoted

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

because

quoted

the patch

quoted

allows a pin
be configured as output, but does not

forces a

quoted

value

quoted

(high/low)
and this is not safe.

A value does not need to be provided until the

system

quoted

enters sleep mode,

quoted

at which point the value is provided via the

PGSR

quoted

registers.

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

OK, see below...

quoted

? ???353? ? static

int

quoted

pxa2xx_mfp_suspend(void)

quoted

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

???

quoted

int i;

quoted

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

???

quoted

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

quoted

? ???358? ?

???

quoted

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

quoted

? ???359? ?

???

quoted

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

quoted

MFP_LPM_KEEP_OUTPUT) &&

quoted

? ???360? ?

???

quoted

? ? ? ???(GPDR(i)

quoted

GPIO_bit(i))) {

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

output

quoted

(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

the current state of GPDR (via gpio_direction_output())

does not apply in

quoted

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

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

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.

quoted

quoted
quoted
? ???361? ?

???

quoted

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

quoted

quoted
GPIO_bit(i))
quoted
? ???362? ?

???

quoted

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

quoted

? ???363? ?

???

quoted

quoted
? ? ? ???else
quoted
? ???364? ?

???

quoted

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

quoted

? ???365? ?

???

quoted

quoted
? ???}
quoted
? ???366? ?

???

quoted

quoted
}
quoted
? ???367? ? 
? ???368? ?

???

quoted

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

i++) {

quoted

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

???

quoted

? ???GPDR(i * 32) =

gpdr_lpm[i];

quoted

quoted
quoted
? ???376? ?

???

quoted

quoted
}
quoted
? ???377? ?

???

quoted

return 0;

quoted

? ???378? ? }

However looking at pxa2xx_mfp_suspend() more

closely it

quoted

is true that the

quoted

GPDR registers are set before the PGSR values

take

quoted

effect (when the

quoted

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

or

quoted

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

a _known value_.

KEEP_OUTPUT does the same where GPIOs are configured as

outputs, as is

quoted

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

configured as inputs.

quoted

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

GPIO_bit(i)) test will satisfy

quoted

your objection.

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

quoted

So the presumption must be that the GPIOs

configured as

quoted

DRIVE_HIGH,

quoted

DRIVER_LOW and now KEEP_OUTPUT already have a

valid

quoted

direction and value

quoted

set at the point the GPDR registers are

forced.

quoted

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

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

no value

quoted

set!

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

normal mode but

quoted

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

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

MFP_LPM_DRIVE_<level>

quoted

The hx4700 has a couple of charging GPIOs which

we want

quoted

to keep HIGH or

quoted

LOW during sleep mode:

% fgrep MFP_LPM_KEEP_OUTPUT

arch/arm/mach-pxa/hx4700.c

quoted

? ???GPIO72_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /*

BQ24022_nCHARGE_EN

quoted

*/

quoted

? ???GPIO96_GPIO |

MFP_LPM_KEEP_OUTPUT,? ? /* BQ24022_ISET2

*/

quoted

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

not do

quoted

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

MFP_LPM_DRIVE_*?

quoted

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

The feeling was that if the unit was charging in normal

mode then it

quoted

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

then it should

quoted

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.

quoted

Actually that's an interim solution until the

bootloader can take control

quoted

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help