[PATCH] ata: ahci: power off unused ports

[PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

power off unused ports

If the port isn't either a drive bay or an external SATA port, do not 
keep the phy powered on if the port is unoccupied. This saves .75 watts
on most laptops.  A module parameter can be used to override this 
behavior and allow the phy's to be powered up regardless of whether it
has externally accessible SATA ports. 

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

---
 drivers/ata/ahci.c        |   21 +++++++++++++++++++++
 drivers/ata/libata-core.c |   24 ++++++++++++++++++++++++
 include/linux/libata.h    |    1 +
 3 files changed, 46 insertions(+)

Index: linux-ahci-phy/drivers/ata/ahci.c
===================================================================
--- linux-ahci-phy.orig/drivers/ata/ahci.c	2008-05-08 14:29:02.000000000 -0700
+++ linux-ahci-phy/drivers/ata/ahci.c	2008-05-08 14:31:05.000000000 -0700
@@ -53,9 +53,13 @@ static int ahci_skip_host_reset;
 module_param_named(skip_host_reset, ahci_skip_host_reset, int, 0444);
 MODULE_PARM_DESC(skip_host_reset, "skip global host reset (0=don't skip, 1=skip)");
 
+static int ahci_power_save = 1;
+module_param_named(power_save, ahci_power_save, int, 0444);
+MODULE_PARM_DESC(power_save, "Power off unused ports (0=don't power off, 1=power off)");
 static int ahci_enable_alpm(struct ata_port *ap,
 		enum link_pm policy);
 static void ahci_disable_alpm(struct ata_port *ap);
+static int ahci_is_hotplug_capable(struct ata_port *ap);
 
 enum {
 	AHCI_PCI_BAR		= 5,
@@ -166,6 +170,8 @@ enum {
 	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_ESP		= (1 << 21), /* External SATA Port */
+	PORT_CMD_HPCP		= (1 << 18), /* port is hot plug capable */
 	PORT_CMD_PMP		= (1 << 17), /* PMP attached */
 	PORT_CMD_LIST_ON	= (1 << 15), /* cmd list DMA engine running */
 	PORT_CMD_FIS_ON		= (1 << 14), /* FIS DMA engine running */
@@ -1900,6 +1906,18 @@ static int ahci_pci_device_resume(struct
 }
 #endif
 
+static int ahci_is_hotplug_capable(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u8 cmd;
+
+	if (!ahci_power_save)
+		return 1;
+
+	cmd = readl(port_mmio + PORT_CMD);
+	return ((cmd & PORT_CMD_HPCP) || (cmd & PORT_CMD_ESP));
+}
+
 static int ahci_port_start(struct ata_port *ap)
 {
 	struct device *dev = ap->host->dev;
@@ -1951,6 +1969,9 @@ static int ahci_port_start(struct ata_po
 
 	ap->private_data = pp;
 
+	/* set some flags based on port capabilities */
+	if (!ahci_is_hotplug_capable(ap))
+		ap->flags |= ATA_FLAG_NO_HOTPLUG;
 	/* engage engines, captain */
 	return ahci_port_resume(ap);
 }
Index: linux-ahci-phy/drivers/ata/libata-core.c
===================================================================
--- linux-ahci-phy.orig/drivers/ata/libata-core.c	2008-05-08 14:28:57.000000000 -0700
+++ linux-ahci-phy/drivers/ata/libata-core.c	2008-05-08 14:29:50.000000000 -0700
@@ -162,6 +162,19 @@ MODULE_DESCRIPTION("Library module for A
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
+static void ata_phy_offline(struct ata_link *link)
+{
+	u32 scontrol;
+	int rc;
+
+	/* set DET to 4 */
+	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
+	if (rc)
+		return;
+	scontrol &= ~0xf;
+	scontrol |= (1 << 2);
+	sata_scr_write(link, SCR_CONTROL, scontrol);
+}
 
 /**
  *	ata_force_cbl - force cable type according to libata.force
@@ -2671,6 +2684,7 @@ void ata_port_disable(struct ata_port *a
 	ap->link.device[0].class = ATA_DEV_NONE;
 	ap->link.device[1].class = ATA_DEV_NONE;
 	ap->flags |= ATA_FLAG_DISABLED;
+	ata_phy_offline(&ap->link);
 }
 
 /**
@@ -5609,6 +5623,8 @@ int ata_host_register(struct ata_host *h
 		if (ap->ops->error_handler) {
 			struct ata_eh_info *ehi = &ap->link.eh_info;
 			unsigned long flags;
+			int device_attached = 0;
+			struct ata_device *dev;
 
 			ata_port_probe(ap);
 
@@ -5627,6 +5643,14 @@ int ata_host_register(struct ata_host *h
 
 			/* wait for EH to finish */
 			ata_port_wait_eh(ap);
+			ata_link_for_each_dev(dev, &ap->link)
+				if (ata_dev_enabled(dev))
+					device_attached++;
+			if (!device_attached &&
+			    (ap->flags & ATA_FLAG_NO_HOTPLUG)) {
+				/* no device present, disable port */
+				ata_port_disable(ap);
+			}
 		} else {
 			DPRINTK("ata%u: bus probe begin\n", ap->print_id);
 			rc = ata_bus_probe(ap);
Index: linux-ahci-phy/include/linux/libata.h
===================================================================
--- linux-ahci-phy.orig/include/linux/libata.h	2008-05-08 14:28:57.000000000 -0700
+++ linux-ahci-phy/include/linux/libata.h	2008-05-08 14:29:50.000000000 -0700
@@ -193,6 +193,7 @@ enum {
 	ATA_FLAG_AN		= (1 << 18), /* controller supports AN */
 	ATA_FLAG_PMP		= (1 << 19), /* controller supports PMP */
 	ATA_FLAG_IPM		= (1 << 20), /* driver can handle IPM */
+	ATA_FLAG_NO_HOTPLUG	= (1 << 21), /* port doesn't support HP */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Fri, 9 May 2008 00:37:13 +0100
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> On Thu, May 08, 2008 at 04:10:08PM -0700, Kristen Carlson Accardi wrote:
> 
> > +static int ahci_is_hotplug_capable(struct ata_port *ap)
> > +{
> > +	void __iomem *port_mmio = ahci_port_base(ap);
> > +	u8 cmd;
> > +
> > +	if (!ahci_power_save)
> > +		return 1;
> > +
> > +	cmd = readl(port_mmio + PORT_CMD);
> > +	return ((cmd & PORT_CMD_HPCP) || (cmd & PORT_CMD_ESP));
> > +}
> 
> Just out of interest, is this set on laptop bays? (Are any of those even 
> native SATA yet?)

I haven't seen any yet that are.  Most of them are PATA.  I expect
that will change in the future.

Re: [PATCH] ata: ahci: power off unused ports

[Matthew Garrett]

On Thu, May 08, 2008 at 04:10:08PM -0700, Kristen Carlson Accardi wrote:

> +static int ahci_is_hotplug_capable(struct ata_port *ap)
> +{
> +	void __iomem *port_mmio = ahci_port_base(ap);
> +	u8 cmd;
> +
> +	if (!ahci_power_save)
> +		return 1;
> +
> +	cmd = readl(port_mmio + PORT_CMD);
> +	return ((cmd & PORT_CMD_HPCP) || (cmd & PORT_CMD_ESP));
> +}

Just out of interest, is this set on laptop bays? (Are any of those even 
native SATA yet?)
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ata: ahci: power off unused ports

[Matthew Garrett]

On Thu, May 08, 2008 at 04:35:55PM -0700, Kristen Carlson Accardi wrote:
> On Fri, 9 May 2008 00:37:13 +0100
> Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > Just out of interest, is this set on laptop bays? (Are any of those even 
> > native SATA yet?)
> 
> I haven't seen any yet that are.  Most of them are PATA.  I expect
> that will change in the future.

Yeah. My vague concern would be that on laptops, we can probably power 
down the port even if it's declared as being hotplug capable - we'll 
probably get an out of band signal from ACPI anyway. Not an issue at the 
moment, but possibly something we'll hit in future.

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

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Fri, 9 May 2008 01:14:58 +0100
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> On Thu, May 08, 2008 at 04:35:55PM -0700, Kristen Carlson Accardi wrote:
> > On Fri, 9 May 2008 00:37:13 +0100
> > Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > > Just out of interest, is this set on laptop bays? (Are any of those even 
> > > native SATA yet?)
> > 
> > I haven't seen any yet that are.  Most of them are PATA.  I expect
> > that will change in the future.
> 
> Yeah. My vague concern would be that on laptops, we can probably power 
> down the port even if it's declared as being hotplug capable - we'll 
> probably get an out of band signal from ACPI anyway. Not an issue at the 
> moment, but possibly something we'll hit in future.
> 

well - when drive bays on laptops are able to be controlled by
AHCI, they may not actually use _EJ0 or send notifications anymore, 
since ACPI will not longer be necessary for hotplugging.  We'll have 
to see what happens.

Re: [PATCH] ata: ahci: power off unused ports

[Mark Lord]

Kristen Carlson Accardi wrote:
> power off unused ports
> 
> If the port isn't either a drive bay or an external SATA port, do not 
> keep the phy powered on if the port is unoccupied. This saves .75 watts
> on most laptops.  A module parameter can be used to override this 
> behavior and allow the phy's to be powered up regardless of whether it
> has externally accessible SATA ports. 
> 
> Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
..
> +static void ata_phy_offline(struct ata_link *link)
> +{
> +	u32 scontrol;
> +	int rc;
> +
> +	/* set DET to 4 */
> +	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
> +	if (rc)
> +		return;
> +	scontrol &= ~0xf;
> +	scontrol |= (1 << 2);
> +	sata_scr_write(link, SCR_CONTROL, scontrol);
> +}
..

I don't like to be picky, but we already have an "ata_link_offline"
function in libata, which tests *whether* a link is offline,
as opposed to *setting* a link to be offline.

So in that context, I find the name ata_phy_offline slightly confusing.
Perhaps something like ata_set_phy_offline would make it more clear?

And a more general note:  I still believe we should have a follow-up
feature to this one, to enable polling for newly inserted drives.

That would allow powering down idle ports to save money/planet/whatever,
but still with hotplug capability.  The polling interval should be
tunable in /sys, with a default of, say, once every couple of seconds.

Thanks for working on this stuff.

Cheers

Port control interface (was Re: [PATCH] ata: ahci: power off unused ports)

[Jeff Garzik]

Mark Lord wrote:
> And a more general note:  I still believe we should have a follow-up
> feature to this one, to enable polling for newly inserted drives.
> 
> That would allow powering down idle ports to save money/planet/whatever,
> but still with hotplug capability.  The polling interval should be
> tunable in /sys, with a default of, say, once every couple of seconds.
> 
> Thanks for working on this stuff.


In the grand open source tradition of "pass the buck", I've long been 
hoping that someone would take a few days, sit down, and hammer out the 
policy side of this.

We -don't- want to do hotplug-active all the time -- the current default 
in all drivers that support device hotplug -- because it needlessly 
keeps parts active that are unused 99.9% of the time [when they are empty].

Admins need a generic way to control SATA ports and links from 
userspace.  Within that, admins need to be able to set a link's hotplug 
state among three choices:  hotplug-active [when supported], 
hotplug-poll, and hotplug-never.  And of course, hotplug-poll's interval 
should be tunable.

And of course this control interface needs to be usable on SAS/SATA 
cards that will be common in the future (broadsas, mvsas are early 
examples).


This is why I look askance at an AHCI BIOS flag.  That's merely a hint, 
and potentially unreliable one at that.  It could just be describing 
what the BIOS writer felt were the ports that _should_ be hotpluggable 
-- i.e. not even describing what is _possible_ but someone's definition 
of "reasonable."  The BIOS flag might even be filled with fuzz (AHCI's 
BIOS-written registers have occasionally been shipped into the field 
uninitialized).

A better solution involves taking the BIOS flag and using that to set a 
default policy that an admin can easily override using the 
above-mentioned control interface.

Because in some cases, that BIOS flag might do the wrong thing, and we 
need to give the admin an ability to undo the damage.

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Lennart Sorensen]

On Fri, May 09, 2008 at 12:37:13AM +0100, Matthew Garrett wrote:
> Just out of interest, is this set on laptop bays? (Are any of those even 
> native SATA yet?)

My wife's Asus R1F does in fact have SATA in use for the drive bay and
allows swapping between the DVD-RW and a second HD (or another battery
if she had one) in that bay.  I would think many laptops do that now.

-- 
Len Sorensen

Re: [PATCH] ata: ahci: power off unused ports

[Matthew Garrett]

On Fri, May 09, 2008 at 11:58:31AM -0400, Lennart Sorensen wrote:
> On Fri, May 09, 2008 at 12:37:13AM +0100, Matthew Garrett wrote:
> > Just out of interest, is this set on laptop bays? (Are any of those even 
> > native SATA yet?)
> 
> My wife's Asus R1F does in fact have SATA in use for the drive bay and
> allows swapping between the DVD-RW and a second HD (or another battery
> if she had one) in that bay.  I would think many laptops do that now.

Genuinely SATA, or PATA bridged to SATA?
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] ata: ahci: power off unused ports

[Lennart Sorensen]

On Fri, May 09, 2008 at 05:06:41PM +0100, Matthew Garrett wrote:
> Genuinely SATA, or PATA bridged to SATA?

SATA drives using intel 945GM chipset which has two native SATA ports.

-- 
Len Sorensen

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Fri, 9 May 2008 12:14:56 -0400
lsorense@csclub.uwaterloo.ca (Lennart Sorensen) wrote:

> On Fri, May 09, 2008 at 05:06:41PM +0100, Matthew Garrett wrote:
> > Genuinely SATA, or PATA bridged to SATA?
> 
> SATA drives using intel 945GM chipset which has two native SATA ports.
> 

Most 945s I've seen in implementation have the DVD/drive bay as PATA
bridged to SATA.

But, this is an irrelevant conversation.  

For drives controlled by ata_piix, they will continue with the 
"hotplug all the time" policy that the rest of libata has until 
someone else patches that driver to mark the drives not hotpluggable
based on whatever method they want to use to determine that.  As we 
get more laptops implementing pure SATA drive bays we can refine 
the patch to use ACPI events to turn the phy back on if 
laptop vendors continue to use ACPI to generate insert/remove events.

Re: [PATCH] ata: ahci: power off unused ports

[Theodore Tso]

On Thu, May 08, 2008 at 04:10:08PM -0700, Kristen Carlson Accardi wrote:
> power off unused ports
> 
> If the port isn't either a drive bay or an external SATA port, do not 
> keep the phy powered on if the port is unoccupied. This saves .75 watts
> on most laptops.  A module parameter can be used to override this 
> behavior and allow the phy's to be powered up regardless of whether it
> has externally accessible SATA ports. 
> 
> Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Hi,

I'm wondering what's the status of this patch?  

if this has hit a subsystem tree yet?  0.75 watts power savings sounds
really cool, but it doesn't seem to be in Linus's mainline yet.  Has
it hit a subsystem tree yet, or is there some issue that still needs
to be resolved before it can go it?

Thanks, regards,

							- Ted

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Mon, 26 May 2008 23:08:54 -0400
Theodore Tso <tytso@mit.edu> wrote:

> On Thu, May 08, 2008 at 04:10:08PM -0700, Kristen Carlson Accardi wrote:
> > power off unused ports
> > 
> > If the port isn't either a drive bay or an external SATA port, do not 
> > keep the phy powered on if the port is unoccupied. This saves .75 watts
> > on most laptops.  A module parameter can be used to override this 
> > behavior and allow the phy's to be powered up regardless of whether it
> > has externally accessible SATA ports. 
> > 
> > Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> 
> Hi,
> 
> I'm wondering what's the status of this patch?  
> 
> if this has hit a subsystem tree yet?  0.75 watts power savings sounds
> really cool, but it doesn't seem to be in Linus's mainline yet.  Has
> it hit a subsystem tree yet, or is there some issue that still needs
> to be resolved before it can go it?
> 
> Thanks, regards,
> 
> 							- Ted
> 

Hi Ted,
As far as I know the patch has gone nowhere.  I believe that 
Jeff wanted something more flexible than the module parameter that
I provided to override the BIOS options.  I am not working on this,
I figured he had a pretty firm idea what he wanted so he was better
equipped to write the patch.

Kristen

Re: [PATCH] ata: ahci: power off unused ports

[Theodore Tso]

On Tue, May 27, 2008 at 02:32:02PM -0700, Kristen Carlson Accardi wrote:
> 
> As far as I know the patch has gone nowhere.  I believe that 
> Jeff wanted something more flexible than the module parameter that
> I provided to override the BIOS options.  I am not working on this,
> I figured he had a pretty firm idea what he wanted so he was better
> equipped to write the patch.

Thanks Kristen,

Can you say which laptops you had tested this on where it saved power?
(Did you test any Thinkpads, in particular?)  I'm wondering if it's
worth trying to forward port your patch as a private mod to my kernel;
30 to 40 minutes of extra battery life is nothing to sneeze at!

						- Ted

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Tue, 27 May 2008 18:59:26 -0400
Theodore Tso <tytso@mit.edu> wrote:

> On Tue, May 27, 2008 at 02:32:02PM -0700, Kristen Carlson Accardi wrote:
> > 
> > As far as I know the patch has gone nowhere.  I believe that 
> > Jeff wanted something more flexible than the module parameter that
> > I provided to override the BIOS options.  I am not working on this,
> > I figured he had a pretty firm idea what he wanted so he was better
> > equipped to write the patch.
> 
> Thanks Kristen,
> 
> Can you say which laptops you had tested this on where it saved power?
> (Did you test any Thinkpads, in particular?)  I'm wondering if it's
> worth trying to forward port your patch as a private mod to my kernel;
> 30 to 40 minutes of extra battery life is nothing to sneeze at!
> 
> 						- Ted
> 

I tested this on an Intel mobile software development platform 
with a newer mobile ICH - the power savings were measured at the actual 
component (via probes on the ICH), so I did not measure the power 
savings at the wall socket, although I would expect the power savings
to be even greater on the other side of the power supply.  So in short,
yes, I think it's worth it to give it a try - the patch is pretty
unintrusive, so it should be that difficult a port to do.

Re: [PATCH] ata: ahci: power off unused ports

[Pavel Machek]

Hi!

> > > As far as I know the patch has gone nowhere.  I believe that 
> > > Jeff wanted something more flexible than the module parameter that
> > > I provided to override the BIOS options.  I am not working on this,
> > > I figured he had a pretty firm idea what he wanted so he was better
> > > equipped to write the patch.
> > 
> > Thanks Kristen,
> > 
> > Can you say which laptops you had tested this on where it saved power?
> > (Did you test any Thinkpads, in particular?)  I'm wondering if it's
> > worth trying to forward port your patch as a private mod to my kernel;
> > 30 to 40 minutes of extra battery life is nothing to sneeze at!
> > 
> > 						- Ted
> > 
> 
> I tested this on an Intel mobile software development platform 
> with a newer mobile ICH - the power savings were measured at the actual 
> component (via probes on the ICH), so I did not measure the power 
> savings at the wall socket, although I would expect the power savings
> to be even greater on the other side of the power supply.  So in short,
> yes, I think it's worth it to give it a try - the patch is pretty
> unintrusive, so it should be that difficult a port to do.

Can you repost the patch? I believe we should push it and only add
complex enable/disable functionality if someone needs it...

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] ata: ahci: power off unused ports

[Jeff Garzik]

Pavel Machek wrote:
> Hi!
> 
>>>> As far as I know the patch has gone nowhere.  I believe that 
>>>> Jeff wanted something more flexible than the module parameter that
>>>> I provided to override the BIOS options.  I am not working on this,
>>>> I figured he had a pretty firm idea what he wanted so he was better
>>>> equipped to write the patch.
>>> Thanks Kristen,
>>>
>>> Can you say which laptops you had tested this on where it saved power?
>>> (Did you test any Thinkpads, in particular?)  I'm wondering if it's
>>> worth trying to forward port your patch as a private mod to my kernel;
>>> 30 to 40 minutes of extra battery life is nothing to sneeze at!
>>>
>>> 						- Ted
>>>
>> I tested this on an Intel mobile software development platform 
>> with a newer mobile ICH - the power savings were measured at the actual 
>> component (via probes on the ICH), so I did not measure the power 
>> savings at the wall socket, although I would expect the power savings
>> to be even greater on the other side of the power supply.  So in short,
>> yes, I think it's worth it to give it a try - the patch is pretty
>> unintrusive, so it should be that difficult a port to do.
> 
> Can you repost the patch? I believe we should push it and only add
> complex enable/disable functionality if someone needs it...

If you are talking about SATA -- incorrect.

The patch deals with policy, and the user MUST have the ability to 
control this stuff.  Otherwise you create a situation where the user 
might be denied hotplug use in valid cases, or similar negative situations.

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Alan Cox]

> If you are talking about SATA -- incorrect.
> 
> The patch deals with policy, and the user MUST have the ability to 
> control this stuff.  Otherwise you create a situation where the user 
> might be denied hotplug use in valid cases, or similar negative situations.

The policy isn't however complicated. Tejun added the stuff for forcing
cable type and mode on setup and has therefore written all the per device
setup code we might need. Alternatively a single

	foo=1/0

option has been fine for acpi and will do fine for this. Total additional
cost - 1 line.

Alan

Re: [PATCH] ata: ahci: power off unused ports

[Jeff Garzik]

Alan Cox wrote:
>> If you are talking about SATA -- incorrect.
>>
>> The patch deals with policy, and the user MUST have the ability to 
>> control this stuff.  Otherwise you create a situation where the user 
>> might be denied hotplug use in valid cases, or similar negative situations.
> 
> The policy isn't however complicated. Tejun added the stuff for forcing
> cable type and mode on setup and has therefore written all the per device
> setup code we might need. Alternatively a single
> 
> 	foo=1/0
> 
> option has been fine for acpi and will do fine for this. Total additional
> cost - 1 line.

The key requirement is per-port control.  Ideally via hdparm or another 
userspace tool, but kernel command line (module options) or sysfs would 
be just fine too.  And agreed, the minimal you need is simply 1/0 for 
the port's policy.

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Alan Cox]

> The key requirement is per-port control.  Ideally via hdparm or another 
> userspace tool, but kernel command line (module options) or sysfs would 
> be just fine too.  And agreed, the minimal you need is simply 1/0 for 
> the port's policy.

We don't need it per port Jeff, you are being quite silly here. Right now
its permanently foo=0 for all ports and nobody has suffered anything too
horrible so being able to turn it off for all ports is clearly quite
sufficient for the neat future. If someone wants it per port they'll send
you a patch later.

Alan

Re: [PATCH] ata: ahci: power off unused ports

[Jeff Garzik]

Alan Cox wrote:
>> The key requirement is per-port control.  Ideally via hdparm or another 
>> userspace tool, but kernel command line (module options) or sysfs would 
>> be just fine too.  And agreed, the minimal you need is simply 1/0 for 
>> the port's policy.
> 
> We don't need it per port Jeff, you are being quite silly here. Right now
> its permanently foo=0 for all ports and nobody has suffered anything too
> horrible so being able to turn it off for all ports is clearly quite
> sufficient for the neat future.

Er, huh?  foo=0 means hotplug continues to work on the unused ports. 
Nobody has suffered because we default to enabling all the goodies.

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Mark Lord]

