[PATCHSET] ahci: convert to new reset mechanism

[PATCHSET] ahci: convert to new reset mechanism

[Tejun Heo]

Hello, Jeff.

This is respin of ahci part of new reset mechanism patchset take #4[1].
I've prepended a patch to convert ahci_fill_cmd_slot() to take *pp
instead of *ap.  This should have been done when posting the reset
patchset.  Sorry about the mess.  Was way too tired, I guess.

This patchset is against the current upstream[2], but will apply fine
over the inline-ata_qc_complete patch[3] I've just posted.

BTW, something wrong about ata_piix patches in the reset patchset?

Thanks.

--
tejun

[1] http://article.gmane.org/gmane.linux.ide/7935
[2] 489ff4c7d167be954f715128790bd80d3c888322
[3] http://article.gmane.org/gmane.linux.ide/7948

[PATCH 2/3] ahci: convert to new reset mechanism

[Tejun Heo]

Convert ahci ->phy_reset to new reset mechanism.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c |   46 +++++++++++++++++++++++++++++-----------------
 1 files changed, 29 insertions(+), 17 deletions(-)

6f3211bfdb66aa3a0b1e703736dc445b10ac4a2d
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 72bdd43..86bccb7 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -190,7 +190,7 @@ static void ahci_scr_write (struct ata_p
 static int ahci_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
 static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc);
 static irqreturn_t ahci_interrupt (int irq, void *dev_instance, struct pt_regs *regs);
-static void ahci_phy_reset(struct ata_port *ap);
+static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes);
 static void ahci_irq_clear(struct ata_port *ap);
 static void ahci_eng_timeout(struct ata_port *ap);
 static int ahci_port_start(struct ata_port *ap);
@@ -230,7 +230,7 @@ static const struct ata_port_operations 
 
 	.tf_read		= ahci_tf_read,
 
-	.phy_reset		= ahci_phy_reset,
+	.probe_reset		= ahci_probe_reset,
 
 	.qc_prep		= ahci_qc_prep,
 	.qc_issue		= ahci_qc_issue,
