[PATCH/RFC] ahci: add support for VIA VT8251

[PATCH/RFC] ahci: add support for VIA VT8251

[Bastiaan Jacques]

This patch adds AHCI support for the VIA VT8251 chipset. The patch does so by:
	1) Adding the PCI device ID.
	2) Adding a workaround in ahci_probe_reset() for a VIA hardware bug.

Signed-off-by: Bastiaan Jacques <b.jacques@planet.nl>
---

The patch is based upon the patch[1] provided by VIA. With the softreset and 
probing work that has been merged, this patch has become much simpler.

I am happy to report that without the workaround in ahci_probe_reset(), the 
chipset works quite well; both my SATA drives are recognised. However, 
softreset does not work (but hardreset does), and ata_std_probeinit delays, 
for my primary SATA drive/port,  due to the "busy" bug. A Command List 
Override will unbusy the drive/port.

Since the CLO sequence also appears in ahci_softreset(), I have moved that 
code into a new ahci_clo() function.

This patch is against 2.6.17-rc1 (and -mm2).

This patch has only been tested by yours truly. Whilst I am waiting for 
feedback from other people, I am sending this patch to solicit comments.

NOTE: My email client cuts off some lines (of context) in my patch because 
they are longer than 78 characters. Therefore, I am making the patch 
available from my web server [2], as well.

Bastiaan

[1] http://lkml.org/lkml/2005/12/11/204
[2] http://mirrors.pagefault.net/linux-via-ahci-latest.patch

--- drivers/scsi/ahci.c.orig	2006-04-11 18:58:32.000000000 +0200
+++ drivers/scsi/ahci.c	2006-04-11 12:44:41.000000000 +0200
@@ -297,6 +297,8 @@
 	  board_ahci }, /* ATI SB600 non-raid */
 	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_ahci }, /* ATI SB600 raid */
+	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  board_ahci }, /* VT8251 */
 	{ }	/* terminate list */
 };

@@ -535,6 +537,17 @@
 	return -1;
 }

+static void ahci_clo(struct ata_port *ap)
+{
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
+	u32 tmp;
+
+	tmp = readl(port_mmio + PORT_CMD);
+	tmp |= PORT_CMD_CLO;
+	writel(tmp, port_mmio + PORT_CMD);
+	readl(port_mmio + PORT_CMD); /* flush */
+}
+
 static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int 
*class)
 {
 	struct ahci_host_priv *hpriv = ap->host_set->private_data;
@@ -565,7 +578,6 @@
 	/* 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;
@@ -573,10 +585,7 @@
 			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 */
+		ahci_clo(ap);

 		if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0,
 				       1, 500)) {
@@ -695,6 +704,16 @@

 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
+	struct pci_dev *pdev = to_pci_dev(ap->host_set->dev);
+
+	/* Workaround for VIA VT8251 "busy" bug */
+	if (pdev->device == 0x3349) {
+		if (ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY) {
+			/* ATA_BUSY hasn't cleared, so send a CLO */
+			ahci_clo(ap);
+		}
+	}
+
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
 				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);

Re: [PATCH/RFC] ahci: add support for VIA VT8251

[Sergey Vlasov]

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

On Tue, 11 Apr 2006 19:15:45 +0200 Bastiaan Jacques wrote:

> This patch adds AHCI support for the VIA VT8251 chipset. The patch does so by:
> 	1) Adding the PCI device ID.
> 	2) Adding a workaround in ahci_probe_reset() for a VIA hardware bug.
> 
> Signed-off-by: Bastiaan Jacques <b.jacques@planet.nl>
> ---
> 
> The patch is based upon the patch[1] provided by VIA. With the softreset and 
> probing work that has been merged, this patch has become much simpler.
> 
> I am happy to report that without the workaround in ahci_probe_reset(), the 
> chipset works quite well; both my SATA drives are recognised. However, 
> softreset does not work (but hardreset does), and ata_std_probeinit delays, 
> for my primary SATA drive/port,  due to the "busy" bug. A Command List 
> Override will unbusy the drive/port.
> 
> Since the CLO sequence also appears in ahci_softreset(), I have moved that 
> code into a new ahci_clo() function.
> 
> This patch is against 2.6.17-rc1 (and -mm2).
> 
> This patch has only been tested by yours truly. Whilst I am waiting for 
> feedback from other people, I am sending this patch to solicit comments.
> 
> NOTE: My email client cuts off some lines (of context) in my patch because 
> they are longer than 78 characters. Therefore, I am making the patch 
> available from my web server [2], as well.
> 
> Bastiaan
> 
> [1] http://lkml.org/lkml/2005/12/11/204
> [2] http://mirrors.pagefault.net/linux-via-ahci-latest.patch
> 
> --- drivers/scsi/ahci.c.orig	2006-04-11 18:58:32.000000000 +0200
> +++ drivers/scsi/ahci.c	2006-04-11 12:44:41.000000000 +0200
> @@ -297,6 +297,8 @@
>  	  board_ahci }, /* ATI SB600 non-raid */
>  	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
>  	  board_ahci }, /* ATI SB600 raid */
> +	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  board_ahci }, /* VT8251 */

You can add a separate host type (e.g., board_via_vt8251_ahci), and then
add an entry for this type to ahci_port_info[].  The entry would have a
special flag in .host_flags, but the same .port_ops, so there will not
be excessive duplication like in the original vendor patch.

Look like other drivers which need workarounds for hardware bugs (e.g.,
sata_sil) do this.

>  	{ }	/* terminate list */
>  };
> 
> @@ -535,6 +537,17 @@
>  	return -1;
>  }
> 
> +static void ahci_clo(struct ata_port *ap)
> +{
> +	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
> +	u32 tmp;
> +
> +	tmp = readl(port_mmio + PORT_CMD);
> +	tmp |= PORT_CMD_CLO;
> +	writel(tmp, port_mmio + PORT_CMD);
> +	readl(port_mmio + PORT_CMD); /* flush */
> +}
> +
>  static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int 
> *class)
>  {
>  	struct ahci_host_priv *hpriv = ap->host_set->private_data;
> @@ -565,7 +578,6 @@
>  	/* check BUSY/DRQ, perform Command List Override if necessary */
>  	ahci_tf_read(ap, &tf);
>  	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
> -		u32 tmp;
> 

The following empty line should probably be deleted too.

>  		if (!(hpriv->cap & HOST_CAP_CLO)) {
>  			rc = -EIO;
> @@ -573,10 +585,7 @@
>  			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 */
> +		ahci_clo(ap);
> 
>  		if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0,
>  				       1, 500)) {
> @@ -695,6 +704,16 @@
> 
>  static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
>  {
> +	struct pci_dev *pdev = to_pci_dev(ap->host_set->dev);
> +
> +	/* Workaround for VIA VT8251 "busy" bug */
> +	if (pdev->device == 0x3349) {

After adding a separate host type and a host flag this will be:

	if (ap->flags & AHCI_FLAG_RESET_NEEDS_CLO) {

> +		if (ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY) {
> +			/* ATA_BUSY hasn't cleared, so send a CLO */
> +			ahci_clo(ap);
> +		}
> +	}
> +
>  	return ata_drive_probe_reset(ap, ata_std_probeinit,
>  				     ahci_softreset, ahci_hardreset,
>  				     ahci_postreset, classes);

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

Re: [PATCH/RFC] ahci: add support for VIA VT8251

[Bastiaan Jacques]

On Tuesday 11 April 2006 19:41, Sergey Vlasov wrote:
> > @@ -565,7 +578,6 @@
> >  	/* check BUSY/DRQ, perform Command List Override if necessary */
> >  	ahci_tf_read(ap, &tf);
> >  	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
> > -		u32 tmp;
>
> The following empty line should probably be deleted too.

Done.

> > +	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > +	  board_ahci }, /* VT8251 */
>
> You can add a separate host type (e.g., board_via_vt8251_ahci), and then
> add an entry for this type to ahci_port_info[].  The entry would have a
> special flag in .host_flags, but the same .port_ops, so there will not
> be excessive duplication like in the original vendor patch.
>
> Look like other drivers which need workarounds for hardware bugs (e.g.,
> sata_sil) do this.

I noticed there already is a similar mechanism in ahci.c (see AHCI_FLAG_MSI). 
I decided to use that instead. Will that be acceptable?


--- drivers/scsi/ahci.c.orig	2006-04-11 18:58:32.000000000 +0200
+++ drivers/scsi/ahci.c	2006-04-11 22:05:37.000000000 +0200
@@ -152,7 +152,8 @@ enum {
 	PORT_CMD_ICC_SLUMBER	= (0x6 << 28), /* Put i/f in slumber state */
 
 	/* hpriv->flags bits */
-	AHCI_FLAG_MSI		= (1 << 0),
+	AHCI_FLAG_MSI			= (1 << 0),
+	AHCI_FLAG_RESET_NEEDS_CLO 	= (1 << 1),
 };
 
 struct ahci_cmd_hdr {
@@ -297,6 +298,8 @@ static const struct pci_device_id ahci_p
 	  board_ahci }, /* ATI SB600 non-raid */
 	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_ahci }, /* ATI SB600 raid */
+	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  board_ahci }, /* VT8251 */
 	{ }	/* terminate list */
 };
 
@@ -535,6 +538,17 @@ static int ahci_poll_register(void __iom
 	return -1;
 }
 
+static void ahci_clo(struct ata_port *ap)
+{
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
+	u32 tmp;
+
+	tmp = readl(port_mmio + PORT_CMD);
+	tmp |= PORT_CMD_CLO;
+	writel(tmp, port_mmio + PORT_CMD);
+	readl(port_mmio + PORT_CMD); /* flush */
+}
+
 static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int 
*class)
 {
 	struct ahci_host_priv *hpriv = ap->host_set->private_data;
@@ -565,18 +579,13 @@ static int ahci_softreset(struct ata_por
 	/* 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 */
+		ahci_clo(ap);
 
 		if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0,
 				       1, 500)) {
@@ -695,6 +704,15 @@ static void ahci_postreset(struct ata_po
 
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
+	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+
+	if (hpriv->flags & AHCI_FLAG_RESET_NEEDS_CLO) {
+		if (ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY) {
+			/* ATA_BUSY hasn't cleared, so send a CLO */
+			ahci_clo(ap);
+		}
+	}
+
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
 				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);
@@ -1266,6 +1284,10 @@ static int ahci_init_one (struct pci_dev
 	if (pdev->vendor == 0x197b)
 		pci_write_config_byte(pdev, 0x41, 0xa1);
 
+	/* VIA VT8251-specific fixup: CLO before reset */
+	if (pdev->device == 0x3349)
+		hpriv->flags |= AHCI_FLAG_RESET_NEEDS_CLO;
+
 	/* initialize adapter */
 	rc = ahci_host_init(probe_ent);
 	if (rc)

Re: [PATCH/RFC] ahci: add support for VIA VT8251

[Jeff Garzik]

Bastiaan Jacques wrote:
> This patch adds AHCI support for the VIA VT8251 chipset. The patch does so by:
> 	1) Adding the PCI device ID.
> 	2) Adding a workaround in ahci_probe_reset() for a VIA hardware bug.
> 
> Signed-off-by: Bastiaan Jacques <b.jacques@planet.nl>
> ---
> 
> The patch is based upon the patch[1] provided by VIA. With the softreset and 
> probing work that has been merged, this patch has become much simpler.
> 
> I am happy to report that without the workaround in ahci_probe_reset(), the 
> chipset works quite well; both my SATA drives are recognised. However, 
> softreset does not work (but hardreset does), and ata_std_probeinit delays, 
> for my primary SATA drive/port,  due to the "busy" bug. A Command List 
> Override will unbusy the drive/port.
> 
> Since the CLO sequence also appears in ahci_softreset(), I have moved that 
> code into a new ahci_clo() function.
> 
> This patch is against 2.6.17-rc1 (and -mm2).
> 
> This patch has only been tested by yours truly. Whilst I am waiting for 
> feedback from other people, I am sending this patch to solicit comments.

