[PATCH REPOST] libata: lengthen interval between SRST set and clear

[PATCH REPOST] libata: lengthen interval between SRST set and clear

[Tejun Heo]

20us delay is not enough for some controllers and they end up not
sending the second H2D FIS to clear SRST resulting in softreset
failure.  This problem has been spotted and diagnosed with SATA trace
by JMicron on sata_nv.

This patch makes ata_bus_softreset() use msleep(1) instead of
udelay(20) between SRST set and clear.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Justin Tsai <justin@jmicron.com>

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 753b015..007020e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2414,15 +2414,15 @@ static unsigned int ata_bus_softreset(st
 	/* software reset.  causes dev0 to be selected */
 	if (ap->flags & ATA_FLAG_MMIO) {
 		writeb(ap->ctl, (void __iomem *) ioaddr->ctl_addr);
-		udelay(20);	/* FIXME: flush */
+		msleep(1);	/* FIXME: flush */
 		writeb(ap->ctl | ATA_SRST, (void __iomem *) ioaddr->ctl_addr);
-		udelay(20);	/* FIXME: flush */
+		msleep(1);	/* FIXME: flush */
 		writeb(ap->ctl, (void __iomem *) ioaddr->ctl_addr);
 	} else {
 		outb(ap->ctl, ioaddr->ctl_addr);
-		udelay(10);
+		msleep(1);
 		outb(ap->ctl | ATA_SRST, ioaddr->ctl_addr);
-		udelay(10);
+		msleep(1);
 		outb(ap->ctl, ioaddr->ctl_addr);
 	}
 
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c

Re: [PATCH REPOST] libata: lengthen interval between SRST set and clear

[Jeff Garzik]

Tejun Heo wrote:
> 20us delay is not enough for some controllers and they end up not
> sending the second H2D FIS to clear SRST resulting in softreset
> failure.  This problem has been spotted and diagnosed with SATA trace
> by JMicron on sata_nv.
> 
> This patch makes ata_bus_softreset() use msleep(1) instead of
> udelay(20) between SRST set and clear.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Justin Tsai <justin@jmicron.com>
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 753b015..007020e 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2414,15 +2414,15 @@ static unsigned int ata_bus_softreset(st
>  	/* software reset.  causes dev0 to be selected */
>  	if (ap->flags & ATA_FLAG_MMIO) {
>  		writeb(ap->ctl, (void __iomem *) ioaddr->ctl_addr);
> -		udelay(20);	/* FIXME: flush */
> +		msleep(1);	/* FIXME: flush */
>  		writeb(ap->ctl | ATA_SRST, (void __iomem *) ioaddr->ctl_addr);
> -		udelay(20);	/* FIXME: flush */
> +		msleep(1);	/* FIXME: flush */
>  		writeb(ap->ctl, (void __iomem *) ioaddr->ctl_addr);
>  	} else {
>  		outb(ap->ctl, ioaddr->ctl_addr);
> -		udelay(10);
> +		msleep(1);
>  		outb(ap->ctl | ATA_SRST, ioaddr->ctl_addr);
> -		udelay(10);
> +		msleep(1);
>  		outb(ap->ctl, ioaddr->ctl_addr);

I'm gonna NAK this one.  The controller is operating out of spec 
(hardware bug) if this is necessary.

The effect of msleep(1) is really msleep(10) or msleep(100) depending on 
the length of a timer tick.  That's just far too long a length of time 
to be asserting SRST, especially for PATA.

I suppose you can create a jmicron_soft_reset() inside ahci.c...

	Jeff

Re: [PATCH REPOST] libata: lengthen interval between SRST set and clear

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> 20us delay is not enough for some controllers and they end up not
>> sending the second H2D FIS to clear SRST resulting in softreset
>> failure.  This problem has been spotted and diagnosed with SATA trace
>> by JMicron on sata_nv.
>>
>> This patch makes ata_bus_softreset() use msleep(1) instead of
>> udelay(20) between SRST set and clear.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> Cc: Justin Tsai <justin@jmicron.com>
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 753b015..007020e 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -2414,15 +2414,15 @@ static unsigned int ata_bus_softreset(st
>>      /* software reset.  causes dev0 to be selected */
>>      if (ap->flags & ATA_FLAG_MMIO) {
>>          writeb(ap->ctl, (void __iomem *) ioaddr->ctl_addr);
>> -        udelay(20);    /* FIXME: flush */
>> +        msleep(1);    /* FIXME: flush */
>>          writeb(ap->ctl | ATA_SRST, (void __iomem *) ioaddr->ctl_addr);
>> -        udelay(20);    /* FIXME: flush */
>> +        msleep(1);    /* FIXME: flush */
>>          writeb(ap->ctl, (void __iomem *) ioaddr->ctl_addr);
>>      } else {
>>          outb(ap->ctl, ioaddr->ctl_addr);
>> -        udelay(10);
>> +        msleep(1);
>>          outb(ap->ctl | ATA_SRST, ioaddr->ctl_addr);
>> -        udelay(10);
>> +        msleep(1);
>>          outb(ap->ctl, ioaddr->ctl_addr);
> 
> I'm gonna NAK this one.  The controller is operating out of spec 
> (hardware bug) if this is necessary.

> The effect of msleep(1) is really msleep(10) or msleep(100) depending on 
> the length of a timer tick.  That's just far too long a length of time 
> to be asserting SRST, especially for PATA.
> 
> I suppose you can create a jmicron_soft_reset() inside ahci.c...

The problem actually has been spotted on sata_nv.  It is suggested that 
this is dependent on what type of device is attached to the controller. 
  This problem seems to be caused if the device is slow to ack the first 
H2D Register FIS.  So, we really don't know which controllers are 
affected by this.

How about doing msleep(1) if it's SATA?

-- 
tejun

Re: [PATCH REPOST] libata: lengthen interval between SRST set and clear

[Jeff Garzik]

Tejun Heo wrote:
> The problem actually has been spotted on sata_nv.  It is suggested that 
> this is dependent on what type of device is attached to the controller. 
>  This problem seems to be caused if the device is slow to ack the first 
> H2D Register FIS.  So, we really don't know which controllers are 
> affected by this.
> 
> How about doing msleep(1) if it's SATA?


Given the minimal amount of problem reports, I'd rather hold off and 
collect data.  What devices are affected?  Is it complaining because 
it's in the middle of a reset when SRST is received?  Is it just one 
model of one device?  Is this definitely reproducible on all SATA 
controllers?  etc.

Too many unknowns right now...

	Jeff

Re: [PATCH REPOST] libata: lengthen interval between SRST set and clear

[Eric D. Mudama]

On 9/28/06, Jeff Garzik <jgarzik@pobox.com> wrote:
> Given the minimal amount of problem reports, I'd rather hold off and
> collect data.  What devices are affected?  Is it complaining because
> it's in the middle of a reset when SRST is received?  Is it just one
> model of one device?  Is this definitely reproducible on all SATA
> controllers?  etc.
>
> Too many unknowns right now...
>
>         Jeff

I agree 100%.  The specification states in section 13.1.1 of the 2.5
SATA specification covering Parallel ATA Emulation for Software Reset:

"Although host software is required to toggle the SRST bit no faster
than 5 us, devices may not rely on the inter-arrival time of received
Register – Host to Device FISes also meeting this timing. Because of
flow control, frame handshaking, and other protocol interlocks,
devices may effectively receive the resulting Register – Device to
Host FISes back-to-back."

Is it possible that the "set" of SRST hasn't actually occurred on the
bus when the "clear" of SRST is issued to the controller?  If that's
the case, then the right answer should be using a mechanism to confirm
transmission of the HTD register FIS before generating the clear for
SRST.  I'm guessing this is basically looking for R_OK from the
device.

Drive firmware has to cope with these latencies (as well as
collisions, especially in NCQ), so surely there's a safe way to do it
in the various controllers.

--eric