@@ -252,8 +252,7 @@ static const struct ata_port_info ahci_p
 	{
 		.sht		= &ahci_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
-				  ATA_FLAG_PIO_DMA,
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
@@ -515,28 +514,35 @@ static void ahci_fill_cmd_slot(struct ah
 	pp->cmd_slot[0].tbl_addr_hi = cpu_to_le32((pp->cmd_tbl_dma >> 16) >> 16);
 }
 
-static void ahci_phy_reset(struct ata_port *ap)
+static int ahci_hardreset(struct ata_port *ap, int verbose, unsigned int *class)
 {
-	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
-	struct ata_device *dev = &ap->device[0];
-	u32 new_tmp, tmp;
+	int rc;
+
+	DPRINTK("ENTER\n");
 
 	ahci_stop_engine(ap);
-	__sata_phy_reset(ap);
+	rc = sata_std_hardreset(ap, verbose, class);
 	ahci_start_engine(ap);
 
-	if (ap->flags & ATA_FLAG_PORT_DISABLED)
-		return;
+	if (rc == 0)
+		*class = ahci_dev_classify(ap);
+	if (*class == ATA_DEV_UNKNOWN)
+		*class = ATA_DEV_NONE;
 
-	dev->class = ahci_dev_classify(ap);
-	if (!ata_dev_present(dev)) {
-		ata_port_disable(ap);
-		return;
-	}
+	DPRINTK("EXIT, rc=%d, class=%u\n", rc, *class);
+	return rc;
+}
+
+static void ahci_postreset(struct ata_port *ap, unsigned int *class)
+{
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
+	u32 new_tmp, tmp;
+
+	ata_std_postreset(ap, class);
 
 	/* Make sure port's ATAPI bit is set appropriately */
 	new_tmp = tmp = readl(port_mmio + PORT_CMD);
-	if (dev->class == ATA_DEV_ATAPI)
+	if (*class == ATA_DEV_ATAPI)
 		new_tmp |= PORT_CMD_ATAPI;
 	else
 		new_tmp &= ~PORT_CMD_ATAPI;
@@ -546,6 +552,12 @@ static void ahci_phy_reset(struct ata_po
 	}
 }
 
+static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
+{
+	return ata_drive_probe_reset(ap, NULL, NULL, ahci_hardreset,
+				     ahci_postreset, classes);
+}
+
 static u8 ahci_check_status(struct ata_port *ap)
 {
 	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
-- 
1.1.5

[PATCH 1/3] ahci: make ahci_fill_cmd_slot() take *pp instead of *ap

[Tejun Heo]

Make ahci_fill_cmd_slot() take struct ahci_port_priv *pp instead of
struct ata_port *ap as suggested by Jeff Garzik.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

47c10126810dfdd091c2ff3f6316cab4ff504e5e
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 98ce6bb..72bdd43 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -507,9 +507,8 @@ static unsigned int ahci_dev_classify(st
 	return ata_dev_classify(&tf);
 }
 
-static void ahci_fill_cmd_slot(struct ata_port *ap, u32 opts)
+static void ahci_fill_cmd_slot(struct ahci_port_priv *pp, u32 opts)
 {
-	struct ahci_port_priv *pp = ap->private_data;
 	pp->cmd_slot[0].opts = cpu_to_le32(opts);
 	pp->cmd_slot[0].status = 0;
 	pp->cmd_slot[0].tbl_addr = cpu_to_le32(pp->cmd_tbl_dma & 0xffffffff);
@@ -622,7 +621,7 @@ static void ahci_qc_prep(struct ata_queu
 	if (is_atapi)
 		opts |= AHCI_CMD_ATAPI;
 
-	ahci_fill_cmd_slot(ap, opts);
+	ahci_fill_cmd_slot(pp, opts);
 }
 
 static void ahci_restart_port(struct ata_port *ap, u32 irq_stat)
-- 
1.1.5

Re: [PATCH 1/3] ahci: make ahci_fill_cmd_slot() take *pp instead of *ap

[Jeff Garzik]

Tejun Heo wrote:
> Make ahci_fill_cmd_slot() take struct ahci_port_priv *pp instead of
> struct ata_port *ap as suggested by Jeff Garzik.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied 1-2

[PATCH 3/3] ahci: add softreset

[Tejun Heo]

Now that libata is smart enought to handle both soft and hard resets,
add softreset method.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c |  135 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 134 insertions(+), 1 deletions(-)

1edad0c8fc8e36457ac1f7c68ba4d86ff63986e3
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 86bccb7..a65895f 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -514,6 +514,138 @@ static void ahci_fill_cmd_slot(struct ah
 	pp->cmd_slot[0].tbl_addr_hi = cpu_to_le32((pp->cmd_tbl_dma >> 16) >> 16);
 }
 
+static int ahci_wait_for_bit(void __iomem *reg, u32 mask, u32 val,
+			     unsigned long interval_msec,
+			     unsigned long timeout_msec)
+{
+	unsigned long timeout;
+	u32 tmp;
+
+	timeout = jiffies + (timeout_msec * HZ) / 1000;
+	do {
+		tmp = readl(reg);
+		if ((tmp & mask) != val)
+			return 0;
+		msleep(interval_msec);
+	} while (time_before(jiffies, timeout));
+
+	return -1;
+}
+
+static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+{
+	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	void __iomem *mmio = ap->host_set->mmio_base;
+	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
+	const u32 cmd_fis_len = 5; /* five dwords */
+	const char *reason = NULL;
+	struct ata_taskfile tf;
+	u8 *fis;
+	int rc;
+
+	DPRINTK("ENTER\n");
+
+	/* prepare for SRST (AHCI-1.1 10.4.1) */
+	rc = ahci_stop_engine(ap);
+	if (rc) {
+		reason = "failed to stop engine";
+		goto fail_restart;
+	}
+
+	/* check BUSY/DRQ, perform Command List Override if necessary */
+	ahci_tf_read(ap, &tf);
+	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
+		u32 tmp;
+
+		if (!(hpriv->cap & HOST_CAP_CLO)) {
+			rc = -EIO;
+			reason = "port busy but no CLO";
+			goto fail_restart;
+		}
+
+		tmp = readl(port_mmio + PORT_CMD);
+		tmp |= PORT_CMD_CLO;
+		writel(tmp, port_mmio + PORT_CMD);
+		readl(port_mmio + PORT_CMD); /* flush */
+
+		if (ahci_wait_for_bit(port_mmio + PORT_CMD,
+				      PORT_CMD_CLO, PORT_CMD_CLO, 1, 500)) {
+			rc = -EIO;
+			reason = "CLO failed";
+			goto fail_restart;
+		}
+	}
+
+	/* restart engine */
+	ahci_start_engine(ap);
+
+	ata_tf_init(ap, &tf, 0);
+	fis = pp->cmd_tbl;
+
+	/* issue the first D2H Register FIS */
+	ahci_fill_cmd_slot(pp, cmd_fis_len | AHCI_CMD_RESET | AHCI_CMD_CLR_BUSY);
+
+	tf.ctl |= ATA_SRST;
+	ata_tf_to_fis(&tf, fis, 0);
+	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
+
+	writel(1, port_mmio + PORT_CMD_ISSUE);
+	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
+
+	if (ahci_wait_for_bit(port_mmio + PORT_CMD_ISSUE, 0x1, 0x1, 1, 500)) {
+		rc = -EIO;
+		reason = "1st FIS failed";
+		goto fail;
+	}
+
+	/* spec says at least 5us, but be generous and sleep for 1ms */
+	msleep(1);
+
+	/* issue the second D2H Register FIS */
+	ahci_fill_cmd_slot(pp, cmd_fis_len);
+
+	tf.ctl &= ~ATA_SRST;
+	ata_tf_to_fis(&tf, fis, 0);
+	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
+
+	writel(1, port_mmio + PORT_CMD_ISSUE);
+	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
+
+	/* spec mandates ">= 2ms" before checking status.
+	 * We wait 150ms, because that was the magic delay used for
+	 * ATAPI devices in Hale Landis's ATADRVR, for the period of time
+	 * between when the ATA command register is written, and then
+	 * status is checked.  Because waiting for "a while" before
+	 * checking status is fine, post SRST, we perform this magic
+	 * delay here as well.
+	 */
+	msleep(150);
+
+	*class = ATA_DEV_NONE;
+	if (sata_dev_present(ap)) {
+		if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
+			rc = -EIO;
+			reason = "device not ready";
+			goto fail;
+		}
+		*class = ahci_dev_classify(ap);
+	}
+
+	DPRINTK("EXIT, class=%u\n", *class);
+	return 0;
+
+ fail_restart:
+	ahci_start_engine(ap);
+ fail:
+	if (verbose)
+		printk(KERN_ERR "ata%u: softreset failed (%s)\n",
+		       ap->id, reason);
+	else
+		DPRINTK("EXIT, rc=%d reason=\"%s\"\n", rc, reason);
+	return rc;
+}
+
 static int ahci_hardreset(struct ata_port *ap, int verbose, unsigned int *class)
 {
 	int rc;
@@ -554,7 +686,8 @@ static void ahci_postreset(struct ata_po
 
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
-	return ata_drive_probe_reset(ap, NULL, NULL, ahci_hardreset,
+	return ata_drive_probe_reset(ap, ata_std_probeinit,
+				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);
 }
 
-- 
1.1.5

Re: [PATCH 3/3] ahci: add softreset

[Jeff Garzik]

Tejun Heo wrote:
> Now that libata is smart enought to handle both soft and hard resets,
> add softreset method.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/ahci.c |  135 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 134 insertions(+), 1 deletions(-)
> 
> 1edad0c8fc8e36457ac1f7c68ba4d86ff63986e3
> diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
> index 86bccb7..a65895f 100644
> --- a/drivers/scsi/ahci.c
> +++ b/drivers/scsi/ahci.c
> @@ -514,6 +514,138 @@ static void ahci_fill_cmd_slot(struct ah
>  	pp->cmd_slot[0].tbl_addr_hi = cpu_to_le32((pp->cmd_tbl_dma >> 16) >> 16);
>  }
>  
> +static int ahci_wait_for_bit(void __iomem *reg, u32 mask, u32 val,
> +			     unsigned long interval_msec,
> +			     unsigned long timeout_msec)
> +{
> +	unsigned long timeout;
> +	u32 tmp;
> +
> +	timeout = jiffies + (timeout_msec * HZ) / 1000;
> +	do {
> +		tmp = readl(reg);
> +		if ((tmp & mask) != val)

IMO the sense of this test should be reversed, as the above line of code 
is the opposite of what the function name implies.


> +			return 0;
> +		msleep(interval_msec);
> +	} while (time_before(jiffies, timeout));
> +
> +	return -1;
> +}
> +
> +static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
> +{
> +	struct ahci_host_priv *hpriv = ap->host_set->private_data;
> +	struct ahci_port_priv *pp = ap->private_data;
> +	void __iomem *mmio = ap->host_set->mmio_base;
> +	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
> +	const u32 cmd_fis_len = 5; /* five dwords */
> +	const char *reason = NULL;
> +	struct ata_taskfile tf;
> +	u8 *fis;
> +	int rc;
> +
> +	DPRINTK("ENTER\n");
> +
> +	/* prepare for SRST (AHCI-1.1 10.4.1) */
> +	rc = ahci_stop_engine(ap);
> +	if (rc) {
> +		reason = "failed to stop engine";
> +		goto fail_restart;
> +	}
> +
> +	/* check BUSY/DRQ, perform Command List Override if necessary */
> +	ahci_tf_read(ap, &tf);
> +	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
> +		u32 tmp;
> +
> +		if (!(hpriv->cap & HOST_CAP_CLO)) {
> +			rc = -EIO;
> +			reason = "port busy but no CLO";
> +			goto fail_restart;
> +		}
> +
> +		tmp = readl(port_mmio + PORT_CMD);
> +		tmp |= PORT_CMD_CLO;
> +		writel(tmp, port_mmio + PORT_CMD);
> +		readl(port_mmio + PORT_CMD); /* flush */
> +
> +		if (ahci_wait_for_bit(port_mmio + PORT_CMD,
> +				      PORT_CMD_CLO, PORT_CMD_CLO, 1, 500)) {
> +			rc = -EIO;
> +			reason = "CLO failed";
> +			goto fail_restart;
> +		}
> +	}
> +
> +	/* restart engine */
> +	ahci_start_engine(ap);
> +
> +	ata_tf_init(ap, &tf, 0);
> +	fis = pp->cmd_tbl;
> +
> +	/* issue the first D2H Register FIS */
> +	ahci_fill_cmd_slot(pp, cmd_fis_len | AHCI_CMD_RESET | AHCI_CMD_CLR_BUSY);
> +
> +	tf.ctl |= ATA_SRST;
> +	ata_tf_to_fis(&tf, fis, 0);
> +	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
> +
> +	writel(1, port_mmio + PORT_CMD_ISSUE);
> +	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
> +
> +	if (ahci_wait_for_bit(port_mmio + PORT_CMD_ISSUE, 0x1, 0x1, 1, 500)) {
> +		rc = -EIO;
> +		reason = "1st FIS failed";
> +		goto fail;
> +	}
> +
> +	/* spec says at least 5us, but be generous and sleep for 1ms */
> +	msleep(1);
> +
> +	/* issue the second D2H Register FIS */
> +	ahci_fill_cmd_slot(pp, cmd_fis_len);
> +
> +	tf.ctl &= ~ATA_SRST;
> +	ata_tf_to_fis(&tf, fis, 0);
> +	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
> +
> +	writel(1, port_mmio + PORT_CMD_ISSUE);
> +	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
> +
> +	/* spec mandates ">= 2ms" before checking status.
> +	 * We wait 150ms, because that was the magic delay used for
> +	 * ATAPI devices in Hale Landis's ATADRVR, for the period of time
> +	 * between when the ATA command register is written, and then
> +	 * status is checked.  Because waiting for "a while" before
> +	 * checking status is fine, post SRST, we perform this magic
> +	 * delay here as well.
> +	 */
> +	msleep(150);
> +
> +	*class = ATA_DEV_NONE;
> +	if (sata_dev_present(ap)) {
> +		if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
> +			rc = -EIO;
> +			reason = "device not ready";
> +			goto fail;
> +		}

On AHCI, are you sure this busy_sleep is even necessary?


> +		*class = ahci_dev_classify(ap);
> +	}
> +
> +	DPRINTK("EXIT, class=%u\n", *class);
> +	return 0;
> +
> + fail_restart:
> +	ahci_start_engine(ap);
> + fail:
> +	if (verbose)
> +		printk(KERN_ERR "ata%u: softreset failed (%s)\n",
> +		       ap->id, reason);
> +	else
> +		DPRINTK("EXIT, rc=%d reason=\"%s\"\n", rc, reason);
> +	return rc;
> +}
> +
>  static int ahci_hardreset(struct ata_port *ap, int verbose, unsigned int *class)
>  {
>  	int rc;
> @@ -554,7 +686,8 @@ static void ahci_postreset(struct ata_po
>  
>  static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
>  {
> -	return ata_drive_probe_reset(ap, NULL, NULL, ahci_hardreset,
> +	return ata_drive_probe_reset(ap, ata_std_probeinit,
> +				     ahci_softreset, ahci_hardreset,
>  				     ahci_postreset, classes);
>  }
>  

Re: [PATCH 3/3] ahci: add softreset

[Tejun]

Jeff Garzik wrote:
>> +static int ahci_wait_for_bit(void __iomem *reg, u32 mask, u32 val,
>> +                 unsigned long interval_msec,
>> +                 unsigned long timeout_msec)
>> +{
>> +    unsigned long timeout;
>> +    u32 tmp;
>> +
>> +    timeout = jiffies + (timeout_msec * HZ) / 1000;
>> +    do {
>> +        tmp = readl(reg);
>> +        if ((tmp & mask) != val)
> 
> 
> IMO the sense of this test should be reversed, as the above line of code 
> is the opposite of what the function name implies.

It didn't use to have @mask and just tested for a single bit to clear. 
Some piece of currently dropped code needed @mask, @val stuff and the 
changed function got left I think.  I'll reverse the testing logic and 
rename the function to something more appropriate - probably 
ahci_poll_register().

> 
>> +            return 0;
>> +        msleep(interval_msec);
>> +    } while (time_before(jiffies, timeout));
>> +
>> +    return -1;
>> +}
>> +
>> +static int ahci_softreset(struct ata_port *ap, int verbose, unsigned 
>> int *class)
>> +{
[--snip--]
>> +    /* issue the second D2H Register FIS */
>> +    ahci_fill_cmd_slot(pp, cmd_fis_len);
>> +
>> +    tf.ctl &= ~ATA_SRST;
>> +    ata_tf_to_fis(&tf, fis, 0);
>> +    fis[1] &= ~(1 << 7);    /* turn off Command FIS bit */
>> +
>> +    writel(1, port_mmio + PORT_CMD_ISSUE);
>> +    readl(port_mmio + PORT_CMD_ISSUE);    /* flush */
>> +
>> +    /* spec mandates ">= 2ms" before checking status.
>> +     * We wait 150ms, because that was the magic delay used for
>> +     * ATAPI devices in Hale Landis's ATADRVR, for the period of time
>> +     * between when the ATA command register is written, and then
>> +     * status is checked.  Because waiting for "a while" before
>> +     * checking status is fine, post SRST, we perform this magic
>> +     * delay here as well.
>> +     */
>> +    msleep(150);
>> +
>> +    *class = ATA_DEV_NONE;
>> +    if (sata_dev_present(ap)) {
>> +        if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
>> +            rc = -EIO;
>> +            reason = "device not ready";
>> +            goto fail;
>> +        }
> 
> 
> On AHCI, are you sure this busy_sleep is even necessary?
> 

We should wait for something before proceeding with classfication.  The 
second FIS turns off SRST and the device will respond with class 
signature when it gets out of reset status, which ends up clearing BUSY. 
  We can either wait for the issue bit to clear as in the the first FIS 
or above.  Do you think waiting for issue bit clearance is better?

Thanks.

-- 
tejun

[PATCH] ahci: add softreset

[Tejun Heo]

Now that libata is smart enought to handle both soft and hard resets,
add softreset method.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

Jeff, as I didn't get reply about the whether waiting is necessary or
not, I've just fixed the first part.

Thanks.

 ahci.c |  135 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 134 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 1c2ab3d..6b36345 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -513,6 +513,138 @@ static void ahci_fill_cmd_slot(struct ah
 	pp->cmd_slot[0].tbl_addr_hi = cpu_to_le32((pp->cmd_tbl_dma >> 16) >> 16);
 }
 
+static int ahci_poll_register(void __iomem *reg, u32 mask, u32 val,
+			      unsigned long interval_msec,
+			      unsigned long timeout_msec)
+{
+	unsigned long timeout;
+	u32 tmp;
+
+	timeout = jiffies + (timeout_msec * HZ) / 1000;
+	do {
+		tmp = readl(reg);
+		if ((tmp & mask) == val)
+			return 0;
+		msleep(interval_msec);
+	} while (time_before(jiffies, timeout));
+
+	return -1;
+}
+
+static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+{
+	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	void __iomem *mmio = ap->host_set->mmio_base;
+	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
+	const u32 cmd_fis_len = 5; /* five dwords */
+	const char *reason = NULL;
+	struct ata_taskfile tf;
+	u8 *fis;
+	int rc;
+
+	DPRINTK("ENTER\n");
+
+	/* prepare for SRST (AHCI-1.1 10.4.1) */
+	rc = ahci_stop_engine(ap);
+	if (rc) {
+		reason = "failed to stop engine";
+		goto fail_restart;
+	}
+
+	/* check BUSY/DRQ, perform Command List Override if necessary */
+	ahci_tf_read(ap, &tf);
+	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
+		u32 tmp;
+
+		if (!(hpriv->cap & HOST_CAP_CLO)) {
+			rc = -EIO;
+			reason = "port busy but no CLO";
+			goto fail_restart;
+		}
+
+		tmp = readl(port_mmio + PORT_CMD);
+		tmp |= PORT_CMD_CLO;
+		writel(tmp, port_mmio + PORT_CMD);
+		readl(port_mmio + PORT_CMD); /* flush */
+
+		if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0,
+				       1, 500)) {
+			rc = -EIO;
+			reason = "CLO failed";
+			goto fail_restart;
+		}
+	}
+
+	/* restart engine */
+	ahci_start_engine(ap);
+
+	ata_tf_init(ap, &tf, 0);
+	fis = pp->cmd_tbl;
+
+	/* issue the first D2H Register FIS */
+	ahci_fill_cmd_slot(pp, cmd_fis_len | AHCI_CMD_RESET | AHCI_CMD_CLR_BUSY);
+
+	tf.ctl |= ATA_SRST;
+	ata_tf_to_fis(&tf, fis, 0);
+	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
+
+	writel(1, port_mmio + PORT_CMD_ISSUE);
+	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
+
+	if (ahci_poll_register(port_mmio + PORT_CMD_ISSUE, 0x1, 0x0, 1, 500)) {
+		rc = -EIO;
+		reason = "1st FIS failed";
+		goto fail;
+	}
+
+	/* spec says at least 5us, but be generous and sleep for 1ms */
+	msleep(1);
+
+	/* issue the second D2H Register FIS */
+	ahci_fill_cmd_slot(pp, cmd_fis_len);
+
+	tf.ctl &= ~ATA_SRST;
+	ata_tf_to_fis(&tf, fis, 0);
+	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
+
+	writel(1, port_mmio + PORT_CMD_ISSUE);
+	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
+
+	/* spec mandates ">= 2ms" before checking status.
+	 * We wait 150ms, because that was the magic delay used for
+	 * ATAPI devices in Hale Landis's ATADRVR, for the period of time
+	 * between when the ATA command register is written, and then
+	 * status is checked.  Because waiting for "a while" before
+	 * checking status is fine, post SRST, we perform this magic
+	 * delay here as well.
+	 */
+	msleep(150);
+
+	*class = ATA_DEV_NONE;
+	if (sata_dev_present(ap)) {
+		if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
+			rc = -EIO;
+			reason = "device not ready";
+			goto fail;
+		}
+		*class = ahci_dev_classify(ap);
+	}
+
+	DPRINTK("EXIT, class=%u\n", *class);
+	return 0;
+
+ fail_restart:
+	ahci_start_engine(ap);
+ fail:
+	if (verbose)
+		printk(KERN_ERR "ata%u: softreset failed (%s)\n",
+		       ap->id, reason);
+	else
+		DPRINTK("EXIT, rc=%d reason=\"%s\"\n", rc, reason);
+	return rc;
+}
+
 static int ahci_hardreset(struct ata_port *ap, int verbose, unsigned int *class)
 {
 	int rc;
@@ -553,7 +685,8 @@ static void ahci_postreset(struct ata_po
 
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
-	return ata_drive_probe_reset(ap, NULL, NULL, ahci_hardreset,
+	return ata_drive_probe_reset(ap, ata_std_probeinit,
+				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);
 }
 

Re: [PATCH] ahci: add softreset

[Jeff Garzik]

Tejun Heo wrote:
> Now that libata is smart enought to handle both soft and hard resets,
> add softreset method.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK, finally

(though I am dropping all your other pending patches as requested)


> +	/* spec says at least 5us, but be generous and sleep for 1ms */
> +	msleep(1);

FWIW msleep simply guarantees a minimum delay.  Depending on the 
scheduler tick granularity, msleep(1) may delay 10-100ms.

This FWIW may or may not warrant updating the above comment, I leave it 
up to you.

	Jeff

Re: [PATCH] ahci: add softreset

[Tejun Heo]

On Tue, Feb 28, 2006 at 01:12:38PM -0500, Jeff Garzik wrote:
> Tejun Heo wrote:
> >Now that libata is smart enought to handle both soft and hard resets,
> >add softreset method.
> >
> >Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ACK, finally
> 
> (though I am dropping all your other pending patches as requested)
> 

Good, and I forgot to resend the regenerated patches after writing I
would do that ASAP.  I was about to do that but then quilt2git didn't
work and, after fixing it, I completely forgot why I fixed quilt2git
in the first place.  Duh... Will send soon.

> 
> >+	/* spec says at least 5us, but be generous and sleep for 1ms */
> >+	msleep(1);
> 
> FWIW msleep simply guarantees a minimum delay.  Depending on the 
> scheduler tick granularity, msleep(1) may delay 10-100ms.
> 
> This FWIW may or may not warrant updating the above comment, I leave it 
> up to you.
> 

I think the comment is okay as it is and, more importantly, it
wouldn't fit in one line with any more detail.  :-P

-- 
tejun

[PATCH] AHCI SATA vendor update from VIA

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 255 bytes --]

I received this ahci.c patch from VIA, and pass it on for review, 
comment, and testing.

This patch won't go in as-is, because it does too much special casing. 
But until we get around to that, people with VIA controllers probably 
want this...

	Jeff



[-- Attachment #2: ahci.c.patch --]
[-- Type: text/plain, Size: 8994 bytes --]

--- ahci.c.orig	2005-11-21 14:24:48.000000000 +0800
+++ ahci.c	2005-11-21 14:25:35.000000000 +0800
@@ -59,6 +59,7 @@
 	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
 
 	board_ahci		= 0,
+	board_via_ahci		= 1,
 
 	/* global controller registers */
 	HOST_CAP		= 0x00, /* host capabilities */
@@ -126,6 +127,7 @@
 	PORT_CMD_LIST_ON	= (1 << 15), /* cmd list DMA engine running */
 	PORT_CMD_FIS_ON		= (1 << 14), /* FIS DMA engine running */
 	PORT_CMD_FIS_RX		= (1 << 4), /* Enable FIS receive DMA engine */
+	PORT_CMD_CLO		= (1 << 3), /* CLO */
 	PORT_CMD_POWER_ON	= (1 << 2), /* Power up device */
 	PORT_CMD_SPIN_UP	= (1 << 1), /* Spin up device */
 	PORT_CMD_START		= (1 << 0), /* Enable port DMA engine */
@@ -183,6 +185,14 @@
 static u8 ahci_check_err(struct ata_port *ap);
 static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
 
+static int via_ahci_qc_issue(struct ata_queued_cmd *qc);
+static void via_ahci_phy_reset(struct ata_port *ap);
+static void via_ahci_port_stop(struct ata_port *ap);
+static int via_ahci_softreset(struct ata_port *ap);
+static unsigned int via_ata_busy_sleep (struct ata_port *ap,
+				    unsigned long tmout_pat,
+			    	    unsigned long tmout);
+
 static Scsi_Host_Template ahci_sht = {
 	.module			= THIS_MODULE,
 	.name			= DRV_NAME,
@@ -231,6 +241,35 @@
 	.host_stop		= ahci_host_stop,
 };
 
+static struct ata_port_operations via_ahci_ops = {
+	.port_disable		= ata_port_disable,
+
+	.check_status		= ahci_check_status,
+	.check_altstatus	= ahci_check_status,
+	.check_err		= ahci_check_err,
+	.dev_select		= ata_noop_dev_select,
+
+	.tf_read		= ahci_tf_read,
+
+
+	.qc_prep		= ahci_qc_prep,
+
+	.eng_timeout		= ahci_eng_timeout,
+
+	.irq_handler		= ahci_interrupt,
+	.irq_clear		= ahci_irq_clear,
+
+	.scr_read		= ahci_scr_read,
+	.scr_write		= ahci_scr_write,
+
+	.port_start		= ahci_port_start,
+	.host_stop		= ahci_host_stop,
+
+	.phy_reset		= via_ahci_phy_reset,
+	.qc_issue		= via_ahci_qc_issue,
+	.port_stop		= via_ahci_port_stop,
+};
+
 static struct ata_port_info ahci_port_info[] = {
 	/* board_ahci */
 	{
@@ -242,6 +281,16 @@
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
+	/* board_via_ahci */
+	{
+		.sht		= &ahci_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
+				  ATA_FLAG_PIO_DMA,
+		.pio_mask	= 0x03, /* pio3-4 */
+		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
+		.port_ops	= &via_ahci_ops,
+	},
 };
 
 static struct pci_device_id ahci_pci_tbl[] = {
@@ -263,6 +312,8 @@
 	  board_ahci }, /* ESB2 */
 	{ PCI_VENDOR_ID_INTEL, 0x2683, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_ahci }, /* ESB2 */
+	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  board_via_ahci }, /* VT8251*/
 	{ }	/* terminate list */
 };
 
@@ -1049,6 +1100,244 @@
 	return rc;
 }
 
+/* START: patch code for VIA VT8251 ahci controller */
+
+static int via_ahci_softreset(struct ata_port *ap)
+{
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
+	struct ahci_port_priv *pp = ap->private_data;
+	u32 tmp,i;
+	u8 *fisbuf;
+
+	VPRINTK("ENTER\n");
+
+	writel(0x00000000, port_mmio + PORT_IRQ_MASK);  /*disable interrupt */
+	readl (port_mmio + PORT_IRQ_MASK);  /* flush */
+
+	/* prepare the software-reset commands */
+
+	/* prepare first command header */
+	memset(pp->cmd_slot, 0, 32);
+	pp->cmd_slot[0].opts = 0x00000505;
+	pp->cmd_slot[0].status = 0;
+	pp->cmd_slot[0].tbl_addr = cpu_to_le32(pp->cmd_tbl_dma & 0xffffffff);
+	pp->cmd_slot[0].tbl_addr_hi = cpu_to_le32((pp->cmd_tbl_dma >> 16) >> 16);
+
+	/* prepare CMDFIS struct */
+	fisbuf = pp->cmd_tbl;
+	memset(fisbuf, 0, 64);
+	fisbuf[0] = 0x27;
+	fisbuf[7] = 0xa0;
+	fisbuf[15] = 0x04;
+	
+	/* trigger the commands */
+	writel(0x1, port_mmio + PORT_CMD_ISSUE);
+	readl (port_mmio + PORT_CMD_ISSUE); /* flush */
+
+	udelay(20);
+    /* wait command complete */
+	for (i = 0; i < 2000; i++) {
+		tmp = readl(port_mmio + PORT_CMD_ISSUE);
+		if ((tmp & 1) == 0) break;
+		msleep(20);
+	}
+
+	if (i == 2000) {
+		printk(KERN_WARNING "TimeOut for Wait Command complete\n");
+		return 1;
+	}
+
+	/* prepare second command header */
+	pp->cmd_slot[0].opts = 0x00000005;
+	pp->cmd_slot[0].status = 0;
+	pp->cmd_slot[0].tbl_addr = cpu_to_le32(pp->cmd_tbl_dma & 0xffffffff);
+	pp->cmd_slot[0].tbl_addr_hi = cpu_to_le32((pp->cmd_tbl_dma >> 16) >> 16);
+
+	fisbuf = pp->cmd_tbl;
+	memset(fisbuf, 0, 64);
+	fisbuf[0] = 0x27;
+	fisbuf[7] = 0xa0;
+	fisbuf[15] = 0x00;
+
+	/* trigger the commands */
+	writel(0x1, port_mmio + PORT_CMD_ISSUE);
+	readl (port_mmio + PORT_CMD_ISSUE); /* flush */
+
+	udelay(20);
+    /* wait command complete */
+	for (i = 0; i < 2000; i++) {
+		tmp = readl(port_mmio + PORT_CMD_ISSUE);
+		if ((tmp & 1) == 0) break;
+		msleep(20);
+	}
+
+	if (i == 2000) {
+		printk(KERN_WARNING "TimeOut for Wait Command complete\n");
+		return 1;
+	}
+
+	// enable port interrupt
+	writel(0xffffffff, port_mmio + PORT_IRQ_MASK);
+	readl (port_mmio + PORT_IRQ_MASK);  /* flush */
+
+	return 0;
+}
+
+static unsigned int via_ata_busy_sleep (struct ata_port *ap,
+				    unsigned long tmout_pat,
+			    	    unsigned long tmout)
+{
+	unsigned long timer_start, timeout;
+	u8 status;
+
+	status = ata_busy_wait(ap, ATA_BUSY, 300);
+	timer_start = jiffies;
+	timeout = timer_start + tmout_pat;
+	while ((status & ATA_BUSY) && (time_before(jiffies, timeout))) {
+		msleep(50);
+		status = ata_busy_wait(ap, ATA_BUSY, 3);
+	}
+
+	if (status & ATA_BUSY)
+		printk(KERN_WARNING "ata%u is slow to respond, "
+		       "please be patient\n", ap->id);
+
+	timeout = timer_start + tmout;
+	while ((status & ATA_BUSY) && (time_before(jiffies, timeout))) {
+		msleep(50);
+		status = ata_chk_status(ap);
+	}
+
+	if (status & ATA_BUSY) {
+		printk(KERN_ERR "ata%u failed to respond (%lu secs)\n",
+		       ap->id, tmout / HZ);
+		return 1;
+	}
+
+	return 0;
+}
+
+static void via_ahci_phy_reset(struct ata_port *ap)
+{
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
+	struct ata_taskfile tf;
+	struct ata_device *dev = &ap->device[0];
+	u32 tmp;
+
+	u32 sstatus,i;
+	unsigned long timeout = jiffies + (HZ * 5);
+	u8 tmp_status;
+	
+	if (ap->flags & ATA_FLAG_SATA_RESET) {
+		/* issue phy wake/reset */
+		scr_write_flush(ap, SCR_CONTROL, 0x301);
+		udelay(400);			/* FIXME: a guess */
+	}
+	scr_write_flush(ap, SCR_CONTROL, 0x300); /* phy wake/clear reset */
+
+	/* wait for phy to become ready, if necessary */
+	do {
+		msleep(200);
+		sstatus = scr_read(ap, SCR_STATUS);
+		if ((sstatus & 0xf) != 1)
+			break;
+	} while (time_before(jiffies, timeout));
+
+	/* TODO: phy layer with polling, timeouts, etc. */
+	if (sata_dev_present(ap))
+		ata_port_probe(ap);
+	else {
+		sstatus = scr_read(ap, SCR_STATUS);
+		printk(KERN_INFO "ata%u: no device found (phy stat %08x)\n",
+		       ap->id, sstatus);
+		ata_port_disable(ap);
+	}
+
+	if (ap->flags & ATA_FLAG_PORT_DISABLED)
+		return;
+
+	/*Fix the VIA busy bug by a software reset*/
+	for (i = 0; i < 100; i++) {
+		tmp_status = ap->ops->check_status(ap);
+		if ((tmp_status & ATA_BUSY) == 0) break;
+		msleep(10);
+	}
+
+	if ((tmp_status & ATA_BUSY)) {
+		DPRINTK("Busy after CommReset, do softreset...\n"); 
+		/*set the PxCMD.CLO bit to clear BUSY and DRQ, to make the reset command sent out*/
+		tmp = readl(port_mmio + PORT_CMD);
+		tmp |= PORT_CMD_CLO;
+		writel(tmp, port_mmio + PORT_CMD);
+		readl(port_mmio + PORT_CMD); /* flush */
+
+		if (via_ahci_softreset(ap)) {
+			printk(KERN_WARNING "softreset failed\n");
+			return;
+		}
+	}
+
+	if (via_ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
+		ata_port_disable(ap);
+		return;
+	}
+
+	ap->cbl = ATA_CBL_SATA;
+
+	if (ap->flags & ATA_FLAG_PORT_DISABLED)
+		return;
+
+	tmp = readl(port_mmio + PORT_SIG);
+	tf.lbah		= (tmp >> 24)	& 0xff;
+	tf.lbam		= (tmp >> 16)	& 0xff;
+	tf.lbal		= (tmp >> 8)	& 0xff;
+	tf.nsect	= (tmp)		& 0xff;
+
+	dev->class = ata_dev_classify(&tf);
+	if (!ata_dev_present(dev))
+		ata_port_disable(ap);
+
+}
+
+static int via_ahci_qc_issue(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	void *port_mmio = (void *) ap->ioaddr.cmd_addr;
+
+    if (qc &&
+        qc->tf.command == ATA_CMD_SET_FEATURES &&
+        qc->tf.feature == SETFEATURES_XFER) {
+        /* skip set xfer mode process */
+
+		ata_qc_complete(qc, 0);
+		qc = NULL;
+        return 0;
+    }
+
+	writel(1, port_mmio + PORT_CMD_ISSUE);
+	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
+
+	return 0;
+}
+
+static void via_ahci_port_stop(struct ata_port *ap)
+{
+	struct device *dev = ap->host_set->dev;
+	struct ahci_port_priv *pp = ap->private_data;
+
+	/* spec says 500 msecs for each PORT_CMD_{START,FIS_RX} bit, so
+	 * this is slightly incorrect.
+	 */
+	msleep(500);
+
+	ap->private_data = NULL;
+	dma_free_coherent(dev, AHCI_PORT_PRIV_DMA_SZ,
+			  pp->cmd_slot, pp->cmd_slot_dma);
+	kfree(pp);
+	ata_port_stop(ap);
+}
+
+/* END: patch code for VIA VT8251 ahci controller */
 
 static int __init ahci_init(void)
 {

Re: [PATCH] AHCI SATA vendor update from VIA

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 3037 bytes --]

On Sun, 11 Dec 2005 23:09:54 -0500 Jeff Garzik wrote:

> I received this ahci.c patch from VIA, and pass it on for review, 
> comment, and testing.
> 
> This patch won't go in as-is, because it does too much special casing. 
> But until we get around to that, people with VIA controllers probably 
> want this...
> 
> 	Jeff

What is needed to get the VT8251 support into the kernel tree?

I have looked at the patch, and it basically does three things:

1) Apparently the VT8251 hardware does not like the standard reset
   sequence performed by __sata_phy_reset() - the phy seems to become
   ready, but the ATA_BUSY bit never goes off.  So the patch authors
   just duplicated ahci_phy_reset(), inserted the whole code of
   __sata_phy_reset() in there, and added this part before the
   ata_busy_sleep() call:

