[PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT
Paul Parsons
lost.distance at yahoo.com
Mon Apr 2 07:30:06 EDT 2012
Hello Igor,
--- On Mon, 2/4/12, Igor Grinberg <grinberg at compulab.co.il> wrote:
> Hi Paul,
>
> Can you, please, configure your mailer to _not_ wrap the
> text?
> It is rally hard to read it after a few replies...
>
> On 04/01/12 20:56, Paul Parsons wrote:
> > Hello Igor,
> >
> > --- On Sun, 1/4/12, Igor Grinberg <grinberg at compulab.co.il>
> wrote:
> >> On 04/01/12 13:56, Paul Parsons
> >> wrote:
> >>> Hello Igor,
> >>>
> >>> --- On Sun, 1/4/12, Igor Grinberg <grinberg at compulab.co.il>
> >> wrote:
> >>>> Hi Paul,
> >>>>
> >>>> On 03/31/12 15:20, Paul Parsons wrote:
> >>>>> Some MFP configurations specify
> >> MFP_LPM_KEEP_OUTPUT to
> >>>> preserve the gpio output
> >>>>> value (HIGH or LOW) during sleep mode.
> For
> >> example:
> >>>>>
> >>>>>
> GPIO72_GPIO |
> >>>> MFP_LPM_KEEP_OUTPUT,
> >>>>>
> >>>>> Unfortunately MFP_LPM_KEEP_OUTPUT makes
> no
> >> special
> >>>> provision for setting the
> >>>>> sleep mode gpio direction, unlike
> >> MFP_LPM_DRIVE_HIGH
> >>>> and MFP_LPM_DRIVE_LOW.
> >>>>> Consequently MFP configurations of the
> form:
> >>>>>
> >>>>>
> GPIO<n>_GPIO |
> >>>> MFP_LPM_KEEP_OUTPUT,
> >>>>>
> >>>>> will set the sleep mode gpio direction
> to
> >> INPUT.
> >>>>>
> >>>>> This patch forces the sleep mode gpio
> direction
> >> to
> >>>> OUTPUT in cases where
> >>>>> MFP_LPM_KEEP_OUTPUT was specified. This
> brings
> >>>> MFP_LPM_KEEP_OUTPUT into line
> >>>>> with MFP_LPM_DRIVE_HIGH and
> MFP_LPM_DRIVE_LOW.
> >>>>>
> >>>>> Signed-off-by: Paul Parsons <lost.distance at yahoo.com>
> >>>>> ---
> >>>>>
> >>>>> diff --git
> a/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>> b/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>>> index b0a8428..3b443199 100644
> >>>>> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>>> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
> >>>>> @@ -96,6 +96,9 @@ static int
> >> __mfp_config_gpio(unsigned
> >>>> gpio, unsigned long c)
> >>>>>
> >> break;
> >>>>> }
> >>>>>
> >>>>> + if (c &
> >> MFP_LPM_KEEP_OUTPUT)
> >>>>> + is_out =
> 1;
> >>>>> +
> >>>>
> >>>> MFP_LPM_KEEP_OUTPUT does not meant to be
> used to
> >> setup the
> >>>> mfp config,
> >>>
> >>> As far as I can see, MFP_LPM_KEEP_OUTPUT is
> *only* used
> >> to setup the
> >>> mfp config:
> >>>
> >>> % xzcat linux-3.4-rc1.tar.xz | fgrep -a
> >> MFP_LPM_KEEP_OUTPUT
> >>> GPIO13_GPIO |
> >> MFP_LPM_KEEP_OUTPUT, /*
> >> CORGI_GPIO_LED_ORANGE */
> >>> GPIO38_GPIO |
> >> MFP_LPM_KEEP_OUTPUT, /*
> CORGI_GPIO_CHRG_ON
> >> */
> >>> GPIO43_GPIO |
> >> MFP_LPM_KEEP_OUTPUT, /*
> >> CORGI_GPIO_CHRG_UKN */
> >>> #define MFP_LPM_KEEP_OUTPUT (0x1
> >> << 25)
> >>> /* set corresponding
> PGSR bit of
> >> those marked MFP_LPM_KEEP_OUTPUT */
> >>> if
> >> ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT)
> &&
> >>> %
> >>
> >> How can you be certain that you don't break corgi,
> with the
> >> proposed patch?
> >
> > On corgi, the three GPIOs configured as
> MFP_LPM_KEEP_OUTPUT:
> > CORGI_GPIO_LED_ORANGE
> > CORGI_GPIO_CHRG_ON
> > CORGI_GPIO_CHRG_UKN
> > are all used as outputs.
>
> Have you verified it, because currently MFP_LPM_KEEP_OUTPUT
> does not do a direction configuration... and IMO, it should
> not!
Yes, all 3 GPIOs are configured as outputs:
CORGI_GPIO_LED_ORANGE in drivers/leds/leds-gpio.c
CORGI_GPIO_CHRG_ON in arch/arm/mach-pxa/corgi_pm.c
CORGI_GPIO_CHRG_UKN in arch/arm/mach-pxa/corgi_pm.c
>
> >
> >>>> but for pins that have been configured for
> output
> >> by
> >>>> gpio_direction_out().
> >>>> (Hint: *_KEEP_*).
> >>>
> >>> MFP_LPM_KEEP_OUTPUT only takes effect when the
> system
> >> enters sleep mode,
> >>> at which point the GPIO directions (GPDR
> registers) are
> >> forced from the
> >>> values privately stored in gpdr_lpm[].
> >>
> >> Correct, but with your patch, the direction will be
> forced
> >> to output,
> >> but the value will be undefined...
> >
> > The values will be defined in pxa2xx_mfp_suspend() when
> it sets the PGSR
> > registers. Or at least they will be on corgi and
> hx4700, but see below...
>
> No, they will not, because the direction can be input...
>
> >
> >>> As far as I can tell MFP_LPM_KEEP_OUTPUT has no
> direct
> >> connection to
> >>> gpio_direction_output().
> >>
> >> Well, it has... in the pxa2xx_mfp_suspend()
> function you've
> >> pasted below.
> >>
> >>>
> >>>> Also, I don't think this is a good idea,
> because
> >> the patch
> >>>> allows a pin
> >>>> be configured as output, but does not
> forces a
> >> value
> >>>> (high/low)
> >>>> and this is not safe.
> >>>
> >>> A value does not need to be provided until the
> system
> >> enters sleep mode,
> >>> at which point the value is provided via the
> PGSR
> >> registers.
> >>
> >> Yes, but you don't force the PGSR to be set...
> >> and that is the problem.
> >
> > OK, see below...
> >
> >>> 353 static
> int
> >> pxa2xx_mfp_suspend(void)
> >>> 354 {
> >>> 355
>
> >> int i;
> >>> 356
> >>> 357
>
> >> /* set corresponding PGSR bit of those marked
> >> MFP_LPM_KEEP_OUTPUT */
> >>> 358
>
> >> for (i = 0; i < pxa_last_gpio; i++) {
> >>> 359
>
> >> if ((gpio_desc[i].config
> &
> >> MFP_LPM_KEEP_OUTPUT) &&
> >>> 360
>
> >> (GPDR(i)
> &
> >> GPIO_bit(i))) {
> >>
> >> Look at the check above...
> >> the PGSR only gets set for the GPIOs configured to
> output
> >> (presumably by gpio_direction_output() function).
> >
> > OK, I understand your objection here.
> >
> > My feeling is that the (GPDR(i) & GPIO_bit(i)) test
> is unwanted because
> > the current state of GPDR (via gpio_direction_output())
> does not apply in
> > sleep mode,
>
> It gets copied to PGSR in the pxa2xx_mfp_suspend() and
> therefore it does apply!
> That is exactly the meaning of MFP_LPM_KEEP_OUTPUT:
> _If_ the pin is output, then _keep_ it output with the
> _runtime_ value.
OK, I see, your interpretation is that MFP_LPM_KEEP_OUTPUT is optional:
it should not apply if the pin is an input at the time
pxa2xx_mfp_suspend() is called.
My interpretation was that MFP_LPM_KEEP_OUTPUT is mandatory: it always
applies regardless of the current pin direction, exactly like
MFP_LPM_DRIVE_HIGH and MFP_LPM_DRIVE_LOW.
If your interpretation is correct then DRIVE_HIGH and DRIVE_LOW will
likewise need to check the state of GPDR at the time
pxa2xx_mfp_suspend() is called.
If my interpretation is correct then having KEEP_OUTPUT ignore the state
of GPDR in pxa2xx_mfp_suspend() is the correct thing to do.
Do you agree that KEEP_OUTPUT, DRIVE_HIGH and DRIVE_LOW should all be
consistent in being either optional or mandatory sleep mode
configurations?
>
> > yet for some reason it is being allowed to prevent the
> > setting of PGSR in the case of GPIO inputs.
>
> Because, the PGSR is valid only for GPIO outputs!
> and has nothing to do with inputs.
I know, it makes no sense to specify KEEP_OUTPUT, DRIVE_HIGH or DRIVE_LOW
for GPIO inputs.
But since DRIVE_HIGH or DRIVE_LOW currently don't prevent that, the
presumption must be that GPIOs configured as DRIVE_HIGH or DRIVE_LOW are
being used as outputs. Likewise for KEEP_OUTPUT?
>
> >
> > Consequently the (GPDR(i) & GPIO_bit(i)) test
> should be removed.
>
> No, absolutely not!
> This will change the meaning of the MFP_LPM_KEEP_OUTPUT
> symbol!
Only if you interpret KEEP_OUTPUT as an optional sleep mode configuration.
> I have a feeling that you are trying to solve a problem that
> does not exist!
The problem is that MFP configurations of the form:
GPIO<n>_GPIO | MFP_LPM_KEEP_OUTPUT,
will always set the sleep mode gpio direction to INPUT.
That is the case with corgi and would also be the case with hx4700.
That is the problem which needs to be solved.
>
> >
> >>> 361
>
> >> if (GPLR(i)
> &
> >> GPIO_bit(i))
> >>> 362
>
> >>
> >> PGSR(gpio_to_bank(i)) |= GPIO_bit(i);
> >>> 363
>
> >> else
> >>> 364
>
> >>
> >> PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i);
> >>> 365
>
> >> }
> >>> 366
>
> >> }
> >>> 367
> >>> 368
>
> >> for (i = 0; i <= gpio_to_bank(pxa_last_gpio);
> i++) {
> >>> 369
> >>> ...
> >>> 375
>
> >> GPDR(i * 32) =
> gpdr_lpm[i];
> >>> 376
>
> >> }
> >>> 377
>
> >> return 0;
> >>> 378 }
> >>>
> >>> However looking at pxa2xx_mfp_suspend() more
> closely it
> >> is true that the
> >>> GPDR registers are set before the PGSR values
> take
> >> effect (when the
> >>> processor enters sleep mode).
> >>> That is no different from MFP_LPM_DRIVE_HIGH
> or
> >> MFP_LPM_DRIVE_LOW though.
> >>
> >> Well, it is different, because the in case of
> >> MFP_LPM_DRIVE_*,
> >> the PGSR is set in the __mfp_config_gpio() function
> and has
> >> a _known value_.
> >
> > KEEP_OUTPUT does the same where GPIOs are configured as
> outputs, as is
> > the case with corgi and hx4700.
> > Your valid objection is for cases where GPIOs are
> configured as inputs.
> > So I presume that removing the (GPDR(i) &
> GPIO_bit(i)) test will satisfy
> > your objection.
>
> again, no, because it will change the meaning of the
> MFP_LPM_KEEP_OUTPUT symbol!
>
> >
> >>> So the presumption must be that the GPIOs
> configured as
> >> DRIVE_HIGH,
> >>> DRIVER_LOW and now KEEP_OUTPUT already have a
> valid
> >> direction and value
> >>> set at the point the GPDR registers are
> forced.
> >>
> >> Yes. That is exactly my point here.
> >> MFP_LPM_DRIVE_* has the value determined and set.
> >> MFP_LPM_KEEP_OUTPUT does not, but with your patch
> it will
> >> effectively
> >> force the GPIO to be output in sleep mode, but with
> no value
> >> set!
> >
> > Only in cases where the GPIO is used as an input in
> normal mode but
> > configured to be an output in sleep mode.
> > Removing the (GPDR(i) & GPIO_bit(i)) test addresses
> that case.
>
> No, because, if the GPIO is input, you will configure it for
> output
> and set some arbitrary value that is seen on the input!
> This is bad!
>
> >
> >>>> Why can't you use:
> >>>> GPIO<n>_GPIO |
> MFP_LPM_DRIVE_<level>
> >>>> ?
> >>>
> >>> The hx4700 has a couple of charging GPIOs which
> we want
> >> to keep HIGH or
> >>> LOW during sleep mode:
> >>>
> >>> % fgrep MFP_LPM_KEEP_OUTPUT
> arch/arm/mach-pxa/hx4700.c
> >>> GPIO72_GPIO |
> >> MFP_LPM_KEEP_OUTPUT, /*
> BQ24022_nCHARGE_EN
> >> */
> >>> GPIO96_GPIO |
> >> MFP_LPM_KEEP_OUTPUT, /* BQ24022_ISET2
> */
> >>> %
> >>
> >> I still, can't understand, why MFP_LPM_DRIVE_* does
> not do
> >> the job for you...
> >> What's the real problem with using
> MFP_LPM_DRIVE_*?
> >> Can't you specify it for those GPIO72/96?
> >
> > The feeling was that if the unit was charging in normal
> mode then it
> > should remain charging in sleep mode, and if it wasn't
> then it should
> > remain not charging.
>
> Ok, finally we are talking about a real problem...
> I see...
> IMO, the best solution will be to add a generic
> GPIOx_GPIO_OUT
> (as already proposed by Haojian) definition (where x is the
> required GPIO number)for your used GPIOs and use it in MFP
> config
> structure for hx4700 along with MFP_LPM_KEEP_OUTPUT.
>
>
> Another solution would be (though a bit hackish):
> GPIOx_GPIO | MFP_LPM_DRIVE_LOW | MFP_LPM_KEEP_OUTPUT
>
> The above should work for you as you would expect - will set
> the GPIO
> to be output low by default, then you will reconfigure it to
> the
> appropriate level from your charger driver and the new level
> will be used
> by the existing logic in pxa2xx_mfp_suspend().
Those solutions are fair enough.
Can we first answer the question of whether KEEP_OUTPUT, DRIVE_HIGH and
DRIVE_LOW are optional or mandatory sleep mode configurations. That may
determine the way forward.
>
> > Actually that's an interim solution until the
> bootloader can take control
> > of sleep mode charging, but we're not there yet.
>
> still you don't want the turn on/off of the charger in the
> suspend sequence,
> so IMO, it must also be fixed in Linux.
> But, please, don't change the meaning of
> MFP_LPM_KEEP_OUTPUT!
> Because, currently it is safe to use it also with inputs
> and we don't want to break this assumption.
Regards,
Paul
More information about the linux-arm-kernel
mailing list