Thanks for doing this.  VIA AHCI support was indeed waiting on Tejun's 
EH work.

Would you please resubmit the latest version of your patch including a 
Signed-off-by line, per guidelines at 
http://linux.yyz.us/patch-format.html ?

	Jeff

Re: [PATCH/RFC] ahci: add support for VIA VT8251

[Tejun Heo]

Hello, Bastiaan.

On Tue, Apr 11, 2006 at 07:15:45PM +0200, Bastiaan Jacques wrote:
> +static void ahci_clo(struct ata_port *ap)
> +{
> +	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
> +	u32 tmp;
> +
> +	tmp = readl(port_mmio + PORT_CMD);
> +	tmp |= PORT_CMD_CLO;
> +	writel(tmp, port_mmio + PORT_CMD);
> +	readl(port_mmio + PORT_CMD); /* flush */
> +}

Can you please make ahci_clo() check for HOST_CAP_CLO and return
-EOPNOTSUPP (or whatever) if it's not supported?

Thanks.

-- 
tejun

Re: [PATCH/RFC] ahci: add support for VIA VT8251

[Bastiaan Jacques]

On Wednesday 12 April 2006 02:42, Tejun Heo wrote:
> Can you please make ahci_clo() check for HOST_CAP_CLO and return
> -EOPNOTSUPP (or whatever) if it's not supported?

Sure. I've also moved the ahci_poll_register() call into ahci_clo().

Jeff: when everybody is happy with my patch, I'll resubmit it to this list in
accordance with the guidelines you linked to (if that's okay with you).


--- drivers/scsi/ahci.c.orig	2006-04-11 18:58:32.000000000 +0200
+++ drivers/scsi/ahci.c	2006-04-12 20:53:23.000000000 +0200
@@ -152,7 +152,8 @@
 	PORT_CMD_ICC_SLUMBER	= (0x6 << 28), /* Put i/f in slumber state */
 
 	/* hpriv->flags bits */
-	AHCI_FLAG_MSI		= (1 << 0),
+	AHCI_FLAG_MSI			= (1 << 0),
+	AHCI_FLAG_RESET_NEEDS_CLO 	= (1 << 1),
 };
 
 struct ahci_cmd_hdr {
@@ -297,6 +298,8 @@
 	  board_ahci }, /* ATI SB600 non-raid */
 	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_ahci }, /* ATI SB600 raid */
+	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  board_ahci }, /* VT8251 */
 	{ }	/* terminate list */
 };
 
@@ -535,9 +538,28 @@
 	return -1;
 }
 
-static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+static int ahci_clo(struct ata_port *ap)
 {
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
 	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	u32 tmp;
+
+	if (!(hpriv->cap & HOST_CAP_CLO))
+		return -EOPNOTSUPP;
+
+	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))
+		return -EIO;
+
+	return 0;
+}
+
+static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+{
 	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);
@@ -565,23 +587,12 @@
 	/* check BUSY/DRQ, perform Command List Override if necessary */
 	ahci_tf_read(ap, &tf);
 	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
-		u32 tmp;
+		if ((rc = ahci_clo(ap))) {
+			if (rc & -EOPNOTSUPP)
+				reason = "port busy but CLO unavailable";
+			else
+				reason = "port busy but CLO failed";
 
-		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;
 		}
 	}
@@ -695,6 +706,15 @@
 
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
+	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+
+	if (hpriv->flags & AHCI_FLAG_RESET_NEEDS_CLO) {
+		if (ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY) {
+			/* ATA_BUSY hasn't cleared, so send a CLO */
+			ahci_clo(ap);
+		}
+	}
+
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
 				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);
@@ -1266,6 +1286,10 @@
 	if (pdev->vendor == 0x197b)
 		pci_write_config_byte(pdev, 0x41, 0xa1);
 
+	/* VIA VT8251-specific fixup: CLO before reset */
+	if (pdev->device == 0x3349)
+		hpriv->flags |= AHCI_FLAG_RESET_NEEDS_CLO;
+
 	/* initialize adapter */
 	rc = ahci_host_init(probe_ent);
 	if (rc)

Re: [PATCH/RFC] ahci: add support for VIA VT8251

[Jeff Garzik]