+	/*Fix the VIA busy bug by a software reset*/
+	for (i = 0; i < 100; i++) {
+		tmp_status = ap->ops->check_status(ap);
+		if ((tmp_status & ATA_BUSY) == 0) break;
+		msleep(10);
+	}
+
+	if ((tmp_status & ATA_BUSY)) {
+		DPRINTK("Busy after CommReset, do softreset...\n"); 
+		/*set the PxCMD.CLO bit to clear BUSY and DRQ, to make the reset command sent out*/
+		tmp = readl(port_mmio + PORT_CMD);
+		tmp |= PORT_CMD_CLO;
+		writel(tmp, port_mmio + PORT_CMD);
+		readl(port_mmio + PORT_CMD); /* flush */
+
+		if (via_ahci_softreset(ap)) {
+			printk(KERN_WARNING "softreset failed\n");
+			return;
+		}
+	}

   Now, if this is really a chip bug, we don't have any choice except
   adding this workaround, but obviously not in this way.  What do you
   think about splitting __sata_phy_reset() in two parts -
   __sata_phy_reset_start() (everything up to the point where
   ata_busy_sleep() is called) and __sata_phy_reset_end()
   (ata_busy_sleep() and the rest), so that the low-level driver could
   insert its own code between these parts?  Or should a hook for this
   be added to ->ops instead?