Jeff Garzik wrote:
> Alan Cox wrote:
>>> If you are talking about SATA -- incorrect.
>>>
>>> The patch deals with policy, and the user MUST have the ability to 
>>> control this stuff.  Otherwise you create a situation where the user 
>>> might be denied hotplug use in valid cases, or similar negative 
>>> situations.
>>
>> The policy isn't however complicated. Tejun added the stuff for forcing
>> cable type and mode on setup and has therefore written all the per device
>> setup code we might need. Alternatively a single
>>
>>     foo=1/0
>>
>> option has been fine for acpi and will do fine for this. Total additional
>> cost - 1 line.
> 
> The key requirement is per-port control.  Ideally via hdparm or another 
> userspace tool, but kernel command line (module options) or sysfs would 
> be just fine too.  And agreed, the minimal you need is simply 1/0 for 
> the port's policy.
..

Btw.. hdparm-8.7 (unreleased) can grok /sys now, so that interface is
as good as any from a userspace viewpoint now.

For the power-off of unused ports, the current patch still sounds
extremely vendor-specific (Intel).

Does it actually work (demonstrate, please) on any other hardware ?

I would still like to see a far more generic solution, with periodic polling
and the like, which would permit use on *any* machines (eg. data centers)
without loss of hotplug capability on those ports.