Bastiaan Jacques wrote:
> Jeff: when everybody is happy with my patch, I'll resubmit it to this list in
> accordance with the guidelines you linked to (if that's okay with you).

Looking pretty good so far, minor comments inline...


> --- drivers/scsi/ahci.c.orig	2006-04-11 18:58:32.000000000 +0200
> +++ drivers/scsi/ahci.c	2006-04-12 20:53:23.000000000 +0200
> @@ -152,7 +152,8 @@
>  	PORT_CMD_ICC_SLUMBER	= (0x6 << 28), /* Put i/f in slumber state */
>  
>  	/* hpriv->flags bits */
> -	AHCI_FLAG_MSI		= (1 << 0),
> +	AHCI_FLAG_MSI			= (1 << 0),
> +	AHCI_FLAG_RESET_NEEDS_CLO 	= (1 << 1),
>  };
>  
>  struct ahci_cmd_hdr {
> @@ -297,6 +298,8 @@
>  	  board_ahci }, /* ATI SB600 non-raid */
>  	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
>  	  board_ahci }, /* ATI SB600 raid */
> +	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  board_ahci }, /* VT8251 */
>  	{ }	/* terminate list */
>  };
>  
> @@ -535,9 +538,28 @@
>  	return -1;
>  }
>  
> -static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
> +static int ahci_clo(struct ata_port *ap)
>  {
> +	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
>  	struct ahci_host_priv *hpriv = ap->host_set->private_data;
> +	u32 tmp;
> +
> +	if (!(hpriv->cap & HOST_CAP_CLO))
> +		return -EOPNOTSUPP;
> +
> +	tmp = readl(port_mmio + PORT_CMD);
> +	tmp |= PORT_CMD_CLO;
> +	writel(tmp, port_mmio + PORT_CMD);
> +	readl(port_mmio + PORT_CMD); /* flush */

ahci_poll_register() should take care of flush for you.


> +	if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0, 1, 500))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
> +{
>  	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);
> @@ -565,23 +587,12 @@
>  	/* check BUSY/DRQ, perform Command List Override if necessary */
>  	ahci_tf_read(ap, &tf);
>  	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
> -		u32 tmp;
> +		if ((rc = ahci_clo(ap))) {

Don't combine an assignment and an 'if' test in the same line.

This style of statement+if is a constant source of bugs, because it is 
much more difficult for the human eye to notice that two statements are 
present rather than one, and it takes longer for the human brain to 
figure out whether the original author intended to use '==' rather than '='.


> +			if (rc & -EOPNOTSUPP)

rc isn't a bitmask, the test should be "rc == -EOPNOTSUPP"


> +				reason = "port busy but CLO unavailable";
> +			else
> +				reason = "port busy but CLO failed";
>  
> -		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;
>  		}
>  	}
> @@ -695,6 +706,15 @@
>  
>  static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
>  {
> +	struct ahci_host_priv *hpriv = ap->host_set->private_data;
> +
> +	if (hpriv->flags & AHCI_FLAG_RESET_NEEDS_CLO) {
> +		if (ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY) {

combine these two 'if' statements into a single 'if' statement with '&&'


> +			/* ATA_BUSY hasn't cleared, so send a CLO */
> +			ahci_clo(ap);
> +		}
> +	}
> +
>  	return ata_drive_probe_reset(ap, ata_std_probeinit,
>  				     ahci_softreset, ahci_hardreset,
>  				     ahci_postreset, classes);
> @@ -1266,6 +1286,10 @@
>  	if (pdev->vendor == 0x197b)
>  		pci_write_config_byte(pdev, 0x41, 0xa1);
>  
> +	/* VIA VT8251-specific fixup: CLO before reset */
> +	if (pdev->device == 0x3349)
> +		hpriv->flags |= AHCI_FLAG_RESET_NEEDS_CLO;

This is flawed because it doesn't check the PCI vendor ID.

In any case, the proper way to do this is:

1) create board_ahci_via next to board_ahci

2) duplicate entry #0 (board_ahci) of ahci_port_info[] as entry #1 
(board_ahci_via).

3) Add AHCI_FLAG_RESET_NEEDS_CLO to entry #1.

4) test ap->flags & AHCI_FLAG_RESET_NEEDS_CLO

5) when adding VIA PCI device to ahci_pci_tbl[], the final entry should 
be 'board_ahci_via'

You could do it has an hpriv->flag, but its easier and more standard to 
pass chip-specific flags in the manner I have described.

	Jeff

Re: [PATCH/RFC] ahci: add support for VIA VT8251

[Bastiaan Jacques]

On Wednesday 12 April 2006 21:25, Jeff Garzik wrote:
> > +	readl(port_mmio + PORT_CMD); /* flush */
> 
> ahci_poll_register() should take care of flush for you.

Done.

> > +		if ((rc = ahci_clo(ap))) {
> 
> Don't combine an assignment and an 'if' test in the same line.

Done.

> > +	/* VIA VT8251-specific fixup: CLO before reset */
> > +	if (pdev->device == 0x3349)
> > +		hpriv->flags |= AHCI_FLAG_RESET_NEEDS_CLO;
> 
> This is flawed because it doesn't check the PCI vendor ID.
> 
> In any case, the proper way to do this is:
> 
> 1) create board_ahci_via next to board_ahci
> 
> 2) duplicate entry #0 (board_ahci) of ahci_port_info[] as entry #1 
> (board_ahci_via).

The vendor patch already contained a similar entry which I used in this
revision. However, I modified it as follows:

a) Named it board_via_vt8251_ahci, as per Sergey Vlasov's suggestion. Future
   VIA chipsets won't necessarily require this workaround.
b) Removed ATA_FLAG_SRST from .host_flags as it appears to be deprecated (and
   soft/hardreset works anyway).
c) Left .pio_mask = 0x03, /* pio3-4 */ from vendor patch intact, as I didn't
   find anything in the AHCI specification that would require otherwise (and
   I assume VIA did this for a reason).

> 3) Add AHCI_FLAG_RESET_NEEDS_CLO to entry #1.
> 
> 4) test ap->flags & AHCI_FLAG_RESET_NEEDS_CLO
> 
> 5) when adding VIA PCI device to ahci_pci_tbl[], the final entry should 
> be 'board_ahci_via'

Done 1-5.


--- drivers/scsi/ahci.c.orig	2006-04-11 18:58:32.000000000 +0200
+++ drivers/scsi/ahci.c	2006-04-12 22:45:37.000000000 +0200
@@ -73,6 +73,7 @@ enum {
 	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
 
 	board_ahci		= 0,
+	board_via_vt8251_ahci   = 1,
 
 	/* global controller registers */
 	HOST_CAP		= 0x00, /* host capabilities */
@@ -153,6 +154,9 @@ enum {
 
 	/* hpriv->flags bits */
 	AHCI_FLAG_MSI		= (1 << 0),
+
+	/* ap->flags bits */
+	AHCI_FLAG_RESET_NEEDS_CLO	= (1 << 24),
 };
 
 struct ahci_cmd_hdr {
@@ -256,6 +260,16 @@ static const struct ata_port_info ahci_p
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
+	/* board_via_vt8251_ahci */
+	{
+		.sht		= &ahci_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
+				  AHCI_FLAG_RESET_NEEDS_CLO,
+		.pio_mask	= 0x03, /* pio3-4 */
+		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
+		.port_ops	= &ahci_ops,
+	},
 };
 
 static const struct pci_device_id ahci_pci_tbl[] = {
@@ -297,6 +311,8 @@ static const struct pci_device_id ahci_p
 	  board_ahci }, /* ATI SB600 non-raid */
 	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_ahci }, /* ATI SB600 raid */
