>Number:         32169
>Category:       kern
>Synopsis:       PIO modes 1 and 2 not used
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Sat Nov 26 22:41:00 +0000 2005
>Originator:     darkstar@city-net.com
>Release:        NetBSD 2.1.0_STABLE
>Organization:
>Description:

  wdc_probe_caps does not detect PIO mode 1 or 2 in the oldpiotiming field,
leading to lower performance on some flash drives.

>How-To-Repeat:

  This should primarily affect some flash controllers (both CF and larger IDE
flash drives).  The affected card I have has a Hitachi XXM2.3.0 Rev 3.00
controller.

>Fix:

  Inspired by changes in OpenBSD.  This patch will also do PIO 1/2 detection
and use flags when bous values are returned in the extended PIO modes.  At
least the flags should be checked in that case, I think, and the mode 1/2
field may be more reliable than the extended fields.  The CF card I have does
not have this problem, so that part of the patch could be modified or removed.

  I can't test if this patch would fully fix the performance problems with
this card since viaide apparently does not support faster speeds in PIO
modes 1 or 2.  The manufacturer claims the card supports PIO mode 4 (and a
few hundered megabytes of testing showed no corruption), however I suspect
"supports" means "will automatically slow down with IORDY".  The card
reports PIO mode 1 capability and does not support SET FEATURES/SET MODE
at all (meaning that forcing any mode currently does not work due to
piomode error).  This patch would allow forcing PIO mode 2 for this card,
but not 3 or 4.  Since the maximum transfer speed I get at PIO 3 on viaide
is 3.2MB/s (raw read), PIO 1 or 2 seem likely to get full speed and it
isn't worth another flag to work around combinations of broken hardware.

Index: ata/ata_wdc.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ata/ata_wdc.c,v
retrieving revision 1.53.2.3.2.4
diff -u -p -r1.53.2.3.2.4 ata_wdc.c
--- ata/ata_wdc.c	18 Jul 2005 03:57:36 -0000	1.53.2.3.2.4
+++ ata/ata_wdc.c	26 Nov 2005 21:44:26 -0000
@@ -258,8 +258,15 @@ wdc_ata_bio_start(struct wdc_channel *ch
 		errstring = "piomode";
 		if (wdcwait(chp, WDCS_DRDY, WDCS_DRDY, ATA_DELAY, wait_flags))
 			goto ctrltimeout;
+		/* Ignore an ABRT error for PIO <= 2 since SET_FEATURES
+		 * SET_MODE 0x08 is for flow control transfer (IORDY), which
+		 * may not be used for PIO <= 2. */
+		if (drvp->PIO_mode <= 2 && ((chp->ch_status & (WDCS_ERR |
+		    WDCS_DWF)) == WDCS_ERR) && chp->ch_error == WDCE_ABRT)
+			goto dma;
 		if (chp->ch_status & (WDCS_ERR | WDCS_DWF))
 			goto ctrlerror;
+dma:
 		if (drvp->drive_flags & DRIVE_UDMA) {
 			wdccommand(chp, drvp->drive, SET_FEATURES, 0, 0, 0,
 			    0x40 | drvp->UDMA_mode, WDSF_SET_MODE);
Index: ic/wdc.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/wdc.c,v
retrieving revision 1.172.2.7.2.10
diff -u -p -r1.172.2.7.2.10 wdc.c
--- ic/wdc.c	23 Aug 2005 13:35:55 -0000	1.172.2.7.2.10
+++ ic/wdc.c	26 Nov 2005 21:44:30 -0000
@@ -1419,13 +1419,13 @@ wdc_probe_caps(struct ata_drive_datas *d
 	if (drvp->drive_flags & DRIVE_ATAPI)
 		drvp->PIO_mode = 3;

+	printed = 0;
 	/*
 	 * It's not in the specs, but it seems that some drive
 	 * returns 0xffff in atap_extensions when this field is invalid
 	 */
 	if (params.atap_extensions != 0xffff &&
 	    (params.atap_extensions & WDC_EXT_MODES)) {
-		printed = 0;
 		/*
 		 * XXX some drives report something wrong here (they claim to
 		 * support PIO mode 8 !). As mode is coded on 3 bits in
@@ -1436,7 +1436,7 @@ wdc_probe_caps(struct ata_drive_datas *d
 			if ((params.atap_piomode_supp & (1 << i)) == 0)
 				continue;
 			if (i > 4)
-				return;
+				goto flags;
 			/*
 			 * See if mode is accepted.
 			 * If the controller can't set its PIO mode,
@@ -1473,7 +1473,7 @@ wdc_probe_caps(struct ata_drive_datas *d
 			 * We didn't find a valid PIO mode.
 			 * Assume the values returned for DMA are buggy too
 			 */
-			return;
+			goto flags;
 		}
 		drvp->drive_flags |= DRIVE_MODE;
 		printed = 0;
@@ -1539,6 +1539,15 @@ wdc_probe_caps(struct ata_drive_datas *d
 		aprint_normal("\n");
 	}

+flags:
+	if (!printed && params.atap_oldpiotiming && params.atap_oldpiotiming
+	    <= 2) {
+		drvp->PIO_cap = drvp->PIO_mode = params.atap_oldpiotiming;
+		drvp->drive_flags |= DRIVE_MODE;
+		aprint_normal("%s: drive supports PIO mode %d\n",
+		    drv_dev->dv_xname, drvp->PIO_mode);
+	}
+
 	/* Try to guess ATA version here, if it didn't get reported */
 	if (drvp->ata_vers == 0) {
 		if (drvp->drive_flags & DRIVE_UDMA)