From mboxrd@z Thu Jan 1 00:00:00 1970 From: grinberg@compulab.co.il (Igor Grinberg) Date: Sun, 01 Apr 2012 15:23:58 +0300 Subject: [PATCH] ARM: pxa: mfp: Force gpio direction for MFP_LPM_KEEP_OUTPUT In-Reply-To: <1333277787.12023.YahooMailClassic@web29010.mail.ird.yahoo.com> References: <1333277787.12023.YahooMailClassic@web29010.mail.ird.yahoo.com> Message-ID: <4F7848DE.2080605@compulab.co.il> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 04/01/12 13:56, Paul Parsons wrote: > Hello Igor, > > --- On Sun, 1/4/12, Igor Grinberg 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_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 >>> --- >>> >>> diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c >> b/arch/arm/mach-pxa/mfp-pxa2xx.c >>> index b0a8428..3b443199 100644 >>> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c >>> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c >>> @@ -96,6 +96,9 @@ static int __mfp_config_gpio(unsigned >> gpio, unsigned long c) >>> break; >>> } >>> >>> + if (c & MFP_LPM_KEEP_OUTPUT) >>> + is_out = 1; >>> + >> >> MFP_LPM_KEEP_OUTPUT does not meant to be used to setup the >> mfp config, > > As far as I can see, MFP_LPM_KEEP_OUTPUT is *only* used to setup the > mfp config: > > % xzcat linux-3.4-rc1.tar.xz | fgrep -a MFP_LPM_KEEP_OUTPUT > GPIO13_GPIO | MFP_LPM_KEEP_OUTPUT, /* CORGI_GPIO_LED_ORANGE */ > GPIO38_GPIO | MFP_LPM_KEEP_OUTPUT, /* CORGI_GPIO_CHRG_ON */ > GPIO43_GPIO | MFP_LPM_KEEP_OUTPUT, /* CORGI_GPIO_CHRG_UKN */ > #define MFP_LPM_KEEP_OUTPUT (0x1 << 25) > /* set corresponding PGSR bit of those marked MFP_LPM_KEEP_OUTPUT */ > if ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT) && > % How can you be certain that you don't break corgi, with the proposed patch? > >> but for pins that have been configured for output by >> gpio_direction_out(). >> (Hint: *_KEEP_*). > > MFP_LPM_KEEP_OUTPUT only takes effect when the system enters sleep mode, > at which point the GPIO directions (GPDR registers) are forced from the > values privately stored in gpdr_lpm[]. Correct, but with your patch, the direction will be forced to output, but the value will be undefined... > As far as I can tell MFP_LPM_KEEP_OUTPUT has no direct connection to > gpio_direction_output(). Well, it has... in the pxa2xx_mfp_suspend() function you've pasted below. > >> Also, I don't think this is a good idea, because the patch >> allows a pin >> be configured as output, but does not forces a value >> (high/low) >> and this is not safe. > > A value does not need to be provided until the system enters sleep mode, > at which point the value is provided via the PGSR registers. Yes, but you don't force the PGSR to be set... and that is the problem. > > 353 static int pxa2xx_mfp_suspend(void) > 354 { > 355 int i; > 356 > 357 /* set corresponding PGSR bit of those marked MFP_LPM_KEEP_OUTPUT */ > 358 for (i = 0; i < pxa_last_gpio; i++) { > 359 if ((gpio_desc[i].config & MFP_LPM_KEEP_OUTPUT) && > 360 (GPDR(i) & GPIO_bit(i))) { Look at the check above... the PGSR only gets set for the GPIOs configured to output (presumably by gpio_direction_output() function). > 361 if (GPLR(i) & GPIO_bit(i)) > 362 PGSR(gpio_to_bank(i)) |= GPIO_bit(i); > 363 else > 364 PGSR(gpio_to_bank(i)) &= ~GPIO_bit(i); > 365 } > 366 } > 367 > 368 for (i = 0; i <= gpio_to_bank(pxa_last_gpio); i++) { > 369 > ... > 375 GPDR(i * 32) = gpdr_lpm[i]; > 376 } > 377 return 0; > 378 } > > However looking at pxa2xx_mfp_suspend() more closely it is true that the > GPDR registers are set before the PGSR values take effect (when the > processor enters sleep mode). > That is no different from MFP_LPM_DRIVE_HIGH or MFP_LPM_DRIVE_LOW though. Well, it is different, because the in case of MFP_LPM_DRIVE_*, the PGSR is set in the __mfp_config_gpio() function and has a _known value_. > So the presumption must be that the GPIOs configured as DRIVE_HIGH, > DRIVER_LOW and now KEEP_OUTPUT already have a valid direction and value > set at the point the GPDR registers are forced. Yes. That is exactly my point here. MFP_LPM_DRIVE_* has the value determined and set. MFP_LPM_KEEP_OUTPUT does not, but with your patch it will effectively force the GPIO to be output in sleep mode, but with no value set! > >> Why can't you use: >> GPIO_GPIO | MFP_LPM_DRIVE_ >> ? > > The hx4700 has a couple of charging GPIOs which we want to keep HIGH or > LOW during sleep mode: > > % fgrep MFP_LPM_KEEP_OUTPUT arch/arm/mach-pxa/hx4700.c > GPIO72_GPIO | MFP_LPM_KEEP_OUTPUT, /* BQ24022_nCHARGE_EN */ > GPIO96_GPIO | MFP_LPM_KEEP_OUTPUT, /* BQ24022_ISET2 */ > % I still, can't understand, why MFP_LPM_DRIVE_* does not do the job for you... What's the real problem with using MFP_LPM_DRIVE_*? Can't you specify it for those GPIO72/96? -- Regards, Igor.