[PATCHSET] libata: update timing and fix pata_amd transfer mode selection

[PATCHSET] libata: update timing and fix pata_amd transfer mode selection

[Tejun Heo]

Hello,

This patchset cleans up and improves PATA timing related code and fix
pata_amd transfer mode selection on top of the improvements.  This
patchset contains the following tweleve patches.

 0001-ata_generic-unindent-loop-in-generic_set_mode.patch
 0002-libata-export-xfermode-PATA-timing-related-functi.patch
 0003-libata-clean-up-xfermode-PATA-timing-related-stuf.patch
 0004-libata-kill-ata_id_to_dma_mode.patch
 0005-libata-xfer_mask-is-unsigned-int-not-unsigned-long.patch
 0006-libata-separate-out-ata_acpi_gtm_xfermask-from-pa.patch
 0007-libata-fix-ata_acpi_gtm_xfermask.patch
 0008-libata-implement-ata_timing_cycle2mode-and-use-it.patch
 0009-libata-implement-ata_acpi_init_gtm.patch
 0010-libata-reimplement-ata_acpi_cbl_80wire-using-ata_.patch
 0011-libata-add-ATA_CBL_PATA_IGN.patch
 0012-pata_amd-update-mode-selection-for-NV-PATAs.patch

0001-0005 are clean ups of timing handling code.  0006-0008 unifies
timing handling code in libata-acpi.c and pata_acpi.c with the core
timing code.

0009 implements initial GTM caching.  I thought about moving this to
LLDs but it's too much hassle.  GTM is host wide and doing it from
->error_handler would require cross-port synchronization.  Left
alternative is adding a separate hook.  Simply doing it during ACPI
attach is simpler.

0010 reimplements ata_acpi_cbl_80wire() using ata_acpi_gtm_xfermask().
This new function takes @gtm argument.  Both users (pata_via and
pata_amd) are converted to pass initial GTM.

0011 implements ATA_CBL_PATA_IGN which tells libata to ignore cable
type and supporess all related handling.

0012 updates mode selection for NV PATA controllers.  We just can't
tell what cable is attached on the controller either from host or
drive side.  So, for those controllers, pata_amd just sets cable type
to ATA_CBL_PATA_IGN and use ->mode_filter to limit transfer mode
according to BIOS/ACPI boot configuration.  If that fails (not
likely), pata_amd simply sets the highest allowed speed and let EH
figure out the mess.  With recent updates, EH should be able to figure
it out pretty soon.

This patchset is against

   #upstream-fixes (6bbfd53d47abd1fb20d7c93a9b19a75970b66f49)
 + update-speeddown patchset [1]

 Thanks.

 --
 tejun

 [1] http://thread.gmane.org/gmane.linux.ide/24497

[PATCH 01/12] ata_generic: unindent loop in generic_set_mode()

[Tejun Heo]