2) via_ahci_qc_issue really just filters out the SETFEATURES_XFER
   command; only VIA can tell why this is needed, and is there a better
   way to do this.  However, at least some duplicated code could be
   removed easily:

static int via_ahci_qc_issue(struct ata_queued_cmd *qc)
{
	if (qc && qc->tf.command == ATA_CMD_SET_FEATURES &&
	    qc->tf.feature == SETFEATURES_XFER) {
		/* skip set xfer mode process */
		ata_qc_complete(qc);
		return 0;
	}
	return ahci_qc_issue(qc);
}

   Would this be acceptable?

3) What via_ahci_port_stop() does, I just don't understand - it is
   basically a copy of ahci_port_stop() at that time, but with clearing
   of the PORT_CMD bits removed - so nothing is stopped actually.
   Again, only VIA can tell why is this needed, but this part of the
   patch looks like a bug.

Geoff Rivell (CC'ed) tried to update the patch for 2.6.15:

http://grivell.home.comcast.net/via_ahci.patch

However, that patch does some things in a different order from the VIA
code - it calls via_ahci_softreset() before __sata_phy_reset(), which
does not seem safe to me.  Geoff, does this really work?

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] AHCI SATA vendor update from VIA

[Jeff Garzik]

Sergey Vlasov wrote:
> What is needed to get the VT8251 support into the kernel tree?

1) Doing what you are doing:  asking questions like this.  :)

2) Watching Tejun Heo's reset work.  He already has an AHCI soft reset 
patch, and the VIA AHCI work really depends on this.


> I have looked at the patch, and it basically does three things:
> 
> 1) Apparently the VT8251 hardware does not like the standard reset
>    sequence performed by __sata_phy_reset() - the phy seems to become
>    ready, but the ATA_BUSY bit never goes off.  So the patch authors
>    just duplicated ahci_phy_reset(), inserted the whole code of
>    __sata_phy_reset() in there, and added this part before the
>    ata_busy_sleep() call:
> 
> +	/*Fix the VIA busy bug by a software reset*/
> +	for (i = 0; i < 100; i++) {
> +		tmp_status = ap->ops->check_status(ap);
> +		if ((tmp_status & ATA_BUSY) == 0) break;
> +		msleep(10);
> +	}
> +
> +	if ((tmp_status & ATA_BUSY)) {
> +		DPRINTK("Busy after CommReset, do softreset...\n"); 
> +		/*set the PxCMD.CLO bit to clear BUSY and DRQ, to make the reset command sent out*/
> +		tmp = readl(port_mmio + PORT_CMD);
> +		tmp |= PORT_CMD_CLO;
> +		writel(tmp, port_mmio + PORT_CMD);
> +		readl(port_mmio + PORT_CMD); /* flush */
> +
> +		if (via_ahci_softreset(ap)) {
> +			printk(KERN_WARNING "softreset failed\n");
> +			return;
> +		}
> +	}
> 
>    Now, if this is really a chip bug, we don't have any choice except
>    adding this workaround, but obviously not in this way.  What do you
>    think about splitting __sata_phy_reset() in two parts -
>    __sata_phy_reset_start() (everything up to the point where
>    ata_busy_sleep() is called) and __sata_phy_reset_end()
>    (ata_busy_sleep() and the rest), so that the low-level driver could
>    insert its own code between these parts?  Or should a hook for this
>    be added to ->ops instead?

