* [RFC PATCH] AHCI: speed up resume
@ 2008-07-03 8:48 Zhang Rui
2008-07-03 23:23 ` Rafael J. Wysocki
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Zhang Rui @ 2008-07-03 8:48 UTC (permalink / raw)
To: linux-pm, linux-kernel; +Cc: tj
During S3 resume, AHCI driver sleeps 1 second to wait for the HBA reset
to finish. This is luxurious, :)
According to the AHCI 1.2 spec, We should poll the HOST_CTL register,
and return error if the host reset is not finished within 1 second.
Test results show that the HBA reset can be done quickly(in usecs).
And this patch may save nearly 1 second during resume.
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
--
drivers/ata/ahci.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
Index: linux-2.6/drivers/ata/ahci.c
===================================================================
--- linux-2.6.orig/drivers/ata/ahci.c 2007-05-03 11:06:33.000000000 +0800
+++ linux-2.6/drivers/ata/ahci.c 2008-07-02 16:25:54.000000000 +0800
@@ -1073,18 +1073,29 @@
/* global controller reset */
if (!ahci_skip_host_reset) {
+ int delay = msecs_to_jiffies(1000);
+ int timeout;
+
tmp = readl(mmio + HOST_CTL);
if ((tmp & HOST_RESET) == 0) {
writel(tmp | HOST_RESET, mmio + HOST_CTL);
readl(mmio + HOST_CTL); /* flush */
}
- /* reset must complete within 1 second, or
+ /*
+ * to perform host reset, OS should set HOST_RESET
+ * and poll until this bit is read to be "0"
+ * reset must complete within 1 second, or
* the hardware should be considered fried.
*/
- ssleep(1);
+ timeout = jiffies + delay;
+ while (jiffies < timeout) {
+ tmp = readl(mmio + HOST_CTL);
+ if (!(tmp & HOST_RESET))
+ break;
+ cpu_relax();
+ }
- tmp = readl(mmio + HOST_CTL);
if (tmp & HOST_RESET) {
dev_printk(KERN_ERR, host->dev,
"controller reset failed (0x%x)\n", tmp);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] AHCI: speed up resume
2008-07-03 8:48 [RFC PATCH] AHCI: speed up resume Zhang Rui
@ 2008-07-03 23:23 ` Rafael J. Wysocki
[not found] ` <200807040123.50337.rjw@sisk.pl>
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2008-07-03 23:23 UTC (permalink / raw)
To: Zhang Rui; +Cc: tj, linux-pm, linux-kernel
On Thursday, 3 of July 2008, Zhang Rui wrote:
> During S3 resume, AHCI driver sleeps 1 second to wait for the HBA reset
> to finish. This is luxurious, :)
>
> According to the AHCI 1.2 spec, We should poll the HOST_CTL register,
> and return error if the host reset is not finished within 1 second.
>
> Test results show that the HBA reset can be done quickly(in usecs).
> And this patch may save nearly 1 second during resume.
That's a lot.
How heavily has it been tested?
Rafael
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> --
> drivers/ata/ahci.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> Index: linux-2.6/drivers/ata/ahci.c
> ===================================================================
> --- linux-2.6.orig/drivers/ata/ahci.c 2007-05-03 11:06:33.000000000 +0800
> +++ linux-2.6/drivers/ata/ahci.c 2008-07-02 16:25:54.000000000 +0800
> @@ -1073,18 +1073,29 @@
>
> /* global controller reset */
> if (!ahci_skip_host_reset) {
> + int delay = msecs_to_jiffies(1000);
> + int timeout;
> +
> tmp = readl(mmio + HOST_CTL);
> if ((tmp & HOST_RESET) == 0) {
> writel(tmp | HOST_RESET, mmio + HOST_CTL);
> readl(mmio + HOST_CTL); /* flush */
> }
>
> - /* reset must complete within 1 second, or
> + /*
> + * to perform host reset, OS should set HOST_RESET
> + * and poll until this bit is read to be "0"
> + * reset must complete within 1 second, or
> * the hardware should be considered fried.
> */
> - ssleep(1);
> + timeout = jiffies + delay;
> + while (jiffies < timeout) {
> + tmp = readl(mmio + HOST_CTL);
> + if (!(tmp & HOST_RESET))
> + break;
> + cpu_relax();
> + }
>
> - tmp = readl(mmio + HOST_CTL);
> if (tmp & HOST_RESET) {
> dev_printk(KERN_ERR, host->dev,
> "controller reset failed (0x%x)\n", tmp);
>
>
>
>
--
"Premature optimization is the root of all evil." - Donald Knuth
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] AHCI: speed up resume
[not found] ` <200807040123.50337.rjw@sisk.pl>
@ 2008-07-04 1:48 ` Zhang Rui
0 siblings, 0 replies; 7+ messages in thread
From: Zhang Rui @ 2008-07-04 1:48 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: tj, linux-pm, linux-kernel
On Fri, 2008-07-04 at 07:23 +0800, Rafael J. Wysocki wrote:
> On Thursday, 3 of July 2008, Zhang Rui wrote:
> > During S3 resume, AHCI driver sleeps 1 second to wait for the HBA
> reset
> > to finish. This is luxurious, :)
> >
> > According to the AHCI 1.2 spec, We should poll the HOST_CTL
> register,
> > and return error if the host reset is not finished within 1 second.
> >
> > Test results show that the HBA reset can be done quickly(in usecs).
> > And this patch may save nearly 1 second during resume.
>
> That's a lot.
>
> How heavily has it been tested?
I only tested it on a SantaRosa laptop.
w/o the patch,
[ 469.923186] ahci_pci_device_resume
[ 469.936105] PM: Writing back config space on device 0000:00:1f.2 at
offset f (was 200, writing 20a)
[ 469.936145] PM: Writing back config space on device 0000:00:1f.2 at
offset 1 (was 2b00007, writing 2b00407)
[ 469.936220] PCI: Setting latency timer of device 0000:00:1f.2 to 64
[ 470.092053] ata4.00: ACPI cmd ef/03:0c:00:00:00:a0 filtered out
[ 470.092053] ata4.00: ACPI cmd ef/03:42:00:00:00:a0 filtered out
[ 470.107675] ata4.00: configured for UDMA/33
[ 470.942195] ahci_pci_device_resume done
w/ the patch applied, run S3 for more than ten times in a row,
[ 571.150339] ahci_pci_device_resume
[ 571.163193] ahci_pci_device_resume done
[ 578.560382] ahci_pci_device_resume
[ 578.576410] ahci_pci_device_resume done
[ 586.065037] ahci_pci_device_resume
[ 586.080806] ahci_pci_device_resume done
[ 770.231756] ahci_pci_device_resume
[ 770.244431] ahci_pci_device_resume done
[ 777.537985] ahci_pci_device_resume
[ 777.553709] ahci_pci_device_resume done
[ 784.805554] ahci_pci_device_resume
[ 784.819660] ahci_pci_device_resume done
[ 791.888451] ahci_pci_device_resume
[ 791.905222] ahci_pci_device_resume done
[ 798.943719] ahci_pci_device_resume
[ 798.959437] ahci_pci_device_resume done
[ 806.438784] ahci_pci_device_resume
[ 806.452070] ahci_pci_device_resume done
[ 813.890609] ahci_pci_device_resume
[ 813.903247] ahci_pci_device_resume done
[ 821.265956] ahci_pci_device_resume
[ 821.278714] ahci_pci_device_resume done
[ 828.609051] ahci_pci_device_resume
[ 828.621795] ahci_pci_device_resume done
[ 835.731528] ahci_pci_device_resume
[ 835.744210] ahci_pci_device_resume done
I don't know if this ssleep(1) is required for some platform specific
problems, but this patch does work well for me. :)
Hmm, we'd better get some comments from Tejun Heo first.
thanks,
rui
>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > --
> > drivers/ata/ahci.c | 17 ++++++++++++++---
> > 1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > Index: linux-2.6/drivers/ata/ahci.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/ata/ahci.c 2007-05-03 11:06:33.000000000
> +0800
> > +++ linux-2.6/drivers/ata/ahci.c 2008-07-02 16:25:54.000000000
> +0800
> > @@ -1073,18 +1073,29 @@
> >
> > /* global controller reset */
> > if (!ahci_skip_host_reset) {
> > + int delay = msecs_to_jiffies(1000);
> > + int timeout;
> > +
> > tmp = readl(mmio + HOST_CTL);
> > if ((tmp & HOST_RESET) == 0) {
> > writel(tmp | HOST_RESET, mmio + HOST_CTL);
> > readl(mmio + HOST_CTL); /* flush */
> > }
> >
> > - /* reset must complete within 1 second, or
> > + /*
> > + * to perform host reset, OS should set HOST_RESET
> > + * and poll until this bit is read to be "0"
> > + * reset must complete within 1 second, or
> > * the hardware should be considered fried.
> > */
> > - ssleep(1);
> > + timeout = jiffies + delay;
> > + while (jiffies < timeout) {
> > + tmp = readl(mmio + HOST_CTL);
> > + if (!(tmp & HOST_RESET))
> > + break;
> > + cpu_relax();
> > + }
> >
> > - tmp = readl(mmio + HOST_CTL);
> > if (tmp & HOST_RESET) {
> > dev_printk(KERN_ERR, host->dev,
> > "controller reset failed (0x%x)\n",
> tmp);
> >
> >
> >
> >
>
>
>
> --
> "Premature optimization is the root of all evil." - Donald Knuth
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] AHCI: speed up resume
2008-07-03 8:48 [RFC PATCH] AHCI: speed up resume Zhang Rui
2008-07-03 23:23 ` Rafael J. Wysocki
[not found] ` <200807040123.50337.rjw@sisk.pl>
@ 2008-07-04 2:48 ` Tejun Heo
[not found] ` <486D8F88.9000904@kernel.org>
3 siblings, 0 replies; 7+ messages in thread
From: Tejun Heo @ 2008-07-04 2:48 UTC (permalink / raw)
To: Zhang Rui; +Cc: linux-pm, linux-kernel
Hello, Zhang.
Zhang Rui wrote:
> During S3 resume, AHCI driver sleeps 1 second to wait for the HBA reset
> to finish. This is luxurious, :)
>
> According to the AHCI 1.2 spec, We should poll the HOST_CTL register,
> and return error if the host reset is not finished within 1 second.
>
> Test results show that the HBA reset can be done quickly(in usecs).
> And this patch may save nearly 1 second during resume.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> --
> drivers/ata/ahci.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> Index: linux-2.6/drivers/ata/ahci.c
> ===================================================================
> --- linux-2.6.orig/drivers/ata/ahci.c 2007-05-03 11:06:33.000000000 +0800
> +++ linux-2.6/drivers/ata/ahci.c 2008-07-02 16:25:54.000000000 +0800
> @@ -1073,18 +1073,29 @@
>
> /* global controller reset */
> if (!ahci_skip_host_reset) {
> + int delay = msecs_to_jiffies(1000);
> + int timeout;
> +
> tmp = readl(mmio + HOST_CTL);
> if ((tmp & HOST_RESET) == 0) {
> writel(tmp | HOST_RESET, mmio + HOST_CTL);
> readl(mmio + HOST_CTL); /* flush */
> }
>
> - /* reset must complete within 1 second, or
> + /*
> + * to perform host reset, OS should set HOST_RESET
> + * and poll until this bit is read to be "0"
> + * reset must complete within 1 second, or
> * the hardware should be considered fried.
> */
> - ssleep(1);
> + timeout = jiffies + delay;
> + while (jiffies < timeout) {
> + tmp = readl(mmio + HOST_CTL);
> + if (!(tmp & HOST_RESET))
> + break;
> + cpu_relax();
> + }
Looks good in principle. Just two things...
1. Please don't busy-loop. e.g. No one would notice 10ms delay between
polls.
2. Please use ata_wait_register().
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] AHCI: speed up resume
[not found] ` <486D8F88.9000904@kernel.org>
@ 2008-07-04 5:32 ` Zhang Rui
[not found] ` <1215149537.3068.18.camel@rzhang-dt.sh.intel.com>
1 sibling, 0 replies; 7+ messages in thread
From: Zhang Rui @ 2008-07-04 5:32 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-pm, linux-kernel
On Fri, 2008-07-04 at 10:48 +0800, Tejun Heo wrote:
> Hello, Zhang.
>
> Zhang Rui wrote:
> > During S3 resume, AHCI driver sleeps 1 second to wait for the HBA
> reset
> > to finish. This is luxurious, :)
> >
> > According to the AHCI 1.2 spec, We should poll the HOST_CTL
> register,
> > and return error if the host reset is not finished within 1 second.
> >
> > Test results show that the HBA reset can be done quickly(in usecs).
> > And this patch may save nearly 1 second during resume.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > --
> > drivers/ata/ahci.c | 17 ++++++++++++++---
> > 1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > Index: linux-2.6/drivers/ata/ahci.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/ata/ahci.c 2007-05-03 11:06:33.000000000
> +0800
> > +++ linux-2.6/drivers/ata/ahci.c 2008-07-02 16:25:54.000000000
> +0800
> > @@ -1073,18 +1073,29 @@
> >
> > /* global controller reset */
> > if (!ahci_skip_host_reset) {
> > + int delay = msecs_to_jiffies(1000);
> > + int timeout;
> > +
> > tmp = readl(mmio + HOST_CTL);
> > if ((tmp & HOST_RESET) == 0) {
> > writel(tmp | HOST_RESET, mmio + HOST_CTL);
> > readl(mmio + HOST_CTL); /* flush */
> > }
> >
> > - /* reset must complete within 1 second, or
> > + /*
> > + * to perform host reset, OS should set HOST_RESET
> > + * and poll until this bit is read to be "0"
> > + * reset must complete within 1 second, or
> > * the hardware should be considered fried.
> > */
> > - ssleep(1);
> > + timeout = jiffies + delay;
> > + while (jiffies < timeout) {
> > + tmp = readl(mmio + HOST_CTL);
> > + if (!(tmp & HOST_RESET))
> > + break;
> > + cpu_relax();
> > + }
>
> Looks good in principle. Just two things...
>
> 1. Please don't busy-loop. e.g. No one would notice 10ms delay
> between
> polls.
>
> 2. Please use ata_wait_register().
>
Thanks for review, refreshed patch attached. :)
From: Zhang Rui <rui.zhang@intel.com>
During resume, sleep 1 second to wait for the HBA reset
to finish is a waste of time.
According to the AHCI 1.2 spec,
We should poll the HOST_CTL register,
and return error if the host reset is not
finished within 1 second.
Test results show that the HBA reset can be done quickly(in usecs).
And this patch may save nearly 1 second during resume.
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
--
drivers/ata/ahci.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
Index: linux-2.6/drivers/ata/ahci.c
===================================================================
--- linux-2.6.orig/drivers/ata/ahci.c 2008-07-04 09:49:05.000000000 +0800
+++ linux-2.6/drivers/ata/ahci.c 2008-07-04 11:31:30.000000000 +0800
@@ -1079,12 +1079,15 @@
readl(mmio + HOST_CTL); /* flush */
}
- /* reset must complete within 1 second, or
+ /*
+ * to perform host reset, OS should set HOST_RESET
+ * and poll until this bit is read to be "0".
+ * reset must complete within 1 second, or
* the hardware should be considered fried.
*/
- ssleep(1);
+ tmp = ata_wait_register(mmio + HOST_CTL, HOST_RESET,
+ HOST_RESET, 10, 1000);
- tmp = readl(mmio + HOST_CTL);
if (tmp & HOST_RESET) {
dev_printk(KERN_ERR, host->dev,
"controller reset failed (0x%x)\n", tmp);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] AHCI: speed up resume
[not found] ` <1215149537.3068.18.camel@rzhang-dt.sh.intel.com>
@ 2008-07-04 8:10 ` Tejun Heo
2008-07-11 13:49 ` Jeff Garzik
1 sibling, 0 replies; 7+ messages in thread
From: Tejun Heo @ 2008-07-04 8:10 UTC (permalink / raw)
To: Zhang Rui; +Cc: linux-pm, linux-kernel
Zhang Rui wrote:
> From: Zhang Rui <rui.zhang@intel.com>
>
> During resume, sleep 1 second to wait for the HBA reset
> to finish is a waste of time.
>
> According to the AHCI 1.2 spec,
> We should poll the HOST_CTL register,
> and return error if the host reset is not
> finished within 1 second.
>
> Test results show that the HBA reset can be done quickly(in usecs).
> And this patch may save nearly 1 second during resume.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Acked-by: Tejun Heo <tj@kernel.org>
--
tejun
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] AHCI: speed up resume
[not found] ` <1215149537.3068.18.camel@rzhang-dt.sh.intel.com>
2008-07-04 8:10 ` Tejun Heo
@ 2008-07-11 13:49 ` Jeff Garzik
1 sibling, 0 replies; 7+ messages in thread
From: Jeff Garzik @ 2008-07-11 13:49 UTC (permalink / raw)
To: Zhang Rui; +Cc: Tejun Heo, linux-pm, linux-kernel
Zhang Rui wrote:
> On Fri, 2008-07-04 at 10:48 +0800, Tejun Heo wrote:
>> Hello, Zhang.
>>
>> Zhang Rui wrote:
>>> During S3 resume, AHCI driver sleeps 1 second to wait for the HBA
>> reset
>>> to finish. This is luxurious, :)
>>>
>>> According to the AHCI 1.2 spec, We should poll the HOST_CTL
>> register,
>>> and return error if the host reset is not finished within 1 second.
>>>
>>> Test results show that the HBA reset can be done quickly(in usecs).
>>> And this patch may save nearly 1 second during resume.
>>>
>>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>>> --
>>> drivers/ata/ahci.c | 17 ++++++++++++++---
>>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>>
>>> Index: linux-2.6/drivers/ata/ahci.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/ata/ahci.c 2007-05-03 11:06:33.000000000
>> +0800
>>> +++ linux-2.6/drivers/ata/ahci.c 2008-07-02 16:25:54.000000000
>> +0800
>>> @@ -1073,18 +1073,29 @@
>>>
>>> /* global controller reset */
>>> if (!ahci_skip_host_reset) {
>>> + int delay = msecs_to_jiffies(1000);
>>> + int timeout;
>>> +
>>> tmp = readl(mmio + HOST_CTL);
>>> if ((tmp & HOST_RESET) == 0) {
>>> writel(tmp | HOST_RESET, mmio + HOST_CTL);
>>> readl(mmio + HOST_CTL); /* flush */
>>> }
>>>
>>> - /* reset must complete within 1 second, or
>>> + /*
>>> + * to perform host reset, OS should set HOST_RESET
>>> + * and poll until this bit is read to be "0"
>>> + * reset must complete within 1 second, or
>>> * the hardware should be considered fried.
>>> */
>>> - ssleep(1);
>>> + timeout = jiffies + delay;
>>> + while (jiffies < timeout) {
>>> + tmp = readl(mmio + HOST_CTL);
>>> + if (!(tmp & HOST_RESET))
>>> + break;
>>> + cpu_relax();
>>> + }
>> Looks good in principle. Just two things...
>>
>> 1. Please don't busy-loop. e.g. No one would notice 10ms delay
>> between
>> polls.
>>
>> 2. Please use ata_wait_register().
>>
> Thanks for review, refreshed patch attached. :)
>
> From: Zhang Rui <rui.zhang@intel.com>
>
> During resume, sleep 1 second to wait for the HBA reset
> to finish is a waste of time.
>
> According to the AHCI 1.2 spec,
> We should poll the HOST_CTL register,
> and return error if the host reset is not
> finished within 1 second.
>
> Test results show that the HBA reset can be done quickly(in usecs).
> And this patch may save nearly 1 second during resume.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> --
> drivers/ata/ahci.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> Index: linux-2.6/drivers/ata/ahci.c
> ===================================================================
> --- linux-2.6.orig/drivers/ata/ahci.c 2008-07-04 09:49:05.000000000 +0800
> +++ linux-2.6/drivers/ata/ahci.c 2008-07-04 11:31:30.000000000 +0800
> @@ -1079,12 +1079,15 @@
> readl(mmio + HOST_CTL); /* flush */
> }
>
> - /* reset must complete within 1 second, or
> + /*
> + * to perform host reset, OS should set HOST_RESET
> + * and poll until this bit is read to be "0".
> + * reset must complete within 1 second, or
> * the hardware should be considered fried.
> */
> - ssleep(1);
> + tmp = ata_wait_register(mmio + HOST_CTL, HOST_RESET,
> + HOST_RESET, 10, 1000);
>
> - tmp = readl(mmio + HOST_CTL);
> if (tmp & HOST_RESET) {
> dev_printk(KERN_ERR, host->dev,
> "controller reset failed (0x%x)\n", tmp);
>
applied
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-07-11 13:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-07-03 8:48 [RFC PATCH] AHCI: speed up resume Zhang Rui
2008-07-03 23:23 ` Rafael J. Wysocki
[not found] ` <200807040123.50337.rjw@sisk.pl>
2008-07-04 1:48 ` Zhang Rui
2008-07-04 2:48 ` Tejun Heo
[not found] ` <486D8F88.9000904@kernel.org>
2008-07-04 5:32 ` Zhang Rui
[not found] ` <1215149537.3068.18.camel@rzhang-dt.sh.intel.com>
2008-07-04 8:10 ` Tejun Heo
2008-07-11 13:49 ` Jeff Garzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox