[PATCH] amd74xx: implement suspend-to-ram

[PATCH] amd74xx: implement suspend-to-ram

[Rafael J. Wysocki]

From: Jason Lunz <lunz@falooley.org>

The amd74xx driver needs to reprogram each drive's PIO timings as well
as the DMA timings on resume from s2ram.  Otherwise, my
nforce3-150-based laptop hangs hard when ide_start_power_step() calls
drive->hwif->ide_dma_check(drive).

Suspend/resume from ram now works with the disk and the cdrom under
load, both with and without DMA enabled.

I'm hardcoding a maximum of 2 ide channels, but other aspects of this
driver (like the amd_80w global) are already doing that.

DMA is re-enabled on resume even if it wasn't on at suspend, but that
doesn't look unusual in drivers/ide/pci.

Signed-off-by: Jason Lunz <lunz@falooley.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
Cc: Vojtech Pavlik <vojtech@suse.cz>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/ide/pci/amd74xx.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

Index: linux-2.6.18-rc2-git1/drivers/ide/pci/amd74xx.c
--- linux-2.6.18-rc2-git1.orig/drivers/ide/pci/amd74xx.c
+++ linux-2.6.18-rc2-git1/drivers/ide/pci/amd74xx.c
@@ -83,6 +83,8 @@
 static ide_pci_device_t *amd_chipset;
 static unsigned int amd_80w;
 static unsigned int amd_clock;
+#define AMD_MAX_CHANNELS	(2)
+static ide_hwif_t *amd_hwifs[AMD_MAX_CHANNELS];
 
 static char *amd_dma[] = { "MWDMA16", "UDMA33", "UDMA66", "UDMA100", "UDMA133" };
 static unsigned char amd_cyc2udma[] = { 6, 6, 5, 4, 0, 1, 1, 2, 2, 3, 3, 3, 3, 3, 3, 7 };
@@ -416,6 +418,8 @@
 {
 	int i;
 
+	amd_hwifs[hwif->channel] = hwif;
+
 	if (hwif->irq == 0) /* 0 is bogus but will do for now */
 		hwif->irq = pci_get_legacy_ide_irq(hwif->pci_dev, hwif->channel);
 
@@ -494,6 +498,55 @@
 	/* 19 */ DECLARE_AMD_DEV("AMD5536"),
 };
 
+#ifdef	CONFIG_PM
+
+static int amd74xx_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	pci_save_state(dev);
+
+	if (state.event == PM_EVENT_SUSPEND) {
+		pci_disable_device(dev);
+		pci_set_power_state(dev, PCI_D3hot);
+	}
+	return 0;
+}
+
+static int amd74xx_resume(struct pci_dev *dev)
+{
+	int retval = 0;
+	int i;
+
+	pci_set_power_state(dev, PCI_D0);
+	retval = pci_enable_device(dev);
+	pci_restore_state(dev);
+
+	for (i = 0; i < AMD_MAX_CHANNELS; i++) {
+		int d;
+
+		if (!amd_hwifs[i])
+			continue;
+
+		for (d = 0; d < MAX_DRIVES; ++d) {
+			ide_drive_t *drive = &amd_hwifs[i]->drives[d];
+			if (drive->present && !__ide_dma_bad_drive(drive)) {
+				/* this is the primary reason this driver needs
+				 * a suspend()/resume() implementation at all.
+				 * Calling amd74xx_ide_dma_check() without also
+				 * calling amd74xx_tune_drive() hangs my
+				 * nforce3-150 system.  ide-io.c will do just
+				 * that later if we're resuming from s2ram.
+				 */
+				amd_hwifs[i]->tuneproc(drive, 255);
+				amd_hwifs[i]->ide_dma_check(drive);
+			}
+		}
+	}
+
+	return retval;
+}
+
+#endif	/* !CONFIG_PM */
+
 static int __devinit amd74xx_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	amd_chipset = amd74xx_chipsets + id->driver_data;
@@ -539,6 +592,10 @@
 	.name		= "AMD_IDE",
 	.id_table	= amd74xx_pci_tbl,
 	.probe		= amd74xx_probe,
+#ifdef CONFIG_PM
+	.suspend	= amd74xx_suspend,
+	.resume		= amd74xx_resume,
+#endif
 };
 
 static int amd74xx_ide_init(void)

Re: [PATCH] amd74xx: implement suspend-to-ram

[Alan Cox]

Ar Gwe, 2006-07-28 am 16:46 +0200, ysgrifennodd Rafael J. Wysocki:
> From: Jason Lunz <lunz@falooley.org>
> 
> The amd74xx driver needs to reprogram each drive's PIO timings as well
> as the DMA timings on resume from s2ram.  Otherwise, my
> nforce3-150-based laptop hangs hard when ide_start_power_step() calls
> drive->hwif->ide_dma_check(drive).

