Re: [PATCH] ide: add ide_set{_max}_pio() (take 2)

Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx> · Wed, 04 Jul 2007 00:04:40 +0400

Bartlomiej Zolnierkiewicz wrote:

* Add IDE_HFLAG_ABUSE_{PREFETCH,FAST_DEVSEL,DMA_MODES} flags
  and set them in ht6560, cmd640, cmd64x and sc1200 host drivers.

* Add set_pio_mode_abuse() for checking if host driver has a non-standard
  ->tuneproc() implementation and use it in do_special().

* Add ide_set_pio() for setting PIO mode (it uses hwif->pio_mask to find
  the maximum PIO mode supported by the host), also add ide_set_max_pio()
  wrapper for ide_set_pio() to use for auto-tuning.  Convert users of
  ->tuneproc to use ide_set{_max}_pio() where possible.  This leaves only
  do_special(), set_using_pio(), ide_hwif_restore() and ide_set_pio() as
  a direct users of ->tuneproc.

* Remove no longer needed ide_get_best_pio_mode() calls and printk-s

   Wait... shouldn't we also un-export it then, and just make static? Well, 
just noticed that there'll be callers left...

  reporting PIO mode selected from ->tuneproc implementations.

* Rename ->tuneproc hook to ->set_pio_mode

   Well, tuneproc() went with speedproc() rather well. :-)

and make 'pio' argument const.

   Isn't it too strict, const value argument?

* Remove stale comment from ide_config_drive_speed().

   Hm, the next logical step would be to remove a call to 
ide_config_drive_speed() from the set_pio_mode() handler, wouldn't it?..

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>

   Alas, had to NAK the patch -- mostly due to ancient QD65xx VLB chips. :-P
There are other nits though...

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

--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -196,8 +196,7 @@ static ide_startstop_t ide_start_power_s
 		return do_rw_taskfile(drive, args);
 
 	case idedisk_pm_restore_pio:	/* Resume step 1 (restore PIO) */
-		if (drive->hwif->tuneproc != NULL)
-			drive->hwif->tuneproc(drive, 255);
+		ide_set_max_pio(drive);
 		/*
 		 * skip idedisk_pm_idle for ATAPI devices
 		 */
@@ -783,6 +782,38 @@ static ide_startstop_t ide_disk_special(
 	return ide_started;
 }
 