But that's probably just wishful thinking at this point.

Cheers

Re: [PATCH] ata: ahci: power off unused ports

[Alan Cox]

> > We don't need it per port Jeff, you are being quite silly here. Right now
> > its permanently foo=0 for all ports and nobody has suffered anything too
> > horrible so being able to turn it off for all ports is clearly quite
> > sufficient for the neat future.
> 
> Er, huh?  foo=0 means hotplug continues to work on the unused ports. 
> Nobody has suffered because we default to enabling all the goodies.

Thats a simple question of what "=0" means - is it "disable=1" or
"enable=1" or to quote Alice in Wonderland

	'When I use a word,' Humpty Dumpty said, in a rather scornful
	tone,' it means just what I choose it to mean, neither more nor
	less

It really doesn't matter if it is on by default or off by default,
whether 0 means off or on, what matters is that the single switch is
quite sufficient initially to improve matters for everyone wanting to use
the feature without harming anyone else.

Sure you might want to make it per port later but someone who wants to
optimise that peculiar case, has a problem and can test it can deal with
that.

Alan

Re: [PATCH] ata: ahci: power off unused ports

[Jeff Garzik]

Mark Lord wrote:
> For the power-off of unused ports, the current patch still sounds
> extremely vendor-specific (Intel).
> 
> Does it actually work (demonstrate, please) on any other hardware ?