+	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  board_via_vt8251_ahci }, /* VT8251 */
 	{ }	/* terminate list */
 };
 
@@ -535,9 +551,27 @@ static int ahci_poll_register(void __iom
 	return -1;
 }
 
-static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+static int ahci_clo(struct ata_port *ap)
 {
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
 	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	u32 tmp;
+
+	if (!(hpriv->cap & HOST_CAP_CLO))
+		return -EOPNOTSUPP;
+
+	tmp = readl(port_mmio + PORT_CMD);
+	tmp |= PORT_CMD_CLO;
+	writel(tmp, port_mmio + PORT_CMD);
+
+	if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0, 1, 500))
+		return -EIO;
+
+	return 0;
+}
+
+static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+{
 	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);
@@ -565,23 +599,13 @@ static int ahci_softreset(struct ata_por
 	/* check BUSY/DRQ, perform Command List Override if necessary */
 	ahci_tf_read(ap, &tf);
 	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
-		u32 tmp;
+		rc = ahci_clo(ap);
 
-		if (!(hpriv->cap & HOST_CAP_CLO)) {
-			rc = -EIO;
-			reason = "port busy but no CLO";
+		if (rc == -EOPNOTSUPP) {
+			reason = "port busy but CLO unavailable";
 			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";
+		} else if (rc == -EIO) {
+			reason = "port busy but CLO failed";
 			goto fail_restart;
 		}
 	}
@@ -695,6 +719,12 @@ static void ahci_postreset(struct ata_po
 
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
+	if (ap->flags & AHCI_FLAG_RESET_NEEDS_CLO &&
+	    ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY) {
+		/* ATA_BUSY hasn't cleared, so send a CLO */
+		ahci_clo(ap);
+	}
+
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
 				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);

Re: [PATCH/RFC] ahci: add support for VIA VT8251

[Jeff Garzik]

Bastiaan Jacques wrote:

Thanks, a couple remaining minor nits:


> --- drivers/scsi/ahci.c.orig	2006-04-11 18:58:32.000000000 +0200
> +++ drivers/scsi/ahci.c	2006-04-12 22:45:37.000000000 +0200
> @@ -73,6 +73,7 @@ enum {
>  	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
>  
>  	board_ahci		= 0,
> +	board_via_vt8251_ahci   = 1,

too long, and puts ahci at the end.

suggest board_ahci_vt8251.


>  	/* global controller registers */
>  	HOST_CAP		= 0x00, /* host capabilities */
> @@ -153,6 +154,9 @@ enum {
>  
>  	/* hpriv->flags bits */
>  	AHCI_FLAG_MSI		= (1 << 0),
> +
> +	/* ap->flags bits */
> +	AHCI_FLAG_RESET_NEEDS_CLO	= (1 << 24),
>  };
>  
>  struct ahci_cmd_hdr {
> @@ -256,6 +260,16 @@ static const struct ata_port_info ahci_p
>  		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
>  		.port_ops	= &ahci_ops,
>  	},
> +	/* board_via_vt8251_ahci */
> +	{
> +		.sht		= &ahci_sht,
> +		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> +				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
> +				  AHCI_FLAG_RESET_NEEDS_CLO,
> +		.pio_mask	= 0x03, /* pio3-4 */

pio mask should be the same as the previous entry (0x1f).

Never trust vendor patches, they are often patched against ancient 
libata versions.


> +		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
> +		.port_ops	= &ahci_ops,
> +	},
>  };
>  
>  static const struct pci_device_id ahci_pci_tbl[] = {
> @@ -297,6 +311,8 @@ static const struct pci_device_id ahci_p
>  	  board_ahci }, /* ATI SB600 non-raid */
>  	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
>  	  board_ahci }, /* ATI SB600 raid */
> +	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  board_via_vt8251_ahci }, /* VT8251 */
>  	{ }	/* terminate list */
>  };
>  
> @@ -535,9 +551,27 @@ static int ahci_poll_register(void __iom
>  	return -1;
>  }
>  
> -static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
> +static int ahci_clo(struct ata_port *ap)
>  {
> +	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
>  	struct ahci_host_priv *hpriv = ap->host_set->private_data;
> +	u32 tmp;
> +
> +	if (!(hpriv->cap & HOST_CAP_CLO))
> +		return -EOPNOTSUPP;
> +
> +	tmp = readl(port_mmio + PORT_CMD);
> +	tmp |= PORT_CMD_CLO;
> +	writel(tmp, port_mmio + PORT_CMD);
> +
> +	if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0, 1, 500))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
> +{
>  	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);
> @@ -565,23 +599,13 @@ static int ahci_softreset(struct ata_por
>  	/* check BUSY/DRQ, perform Command List Override if necessary */
>  	ahci_tf_read(ap, &tf);
>  	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
> -		u32 tmp;
> +		rc = ahci_clo(ap);
>  
> -		if (!(hpriv->cap & HOST_CAP_CLO)) {
> -			rc = -EIO;
> -			reason = "port busy but no CLO";
> +		if (rc == -EOPNOTSUPP) {
> +			reason = "port busy but CLO unavailable";
>  			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";
> +		} else if (rc == -EIO) {
> +			reason = "port busy but CLO failed";
>  			goto fail_restart;
>  		}
>  	}
> @@ -695,6 +719,12 @@ static void ahci_postreset(struct ata_po
>  
>  static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
>  {
> +	if (ap->flags & AHCI_FLAG_RESET_NEEDS_CLO &&

Please add parens for easy reading

> +	    ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY) {

likewise

	Jeff

[PATCH 2.6.17-rc1-mm2 1/1] ahci: add support for VIA VT8251

[Bastiaan Jacques]

Adds AHCI support for the VIA VT8251.

Includes a workaround for a hardware bug which requires a Command List
Override before reset.

Signed-off-by: Bastiaan Jacques <b.jacques@planet.nl>
---

--- linux-2.6.17-rc1-mm2.orig/drivers/scsi/ahci.c	2006-04-10 19:10:34.000000000 +0200
+++ linux-2.6.17-rc1-mm2/drivers/scsi/ahci.c	2006-04-12 23:57:26.000000000 +0200
@@ -73,6 +73,7 @@ enum {
 	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
 
 	board_ahci		= 0,
+	board_ahci_vt8251   = 1,
 
 	/* global controller registers */
 	HOST_CAP		= 0x00, /* host capabilities */
@@ -153,6 +154,9 @@ enum {
 
 	/* hpriv->flags bits */
 	AHCI_FLAG_MSI		= (1 << 0),
+
+	/* ap->flags bits */
+	AHCI_FLAG_RESET_NEEDS_CLO	= (1 << 24),
 };
 
 struct ahci_cmd_hdr {
@@ -256,6 +260,16 @@ static const struct ata_port_info ahci_p
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
+	/* board_ahci_vt8251 */
+	{
+		.sht		= &ahci_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
+				  AHCI_FLAG_RESET_NEEDS_CLO,
+		.pio_mask	= 0x1f, /* pio0-4 */
+		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
+		.port_ops	= &ahci_ops,
+	},
 };
 
 static const struct pci_device_id ahci_pci_tbl[] = {
@@ -297,6 +311,8 @@ static const struct pci_device_id ahci_p
 	  board_ahci }, /* ATI SB600 non-raid */
 	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_ahci }, /* ATI SB600 raid */
+	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  board_ahci_vt8251 }, /* VT8251 */
 	{ }	/* terminate list */
 };
 
@@ -535,9 +551,27 @@ static int ahci_poll_register(void __iom
 	return -1;
 }
 
-static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+static int ahci_clo(struct ata_port *ap)
 {
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
 	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	u32 tmp;
+
+	if (!(hpriv->cap & HOST_CAP_CLO))
+		return -EOPNOTSUPP;
+
+	tmp = readl(port_mmio + PORT_CMD);
+	tmp |= PORT_CMD_CLO;
+	writel(tmp, port_mmio + PORT_CMD);
+
+	if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0, 1, 500))
+		return -EIO;
+
+	return 0;
+}
+
+static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+{
 	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);
