Skip to contentSkip to conversation

ide: remove ide_find_best_pio_mode()

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 3 messages Jun 30, 2007 – Jul 3, 2007 libata

lore
Share & tools
lore
#0 lore raw

PATCH 8/15 ide: remove ide_find_best_pio_mode()

* Add ->host_flags to ide_hwif_t to store ide_pci_device_t.host_flags, assign it in setup-pci.c:ide_pci_setup_ports().

* Add IDE_HFLAG_PIO_NO_{BLACKLIST,DOWNGRADE} to ide_pci_device_t.host_flags and teach ide_get_best_pio_mode() about them. Also remove needless !drive->id check while at it (drive->id is always present).

* Convert amd74xx, via82cxxx and ide-timing.h to use ide_get_best_pio_mode() and then remove no longer needed ide_find_best_pio_mode().

There should be no functionality changes caused by this patch.

---
 drivers/ide/ide-lib.c       |   13 +++++++------
 drivers/ide/ide-timing.h    |   18 ++----------------
 drivers/ide/ide.c           |    2 ++
 drivers/ide/pci/amd74xx.c   |   20 ++++++++++++--------
 drivers/ide/pci/via82cxxx.c |   22 +++++++++++++---------
 drivers/ide/setup-pci.c     |    2 ++
 include/linux/ide.h         |    7 +++++++
 7 files changed, 45 insertions(+), 39 deletions(-)
drivers/ide/ide-lib.c +7 -6
Index: b/drivers/ide/ide-lib.c
===================================================================
@@ -291,11 +291,11 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 	struct hd_driveid* id = drive->id;
 	int overridden  = 0;
 
