[patch 1/3] Store interrupt value

[patch 1/3] Store interrupt value

[Kristen Carlson Accardi]

Use a stored value for which interrupts to enable.  Changing this allows
us to selectively turn off certain interrupts later and have them 
stay off.

Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/drivers/ata/ahci.c
===================================================================
--- 2.6-git.orig/drivers/ata/ahci.c
+++ 2.6-git/drivers/ata/ahci.c
@@ -211,6 +211,7 @@ struct ahci_port_priv {
 	unsigned int		ncq_saw_d2h:1;
 	unsigned int		ncq_saw_dmas:1;
 	unsigned int		ncq_saw_sdb:1;
+	u32 			intr_mask;	/* interrupts to enable */
 };
 
 static u32 ahci_scr_read (struct ata_port *ap, unsigned int sc_reg);
@@ -1384,6 +1385,7 @@ static void ahci_thaw(struct ata_port *a
 	void __iomem *mmio = ap->host->iomap[AHCI_PCI_BAR];
 	void __iomem *port_mmio = ahci_port_base(ap);
 	u32 tmp;
+	struct ahci_port_priv *pp = ap->private_data;
 
 	/* clear IRQ */
 	tmp = readl(port_mmio + PORT_IRQ_STAT);
@@ -1391,7 +1393,7 @@ static void ahci_thaw(struct ata_port *a
 	writel(1 << ap->port_no, mmio + HOST_IRQ_STAT);
 
 	/* turn IRQ back on */
-	writel(DEF_PORT_IRQ, port_mmio + PORT_IRQ_MASK);
+	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
 static void ahci_error_handler(struct ata_port *ap)
@@ -1547,6 +1549,12 @@ static int ahci_port_start(struct ata_po
 	pp->cmd_tbl = mem;
 	pp->cmd_tbl_dma = mem_dma;
 
+	/*
+ 	 * Save off initial list of interrupts to be enabled.
+ 	 * This could be changed later
+ 	 */
+	pp->intr_mask = DEF_PORT_IRQ;
+
 	ap->private_data = pp;
 
 	/* power up port */

-- 

Re: [patch 1/3] Store interrupt value

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> Use a stored value for which interrupts to enable.  Changing this allows
> us to selectively turn off certain interrupts later and have them 
> stay off.
> 
> Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Seems OK, though a bit disappointing that this cannot be initial set in
ahci_save_initial_config() [which admittedly only saves per-HBA settings 
at the moment, but due to new init model, need not be limited as such]

[patch 2/3] Expose Power Management Policy option to users

[Kristen Carlson Accardi]

This patch will modify the scsi and ata subsystem to allow
users to set a power management policy for the link.
libata drivers can define a function (enable_pm) that will
perform hardware specific actions to enable whatever power
management policy the user sets up if the driver supports
it.  This power management policy will be activated after
all disks have been enumerated and intialized.

The scsi subsystem will create a new sysfs file for each
host in /sys/class/scsi_host called "link_power_management_policy".
This file can have 3 possible values:

Value		Meaning
-------------------------------------------------------------------
min_power	User wishes the link to conserve power as much as
		possible, even at the cost of some performance

max_performance User wants priority to be on performance, not power
		savings

medium_power	User wants power savings, with less performance cost
		than min_power (but less power savings as well).


Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
Index: 2.6-git/drivers/ata/libata-scsi.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-scsi.c
+++ 2.6-git/drivers/ata/libata-scsi.c
@@ -2890,6 +2890,51 @@ void ata_scsi_simulate(struct ata_device
 	}
 }
 
+int ata_scsi_set_link_pm_policy(struct Scsi_Host *shost,
+		enum scsi_host_link_pm policy)
+{
+	struct ata_port *ap = ata_shost_to_port(shost);
+	int rc = -EINVAL;
+	int i;
+
+	/*
+ 	 * make sure no broken devices are on this port,
+ 	 * and that all devices support interface power
+ 	 * management
+ 	 */
+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
+		struct ata_device *dev = &ap->device[i];
+
+		/* only check drives which exist */
+		if (!ata_dev_enabled(dev))
+			continue;
+
+		/*
+ 		 * do we need to handle the case where we've hotplugged
+ 		 * a broken drive (since hotplug and ALPM are mutually
+ 		 * exclusive) ?
+ 		 *
+ 		 * If so, if we detect a broken drive on a port with
+ 		 * alpm already enabled, then we should reset the policy
+ 		 * to off for the entire port.
+ 		 */
+		if ((dev->horkage & ATA_HORKAGE_ALPM) ||
+			!(dev->flags & ATA_DFLAG_IPM)) {
+			ata_dev_printk(dev, KERN_ERR,
+				"Unable to set Link PM policy\n");
+			ap->pm_policy = SHOST_MAX_PERFORMANCE;
+		}
+	}
+
+	if (ap->ops->enable_pm)
+		rc = ap->ops->enable_pm(ap, policy);
+
+	if (!rc)
+		shost->shost_link_pm_policy = ap->pm_policy;
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ata_scsi_set_link_pm_policy);
+
 int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 {
 	int i, rc;
@@ -2912,7 +2957,7 @@ int ata_scsi_add_hosts(struct ata_host *
 		shost->max_lun = 1;
 		shost->max_channel = 1;
 		shost->max_cmd_len = 16;
-
+		shost->shost_link_pm_policy = ap->pm_policy;
 		rc = scsi_add_host(ap->scsi_host, ap->host->dev);
 		if (rc)
 			goto err_add;
Index: 2.6-git/drivers/scsi/hosts.c
===================================================================
--- 2.6-git.orig/drivers/scsi/hosts.c
+++ 2.6-git/drivers/scsi/hosts.c
@@ -54,6 +54,25 @@ static struct class shost_class = {
 };
 
 /**
+ *	scsi_host_set_link_pm - Change the link power management policy
+ *	@shost:	scsi host to change the policy of.
+ *	@policy:	policy to change to.
+ *
+ *	Returns zero if successful or an error if the requested
+ *	transition is illegal.
+ **/
+int scsi_host_set_link_pm(struct Scsi_Host *shost,
+		enum scsi_host_link_pm policy)
+{
+	struct scsi_host_template *sht = shost->hostt;
+	if (sht->set_link_pm_policy)
+		sht->set_link_pm_policy(shost, policy);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(scsi_host_set_link_pm);
+
+/**
  *	scsi_host_set_state - Take the given host through the host
  *		state model.
  *	@shost:	scsi host to change the state of.
Index: 2.6-git/drivers/scsi/scsi_sysfs.c
===================================================================
--- 2.6-git.orig/drivers/scsi/scsi_sysfs.c
+++ 2.6-git/drivers/scsi/scsi_sysfs.c
@@ -189,6 +189,74 @@ show_shost_state(struct class_device *cl
 
 static CLASS_DEVICE_ATTR(state, S_IRUGO | S_IWUSR, show_shost_state, store_shost_state);
 
+static const struct {
+	enum scsi_host_link_pm	value;
+	char			*name;
+} shost_link_pm_policy[] = {
+	{ SHOST_NOT_AVAILABLE, "max_performance" },
+	{ SHOST_MIN_POWER, "min_power" },
+	{ SHOST_MAX_PERFORMANCE, "max_performance" },
+	{ SHOST_MEDIUM_POWER, "medium_power" },
+};
+
+const char *scsi_host_link_pm_policy(enum scsi_host_link_pm policy)
+{
+	int i;
+	char *name = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(shost_link_pm_policy); i++) {
+		if (shost_link_pm_policy[i].value == policy) {
+			name = shost_link_pm_policy[i].name;
+			break;
+		}
+	}
+	return name;
+}
+
+static ssize_t store_link_pm_policy(struct class_device *class_dev,
+	const char *buf, size_t count)
+{
+	struct Scsi_Host *shost = class_to_shost(class_dev);
+	enum scsi_host_link_pm policy = 0;
+	int i;
+
+	/*
+ 	 * we are skipping array location 0 on purpose - this
+ 	 * is because a value of SHOST_NOT_AVAILABLE is displayed
+ 	 * to the user as max_performance, but when the user
+ 	 * writes "max_performance", they actually want the
+ 	 * value to match SHOST_MAX_PERFORMANCE.
+ 	 */
+	for (i = 1; i < ARRAY_SIZE(shost_link_pm_policy); i++) {
+		const int len = strlen(shost_link_pm_policy[i].name);
+		if (strncmp(shost_link_pm_policy[i].name, buf, len) == 0 &&
+		   buf[len] == '\n') {
+			policy = shost_link_pm_policy[i].value;
+			break;
+		}
+	}
+	if (!policy)
+		return -EINVAL;
+
+	if (scsi_host_set_link_pm(shost, policy))
+		return -EINVAL;
+	return count;
+}
+static ssize_t
+show_link_pm_policy(struct class_device *class_dev, char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(class_dev);
+	const char *policy =
+		scsi_host_link_pm_policy(shost->shost_link_pm_policy);
+
+	if (!policy)
+		return -EINVAL;
+
+	return snprintf(buf, 23, "%s\n", policy);
+}
+static CLASS_DEVICE_ATTR(link_power_management_policy, S_IRUGO | S_IWUSR,
+		show_link_pm_policy, store_link_pm_policy);
+
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(host_busy, "%hu\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
@@ -207,6 +275,7 @@ static struct class_device_attribute *sc
 	&class_device_attr_proc_name,
 	&class_device_attr_scan,
 	&class_device_attr_state,
+	&class_device_attr_link_power_management_policy,
 	NULL
 };
 
Index: 2.6-git/include/scsi/scsi_host.h
===================================================================
--- 2.6-git.orig/include/scsi/scsi_host.h
+++ 2.6-git/include/scsi/scsi_host.h
@@ -42,6 +42,16 @@ enum scsi_eh_timer_return {
 	EH_RESET_TIMER,
 };
 
+/*
+ * shost pm policy: If you alter this, you also need to alter scsi_sysfs.c
+ * (for the ascii descriptions)
+ */
+enum scsi_host_link_pm {
+	SHOST_NOT_AVAILABLE,
+	SHOST_MIN_POWER,
+	SHOST_MAX_PERFORMANCE,
+	SHOST_MEDIUM_POWER,
+};
 
 struct scsi_host_template {
 	struct module *module;
@@ -345,6 +355,12 @@ struct scsi_host_template {
 	int (*suspend)(struct scsi_device *, pm_message_t state);
 
 	/*
+ 	 * link power management support
+ 	 */
+	int (*set_link_pm_policy)(struct Scsi_Host *, enum scsi_host_link_pm);
+	enum scsi_host_link_pm default_link_pm_policy;
+
+	/*
 	 * Name of proc directory
 	 */
 	char *proc_name;
@@ -642,6 +658,7 @@ struct Scsi_Host {
 	
 
 	enum scsi_host_state shost_state;
+	enum scsi_host_link_pm shost_link_pm_policy;
 
 	/* ldm bits */
 	struct device		shost_gendev;
@@ -749,4 +766,5 @@ extern struct Scsi_Host *scsi_register(s
 extern void scsi_unregister(struct Scsi_Host *);
 extern int scsi_host_set_state(struct Scsi_Host *, enum scsi_host_state);
 
+extern int scsi_host_set_link_pm(struct Scsi_Host *, enum scsi_host_link_pm);
 #endif /* _SCSI_SCSI_HOST_H */
Index: 2.6-git/include/linux/libata.h
===================================================================
--- 2.6-git.orig/include/linux/libata.h
+++ 2.6-git/include/linux/libata.h
@@ -136,6 +136,7 @@ enum {
 	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
 	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
 	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
+	ATA_DFLAG_IPM		= (1 << 6), /* device supports interface PM */
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
@@ -299,6 +300,7 @@ enum {
 	ATA_HORKAGE_NONCQ	= (1 << 2),	/* Don't use NCQ */
 	ATA_HORKAGE_MAX_SEC_128	= (1 << 3),	/* Limit max sects to 128 */
 	ATA_HORKAGE_DMA_RW_ONLY	= (1 << 4),	/* ATAPI DMA for RW only */
+	ATA_HORKAGE_ALPM	= (1 << 5), 	/* ALPM problems */
 };
 
 enum hsm_task_states {
@@ -547,6 +549,7 @@ struct ata_port {
 
 	pm_message_t		pm_mesg;
 	int			*pm_result;
+	enum scsi_host_link_pm	pm_policy;
 
 	void			*private_data;
 
@@ -606,7 +609,7 @@ struct ata_port_operations {
 
 	int (*port_suspend) (struct ata_port *ap, pm_message_t mesg);
 	int (*port_resume) (struct ata_port *ap);
-
+	int (*enable_pm) (struct ata_port *ap, enum scsi_host_link_pm policy);
 	int (*port_start) (struct ata_port *ap);
 	void (*port_stop) (struct ata_port *ap);
 
@@ -811,7 +814,7 @@ extern int ata_cable_40wire(struct ata_p
 extern int ata_cable_80wire(struct ata_port *ap);
 extern int ata_cable_sata(struct ata_port *ap);
 extern int ata_cable_unknown(struct ata_port *ap);
-
+extern int ata_scsi_set_link_pm_policy(struct Scsi_Host *shost, enum scsi_host_link_pm);
 /*
  * Timing helpers
  */
Index: 2.6-git/drivers/ata/libata-core.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-core.c
+++ 2.6-git/drivers/ata/libata-core.c
@@ -2019,6 +2019,9 @@ int ata_dev_configure(struct ata_device 
 	if (dev->flags & ATA_DFLAG_LBA48)
 		dev->max_sectors = ATA_MAX_SECTORS_LBA48;
 
+	if (ata_id_has_hipm(dev->id))
+		dev->flags |= ATA_DFLAG_IPM;
+
 	if (dev->horkage & ATA_HORKAGE_DIAGNOSTIC) {
 		/* Let the user know. We don't want to disallow opens for
 		   rescue purposes, or in case the vendor is just a blithering
@@ -2048,6 +2051,13 @@ int ata_dev_configure(struct ata_device 
 	if (ata_device_blacklisted(dev) & ATA_HORKAGE_DMA_RW_ONLY)
 		dev->horkage |= ATA_HORKAGE_DMA_RW_ONLY;
 
+	if (ata_device_blacklisted(dev) & ATA_HORKAGE_ALPM) {
+		dev->horkage |= ATA_HORKAGE_ALPM;
+
+		/* reset link pm_policy for this port to no pm */
+		ap->pm_policy = SHOST_MAX_PERFORMANCE;
+	}
+
 	if (ap->ops->dev_config)
 		ap->ops->dev_config(dev);
 
@@ -6389,6 +6399,8 @@ int ata_host_register(struct ata_host *h
 		struct ata_port *ap = host->ports[i];
 
 		ata_scsi_scan_host(ap);
+		ata_scsi_set_link_pm_policy(ap->scsi_host,
+				ap->pm_policy);
 	}
 
 	return 0;
Index: 2.6-git/Documentation/scsi/link_power_management_policy.txt
===================================================================
--- /dev/null
+++ 2.6-git/Documentation/scsi/link_power_management_policy.txt
@@ -0,0 +1,19 @@
+This parameter allows the user to set the link (interface) power management.
+There are 3 possible options:
+
+Value			Effect
+----------------------------------------------------------------------------
+min_power		Tell the controller to try to make the link use the
+			least possible power when possible.  This may
+			sacrifice some performance due to increased latency
+			when coming out of lower power states.
+
+max_performance		Generally, this means no power management.  Tell
+			the controller to have performance be a priority
+			over power management.
+
+medium_power		Tell the controller to enter a lower power state
+			when possible, but do not enter the lowest power
+			state, thus improving latency over min_power setting.
+
+
Index: 2.6-git/include/linux/ata.h
===================================================================
--- 2.6-git.orig/include/linux/ata.h
+++ 2.6-git/include/linux/ata.h
@@ -320,6 +320,8 @@ struct ata_taskfile {
 	  ((u64) (id)[(n) + 0]) )
 
 #define ata_id_cdb_intr(id)	(((id)[0] & 0x60) == 0x20)
+#define ata_id_has_hipm(id)	((id)[76] & (1 << 9))
+#define ata_id_has_dipm(id)	((id)[78] & (1 << 3))
 
 static inline unsigned int ata_id_major_version(const u16 *id)
 {

-- 

Re: [patch 2/3] Expose Power Management Policy option to users

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> This patch will modify the scsi and ata subsystem to allow
> users to set a power management policy for the link.
> libata drivers can define a function (enable_pm) that will
> perform hardware specific actions to enable whatever power
> management policy the user sets up if the driver supports
> it.  This power management policy will be activated after
> all disks have been enumerated and intialized.
> 
> The scsi subsystem will create a new sysfs file for each
> host in /sys/class/scsi_host called "link_power_management_policy".
> This file can have 3 possible values:
> 
> Value		Meaning
> -------------------------------------------------------------------
> min_power	User wishes the link to conserve power as much as
> 		possible, even at the cost of some performance
> 
> max_performance User wants priority to be on performance, not power
> 		savings
> 
> medium_power	User wants power savings, with less performance cost
> 		than min_power (but less power savings as well).
> 
> 
> Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

seems OK at first glance, though I request that ata and scsi portions be 
split into separate patches

Re: [patch 2a/3] Expose Power Management Policy option to users

[Kristen Carlson Accardi]

Expose Power Management Policy option to users

This patch will modify the scsi subsystem to allow
users to set a power management policy for the link.

The scsi subsystem will create a new sysfs file for each
host in /sys/class/scsi_host called "link_power_management_policy".
This file can have 3 possible values:

Value		Meaning
-------------------------------------------------------------------
min_power	User wishes the link to conserve power as much as
		possible, even at the cost of some performance

max_performance User wants priority to be on performance, not power
		savings

medium_power	User wants power savings, with less performance cost
		than min_power (but less power savings as well).


Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
Index: 2.6-git/drivers/scsi/hosts.c
===================================================================
--- 2.6-git.orig/drivers/scsi/hosts.c
+++ 2.6-git/drivers/scsi/hosts.c
@@ -54,6 +54,25 @@ static struct class shost_class = {
 };
 
 /**
+ *	scsi_host_set_link_pm - Change the link power management policy
+ *	@shost:	scsi host to change the policy of.
+ *	@policy:	policy to change to.
+ *
+ *	Returns zero if successful or an error if the requested
+ *	transition is illegal.
+ **/
+int scsi_host_set_link_pm(struct Scsi_Host *shost,
+		enum scsi_host_link_pm policy)
+{
+	struct scsi_host_template *sht = shost->hostt;
+	if (sht->set_link_pm_policy)
+		sht->set_link_pm_policy(shost, policy);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(scsi_host_set_link_pm);
+
+/**
  *	scsi_host_set_state - Take the given host through the host
  *		state model.
  *	@shost:	scsi host to change the state of.
Index: 2.6-git/drivers/scsi/scsi_sysfs.c
===================================================================
--- 2.6-git.orig/drivers/scsi/scsi_sysfs.c
+++ 2.6-git/drivers/scsi/scsi_sysfs.c
@@ -189,6 +189,74 @@ show_shost_state(struct class_device *cl
 
 static CLASS_DEVICE_ATTR(state, S_IRUGO | S_IWUSR, show_shost_state, store_shost_state);
 
+static const struct {
+	enum scsi_host_link_pm	value;
+	char			*name;
+} shost_link_pm_policy[] = {
+	{ SHOST_NOT_AVAILABLE, "max_performance" },
+	{ SHOST_MIN_POWER, "min_power" },
+	{ SHOST_MAX_PERFORMANCE, "max_performance" },
+	{ SHOST_MEDIUM_POWER, "medium_power" },
+};
+
+const char *scsi_host_link_pm_policy(enum scsi_host_link_pm policy)
+{
+	int i;
+	char *name = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(shost_link_pm_policy); i++) {
+		if (shost_link_pm_policy[i].value == policy) {
+			name = shost_link_pm_policy[i].name;
+			break;
+		}
+	}
+	return name;
+}
+
+static ssize_t store_link_pm_policy(struct class_device *class_dev,
+	const char *buf, size_t count)
+{
+	struct Scsi_Host *shost = class_to_shost(class_dev);
+	enum scsi_host_link_pm policy = 0;
+	int i;
+
+	/*
+ 	 * we are skipping array location 0 on purpose - this
+ 	 * is because a value of SHOST_NOT_AVAILABLE is displayed
+ 	 * to the user as max_performance, but when the user
+ 	 * writes "max_performance", they actually want the
+ 	 * value to match SHOST_MAX_PERFORMANCE.
+ 	 */
+	for (i = 1; i < ARRAY_SIZE(shost_link_pm_policy); i++) {
+		const int len = strlen(shost_link_pm_policy[i].name);
+		if (strncmp(shost_link_pm_policy[i].name, buf, len) == 0 &&
+		   buf[len] == '\n') {
+			policy = shost_link_pm_policy[i].value;
+			break;
+		}
+	}
+	if (!policy)
+		return -EINVAL;
+
+	if (scsi_host_set_link_pm(shost, policy))
+		return -EINVAL;
+	return count;
+}
+static ssize_t
+show_link_pm_policy(struct class_device *class_dev, char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(class_dev);
+	const char *policy =
+		scsi_host_link_pm_policy(shost->shost_link_pm_policy);
+
+	if (!policy)
+		return -EINVAL;
+
+	return snprintf(buf, 23, "%s\n", policy);
+}
+static CLASS_DEVICE_ATTR(link_power_management_policy, S_IRUGO | S_IWUSR,
+		show_link_pm_policy, store_link_pm_policy);
+
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(host_busy, "%hu\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
@@ -207,6 +275,7 @@ static struct class_device_attribute *sc
 	&class_device_attr_proc_name,
 	&class_device_attr_scan,
 	&class_device_attr_state,
+	&class_device_attr_link_power_management_policy,
 	NULL
 };
 
Index: 2.6-git/include/scsi/scsi_host.h
===================================================================
--- 2.6-git.orig/include/scsi/scsi_host.h
+++ 2.6-git/include/scsi/scsi_host.h
@@ -42,6 +42,16 @@ enum scsi_eh_timer_return {
 	EH_RESET_TIMER,
 };
 
+/*
+ * shost pm policy: If you alter this, you also need to alter scsi_sysfs.c
+ * (for the ascii descriptions)
+ */
+enum scsi_host_link_pm {
+	SHOST_NOT_AVAILABLE,
+	SHOST_MIN_POWER,
+	SHOST_MAX_PERFORMANCE,
+	SHOST_MEDIUM_POWER,
+};
 
 struct scsi_host_template {
 	struct module *module;
@@ -345,6 +355,12 @@ struct scsi_host_template {
 	int (*suspend)(struct scsi_device *, pm_message_t state);
 
 	/*
+ 	 * link power management support
+ 	 */
+	int (*set_link_pm_policy)(struct Scsi_Host *, enum scsi_host_link_pm);
+	enum scsi_host_link_pm default_link_pm_policy;
+
+	/*
 	 * Name of proc directory
 	 */
 	char *proc_name;
@@ -642,6 +658,7 @@ struct Scsi_Host {
 	
 
 	enum scsi_host_state shost_state;
+	enum scsi_host_link_pm shost_link_pm_policy;
 
 	/* ldm bits */
 	struct device		shost_gendev;
@@ -749,4 +766,5 @@ extern struct Scsi_Host *scsi_register(s
 extern void scsi_unregister(struct Scsi_Host *);
 extern int scsi_host_set_state(struct Scsi_Host *, enum scsi_host_state);
 
+extern int scsi_host_set_link_pm(struct Scsi_Host *, enum scsi_host_link_pm);
 #endif /* _SCSI_SCSI_HOST_H */
Index: 2.6-git/Documentation/scsi/link_power_management_policy.txt
===================================================================
--- /dev/null
+++ 2.6-git/Documentation/scsi/link_power_management_policy.txt
@@ -0,0 +1,19 @@
+This parameter allows the user to set the link (interface) power management.
+There are 3 possible options:
+
+Value			Effect
+----------------------------------------------------------------------------
+min_power		Tell the controller to try to make the link use the
+			least possible power when possible.  This may
+			sacrifice some performance due to increased latency
+			when coming out of lower power states.
+
+max_performance		Generally, this means no power management.  Tell
+			the controller to have performance be a priority
+			over power management.
+
+medium_power		Tell the controller to enter a lower power state
+			when possible, but do not enter the lowest power
+			state, thus improving latency over min_power setting.
+
+

Re: [patch 2a/3] Expose Power Management Policy option to users

[James Bottomley]

On Tue, 2007-06-12 at 10:46 -0700, Kristen Carlson Accardi wrote:
> Expose Power Management Policy option to users
> 
> This patch will modify the scsi subsystem to allow
> users to set a power management policy for the link.
> 
> The scsi subsystem will create a new sysfs file for each
> host in /sys/class/scsi_host called "link_power_management_policy".
> This file can have 3 possible values:

I'm afraid the host isn't really the right place to put the link power
management policy (assuming you want to manage the individual links
separately) because there isn't a one to one correspondence between
links and hosts.

To take the model I understand: SAS; the links are managed at the phy
level, so the power policy should be set there and thus should probably
be a property of the phy object, which doesn't even exist in the SCSI
model, it only exists in the transport class.  It strikes me that even
for ATA, the same thing is probably true.

Now, I can see that the power management models of all the transports
might share some similarities (particularly at this three stage granular
level); if so, it might make sense to export helpers from the mid-layer
for the transport classes to use for this.

> Value		Meaning
> -------------------------------------------------------------------
> min_power	User wishes the link to conserve power as much as
> 		possible, even at the cost of some performance
> 
> max_performance User wants priority to be on performance, not power
> 		savings
> 
> medium_power	User wants power savings, with less performance cost
> 		than min_power (but less power savings as well).

These seem like nicely sane and generic values.

James

Re: [patch 2a/3] Expose Power Management Policy option to users

[Jeff Garzik]

James Bottomley wrote:
> To take the model I understand: SAS; the links are managed at the phy
> level, so the power policy should be set there and thus should probably
> be a property of the phy object, which doesn't even exist in the SCSI
> model, it only exists in the transport class.  It strikes me that even
> for ATA, the same thing is probably true.
> 
> Now, I can see that the power management models of all the transports
> might share some similarities (particularly at this three stage granular
> level); if so, it might make sense to export helpers from the mid-layer
> for the transport classes to use for this.


Agreed, in principle.

	Jeff

Re: [patch 2a/3] Expose Power Management Policy option to users

[Kristen Carlson Accardi]

On Wed, 13 Jun 2007 08:26:48 -0700
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> On Tue, 2007-06-12 at 10:46 -0700, Kristen Carlson Accardi wrote:
> > Expose Power Management Policy option to users
> > 
> > This patch will modify the scsi subsystem to allow
> > users to set a power management policy for the link.
> > 
> > The scsi subsystem will create a new sysfs file for each
> > host in /sys/class/scsi_host called "link_power_management_policy".
> > This file can have 3 possible values:
> 
> I'm afraid the host isn't really the right place to put the link power
> management policy (assuming you want to manage the individual links
> separately) because there isn't a one to one correspondence between
> links and hosts.
> 
> To take the model I understand: SAS; the links are managed at the phy
> level, so the power policy should be set there and thus should probably
> be a property of the phy object, which doesn't even exist in the SCSI
> model, it only exists in the transport class.  It strikes me that even
> for ATA, the same thing is probably true.
> 
> Now, I can see that the power management models of all the transports
> might share some similarities (particularly at this three stage granular
> level); if so, it might make sense to export helpers from the mid-layer
> for the transport classes to use for this.

Ok - sorry for my ignorance about SCSI - but my sources (i.e. Arjan) tell 
me that the problem is that Link in ATA land means something different than 
Link in SCSI land, and that what I really need to do is leave this code under
the Host class, but rename it to something that more accurately reflects
what it means under SCSI.  So, is the word "segment" more appropriate?
Should I rename the file to "segment_power_management_policy"?

Thanks,
kristen


> 
> > Value		Meaning
> > -------------------------------------------------------------------
> > min_power	User wishes the link to conserve power as much as
> > 		possible, even at the cost of some performance
> > 
> > max_performance User wants priority to be on performance, not power
> > 		savings
> > 
> > medium_power	User wants power savings, with less performance cost
> > 		than min_power (but less power savings as well).
> 
> These seem like nicely sane and generic values.
> 
> James
> 

Re: [patch 2a/3] Expose Power Management Policy option to users

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> Ok - sorry for my ignorance about SCSI - but my sources (i.e. Arjan) tell 
> me that the problem is that Link in ATA land means something different than 
> Link in SCSI land, and that what I really need to do is leave this code under
> the Host class, but rename it to something that more accurately reflects
> what it means under SCSI.

James' analogy holds, and is even more true once SATA Port Multipliers 
are in the picture.  Then you have remote SATA phys.  And James has 
essentially stated a long term libata problem:  libata wants its own ATA 
transport class, and perhaps a cleaning-up and coalescing of the 
in-kernel SATA phy objects and processes.

The main difference is that SATA doesn't have to worry about target phys 
and initiator phys, largely just the initiator phy.  And phys in SATA 
don't have unique identifiers (WWNs).

I don't think we should delay ALPM in order to complete phy objects and 
an ATA transport class.  But OTOH a transport class may be the best 
place to put these new sysfs nodes.  But#2, that train of logic leads 
one down the road of implementing a minimal ATA transport class across 
all supported ATA devices, which is something that probably only James 
is an expert at (transport classes that is, not ATA).


> Should I rename the file to "segment_power_management_policy"?

No.

	Jeff

Re: [patch 2b/3] Expose Power Management Policy option to users

[Kristen Carlson Accardi]

Enable link power management for ata drivers

libata drivers can define a function (enable_pm) that will
perform hardware specific actions to enable whatever power
management policy the user set up from the scsi sysfs 
interface if the driver supports it.  This power management 
policy will be activated after all disks have been 
enumerated and intialized.

Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
Index: 2.6-git/drivers/ata/libata-scsi.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-scsi.c
+++ 2.6-git/drivers/ata/libata-scsi.c
@@ -2890,6 +2890,51 @@ void ata_scsi_simulate(struct ata_device
 	}
 }
 
+int ata_scsi_set_link_pm_policy(struct Scsi_Host *shost,
+		enum scsi_host_link_pm policy)
+{
+	struct ata_port *ap = ata_shost_to_port(shost);
+	int rc = -EINVAL;
+	int i;
+
+	/*
+ 	 * make sure no broken devices are on this port,
+ 	 * and that all devices support interface power
+ 	 * management
+ 	 */
+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
+		struct ata_device *dev = &ap->device[i];
+
+		/* only check drives which exist */
+		if (!ata_dev_enabled(dev))
+			continue;
+
+		/*
+ 		 * do we need to handle the case where we've hotplugged
+ 		 * a broken drive (since hotplug and ALPM are mutually
+ 		 * exclusive) ?
+ 		 *
+ 		 * If so, if we detect a broken drive on a port with
+ 		 * alpm already enabled, then we should reset the policy
+ 		 * to off for the entire port.
+ 		 */
+		if ((dev->horkage & ATA_HORKAGE_ALPM) ||
+			!(dev->flags & ATA_DFLAG_IPM)) {
+			ata_dev_printk(dev, KERN_ERR,
+				"Unable to set Link PM policy\n");
+			ap->pm_policy = SHOST_MAX_PERFORMANCE;
+		}
+	}
+
+	if (ap->ops->enable_pm)
+		rc = ap->ops->enable_pm(ap, policy);
+
+	if (!rc)
+		shost->shost_link_pm_policy = ap->pm_policy;
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ata_scsi_set_link_pm_policy);
+
 int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 {
 	int i, rc;
@@ -2912,7 +2957,7 @@ int ata_scsi_add_hosts(struct ata_host *
 		shost->max_lun = 1;
 		shost->max_channel = 1;
 		shost->max_cmd_len = 16;
-
+		shost->shost_link_pm_policy = ap->pm_policy;
 		rc = scsi_add_host(ap->scsi_host, ap->host->dev);
 		if (rc)
 			goto err_add;
Index: 2.6-git/include/linux/libata.h
===================================================================
--- 2.6-git.orig/include/linux/libata.h
+++ 2.6-git/include/linux/libata.h
@@ -136,6 +136,7 @@ enum {
 	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
 	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
 	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
+	ATA_DFLAG_IPM		= (1 << 6), /* device supports interface PM */
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
@@ -300,6 +301,7 @@ enum {
 	ATA_HORKAGE_NONCQ	= (1 << 2),	/* Don't use NCQ */
 	ATA_HORKAGE_MAX_SEC_128	= (1 << 3),	/* Limit max sects to 128 */
 	ATA_HORKAGE_DMA_RW_ONLY	= (1 << 4),	/* ATAPI DMA for RW only */
+	ATA_HORKAGE_ALPM	= (1 << 5), 	/* ALPM problems */
 };
 
 enum hsm_task_states {
@@ -548,6 +550,7 @@ struct ata_port {
 
 	pm_message_t		pm_mesg;
 	int			*pm_result;
+	enum scsi_host_link_pm	pm_policy;
 
 	void			*private_data;
 
@@ -607,7 +610,7 @@ struct ata_port_operations {
 
 	int (*port_suspend) (struct ata_port *ap, pm_message_t mesg);
 	int (*port_resume) (struct ata_port *ap);
-
+	int (*enable_pm) (struct ata_port *ap, enum scsi_host_link_pm policy);
 	int (*port_start) (struct ata_port *ap);
 	void (*port_stop) (struct ata_port *ap);
 
@@ -812,7 +815,7 @@ extern int ata_cable_40wire(struct ata_p
 extern int ata_cable_80wire(struct ata_port *ap);
 extern int ata_cable_sata(struct ata_port *ap);
 extern int ata_cable_unknown(struct ata_port *ap);
-
+extern int ata_scsi_set_link_pm_policy(struct Scsi_Host *shost, enum scsi_host_link_pm);
 /*
  * Timing helpers
  */
Index: 2.6-git/drivers/ata/libata-core.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-core.c
+++ 2.6-git/drivers/ata/libata-core.c
@@ -2019,6 +2019,9 @@ int ata_dev_configure(struct ata_device 
 	if (dev->flags & ATA_DFLAG_LBA48)
 		dev->max_sectors = ATA_MAX_SECTORS_LBA48;
 
+	if (ata_id_has_hipm(dev->id))
+		dev->flags |= ATA_DFLAG_IPM;
+
 	if (dev->horkage & ATA_HORKAGE_DIAGNOSTIC) {
 		/* Let the user know. We don't want to disallow opens for
 		   rescue purposes, or in case the vendor is just a blithering
@@ -2048,6 +2051,13 @@ int ata_dev_configure(struct ata_device 
 	if (ata_device_blacklisted(dev) & ATA_HORKAGE_DMA_RW_ONLY)
 		dev->horkage |= ATA_HORKAGE_DMA_RW_ONLY;
 
+	if (ata_device_blacklisted(dev) & ATA_HORKAGE_ALPM) {
+		dev->horkage |= ATA_HORKAGE_ALPM;
+
+		/* reset link pm_policy for this port to no pm */
+		ap->pm_policy = SHOST_MAX_PERFORMANCE;
+	}
+
 	if (ap->ops->dev_config)
 		ap->ops->dev_config(dev);
 
@@ -6394,6 +6404,8 @@ int ata_host_register(struct ata_host *h
 		struct ata_port *ap = host->ports[i];
 
 		ata_scsi_scan_host(ap);
+		ata_scsi_set_link_pm_policy(ap->scsi_host,
+				ap->pm_policy);
 	}
 
 	return 0;
Index: 2.6-git/include/linux/ata.h
===================================================================
--- 2.6-git.orig/include/linux/ata.h
+++ 2.6-git/include/linux/ata.h
@@ -320,6 +320,8 @@ struct ata_taskfile {
 	  ((u64) (id)[(n) + 0]) )
 
 #define ata_id_cdb_intr(id)	(((id)[0] & 0x60) == 0x20)
+#define ata_id_has_hipm(id)	((id)[76] & (1 << 9))
+#define ata_id_has_dipm(id)	((id)[78] & (1 << 3))
 
 static inline unsigned int ata_id_major_version(const u16 *id)
 {

Re: [patch 2b/3] Expose Power Management Policy option to users

[Kristen Carlson Accardi]

Enable link power management for ata drivers

libata drivers can define a function (enable_pm) that will
perform hardware specific actions to enable whatever power
management policy the user set up from the scsi sysfs 
interface if the driver supports it.  This power management 
policy will be activated after all disks have been 
enumerated and initialized.  Drivers should also define
disable_pm, which will turn off link power management, but
not change link power management policy.

Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

---
I've updated this patch to fix a problem with suspend breaking when
alpm is enabled.  To fix this, I've changed the patch to allow drivers
to have a disable_pm function which can be used by the host controller
to turn off link power managment before requesting an PM activity.  
We will turn it back on after Resume.


This whole series can be found at:
http://www.kernel.org/pub/linux/kernel/people/kristen/patches/SATA/alpm/

Index: 2.6-git/drivers/ata/libata-scsi.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-scsi.c
+++ 2.6-git/drivers/ata/libata-scsi.c
@@ -2909,6 +2909,51 @@ void ata_scsi_simulate(struct ata_device
 	}
 }
 
+int ata_scsi_set_link_pm_policy(struct Scsi_Host *shost,
+		enum scsi_host_link_pm policy)
+{
+	struct ata_port *ap = ata_shost_to_port(shost);
+	int rc = -EINVAL;
+	int i;
+
+	/*
+ 	 * make sure no broken devices are on this port,
+ 	 * and that all devices support interface power
+ 	 * management
+ 	 */
+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
+		struct ata_device *dev = &ap->device[i];
+
+		/* only check drives which exist */
+		if (!ata_dev_enabled(dev))
+			continue;
+
+		/*
+ 		 * do we need to handle the case where we've hotplugged
+ 		 * a broken drive (since hotplug and ALPM are mutually
+ 		 * exclusive) ?
+ 		 *
+ 		 * If so, if we detect a broken drive on a port with
+ 		 * alpm already enabled, then we should reset the policy
+ 		 * to off for the entire port.
+ 		 */
+		if ((dev->horkage & ATA_HORKAGE_ALPM) ||
+			!(dev->flags & ATA_DFLAG_IPM)) {
+			ata_dev_printk(dev, KERN_ERR,
+				"Unable to set Link PM policy\n");
+			ap->pm_policy = SHOST_MAX_PERFORMANCE;
+		}
+	}
+
+	if (ap->ops->enable_pm)
+		rc = ap->ops->enable_pm(ap, policy);
+
+	if (!rc)
+		shost->shost_link_pm_policy = ap->pm_policy;
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ata_scsi_set_link_pm_policy);
+
 int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 {
 	int i, rc;
@@ -2931,7 +2976,7 @@ int ata_scsi_add_hosts(struct ata_host *
 		shost->max_lun = 1;
 		shost->max_channel = 1;
 		shost->max_cmd_len = 16;
-
+		shost->shost_link_pm_policy = ap->pm_policy;
 		rc = scsi_add_host(ap->scsi_host, ap->host->dev);
 		if (rc)
 			goto err_add;
Index: 2.6-git/include/linux/libata.h
===================================================================
--- 2.6-git.orig/include/linux/libata.h
+++ 2.6-git/include/linux/libata.h
@@ -136,6 +136,7 @@ enum {
 	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
 	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
 	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
+	ATA_DFLAG_IPM		= (1 << 6), /* device supports interface PM */
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
@@ -299,6 +300,7 @@ enum {
 	ATA_HORKAGE_NONCQ	= (1 << 2),	/* Don't use NCQ */
 	ATA_HORKAGE_MAX_SEC_128	= (1 << 3),	/* Limit max sects to 128 */
 	ATA_HORKAGE_DMA_RW_ONLY	= (1 << 4),	/* ATAPI DMA for RW only */
+	ATA_HORKAGE_ALPM	= (1 << 5), 	/* ALPM problems */
 };
 
 enum hsm_task_states {
@@ -547,6 +549,7 @@ struct ata_port {
 
 	pm_message_t		pm_mesg;
 	int			*pm_result;
+	enum scsi_host_link_pm	pm_policy;
 
 	void			*private_data;
 
@@ -606,7 +609,8 @@ struct ata_port_operations {
 
 	int (*port_suspend) (struct ata_port *ap, pm_message_t mesg);
 	int (*port_resume) (struct ata_port *ap);
-
+	int (*enable_pm) (struct ata_port *ap, enum scsi_host_link_pm policy);
+	int (*disable_pm) (struct ata_port *ap);
 	int (*port_start) (struct ata_port *ap);
 	void (*port_stop) (struct ata_port *ap);
 
@@ -812,7 +816,7 @@ extern int ata_cable_40wire(struct ata_p
 extern int ata_cable_80wire(struct ata_port *ap);
 extern int ata_cable_sata(struct ata_port *ap);
 extern int ata_cable_unknown(struct ata_port *ap);
-
+extern int ata_scsi_set_link_pm_policy(struct Scsi_Host *shost, enum scsi_host_link_pm);
 /*
  * Timing helpers
  */
Index: 2.6-git/drivers/ata/libata-core.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-core.c
+++ 2.6-git/drivers/ata/libata-core.c
@@ -2021,6 +2021,9 @@ int ata_dev_configure(struct ata_device 
 	if (dev->flags & ATA_DFLAG_LBA48)
 		dev->max_sectors = ATA_MAX_SECTORS_LBA48;
 
+	if (ata_id_has_hipm(dev->id))
+		dev->flags |= ATA_DFLAG_IPM;
+
 	if (dev->horkage & ATA_HORKAGE_DIAGNOSTIC) {
 		/* Let the user know. We don't want to disallow opens for
 		   rescue purposes, or in case the vendor is just a blithering
@@ -2050,6 +2053,13 @@ int ata_dev_configure(struct ata_device 
 	if (ata_device_blacklisted(dev) & ATA_HORKAGE_DMA_RW_ONLY)
 		dev->horkage |= ATA_HORKAGE_DMA_RW_ONLY;
 
+	if (ata_device_blacklisted(dev) & ATA_HORKAGE_ALPM) {
+		dev->horkage |= ATA_HORKAGE_ALPM;
+
+		/* reset link pm_policy for this port to no pm */
+		ap->pm_policy = SHOST_MAX_PERFORMANCE;
+	}
+
 	if (ap->ops->dev_config)
 		ap->ops->dev_config(dev);
 
@@ -5823,6 +5833,28 @@ int ata_flush_cache(struct ata_device *d
 	return 0;
 }
 
+static void ata_host_disable_link_pm(struct ata_host *host)
+{
+	int i;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+		if (ap->ops->disable_pm)
+			ap->ops->disable_pm(ap);
+	}
+}
+
+static void ata_host_enable_link_pm(struct ata_host *host)
+{
+	int i;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+		ata_scsi_set_link_pm_policy(ap->scsi_host,
+				ap->pm_policy);
+	}
+}
+
 #ifdef CONFIG_PM
 static int ata_host_request_pm(struct ata_host *host, pm_message_t mesg,
 			       unsigned int action, unsigned int ehi_flags,
@@ -5890,6 +5922,12 @@ int ata_host_suspend(struct ata_host *ho
 {
 	int rc;
 
+	/*
+ 	 * disable link pm on all ports before requesting
+ 	 * any pm activity
+ 	 */
+	ata_host_disable_link_pm(host);
+
 	rc = ata_host_request_pm(host, mesg, 0, ATA_EHI_QUIET, 1);
 	if (rc == 0)
 		host->dev->power.power_state = mesg;
@@ -5912,6 +5950,9 @@ void ata_host_resume(struct ata_host *ho
 	ata_host_request_pm(host, PMSG_ON, ATA_EH_SOFTRESET,
 			    ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, 0);
 	host->dev->power.power_state = PMSG_ON;
+
+	/* reenable link pm */
+	ata_host_enable_link_pm(host);
 }
 #endif
 
@@ -6401,6 +6442,8 @@ int ata_host_register(struct ata_host *h
 		struct ata_port *ap = host->ports[i];
 
 		ata_scsi_scan_host(ap);
+		ata_scsi_set_link_pm_policy(ap->scsi_host,
+				ap->pm_policy);
 	}
 
 	return 0;
Index: 2.6-git/include/linux/ata.h
===================================================================
--- 2.6-git.orig/include/linux/ata.h
+++ 2.6-git/include/linux/ata.h
@@ -321,6 +321,8 @@ struct ata_taskfile {
 	  ((u64) (id)[(n) + 0]) )
 
 #define ata_id_cdb_intr(id)	(((id)[0] & 0x60) == 0x20)
+#define ata_id_has_hipm(id)	((id)[76] & (1 << 9))
+#define ata_id_has_dipm(id)	((id)[78] & (1 << 3))
 
 static inline unsigned int ata_id_major_version(const u16 *id)
 {

[patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Kristen Carlson Accardi]

This patch will set the correct bits to turn on Aggressive
Link Power Management (ALPM) for the ahci driver.  This
will cause the controller and disk to negotiate a lower
power state for the link when there is no activity (see
the AHCI 1.x spec for details).  This feature is mutually
exclusive with Hot Plug, so when ALPM is enabled, Hot Plug
is disabled.  ALPM will be enabled by default, but it is
settable via the scsi host syfs interface.  Possible 
settings for this feature are:

Setting		Effect
----------------------------------------------------------
min_power	ALPM is enabled, and link set to enter 
		lowest power state (SLUMBER) when idle
		Hot plug not allowed.

max_performance	ALPM is disabled, Hot Plug is allowed

medium_power	ALPM is enabled, and link set to enter
		second lowest power state (PARTIAL) when
		idle.  Hot plug not allowed.

Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/drivers/ata/ahci.c
===================================================================
--- 2.6-git.orig/drivers/ata/ahci.c
+++ 2.6-git/drivers/ata/ahci.c
@@ -48,6 +48,9 @@
 #define DRV_NAME	"ahci"
 #define DRV_VERSION	"2.2"
 
+static int ahci_enable_alpm(struct ata_port *ap,
+		enum scsi_host_link_pm policy);
+static int ahci_disable_alpm(struct ata_port *ap);
 
 enum {
 	AHCI_PCI_BAR		= 5,
@@ -97,6 +100,7 @@ enum {
 	/* HOST_CAP bits */
 	HOST_CAP_SSC		= (1 << 14), /* Slumber capable */
 	HOST_CAP_CLO		= (1 << 24), /* Command List Override support */
+	HOST_CAP_ALPM		= (1 << 26), /* Aggressive Link PM support */
 	HOST_CAP_SSS		= (1 << 27), /* Staggered Spin-up */
 	HOST_CAP_NCQ		= (1 << 30), /* Native Command Queueing */
 	HOST_CAP_64		= (1 << 31), /* PCI DAC (64-bit DMA) support */
@@ -151,6 +155,8 @@ enum {
 				  PORT_IRQ_PIOS_FIS | PORT_IRQ_D2H_REG_FIS,
 
 	/* PORT_CMD bits */
+	PORT_CMD_ASP		= (1 << 27), /* Aggressive Slumber/Partial */
+	PORT_CMD_ALPE		= (1 << 26), /* Aggressive Link PM enable */
 	PORT_CMD_ATAPI		= (1 << 24), /* Device is ATAPI */
 	PORT_CMD_LIST_ON	= (1 << 15), /* cmd list DMA engine running */
 	PORT_CMD_FIS_ON		= (1 << 14), /* FIS DMA engine running */
@@ -171,6 +177,7 @@ enum {
 	AHCI_FLAG_HONOR_PI		= (1 << 26), /* honor PORTS_IMPL */
 	AHCI_FLAG_IGN_SERR_INTERNAL	= (1 << 27), /* ignore SERR_INTERNAL */
 	AHCI_FLAG_32BIT_ONLY		= (1 << 28), /* force 32bit */
+	AHCI_FLAG_NO_HOTPLUG		= (1 << 29), /* ignore PxSERR.DIAG.N */
 
 	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
@@ -253,6 +260,7 @@ static struct scsi_host_template ahci_sh
 	.slave_configure	= ata_scsi_slave_config,
 	.slave_destroy		= ata_scsi_slave_destroy,
 	.bios_param		= ata_std_bios_param,
+	.set_link_pm_policy	= ata_scsi_set_link_pm_policy,
 };
 
 static const struct ata_port_operations ahci_ops = {
@@ -284,6 +292,7 @@ static const struct ata_port_operations 
 	.port_suspend		= ahci_port_suspend,
 	.port_resume		= ahci_port_resume,
 #endif
+	.enable_pm		= ahci_enable_alpm,
 
 	.port_start		= ahci_port_start,
 	.port_stop		= ahci_port_stop,
@@ -695,6 +704,152 @@ static void ahci_power_up(struct ata_por
 	writel(cmd | PORT_CMD_ICC_ACTIVE, port_mmio + PORT_CMD);
 }
 
+static int ahci_disable_alpm(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u32 cmd, scontrol;
+	struct ahci_port_priv *pp = ap->private_data;
+
+	/*
+ 	 * disable Interface Power Management State Transitions
+ 	 * This is accomplished by setting bits 8:11 of the
+ 	 * SATA Control register
+ 	 */
+	scontrol = readl(port_mmio + PORT_SCR_CTL);
+	scontrol |= (0x3 << 8);
+	writel(scontrol, port_mmio + PORT_SCR_CTL);
+
+	/* get the existing command bits */
+	cmd = readl(port_mmio + PORT_CMD);
+
+	/* disable ALPM and ASP */
+	cmd &= ~PORT_CMD_ASP;
+	cmd &= ~PORT_CMD_ALPE;
+
+	/* force the interface back to active */
+	cmd |= PORT_CMD_ICC_ACTIVE;
+
+	/* write out new cmd value */
+	writel(cmd, port_mmio + PORT_CMD);
+	cmd = readl(port_mmio + PORT_CMD);
+
+	/* wait 10ms to be sure we've come out of any low power state */
+	msleep(10);
+
+	/* clear out any PhyRdy stuff from interrupt status */
+	writel(PORT_IRQ_PHYRDY, port_mmio + PORT_IRQ_STAT);
+
+	/* go ahead and clean out PhyRdy Change from Serror too */
+	ahci_scr_write(ap, SCR_ERROR, (1 << 16));
+
+	/*
+ 	 * Clear flag to indicate that we should ignore all PhyRdy
+ 	 * state changes
+ 	 */
+	ap->flags &= ~AHCI_FLAG_NO_HOTPLUG;
+
+	/*
+ 	 * Enable interrupts on Phy Ready.
+ 	 */
+	pp->intr_mask |= PORT_IRQ_PHYRDY;
+	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+	ap->pm_policy = SHOST_MAX_PERFORMANCE;
+	return 0;
+}
+
+static int ahci_enable_alpm(struct ata_port *ap,
+	enum scsi_host_link_pm policy)
+{
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u32 cmd, scontrol, sstatus;
+	struct ahci_port_priv *pp = ap->private_data;
+	u32 asp;
+
+	/* Make sure the host is capable of link power management */
+	if (!(hpriv->cap & HOST_CAP_ALPM)) {
+		ap->pm_policy = SHOST_NOT_AVAILABLE;
+		return -EINVAL;
+	}
+
+	/* make sure we have a device attached */
+	sstatus = readl(port_mmio + PORT_SCR_STAT);
+	if (!(sstatus & 0xf00)) {
+		ap->pm_policy = SHOST_NOT_AVAILABLE;
+		return -EINVAL;
+	}
+
+	switch(policy) {
+	case SHOST_MAX_PERFORMANCE:
+		return ahci_disable_alpm(ap);
+	case SHOST_NOT_AVAILABLE:
+	case SHOST_MIN_POWER:
+		/*
+ 		 * if we came here with SHOST_NOT_AVAILABLE,
+ 		 * it just means this is the first time we
+ 		 * have tried to enable - so try to do
+ 		 * min_power
+ 		 */
+		ap->pm_policy = SHOST_MIN_POWER;
+
+		/* configure HBA to enter SLUMBER */
+		asp = PORT_CMD_ASP;
+		break;
+	case SHOST_MEDIUM_POWER:
+		/* configure HBA to enter PARTIAL */
+		ap->pm_policy = policy;
+		asp = 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*
+ 	 * Disable interrupts on Phy Ready. This keeps us from
+ 	 * getting woken up due to spurious phy ready interrupts
+	 * TBD - Hot plug should be done via polling now, is
+	 * that even supported?
+ 	 */
+	pp->intr_mask &= ~PORT_IRQ_PHYRDY;
+	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	/*
+ 	 * Set a flag to indicate that we should ignore all PhyRdy
+ 	 * state changes since these can happen now whenever we
+ 	 * change link state
+ 	 */
+	ap->flags |= AHCI_FLAG_NO_HOTPLUG;
+
+	/* get the existing command bits */
+	cmd = readl(port_mmio + PORT_CMD);
+
+	/*
+ 	 * enable Interface Power Management State Transitions
+ 	 * This is accomplished by clearing bits 8:11 of the
+ 	 * SATA Control register
+ 	 */
+	scontrol = readl(port_mmio + PORT_SCR_CTL);
+	scontrol &= ~(0x3 << 8);
+	writel(scontrol, port_mmio + PORT_SCR_CTL);
+
+	/*
+ 	 * Set ASP based on Policy
+ 	 */
+	cmd |= asp;
+
+	/*
+ 	 * Setting this bit will instruct the HBA to aggressively
+ 	 * enter a lower power link state when it's appropriate and
+ 	 * based on the value set above for ASP
+ 	 */
+	cmd |= PORT_CMD_ALPE;
+
+	/* write out new cmd value */
+	writel(cmd, port_mmio + PORT_CMD);
+	cmd = readl(port_mmio + PORT_CMD);
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static void ahci_power_down(struct ata_port *ap)
 {
@@ -1220,6 +1375,10 @@ static void ahci_host_intr(struct ata_po
 	status = readl(port_mmio + PORT_IRQ_STAT);
 	writel(status, port_mmio + PORT_IRQ_STAT);
 
+	if ((ap->flags & AHCI_FLAG_NO_HOTPLUG) &&
+		(status & PORT_IRQ_PHYRDY))
+		status &= ~PORT_IRQ_PHYRDY;
+
 	if (unlikely(status & PORT_IRQ_ERROR)) {
 		ahci_error_intr(ap, status);
 		return;
@@ -1735,6 +1894,7 @@ static int ahci_init_one(struct pci_dev 
 
 			ap->ioaddr.cmd_addr = port_mmio;
 			ap->ioaddr.scr_addr = port_mmio + PORT_SCR;
+			ap->pm_policy = SHOST_NOT_AVAILABLE;
 		} else
 			host->ports[i]->ops = &ata_dummy_port_ops;
 	}

-- 

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> This patch will set the correct bits to turn on Aggressive
> Link Power Management (ALPM) for the ahci driver.  This
> will cause the controller and disk to negotiate a lower
> power state for the link when there is no activity (see
> the AHCI 1.x spec for details).  This feature is mutually
> exclusive with Hot Plug, so when ALPM is enabled, Hot Plug
> is disabled.  ALPM will be enabled by default, but it is
> settable via the scsi host syfs interface.  Possible 
> settings for this feature are:
> 
> Setting		Effect
> ----------------------------------------------------------
> min_power	ALPM is enabled, and link set to enter 
> 		lowest power state (SLUMBER) when idle
> 		Hot plug not allowed.
> 
> max_performance	ALPM is disabled, Hot Plug is allowed
> 
> medium_power	ALPM is enabled, and link set to enter
> 		second lowest power state (PARTIAL) when
> 		idle.  Hot plug not allowed.
> 
> Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

seems OK at first glance, though I'll have questions about hardware 
behavior once I get off this day-long Intel conference call :)

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Henrique de Moraes Holschuh]

On Mon, 11 Jun 2007, Kristen Carlson Accardi wrote:
> Setting		Effect
> ----------------------------------------------------------
> min_power	ALPM is enabled, and link set to enter 
> 		lowest power state (SLUMBER) when idle
> 		Hot plug not allowed.
> 
> max_performance	ALPM is disabled, Hot Plug is allowed
> 
> medium_power	ALPM is enabled, and link set to enter
> 		second lowest power state (PARTIAL) when
> 		idle.  Hot plug not allowed.

Just some food for thought:

If you split it into a enable/disable (0/1) attribute, and a level attribute
(some sort of integer scale, or "min", "medium", "max" if you must use
strings.  You could use four levels to mimic the PCI device power state, for
example), it might make its usage more generic, and easier from userspace,
as it decouples the need to turn it on/off from the need to know which level
the user wants it set to when you turn it on.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Arjan van de Ven]

Henrique de Moraes Holschuh wrote:
> On Mon, 11 Jun 2007, Kristen Carlson Accardi wrote:
>> Setting		Effect
>> ----------------------------------------------------------
>> min_power	ALPM is enabled, and link set to enter 
>> 		lowest power state (SLUMBER) when idle
>> 		Hot plug not allowed.
>>
>> max_performance	ALPM is disabled, Hot Plug is allowed
>>
>> medium_power	ALPM is enabled, and link set to enter
>> 		second lowest power state (PARTIAL) when
>> 		idle.  Hot plug not allowed.
> 
> Just some food for thought:
> 
> If you split it into a enable/disable (0/1) attribute, and a level attribute

on/off doesn't really make sense if the question is "do you favor 
power or do you favor performance".......

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Dagfinn Ilmari Mannsåker]

Arjan van de Ven <arjan@linux.intel.com> writes:

> Henrique de Moraes Holschuh wrote:
>> On Mon, 11 Jun 2007, Kristen Carlson Accardi wrote:
>>> Setting		Effect
>>> ----------------------------------------------------------
>>> min_power	ALPM is enabled, and link set to enter 		lowest
>>> power state (SLUMBER) when idle
>>> 		Hot plug not allowed.
>>>
>>> max_performance	ALPM is disabled, Hot Plug is allowed
>>>
>>> medium_power	ALPM is enabled, and link set to enter
>>> 		second lowest power state (PARTIAL) when
>>> 		idle.  Hot plug not allowed.
>> Just some food for thought:
>> If you split it into a enable/disable (0/1) attribute, and a level
>> attribute
>
> on/off doesn't really make sense if the question is "do you favor power
> or do you favor performance".......

How about just making it a numeric scale with 0 meaning no power saving
and then some fixed number of levels (e.g 0-9)?

-- 
ilmari
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Jeff Garzik]

Dagfinn Ilmari Mannsåker wrote:
> Arjan van de Ven <arjan@linux.intel.com> writes:
> 
>> Henrique de Moraes Holschuh wrote:
>>> On Mon, 11 Jun 2007, Kristen Carlson Accardi wrote:
>>>> Setting		Effect
>>>> ----------------------------------------------------------
>>>> min_power	ALPM is enabled, and link set to enter 		lowest
>>>> power state (SLUMBER) when idle
>>>> 		Hot plug not allowed.
>>>>
>>>> max_performance	ALPM is disabled, Hot Plug is allowed
>>>>
>>>> medium_power	ALPM is enabled, and link set to enter
>>>> 		second lowest power state (PARTIAL) when
>>>> 		idle.  Hot plug not allowed.
>>> Just some food for thought:
>>> If you split it into a enable/disable (0/1) attribute, and a level
>>> attribute
>> on/off doesn't really make sense if the question is "do you favor power
>> or do you favor performance".......
> 
> How about just making it a numeric scale with 0 meaning no power saving
> and then some fixed number of levels (e.g 0-9)?

The original proposal seems far more intuitive than these alternatives.

	Jeff

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Henrique de Moraes Holschuh]

On Mon, 11 Jun 2007, Jeff Garzik wrote:
> >>on/off doesn't really make sense if the question is "do you favor power
> >>or do you favor performance".......

Actually, it does if you think of it as "do you need hotplug right now or
not?".

> >How about just making it a numeric scale with 0 meaning no power saving
> >and then some fixed number of levels (e.g 0-9)?
> 
> The original proposal seems far more intuitive than these alternatives.

There is nothing intuitive about the text or the levels.  All cases need
proper documentation.  I'd never expect link powersaving to kill hotplug,
unless I read the AHCI docs.

And enable/disable ain't intuitive either :(  But enable/disable is useful
to get stuff like SATA bay hotplug, dock/undock and other stuff that needs
hotplug to be working right, unless we can make that automatic so that power
saving is always disabled in all situations we'd need hotplug to be working?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Arjan van de Ven]

Henrique de Moraes Holschuh wrote:
> On Mon, 11 Jun 2007, Jeff Garzik wrote:
>>>> on/off doesn't really make sense if the question is "do you favor power
>>>> or do you favor performance".......
> 
> Actually, it does if you think of it as "do you need hotplug right now or
> not?".

that's a temporary shortcoming; even with these power savings you can 
do hotplug as long as you're willing to poll for it at a reasonable 
interval and are willing to wait the time between polls for a hotplug 
to take effect..

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Matthew Garrett]

On Mon, Jun 11, 2007 at 08:59:46PM -0700, Arjan van de Ven wrote:

> that's a temporary shortcoming; even with these power savings you can 
> do hotplug as long as you're willing to poll for it at a reasonable 
> interval and are willing to wait the time between polls for a hotplug 
> to take effect..

On laptops, I suspect that we'll probably get an ACPI interrupt even if 
the AHCI hotplug pathway can't manage.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Henrique de Moraes Holschuh]

On Tue, 12 Jun 2007, Matthew Garrett wrote:
> On Mon, Jun 11, 2007 at 08:59:46PM -0700, Arjan van de Ven wrote:
> > that's a temporary shortcoming; even with these power savings you can 
> > do hotplug as long as you're willing to poll for it at a reasonable 
> > interval and are willing to wait the time between polls for a hotplug 
> > to take effect..
> 
> On laptops, I suspect that we'll probably get an ACPI interrupt even if 
> the AHCI hotplug pathway can't manage.

As long as we don't crash the drive or AHCI controller because we hotplugged
it in a way it didn't like (like trying to hotplug a ICH5R would cause).

It is not like 1Hz would not be fast enough for laptop bays and docks, so
the polling frequency would not be a problem.  I still prefer when levels
and on/off are kept separate, but if it won't stand in the way of hotplug, I
certainly don't care enough to bother.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Matthew Garrett]

On Tue, Jun 12, 2007 at 09:18:19AM -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 12 Jun 2007, Matthew Garrett wrote:
> > 
> > On laptops, I suspect that we'll probably get an ACPI interrupt even if 
> > the AHCI hotplug pathway can't manage.
> 
> As long as we don't crash the drive or AHCI controller because we hotplugged
> it in a way it didn't like (like trying to hotplug a ICH5R would cause).

Laptop bays are designed to deal with hotplugging PATA - I don't think 
this is too much of an issue :)

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Henrique de Moraes Holschuh]

On Tue, 12 Jun 2007, Matthew Garrett wrote:
> On Tue, Jun 12, 2007 at 09:18:19AM -0300, Henrique de Moraes Holschuh wrote:
> > On Tue, 12 Jun 2007, Matthew Garrett wrote:
> > > 
> > > On laptops, I suspect that we'll probably get an ACPI interrupt even if 
> > > the AHCI hotplug pathway can't manage.
> > 
> > As long as we don't crash the drive or AHCI controller because we hotplugged
> > it in a way it didn't like (like trying to hotplug a ICH5R would cause).
> 
> Laptop bays are designed to deal with hotplugging PATA - I don't think 
> this is too much of an issue :)

The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
cards use usb2.0 and pci-e hotplug...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Matthew Garrett]

On Tue, Jun 12, 2007 at 11:17:14AM -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 12 Jun 2007, Matthew Garrett wrote:
> > Laptop bays are designed to deal with hotplugging PATA - I don't think 
> > this is too much of an issue :)
> 
> The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
> cards use usb2.0 and pci-e hotplug...

Yes, but they'll also send an ACPI interrupt even if the SATA host 
controller doesn't - it's part of the spec for bays.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Tejun Heo]

Matthew Garrett wrote:
> On Tue, Jun 12, 2007 at 11:17:14AM -0300, Henrique de Moraes Holschuh wrote:
>> On Tue, 12 Jun 2007, Matthew Garrett wrote:
>>> Laptop bays are designed to deal with hotplugging PATA - I don't think 
>>> this is too much of an issue :)
>> The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
>> cards use usb2.0 and pci-e hotplug...
> 
> Yes, but they'll also send an ACPI interrupt even if the SATA host 
> controller doesn't - it's part of the spec for bays.

Does the spec mandate that the ACPI interrupt shouldn't depend on SATA
phy status?  I don't think vendors are likely to implement separate
mechanism when SATA phy status can do the job fine.

-- 
tejun

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Matthew Garrett]

On Wed, Jun 13, 2007 at 12:45:21AM +0900, Tejun Heo wrote:
> Matthew Garrett wrote:
> > Yes, but they'll also send an ACPI interrupt even if the SATA host 
> > controller doesn't - it's part of the spec for bays.
> 
> Does the spec mandate that the ACPI interrupt shouldn't depend on SATA
> phy status?  I don't think vendors are likely to implement separate
> mechanism when SATA phy status can do the job fine.

I suspect that the spec allows them to do that, but think that it's 
unlikely to actually happen in most cases. Bear in mind that Windows 
doesn't tend to drive the hardware in AHCI mode, and that their 
implementation is likely to be very similar to the implementation they 
used for PATA devices.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Jeff Garzik]

Matthew Garrett wrote:
> On Tue, Jun 12, 2007 at 11:17:14AM -0300, Henrique de Moraes Holschuh wrote:
>> On Tue, 12 Jun 2007, Matthew Garrett wrote:
>>> Laptop bays are designed to deal with hotplugging PATA - I don't think 
>>> this is too much of an issue :)
>> The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
>> cards use usb2.0 and pci-e hotplug...
> 
> Yes, but they'll also send an ACPI interrupt even if the SATA host 
> controller doesn't - it's part of the spec for bays.

Regardless, having a laptop does not imply having a docking bay.

	Jeff

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Matthew Garrett]

On Tue, Jun 12, 2007 at 11:46:56AM -0400, Jeff Garzik wrote:
> Matthew Garrett wrote:
> >On Tue, Jun 12, 2007 at 11:17:14AM -0300, Henrique de Moraes Holschuh 
> >wrote:
> >>On Tue, 12 Jun 2007, Matthew Garrett wrote:
> >>>Laptop bays are designed to deal with hotplugging PATA - I don't think 
> >>>this is too much of an issue :)
> >>The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
> >>cards use usb2.0 and pci-e hotplug...
> >
> >Yes, but they'll also send an ACPI interrupt even if the SATA host 
> >controller doesn't - it's part of the spec for bays.
> 
> Regardless, having a laptop does not imply having a docking bay.

Excluding the corner case of an Expresscard SATA controller (where I 
suspect you'd want different policy), I doubt there are any cases where 
you have a laptop with hotplug capabilities without it being implemented 
as an ACPI bay.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Jeff Garzik]

Matthew Garrett wrote:
> Excluding the corner case of an Expresscard SATA controller (where I 
> suspect you'd want different policy), I doubt there are any cases where 
> you have a laptop with hotplug capabilities without it being implemented 
> as an ACPI bay.

Cardbus card.

	Jeff

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Kristen Carlson Accardi]

On Tue, 12 Jun 2007 11:46:56 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Matthew Garrett wrote:
> > On Tue, Jun 12, 2007 at 11:17:14AM -0300, Henrique de Moraes Holschuh wrote:
> >> On Tue, 12 Jun 2007, Matthew Garrett wrote:
> >>> Laptop bays are designed to deal with hotplugging PATA - I don't think 
> >>> this is too much of an issue :)
> >> The new SATA ones use the SATA hardware hotplug ;-)   Just like the pci-e
> >> cards use usb2.0 and pci-e hotplug...
> > 
> > Yes, but they'll also send an ACPI interrupt even if the SATA host 
> > controller doesn't - it's part of the spec for bays.
> 
> Regardless, having a laptop does not imply having a docking bay.
> 
> 	Jeff
> 

For bay devices, we can use ACPI just like we do now.  For non-bay devices,
we can implement hotplug via polling when ALPM is enabled.  In my experience
most laptop vendors implement extra drive as either PATA in a dock station, 
USB in a dock station, or a bay device either on the dock station, or 
on the laptop itself.


Kristen

[patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Kristen Carlson Accardi]

Enable Aggressive Link Power management for AHCI controllers.

This patch will set the correct bits to turn on Aggressive
Link Power Management (ALPM) for the ahci driver.  This
will cause the controller and disk to negotiate a lower
power state for the link when there is no activity (see
the AHCI 1.x spec for details).  This feature is mutually
exclusive with Hot Plug, so when ALPM is enabled, Hot Plug
is disabled.  ALPM will be enabled by default, but it is
settable via the scsi host syfs interface.  Possible 
settings for this feature are:

Setting		Effect
----------------------------------------------------------
min_power	ALPM is enabled, and link set to enter 
		lowest power state (SLUMBER) when idle
		Hot plug not allowed.

max_performance	ALPM is disabled, Hot Plug is allowed

medium_power	ALPM is enabled, and link set to enter
		second lowest power state (PARTIAL) when
		idle.  Hot plug not allowed.

Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

---
I've changed this patch to define "disable_pm", and to change
the behavior of ahci_disable_alpm() to not change the link
pm policy, and just turn off alpm.

this whole patch series can be found here:
http://www.kernel.org/pub/linux/kernel/people/kristen/patches/SATA/alpm/

Index: 2.6-git/drivers/ata/ahci.c
===================================================================
--- 2.6-git.orig/drivers/ata/ahci.c
+++ 2.6-git/drivers/ata/ahci.c
@@ -48,6 +48,9 @@
 #define DRV_NAME	"ahci"
 #define DRV_VERSION	"2.2"
 
+static int ahci_enable_alpm(struct ata_port *ap,
+		enum scsi_host_link_pm policy);
+static int ahci_disable_alpm(struct ata_port *ap);
 
 enum {
 	AHCI_PCI_BAR		= 5,
@@ -97,6 +100,7 @@ enum {
 	/* HOST_CAP bits */
 	HOST_CAP_SSC		= (1 << 14), /* Slumber capable */
 	HOST_CAP_CLO		= (1 << 24), /* Command List Override support */
+	HOST_CAP_ALPM		= (1 << 26), /* Aggressive Link PM support */
 	HOST_CAP_SSS		= (1 << 27), /* Staggered Spin-up */
 	HOST_CAP_NCQ		= (1 << 30), /* Native Command Queueing */
 	HOST_CAP_64		= (1 << 31), /* PCI DAC (64-bit DMA) support */
@@ -151,6 +155,8 @@ enum {
 				  PORT_IRQ_PIOS_FIS | PORT_IRQ_D2H_REG_FIS,
 
 	/* PORT_CMD bits */
+	PORT_CMD_ASP		= (1 << 27), /* Aggressive Slumber/Partial */
+	PORT_CMD_ALPE		= (1 << 26), /* Aggressive Link PM enable */
 	PORT_CMD_ATAPI		= (1 << 24), /* Device is ATAPI */
 	PORT_CMD_LIST_ON	= (1 << 15), /* cmd list DMA engine running */
 	PORT_CMD_FIS_ON		= (1 << 14), /* FIS DMA engine running */
@@ -171,6 +177,7 @@ enum {
 	AHCI_FLAG_HONOR_PI		= (1 << 26), /* honor PORTS_IMPL */
 	AHCI_FLAG_IGN_SERR_INTERNAL	= (1 << 27), /* ignore SERR_INTERNAL */
 	AHCI_FLAG_32BIT_ONLY		= (1 << 28), /* force 32bit */
+	AHCI_FLAG_NO_HOTPLUG		= (1 << 29), /* ignore PxSERR.DIAG.N */
 
 	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
@@ -253,6 +260,7 @@ static struct scsi_host_template ahci_sh
 	.slave_configure	= ata_scsi_slave_config,
 	.slave_destroy		= ata_scsi_slave_destroy,
 	.bios_param		= ata_std_bios_param,
+	.set_link_pm_policy	= ata_scsi_set_link_pm_policy,
 };
 
 static const struct ata_port_operations ahci_ops = {
@@ -284,6 +292,8 @@ static const struct ata_port_operations 
 	.port_suspend		= ahci_port_suspend,
 	.port_resume		= ahci_port_resume,
 #endif
+	.enable_pm		= ahci_enable_alpm,
+	.disable_pm		= ahci_disable_alpm,
 
 	.port_start		= ahci_port_start,
 	.port_stop		= ahci_port_stop,
@@ -719,6 +729,158 @@ static void ahci_power_up(struct ata_por
 	writel(cmd | PORT_CMD_ICC_ACTIVE, port_mmio + PORT_CMD);
 }
 
+static int ahci_disable_alpm(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u32 cmd, scontrol;
+	struct ahci_port_priv *pp = ap->private_data;
+
+	/*
+ 	 * disable Interface Power Management State Transitions
+ 	 * This is accomplished by setting bits 8:11 of the
+ 	 * SATA Control register
+ 	 */
+	scontrol = readl(port_mmio + PORT_SCR_CTL);
+	scontrol |= (0x3 << 8);
+	writel(scontrol, port_mmio + PORT_SCR_CTL);
+
+	/* get the existing command bits */
+	cmd = readl(port_mmio + PORT_CMD);
+
+	/* disable ALPM and ASP */
+	cmd &= ~PORT_CMD_ASP;
+	cmd &= ~PORT_CMD_ALPE;
+
+	/* force the interface back to active */
+	cmd |= PORT_CMD_ICC_ACTIVE;
+
+	/* write out new cmd value */
+	writel(cmd, port_mmio + PORT_CMD);
+	cmd = readl(port_mmio + PORT_CMD);
+
+	/* wait 10ms to be sure we've come out of any low power state */
+	msleep(10);
+
+	/* clear out any PhyRdy stuff from interrupt status */
+	writel(PORT_IRQ_PHYRDY, port_mmio + PORT_IRQ_STAT);
+
+	/* go ahead and clean out PhyRdy Change from Serror too */
+	ahci_scr_write(ap, SCR_ERROR, (1 << 16));
+
+	/*
+ 	 * Clear flag to indicate that we should ignore all PhyRdy
+ 	 * state changes
+ 	 */
+	ap->flags &= ~AHCI_FLAG_NO_HOTPLUG;
+
+	/*
+ 	 * Enable interrupts on Phy Ready.
+ 	 */
+	pp->intr_mask |= PORT_IRQ_PHYRDY;
+	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	/*
+ 	 * don't change the link pm policy - we can be called
+ 	 * just to turn of link pm temporarily
+ 	 */
+	return 0;
+}
+
+static int ahci_enable_alpm(struct ata_port *ap,
+	enum scsi_host_link_pm policy)
+{
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u32 cmd, scontrol, sstatus;
+	struct ahci_port_priv *pp = ap->private_data;
+	u32 asp;
+
+	/* Make sure the host is capable of link power management */
+	if (!(hpriv->cap & HOST_CAP_ALPM)) {
+		ap->pm_policy = SHOST_NOT_AVAILABLE;
+		return -EINVAL;
+	}
+
+	/* make sure we have a device attached */
+	sstatus = readl(port_mmio + PORT_SCR_STAT);
+	if (!(sstatus & 0xf00)) {
+		ap->pm_policy = SHOST_NOT_AVAILABLE;
+		return -EINVAL;
+	}
+
+	switch(policy) {
+	case SHOST_MAX_PERFORMANCE:
+		ahci_disable_alpm(ap);
+		ap->pm_policy = policy;
+		return 0;
+	case SHOST_NOT_AVAILABLE:
+	case SHOST_MIN_POWER:
+		/*
+ 		 * if we came here with SHOST_NOT_AVAILABLE,
+ 		 * it just means this is the first time we
+ 		 * have tried to enable - so try to do
+ 		 * min_power
+ 		 */
+		ap->pm_policy = SHOST_MIN_POWER;
+
+		/* configure HBA to enter SLUMBER */
+		asp = PORT_CMD_ASP;
+		break;
+	case SHOST_MEDIUM_POWER:
+		/* configure HBA to enter PARTIAL */
+		ap->pm_policy = policy;
+		asp = 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*
+ 	 * Disable interrupts on Phy Ready. This keeps us from
+ 	 * getting woken up due to spurious phy ready interrupts
+	 * TBD - Hot plug should be done via polling now, is
+	 * that even supported?
+ 	 */
+	pp->intr_mask &= ~PORT_IRQ_PHYRDY;
+	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
+
+	/*
+ 	 * Set a flag to indicate that we should ignore all PhyRdy
+ 	 * state changes since these can happen now whenever we
+ 	 * change link state
+ 	 */
+	ap->flags |= AHCI_FLAG_NO_HOTPLUG;
+
+	/* get the existing command bits */
+	cmd = readl(port_mmio + PORT_CMD);
+
+	/*
+ 	 * enable Interface Power Management State Transitions
+ 	 * This is accomplished by clearing bits 8:11 of the
+ 	 * SATA Control register
+ 	 */
+	scontrol = readl(port_mmio + PORT_SCR_CTL);
+	scontrol &= ~(0x3 << 8);
+	writel(scontrol, port_mmio + PORT_SCR_CTL);
+
+	/*
+ 	 * Set ASP based on Policy
+ 	 */
+	cmd |= asp;
+
+	/*
+ 	 * Setting this bit will instruct the HBA to aggressively
+ 	 * enter a lower power link state when it's appropriate and
+ 	 * based on the value set above for ASP
+ 	 */
+	cmd |= PORT_CMD_ALPE;
+
+	/* write out new cmd value */
+	writel(cmd, port_mmio + PORT_CMD);
+	cmd = readl(port_mmio + PORT_CMD);
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static void ahci_power_down(struct ata_port *ap)
 {
@@ -1244,6 +1406,10 @@ static void ahci_host_intr(struct ata_po
 	status = readl(port_mmio + PORT_IRQ_STAT);
 	writel(status, port_mmio + PORT_IRQ_STAT);
 
+	if ((ap->flags & AHCI_FLAG_NO_HOTPLUG) &&
+		(status & PORT_IRQ_PHYRDY))
+		status &= ~PORT_IRQ_PHYRDY;
+
 	if (unlikely(status & PORT_IRQ_ERROR)) {
 		ahci_error_intr(ap, status);
 		return;
@@ -1759,6 +1925,7 @@ static int ahci_init_one(struct pci_dev 
 
 			ap->ioaddr.cmd_addr = port_mmio;
 			ap->ioaddr.scr_addr = port_mmio + PORT_SCR;
+			ap->pm_policy = SHOST_NOT_AVAILABLE;
 		} else
 			host->ports[i]->ops = &ata_dummy_port_ops;
 	}

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Jens Axboe]

On Wed, Jun 20 2007, Kristen Carlson Accardi wrote:
> Enable Aggressive Link Power management for AHCI controllers.
> 
> This patch will set the correct bits to turn on Aggressive
> Link Power Management (ALPM) for the ahci driver.  This
> will cause the controller and disk to negotiate a lower
> power state for the link when there is no activity (see
> the AHCI 1.x spec for details).  This feature is mutually
> exclusive with Hot Plug, so when ALPM is enabled, Hot Plug
> is disabled.  ALPM will be enabled by default, but it is
> settable via the scsi host syfs interface.  Possible 
> settings for this feature are:
> 
> Setting		Effect
> ----------------------------------------------------------
> min_power	ALPM is enabled, and link set to enter 
> 		lowest power state (SLUMBER) when idle
> 		Hot plug not allowed.
> 
> max_performance	ALPM is disabled, Hot Plug is allowed
> 
> medium_power	ALPM is enabled, and link set to enter
> 		second lowest power state (PARTIAL) when
> 		idle.  Hot plug not allowed.
> 
> Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

A suggestion (it comes with a patch!) - default to max_power/almp off,
not min_power. For two reasons:

- There's such a big performance difference between the two, you really
  want max_power when booting.

- It's a lot better to default to no change, than default to enabling
  something new.

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 841cf0a..e7a2072 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -786,8 +786,7 @@ static int ahci_disable_alpm(struct ata_port *ap)
 	return 0;
 }
 
-static int ahci_enable_alpm(struct ata_port *ap,
-	enum scsi_host_link_pm policy)
+static int ahci_enable_alpm(struct ata_port *ap, enum scsi_host_link_pm policy)
 {
 	struct ahci_host_priv *hpriv = ap->host->private_data;
 	void __iomem *port_mmio = ahci_port_base(ap);
@@ -808,19 +807,19 @@ static int ahci_enable_alpm(struct ata_port *ap,
 		return -EINVAL;
 	}
 
-	switch(policy) {
+	switch (policy) {
 	case SHOST_MAX_PERFORMANCE:
-		ahci_disable_alpm(ap);
-		ap->pm_policy = policy;
-		return 0;
 	case SHOST_NOT_AVAILABLE:
-	case SHOST_MIN_POWER:
 		/*
  		 * if we came here with SHOST_NOT_AVAILABLE,
  		 * it just means this is the first time we
- 		 * have tried to enable - so try to do
- 		 * min_power
+ 		 * have tried to enable - default to max performance,
+		 * and let the user go to lower power modes on request.
  		 */
+		ahci_disable_alpm(ap);
+		ap->pm_policy = SHOST_MAX_PERFORMANCE;
+		return 0;
+	case SHOST_MIN_POWER:
 		ap->pm_policy = SHOST_MIN_POWER;
 
 		/* configure HBA to enter SLUMBER */

-- 
Jens Axboe

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Kristen Carlson Accardi]

On Thu, 21 Jun 2007 15:08:32 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Wed, Jun 20 2007, Kristen Carlson Accardi wrote:
> > Enable Aggressive Link Power management for AHCI controllers.
> > 
> > This patch will set the correct bits to turn on Aggressive
> > Link Power Management (ALPM) for the ahci driver.  This
> > will cause the controller and disk to negotiate a lower
> > power state for the link when there is no activity (see
> > the AHCI 1.x spec for details).  This feature is mutually
> > exclusive with Hot Plug, so when ALPM is enabled, Hot Plug
> > is disabled.  ALPM will be enabled by default, but it is
> > settable via the scsi host syfs interface.  Possible 
> > settings for this feature are:
> > 
> > Setting		Effect
> > ----------------------------------------------------------
> > min_power	ALPM is enabled, and link set to enter 
> > 		lowest power state (SLUMBER) when idle
> > 		Hot plug not allowed.
> > 
> > max_performance	ALPM is disabled, Hot Plug is allowed
> > 
> > medium_power	ALPM is enabled, and link set to enter
> > 		second lowest power state (PARTIAL) when
> > 		idle.  Hot plug not allowed.
> > 
> > Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> 
> A suggestion (it comes with a patch!) - default to max_power/almp off,
> not min_power. For two reasons:
> 
> - There's such a big performance difference between the two, you really
>   want max_power when booting.
> 
> - It's a lot better to default to no change, than default to enabling
>   something new.

Sounds reasonable to me.  Distros/users can decide if they want to have
scripts that enable this after boot to run at min_power. 

Acked-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>


> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 841cf0a..e7a2072 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -786,8 +786,7 @@ static int ahci_disable_alpm(struct ata_port *ap)
>  	return 0;
>  }
>  
> -static int ahci_enable_alpm(struct ata_port *ap,
> -	enum scsi_host_link_pm policy)
> +static int ahci_enable_alpm(struct ata_port *ap, enum scsi_host_link_pm policy)
>  {
>  	struct ahci_host_priv *hpriv = ap->host->private_data;
>  	void __iomem *port_mmio = ahci_port_base(ap);
> @@ -808,19 +807,19 @@ static int ahci_enable_alpm(struct ata_port *ap,
>  		return -EINVAL;
>  	}
>  
> -	switch(policy) {
> +	switch (policy) {
>  	case SHOST_MAX_PERFORMANCE:
> -		ahci_disable_alpm(ap);
> -		ap->pm_policy = policy;
> -		return 0;
>  	case SHOST_NOT_AVAILABLE:
> -	case SHOST_MIN_POWER:
>  		/*
>   		 * if we came here with SHOST_NOT_AVAILABLE,
>   		 * it just means this is the first time we
> - 		 * have tried to enable - so try to do
> - 		 * min_power
> + 		 * have tried to enable - default to max performance,
> +		 * and let the user go to lower power modes on request.
>   		 */
> +		ahci_disable_alpm(ap);
> +		ap->pm_policy = SHOST_MAX_PERFORMANCE;
> +		return 0;
> +	case SHOST_MIN_POWER:
>  		ap->pm_policy = SHOST_MIN_POWER;
>  
>  		/* configure HBA to enter SLUMBER */
> 
> -- 
> Jens Axboe
> 

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Jens Axboe]

On Fri, Jun 22 2007, Kristen Carlson Accardi wrote:
> On Thu, 21 Jun 2007 15:08:32 +0200
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Wed, Jun 20 2007, Kristen Carlson Accardi wrote:
> > > Enable Aggressive Link Power management for AHCI controllers.
> > > 
> > > This patch will set the correct bits to turn on Aggressive
> > > Link Power Management (ALPM) for the ahci driver.  This
> > > will cause the controller and disk to negotiate a lower
> > > power state for the link when there is no activity (see
> > > the AHCI 1.x spec for details).  This feature is mutually
> > > exclusive with Hot Plug, so when ALPM is enabled, Hot Plug
> > > is disabled.  ALPM will be enabled by default, but it is
> > > settable via the scsi host syfs interface.  Possible 
> > > settings for this feature are:
> > > 
> > > Setting		Effect
> > > ----------------------------------------------------------
> > > min_power	ALPM is enabled, and link set to enter 
> > > 		lowest power state (SLUMBER) when idle
> > > 		Hot plug not allowed.
> > > 
> > > max_performance	ALPM is disabled, Hot Plug is allowed
> > > 
> > > medium_power	ALPM is enabled, and link set to enter
> > > 		second lowest power state (PARTIAL) when
> > > 		idle.  Hot plug not allowed.
> > > 
> > > Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> > 
> > A suggestion (it comes with a patch!) - default to max_power/almp off,
> > not min_power. For two reasons:
> > 
> > - There's such a big performance difference between the two, you really
> >   want max_power when booting.
> > 
> > - It's a lot better to default to no change, than default to enabling
> >   something new.
> 
> Sounds reasonable to me.  Distros/users can decide if they want to have
> scripts that enable this after boot to run at min_power. 

Exactly, it needs to be handled by some power management daemon anyway
and be integrated with power savings in general. You could use io load
to determine when to enable/disable alpm, for instance.

> Acked-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Will you integrate it into the next posting?

-- 
Jens Axboe

Re: [patch 3/3] Enable Aggressive Link Power management for AHCI controllers.

[Kristen Carlson Accardi]

On Fri, 22 Jun 2007 21:00:00 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> Exactly, it needs to be handled by some power management daemon anyway
> and be integrated with power savings in general. You could use io load
> to determine when to enable/disable alpm, for instance.
> 
> > Acked-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> 
> Will you integrate it into the next posting?

yes, I will do that.


Kristen

[patch 1/3] Store interrupt value

[Kristen Carlson Accardi]

Use a stored value for which interrupts to enable.  Changing this allows
us to selectively turn off certain interrupts later and have them 
stay off.

Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/drivers/ata/ahci.c
===================================================================
--- 2.6-git.orig/drivers/ata/ahci.c
+++ 2.6-git/drivers/ata/ahci.c
@@ -175,6 +175,7 @@ enum {
 	AHCI_FLAG_32BIT_ONLY		= (1 << 28), /* force 32bit */
 	AHCI_FLAG_MV_PATA		= (1 << 29), /* PATA port */
 	AHCI_FLAG_NO_MSI		= (1 << 30), /* no PCI MSI */
+	AHCI_FLAG_NO_HOTPLUG		= (1 << 31), /* ignore PxSERR.DIAG.N */
 
 	AHCI_FLAG_COMMON		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 					  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
@@ -215,6 +216,7 @@ struct ahci_port_priv {
 	unsigned int		ncq_saw_d2h:1;
 	unsigned int		ncq_saw_dmas:1;
 	unsigned int		ncq_saw_sdb:1;
+	u32 			intr_mask;	/* interrupts to enable */
 };
 
 static int ahci_scr_read(struct ata_port *ap, unsigned int sc_reg, u32 *val);
@@ -1518,6 +1520,7 @@ static void ahci_thaw(struct ata_port *a
 	void __iomem *mmio = ap->host->iomap[AHCI_PCI_BAR];
 	void __iomem *port_mmio = ahci_port_base(ap);
 	u32 tmp;
+	struct ahci_port_priv *pp = ap->private_data;
 
 	/* clear IRQ */
 	tmp = readl(port_mmio + PORT_IRQ_STAT);
@@ -1525,7 +1528,7 @@ static void ahci_thaw(struct ata_port *a
 	writel(1 << ap->port_no, mmio + HOST_IRQ_STAT);
 
 	/* turn IRQ back on */
-	writel(DEF_PORT_IRQ, port_mmio + PORT_IRQ_MASK);
+	writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK);
 }
 
 static void ahci_error_handler(struct ata_port *ap)
@@ -1679,6 +1682,12 @@ static int ahci_port_start(struct ata_po
 	pp->cmd_tbl = mem;
 	pp->cmd_tbl_dma = mem_dma;
 
+	/*
+ 	 * Save off initial list of interrupts to be enabled.
+ 	 * This could be changed later
+ 	 */
+	pp->intr_mask = DEF_PORT_IRQ;
+
 	ap->private_data = pp;
 
 	/* engage engines, captain */

--