Subject: Re: kern/22869: Slave IDE drive not detected
To: Manuel Bouyer <bouyer@antioche.eu.org>
From: Charles M. Hannum <abuse@spamalicious.com>
List: tech-kern
Date: 09/23/2003 07:42:27
> Also, for your CF card, you may be hitting the problem handled by
> WDC_CAPABILITY_NO_EXTRA_RESETS.

I'm not at all convinced that "quirk" is actually necessary.  Among
other things, there was a missing delay() before it started polling
the status register in wdcreset().

I've reworked this a little to not try to do an extra IDENTIFY
command.  The following patch mostly alleviates the problem on my
laptop, and makes CF card insertion reasonably fast.

Unless someone can provide negative test results for this, I'm going
to commit it in the next 2 days.


Index: ata/ata.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ata/ata.c,v
retrieving revision 1.18
diff -u -r1.18 ata.c
--- ata/ata.c	2003/01/27 21:27:52	1.18
+++ ata/ata.c	2003/09/23 07:37:55
@@ -87,12 +87,12 @@
 	if (drvp->drive_flags & DRIVE_ATA) {
 		wdc_c.r_command = WDCC_IDENTIFY;
 		wdc_c.r_st_bmask = WDCS_DRDY;
-		wdc_c.r_st_pmask = WDCS_DRQ;
+		wdc_c.r_st_pmask = 0;
 		wdc_c.timeout = 3000; /* 3s */
 	} else if (drvp->drive_flags & DRIVE_ATAPI) {
 		wdc_c.r_command = ATAPI_IDENTIFY_DEVICE;
 		wdc_c.r_st_bmask = 0;
-		wdc_c.r_st_pmask = WDCS_DRQ;
+		wdc_c.r_st_pmask = 0;
 		wdc_c.timeout = 10000; /* 10s */
 	} else {
 		WDCDEBUG_PRINT(("wdc_ata_get_parms: no disks\n"),
@@ -162,7 +162,7 @@
 	wdc_c.r_st_pmask = 0;
 	wdc_c.r_precomp = WDSF_SET_MODE;
 	wdc_c.r_count = mode;
-	wdc_c.flags = AT_READ | flags;
+	wdc_c.flags = flags;
 	wdc_c.timeout = 1000; /* 1s */
 	if (wdc_exec_command(drvp, &wdc_c) != WDC_COMPLETE)
 		return CMD_AGAIN;
Index: ic/wdc.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/wdc.c,v
retrieving revision 1.125
diff -u -r1.125 wdc.c
--- ic/wdc.c	2003/09/19 21:36:02	1.125
+++ ic/wdc.c	2003/09/23 07:37:56
@@ -205,14 +205,14 @@
 			chp->wdc->select(chp,0);
 		bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
 		    WDSD_IBM);
-		delay(10);
+		delay(10);	/* 400ns delay */
 		st0 = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_status);
 		
 		if (chp->wdc && (chp->wdc->cap & WDC_CAPABILITY_SELECT))
 			chp->wdc->select(chp,1);
 		bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
 		    WDSD_IBM | 0x10);
-		delay(10);
+		delay(10);	/* 400ns delay */
 		st1 = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_status);
 
 		WDCDEBUG_PRINT(("%s:%d: before reset, st0=0x%x, st1=0x%x\n",
@@ -231,14 +231,14 @@
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
 			    WDSD_IBM);
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_cyl_lo,
-			    0x01);
+			    0x02);
 			if (bus_space_read_1(chp->cmd_iot, chp->cmd_ioh,
-			    wd_cyl_lo) != 0x01)
+			    wd_cyl_lo) != 0x02)
 				ret_value &= ~0x01;
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_cyl_lo,
-			    0x02);
+			    0x01);
 			if (bus_space_read_1(chp->cmd_iot, chp->cmd_ioh,
-			    wd_cyl_lo) != 0x02)
+			    wd_cyl_lo) != 0x01)
 				ret_value &= ~0x01;
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sector,
 			    0x01);
@@ -250,6 +250,9 @@
 			if (bus_space_read_1(chp->cmd_iot, chp->cmd_ioh,
 			    wd_sector) != 0x02)
 				ret_value &= ~0x01;
+			if (bus_space_read_1(chp->cmd_iot, chp->cmd_ioh,
+			    wd_cyl_lo) != 0x01)
+				ret_value &= ~0x01;
 		}
 
 		/* Register writability test, drive 1. */
@@ -259,14 +262,14 @@
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
 			    WDSD_IBM | 0x10);
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_cyl_lo,
-			    0x01);
+			    0x02);
 			if (bus_space_read_1(chp->cmd_iot, chp->cmd_ioh,
-			    wd_cyl_lo) != 0x01)
+			    wd_cyl_lo) != 0x02)
 				ret_value &= ~0x02;
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_cyl_lo,
-			    0x02);
+			    0x01);
 			if (bus_space_read_1(chp->cmd_iot, chp->cmd_ioh,
-			    wd_cyl_lo) != 0x02)
+			    wd_cyl_lo) != 0x01)
 				ret_value &= ~0x02;
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sector,
 			    0x01);
@@ -278,28 +281,9 @@
 			if (bus_space_read_1(chp->cmd_iot, chp->cmd_ioh,
 			    wd_sector) != 0x02)
 				ret_value &= ~0x02;
-		}
-
-		/* Register ghost test. */
-		if (ret_value == 0x03) {
-			if (chp->wdc && (chp->wdc->cap & WDC_CAPABILITY_SELECT))
-				chp->wdc->select(chp,1);
-			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
-			    WDSD_IBM | 0x10);
-			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sector, 0x01);
-			if (chp->wdc && (chp->wdc->cap & WDC_CAPABILITY_SELECT))
-				chp->wdc->select(chp,0);
-			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
-			    WDSD_IBM);
-			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sector, 0x02);
-			if (chp->wdc && (chp->wdc->cap & WDC_CAPABILITY_SELECT))
-				chp->wdc->select(chp,1);
-			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
-			    WDSD_IBM | 0x10);
-			if (bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_sector) == 0x02) {
-				printf("ghost detected\n");
-				ret_value = 0x01;
-			}
+			if (bus_space_read_1(chp->cmd_iot, chp->cmd_ioh,
+			    wd_cyl_lo) != 0x01)
+				ret_value &= ~0x02;
 		}
 
 		if (ret_value == 0)
@@ -312,16 +296,13 @@
 	/* assert SRST, wait for reset to complete */
 	bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
 	    WDSD_IBM);
-	delay(10);
+	delay(10);	/* 400ns delay */
 	bus_space_write_1(chp->ctl_iot, chp->ctl_ioh, wd_aux_ctlr,
-	    WDCTL_RST | WDCTL_IDS); 
-	DELAY(1000);
-	bus_space_write_1(chp->ctl_iot, chp->ctl_ioh, wd_aux_ctlr,
-	    WDCTL_IDS);
-	delay(1000);
+	    WDCTL_RST | WDCTL_IDS | WDCTL_4BIT); 
+	delay(2000);
 	(void) bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_error);
 	bus_space_write_1(chp->ctl_iot, chp->ctl_ioh, wd_aux_ctlr, WDCTL_4BIT);
-	delay(10);
+	delay(10);	/* 400ns delay */
 
 	ret_value = __wdcwait_reset(chp, ret_value);
 	WDCDEBUG_PRINT(("%s:%d: after reset, ret_value=0x%d\n",
@@ -345,7 +326,7 @@
 			chp->wdc->select(chp,drive);
 		bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
 		    WDSD_IBM | (drive << 4));
-		delay(10);
+		delay(10);	/* 400ns delay */
 		/* Save registers contents */
 		sc = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_seccnt);
 		sn = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_sector);
@@ -464,7 +445,7 @@
 				chp->wdc->select(chp,i);
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
 			    WDSD_IBM | (i << 4));
-			delay(10);
+			delay(10);	/* 400ns delay */
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh,
 			    wd_error, 0x58);
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh,
@@ -479,11 +460,6 @@
 				    chp->channel, i), DEBUG_PROBE);
 				    chp->ch_drive[i].drive_flags &= ~DRIVE_OLD;
 			}
-			if (chp->wdc->cap & WDC_CAPABILITY_SELECT)
-				chp->wdc->select(chp,i);
-			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
-			    WDSD_IBM | (i << 4));
-			delay(100);
 			if (wait_for_ready(chp, 10000) != 0) {
 				WDCDEBUG_PRINT(("%s:%d:%d: not ready\n",
 				    chp->wdc->sc_dev.dv_xname,
@@ -493,6 +469,7 @@
 			}
 			bus_space_write_1(chp->cmd_iot, chp->cmd_ioh,
 			    wd_command, WDCC_RECAL);
+			delay(10);	/* 400ns delay */
 			if (wait_for_ready(chp, 10000) != 0) {
 				WDCDEBUG_PRINT(("%s:%d:%d: WDCC_RECAL failed\n",
 				    chp->wdc->sc_dev.dv_xname,
@@ -855,15 +832,13 @@
 		chp->wdc->select(chp,0);
 	bus_space_write_1(chp->cmd_iot, chp->cmd_ioh, wd_sdh,
 	    WDSD_IBM); /* master */
-	bus_space_write_1(chp->ctl_iot, chp->ctl_ioh, wd_aux_ctlr,
-	    WDCTL_RST | WDCTL_IDS);
-	delay(1000);
+	delay(10);	/* 400ns delay */
 	bus_space_write_1(chp->ctl_iot, chp->ctl_ioh, wd_aux_ctlr,
-	    WDCTL_IDS);
-	delay(1000);
+	    WDCTL_RST | WDCTL_IDS | WDCTL_4BIT);
+	delay(2000);
 	(void) bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_error);
-	bus_space_write_1(chp->ctl_iot, chp->ctl_ioh, wd_aux_ctlr,
-	    WDCTL_4BIT);
+	bus_space_write_1(chp->ctl_iot, chp->ctl_ioh, wd_aux_ctlr, WDCTL_4BIT);
+	delay(10);	/* 400ns delay */
 
 	drv_mask1 = (chp->ch_drive[0].drive_flags & DRIVE) ? 0x01:0x00;
 	drv_mask1 |= (chp->ch_drive[1].drive_flags & DRIVE) ? 0x02:0x00;
@@ -886,7 +861,7 @@
 	int drv_mask;
 {
 	int timeout;
-	u_int8_t st0, st1;
+	u_int8_t st0, er0, st1, er1;
 #ifdef WDCDEBUG
 	u_int8_t sc0, sn0, cl0, ch0;
 	u_int8_t sc1, sn1, cl1, ch1;
@@ -899,6 +874,7 @@
 		    WDSD_IBM); /* master */
 		delay(10);
 		st0 = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_status);
+		er0 = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_error);
 #ifdef WDCDEBUG
 		sc0 = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_seccnt);
 		sn0 = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_sector);
@@ -911,6 +887,7 @@
 		    WDSD_IBM | 0x10); /* slave */
 		delay(10);
 		st1 = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_status);
+		er1 = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_error);
 #ifdef WDCDEBUG
 		sc1 = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_seccnt);
 		sn1 = bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_sector);
@@ -944,6 +921,10 @@
 	if (st1 & WDCS_BSY)
 		drv_mask &= ~0x02;
 end:
+	if (er0 != 0x01 && er0 != 0x81)
+		drv_mask &= ~0x01;
+	if (er1 != 0x01)
+		drv_mask &= ~0x02;
 	WDCDEBUG_PRINT(("%s:%d:0: after reset, sc=0x%x sn=0x%x "
 	    "cl=0x%x ch=0x%x\n",
 	     chp->wdc ? chp->wdc->sc_dev.dv_xname : "wdcprobe",
@@ -953,9 +934,9 @@
 	     chp->wdc ? chp->wdc->sc_dev.dv_xname : "wdcprobe",
 	     chp->channel, sc1, sn1, cl1, ch1), DEBUG_PROBE);
 
-	WDCDEBUG_PRINT(("%s:%d: wdcwait_reset() end, st0=0x%x, st1=0x%x\n",
+	WDCDEBUG_PRINT(("%s:%d: wdcwait_reset() end, st0=0x%x er0=0x%x, st1=0x%x er1=0x%x\n",
 	    chp->wdc ? chp->wdc->sc_dev.dv_xname : "wdcprobe", chp->channel,
-	    st0, st1), DEBUG_PROBE);
+	    st0, er0, st1, er1), DEBUG_PROBE);
 
 	return drv_mask;
 }
@@ -981,7 +962,7 @@
 	for (;;) {
 		chp->ch_status = status =
 		    bus_space_read_1(chp->cmd_iot, chp->cmd_ioh, wd_status);
-		if ((status & WDCS_BSY) == 0 && (status & mask) == bits)
+		if ((status & (WDCS_BSY | mask)) == bits)
 			break;
 		if (++time > timeout) {
 			WDCDEBUG_PRINT(("wdcwait: timeout (time=%d), "
@@ -1473,28 +1454,27 @@
 		 * in its initial state
 		 */
 		if (wdcwait(chp, wdc_c->r_st_bmask | WDCS_DRQ,
-		    wdc_c->r_st_bmask, (irq == 0)  ? wdc_c->timeout : 0) != 0) {
+		    wdc_c->r_st_bmask, (irq == 0)  ? wdc_c->timeout : 0)) {
 			if (irq && (xfer->c_flags & C_TIMEOU) == 0) 
 				return 0; /* IRQ was not for us */
 			wdc_c->flags |= AT_TIMEOU;
-			__wdccommand_done(chp, xfer);
-			return 1;
 		}
-		wdc_c->flags |= AT_DONE;
-		__wdccommand_done(chp, xfer);
-		return 1;
+		goto out;
 	}
 	if (wdcwait(chp, wdc_c->r_st_pmask, wdc_c->r_st_pmask,
 	     (irq == 0)  ? wdc_c->timeout : 0)) {
 		if (irq && (xfer->c_flags & C_TIMEOU) == 0) 
 			return 0; /* IRQ was not for us */
 		wdc_c->flags |= AT_TIMEOU;
-		__wdccommand_done(chp, xfer);
-		return 1;
+		goto out;
 	}
 	if (chp->wdc->cap & WDC_CAPABILITY_IRQACK)
 		chp->wdc->irqack(chp);
 	if (wdc_c->flags & AT_READ) {
+		if ((chp->ch_status & WDCS_DRQ) == 0) {
+			wdc_c->flags |= AT_TIMEOU;
+			goto out;
+		}
 		if (chp->ch_drive[xfer->drive].drive_flags & DRIVE_CAP32) {
 			bus_space_read_multi_4(chp->data32iot, chp->data32ioh,
 			    0, (u_int32_t*)data, bcount >> 2);
@@ -1506,10 +1486,11 @@
 			    wd_data, (u_int16_t *)data, bcount >> 1);
 		/* at this point the drive should be in its initial state */
 		wdc_c->flags |= AT_XFDONE;
-		if (wdcwait(chp, wdc_c->r_st_bmask | WDCS_DRQ,
-		    wdc_c->r_st_bmask, 100) != 0)
-			wdc_c->flags |= AT_TIMEOU;
 	} else if (wdc_c->flags & AT_WRITE) {
+		if ((chp->ch_status & WDCS_DRQ) == 0) {
+			wdc_c->flags |= AT_TIMEOU;
+			goto out;
+		}
 		if (chp->ch_drive[xfer->drive].drive_flags & DRIVE_CAP32) {
 			bus_space_write_multi_4(chp->data32iot, chp->data32ioh,
 			    0, (u_int32_t*)data, bcount >> 2);
@@ -1529,6 +1510,7 @@
 			goto again;
 		}
 	}
+out:
 	__wdccommand_done(chp, xfer);
 	return 1;
 }