Re: [PATCH] pata_scc.c: Workaround for errata A308

Re: [PATCH] pata_scc.c: Workaround for errata A308

[Jeff Garzik]

Akira Iguchi wrote:
> Workaround for errata A308: turn down the UDMA mode and retry
> the DMA command when the data lost condition is detected.
> 
> Signed-off-by: Kou Ishizaki <kou.ishizaki@toshiba.co.jp>
> Signed-off-by: Akira Iguchi <akira2.iguchi@toshiba.co.jp>

applied

[PATCH] pata_scc.c: Workaround for errata A308

[Akira Iguchi]

Workaround for errata A308: turn down the UDMA mode and retry
the DMA command when the data lost condition is detected.

Signed-off-by: Kou Ishizaki <kou.ishizaki@toshiba.co.jp>
Signed-off-by: Akira Iguchi <akira2.iguchi@toshiba.co.jp>
---

diff -purN -X linux-powerpc-git/Documentation/dontdiff linux-powerpc-git/drivers/ata/pata_scc.c linux-powerpc-git.mod/drivers/ata/pata_scc.c
--- linux-powerpc-git/drivers/ata/pata_scc.c	2007-06-11 12:03:24.000000000 +0900
+++ linux-powerpc-git.mod/drivers/ata/pata_scc.c	2007-07-10 17:13:01.000000000 +0900
@@ -238,6 +238,12 @@ static void scc_set_dmamode (struct ata_
 	else
 		offset = 0;	/* 100MHz */
 
+	/* errata A308 workaround: limit ATAPI UDMA mode to UDMA4 */
+	if (adev->class == ATA_DEV_ATAPI && speed > XFER_UDMA_4) {
+		printk(KERN_INFO "%s: limit ATAPI UDMA to UDMA4\n", DRV_NAME);
+		speed = XFER_UDMA_4;
+	}
+
 	if (speed >= XFER_UDMA_0)
 		idx = speed - XFER_UDMA_0;
 	else
@@ -724,22 +730,36 @@ static void scc_bmdma_stop (struct ata_q
 
 static u8 scc_bmdma_status (struct ata_port *ap)
 {
-	u8 host_stat;
 	void __iomem *mmio = ap->ioaddr.bmdma_addr;
-
-	host_stat = in_be32(mmio + SCC_DMA_STATUS);
-
-	/* Workaround for PTERADD: emulate DMA_INTR when
-	 * - IDE_STATUS[ERR] = 1
-	 * - INT_STATUS[INTRQ] = 1
-	 * - DMA_STATUS[IORACTA] = 1
-	 */
-	if (!(host_stat & ATA_DMA_INTR)) {
-		u32 int_status = in_be32(mmio + SCC_DMA_INTST);
-		if (ata_altstatus(ap) & ATA_ERR &&
-		    int_status & INTSTS_INTRQ &&
-		    host_stat & ATA_DMA_ACTIVE)
-			host_stat |= ATA_DMA_INTR;
+	u8 host_stat = in_be32(mmio + SCC_DMA_STATUS);
+	u32 int_status = in_be32(mmio + SCC_DMA_INTST);
+	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
+	static int retry = 0;
+
+	/* return if IOS_SS is cleared */
+	if (!(in_be32(mmio + SCC_DMA_CMD) & ATA_DMA_START))
+		return host_stat;
+
+	/* errata A252,A308 workaround: Step4 */
+	if (ata_altstatus(ap) & ATA_ERR && int_status & INTSTS_INTRQ)
+		return (host_stat | ATA_DMA_INTR);
+
+	/* errata A308 workaround Step5 */
+	if (int_status & INTSTS_IOIRQS) {
+		host_stat |= ATA_DMA_INTR;
+
+		/* We don't check ATAPI DMA because it is limited to UDMA4 */
+		if ((qc->tf.protocol == ATA_PROT_DMA &&
+		     qc->dev->xfer_mode > XFER_UDMA_4)) {
+			if (!(int_status & INTSTS_ACTEINT)) {
+				printk(KERN_WARNING "ata%u: data lost occurred. (ACTEINT==0, retry:%d)\n",
+				       ap->print_id, retry);
+				host_stat |= ATA_DMA_ERR;
+				if (retry++)
+					ap->udma_mask >>= 1;
+			} else
+				retry = 0;
+		}
 	}
 
 	return host_stat;

Re: [PATCH] pata_scc.c: Workaround for errata A308

[Alan Cox]

> +++ linux-powerpc-git.mod/drivers/ata/pata_scc.c	2007-07-10 17:13:01.000000000 +0900
> @@ -238,6 +238,12 @@ static void scc_set_dmamode (struct ata_
>  	else
>  		offset = 0;	/* 100MHz */
>  
> +	/* errata A308 workaround: limit ATAPI UDMA mode to UDMA4 */
> +	if (adev->class == ATA_DEV_ATAPI && speed > XFER_UDMA_4) {
> +		printk(KERN_INFO "%s: limit ATAPI UDMA to UDMA4\n", DRV_NAME);
> +		speed = XFER_UDMA_4;
> +	}
> +

NAK

You should not be modifying the speed you set in set_dmamode. Use the
mode_filter hook to mask the higher modes for ATAPI instead.


> +	/* errata A252,A308 workaround: Step4 */
> +	if (ata_altstatus(ap) & ATA_ERR && int_status & INTSTS_INTRQ)

Can we have more brackets here for clarity ?

> +		/* We don't check ATAPI DMA because it is limited to UDMA4 */
> +		if ((qc->tf.protocol == ATA_PROT_DMA &&
> +		     qc->dev->xfer_mode > XFER_UDMA_4)) {
> +			if (!(int_status & INTSTS_ACTEINT)) {
> +				printk(KERN_WARNING "ata%u: data lost occurred. (ACTEINT==0, retry:%d)\n",
> +				       ap->print_id, retry);

"data loss occurred", and even that sounds veyr scary so we should
probably try a more friendly message indicating that the user data has
not been lost but that the operation failed

> +				host_stat |= ATA_DMA_ERR;
> +				if (retry++)
> +					ap->udma_mask >>= 1;

NAK - you don't know that you can safely mess with modes this way.
udma_mask might contain holes. You need to do ap->udma_mask &= ~(1 <<
current UDMA mode);

Alan