| Message ID | 1299263051-3739-1-git-send-email-kristen@linux.intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello, On Fri, Mar 04, 2011 at 10:24:11AM -0800, Kristen Carlson Accardi wrote: > case ATA_LPM_MIN_POWER: > - /* no restrictions on LPM transitions */ > - scontrol &= ~(0x3 << 8); > + if (ata_link_nr_enabled(link) > 0) > + /* no restrictions on LPM transitions */ > + scontrol &= ~(0x3 << 8); > + else { > + /* empty port, power off */ > + scontrol &= ~0xf; > + scontrol |= (0x1 << 2); > + } Why not just do the following? scontrol &= ~(0x3 << 8); /* if empty, power off */ if (!ata_link_nr_enabled(link)) { scontrol &= ~0xf; scontrol |= 0x1 << 2; } Also, can you please provide some details on on which hardware you tested the change and how much power it actually saved? Thanks.
On Fri, 4 Mar 2011 19:38:36 +0100 Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Fri, Mar 04, 2011 at 10:24:11AM -0800, Kristen Carlson Accardi wrote: > > case ATA_LPM_MIN_POWER: > > - /* no restrictions on LPM transitions */ > > - scontrol &= ~(0x3 << 8); > > + if (ata_link_nr_enabled(link) > 0) > > + /* no restrictions on LPM transitions */ > > + scontrol &= ~(0x3 << 8); > > + else { > > + /* empty port, power off */ > > + scontrol &= ~0xf; > > + scontrol |= (0x1 << 2); > > + } > > Why not just do the following? > > scontrol &= ~(0x3 << 8); because, the IPM value is meaningless if we have no devices, so this is a wasted operation. > /* if empty, power off */ > if (!ata_link_nr_enabled(link)) { > scontrol &= ~0xf; > scontrol |= 0x1 << 2; > } > > Also, can you please provide some details on on which hardware you > tested the change and how much power it actually saved? > This change was tested on Intel's Sandy Bridge platform. Your power savings will depend on how many implemented ports you have, and how many are not being used. For a system I tested with which had 2 ports which I could turn off, I saved 300mW. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Fri, Mar 04, 2011 at 10:47:56AM -0800, Kristen Carlson Accardi wrote: > > Why not just do the following? > > > > scontrol &= ~(0x3 << 8); > > because, the IPM value is meaningless if we have no devices, > so this is a wasted operation. What? You're trying to save one AND operation to a value which is already in register in libata powersave path? That's wrong on so many levels (it's not even a proper optimization, think about it). Just make it easier on the eyes. > This change was tested on Intel's Sandy Bridge platform. Your power > savings will depend on how many implemented ports you have, and how > many are not being used. For a system I tested with which had 2 ports > which I could turn off, I saved 300mW. Can you please test at least on a couple different hardware and make sure that the port can be brought back after put to sleep? ie. Verify that hotplug still works after min_power -> max_performance? Also, please add the test results in the patch description. Thanks.
On Sat, 5 Mar 2011 09:01:58 +0100 Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Fri, Mar 04, 2011 at 10:47:56AM -0800, Kristen Carlson Accardi wrote: > > > Why not just do the following? > > > > > > scontrol &= ~(0x3 << 8); > > > > because, the IPM value is meaningless if we have no devices, > > so this is a wasted operation. > > What? You're trying to save one AND operation to a value which is > already in register in libata powersave path? That's wrong on so many > levels (it's not even a proper optimization, think about it). Just > make it easier on the eyes. debating this is even more of a waste of an operation, so I'll be glad to do this whatever way suites your taste. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 07, 2011 at 09:37:42AM -0800, Kristen Carlson Accardi wrote: > debating this is even more of a waste of an operation, so I'll be glad > to do this whatever way suites your taste. Great, thank you.
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index d4e52e2..a650bef 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -3619,8 +3619,14 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy, scontrol |= (0x2 << 8); break; case ATA_LPM_MIN_POWER: - /* no restrictions on LPM transitions */ - scontrol &= ~(0x3 << 8); + if (ata_link_nr_enabled(link) > 0) + /* no restrictions on LPM transitions */ + scontrol &= ~(0x3 << 8); + else { + /* empty port, power off */ + scontrol &= ~0xf; + scontrol |= (0x1 << 2); + } break; default: WARN_ON(1); diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 17a6378..f0a31b7 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -3381,7 +3381,7 @@ fail: return rc; } -static int ata_link_nr_enabled(struct ata_link *link) +int ata_link_nr_enabled(struct ata_link *link) { struct ata_device *dev; int cnt = 0; diff --git a/include/linux/libata.h b/include/linux/libata.h index c9c5d7a..8e78052 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1155,6 +1155,7 @@ extern void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset, ata_reset_fn_t softreset, ata_reset_fn_t hardreset, ata_postreset_fn_t postreset); extern void ata_std_error_handler(struct ata_port *ap); +extern int ata_link_nr_enabled(struct ata_link *link); /* * Base operations to inherit from and initializers for sht
Give users the option of completely powering off unoccupied SATA ports using the existing min_power link_power_management_policy option. When the use selects this option on an empty port, we will power the port off by setting DET to off. For occupied ports, behavior is unchanged. Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> --- drivers/ata/libata-core.c | 10 ++++++++-- drivers/ata/libata-eh.c | 2 +- include/linux/libata.h | 1 + 3 files changed, 10 insertions(+), 3 deletions(-)