Tejun's stuff broke up this sequence, so it should be much easier to 
utilize his new reset code (in libata-dev.git#upstream, queued for 2.6.17).


> 2) via_ahci_qc_issue really just filters out the SETFEATURES_XFER
>    command; only VIA can tell why this is needed, and is there a better
>    way to do this.  However, at least some duplicated code could be
>    removed easily:
> 
> static int via_ahci_qc_issue(struct ata_queued_cmd *qc)
> {
> 	if (qc && qc->tf.command == ATA_CMD_SET_FEATURES &&
> 	    qc->tf.feature == SETFEATURES_XFER) {
> 		/* skip set xfer mode process */
> 		ata_qc_complete(qc);
> 		return 0;
> 	}
> 	return ahci_qc_issue(qc);
> }
> 
>    Would this be acceptable?

I wonder first if this actually solves some problems.  I would prefer to 
-not- do this, and see what happens.


> 3) What via_ahci_port_stop() does, I just don't understand - it is
>    basically a copy of ahci_port_stop() at that time, but with clearing
>    of the PORT_CMD bits removed - so nothing is stopped actually.
>    Again, only VIA can tell why is this needed, but this part of the
>    patch looks like a bug.

As your instinct seems to be, I would prefer to avoid this change if 
possible.

	Jeff