Ding...  correct.  We need something per-port that's not Intel specific.

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Mon, 02 Jun 2008 05:48:16 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Alan Cox wrote:
> >> The key requirement is per-port control.  Ideally via hdparm or another 
> >> userspace tool, but kernel command line (module options) or sysfs would 
> >> be just fine too.  And agreed, the minimal you need is simply 1/0 for 
> >> the port's policy.
> > 
> > We don't need it per port Jeff, you are being quite silly here. Right now
> > its permanently foo=0 for all ports and nobody has suffered anything too
> > horrible so being able to turn it off for all ports is clearly quite
> > sufficient for the neat future.
> 
> Er, huh?  foo=0 means hotplug continues to work on the unused ports. 
> Nobody has suffered because we default to enabling all the goodies.
> 
> 	Jeff
> 
> 

well - I disagree about nobody suffering.  I would argue that most 
users don't use hotplug and would prefer power savings to be the 
default, but the patch I sent implemented a module parameter to allow 
disable power savings if the user desires hotplug on every port.  
Also keep in mind that this patch would still allow hotplug on a per 
port basis if the BIOS tells us that this is either an external SATA 
port or a port with some kind of externally accessible mechanism.  
Here's the patch I sent so you can all review it again.

power off unused ports

If the port isn't either a drive bay or an external SATA port, do not 
keep the phy powered on if the port is unoccupied. This saves .75 watts
on most laptops.  A module parameter can be used to override this 
behavior and allow the phy's to be powered up regardless of whether it
has externally accessible SATA ports. 

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

---
 drivers/ata/ahci.c        |   21 +++++++++++++++++++++
 drivers/ata/libata-core.c |   24 ++++++++++++++++++++++++
 include/linux/libata.h    |    1 +
 3 files changed, 46 insertions(+)

Index: linux-ahci-phy/drivers/ata/ahci.c
===================================================================
--- linux-ahci-phy.orig/drivers/ata/ahci.c	2008-05-08 14:29:02.000000000 -0700
+++ linux-ahci-phy/drivers/ata/ahci.c	2008-05-08 14:31:05.000000000 -0700
@@ -53,9 +53,13 @@ static int ahci_skip_host_reset;
 module_param_named(skip_host_reset, ahci_skip_host_reset, int, 0444);
 MODULE_PARM_DESC(skip_host_reset, "skip global host reset (0=don't skip, 1=skip)");
 
+static int ahci_power_save = 1;
+module_param_named(power_save, ahci_power_save, int, 0444);
+MODULE_PARM_DESC(power_save, "Power off unused ports (0=don't power off, 1=power off)");
 static int ahci_enable_alpm(struct ata_port *ap,
 		enum link_pm policy);
 static void ahci_disable_alpm(struct ata_port *ap);
+static int ahci_is_hotplug_capable(struct ata_port *ap);
 
 enum {
 	AHCI_PCI_BAR		= 5,
@@ -166,6 +170,8 @@ enum {
 	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_ESP		= (1 << 21), /* External SATA Port */
+	PORT_CMD_HPCP		= (1 << 18), /* port is hot plug capable */
 	PORT_CMD_PMP		= (1 << 17), /* PMP attached */
 	PORT_CMD_LIST_ON	= (1 << 15), /* cmd list DMA engine running */
 	PORT_CMD_FIS_ON		= (1 << 14), /* FIS DMA engine running */
@@ -1900,6 +1906,18 @@ static int ahci_pci_device_resume(struct
 }
 #endif
 
+static int ahci_is_hotplug_capable(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u8 cmd;
+
+	if (!ahci_power_save)
+		return 1;
+
+	cmd = readl(port_mmio + PORT_CMD);
+	return ((cmd & PORT_CMD_HPCP) || (cmd & PORT_CMD_ESP));
+}
+
 static int ahci_port_start(struct ata_port *ap)
 {
 	struct device *dev = ap->host->dev;
@@ -1951,6 +1969,9 @@ static int ahci_port_start(struct ata_po
 
 	ap->private_data = pp;
 
+	/* set some flags based on port capabilities */
+	if (!ahci_is_hotplug_capable(ap))
+		ap->flags |= ATA_FLAG_NO_HOTPLUG;
 	/* engage engines, captain */
 	return ahci_port_resume(ap);
 }
Index: linux-ahci-phy/drivers/ata/libata-core.c
===================================================================
--- linux-ahci-phy.orig/drivers/ata/libata-core.c	2008-05-08 14:28:57.000000000 -0700
+++ linux-ahci-phy/drivers/ata/libata-core.c	2008-05-08 14:29:50.000000000 -0700
@@ -162,6 +162,19 @@ MODULE_DESCRIPTION("Library module for A
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
+static void ata_phy_offline(struct ata_link *link)
+{
+	u32 scontrol;
+	int rc;
+
+	/* set DET to 4 */
+	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
+	if (rc)
+		return;
+	scontrol &= ~0xf;
+	scontrol |= (1 << 2);
+	sata_scr_write(link, SCR_CONTROL, scontrol);
+}
 
 /**
  *	ata_force_cbl - force cable type according to libata.force
@@ -2671,6 +2684,7 @@ void ata_port_disable(struct ata_port *a
 	ap->link.device[0].class = ATA_DEV_NONE;
 	ap->link.device[1].class = ATA_DEV_NONE;
 	ap->flags |= ATA_FLAG_DISABLED;
+	ata_phy_offline(&ap->link);
 }
 
 /**
@@ -5609,6 +5623,8 @@ int ata_host_register(struct ata_host *h
 		if (ap->ops->error_handler) {
 			struct ata_eh_info *ehi = &ap->link.eh_info;
 			unsigned long flags;
+			int device_attached = 0;
+			struct ata_device *dev;
 
 			ata_port_probe(ap);
 
@@ -5627,6 +5643,14 @@ int ata_host_register(struct ata_host *h
 
 			/* wait for EH to finish */
 			ata_port_wait_eh(ap);
+			ata_link_for_each_dev(dev, &ap->link)
+				if (ata_dev_enabled(dev))
+					device_attached++;
+			if (!device_attached &&
+			    (ap->flags & ATA_FLAG_NO_HOTPLUG)) {
+				/* no device present, disable port */
+				ata_port_disable(ap);
+			}
 		} else {
 			DPRINTK("ata%u: bus probe begin\n", ap->print_id);
 			rc = ata_bus_probe(ap);
Index: linux-ahci-phy/include/linux/libata.h
===================================================================
--- linux-ahci-phy.orig/include/linux/libata.h	2008-05-08 14:28:57.000000000 -0700
+++ linux-ahci-phy/include/linux/libata.h	2008-05-08 14:29:50.000000000 -0700
@@ -193,6 +193,7 @@ enum {
 	ATA_FLAG_AN		= (1 << 18), /* controller supports AN */
 	ATA_FLAG_PMP		= (1 << 19), /* controller supports PMP */
 	ATA_FLAG_IPM		= (1 << 20), /* driver can handle IPM */
+	ATA_FLAG_NO_HOTPLUG	= (1 << 21), /* port doesn't support HP */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Mon, 02 Jun 2008 09:03:04 -0400
Mark Lord <liml@rtr.ca> wrote:

> Jeff Garzik wrote:
> > Alan Cox wrote:
> >>> If you are talking about SATA -- incorrect.
> >>>
> >>> The patch deals with policy, and the user MUST have the ability to 
> >>> control this stuff.  Otherwise you create a situation where the user 
> >>> might be denied hotplug use in valid cases, or similar negative 
> >>> situations.
> >>
> >> The policy isn't however complicated. Tejun added the stuff for forcing
> >> cable type and mode on setup and has therefore written all the per device
> >> setup code we might need. Alternatively a single
> >>
> >>     foo=1/0
> >>
> >> option has been fine for acpi and will do fine for this. Total additional
> >> cost - 1 line.
> > 
> > The key requirement is per-port control.  Ideally via hdparm or another 
> > userspace tool, but kernel command line (module options) or sysfs would 
> > be just fine too.  And agreed, the minimal you need is simply 1/0 for 
> > the port's policy.
> ..
> 
> Btw.. hdparm-8.7 (unreleased) can grok /sys now, so that interface is
> as good as any from a userspace viewpoint now.
> 
> For the power-off of unused ports, the current patch still sounds
> extremely vendor-specific (Intel).

Wrong - this patch is implemented according to the AHCI spec and
has absolutely nothing vendor specific in it.

> 
> Does it actually work (demonstrate, please) on any other hardware ?

If someone would like to test it on another AHCI compliant chipset
that would be great.  I have none.

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Mon, 02 Jun 2008 12:07:02 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Mark Lord wrote:
> > For the power-off of unused ports, the current patch still sounds
> > extremely vendor-specific (Intel).
> > 
> > Does it actually work (demonstrate, please) on any other hardware ?
> 
> Ding...  correct.  We need something per-port that's not Intel specific.
> 
> 	Jeff
> 
> 
> 

Can you tell me what part of this patch is vendor specific?  Maybe
I am misunderstanding what you are saying.  Here's that patch again
in case you need to look at it again.

power off unused ports

If the port isn't either a drive bay or an external SATA port, do not 
keep the phy powered on if the port is unoccupied. This saves .75 watts
on most laptops.  A module parameter can be used to override this 
behavior and allow the phy's to be powered up regardless of whether it
has externally accessible SATA ports. 

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

---
 drivers/ata/ahci.c        |   21 +++++++++++++++++++++
 drivers/ata/libata-core.c |   24 ++++++++++++++++++++++++
 include/linux/libata.h    |    1 +
 3 files changed, 46 insertions(+)

Index: linux-ahci-phy/drivers/ata/ahci.c
===================================================================
--- linux-ahci-phy.orig/drivers/ata/ahci.c	2008-05-08 14:29:02.000000000 -0700
+++ linux-ahci-phy/drivers/ata/ahci.c	2008-05-08 14:31:05.000000000 -0700
@@ -53,9 +53,13 @@ static int ahci_skip_host_reset;
 module_param_named(skip_host_reset, ahci_skip_host_reset, int, 0444);
 MODULE_PARM_DESC(skip_host_reset, "skip global host reset (0=don't skip, 1=skip)");
 
+static int ahci_power_save = 1;
+module_param_named(power_save, ahci_power_save, int, 0444);
+MODULE_PARM_DESC(power_save, "Power off unused ports (0=don't power off, 1=power off)");
 static int ahci_enable_alpm(struct ata_port *ap,
 		enum link_pm policy);
 static void ahci_disable_alpm(struct ata_port *ap);
+static int ahci_is_hotplug_capable(struct ata_port *ap);
 
 enum {
 	AHCI_PCI_BAR		= 5,
@@ -166,6 +170,8 @@ enum {
 	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_ESP		= (1 << 21), /* External SATA Port */
+	PORT_CMD_HPCP		= (1 << 18), /* port is hot plug capable */
 	PORT_CMD_PMP		= (1 << 17), /* PMP attached */
 	PORT_CMD_LIST_ON	= (1 << 15), /* cmd list DMA engine running */
 	PORT_CMD_FIS_ON		= (1 << 14), /* FIS DMA engine running */
@@ -1900,6 +1906,18 @@ static int ahci_pci_device_resume(struct
 }
 #endif
 
+static int ahci_is_hotplug_capable(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u8 cmd;
+
+	if (!ahci_power_save)
+		return 1;
+
+	cmd = readl(port_mmio + PORT_CMD);
+	return ((cmd & PORT_CMD_HPCP) || (cmd & PORT_CMD_ESP));
+}
+
 static int ahci_port_start(struct ata_port *ap)
 {
 	struct device *dev = ap->host->dev;
@@ -1951,6 +1969,9 @@ static int ahci_port_start(struct ata_po
 
 	ap->private_data = pp;
 
+	/* set some flags based on port capabilities */
+	if (!ahci_is_hotplug_capable(ap))
+		ap->flags |= ATA_FLAG_NO_HOTPLUG;
 	/* engage engines, captain */
 	return ahci_port_resume(ap);
 }
Index: linux-ahci-phy/drivers/ata/libata-core.c
===================================================================
--- linux-ahci-phy.orig/drivers/ata/libata-core.c	2008-05-08 14:28:57.000000000 -0700
+++ linux-ahci-phy/drivers/ata/libata-core.c	2008-05-08 14:29:50.000000000 -0700
@@ -162,6 +162,19 @@ MODULE_DESCRIPTION("Library module for A
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
+static void ata_phy_offline(struct ata_link *link)
+{
+	u32 scontrol;
+	int rc;
+
+	/* set DET to 4 */
+	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
+	if (rc)
+		return;
+	scontrol &= ~0xf;
+	scontrol |= (1 << 2);
+	sata_scr_write(link, SCR_CONTROL, scontrol);
+}
 
 /**
  *	ata_force_cbl - force cable type according to libata.force
@@ -2671,6 +2684,7 @@ void ata_port_disable(struct ata_port *a
 	ap->link.device[0].class = ATA_DEV_NONE;
 	ap->link.device[1].class = ATA_DEV_NONE;
 	ap->flags |= ATA_FLAG_DISABLED;
+	ata_phy_offline(&ap->link);
 }
 
 /**
@@ -5609,6 +5623,8 @@ int ata_host_register(struct ata_host *h
 		if (ap->ops->error_handler) {
 			struct ata_eh_info *ehi = &ap->link.eh_info;
 			unsigned long flags;
+			int device_attached = 0;
+			struct ata_device *dev;
 
 			ata_port_probe(ap);
 
@@ -5627,6 +5643,14 @@ int ata_host_register(struct ata_host *h
 
 			/* wait for EH to finish */
 			ata_port_wait_eh(ap);
+			ata_link_for_each_dev(dev, &ap->link)
+				if (ata_dev_enabled(dev))
+					device_attached++;
+			if (!device_attached &&
+			    (ap->flags & ATA_FLAG_NO_HOTPLUG)) {
+				/* no device present, disable port */
+				ata_port_disable(ap);
+			}
 		} else {
 			DPRINTK("ata%u: bus probe begin\n", ap->print_id);
 			rc = ata_bus_probe(ap);
Index: linux-ahci-phy/include/linux/libata.h
===================================================================
--- linux-ahci-phy.orig/include/linux/libata.h	2008-05-08 14:28:57.000000000 -0700
+++ linux-ahci-phy/include/linux/libata.h	2008-05-08 14:29:50.000000000 -0700
@@ -193,6 +193,7 @@ enum {
 	ATA_FLAG_AN		= (1 << 18), /* controller supports AN */
 	ATA_FLAG_PMP		= (1 << 19), /* controller supports PMP */
 	ATA_FLAG_IPM		= (1 << 20), /* driver can handle IPM */
+	ATA_FLAG_NO_HOTPLUG	= (1 << 21), /* port doesn't support HP */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be

Re: [PATCH] ata: ahci: power off unused ports

[Greg Freemyer]

On Mon, Jun 2, 2008 at 12:07 PM, Jeff Garzik <jeff@garzik.org> wrote:
> Mark Lord wrote:
>>
>> For the power-off of unused ports, the current patch still sounds
>> extremely vendor-specific (Intel).
>>
>> Does it actually work (demonstrate, please) on any other hardware ?
>
> Ding...  correct.  We need something per-port that's not Intel specific.
>
>        Jeff

But, isn't Alan correct that the new logic is useful to some and with
a simple boot option is can be used to enable / disable it.

So if it is defaulted it to disabled, and the end user is allowed to
have a boot option to enable it, you get a reasonable first step for
now.

Greg
-- 
Greg Freemyer
Litigation Triage Solutions Specialist
http://www.linkedin.com/in/gregfreemyer
First 99 Days Litigation White Paper -
http://www.norcrossgroup.com/forms/whitepapers/99%20Days%20whitepaper.pdf

The Norcross Group
The Intersection of Evidence & Technology
http://www.norcrossgroup.com

Re: [PATCH] ata: ahci: power off unused ports

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> On Mon, 02 Jun 2008 09:03:04 -0400
> Mark Lord <liml@rtr.ca> wrote:
> 
>> Jeff Garzik wrote:
>>> Alan Cox wrote:
>>>>> If you are talking about SATA -- incorrect.
>>>>>
>>>>> The patch deals with policy, and the user MUST have the ability to 
>>>>> control this stuff.  Otherwise you create a situation where the user 
>>>>> might be denied hotplug use in valid cases, or similar negative 
>>>>> situations.
>>>> The policy isn't however complicated. Tejun added the stuff for forcing
>>>> cable type and mode on setup and has therefore written all the per device
>>>> setup code we might need. Alternatively a single
>>>>
>>>>     foo=1/0
>>>>
>>>> option has been fine for acpi and will do fine for this. Total additional
>>>> cost - 1 line.
>>> The key requirement is per-port control.  Ideally via hdparm or another 
>>> userspace tool, but kernel command line (module options) or sysfs would 
>>> be just fine too.  And agreed, the minimal you need is simply 1/0 for 
>>> the port's policy.
>> ..
>>
>> Btw.. hdparm-8.7 (unreleased) can grok /sys now, so that interface is
>> as good as any from a userspace viewpoint now.
>>
>> For the power-off of unused ports, the current patch still sounds
>> extremely vendor-specific (Intel).
> 
> Wrong - this patch is implemented according to the AHCI spec and
> has absolutely nothing vendor specific in it.

ITHM AHCI-specific, and not usable by non-AHCI SATA controllers -- which 
we would want in any solution.

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> Can you tell me what part of this patch is vendor specific?  Maybe
> I am misunderstanding what you are saying.  Here's that patch again
> in case you need to look at it again.


It's specific to AHCI, and does not cover other controllers.

Certainly PORT_CMD_HPCP must be, but the concept itself is not at all 
AHCI-specific.

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Mon, 02 Jun 2008 13:45:15 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Kristen Carlson Accardi wrote:
> > Can you tell me what part of this patch is vendor specific?  Maybe
> > I am misunderstanding what you are saying.  Here's that patch again
> > in case you need to look at it again.
> 
> 
> It's specific to AHCI, and does not cover other controllers.
> 
> Certainly PORT_CMD_HPCP must be, but the concept itself is not at all 
> AHCI-specific.
> 
> 	Jeff
> 
> 


@@ -5627,6 +5643,14 @@ int ata_host_register(struct ata_host *h

                        /* wait for EH to finish */
                        ata_port_wait_eh(ap);
+                       ata_link_for_each_dev(dev, &ap->link)
+                               if (ata_dev_enabled(dev))
+                                       device_attached++;
+                       if (!device_attached &&
+                           (ap->flags & ATA_FLAG_NO_HOTPLUG)) {
+                               /* no device present, disable port */
+                               ata_port_disable(ap);
+                       }
                } else {

Here you can see that it would be very easy for another driver
to just add code to set the NO_HOTPLUG flag and then this code
will work for them as well, since we power off the phy using
DET which is specified by SATA.

here's that code:
+static void ata_phy_offline(struct ata_link *link)
+{
+       u32 scontrol;
+       int rc;
+
+       /* set DET to 4 */
+       rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
+       if (rc)
+               return;
+       scontrol &= ~0xf;
+       scontrol |= (1 << 2);
+       sata_scr_write(link, SCR_CONTROL, scontrol);
+}

while it's true that I only modified ahci.c to set
the flag, any other driver writer can do that for the
other drivers based on whatever they want to use to
determine whether to set the NO_HOTPLUG flag.

Re: [PATCH] ata: ahci: power off unused ports

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> Here you can see that it would be very easy for another driver
> to just add code to set the NO_HOTPLUG flag and then this code
> will work for them as well, since we power off the phy using
> DET which is specified by SATA.

> here's that code:

Agreed.  The main discussion in this sub-thread is more about user 
interface.  The user interface in this case, a module option, is 
specific to AHCI.

Surely you can see how it is a bit silly to force each driver writer to 
create the same user interface in each driver, to support a generic concept.

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Mon, 02 Jun 2008 14:15:27 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Kristen Carlson Accardi wrote:
> > Here you can see that it would be very easy for another driver
> > to just add code to set the NO_HOTPLUG flag and then this code
> > will work for them as well, since we power off the phy using
> > DET which is specified by SATA.
> 
> > here's that code:
> 
> Agreed.  The main discussion in this sub-thread is more about user 
> interface.  The user interface in this case, a module option, is 
> specific to AHCI.
> 
> Surely you can see how it is a bit silly to force each driver writer to 
> create the same user interface in each driver, to support a generic concept.
> 
> 	Jeff
> 
> 
Not all drivers will need a user interface to turn off hotplug
I would think.  At any rate - I would think it'd be better to let
driver writers decide how they want their drivers to behave wrt
hotplug and power instead of forcing a generic policy on everyone.

This patch would provide users of AHCI controllers a way to save
power now, while you work on the grand scheme for polling/turning on off
hotplug via sysfs.  It's an interim solution that impacts nobody but
ahci users and is can be easily integrated into whatever solution you
eventually work out.

Re: [PATCH] ata: ahci: power off unused ports

[Ric Wheeler]

Kristen Carlson Accardi wrote:
> On Mon, 02 Jun 2008 14:15:27 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
>
>   
>> Kristen Carlson Accardi wrote:
>>     
>>> Here you can see that it would be very easy for another driver
>>> to just add code to set the NO_HOTPLUG flag and then this code
>>> will work for them as well, since we power off the phy using
>>> DET which is specified by SATA.
>>>       
>>> here's that code:
>>>       
>> Agreed.  The main discussion in this sub-thread is more about user 
>> interface.  The user interface in this case, a module option, is 
>> specific to AHCI.
>>
>> Surely you can see how it is a bit silly to force each driver writer to 
>> create the same user interface in each driver, to support a generic concept.
>>
>> 	Jeff
>>
>>
>>     
> Not all drivers will need a user interface to turn off hotplug
> I would think.  At any rate - I would think it'd be better to let
> driver writers decide how they want their drivers to behave wrt
> hotplug and power instead of forcing a generic policy on everyone.
>
> This patch would provide users of AHCI controllers a way to save
> power now, while you work on the grand scheme for polling/turning on off
> hotplug via sysfs.  It's an interim solution that impacts nobody but
> ahci users and is can be easily integrated into whatever solution you
> eventually work out.
>   
I like the patch -  it seems that it will help a lot of users out near 
term in a very positive way while we iterate on the broader solution,

ric

Re: [PATCH] ata: ahci: power off unused ports

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> On Mon, 02 Jun 2008 14:15:27 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> Kristen Carlson Accardi wrote:
>>> Here you can see that it would be very easy for another driver
>>> to just add code to set the NO_HOTPLUG flag and then this code
>>> will work for them as well, since we power off the phy using
>>> DET which is specified by SATA.
>>> here's that code:
>> Agreed.  The main discussion in this sub-thread is more about user 
>> interface.  The user interface in this case, a module option, is 
>> specific to AHCI.
>>
>> Surely you can see how it is a bit silly to force each driver writer to 
>> create the same user interface in each driver, to support a generic concept.
>>
>> 	Jeff
>>
>>
> Not all drivers will need a user interface to turn off hotplug
> I would think.  At any rate - I would think it'd be better to let
> driver writers decide how they want their drivers to behave wrt
> hotplug and power instead of forcing a generic policy on everyone.
> 
> This patch would provide users of AHCI controllers a way to save
> power now, while you work on the grand scheme for polling/turning on off
> hotplug via sysfs.  It's an interim solution that impacts nobody but
> ahci users and is can be easily integrated into whatever solution you
> eventually work out.

It's a one-off driver-specific interface that will have to be supported 
even after the same feature is available via libata core...  that's not 
the path to scalable, sustainable engineering.

I know user interfaces are annoying because you have to think about 
chips other than your own, but that's life.  Other hardware vendors have 
to do it too.

Letting each driver have a different user interface is /unfriendly/ to 
both developers users.  It's easiest for Intel kernel developers, but 
that is not our target audience :)

The biggest power savings for the largest amount of users can be had if 
you take a moment and figure out what's best for Linux, rather than what 
is best for Intel.

Because you can be damned sure SATA users with non-AHCI chips want this 
power savings too...  let's not put roadblocks to that in place in the 
beginning (by adding one-off interfaces).

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Jeff Garzik]

Ric Wheeler wrote:
> Kristen Carlson Accardi wrote:
>> Not all drivers will need a user interface to turn off hotplug
>> I would think.  At any rate - I would think it'd be better to let
>> driver writers decide how they want their drivers to behave wrt
>> hotplug and power instead of forcing a generic policy on everyone.
>>
>> This patch would provide users of AHCI controllers a way to save
>> power now, while you work on the grand scheme for polling/turning on off
>> hotplug via sysfs.  It's an interim solution that impacts nobody but
>> ahci users and is can be easily integrated into whatever solution you
>> eventually work out.

> I like the patch -  it seems that it will help a lot of users out near 
> term in a very positive way while we iterate on the broader solution,

A better patch would enable the _possibility_ of power savings on 
non-AHCI chips, and not add a one-off AHCI-specific user interface that 
must be supported for years to come.

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Ric Wheeler]

Jeff Garzik wrote:
> Ric Wheeler wrote:
>> Kristen Carlson Accardi wrote:
>>> Not all drivers will need a user interface to turn off hotplug
>>> I would think.  At any rate - I would think it'd be better to let
>>> driver writers decide how they want their drivers to behave wrt
>>> hotplug and power instead of forcing a generic policy on everyone.
>>>
>>> This patch would provide users of AHCI controllers a way to save
>>> power now, while you work on the grand scheme for polling/turning on 
>>> off
>>> hotplug via sysfs.  It's an interim solution that impacts nobody but
>>> ahci users and is can be easily integrated into whatever solution you
>>> eventually work out.
>
>> I like the patch -  it seems that it will help a lot of users out 
>> near term in a very positive way while we iterate on the broader 
>> solution,
>
> A better patch would enable the _possibility_ of power savings on 
> non-AHCI chips, and not add a one-off AHCI-specific user interface 
> that must be supported for years to come.
>
>     Jeff
I think that the patch does that first part pretty reasonably for all 
chips as Kristen explained before.

If the user interface is the only obstacle and the base code seems to be 
flexible enough for any device to take advantage of, it would still seem 
to be a positive step forward to put in that base functionality (even 
without the module param) to enable power saving.

It is always good to get the complete solution in (how hard can the user 
interface be to code once we agree on what it should be ;-)), but this 
seems to be a good first step.

ric

Re: [PATCH] ata: ahci: power off unused ports

[Jeff Garzik]

Ric Wheeler wrote:
> Jeff Garzik wrote:
>> Ric Wheeler wrote:
>>> Kristen Carlson Accardi wrote:
>>>> Not all drivers will need a user interface to turn off hotplug
>>>> I would think.  At any rate - I would think it'd be better to let
>>>> driver writers decide how they want their drivers to behave wrt
>>>> hotplug and power instead of forcing a generic policy on everyone.
>>>>
>>>> This patch would provide users of AHCI controllers a way to save
>>>> power now, while you work on the grand scheme for polling/turning on 
>>>> off
>>>> hotplug via sysfs.  It's an interim solution that impacts nobody but
>>>> ahci users and is can be easily integrated into whatever solution you
>>>> eventually work out.
>>
>>> I like the patch -  it seems that it will help a lot of users out 
>>> near term in a very positive way while we iterate on the broader 
>>> solution,
>>
>> A better patch would enable the _possibility_ of power savings on 
>> non-AHCI chips, and not add a one-off AHCI-specific user interface 
>> that must be supported for years to come.
>>
>>     Jeff
> I think that the patch does that first part pretty reasonably for all 
> chips as Kristen explained before.
> 
> If the user interface is the only obstacle and the base code seems to be 
> flexible enough for any device to take advantage of, it would still seem 
> to be a positive step forward to put in that base functionality (even 
> without the module param) to enable power saving.
> 
> It is always good to get the complete solution in (how hard can the user 
> interface be to code once we agree on what it should be ;-)), but this 
> seems to be a good first step.

A prep step without the module param (ie. user interface) is fine. 
That's how we deployed Async Notification.  Kristen's AN stuff went into 
libata well before the SCSI API was settled upon.

	Jeff

Re: [PATCH] ata: ahci: power off unused ports

[Matthew Garrett]

On Mon, Jun 02, 2008 at 02:40:51PM -0400, Jeff Garzik wrote:

> A better patch would enable the _possibility_ of power savings on 
> non-AHCI chips, and not add a one-off AHCI-specific user interface that 
> must be supported for years to come.

So how about somethig like the following, where the module parameter 
just gets moved to libata rather than ahci? Entirely untested, I don't 
have ahci on this machine.

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 97f83fb..5913ebb 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -56,6 +56,7 @@ MODULE_PARM_DESC(skip_host_reset, "skip global host reset (0=don't skip, 1=skip)
 static int ahci_enable_alpm(struct ata_port *ap,
 		enum link_pm policy);
 static void ahci_disable_alpm(struct ata_port *ap);
+static int ahci_is_hotplug_capable(struct ata_port *ap);
 
 enum {
 	AHCI_PCI_BAR		= 5,
@@ -166,6 +167,8 @@ enum {
 	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_ESP		= (1 << 21), /* External SATA Port */
+	PORT_CMD_HPCP		= (1 << 18), /* port is hot plug capable */
 	PORT_CMD_PMP		= (1 << 17), /* PMP attached */
 	PORT_CMD_LIST_ON	= (1 << 15), /* cmd list DMA engine running */
 	PORT_CMD_FIS_ON		= (1 << 14), /* FIS DMA engine running */
@@ -1900,6 +1903,15 @@ static int ahci_pci_device_resume(struct pci_dev *pdev)
 }
 #endif
 
+static int ahci_is_hotplug_capable(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u8 cmd;
+
+	cmd = readl(port_mmio + PORT_CMD);
+	return ((cmd & PORT_CMD_HPCP) || (cmd & PORT_CMD_ESP));
+}
+
 static int ahci_port_start(struct ata_port *ap)
 {
 	struct device *dev = ap->host->dev;
@@ -1951,6 +1963,9 @@ static int ahci_port_start(struct ata_port *ap)
 
 	ap->private_data = pp;
 
+	/* set some flags based on port capabilities */
+	if (!ahci_is_hotplug_capable(ap))
+		ap->flags |= ATA_FLAG_NO_HOTPLUG;
 	/* engage engines, captain */
 	return ahci_port_resume(ap);
 }
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3c89f20..209efe5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -157,11 +157,28 @@ int libata_allow_tpm = 0;
 module_param_named(allow_tpm, libata_allow_tpm, int, 0444);
 MODULE_PARM_DESC(allow_tpm, "Permit the use of TPM commands");
 
+static int sata_power_save = 1;
+module_param_named(sata_power_save, sata_power_save, int, 0644);
+MODULE_PARM_DESC(sata_power_save, "Power off unused ports (0=don't power off, 1=power off)");
+
 MODULE_AUTHOR("Jeff Garzik");
 MODULE_DESCRIPTION("Library module for ATA devices");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
+static void ata_phy_offline(struct ata_link *link)
+{
+	u32 scontrol;
+	int rc;
+
+	/* set DET to 4 */
+	rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
+	if (rc)
+		return;
+	scontrol &= ~0xf;
+	scontrol |= (1 << 2);
+	sata_scr_write(link, SCR_CONTROL, scontrol);
+}
 
 /**
  *	ata_force_cbl - force cable type according to libata.force
@@ -2678,6 +2695,7 @@ void ata_port_disable(struct ata_port *ap)
 	ap->link.device[0].class = ATA_DEV_NONE;
 	ap->link.device[1].class = ATA_DEV_NONE;
 	ap->flags |= ATA_FLAG_DISABLED;
+	ata_phy_offline(&ap->link);
 }
 
 /**
@@ -5614,6 +5632,8 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 		if (ap->ops->error_handler) {
 			struct ata_eh_info *ehi = &ap->link.eh_info;
 			unsigned long flags;
+			int device_attached = 0;
+			struct ata_device *dev;
 
 			ata_port_probe(ap);
 
@@ -5632,6 +5652,14 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 
 			/* wait for EH to finish */
 			ata_port_wait_eh(ap);
+			ata_link_for_each_dev(dev, &ap->link)
+				if (ata_dev_enabled(dev))
+					device_attached++;
+			if (!device_attached && sata_power_save &&
+			    (ap->flags & ATA_FLAG_NO_HOTPLUG)) {
+				/* no device present, disable port */
+				ata_port_disable(ap);
+			}
 		} else {
 			DPRINTK("ata%u: bus probe begin\n", ap->print_id);
 			rc = ata_bus_probe(ap);

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

Re: [PATCH] ata: ahci: power off unused ports

[Kristen Carlson Accardi]

On Mon, 02 Jun 2008 14:38:55 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Kristen Carlson Accardi wrote:
> > On Mon, 02 Jun 2008 14:15:27 -0400
> > Jeff Garzik <jeff@garzik.org> wrote:
> > 
> >> Kristen Carlson Accardi wrote:
> >>> Here you can see that it would be very easy for another driver
> >>> to just add code to set the NO_HOTPLUG flag and then this code
> >>> will work for them as well, since we power off the phy using
> >>> DET which is specified by SATA.
> >>> here's that code:
> >> Agreed.  The main discussion in this sub-thread is more about user 
> >> interface.  The user interface in this case, a module option, is 
> >> specific to AHCI.
> >>
> >> Surely you can see how it is a bit silly to force each driver writer to 
> >> create the same user interface in each driver, to support a generic concept.
> >>
> >> 	Jeff
> >>
> >>
> > Not all drivers will need a user interface to turn off hotplug
> > I would think.  At any rate - I would think it'd be better to let
> > driver writers decide how they want their drivers to behave wrt
> > hotplug and power instead of forcing a generic policy on everyone.
> > 
> > This patch would provide users of AHCI controllers a way to save
> > power now, while you work on the grand scheme for polling/turning on off
> > hotplug via sysfs.  It's an interim solution that impacts nobody but
> > ahci users and is can be easily integrated into whatever solution you
> > eventually work out.
> 
> It's a one-off driver-specific interface that will have to be supported 
> even after the same feature is available via libata core...  that's not 
> the path to scalable, sustainable engineering.

Jeff - I think you are misunderstanding what the module parameter 
is doing.  It is not a substitute for the sysfs interface that you
proposed.  You would not want to get rid of it ever.  What it does
is set the default of the AHCI driver to favor the BIOS settings
for which ports are hotpluggable.  the parameter is there to allow
users to override the BIOS settings at module load time.  Once you
get your sysfs interface in, you would then be able to override
the default at run time.  

> 
> I know user interfaces are annoying because you have to think about 
> chips other than your own, but that's life.  Other hardware vendors have 
> to do it too.

User interfaces are annoying because it takes 2 maintainers to agree
on them, that is all.  Surely you remember how long it took AN to
get in?  It was about 9 months.  This patch can go in now while we
spend 9 months deciding how to do the ultimate sysfs interface.

> 
> Letting each driver have a different user interface is /unfriendly/ to 
> both developers users.  It's easiest for Intel kernel developers, but 
> that is not our target audience :)
> 
> The biggest power savings for the largest amount of users can be had if 
> you take a moment and figure out what's best for Linux, rather than what 
> is best for Intel.
> 
> Because you can be damned sure SATA users with non-AHCI chips want this 
> power savings too...  let's not put roadblocks to that in place in the 
> beginning (by adding one-off interfaces).

As I've been trying to explain to you.  a) this isn't a one off interface.
b) this patch doesn't prevent you at all from adding a generic interface
later.  Perhaps you have a non-technical reason for not wanting this
patch, but I'd like to make sure there's nothing here that you are
just not understanding or something that I can fix.  I obviously can't
fix your idea that I only care about Intel chips, not matter how utterly
wrong and not supported by the facts that is.