NAK

This beings in the IDE power step code. You should do that as a step
before the win_idleimmediate I suspect. Theory is right, diagnosis is
right, implementation is in the wrong place.

You'll make a lot more people happy by fixing it in ide-io

Alan

Re: [PATCH] amd74xx: implement suspend-to-ram

[Jason Lunz]

On Fri, Jul 28, 2006 at 05:51:55PM +0100, Alan Cox wrote:
> This beings in the IDE power step code. You should do that as a step
> before the win_idleimmediate I suspect. Theory is right, diagnosis is
> right, implementation is in the wrong place.
> 
> You'll make a lot more people happy by fixing it in ide-io

OK, I'll see about moving it there. Will this still be
controller-specific, or are you suggesting this is something ide ought
to do globally?

I poked around in ide-io.c a little while writing the patch, but my
assumption so far has been that the core ide suspend is OK wrt s2ram,
since I never hear IDE cited as the reason for s2ram failure. Usually
it's ACPI or video problems.

I'm beginning to suspect that the real problem is that everyone who
experiences ide-related hangs when attempting s2ram switches to s2disk,
which sidesteps ide problems since the ide controller stays alive
throughout the process and gets reinitialized on resume. So I guess
ide/s2ram problems could be more common than is widely known.

Jason

Re: [PATCH] amd74xx: implement suspend-to-ram

[Brad Campbell]

Jason Lunz wrote:

> 
> I poked around in ide-io.c a little while writing the patch, but my
> assumption so far has been that the core ide suspend is OK wrt s2ram,
> since I never hear IDE cited as the reason for s2ram failure. Usually
> it's ACPI or video problems.

Actually I had exactly your issue on an ICH6 and ended up working around it by moving to Alan's 
latest libata code.

0000:00:1f.1 IDE interface: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) IDE Controller 
(rev 03)

If you were to patch up IDE I'd be happy to run up a couple of test kernels and test it out on an 
Intel chipset also.

Brad
-- 
"Human beings, who are almost unique in having the ability
to learn from the experience of others, are also remarkable
for their apparent disinclination to do so." -- Douglas Adams

Re: [PATCH] amd74xx: implement suspend-to-ram

[Alan Cox]

Ar Gwe, 2006-07-28 am 13:13 -0400, ysgrifennodd Jason Lunz:
> OK, I'll see about moving it there. Will this still be
> controller-specific, or are you suggesting this is something ide ought
> to do globally?

It should be done globally. In many cases the chips start up from power
on configured for PIO 0 so that side happens to work, but not all chips
do this as you've found out. Setting the PIO side correctly is a fix
even if its not a bug people hit a lot.

Alan

Re: [PATCH] amd74xx: implement suspend-to-ram

[Benjamin Herrenschmidt]

On Fri, 2006-07-28 at 21:00 +0100, Alan Cox wrote:
> Ar Gwe, 2006-07-28 am 13:13 -0400, ysgrifennodd Jason Lunz:
> > OK, I'll see about moving it there. Will this still be
> > controller-specific, or are you suggesting this is something ide ought
> > to do globally?
> 
> It should be done globally. In many cases the chips start up from power
> on configured for PIO 0 so that side happens to work, but not all chips
> do this as you've found out. Setting the PIO side correctly is a fix
> even if its not a bug people hit a lot.

It's actually incorrect to just "restore" the previous timings. In many
case, the disk will have been reset too. The safe thing is to force the
controller in PIO0 mode at resume, and the best place to do that is from
the controller's own resume(), as I do on pmac. It's the parent of the
hwif and thus will be resumed before the later.

Later on, the state machine will redo the tuning to get to a better
speed, including sending the right commands to also reconfigure the
drive.

Ben.

[patch, rft] ide: reprogram disk pio timings on resume

[Jason Lunz]

Add a step to the IDE PM state machine that reprograms disk PIO timings
as the first step on resume. This prevents ide deadlock on
resume-from-ram on my nforce3-based laptop.

An earlier implementation was written entirely within the amd74xx ide
driver, but Alan helpfully pointed out that this is the correct thing to
do globally. Still, I'm only calling hwif->tuneproc() for disks, based
on two things:

 - The existing state machine is already passed over for non-disk drives
 - Previous testing on my laptop shows that the hangs are related only
   to the disk - suspend/resume from a livecd showed that there's no
   need for this on the cdrom.

Signed-off-by: Jason Lunz <lunz@falooley.org>

---

Alan: I'm doing tuneproc() before WIN_IDLEIMMEDIATE as you said. It
works. I'd otherwise have done it after, based solely on the name -
thanks for the suggestion.

David, Rafael: It would be helpful if you could verify that this patch
cures your ide-relate hangs on s2ram as well (with my previous patch to
amd74xx unapplied). 

Brad: thanks for volunteering to test! You'll be the lucky first person
to try this on a non-nforce ide chipset. good luck.