Re: [PATCH] AHCI SATA vendor update from VIA

[Tejun Heo]

Hello,

Jeff Garzik wrote:
> Sergey Vlasov wrote:
>> What is needed to get the VT8251 support into the kernel tree?
> 
> 1) Doing what you are doing:  asking questions like this.  :)
> 
> 2) Watching Tejun Heo's reset work.  He already has an AHCI soft reset 
> patch, and the VIA AHCI work really depends on this.
> 

BTW, what happened to AHCI softreset patch. It got acked[1], but it has 
not made into the tree yet. Do you want me to regenerate it against the 
current tree? Or is there anything holding it from going into the tree?

> 
>> I have looked at the patch, and it basically does three things:
>>
>> 1) Apparently the VT8251 hardware does not like the standard reset
>>    sequence performed by __sata_phy_reset() - the phy seems to become
>>    ready, but the ATA_BUSY bit never goes off.  So the patch authors
>>    just duplicated ahci_phy_reset(), inserted the whole code of
>>    __sata_phy_reset() in there, and added this part before the
>>    ata_busy_sleep() call:
>>
>> +    /*Fix the VIA busy bug by a software reset*/
>> +    for (i = 0; i < 100; i++) {
>> +        tmp_status = ap->ops->check_status(ap);
>> +        if ((tmp_status & ATA_BUSY) == 0) break;
>> +        msleep(10);
>> +    }
>> +
>> +    if ((tmp_status & ATA_BUSY)) {
>> +        DPRINTK("Busy after CommReset, do softreset...\n"); +        
>> /*set the PxCMD.CLO bit to clear BUSY and DRQ, to make the reset 
>> command sent out*/
>> +        tmp = readl(port_mmio + PORT_CMD);
>> +        tmp |= PORT_CMD_CLO;
>> +        writel(tmp, port_mmio + PORT_CMD);
>> +        readl(port_mmio + PORT_CMD); /* flush */
>> +
>> +        if (via_ahci_softreset(ap)) {
>> +            printk(KERN_WARNING "softreset failed\n");
>> +            return;
>> +        }
>> +    }
>>
>>    Now, if this is really a chip bug, we don't have any choice except
>>    adding this workaround, but obviously not in this way.  What do you
>>    think about splitting __sata_phy_reset() in two parts -
>>    __sata_phy_reset_start() (everything up to the point where
>>    ata_busy_sleep() is called) and __sata_phy_reset_end()
>>    (ata_busy_sleep() and the rest), so that the low-level driver could
>>    insert its own code between these parts?  Or should a hook for this
>>    be added to ->ops instead?
> 
> Tejun's stuff broke up this sequence, so it should be much easier to 
> utilize his new reset code (in libata-dev.git#upstream, queued for 2.6.17).
> 

If hardreset never works on via ahci, simply omitting hardreset in 
ahci_probe_reset() routine for that controller should do the job (with 
the AHCI softreset patch applied of course).

-- 
tejun

[1] http://thread.gmane.org/gmane.linux.ide/7952

Re: [PATCH] AHCI SATA vendor update from VIA

[Jeff Garzik]

Tejun Heo wrote:
> Hello,
> 
> Jeff Garzik wrote:
> 
>> Sergey Vlasov wrote:
>>
>>> What is needed to get the VT8251 support into the kernel tree?
>>
>>
>> 1) Doing what you are doing:  asking questions like this.  :)
>>
>> 2) Watching Tejun Heo's reset work.  He already has an AHCI soft reset 
>> patch, and the VIA AHCI work really depends on this.
>>
> 
> BTW, what happened to AHCI softreset patch. It got acked[1], but it has 
> not made into the tree yet. Do you want me to regenerate it against the 
> current tree? Or is there anything holding it from going into the tree?

