Re: [PATCH] Fixup ahci suspend / resume

[Hannes Reinecke <hare@suse.de>]

Randy.Dunlap wrote:
> On Tue, 28 Feb 2006 18:25:43 +0100 Jens Axboe wrote:
> 
>> On Tue, Feb 28 2006, Hannes Reinecke wrote:
>>> Jeff Garzik wrote:
>>>> Jens Axboe wrote:
>>>>> I'm sure Hannes will regenerate against upstream as well if necessary,
>>>>> however that depends on when this should be applied.
>>>> It's far too late and too intrusive for 2.6.16-rc.
>>>>
>>>> It's #upstream or <null>.
>>>>
>>> Calm down. Of course I will regenerate is against #upstream.
>>> In fact, I've done so initially but wasn't sure what the status of
>>> libata-dev is. So I've diffed it against linux-git instead.
>>>
>>> Updated patch to follow.
>> Thanks Hannes. I wasn't so much worried about it going directly in (I
>> have no problem waiting), just that there seemed to be some disagreement
>> on where suspend is at and what we can "support".
> 
> Hi Hannes,
> 
> Did you ever send the updated patch?  I need it to use with the
> rest of the ata-acpi objects patchset for #upstream (actually for -mm).
> 
Oh, sorry. I got tangled up with other issues (namely ACPI integration
for IDE drives :-).
Patch against libata-dev is attached.

Please apply.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux Products GmbH		S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

ahci: Fixup ahci suspend / resume

The current ahci driver has the problem that it doesn't resume properly.
Or rather, that resuming is unstable.
Reason being that AHCI has 4 registers containing the DMA address it
should write things to. Of course there is no guarantee that Linux has
assigned the same address to the DMA area across reboots.
So we should better re-initialize those registers after resume.

The patch also improves the port_start / port_stop routines to be more
closely modelled after the spec. This also avoids a nasty msleep(500)
during initialisation.

And as we now have the individual routines split off we can as well
use them during startup and not rely on any hardcoded values here.

Signed-off-by: Hannes Reinecke <hare@suse.de>

diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 1c2ab3d..e0dc933 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -30,6 +30,12 @@
  * http://www.intel.com/technology/serialata/pdf/rev1_0.pdf
  * http://www.intel.com/technology/serialata/pdf/rev1_1.pdf
  *
+ * TODO:
+ * - Error handling; some routines might fail (eg port_suspend / _resume)
+ *   upon which a port reset should be done
+ * - Proper handling of additional features: cold-presence detection,
+ *   staggered spin-up, power modes. Currently we're calling those commands
+ *   as we pleas.
  */
 
 #include <linux/kernel.h>
@@ -44,6 +50,7 @@
 #include <linux/device.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_device.h>
 #include <linux/libata.h>
 #include <asm/io.h>
 
@@ -194,11 +201,21 @@ static int ahci_probe_reset(struct ata_p
 static void ahci_irq_clear(struct ata_port *ap);
 static void ahci_eng_timeout(struct ata_port *ap);
 static int ahci_port_start(struct ata_port *ap);
+static int ahci_port_resume(struct ata_port *ap);
+static int ahci_port_suspend(struct ata_port *ap);
 static void ahci_port_stop(struct ata_port *ap);
+static int ahci_start_engine(void __iomem *port_mmio);
+static int ahci_stop_engine(void __iomem *port_mmio);
+static void ahci_start_fis_rx(void __iomem *port_mmio, 
+			      struct ahci_port_priv *pp,
+			      struct ahci_host_priv *hpriv);
+static int ahci_stop_fis_rx(void __iomem *port_mmio);
 static void ahci_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
 static void ahci_qc_prep(struct ata_queued_cmd *qc);
 static u8 ahci_check_status(struct ata_port *ap);
 static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
+static int ahci_scsi_device_suspend(struct scsi_device *sdev);
+static int ahci_scsi_device_resume(struct scsi_device *sdev);
 static void ahci_remove_one (struct pci_dev *pdev);
 
 static struct scsi_host_template ahci_sht = {
@@ -218,6 +235,8 @@ static struct scsi_host_template ahci_sh
 	.dma_boundary		= AHCI_DMA_BOUNDARY,
 	.slave_configure	= ata_scsi_slave_config,
 	.bios_param		= ata_std_bios_param,
+	.resume			= ahci_scsi_device_resume,
+	.suspend		= ahci_scsi_device_suspend,
 };
 
 static const struct ata_port_operations ahci_ops = {
@@ -302,6 +321,8 @@ static struct pci_driver ahci_pci_driver
 	.id_table		= ahci_pci_tbl,
 	.probe			= ahci_init_one,
 	.remove			= ahci_remove_one,
+	.suspend		= ata_pci_device_suspend,
+	.resume			= ata_pci_device_resume,       
 };
 
 
@@ -375,42 +396,24 @@ static int ahci_port_start(struct ata_po
 
 	ap->private_data = pp;
 
-	if (hpriv->cap & HOST_CAP_64)
-		writel((pp->cmd_slot_dma >> 16) >> 16, port_mmio + PORT_LST_ADDR_HI);
-	writel(pp->cmd_slot_dma & 0xffffffff, port_mmio + PORT_LST_ADDR);
-	readl(port_mmio + PORT_LST_ADDR); /* flush */
-
-	if (hpriv->cap & HOST_CAP_64)
-		writel((pp->rx_fis_dma >> 16) >> 16, port_mmio + PORT_FIS_ADDR_HI);
-	writel(pp->rx_fis_dma & 0xffffffff, port_mmio + PORT_FIS_ADDR);
-	readl(port_mmio + PORT_FIS_ADDR); /* flush */
-
-	writel(PORT_CMD_ICC_ACTIVE | PORT_CMD_FIS_RX |
-	       PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP |
-	       PORT_CMD_START, port_mmio + PORT_CMD);
-	readl(port_mmio + PORT_CMD); /* flush */
-
-	return 0;
+	/*
+	 * Driver is setup, now initialize the HBA
+	 */
+	ahci_start_fis_rx(port_mmio, pp, hpriv);
+	
+	rc = ahci_start_engine(port_mmio);
+	if (rc)
+		printk(KERN_WARNING "ata%u: could not start DMA engine\n", 
+		       ap->id);
+	return rc;
 }
 
-
 static void ahci_port_stop(struct ata_port *ap)
 {
 	struct device *dev = ap->host_set->dev;
 	struct ahci_port_priv *pp = ap->private_data;
-	void __iomem *mmio = ap->host_set->mmio_base;
-	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
-	u32 tmp;
 
-	tmp = readl(port_mmio + PORT_CMD);
-	tmp &= ~(PORT_CMD_START | PORT_CMD_FIS_RX);
-	writel(tmp, port_mmio + PORT_CMD);
-	readl(port_mmio + PORT_CMD); /* flush */
-
-	/* spec says 500 msecs for each PORT_CMD_{START,FIS_RX} bit, so
-	 * this is slightly incorrect.
-	 */
-	msleep(500);
+	ahci_port_suspend(ap);
 
 	ap->private_data = NULL;
 	dma_free_coherent(dev, AHCI_PORT_PRIV_DMA_SZ,
@@ -419,6 +422,45 @@ static void ahci_port_stop(struct ata_po
 	kfree(pp);
 }
 
+static int ahci_port_resume(struct ata_port *ap)
+{
+	void __iomem *mmio = ap->host_set->mmio_base;
+	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
+	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
+
+	/*
+	 * Enable FIS reception
+	 */
+	ahci_start_fis_rx(port_mmio, pp, hpriv);
+
+	/*
+	 * Enable DMA
+	 */
+	return ahci_start_engine(port_mmio);
+}
+
+static int ahci_port_suspend(struct ata_port *ap)
+{
+	void __iomem *mmio = ap->host_set->mmio_base;
+	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
+	int rc;
+
+	/*
+	 * Disable DMA
+	 */
+	rc = ahci_stop_engine(port_mmio);
+	if (rc) {
+		printk(KERN_WARNING "ata%u: DMA engine busy\n", ap->id);
+		return rc;
+	}
+
+	/*
+	 * Disable FIS reception
+	 */
+	return ahci_stop_fis_rx(port_mmio);
+}
+
 static u32 ahci_scr_read (struct ata_port *ap, unsigned int sc_reg_in)
 {
 	unsigned int sc_reg;
@@ -453,19 +495,23 @@ static void ahci_scr_write (struct ata_p
 	writel(val, (void __iomem *) ap->ioaddr.scr_addr + (sc_reg * 4));
 }
 
-static int ahci_stop_engine(struct ata_port *ap)
+static int ahci_stop_engine(void __iomem *port_mmio)
 {
-	void __iomem *mmio = ap->host_set->mmio_base;
-	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
 	int work;
 	u32 tmp;
 
 	tmp = readl(port_mmio + PORT_CMD);
+	/* Check if the HBA is idle */
+	if ((tmp & (PORT_CMD_START | PORT_CMD_LIST_ON)) == 0)
+		return 0;
+
+	/* Setting HBA to idle */
 	tmp &= ~PORT_CMD_START;
 	writel(tmp, port_mmio + PORT_CMD);
 
-	/* wait for engine to stop.  TODO: this could be
-	 * as long as 500 msec
+	/* 
+	 * wait for engine to stop.
+	 * This could be as long as 500 msec
 	 */
 	work = 1000;
 	while (work-- > 0) {
@@ -478,16 +524,120 @@ static int ahci_stop_engine(struct ata_p
 	return -EIO;
 }
 
-static void ahci_start_engine(struct ata_port *ap)
+static int ahci_start_engine(void __iomem *port_mmio)
 {
-	void __iomem *mmio = ap->host_set->mmio_base;
-	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
 	u32 tmp;
+	int work = 1000;
 
+	/*
+	 * Get current status
+	 */
 	tmp = readl(port_mmio + PORT_CMD);
+
+	/*
+	 * AHCI rev 1.1 section 10.3.1:
+	 * Software shall not set PxCMD.ST to â??1â?? until it verifies
+	 * that PxCMD.CR is â??0â?? and has set PxCMD.FRE to â??1â??.
+	 */
+	if ((tmp & PORT_CMD_FIS_RX) == 0) {
+		return -EIO;
+	}
+
+	/* 
+	 * wait for DMA engine to become idle.
+	 */
+	if (tmp & PORT_CMD_LIST_ON) {
+		while (work-- > 0) {
+			tmp = readl(port_mmio + PORT_CMD);
+			if ((tmp & PORT_CMD_LIST_ON) == 0)
+				break;
+			udelay(10);
+		}
+	}
+
+	if (!work) {
+		/*
+		 * We need to do a port reset / HBA reset here.
+		 */
+		return -EIO;
+	}
+
+	/*
+	 * Start DMA
+	 */
 	tmp |= PORT_CMD_START;
 	writel(tmp, port_mmio + PORT_CMD);
 	readl(port_mmio + PORT_CMD); /* flush */
+
+	return 0;
+}
+
+static int ahci_stop_fis_rx(void __iomem *port_mmio)
+{
+	u32 tmp;
+	int work = 1000;
+
+	/*
+	 * Get current status
+	 */
+	tmp = readl(port_mmio + PORT_CMD);
+
+	/*
+	 * Disable FIS reception
+	 */
+	if (tmp & PORT_CMD_FIS_RX) {
+		tmp &= ~(PORT_CMD_FIS_RX);
+		writel(tmp, port_mmio + PORT_CMD);
+		tmp = readl(port_mmio + PORT_CMD); /* flush */
+	}
+
+	/*
+	 * Wait for HBA to become idle.
+	 * This could be as long as 500 msec
+	 */
+	if (tmp & PORT_CMD_FIS_ON) {
+		work = 1000;
+		while (work-- > 0) {
+			tmp = readl(port_mmio + PORT_CMD);
+			if ((tmp & PORT_CMD_FIS_ON) == 0)
+				return 0;
+			udelay(10);
+		}
+	}
+	
+	return -EIO;
+}
+
+static void ahci_start_fis_rx(void __iomem *port_mmio, 
+			      struct ahci_port_priv *pp,
+			      struct ahci_host_priv *hpriv)
+{
+	/*
+	 * Enable FIS reception
+	 */
+	if (hpriv->cap & HOST_CAP_64)
+		writel((pp->cmd_slot_dma >> 16) >> 16, port_mmio + PORT_LST_ADDR_HI);
+	writel(pp->cmd_slot_dma & 0xffffffff, port_mmio + PORT_LST_ADDR);
+	readl(port_mmio + PORT_LST_ADDR); /* flush */
+
+	if (hpriv->cap & HOST_CAP_64)
+		writel((pp->rx_fis_dma >> 16) >> 16, port_mmio + PORT_FIS_ADDR_HI);
+	writel(pp->rx_fis_dma & 0xffffffff, port_mmio + PORT_FIS_ADDR);
+	readl(port_mmio + PORT_FIS_ADDR); /* flush */
+
+	/*
+	 * This is wrong. We should only activate
+	 * FIX_RX here; everything else should be handled
+	 * separately.
+	 * Some bits might not even be settable here
+	 * as they depend on the respective feature to be
+	 * implemented (Staggered Spin-up, 
+	 * Cold-presence detection etc.)
+	 */
+	writel(PORT_CMD_ICC_ACTIVE | PORT_CMD_FIS_RX |
+	       PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP,
+	       port_mmio + PORT_CMD);
+	readl(port_mmio + PORT_CMD); /* flush */
 }
 
 static unsigned int ahci_dev_classify(struct ata_port *ap)
@@ -515,13 +665,16 @@ static void ahci_fill_cmd_slot(struct ah
 
 static int ahci_hardreset(struct ata_port *ap, int verbose, unsigned int *class)
 {
+	void __iomem *mmio = ap->host_set->mmio_base;
+	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
 	int rc;
 
 	DPRINTK("ENTER\n");
 
-	ahci_stop_engine(ap);
+	ahci_stop_engine(port_mmio);
 	rc = sata_std_hardreset(ap, verbose, class);
-	ahci_start_engine(ap);
+	if (!rc)
+		rc = ahci_start_engine(port_mmio);
 
 	if (rc == 0)
 		*class = ahci_dev_classify(ap);
@@ -641,6 +794,7 @@ static void ahci_restart_port(struct ata
 	void __iomem *mmio = ap->host_set->mmio_base;
 	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
 	u32 tmp;
+	int rc;
 
 	if ((ap->device[0].class != ATA_DEV_ATAPI) ||
 	    ((irq_stat & PORT_IRQ_TF_ERR) == 0))
@@ -656,7 +810,7 @@ static void ahci_restart_port(struct ata
 			readl(port_mmio + PORT_SCR_ERR));
 
 	/* stop DMA */
-	ahci_stop_engine(ap);
+	rc = ahci_stop_engine(port_mmio);
 
 	/* clear SATA phy error, if any */
 	tmp = readl(port_mmio + PORT_SCR_ERR);
@@ -666,7 +820,7 @@ static void ahci_restart_port(struct ata
 	 * if so, issue COMRESET
 	 */
 	tmp = readl(port_mmio + PORT_TFDATA);
-	if (tmp & (ATA_BUSY | ATA_DRQ)) {
+	if (rc || (tmp & (ATA_BUSY | ATA_DRQ))) {
 		writel(0x301, port_mmio + PORT_SCR_CTL);
 		readl(port_mmio + PORT_SCR_CTL); /* flush */
 		udelay(10);
@@ -675,7 +829,7 @@ static void ahci_restart_port(struct ata
 	}
 
 	/* re-start DMA */
-	ahci_start_engine(ap);
+	rc = ahci_start_engine(port_mmio);
 }
 
 static void ahci_eng_timeout(struct ata_port *ap)
@@ -823,6 +977,31 @@ static unsigned int ahci_qc_issue(struct
 	return 0;
 }
 
+int ahci_scsi_device_suspend(struct scsi_device *sdev)
+{
+	struct ata_port *ap = (struct ata_port *) &sdev->host->hostdata[0];
+	struct ata_device *dev = &ap->device[sdev->id];
+	int rc;
+
+	rc = ata_device_suspend(ap, dev);
+
+	if (!rc) {
+		rc = ahci_port_suspend(ap);
+	}
+
+	return rc;
+}
+
+int ahci_scsi_device_resume(struct scsi_device *sdev)
+{
+	struct ata_port *ap = (struct ata_port *) &sdev->host->hostdata[0];
+	struct ata_device *dev = &ap->device[sdev->id];
+
+	ahci_port_resume(ap);
+
+	return ata_device_resume(ap, dev);
+}
+
 static void ahci_setup_port(struct ata_ioports *port, unsigned long base,
 			    unsigned int port_idx)
 {
@@ -930,23 +1109,18 @@ static int ahci_host_init(struct ata_pro
 				(unsigned long) mmio, i);
 
 		/* make sure port is not active */
-		tmp = readl(port_mmio + PORT_CMD);
-		VPRINTK("PORT_CMD 0x%x\n", tmp);
-		if (tmp & (PORT_CMD_LIST_ON | PORT_CMD_FIS_ON |
-			   PORT_CMD_FIS_RX | PORT_CMD_START)) {
-			tmp &= ~(PORT_CMD_LIST_ON | PORT_CMD_FIS_ON |
-				 PORT_CMD_FIS_RX | PORT_CMD_START);
-			writel(tmp, port_mmio + PORT_CMD);
-			readl(port_mmio + PORT_CMD); /* flush */
-
-			/* spec says 500 msecs for each bit, so
-			 * this is slightly incorrect.
-			 */
-			msleep(500);
-		}
+		ahci_stop_engine(port_mmio);
+		ahci_stop_fis_rx(port_mmio);
+
+		/*
+		 * TODO: port / HBA reset if the above fails
+		 */
 
 		writel(PORT_CMD_SPIN_UP, port_mmio + PORT_CMD);
 
+		/*
+		 * Wait for communications link to establish
+		 */
 		j = 0;
 		while (j < 100) {
 			msleep(10);