Add DMA support for the VIA 82C586 & 82C686 chips, also rearrange

to fall back to slower speeds if the faster ones fails to probe.

Log and retry request on UDMA CRC errors.

Fix a couple of warnings.

9 files changed, 228 insertions, 140 deletions

diff --git a/sys/dev/ata/ata-all.c b/sys/dev/ata/ata-all.c
index 99f677c..cf3beb7 100644
--- a/sys/dev/ata/ata-all.c
+++ b/sys/dev/ata/ata-all.c
@@ -201,9 +201,10 @@ ata_pcimatch(device_t dev)
 	return "Promise Ultra/66 IDE controller";
     case 0x00041103:
 	return "HighPoint HPT366 IDE controller";
+    case 0x05711106: /* 82c586 & 82c686 */
+	return "VIA Apollo IDE controller";
 
    /* unsupported but known chipsets, generic DMA only */
-    case 0x05711106: /* 82c586 */
     case 0x05961106: /* 82c596 */
 	return "VIA Apollo IDE controller (generic mode)";
     case 0x06401095:
diff --git a/sys/dev/ata/ata-all.h b/sys/dev/ata/ata-all.h
index 00fdc73..d4d8917 100644
--- a/sys/dev/ata/ata-all.h
+++ b/sys/dev/ata/ata-all.h
@@ -31,7 +31,13 @@
 /* ATA register defines */
 #define ATA_DATA			0x00	/* data register */
 #define ATA_ERROR			0x01	/* (R) error register */
+#define		ATA_E_NM		0x02	/* no media */
 #define		ATA_E_ABORT		0x04	/* command aborted */
+#define		ATA_E_MCR		0x08	/* media change request */
+#define		ATA_E_IDNF		0x10	/* ID not found */
+#define		ATA_E_MC		0x20	/* media changed */
+#define		ATA_E_UNC		0x40	/* uncorrectable data */
+#define		ATA_E_ICRC		0x80	/* UDMA crc error */
 
 #define ATA_FEATURE			0x01	/* (W) feature register */
 #define		ATA_F_DMA		0x01	/* enable DMA */
diff --git a/sys/dev/ata/ata-disk.c b/sys/dev/ata/ata-disk.c
index a11f8bb..b8a083a 100644
--- a/sys/dev/ata/ata-disk.c
+++ b/sys/dev/ata/ata-disk.c
@@ -116,8 +116,8 @@ static __inline int
 udmamode(struct ata_params *ap)
 {
     if (ap->atavalid & 4) {
-	if (ap->udmamodes & 0x10 && ap->cblid) return 4;
-	if (ap->udmamodes & 0x08 && ap->cblid) return 3;
+	if (ap->udmamodes & 0x10) return (ap->cblid ? 4 : 2);
+	if (ap->udmamodes & 0x08) return (ap->cblid ? 3 : 2);
 	if (ap->udmamodes & 0x04) return 2;
 	if (ap->udmamodes & 0x02) return 1;
 	if (ap->udmamodes & 0x01) return 0;
@@ -195,7 +195,7 @@ ad_attach(void *notused)
 		    adp->flags |= AD_F_DMA_ENABLED;
 
 		/* use tagged queueing if supported (not yet) */
-		if ((adp->num_tags = adp->ata_parm->queuelen & 0x1f))
+		if ((adp->num_tags = (adp->ata_parm->queuelen & 0x1f) + 1))
 		    adp->flags |= AD_F_TAG_ENABLED;
 
 		/* store our softc */
@@ -206,9 +206,10 @@ ad_attach(void *notused)
 		      sizeof(revision_buf));
 
 		if (bootverbose)
-		    printf("ad%d: piomode=%d dmamode=%d udmamode=%d\n",
+		    printf("ad%d: piomode=%d dmamode=%d udmamode=%d cblid=%d\n",
 		           adp->lun, apiomode(adp->ata_parm),
-		           wdmamode(adp->ata_parm), udmamode(adp->ata_parm));
+		           wdmamode(adp->ata_parm), udmamode(adp->ata_parm),
+			   adp->ata_parm->cblid);
 
 		printf("ad%d: <%s/%s> ATA-%c disk at ata%d as %s\n", 
 		       adp->lun, model_buf, revision_buf,
@@ -532,14 +533,29 @@ ad_interrupt(struct ad_request *request)
     if (adp->controller->status & (ATA_S_ERROR | ATA_S_CORR) ||
 	(request->flags & AR_F_DMA_USED && dma_stat != ATA_BMSTAT_INTERRUPT)) {
 oops:
-	printf("ad%d: status=%02x error=%02x\n", 
-	       adp->lun, adp->controller->status, adp->controller->error);
 	if (adp->controller->status & ATA_S_ERROR) {
-	    printf("ad_interrupt: hard error\n"); 
-	    request->flags |= AR_F_ERROR;
+	    printf("ad%d: %s %s error blk# %d", adp->lun,
+		   (adp->controller->error & ATA_E_ICRC) ? "UDMA CRC" : "HARD",
+		   (request->flags & AR_F_READ) ? "READ" : "WRITE",
+		   request->blockaddr + (request->donecount / DEV_BSIZE)); 
+	    /* if this is a UDMA CRC error, reinject request */
+	    if (adp->controller->error & ATA_E_ICRC &&
+		request->retries++ < AD_MAX_RETRIES) {
+    		/* disarm timeout for this transfer */
+    		untimeout((timeout_t *)ad_timeout, request, 
+			  request->timeout_handle);
+		TAILQ_INSERT_HEAD(&adp->controller->ata_queue, request, chain);
+		printf(" retrying\n");
+		return ATA_OP_FINISHED;
+	    }
+	    else {
+		printf(" status=%02x error=%02x\n", 
+			adp->controller->status, adp->controller->error);
+		request->flags |= AR_F_ERROR;
+	    }
 	}
-	if (adp->controller->status & ATA_S_CORR)
-	    printf("ad_interrupt: soft error ECC corrected\n"); 
+	else if (adp->controller->status & ATA_S_CORR)
+	    printf("ad%d: soft error ECC corrected\n", adp->lun); 
     }
 
     /* if this was a PIO read operation, get the data */
@@ -634,11 +650,9 @@ ad_timeout(struct ad_request *request)
     if (request->flags & AR_F_DMA_USED)
 	ata_dmadone(adp->controller);
 
-    if (request->retries < AD_MAX_RETRIES) {
-	/* reinject this request */
-	request->retries++;
+    /* if retries still permit, reinject this request */
+    if (request->retries++ < AD_MAX_RETRIES)
 	TAILQ_INSERT_HEAD(&adp->controller->ata_queue, request, chain);
-    }
     else {
 	/* retries all used up, return error */
 	request->bp->b_error = EIO;
diff --git a/sys/dev/ata/ata-disk.h b/sys/dev/ata/ata-disk.h
index 4e8e79d..7415d52 100644
--- a/sys/dev/ata/ata-disk.h
+++ b/sys/dev/ata/ata-disk.h
@@ -114,10 +114,10 @@ struct ata_params {
     int16_t	enherasetime;
     int16_t	apmlevel;
     int16_t	masterpasswdrev;
-    u_int16_t	masterhwres:8;
-    u_int16_t	slavehwres:5;
-    u_int16_t	cblid:1;
-    u_int16_t	reserved93_1415:2;
+    u_int16_t	masterhwres	:8;
+    u_int16_t	slavehwres	:5;
+    u_int16_t	cblid		:1;
+    u_int16_t	reserved93_1415	:2;
     int16_t	reserved94[32];
     int16_t	rmvstat;
     int16_t	securstat;
diff --git a/sys/dev/ata/ata-dma.c b/sys/dev/ata/ata-dma.c
index f85949c..fe74a7a 100644
--- a/sys/dev/ata/ata-dma.c
+++ b/sys/dev/ata/ata-dma.c
@@ -105,16 +105,16 @@ ata_dmainit(struct ata_softc *scp, int32_t device,
 		printf("ata%d: %s: %s setting up UDMA2 mode on PIIX4 chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    devno = (scp->unit << 1) + ((device == ATA_MASTER) ? 0 : 1);
-	    mask48 = (1 << devno) + (3 << (16 + (devno << 2)));
-	    new48 = (1 << devno) + (2 << (16 + (devno << 2)));
-	    pci_write_config(scp->dev, 0x48, 
-			     (pci_read_config(scp->dev, 0x48, 4) &
-			      ~mask48) | new48, 4);
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA2;
-	    return 0;
+	    if (!error) {
+		devno = (scp->unit << 1) + ((device == ATA_MASTER) ? 0 : 1);
+		mask48 = (1 << devno) + (3 << (16 + (devno << 2)));
+		new48 = (1 << devno) + (2 << (16 + (devno << 2)));
+		pci_write_config(scp->dev, 0x48, 
+				 (pci_read_config(scp->dev, 0x48, 4) &
+				 ~mask48) | new48, 4);
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA2;
+		return 0;
+	    }
 	}
 	/* FALLTHROUGH */
 
@@ -144,34 +144,36 @@ ata_dmainit(struct ata_softc *scp, int32_t device,
 		printf("ata%d: %s: %s setting up WDMA2 mode on PIIX4 chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    if (device == ATA_MASTER) {
-		mask40 = 0x0000330f;
-		new40 = 0x00002307;
-		mask44 = 0;
-		new44 = 0;
-	    } else {
-		mask40 = 0x000000f0;
-		new40 = 0x00000070;
-		mask44 = 0x0000000f;
-		new44 = 0x0000000b;
-	    }
-	    if (scp->unit) {
-		mask40 <<= 16;
-		new40 <<= 16;
-		mask44 <<= 4;
-		new44 <<= 4;
+	    if (!error) {
+		if (device == ATA_MASTER) {
+		    mask40 = 0x0000330f;
+		    new40 = 0x00002307;
+		    mask44 = 0;
+		    new44 = 0;
+		}
+		else {
+		    mask40 = 0x000000f0;
+		    new40 = 0x00000070;
+		    mask44 = 0x0000000f;
+		    new44 = 0x0000000b;
+		}
+		if (scp->unit) {
+		    mask40 <<= 16;
+		    new40 <<= 16;
+		    mask44 <<= 4;
+		    new44 <<= 4;
+		}
+		pci_write_config(scp->dev, 0x40,
+				 (pci_read_config(scp->dev, 0x40, 4) & ~mask40)|
+ 				 new40, 4);
+		pci_write_config(scp->dev, 0x44,
+				 (pci_read_config(scp->dev, 0x44, 4) & ~mask44)|
+ 				 new44, 4);
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
+		return 0;
 	    }
-	    pci_write_config(scp->dev, 0x40,
-			     (pci_read_config(scp->dev, 0x40, 4) & ~mask40) |
-			     new40, 4);
-	    pci_write_config(scp->dev, 0x44,
-			     (pci_read_config(scp->dev, 0x44, 4) & ~mask44) |
-			     new44, 4);
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
-	    return 0;
 	}
+	/* we could set PIO mode timings, but we assume the BIOS did that */
 	break;
 
     case 0x12308086:	/* Intel PIIX */
@@ -185,7 +187,7 @@ ata_dmainit(struct ata_softc *scp, int32_t device,
 		   "DMA disabled\n", scp->lun);
 	    break;
 	}
-	if (udmamode >=2) {
+	if (udmamode >= 2) {
 	    int32_t word54 = pci_read_config(scp->dev, 0x54, 4);
 	
 	    error = ata_command(scp, device, ATA_C_SETFEATURES, 0, 0, 0,
@@ -194,33 +196,104 @@ ata_dmainit(struct ata_softc *scp, int32_t device,
 		printf("ata%d: %s: %s setting up UDMA2 mode on Aladdin chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    word54 |= 0x5555;
-	    word54 |= (0x0a << (16 + (scp->unit << 3) + (device << 2)));
-	    pci_write_config(scp->dev, 0x54, word54, 4);
-	    pci_write_config(scp->dev, 0x53, 
-			     pci_read_config(scp->dev, 0x53, 1) | 0x03, 1);
-	    scp->flags |= ATA_ATAPI_DMA_RO;
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA2;
-	    return 0;
-		
+	    if (!error) {
+		word54 |= 0x5555;
+		word54 |= (0x0a << (16 + (scp->unit << 3) + (device << 2)));
+		pci_write_config(scp->dev, 0x54, word54, 4);
+		pci_write_config(scp->dev, 0x53, 
+				 pci_read_config(scp->dev, 0x53, 1) | 0x03, 1);
+		scp->flags |= ATA_ATAPI_DMA_RO;
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA2;
+		return 0;
+	    }
 	}
-	else if (wdmamode >= 2 && apiomode >= 4) {
+	if (wdmamode >= 2 && apiomode >= 4) {
 	    error = ata_command(scp, device, ATA_C_SETFEATURES, 0, 0, 0,
 				ATA_WDMA2, ATA_C_F_SETXFER, ATA_WAIT_READY);
 	    if (bootverbose)
 		printf("ata%d: %s: %s setting up WDMA2 mode on Aladdin chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    pci_write_config(scp->dev, 0x53, 
-			     pci_read_config(scp->dev, 0x53, 1) | 0x03, 1);
-	    scp->flags |= ATA_ATAPI_DMA_RO;
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
-	    return 0;
+	    if (!error) {
+		pci_write_config(scp->dev, 0x53, 
+				 pci_read_config(scp->dev, 0x53, 1) | 0x03, 1);
+		scp->flags |= ATA_ATAPI_DMA_RO;
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
+		return 0;
+	    }
+	}
+	/* we could set PIO mode timings, but we assume the BIOS did that */
+	break;
+
+    case 0x05711106:	/* VIA Apollo 82c586 / 82c686 */
+	devno = (scp->unit << 1) + ((device == ATA_MASTER) ? 0 : 1);
+	if (udmamode >= 2 && pci_read_config(scp->dev, 0x0d, 1) >= 0x20) {
+	    int8_t byte = pci_read_config(scp->dev, 0x53 - devno, 1);
+
+	    /* enable UDMA transfer modes setting by SETFEATURES cmd */
+	    pci_write_config(scp->dev, 0x53 - devno, (byte & 0x1c) | 0x40, 1);
+
+	    if (udmamode >= 4) {
+		error = ata_command(scp, device, ATA_C_SETFEATURES, 0, 0, 0,
+				    ATA_UDMA4, ATA_C_F_SETXFER, ATA_WAIT_READY);
+		if (bootverbose)
+		    printf("ata%d: %s: %s setting up UDMA4 mode on VIA chip\n",
+			   scp->lun, (device == ATA_MASTER) ? "master":"slave",
+			   (error) ? "failed" : "success");
+		if (!error) {
+		    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA4;
+		    return 0;
+		}
+	    }
+	    error = ata_command(scp, device, ATA_C_SETFEATURES, 0, 0, 0,
+				ATA_UDMA2, ATA_C_F_SETXFER, ATA_WAIT_READY);
+	    if (bootverbose)
+		printf("ata%d: %s: %s setting up UDMA2 mode on VIA chip\n",
+		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
+		       (error) ? "failed" : "success");
+	    if (!error) {
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA2;
+		return 0;
+	    }
+	    pci_write_config(scp->dev, 0x53 - devno, byte, 1);
+	}
+	if (wdmamode >= 2 && apiomode >= 4) {
+	    int8_t byte;
+
+	    /* set prefetch, postwrite */
+	    byte = pci_read_config(scp->dev, 0x41, 1);
+	    pci_write_config(scp->dev, 0x41, byte | 0xf0, 1);
+	    
+	    /* set fifo configuration half'n'half */
+	    byte = pci_read_config(scp->dev, 0x43, 1);
+	    pci_write_config(scp->dev, 0x43, (byte & 0x90) | 0x2a, 1);
+	   
+	    /* set status register read retry */
+	    byte = pci_read_config(scp->dev, 0x44, 1);
+	    pci_write_config(scp->dev, 0x44, byte | 0x08, 1);
+	    
+	    /* set DMA read & end-of-sector fifo flush */
+	    byte = pci_read_config(scp->dev, 0x46, 1);
+	    pci_write_config(scp->dev, 0x46, (byte & 0x0c) | 0xf0, 1);
+
+	    /* set WDMA2 mode timing */
+	    pci_write_config(scp->dev, 0x4b - devno, 0x31 , 1);
+
+	    /* set sector size */
+	    pci_write_config(scp->dev, scp->unit ? 0x68 : 0x60, DEV_BSIZE, 2);
+
+	    error = ata_command(scp, device, ATA_C_SETFEATURES, 0, 0, 0,
+				ATA_WDMA2, ATA_C_F_SETXFER, ATA_WAIT_READY);
+	    if (bootverbose)
+		printf("ata%d: %s: %s setting up WDMA2 mode on VIA chip\n",
+		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
+		       (error) ? "failed" : "success");
+	    if (!error) {
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
+		return 0;
+	    }
 	}
+	/* we could set PIO mode timings, but we assume the BIOS did that */
 	break;
 
     case 0x4d33105a:	/* Promise Ultra33 / FastTrak33 controllers */
@@ -239,49 +312,47 @@ ata_dmainit(struct ata_softc *scp, int32_t device,
 		printf("ata%d: %s: %s setting up UDMA4 mode on Promise chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    outb(scp->bmaddr + 0x11, inl(scp->bmaddr + 0x11) | scp->unit ? 8:2);
-	    pci_write_config(scp->dev, 0x60 + (devno << 2), 0x004127f3, 4);
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA4;
-	    return 0;
+	    if (!error) {
+		outb(scp->bmaddr+0x11, inl(scp->bmaddr+0x11) | scp->unit ? 8:2);
+		pci_write_config(scp->dev, 0x60 + (devno << 2), 0x004127f3, 4);
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA4;
+		return 0;
+	    }
 	}
-	if (udmamode >=2) {
+	if (udmamode >= 2) {
 	    error = ata_command(scp, device, ATA_C_SETFEATURES, 0, 0, 0,
 				ATA_UDMA2, ATA_C_F_SETXFER, ATA_WAIT_READY);
 	    if (bootverbose)
 		printf("ata%d: %s: %s setting up UDMA2 mode on Promise chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    pci_write_config(scp->dev, 0x60 + (devno << 2), 0x004127f3, 4);
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA2;
-	    return 0;
+	    if (!error) {
+		pci_write_config(scp->dev, 0x60 + (devno << 2), 0x004127f3, 4);
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA2;
+		return 0;
+	    }
 	}
-	else if (wdmamode >= 2 && apiomode >= 4) {
+	if (wdmamode >= 2 && apiomode >= 4) {
 	    error = ata_command(scp, device, ATA_C_SETFEATURES, 0, 0, 0,
 				ATA_WDMA2, ATA_C_F_SETXFER, ATA_WAIT_READY);
 	    if (bootverbose)
 		printf("ata%d: %s: %s setting up WDMA2 mode on Promise chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    pci_write_config(scp->dev, 0x60 + (devno << 2), 0x004367f3, 4);
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
-	    return 0;
-	}
-	else {
-	    if (bootverbose)
-		printf("ata%d: %s: setting PIO mode on Promise chip\n",
-		       scp->lun, (device == ATA_MASTER) ? "master" : "slave");
-	    pci_write_config(scp->dev, 0x60 + (devno << 2), 0x004fe924, 4);
+	    if (!error) {
+		pci_write_config(scp->dev, 0x60 + (devno << 2), 0x004367f3, 4);
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
+		return 0;
+	    }
 	}
+	if (bootverbose)
+	    printf("ata%d: %s: setting PIO mode on Promise chip\n",
+		   scp->lun, (device == ATA_MASTER) ? "master" : "slave");
+	pci_write_config(scp->dev, 0x60 + (devno << 2), 0x004fe924, 4);
 	break;
     
     case 0x00041103:	/* HighPoint HPT366 IDE controller */
-	/* punt on ATAPI devices for now */
+	/* no ATAPI devices for now */
 	if ((device == ATA_MASTER && scp->devices & ATA_ATAPI_MASTER) ||
 	    (device == ATA_SLAVE && scp->devices & ATA_ATAPI_SLAVE))
 	    break;
@@ -294,11 +365,11 @@ ata_dmainit(struct ata_softc *scp, int32_t device,
 		printf("ata%d: %s: %s setting up UDMA4 mode on HPT366 chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    hpt366_timing(scp, device, ATA_MODE_UDMA4);
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA4;
-	    return 0;
+	    if (!error) {
+		hpt366_timing(scp, device, ATA_MODE_UDMA4);
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA4;
+		return 0;
+	    }
 	}
 	if (udmamode >=3 && !(pci_read_config(scp->dev, 0x5a, 1) & 0x2)) {
 	    error = ata_command(scp, device, ATA_C_SETFEATURES, 0, 0, 0,
@@ -307,44 +378,42 @@ ata_dmainit(struct ata_softc *scp, int32_t device,
 		printf("ata%d: %s: %s setting up UDMA3 mode on HPT366 chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    hpt366_timing(scp, device, ATA_MODE_UDMA3);
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA3;
-	    return 0;
+	    if (!error) {
+		hpt366_timing(scp, device, ATA_MODE_UDMA3);
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA3;
+		return 0;
+	    }
 	}
-	if (udmamode >=2) {
+	if (udmamode >= 2) {
 	    error = ata_command(scp, device, ATA_C_SETFEATURES, 0, 0, 0,
 				ATA_UDMA2, ATA_C_F_SETXFER, ATA_WAIT_READY);
 	    if (bootverbose)
 		printf("ata%d: %s: %s setting up UDMA2 mode on HPT366 chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    hpt366_timing(scp, device, ATA_MODE_UDMA2);
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA2;
-	    return 0;
+	    if (!error) {
+		hpt366_timing(scp, device, ATA_MODE_UDMA2);
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_UDMA2;
+		return 0;
+	    }
 	}
-	else if (wdmamode >= 2 && apiomode >= 4) {
+	if (wdmamode >= 2 && apiomode >= 4) {
 	    error = ata_command(scp, device, ATA_C_SETFEATURES, 0, 0, 0,
 				ATA_WDMA2, ATA_C_F_SETXFER, ATA_WAIT_READY);
 	    if (bootverbose)
 		printf("ata%d: %s: %s setting up WDMA2 mode on HPT366 chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    hpt366_timing(scp, device, ATA_MODE_WDMA2);
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
-	    return 0;
-	}
-	else {
-	    if (bootverbose)
-		printf("ata%d: %s: setting PIO mode on HPT366 chip\n",
-		       scp->lun, (device == ATA_MASTER) ? "master" : "slave");
-	    hpt366_timing(scp, device, ATA_MODE_PIO);
+	    if (!error) {
+		hpt366_timing(scp, device, ATA_MODE_WDMA2);
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
+		return 0;
+	    }
 	}
+	if (bootverbose)
+	    printf("ata%d: %s: setting PIO mode on HPT366 chip\n",
+		   scp->lun, (device == ATA_MASTER) ? "master" : "slave");
+	hpt366_timing(scp, device, ATA_MODE_PIO);
 	break;
 
     default:		/* unknown controller chip */
@@ -364,10 +433,10 @@ ata_dmainit(struct ata_softc *scp, int32_t device,
 		printf("ata%d: %s: %s setting up WDMA2 mode on generic chip\n",
 		       scp->lun, (device == ATA_MASTER) ? "master" : "slave",
 		       (error) ? "failed" : "success");
-	    if (error)
-		break;
-	    scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
-	    return 0;
+	    if (!error) {
+		scp->mode[(device == ATA_MASTER) ? 0 : 1] = ATA_MODE_WDMA2;
+		return 0;
+	    }
 	}
     }
     free(dmatab, M_DEVBUF);
diff --git a/sys/dev/ata/atapi-all.c b/sys/dev/ata/atapi-all.c
index 99425bf..831625a 100644
--- a/sys/dev/ata/atapi-all.c
+++ b/sys/dev/ata/atapi-all.c
@@ -645,11 +645,9 @@ atapi_timeout(struct atapi_request *request)
     if (request->flags & ATAPI_F_DMA_USED)
 	ata_dmadone(atp->controller);
 
-    if (request->retries < ATAPI_MAX_RETRIES) {
-        /* reinject this request */
-        request->retries++;
+    /* if retries still permit, reinject this request */
+    if (request->retries++ < ATAPI_MAX_RETRIES)
         TAILQ_INSERT_HEAD(&atp->controller->atapi_queue, request, chain);
-    }
     else {
         /* retries all used up, return error */
 	request->result = ATAPI_SK_RESERVED | ATAPI_E_ABRT;
diff --git a/sys/dev/ata/atapi-cd.c b/sys/dev/ata/atapi-cd.c
index 5e01911..cc18ecf 100644
--- a/sys/dev/ata/atapi-cd.c
+++ b/sys/dev/ata/atapi-cd.c
@@ -104,7 +104,7 @@ acdattach(struct atapi_softc *atp)
 {
     struct acd_softc *cdp;
     struct changer *chp;
-    int32_t error, count;
+    int32_t count, error = 0;
     static int32_t acd_cdev_done = 0, acdnlun = 0;
 
     if (!acd_cdev_done) {
diff --git a/sys/dev/ata/atapi-fd.c b/sys/dev/ata/atapi-fd.c
index a58e6bd..3872b54 100644
--- a/sys/dev/ata/atapi-fd.c
+++ b/sys/dev/ata/atapi-fd.c
@@ -127,7 +127,7 @@ afd_sense(struct afd_softc *fdp)
     int8_t ccb[16] = { ATAPI_MODE_SENSE_BIG, 0, ATAPI_REWRITEABLE_CAP_PAGE,
 		       0, 0, 0, 0, sizeof(buffer)>>8, sizeof(buffer) & 0xff,
 		       0, 0, 0, 0, 0, 0, 0 };
-    int32_t error, count;
+    int32_t count, error = 0;
 
     bzero(buffer, sizeof(buffer));
     /* get drive capabilities, some drives needs this repeated */
diff --git a/sys/dev/ata/atapi-tape.c b/sys/dev/ata/atapi-tape.c
index 70fcbc9..5a83a89 100644
--- a/sys/dev/ata/atapi-tape.c
+++ b/sys/dev/ata/atapi-tape.c
@@ -154,7 +154,7 @@ astattach(struct atapi_softc *atp)
 static int32_t
 ast_sense(struct ast_softc *stp)
 {
-    int32_t count, error;
+    int32_t count, error = 0;
 
     /* get drive capabilities, some drives needs this repeated */
     for (count = 0 ; count < 5 ; count++) {