+/*
+ * handle HDIO_SET_PIO_MODE ioctl abusers here, eventually it will go away
+ */
+static int set_pio_mode_abuse(ide_hwif_t *hwif, u8 req_pio)
+{
+	int abuse;

   Do we really need this variable?

+
+	switch (req_pio) {
+	case 202:
+	case 201:
+	case 200:
+	case 102:
+	case 101:
+	case 100:
+		abuse = (hwif->host_flags & IDE_HFLAG_ABUSE_DMA_MODES) ? 1 : 0;
+		break;
+	case 9:
+	case 8:
+		abuse = (hwif->host_flags & IDE_HFLAG_ABUSE_PREFETCH) ? 1 : 0;
+		break;
+	case 7:
+	case 6:
+		abuse = (hwif->host_flags & IDE_HFLAG_ABUSE_FAST_DEVSEL) ? 1 : 0;
+		break;
+	default:
+		abuse = 0;
+		break;
+	}
+
+	return abuse;

    Why not just return from the switch stmt itself?

Index: b/drivers/ide/legacy/qd65xx.c
===================================================================
--- a/drivers/ide/legacy/qd65xx.c
+++ b/drivers/ide/legacy/qd65xx.c
@@ -224,15 +224,14 @@ static void qd_set_timing (ide_drive_t *
 	printk(KERN_DEBUG "%s: %#x\n", drive->name, timing);
 }
 
-/*
- * qd6500_tune_drive
- */
-
-static void qd6500_tune_drive (ide_drive_t *drive, u8 pio)
+static void qd6500_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
 	int active_time   = 175;
 	int recovery_time = 415; /* worst case values from the dos driver */
 
+	/*
+	 * FIXME: use "pio" value
+	 */

   The drive->id is also asking to be put into local variable...

 	if (drive->id && !qd_find_disk_type(drive, &active_time, &recovery_time)
 		&& drive->id->tPIO && (drive->id->field_valid & 0x02)
 		&& drive->id->eide_pio >= 240) {
@@ -246,11 +245,7 @@ static void qd6500_tune_drive (ide_drive
 	qd_set_timing(drive, qd6500_compute_timing(HWIF(drive), active_time, recovery_time));
 }
 
-/*
- * qd6580_tune_drive
- */
-
-static void qd6580_tune_drive (ide_drive_t *drive, u8 pio)
+static void qd6580_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
 	int base = HWIF(drive)->select_data;
 	unsigned int cycle_time;
[...]
@@ -335,8 +329,7 @@ static int __init qd_testreg(int port)
  */
 
 static void __init qd_setup(ide_hwif_t *hwif, int base, int config,
-			    unsigned int data0, unsigned int data1,
-			    void (*tuneproc) (ide_drive_t *, u8 pio))
+			    unsigned int data0, unsigned int data1)
 {
 	hwif->chipset = ide_qd65xx;
 	hwif->channel = hwif->index;
@@ -347,8 +340,6 @@ static void __init qd_setup(ide_hwif_t *
 	hwif->drives[0].io_32bit =
 	hwif->drives[1].io_32bit = 1;
 	hwif->pio_mask = ATA_PIO4;
-	hwif->tuneproc = tuneproc;

   Not sure if repeating this line thrice instead was really an improvement...

-	probe_hwif_init(hwif);
 }
 
 /*
@@ -361,7 +352,7 @@ static void __exit qd_unsetup(ide_hwif_t
 {
 	u8 config = hwif->config_data;
 	int base = hwif->select_data;
-	void *tuneproc = (void *) hwif->tuneproc;
+	void *set_pio_mode = (void *)hwif->set_pio_mode;
 
 	if (hwif->chipset != ide_qd65xx)
 		return;
@@ -369,12 +360,12 @@ static void __exit qd_unsetup(ide_hwif_t
 	printk(KERN_NOTICE "%s: back to defaults\n", hwif->name);
 
 	hwif->selectproc = NULL;
-	hwif->tuneproc = NULL;
+	hwif->set_pio_mode = NULL;
 
-	if (tuneproc == (void *) qd6500_tune_drive) {
+	if (set_pio_mode == (void *)qd6500_tune_drive) {

   Wait, you've just renamed it to qd6580_set_pio_mode()!

 		// will do it for both
 		qd_write_reg(QD6500_DEF_DATA, QD_TIMREG(&hwif->drives[0]));
-	} else if (tuneproc == (void *) qd6580_tune_drive) {
+	} else if (set_pio_mode == (void *)qd6580_tune_drive) {

   Same here...

Index: b/drivers/ide/pci/alim15x3.c
===================================================================
--- a/drivers/ide/pci/alim15x3.c
+++ b/drivers/ide/pci/alim15x3.c
@@ -283,17 +283,14 @@ static int ali_get_info (char *buffer, c
 #endif  /* defined(DISPLAY_ALI_TIMINGS) && defined(CONFIG_IDE_PROC_FS) */
 
 /**
- *	ali15x3_tune_pio	-	set up chipset for PIO mode
+ *	ali_tune_pio	-	set up chipset for PIO mode
  *	@drive: drive to tune
  *	@pio: desired mode
  *
- *	Select the best PIO mode for the drive in question.
- *	Then program the controller for this mode.
- *
- *	Returns the PIO mode programmed.
+ *	Program the controller for @pio PIO mode.

   Rather "for PIO mode @pio."

  */
- 
-static u8 ali15x3_tune_pio (ide_drive_t *drive, u8 pio)
+
+void ali_tune_pio(ide_drive_t *drive, const u8 pio)

   Wait... shouldn't it remain static?

Index: b/drivers/ide/pci/cs5535.c
===================================================================
--- a/drivers/ide/pci/cs5535.c
+++ b/drivers/ide/pci/cs5535.c
@@ -144,24 +144,20 @@ static int cs5535_set_drive(ide_drive_t 
[...]
+static void cs5535_set_pio_mode(ide_drive_t *drive, const u8 pio)
+{
+	ide_config_drive_speed(drive, XFER_PIO_0 + pio);
+	/* FIXME: "XFER_PIO_0 + pio" should be passed instead of "pio" */

   Subject to a separate patch? Why not just push it ahead of this? I see -- 
lack of time... :-)

+	cs5535_set_speed(drive, pio);
 }
 
 static int cs5535_dma_check(ide_drive_t *drive)
Index: b/drivers/ide/pci/it8213.c
===================================================================
--- a/drivers/ide/pci/it8213.c
+++ b/drivers/ide/pci/it8213.c
@@ -4,6 +4,8 @@
  * Copyright (C) 2006 Jack Lee
  * Copyright (C) 2006 Alan Cox
  * Copyright (C) 2007 Bartlomiej Zolnierkiewicz
+ *
+ * TODO: make ->set_pio_mode method set transfer mode on the device

   IMHO, this actually better be done outside of this method (if possible).
Sigh, that would undo many of my prior fixes...

@@ -193,7 +194,9 @@ static int it8213_tune_chipset (ide_driv
 		if (reg55 & w_flag)
 			pci_write_config_byte(dev, 0x55, (u8) reg55 & ~w_flag);
 	}
-	it8213_tuneproc(drive, it8213_dma_2_pio(speed));
+
+	it8213_set_pio_mode(drive, it8213_dma_2_pio(speed));

   Bleh... Still haven't "divorced" PIO/DMA timings -- need to get this done 
finally. :-/

Index: b/drivers/ide/pci/it821x.c
===================================================================
--- a/drivers/ide/pci/it821x.c
+++ b/drivers/ide/pci/it821x.c
@@ -274,9 +274,8 @@ static int it821x_tunepio(ide_drive_t *d
 	return ide_config_drive_speed(drive, XFER_PIO_0 + set_pio);
 }
 
-static void it821x_tuneproc(ide_drive_t *drive, u8 pio)
+static void it821x_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
-	pio = ide_get_best_pio_mode(drive, pio, 4);
 	(void)it821x_tunepio(drive, pio);
 }

   This way, it turns into quite a useless wrapper for it821x_tunepio() --
I think ide_config_drive_speed() call should be removed from there at the 
expense of the callers...

Index: b/drivers/ide/pci/jmicron.c
===================================================================
--- a/drivers/ide/pci/jmicron.c
+++ b/drivers/ide/pci/jmicron.c
@@ -83,7 +83,7 @@ static u8 __devinit ata66_jmicron(ide_hw
 	return ATA_CBL_PATA80;
 }
 
-static void jmicron_tuneproc (ide_drive_t *drive, byte mode_wanted)
+static void jmicron_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
 	return;

   Is it useful at all?! With a claimed support of PIO5, I guess this driver 
*at least* deserves FIXME.  Probably I'll have to withdraw my ACK on the PIO 
mask patch... :-|

Index: b/drivers/ide/pci/opti621.c
===================================================================
--- a/drivers/ide/pci/opti621.c
+++ b/drivers/ide/pci/opti621.c
@@ -147,12 +147,13 @@ static void compute_pios(ide_drive_t *dr
 	int d;
 	ide_hwif_t *hwif = HWIF(drive);
 
-	drive->drive_data = ide_get_best_pio_mode(drive, pio, OPTI621_MAX_PIO);
+	drive->drive_data = pio;
+
 	for (d = 0; d < 2; ++d) {
 		drive = &hwif->drives[d];
 		if (drive->present) {
 			if (drive->drive_data == PIO_DONT_KNOW)
-				drive->drive_data = ide_get_best_pio_mode(drive, 255, OPTI621_MAX_PIO);
+				drive->drive_data = ide_get_best_pio_mode(drive, 255, 3);

   Ah, I see there will still be ide_get_best_pio_mode() callers left...

Index: b/drivers/ide/pci/pdc202xx_new.c
===================================================================
--- a/drivers/ide/pci/pdc202xx_new.c
+++ b/drivers/ide/pci/pdc202xx_new.c
@@ -217,9 +217,8 @@ static int pdcnew_tune_chipset(ide_drive
 	return err;
 }
 
-static void pdcnew_tune_drive(ide_drive_t *drive, u8 pio)
+static void pdc_set_pio_mode(ide_drive_t *drive, const u8 pio)

   Why not just keep it named pdcnew_* to not confuse with pdc202xx_old?

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
--- a/drivers/ide/pci/pdc202xx_old.c
+++ b/drivers/ide/pci/pdc202xx_old.c
@@ -143,9 +143,8 @@ static int pdc202xx_tune_chipset (ide_dr
 	return ide_config_drive_speed(drive, speed);
 }
 
-static void pdc202xx_tune_drive(ide_drive_t *drive, u8 pio)
+static void pdc_set_pio_mode(ide_drive_t *drive, const u8 pio)

   Why not just keep it named pdc202xx_* or make it pdc[_]old_ to not confuse 
with pdc202xx_new?

@@ -307,10 +306,11 @@ static void pdc202xx_reset (ide_drive_t 
 {
 	ide_hwif_t *hwif	= HWIF(drive);
 	ide_hwif_t *mate	= hwif->mate;
-	
+
 	pdc202xx_reset_host(hwif);
 	pdc202xx_reset_host(mate);

   Bleh... this double reset horror still needs to be sorted out as well. I'm 
not at all sure it's useful -- its assumed purpose is to be able to set MWDMA 
modes after UDMA (can't do this w/o reset).

-	pdc202xx_tune_drive(drive, 255);
+
+	ide_set_max_pio(drive);

   I wonder why the code doesn't retune all 4 drives? :-/

Index: b/drivers/ide/pci/scc_pata.c
===================================================================
--- a/drivers/ide/pci/scc_pata.c
+++ b/drivers/ide/pci/scc_pata.c
@@ -338,7 +320,7 @@ static int scc_config_drive_for_dma(ide_
 		return 0;
 
 	if (ide_use_fast_pio(drive))
-		scc_tuneproc(drive, 4);
+		scc_set_pio_mode(drive, 4); /* FIXME */

   Well, such simplistic fixes would just better go ahead of the big patches...

Index: b/drivers/ide/pci/sis5513.c
===================================================================
--- a/drivers/ide/pci/sis5513.c
+++ b/drivers/ide/pci/sis5513.c
@@ -519,14 +519,13 @@ static void config_art_rwp_pio (ide_driv
 	}
 }
 
-static int sis5513_tune_drive(ide_drive_t *drive, u8 pio)
+static int sis5513_tune_drive(ide_drive_t *drive, const u8 pio)
 {
-	pio = ide_get_best_pio_mode(drive, pio, 4);
 	config_art_rwp_pio(drive, pio);
 	return ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
 
-static void sis5513_tuneproc(ide_drive_t *drive, u8 pio)
+static void sis_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
 	(void)sis5513_tune_drive(drive, pio);

   Nearly useless wrapper again... :-/

Index: b/drivers/ide/pci/sl82c105.c
===================================================================
--- a/drivers/ide/pci/sl82c105.c
+++ b/drivers/ide/pci/sl82c105.c
@@ -75,16 +75,12 @@ static unsigned int get_pio_timings(ide_
 /*
  * Configure the chipset for PIO mode.
  */
-static u8 sl82c105_tune_pio(ide_drive_t *drive, u8 pio)
+static void sl82c105_tune_pio(ide_drive_t *drive, const u8 pio)
 {
 	struct pci_dev *dev	= HWIF(drive)->pci_dev;
 	int reg			= 0x44 + drive->dn * 4;
 	u16 drv_ctrl;
 
-	DBG(("sl82c105_tune_pio(drive:%s, pio:%u)\n", drive->name, pio));

@@ -327,11 +321,10 @@ static void sl82c105_resetproc(ide_drive
  * We only deal with PIO mode here - DMA mode 'using_dma' is not
  * initialised at the point that this function is called.
  */
-static void sl82c105_tune_drive(ide_drive_t *drive, u8 pio)
+static void sl82c105_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
-	DBG(("sl82c105_tune_drive(drive:%s, pio:%u)\n", drive->name, pio));

   You leave the DBG() stuff alone! :-D

Index: b/drivers/ide/pci/tc86c001.c
===================================================================
--- a/drivers/ide/pci/tc86c001.c
+++ b/drivers/ide/pci/tc86c001.c
@@ -45,9 +45,8 @@ static int tc86c001_tune_chipset(ide_dri
 	return ide_config_drive_speed(drive, speed);
 }
 
-static void tc86c001_tune_drive(ide_drive_t *drive, u8 pio)
+static void tc86c001_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
-	pio = ide_get_best_pio_mode(drive, pio, 4);
 	(void) tc86c001_tune_chipset(drive, XFER_PIO_0 + pio);

   Another wrapper... well, no -- this wraps around the speedproc() method.
Well, there'll be the same in quite a few drivers -- which makes me wonder 
whether we need the separate PIO tuning method at all.

Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -634,7 +634,7 @@ typedef struct ide_drive_s {
 
 	unsigned int	bios_cyl;	/* BIOS/fdisk/LILO number of cyls */
 	unsigned int	cyl;		/* "real" number of cyls */
-	unsigned int	drive_data;	/* use by tuneproc/selectproc */
+	unsigned int	drive_data;	/* use by set_pio_mode/selectproc */

   Wouldn't it sound better as "used by"?

MBR, Sergei
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