@@ -565,23 +599,13 @@ static int ahci_softreset(struct ata_por
 	/* check BUSY/DRQ, perform Command List Override if necessary */
 	ahci_tf_read(ap, &tf);
 	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
-		u32 tmp;
+		rc = ahci_clo(ap);
 
-		if (!(hpriv->cap & HOST_CAP_CLO)) {
-			rc = -EIO;
-			reason = "port busy but no CLO";
+		if (rc == -EOPNOTSUPP) {
+			reason = "port busy but CLO unavailable";
 			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";
+		} else if (rc == -EIO) {
+			reason = "port busy but CLO failed";
 			goto fail_restart;
 		}
 	}
@@ -695,6 +719,12 @@ static void ahci_postreset(struct ata_po
 
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
+	if ((ap->flags & AHCI_FLAG_RESET_NEEDS_CLO) &&
+	    (ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY)) {
+		/* ATA_BUSY hasn't cleared, so send a CLO */
+		ahci_clo(ap);
+	}
+
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
 				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);

[PATCH 2.6.17-rc1-mm2 1/1] ahci: add support for VIA VT8251

[Bastiaan Jacques]

On Thursday 13 April 2006 00:19, Bastiaan Jacques wrote:
 
Adds AHCI support for the VIA VT8251.

Includes a workaround for a hardware bug which requires a Command List
Override before reset.

Signed-off-by: Bastiaan Jacques <b.jacques@planet.nl>
---

And let's not forget indentation..

--- linux-2.6.17-rc1-mm2.orig/drivers/scsi/ahci.c	2006-04-10 19:10:34.000000000 +0200
+++ linux-2.6.17-rc1-mm2/drivers/scsi/ahci.c	2006-04-13 00:22:28.000000000 +0200
@@ -73,6 +73,7 @@ enum {
 	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
 
 	board_ahci		= 0,
+	board_ahci_vt8251	= 1,
 
 	/* global controller registers */
 	HOST_CAP		= 0x00, /* host capabilities */
@@ -153,6 +154,9 @@ enum {
 
 	/* hpriv->flags bits */
 	AHCI_FLAG_MSI		= (1 << 0),
+
+	/* ap->flags bits */
+	AHCI_FLAG_RESET_NEEDS_CLO	= (1 << 24),
 };
 
 struct ahci_cmd_hdr {
@@ -256,6 +260,16 @@ static const struct ata_port_info ahci_p
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
+	/* board_ahci_vt8251 */
+	{
+		.sht		= &ahci_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
+				  AHCI_FLAG_RESET_NEEDS_CLO,
+		.pio_mask	= 0x1f, /* pio0-4 */
+		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
+		.port_ops	= &ahci_ops,
+	},
 };
 
 static const struct pci_device_id ahci_pci_tbl[] = {
@@ -297,6 +311,8 @@ static const struct pci_device_id ahci_p
 	  board_ahci }, /* ATI SB600 non-raid */
 	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_ahci }, /* ATI SB600 raid */
+	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  board_ahci_vt8251 }, /* VT8251 */
 	{ }	/* terminate list */
 };
 
@@ -535,9 +551,27 @@ static int ahci_poll_register(void __iom
 	return -1;
 }
 
-static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+static int ahci_clo(struct ata_port *ap)
 {
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
 	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	u32 tmp;
+
+	if (!(hpriv->cap & HOST_CAP_CLO))
+		return -EOPNOTSUPP;
+
+	tmp = readl(port_mmio + PORT_CMD);
+	tmp |= PORT_CMD_CLO;
+	writel(tmp, port_mmio + PORT_CMD);
+
+	if (ahci_poll_register(port_mmio + PORT_CMD, PORT_CMD_CLO, 0x0, 1, 500))
+		return -EIO;
+
+	return 0;
+}
+
+static int ahci_softreset(struct ata_port *ap, int verbose, unsigned int *class)
+{
 	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);
@@ -565,23 +599,13 @@ static int ahci_softreset(struct ata_por
 	/* check BUSY/DRQ, perform Command List Override if necessary */
 	ahci_tf_read(ap, &tf);
 	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
-		u32 tmp;
+		rc = ahci_clo(ap);
 
-		if (!(hpriv->cap & HOST_CAP_CLO)) {
-			rc = -EIO;
-			reason = "port busy but no CLO";
+		if (rc == -EOPNOTSUPP) {
+			reason = "port busy but CLO unavailable";
 			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";
+		} else if (rc == -EIO) {
+			reason = "port busy but CLO failed";
 			goto fail_restart;
 		}
 	}
@@ -695,6 +719,12 @@ static void ahci_postreset(struct ata_po
 
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
+	if ((ap->flags & AHCI_FLAG_RESET_NEEDS_CLO) &&
+	    (ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY)) {
+		/* ATA_BUSY hasn't cleared, so send a CLO */
+		ahci_clo(ap);
+	}
+
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
 				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);

Re: [PATCH 2.6.17-rc1-mm2 1/1] ahci: add support for VIA VT8251

[Jeff Garzik]

Bastiaan Jacques wrote:
> Adds AHCI support for the VIA VT8251.
> 
> Includes a workaround for a hardware bug which requires a Command List
> Override before reset.
> 
> Signed-off-by: Bastiaan Jacques <b.jacques@planet.nl>

ACK.

It doesn't currently apply to branch 'upstream'[1] of libata-dev.git. 
Can you send a patch against that?

	Jeff



[1] 26ec634c31a11a003040e10b4d650495158632fd

[PATCH 2.6.17-rc1-mm2 1/1] ahci: add support for VIA VT8251

[Bastiaan Jacques]

Adds AHCI support for the VIA VT8251.

Includes a workaround for a hardware bug which requires a Command List
Override before reset.

Signed-off-by: Bastiaan Jacques <b.jacques@planet.nl>
Acked-by: Jeff Garzik <jeff@garzik.org>
---

This patch is against libata-dev.git upstream[1].

[1] 26ec634c31a11a003040e10b4d650495158632fd


diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 1b8429c..3e3b519 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -73,6 +73,7 @@ enum {
 	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */

 	board_ahci		= 0,
+	board_ahci_vt8251	= 1,

 	/* global controller registers */
 	HOST_CAP		= 0x00, /* host capabilities */
@@ -153,6 +154,9 @@ enum {

 	/* hpriv->flags bits */
 	AHCI_FLAG_MSI		= (1 << 0),
+
+	/* ap->flags bits */
+	AHCI_FLAG_RESET_NEEDS_CLO	= (1 << 24),
 };

 struct ahci_cmd_hdr {
@@ -255,6 +259,16 @@ static const struct ata_port_info ahci_p
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
+	/* board_ahci_vt8251 */
+	{
+		.sht		= &ahci_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
+				  AHCI_FLAG_RESET_NEEDS_CLO,
+		.pio_mask	= 0x1f, /* pio0-4 */
+		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
+		.port_ops	= &ahci_ops,
+	},
 };

 static const struct pci_device_id ahci_pci_tbl[] = {
@@ -296,6 +310,8 @@ static const struct pci_device_id ahci_p
 	  board_ahci }, /* ATI SB600 non-raid */
 	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_ahci }, /* ATI SB600 raid */
+	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  board_ahci_vt8251 }, /* VT8251 */
 	{ }	/* terminate list */
 };

@@ -516,9 +532,29 @@ 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_softreset(struct ata_port *ap, unsigned int *class)
+static int ahci_clo(struct ata_port *ap)
 {
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
 	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	u32 tmp;
+
+	if (!(hpriv->cap & HOST_CAP_CLO))
+		return -EOPNOTSUPP;
+
+	tmp = readl(port_mmio + PORT_CMD);
+	tmp |= PORT_CMD_CLO;
+	writel(tmp, port_mmio + PORT_CMD);
+
+	tmp = ata_wait_register(port_mmio + PORT_CMD,
+				PORT_CMD_CLO, PORT_CMD_CLO, 1, 500);
+	if (tmp & PORT_CMD_CLO)
+		return -EIO;
+
+	return 0;
+}
+
+static int ahci_softreset(struct ata_port *ap, unsigned int *class)
+{
 	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);
@@ -547,21 +583,13 @@ static int ahci_softreset(struct ata_por
 	/* check BUSY/DRQ, perform Command List Override if necessary */
 	ahci_tf_read(ap, &tf);
 	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
-		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);
+		rc = ahci_clo(ap);

-		tmp = ata_wait_register(port_mmio + PORT_CMD,
-					PORT_CMD_CLO, PORT_CMD_CLO, 1, 500);
-		if (tmp & PORT_CMD_CLO) {
-			rc = -EIO;
-			reason = "CLO failed";
+		if (rc == -EOPNOTSUPP) {
+			reason = "port busy but CLO unavailable";
+			goto fail_restart;
+		} else if (rc == -EIO) {
+			reason = "port busy but CLO failed";
 			goto fail_restart;
 		}
 	}
@@ -672,6 +700,12 @@ static void ahci_postreset(struct ata_po

 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
+	if ((ap->flags & AHCI_FLAG_RESET_NEEDS_CLO) &&
+	    (ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY)) {
+		/* ATA_BUSY hasn't cleared, so send a CLO */
+		ahci_clo(ap);
+	}
+
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
 				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);

Re: [PATCH 2.6.17-rc1-mm2 1/1] ahci: add support for VIA VT8251

[Tejun Heo]

Hello, Bastiaan.

Looks good to me.  Please see below.

Bastiaan Jacques wrote:
> Adds AHCI support for the VIA VT8251.
> 
> Includes a workaround for a hardware bug which requires a Command List
> Override before reset.
> 
> Signed-off-by: Bastiaan Jacques <b.jacques@planet.nl>
> Acked-by: Jeff Garzik <jeff@garzik.org>
> ---
[--snip--]
>  static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
>  {
> +	if ((ap->flags & AHCI_FLAG_RESET_NEEDS_CLO) &&
> +	    (ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY)) {

I'm kind of uncomfortable with busy sleeping with @cnt at 1000.  That's 
 >> 10ms of busy waiting.  However, this is a minor issue and even if it 
gets changed it probably belongs to another patch.

Hmmm, ata_busy_sleep() would be a bit weird with the 'be patient' 
message and I always thought the function contained way too much busy 
waiting.  Processors are blazing fast and context switch is cheap these 
days.

Maybe we need to update ata_busy_sleep() so that it ignores tmout_pat if 
it's zero.  What do you think, Jeff?

> +		/* ATA_BUSY hasn't cleared, so send a CLO */
> +		ahci_clo(ap);
> +	}
> +

-- 
tejun

[PATCH libata-dev.git upstream 1/1] ahci: add support for VIA VT8251

[Bastiaan Jacques]

Adds AHCI support for the VIA VT8251.

Includes a workaround for a hardware bug which requires a Command List
Override before softreset.

Signed-off-by: Bastiaan Jacques <b.jacques@planet.nl>
---

I have been informed that VIA have also made a patch available for the
sata_via driver for this chip. Do you think it would be useful to merge
that patch in addition to the AHCI one, or does AHCI support make the
"native" sata_via driver obsolete?

Below is the latest version of the (AHCI) patch, with a small change to
the error handling in ahci_softreset(): test for "if (rc)" just in case
someone decides to change the error return values of ahci_clo().


This patch is against libata-dev.git upstream[1].

[1] 26ec634c31a11a003040e10b4d650495158632fd


diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 1b8429c..2fe4850 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -73,6 +73,7 @@ enum {
 	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
 
 	board_ahci		= 0,
+	board_ahci_vt8251	= 1,
 
 	/* global controller registers */
 	HOST_CAP		= 0x00, /* host capabilities */
@@ -153,6 +154,9 @@ enum {
 
 	/* hpriv->flags bits */
 	AHCI_FLAG_MSI		= (1 << 0),
+
+	/* ap->flags bits */
+	AHCI_FLAG_RESET_NEEDS_CLO	= (1 << 24),
 };
 
 struct ahci_cmd_hdr {
@@ -255,6 +259,16 @@ static const struct ata_port_info ahci_p
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
+	/* board_ahci_vt8251 */
+	{
+		.sht		= &ahci_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
+				  AHCI_FLAG_RESET_NEEDS_CLO,
+		.pio_mask	= 0x1f, /* pio0-4 */
+		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
+		.port_ops	= &ahci_ops,
+	},
 };
 
 static const struct pci_device_id ahci_pci_tbl[] = {
@@ -296,6 +310,8 @@ static const struct pci_device_id ahci_p
 	  board_ahci }, /* ATI SB600 non-raid */
 	{ PCI_VENDOR_ID_ATI, 0x4381, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 	  board_ahci }, /* ATI SB600 raid */
+	{ PCI_VENDOR_ID_VIA, 0x3349, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+	  board_ahci_vt8251 }, /* VT8251 */
 	{ }	/* terminate list */
 };
 
@@ -516,9 +532,29 @@ 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_softreset(struct ata_port *ap, unsigned int *class)
+static int ahci_clo(struct ata_port *ap)
 {
+	void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr;
 	struct ahci_host_priv *hpriv = ap->host_set->private_data;
+	u32 tmp;
+
+	if (!(hpriv->cap & HOST_CAP_CLO))
+		return -EOPNOTSUPP;
+
+	tmp = readl(port_mmio + PORT_CMD);
+	tmp |= PORT_CMD_CLO;
+	writel(tmp, port_mmio + PORT_CMD);
+
+	tmp = ata_wait_register(port_mmio + PORT_CMD,
+				PORT_CMD_CLO, PORT_CMD_CLO, 1, 500);
+	if (tmp & PORT_CMD_CLO)
+		return -EIO;
+
+	return 0;
+}
+
+static int ahci_softreset(struct ata_port *ap, unsigned int *class)
+{
 	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);
@@ -547,21 +583,13 @@ static int ahci_softreset(struct ata_por
 	/* check BUSY/DRQ, perform Command List Override if necessary */
 	ahci_tf_read(ap, &tf);
 	if (tf.command & (ATA_BUSY | ATA_DRQ)) {
-		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);
+		rc = ahci_clo(ap);
 
-		tmp = ata_wait_register(port_mmio + PORT_CMD,
-					PORT_CMD_CLO, PORT_CMD_CLO, 1, 500);
-		if (tmp & PORT_CMD_CLO) {
-			rc = -EIO;
-			reason = "CLO failed";
+		if (rc == -EOPNOTSUPP) {
+			reason = "port busy but CLO unavailable";
+			goto fail_restart;
+		} else if (rc) {
+			reason = "port busy but CLO failed";
 			goto fail_restart;
 		}
 	}
@@ -672,6 +700,12 @@ static void ahci_postreset(struct ata_po
 
 static int ahci_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
+	if ((ap->flags & AHCI_FLAG_RESET_NEEDS_CLO) &&
+	    (ata_busy_wait(ap, ATA_BUSY, 1000) & ATA_BUSY)) {
+		/* ATA_BUSY hasn't cleared, so send a CLO */
+		ahci_clo(ap);
+	}
+
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
 				     ahci_softreset, ahci_hardreset,
 				     ahci_postreset, classes);