Skip to contentSkip to conversation

pxa/hx4700: Fix basic suspend/resume

PATCH New Paul Parsons 5 participants 14 messages May 11, 2011 – Feb 27, 2012

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Basic suspend/resume is fixed by ensuring that the PGSR registers are set correctly before sleep mode is entered. In particular four of the active low resets need to be driven high while in sleep mode, otherwise the unit resets itself instead of suspending. Another problem was that the PCFR_GPROD bit is apparently set by the HTC bootloader; this caused GPIO reset (i.e. the reset button) to fail immediately after returning from sleep mode.

Driver-specific bugs still remain. In particular the mmc driver does not resume properly after returning from sleep mode, thus preventing sleep mode from being entered a second time.

---

arch/arm/mach-pxa/hx4700.c +12 -1
@@ -120,7 +120,11 @@ static unsigned long hx4700_pin_config[]
 	GPIO19_SSP2_SCLK,
 	GPIO86_SSP2_RXD,
 	GPIO87_SSP2_TXD,
-	GPIO88_GPIO,
+	MFP_CFG_OUT(GPIO88, AF0, KEEP_OUTPUT),	/* TSC2046_CS */
+
+	/* BQ24022 Regulator */
+	MFP_CFG_OUT(GPIO72, AF0, KEEP_OUTPUT),	/* BQ24022_nCHARGE_EN */
+	MFP_CFG_OUT(GPIO96, AF0, KEEP_OUTPUT),	/* BQ24022_ISET2 */
 
 	/* HX4700 specific input GPIOs */
 	GPIO12_GPIO,	/* ASIC3_IRQ */
@@ -133,6 +137,11 @@ static unsigned long hx4700_pin_config[]
 	GPIO108_GPIO,	/* GSM_READY */
 	GPIO58_GPIO,	/* TSC2046_nPENIRQ */
 	GPIO66_GPIO,	/* nSDIO_IRQ */
+
+	MFP_CFG_OUT(GPIO61, AF0, DRIVE_HIGH),	/* W3220_nRESET */
+	MFP_CFG_OUT(GPIO71, AF0, DRIVE_HIGH),	/* ASIC3_nRESET */
+	MFP_CFG_OUT(GPIO81, AF0, DRIVE_HIGH),	/* CPU_GP_nRESET */
+	MFP_CFG_OUT(GPIO116, AF0, DRIVE_HIGH),	/* CPU_HW_nRESET */
 };
 
 #define HX4700_GPIO_IN(num, _desc) \