Please be careful - I know next to nothing about IDE; I just know that
this make s2ram work on my laptop. This patch means altering ide resume
for all systems where it currently works already. I suppose there could
be regressions.

 drivers/ide/ide-io.c |   25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Index: linux-2.6.18-rc2-git6/drivers/ide/ide-io.c
===================================================================
--- linux-2.6.18-rc2-git6.orig/drivers/ide/ide-io.c
+++ linux-2.6.18-rc2-git6/drivers/ide/ide-io.c
@@ -135,7 +135,8 @@
 	ide_pm_flush_cache	= ide_pm_state_start_suspend,
 	idedisk_pm_standby,
 
-	idedisk_pm_idle		= ide_pm_state_start_resume,
+	idedisk_pm_restore_pio	= ide_pm_state_start_resume,
+	idedisk_pm_idle,
 	ide_pm_restore_dma,
 };
 
@@ -156,7 +157,10 @@
 	case idedisk_pm_standby:	/* Suspend step 2 (standby) complete */
 		pm->pm_step = ide_pm_state_completed;
 		break;
-	case idedisk_pm_idle:		/* Resume step 1 (idle) complete */
+	case idedisk_pm_restore_pio:	/* Resume step 1 complete */
+		pm->pm_step = idedisk_pm_idle;
+		break;
+	case idedisk_pm_idle:		/* Resume step 2 (idle) complete */
 		pm->pm_step = ide_pm_restore_dma;
 		break;
 	}
@@ -170,8 +174,11 @@
 	memset(args, 0, sizeof(*args));
 
 	if (drive->media != ide_disk) {
-		/* skip idedisk_pm_idle for ATAPI devices */
-		if (pm->pm_step == idedisk_pm_idle)
+		/*
+		 * skip idedisk_pm_restore_pio and idedisk_pm_idle for ATAPI
+		 * devices
+		 */
+		if (pm->pm_step == idedisk_pm_restore_pio)
 			pm->pm_step = ide_pm_restore_dma;
 	}
 
@@ -198,13 +205,19 @@
 		args->handler	   = &task_no_data_intr;
 		return do_rw_taskfile(drive, args);
 
-	case idedisk_pm_idle:		/* Resume step 1 (idle) */
+	case idedisk_pm_restore_pio:	/* Resume step 1 (restore PIO) */
+		if (drive->hwif->tuneproc != NULL)
+			drive->hwif->tuneproc(drive, 255);
+		ide_complete_power_step(drive, rq, 0, 0);
+		return ide_stopped;
+
+	case idedisk_pm_idle:		/* Resume step 2 (idle) */
 		args->tfRegister[IDE_COMMAND_OFFSET] = WIN_IDLEIMMEDIATE;
 		args->command_type = IDE_DRIVE_TASK_NO_DATA;
 		args->handler = task_no_data_intr;
 		return do_rw_taskfile(drive, args);
 
-	case ide_pm_restore_dma:	/* Resume step 2 (restore DMA) */
+	case ide_pm_restore_dma:	/* Resume step 3 (restore DMA) */
 		/*
 		 * Right now, all we do is call hwif->ide_dma_check(drive),
 		 * we could be smarter and check for current xfer_speed

Re: [patch, rft] ide: reprogram disk pio timings on resume

[Brad Campbell]

Jason Lunz wrote:
> Add a step to the IDE PM state machine that reprograms disk PIO timings
> as the first step on resume. This prevents ide deadlock on
> resume-from-ram on my nforce3-based laptop.
> 
<snip>
> 
> Brad: thanks for volunteering to test! You'll be the lucky first person
> to try this on a non-nforce ide chipset. good luck.

No change to the behaviour here.. It does not hurt, but it does not help either.
I have not been able to get logs out of this machine yet while it's resuming. (no serial port and 
don't have another machine handy to test with netconsole)

Hard disk locks up for 30 secs and then resumes with a dropped interrupt message and I get a console 
back but all hard disk access result in timeouts. (I'm testing from a complete system initramfs)

As I said, with 2.6.18-rc2 and the libata patches from -mm1 it takes about 10 seconds to come back, 
but resumes reliably.


Brad
-- 
"Human beings, who are almost unique in having the ability
to learn from the experience of others, are also remarkable
for their apparent disinclination to do so." -- Douglas Adams

Re: [patch, rft] ide: reprogram disk pio timings on resume

[Alan Cox]

Ar Sad, 2006-07-29 am 19:34 -0400, ysgrifennodd Jason Lunz:
> driver, but Alan helpfully pointed out that this is the correct thing to
> do globally. Still, I'm only calling hwif->tuneproc() for disks, based
> on two things:
>
>  - The existing state machine is already passed over for non-disk drives

CDs should in theory also need the PIO restore.

Otherwise looks excellent