Please resend, the only pending patch I have from you is the ATA 
transport class patch (thanks for doing that BTW), which is on hold 
waiting for SCSI updates.

	Jeff

[PATCH] ahci: add softreset

[Tejun Heo]

Now that libata is smart enought to handle both soft and hard resets,
add softreset method.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

There has been no relevant change for AHCI since last post.  The
following patch is identical to the last posting.

 ahci.c |  135 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 134 insertions(+), 1 deletion(-)

--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -513,6 +513,138 @@ static void ahci_fill_cmd_slot(struct ah
 	pp->cmd_slot[0].tbl_addr_hi = cpu_to_le32((pp->cmd_tbl_dma >> 16) >> 16);
 }
 
+static int ahci_poll_register(void __iomem *reg, u32 mask, u32 val,
+			      unsigned long interval_msec,
+			      unsigned long timeout_msec)
+{
+	unsigned long timeout;
+	u32 tmp;
+
+	timeout = jiffies + (timeout_msec * HZ) / 1000;
+	do {
+		tmp = readl(reg);
+		if ((tmp & mask) == val)
+			return 0;
+		msleep(interval_msec);
+	} while (time_before(jiffies, timeout));
+
+	return -1;
+}
+
+static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+{
+	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	void __iomem *mmio = ap->host_set->mmio_base;
+	void __iomem *port_mmio = ahci_port_base(mmio, ap->port_no);
+	const u32 cmd_fis_len = 5; /* five dwords */
+	const char *reason = NULL;
+	struct ata_taskfile tf;
+	u8 *fis;
+	int rc;
+
+	DPRINTK("ENTER\n");
+
+	/* prepare for SRST (AHCI-1.1 10.4.1) */
+	rc = ahci_stop_engine(ap);
+	if (rc) {
+		reason = "failed to stop engine";
+		goto fail_restart;
+	}
+
+	/* check BUSY/DRQ, perform Command List Override if necessary */
+	ahci_tf_read(ap, &tf);
+	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
+		u32 tmp;
+
+		if (!(hpriv->cap & HOST_CAP_CLO)) {
+			rc = -EIO;
+			reason = "port busy but no CLO";
+			goto fail_restart;
+		}
+
+		tmp = readl(port_mmio + PORT_CMD);
+		tmp |= PORT_CMD_CLO;
+		writel(tmp, port_mmio + PORT_CMD);
+		readl(port_mmio + PORT_CMD); /* flush */
+
+		if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0,
+				       1, 500)) {
+			rc = -EIO;
+			reason = "CLO failed";
+			goto fail_restart;
+		}
+	}
+
+	/* restart engine */
+	ahci_start_engine(ap);
+
+	ata_tf_init(ap, &tf, 0);
+	fis = pp->cmd_tbl;
+
+	/* issue the first D2H Register FIS */
+	ahci_fill_cmd_slot(pp, cmd_fis_len | AHCI_CMD_RESET | AHCI_CMD_CLR_BUSY);
+
+	tf.ctl |= ATA_SRST;
+	ata_tf_to_fis(&tf, fis, 0);
+	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
+
+	writel(1, port_mmio + PORT_CMD_ISSUE);
+	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
+
+	if (ahci_poll_register(port_mmio + PORT_CMD_ISSUE, 0x1, 0x0, 1, 500)) {
+		rc = -EIO;
+		reason = "1st FIS failed";
+		goto fail;
+	}
+
+	/* spec says at least 5us, but be generous and sleep for 1ms */
+	msleep(1);
+
+	/* issue the second D2H Register FIS */
+	ahci_fill_cmd_slot(pp, cmd_fis_len);
+
+	tf.ctl &= ~ATA_SRST;
+	ata_tf_to_fis(&tf, fis, 0);
+	fis[1] &= ~(1 << 7);	/* turn off Command FIS bit */
+
+	writel(1, port_mmio + PORT_CMD_ISSUE);
+	readl(port_mmio + PORT_CMD_ISSUE);	/* flush */
+
+	/* spec mandates ">= 2ms" before checking status.
+	 * We wait 150ms, because that was the magic delay used for
+	 * ATAPI devices in Hale Landis's ATADRVR, for the period of time
+	 * between when the ATA command register is written, and then
+	 * status is checked.  Because waiting for "a while" before
+	 * checking status is fine, post SRST, we perform this magic
+	 * delay here as well.
+	 */
+	msleep(150);
+
+	*class = ATA_DEV_NONE;
+	if (sata_dev_present(ap)) {
+		if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
+			rc = -EIO;
+			reason = "device not ready";
+			goto fail;
+		}
+		*class = ahci_dev_classify(ap);
+	}
+
+	DPRINTK("EXIT, class=%u\n", *class);
+	return 0;
+
+ fail_restart:
+	ahci_start_engine(ap);
+ fail:
+	if (verbose)
+		printk(KERN_ERR "ata%u: softreset failed (%s)\n",
+		       ap->id, reason);
+	else
+		DPRINTK("EXIT, rc=%d reason=\"%s\"\n", rc, reason);
+	return rc;
+}
+
 static int ahci_hardreset(struct ata_port *ap, int verbose, unsigned int *class)
 {
 	int rc;
@@ -553,7 +685,8 @@ static void ahci_postreset(struct ata_po
 
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
-	return ata_drive_probe_reset(ap, NULL, NULL, ahci_hardreset,
+	return ata_drive_probe_reset(ap, ata_std_probeinit,
+				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);
 }
 

Re: [PATCH] ahci: add softreset

[Jeff Garzik]

Tejun Heo wrote:
> Now that libata is smart enought to handle both soft and hard resets,
> add softreset method.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied