[PATCH/RFC] ata_set_mode() to send SET FEATURES first

[PATCH/RFC] ata_set_mode() to send SET FEATURES first

[Albert Lee]

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

Jeff:

  Changes to ata_set_mode() for your review/comment:
1. Re-order ata_set_mode() such that "SET FEATURES" is sent to the device before
   the driver set the timing registers.

   Promise HBA hardware will look at the SET FEATURES and set the timing registers
    automatically. Under 133MHz PLL, the values set by the hardware is incorrect.
   Because ata_set_mode() call low-level driver to set timing registers 
   before "SET FEATURES", the correct value set by the software driver is later overwritten
   by hardware when "SET FEATURES" seen by the hardware.

2. Remove the restriction that master/slave devices cannot be set to different mode
   as in ata_get_mode_mask().

 The patch is against the libata-dev-2.6 tree.

Albert

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

*********************************************************************************
--- libata-dev-2.6-ori/drivers/scsi/libata-core.c 2004-11-10 17:53:24.000000000 +0800
+++ libata-dev-2.6/drivers/scsi/libata-core.c 2004-11-12 21:02:10.188293547 +0800
@@ -52,7 +52,7 @@
         unsigned long tmout_pat,
             unsigned long tmout);
 static void ata_set_mode(struct ata_port *ap);
-static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev);
+static int ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev);
 static unsigned int ata_get_mode_mask(struct ata_port *ap, int shift);
 static int fgb(u32 bitmap);
 static int ata_choose_xfer_mode(struct ata_port *ap,
@@ -1344,32 +1344,41 @@
 
  return 0xff;
 }
-
-static void ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
+/**
+ * Map xfer_mask to xfer_mode and issue set features to the device
+ */
+static int ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev, unsigned int xfer_mask, int shift)
 {
  int ofs, idx;
  u8 base;
+ int rc;
 
- if (!ata_dev_present(dev) || (ap->flags & ATA_FLAG_PORT_DISABLED))
-  return;
-
- if (dev->xfer_shift == ATA_SHIFT_PIO)
-  dev->flags |= ATA_DFLAG_PIO;
+ base = base_from_shift(shift);
 
- ata_dev_set_xfermode(ap, dev);
+ ofs = fgb(xfer_mask);
+ dev->xfer_mode = base + ofs;
 
- base = base_from_shift(dev->xfer_shift);
- ofs = dev->xfer_mode - base;
- idx = ofs + dev->xfer_shift;
+ dev->xfer_shift = shift;
+ idx = dev->xfer_shift + ofs;
  WARN_ON(idx >= ARRAY_SIZE(xfer_mode_str));
+  
+ if (shift == ATA_SHIFT_PIO) {
+  dev->pio_mode = dev->xfer_mode;
+ } else {
+  dev->dma_mode = dev->xfer_mode;
+ }
 
  DPRINTK("idx=%d xfer_shift=%u, xfer_mode=0x%x, base=0x%x, offset=%d\n",
   idx, dev->xfer_shift, (int)dev->xfer_mode, (int)base, ofs);
 
+ /* FIXME: return error if set feature fails */
+ rc = ata_dev_set_xfermode(ap, dev);
+
  printk(KERN_INFO "ata%u: dev %u configured for %s\n",
   ap->id, dev->devno, xfer_mode_str[idx]);
+ 
+ return rc;
 }
-
 static int ata_host_set_pio(struct ata_port *ap)
 {
  unsigned int mask;
@@ -1419,7 +1428,92 @@
   }
  }
 }
+/**
+ * Find the transfer mode between the HBA and the device
+ */
+static int ata_set_xfermode(struct ata_port *ap, struct ata_device *dev)
+{
+ unsigned int dev_pio_mask   = 0x07; /* PIO mode 0-2 */
+ unsigned int dev_mwdma_mask = 0;
+ unsigned int dev_udma_mask  = 0;
+
+ unsigned int xfer_pio_mask;
+ unsigned int xfer_mwdma_mask;
+ unsigned int xfer_udma_mask;
+
+ int no_dma = 1;
+
+ /* step 1. Assemble the PIO, MWDMA and UDMA mask of the device */
+ if (dev->id[53] & 0x02) // Is word 64 valid?
+  /* spec doesn't return explicit support for
+   * PIO0-2, so we fake it
+   */
+  dev_pio_mask |= (dev->id[ATA_ID_PIO_MODES] & 0x03) << 3;
+ 
+ dev_mwdma_mask = dev->id[ATA_ID_MWDMA_MODES] & 0x07;
+ 
+ if (dev->id[53] & 0x04) /* Is word 88 valid */
+  dev_udma_mask = dev->id[ATA_ID_UDMA_MODES] & 0xff;
+
+ /* FIXME: Check device black list for workable mask */
+ 
+ /* step 2. Find mode that both HBA and device can support */
+ xfer_pio_mask   = ap->pio_mask   & dev_pio_mask;
+ xfer_mwdma_mask = ap->mwdma_mask & dev_mwdma_mask;
+ xfer_udma_mask  = ap->udma_mask  & dev_udma_mask;
+ 
+ /* step 3. Check whether 80-conductor wire/SATA is needed and present */
+ if (xfer_udma_mask & (~ATA_UDMA_MASK_40C)) {
+  /* UDMA 3 or above.
+   * 80-conductor wire/SATA cable is needed
+   */
+  if ((ap->cbl == ATA_CBL_PATA40) ||
+      (ap->cbl == ATA_CBL_PATA_UNK)) {
+   printk(KERN_WARNING "ata%u: 40-conductor cable detected on port %d, transfer speed reduced to UDMA-33\n", 
+          ap->id, ap->port_no);
+   xfer_udma_mask &= ATA_UDMA_MASK_40C;
+  }
+ }
+ 
+ /* step 4. Map xfer_mask to xfer_mode
+  * Issue set features to the device for PIO mode.
+  */
+
+ ata_dev_set_mode(ap, dev, xfer_pio_mask, ATA_SHIFT_PIO);
+
+ 
+ /* step 5. Map xfer_mask to xfer_mode
+  * Issue set features to the device for MWDMA/UDMA mode.
+  */
+ if (xfer_udma_mask) {
+  no_dma = ata_dev_set_mode(ap, dev, xfer_udma_mask, ATA_SHIFT_UDMA);
+ } 
+ 
+ if (no_dma && xfer_mwdma_mask) 
+  /* Try MWDMA */
+  no_dma = ata_dev_set_mode(ap, dev, xfer_mwdma_mask, ATA_SHIFT_MWDMA);
 
+ if (no_dma)
+  /* Forced to PIO only */
+  dev->flags |= ATA_DFLAG_PIO;
+ 
+ /* step 6. Here the lowlevel driver change the HBA timing registers.
+  * This has to be done after "set features" to device
+  * because some HBA hardware (ex. Promise) will look at SET FEATURES
+  * and change the timing registers.
+  */
+
+ if (ap->ops->set_piomode)
+  ap->ops->set_piomode(ap, dev);
+
+ if (ap->ops->set_dmamode)
+  ap->ops->set_dmamode(ap, dev);
+ 
+ if (ap->ops->post_set_mode)
+  ap->ops->post_set_mode(ap);
+
+ return 0;
+}
 /**
  * ata_set_mode - Program timings and issue SET FEATURES - XFER
  * @ap: port on which timings will be programmed
@@ -1429,38 +1523,25 @@
  */
 static void ata_set_mode(struct ata_port *ap)
 {
- unsigned int i, xfer_shift;
- u8 xfer_mode;
- int rc;
+ int i;
 
- /* step 1: always set host PIO timings */
- rc = ata_host_set_pio(ap);
- if (rc)
-  goto err_out;
+ if (ap->flags & ATA_FLAG_PORT_DISABLED) 
+  return;
 
- /* step 2: choose the best data xfer mode */
- xfer_mode = xfer_shift = 0;
- rc = ata_choose_xfer_mode(ap, &xfer_mode, &xfer_shift);
- if (rc)
-  goto err_out;
+ for (i=0; i < ATA_MAX_DEVICES; i++) {
 
- /* step 3: if that xfer mode isn't PIO, set host DMA timings */
- if (xfer_shift != ATA_SHIFT_PIO)
-  ata_host_set_dma(ap, xfer_mode, xfer_shift);
-
- /* step 4: update devices' xfer mode */
- ata_dev_set_mode(ap, &ap->device[0]);
- ata_dev_set_mode(ap, &ap->device[1]);
+  struct ata_device *dev = &ap->device[i];
 
- if (ap->flags & ATA_FLAG_PORT_DISABLED)
-  return;
+  if (ata_dev_present(dev)) {
+   
+   ata_set_xfermode(ap, dev);
 
- if (ap->ops->post_set_mode)
-  ap->ops->post_set_mode(ap);
+   /* Is this needed? */
+   if (ap->flags & ATA_FLAG_PORT_DISABLED)
+    return;
 
- for (i = 0; i < 2; i++) {
-  struct ata_device *dev = &ap->device[i];
-  ata_dev_set_protocol(dev);
+   ata_dev_set_protocol(dev);
+  }
  }
 
  return;
@@ -1834,7 +1915,7 @@
  * LOCKING:
  */
 
-static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
+static int ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
 {
  DECLARE_COMPLETION(wait);
  struct ata_queued_cmd *qc;
@@ -1866,6 +1947,9 @@
   wait_for_completion(&wait);
 
  DPRINTK("EXIT\n");
+
+ /* FIXME: Return the result of set features */
+ return 0;
 }
 
 /**

[-- Attachment #2: ata_set_mode.patch --]
[-- Type: application/octet-stream, Size: 6633 bytes --]

--- libata-dev-2.6-ori/drivers/scsi/libata-core.c	2004-11-10 17:53:24.000000000 +0800
+++ libata-dev-2.6/drivers/scsi/libata-core.c	2004-11-12 21:02:10.188293547 +0800
@@ -52,7 +52,7 @@
 				    unsigned long tmout_pat,
 			    	    unsigned long tmout);
 static void ata_set_mode(struct ata_port *ap);
-static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev);
+static int ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev);
 static unsigned int ata_get_mode_mask(struct ata_port *ap, int shift);
 static int fgb(u32 bitmap);
 static int ata_choose_xfer_mode(struct ata_port *ap,
@@ -1344,32 +1344,41 @@
 
 	return 0xff;
 }
-
-static void ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
+/**
+ * Map xfer_mask to xfer_mode and issue set features to the device
+ */
+static int ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev, unsigned int xfer_mask, int shift)
 {
 	int ofs, idx;
 	u8 base;
+	int rc;
 
-	if (!ata_dev_present(dev) || (ap->flags & ATA_FLAG_PORT_DISABLED))
-		return;
-
-	if (dev->xfer_shift == ATA_SHIFT_PIO)
-		dev->flags |= ATA_DFLAG_PIO;
+	base = base_from_shift(shift);
 
-	ata_dev_set_xfermode(ap, dev);
+	ofs = fgb(xfer_mask);
+	dev->xfer_mode = base + ofs;
 
-	base = base_from_shift(dev->xfer_shift);
-	ofs = dev->xfer_mode - base;
-	idx = ofs + dev->xfer_shift;
+	dev->xfer_shift = shift;
+	idx = dev->xfer_shift + ofs;
 	WARN_ON(idx >= ARRAY_SIZE(xfer_mode_str));
+		
+	if (shift == ATA_SHIFT_PIO) {
+		dev->pio_mode = dev->xfer_mode;
+	} else {
+		dev->dma_mode = dev->xfer_mode;
+	}
 
 	DPRINTK("idx=%d xfer_shift=%u, xfer_mode=0x%x, base=0x%x, offset=%d\n",
 		idx, dev->xfer_shift, (int)dev->xfer_mode, (int)base, ofs);
 
+	/* FIXME: return error if set feature fails */
+	rc = ata_dev_set_xfermode(ap, dev);
+
 	printk(KERN_INFO "ata%u: dev %u configured for %s\n",
 		ap->id, dev->devno, xfer_mode_str[idx]);
+	
+	return rc;
 }
-
 static int ata_host_set_pio(struct ata_port *ap)
 {
 	unsigned int mask;
@@ -1419,7 +1428,92 @@
 		}
 	}
 }
+/**
+ * Find the transfer mode between the HBA and the device
+ */
+static int ata_set_xfermode(struct ata_port *ap, struct ata_device *dev)
+{
+	unsigned int dev_pio_mask   = 0x07; /* PIO mode 0-2 */
+	unsigned int dev_mwdma_mask = 0;
+	unsigned int dev_udma_mask  = 0;
+
+	unsigned int xfer_pio_mask;
+	unsigned int xfer_mwdma_mask;
+	unsigned int xfer_udma_mask;
+
+	int no_dma = 1;
+
+	/* step 1. Assemble the PIO, MWDMA and UDMA mask of the device */
+	if (dev->id[53] & 0x02) // Is word 64 valid?
+		/* spec doesn't return explicit support for
+		 * PIO0-2, so we fake it
+		 */
+		dev_pio_mask |= (dev->id[ATA_ID_PIO_MODES] & 0x03) << 3;
+	
+	dev_mwdma_mask = dev->id[ATA_ID_MWDMA_MODES] & 0x07;
+	
+	if (dev->id[53] & 0x04) /* Is word 88 valid */
+		dev_udma_mask = dev->id[ATA_ID_UDMA_MODES] & 0xff;
+
+	/* FIXME: Check device black list for workable mask */
+	
+	/* step 2. Find mode that both HBA and device can support */
+	xfer_pio_mask   = ap->pio_mask   & dev_pio_mask;
+	xfer_mwdma_mask = ap->mwdma_mask & dev_mwdma_mask;
+	xfer_udma_mask  = ap->udma_mask  & dev_udma_mask;
+	
+	/* step 3. Check whether 80-conductor wire/SATA is needed and present */
+	if (xfer_udma_mask & (~ATA_UDMA_MASK_40C)) {
+		/* UDMA 3 or above.
+		 * 80-conductor wire/SATA cable is needed
+		 */
+		if ((ap->cbl == ATA_CBL_PATA40) ||
+		    (ap->cbl == ATA_CBL_PATA_UNK)) {
+			printk(KERN_WARNING "ata%u: 40-conductor cable detected on port %d, transfer speed reduced to UDMA-33\n", 
+			       ap->id, ap->port_no);
+			xfer_udma_mask &= ATA_UDMA_MASK_40C;
+		}
+	}
+	
+	/* step 4. Map xfer_mask to xfer_mode
+	 * Issue set features to the device for PIO mode.
+	 */
+
+	ata_dev_set_mode(ap, dev, xfer_pio_mask, ATA_SHIFT_PIO);
+
+	
+	/* step 5. Map xfer_mask to xfer_mode
+	 * Issue set features to the device for MWDMA/UDMA mode.
+	 */
+	if (xfer_udma_mask) {
+		no_dma = ata_dev_set_mode(ap, dev, xfer_udma_mask, ATA_SHIFT_UDMA);
+	} 
+	
+	if (no_dma && xfer_mwdma_mask)	
+		/* Try MWDMA */
+		no_dma = ata_dev_set_mode(ap, dev, xfer_mwdma_mask, ATA_SHIFT_MWDMA);
 
+	if (no_dma)
+		/* Forced to PIO only */
+		dev->flags |= ATA_DFLAG_PIO;
+	
+	/* step 6. Here the lowlevel driver change the HBA timing registers.
+	 * This has to be done after "set features" to device
+	 * because some HBA hardware (ex. Promise) will look at SET FEATURES
+	 * and change the timing registers.
+	 */
+
+	if (ap->ops->set_piomode)
+		ap->ops->set_piomode(ap, dev);
+
+	if (ap->ops->set_dmamode)
+		ap->ops->set_dmamode(ap, dev);
+	
+	if (ap->ops->post_set_mode)
+		ap->ops->post_set_mode(ap);
+
+	return 0;
+}
 /**
  *	ata_set_mode - Program timings and issue SET FEATURES - XFER
  *	@ap: port on which timings will be programmed
@@ -1429,38 +1523,25 @@
  */
 static void ata_set_mode(struct ata_port *ap)
 {
-	unsigned int i, xfer_shift;
-	u8 xfer_mode;
-	int rc;
+	int i;
 
-	/* step 1: always set host PIO timings */
-	rc = ata_host_set_pio(ap);
-	if (rc)
-		goto err_out;
+	if (ap->flags & ATA_FLAG_PORT_DISABLED) 
+		return;
 
-	/* step 2: choose the best data xfer mode */
-	xfer_mode = xfer_shift = 0;
-	rc = ata_choose_xfer_mode(ap, &xfer_mode, &xfer_shift);
-	if (rc)
-		goto err_out;
+	for (i=0; i < ATA_MAX_DEVICES; i++) {
 
-	/* step 3: if that xfer mode isn't PIO, set host DMA timings */
-	if (xfer_shift != ATA_SHIFT_PIO)
-		ata_host_set_dma(ap, xfer_mode, xfer_shift);
-
-	/* step 4: update devices' xfer mode */
-	ata_dev_set_mode(ap, &ap->device[0]);
-	ata_dev_set_mode(ap, &ap->device[1]);
+		struct ata_device *dev = &ap->device[i];
 
-	if (ap->flags & ATA_FLAG_PORT_DISABLED)
-		return;
+		if (ata_dev_present(dev)) {
+			
+			ata_set_xfermode(ap, dev);
 
-	if (ap->ops->post_set_mode)
-		ap->ops->post_set_mode(ap);
+			/* Is this needed? */
+			if (ap->flags & ATA_FLAG_PORT_DISABLED)
+				return;
 
-	for (i = 0; i < 2; i++) {
-		struct ata_device *dev = &ap->device[i];
-		ata_dev_set_protocol(dev);
+			ata_dev_set_protocol(dev);
+		}
 	}
 
 	return;
@@ -1834,7 +1915,7 @@
  *	LOCKING:
  */
 
-static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
+static int ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
 {
 	DECLARE_COMPLETION(wait);
 	struct ata_queued_cmd *qc;
@@ -1866,6 +1947,9 @@
 		wait_for_completion(&wait);
 
 	DPRINTK("EXIT\n");
+
+	/* FIXME: Return the result of set features */
+	return 0;
 }
 
 /**

Re: [PATCH/RFC] ata_set_mode() to send SET FEATURES first

[Doug Maxey]

On Fri, 12 Nov 2004 21:27:37 +0800, "Albert Lee" wrote:
>Jeff:
>
>  Changes to ata_set_mode() for your review/comment:
>1. Re-order ata_set_mode() such that "SET FEATURES" is sent to the device before
>   the driver set the timing registers.

Allowing SET FEATURES to automatically set the mode is a prescription for 
disaster.   There is of course no problem when there is only one drive.  If 
there are multiple drives, the data stream going to the other will be 
corrupted on a bus timing mode change.  So I believe it is best to disallow 
this 'feature' of the chip. :-/


>
>   Promise HBA hardware will look at the SET FEATURES and set the timing registers
>    automatically. Under 133MHz PLL, the values set by the hardware is incorrect.
>   Because ata_set_mode() call low-level driver to set timing registers 
>   before "SET FEATURES", the correct value set by the software driver is later overwritten
>   by hardware when "SET FEATURES" seen by the hardware.
>

++doug

Re: [PATCH/RFC] ata_set_mode() to send SET FEATURES first

[Jeff Garzik]

I basically agree with Doug, but for a different reason.

If the hardware is setting an incorrect timing value, then the solution
is to disable that hardware feature, and manually set the timings.

	Jeff

Re: [PATCH/RFC] ata_set_mode() to send SET FEATURES first

[Albert Lee]

  I got it. It is the right thing to disable that hardware feature, rather
than change the sequence in ata_set_mode().

  I just checked the Promise manual but cannot find any hint to disable 
that feature. I'll ask Promise engineers how to do that. 
Thanks for the comments.

Albert

----- Original Message ----- 
From: "Jeff Garzik" <jgarzik@pobox.com>
To: "Albert Lee" <albertcc@tw.ibm.com>
Cc: "IDE Linux" <linux-ide@vger.kernel.org>
Sent: Sunday, November 14, 2004 9:23 AM
Subject: Re: [PATCH/RFC] ata_set_mode() to send SET FEATURES first


> I basically agree with Doug, but for a different reason.
> 
> If the hardware is setting an incorrect timing value, then the solution
> is to disable that hardware feature, and manually set the timings.
> 
> Jeff
> 
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>