-	if (mode_wanted != 255) {
-		pio_mode = mode_wanted;
-	} else if (!drive->id) {
-		pio_mode = 0;
-	} else if ((pio_mode = ide_scan_pio_blacklist(id->model)) != -1) {
+	if (mode_wanted != 255)
+		return min_t(u8, mode_wanted, max_mode);
+
+	if ((drive->hwif->host_flags & IDE_HFLAG_PIO_NO_BLACKLIST) == 0 &&
+	    (pio_mode = ide_scan_pio_blacklist(id->model)) != -1) {
 		printk(KERN_INFO "%s: is on PIO blacklist\n", drive->name);
 	} else {
 		pio_mode = id->tPIO;
@@ -324,7 +324,8 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 		/*
 		 * Conservative "downgrade" for all pre-ATA2 drives
 		 */
-		if (pio_mode && pio_mode < 4) {
+		if ((drive->hwif->host_flags & IDE_HFLAG_PIO_NO_DOWNGRADE) == 0 &&
+		    pio_mode && pio_mode < 4) {
 			pio_mode--;
 			printk(KERN_INFO "%s: applying conservative "
 					 "PIO \"downgrade\"\n", drive->name);
drivers/ide/ide-timing.h +2 -16
Index: b/drivers/ide/ide-timing.h
===================================================================
@@ -106,21 +106,6 @@ static struct ide_timing ide_timing[] = 
 #define XFER_EPIO	0x01
 #define XFER_PIO	0x00
 
-static short ide_find_best_pio_mode(ide_drive_t *drive)
-{
-	struct hd_driveid *id = drive->id;
-	short best = 0;
-
-	/* EIDE PIO modes */
-	if ((id->field_valid & 2) && (id->capability & 8)) {
-		if ((best = (drive->id->eide_pio_modes & 4) ? XFER_PIO_5 :
-			    (drive->id->eide_pio_modes & 2) ? XFER_PIO_4 :
-			    (drive->id->eide_pio_modes & 1) ? XFER_PIO_3 : 0)) return best;
-	}
-
-	return min_t(u8, id->tPIO, 2);
-}
-
 static void ide_timing_quantize(struct ide_timing *t, struct ide_timing *q, int T, int UT)
 {
 	q->setup   = EZ(t->setup   * 1000,  T);
@@ -210,7 +195,8 @@ static int ide_timing_compute(ide_drive_
  */
 
 	if ((speed & XFER_MODE) != XFER_PIO) {
-		ide_timing_compute(drive, ide_find_best_pio_mode(drive), &p, T, UT);
+		u8 pio = ide_get_best_pio_mode(drive, 255, 5);
+		ide_timing_compute(drive, XFER_PIO_0 + pio, &p, T, UT);
 		ide_timing_merge(&p, t, t, IDE_TIMING_ALL);
 	}
 
drivers/ide/ide.c +2 -0
Index: b/drivers/ide/ide.c
===================================================================
@@ -455,6 +455,8 @@ static void ide_hwif_restore(ide_hwif_t 
 	hwif->straight8			= tmp_hwif->straight8;
 	hwif->bus_state			= tmp_hwif->bus_state;
 
+	hwif->host_flags		= tmp_hwif->host_flags;
+
 	hwif->atapi_dma			= tmp_hwif->atapi_dma;
 	hwif->ultra_mask		= tmp_hwif->ultra_mask;
 	hwif->mwdma_mask		= tmp_hwif->mwdma_mask;
drivers/ide/pci/amd74xx.c +12 -8
Index: b/drivers/ide/pci/amd74xx.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * Version 2.20
+ * Version 2.21
  *
  * AMD 755/756/766/8111 and nVidia nForce/2/2s/3/3s/CK804/MCP04
  * IDE driver for Linux.
@@ -275,10 +275,8 @@ static int amd_set_drive(ide_drive_t *dr
 
 static void amd74xx_tune_drive(ide_drive_t *drive, u8 pio)
 {
-	if (pio == 255) {
-		amd_set_drive(drive, ide_find_best_pio_mode(drive));
-		return;
-	}
+	if (pio == 255)
+		pio = ide_get_best_pio_mode(drive, 255, 5);
 
 	amd_set_drive(drive, XFER_PIO_0 + min_t(byte, pio, 5));
 }
@@ -287,12 +285,14 @@ static int amd74xx_ide_dma_check(ide_dri
 {
 	u8 speed = ide_max_dma_mode(drive);
 
-	if (speed == 0)
-		speed = ide_find_best_pio_mode(drive);
+	if (speed == 0) {
+		amd74xx_tune_drive(drive, 255);
+		return -1;
+	}
 
 	amd_set_drive(drive, speed);
 
-	if (drive->autodma && (speed & XFER_MODE) != XFER_PIO)
+	if (drive->autodma)
 		return 0;
 
 	return -1;
@@ -455,6 +455,8 @@ static void __devinit init_hwif_amd74xx(
 		.autodma	= AUTODMA,				\
 		.enablebits	= {{0x40,0x02,0x02}, {0x40,0x01,0x01}},	\
 		.bootable	= ON_BOARD,				\
+		.host_flags	= IDE_HFLAG_PIO_NO_BLACKLIST		\
+				| IDE_HFLAG_PIO_NO_DOWNGRADE,		\
 	}
 
 #define DECLARE_NV_DEV(name_str)					\
@@ -465,6 +467,8 @@ static void __devinit init_hwif_amd74xx(
 		.autodma	= AUTODMA,				\
 		.enablebits	= {{0x50,0x02,0x02}, {0x50,0x01,0x01}},	\
 		.bootable	= ON_BOARD,				\
+		.host_flags	= IDE_HFLAG_PIO_NO_BLACKLIST		\
+				| IDE_HFLAG_PIO_NO_DOWNGRADE,		\
 	}
 
 static ide_pci_device_t amd74xx_chipsets[] __devinitdata = {
drivers/ide/pci/via82cxxx.c +13 -9
Index: b/drivers/ide/pci/via82cxxx.c
===================================================================
@@ -1,6 +1,6 @@
 /*
  *
- * Version 3.45
+ * Version 3.46
  *
  * VIA IDE driver for Linux. Supported southbridges:
  *
@@ -203,10 +203,8 @@ static int via_set_drive(ide_drive_t *dr
 
 static void via82cxxx_tune_drive(ide_drive_t *drive, u8 pio)
 {
-	if (pio == 255) {
-		via_set_drive(drive, ide_find_best_pio_mode(drive));
-		return;
-	}
+	if (pio == 255)
+		pio = ide_get_best_pio_mode(drive, 255, 5);
 
 	via_set_drive(drive, XFER_PIO_0 + min_t(u8, pio, 5));
 }
@@ -223,12 +221,14 @@ static int via82cxxx_ide_dma_check (ide_
 {
 	u8 speed = ide_max_dma_mode(drive);
 
-	if (speed == 0)
-		speed = ide_find_best_pio_mode(drive);
+	if (speed == 0) {
+		via82cxxx_tune_drive(drive, 255);
+		return -1;
+	}
 
 	via_set_drive(drive, speed);
 
-	if (drive->autodma && (speed & XFER_MODE) != XFER_PIO)
+	if (drive->autodma)
 		return 0;
 
 	return -1;
@@ -503,7 +503,9 @@ static ide_pci_device_t via82cxxx_chipse
 		.init_hwif	= init_hwif_via82cxxx,
 		.autodma	= NOAUTODMA,
 		.enablebits	= {{0x40,0x02,0x02}, {0x40,0x01,0x01}},
-		.bootable	= ON_BOARD
+		.bootable	= ON_BOARD,
+		.host_flags	= IDE_HFLAG_PIO_NO_BLACKLIST
+				| IDE_HFLAG_PIO_NO_DOWNGRADE,
 	},{	/* 1 */
 		.name		= "VP_IDE",
 		.init_chipset	= init_chipset_via82cxxx,
@@ -511,6 +513,8 @@ static ide_pci_device_t via82cxxx_chipse
 		.autodma	= AUTODMA,
 		.enablebits	= {{0x00,0x00,0x00}, {0x00,0x00,0x00}},
 		.bootable	= ON_BOARD,
+		.host_flags	= IDE_HFLAG_PIO_NO_BLACKLIST
+				| IDE_HFLAG_PIO_NO_DOWNGRADE,
 	}
 };
 
drivers/ide/setup-pci.c +2 -0
Index: b/drivers/ide/setup-pci.c
===================================================================
@@ -613,6 +613,8 @@ void ide_pci_setup_ports(struct pci_dev 
 		else
 			ide_hwif_setup_dma(dev, d, hwif);
 bypass_legacy_dma:
+		hwif->host_flags = d->host_flags;
+
 		if (d->init_hwif)
 			/* Call chipset-specific routine
 			 * for each enabled hwif
include/linux/ide.h +7 -0
Index: b/include/linux/ide.h
===================================================================
@@ -681,6 +681,8 @@ typedef struct hwif_s {
 	u8 straight8;	/* Alan's straight 8 check */
 	u8 bus_state;	/* power state of the IDE bus */
 
+	u8 host_flags;
+
 	u8 atapi_dma;	/* host supports atapi_dma */
 	u8 ultra_mask;
 	u8 mwdma_mask;
@@ -1245,7 +1247,12 @@ typedef struct ide_pci_enablebit_s {
 enum {
 	/* Uses ISA control ports not PCI ones. */
 	IDE_HFLAG_ISA_PORTS		= (1 << 0),
+	/* single port device */
 	IDE_HFLAG_SINGLE		= (1 << 1),
+	/* don't use legacy PIO blacklist */
+	IDE_HFLAG_PIO_NO_BLACKLIST	= (1 << 2),
+	/* don't use conservative PIO "downgrade" */
+	IDE_HFLAG_PIO_NO_DOWNGRADE	= (1 << 3),
 };
 
 typedef struct ide_pci_device_s {
Acked-by on v1 8/15
#1 lore raw
Quoted 7 lines · Bartlomiej Zolnierkiewicz

* Add ->host_flags to ide_hwif_t to store ide_pci_device_t.host_flags, assign it in setup-pci.c:ide_pci_setup_ports().

* Add IDE_HFLAG_PIO_NO_{BLACKLIST,DOWNGRADE} to ide_pci_device_t.host_flags and teach ide_get_best_pio_mode() about them. Also remove needless !drive->id check while at it (drive->id is always present).

Quoted 6 lines

* Convert amd74xx, via82cxxx and ide-timing.h to use ide_get_best_pio_mode() and then remove no longer needed ide_find_best_pio_mode().

There should be no functionality changes caused by this patch.

Still I have something to ask...

Quoted 7 lines

Index: b/drivers/ide/ide-lib.c
===================================================================

drivers/ide/ide-lib.c +0 -0
@@ -291,11 +291,11 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 	struct hd_driveid* id = drive->id;
 	int overridden  = 0;

drivers/ide/ide-lib.c u8 ide_get_best_pio_mode (ide_drive_t *d · line 297

Quoted 10 lines
drivers/ide/ide-lib.c +4 -5
 
-	if (mode_wanted != 255) {
-		pio_mode = mode_wanted;
-	} else if (!drive->id) {
-		pio_mode = 0;
-	} else if ((pio_mode = ide_scan_pio_blacklist(id->model)) != -1) {
+	if (mode_wanted != 255)
+		return min_t(u8, mode_wanted, max_mode);
+
+	if ((drive->hwif->host_flags & IDE_HFLAG_PIO_NO_BLACKLIST) == 0 &&

C also has ! operator. ;-)

drivers/ide/ide-lib.c u8 ide_get_best_pio_mode (ide_drive_t *d · line 327

Quoted 10 lines
drivers/ide/ide-lib.c +2 -1
+	    (pio_mode = ide_scan_pio_blacklist(id->model)) != -1) {
 		printk(KERN_INFO "%s: is on PIO blacklist\n", drive->name);
 	} else {
 		pio_mode = id->tPIO;
@@ -324,7 +324,8 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 		/*
 		 * Conservative "downgrade" for all pre-ATA2 drives
 		 */
-		if (pio_mode && pio_mode < 4) {
+		if ((drive->hwif->host_flags & IDE_HFLAG_PIO_NO_DOWNGRADE) == 0 &&

It really does! :-)

Quoted 18 lines
drivers/ide/ide-lib.c +1 -0
+		    pio_mode && pio_mode < 4) {
 			pio_mode--;
 			printk(KERN_INFO "%s: applying conservative "
 					 "PIO \"downgrade\"\n", drive->name);

Index: b/drivers/ide/pci/amd74xx.c
===================================================================

drivers/ide/pci/amd74xx.c +1 -1
@@ -1,5 +1,5 @@
 /*
- * Version 2.20
+ * Version 2.21
  *
  * AMD 755/756/766/8111 and nVidia nForce/2/2s/3/3s/CK804/MCP04
  * IDE driver for Linux.
@@ -275,10 +275,8 @@ static int amd_set_drive(ide_drive_t *dr
 
 static void amd74xx_tune_drive(ide_drive_t *drive, u8 pio)

drivers/ide/pci/amd74xx.c static int amd_set_drive(ide_drive_t *dr · line 282

Quoted 10 lines
drivers/ide/pci/amd74xx.c +2 -4
 {
-	if (pio == 255) {
-		amd_set_drive(drive, ide_find_best_pio_mode(drive));
-		return;
-	}
+	if (pio == 255)
+		pio = ide_get_best_pio_mode(drive, 255, 5);
 
 	amd_set_drive(drive, XFER_PIO_0 + min_t(byte, pio, 5));
 }

Erm, I don't get it -- why not just use ide_get_best_pio_mode() for every
'pio'? There should be no functionality changes, no?

Quoted 14 lines

Index: b/drivers/ide/pci/via82cxxx.c
===================================================================

drivers/ide/pci/via82cxxx.c +1 -1
@@ -1,6 +1,6 @@
 /*
  *
- * Version 3.45
+ * Version 3.46
  *
  * VIA IDE driver for Linux. Supported southbridges:
  *
@@ -203,10 +203,8 @@ static int via_set_drive(ide_drive_t *dr
 

drivers/ide/pci/via82cxxx.c static int via_set_drive(ide_drive_t *dr · line 209

Quoted 10 lines
drivers/ide/pci/via82cxxx.c +2 -4
 static void via82cxxx_tune_drive(ide_drive_t *drive, u8 pio)
 {
-	if (pio == 255) {
-		via_set_drive(drive, ide_find_best_pio_mode(drive));
-		return;
-	}
+	if (pio == 255)
+		pio = ide_get_best_pio_mode(drive, 255, 5);
 
 	via_set_drive(drive, XFER_PIO_0 + min_t(u8, pio, 5));

Same question here...

MBR, Sergei

#2 lore raw
Quoted 9 lines · Sergei Shtylyov
Quoted 6 lines · Bartlomiej Zolnierkiewicz

* Add ->host_flags to ide_hwif_t to store ide_pci_device_t.host_flags, assign it in setup-pci.c:ide_pci_setup_ports().

* Add IDE_HFLAG_PIO_NO_{BLACKLIST,DOWNGRADE} to ide_pci_device_t.host_flags and teach ide_get_best_pio_mode() about them. Also remove needless !drive->id check while at it (drive->id is always present).

Quoted 8 lines
Quoted 6 lines

* Convert amd74xx, via82cxxx and ide-timing.h to use ide_get_best_pio_mode() and then remove no longer needed ide_find_best_pio_mode().

There should be no functionality changes caused by this patch.

added

Quoted 10 lines

Still I have something to ask...

Quoted 8 lines

Index: b/drivers/ide/ide-lib.c
===================================================================

drivers/ide/ide-lib.c +0 -0
@@ -291,11 +291,11 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 	struct hd_driveid* id = drive->id;
 	int overridden  = 0;
 
Quoted 11 lines

drivers/ide/ide-lib.c u8 ide_get_best_pio_mode (ide_drive_t *d · line 297

Quoted 9 lines
drivers/ide/ide-lib.c +4 -5
-	if (mode_wanted != 255) {
-		pio_mode = mode_wanted;
-	} else if (!drive->id) {
-		pio_mode = 0;
-	} else if ((pio_mode = ide_scan_pio_blacklist(id->model)) != -1) {
+	if (mode_wanted != 255)
+		return min_t(u8, mode_wanted, max_mode);
+
+	if ((drive->hwif->host_flags & IDE_HFLAG_PIO_NO_BLACKLIST) == 0 &&

C also has ! operator. ;-)

IMO !() is less readable and more error-prone than () == 0.

Matter of style.

Quoted 4 lines
Quoted 4 lines
drivers/ide/ide-lib.c +1 -0
+	    (pio_mode = ide_scan_pio_blacklist(id->model)) != -1) {
 		printk(KERN_INFO "%s: is on PIO blacklist\n", drive->name);
 	} else {
 		pio_mode = id->tPIO;
Quoted 8 lines

drivers/ide/ide-lib.c u8 ide_get_best_pio_mode (ide_drive_t *d · line 327

Quoted 6 lines
drivers/ide/ide-lib.c +1 -1
@@ -324,7 +324,8 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 		/*
 		 * Conservative "downgrade" for all pre-ATA2 drives
 		 */
-		if (pio_mode && pio_mode < 4) {
+		if ((drive->hwif->host_flags & IDE_HFLAG_PIO_NO_DOWNGRADE) == 0 &&

It really does! :-)

This turns into ExtremeNitpicking. ;-)

Quoted 26 lines
Quoted 26 lines
drivers/ide/ide-lib.c +1 -0
+		    pio_mode && pio_mode < 4) {
 			pio_mode--;
 			printk(KERN_INFO "%s: applying conservative "
 					 "PIO \"downgrade\"\n", drive->name);

Index: b/drivers/ide/pci/amd74xx.c
===================================================================

drivers/ide/pci/amd74xx.c +3 -5
@@ -1,5 +1,5 @@
 /*
- * Version 2.20
+ * Version 2.21
  *
  * AMD 755/756/766/8111 and nVidia nForce/2/2s/3/3s/CK804/MCP04
  * IDE driver for Linux.
@@ -275,10 +275,8 @@ static int amd_set_drive(ide_drive_t *dr
 
 static void amd74xx_tune_drive(ide_drive_t *drive, u8 pio)
 {
-	if (pio == 255) {
-		amd_set_drive(drive, ide_find_best_pio_mode(drive));
-		return;
-	}
+	if (pio == 255)
+		pio = ide_get_best_pio_mode(drive, 255, 5);
 
Quoted 5 lines

drivers/ide/pci/amd74xx.c static int amd_set_drive(ide_drive_t *dr · line 282

Quoted 2 lines
drivers/ide/pci/amd74xx.c +0 -0
 	amd_set_drive(drive, XFER_PIO_0 + min_t(byte, pio, 5));
 }

Erm, I don't get it -- why not just use ide_get_best_pio_mode() for every
'pio'? There should be no functionality changes, no?

Indeed but ide_get_best_pio_mode() call goes away completely in patch #13/15.

Please don't miss "The Big Picture".

Quoted 23 lines
Quoted 23 lines

Index: b/drivers/ide/pci/via82cxxx.c
===================================================================

drivers/ide/pci/via82cxxx.c +3 -5
@@ -1,6 +1,6 @@
 /*
  *
- * Version 3.45
+ * Version 3.46
  *
  * VIA IDE driver for Linux. Supported southbridges:
  *
@@ -203,10 +203,8 @@ static int via_set_drive(ide_drive_t *dr
 
 static void via82cxxx_tune_drive(ide_drive_t *drive, u8 pio)
 {
-	if (pio == 255) {
-		via_set_drive(drive, ide_find_best_pio_mode(drive));
-		return;
-	}
+	if (pio == 255)
+		pio = ide_get_best_pio_mode(drive, 255, 5);
 
Quoted 3 lines

drivers/ide/pci/via82cxxx.c static int via_set_drive(ide_drive_t *dr · line 209

Quoted 1 line
drivers/ide/pci/via82cxxx.c +0 -0
 	via_set_drive(drive, XFER_PIO_0 + min_t(u8, pio, 5));

Same question here...

ditto

General remark: please note that the keeping the development _process_ steady is much more important than getting _single_ change perfect (it just need to be good enough) because otherwise we end up investing too much time on things which are not that important in the long-term and time is a very limited resource.

Well, I tended to be perfectionist myself until I took the grasp of above.

;-)

Thanks,
Bart