Unindent loop body in generic_set_mode().  This is to ease future
change.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/ata_generic.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index 9032998..fb5434c 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -54,21 +54,22 @@ static int generic_set_mode(struct ata_link *link, struct ata_device **unused)
 		dma_enabled = ioread8(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
 
 	ata_link_for_each_dev(dev, link) {
-		if (ata_dev_enabled(dev)) {
-			/* We don't really care */
-			dev->pio_mode = XFER_PIO_0;
-			dev->dma_mode = XFER_MW_DMA_0;
-			/* We do need the right mode information for DMA or PIO
-			   and this comes from the current configuration flags */
-			if (dma_enabled & (1 << (5 + dev->devno))) {
-				ata_id_to_dma_mode(dev, XFER_MW_DMA_0);
-				dev->flags &= ~ATA_DFLAG_PIO;
-			} else {
-				ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
-				dev->xfer_mode = XFER_PIO_0;
-				dev->xfer_shift = ATA_SHIFT_PIO;
-				dev->flags |= ATA_DFLAG_PIO;
-			}
+		if (!ata_dev_enabled(dev))
+			continue;
+
+		/* We don't really care */
+		dev->pio_mode = XFER_PIO_0;
+		dev->dma_mode = XFER_MW_DMA_0;
+		/* We do need the right mode information for DMA or PIO
+		   and this comes from the current configuration flags */
+		if (dma_enabled & (1 << (5 + dev->devno))) {
+			ata_id_to_dma_mode(dev, XFER_MW_DMA_0);
+			dev->flags &= ~ATA_DFLAG_PIO;
+		} else {
+			ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
+			dev->xfer_mode = XFER_PIO_0;
+			dev->xfer_shift = ATA_SHIFT_PIO;
+			dev->flags |= ATA_DFLAG_PIO;
 		}
 	}
 	return 0;
-- 
1.5.2.4

[PATCH 02/12] libata: export xfermode / PATA timing related functions

[Tejun Heo]

Export the following xfermode related functions.

* ata_pack_xfermask()
* ata_unpack_xfermask()
* ata_xfer_mask2mode()
* ata_xfer_mode2mask()
* ata_xfer_mode2shift()
* ata_mode_string()
* ata_id_xfermask()
* ata_timing_find_mode()

These functions will be used later by LLD updates.  While at it,
change unsigned short @speed to u8 @xfer_mode in
ata_timing_find_mode() for consistency.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |   33 +++++++++++++++++++--------------
 include/linux/libata.h    |   10 ++++++++++
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index bd77927..d4506f8 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -441,9 +441,8 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
  *	RETURNS:
  *	Packed xfer_mask.
  */
-static unsigned int ata_pack_xfermask(unsigned int pio_mask,
-				      unsigned int mwdma_mask,
-				      unsigned int udma_mask)
+unsigned int ata_pack_xfermask(unsigned int pio_mask,
+			       unsigned int mwdma_mask, unsigned int udma_mask)
 {
 	return ((pio_mask << ATA_SHIFT_PIO) & ATA_MASK_PIO) |
 		((mwdma_mask << ATA_SHIFT_MWDMA) & ATA_MASK_MWDMA) |
@@ -460,10 +459,8 @@ static unsigned int ata_pack_xfermask(unsigned int pio_mask,
  *	Unpack @xfer_mask into @pio_mask, @mwdma_mask and @udma_mask.
  *	Any NULL distination masks will be ignored.
  */
-static void ata_unpack_xfermask(unsigned int xfer_mask,
-				unsigned int *pio_mask,
-				unsigned int *mwdma_mask,
-				unsigned int *udma_mask)
+void ata_unpack_xfermask(unsigned int xfer_mask, unsigned int *pio_mask,
+			 unsigned int *mwdma_mask, unsigned int *udma_mask)
 {
 	if (pio_mask)
 		*pio_mask = (xfer_mask & ATA_MASK_PIO) >> ATA_SHIFT_PIO;
@@ -496,7 +493,7 @@ static const struct ata_xfer_ent {
  *	RETURNS:
  *	Matching XFER_* value, 0 if no match found.
  */
-static u8 ata_xfer_mask2mode(unsigned int xfer_mask)
+u8 ata_xfer_mask2mode(unsigned int xfer_mask)
 {
 	int highbit = fls(xfer_mask) - 1;
 	const struct ata_xfer_ent *ent;
@@ -519,7 +516,7 @@ static u8 ata_xfer_mask2mode(unsigned int xfer_mask)
  *	RETURNS:
  *	Matching xfer_mask, 0 if no match found.
  */
-static unsigned int ata_xfer_mode2mask(u8 xfer_mode)
+unsigned int ata_xfer_mode2mask(u8 xfer_mode)
 {
 	const struct ata_xfer_ent *ent;
 
@@ -541,7 +538,7 @@ static unsigned int ata_xfer_mode2mask(u8 xfer_mode)
  *	RETURNS:
  *	Matching xfer_shift, -1 if no match found.
  */
-static int ata_xfer_mode2shift(unsigned int xfer_mode)
+int ata_xfer_mode2shift(unsigned int xfer_mode)
 {
 	const struct ata_xfer_ent *ent;
 
@@ -565,7 +562,7 @@ static int ata_xfer_mode2shift(unsigned int xfer_mode)
  *	Constant C string representing highest speed listed in
  *	@mode_mask, or the constant C string "<n/a>".
  */
-static const char *ata_mode_string(unsigned int xfer_mask)
+const char *ata_mode_string(unsigned int xfer_mask)
 {
 	static const char * const xfer_mode_str[] = {
 		"PIO0",
@@ -1454,7 +1451,7 @@ static inline void ata_dump_id(const u16 *id)
  *	RETURNS:
  *	Computed xfermask
  */
-static unsigned int ata_id_xfermask(const u16 *id)
+unsigned int ata_id_xfermask(const u16 *id)
 {
 	unsigned int pio_mask, mwdma_mask, udma_mask;
 
@@ -2908,11 +2905,11 @@ void ata_timing_merge(const struct ata_timing *a, const struct ata_timing *b,
 	if (what & ATA_TIMING_UDMA   ) m->udma    = max(a->udma,    b->udma);
 }
 
-static const struct ata_timing *ata_timing_find_mode(unsigned short speed)
+const struct ata_timing *ata_timing_find_mode(u8 xfer_mode)
 {
 	const struct ata_timing *t;
 
-	for (t = ata_timing; t->mode != speed; t++)
+	for (t = ata_timing; t->mode != xfer_mode; t++)
 		if (t->mode == 0xFF)
 			return NULL;
 	return t;
@@ -7578,6 +7575,13 @@ EXPORT_SYMBOL_GPL(ata_std_dev_select);
 EXPORT_SYMBOL_GPL(sata_print_link_status);
 EXPORT_SYMBOL_GPL(ata_tf_to_fis);
 EXPORT_SYMBOL_GPL(ata_tf_from_fis);
+EXPORT_SYMBOL_GPL(ata_pack_xfermask);
+EXPORT_SYMBOL_GPL(ata_unpack_xfermask);
+EXPORT_SYMBOL_GPL(ata_xfer_mask2mode);
+EXPORT_SYMBOL_GPL(ata_xfer_mode2mask);
+EXPORT_SYMBOL_GPL(ata_xfer_mode2shift);
+EXPORT_SYMBOL_GPL(ata_mode_string);
+EXPORT_SYMBOL_GPL(ata_id_xfermask);
 EXPORT_SYMBOL_GPL(ata_check_status);
 EXPORT_SYMBOL_GPL(ata_altstatus);
 EXPORT_SYMBOL_GPL(ata_exec_command);
@@ -7645,6 +7649,7 @@ EXPORT_SYMBOL_GPL(ata_id_to_dma_mode);
 EXPORT_SYMBOL_GPL(ata_scsi_simulate);
 
 EXPORT_SYMBOL_GPL(ata_pio_need_iordy);
+EXPORT_SYMBOL_GPL(ata_timing_find_mode);
 EXPORT_SYMBOL_GPL(ata_timing_compute);
 EXPORT_SYMBOL_GPL(ata_timing_merge);
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3609c07..2198a61 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -849,6 +849,15 @@ extern void ata_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
 extern void ata_tf_to_fis(const struct ata_taskfile *tf,
 			  u8 pmp, int is_cmd, u8 *fis);
 extern void ata_tf_from_fis(const u8 *fis, struct ata_taskfile *tf);
+extern unsigned int ata_pack_xfermask(unsigned int pio_mask,
+			unsigned int mwdma_mask, unsigned int udma_mask);
+extern void ata_unpack_xfermask(unsigned int xfer_mask, unsigned int *pio_mask,
+			unsigned int *mwdma_mask, unsigned int *udma_mask);
+extern u8 ata_xfer_mask2mode(unsigned int xfer_mask);
+extern unsigned int ata_xfer_mode2mask(u8 xfer_mode);
+extern int ata_xfer_mode2shift(unsigned int xfer_mode);
+extern const char *ata_mode_string(unsigned int xfer_mask);
+extern unsigned int ata_id_xfermask(const u16 *id);
 extern void ata_noop_dev_select(struct ata_port *ap, unsigned int device);
 extern void ata_std_dev_select(struct ata_port *ap, unsigned int device);
 extern u8 ata_check_status(struct ata_port *ap);
@@ -918,6 +927,7 @@ extern int ata_cable_unknown(struct ata_port *ap);
  */
 
 extern unsigned int ata_pio_need_iordy(const struct ata_device *);
+extern const struct ata_timing *ata_timing_find_mode(u8 xfer_mode);
 extern int ata_timing_compute(struct ata_device *, unsigned short,
 			      struct ata_timing *, int, int);
 extern void ata_timing_merge(const struct ata_timing *,
-- 
1.5.2.4

[PATCH 03/12] libata: clean up xfermode / PATA timing related stuff

[Tejun Heo]

* s/ATA_BITS_(PIO|MWDMA|UDMA)/ATA_NR_\1_MODES/g

* Consistently use 0xff to indicate invalid transfer mode (0x00 is
  valid for PIO_SLOW).

* Make ata_xfer_mode2mask() return proper mode mask instead of just
  the highest bit.

* Sort ata_timing table in increasing xfermode order and update
  ata_timing_find_mode() accordingly.

This patch doesn't introduce any behavior change.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |   82 ++++++++++++++++++++++-----------------------
 include/linux/libata.h    |   21 ++++++-----
 2 files changed, 52 insertions(+), 51 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d4506f8..dc7e2f1 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -474,9 +474,9 @@ static const struct ata_xfer_ent {
 	int shift, bits;
 	u8 base;
 } ata_xfer_tbl[] = {
-	{ ATA_SHIFT_PIO, ATA_BITS_PIO, XFER_PIO_0 },
-	{ ATA_SHIFT_MWDMA, ATA_BITS_MWDMA, XFER_MW_DMA_0 },
-	{ ATA_SHIFT_UDMA, ATA_BITS_UDMA, XFER_UDMA_0 },
+	{ ATA_SHIFT_PIO, ATA_NR_PIO_MODES, XFER_PIO_0 },
+	{ ATA_SHIFT_MWDMA, ATA_NR_MWDMA_MODES, XFER_MW_DMA_0 },
+	{ ATA_SHIFT_UDMA, ATA_NR_UDMA_MODES, XFER_UDMA_0 },
 	{ -1, },
 };
 
@@ -491,7 +491,7 @@ static const struct ata_xfer_ent {
  *	None.
  *
  *	RETURNS:
- *	Matching XFER_* value, 0 if no match found.
+ *	Matching XFER_* value, 0xff if no match found.
  */
 u8 ata_xfer_mask2mode(unsigned int xfer_mask)
 {
@@ -501,7 +501,7 @@ u8 ata_xfer_mask2mode(unsigned int xfer_mask)
 	for (ent = ata_xfer_tbl; ent->shift >= 0; ent++)
 		if (highbit >= ent->shift && highbit < ent->shift + ent->bits)
 			return ent->base + highbit - ent->shift;
-	return 0;
+	return 0xff;
 }
 
 /**
@@ -522,7 +522,8 @@ unsigned int ata_xfer_mode2mask(u8 xfer_mode)
 
 	for (ent = ata_xfer_tbl; ent->shift >= 0; ent++)
 		if (xfer_mode >= ent->base && xfer_mode < ent->base + ent->bits)
-			return 1 << (ent->shift + xfer_mode - ent->base);
+			return ((2 << (ent->shift + xfer_mode - ent->base)) - 1)
+				& ~((1 << ent->shift) - 1);
 	return 0;
 }
 
@@ -1300,7 +1301,7 @@ void ata_id_to_dma_mode(struct ata_device *dev, u8 unknown)
 	/* Select the mode in use */
 	mode = ata_xfer_mask2mode(mask);
 
-	if (mode != 0) {
+	if (mode != 0xff) {
 		ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
 		       ata_mode_string(mask));
 	} else {
@@ -2841,38 +2842,33 @@ int sata_set_spd(struct ata_link *link)
  */
 
 static const struct ata_timing ata_timing[] = {
+/*	{ XFER_PIO_SLOW, 120, 290, 240, 960, 290, 240, 960,   0 }, */
+	{ XFER_PIO_0,     70, 290, 240, 600, 165, 150, 600,   0 },
+	{ XFER_PIO_1,     50, 290,  93, 383, 125, 100, 383,   0 },
+	{ XFER_PIO_2,     30, 290,  40, 330, 100,  90, 240,   0 },
+	{ XFER_PIO_3,     30,  80,  70, 180,  80,  70, 180,   0 },
+	{ XFER_PIO_4,     25,  70,  25, 120,  70,  25, 120,   0 },
+	{ XFER_PIO_5,     15,  65,  25, 100,  65,  25, 100,   0 },
+	{ XFER_PIO_6,     10,  55,  20,  80,  55,  20,  80,   0 },
 
-	{ XFER_UDMA_6,     0,   0,   0,   0,   0,   0,   0,  15 },
-	{ XFER_UDMA_5,     0,   0,   0,   0,   0,   0,   0,  20 },
-	{ XFER_UDMA_4,     0,   0,   0,   0,   0,   0,   0,  30 },
-	{ XFER_UDMA_3,     0,   0,   0,   0,   0,   0,   0,  45 },
+	{ XFER_SW_DMA_0, 120,   0,   0,   0, 480, 480, 960,   0 },
+	{ XFER_SW_DMA_1,  90,   0,   0,   0, 240, 240, 480,   0 },
+	{ XFER_SW_DMA_2,  60,   0,   0,   0, 120, 120, 240,   0 },
 
-	{ XFER_MW_DMA_4,  25,   0,   0,   0,  55,  20,  80,   0 },
+	{ XFER_MW_DMA_0,  60,   0,   0,   0, 215, 215, 480,   0 },
+	{ XFER_MW_DMA_1,  45,   0,   0,   0,  80,  50, 150,   0 },
+	{ XFER_MW_DMA_2,  25,   0,   0,   0,  70,  25, 120,   0 },
 	{ XFER_MW_DMA_3,  25,   0,   0,   0,  65,  25, 100,   0 },
-	{ XFER_UDMA_2,     0,   0,   0,   0,   0,   0,   0,  60 },
-	{ XFER_UDMA_1,     0,   0,   0,   0,   0,   0,   0,  80 },
-	{ XFER_UDMA_0,     0,   0,   0,   0,   0,   0,   0, 120 },
+	{ XFER_MW_DMA_4,  25,   0,   0,   0,  55,  20,  80,   0 },
 
 /*	{ XFER_UDMA_SLOW,  0,   0,   0,   0,   0,   0,   0, 150 }, */
-
-	{ XFER_MW_DMA_2,  25,   0,   0,   0,  70,  25, 120,   0 },
-	{ XFER_MW_DMA_1,  45,   0,   0,   0,  80,  50, 150,   0 },
-	{ XFER_MW_DMA_0,  60,   0,   0,   0, 215, 215, 480,   0 },
-
-	{ XFER_SW_DMA_2,  60,   0,   0,   0, 120, 120, 240,   0 },
-	{ XFER_SW_DMA_1,  90,   0,   0,   0, 240, 240, 480,   0 },
-	{ XFER_SW_DMA_0, 120,   0,   0,   0, 480, 480, 960,   0 },
-
-	{ XFER_PIO_6,     10,  55,  20,  80,  55,  20,  80,   0 },
-	{ XFER_PIO_5,     15,  65,  25, 100,  65,  25, 100,   0 },
-	{ XFER_PIO_4,     25,  70,  25, 120,  70,  25, 120,   0 },
-	{ XFER_PIO_3,     30,  80,  70, 180,  80,  70, 180,   0 },
-
-	{ XFER_PIO_2,     30, 290,  40, 330, 100,  90, 240,   0 },
-	{ XFER_PIO_1,     50, 290,  93, 383, 125, 100, 383,   0 },
-	{ XFER_PIO_0,     70, 290, 240, 600, 165, 150, 600,   0 },
-
-/*	{ XFER_PIO_SLOW, 120, 290, 240, 960, 290, 240, 960,   0 }, */
+	{ XFER_UDMA_0,     0,   0,   0,   0,   0,   0,   0, 120 },
+	{ XFER_UDMA_1,     0,   0,   0,   0,   0,   0,   0,  80 },
+	{ XFER_UDMA_2,     0,   0,   0,   0,   0,   0,   0,  60 },
+	{ XFER_UDMA_3,     0,   0,   0,   0,   0,   0,   0,  45 },
+	{ XFER_UDMA_4,     0,   0,   0,   0,   0,   0,   0,  30 },
+	{ XFER_UDMA_5,     0,   0,   0,   0,   0,   0,   0,  20 },
+	{ XFER_UDMA_6,     0,   0,   0,   0,   0,   0,   0,  15 },
 
 	{ 0xFF }
 };
@@ -2907,12 +2903,14 @@ void ata_timing_merge(const struct ata_timing *a, const struct ata_timing *b,
 
 const struct ata_timing *ata_timing_find_mode(u8 xfer_mode)
 {
-	const struct ata_timing *t;
+	const struct ata_timing *t = ata_timing;
+
+	while (xfer_mode > t->mode)
+		t++;
 
-	for (t = ata_timing; t->mode != xfer_mode; t++)
-		if (t->mode == 0xFF)
-			return NULL;
-	return t;
+	if (xfer_mode == t->mode)
+		return t;
+	return NULL;
 }
 
 int ata_timing_compute(struct ata_device *adev, unsigned short speed,
@@ -3175,7 +3173,7 @@ int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
 		dev->dma_mode = ata_xfer_mask2mode(dma_mask);
 
 		found = 1;
-		if (dev->dma_mode)
+		if (dev->dma_mode != 0xff)
 			used_dma = 1;
 	}
 	if (!found)
@@ -3186,7 +3184,7 @@ int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
 		if (!ata_dev_enabled(dev))
 			continue;
 
-		if (!dev->pio_mode) {
+		if (dev->pio_mode == 0xff) {
 			ata_dev_printk(dev, KERN_WARNING, "no PIO support\n");
 			rc = -EINVAL;
 			goto out;
@@ -3200,7 +3198,7 @@ int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
 
 	/* step 3: set host DMA timings */
 	ata_link_for_each_dev(dev, link) {
-		if (!ata_dev_enabled(dev) || !dev->dma_mode)
+		if (!ata_dev_enabled(dev) || dev->dma_mode == 0xff)
 			continue;
 
 		dev->xfer_mode = dev->dma_mode;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2198a61..4deb8a7 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -267,17 +267,20 @@ enum {
 	/* encoding various smaller bitmaps into a single
 	 * unsigned int bitmap
 	 */
-	ATA_BITS_PIO		= 7,
-	ATA_BITS_MWDMA		= 5,
-	ATA_BITS_UDMA		= 8,
+	ATA_NR_PIO_MODES	= 7,
+	ATA_NR_MWDMA_MODES	= 5,
+	ATA_NR_UDMA_MODES	= 8,
 
 	ATA_SHIFT_PIO		= 0,
-	ATA_SHIFT_MWDMA		= ATA_SHIFT_PIO + ATA_BITS_PIO,
-	ATA_SHIFT_UDMA		= ATA_SHIFT_MWDMA + ATA_BITS_MWDMA,
-
-	ATA_MASK_PIO		= ((1 << ATA_BITS_PIO) - 1) << ATA_SHIFT_PIO,
-	ATA_MASK_MWDMA		= ((1 << ATA_BITS_MWDMA) - 1) << ATA_SHIFT_MWDMA,
-	ATA_MASK_UDMA		= ((1 << ATA_BITS_UDMA) - 1) << ATA_SHIFT_UDMA,
+	ATA_SHIFT_MWDMA		= ATA_SHIFT_PIO + ATA_NR_PIO_MODES,
+	ATA_SHIFT_UDMA		= ATA_SHIFT_MWDMA + ATA_NR_MWDMA_MODES,
+
+	ATA_MASK_PIO		= ((1 << ATA_NR_PIO_MODES) - 1)
+					<< ATA_SHIFT_PIO,
+	ATA_MASK_MWDMA		= ((1 << ATA_NR_MWDMA_MODES) - 1)
+					<< ATA_SHIFT_MWDMA,
+	ATA_MASK_UDMA		= ((1 << ATA_NR_UDMA_MODES) - 1)
+					<< ATA_SHIFT_UDMA,
 
 	/* size of buffer to pad xfers ending on unaligned boundaries */
 	ATA_DMA_PAD_SZ		= 4,
-- 
1.5.2.4

[PATCH 04/12] libata: kill ata_id_to_dma_mode()

[Tejun Heo]

ata_id_to_dma_mode() isn't quite generic.  The function is basically
privately implemented ata_id_xfermask() combined with hardcoded mode
printing and configuration which are specific to ata_generic.

Kill the function and open code it in generic_set_mode() using generic
xfermode handling functions.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/ata_generic.c |   17 ++++++++++++++++-
 drivers/ata/libata-core.c |   43 -------------------------------------------
 include/linux/libata.h    |    1 -
 3 files changed, 16 insertions(+), 45 deletions(-)

diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index fb5434c..d11a4f1 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -63,7 +63,22 @@ static int generic_set_mode(struct ata_link *link, struct ata_device **unused)
 		/* We do need the right mode information for DMA or PIO
 		   and this comes from the current configuration flags */
 		if (dma_enabled & (1 << (5 + dev->devno))) {
-			ata_id_to_dma_mode(dev, XFER_MW_DMA_0);
+			unsigned int xfer_mask = ata_id_xfermask(dev->id);
+			const char *name;
+
+			if (xfer_mask & (ATA_MASK_MWDMA | ATA_MASK_UDMA))
+				name = ata_mode_string(xfer_mask);
+			else {
+				/* SWDMA perhaps? */
+				name = "DMA";
+				xfer_mask |= ata_xfer_mode2mask(XFER_MW_DMA_0);
+			}
+
+			ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
+				       name);
+
+			dev->xfer_mode = ata_xfer_mask2mode(xfer_mask);
+			dev->xfer_shift = ata_xfer_mode2shift(dev->xfer_mode);
 			dev->flags &= ~ATA_DFLAG_PIO;
 		} else {
 			ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index dc7e2f1..0eae1c6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1274,48 +1274,6 @@ static int ata_hpa_resize(struct ata_device *dev)
 }
 
 /**
- *	ata_id_to_dma_mode	-	Identify DMA mode from id block
- *	@dev: device to identify
- *	@unknown: mode to assume if we cannot tell
- *
- *	Set up the timing values for the device based upon the identify
- *	reported values for the DMA mode. This function is used by drivers
- *	which rely upon firmware configured modes, but wish to report the
- *	mode correctly when possible.
- *
- *	In addition we emit similarly formatted messages to the default
- *	ata_dev_set_mode handler, in order to provide consistency of
- *	presentation.
- */
-
-void ata_id_to_dma_mode(struct ata_device *dev, u8 unknown)
-{
-	unsigned int mask;
-	u8 mode;
-
-	/* Pack the DMA modes */
-	mask = ((dev->id[63] >> 8) << ATA_SHIFT_MWDMA) & ATA_MASK_MWDMA;
-	if (dev->id[53] & 0x04)
-		mask |= ((dev->id[88] >> 8) << ATA_SHIFT_UDMA) & ATA_MASK_UDMA;
-
-	/* Select the mode in use */
-	mode = ata_xfer_mask2mode(mask);
-
-	if (mode != 0xff) {
-		ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
-		       ata_mode_string(mask));
-	} else {
-		/* SWDMA perhaps ? */
-		mode = unknown;
-		ata_dev_printk(dev, KERN_INFO, "configured for DMA\n");
-	}
-
-	/* Configure the device reporting */
-	dev->xfer_mode = mode;
-	dev->xfer_shift = ata_xfer_mode2shift(mode);
-}
-
-/**
  *	ata_noop_dev_select - Select device 0/1 on ATA bus
  *	@ap: ATA channel to manipulate
  *	@device: ATA device (numbered from zero) to select
@@ -7643,7 +7601,6 @@ EXPORT_SYMBOL_GPL(ata_host_resume);
 #endif /* CONFIG_PM */
 EXPORT_SYMBOL_GPL(ata_id_string);
 EXPORT_SYMBOL_GPL(ata_id_c_string);
-EXPORT_SYMBOL_GPL(ata_id_to_dma_mode);
 EXPORT_SYMBOL_GPL(ata_scsi_simulate);
 
 EXPORT_SYMBOL_GPL(ata_pio_need_iordy);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 4deb8a7..f84106a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -888,7 +888,6 @@ extern void ata_id_string(const u16 *id, unsigned char *s,
 			  unsigned int ofs, unsigned int len);
 extern void ata_id_c_string(const u16 *id, unsigned char *s,
 			    unsigned int ofs, unsigned int len);
-extern void ata_id_to_dma_mode(struct ata_device *dev, u8 unknown);
 extern void ata_bmdma_setup(struct ata_queued_cmd *qc);
 extern void ata_bmdma_start(struct ata_queued_cmd *qc);
 extern void ata_bmdma_stop(struct ata_queued_cmd *qc);
-- 
1.5.2.4

Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()

[Alan Cox]

On Tue,  6 Nov 2007 14:39:02 +0900
Tejun Heo <htejun@gmail.com> wrote:

> ata_id_to_dma_mode() isn't quite generic.  The function is basically
> privately implemented ata_id_xfermask() combined with hardcoded mode
> printing and configuration which are specific to ata_generic.

I anticpiated other users. This seems a backward move - we end up
exporting lots of low level detail into the drivers which should be none
of their business.

Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()

[Tejun Heo]

Alan Cox wrote:
> On Tue,  6 Nov 2007 14:39:02 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> ata_id_to_dma_mode() isn't quite generic.  The function is basically
>> privately implemented ata_id_xfermask() combined with hardcoded mode
>> printing and configuration which are specific to ata_generic.
> 
> I anticpiated other users. This seems a backward move - we end up
> exporting lots of low level detail into the drivers which should be none
> of their business.

Those functions need to be exported anyway for pata_amd and I think it's
natural to export them as xfer mode and mask handling is pretty
fundamental and having those helpers around is handy when implementing
LLD-specific mode selection.

ata_id_to_dma_mode() just seemed a bit too specific to me.  Sans
ata_id_xfermask() part, printing configured mode and using "DMA" when
there's no applicable DMA mode don't seem too useful as generic xfer
mode/mask helper.  I agree that the latter part of the function can be
factored tho (setting xfer_mode, shift and clearing PIO flag according
to determined xfer_mask).  But since ata_generic() is currently the only
user, factoring that part out seems an overkill.

Well, I guess this is a matter of taste after all.  How about letting
Jeff determine it?  I'm okay as long as generic ata_id_xfermask() is
used there.

Thanks.

-- 
tejun

Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()

[Jeff Garzik]

Tejun Heo wrote:
> ata_id_to_dma_mode() isn't quite generic.  The function is basically
> privately implemented ata_id_xfermask() combined with hardcoded mode
> printing and configuration which are specific to ata_generic.
> 
> Kill the function and open code it in generic_set_mode() using generic
> xfermode handling functions.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK patches 1-4

[PATCH 05/12] libata: xfer_mask is unsigned int not unsigned long

[Tejun Heo]

xfer_mask is unsigned int not unsigned long.  Change ->mode_filter to
take and return unsigned int.

While at it, rename @adev of ata_pci_default_filter() to @dev for
consistency.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-sff.c       |    5 +++--
 drivers/ata/pata_acpi.c        |    2 +-
 drivers/ata/pata_ali.c         |    2 +-
 drivers/ata/pata_hpt366.c      |    2 +-
 drivers/ata/pata_hpt37x.c      |    4 ++--
 drivers/ata/pata_pdc2027x.c    |    4 ++--
 drivers/ata/pata_scc.c         |    2 +-
 drivers/ata/pata_serverworks.c |    4 ++--
 include/linux/libata.h         |    5 +++--
 9 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 48acc09..2f2ca3d 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -877,12 +877,13 @@ int ata_pci_clear_simplex(struct pci_dev *pdev)
 	return 0;
 }
 
-unsigned long ata_pci_default_filter(struct ata_device *adev, unsigned long xfer_mask)
+unsigned int ata_pci_default_filter(struct ata_device *dev,
+				    unsigned int xfer_mask)
 {
 	/* Filter out DMA modes if the device has been configured by
 	   the BIOS as PIO only */
 
-	if (adev->link->ap->ioaddr.bmdma_addr == NULL)
+	if (dev->link->ap->ioaddr.bmdma_addr == NULL)
 		xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
 	return xfer_mask;
 }
diff --git a/drivers/ata/pata_acpi.c b/drivers/ata/pata_acpi.c
index e4542ab..2323646 100644
--- a/drivers/ata/pata_acpi.c
+++ b/drivers/ata/pata_acpi.c
@@ -164,7 +164,7 @@ static unsigned long pacpi_discover_modes(struct ata_port *ap, struct ata_device
  *	this case the list of discovered valid modes obtained by ACPI probing
  */
 
-static unsigned long pacpi_mode_filter(struct ata_device *adev, unsigned long mask)
+static unsigned int pacpi_mode_filter(struct ata_device *adev, unsigned int mask)
 {
 	struct pata_acpi *acpi = adev->link->ap->private_data;
 	return ata_pci_default_filter(adev, mask & acpi->mask[adev->devno]);
diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
index 364534e..aae8a44 100644
--- a/drivers/ata/pata_ali.c
+++ b/drivers/ata/pata_ali.c
@@ -105,7 +105,7 @@ static int ali_c2_cable_detect(struct ata_port *ap)
  *	fix that later on. Also ensure we do not do UDMA on WDC drives
  */
 
-static unsigned long ali_20_filter(struct ata_device *adev, unsigned long mask)
+static unsigned int ali_20_filter(struct ata_device *adev, unsigned int mask)
 {
 	char model_num[ATA_ID_PROD_LEN + 1];
 	/* No DMA on anything but a disk for now */
diff --git a/drivers/ata/pata_hpt366.c b/drivers/ata/pata_hpt366.c
index 0713872..1f277d5 100644
--- a/drivers/ata/pata_hpt366.c
+++ b/drivers/ata/pata_hpt366.c
@@ -174,7 +174,7 @@ static int hpt_dma_blacklisted(const struct ata_device *dev, char *modestr, cons
  *	Block UDMA on devices that cause trouble with this controller.
  */
 
-static unsigned long hpt366_filter(struct ata_device *adev, unsigned long mask)
+static unsigned int hpt366_filter(struct ata_device *adev, unsigned int mask)
 {
 	if (adev->class == ATA_DEV_ATA) {
 		if (hpt_dma_blacklisted(adev, "UDMA",  bad_ata33))
diff --git a/drivers/ata/pata_hpt37x.c b/drivers/ata/pata_hpt37x.c
index 3816b86..37f327e 100644
--- a/drivers/ata/pata_hpt37x.c
+++ b/drivers/ata/pata_hpt37x.c
@@ -275,7 +275,7 @@ static const char *bad_ata100_5[] = {
  *	Block UDMA on devices that cause trouble with this controller.
  */
 
-static unsigned long hpt370_filter(struct ata_device *adev, unsigned long mask)
+static unsigned int hpt370_filter(struct ata_device *adev, unsigned int mask)
 {
 	if (adev->class == ATA_DEV_ATA) {
 		if (hpt_dma_blacklisted(adev, "UDMA", bad_ata33))
@@ -293,7 +293,7 @@ static unsigned long hpt370_filter(struct ata_device *adev, unsigned long mask)
  *	Block UDMA on devices that cause trouble with this controller.
  */
 
-static unsigned long hpt370a_filter(struct ata_device *adev, unsigned long mask)
+static unsigned int hpt370a_filter(struct ata_device *adev, unsigned int mask)
 {
 	if (adev->class == ATA_DEV_ATA) {
 		if (hpt_dma_blacklisted(adev, "UDMA100", bad_ata100_5))
diff --git a/drivers/ata/pata_pdc2027x.c b/drivers/ata/pata_pdc2027x.c
index 2622577..dd20239 100644
--- a/drivers/ata/pata_pdc2027x.c
+++ b/drivers/ata/pata_pdc2027x.c
@@ -67,7 +67,7 @@ static void pdc2027x_error_handler(struct ata_port *ap);
 static void pdc2027x_set_piomode(struct ata_port *ap, struct ata_device *adev);
 static void pdc2027x_set_dmamode(struct ata_port *ap, struct ata_device *adev);
 static int pdc2027x_check_atapi_dma(struct ata_queued_cmd *qc);
-static unsigned long pdc2027x_mode_filter(struct ata_device *adev, unsigned long mask);
+static unsigned int pdc2027x_mode_filter(struct ata_device *adev, unsigned int mask);
 static int pdc2027x_cable_detect(struct ata_port *ap);
 static int pdc2027x_set_mode(struct ata_link *link, struct ata_device **r_failed);
 
@@ -336,7 +336,7 @@ static void pdc2027x_error_handler(struct ata_port *ap)
  *	Block UDMA on devices that cause trouble with this controller.
  */
 
-static unsigned long pdc2027x_mode_filter(struct ata_device *adev, unsigned long mask)
+static unsigned int pdc2027x_mode_filter(struct ata_device *adev, unsigned int mask)
 {
 	unsigned char model_num[ATA_ID_PROD_LEN + 1];
 	struct ata_device *pair = ata_dev_pair(adev);
diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c
index ea2ef9f..0edcdf7 100644
--- a/drivers/ata/pata_scc.c
+++ b/drivers/ata/pata_scc.c
@@ -258,7 +258,7 @@ static void scc_set_dmamode (struct ata_port *ap, struct ata_device *adev)
 		 JCTSStbl[offset][idx] << 16 | JCENVTtbl[offset][idx]);
 }
 
-unsigned long scc_mode_filter(struct ata_device *adev, unsigned long mask)
+unsigned int scc_mode_filter(struct ata_device *adev, unsigned int mask)
 {
 	/* errata A308 workaround: limit ATAPI UDMA mode to UDMA4 */
 	if (adev->class == ATA_DEV_ATAPI &&
diff --git a/drivers/ata/pata_serverworks.c b/drivers/ata/pata_serverworks.c
index 8bed888..708c789 100644
--- a/drivers/ata/pata_serverworks.c
+++ b/drivers/ata/pata_serverworks.c
@@ -195,7 +195,7 @@ static u8 serverworks_is_csb(struct pci_dev *pdev)
  *	bug we hit.
  */
 
-static unsigned long serverworks_osb4_filter(struct ata_device *adev, unsigned long mask)
+static unsigned int serverworks_osb4_filter(struct ata_device *adev, unsigned int mask)
 {
 	if (adev->class == ATA_DEV_ATA)
 		mask &= ~ATA_MASK_UDMA;
@@ -211,7 +211,7 @@ static unsigned long serverworks_osb4_filter(struct ata_device *adev, unsigned l
  *	Check the blacklist and disable UDMA5 if matched
  */
 
-static unsigned long serverworks_csb_filter(struct ata_device *adev, unsigned long mask)
+static unsigned int serverworks_csb_filter(struct ata_device *adev, unsigned int mask)
 {
 	const char *p;
 	char model_num[ATA_ID_PROD_LEN + 1];
diff --git a/include/linux/libata.h b/include/linux/libata.h
index f84106a..80c711a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -668,7 +668,7 @@ struct ata_port_operations {
 
 	void (*set_piomode) (struct ata_port *, struct ata_device *);
 	void (*set_dmamode) (struct ata_port *, struct ata_device *);
-	unsigned long (*mode_filter) (struct ata_device *, unsigned long);
+	unsigned int (*mode_filter) (struct ata_device *, unsigned int);
 
 	void (*tf_load) (struct ata_port *ap, const struct ata_taskfile *tf);
 	void (*tf_read) (struct ata_port *ap, struct ata_taskfile *tf);
@@ -989,7 +989,8 @@ extern int ata_pci_prepare_sff_host(struct pci_dev *pdev,
 				    const struct ata_port_info * const * ppi,
 				    struct ata_host **r_host);
 extern int pci_test_config_bits(struct pci_dev *pdev, const struct pci_bits *bits);
-extern unsigned long ata_pci_default_filter(struct ata_device *, unsigned long);
+extern unsigned int ata_pci_default_filter(struct ata_device *dev,
+					   unsigned int xfer_mask);
 #endif /* CONFIG_PCI */
 
 /*
-- 
1.5.2.4

Re: [PATCH 05/12] libata: xfer_mask is unsigned int not unsigned long

[Alan Cox]

On Tue,  6 Nov 2007 14:39:03 +0900
Tejun Heo <htejun@gmail.com> wrote:

> xfer_mask is unsigned int not unsigned long.  Change ->mode_filter to
> take and return unsigned int.
> 
> While at it, rename @adev of ata_pci_default_filter() to @dev for
> consistency.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

The filter type was purposefully unsigned long to allow for expansion (eg
for SWDMA) without breaking drivers. No problem with it changing but I'd
say "unsigned int" was the worst possible choice - its now shorter (no
room for expansion) and size dependant on arch. u32 would be shorter and
consistent across all platforms, ulong would have more room for expansion.

Alan

Re: [PATCH 05/12] libata: xfer_mask is unsigned int not unsigned long

[Tejun Heo]

Alan Cox wrote:
> On Tue,  6 Nov 2007 14:39:03 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> xfer_mask is unsigned int not unsigned long.  Change ->mode_filter to
>> take and return unsigned int.
>>
>> While at it, rename @adev of ata_pci_default_filter() to @dev for
>> consistency.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> The filter type was purposefully unsigned long to allow for expansion (eg
> for SWDMA) without breaking drivers. No problem with it changing but I'd
> say "unsigned int" was the worst possible choice - its now shorter (no
> room for expansion) and size dependant on arch. u32 would be shorter and
> consistent across all platforms, ulong would have more room for expansion.

I agree it should be one of u* types and am happy to convert.  This
patch is primarily for consistency as in libata-core, xfer_mask is
unsigned int.  My first proposal was u32 too but Jeff vetoed it.  IIRC,
Jeff has affection for machine-independent integral types.  :-)

Anyways, I think ulong is worse than uint because ulong differs in size
between 32 and 64 archs.  What's the good of extra 32bits in flags space
if it's not there on 32bit archs?  Also, we currently only use 20bits of
xfermask, 12 extra bits seem enough for the time, especially as
everything is moving over to SATA where xfermode doesn't really matter, no?

Jeff, are you okay with u32 or 64?

-- 
tejun

Re: [PATCH 05/12] libata: xfer_mask is unsigned int not unsigned long

[Jeff Garzik]

Tejun Heo wrote:
> Alan Cox wrote:
>> On Tue,  6 Nov 2007 14:39:03 +0900
>> Tejun Heo <htejun@gmail.com> wrote:
>>
>>> xfer_mask is unsigned int not unsigned long.  Change ->mode_filter to
>>> take and return unsigned int.
>>>
>>> While at it, rename @adev of ata_pci_default_filter() to @dev for
>>> consistency.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> The filter type was purposefully unsigned long to allow for expansion (eg
>> for SWDMA) without breaking drivers. No problem with it changing but I'd
>> say "unsigned int" was the worst possible choice - its now shorter (no
>> room for expansion) and size dependant on arch. u32 would be shorter and
>> consistent across all platforms, ulong would have more room for expansion.
> 
> I agree it should be one of u* types and am happy to convert.  This
> patch is primarily for consistency as in libata-core, xfer_mask is
> unsigned int.  My first proposal was u32 too but Jeff vetoed it.  IIRC,
> Jeff has affection for machine-independent integral types.  :-)
> 
> Anyways, I think ulong is worse than uint because ulong differs in size
> between 32 and 64 archs.  What's the good of extra 32bits in flags space
> if it's not there on 32bit archs?  Also, we currently only use 20bits of
> xfermask, 12 extra bits seem enough for the time, especially as
> everything is moving over to SATA where xfermode doesn't really matter, no?
> 
> Jeff, are you okay with u32 or 64?

unsigned long is IMO the best choice for bitmaps.

* it is a machine int on all(?) architectures

* it is 32-bit on 32-bit architectures

* it is consistent with test_bit(), set_bit() and lib/bitmap.c interfaces

* conversion to test_bit() and lib/bitmap.c interfaces as a future step 
is trivial

* your structs inflate on 64-bit due to pointers anyway, so I see little 
real consequence to using the lower 32 bits of an unsigned long as a 
portable meme.

	Jeff

Re: [PATCH 05/12] libata: xfer_mask is unsigned int not unsigned long

[Tejun Heo]

Jeff Garzik wrote:
>> Jeff, are you okay with u32 or 64?
> 
> unsigned long is IMO the best choice for bitmaps.
> 
> * it is a machine int on all(?) architectures

I don't really see much point in this.  What's the advantage of a
machine int for xfer mask?

> * it is 32-bit on 32-bit architectures

The problem I see for using native integral types for flags or mask is
that it's not fixed in size, so you can only use the smallest of all the
supported architectures.  We know for all archs we support, both
unsigned int and unsigned long are at least 32bits long but to me making
the assumption about expected number of bits using u32 or u64 is much
more important than other considerations.

> * it is consistent with test_bit(), set_bit() and lib/bitmap.c interfaces
> 
> * conversion to test_bit() and lib/bitmap.c interfaces as a future step
> is trivial

I don't think it's likely that we'll need those heavy machineries for
xfermask and the correct approach is to convert when the need actually
arises.

> * your structs inflate on 64-bit due to pointers anyway, so I see little
> real consequence to using the lower 32 bits of an unsigned long as a
> portable meme.

I'm not trying to optimize performance or size.  I'm trying to make
programming assumptions clear.  I think it's silly to optimize anything
for xfermask.  It just has to work and be clear to people working with it.

An optimal but overkill solution would be using fast_u32 or fast_u64
types such that the compiler can choose as necessary but as we don't
have that yet && xfermask handling is a very very cold path, I think we
should be aiming for clarity.

Thanks.

-- 
tejun

Re: [PATCH 05/12] libata: xfer_mask is unsigned int not unsigned long

[Jeff Garzik]

Tejun Heo wrote:
> xfer_mask is unsigned int not unsigned long.  Change ->mode_filter to
> take and return unsigned int.
> 
> While at it, rename @adev of ata_pci_default_filter() to @dev for
> consistency.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-sff.c       |    5 +++--
>  drivers/ata/pata_acpi.c        |    2 +-
>  drivers/ata/pata_ali.c         |    2 +-
>  drivers/ata/pata_hpt366.c      |    2 +-
>  drivers/ata/pata_hpt37x.c      |    4 ++--
>  drivers/ata/pata_pdc2027x.c    |    4 ++--
>  drivers/ata/pata_scc.c         |    2 +-
>  drivers/ata/pata_serverworks.c |    4 ++--
>  include/linux/libata.h         |    5 +++--
>  9 files changed, 16 insertions(+), 14 deletions(-)

as I noted I permit unsigned long, which is a naturally aligned machine 
int on our platforms.

Consistency patches in the other direction (moving ATA_MASK_* to a 
separate enum and marking with UL suffix) are welcome.

Re: [PATCH 05/12] libata: xfer_mask is unsigned int not unsigned long

[Jeff Garzik]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> xfer_mask is unsigned int not unsigned long.  Change ->mode_filter to
>> take and return unsigned int.
>>
>> While at it, rename @adev of ata_pci_default_filter() to @dev for
>> consistency.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>>  drivers/ata/libata-sff.c       |    5 +++--
>>  drivers/ata/pata_acpi.c        |    2 +-
>>  drivers/ata/pata_ali.c         |    2 +-
>>  drivers/ata/pata_hpt366.c      |    2 +-
>>  drivers/ata/pata_hpt37x.c      |    4 ++--
>>  drivers/ata/pata_pdc2027x.c    |    4 ++--
>>  drivers/ata/pata_scc.c         |    2 +-
>>  drivers/ata/pata_serverworks.c |    4 ++--
>>  include/linux/libata.h         |    5 +++--
>>  9 files changed, 16 insertions(+), 14 deletions(-)
> 
> as I noted I permit unsigned long, which is a naturally aligned machine 
> int on our platforms.

er, s/permit/prefer/

Re: [PATCH 05/12] libata: xfer_mask is unsigned int not unsigned long

[Tejun Heo]

Jeff Garzik wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> xfer_mask is unsigned int not unsigned long.  Change ->mode_filter to
>>> take and return unsigned int.
>>>
>>> While at it, rename @adev of ata_pci_default_filter() to @dev for
>>> consistency.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>> ---
>>>  drivers/ata/libata-sff.c       |    5 +++--
>>>  drivers/ata/pata_acpi.c        |    2 +-
>>>  drivers/ata/pata_ali.c         |    2 +-
>>>  drivers/ata/pata_hpt366.c      |    2 +-
>>>  drivers/ata/pata_hpt37x.c      |    4 ++--
>>>  drivers/ata/pata_pdc2027x.c    |    4 ++--
>>>  drivers/ata/pata_scc.c         |    2 +-
>>>  drivers/ata/pata_serverworks.c |    4 ++--
>>>  include/linux/libata.h         |    5 +++--
>>>  9 files changed, 16 insertions(+), 14 deletions(-)
>>
>> as I noted I permit unsigned long, which is a naturally aligned
>> machine int on our platforms.
> 
> er, s/permit/prefer/

Alright.  I don't agree but don't have much problem either.  Will update
them the other way.

-- 
tejun

[PATCH 06/12] libata: separate out ata_acpi_gtm_xfermask() from pacpi_discover_modes()

[Tejun Heo]

Finding out matching transfer mode from ACPI GTM values is useful for
other purposes too.  Separate out the function and timing tables from
pata_acpi::pacpi_discover_modes().

Other than checking shared-configuration bit after doing
ata_acpi_gtm() in pacpi_discover_modes() which should be safe, this
patch doesn't introduce any behavior change.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/ata/libata-acpi.c |   78 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/pata_acpi.c   |   66 ++++++-------------------------------
 include/linux/libata.h    |   25 ++++++++++++++
 3 files changed, 114 insertions(+), 55 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 08a52dd..5ffa542 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -424,6 +424,84 @@ int ata_acpi_cbl_80wire(struct ata_port *ap)
 
 EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
 
+/* Welcome to ACPI, bring a bucket */
+const unsigned int ata_acpi_pio_cycle[7] = {
+	600, 383, 240, 180, 120, 100, 80
+};
+EXPORT_SYMBOL_GPL(ata_acpi_pio_cycle);
+
+const unsigned int ata_acpi_mwdma_cycle[5] = {
+	480, 150, 120, 100, 80
+};
+EXPORT_SYMBOL_GPL(ata_acpi_mwdma_cycle);
+
+const unsigned int ata_acpi_udma_cycle[7] = {
+	120, 80, 60, 45, 30, 20, 15
+};
+EXPORT_SYMBOL_GPL(ata_acpi_udma_cycle);
+
+/**
+ * ata_acpi_gtm_xfermode - determine xfermode from GTM parameter
+ * @dev: target device
+ * @gtm: GTM parameter to use
+ *
+ * Determine xfermask for @dev from @gtm.
+ *
+ * LOCKING:
+ * None.
+ *
+ * RETURNS:
+ * Determined xfermask.
+ */
+unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
+				   const struct ata_acpi_gtm *gtm)
+{
+	int unit, i;
+	u32 t;
+	unsigned long mask = (0x7f << ATA_SHIFT_UDMA) | (0x7 << ATA_SHIFT_MWDMA) | (0x1F << ATA_SHIFT_PIO);
+
+	/* we always use the 0 slot for crap hardware */
+	unit = dev->devno;
+	if (!(gtm->flags & 0x10))
+		unit = 0;
+
+	/* start by scanning for PIO modes */
+	for (i = 0; i < 7; i++) {
+		t = gtm->drive[unit].pio;
+		if (t <= ata_acpi_pio_cycle[i]) {
+			mask |= (2 << (ATA_SHIFT_PIO + i)) - 1;
+			break;
+		}
+	}
+
+	/* See if we have MWDMA or UDMA data. We don't bother with
+	 * MWDMA if UDMA is available as this means the BIOS set UDMA
+	 * and our error changedown if it works is UDMA to PIO anyway.
+	 */
+	if (gtm->flags & (1 << (2 * unit))) {
+		/* MWDMA */
+		for (i = 0; i < 5; i++) {
+			t = gtm->drive[unit].dma;
+			if (t <= ata_acpi_mwdma_cycle[i]) {
+				mask |= (2 << (ATA_SHIFT_MWDMA + i)) - 1;
+				break;
+			}
+		}
+	} else {
+		/* UDMA */
+		for (i = 0; i < 7; i++) {
+			t = gtm->drive[unit].dma;
+			if (t <= ata_acpi_udma_cycle[i]) {
+				mask |= (2 << (ATA_SHIFT_UDMA + i)) - 1;
+				break;
+			}
+		}
+	}
+
+	return mask;
+}
+EXPORT_SYMBOL_GPL(ata_acpi_gtm_xfermask);
+
 /**
  * taskfile_load_raw - send taskfile registers to host controller
  * @dev: target ATA device
diff --git a/drivers/ata/pata_acpi.c b/drivers/ata/pata_acpi.c
index 2323646..fc6e3a5 100644
--- a/drivers/ata/pata_acpi.c
+++ b/drivers/ata/pata_acpi.c
@@ -81,17 +81,6 @@ static void pacpi_error_handler(struct ata_port *ap)
 				  NULL, ata_std_postreset);
 }
 
-/* Welcome to ACPI, bring a bucket */
-static const unsigned int pio_cycle[7] = {
-	600, 383, 240, 180, 120, 100, 80
-};
-static const unsigned int mwdma_cycle[5] = {
-	480, 150, 120, 100, 80
-};
-static const unsigned int udma_cycle[7] = {
-	120, 80, 60, 45, 30, 20, 15
-};
-
 /**
  *	pacpi_discover_modes	-	filter non ACPI modes
  *	@adev: ATA device
@@ -103,56 +92,20 @@ static const unsigned int udma_cycle[7] = {
 
 static unsigned long pacpi_discover_modes(struct ata_port *ap, struct ata_device *adev)
 {
-	int unit = adev->devno;
 	struct pata_acpi *acpi = ap->private_data;
-	int i;
-	u32 t;
-	unsigned long mask = (0x7f << ATA_SHIFT_UDMA) | (0x7 << ATA_SHIFT_MWDMA) | (0x1F << ATA_SHIFT_PIO);
-
 	struct ata_acpi_gtm probe;
+	unsigned int xfer_mask;
 
 	probe = acpi->gtm;
 
-	/* We always use the 0 slot for crap hardware */
-	if (!(probe.flags & 0x10))
-		unit = 0;
-
 	ata_acpi_gtm(ap, &probe);
 
-	/* Start by scanning for PIO modes */
-	for (i = 0; i < 7; i++) {
-		t = probe.drive[unit].pio;
-		if (t <= pio_cycle[i]) {
-			mask |= (2 << (ATA_SHIFT_PIO + i)) - 1;
-			break;
-		}
-	}
+	xfer_mask = ata_acpi_gtm_xfermask(adev, &probe);
 
-	/* See if we have MWDMA or UDMA data. We don't bother with MWDMA
-	   if UDMA is availabe as this means the BIOS set UDMA and our
-	   error changedown if it works is UDMA to PIO anyway */
-	if (probe.flags & (1 << (2 * unit))) {
-		/* MWDMA */
-		for (i = 0; i < 5; i++) {
-			t = probe.drive[unit].dma;
-			if (t <= mwdma_cycle[i]) {
-				mask |= (2 << (ATA_SHIFT_MWDMA + i)) - 1;
-				break;
-			}
-		}
-	} else {
-		/* UDMA */
-		for (i = 0; i < 7; i++) {
-			t = probe.drive[unit].dma;
-			if (t <= udma_cycle[i]) {
-				mask |= (2 << (ATA_SHIFT_UDMA + i)) - 1;
-				break;
-			}
-		}
-	}
-	if (mask & (0xF8 << ATA_SHIFT_UDMA))
+	if (xfer_mask & (0xF8 << ATA_SHIFT_UDMA))
 		ap->cbl = ATA_CBL_PATA80;
-	return mask;
+
+	return xfer_mask;
 }
 
 /**
@@ -185,7 +138,8 @@ static void pacpi_set_piomode(struct ata_port *ap, struct ata_device *adev)
 		unit = 0;
 
 	/* Now stuff the nS values into the structure */
-	acpi->gtm.drive[unit].pio = pio_cycle[adev->pio_mode - XFER_PIO_0];
+	acpi->gtm.drive[unit].pio =
+		ata_acpi_pio_cycle[adev->pio_mode - XFER_PIO_0];
 	ata_acpi_stm(ap, &acpi->gtm);
 	/* See what mode we actually got */
 	ata_acpi_gtm(ap, &acpi->gtm);
@@ -207,10 +161,12 @@ static void pacpi_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 
 	/* Now stuff the nS values into the structure */
 	if (adev->dma_mode >= XFER_UDMA_0) {
-		acpi->gtm.drive[unit].dma = udma_cycle[adev->dma_mode - XFER_UDMA_0];
+		acpi->gtm.drive[unit].dma =
+			ata_acpi_udma_cycle[adev->dma_mode - XFER_UDMA_0];
 		acpi->gtm.flags |= (1 << (2 * unit));
 	} else {
-		acpi->gtm.drive[unit].dma = mwdma_cycle[adev->dma_mode - XFER_MW_DMA_0];
+		acpi->gtm.drive[unit].dma =
+			ata_acpi_mwdma_cycle[adev->dma_mode - XFER_MW_DMA_0];
 		acpi->gtm.flags &= ~(1 << (2 * unit));
 	}
 	ata_acpi_stm(ap, &acpi->gtm);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 80c711a..e626137 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -955,11 +955,36 @@ enum {
 
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
+extern const unsigned int ata_acpi_pio_cycle[7];
+extern const unsigned int ata_acpi_mwdma_cycle[5];
+extern const unsigned int ata_acpi_udma_cycle[7];
+
 extern int ata_acpi_cbl_80wire(struct ata_port *ap);
 int ata_acpi_stm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
 int ata_acpi_gtm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
+unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
+				   const struct ata_acpi_gtm *gtm);
+
 #else
 static inline int ata_acpi_cbl_80wire(struct ata_port *ap) { return 0; }
+
+static inline int ata_acpi_stm(const struct ata_port *ap,
+			       struct ata_acpi_gtm *stm)
+{
+	return -ENOSYS;
+}
+
+static inline int ata_acpi_gtm(const struct ata_port *ap,
+			       struct ata_acpi_gtm *stm)
+{
+	return -ENOSYS;
+}
+
+static inline unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
+					const struct ata_acpi_gtm *gtm)
+{
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_PCI
-- 
1.5.2.4

Re: [PATCH 06/12] libata: separate out ata_acpi_gtm_xfermask() from pacpi_discover_modes()

[Alan Cox]

> +/* Welcome to ACPI, bring a bucket */
> +const unsigned int ata_acpi_pio_cycle[7] = {
> +	600, 383, 240, 180, 120, 100, 80
> +};
> +EXPORT_SYMBOL_GPL(ata_acpi_pio_cycle);
> +
> +const unsigned int ata_acpi_mwdma_cycle[5] = {
> +	480, 150, 120, 100, 80
> +};
> +EXPORT_SYMBOL_GPL(ata_acpi_mwdma_cycle);
> +
> +const unsigned int ata_acpi_udma_cycle[7] = {
> +	120, 80, 60, 45, 30, 20, 15
> +};
> +EXPORT_SYMBOL_GPL(ata_acpi_udma_cycle);

Do we really need to keep exporting all these things. So far this patch
set has exported a set of very specific ACPI arrays and a load of
internal functions. That to me says the splitting up is wrong.

One option would be to make those tables private and simply make the
pata_acpi driver use the ata_timing functions

Alan

Re: [PATCH 06/12] libata: separate out ata_acpi_gtm_xfermask() from pacpi_discover_modes()

[Tejun Heo]

Alan Cox wrote:
>> +/* Welcome to ACPI, bring a bucket */
>> +const unsigned int ata_acpi_pio_cycle[7] = {
>> +	600, 383, 240, 180, 120, 100, 80
>> +};
>> +EXPORT_SYMBOL_GPL(ata_acpi_pio_cycle);
>> +
>> +const unsigned int ata_acpi_mwdma_cycle[5] = {
>> +	480, 150, 120, 100, 80
>> +};
>> +EXPORT_SYMBOL_GPL(ata_acpi_mwdma_cycle);
>> +
>> +const unsigned int ata_acpi_udma_cycle[7] = {
>> +	120, 80, 60, 45, 30, 20, 15
>> +};
>> +EXPORT_SYMBOL_GPL(ata_acpi_udma_cycle);
> 
> Do we really need to keep exporting all these things. So far this patch
> set has exported a set of very specific ACPI arrays and a load of
> internal functions. That to me says the splitting up is wrong.
> 
> One option would be to make those tables private and simply make the
> pata_acpi driver use the ata_timing functions

This is mid-step of merging ACPI timing handling into the standard
ata_timing mechanism.  These will go away in later patch.

-- 
tejun

Re: [PATCH 06/12] libata: separate out ata_acpi_gtm_xfermask() from pacpi_discover_modes()

[Jeff Garzik]

Tejun Heo wrote:
> Finding out matching transfer mode from ACPI GTM values is useful for
> other purposes too.  Separate out the function and timing tables from
> pata_acpi::pacpi_discover_modes().
> 
> Other than checking shared-configuration bit after doing
> ata_acpi_gtm() in pacpi_discover_modes() which should be safe, this
> patch doesn't introduce any behavior change.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
>  drivers/ata/libata-acpi.c |   78 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/ata/pata_acpi.c   |   66 ++++++-------------------------------
>  include/linux/libata.h    |   25 ++++++++++++++
>  3 files changed, 114 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 08a52dd..5ffa542 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -424,6 +424,84 @@ int ata_acpi_cbl_80wire(struct ata_port *ap)
>  
>  EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
>  
> +/* Welcome to ACPI, bring a bucket */
> +const unsigned int ata_acpi_pio_cycle[7] = {
> +	600, 383, 240, 180, 120, 100, 80
> +};
> +EXPORT_SYMBOL_GPL(ata_acpi_pio_cycle);
> +
> +const unsigned int ata_acpi_mwdma_cycle[5] = {
> +	480, 150, 120, 100, 80
> +};
> +EXPORT_SYMBOL_GPL(ata_acpi_mwdma_cycle);
> +
> +const unsigned int ata_acpi_udma_cycle[7] = {
> +	120, 80, 60, 45, 30, 20, 15
> +};
> +EXPORT_SYMBOL_GPL(ata_acpi_udma_cycle);

these exports seem a bit ugly

If pressed I will apply, but...

[PATCH 07/12] libata: fix ata_acpi_gtm_xfermask()

[Tejun Heo]

ata_acpi_gtm_xfermask() as separated out from pacpi_discover_modes()
has various bugs.  Fix them.

* The wrong comparison operator is used when finding for matching
  cycle resulting totally bogus result.

* With the comparion operator fixed, boundary condtion handling is
  clumsy.

* Setting of any DMA mask bit set all bits in PIO mask.

* MWDMA and UDMA blocks are swapped.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/ata/libata-acpi.c |   42 +++++++++++++++++++++---------------------
 1 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 5ffa542..06d9961 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -456,49 +456,49 @@ EXPORT_SYMBOL_GPL(ata_acpi_udma_cycle);
 unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
 				   const struct ata_acpi_gtm *gtm)
 {
+	unsigned int pio_mask = 0, mwdma_mask = 0, udma_mask = 0;
 	int unit, i;
 	u32 t;
-	unsigned long mask = (0x7f << ATA_SHIFT_UDMA) | (0x7 << ATA_SHIFT_MWDMA) | (0x1F << ATA_SHIFT_PIO);
 
 	/* we always use the 0 slot for crap hardware */
 	unit = dev->devno;
 	if (!(gtm->flags & 0x10))
 		unit = 0;
 
+	/* Values larger than the longest cycle results in 0 mask
+	 * while values equal to smaller than the shortest cycle
+	 * results in mask which includes all supported modes.
+	 * Disabled transfer method has the value of 0xffffffff which
+	 * will always result in 0 mask.
+	 */
+
 	/* start by scanning for PIO modes */
-	for (i = 0; i < 7; i++) {
-		t = gtm->drive[unit].pio;
-		if (t <= ata_acpi_pio_cycle[i]) {
-			mask |= (2 << (ATA_SHIFT_PIO + i)) - 1;
+	t = gtm->drive[unit].pio;
+	for (i = 0; i < ARRAY_SIZE(ata_acpi_pio_cycle); i++)
+		if (t > ata_acpi_pio_cycle[i])
 			break;
-		}
-	}
+	pio_mask = (1 << i) - 1;
 
 	/* See if we have MWDMA or UDMA data. We don't bother with
 	 * MWDMA if UDMA is available as this means the BIOS set UDMA
 	 * and our error changedown if it works is UDMA to PIO anyway.
 	 */
-	if (gtm->flags & (1 << (2 * unit))) {
+	t = gtm->drive[unit].dma;
+	if (!(gtm->flags & (1 << (2 * unit)))) {
 		/* MWDMA */
-		for (i = 0; i < 5; i++) {
-			t = gtm->drive[unit].dma;
-			if (t <= ata_acpi_mwdma_cycle[i]) {
-				mask |= (2 << (ATA_SHIFT_MWDMA + i)) - 1;
+		for (i = 0; i < ARRAY_SIZE(ata_acpi_mwdma_cycle); i++)
+			if (t > ata_acpi_mwdma_cycle[i])
 				break;
-			}
-		}
+		mwdma_mask = (1 << i) - 1;
 	} else {
 		/* UDMA */
-		for (i = 0; i < 7; i++) {
-			t = gtm->drive[unit].dma;
-			if (t <= ata_acpi_udma_cycle[i]) {
-				mask |= (2 << (ATA_SHIFT_UDMA + i)) - 1;
+		for (i = 0; i < ARRAY_SIZE(ata_acpi_udma_cycle); i++)
+			if (t > ata_acpi_udma_cycle[i])
 				break;
-			}
-		}
+		udma_mask = (1 << i) - 1;
 	}
 
-	return mask;
+	return ata_pack_xfermask(pio_mask, mwdma_mask, udma_mask);
 }
 EXPORT_SYMBOL_GPL(ata_acpi_gtm_xfermask);
 
-- 
1.5.2.4

Re: [PATCH 07/12] libata: fix ata_acpi_gtm_xfermask()

[Alan Cox]

On Tue,  6 Nov 2007 14:39:05 +0900
Tejun Heo <htejun@gmail.com> wrote:

> ata_acpi_gtm_xfermask() as separated out from pacpi_discover_modes()
> has various bugs.  Fix them.
> 
> * The wrong comparison operator is used when finding for matching
>   cycle resulting totally bogus result.
> 
> * With the comparion operator fixed, boundary condtion handling is
>   clumsy.
> 
> * Setting of any DMA mask bit set all bits in PIO mask.
> 
> * MWDMA and UDMA blocks are swapped.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---

Looks good to me

Re: [PATCH 07/12] libata: fix ata_acpi_gtm_xfermask()

[Jeff Garzik]

Tejun Heo wrote:
> ata_acpi_gtm_xfermask() as separated out from pacpi_discover_modes()
> has various bugs.  Fix them.
> 
> * The wrong comparison operator is used when finding for matching
>   cycle resulting totally bogus result.
> 
> * With the comparion operator fixed, boundary condtion handling is
>   clumsy.
> 
> * Setting of any DMA mask bit set all bits in PIO mask.
> 
> * MWDMA and UDMA blocks are swapped.

shouldn't this be combined with patch #6?

Re: [PATCH 07/12] libata: fix ata_acpi_gtm_xfermask()

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> ata_acpi_gtm_xfermask() as separated out from pacpi_discover_modes()
>> has various bugs.  Fix them.
>>
>> * The wrong comparison operator is used when finding for matching
>>   cycle resulting totally bogus result.
>>
>> * With the comparion operator fixed, boundary condtion handling is
>>   clumsy.
>>
>> * Setting of any DMA mask bit set all bits in PIO mask.
>>
>> * MWDMA and UDMA blocks are swapped.
> 
> shouldn't this be combined with patch #6?

I did it that way at first but that made the patch difficult to review
because the content changes (bug fixes) are hidden by moving whole
function from pata_acpi to libata-acpi, so I separated out the fix part
into this patch.

Thanks.

-- 
tejun

[PATCH 08/12] libata: implement ata_timing_cycle2mode() and use it in libata-acpi and pata_acpi

[Tejun Heo]

libata-acpi is using separate timing tables for transfer modes
although libata-core has the complete ata_timing table.  Implement
ata_timing_cycle2mode() to look for matching mode given transfer type
and cycle duration and use it in libata-acpi and pata_acpi to replace
private timing tables.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/ata/libata-acpi.c |   62 +++++++++++----------------------------------
 drivers/ata/libata-core.c |   52 +++++++++++++++++++++++++++++++++++++
 drivers/ata/pata_acpi.c   |   13 +++++----
 include/linux/libata.h    |    5 +---
 4 files changed, 75 insertions(+), 57 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 06d9961..7d982c2 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -424,22 +424,6 @@ int ata_acpi_cbl_80wire(struct ata_port *ap)
 
 EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
 
-/* Welcome to ACPI, bring a bucket */
-const unsigned int ata_acpi_pio_cycle[7] = {
-	600, 383, 240, 180, 120, 100, 80
-};
-EXPORT_SYMBOL_GPL(ata_acpi_pio_cycle);
-
-const unsigned int ata_acpi_mwdma_cycle[5] = {
-	480, 150, 120, 100, 80
-};
-EXPORT_SYMBOL_GPL(ata_acpi_mwdma_cycle);
-
-const unsigned int ata_acpi_udma_cycle[7] = {
-	120, 80, 60, 45, 30, 20, 15
-};
-EXPORT_SYMBOL_GPL(ata_acpi_udma_cycle);
-
 /**
  * ata_acpi_gtm_xfermode - determine xfermode from GTM parameter
  * @dev: target device
@@ -456,49 +440,33 @@ EXPORT_SYMBOL_GPL(ata_acpi_udma_cycle);
 unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
 				   const struct ata_acpi_gtm *gtm)
 {
-	unsigned int pio_mask = 0, mwdma_mask = 0, udma_mask = 0;
-	int unit, i;
-	u32 t;
+	unsigned int xfer_mask = 0;
+	unsigned int type;
+	int unit;
+	u8 mode;
 
 	/* we always use the 0 slot for crap hardware */
 	unit = dev->devno;
 	if (!(gtm->flags & 0x10))
 		unit = 0;
 
-	/* Values larger than the longest cycle results in 0 mask
-	 * while values equal to smaller than the shortest cycle
-	 * results in mask which includes all supported modes.
-	 * Disabled transfer method has the value of 0xffffffff which
-	 * will always result in 0 mask.
-	 */
-
-	/* start by scanning for PIO modes */
-	t = gtm->drive[unit].pio;
-	for (i = 0; i < ARRAY_SIZE(ata_acpi_pio_cycle); i++)
-		if (t > ata_acpi_pio_cycle[i])
-			break;
-	pio_mask = (1 << i) - 1;
+	/* PIO */
+	mode = ata_timing_cycle2mode(ATA_SHIFT_PIO, gtm->drive[unit].pio);
+	xfer_mask |= ata_xfer_mode2mask(mode);
 
 	/* See if we have MWDMA or UDMA data. We don't bother with
 	 * MWDMA if UDMA is available as this means the BIOS set UDMA
 	 * and our error changedown if it works is UDMA to PIO anyway.
 	 */
-	t = gtm->drive[unit].dma;
-	if (!(gtm->flags & (1 << (2 * unit)))) {
-		/* MWDMA */
-		for (i = 0; i < ARRAY_SIZE(ata_acpi_mwdma_cycle); i++)
-			if (t > ata_acpi_mwdma_cycle[i])
-				break;
-		mwdma_mask = (1 << i) - 1;
-	} else {
-		/* UDMA */
-		for (i = 0; i < ARRAY_SIZE(ata_acpi_udma_cycle); i++)
-			if (t > ata_acpi_udma_cycle[i])
-				break;
-		udma_mask = (1 << i) - 1;
-	}
+	if (!(gtm->flags & (1 << (2 * unit))))
+		type = ATA_SHIFT_MWDMA;
+	else
+		type = ATA_SHIFT_UDMA;
+
+	mode = ata_timing_cycle2mode(type, gtm->drive[unit].dma);
+	xfer_mask |= ata_xfer_mode2mask(mode);
 
-	return ata_pack_xfermask(pio_mask, mwdma_mask, udma_mask);
+	return xfer_mask;
 }
 EXPORT_SYMBOL_GPL(ata_acpi_gtm_xfermask);
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 0eae1c6..4372099 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2943,6 +2943,57 @@ int ata_timing_compute(struct ata_device *adev, unsigned short speed,
 }
 
 /**
+ *	ata_timing_cycle2mode - find xfer mode for the specified cycle duration
+ *	@xfer_shift: ATA_SHIFT_* value for transfer type to examine.
+ *	@cycle: cycle duration in ns
+ *
+ *	Return matching xfer mode for @cycle.  The returned mode is of
+ *	the transfer type specified by @xfer_shift.  If @cycle is too
+ *	slow for @xfer_shift, 0xff is returned.  If @cycle is faster
+ *	than the fastest known mode, the fasted mode is returned.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	Matching xfer_mode, 0xff if no match found.
+ */
+u8 ata_timing_cycle2mode(unsigned int xfer_shift, int cycle)
+{
+	u8 base_mode = 0xff, last_mode = 0xff;
+	const struct ata_xfer_ent *ent;
+	const struct ata_timing *t;
+
+	for (ent = ata_xfer_tbl; ent->shift >= 0; ent++)
+		if (ent->shift == xfer_shift)
+			base_mode = ent->base;
+
+	for (t = ata_timing_find_mode(base_mode);
+	     t && ata_xfer_mode2shift(t->mode) == xfer_shift; t++) {
+		unsigned short this_cycle;
+
+		switch (xfer_shift) {
+		case ATA_SHIFT_PIO:
+		case ATA_SHIFT_MWDMA:
+			this_cycle = t->cycle;
+			break;
+		case ATA_SHIFT_UDMA:
+			this_cycle = t->udma;
+			break;
+		default:
+			return 0xff;
+		}
+
+		if (cycle > this_cycle)
+			break;
+
+		last_mode = t->mode;
+	}
+
+	return last_mode;
+}
+
+/**
  *	ata_down_xfermask_limit - adjust dev xfer masks downward
  *	@dev: Device to adjust xfer masks
  *	@sel: ATA_DNXFER_* selector
@@ -7607,6 +7658,7 @@ EXPORT_SYMBOL_GPL(ata_pio_need_iordy);
 EXPORT_SYMBOL_GPL(ata_timing_find_mode);
 EXPORT_SYMBOL_GPL(ata_timing_compute);
 EXPORT_SYMBOL_GPL(ata_timing_merge);
+EXPORT_SYMBOL_GPL(ata_timing_cycle2mode);
 
 #ifdef CONFIG_PCI
 EXPORT_SYMBOL_GPL(pci_test_config_bits);
diff --git a/drivers/ata/pata_acpi.c b/drivers/ata/pata_acpi.c
index fc6e3a5..8059259 100644
--- a/drivers/ata/pata_acpi.c
+++ b/drivers/ata/pata_acpi.c
@@ -133,13 +133,14 @@ static void pacpi_set_piomode(struct ata_port *ap, struct ata_device *adev)
 {
 	int unit = adev->devno;
 	struct pata_acpi *acpi = ap->private_data;
+	const struct ata_timing *t;
 
 	if (!(acpi->gtm.flags & 0x10))
 		unit = 0;
 
 	/* Now stuff the nS values into the structure */
-	acpi->gtm.drive[unit].pio =
-		ata_acpi_pio_cycle[adev->pio_mode - XFER_PIO_0];
+	t = ata_timing_find_mode(adev->pio_mode);
+	acpi->gtm.drive[unit].pio = t->cycle;
 	ata_acpi_stm(ap, &acpi->gtm);
 	/* See what mode we actually got */
 	ata_acpi_gtm(ap, &acpi->gtm);
@@ -155,18 +156,18 @@ static void pacpi_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 {
 	int unit = adev->devno;
 	struct pata_acpi *acpi = ap->private_data;
+	const struct ata_timing *t;
 
 	if (!(acpi->gtm.flags & 0x10))
 		unit = 0;
 
 	/* Now stuff the nS values into the structure */
+	t = ata_timing_find_mode(adev->dma_mode);
 	if (adev->dma_mode >= XFER_UDMA_0) {
-		acpi->gtm.drive[unit].dma =
-			ata_acpi_udma_cycle[adev->dma_mode - XFER_UDMA_0];
+		acpi->gtm.drive[unit].dma = t->udma;
 		acpi->gtm.flags |= (1 << (2 * unit));
 	} else {
-		acpi->gtm.drive[unit].dma =
-			ata_acpi_mwdma_cycle[adev->dma_mode - XFER_MW_DMA_0];
+		acpi->gtm.drive[unit].dma = t->cycle;
 		acpi->gtm.flags &= ~(1 << (2 * unit));
 	}
 	ata_acpi_stm(ap, &acpi->gtm);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index e626137..3b9cebe 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -935,6 +935,7 @@ extern int ata_timing_compute(struct ata_device *, unsigned short,
 extern void ata_timing_merge(const struct ata_timing *,
 			     const struct ata_timing *, struct ata_timing *,
 			     unsigned int);
+extern u8 ata_timing_cycle2mode(unsigned int xfer_shift, int cycle);
 
 enum {
 	ATA_TIMING_SETUP	= (1 << 0),
@@ -955,10 +956,6 @@ enum {
 
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
-extern const unsigned int ata_acpi_pio_cycle[7];
-extern const unsigned int ata_acpi_mwdma_cycle[5];
-extern const unsigned int ata_acpi_udma_cycle[7];
-
 extern int ata_acpi_cbl_80wire(struct ata_port *ap);
 int ata_acpi_stm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
 int ata_acpi_gtm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
-- 
1.5.2.4

Re: [PATCH 08/12] libata: implement ata_timing_cycle2mode() and use it in libata-acpi and pata_acpi

[Alan Cox]

On Tue,  6 Nov 2007 14:39:06 +0900
Tejun Heo <htejun@gmail.com> wrote:

> libata-acpi is using separate timing tables for transfer modes
> although libata-core has the complete ata_timing table.  Implement
> ata_timing_cycle2mode() to look for matching mode given transfer type
> and cycle duration and use it in libata-acpi and pata_acpi to replace
> private timing tables.

Ok that makes sense having now seen the second chunk of patches. Just one
problem:


> +	/* PIO */
> +	mode = ata_timing_cycle2mode(ATA_SHIFT_PIO, gtm->drive[unit].pio);
> +	xfer_mask |= ata_xfer_mode2mask(mode);

No check v 0xFF

> + *	ata_timing_cycle2mode - find xfer mode for the specified cycle duration
> + *	@xfer_shift: ATA_SHIFT_* value for transfer type to examine.
> + *	@cycle: cycle duration in ns
> + *
> + *	Return matching xfer mode for @cycle.  The returned mode is of
> + *	the transfer type specified by @xfer_shift.  If @cycle is too
> + *	slow for @xfer_shift, 0xff is returned.  If @cycle is faster
> + *	than the fastest known mode, the fasted m

Can return 0xFF

Re: [PATCH 08/12] libata: implement ata_timing_cycle2mode() and use it in libata-acpi and pata_acpi

[Tejun Heo]

Alan Cox wrote:
>> +	/* PIO */
>> +	mode = ata_timing_cycle2mode(ATA_SHIFT_PIO, gtm->drive[unit].pio);
>> +	xfer_mask |= ata_xfer_mode2mask(mode);
> 
> No check v 0xFF
> 
>> + *	ata_timing_cycle2mode - find xfer mode for the specified cycle duration
>> + *	@xfer_shift: ATA_SHIFT_* value for transfer type to examine.
>> + *	@cycle: cycle duration in ns
>> + *
>> + *	Return matching xfer mode for @cycle.  The returned mode is of
>> + *	the transfer type specified by @xfer_shift.  If @cycle is too
>> + *	slow for @xfer_shift, 0xff is returned.  If @cycle is faster
>> + *	than the fastest known mode, the fasted m
> 
> Can return 0xFF

The check is implicit in ata_xfer_mode2mask(), 0xff translates to
xfermask 0, vice-versa.  0xff is internally a valid value indicating no
mode.

Thanks.

-- 
tejun

Re: [PATCH 08/12] libata: implement ata_timing_cycle2mode() and use it in libata-acpi and pata_acpi

[Jeff Garzik]

Tejun Heo wrote:
> libata-acpi is using separate timing tables for transfer modes
> although libata-core has the complete ata_timing table.  Implement
> ata_timing_cycle2mode() to look for matching mode given transfer type
> and cycle duration and use it in libata-acpi and pata_acpi to replace
> private timing tables.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
>  drivers/ata/libata-acpi.c |   62 +++++++++++----------------------------------
>  drivers/ata/libata-core.c |   52 +++++++++++++++++++++++++++++++++++++
>  drivers/ata/pata_acpi.c   |   13 +++++----
>  include/linux/libata.h    |    5 +---
>  4 files changed, 75 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 06d9961..7d982c2 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -424,22 +424,6 @@ int ata_acpi_cbl_80wire(struct ata_port *ap)
>  
>  EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
>  
> -/* Welcome to ACPI, bring a bucket */
> -const unsigned int ata_acpi_pio_cycle[7] = {
> -	600, 383, 240, 180, 120, 100, 80
> -};
> -EXPORT_SYMBOL_GPL(ata_acpi_pio_cycle);
> -
> -const unsigned int ata_acpi_mwdma_cycle[5] = {
> -	480, 150, 120, 100, 80
> -};
> -EXPORT_SYMBOL_GPL(ata_acpi_mwdma_cycle);
> -
> -const unsigned int ata_acpi_udma_cycle[7] = {
> -	120, 80, 60, 45, 30, 20, 15
> -};
> -EXPORT_SYMBOL_GPL(ata_acpi_udma_cycle);

ah... they go away. nice.

[PATCH 09/12] libata: implement ata_acpi_init_gtm()

[Tejun Heo]

Add dev->__acpi_init_gtm and store initial GTM values on host
initialization.  If the field is valid, ata_acpi_init_gtm() returns
pointer to the saved GTM structure; otherwise, NULL.  This is to
remember BIOS/firmware programmed initial timing for later use before
reset and mode configuration modify it.  The accessor is there to make
building w/o ACPI easy dev->__acpi_init doesn't exist if ACPI is not
enabled.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-acpi.c |    3 +++
 include/linux/libata.h    |   14 +++++++++++++-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 7d982c2..31365b3 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -94,6 +94,9 @@ static void ata_acpi_associate_ide_port(struct ata_port *ap)
 
 		dev->acpi_handle = acpi_get_child(ap->acpi_handle, i);
 	}
+
+	if (ata_acpi_gtm(ap, &ap->__acpi_init_gtm) == 0)
+		ap->pflags |= ATA_PFLAG_INIT_GTM_VALID;
 }
 
 static void ata_acpi_handle_hotplug(struct ata_port *ap, struct kobject *kobj,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3b9cebe..bad280f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -213,6 +213,7 @@ enum {
 	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
 	ATA_PFLAG_GTM_VALID	= (1 << 19), /* acpi_gtm data valid */
+	ATA_PFLAG_INIT_GTM_VALID = (1 << 20), /* initial gtm data valid */
 
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */
@@ -659,6 +660,7 @@ struct ata_port {
 #ifdef CONFIG_ATA_ACPI
 	acpi_handle		acpi_handle;
 	struct ata_acpi_gtm	acpi_gtm;
+	struct ata_acpi_gtm	__acpi_init_gtm; /* use ata_acpi_init_gtm() */
 #endif
 	u8			sector_buf[ATA_SECT_SIZE]; /* owned by EH */
 };
@@ -956,13 +958,23 @@ enum {
 
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
+static inline const struct ata_acpi_gtm *ata_acpi_init_gtm(struct ata_port *ap)
+{
+	if (ap->pflags & ATA_PFLAG_INIT_GTM_VALID)
+		return &ap->__acpi_init_gtm;
+	return NULL;
+}
 extern int ata_acpi_cbl_80wire(struct ata_port *ap);
 int ata_acpi_stm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
 int ata_acpi_gtm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
 unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
 				   const struct ata_acpi_gtm *gtm);
-
 #else
+static inline const struct ata_acpi_gtm *ata_acpi_init_gtm(struct ata_port *ap)
+{
+	return NULL;
+}
+
 static inline int ata_acpi_cbl_80wire(struct ata_port *ap) { return 0; }
 
 static inline int ata_acpi_stm(const struct ata_port *ap,
-- 
1.5.2.4

[PATCH 10/12] libata: reimplement ata_acpi_cbl_80wire() using ata_acpi_gtm_xfermask()

[Tejun Heo]

Reimplement ata_acpi_cbl_80wire() using ata_acpi_gtm_xfermask() and
while at it relocate the function below ata_acpi_gtm_xfermask().

New ata_acpi_cbl_80wire() implementation takes @gtm, in both pata_via
and pata_amd, use the initial GTM value.  Both are trying to peek
initial BIOS configuration, so using initial caching value makes
sense.  This fixes ACPI part of cable detection in pata_amd which
previously always returned 0 because configuring PIO0 during reset
clears DMA configuration.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/ata/libata-acpi.c |   66 +++++++++++++++++++--------------------------
 drivers/ata/pata_amd.c    |    3 +-
 drivers/ata/pata_via.c    |    3 +-
 include/linux/libata.h    |   10 +++++--
 4 files changed, 39 insertions(+), 43 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 31365b3..281dd2d 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -390,44 +390,6 @@ static int ata_dev_get_GTF(struct ata_device *dev, struct ata_acpi_gtf **gtf,
 }
 
 /**
- * ata_acpi_cbl_80wire		-	Check for 80 wire cable
- * @ap: Port to check
- *
- * Return 1 if the ACPI mode data for this port indicates the BIOS selected
- * an 80wire mode.
- */
-
-int ata_acpi_cbl_80wire(struct ata_port *ap)
-{
-	struct ata_acpi_gtm gtm;
-	int valid = 0;
-
-	/* No _GTM data, no information */
-	if (ata_acpi_gtm(ap, &gtm) < 0)
-		return 0;
-
-	/* Split timing, DMA enabled */
-	if ((gtm.flags & 0x11) == 0x11 && gtm.drive[0].dma < 55)
-		valid |= 1;
-	if ((gtm.flags & 0x14) == 0x14 && gtm.drive[1].dma < 55)
-		valid |= 2;
-	/* Shared timing, DMA enabled */
-	if ((gtm.flags & 0x11) == 0x01 && gtm.drive[0].dma < 55)
-		valid |= 1;
-	if ((gtm.flags & 0x14) == 0x04 && gtm.drive[0].dma < 55)
-		valid |= 2;
-
-	/* Drive check */
-	if ((valid & 1) && ata_dev_enabled(&ap->link.device[0]))
-		return 1;
-	if ((valid & 2) && ata_dev_enabled(&ap->link.device[1]))
-		return 1;
-	return 0;
-}
-
-EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
-
-/**
  * ata_acpi_gtm_xfermode - determine xfermode from GTM parameter
  * @dev: target device
  * @gtm: GTM parameter to use
@@ -474,6 +436,34 @@ unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
 EXPORT_SYMBOL_GPL(ata_acpi_gtm_xfermask);
 
 /**
+ * ata_acpi_cbl_80wire		-	Check for 80 wire cable
+ * @ap: Port to check
+ * @gtm: GTM data to use
+ *
+ * Return 1 if the @gtm indicates the BIOS selected an 80wire mode.
+ */
+int ata_acpi_cbl_80wire(struct ata_port *ap, const struct ata_acpi_gtm *gtm)
+{
+	struct ata_device *dev;
+
+	ata_link_for_each_dev(dev, &ap->link) {
+		unsigned int xfer_mask, udma_mask;
+
+		if (!ata_dev_enabled(dev))
+			continue;
+
+		xfer_mask = ata_acpi_gtm_xfermask(dev, gtm);
+		ata_unpack_xfermask(xfer_mask, NULL, NULL, &udma_mask);
+
+		if (udma_mask & ~ATA_UDMA_MASK_40C)
+			return 1;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
+
+/**
  * taskfile_load_raw - send taskfile registers to host controller
  * @dev: target ATA device
  * @gtf: raw ATA taskfile register set (0x1f1 - 0x1f7)
diff --git a/drivers/ata/pata_amd.c b/drivers/ata/pata_amd.c
index c5779ad..f65fa6c 100644
--- a/drivers/ata/pata_amd.c
+++ b/drivers/ata/pata_amd.c
@@ -271,7 +271,8 @@ static int nv_cable_detect(struct ata_port *ap)
  	if ((udma & 0xC4) == 0xC4 || (udma & 0xC400) == 0xC400)
 		cbl = ATA_CBL_PATA80;
 	/* And a triple check across suspend/resume with ACPI around */
-	if (ata_acpi_cbl_80wire(ap))
+	if (ata_acpi_init_gtm(ap) &&
+	    ata_acpi_cbl_80wire(ap, ata_acpi_init_gtm(ap)))
 		cbl = ATA_CBL_PATA80;
 	return cbl;
 }
diff --git a/drivers/ata/pata_via.c b/drivers/ata/pata_via.c
index a4175fb..3f28ed4 100644
--- a/drivers/ata/pata_via.c
+++ b/drivers/ata/pata_via.c
@@ -185,7 +185,8 @@ static int via_cable_detect(struct ata_port *ap) {
 	if (ata66 & (0x10100000 >> (16 * ap->port_no)))
 		return ATA_CBL_PATA80;
 	/* Check with ACPI so we can spot BIOS reported SATA bridges */
-	if (ata_acpi_cbl_80wire(ap))
+	if (ata_acpi_init_gtm(ap) &&
+	    ata_acpi_cbl_80wire(ap, ata_acpi_init_gtm(ap)))
 		return ATA_CBL_PATA80;
 	return ATA_CBL_PATA40;
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index bad280f..5ad2186 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -964,19 +964,17 @@ static inline const struct ata_acpi_gtm *ata_acpi_init_gtm(struct ata_port *ap)
 		return &ap->__acpi_init_gtm;
 	return NULL;
 }
-extern int ata_acpi_cbl_80wire(struct ata_port *ap);
 int ata_acpi_stm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
 int ata_acpi_gtm(const struct ata_port *ap, struct ata_acpi_gtm *stm);
 unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
 				   const struct ata_acpi_gtm *gtm);
+int ata_acpi_cbl_80wire(struct ata_port *ap, const struct ata_acpi_gtm *gtm);
 #else
 static inline const struct ata_acpi_gtm *ata_acpi_init_gtm(struct ata_port *ap)
 {
 	return NULL;
 }
 
-static inline int ata_acpi_cbl_80wire(struct ata_port *ap) { return 0; }
-
 static inline int ata_acpi_stm(const struct ata_port *ap,
 			       struct ata_acpi_gtm *stm)
 {
@@ -994,6 +992,12 @@ static inline unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
 {
 	return 0;
 }
+
+static inline int ata_acpi_cbl_80wire(struct ata_port *ap,
+				      const struct ata_acpi_gtm *gtm)
+{
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_PCI
-- 
1.5.2.4

[PATCH 11/12] libata: add ATA_CBL_PATA_IGN

[Tejun Heo]

ATA_CBL_PATA_UNK indicates that the cable type can't be determined
from the host side and might be either 80c or 40c.  libata applies
drive or other generic limit in this case.  However, there are
controllers where both host and drive side detections are
misimplemented and the driver has to rely solely on private method -
peeking BIOS or ACPI configuration or using some other private
mechanism.

This patch adds ATA_CBL_PATA_IGN which tells libata to ignore the
cable type completely and just let the LLD determine the transfer mode
via host transfer mode masks and ->mode_filter().

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/ata/libata-core.c |   13 +++++++++++++
 include/linux/ata.h       |    7 ++++---
 include/linux/libata.h    |    1 +
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 4372099..f75a26c 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2331,6 +2331,18 @@ int ata_cable_unknown(struct ata_port *ap)
 }
 
 /**
+ *	ata_cable_ignore	-	return ignored PATA cable.
+ *	@ap: port
+ *
+ *	Helper method for drivers which don't use cable type to limit
+ *	transfer mode.
+ */
+int ata_cable_ignore(struct ata_port *ap)
+{
+	return ATA_CBL_PATA_IGN;
+}
+
+/**
  *	ata_cable_sata	-	return SATA cable type
  *	@ap: port
  *
@@ -7707,4 +7719,5 @@ EXPORT_SYMBOL_GPL(ata_dev_try_classify);
 EXPORT_SYMBOL_GPL(ata_cable_40wire);
 EXPORT_SYMBOL_GPL(ata_cable_80wire);
 EXPORT_SYMBOL_GPL(ata_cable_unknown);
+EXPORT_SYMBOL_GPL(ata_cable_ignore);
 EXPORT_SYMBOL_GPL(ata_cable_sata);
diff --git a/include/linux/ata.h b/include/linux/ata.h
index e1323e2..d942dba 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -271,9 +271,10 @@ enum {
 	ATA_CBL_NONE		= 0,
 	ATA_CBL_PATA40		= 1,
 	ATA_CBL_PATA80		= 2,
-	ATA_CBL_PATA40_SHORT	= 3,		/* 40 wire cable to high UDMA spec */
-	ATA_CBL_PATA_UNK	= 4,
-	ATA_CBL_SATA		= 5,
+	ATA_CBL_PATA40_SHORT	= 3,	/* 40 wire cable to high UDMA spec */
+	ATA_CBL_PATA_UNK	= 4,	/* don't know, maybe 80c? */
+	ATA_CBL_PATA_IGN	= 5,	/* don't know, ignore cable handling */
+	ATA_CBL_SATA		= 6,
 
 	/* SATA Status and Control Registers */
 	SCR_STATUS		= 0,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5ad2186..5145a5c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -924,6 +924,7 @@ extern u8 ata_irq_on(struct ata_port *ap);
 extern int ata_cable_40wire(struct ata_port *ap);
 extern int ata_cable_80wire(struct ata_port *ap);
 extern int ata_cable_sata(struct ata_port *ap);
+extern int ata_cable_ignore(struct ata_port *ap);
 extern int ata_cable_unknown(struct ata_port *ap);
 
 /*
-- 
1.5.2.4

Re: [PATCH 11/12] libata: add ATA_CBL_PATA_IGN

[Alan Cox]

> This patch adds ATA_CBL_PATA_IGN which tells libata to ignore the
> cable type completely and just let the LLD determine the transfer mode
> via host transfer mode masks and ->mode_filter().
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>

Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH 11/12] libata: add ATA_CBL_PATA_IGN

[Tejun Heo]

Alan Cox wrote:
>> This patch adds ATA_CBL_PATA_IGN which tells libata to ignore the
>> cable type completely and just let the LLD determine the transfer mode
>> via host transfer mode masks and ->mode_filter().
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> 
> Acked-by: Alan Cox <alan@redhat.com>
> 

I think this can be applied to pata_acpi too as we don't have any idea
about actual cable there.

Thanks.

-- 
tejun

Re: [PATCH 11/12] libata: add ATA_CBL_PATA_IGN

[Alan Cox]

On Tue, 06 Nov 2007 20:02:28 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Alan Cox wrote:
> >> This patch adds ATA_CBL_PATA_IGN which tells libata to ignore the
> >> cable type completely and just let the LLD determine the transfer mode
> >> via host transfer mode masks and ->mode_filter().
> >>
> >> Signed-off-by: Tejun Heo <htejun@gmail.com>
> >> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> > 
> > Acked-by: Alan Cox <alan@redhat.com>
> > 
> 
> I think this can be applied to pata_acpi too as we don't have any idea
> about actual cable there.

Agreed entirely - and its beginning to look like pata_via is also a
candidate as some boards have real issues.

[PATCH 12/12] pata_amd: update mode selection for NV PATAs

[Tejun Heo]

Cable detection on NV PATA hosts isn't implemented and the CBLID-
cable isn't wired according to the sepc either, so both host-side and
generic drive-side cable detections are broken.  Till now,
nv_cable_detect() relied on peeking BIOS and ACPI configurations to
upgrade to 80C but this often results in misdetection of 40C cable as
80C.  Also, the original implementation was broken in that by the time
BIOS configuration is read it has already been cleared by programming
PIO0 during reset.

This patch reimplements NV mode selection such that...

* BIOS configuration value is stored during driver attach and restored
  on detach.

* Cable type is fixed to ATA_CBL_PATA_IGN and mode selection is soley
  done by nv_mode_filter() which peeks both BIOS and ACPI
  configurations and filter accordingly.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/ata/pata_amd.c |  129 ++++++++++++++++++++++++++++++++++-------------
 1 files changed, 93 insertions(+), 36 deletions(-)

diff --git a/drivers/ata/pata_amd.c b/drivers/ata/pata_amd.c
index f65fa6c..711e8bb 100644
--- a/drivers/ata/pata_amd.c
+++ b/drivers/ata/pata_amd.c
@@ -219,6 +219,62 @@ static void amd133_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 	timing_setup(ap, adev, 0x40, adev->dma_mode, 4);
 }
 
+/* Both host-side and drive-side detection results are worthless on NV
+ * PATAs.  Ignore them and just follow what BIOS configured.  Both the
+ * current configuration in PCI config reg and ACPI GTM result are
+ * cached during driver attach and are consulted to select transfer
+ * mode.
+ */
+static unsigned int nv_mode_filter(struct ata_device *dev,
+				   unsigned int xfer_mask)
+{
+	static const unsigned int udma_mask_map[] =
+		{ ATA_UDMA2, ATA_UDMA1, ATA_UDMA0, 0,
+		  ATA_UDMA3, ATA_UDMA4, ATA_UDMA5, ATA_UDMA6 };
+	struct ata_port *ap = dev->link->ap;
+	char acpi_str[32] = "";
+	u32 saved_udma, udma;
+	const struct ata_acpi_gtm *gtm;
+	unsigned int bios_limit = 0, acpi_limit = 0, limit;
+
+	/* find out what BIOS configured */
+	udma = saved_udma = (unsigned long)ap->host->private_data;
+
+	if (ap->port_no == 0)
+		udma >>= 16;
+	if (dev->devno == 0)
+		udma >>= 8;
+
+	if ((udma & 0xc0) == 0xc0)
+		bios_limit = ata_pack_xfermask(0, 0, udma_mask_map[udma & 0x7]);
+
+	/* consult ACPI GTM too */
+	gtm = ata_acpi_init_gtm(ap);
+	if (gtm) {
+		acpi_limit = ata_acpi_gtm_xfermask(dev, gtm);
+
+		snprintf(acpi_str, sizeof(acpi_str), " (%u:%u:0x%x)",
+			 gtm->drive[0].dma, gtm->drive[1].dma, gtm->flags);
+	}
+
+	/* be optimistic, EH can take care of things if something goes wrong */
+	limit = bios_limit | acpi_limit;
+
+	/* If PIO or DMA isn't configured at all, don't limit.  Let EH
+	 * handle it.
+	 */
+	if (!(limit & ATA_MASK_PIO))
+		limit |= ATA_MASK_PIO;
+	if (!(limit & (ATA_MASK_MWDMA | ATA_MASK_UDMA)))
+		limit |= ATA_MASK_MWDMA | ATA_MASK_UDMA;
+
+	ata_port_printk(ap, KERN_DEBUG, "nv_mode_filter: 0x%x&0x%x->0x%x, "
+			"BIOS=0x%x (0x%x) ACPI=0x%x%s\n",
+			xfer_mask, limit, xfer_mask & limit, bios_limit,
+			saved_udma, acpi_limit, acpi_str);
+
+	return xfer_mask & limit;
+}
 
 /**
  *	nv_probe_init	-	cable detection
@@ -251,32 +307,6 @@ static void nv_error_handler(struct ata_port *ap)
 			       ata_std_postreset);
 }
 
-static int nv_cable_detect(struct ata_port *ap)
-{
-	static const u8 bitmask[2] = {0x03, 0x0C};
-	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
-	u8 ata66;
-	u16 udma;
-	int cbl;
-
-	pci_read_config_byte(pdev, 0x52, &ata66);
-	if (ata66 & bitmask[ap->port_no])
-		cbl = ATA_CBL_PATA80;
-	else
-		cbl = ATA_CBL_PATA40;
-
- 	/* We now have to double check because the Nvidia boxes BIOS
- 	   doesn't always set the cable bits but does set mode bits */
- 	pci_read_config_word(pdev, 0x62 - 2 * ap->port_no, &udma);
- 	if ((udma & 0xC4) == 0xC4 || (udma & 0xC400) == 0xC400)
-		cbl = ATA_CBL_PATA80;
-	/* And a triple check across suspend/resume with ACPI around */
-	if (ata_acpi_init_gtm(ap) &&
-	    ata_acpi_cbl_80wire(ap, ata_acpi_init_gtm(ap)))
-		cbl = ATA_CBL_PATA80;
-	return cbl;
-}
-
 /**
  *	nv100_set_piomode	-	set initial PIO mode data
  *	@ap: ATA interface
@@ -314,6 +344,14 @@ static void nv133_set_dmamode(struct ata_port *ap, struct ata_device *adev)
 	timing_setup(ap, adev, 0x50, adev->dma_mode, 4);
 }
 
+static void nv_host_stop(struct ata_host *host)
+{
+	u32 udma = (unsigned long)host->private_data;
+
+	/* restore PCI config register 0x60 */
+	pci_write_config_dword(to_pci_dev(host->dev), 0x60, udma);
+}
+
 static struct scsi_host_template amd_sht = {
 	.module			= THIS_MODULE,
 	.name			= DRV_NAME,
@@ -478,7 +516,8 @@ static struct ata_port_operations nv100_port_ops = {
 	.thaw		= ata_bmdma_thaw,
 	.error_handler	= nv_error_handler,
 	.post_internal_cmd = ata_bmdma_post_internal_cmd,
-	.cable_detect	= nv_cable_detect,
+	.cable_detect	= ata_cable_ignore,
+	.mode_filter	= nv_mode_filter,
 
 	.bmdma_setup 	= ata_bmdma_setup,
 	.bmdma_start 	= ata_bmdma_start,
@@ -495,6 +534,7 @@ static struct ata_port_operations nv100_port_ops = {
 	.irq_on		= ata_irq_on,
 
 	.port_start	= ata_sff_port_start,
+	.host_stop	= nv_host_stop,
 };
 
 static struct ata_port_operations nv133_port_ops = {
@@ -511,7 +551,8 @@ static struct ata_port_operations nv133_port_ops = {
 	.thaw		= ata_bmdma_thaw,
 	.error_handler	= nv_error_handler,
 	.post_internal_cmd = ata_bmdma_post_internal_cmd,
-	.cable_detect	= nv_cable_detect,
+	.cable_detect	= ata_cable_ignore,
+	.mode_filter	= nv_mode_filter,
 
 	.bmdma_setup 	= ata_bmdma_setup,
 	.bmdma_start 	= ata_bmdma_start,
@@ -528,6 +569,7 @@ static struct ata_port_operations nv133_port_ops = {
 	.irq_on		= ata_irq_on,
 
 	.port_start	= ata_sff_port_start,
+	.host_stop	= nv_host_stop,
 };
 
 static int amd_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
@@ -614,7 +656,8 @@ static int amd_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 			.port_ops = &amd100_port_ops
 		}
 	};
-	const struct ata_port_info *ppi[] = { NULL, NULL };
+	struct ata_port_info pi;
+	const struct ata_port_info *ppi[] = { &pi, NULL };
 	static int printed_version;
 	int type = id->driver_data;
 	u8 fifo;
@@ -628,6 +671,19 @@ static int amd_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (type == 1 && pdev->revision > 0x7)
 		type = 2;
 
+	/* Serenade ? */
+	if (type == 5 && pdev->subsystem_vendor == PCI_VENDOR_ID_AMD &&
+			 pdev->subsystem_device == PCI_DEVICE_ID_AMD_SERENADE)
+		type = 6;	/* UDMA 100 only */
+
+	/*
+	 * Okay, type is determined now.  Apply type-specific workarounds.
+	 */
+	pi = info[type];
+
+	if (type < 3)
+		ata_pci_clear_simplex(pdev);
+
 	/* Check for AMD7411 */
 	if (type == 3)
 		/* FIFO is broken */
@@ -635,16 +691,17 @@ static int amd_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	else
 		pci_write_config_byte(pdev, 0x41, fifo | 0xF0);
 
-	/* Serenade ? */
-	if (type == 5 && pdev->subsystem_vendor == PCI_VENDOR_ID_AMD &&
-			 pdev->subsystem_device == PCI_DEVICE_ID_AMD_SERENADE)
-		type = 6;	/* UDMA 100 only */
+	/* Cable detection on Nvidia chips doesn't work too well,
+	 * cache BIOS programmed UDMA mode.
+	 */
+	if (type == 7 || type == 8) {
+		u32 udma;
 
-	if (type < 3)
-		ata_pci_clear_simplex(pdev);
+		pci_read_config_dword(pdev, 0x60, &udma);
+		pi.private_data = (void *)(unsigned long)udma;
+	}
 
 	/* And fire it up */
-	ppi[0] = &info[type];
 	return ata_pci_init_one(pdev, ppi);
 }
 
-- 
1.5.2.4

Re: [PATCH 12/12] pata_amd: update mode selection for NV PATAs

[Alan Cox]

Looks good to me

Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCHSET] libata: update timing and fix pata_amd transfer mode selection

[Tejun Heo]

Tejun Heo wrote:
> Hello,
> 
> This patchset cleans up and improves PATA timing related code and fix
> pata_amd transfer mode selection on top of the improvements.  This
> patchset contains the following tweleve patches.
> 
>  0001-ata_generic-unindent-loop-in-generic_set_mode.patch
>  0002-libata-export-xfermode-PATA-timing-related-functi.patch
>  0003-libata-clean-up-xfermode-PATA-timing-related-stuf.patch
>  0004-libata-kill-ata_id_to_dma_mode.patch
>  0005-libata-xfer_mask-is-unsigned-int-not-unsigned-long.patch
>  0006-libata-separate-out-ata_acpi_gtm_xfermask-from-pa.patch
>  0007-libata-fix-ata_acpi_gtm_xfermask.patch
>  0008-libata-implement-ata_timing_cycle2mode-and-use-it.patch
>  0009-libata-implement-ata_acpi_init_gtm.patch
>  0010-libata-reimplement-ata_acpi_cbl_80wire-using-ata_.patch
>  0011-libata-add-ATA_CBL_PATA_IGN.patch
>  0012-pata_amd-update-mode-selection-for-NV-PATAs.patch
> 
> 0001-0005 are clean ups of timing handling code.  0006-0008 unifies
> timing handling code in libata-acpi.c and pata_acpi.c with the core
> timing code.
> 
> 0009 implements initial GTM caching.  I thought about moving this to
> LLDs but it's too much hassle.  GTM is host wide and doing it from
> ->error_handler would require cross-port synchronization.  Left
> alternative is adding a separate hook.  Simply doing it during ACPI
> attach is simpler.
> 
> 0010 reimplements ata_acpi_cbl_80wire() using ata_acpi_gtm_xfermask().
> This new function takes @gtm argument.  Both users (pata_via and
> pata_amd) are converted to pass initial GTM.
> 
> 0011 implements ATA_CBL_PATA_IGN which tells libata to ignore cable
> type and supporess all related handling.
> 
> 0012 updates mode selection for NV PATA controllers.  We just can't
> tell what cable is attached on the controller either from host or
> drive side.  So, for those controllers, pata_amd just sets cable type
> to ATA_CBL_PATA_IGN and use ->mode_filter to limit transfer mode
> according to BIOS/ACPI boot configuration.  If that fails (not
> likely), pata_amd simply sets the highest allowed speed and let EH
> figure out the mess.  With recent updates, EH should be able to figure
> it out pretty soon.
> 
> This patchset is against
> 
>    #upstream-fixes (6bbfd53d47abd1fb20d7c93a9b19a75970b66f49)
>  + update-speeddown patchset [1]
> 
>  Thanks.
> 
>  --
>  tejun

Ping.

-- 
tejun

Re: [PATCHSET] libata: update timing and fix pata_amd transfer mode selection

[Jeff Garzik]

Tejun Heo wrote:
> Hello,
> 
> This patchset cleans up and improves PATA timing related code and fix
> pata_amd transfer mode selection on top of the improvements.  This
> patchset contains the following tweleve patches.
> 
>  0001-ata_generic-unindent-loop-in-generic_set_mode.patch
>  0002-libata-export-xfermode-PATA-timing-related-functi.patch
>  0003-libata-clean-up-xfermode-PATA-timing-related-stuf.patch
>  0004-libata-kill-ata_id_to_dma_mode.patch
>  0005-libata-xfer_mask-is-unsigned-int-not-unsigned-long.patch
>  0006-libata-separate-out-ata_acpi_gtm_xfermask-from-pa.patch
>  0007-libata-fix-ata_acpi_gtm_xfermask.patch
>  0008-libata-implement-ata_timing_cycle2mode-and-use-it.patch
>  0009-libata-implement-ata_acpi_init_gtm.patch
>  0010-libata-reimplement-ata_acpi_cbl_80wire-using-ata_.patch
>  0011-libata-add-ATA_CBL_PATA_IGN.patch
>  0012-pata_amd-update-mode-selection-for-NV-PATAs.patch
> 
> 0001-0005 are clean ups of timing handling code.  0006-0008 unifies
> timing handling code in libata-acpi.c and pata_acpi.c with the core
> timing code.
> 
> 0009 implements initial GTM caching.  I thought about moving this to
> LLDs but it's too much hassle.  GTM is host wide and doing it from
> ->error_handler would require cross-port synchronization.  Left
> alternative is adding a separate hook.  Simply doing it during ACPI
> attach is simpler.
> 
> 0010 reimplements ata_acpi_cbl_80wire() using ata_acpi_gtm_xfermask().
> This new function takes @gtm argument.  Both users (pata_via and
> pata_amd) are converted to pass initial GTM.
> 
> 0011 implements ATA_CBL_PATA_IGN which tells libata to ignore cable
> type and supporess all related handling.
> 
> 0012 updates mode selection for NV PATA controllers.  We just can't
> tell what cable is attached on the controller either from host or
> drive side.  So, for those controllers, pata_amd just sets cable type
> to ATA_CBL_PATA_IGN and use ->mode_filter to limit transfer mode
> according to BIOS/ACPI boot configuration.  If that fails (not
> likely), pata_amd simply sets the highest allowed speed and let EH
> figure out the mess.  With recent updates, EH should be able to figure
> it out pretty soon.

overall this seems pretty sane...  but I lean towards applying it in 
2.6.25 rather than 2.6.24, since we are fairly deep into 2.6.24-rc at 
this point, with little time to test these and the speed down changes to 
ensure no last-minute breakage.

[PATCHSET] libata: improve timing code and fix pata_amd transfer mode selection, take #2

[Tejun Heo]

Hello,

This is the second take of improve-timing-code-and-fix-pata_amd
patchset.

Changes from the last take [L] are...

* 0005-libata-xfer_mask-is-unsigned-int-not-unsigned-long.patch is
  reversed such that xfer_mask is consistently unsigned long.

This patchset is against

   #upstream (51a7ee37eaa85b8c35fe6090618e34ed4ce1d316)
 + improve-speed-down patchset, take #3  [1]

This patchset is also available as the following git tree.

master.kernel.org:/pub/scm/linux/kernel/git/tj/libata-dev.git improve-timing-code-and-fix-pata_amd

Thanks.

--
tejun

[L] http://thread.gmane.org/gmane.linux.ide/24524
[1] http://thread.gmane.org/gmane.linux.ide/25406

[PATCH 04/12] libata: kill ata_id_to_dma_mode()

[Tejun Heo]

ata_id_to_dma_mode() isn't quite generic.  The function is basically
privately implemented ata_id_xfermask() combined with hardcoded mode
printing and configuration which are specific to ata_generic.

Kill the function and open code it in generic_set_mode() using generic
xfermode handling functions.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/ata_generic.c |   17 ++++++++++++++++-
 drivers/ata/libata-core.c |   43 -------------------------------------------
 include/linux/libata.h    |    1 -
 3 files changed, 16 insertions(+), 45 deletions(-)

diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index fb5434c..d11a4f1 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -63,7 +63,22 @@ static int generic_set_mode(struct ata_link *link, struct ata_device **unused)
 		/* We do need the right mode information for DMA or PIO
 		   and this comes from the current configuration flags */
 		if (dma_enabled & (1 << (5 + dev->devno))) {
-			ata_id_to_dma_mode(dev, XFER_MW_DMA_0);
+			unsigned int xfer_mask = ata_id_xfermask(dev->id);
+			const char *name;
+
+			if (xfer_mask & (ATA_MASK_MWDMA | ATA_MASK_UDMA))
+				name = ata_mode_string(xfer_mask);
+			else {
+				/* SWDMA perhaps? */
+				name = "DMA";
+				xfer_mask |= ata_xfer_mode2mask(XFER_MW_DMA_0);
+			}
+
+			ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
+				       name);
+
+			dev->xfer_mode = ata_xfer_mask2mode(xfer_mask);
+			dev->xfer_shift = ata_xfer_mode2shift(dev->xfer_mode);
 			dev->flags &= ~ATA_DFLAG_PIO;
 		} else {
 			ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 4f06ba7..88ed491 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1286,48 +1286,6 @@ static int ata_hpa_resize(struct ata_device *dev)
 }
 
 /**
- *	ata_id_to_dma_mode	-	Identify DMA mode from id block
- *	@dev: device to identify
- *	@unknown: mode to assume if we cannot tell
- *
- *	Set up the timing values for the device based upon the identify
- *	reported values for the DMA mode. This function is used by drivers
- *	which rely upon firmware configured modes, but wish to report the
- *	mode correctly when possible.
- *
- *	In addition we emit similarly formatted messages to the default
- *	ata_dev_set_mode handler, in order to provide consistency of
- *	presentation.
- */
-
-void ata_id_to_dma_mode(struct ata_device *dev, u8 unknown)
-{
-	unsigned int mask;
-	u8 mode;
-
-	/* Pack the DMA modes */
-	mask = ((dev->id[63] >> 8) << ATA_SHIFT_MWDMA) & ATA_MASK_MWDMA;
-	if (dev->id[53] & 0x04)
-		mask |= ((dev->id[88] >> 8) << ATA_SHIFT_UDMA) & ATA_MASK_UDMA;
-
-	/* Select the mode in use */
-	mode = ata_xfer_mask2mode(mask);
-
-	if (mode != 0xff) {
-		ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
-		       ata_mode_string(mask));
-	} else {
-		/* SWDMA perhaps ? */
-		mode = unknown;
-		ata_dev_printk(dev, KERN_INFO, "configured for DMA\n");
-	}
-
-	/* Configure the device reporting */
-	dev->xfer_mode = mode;
-	dev->xfer_shift = ata_xfer_mode2shift(mode);
-}
-
-/**
  *	ata_noop_dev_select - Select device 0/1 on ATA bus
  *	@ap: ATA channel to manipulate
  *	@device: ATA device (numbered from zero) to select
@@ -7620,7 +7578,6 @@ EXPORT_SYMBOL_GPL(ata_host_resume);
 #endif /* CONFIG_PM */
 EXPORT_SYMBOL_GPL(ata_id_string);
 EXPORT_SYMBOL_GPL(ata_id_c_string);
-EXPORT_SYMBOL_GPL(ata_id_to_dma_mode);
 EXPORT_SYMBOL_GPL(ata_scsi_simulate);
 
 EXPORT_SYMBOL_GPL(ata_pio_need_iordy);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2a8c925..006183e 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -887,7 +887,6 @@ extern void ata_id_string(const u16 *id, unsigned char *s,
 			  unsigned int ofs, unsigned int len);
 extern void ata_id_c_string(const u16 *id, unsigned char *s,
 			    unsigned int ofs, unsigned int len);
-extern void ata_id_to_dma_mode(struct ata_device *dev, u8 unknown);
 extern void ata_bmdma_setup(struct ata_queued_cmd *qc);
 extern void ata_bmdma_start(struct ata_queued_cmd *qc);
 extern void ata_bmdma_stop(struct ata_queued_cmd *qc);
-- 
1.5.2.4

Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()

[Alan Cox]

On Tue, 27 Nov 2007 19:43:41 +0900
Tejun Heo <htejun@gmail.com> wrote:

> ata_id_to_dma_mode() isn't quite generic.  The function is basically
> privately implemented ata_id_xfermask() combined with hardcoded mode
> printing and configuration which are specific to ata_generic.
> 
> Kill the function and open code it in generic_set_mode() using generic
> xfermode handling functions.

That one I still think is a bad idea. This sort of internal knowledge
belongs in library routines. It is a pretty generic function for the case
where the device is using existing modes and wants to display them.

Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()

[Tejun Heo]

Alan Cox wrote:
> On Tue, 27 Nov 2007 19:43:41 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> ata_id_to_dma_mode() isn't quite generic.  The function is basically
>> privately implemented ata_id_xfermask() combined with hardcoded mode
>> printing and configuration which are specific to ata_generic.
>>
>> Kill the function and open code it in generic_set_mode() using generic
>> xfermode handling functions.
> 
> That one I still think is a bad idea. This sort of internal knowledge
> belongs in library routines. It is a pretty generic function for the case
> where the device is using existing modes and wants to display them.

The thing that bothers me is that the code assumes SWDMA and reports
"DMA" and sets xfer_mask for MWDMA0.  This is specific to ata_generic
and with that part removed, what can be factored out is...

	ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
		       ata_mode_string(xfer_mask));
	dev->xfer_mode = ata_xfer_mask2mode(xfer_mask);
	dev->xfer_shift = ata_xfer_mode2shift(dev->xfer_mode);

Unfortunately, ata_generic won't be able to use such generic helper
because it uses unspecified DMA and PIO modes which generic helper
wouldn't support.

Unless we are gonna have other drivers which would blindly trust device
setting regarding PIO/DMA modes, I don't think ata_id_to_dma_mode()
would be useful, and if we are going to have more such drivers, we at
least need to rename ata_id_to_dma_mode() to note that the function
blindly trusts device reported configuration.

Thanks.

-- 
tejun

Re: [PATCH 04/12] libata: kill ata_id_to_dma_mode()

[Jeff Garzik]

Tejun Heo wrote:
> ata_id_to_dma_mode() isn't quite generic.  The function is basically
> privately implemented ata_id_xfermask() combined with hardcoded mode
> printing and configuration which are specific to ata_generic.
> 
> Kill the function and open code it in generic_set_mode() using generic
> xfermode handling functions.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

[responding to Alan's objection, and subsequent thread]

Yeah, it's a matter of taste.  I applied, figuring "in for a dime in for 
a dozen" :)

I'm not thrilled with all these fine-grained exports of timing tables 
and mode mask functions.

I applied the patches, but I would really like to hide more of these 
details away from LLD [if possible, the obvious disclaimed...]

	Jeff