@@ -847,6 +872,8 @@ static struct gpio_ress global_gpios[] =
 
 static void __init hx4700_init(void)
 {
+	PCFR &= ~PCFR_GPROD;	/* Else GPIO reset fails after sleep mode */
+
 	pxa2xx_mfp_config(ARRAY_AND_SIZE(hx4700_pin_config));
 	hx4700_gpio_request(ARRAY_AND_SIZE(global_gpios));
 
#1 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Quoted 2 lines · Paul Parsons

Driver-specific bugs still remain. In particular the mmc driver does not resume properly after returning from sleep mode, thus preventing sleep mode from being entered a second time.

Sorry, no comments about your patch, just wanted to say about mmc driver. Some time ago when I tried to look at suspend on hx4700, this patch helped me to survive suspend-wakeup cycle with root on MMC:

drivers/mfd/asic3.c +2 -0
index 4896a3c..4c686d9 100644
@@ -895,6 +895,8 @@ static struct mfd_cell asic3_cell_mmc = {
 	.name          = "tmio-mmc",
 	.enable        = asic3_mmc_enable,
 	.disable       = asic3_mmc_disable,
+	.suspend	= asic3_mmc_disable,
+	.resume		= asic3_mmc_enable,
 	.driver_data   = &asic3_mmc_data,
 	.num_resources = ARRAY_SIZE(asic3_mmc_resources),
 	.resources     = asic3_mmc_resources,

That was around 2.6.37. Not sure if it still helps and I have no possibility to check - tmio_mmc on ASIC3 seems to be totally borken in 2.6.39-rcX - it doesn't even detect card on my hx4700 :( And that's pity, as I mainly use SD for rootfs.

Signature · 2 lines
Best regards,
Dmitry "MAD" Artamonow
#2 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Hi Dmitry,

That patch found its way into 2.6.39-rc7 and earlier so I'm already using it. Thanks anyway.

As for the tmio_mmc driver, it more or less works for me in 2.6.39-rc7, though it does produce a lot of "tmio_mmc: Spurious irq" messages. My SD card is detected OK and reading / writing it works fine. I'll try to look into both the spurious IRQ problem and the suspend / resume bug(s).

mmc0 at 0xc489c800 irq 130
...
mmc0: new SD card at address 1234
mmcblk0: mmc0:1234 SD01G 972 MiB
tmio_mmc: Spurious irq, disabling! 0x00800780 0x833f0304 0x00000000
mmcblk0: p1
tmio_mmc: Spurious irq, disabling! 0x00800780 0x833f0304 0x00000000
...

Regards,
Paul

Quoted 54 lines · Dmitry Artamonow

From: Dmitry Artamonow <[email protected]>
Subject: Re: [PATCH] pxa/hx4700: Fix basic suspend/resume
To: "Paul Parsons" <[email protected]>
Cc: eric.y.miao at gmail.com, philipp.zabel at gmail.com, linux-arm-kernel at lists.infradead.org
Date: Friday, 13 May, 2011, 11:32

Quoted 2 lines · Paul Parsons

Driver-specific bugs still remain. In particular the

mmc driver does not resume properly after returning from sleep mode, thus preventing sleep mode from being entered a second time.

Sorry, no comments about your patch, just wanted to say about mmc
driver. Some time ago when I tried to look at suspend on hx4700,
this patch helped me to survive suspend-wakeup cycle with root on MMC:

drivers/mfd/asic3.c +0 -0
index 4896a3c..4c686d9 100644
@@ -895,6 +895,8 @@ static struct mfd_cell asic3_cell_mmc =
 {
 ??? .name? ? ? ? ?
 = "tmio-mmc",
 ??? .enable? ? ? ? =
 asic3_mmc_enable,
 ??? .disable? ?

???= asic3_mmc_disable,
+??? .suspend??? =
asic3_mmc_disable,
+??? .resume???
??? = asic3_mmc_enable,
??? .driver_data???=
&asic3_mmc_data,
??? .num_resources =
ARRAY_SIZE(asic3_mmc_resources),
??? .resources? ???=
asic3_mmc_resources,

That was around 2.6.37. Not sure if it still helps and I have no possibility
to check - tmio_mmc on ASIC3 seems to be totally borken in 2.6.39-rcX - it
doesn't even detect card on my hx4700 :( And that's pity, as I mainly use
SD for rootfs.

Signature · 2 lines
Best regards,
Dmitry "MAD" Artamonow
#3 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Quoted 4 lines · Paul Parsons

Hi Dmitry,

That patch found its way into 2.6.39-rc7 and earlier so I'm already using it. Thanks anyway.

Erm, do you have some special 2.6.39-rc7? :) I can't see in mine any .suspend/.resume callbacks defined in asic3.c for tmio_mmc cell. Or have you meant some other changes? BTW, I quickly looked at tmio_mmc.c and it seems very suspicious that it calls cell->disable(dev) on suspend, but cell->resume(dev) on resume. As I said earlier, .resume isn't defined in asic3.c for tmio_mmc cell - so probably that's the culprit.

Quoted 4 lines

As for the tmio_mmc driver, it more or less works for me in 2.6.39-rc7, though it does produce a lot of "tmio_mmc: Spurious irq" messages. My SD card is detected OK and reading / writing it works fine. I'll try to look into both the spurious IRQ problem and the suspend / resume bug(s).

Ah, so mmc works for you in 2.6.39-rc7? Nice to hear! Wonder why it doesn't work for me. I suspect this may be connected to the fact that I boot from WinCE using HaRET - probably Windows leaves controller in some inconsistent state and driver doesn't reset hardware properly.

As for "Spurious irq, disabling!" messages, I'm seeing them for a long time - since pr_debug was turned into pr_warning in commit 311f3ac768 ("mmc: add DMA support to tmio_mmc driver, when used on SuperH") So I suspect this issue (if it's issue at all) has always been there, just unseen before that commit. Not sure what causes those spurious interrupts - maybe it's some specific of TMIO hardware in ASIC3?

Signature · 2 lines
Best regards,
Dmitry "MAD" Artamonow
#4 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Hi Dmitry,

Quoted 4 lines

Erm, do you have some special 2.6.39-rc7? :) I can't see in mine
any .suspend/.resume callbacks defined in asic3.c for tmio_mmc cell.

Now neither can I. Strange. Maybe I noticed that the functions asic3_mmc_enable and asic3_mmc_disable were already in the table and looked no further. Or maybe I applied the patch and forgot, or thought it had failed. Whatever the reason, I just applied it for real and it works; mmc now behaves itself after suspend/resume. Someone needs to submit that patch.

Quoted 8 lines

Ah, so mmc works for you in 2.6.39-rc7?? Nice to hear! Wonder why it
doesn't work for me. I suspect this may be connected to the fact that
I boot from WinCE using HaRET - probably Windows leaves controller in
some inconsistent state and driver doesn't reset hardware properly.

Maybe. I'm using the SDG bootloader v1.2.5 and have long since dumped WinCE. In fact I patched the bootloader to change the flash partitions and free up the 0.5Mb of flash (between 0.5Mb and 1.0Mb) used for the WinCE registry or something. Thus I can no longer use WinCE even if I wanted to.

Quoted 11 lines

As for "Spurious irq, disabling!" messages, I'm seeing them for a long
time - since pr_debug was turned into pr_warning in commit 311f3ac768
("mmc: add DMA support to tmio_mmc driver, when used on SuperH")
So I suspect this issue (if it's issue at all) has always been there, just
unseen before that commit. Not sure what causes those spurious interrupts -
maybe it's some specific of TMIO hardware in ASIC3?

Today I found what causes the spurious interrupts. It's a race condition caused by the interrupt handler looping in a while loop.

At the end of a multiple read operation the handler clears the final RXRDY status bit in one iteration and then clears the DATAEND status bit in the next iteration. However the DATAEND interrupt is still queued in the system somewhere and can't be delivered until the handler has returned. So whether the handler detects spurious interrupts depends on how quickly the DATAEND follows the final RXRDY. Similarly for multiple writes.

Try this quick hack:

drivers/mmc/host/tmio_mmc_pio.c +1 -1
@@ -603,7 +603,7 @@ static irqreturn_t tmio_mmc_irq(int irq,
 		goto out;
 	}
 
-	while (ireg) {
+	if (ireg) {
 		/* Card insert / remove attempts */
 		if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
 			tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |

Regards,
Paul

#5 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Quoted 10 lines · Paul Parsons

Today I found what causes the spurious interrupts. It's a race condition caused by the interrupt handler looping in a while loop.

At the end of a multiple read operation the handler clears the final RXRDY status bit in one iteration and then clears the DATAEND status bit in the next iteration. However the DATAEND interrupt is still queued in the system somewhere and can't be delivered until the handler has returned. So whether the handler detects spurious interrupts depends on how quickly the DATAEND follows the final RXRDY. Similarly for multiple writes.

Try this quick hack:

drivers/mmc/host/tmio_mmc_pio.c static irqreturn_t tmio_mmc_irq(int irq, · line 609

Quoted 8 lines
drivers/mmc/host/tmio_mmc_pio.c +1 -1
@@ -603,7 +603,7 @@ static irqreturn_t tmio_mmc_irq(int irq,
 		goto out;
 	}
 
-	while (ireg) {
+	if (ireg) {
 		/* Card insert / remove attempts */
 		if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
 			tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |

Well, it really seems to make all those "Spurious IRQ" messages disappear. (and btw, MMC magically started to work for me in -rc7. Probably that were some earlier -rc's that didn't work and issue was fixed in -rc7. Or maybe I'm just lucky today)

I think it's worth to submit it to tmio_mmc maintainers along with the description of this situation on ASIC3. Even if it won't be accepted, it'll make them aware of the problem and maybe some clean solution could be worked out then.

Signature · 2 lines
Best regards,
Dmitry "MAD" Artamonow
#6 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Hi Dmitry,

Quoted 7 lines

Well, it really seems to make all those "Spurious IRQ" messages disappear.
(and btw, MMC magically started to work for me in -rc7. Probably that were
some earlier -rc's that didn't work and issue was fixed in -rc7. Or maybe
I'm just lucky today)

That's good news.

Quoted 7 lines

I think it's worth to submit it to tmio_mmc maintainers along with the
description of this situation on ASIC3. Even if it won't be accepted,
it'll make them aware of the problem and maybe some clean solution could
be worked out then.

I have just now submitted a patch to linux-mmc.

Regards,
Paul

#7 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

In reply to Paul Parsons

Am Mittwoch, den 11.05.2011, 19:49 +0000 schrieb Paul Parsons:

Quoted 9 lines

Basic suspend/resume is fixed by ensuring that the PGSR registers are set correctly before sleep mode is entered. In particular four of the active low resets need to be driven high while in sleep mode, otherwise the unit resets itself instead of suspending. Another problem was that the PCFR_GPROD bit is apparently set by the HTC bootloader; this caused GPIO reset (i.e. the reset button) to fail immediately after returning from sleep mode.

Driver-specific bugs still remain. In particular the mmc driver does not resume properly after returning from sleep mode, thus preventing sleep mode from being entered a second time.

---

arch/arm/mach-pxa/hx4700.c static unsigned long hx4700_pin_config[] · line 123

Quoted 5 lines
arch/arm/mach-pxa/hx4700.c +1 -1
@@ -120,7 +120,11 @@ static unsigned long hx4700_pin_config[]
 	GPIO19_SSP2_SCLK,
 	GPIO86_SSP2_RXD,
 	GPIO87_SSP2_TXD,
-	GPIO88_GPIO,
+	MFP_CFG_OUT(GPIO88, AF0, KEEP_OUTPUT),	/* TSC2046_CS */

What you want here is to keep the SPI chip select deasserted (high) during suspend, so I think that should be DRIVE_HIGH instead.

Otherwise,

regards
Philipp

maintainer
#8 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Quoted 19 lines · Philipp Zabel

Am Mittwoch, den 11.05.2011, 19:49 +0000 schrieb Paul Parsons:

Quoted 9 lines

Basic suspend/resume is fixed by ensuring that the PGSR registers are set correctly before sleep mode is entered. In particular four of the active low resets need to be driven high while in sleep mode, otherwise the unit resets itself instead of suspending. Another problem was that the PCFR_GPROD bit is apparently set by the HTC bootloader; this caused GPIO reset (i.e. the reset button) to fail immediately after returning from sleep mode.

Driver-specific bugs still remain. In particular the mmc driver does not resume properly after returning from sleep mode, thus preventing sleep mode from being entered a second time.

---

arch/arm/mach-pxa/hx4700.c ? ? ? 2011-05-11 20:21:54.026109594 +0100 static unsigned long hx4700_pin_config[] · line 123

Quoted 5 lines
arch/arm/mach-pxa/hx4700.c ? ? ? 2011-05-11 20:21:54.026109594 +0100 renamed from arch/arm/mach-pxa/hx4700.c ? ? ? 2011-05-11 00:54:10.787247169 +0100 +1 -1
@@ -120,7 +120,11 @@ static unsigned long hx4700_pin_config[]
 ? ? ? GPIO19_SSP2_SCLK,
 ? ? ? GPIO86_SSP2_RXD,
 ? ? ? GPIO87_SSP2_TXD,
- ? ? GPIO88_GPIO,
+ ? ? MFP_CFG_OUT(GPIO88, AF0, KEEP_OUTPUT), ?/* TSC2046_CS */

What you want here is to keep the SPI chip select deasserted (high) during suspend, so I think that should be DRIVE_HIGH instead.

Quoted 3 lines

Otherwise,

This patch keeps a lot of devices in power-on mode (not reset mode). Is it necessary?

Thanks
Haojian

#9 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Hello Haojian,

Quoted 11 lines
Quoted 1 line

What you want here is to keep the SPI chip select

deasserted (high)

Quoted 1 line

during suspend, so I think that should be DRIVE_HIGH

instead.

Quoted 4 lines

Otherwise,

This patch keeps a lot of devices in power-on mode (not reset mode).
Is it necessary?

No, of course not. This patch is just a first step to get hx4700 suspend mode working; currently it does not work at all.

The suspend mode disposition of devices and GPIOs still needs to be reviewed at some point. For example, someone else discovered that the two Bluetooth outputs (GPIO43, GPIO45) account for approximately one third of the suspend mode power consumption! Correcting that will be my next patch.

Regards,
Paul

maintainer Applied
#10 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Quoted 14 lines · Paul Parsons

Hello Haojian,

Quoted 11 lines
Quoted 1 line

What you want here is to keep the SPI chip select

deasserted (high)

Quoted 1 line

during suspend, so I think that should be DRIVE_HIGH

instead.

Quoted 4 lines

Otherwise,

This patch keeps a lot of devices in power-on mode (not reset mode).
Is it necessary?

Quoted 8 lines

No, of course not. This patch is just a first step to get hx4700 suspend mode working; currently it does not work at all.

The suspend mode disposition of devices and GPIOs still needs to be reviewed at some point. For example, someone else discovered that the two Bluetooth outputs (GPIO43, GPIO45) account for approximately one third of the suspend mode power consumption! Correcting that will be my next patch.

Applied.

#11 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

In reply to Philipp Zabel
Quoted 5 lines · Philipp Zabel

Am Mittwoch, den 11.05.2011, 19:49 +0000 schrieb Paul Parsons:

Quoted 4 lines

Basic suspend/resume is fixed by ensuring that the PGSR registers are set correctly before sleep mode is entered. In particular four of the active low resets need to be driven high while in sleep mode, otherwise the unit resets itself instead of suspending. Another problem was that the PCFR_GPROD bit is apparently set by the HTC bootloader; this caused GPIO reset (i.e. the reset button) to fail immediately after returning from sleep mode.

Driver-specific bugs still remain. In particular the mmc driver does not resume properly after returning from sleep mode, thus preventing sleep mode from being entered a second time.

Quoted 13 lines

arch/arm/mach-pxa/hx4700.c static unsigned long hx4700_pin_config[] · line 123

Quoted 10 lines

---

arch/arm/mach-pxa/hx4700.c +1 -1
@@ -120,7 +120,11 @@ static unsigned long hx4700_pin_config[]
 	GPIO19_SSP2_SCLK,
 	GPIO86_SSP2_RXD,
 	GPIO87_SSP2_TXD,
-	GPIO88_GPIO,
+	MFP_CFG_OUT(GPIO88, AF0, KEEP_OUTPUT),	/* TSC2046_CS */

What you want here is to keep the SPI chip select deasserted (high) during suspend, so I think that should be DRIVE_HIGH instead.

And I don't think we allowed folk to use MFP_CFG_xxx() macros in their board support code. Is there something wrong with:

GPIO88_GPIO | MFP_LPM_KEEP_OUTPUT

?

maintainer
#12 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

Quoted 20 lines · Russell King - ARM Linux
Quoted 5 lines · Philipp Zabel

Am Mittwoch, den 11.05.2011, 19:49 +0000 schrieb Paul Parsons:

Quoted 4 lines

Basic suspend/resume is fixed by ensuring that the PGSR registers are set correctly before sleep mode is entered. In particular four of the active low resets need to be driven high while in sleep mode, otherwise the unit resets itself instead of suspending. Another problem was that the PCFR_GPROD bit is apparently set by the HTC bootloader; this caused GPIO reset (i.e. the reset button) to fail immediately after returning from sleep mode.

Driver-specific bugs still remain. In particular the mmc driver does not resume properly after returning from sleep mode, thus preventing sleep mode from being entered a second time.

Quoted 13 lines

arch/arm/mach-pxa/hx4700.c ? ? 2011-05-11 20:21:54.026109594 +0100 static unsigned long hx4700_pin_config[] · line 123

Quoted 10 lines

---

arch/arm/mach-pxa/hx4700.c ? ? 2011-05-11 20:21:54.026109594 +0100 renamed from arch/arm/mach-pxa/hx4700.c ? ? 2011-05-11 00:54:10.787247169 +0100 +1 -1
@@ -120,7 +120,11 @@ static unsigned long hx4700_pin_config[]
 ? ? GPIO19_SSP2_SCLK,
 ? ? GPIO86_SSP2_RXD,
 ? ? GPIO87_SSP2_TXD,
- ? GPIO88_GPIO,
+ ? MFP_CFG_OUT(GPIO88, AF0, KEEP_OUTPUT), ?/* TSC2046_CS */

What you want here is to keep the SPI chip select deasserted (high) during suspend, so I think that should be DRIVE_HIGH instead.

Quoted 5 lines

And I don't think we allowed folk to use MFP_CFG_xxx() macros in their board support code. ?Is there something wrong with:

? ? ? ?GPIO88_GPIO | MFP_LPM_KEEP_OUTPUT

Yes, it should be MFPR_LPM_DRIVE_HIGH or MFPR_LPM_PULL_HIGH. Thanks for your reminder.

Paul,

Could you help to format the patch again?

Best Regards
Haojian

#13 lore raw

PATCH 1/1 pxa/hx4700: Fix basic suspend/resume

In reply to Russell King - ARM Linux

Hello Russell,

Quoted 19 lines

clean-2.6.39-rc7/arch/arm/mach-pxa/hx4700.c???

2011-05-11 00:54:10.787247169 +0100
Quoted 1 line
Quoted 1 line

+++

linux-2.6.39-rc7/arch/arm/mach-pxa/hx4700.c???

2011-05-11 20:21:54.026109594 +0100
Quoted 1 line

Review static unsigned long

Quoted 1 line
(quoted diff) +0 -0
@@ -120,7 +120,11 @@ static unsigned long

hx4700_pin_config[]

Quoted 5 lines

Review static unsigned long · line 123

Quoted 5 lines
(quoted diff) +1 -1
 ? ??? GPIO19_SSP2_SCLK,
 ? ??? GPIO86_SSP2_RXD,
 ? ??? GPIO87_SSP2_TXD,
-??? GPIO88_GPIO,
+??? MFP_CFG_OUT(GPIO88, AF0,

KEEP_OUTPUT),??? /* TSC2046_CS */

Quoted 2 lines

What you want here is to keep the SPI chip select

deasserted (high)

Quoted 1 line

during suspend, so I think that should be DRIVE_HIGH

instead.

Quoted 7 lines

And I don't think we allowed folk to use MFP_CFG_xxx() macros in their
board support code.? Is there something wrong with:

??? GPIO88_GPIO | MFP_LPM_KEEP_OUTPUT

?

Yes there is; the reason for using MFP_CFG_OUT() was to set the direction of GPIO88 to output.

Currently, all of the GPIO88 definitions in <mach/mfp-pxa27x.h> are inputs, whereas the hx4700 uses it as an output. I could have added a new GPIO88 definition to <mach/mfp-pxa27x.h> to support the hx4700 only, but - based on an earlier experience - I thought that would have been frowned on.

Presumably if MFP_CFG_OUT() is not allowed to be used in board support code then neither is MFP_DIR_OUT, which is defined in the same file.

Is there another compile-time solution? Or should I just add an hx4700-specific definition of GPIO88 to <mach/mfp-pxa27x.h> ?

Regards,
Paul