(2.6.20-rc7) cmd64x: fix PIO mode setup

PATCH In review Sergei Shtylyov 4 participants 69 messages Feb 3, 2007 – Nov 16, 2007 3 messages not in the archive libata

lore

Share & tools

lore

Sergei Shtylyov

author

2007-02-03 20:09 UTC #0 lore raw

PATCH 1/1 (2.6.20-rc7) cmd64x: fix PIO mode setup

The driver's tuneproc() method fails to set the drive's own speed -- fix this by renaming the function to cmd64x_tune_pio(), making it return the mode set, and "wrapping" the new tuneproc() method around it; while at it, also get rid of the non-working prefetch control code, remove redundant PIO5 mode limitation, and make cmdprintk() give more sensible mode info. Also, get rid of the broken config_chipset_for_pio() which always tried to set PIO4 instead auto-tuning PIO. Note that removing a call from config_chipset_for_dma() breaks "rudimentary" MWDMA2 support which can only work because of the timing registers being pre- setup for PIO4 since the code in the speedproc() method which sets up the chip for SW/MW DMA is completely bogus (!) and I'm going to remove it for the time being in the next patch...
Oh, and add the missing PIO5 support to the speedproc() method while at it. :-)

Warning: compile tested only -- getting to the real hardware isn't that easy...

Signed-off-by Sergei Shtylyov <[email protected]>

 drivers/ide/pci/cmd64x.c |   92 ++++++++++++++++-------------------------------
 1 files changed, 32 insertions(+), 60 deletions(-)

drivers/ide/pci/cmd64x.c +32 ~~-60~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -262,43 +262,25 @@ static void program_drive_counts (ide_dr
 }
 
 /*
- * Attempts to set the interface PIO mode.
- * The preferred method of selecting PIO modes (e.g. mode 4) is 
- * "echo 'piomode:4' > /proc/ide/hdx/settings".  Special cases are
- * 8: prefetch off, 9: prefetch on, 255: auto-select best mode.
- * Called with 255 at boot time.
+ * This routine selects drive's best PIO mode, calculates setup/active/recovery
+ * counts, and writes them into the chipset registers.
  */
-
-static void cmd64x_tuneproc (ide_drive_t *drive, u8 mode_wanted)
+static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted)
 {
 	int setup_time, active_time, recovery_time;
 	int clock_time, pio_mode, cycle_time;
 	u8 recovery_count2, cycle_count;
 	int setup_count, active_count, recovery_count;
 	int bus_speed = system_bus_clock();
-	/*byte b;*/
 	ide_pio_data_t  d;
 
-	switch (mode_wanted) {
-		case 8: /* set prefetch off */
-		case 9: /* set prefetch on */
-			mode_wanted &= 1;
-			/*set_prefetch_mode(index, mode_wanted);*/
-			cmdprintk("%s: %sabled cmd640 prefetch\n",
-				drive->name, mode_wanted ? "en" : "dis");
-			return;
-	}
-
-	mode_wanted = ide_get_best_pio_mode (drive, mode_wanted, 5, &d);
-	pio_mode = d.pio_mode;
+	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &d);
 	cycle_time = d.cycle_time;
 
 	/*
 	 * I copied all this complicated stuff from cmd640.c and made a few
 	 * minor changes.  For now I am just going to pray that it is correct.
 	 */
-	if (pio_mode > 5)
-		pio_mode = 5;
 	setup_time  = ide_pio_timings[pio_mode].setup_time;
 	active_time = ide_pio_timings[pio_mode].active_time;
 	recovery_time = cycle_time - (setup_time + active_time);

@@ -320,22 +302,27 @@ static void cmd64x_tuneproc (ide_drive_t
 	if (active_count > 16)
 		active_count = 16; /* maximum allowed by cmd646 */
 
-	/*
-	 * In a perfect world, we might set the drive pio mode here
-	 * (using WIN_SETFEATURE) before continuing.
-	 *
-	 * But we do not, because:
-	 *	1) this is the wrong place to do it
-	 *		(proper is do_special() in ide.c)
-	 * 	2) in practice this is rarely, if ever, necessary
-	 */
 	program_drive_counts (drive, setup_count, active_count, recovery_count);
 
-	cmdprintk("%s: selected cmd646 PIO mode%d : %d (%dns)%s, "
+	cmdprintk("%s: PIO mode wanted %d, selected %d (%dns)%s, "
 		"clocks=%d/%d/%d\n",
-		drive->name, pio_mode, mode_wanted, cycle_time,
+		drive->name, mode_wanted, pio_mode, cycle_time,
 		d.overridden ? " (overriding vendor mode)" : "",
 		setup_count, active_count, recovery_count);
+
+	return pio_mode;
+}
+
+/*
+ * Attempts to set drive's PIO mode.
+ * The preferred method of selecting PIO modes (e.g. mode 4) is
+ * "echo 'piomode:4' > /proc/ide/hdx/settings".
+ * Special case is 255: auto-select best mode (used at boot time).
+ */
+static void cmd64x_tune_drive (ide_drive_t *drive, u8 pio)
+{
+	pio = cmd64x_tune_pio(drive, pio);
+	(void) ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
 
 static u8 cmd64x_ratemask (ide_drive_t *drive)

@@ -387,22 +374,6 @@ static u8 cmd64x_ratemask (ide_drive_t *
 	return mode;
 }
 
-static void config_cmd64x_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-	u8 speed	= 0x00;
-	u8 set_pio	= ide_get_best_pio_mode(drive, 4, 5, NULL);
-
-	cmd64x_tuneproc(drive, set_pio);
-	speed = XFER_PIO_0 + set_pio;
-	if (set_speed)
-		(void) ide_config_drive_speed(drive, speed);
-}
-
-static void config_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-	config_cmd64x_chipset_for_pio(drive, set_speed);
-}
-
 static int cmd64x_tune_chipset (ide_drive_t *drive, u8 xferspeed)
 {
 	ide_hwif_t *hwif	= HWIF(drive);

@@ -414,7 +385,7 @@ static int cmd64x_tune_chipset (ide_driv
 
 	u8 speed	= ide_rate_filter(cmd64x_ratemask(drive), xferspeed);
 
-	if (speed > XFER_PIO_4) {
+	if (speed >= XFER_SW_DMA_0) {
 		(void) pci_read_config_byte(dev, pciD, &regD);
 		(void) pci_read_config_byte(dev, pciU, &regU);
 		regD &= ~(unit ? 0x40 : 0x20);

@@ -438,17 +409,20 @@ static int cmd64x_tune_chipset (ide_driv
 		case XFER_SW_DMA_2:	regD |= (unit ? 0x40 : 0x10); break;
 		case XFER_SW_DMA_1:	regD |= (unit ? 0x80 : 0x20); break;
 		case XFER_SW_DMA_0:	regD |= (unit ? 0xC0 : 0x30); break;
-		case XFER_PIO_4:	cmd64x_tuneproc(drive, 4); break;
-		case XFER_PIO_3:	cmd64x_tuneproc(drive, 3); break;
-		case XFER_PIO_2:	cmd64x_tuneproc(drive, 2); break;
-		case XFER_PIO_1:	cmd64x_tuneproc(drive, 1); break;
-		case XFER_PIO_0:	cmd64x_tuneproc(drive, 0); break;
+		case XFER_PIO_5:
+		case XFER_PIO_4:
+		case XFER_PIO_3:
+		case XFER_PIO_2:
+		case XFER_PIO_1:
+		case XFER_PIO_0:
+			(void) cmd64x_tune_pio(drive, speed - XFER_PIO_0);
+			break;
 
 		default:
 			return 1;
 	}
 
-	if (speed > XFER_PIO_4) {
+	if (speed >= XFER_SW_DMA_0) {
 		(void) pci_write_config_byte(dev, pciU, regU);
 		regD |= (unit ? 0x40 : 0x20);
 		(void) pci_write_config_byte(dev, pciD, regD);

@@ -461,8 +435,6 @@ static int config_chipset_for_dma (ide_d
 {
 	u8 speed	= ide_dma_speed(drive, cmd64x_ratemask(drive));
 
-	config_chipset_for_pio(drive, !speed);
-
 	if (!speed)
 		return 0;

@@ -491,7 +463,7 @@ static int cmd64x_config_drive_for_dma (
 
 	} else if ((id->capability & 8) || (id->field_valid & 2)) {
 fast_ata_pio:
-		config_chipset_for_pio(drive, 1);
+		cmd64x_tune_drive(drive, 255);
 		return hwif->ide_dma_off_quietly(drive);
 	}
 	/* IORDY not supported */

@@ -694,7 +666,7 @@ static void __devinit init_hwif_cmd64x(i
 	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
 	class_rev &= 0xff;
 
-	hwif->tuneproc  = &cmd64x_tuneproc;
+	hwif->tuneproc  = &cmd64x_tune_drive;
 	hwif->speedproc = &cmd64x_tune_chipset;
 
 	if (!hwif->dma_base) {

Sergei Shtylyov

author

2007-02-03 21:04 UTC #1 lore raw

Re: [PATCH] (2.6.20-rc7) cmd64x: fix PIO mode setup (take 2)

[Finally forgot to stamp MV's copyright on the driver, so here's take #2...]

The driver's tuneproc() method fails to set the drive's own speed -- fix this by renaming the function to cmd64x_tune_pio(), making it return the mode set, and "wrapping" the new tuneproc() method around it; while at it, also get rid of the non-working prefetch control code, remove redundant PIO5 mode limitation, and make cmdprintk() give more sensible mode info. Also, get rid of the broken config_chipset_for_pio() which always tried to set PIO4 instead auto-tuning PIO. Note that removing a call from config_chipset_for_dma() breaks "rudimentary" MWDMA2 support which can only work because of the timing registers being pre- setup for PIO4 since the code in the speedproc() method which sets up the chip for SW/MW DMA is completely bogus (!) and I'm going to remove it for the time being in the next patch.
Oh, and add the missing PIO5 support to the speedproc() method while at it. :-)

Warning: compile tested only -- getting to the real hardware isn't that easy...

Signed-off-by Sergei Shtylyov <[email protected]>

 drivers/ide/pci/cmd64x.c |   95 ++++++++++++++++-------------------------------
 1 files changed, 34 insertions(+), 61 deletions(-)

drivers/ide/pci/cmd64x.c +34 ~~-61~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.30	Sept 10, 2002
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.41	Feb 3, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -12,6 +12,7 @@
  * Copyright (C) 1998		David S. Miller ([email protected])
  *
  * Copyright (C) 1999-2002	Andre Hedrick <[email protected]>
+ * Copyright (C) 2007		MontaVista Software, Inc. <[email protected]>
  */
 
 #include <linux/module.h>

@@ -262,43 +263,25 @@ static void program_drive_counts (ide_dr
 }
 
 /*
- * Attempts to set the interface PIO mode.
- * The preferred method of selecting PIO modes (e.g. mode 4) is 
- * "echo 'piomode:4' > /proc/ide/hdx/settings".  Special cases are
- * 8: prefetch off, 9: prefetch on, 255: auto-select best mode.
- * Called with 255 at boot time.
+ * This routine selects drive's best PIO mode, calculates setup/active/recovery
+ * counts, and writes them into the chipset registers.
  */
-
-static void cmd64x_tuneproc (ide_drive_t *drive, u8 mode_wanted)
+static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted)
 {
 	int setup_time, active_time, recovery_time;
 	int clock_time, pio_mode, cycle_time;
 	u8 recovery_count2, cycle_count;
 	int setup_count, active_count, recovery_count;
 	int bus_speed = system_bus_clock();
-	/*byte b;*/
 	ide_pio_data_t  d;
 
-	switch (mode_wanted) {
-		case 8: /* set prefetch off */
-		case 9: /* set prefetch on */
-			mode_wanted &= 1;
-			/*set_prefetch_mode(index, mode_wanted);*/
-			cmdprintk("%s: %sabled cmd640 prefetch\n",
-				drive->name, mode_wanted ? "en" : "dis");
-			return;
-	}
-
-	mode_wanted = ide_get_best_pio_mode (drive, mode_wanted, 5, &d);
-	pio_mode = d.pio_mode;
+	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &d);
 	cycle_time = d.cycle_time;
 
 	/*
 	 * I copied all this complicated stuff from cmd640.c and made a few
 	 * minor changes.  For now I am just going to pray that it is correct.
 	 */
-	if (pio_mode > 5)
-		pio_mode = 5;
 	setup_time  = ide_pio_timings[pio_mode].setup_time;
 	active_time = ide_pio_timings[pio_mode].active_time;
 	recovery_time = cycle_time - (setup_time + active_time);

@@ -320,22 +303,27 @@ static void cmd64x_tuneproc (ide_drive_t
 	if (active_count > 16)
 		active_count = 16; /* maximum allowed by cmd646 */
 
-	/*
-	 * In a perfect world, we might set the drive pio mode here
-	 * (using WIN_SETFEATURE) before continuing.
-	 *
-	 * But we do not, because:
-	 *	1) this is the wrong place to do it
-	 *		(proper is do_special() in ide.c)
-	 * 	2) in practice this is rarely, if ever, necessary
-	 */
 	program_drive_counts (drive, setup_count, active_count, recovery_count);
 
-	cmdprintk("%s: selected cmd646 PIO mode%d : %d (%dns)%s, "
+	cmdprintk("%s: PIO mode wanted %d, selected %d (%dns)%s, "
 		"clocks=%d/%d/%d\n",
-		drive->name, pio_mode, mode_wanted, cycle_time,
+		drive->name, mode_wanted, pio_mode, cycle_time,
 		d.overridden ? " (overriding vendor mode)" : "",
 		setup_count, active_count, recovery_count);
+
+	return pio_mode;
+}
+
+/*
+ * Attempts to set drive's PIO mode.
+ * The preferred method of selecting PIO modes (e.g. mode 4) is
+ * "echo 'piomode:4' > /proc/ide/hdx/settings".
+ * Special case is 255: auto-select best mode (used at boot time).
+ */
+static void cmd64x_tune_drive (ide_drive_t *drive, u8 pio)
+{
+	pio = cmd64x_tune_pio(drive, pio);
+	(void) ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
 
 static u8 cmd64x_ratemask (ide_drive_t *drive)

@@ -387,22 +375,6 @@ static u8 cmd64x_ratemask (ide_drive_t *
 	return mode;
 }
 
-static void config_cmd64x_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-	u8 speed	= 0x00;
-	u8 set_pio	= ide_get_best_pio_mode(drive, 4, 5, NULL);
-
-	cmd64x_tuneproc(drive, set_pio);
-	speed = XFER_PIO_0 + set_pio;
-	if (set_speed)
-		(void) ide_config_drive_speed(drive, speed);
-}
-
-static void config_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-	config_cmd64x_chipset_for_pio(drive, set_speed);
-}
-
 static int cmd64x_tune_chipset (ide_drive_t *drive, u8 xferspeed)
 {
 	ide_hwif_t *hwif	= HWIF(drive);

@@ -414,7 +386,7 @@ static int cmd64x_tune_chipset (ide_driv
 
 	u8 speed	= ide_rate_filter(cmd64x_ratemask(drive), xferspeed);
 
-	if (speed > XFER_PIO_4) {
+	if (speed >= XFER_SW_DMA_0) {
 		(void) pci_read_config_byte(dev, pciD, &regD);
 		(void) pci_read_config_byte(dev, pciU, &regU);
 		regD &= ~(unit ? 0x40 : 0x20);

@@ -438,17 +410,20 @@ static int cmd64x_tune_chipset (ide_driv
 		case XFER_SW_DMA_2:	regD |= (unit ? 0x40 : 0x10); break;
 		case XFER_SW_DMA_1:	regD |= (unit ? 0x80 : 0x20); break;
 		case XFER_SW_DMA_0:	regD |= (unit ? 0xC0 : 0x30); break;
-		case XFER_PIO_4:	cmd64x_tuneproc(drive, 4); break;
-		case XFER_PIO_3:	cmd64x_tuneproc(drive, 3); break;
-		case XFER_PIO_2:	cmd64x_tuneproc(drive, 2); break;
-		case XFER_PIO_1:	cmd64x_tuneproc(drive, 1); break;
-		case XFER_PIO_0:	cmd64x_tuneproc(drive, 0); break;
+		case XFER_PIO_5:
+		case XFER_PIO_4:
+		case XFER_PIO_3:
+		case XFER_PIO_2:
+		case XFER_PIO_1:
+		case XFER_PIO_0:
+			(void) cmd64x_tune_pio(drive, speed - XFER_PIO_0);
+			break;
 
 		default:
 			return 1;
 	}
 
-	if (speed > XFER_PIO_4) {
+	if (speed >= XFER_SW_DMA_0) {
 		(void) pci_write_config_byte(dev, pciU, regU);
 		regD |= (unit ? 0x40 : 0x20);
 		(void) pci_write_config_byte(dev, pciD, regD);

@@ -461,8 +436,6 @@ static int config_chipset_for_dma (ide_d
 {
 	u8 speed	= ide_dma_speed(drive, cmd64x_ratemask(drive));
 
-	config_chipset_for_pio(drive, !speed);
-
 	if (!speed)
 		return 0;

@@ -491,7 +464,7 @@ static int cmd64x_config_drive_for_dma (
 
 	} else if ((id->capability & 8) || (id->field_valid & 2)) {
 fast_ata_pio:
-		config_chipset_for_pio(drive, 1);
+		cmd64x_tune_drive(drive, 255);
 		return hwif->ide_dma_off_quietly(drive);
 	}
 	/* IORDY not supported */

@@ -694,7 +667,7 @@ static void __devinit init_hwif_cmd64x(i
 	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
 	class_rev &= 0xff;
 
-	hwif->tuneproc  = &cmd64x_tuneproc;
+	hwif->tuneproc  = &cmd64x_tune_drive;
 	hwif->speedproc = &cmd64x_tune_chipset;
 
 	if (!hwif->dma_base) {

Bartlomiej Zolnierkiewicz

2007-02-03 22:20 UTC #2 lore raw

Re: [PATCH] (2.6.20-rc7) cmd64x: fix PIO mode setup (take 2)

Hi,

Quoted 14 lines · Sergei Shtylyov

[Finally forgot to stamp MV's copyright on the driver, so here's take #2...]
The driver's tuneproc() method fails to set the drive's own speed -- fix this by renaming the function to cmd64x_tune_pio(), making it return the mode set, and "wrapping" the new tuneproc() method around it; while at it, also get rid of the non-working prefetch control code, remove redundant PIO5 mode limitation, and make cmdprintk() give more sensible mode info. Also, get rid of the broken config_chipset_for_pio() which always tried to set PIO4 instead auto-tuning PIO. Note that removing a call from config_chipset_for_dma() breaks "rudimentary" MWDMA2 support which can only work because of the timing registers being pre- setup for PIO4 since the code in the speedproc() method which sets up the chip for SW/MW DMA is completely bogus (!) and I'm going to remove it for the time being in the next patch.
Oh, and add the missing PIO5 support to the speedproc() method while at it. :-)

Generally looks fine, some comments below.

Quoted 55 lines

Warning: compile tested only -- getting to the real hardware isn't that easy...

Signed-off-by Sergei Shtylyov <[email protected]>

 drivers/ide/pci/cmd64x.c |   95 ++++++++++++++++-------------------------------
 1 files changed, 34 insertions(+), 61 deletions(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================

drivers/ide/pci/cmd64x.c +5 ~~-13~~

@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.30	Sept 10, 2002
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.41	Feb 3, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -12,6 +12,7 @@
  * Copyright (C) 1998		David S. Miller ([email protected])
  *
  * Copyright (C) 1999-2002	Andre Hedrick <[email protected]>
+ * Copyright (C) 2007		MontaVista Software, Inc. <[email protected]>
  */
 
 #include <linux/module.h>

@@ -262,43 +263,25 @@ static void program_drive_counts (ide_dr
 }
 
 /*
- * Attempts to set the interface PIO mode.
- * The preferred method of selecting PIO modes (e.g. mode 4) is 
- * "echo 'piomode:4' > /proc/ide/hdx/settings".  Special cases are
- * 8: prefetch off, 9: prefetch on, 255: auto-select best mode.
- * Called with 255 at boot time.
+ * This routine selects drive's best PIO mode, calculates setup/active/recovery
+ * counts, and writes them into the chipset registers.
  */
-
-static void cmd64x_tuneproc (ide_drive_t *drive, u8 mode_wanted)
+static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted)
 {
 	int setup_time, active_time, recovery_time;
 	int clock_time, pio_mode, cycle_time;
 	u8 recovery_count2, cycle_count;
 	int setup_count, active_count, recovery_count;
 	int bus_speed = system_bus_clock();
-	/*byte b;*/
 	ide_pio_data_t  d;
 
-	switch (mode_wanted) {
-		case 8: /* set prefetch off */
-		case 9: /* set prefetch on */
-			mode_wanted &= 1;

drivers/ide/pci/cmd64x.c static void program_drive_counts (ide_dr · line 279

Quoted 10 lines

drivers/ide/pci/cmd64x.c +1 -8

-			/*set_prefetch_mode(index, mode_wanted);*/
-			cmdprintk("%s: %sabled cmd640 prefetch\n",
-				drive->name, mode_wanted ? "en" : "dis");
-			return;
-	}
-
-	mode_wanted = ide_get_best_pio_mode (drive, mode_wanted, 5, &d);
-	pio_mode = d.pio_mode;
+	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &d);
 	cycle_time = d.cycle_time;

After this change if an unaware user passes "8" or "9" to enable/disable prefetch (user doesn't know that it has never worked) it will result in PIO5 being programmed.

I don't think that this is a real world issue but for paranoia reasons please add pio == 8/9 check to cmd64x_tune_drive(), something like:

	/*
	 * letf-over from prefetch setting (pio == 8/9),
	 * needed to prevent PIO5 from being programmed
	 */
	if (pio == 8 || pio == 9)
		return;

This will vanish when core code will do filtering of user space PIO mode change requests...

[ ... ]

drivers/ide/pci/cmd64x.c static void cmd64x_tuneproc (ide_drive_t · line 322

Quoted 6 lines

drivers/ide/pci/cmd64x.c +6 -0

+/*
+ * Attempts to set drive's PIO mode.
+ * The preferred method of selecting PIO modes (e.g. mode 4) is
+ * "echo 'piomode:4' > /proc/ide/hdx/settings".
+ * Special case is 255: auto-select best mode (used at boot time).
+ */

The preferred method is to let the driver do the tuning and if for some reason somebody wants to change the PIO mode, the ioctl interface is preferred over the deprecated "/proc/ide/hdx/settings".

Therefore please remove this comment while at it.

Quoted 26 lines

drivers/ide/pci/cmd64x.c +4 ~~-15~~

+static void cmd64x_tune_drive (ide_drive_t *drive, u8 pio)
+{
+	pio = cmd64x_tune_pio(drive, pio);
+	(void) ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
 
 static u8 cmd64x_ratemask (ide_drive_t *drive)

@@ -387,22 +375,6 @@ static u8 cmd64x_ratemask (ide_drive_t *
 	return mode;
 }
 
-static void config_cmd64x_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-	u8 speed	= 0x00;
-	u8 set_pio	= ide_get_best_pio_mode(drive, 4, 5, NULL);
-
-	cmd64x_tuneproc(drive, set_pio);
-	speed = XFER_PIO_0 + set_pio;
-	if (set_speed)
-		(void) ide_config_drive_speed(drive, speed);
-}
-
-static void config_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-	config_cmd64x_chipset_for_pio(drive, set_speed);
-}

[ ... ]

drivers/ide/pci/cmd64x.c static int config_chipset_for_dma (ide_d · line 465

Quoted 6 lines

drivers/ide/pci/cmd64x.c +0 -2

@@ -461,8 +436,6 @@ static int config_chipset_for_dma (ide_d
 {
 	u8 speed	= ide_dma_speed(drive, cmd64x_ratemask(drive));
 
-	config_chipset_for_pio(drive, !speed);
-

While this was always incorrectly setting PIO4, the PIO4 is "the usual" case and for this driver we need to program PIO explicitly even when using DMA. The core code doesn't program PIO mode unless told to (->autotune flag == 1) so after the above change PIO mode won't be programmed et all.

I think that we now need to set ->autotune unconditionally in init_hwif_cmd64x().

Thanks,
Bart

Sergei Shtylyov

author

2007-02-05 13:30 UTC #3 lore raw

Re: [PATCH] (2.6.20-rc7) cmd64x: fix PIO mode setup (take 2)

Hello.

Quoted 78 lines · Bartlomiej Zolnierkiewicz

Quoted 14 lines
[Finally forgot to stamp MV's copyright on the driver, so here's take #2...]
The driver's tuneproc() method fails to set the drive's own speed -- fix this by renaming the function to cmd64x_tune_pio(), making it return the mode set, and "wrapping" the new tuneproc() method around it; while at it, also get rid of the non-working prefetch control code, remove redundant PIO5 mode limitation, and make cmdprintk() give more sensible mode info. Also, get rid of the broken config_chipset_for_pio() which always tried to set PIO4 instead auto-tuning PIO. Note that removing a call from config_chipset_for_dma() breaks "rudimentary" MWDMA2 support which can only work because of the timing registers being pre- setup for PIO4 since the code in the speedproc() method which sets up the chip for SW/MW DMA is completely bogus (!) and I'm going to remove it for the time being in the next patch.
Oh, and add the missing PIO5 support to the speedproc() method while at it. :-)
Generally looks fine, some comments below.
Quoted 61 lines
Warning: compile tested only -- getting to the real hardware isn't that easy...
Signed-off-by Sergei Shtylyov <[email protected]>
drivers/ide/pci/cmd64x.c | 95 ++++++++++++++++-------------------------------
1 files changed, 34 insertions(+), 61 deletions(-)
Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
drivers/ide/pci/cmd64x.c +5 ~~-19~~
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
 *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.30	Sept 10, 2002
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.41	Feb 3, 2007
 *
 * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
 *           Note, this driver is not used at all on other systems because
@@ -12,6 +12,7 @@
 * Copyright (C) 1998		David S. Miller ([email protected])
 *
 * Copyright (C) 1999-2002	Andre Hedrick <[email protected]>
+ * Copyright (C) 2007		MontaVista Software, Inc. <[email protected]>
 */
 
 #include <linux/module.h>
@@ -262,43 +263,25 @@ static void program_drive_counts (ide_dr
 }
 
 /*
- * Attempts to set the interface PIO mode.
- * The preferred method of selecting PIO modes (e.g. mode 4) is 
- * "echo 'piomode:4' > /proc/ide/hdx/settings".  Special cases are
- * 8: prefetch off, 9: prefetch on, 255: auto-select best mode.
- * Called with 255 at boot time.
+ * This routine selects drive's best PIO mode, calculates setup/active/recovery
+ * counts, and writes them into the chipset registers.
 */
-
-static void cmd64x_tuneproc (ide_drive_t *drive, u8 mode_wanted)
+static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted)
 {
 	int setup_time, active_time, recovery_time;
 	int clock_time, pio_mode, cycle_time;
 	u8 recovery_count2, cycle_count;
 	int setup_count, active_count, recovery_count;
 	int bus_speed = system_bus_clock();
-	/*byte b;*/
 	ide_pio_data_t  d;
 
-	switch (mode_wanted) {
-		case 8: /* set prefetch off */
-		case 9: /* set prefetch on */
-			mode_wanted &= 1;
-			/*set_prefetch_mode(index, mode_wanted);*/
-			cmdprintk("%s: %sabled cmd640 prefetch\n",
-				drive->name, mode_wanted ? "en" : "dis");
-			return;
-	}
-

Quoted 8 lines

drivers/ide/pci/cmd64x.c static void program_drive_counts (ide_dr · line 279
Quoted 4 lines
drivers/ide/pci/cmd64x.c +1 -2
-	mode_wanted = ide_get_best_pio_mode (drive, mode_wanted, 5, &d);
-	pio_mode = d.pio_mode;
+	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &d);
 	cycle_time = d.cycle_time;
After this change if an unaware user passes "8" or "9" to enable/disable prefetch (user doesn't know that it has never worked) it will result in PIO5 being programmed.

Yes. :-)

Quoted 9 lines

I don't think that this is a real world issue but for paranoia reasons please add pio == 8/9 check to cmd64x_tune_drive(), something like:
	/*
	 * letf-over from prefetch setting (pio == 8/9),
	 * needed to prevent PIO5 from being programmed
	 */
	if (pio == 8 || pio == 9)
		return;

OK, I'll probably just leave the prefetch code where it was.

Quoted 12 lines

This will vanish when core code will do filtering of user space PIO mode change requests...
[ ... ]
drivers/ide/pci/cmd64x.c static void cmd64x_tuneproc (ide_drive_t · line 322
Quoted 6 lines
drivers/ide/pci/cmd64x.c +6 -0
+/*
+ * Attempts to set drive's PIO mode.
+ * The preferred method of selecting PIO modes (e.g. mode 4) is
+ * "echo 'piomode:4' > /proc/ide/hdx/settings".
+ * Special case is 255: auto-select best mode (used at boot time).
+ */

Quoted 5 lines

The preferred method is to let the driver do the tuning and if for some reason somebody wants to change the PIO mode, the ioctl interface is preferred over the deprecated "/proc/ide/hdx/settings".
Therefore please remove this comment while at it.

Will do.

Quoted 33 lines

Quoted 26 lines

drivers/ide/pci/cmd64x.c +4 -0

+static void cmd64x_tune_drive (ide_drive_t *drive, u8 pio)
+{
+	pio = cmd64x_tune_pio(drive, pio);
+	(void) ide_config_drive_speed(drive, XFER_PIO_0 + pio);

}

static u8 cmd64x_ratemask (ide_drive_t *drive)

drivers/ide/pci/cmd64x.c +0 ~~-15~~

@@ -387,22 +375,6 @@ static u8 cmd64x_ratemask (ide_drive_t *
 	return mode;
 }
 
-static void config_cmd64x_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-	u8 speed	= 0x00;
-	u8 set_pio	= ide_get_best_pio_mode(drive, 4, 5, NULL);
-
-	cmd64x_tuneproc(drive, set_pio);
-	speed = XFER_PIO_0 + set_pio;
-	if (set_speed)
-		(void) ide_config_drive_speed(drive, speed);
-}
-
-static void config_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-	config_cmd64x_chipset_for_pio(drive, set_speed);
-}

[ ... ]

Quoted 4 lines

drivers/ide/pci/cmd64x.c +0 -0

@@ -461,8 +436,6 @@ static int config_chipset_for_dma (ide_d
 {
 	u8 speed	= ide_dma_speed(drive, cmd64x_ratemask(drive));

Quoted 5 lines

drivers/ide/pci/cmd64x.c static int config_chipset_for_dma (ide_d · line 465
Quoted 2 lines
drivers/ide/pci/cmd64x.c +0 -2
-	config_chipset_for_pio(drive, !speed);
-
While this was always incorrectly setting PIO4, the PIO4 is "the usual" case and for this driver we need to program PIO explicitly even when using DMA.

Hm, why it's *so* special, i.e. why almost all the other drivers can get
away without it (the majority seems to have autotune set *only* if hwif->dma_base is seen as 0 in the init_hwif() method? :-/

Quoted 4 lines

The core code doesn't program PIO mode unless told to (->autotune flag == 1) so after the above change PIO mode won't be programmed et all.
I think that we now need to set ->autotune unconditionally in init_hwif_cmd64x().

No problem. This actually seems the right thing to do in all drivers,
just like the libata core does. :-)

MBR, Sergei

Sergei Shtylyov

author

2007-02-05 13:57 UTC #4 lore raw

Re: [PATCH] (2.6.20-rc7) cmd64x: fix PIO mode setup (take 2)

Quoted 8 lines · Hello, I wrote:

Quoted 7 lines
drivers/ide/pci/cmd64x.c
Quoted 4 lines
Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
drivers/ide/pci/cmd64x.c +0 -0
While this was always incorrectly setting PIO4, the PIO4 is "the usual" case and for this driver we need to program PIO explicitly even when using DMA.

Quoted 7 lines

Hm, why it's *so* special, i.e. why almost all the other drivers can
get away without it (the majority seems to have autotune set *only* if hwif->dma_base is seen as 0 in the init_hwif() method? :-/
Quoted 4 lines
The core code doesn't program PIO mode unless told to (->autotune flag == 1) so after the above change PIO mode won't be programmed et all.
I think that we now need to set ->autotune unconditionally in init_hwif_cmd64x().

Don't think we *need* to. Look at the code at the end of init_chipset()
method. Those values it writes to ARTTIM/DRWTIM registers already matches PIO4! That's another question what this code is doing there, being both duplicate and misplaced. :-)

Quoted 2 lines

No problem. This actually seems the right thing to do in all
drivers, just like the libata core does. :-)

MBR, Sergei

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bartlomiej Zolnierkiewicz

2007-02-06 23:39 UTC #6 lore raw

Re: [PATCH] (2.6.20-rc7) cmd64x: fix PIO mode setup (take 2)

Quoted 18 lines · Sergei Shtylyov

Quoted 14 lines · Hello, I wrote:
Quoted 7 lines
drivers/ide/pci/cmd64x.c
Quoted 4 lines
Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
drivers/ide/pci/cmd64x.c +0 -0
While this was always incorrectly setting PIO4, the PIO4 is "the usual" case and for this driver we need to program PIO explicitly even when using DMA.
Hm, why it's *so* special, i.e. why almost all the other drivers can
get away without it (the majority seems to have autotune set *only* if hwif->dma_base is seen as 0 in the init_hwif() method? :-/
Quoted 2 lines
The core code doesn't program PIO mode unless told to (->autotune flag == 1) so after the above change PIO mode won't be programmed et all.

Quoted 7 lines

Quoted 2 lines
Quoted 2 lines
I think that we now need to set ->autotune unconditionally in init_hwif_cmd64x().
Don't think we *need* to. Look at the code at the end of init_chipset()
method. Those values it writes to ARTTIM/DRWTIM registers already matches PIO4! That's another question what this code is doing there, being both duplicate and misplaced. :-)

For me it looks like a bunch of hacks to get things working on BIOS/firmware-less and non-x86 systems. :)

[ This code pre-dates PIO4 forcing in config_cmd64x_chipset_for_pio(). ]

Bart

Sergei Shtylyov

author

2007-02-07 12:50 UTC #7 lore raw

Re: [PATCH] (2.6.20-rc7) cmd64x: fix PIO mode setup (take 2)

Hello.

Quoted 23 lines · Bartlomiej Zolnierkiewicz

Quoted 15 lines
Quoted 14 lines
Quoted 7 lines
drivers/ide/pci/cmd64x.c
Quoted 4 lines
Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
drivers/ide/pci/cmd64x.c +0 -0
While this was always incorrectly setting PIO4, the PIO4 is "the usual" case and for this driver we need to program PIO explicitly even when using DMA.
Hm, why it's *so* special, i.e. why almost all the other drivers can
get away without it (the majority seems to have autotune set *only* if hwif->dma_base is seen as 0 in the init_hwif() method? :-/
Quoted 2 lines
The core code doesn't program PIO mode unless told to (->autotune flag == 1) so after the above change PIO mode won't be programmed et all.
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
I think that we now need to set ->autotune unconditionally in init_hwif_cmd64x().
Don't think we *need* to. Look at the code at the end of init_chipset()
method. Those values it writes to ARTTIM/DRWTIM registers already matches PIO4! That's another question what this code is doing there, being both duplicate and misplaced. :-)

Quoted 4 lines

For me it looks like a bunch of hacks to get things working on BIOS/firmware-less and non-x86 systems. :)
[ This code pre-dates PIO4 forcing in config_cmd64x_chipset_for_pio(). ]

The "#ifdef __i386__" part looked the most mysterous: why anyone would
want to program less *IDE* address setup on x86 than on non-x86? :-O
I was going to remove all that crap as well but decided to defer it,
mostly because of that part...

Quoted 1 line

Bart

MBR, Sergei

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bartlomiej Zolnierkiewicz

2007-02-06 23:28 UTC #9 lore raw

Re: [PATCH] (2.6.20-rc7) cmd64x: fix PIO mode setup (take 2)

Hi,

Quoted 169 lines · Sergei Shtylyov

Hello.
Quoted 15 lines · Bartlomiej Zolnierkiewicz
Quoted 14 lines
[Finally forgot to stamp MV's copyright on the driver, so here's take #2...]
The driver's tuneproc() method fails to set the drive's own speed -- fix this by renaming the function to cmd64x_tune_pio(), making it return the mode set, and "wrapping" the new tuneproc() method around it; while at it, also get rid of the non-working prefetch control code, remove redundant PIO5 mode limitation, and make cmdprintk() give more sensible mode info. Also, get rid of the broken config_chipset_for_pio() which always tried to set PIO4 instead auto-tuning PIO. Note that removing a call from config_chipset_for_dma() breaks "rudimentary" MWDMA2 support which can only work because of the timing registers being pre- setup for PIO4 since the code in the speedproc() method which sets up the chip for SW/MW DMA is completely bogus (!) and I'm going to remove it for the time being in the next patch.
Oh, and add the missing PIO5 support to the speedproc() method while at it. :-)
Quoted 3 lines
Generally looks fine, some comments below.
Quoted 1 line
Warning: compile tested only -- getting to the real hardware isn't
that easy...
Quoted 3 lines
Quoted 3 lines
Signed-off-by Sergei Shtylyov <[email protected]>
drivers/ide/pci/cmd64x.c | 95
++++++++++++++++-------------------------------
Quoted 56 lines
Quoted 56 lines
1 files changed, 34 insertions(+), 61 deletions(-)
Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
drivers/ide/pci/cmd64x.c +5 ~~-19~~
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
 *
- * linux/drivers/ide/pci/cmd64x.c            Version 1.30    Sept 10, 2002
+ * linux/drivers/ide/pci/cmd64x.c            Version 1.41    Feb 3, 2007
 *
 * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
 *           Note, this driver is not used at all on other systems because
@@ -12,6 +12,7 @@
 * Copyright (C) 1998                David S. Miller ([email protected])
 *
 * Copyright (C) 1999-2002   Andre Hedrick <[email protected]>
+ * Copyright (C) 2007                MontaVista Software, Inc. <[email protected]>
 */
 
 #include <linux/module.h>
@@ -262,43 +263,25 @@ static void program_drive_counts (ide_dr
 }
 
 /*
- * Attempts to set the interface PIO mode.
- * The preferred method of selecting PIO modes (e.g. mode 4) is
- * "echo 'piomode:4' > /proc/ide/hdx/settings".  Special cases are
- * 8: prefetch off, 9: prefetch on, 255: auto-select best mode.
- * Called with 255 at boot time.
+ * This routine selects drive's best PIO mode, calculates setup/active/recovery
+ * counts, and writes them into the chipset registers.
 */
-
-static void cmd64x_tuneproc (ide_drive_t *drive, u8 mode_wanted)
+static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted)
 {
     int setup_time, active_time, recovery_time;
     int clock_time, pio_mode, cycle_time;
     u8 recovery_count2, cycle_count;
     int setup_count, active_count, recovery_count;
     int bus_speed = system_bus_clock();
-     /*byte b;*/
     ide_pio_data_t  d;
 
-     switch (mode_wanted) {
-             case 8: /* set prefetch off */
-             case 9: /* set prefetch on */
-                     mode_wanted &= 1;
-                     /*set_prefetch_mode(index, mode_wanted);*/
-                     cmdprintk("%s: %sabled cmd640 prefetch\n",
-                             drive->name, mode_wanted ? "en" : "dis");
-                     return;
-     }
-
Quoted 8 lines
drivers/ide/pci/cmd64x.c static void program_drive_counts (ide_dr · line 279
Quoted 4 lines
drivers/ide/pci/cmd64x.c +1 -2
-     mode_wanted = ide_get_best_pio_mode (drive, mode_wanted, 5, &d);
-     pio_mode = d.pio_mode;
+     pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &d);
     cycle_time = d.cycle_time;
After this change if an unaware user passes "8" or "9" to enable/disable prefetch (user doesn't know that it has never worked) it will result in PIO5 being programmed.
   Yes. :-)
Quoted 9 lines
I don't think that this is a real world issue but for paranoia reasons please add pio == 8/9 check to cmd64x_tune_drive(), something like:
      /*
       * letf-over from prefetch setting (pio == 8/9),
       * needed to prevent PIO5 from being programmed
       */
      if (pio == 8 || pio == 9)
              return;
OK, I'll probably just leave the prefetch code where it was.
Quoted 12 lines
This will vanish when core code will do filtering of user space PIO mode change requests...
[ ... ]
drivers/ide/pci/cmd64x.c static void cmd64x_tuneproc (ide_drive_t · line 322
Quoted 6 lines
drivers/ide/pci/cmd64x.c +6 -0
+/*
+ * Attempts to set drive's PIO mode.
+ * The preferred method of selecting PIO modes (e.g. mode 4) is
+ * "echo 'piomode:4' > /proc/ide/hdx/settings".
+ * Special case is 255: auto-select best mode (used at boot time).
+ */
Quoted 5 lines
The preferred method is to let the driver do the tuning and if for some reason somebody wants to change the PIO mode, the ioctl interface is preferred over the deprecated "/proc/ide/hdx/settings".
Therefore please remove this comment while at it.
Will do.
Quoted 34 lines
Quoted 27 lines
drivers/ide/pci/cmd64x.c +4 -0
+static void cmd64x_tune_drive (ide_drive_t *drive, u8 pio)
+{
+     pio = cmd64x_tune_pio(drive, pio);
+     (void) ide_config_drive_speed(drive, XFER_PIO_0 + pio);
}
static u8 cmd64x_ratemask (ide_drive_t *drive)
drivers/ide/pci/cmd64x.c +0 ~~-15~~
@@ -387,22 +375,6 @@ static u8 cmd64x_ratemask (ide_drive_t *
     return mode;
 }
 
-static void config_cmd64x_chipset_for_pio (ide_drive_t *drive, u8
 set_speed)
-{
-     u8 speed        = 0x00;
-     u8 set_pio      = ide_get_best_pio_mode(drive, 4, 5, NULL);
-
-     cmd64x_tuneproc(drive, set_pio);
-     speed = XFER_PIO_0 + set_pio;
-     if (set_speed)
-             (void) ide_config_drive_speed(drive, speed);
-}
-
-static void config_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-     config_cmd64x_chipset_for_pio(drive, set_speed);
-}
[ ... ]
Quoted 4 lines
drivers/ide/pci/cmd64x.c +0 -0
@@ -461,8 +436,6 @@ static int config_chipset_for_dma (ide_d
 {
     u8 speed        = ide_dma_speed(drive, cmd64x_ratemask(drive));
 
Quoted 6 lines
drivers/ide/pci/cmd64x.c static int config_chipset_for_dma (ide_d · line 465
Quoted 2 lines
drivers/ide/pci/cmd64x.c +0 -2
-     config_chipset_for_pio(drive, !speed);
-
While this was always incorrectly setting PIO4, the PIO4 is "the usual" case and for this driver we need to program PIO explicitly even when using DMA.

Quoted 3 lines

Hm, why it's *so* special, i.e. why almost all the other drivers can get
away without it (the majority seems to have autotune set *only* if hwif->dma_base is seen as 0 in the init_hwif() method? :-/

This is really a bug and drivers get away with it because for many systems BIOS will setup the correct PIO mode and/or ->speeproc method also sets the PIO mode on the chipset while setting DMA.

In cmd64x case getting PIO setup right matters a bit more since there are many add-on cards using CMD/SiI chipsets and they can also be used on non-x86 systems (I don't think that there are non-x86 BIOS/firmwares for these cards).

Quoted 8 lines

Quoted 5 lines
The core code doesn't program PIO mode unless told to (->autotune flag == 1) so after the above change PIO mode won't be programmed et all.
I think that we now need to set ->autotune unconditionally in init_hwif_cmd64x().
No problem. This actually seems the right thing to do in all drivers,
just like the libata core does. :-)

Yes.

Thanks,
Bart

Sergei Shtylyov

author

2007-02-06 14:45 UTC #10 lore raw

PATCH 1/1 (2.6.20) cmd64x: fix PIO mode setup (take 3)

In reply to Sergei Shtylyov

The driver's tuneproc() method fails to set the drive's own speed -- fix this by renaming the function to cmd64x_tune_pio(), making it return the mode set, and "wrapping" the new tuneproc() method around it; while at it, also get rid of the non-working prefetch control code (filtering out related argument values in the "wrapper"), remove redundant PIO5 mode limitation, make cmdprintk() give more sensible mode info, and remove mention about the obsolete /proc/ interface. Get rid of the broken config_chipset_for_pio() which always tried to set PIO4, switch to always auto-tuning PIO instead.
Oh, and add the missing PIO5 support to the speedproc() method while at it. :-)

Warning: compile tested only -- getting to the real hardware isn't that easy...

Signed-off-by Sergei Shtylyov <[email protected]>

 drivers/ide/pci/cmd64x.c |  108 ++++++++++++++++++-----------------------------
 1 files changed, 43 insertions(+), 65 deletions(-)

drivers/ide/pci/cmd64x.c +43 ~~-65~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.30	Sept 10, 2002
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.41	Feb 3, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -12,6 +12,7 @@
  * Copyright (C) 1998		David S. Miller ([email protected])
  *
  * Copyright (C) 1999-2002	Andre Hedrick <[email protected]>
+ * Copyright (C) 2007		MontaVista Software, Inc. <[email protected]>
  */
 
 #include <linux/module.h>

@@ -262,43 +263,25 @@ static void program_drive_counts (ide_dr
 }
 
 /*
- * Attempts to set the interface PIO mode.
- * The preferred method of selecting PIO modes (e.g. mode 4) is 
- * "echo 'piomode:4' > /proc/ide/hdx/settings".  Special cases are
- * 8: prefetch off, 9: prefetch on, 255: auto-select best mode.
- * Called with 255 at boot time.
+ * This routine selects drive's best PIO mode, calculates setup/active/recovery
+ * counts, and then writes them into the chipset registers.
  */
-
-static void cmd64x_tuneproc (ide_drive_t *drive, u8 mode_wanted)
+static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted)
 {
 	int setup_time, active_time, recovery_time;
 	int clock_time, pio_mode, cycle_time;
 	u8 recovery_count2, cycle_count;
 	int setup_count, active_count, recovery_count;
 	int bus_speed = system_bus_clock();
-	/*byte b;*/
 	ide_pio_data_t  d;
 
-	switch (mode_wanted) {
-		case 8: /* set prefetch off */
-		case 9: /* set prefetch on */
-			mode_wanted &= 1;
-			/*set_prefetch_mode(index, mode_wanted);*/
-			cmdprintk("%s: %sabled cmd640 prefetch\n",
-				drive->name, mode_wanted ? "en" : "dis");
-			return;
-	}
-
-	mode_wanted = ide_get_best_pio_mode (drive, mode_wanted, 5, &d);
-	pio_mode = d.pio_mode;
+	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &d);
 	cycle_time = d.cycle_time;
 
 	/*
 	 * I copied all this complicated stuff from cmd640.c and made a few
 	 * minor changes.  For now I am just going to pray that it is correct.
 	 */
-	if (pio_mode > 5)
-		pio_mode = 5;
 	setup_time  = ide_pio_timings[pio_mode].setup_time;
 	active_time = ide_pio_timings[pio_mode].active_time;
 	recovery_time = cycle_time - (setup_time + active_time);

@@ -320,22 +303,33 @@ static void cmd64x_tuneproc (ide_drive_t
 	if (active_count > 16)
 		active_count = 16; /* maximum allowed by cmd646 */
 
-	/*
-	 * In a perfect world, we might set the drive pio mode here
-	 * (using WIN_SETFEATURE) before continuing.
-	 *
-	 * But we do not, because:
-	 *	1) this is the wrong place to do it
-	 *		(proper is do_special() in ide.c)
-	 * 	2) in practice this is rarely, if ever, necessary
-	 */
 	program_drive_counts (drive, setup_count, active_count, recovery_count);
 
-	cmdprintk("%s: selected cmd646 PIO mode%d : %d (%dns)%s, "
+	cmdprintk("%s: PIO mode wanted %d, selected %d (%dns)%s, "
 		"clocks=%d/%d/%d\n",
-		drive->name, pio_mode, mode_wanted, cycle_time,
+		drive->name, mode_wanted, pio_mode, cycle_time,
 		d.overridden ? " (overriding vendor mode)" : "",
 		setup_count, active_count, recovery_count);
+
+	return pio_mode;
+}
+
+/*
+ * Attempts to set drive's PIO mode.
+ * Special cases are 8: prefetch off, 9: prefetch on (both never worked),
+ * and 255: auto-select best mode (used at boot time).
+ */
+static void cmd64x_tune_drive (ide_drive_t *drive, u8 pio)
+{
+	/*
+	 * Filter out the prefetch control values
+	 * to prevent PIO5 from being programmed
+	 */
+	if (pio == 8 || pio == 9)
+		return;
+
+	pio = cmd64x_tune_pio(drive, pio);
+	(void) ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
 
 static u8 cmd64x_ratemask (ide_drive_t *drive)

@@ -387,22 +381,6 @@ static u8 cmd64x_ratemask (ide_drive_t *
 	return mode;
 }
 
-static void config_cmd64x_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-	u8 speed	= 0x00;
-	u8 set_pio	= ide_get_best_pio_mode(drive, 4, 5, NULL);
-
-	cmd64x_tuneproc(drive, set_pio);
-	speed = XFER_PIO_0 + set_pio;
-	if (set_speed)
-		(void) ide_config_drive_speed(drive, speed);
-}
-
-static void config_chipset_for_pio (ide_drive_t *drive, u8 set_speed)
-{
-	config_cmd64x_chipset_for_pio(drive, set_speed);
-}
-
 static int cmd64x_tune_chipset (ide_drive_t *drive, u8 xferspeed)
 {
 	ide_hwif_t *hwif	= HWIF(drive);

@@ -414,7 +392,7 @@ static int cmd64x_tune_chipset (ide_driv
 
 	u8 speed	= ide_rate_filter(cmd64x_ratemask(drive), xferspeed);
 
-	if (speed > XFER_PIO_4) {
+	if (speed >= XFER_SW_DMA_0) {
 		(void) pci_read_config_byte(dev, pciD, &regD);
 		(void) pci_read_config_byte(dev, pciU, &regU);
 		regD &= ~(unit ? 0x40 : 0x20);

@@ -438,17 +416,20 @@ static int cmd64x_tune_chipset (ide_driv
 		case XFER_SW_DMA_2:	regD |= (unit ? 0x40 : 0x10); break;
 		case XFER_SW_DMA_1:	regD |= (unit ? 0x80 : 0x20); break;
 		case XFER_SW_DMA_0:	regD |= (unit ? 0xC0 : 0x30); break;
-		case XFER_PIO_4:	cmd64x_tuneproc(drive, 4); break;
-		case XFER_PIO_3:	cmd64x_tuneproc(drive, 3); break;
-		case XFER_PIO_2:	cmd64x_tuneproc(drive, 2); break;
-		case XFER_PIO_1:	cmd64x_tuneproc(drive, 1); break;
-		case XFER_PIO_0:	cmd64x_tuneproc(drive, 0); break;
+		case XFER_PIO_5:
+		case XFER_PIO_4:
+		case XFER_PIO_3:
+		case XFER_PIO_2:
+		case XFER_PIO_1:
+		case XFER_PIO_0:
+			(void) cmd64x_tune_pio(drive, speed - XFER_PIO_0);
+			break;
 
 		default:
 			return 1;
 	}
 
-	if (speed > XFER_PIO_4) {
+	if (speed >= XFER_SW_DMA_0) {
 		(void) pci_write_config_byte(dev, pciU, regU);
 		regD |= (unit ? 0x40 : 0x20);
 		(void) pci_write_config_byte(dev, pciD, regD);

@@ -461,8 +442,6 @@ static int config_chipset_for_dma (ide_d
 {
 	u8 speed	= ide_dma_speed(drive, cmd64x_ratemask(drive));
 
-	config_chipset_for_pio(drive, !speed);
-
 	if (!speed)
 		return 0;

@@ -491,7 +470,7 @@ static int cmd64x_config_drive_for_dma (
 
 	} else if ((id->capability & 8) || (id->field_valid & 2)) {
 fast_ata_pio:
-		config_chipset_for_pio(drive, 1);
+		cmd64x_tune_drive(drive, 255);
 		return hwif->ide_dma_off_quietly(drive);
 	}
 	/* IORDY not supported */

@@ -694,14 +673,13 @@ static void __devinit init_hwif_cmd64x(i
 	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
 	class_rev &= 0xff;
 
-	hwif->tuneproc  = &cmd64x_tuneproc;
+	hwif->tuneproc  = &cmd64x_tune_drive;
 	hwif->speedproc = &cmd64x_tune_chipset;
 
-	if (!hwif->dma_base) {
-		hwif->drives[0].autotune = 1;
-		hwif->drives[1].autotune = 1;
+	hwif->drives[0].autotune = hwif->drives[1].autotune = 1;
+
+	if (!hwif->dma_base)
 		return;
-	}
 
 	hwif->atapi_dma = 1;

Mikael Pettersson

2007-02-06 21:12 UTC #11 lore raw

Re: [PATCH] (2.6.20) cmd64x: fix PIO mode setup (take 3)

Quoted 11 lines · Sergei Shtylyov

The driver's tuneproc() method fails to set the drive's own speed -- fix this by renaming the function to cmd64x_tune_pio(), making it return the mode set, and "wrapping" the new tuneproc() method around it; while at it, also get rid of the non-working prefetch control code (filtering out related argument values in the "wrapper"), remove redundant PIO5 mode limitation, make cmdprintk() give more sensible mode info, and remove mention about the obsolete /proc/ interface. Get rid of the broken config_chipset_for_pio() which always tried to set PIO4, switch to always auto-tuning PIO instead.
Oh, and add the missing PIO5 support to the speedproc() method while at it. :-)
Warning: compile tested only -- getting to the real hardware isn't that easy...

Worked fine on my SPARC Ultra5 with a CMD646 IDE controller.

/Mikael

Bartlomiej Zolnierkiewicz

2007-02-07 00:23 UTC #12 lore raw

Re: [PATCH] (2.6.20) cmd64x: fix PIO mode setup (take 3)

Quoted 11 lines · Mikael Pettersson

Quoted 10 lines · Sergei Shtylyov
The driver's tuneproc() method fails to set the drive's own speed -- fix this by renaming the function to cmd64x_tune_pio(), making it return the mode set, and "wrapping" the new tuneproc() method around it; while at it, also get rid of the non-working prefetch control code (filtering out related argument values in the "wrapper"), remove redundant PIO5 mode limitation, make cmdprintk() give more sensible mode info, and remove mention about the obsolete /proc/ interface. Get rid of the broken config_chipset_for_pio() which always tried to set PIO4, switch to always auto-tuning PIO instead.
Oh, and add the missing PIO5 support to the speedproc() method while at it. :-)

Quoted 3 lines

Quoted 1 line
Warning: compile tested only -- getting to the real hardware isn't that easy...
Worked fine on my SPARC Ultra5 with a CMD646 IDE controller.

Thanks! I updated the patch description accordingly.

Bart

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bartlomiej Zolnierkiewicz

Applied

2007-02-06 23:42 UTC #14 lore raw

Re: [PATCH] (2.6.20) cmd64x: fix PIO mode setup (take 3)

Quoted 11 lines · Sergei Shtylyov

The driver's tuneproc() method fails to set the drive's own speed -- fix this by renaming the function to cmd64x_tune_pio(), making it return the mode set, and "wrapping" the new tuneproc() method around it; while at it, also get rid of the non-working prefetch control code (filtering out related argument values in the "wrapper"), remove redundant PIO5 mode limitation, make cmdprintk() give more sensible mode info, and remove mention about the obsolete /proc/ interface. Get rid of the broken config_chipset_for_pio() which always tried to set PIO4, switch to always auto-tuning PIO instead.
Oh, and add the missing PIO5 support to the speedproc() method while at it. :-)

Quoted 3 lines

Warning: compile tested only -- getting to the real hardware isn't that easy...
Signed-off-by Sergei Shtylyov <[email protected]>

thanks for updating the patch

applied

Sergei Shtylyov

author

2007-02-09 22:29 UTC #15 lore raw

PATCH 1/1 (pata-2.6 fix queue) cmd64x: fix recovery time calculation

In reply to Sergei Shtylyov

The driver wrongly takes the address setup time into account when calculating the PIO recovery time -- this leads to slight overclocking of the PIO modes 0 and 1 (so, the prayers failed to help, as usual :-). Rework the code to be calculating recovery clock count as a difference between the total cycle count and the active count (we don't need to calculate the recovery time itself since it's not specified for the PIO modes 0 to 2, and for modes 3 and 4 this formula gives enough recovery time anyway in the chip's supported PCI frequency range).

This patch has been inspired by reading the datasheets and looking at what the libata driver does; it has been compile-tested only (as usual :-) but anyway, the new code gives the same or longer recovery times than the old one...

Signed-off-by Sergei Shtylyov <[email protected]>

 drivers/ide/pci/cmd64x.c |   47 +++++++++++++++++++++--------------------------
 1 files changed, 21 insertions(+), 26 deletions(-)

drivers/ide/pci/cmd64x.c +21 ~~-26~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.42	Feb 6, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.43	Feb 8, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -262,46 +262,41 @@ static void program_drive_counts (ide_dr
 	local_irq_restore(flags);
 }
 
+static u8 quantize_timing(int timing, int quant)
+{
+	return (timing + quant - 1) / quant;
+}
+
 /*
  * This routine selects drive's best PIO mode, calculates setup/active/recovery
  * counts, and then writes them into the chipset registers.
  */
 static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted)
 {
-	int setup_time, active_time, recovery_time;
-	int clock_time, pio_mode, cycle_time;
-	u8 recovery_count2, cycle_count;
-	int setup_count, active_count, recovery_count;
-	int bus_speed = system_bus_clock();
-	ide_pio_data_t  d;
+	int setup_time, active_time, cycle_time;
+	u8  cycle_count, setup_count, active_count, recovery_count;
+	u8  pio_mode;
+	int clock_time = 1000 / system_bus_clock();
+	ide_pio_data_t pio;
 
-	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &d);
-	cycle_time = d.cycle_time;
+	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &pio);
+	cycle_time = pio.cycle_time;
 
-	/*
-	 * I copied all this complicated stuff from cmd640.c and made a few
-	 * minor changes.  For now I am just going to pray that it is correct.
-	 */
 	setup_time  = ide_pio_timings[pio_mode].setup_time;
 	active_time = ide_pio_timings[pio_mode].active_time;
-	recovery_time = cycle_time - (setup_time + active_time);
-	clock_time = 1000 / bus_speed;
-	cycle_count = (cycle_time + clock_time - 1) / clock_time;
-
-	setup_count = (setup_time + clock_time - 1) / clock_time;
-
-	active_count = (active_time + clock_time - 1) / clock_time;
-
-	recovery_count = (recovery_time + clock_time - 1) / clock_time;
-	recovery_count2 = cycle_count - (setup_count + active_count);
-	if (recovery_count2 > recovery_count)
-		recovery_count = recovery_count2;
+
+	setup_count  = quantize_timing( setup_time, clock_time);
+	cycle_count  = quantize_timing( cycle_time, clock_time);
+	active_count = quantize_timing(active_time, clock_time);
+
+	recovery_count = cycle_count - active_count;
+	/* program_drive_counts() takes care of zero recovery cycles */
 	if (recovery_count > 16) {
 		active_count += recovery_count - 16;
 		recovery_count = 16;
 	}
 	if (active_count > 16)
-		active_count = 16; /* maximum allowed by cmd646 */
+		active_count = 16; /* maximum allowed by cmd64x */
 
 	program_drive_counts (drive, setup_count, active_count, recovery_count);

Bartlomiej Zolnierkiewicz

Applied

2007-02-10 00:09 UTC #16 lore raw

Re: [PATCH] (pata-2.6 fix queue) cmd64x: fix recovery time calculation

Quoted 8 lines · Sergei Shtylyov

The driver wrongly takes the address setup time into account when calculating the PIO recovery time -- this leads to slight overclocking of the PIO modes 0 and 1 (so, the prayers failed to help, as usual :-). Rework the code to be calculating recovery clock count as a difference between the total cycle count and the active count (we don't need to calculate the recovery time itself since it's not specified for the PIO modes 0 to 2, and for modes 3 and 4 this formula gives enough recovery time anyway in the chip's supported PCI frequency range).

Quoted 5 lines

This patch has been inspired by reading the datasheets and looking at what the libata driver does; it has been compile-tested only (as usual :-) but anyway, the new code gives the same or longer recovery times than the old one...
Signed-off-by Sergei Shtylyov <[email protected]>

applied

Sergei Shtylyov

author

2007-02-14 21:42 UTC #17 lore raw

PATCH 1/1 (pata-2.6 fix queue) cmd64x: interrupt status fixes

In reply to Sergei Shtylyov

The driver's ide_dma_test_irq() method was reading the MRDMODE register even on PCI0643/6 where it was write-only -- fix this by always reading the "backward- compatible" interrupt bits, renaming dma_alt_stat to irq_stat as these interrupt status bits are not coupled to DMA.
In addition, wrong interrupt bit was tested/cleared for the primary channel -- it's bit 2 in all the chip specs and the driver used bit 1... :-/

---
Warning: as usual, this has only been compile-tested. :-)

 drivers/ide/pci/cmd64x.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

drivers/ide/pci/cmd64x.c +12 ~~-12~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.43	Feb 8, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.44	Feb 13, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -39,7 +39,7 @@
  * CMD64x specific registers definition.
  */
 #define CFR		0x50
-#define   CFR_INTR_CH0		0x02
+#define   CFR_INTR_CH0		0x04
 #define CNTRL		0x51
 #define	  CNTRL_DIS_RA0		0x40
 #define   CNTRL_DIS_RA1		0x80

@@ -510,19 +510,19 @@ static int cmd64x_ide_dma_end (ide_drive
 
 static int cmd64x_ide_dma_test_irq (ide_drive_t *drive)
 {
-	ide_hwif_t *hwif		= HWIF(drive);
-	struct pci_dev *dev		= hwif->pci_dev;
-        u8 dma_alt_stat = 0, mask	= (hwif->channel) ? MRDMODE_INTR_CH1 :
-							    MRDMODE_INTR_CH0;
-	u8 dma_stat = hwif->INB(hwif->dma_status);
+	ide_hwif_t *hwif	= HWIF(drive);
+	struct pci_dev *dev	= hwif->pci_dev;
+	u8 irq_reg		= hwif->channel ? ARTTIM23 : CFR;
+	u8 irq_stat = 0, mask	= hwif->channel ? ARTTIM23_INTR_CH1 : CFR_INTR_CH0;
+	u8 dma_stat		= hwif->INB(hwif->dma_status);
+
+	(void) pci_read_config_byte(dev, irq_reg, &irq_stat);
 
-	(void) pci_read_config_byte(dev, MRDMODE, &dma_alt_stat);
 #ifdef DEBUG
-	printk("%s: dma_stat: 0x%02x dma_alt_stat: "
-		"0x%02x mask: 0x%02x\n", drive->name,
-		dma_stat, dma_alt_stat, mask);
+	printk("%s: dma_stat: 0x%02x irq_stat: 0x%02x mask: 0x%02x\n",
+	       drive->name, dma_stat, irq_stat, mask);
 #endif
-	if (!(dma_alt_stat & mask))
+	if (!(irq_stat & mask))
 		return 0;
 
 	/* return 1 if INTR asserted */

Sergei Shtylyov

author

2007-02-14 22:35 UTC #18 lore raw

PATCH 1/1 (pata-2.6 fix queue) cmd64x: add/fix enablebits

In reply to Sergei Shtylyov

The IDE core looks at the wrong bit when checking if the secondary channel is enabled on PCI0646 -- CFR bit 8 is read-ahead disable, bit 3 is the correct one. Starting with PCI0646U chip, the primary channel can also be enbled/disabled -- so, add 'enablebits' initializers to each 'ide_pci_device_t' structure, handling the original PCI0646 via adding the init_setup() method and clearing the 'reg' field there if necessary...

Signed-off-by Sergei Shtylyov <[email protected]>

---
Warning: compile-tested only, use on your own risk! ;-)

 drivers/ide/pci/cmd64x.c |   39 ++++++++++++++++++++++++++++++++++++---
 1 files changed, 36 insertions(+), 3 deletions(-)

drivers/ide/pci/cmd64x.c +36 -3

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.44	Feb 13, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.45	Feb 14, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -705,42 +705,75 @@ static void __devinit init_hwif_cmd64x(i
 	hwif->drives[1].autodma = hwif->autodma;
 }
 
+static int __devinit init_setup_cmd64x(struct pci_dev *dev, ide_pci_device_t *d)
+{
+	return ide_setup_pci_device(dev, d);
+}
+
+static int __devinit init_setup_cmd646(struct pci_dev *dev, ide_pci_device_t *d)
+{
+	u8 rev = 0;
+
+	/*
+	 * The original PCI0646 didn't have the primary channel enable bit,
+	 * it appeared starting with PCI0646U (i.e. revision ID 3).
+	 */
+	pci_read_config_byte(dev, PCI_REVISION_ID, &rev);
+	if (rev < 3)
+		d->enablebits[0].reg = 0;
+
+	return ide_setup_pci_device(dev, d);
+}
+
 static ide_pci_device_t cmd64x_chipsets[] __devinitdata = {
 	{	/* 0 */
 		.name		= "CMD643",
+		.init_setup	= init_setup_cmd64x,
 		.init_chipset	= init_chipset_cmd64x,
 		.init_hwif	= init_hwif_cmd64x,
 		.channels	= 2,
 		.autodma	= AUTODMA,
+		.enablebits	= {{0x00,0x00,0x00}, {0x51,0x08,0x08}},
 		.bootable	= ON_BOARD,
 	},{	/* 1 */
 		.name		= "CMD646",
+		.init_setup	= init_setup_cmd646,
 		.init_chipset	= init_chipset_cmd64x,
 		.init_hwif	= init_hwif_cmd64x,
 		.channels	= 2,
 		.autodma	= AUTODMA,
-		.enablebits	= {{0x00,0x00,0x00}, {0x51,0x80,0x80}},
+		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
 		.bootable	= ON_BOARD,
 	},{	/* 2 */
 		.name		= "CMD648",
+		.init_setup	= init_setup_cmd64x,
 		.init_chipset	= init_chipset_cmd64x,
 		.init_hwif	= init_hwif_cmd64x,
 		.channels	= 2,
 		.autodma	= AUTODMA,
+		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
 		.bootable	= ON_BOARD,
 	},{	/* 3 */
 		.name		= "CMD649",
+		.init_setup	= init_setup_cmd64x,
 		.init_chipset	= init_chipset_cmd64x,
 		.init_hwif	= init_hwif_cmd64x,
 		.channels	= 2,
 		.autodma	= AUTODMA,
+		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
 		.bootable	= ON_BOARD,
 	}
 };
 
+/*
+ * We may have to modify enablebits for PCI0646, so we'd better pass
+ * a local copy of the ide_pci_device_t structure down the call chain...
+ */
 static int __devinit cmd64x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	return ide_setup_pci_device(dev, &cmd64x_chipsets[id->driver_data]);
+	ide_pci_device_t d = cmd64x_chipsets[id->driver_data];
+
+	return d.init_setup(dev, &d);
 }
 
 static struct pci_device_id cmd64x_pci_tbl[] = {

Bartlomiej Zolnierkiewicz

Applied

2007-02-19 23:03 UTC #19 lore raw

Re: [PATCH] (pata-2.6 fix queue) cmd64x: add/fix enablebits

Quoted 2 lines · Sergei Shtylyov

The IDE core looks at the wrong bit when checking if the secondary channel is enabled on PCI0646 -- CFR bit 8 is read-ahead disable, bit 3 is the correct one.

I guess that you meant CNTRL here?

[ I corrected this in the applied patch ]

Quoted 6 lines

Starting with PCI0646U chip, the primary channel can also be enbled/disabled -- so, add 'enablebits' initializers to each 'ide_pci_device_t' structure, handling the original PCI0646 via adding the init_setup() method and clearing the 'reg' field there if necessary...
Signed-off-by Sergei Shtylyov <[email protected]>

applied

Sergei Shtylyov

author

2007-02-20 14:28 UTC #20 lore raw

Re: [PATCH] (pata-2.6 fix queue) cmd64x: add/fix enablebits

Hello.

Quoted 4 lines · Bartlomiej Zolnierkiewicz

Quoted 2 lines
The IDE core looks at the wrong bit when checking if the secondary channel is enabled on PCI0646 -- CFR bit 8 is read-ahead disable, bit 3 is the correct one.
I guess that you meant CNTRL here?

Yeah, and bit 7. :-<

Quoted 10 lines

[ I corrected this in the applied patch ]
Quoted 6 lines
Starting with PCI0646U chip, the primary channel can also be enbled/disabled -- so, add 'enablebits' initializers to each 'ide_pci_device_t' structure, handling the original PCI0646 via adding the init_setup() method and clearing the 'reg' field there if necessary...
Signed-off-by Sergei Shtylyov <[email protected]>
applied

Thanks!

MBR, Sergei

Bartlomiej Zolnierkiewicz

2007-02-23 20:05 UTC #21 lore raw

Re: [PATCH] (pata-2.6 fix queue) cmd64x: add/fix enablebits

Quoted 10 lines · Sergei Shtylyov

Hello.
Quoted 4 lines · Bartlomiej Zolnierkiewicz
Quoted 2 lines
The IDE core looks at the wrong bit when checking if the secondary channel is enabled on PCI0646 -- CFR bit 8 is read-ahead disable, bit 3 is the correct one.
I guess that you meant CNTRL here?
    Yeah, and bit 7. :-<

fixed, thanks

Sergei Shtylyov

author

2007-04-14 19:31 UTC #22 lore raw

PATCH 1/1 cmd64x: add/fix enablebits (take 2)

In reply to Sergei Shtylyov

The IDE core looks at the wrong bit when checking if the secondary channel is enabled on PCI0646 -- CNTRL register bit 7 is read-ahead disable, bit 3 is the correct one.
Starting with PCI0646U chip, the primary channel can also be enabled/disabled -- so, add 'enablebits' initializers to each 'ide_pci_device_t' structure, handling the original PCI0646 via adding the init_setup() method and clearing the 'reg' field there if necessary...

Signed-off-by Sergei Shtylyov <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
This is an update due to patch order changes. Has also been tested on PCI-649.

 drivers/ide/pci/cmd64x.c |   39 ++++++++++++++++++++++++++++++++++++---
 1 files changed, 36 insertions(+), 3 deletions(-)

drivers/ide/pci/cmd64x.c +36 -3

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/cmd64x.c		Version 1.44	Mar 12, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.45	Mar 14, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Due to massive hardware bugs, UltraDMA is only supported

@@ -684,42 +684,75 @@ static void __devinit init_hwif_cmd64x(i
 	hwif->drives[1].autodma = hwif->autodma;
 }
 
+static int __devinit init_setup_cmd64x(struct pci_dev *dev, ide_pci_device_t *d)
+{
+	return ide_setup_pci_device(dev, d);
+}
+
+static int __devinit init_setup_cmd646(struct pci_dev *dev, ide_pci_device_t *d)
+{
+	u8 rev = 0;
+
+	/*
+	 * The original PCI0646 didn't have the primary channel enable bit,
+	 * it appeared starting with PCI0646U (i.e. revision ID 3).
+	 */
+	pci_read_config_byte(dev, PCI_REVISION_ID, &rev);
+	if (rev < 3)
+		d->enablebits[0].reg = 0;
+
+	return ide_setup_pci_device(dev, d);
+}
+
 static ide_pci_device_t cmd64x_chipsets[] __devinitdata = {
 	{	/* 0 */
 		.name		= "CMD643",
+		.init_setup	= init_setup_cmd64x,
 		.init_chipset	= init_chipset_cmd64x,
 		.init_hwif	= init_hwif_cmd64x,
 		.channels	= 2,
 		.autodma	= AUTODMA,
+		.enablebits	= {{0x00,0x00,0x00}, {0x51,0x08,0x08}},
 		.bootable	= ON_BOARD,
 	},{	/* 1 */
 		.name		= "CMD646",
+		.init_setup	= init_setup_cmd646,
 		.init_chipset	= init_chipset_cmd64x,
 		.init_hwif	= init_hwif_cmd64x,
 		.channels	= 2,
 		.autodma	= AUTODMA,
-		.enablebits	= {{0x00,0x00,0x00}, {0x51,0x80,0x80}},
+		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
 		.bootable	= ON_BOARD,
 	},{	/* 2 */
 		.name		= "CMD648",
+		.init_setup	= init_setup_cmd64x,
 		.init_chipset	= init_chipset_cmd64x,
 		.init_hwif	= init_hwif_cmd64x,
 		.channels	= 2,
 		.autodma	= AUTODMA,
+		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
 		.bootable	= ON_BOARD,
 	},{	/* 3 */
 		.name		= "CMD649",
+		.init_setup	= init_setup_cmd64x,
 		.init_chipset	= init_chipset_cmd64x,
 		.init_hwif	= init_hwif_cmd64x,
 		.channels	= 2,
 		.autodma	= AUTODMA,
+		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
 		.bootable	= ON_BOARD,
 	}
 };
 
+/*
+ * We may have to modify enablebits for PCI0646, so we'd better pass
+ * a local copy of the ide_pci_device_t structure down the call chain...
+ */
 static int __devinit cmd64x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	return ide_setup_pci_device(dev, &cmd64x_chipsets[id->driver_data]);
+	ide_pci_device_t d = cmd64x_chipsets[id->driver_data];
+
+	return d.init_setup(dev, &d);
 }
 
 static struct pci_device_id cmd64x_pci_tbl[] = {

Bartlomiej Zolnierkiewicz

Applied

2007-04-23 22:30 UTC #23 lore raw

Re: [PATCH pata-2.6] cmd64x: add/fix enablebits (take 2)

Quoted 8 lines · Sergei Shtylyov

The IDE core looks at the wrong bit when checking if the secondary channel is enabled on PCI0646 -- CNTRL register bit 7 is read-ahead disable, bit 3 is the correct one.
Starting with PCI0646U chip, the primary channel can also be enabled/disabled -- so, add 'enablebits' initializers to each 'ide_pci_device_t' structure, handling the original PCI0646 via adding the init_setup() method and clearing the 'reg' field there if necessary...

Quoted 5 lines

Signed-off-by Sergei Shtylyov <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
This is an update due to patch order changes. Has also been tested on PCI-649.

applied

Sergei Shtylyov

author

2007-02-15 13:53 UTC #24 lore raw

PATCH 1/1 (pata-2.6 fix queue) cmd64x: interrupt status fixes (resend)

In reply to Sergei Shtylyov

Signed-off-by Sergei Shtylyov <[email protected]>

---
Warning: as usual, this has only been compile-tested. :-) Grrr, lost signoff somewhere again, so resending...

 drivers/ide/pci/cmd64x.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

drivers/ide/pci/cmd64x.c +12 ~~-12~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.43	Feb 8, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.44	Feb 13, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -39,7 +39,7 @@
  * CMD64x specific registers definition.
  */
 #define CFR		0x50
-#define   CFR_INTR_CH0		0x02
+#define   CFR_INTR_CH0		0x04
 #define CNTRL		0x51
 #define	  CNTRL_DIS_RA0		0x40
 #define   CNTRL_DIS_RA1		0x80

@@ -510,19 +510,19 @@ static int cmd64x_ide_dma_end (ide_drive
 
 static int cmd64x_ide_dma_test_irq (ide_drive_t *drive)
 {
-	ide_hwif_t *hwif		= HWIF(drive);
-	struct pci_dev *dev		= hwif->pci_dev;
-        u8 dma_alt_stat = 0, mask	= (hwif->channel) ? MRDMODE_INTR_CH1 :
-							    MRDMODE_INTR_CH0;
-	u8 dma_stat = hwif->INB(hwif->dma_status);
+	ide_hwif_t *hwif	= HWIF(drive);
+	struct pci_dev *dev	= hwif->pci_dev;
+	u8 irq_reg		= hwif->channel ? ARTTIM23 : CFR;
+	u8 irq_stat = 0, mask	= hwif->channel ? ARTTIM23_INTR_CH1 : CFR_INTR_CH0;
+	u8 dma_stat		= hwif->INB(hwif->dma_status);
+
+	(void) pci_read_config_byte(dev, irq_reg, &irq_stat);
 
-	(void) pci_read_config_byte(dev, MRDMODE, &dma_alt_stat);
 #ifdef DEBUG
-	printk("%s: dma_stat: 0x%02x dma_alt_stat: "
-		"0x%02x mask: 0x%02x\n", drive->name,
-		dma_stat, dma_alt_stat, mask);
+	printk("%s: dma_stat: 0x%02x irq_stat: 0x%02x mask: 0x%02x\n",
+	       drive->name, dma_stat, irq_stat, mask);
 #endif
-	if (!(dma_alt_stat & mask))
+	if (!(irq_stat & mask))
 		return 0;
 
 	/* return 1 if INTR asserted */

Bartlomiej Zolnierkiewicz

Applied

2007-02-19 22:58 UTC #25 lore raw

Re: [PATCH] (pata-2.6 fix queue) cmd64x: interrupt status fixes (resend)

Quoted 8 lines · Sergei Shtylyov

The driver's ide_dma_test_irq() method was reading the MRDMODE register even on PCI0643/6 where it was write-only -- fix this by always reading the "backward- compatible" interrupt bits, renaming dma_alt_stat to irq_stat as these interrupt status bits are not coupled to DMA.
In addition, wrong interrupt bit was tested/cleared for the primary channel -- it's bit 2 in all the chip specs and the driver used bit 1... :-/
Signed-off-by Sergei Shtylyov <[email protected]>

applied

Sergei Shtylyov

author

2007-04-14 19:16 UTC #26 lore raw

PATCH 1/1 cmd64x: interrupt status fixes (take 2)

In reply to Sergei Shtylyov

The driver's ide_dma_test_irq() method was reading the MRDMODE register even on PCI0643/6 where it was write-only -- fix this by always reading the "backward- compatible" interrupt bits, renaming dma_alt_stat to irq_stat as the interrupt status bits are not coupled to DMA.
In addition, wrong interrupt bit was tested/cleared for the primary channel -- it's bit 2 in all the chip specs and the driver used bit 1... :-/

Signed-off-by Sergei Shtylyov <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
This is an update due to patch order changes. Has also been tested on PCI-649.

 drivers/ide/pci/cmd64x.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

drivers/ide/pci/cmd64x.c +12 ~~-12~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/cmd64x.c		Version 1.43	Mar 10, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.44	Mar 12, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Due to massive hardware bugs, UltraDMA is only supported

@@ -36,7 +36,7 @@
  * CMD64x specific registers definition.
  */
 #define CFR		0x50
-#define   CFR_INTR_CH0		0x02
+#define   CFR_INTR_CH0		0x04
 #define CNTRL		0x51
 #define	  CNTRL_DIS_RA0		0x40
 #define   CNTRL_DIS_RA1		0x80

@@ -488,19 +488,19 @@ static int cmd64x_ide_dma_end (ide_drive
 
 static int cmd64x_ide_dma_test_irq (ide_drive_t *drive)
 {
-	ide_hwif_t *hwif		= HWIF(drive);
-	struct pci_dev *dev		= hwif->pci_dev;
-        u8 dma_alt_stat = 0, mask	= (hwif->channel) ? MRDMODE_INTR_CH1 :
-							    MRDMODE_INTR_CH0;
-	u8 dma_stat = inb(hwif->dma_status);
+	ide_hwif_t *hwif	= HWIF(drive);
+	struct pci_dev *dev	= hwif->pci_dev;
+	u8 irq_reg		= hwif->channel ? ARTTIM23 : CFR;
+	u8 irq_stat = 0, mask	= hwif->channel ? ARTTIM23_INTR_CH1 : CFR_INTR_CH0;
+	u8 dma_stat		= inb(hwif->dma_status);
+
+	(void) pci_read_config_byte(dev, irq_reg, &irq_stat);
 
-	(void) pci_read_config_byte(dev, MRDMODE, &dma_alt_stat);
 #ifdef DEBUG
-	printk("%s: dma_stat: 0x%02x dma_alt_stat: "
-		"0x%02x mask: 0x%02x\n", drive->name,
-		dma_stat, dma_alt_stat, mask);
+	printk("%s: dma_stat: 0x%02x irq_stat: 0x%02x mask: 0x%02x\n",
+	       drive->name, dma_stat, irq_stat, mask);
 #endif
-	if (!(dma_alt_stat & mask))
+	if (!(irq_stat & mask))
 		return 0;
 
 	/* return 1 if INTR asserted */

Bartlomiej Zolnierkiewicz

Applied

2007-04-23 22:30 UTC #27 lore raw

Re: [PATCH pata-2.6] cmd64x: interrupt status fixes (take 2)

Quoted 7 lines · Sergei Shtylyov

The driver's ide_dma_test_irq() method was reading the MRDMODE register even on PCI0643/6 where it was write-only -- fix this by always reading the "backward- compatible" interrupt bits, renaming dma_alt_stat to irq_stat as the interrupt status bits are not coupled to DMA.
In addition, wrong interrupt bit was tested/cleared for the primary channel -- it's bit 2 in all the chip specs and the driver used bit 1... :-/

Quoted 5 lines

Signed-off-by Sergei Shtylyov <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
This is an update due to patch order changes. Has also been tested on PCI-649.

applied

Sergei Shtylyov

author

2007-02-15 19:17 UTC #28 lore raw

PATCH 1/1 (pata-2.6 fix queue) cmd64x: procfs code fixes/cleanups

In reply to Sergei Shtylyov

Fix several issues with the driver's procfs output:

- when testing if channel is enabled, the code looks at the "simplex" bits, not at the real enable bits -- add #define for the primary channel enable bit;

- UltraDMA modes 0, 1, 3 for slave drive reported incorrectly due to using the master drive's clock cycle resolution bit.

While at it, also perform some cleanups:

- don't read from the registers which are not used for dump;

- correct the chipset names (from CMDxxx to PCI-xxx);

- rework UltraDMA mode dump code;

- remove PIO mode dump code that has never been finished;

- remove the duplicate interrupt status dump (the MRDMODE register bits mirror those in the CFR and ARTTIM23 registers);

- add spaces in the ?: operators...

Signed-off-by Sergei Shtylyov <[email protected]>

---
Warning: as usual, this has only been compile-tested. :-)

 drivers/ide/pci/cmd64x.c |   99 ++++++++++++++++++-----------------------------
 1 files changed, 39 insertions(+), 60 deletions(-)

drivers/ide/pci/cmd64x.c +39 ~~-60~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.45	Feb 14, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.46	Feb 15, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -41,9 +41,10 @@
 #define CFR		0x50
 #define   CFR_INTR_CH0		0x04
 #define CNTRL		0x51
-#define	  CNTRL_DIS_RA0		0x40
-#define   CNTRL_DIS_RA1		0x80
-#define	  CNTRL_ENA_2ND		0x08
+#define   CNTRL_ENA_1ST 	0x04
+#define   CNTRL_ENA_2ND 	0x08
+#define   CNTRL_DIS_RA0 	0x40
+#define   CNTRL_DIS_RA1 	0x80
 
 #define	CMDTIM		0x52
 #define	ARTTIM0		0x53

@@ -90,23 +91,15 @@ static int n_cmd_devs;
 static char * print_cmd64x_get_info (char *buf, struct pci_dev *dev, int index)
 {
 	char *p = buf;
-
-	u8 reg53 = 0, reg54 = 0, reg55 = 0, reg56 = 0;	/* primary */
-	u8 reg57 = 0, reg58 = 0, reg5b;			/* secondary */
 	u8 reg72 = 0, reg73 = 0;			/* primary */
 	u8 reg7a = 0, reg7b = 0;			/* secondary */
-	u8 reg50 = 0, reg71 = 0;			/* extra */
+	u8 reg50 = 1, reg51 = 1, reg57 = 0, reg71 = 0;	/* extra */
 
 	p += sprintf(p, "\nController: %d\n", index);
-	p += sprintf(p, "CMD%x Chipset.\n", dev->device);
+	p += sprintf(p, "PCI-%x Chipset.\n", dev->device);
 	(void) pci_read_config_byte(dev, CFR,       &reg50);
-	(void) pci_read_config_byte(dev, ARTTIM0,   &reg53);
-	(void) pci_read_config_byte(dev, DRWTIM0,   &reg54);
-	(void) pci_read_config_byte(dev, ARTTIM1,   &reg55);
-	(void) pci_read_config_byte(dev, DRWTIM1,   &reg56);
-	(void) pci_read_config_byte(dev, ARTTIM2,   &reg57);
-	(void) pci_read_config_byte(dev, DRWTIM2,   &reg58);
-	(void) pci_read_config_byte(dev, DRWTIM3,   &reg5b);
+	(void) pci_read_config_byte(dev, CNTRL,     &reg51);
+	(void) pci_read_config_byte(dev, ARTTIM23,  &reg57);
 	(void) pci_read_config_byte(dev, MRDMODE,   &reg71);
 	(void) pci_read_config_byte(dev, BMIDESR0,  &reg72);
 	(void) pci_read_config_byte(dev, UDIDETCR0, &reg73);

@@ -116,57 +109,43 @@ static char * print_cmd64x_get_info (cha
 	p += sprintf(p, "--------------- Primary Channel "
 			"---------------- Secondary Channel "
 			"-------------\n");
-	p += sprintf(p, "                %sabled           "
-			"              %sabled\n",
-		(reg72&0x80)?"dis":" en",
-		(reg7a&0x80)?"dis":" en");
+	p += sprintf(p, "                %sabled         "
+			"                %sabled\n",
+		(reg51 & CNTRL_ENA_1ST) ? "dis" : " en",
+		(reg51 & CNTRL_ENA_2ND) ? "dis" : " en");
 	p += sprintf(p, "--------------- drive0 "
 		"--------- drive1 -------- drive0 "
 		"---------- drive1 ------\n");
 	p += sprintf(p, "DMA enabled:    %s              %s"
 			"             %s               %s\n",
-		(reg72&0x20)?"yes":"no ", (reg72&0x40)?"yes":"no ",
-		(reg7a&0x20)?"yes":"no ", (reg7a&0x40)?"yes":"no ");
+		(reg72 & 0x20) ? "yes" : "no ", (reg72 & 0x40)? "yes" : "no ",
+		(reg7a & 0x20) ? "yes" : "no ", (reg7a & 0x40)? "yes" : "no ");
 
-	p += sprintf(p, "DMA Mode:       %s(%s)          %s(%s)",
-		(reg72&0x20)?((reg73&0x01)?"UDMA":" DMA"):" PIO",
-		(reg72&0x20)?(
-			((reg73&0x30)==0x30)?(((reg73&0x35)==0x35)?"3":"0"):
-			((reg73&0x20)==0x20)?(((reg73&0x25)==0x25)?"3":"1"):
-			((reg73&0x10)==0x10)?(((reg73&0x15)==0x15)?"4":"2"):
-			((reg73&0x00)==0x00)?(((reg73&0x05)==0x05)?"5":"2"):
-			"X"):"?",
-		(reg72&0x40)?((reg73&0x02)?"UDMA":" DMA"):" PIO",
-		(reg72&0x40)?(
-			((reg73&0xC0)==0xC0)?(((reg73&0xC5)==0xC5)?"3":"0"):
-			((reg73&0x80)==0x80)?(((reg73&0x85)==0x85)?"3":"1"):
-			((reg73&0x40)==0x40)?(((reg73&0x4A)==0x4A)?"4":"2"):
-			((reg73&0x00)==0x00)?(((reg73&0x0A)==0x0A)?"5":"2"):
-			"X"):"?");
-	p += sprintf(p, "         %s(%s)           %s(%s)\n",
-		(reg7a&0x20)?((reg7b&0x01)?"UDMA":" DMA"):" PIO",
-		(reg7a&0x20)?(
-			((reg7b&0x30)==0x30)?(((reg7b&0x35)==0x35)?"3":"0"):
-			((reg7b&0x20)==0x20)?(((reg7b&0x25)==0x25)?"3":"1"):
-			((reg7b&0x10)==0x10)?(((reg7b&0x15)==0x15)?"4":"2"):
-			((reg7b&0x00)==0x00)?(((reg7b&0x05)==0x05)?"5":"2"):
-			"X"):"?",
-		(reg7a&0x40)?((reg7b&0x02)?"UDMA":" DMA"):" PIO",
-		(reg7a&0x40)?(
-			((reg7b&0xC0)==0xC0)?(((reg7b&0xC5)==0xC5)?"3":"0"):
-			((reg7b&0x80)==0x80)?(((reg7b&0x85)==0x85)?"3":"1"):
-			((reg7b&0x40)==0x40)?(((reg7b&0x4A)==0x4A)?"4":"2"):
-			((reg7b&0x00)==0x00)?(((reg7b&0x0A)==0x0A)?"5":"2"):
-			"X"):"?" );
-	p += sprintf(p, "PIO Mode:       %s                %s"
-			"               %s                 %s\n",
-			"?", "?", "?", "?");
-	p += sprintf(p, "                %s                     %s\n",
-		(reg50 & CFR_INTR_CH0) ? "interrupting" : "polling     ",
-		(reg57 & ARTTIM23_INTR_CH1) ? "interrupting" : "polling");
+	p += sprintf(p, "UDMA Mode:     %s (%c)         %s (%c)",
+		( reg73 & 0x01) ? " on" : "off",
+		((reg73 & 0x30) == 0x30) ? ((reg73 & 0x04) ? '3' : '0') :
+		((reg73 & 0x30) == 0x20) ? ((reg73 & 0x04) ? '3' : '1') :
+		((reg73 & 0x30) == 0x10) ? ((reg73 & 0x04) ? '4' : '2') :
+		((reg73 & 0x30) == 0x00) ? ((reg73 & 0x04) ? '5' : '2') : '?',
+		( reg73 & 0x02) ? " on" : "off",
+		((reg73 & 0xC0) == 0xC0) ? ((reg73 & 0x08) ? '3' : '0') :
+		((reg73 & 0xC0) == 0x80) ? ((reg73 & 0x08) ? '3' : '1') :
+		((reg73 & 0xC0) == 0x40) ? ((reg73 & 0x08) ? '4' : '2') :
+		((reg73 & 0xC0) == 0x00) ? ((reg73 & 0x08) ? '5' : '2') : '?');
+	p += sprintf(p, "        %s (%c)          %s (%c)\n",
+		( reg7b & 0x01) ? " on" : "off",
+		((reg7b & 0x30) == 0x30) ? ((reg7b & 0x04) ? '3' : '0') :
+		((reg7b & 0x30) == 0x20) ? ((reg7b & 0x04) ? '3' : '1') :
+		((reg7b & 0x30) == 0x10) ? ((reg7b & 0x04) ? '4' : '2') :
+		((reg7b & 0x30) == 0x00) ? ((reg7b & 0x04) ? '5' : '2') : '?',
+		( reg7b & 0x02) ? " on" : "off",
+		((reg7b & 0xC0) == 0xC0) ? ((reg7b & 0x08) ? '3' : '0'):
+		((reg7b & 0xC0) == 0x80) ? ((reg7b & 0x08) ? '3' : '1'):
+		((reg7b & 0xC0) == 0x40) ? ((reg7b & 0x08) ? '4' : '2'):
+		((reg7b & 0xC0) == 0x00) ? ((reg7b & 0x08) ? '5' : '2'): '?');
 	p += sprintf(p, "                %s                          %s\n",
-		(reg71 & MRDMODE_INTR_CH0) ? "pending" : "clear  ",
-		(reg71 & MRDMODE_INTR_CH1) ? "pending" : "clear");
+		(reg50 & CFR_INTR_CH0	  ) ? "pending" : "clear  ",
+		(reg57 & ARTTIM23_INTR_CH1) ? "pending" : "clear  ");
 	p += sprintf(p, "                %s                          %s\n",
 		(reg71 & MRDMODE_BLK_CH0) ? "blocked" : "enabled",
 		(reg71 & MRDMODE_BLK_CH1) ? "blocked" : "enabled");

Bartlomiej Zolnierkiewicz

Applied

2007-02-19 23:04 UTC #29 lore raw

Re: [PATCH] (pata-2.6 fix queue) cmd64x: procfs code fixes/cleanups

Quoted 16 lines · Sergei Shtylyov

Fix several issues with the driver's procfs output:
- when testing if channel is enabled, the code looks at the "simplex" bits, not at the real enable bits -- add #define for the primary channel enable bit;
- UltraDMA modes 0, 1, 3 for slave drive reported incorrectly due to using the master drive's clock cycle resolution bit.
While at it, also perform some cleanups:
- don't read from the registers which are not used for dump;
- correct the chipset names (from CMDxxx to PCI-xxx);
- rework UltraDMA mode dump code;

Quoted 8 lines

- remove PIO mode dump code that has never been finished;
- remove the duplicate interrupt status dump (the MRDMODE register bits mirror those in the CFR and ARTTIM23 registers);
- add spaces in the ?: operators...
Signed-off-by Sergei Shtylyov <[email protected]>

applied

Sergei Shtylyov

author

2007-04-14 19:40 UTC #30 lore raw

PATCH 1/1 cmd64x: procfs code fixes/cleanups (take 2)

In reply to Sergei Shtylyov

Fix several issues with the driver's procfs output:

- when testing if channel is enabled, the code looks at the "simplex" bits, not at the real enable bits -- add #define for the primary channel enable bit;

- UltraDMA modes 0, 1, 3 for slave drive reported incorrectly due to using the master drive's clock cycle resolution bit.

While at it, also perform the following cleanups:

- don't print extra newline before the first controller's dump;

- correct the chipset names (from CMDxxx to PCI-xxx)

- don't read from the registers which aren't used for dump;

- better align the table column sizes;

- rework UltraDMA mode dump code;

- remove PIO mode dump code that has never been finished;

- remove the duplicate interrupt status (the MRDMODE register bits mirror those those in the CFR and ARTTIM23 registers) and fold the dump into single line;

- correct the style of the ?: operators...

Signed-off-by Sergei Shtylyov <[email protected]>

---
This version fixes an issue with the channel enable misreporting in the older patch, adds workaround for missing primary channel enable bit on PCI0643/6, and also beuatifies the overall table format. Has been tested on PCI-649 as well.

 drivers/ide/pci/cmd64x.c |  129 ++++++++++++++++++++---------------------------
 1 files changed, 55 insertions(+), 74 deletions(-)

drivers/ide/pci/cmd64x.c +55 ~~-74~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/cmd64x.c		Version 1.45	Mar 14, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.46	Mar 16, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Due to massive hardware bugs, UltraDMA is only supported

@@ -38,9 +38,10 @@
 #define CFR		0x50
 #define   CFR_INTR_CH0		0x04
 #define CNTRL		0x51
-#define	  CNTRL_DIS_RA0		0x40
-#define   CNTRL_DIS_RA1		0x80
-#define	  CNTRL_ENA_2ND		0x08
+#define   CNTRL_ENA_1ST 	0x04
+#define   CNTRL_ENA_2ND 	0x08
+#define   CNTRL_DIS_RA0 	0x40
+#define   CNTRL_DIS_RA1 	0x80
 
 #define	CMDTIM		0x52
 #define	ARTTIM0		0x53

@@ -87,86 +88,67 @@ static int n_cmd_devs;
 static char * print_cmd64x_get_info (char *buf, struct pci_dev *dev, int index)
 {
 	char *p = buf;
-
-	u8 reg53 = 0, reg54 = 0, reg55 = 0, reg56 = 0;	/* primary */
-	u8 reg57 = 0, reg58 = 0, reg5b;			/* secondary */
 	u8 reg72 = 0, reg73 = 0;			/* primary */
 	u8 reg7a = 0, reg7b = 0;			/* secondary */
-	u8 reg50 = 0, reg71 = 0;			/* extra */
+	u8 reg50 = 1, reg51 = 1, reg57 = 0, reg71 = 0;	/* extra */
+	u8 rev = 0;
 
 	p += sprintf(p, "\nController: %d\n", index);
-	p += sprintf(p, "CMD%x Chipset.\n", dev->device);
+	p += sprintf(p, "PCI-%x Chipset.\n", dev->device);
+
 	(void) pci_read_config_byte(dev, CFR,       &reg50);
-	(void) pci_read_config_byte(dev, ARTTIM0,   &reg53);
-	(void) pci_read_config_byte(dev, DRWTIM0,   &reg54);
-	(void) pci_read_config_byte(dev, ARTTIM1,   &reg55);
-	(void) pci_read_config_byte(dev, DRWTIM1,   &reg56);
-	(void) pci_read_config_byte(dev, ARTTIM2,   &reg57);
-	(void) pci_read_config_byte(dev, DRWTIM2,   &reg58);
-	(void) pci_read_config_byte(dev, DRWTIM3,   &reg5b);
+	(void) pci_read_config_byte(dev, CNTRL,     &reg51);
+	(void) pci_read_config_byte(dev, ARTTIM23,  &reg57);
 	(void) pci_read_config_byte(dev, MRDMODE,   &reg71);
 	(void) pci_read_config_byte(dev, BMIDESR0,  &reg72);
 	(void) pci_read_config_byte(dev, UDIDETCR0, &reg73);
 	(void) pci_read_config_byte(dev, BMIDESR1,  &reg7a);
 	(void) pci_read_config_byte(dev, UDIDETCR1, &reg7b);
 
-	p += sprintf(p, "--------------- Primary Channel "
-			"---------------- Secondary Channel "
-			"-------------\n");
-	p += sprintf(p, "                %sabled           "
-			"              %sabled\n",
-		(reg72&0x80)?"dis":" en",
-		(reg7a&0x80)?"dis":" en");
-	p += sprintf(p, "--------------- drive0 "
-		"--------- drive1 -------- drive0 "
-		"---------- drive1 ------\n");
-	p += sprintf(p, "DMA enabled:    %s              %s"
-			"             %s               %s\n",
-		(reg72&0x20)?"yes":"no ", (reg72&0x40)?"yes":"no ",
-		(reg7a&0x20)?"yes":"no ", (reg7a&0x40)?"yes":"no ");
-
-	p += sprintf(p, "DMA Mode:       %s(%s)          %s(%s)",
-		(reg72&0x20)?((reg73&0x01)?"UDMA":" DMA"):" PIO",
-		(reg72&0x20)?(
-			((reg73&0x30)==0x30)?(((reg73&0x35)==0x35)?"3":"0"):
-			((reg73&0x20)==0x20)?(((reg73&0x25)==0x25)?"3":"1"):
-			((reg73&0x10)==0x10)?(((reg73&0x15)==0x15)?"4":"2"):
-			((reg73&0x00)==0x00)?(((reg73&0x05)==0x05)?"5":"2"):
-			"X"):"?",
-		(reg72&0x40)?((reg73&0x02)?"UDMA":" DMA"):" PIO",
-		(reg72&0x40)?(
-			((reg73&0xC0)==0xC0)?(((reg73&0xC5)==0xC5)?"3":"0"):
-			((reg73&0x80)==0x80)?(((reg73&0x85)==0x85)?"3":"1"):
-			((reg73&0x40)==0x40)?(((reg73&0x4A)==0x4A)?"4":"2"):
-			((reg73&0x00)==0x00)?(((reg73&0x0A)==0x0A)?"5":"2"):
-			"X"):"?");
-	p += sprintf(p, "         %s(%s)           %s(%s)\n",
-		(reg7a&0x20)?((reg7b&0x01)?"UDMA":" DMA"):" PIO",
-		(reg7a&0x20)?(
-			((reg7b&0x30)==0x30)?(((reg7b&0x35)==0x35)?"3":"0"):
-			((reg7b&0x20)==0x20)?(((reg7b&0x25)==0x25)?"3":"1"):
-			((reg7b&0x10)==0x10)?(((reg7b&0x15)==0x15)?"4":"2"):
-			((reg7b&0x00)==0x00)?(((reg7b&0x05)==0x05)?"5":"2"):
-			"X"):"?",
-		(reg7a&0x40)?((reg7b&0x02)?"UDMA":" DMA"):" PIO",
-		(reg7a&0x40)?(
-			((reg7b&0xC0)==0xC0)?(((reg7b&0xC5)==0xC5)?"3":"0"):
-			((reg7b&0x80)==0x80)?(((reg7b&0x85)==0x85)?"3":"1"):
-			((reg7b&0x40)==0x40)?(((reg7b&0x4A)==0x4A)?"4":"2"):
-			((reg7b&0x00)==0x00)?(((reg7b&0x0A)==0x0A)?"5":"2"):
-			"X"):"?" );
-	p += sprintf(p, "PIO Mode:       %s                %s"
-			"               %s                 %s\n",
-			"?", "?", "?", "?");
-	p += sprintf(p, "                %s                     %s\n",
-		(reg50 & CFR_INTR_CH0) ? "interrupting" : "polling     ",
-		(reg57 & ARTTIM23_INTR_CH1) ? "interrupting" : "polling");
-	p += sprintf(p, "                %s                          %s\n",
-		(reg71 & MRDMODE_INTR_CH0) ? "pending" : "clear  ",
-		(reg71 & MRDMODE_INTR_CH1) ? "pending" : "clear");
-	p += sprintf(p, "                %s                          %s\n",
-		(reg71 & MRDMODE_BLK_CH0) ? "blocked" : "enabled",
-		(reg71 & MRDMODE_BLK_CH1) ? "blocked" : "enabled");
+	/* PCI0643/6 originally didn't have the primary channel enable bit */
+	(void) pci_read_config_byte(dev, PCI_REVISION_ID, &rev);
+	if ((dev->device == PCI_DEVICE_ID_CMD_643) ||
+	    (dev->device == PCI_DEVICE_ID_CMD_646 && rev < 3))
+		reg51 |= CNTRL_ENA_1ST;
+
+	p += sprintf(p, "---------------- Primary Channel "
+			"---------------- Secondary Channel ------------\n");
+	p += sprintf(p, "                 %s                         %s\n",
+		 (reg51 & CNTRL_ENA_1ST) ? "enabled " : "disabled",
+		 (reg51 & CNTRL_ENA_2ND) ? "enabled " : "disabled");
+	p += sprintf(p, "---------------- drive0 --------- drive1 "
+			"-------- drive0 --------- drive1 ------\n");
+	p += sprintf(p, "DMA enabled:     %s              %s"
+			"             %s              %s\n",
+		(reg72 & 0x20) ? "yes" : "no ", (reg72 & 0x40) ? "yes" : "no ",
+		(reg7a & 0x20) ? "yes" : "no ", (reg7a & 0x40) ? "yes" : "no ");
+	p += sprintf(p, "UltraDMA mode:   %s (%c)          %s (%c)",
+		( reg73 & 0x01) ? " on" : "off",
+		((reg73 & 0x30) == 0x30) ? ((reg73 & 0x04) ? '3' : '0') :
+		((reg73 & 0x30) == 0x20) ? ((reg73 & 0x04) ? '3' : '1') :
+		((reg73 & 0x30) == 0x10) ? ((reg73 & 0x04) ? '4' : '2') :
+		((reg73 & 0x30) == 0x00) ? ((reg73 & 0x04) ? '5' : '2') : '?',
+		( reg73 & 0x02) ? " on" : "off",
+		((reg73 & 0xC0) == 0xC0) ? ((reg73 & 0x08) ? '3' : '0') :
+		((reg73 & 0xC0) == 0x80) ? ((reg73 & 0x08) ? '3' : '1') :
+		((reg73 & 0xC0) == 0x40) ? ((reg73 & 0x08) ? '4' : '2') :
+		((reg73 & 0xC0) == 0x00) ? ((reg73 & 0x08) ? '5' : '2') : '?');
+	p += sprintf(p, "         %s (%c)          %s (%c)\n",
+		( reg7b & 0x01) ? " on" : "off",
+		((reg7b & 0x30) == 0x30) ? ((reg7b & 0x04) ? '3' : '0') :
+		((reg7b & 0x30) == 0x20) ? ((reg7b & 0x04) ? '3' : '1') :
+		((reg7b & 0x30) == 0x10) ? ((reg7b & 0x04) ? '4' : '2') :
+		((reg7b & 0x30) == 0x00) ? ((reg7b & 0x04) ? '5' : '2') : '?',
+		( reg7b & 0x02) ? " on" : "off",
+		((reg7b & 0xC0) == 0xC0) ? ((reg7b & 0x08) ? '3' : '0') :
+		((reg7b & 0xC0) == 0x80) ? ((reg7b & 0x08) ? '3' : '1') :
+		((reg7b & 0xC0) == 0x40) ? ((reg7b & 0x08) ? '4' : '2') :
+		((reg7b & 0xC0) == 0x00) ? ((reg7b & 0x08) ? '5' : '2') : '?');
+	p += sprintf(p, "Interrupt:       %s, %s                 %s, %s\n",
+		(reg71 & MRDMODE_BLK_CH0  ) ? "blocked" : "enabled",
+		(reg50 & CFR_INTR_CH0	  ) ? "pending" : "clear  ",
+		(reg71 & MRDMODE_BLK_CH1  ) ? "blocked" : "enabled",
+		(reg57 & ARTTIM23_INTR_CH1) ? "pending" : "clear  ");
 
 	return (char *)p;
 }

@@ -176,7 +158,6 @@ static int cmd64x_get_info (char *buffer
 	char *p = buffer;
 	int i;
 
-	p += sprintf(p, "\n");
 	for (i = 0; i < n_cmd_devs; i++) {
 		struct pci_dev *dev	= cmd_devs[i];
 		p = print_cmd64x_get_info(p, dev, i);

Bartlomiej Zolnierkiewicz

Applied

2007-04-23 22:30 UTC #31 lore raw

Re: [PATCH pata-2.6] cmd64x: procfs code fixes/cleanups (take 2)

Quoted 25 lines · Sergei Shtylyov

Fix several issues with the driver's procfs output:
- when testing if channel is enabled, the code looks at the "simplex" bits, not at the real enable bits -- add #define for the primary channel enable bit;
- UltraDMA modes 0, 1, 3 for slave drive reported incorrectly due to using the master drive's clock cycle resolution bit.
While at it, also perform the following cleanups:
- don't print extra newline before the first controller's dump;
- correct the chipset names (from CMDxxx to PCI-xxx)
- don't read from the registers which aren't used for dump;
- better align the table column sizes;
- rework UltraDMA mode dump code;
- remove PIO mode dump code that has never been finished;
- remove the duplicate interrupt status (the MRDMODE register bits mirror those those in the CFR and ARTTIM23 registers) and fold the dump into single line;

Quoted 8 lines

- correct the style of the ?: operators...
Signed-off-by Sergei Shtylyov <[email protected]>
---
This version fixes an issue with the channel enable misreporting in the older patch, adds workaround for missing primary channel enable bit on PCI0643/6, and also beuatifies the overall table format. Has been tested on PCI-649 as well.

applied

Sergei Shtylyov

author

2007-02-16 20:15 UTC #32 lore raw

PATCH 1/1 (pata-2.6 fix queue) cmd64x:

In reply to Sergei Shtylyov

Fold the parts of the ide_dma_end() methods indetical to __ide_dma_end() into a mere call to it.
Start using faster versions of the ide_dma_end() and ide_dma_test_irq() methods for the PCI0646U and newer chips that have the duplicate interrupt status bits in the I/O mapped MRDMODE register, determing what methods to use at the driver load time. Do some cleanup/renaming in the "old" ide_dma_test_irq() method.

While at it, fix minor issues with PCI0646 chipset reporting:

- "IRQ workaround enabled" printed out not only for revision 0x01;

- "CMD646: chipset revision" printed twice (by IDE core and the driver itself);

- empty/pointless switch cases for the chips other than PCI0646.

Signed-off-by Sergei Shtylyov <[email protected]>

---
Warning: this is derived from the specs and completely untested. Shouldn't harm though, as those interrupt bits don't seem to really affect anything, being mere IDE INTR signal latches...

 drivers/ide/pci/cmd64x.c |  151 ++++++++++++++++++++++++-----------------------
 1 files changed, 80 insertions(+), 71 deletions(-)

drivers/ide/pci/cmd64x.c +80 ~~-71~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.46	Feb 15, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.47	Feb 16, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -445,67 +445,80 @@ fast_ata_pio:
 	return 0;
 }
 
-static int cmd64x_alt_dma_status (struct pci_dev *dev)
+static int cmd648_ide_dma_end (ide_drive_t *drive)
 {
-	switch(dev->device) {
-		case PCI_DEVICE_ID_CMD_648:
-		case PCI_DEVICE_ID_CMD_649:
-			return 1;
-		default:
-			break;
-	}
-	return 0;
+	ide_hwif_t *hwif	= HWIF(drive);
+	int err			= __ide_dma_end(drive);
+	u8  irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
+						  MRDMODE_INTR_CH0;
+	u8  mrdmode		= inb(hwif->dma_master + 0x01);
+
+	/* clear the interrupt bit */
+	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+
+	return err;
 }
 
 static int cmd64x_ide_dma_end (ide_drive_t *drive)
 {
-	u8 dma_stat = 0, dma_cmd = 0;
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct pci_dev *dev	= hwif->pci_dev;
+	int irq_reg		= hwif->channel ? ARTTIM23 : CFR;
+	u8  irq_mask		= hwif->channel ? ARTTIM23_INTR_CH1 :
+						  CFR_INTR_CH0;
+	u8  irq_stat		= 0;
+	int err			= __ide_dma_end(drive);
 
-	drive->waiting_for_dma = 0;
-	/* read DMA command state */
-	dma_cmd = hwif->INB(hwif->dma_command);
-	/* stop DMA */
-	hwif->OUTB((dma_cmd & ~1), hwif->dma_command);
-	/* get DMA status */
-	dma_stat = hwif->INB(hwif->dma_status);
-	/* clear the INTR & ERROR bits */
-	hwif->OUTB(dma_stat|6, hwif->dma_status);
-	if (cmd64x_alt_dma_status(dev)) {
-		u8 dma_intr	= 0;
-		u8 dma_mask	= (hwif->channel) ? ARTTIM23_INTR_CH1 :
-						    CFR_INTR_CH0;
-		u8 dma_reg	= (hwif->channel) ? ARTTIM2 : CFR;
-		(void) pci_read_config_byte(dev, dma_reg, &dma_intr);
-		/* clear the INTR bit */
-		(void) pci_write_config_byte(dev, dma_reg, dma_intr|dma_mask);
-	}
-	/* purge DMA mappings */
-	ide_destroy_dmatable(drive);
-	/* verify good DMA status */
-	return (dma_stat & 7) != 4;
+	(void) pci_read_config_byte(dev, irq_reg, &irq_stat);
+	/* clear the interrupt bit */
+	(void) pci_write_config_byte(dev, irq_reg, irq_stat | irq_mask);
+
+	return err;
+}
+
+static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
+{
+	ide_hwif_t *hwif	= HWIF(drive);
+	u8 irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
+						  MRDMODE_INTR_CH0;
+	u8 dma_stat		= inb(hwif->dma_status);
+	u8 mrdmode		= inb(hwif->dma_master + 0x01);
+
+#ifdef DEBUG
+	printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
+	       drive->name, dma_stat, mrdmode, irq_mask);
+#endif
+	if (!(mrdmode & irq_mask))
+		return 0;
+
+	/* return 1 if INTR asserted */
+	if (dma_stat & 4)
+		return 1;
+
+	return 0;
 }
 
 static int cmd64x_ide_dma_test_irq (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct pci_dev *dev	= hwif->pci_dev;
-	u8 irq_reg		= hwif->channel ? ARTTIM23 : CFR;
-	u8 irq_stat = 0, mask	= hwif->channel ? ARTTIM23_INTR_CH1 : CFR_INTR_CH0;
-	u8 dma_stat		= hwif->INB(hwif->dma_status);
+	int irq_reg		= hwif->channel ? ARTTIM23 : CFR;
+	u8  irq_mask		= hwif->channel ? ARTTIM23_INTR_CH1 :
+						  CFR_INTR_CH0;
+	u8  dma_stat		= inb(hwif->dma_status);
+	u8  irq_stat		= 0;
 
 	(void) pci_read_config_byte(dev, irq_reg, &irq_stat);
 
 #ifdef DEBUG
-	printk("%s: dma_stat: 0x%02x irq_stat: 0x%02x mask: 0x%02x\n",
-	       drive->name, dma_stat, irq_stat, mask);
+	printk("%s: dma_stat: 0x%02x irq_stat: 0x%02x irq_mask: 0x%02x\n",
+	       drive->name, dma_stat, irq_stat, irq_mask);
 #endif
-	if (!(irq_stat & mask))
+	if (!(irq_stat & irq_mask))
 		return 0;
 
 	/* return 1 if INTR asserted */
-	if ((dma_stat & 4) == 4)
+	if (dma_stat & 4)
 		return 1;
 
 	return 0;

@@ -544,32 +557,21 @@ static unsigned int __devinit init_chips
 	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
 	class_rev &= 0xff;
 
-	switch(dev->device) {
-		case PCI_DEVICE_ID_CMD_643:
-			break;
-		case PCI_DEVICE_ID_CMD_646:
-			printk(KERN_INFO "%s: chipset revision 0x%02X, ", name, class_rev);
-			switch(class_rev) {
-				case 0x07:
-				case 0x05:
-					printk("UltraDMA Capable");
-					break;
-				case 0x03:
-					printk("MultiWord DMA Force Limited");
-					break;
-				case 0x01:
-				default:
-					printk("MultiWord DMA Limited, IRQ workaround enabled");
-					break;
-				}
-			printk("\n");
-                        break;
-		case PCI_DEVICE_ID_CMD_648:
-		case PCI_DEVICE_ID_CMD_649:
+	if (dev->device == PCI_DEVICE_ID_CMD_646)
+		switch(class_rev) {
+		case 0x07:
+		case 0x05:
+			printk("%s: UltraDMA capable", name);
 			break;
+		case 0x03:
 		default:
+			printk("%s: MultiWord DMA force limited", name);
 			break;
-	}
+		case 0x01:
+			printk("%s: MultiWord DMA limited, "
+			       "IRQ workaround enabled\n", name);
+			break;
+		}
 
 	/* Set a good latency timer and cache line size value. */
 	(void) pci_write_config_byte(dev, PCI_LATENCY_TIMER, 64);

@@ -661,23 +663,30 @@ static void __devinit init_hwif_cmd64x(i
 		hwif->ultra_mask = 0x1f;
 
 	hwif->ide_dma_check = &cmd64x_config_drive_for_dma;
-	if (!(hwif->udma_four))
+	if (!hwif->udma_four)
 		hwif->udma_four = ata66_cmd64x(hwif);
 
-	if (dev->device == PCI_DEVICE_ID_CMD_646) {
+	switch(dev->device) {
+	case PCI_DEVICE_ID_CMD_648:
+	case PCI_DEVICE_ID_CMD_649:
+	alt_irq_bits:
+		hwif->ide_dma_end = &cmd648_ide_dma_end;
+		hwif->ide_dma_test_irq = &cmd648_ide_dma_test_irq;
+		break;
+	case PCI_DEVICE_ID_CMD_646:
 		hwif->chipset = ide_cmd646;
 		if (class_rev == 0x01) {
 			hwif->ide_dma_end = &cmd646_1_ide_dma_end;
-		} else {
-			hwif->ide_dma_end = &cmd64x_ide_dma_end;
-			hwif->ide_dma_test_irq = &cmd64x_ide_dma_test_irq;
-		}
-	} else {
+			break;
+		} else if (class_rev >= 0x03)
+			goto alt_irq_bits;
+		/* fall thru */
+	default:
 		hwif->ide_dma_end = &cmd64x_ide_dma_end;
 		hwif->ide_dma_test_irq = &cmd64x_ide_dma_test_irq;
+		break;
 	}
 
-
 	if (!noautodma)
 		hwif->autodma = 1;
 	hwif->drives[0].autodma = hwif->autodma;

Sergei Shtylyov

author

2007-02-16 20:21 UTC #33 lore raw

PATCH 1/1 (pata-2.6 fix queue) cmd64x: use interrupt status from MRDMODE register

In reply to Sergei Shtylyov

While at it, fix minor issues with PCI0646 chipset reporting:

- "IRQ workaround enabled" printed out not only for revision 0x01;

- "CMD646: chipset revision" printed twice (by IDE core and the driver itself);

- empty/pointless switch cases for the chips other than PCI0646.

Signed-off-by Sergei Shtylyov <[email protected]>

---
Warning: this is derived from the specs and completely untested. Shouldn't harm though, as those interrupt bits don't seem to really affect anything, being mere IDE INTR signal latches...

Argh! Forgot the summary -- resending... :-<

 drivers/ide/pci/cmd64x.c |  151 ++++++++++++++++++++++++-----------------------
 1 files changed, 80 insertions(+), 71 deletions(-)

drivers/ide/pci/cmd64x.c +80 ~~-71~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.46	Feb 15, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.47	Feb 16, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -445,67 +445,80 @@ fast_ata_pio:
 	return 0;
 }
 
-static int cmd64x_alt_dma_status (struct pci_dev *dev)
+static int cmd648_ide_dma_end (ide_drive_t *drive)
 {
-	switch(dev->device) {
-		case PCI_DEVICE_ID_CMD_648:
-		case PCI_DEVICE_ID_CMD_649:
-			return 1;
-		default:
-			break;
-	}
-	return 0;
+	ide_hwif_t *hwif	= HWIF(drive);
+	int err			= __ide_dma_end(drive);
+	u8  irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
+						  MRDMODE_INTR_CH0;
+	u8  mrdmode		= inb(hwif->dma_master + 0x01);
+
+	/* clear the interrupt bit */
+	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+
+	return err;
 }
 
 static int cmd64x_ide_dma_end (ide_drive_t *drive)
 {
-	u8 dma_stat = 0, dma_cmd = 0;
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct pci_dev *dev	= hwif->pci_dev;
+	int irq_reg		= hwif->channel ? ARTTIM23 : CFR;
+	u8  irq_mask		= hwif->channel ? ARTTIM23_INTR_CH1 :
+						  CFR_INTR_CH0;
+	u8  irq_stat		= 0;
+	int err			= __ide_dma_end(drive);
 
-	drive->waiting_for_dma = 0;
-	/* read DMA command state */
-	dma_cmd = hwif->INB(hwif->dma_command);
-	/* stop DMA */
-	hwif->OUTB((dma_cmd & ~1), hwif->dma_command);
-	/* get DMA status */
-	dma_stat = hwif->INB(hwif->dma_status);
-	/* clear the INTR & ERROR bits */
-	hwif->OUTB(dma_stat|6, hwif->dma_status);
-	if (cmd64x_alt_dma_status(dev)) {
-		u8 dma_intr	= 0;
-		u8 dma_mask	= (hwif->channel) ? ARTTIM23_INTR_CH1 :
-						    CFR_INTR_CH0;
-		u8 dma_reg	= (hwif->channel) ? ARTTIM2 : CFR;
-		(void) pci_read_config_byte(dev, dma_reg, &dma_intr);
-		/* clear the INTR bit */
-		(void) pci_write_config_byte(dev, dma_reg, dma_intr|dma_mask);
-	}
-	/* purge DMA mappings */
-	ide_destroy_dmatable(drive);
-	/* verify good DMA status */
-	return (dma_stat & 7) != 4;
+	(void) pci_read_config_byte(dev, irq_reg, &irq_stat);
+	/* clear the interrupt bit */
+	(void) pci_write_config_byte(dev, irq_reg, irq_stat | irq_mask);
+
+	return err;
+}
+
+static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
+{
+	ide_hwif_t *hwif	= HWIF(drive);
+	u8 irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
+						  MRDMODE_INTR_CH0;
+	u8 dma_stat		= inb(hwif->dma_status);
+	u8 mrdmode		= inb(hwif->dma_master + 0x01);
+
+#ifdef DEBUG
+	printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
+	       drive->name, dma_stat, mrdmode, irq_mask);
+#endif
+	if (!(mrdmode & irq_mask))
+		return 0;
+
+	/* return 1 if INTR asserted */
+	if (dma_stat & 4)
+		return 1;
+
+	return 0;
 }
 
 static int cmd64x_ide_dma_test_irq (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct pci_dev *dev	= hwif->pci_dev;
-	u8 irq_reg		= hwif->channel ? ARTTIM23 : CFR;
-	u8 irq_stat = 0, mask	= hwif->channel ? ARTTIM23_INTR_CH1 : CFR_INTR_CH0;
-	u8 dma_stat		= hwif->INB(hwif->dma_status);
+	int irq_reg		= hwif->channel ? ARTTIM23 : CFR;
+	u8  irq_mask		= hwif->channel ? ARTTIM23_INTR_CH1 :
+						  CFR_INTR_CH0;
+	u8  dma_stat		= inb(hwif->dma_status);
+	u8  irq_stat		= 0;
 
 	(void) pci_read_config_byte(dev, irq_reg, &irq_stat);
 
 #ifdef DEBUG
-	printk("%s: dma_stat: 0x%02x irq_stat: 0x%02x mask: 0x%02x\n",
-	       drive->name, dma_stat, irq_stat, mask);
+	printk("%s: dma_stat: 0x%02x irq_stat: 0x%02x irq_mask: 0x%02x\n",
+	       drive->name, dma_stat, irq_stat, irq_mask);
 #endif
-	if (!(irq_stat & mask))
+	if (!(irq_stat & irq_mask))
 		return 0;
 
 	/* return 1 if INTR asserted */
-	if ((dma_stat & 4) == 4)
+	if (dma_stat & 4)
 		return 1;
 
 	return 0;

@@ -544,32 +557,21 @@ static unsigned int __devinit init_chips
 	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
 	class_rev &= 0xff;
 
-	switch(dev->device) {
-		case PCI_DEVICE_ID_CMD_643:
-			break;
-		case PCI_DEVICE_ID_CMD_646:
-			printk(KERN_INFO "%s: chipset revision 0x%02X, ", name, class_rev);
-			switch(class_rev) {
-				case 0x07:
-				case 0x05:
-					printk("UltraDMA Capable");
-					break;
-				case 0x03:
-					printk("MultiWord DMA Force Limited");
-					break;
-				case 0x01:
-				default:
-					printk("MultiWord DMA Limited, IRQ workaround enabled");
-					break;
-				}
-			printk("\n");
-                        break;
-		case PCI_DEVICE_ID_CMD_648:
-		case PCI_DEVICE_ID_CMD_649:
+	if (dev->device == PCI_DEVICE_ID_CMD_646)
+		switch(class_rev) {
+		case 0x07:
+		case 0x05:
+			printk("%s: UltraDMA capable", name);
 			break;
+		case 0x03:
 		default:
+			printk("%s: MultiWord DMA force limited", name);
 			break;
-	}
+		case 0x01:
+			printk("%s: MultiWord DMA limited, "
+			       "IRQ workaround enabled\n", name);
+			break;
+		}
 
 	/* Set a good latency timer and cache line size value. */
 	(void) pci_write_config_byte(dev, PCI_LATENCY_TIMER, 64);

@@ -661,23 +663,30 @@ static void __devinit init_hwif_cmd64x(i
 		hwif->ultra_mask = 0x1f;
 
 	hwif->ide_dma_check = &cmd64x_config_drive_for_dma;
-	if (!(hwif->udma_four))
+	if (!hwif->udma_four)
 		hwif->udma_four = ata66_cmd64x(hwif);
 
-	if (dev->device == PCI_DEVICE_ID_CMD_646) {
+	switch(dev->device) {
+	case PCI_DEVICE_ID_CMD_648:
+	case PCI_DEVICE_ID_CMD_649:
+	alt_irq_bits:
+		hwif->ide_dma_end = &cmd648_ide_dma_end;
+		hwif->ide_dma_test_irq = &cmd648_ide_dma_test_irq;
+		break;
+	case PCI_DEVICE_ID_CMD_646:
 		hwif->chipset = ide_cmd646;
 		if (class_rev == 0x01) {
 			hwif->ide_dma_end = &cmd646_1_ide_dma_end;
-		} else {
-			hwif->ide_dma_end = &cmd64x_ide_dma_end;
-			hwif->ide_dma_test_irq = &cmd64x_ide_dma_test_irq;
-		}
-	} else {
+			break;
+		} else if (class_rev >= 0x03)
+			goto alt_irq_bits;
+		/* fall thru */
+	default:
 		hwif->ide_dma_end = &cmd64x_ide_dma_end;
 		hwif->ide_dma_test_irq = &cmd64x_ide_dma_test_irq;
+		break;
 	}
 
-
 	if (!noautodma)
 		hwif->autodma = 1;
 	hwif->drives[0].autodma = hwif->autodma;

Bartlomiej Zolnierkiewicz

Applied

2007-02-23 20:03 UTC #34 lore raw

Re: [PATCH] (pata-2.6 fix queue) cmd64x: use interrupt status from MRDMODE register

Quoted 9 lines · Sergei Shtylyov

Fold the parts of the ide_dma_end() methods indetical to __ide_dma_end() into a mere call to it.
Start using faster versions of the ide_dma_end() and ide_dma_test_irq() methods for the PCI0646U and newer chips that have the duplicate interrupt status bits in the I/O mapped MRDMODE register, determing what methods to use at the driver load time. Do some cleanup/renaming in the "old" ide_dma_test_irq() method.
While at it, fix minor issues with PCI0646 chipset reporting:

Quoted 7 lines

- "IRQ workaround enabled" printed out not only for revision 0x01;
- "CMD646: chipset revision" printed twice (by IDE core and the driver itself);
- empty/pointless switch cases for the chips other than PCI0646.
Signed-off-by Sergei Shtylyov <[email protected]>

applied

Sergei Shtylyov

author

2007-04-14 19:52 UTC #35 lore raw

PATCH 1/1 cmd64x: use interrupt status from MRDMODE register (take 2)

In reply to Sergei Shtylyov

Fold the parts of the ide_dma_end() methods identical to __ide_dma_end() into a mere call to it.
Start using faster versions of the ide_dma_end() and ide_dma_test_irq() methods for the PCI0646U and newer chips that have the duplicate interrupt status bits in the I/O mapped MRDMODE register, determing what methods to use at the driver load time. Do some cleanup/renaming in the "old" ide_dma_test_irq() method too.

Signed-off-by Sergei Shtylyov <[email protected]>

---
The fixes to PCI0646 chip reporting and some other changes from the older patch have been moved into a separate patch (to be posted RSN), otherwise there's a couple of style changes added. The patch have received testing on PCI-649 now.

 drivers/ide/pci/cmd64x.c |  117 +++++++++++++++++++++++++++--------------------
 1 files changed, 69 insertions(+), 48 deletions(-)

drivers/ide/pci/cmd64x.c +69 ~~-48~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/cmd64x.c		Version 1.46	Mar 16, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.47	Mar 19, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Due to massive hardware bugs, UltraDMA is only supported

@@ -425,67 +425,80 @@ static int cmd64x_config_drive_for_dma (
 	return -1;
 }
 
-static int cmd64x_alt_dma_status (struct pci_dev *dev)
+static int cmd648_ide_dma_end (ide_drive_t *drive)
 {
-	switch(dev->device) {
-		case PCI_DEVICE_ID_CMD_648:
-		case PCI_DEVICE_ID_CMD_649:
-			return 1;
-		default:
-			break;
-	}
-	return 0;
+	ide_hwif_t *hwif	= HWIF(drive);
+	int err			= __ide_dma_end(drive);
+	u8  irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
+						  MRDMODE_INTR_CH0;
+	u8  mrdmode		= inb(hwif->dma_master + 0x01);
+
+	/* clear the interrupt bit */
+	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+
+	return err;
 }
 
 static int cmd64x_ide_dma_end (ide_drive_t *drive)
 {
-	u8 dma_stat = 0, dma_cmd = 0;
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct pci_dev *dev	= hwif->pci_dev;
+	int irq_reg		= hwif->channel ? ARTTIM23 : CFR;
+	u8  irq_mask		= hwif->channel ? ARTTIM23_INTR_CH1 :
+						  CFR_INTR_CH0;
+	u8  irq_stat		= 0;
+	int err			= __ide_dma_end(drive);
 
-	drive->waiting_for_dma = 0;
-	/* read DMA command state */
-	dma_cmd = inb(hwif->dma_command);
-	/* stop DMA */
-	outb(dma_cmd & ~1, hwif->dma_command);
-	/* get DMA status */
-	dma_stat = inb(hwif->dma_status);
-	/* clear the INTR & ERROR bits */
-	outb(dma_stat | 6, hwif->dma_status);
-	if (cmd64x_alt_dma_status(dev)) {
-		u8 dma_intr	= 0;
-		u8 dma_mask	= (hwif->channel) ? ARTTIM23_INTR_CH1 :
-						    CFR_INTR_CH0;
-		u8 dma_reg	= (hwif->channel) ? ARTTIM2 : CFR;
-		(void) pci_read_config_byte(dev, dma_reg, &dma_intr);
-		/* clear the INTR bit */
-		(void) pci_write_config_byte(dev, dma_reg, dma_intr|dma_mask);
-	}
-	/* purge DMA mappings */
-	ide_destroy_dmatable(drive);
-	/* verify good DMA status */
-	return (dma_stat & 7) != 4;
+	(void) pci_read_config_byte(dev, irq_reg, &irq_stat);
+	/* clear the interrupt bit */
+	(void) pci_write_config_byte(dev, irq_reg, irq_stat | irq_mask);
+
+	return err;
+}
+
+static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
+{
+	ide_hwif_t *hwif	= HWIF(drive);
+	u8 irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
+						  MRDMODE_INTR_CH0;
+	u8 dma_stat		= inb(hwif->dma_status);
+	u8 mrdmode		= inb(hwif->dma_master + 0x01);
+
+#ifdef DEBUG
+	printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
+	       drive->name, dma_stat, mrdmode, irq_mask);
+#endif
+	if (!(mrdmode & irq_mask))
+		return 0;
+
+	/* return 1 if INTR asserted */
+	if (dma_stat & 4)
+		return 1;
+
+	return 0;
 }
 
 static int cmd64x_ide_dma_test_irq (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
 	struct pci_dev *dev	= hwif->pci_dev;
-	u8 irq_reg		= hwif->channel ? ARTTIM23 : CFR;
-	u8 irq_stat = 0, mask	= hwif->channel ? ARTTIM23_INTR_CH1 : CFR_INTR_CH0;
-	u8 dma_stat		= inb(hwif->dma_status);
+	int irq_reg		= hwif->channel ? ARTTIM23 : CFR;
+	u8  irq_mask		= hwif->channel ? ARTTIM23_INTR_CH1 :
+						  CFR_INTR_CH0;
+	u8  dma_stat		= inb(hwif->dma_status);
+	u8  irq_stat		= 0;
 
 	(void) pci_read_config_byte(dev, irq_reg, &irq_stat);
 
 #ifdef DEBUG
-	printk("%s: dma_stat: 0x%02x irq_stat: 0x%02x mask: 0x%02x\n",
-	       drive->name, dma_stat, irq_stat, mask);
+	printk("%s: dma_stat: 0x%02x irq_stat: 0x%02x irq_mask: 0x%02x\n",
+	       drive->name, dma_stat, irq_stat, irq_mask);
 #endif
-	if (!(irq_stat & mask))
+	if (!(irq_stat & irq_mask))
 		return 0;
 
 	/* return 1 if INTR asserted */
-	if ((dma_stat & 4) == 4)
+	if (dma_stat & 4)
 		return 1;
 
 	return 0;

@@ -645,17 +658,25 @@ static void __devinit init_hwif_cmd64x(i
 	if (!(hwif->udma_four))
 		hwif->udma_four = ata66_cmd64x(hwif);
 
-	if (dev->device == PCI_DEVICE_ID_CMD_646) {
+	switch(dev->device) {
+	case PCI_DEVICE_ID_CMD_648:
+	case PCI_DEVICE_ID_CMD_649:
+	alt_irq_bits:
+		hwif->ide_dma_end	= &cmd648_ide_dma_end;
+		hwif->ide_dma_test_irq	= &cmd648_ide_dma_test_irq;
+		break;
+	case PCI_DEVICE_ID_CMD_646:
 		hwif->chipset = ide_cmd646;
 		if (class_rev == 0x01) {
 			hwif->ide_dma_end = &cmd646_1_ide_dma_end;
-		} else {
-			hwif->ide_dma_end = &cmd64x_ide_dma_end;
-			hwif->ide_dma_test_irq = &cmd64x_ide_dma_test_irq;
-		}
-	} else {
-		hwif->ide_dma_end = &cmd64x_ide_dma_end;
-		hwif->ide_dma_test_irq = &cmd64x_ide_dma_test_irq;
+			break;
+		} else if (class_rev >= 0x03)
+			goto alt_irq_bits;
+		/* fall thru */
+	default:
+		hwif->ide_dma_end	= &cmd64x_ide_dma_end;
+		hwif->ide_dma_test_irq	= &cmd64x_ide_dma_test_irq;
+		break;
 	}

Bartlomiej Zolnierkiewicz

Applied

2007-04-23 22:30 UTC #36 lore raw

Re: [PATCH pata-2.6] cmd64x: use interrupt status from MRDMODE register (take 2)

Quoted 7 lines · Sergei Shtylyov

Fold the parts of the ide_dma_end() methods identical to __ide_dma_end() into a mere call to it.
Start using faster versions of the ide_dma_end() and ide_dma_test_irq() methods for the PCI0646U and newer chips that have the duplicate interrupt status bits in the I/O mapped MRDMODE register, determing what methods to use at the driver load time. Do some cleanup/renaming in the "old" ide_dma_test_irq() method too.

Quoted 6 lines

Signed-off-by Sergei Shtylyov <[email protected]>
---
The fixes to PCI0646 chip reporting and some other changes from the older patch have been moved into a separate patch (to be posted RSN), otherwise there's a couple of style changes added. The patch have received testing on PCI-649 now.

applied

Sergei Shtylyov

author

2007-04-18 19:37 UTC #37 lore raw

PATCH 1/1 hpt366: fix kernel oops with HPT302N

In reply to Sergei Shtylyov

The driver crashes the kernel on HPT302N chips due to the missing initializer for 'hpt302n.settings' having been unfortunately overlooked so far. :-<

Much thanks to Mike Mattie for pin-pointing the reason of crash.

Signed-off-by Sergei Shtylyov <[email protected]>

---
Posting at last -- please verify...

 drivers/ide/pci/hpt366.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

drivers/ide/pci/hpt366.c +3 -2

Index: linux-2.6/drivers/ide/pci/hpt366.c
===================================================================
@@ -1,10 +1,10 @@
 /*
- * linux/drivers/ide/pci/hpt366.c		Version 1.01	Dec 23, 2006
+ * linux/drivers/ide/pci/hpt366.c		Version 1.02	Apr 18, 2007
  *
  * Copyright (C) 1999-2003		Andre Hedrick <[email protected]>
  * Portions Copyright (C) 2001	        Sun Microsystems, Inc.
  * Portions Copyright (C) 2003		Red Hat Inc
- * Portions Copyright (C) 2005-2006	MontaVista Software, Inc.
+ * Portions Copyright (C) 2005-2007	MontaVista Software, Inc.
  *
  * Thanks to HighPoint Technologies for their assistance, and hardware.
  * Special Thanks to Jon Burchmore in SanDiego for the deep pockets, his

@@ -494,6 +494,7 @@ static struct hpt_info hpt302n __devinit
 	.chip_type	= HPT302N,
 	.max_mode	= HPT302_ALLOW_ATA133_6 ? 4 : 3,
 	.dpll_clk	= 77,
+	.settings	= hpt37x_settings
 };
 
 static struct hpt_info hpt371n __devinitdata = {

Bartlomiej Zolnierkiewicz

Applied

2007-04-20 20:15 UTC #38 lore raw

Re: [PATCH pata-2.6 fix queue] hpt366: fix kernel oops with HPT302N

Quoted 6 lines · Sergei Shtylyov

The driver crashes the kernel on HPT302N chips due to the missing initializer for 'hpt302n.settings' having been unfortunately overlooked so far. :-<
Much thanks to Mike Mattie for pin-pointing the reason of crash.
Signed-off-by Sergei Shtylyov <[email protected]>

applied

Sergei Shtylyov

author

2007-04-22 18:04 UTC #39 lore raw

PATCH 1/1 aec62xx: fix PIO/DMA setup issues

In reply to Sergei Shtylyov

Teach the driver's tuneproc() method to do PIO auto-runing properly since it treated 5 instead of 255 as auto-tune request, and also passed the mode limit of PIO5 to ide_get_best_pio_mode() despite supporting up to PIO4 only.

While at it, also:

- remove the driver's wrong claim about supporting SWDMA modes;

- stop hooking ide_dma_timeout() method as the handler clearly doesn't fit for the task...

Signed-off-by Sergei Shtylyov <[email protected]>

---
Warning: the patch has only been compile tested (the driver was on the way of some cleanup -- that's why I got around to fixing it :-).

 drivers/ide/pci/aec62xx.c |   22 ++++++----------------
 1 files changed, 6 insertions(+), 16 deletions(-)

drivers/ide/pci/aec62xx.c +6 ~~-16~~

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
@@ -1,7 +1,8 @@
 /*
- * linux/drivers/ide/pci/aec62xx.c		Version 0.11	March 27, 2002
+ * linux/drivers/ide/pci/aec62xx.c		Version 0.21	Apr 21, 2007
  *
  * Copyright (C) 1999-2002	Andre Hedrick <[email protected]>
+ * Copyright (C) 2007		MontaVista Software, Inc. <[email protected]>
  *
  */

@@ -193,18 +194,8 @@ static int config_chipset_for_dma (ide_d
 
 static void aec62xx_tune_drive (ide_drive_t *drive, u8 pio)
 {
-	u8 speed = 0;
-	u8 new_pio = XFER_PIO_0 + ide_get_best_pio_mode(drive, 255, 5, NULL);
-
-	switch(pio) {
-		case 5:		speed = new_pio; break;
-		case 4:		speed = XFER_PIO_4; break;
-		case 3:		speed = XFER_PIO_3; break;
-		case 2:		speed = XFER_PIO_2; break;
-		case 1:		speed = XFER_PIO_1; break;
-		default:	speed = XFER_PIO_0; break;
-	}
-	(void) aec62xx_tune_chipset(drive, speed);
+	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);
+	(void) aec62xx_tune_chipset(drive, pio + XFER_PIO_0);
 }
 
 static int aec62xx_config_drive_xfer_rate (ide_drive_t *drive)

@@ -213,7 +204,7 @@ static int aec62xx_config_drive_xfer_rat
 		return 0;
 
 	if (ide_use_fast_pio(drive))
-		aec62xx_tune_drive(drive, 5);
+		aec62xx_tune_drive(drive, 255);
 
 	return -1;
 }

@@ -288,11 +279,10 @@ static void __devinit init_hwif_aec62xx(
 
 	hwif->ultra_mask = 0x7f;
 	hwif->mwdma_mask = 0x07;
-	hwif->swdma_mask = 0x07;
 
 	hwif->ide_dma_check	= &aec62xx_config_drive_xfer_rate;
 	hwif->ide_dma_lostirq	= &aec62xx_irq_timeout;
-	hwif->ide_dma_timeout	= &aec62xx_irq_timeout;
+
 	if (!noautodma)
 		hwif->autodma = 1;
 	hwif->drives[0].autodma = hwif->autodma;

Bartlomiej Zolnierkiewicz

Applied

2007-04-23 22:30 UTC #40 lore raw

Re: [PATCH pata-2.6 fix queue] aec62xx: fix PIO/DMA setup issues

Quoted 8 lines · Sergei Shtylyov

Teach the driver's tuneproc() method to do PIO auto-runing properly since it treated 5 instead of 255 as auto-tune request, and also passed the mode limit of PIO5 to ide_get_best_pio_mode() despite supporting up to PIO4 only.
While at it, also:
- remove the driver's wrong claim about supporting SWDMA modes;

Quoted 8 lines

- stop hooking ide_dma_timeout() method as the handler clearly doesn't fit for the task...
Signed-off-by Sergei Shtylyov <[email protected]>
---
Warning: the patch has only been compile tested (the driver was on the way of some cleanup -- that's why I got around to fixing it :-).

applied

Sergei Shtylyov

author

2007-05-10 20:00 UTC #41 lore raw

PATCH 1/1 cmd64x: init. code cleanup

In reply to Sergei Shtylyov

---
The patch have been tested on PCI-649 only.

Sergei Shtylyov

author

2007-05-10 20:02 UTC #42 lore raw

PATCH 1/1 cmd64x: init. code cleanup

In reply to Sergei Shtylyov

Fix two minor issues with PCI0646 chip reporting in the init_chipset() method:
"IRQ workaround enabled" message printed out not only for revision 0x01 and "CMD646: chipset revision" printed twice (by IDE core and the driver itself). Also, remove empty/pointless switch cases for the chips other than PCI0646, duplicate write to the MRDMODE register when enabling interrupts and MEMORY READ LINE cycles, and needless/misplaced initialization of the timing registers in this method.
Switch to reading only the PCI revision ID register itself, not the whole 32 bits at its address in init_chipset() and init_hwif() methods; in addition, get rid of the useless clearing of hwif->autodma and perform some cosmetic style changes in the latter method.
Refactor ata66_cmd64x() by moving all the code into the 'switch' statement, renaming/adding variables, and fixing the coding style.
While at it, finally get rid of the meaningless aliasing register #define's...

Signed-off-by Sergei Shtylyov <[email protected]>

---
Oops, hit the "send" button too soon the first time. :-) The patch should apply somewhere near the start of pata-2.6 patchset. It has been tested on PCI-649 only.

 drivers/ide/pci/cmd64x.c |  126 ++++++++++++++++-------------------------------
 1 files changed, 45 insertions(+), 81 deletions(-)

drivers/ide/pci/cmd64x.c +45 ~~-81~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/cmd64x.c		Version 1.47	Mar 19, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.50	May 10, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Due to massive hardware bugs, UltraDMA is only supported

@@ -52,9 +52,6 @@
 #define   ARTTIM23_DIS_RA2	0x04
 #define   ARTTIM23_DIS_RA3	0x08
 #define   ARTTIM23_INTR_CH1	0x10
-#define ARTTIM2		0x57
-#define ARTTIM3		0x57
-#define DRWTIM23	0x58
 #define DRWTIM2		0x58
 #define BRST		0x59
 #define DRWTIM3		0x5b

@@ -482,71 +479,43 @@ static int cmd646_1_ide_dma_end (ide_dri
 
 static unsigned int __devinit init_chipset_cmd64x(struct pci_dev *dev, const char *name)
 {
-	u32 class_rev = 0;
 	u8 mrdmode = 0;
 
-	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
-	class_rev &= 0xff;
+	if (dev->device == PCI_DEVICE_ID_CMD_646) {
+		u8 rev = 0;
 
-	switch(dev->device) {
-		case PCI_DEVICE_ID_CMD_643:
-			break;
-		case PCI_DEVICE_ID_CMD_646:
-			printk(KERN_INFO "%s: chipset revision 0x%02X, ", name, class_rev);
-			switch(class_rev) {
-				case 0x07:
-				case 0x05:
-					printk("UltraDMA Capable");
-					break;
-				case 0x03:
-					printk("MultiWord DMA Force Limited");
-					break;
-				case 0x01:
-				default:
-					printk("MultiWord DMA Limited, IRQ workaround enabled");
-					break;
-				}
-			printk("\n");
-                        break;
-		case PCI_DEVICE_ID_CMD_648:
-		case PCI_DEVICE_ID_CMD_649:
+		pci_read_config_byte(dev, PCI_REVISION_ID, &rev);
+
+		switch (rev) {
+		case 0x07:
+		case 0x05:
+			printk("%s: UltraDMA capable", name);
 			break;
+		case 0x03:
 		default:
+			printk("%s: MultiWord DMA force limited", name);
+			break;
+		case 0x01:
+			printk("%s: MultiWord DMA limited, "
+			       "IRQ workaround enabled\n", name);
 			break;
+		}
 	}
 
 	/* Set a good latency timer and cache line size value. */
 	(void) pci_write_config_byte(dev, PCI_LATENCY_TIMER, 64);
 	/* FIXME: pci_set_master() to ensure a good latency timer value */
 
-	/* Setup interrupts. */
-	(void) pci_read_config_byte(dev, MRDMODE, &mrdmode);
-	mrdmode &= ~(0x30);
-	(void) pci_write_config_byte(dev, MRDMODE, mrdmode);
-
-	/* Use MEMORY READ LINE for reads.
-	 * NOTE: Although not mentioned in the PCI0646U specs,
-	 *       these bits are write only and won't be read
-	 *       back as set or not.  The PCI0646U2 specs clarify
-	 *       this point.
+	/*
+	 * Enable interrupts, select MEMORY READ LINE for reads.
+	 *
+	 * NOTE: although not mentioned in the PCI0646U specs,
+	 * bits 0-1 are write only and won't be read back as
+	 * set or not -- PCI0646U2 specs clarify this point.
 	 */
-	(void) pci_write_config_byte(dev, MRDMODE, mrdmode | 0x02);
-
-	/* Set reasonable active/recovery/address-setup values. */
-	(void) pci_write_config_byte(dev, ARTTIM0,  0x40);
-	(void) pci_write_config_byte(dev, DRWTIM0,  0x3f);
-	(void) pci_write_config_byte(dev, ARTTIM1,  0x40);
-	(void) pci_write_config_byte(dev, DRWTIM1,  0x3f);
-#ifdef __i386__
-	(void) pci_write_config_byte(dev, ARTTIM23, 0x1c);
-#else
-	(void) pci_write_config_byte(dev, ARTTIM23, 0x5c);
-#endif
-	(void) pci_write_config_byte(dev, DRWTIM23, 0x3f);
-	(void) pci_write_config_byte(dev, DRWTIM3,  0x3f);
-#ifdef CONFIG_PPC
-	(void) pci_write_config_byte(dev, UDIDETCR0, 0xf0);
-#endif /* CONFIG_PPC */
+	(void) pci_read_config_byte (dev, MRDMODE, &mrdmode);
+	mrdmode &= ~0x30;
+	(void) pci_write_config_byte(dev, MRDMODE, (mrdmode | 0x02));
 
 #if defined(DISPLAY_CMD64X_TIMINGS) && defined(CONFIG_IDE_PROC_FS)

@@ -563,27 +532,25 @@ static unsigned int __devinit init_chips
 
 static unsigned int __devinit ata66_cmd64x(ide_hwif_t *hwif)
 {
-	u8 ata66 = 0, mask = (hwif->channel) ? 0x02 : 0x01;
+	struct pci_dev  *dev	= hwif->pci_dev;
+	u8 bmidecsr = 0, mask	= hwif->channel ? 0x02 : 0x01;
 
-	switch(hwif->pci_dev->device) {
-		case PCI_DEVICE_ID_CMD_643:
-		case PCI_DEVICE_ID_CMD_646:
-			return ata66;
-		default:
-			break;
+	switch (dev->device) {
+	case PCI_DEVICE_ID_CMD_648:
+	case PCI_DEVICE_ID_CMD_649:
+ 		pci_read_config_byte(dev, BMIDECSR, &bmidecsr);
+		return (bmidecsr & mask) ? 1 : 0;
+	default:
+		return 0;
 	}
-	pci_read_config_byte(hwif->pci_dev, BMIDECSR, &ata66);
-	return (ata66 & mask) ? 1 : 0;
 }
 
 static void __devinit init_hwif_cmd64x(ide_hwif_t *hwif)
 {
 	struct pci_dev *dev	= hwif->pci_dev;
-	unsigned int class_rev;
+	u8 rev			= 0;
 
-	hwif->autodma = 0;
-	pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
-	class_rev &= 0xff;
+	pci_read_config_byte(dev, PCI_REVISION_ID, &rev);
 
 	hwif->tuneproc  = &cmd64x_tune_drive;
 	hwif->speedproc = &cmd64x_tune_chipset;

@@ -593,8 +560,8 @@ static void __devinit init_hwif_cmd64x(i
 	if (!hwif->dma_base)
 		return;
 
-	hwif->atapi_dma = 1;
-
+	hwif->atapi_dma  = 1;
+	hwif->mwdma_mask = 0x07;
 	hwif->ultra_mask = hwif->cds->udma_mask;
 
 	/*

@@ -609,16 +576,15 @@ static void __devinit init_hwif_cmd64x(i
 	 *
 	 * So we only do UltraDMA on revision 0x05 and 0x07 chipsets.
 	 */
-	if (dev->device == PCI_DEVICE_ID_CMD_646 && class_rev < 5)
+	if (dev->device == PCI_DEVICE_ID_CMD_646 && rev < 5)
 		hwif->ultra_mask = 0x00;
 
-	hwif->mwdma_mask = 0x07;
-
 	hwif->ide_dma_check = &cmd64x_config_drive_for_dma;
-	if (!(hwif->udma_four))
+
+	if (!hwif->udma_four)
 		hwif->udma_four = ata66_cmd64x(hwif);
 
-	switch(dev->device) {
+	switch (dev->device) {
 	case PCI_DEVICE_ID_CMD_648:
 	case PCI_DEVICE_ID_CMD_649:
 	alt_irq_bits:

@@ -627,10 +593,10 @@ static void __devinit init_hwif_cmd64x(i
 		break;
 	case PCI_DEVICE_ID_CMD_646:
 		hwif->chipset = ide_cmd646;
-		if (class_rev == 0x01) {
+		if (rev == 0x01) {
 			hwif->ide_dma_end = &cmd646_1_ide_dma_end;
 			break;
-		} else if (class_rev >= 0x03)
+		} else if (rev >= 0x03)
 			goto alt_irq_bits;
 		/* fall thru */
 	default:

@@ -639,11 +605,9 @@ static void __devinit init_hwif_cmd64x(i
 		break;
 	}
 
-
 	if (!noautodma)
 		hwif->autodma = 1;
-	hwif->drives[0].autodma = hwif->autodma;
-	hwif->drives[1].autodma = hwif->autodma;
+	hwif->drives[0].autodma = hwif->drives[1].autodma = hwif->autodma;
 }
 
 static int __devinit init_setup_cmd64x(struct pci_dev *dev, ide_pci_device_t *d)

Bartlomiej Zolnierkiewicz

Applied

2007-05-16 00:46 UTC #43 lore raw

Re: [PATCH pata-2.6 fix queue] cmd64x: init. code cleanup

Quoted 15 lines · Sergei Shtylyov

Fix two minor issues with PCI0646 chip reporting in the init_chipset() method:
"IRQ workaround enabled" message printed out not only for revision 0x01 and "CMD646: chipset revision" printed twice (by IDE core and the driver itself). Also, remove empty/pointless switch cases for the chips other than PCI0646, duplicate write to the MRDMODE register when enabling interrupts and MEMORY READ LINE cycles, and needless/misplaced initialization of the timing registers in this method.
Switch to reading only the PCI revision ID register itself, not the whole 32 bits at its address in init_chipset() and init_hwif() methods; in addition, get rid of the useless clearing of hwif->autodma and perform some cosmetic style changes in the latter method.
Refactor ata66_cmd64x() by moving all the code into the 'switch' statement, renaming/adding variables, and fixing the coding style.
While at it, finally get rid of the meaningless aliasing register #define's...

Quoted 1 line

Signed-off-by Sergei Shtylyov <[email protected]>

applied

Sergei Shtylyov

author

2007-05-11 19:30 UTC #44 lore raw

PATCH 1/1 aec62xx: rework init_setup_aec6x80()

In reply to Sergei Shtylyov

Rework init_setup_aec6x80() so that it won't rewrite the constant name strings anymore -- in order to do this:

- in aec62xx_init_one(), pass a local copy of 'struct pci_device_id' down the call chain;

- change the names for in aec62xx_chipsets[] to default to AEC-6280[R];

- override the 'name' field in init_setup_aec6x80() only if bit 4 of the DMA status register is set.

While at it, also change the 'udma_mask' field for AEC-6x80R chips in this function and remove the code doing the same from the init_hwif() method...

Signed-off-by Sergei Shtylyov <[email protected]>

---
The patch should apply somewhere near the start of pata-2.6 patchset.
Warning: the patch has only been compile tested.

 drivers/ide/pci/aec62xx.c |   35 +++++++++++++----------------------
 1 files changed, 13 insertions(+), 22 deletions(-)

drivers/ide/pci/aec62xx.c +13 ~~-22~~

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/aec62xx.c		Version 0.21	Apr 21, 2007
+ * linux/drivers/ide/pci/aec62xx.c		Version 0.22	Apr 23, 2007
  *
  * Copyright (C) 1999-2002	Andre Hedrick <[email protected]>
  * Copyright (C) 2007		MontaVista Software, Inc. <[email protected]>

@@ -243,14 +243,6 @@ static void __devinit init_hwif_aec62xx(
 	}
 
 	hwif->ultra_mask = hwif->cds->udma_mask;
-
-	/* atp865 and atp865r */
-	if (hwif->ultra_mask == 0x3f) {
-		/* check bit 0x10 of DMA status register */
-		if (inb(pci_resource_start(dev, 4) + 2) & 0x10)
- 			hwif->ultra_mask = 0x7f; /* udma0-6 */
-	}
-
 	hwif->mwdma_mask = 0x07;
 
 	hwif->ide_dma_check	= &aec62xx_config_drive_xfer_rate;

@@ -291,16 +283,12 @@ static int __devinit init_setup_aec62xx(
 
 static int __devinit init_setup_aec6x80(struct pci_dev *dev, ide_pci_device_t *d)
 {
-	unsigned long bar4reg = pci_resource_start(dev, 4);
+	unsigned long dma_base = pci_resource_start(dev, 4);
 
-	if (inb(bar4reg+2) & 0x10) {
-		strcpy(d->name, "AEC6880");
-		if (dev->device == PCI_DEVICE_ID_ARTOP_ATP865R)
-			strcpy(d->name, "AEC6880R");
-	} else {
-		strcpy(d->name, "AEC6280");
-		if (dev->device == PCI_DEVICE_ID_ARTOP_ATP865R)
-			strcpy(d->name, "AEC6280R");
+	if (inb(dma_base + 2) & 0x10) {
+		d->name = (dev->device == PCI_DEVICE_ID_ARTOP_ATP865R) ?
+			  "AEC6880R" : "AEC6880";
+		d->udma_mask = 0x7f; /* udma0-6 */
 	}
 
 	return ide_setup_pci_device(dev, d);

@@ -340,7 +328,7 @@ static ide_pci_device_t aec62xx_chipsets
 		.bootable	= NEVER_BOARD,
 		.udma_mask	= 0x1f, /* udma0-4 */
 	},{	/* 3 */
-		.name		= "AEC6X80",
+		.name		= "AEC6280",
 		.init_setup	= init_setup_aec6x80,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,

@@ -350,7 +338,7 @@ static ide_pci_device_t aec62xx_chipsets
 		.bootable	= OFF_BOARD,
 		.udma_mask	= 0x3f, /* udma0-5 */
 	},{	/* 4 */
-		.name		= "AEC6X80R",
+		.name		= "AEC6280R",
 		.init_setup	= init_setup_aec6x80,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,

@@ -370,13 +358,16 @@ static ide_pci_device_t aec62xx_chipsets
  *
  *	Called when the PCI registration layer (or the IDE initialization)
  *	finds a device matching our IDE device tables.
+ *
+ *	NOTE: since we're going to modify the 'name' field for AEC-6[26]80[R]
+ *	chips, pass a local copy of 'struct pci_device_id' down the call chain.
  */
  
 static int __devinit aec62xx_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	ide_pci_device_t *d = &aec62xx_chipsets[id->driver_data];
+	ide_pci_device_t d = aec62xx_chipsets[id->driver_data];
 
-	return d->init_setup(dev, d);
+	return d.init_setup(dev, &d);
 }
 
 static struct pci_device_id aec62xx_pci_tbl[] = {

Bartlomiej Zolnierkiewicz

Applied

2007-05-16 00:46 UTC #45 lore raw

Re: [PATCH pata-2.6 fix queue] aec62xx: rework init_setup_aec6x80()

Quoted 8 lines · Sergei Shtylyov

Rework init_setup_aec6x80() so that it won't rewrite the constant name strings anymore -- in order to do this:
- in aec62xx_init_one(), pass a local copy of 'struct pci_device_id' down the call chain;
- change the names for in aec62xx_chipsets[] to default to AEC-6280[R];

Quoted 7 lines

- override the 'name' field in init_setup_aec6x80() only if bit 4 of the DMA status register is set.
While at it, also change the 'udma_mask' field for AEC-6x80R chips in this function and remove the code doing the same from the init_hwif() method...
Signed-off-by Sergei Shtylyov <[email protected]>

applied

Sergei Shtylyov

author

2007-05-11 21:10 UTC #46 lore raw

PATCH 1/1 aec62xx: remove init_dma() method

In reply to Sergei Shtylyov

Get rid of the init_dma() method (which had no particular reason to exist) by folding it into the init_hwif() method. While at it, also perform some cleanup in the latter method:

- get rid of the useless clearing of hwif->autodma;

- fold the serialization code into one 'if' statement;

- fold setting the drives' 'autotune' and 'autodma' fields into the single statements...

Signed-off-by Sergei Shtylyov <[email protected]>

---
The patch should apply somewhere near the start of pata-2.6 patchset.
Warning: the patch has only been compile tested.

 drivers/ide/pci/aec62xx.c |   63 ++++++++++++++++++----------------------------
 1 files changed, 26 insertions(+), 37 deletions(-)

drivers/ide/pci/aec62xx.c +26 ~~-37~~

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/aec62xx.c		Version 0.22	Apr 23, 2007
+ * linux/drivers/ide/pci/aec62xx.c		Version 0.23	Apr 27, 2007
  *
  * Copyright (C) 1999-2002	Andre Hedrick <[email protected]>
  * Copyright (C) 2007		MontaVista Software, Inc. <[email protected]>

@@ -224,21 +224,18 @@ static unsigned int __devinit init_chips
 
 static void __devinit init_hwif_aec62xx(ide_hwif_t *hwif)
 {
-	struct pci_dev *dev = hwif->pci_dev;
+	struct pci_dev *dev	= hwif->pci_dev;
+	u8 reg54 = 0,  mask	= hwif->channel ? 0xf0 : 0x0f;
+	unsigned long flags;
 
-	hwif->autodma = 0;
 	hwif->tuneproc = &aec62xx_tune_drive;
 	hwif->speedproc = &aec62xx_tune_chipset;
 
-	if (dev->device == PCI_DEVICE_ID_ARTOP_ATP850UF)
-		hwif->serialized = hwif->channel;
-
-	if (hwif->mate)
-		hwif->mate->serialized = hwif->serialized;
+	if (dev->device == PCI_DEVICE_ID_ARTOP_ATP850UF && hwif->mate)
+		hwif->mate->serialized = hwif->serialized = 1;
 
 	if (!hwif->dma_base) {
-		hwif->drives[0].autotune = 1;
-		hwif->drives[1].autotune = 1;
+		hwif->drives[0].autotune = hwif->drives[1].autotune = 1;
 		return;
 	}

@@ -248,32 +245,29 @@ static void __devinit init_hwif_aec62xx(
 	hwif->ide_dma_check	= &aec62xx_config_drive_xfer_rate;
 	hwif->ide_dma_lostirq	= &aec62xx_irq_timeout;
 
-	if (!noautodma)
-		hwif->autodma = 1;
-	hwif->drives[0].autodma = hwif->autodma;
-	hwif->drives[1].autodma = hwif->autodma;
-}
-
-static void __devinit init_dma_aec62xx(ide_hwif_t *hwif, unsigned long dmabase)
-{
-	struct pci_dev *dev	= hwif->pci_dev;
-
-	if (dev->device == PCI_DEVICE_ID_ARTOP_ATP850UF) {
-		u8 reg54h = 0;
-		unsigned long flags;
-
+	switch (dev->device) {
+	case PCI_DEVICE_ID_ARTOP_ATP850UF:
 		spin_lock_irqsave(&ide_lock, flags);
-		pci_read_config_byte(dev, 0x54, &reg54h);
-		pci_write_config_byte(dev, 0x54, reg54h & ~(hwif->channel ? 0xF0 : 0x0F));
+		pci_read_config_byte (dev, 0x54, &reg54);
+		pci_write_config_byte(dev, 0x54, (reg54 & ~mask));
 		spin_unlock_irqrestore(&ide_lock, flags);
-	} else {
-		u8 ata66	= 0;
-		pci_read_config_byte(hwif->pci_dev, 0x49, &ata66);
-	        if (!(hwif->udma_four))
-			hwif->udma_four = (ata66&(hwif->channel?0x02:0x01))?0:1;
+		break;
+	case PCI_DEVICE_ID_ARTOP_ATP865:
+	case PCI_DEVICE_ID_ARTOP_ATP865R:
+	case PCI_DEVICE_ID_ARTOP_ATP860:
+	case PCI_DEVICE_ID_ARTOP_ATP860R:
+	        if (!hwif->udma_four) {
+			u8 ata66 = 0, mask = hwif->channel ? 0x02 : 0x01;
+
+			pci_read_config_byte(hwif->pci_dev, 0x49, &ata66);
+			hwif->udma_four = (ata66 & mask) ? 0 : 1;
+		}
+		break;
 	}
 
-	ide_setup_dma(hwif, dmabase, 8);
+	if (!noautodma)
+		hwif->autodma = 1;
+	hwif->drives[0].autodma = hwif->drives[1].autodma = hwif->autodma;
 }
 
 static int __devinit init_setup_aec62xx(struct pci_dev *dev, ide_pci_device_t *d)

@@ -300,7 +294,6 @@ static ide_pci_device_t aec62xx_chipsets
 		.init_setup	= init_setup_aec62xx,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,
-		.init_dma	= init_dma_aec62xx,
 		.channels	= 2,
 		.autodma	= AUTODMA,
 		.enablebits	= {{0x4a,0x02,0x02}, {0x4a,0x04,0x04}},

@@ -311,7 +304,6 @@ static ide_pci_device_t aec62xx_chipsets
 		.init_setup	= init_setup_aec62xx,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,
-		.init_dma	= init_dma_aec62xx,
 		.channels	= 2,
 		.autodma	= NOAUTODMA,
 		.bootable	= OFF_BOARD,

@@ -321,7 +313,6 @@ static ide_pci_device_t aec62xx_chipsets
 		.init_setup	= init_setup_aec62xx,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,
-		.init_dma	= init_dma_aec62xx,
 		.channels	= 2,
 		.autodma	= AUTODMA,
 		.enablebits	= {{0x4a,0x02,0x02}, {0x4a,0x04,0x04}},

@@ -332,7 +323,6 @@ static ide_pci_device_t aec62xx_chipsets
 		.init_setup	= init_setup_aec6x80,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,
-		.init_dma	= init_dma_aec62xx,
 		.channels	= 2,
 		.autodma	= AUTODMA,
 		.bootable	= OFF_BOARD,

@@ -342,7 +332,6 @@ static ide_pci_device_t aec62xx_chipsets
 		.init_setup	= init_setup_aec6x80,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,
-		.init_dma	= init_dma_aec62xx,
 		.channels	= 2,
 		.autodma	= AUTODMA,
 		.enablebits	= {{0x4a,0x02,0x02}, {0x4a,0x04,0x04}},

Bartlomiej Zolnierkiewicz

Applied

2007-05-16 00:46 UTC #47 lore raw

Re: [PATCH pata-2.6 fix queue] aec62xx: remove init_dma() method

Quoted 4 lines · Sergei Shtylyov

Get rid of the init_dma() method (which had no particular reason to exist) by folding it into the init_hwif() method. While at it, also perform some cleanup in the latter method:

Quoted 8 lines

- get rid of the useless clearing of hwif->autodma;
- fold the serialization code into one 'if' statement;
- fold setting the drives' 'autotune' and 'autodma' fields into the single statements...
Signed-off-by Sergei Shtylyov <[email protected]>

applied

Sergei Shtylyov

author

2007-05-11 21:21 UTC #48 lore raw

PATCH 1/1 aec62xx: kill speedproc() method wrapper

In reply to Sergei Shtylyov

There's no reason to have the speedproc() method wrapper for the two quite different chip families, so just get rid of it.

Signed-off-by Sergei Shtylyov <[email protected]>

---
The patch should apply somewhere near the start of pata-2.6 patchset.
Warning: the patch has only been compile tested.

 drivers/ide/pci/aec62xx.c |   24 ++++++------------------
 1 files changed, 6 insertions(+), 18 deletions(-)

drivers/ide/pci/aec62xx.c +6 ~~-18~~

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/aec62xx.c		Version 0.23	Apr 27, 2007
+ * linux/drivers/ide/pci/aec62xx.c		Version 0.24	Apr 28, 2007
  *
  * Copyright (C) 1999-2002	Andre Hedrick <[email protected]>
  * Copyright (C) 2007		MontaVista Software, Inc. <[email protected]>

@@ -140,25 +140,10 @@ static int aec6260_tune_chipset (ide_dri
 	return(ide_config_drive_speed(drive, speed));
 }
 
-static int aec62xx_tune_chipset (ide_drive_t *drive, u8 speed)
-{
-	switch (HWIF(drive)->pci_dev->device) {
-		case PCI_DEVICE_ID_ARTOP_ATP865:
-		case PCI_DEVICE_ID_ARTOP_ATP865R:
-		case PCI_DEVICE_ID_ARTOP_ATP860:
-		case PCI_DEVICE_ID_ARTOP_ATP860R:
-			return ((int) aec6260_tune_chipset(drive, speed));
-		case PCI_DEVICE_ID_ARTOP_ATP850UF:
-			return ((int) aec6210_tune_chipset(drive, speed));
-		default:
-			return -1;
-	}
-}
-
 static void aec62xx_tune_drive (ide_drive_t *drive, u8 pio)
 {
 	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);
-	(void) aec62xx_tune_chipset(drive, pio + XFER_PIO_0);
+	(void) HWIF(drive)->speedproc(drive, pio + XFER_PIO_0);
 }
 
 static int aec62xx_config_drive_xfer_rate (ide_drive_t *drive)

@@ -229,7 +214,6 @@ static void __devinit init_hwif_aec62xx(
 	unsigned long flags;
 
 	hwif->tuneproc = &aec62xx_tune_drive;
-	hwif->speedproc = &aec62xx_tune_chipset;
 
 	if (dev->device == PCI_DEVICE_ID_ARTOP_ATP850UF && hwif->mate)
 		hwif->mate->serialized = hwif->serialized = 1;

@@ -251,6 +235,8 @@ static void __devinit init_hwif_aec62xx(
 		pci_read_config_byte (dev, 0x54, &reg54);
 		pci_write_config_byte(dev, 0x54, (reg54 & ~mask));
 		spin_unlock_irqrestore(&ide_lock, flags);
+
+		hwif->speedproc = &aec6210_tune_chipset;
 		break;
 	case PCI_DEVICE_ID_ARTOP_ATP865:
 	case PCI_DEVICE_ID_ARTOP_ATP865R:

@@ -262,6 +248,8 @@ static void __devinit init_hwif_aec62xx(
 			pci_read_config_byte(hwif->pci_dev, 0x49, &ata66);
 			hwif->udma_four = (ata66 & mask) ? 0 : 1;
 		}
+
+		hwif->speedproc = &aec6260_tune_chipset;
 		break;
 	}

Bartlomiej Zolnierkiewicz

Applied

2007-05-16 00:46 UTC #49 lore raw

Re: [PATCH pata-2.6 fix queue] aec62xx: kill speedproc() method wrapper

Quoted 4 lines · Sergei Shtylyov

There's no reason to have the speedproc() method wrapper for the two quite different chip families, so just get rid of it.
Signed-off-by Sergei Shtylyov <[email protected]>

applied

Sergei Shtylyov

author

2007-05-16 14:18 UTC #50 lore raw

Re: [PATCH pata-2.6 fix queue] aec62xx: kill speedproc() method wrapper

Hello.

Quoted 6 lines · Bartlomiej Zolnierkiewicz

Quoted 4 lines
There's no reason to have the speedproc() method wrapper for the two quite different chip families, so just get rid of it.
Signed-off-by Sergei Shtylyov <[email protected]>
applied

I forgot to notice/mention the side effect: there would be no speedproc()
method installed if hwif->dma_base happens to be 0.

MBR, Sergei

Bartlomiej Zolnierkiewicz

2007-05-24 01:23 UTC #51 lore raw

Re: [PATCH pata-2.6 fix queue] aec62xx: kill speedproc() method wrapper

Hi,

Quoted 7 lines · Sergei Shtylyov

Hello.
Quoted 2 lines · Bartlomiej Zolnierkiewicz
Quoted 2 lines
There's no reason to have the speedproc() method wrapper for the two quite different chip families, so just get rid of it.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Signed-off-by Sergei Shtylyov <[email protected]>
applied
I forgot to notice/mention the side effect: there would be no speedproc()
method installed if hwif->dma_base happens to be 0.

It doesn't sound too nice since ->autotune is always set and ->tuneproc uses ->speedproc unconditionally (=> OOPS).

Looks like we really need an extra

if (atp850)
choose atp850 speedproc
else
choose atp86x speedproc

before hwif->dma_base check... or maybe even separate ->init_hwif methods for atp850 and atp86x.

Thanks,
Bart

Sergei Shtylyov

author

2007-05-24 13:17 UTC #52 lore raw

Re: [PATCH pata-2.6 fix queue] aec62xx: kill speedproc() method wrapper

Quoted 7 lines · Bartlomiej Zolnierkiewicz

Quoted 6 lines
Quoted 4 lines · Bartlomiej Zolnierkiewicz
Quoted 4 lines
There's no reason to have the speedproc() method wrapper for the two quite different chip families, so just get rid of it.
Signed-off-by Sergei Shtylyov <[email protected]>

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
applied
I forgot to notice/mention the side effect: there would be no speedproc()
method installed if hwif->dma_base happens to be 0.
It doesn't sound too nice since ->autotune is always set and ->tuneproc uses ->speedproc unconditionally (=> OOPS).

Yeah, I have overlooked that -- will need to be reworked.

Quoted 6 lines

Looks like we really need an extra
if (atp850)
choose atp850 speedproc
else
choose atp86x speedproc

Yeah, I'll add this to the initial if statement dealing with serialization.

Quoted 2 lines

before hwif->dma_base check... or maybe even separate ->init_hwif methods for atp850 and atp86x.

Probably doesn't worth it...

Quoted 2 lines

Thanks,
Bart

WBR, Sergei

Sergei Shtylyov

author

2007-05-24 16:42 UTC #53 lore raw

PATCH 1/1 aec62xx: remove init_dma() method (take 2)

In reply to Sergei Shtylyov

Get rid of the init_dma() method (which had no particular reason to exist) by folding it into the init_hwif() method. While at it, also perform some cleanup in the latter method:

- get rid of the useless clearing of hwif->autodma;

- fold the serialization code into one 'if' statement;

- fold setting the drives' 'autotune' and 'autodma' fields into the single statements...

Signed-off-by Sergei Shtylyov <[email protected]>

---
In this version of the patch I decided not to introduce 'switch' statement into the init_hwif() method. Again, the patch has only been compile tested...

 drivers/ide/pci/aec62xx.c |   51 ++++++++++++++--------------------------------
 1 files changed, 16 insertions(+), 35 deletions(-)

drivers/ide/pci/aec62xx.c +16 ~~-35~~

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/aec62xx.c		Version 0.22	Apr 23, 2007
+ * linux/drivers/ide/pci/aec62xx.c		Version 0.23	May 23, 2007
  *
  * Copyright (C) 1999-2002	Andre Hedrick <[email protected]>
  * Copyright (C) 2007		MontaVista Software, Inc. <[email protected]>

@@ -220,21 +220,18 @@ static unsigned int __devinit init_chips
 
 static void __devinit init_hwif_aec62xx(ide_hwif_t *hwif)
 {
-	struct pci_dev *dev = hwif->pci_dev;
+	struct pci_dev *dev	= hwif->pci_dev;
+	u8 reg54 = 0,  mask	= hwif->channel ? 0xf0 : 0x0f;
+	unsigned long flags;
 
-	hwif->autodma = 0;
 	hwif->tuneproc = &aec62xx_tune_drive;
 	hwif->speedproc = &aec62xx_tune_chipset;
 
-	if (dev->device == PCI_DEVICE_ID_ARTOP_ATP850UF)
-		hwif->serialized = hwif->channel;
-
-	if (hwif->mate)
-		hwif->mate->serialized = hwif->serialized;
+	if (dev->device == PCI_DEVICE_ID_ARTOP_ATP850UF && hwif->mate)
+		hwif->mate->serialized = hwif->serialized = 1;
 
 	if (!hwif->dma_base) {
-		hwif->drives[0].autotune = 1;
-		hwif->drives[1].autotune = 1;
+		hwif->drives[0].autotune = hwif->drives[1].autotune = 1;
 		return;
 	}

@@ -244,32 +241,21 @@ static void __devinit init_hwif_aec62xx(
 	hwif->ide_dma_check	= &aec62xx_config_drive_xfer_rate;
 	hwif->dma_lost_irq	= &aec62xx_dma_lost_irq;
 
-	if (!noautodma)
-		hwif->autodma = 1;
-	hwif->drives[0].autodma = hwif->autodma;
-	hwif->drives[1].autodma = hwif->autodma;
-}
-
-static void __devinit init_dma_aec62xx(ide_hwif_t *hwif, unsigned long dmabase)
-{
-	struct pci_dev *dev	= hwif->pci_dev;
-
 	if (dev->device == PCI_DEVICE_ID_ARTOP_ATP850UF) {
-		u8 reg54h = 0;
-		unsigned long flags;
-
 		spin_lock_irqsave(&ide_lock, flags);
-		pci_read_config_byte(dev, 0x54, &reg54h);
-		pci_write_config_byte(dev, 0x54, reg54h & ~(hwif->channel ? 0xF0 : 0x0F));
+		pci_read_config_byte (dev, 0x54, &reg54);
+		pci_write_config_byte(dev, 0x54, (reg54 & ~mask));
 		spin_unlock_irqrestore(&ide_lock, flags);
-	} else {
-		u8 ata66	= 0;
+	} else if (!hwif->udma_four)
+		u8 ata66 = 0, mask = hwif->channel ? 0x02 : 0x01;
+
 		pci_read_config_byte(hwif->pci_dev, 0x49, &ata66);
-	        if (!(hwif->udma_four))
-			hwif->udma_four = (ata66&(hwif->channel?0x02:0x01))?0:1;
+		hwif->udma_four = (ata66 & mask) ? 0 : 1;
 	}
 
-	ide_setup_dma(hwif, dmabase, 8);
+	if (!noautodma)
+		hwif->autodma = 1;
+	hwif->drives[0].autodma = hwif->drives[1].autodma = hwif->autodma;
 }
 
 static int __devinit init_setup_aec62xx(struct pci_dev *dev, ide_pci_device_t *d)

@@ -296,7 +282,6 @@ static ide_pci_device_t aec62xx_chipsets
 		.init_setup	= init_setup_aec62xx,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,
-		.init_dma	= init_dma_aec62xx,
 		.channels	= 2,
 		.autodma	= AUTODMA,
 		.enablebits	= {{0x4a,0x02,0x02}, {0x4a,0x04,0x04}},

@@ -307,7 +292,6 @@ static ide_pci_device_t aec62xx_chipsets
 		.init_setup	= init_setup_aec62xx,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,
-		.init_dma	= init_dma_aec62xx,
 		.channels	= 2,
 		.autodma	= NOAUTODMA,
 		.bootable	= OFF_BOARD,

@@ -317,7 +301,6 @@ static ide_pci_device_t aec62xx_chipsets
 		.init_setup	= init_setup_aec62xx,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,
-		.init_dma	= init_dma_aec62xx,
 		.channels	= 2,
 		.autodma	= AUTODMA,
 		.enablebits	= {{0x4a,0x02,0x02}, {0x4a,0x04,0x04}},

@@ -328,7 +311,6 @@ static ide_pci_device_t aec62xx_chipsets
 		.init_setup	= init_setup_aec6x80,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,
-		.init_dma	= init_dma_aec62xx,
 		.channels	= 2,
 		.autodma	= AUTODMA,
 		.bootable	= OFF_BOARD,

@@ -338,7 +320,6 @@ static ide_pci_device_t aec62xx_chipsets
 		.init_setup	= init_setup_aec6x80,
 		.init_chipset	= init_chipset_aec62xx,
 		.init_hwif	= init_hwif_aec62xx,
-		.init_dma	= init_dma_aec62xx,
 		.channels	= 2,
 		.autodma	= AUTODMA,
 		.enablebits	= {{0x4a,0x02,0x02}, {0x4a,0x04,0x04}},

Sergei Shtylyov

author

2007-05-24 16:44 UTC #54 lore raw

PATCH 1/1 aec62xx: kill speedproc() method wrapper (take 2)

In reply to Sergei Shtylyov

There's no reason to have the speedproc() method wrapper for the two quite different chip families, so just get rid of it.

Signed-off-by Sergei Shtylyov <[email protected]>

---
Tthis version of the patch properly installs the speedproc() method. Again, the patch has only been compile tested...

 drivers/ide/pci/aec62xx.c |   28 ++++++++--------------------
 1 files changed, 8 insertions(+), 20 deletions(-)

drivers/ide/pci/aec62xx.c +8 ~~-20~~

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/aec62xx.c		Version 0.23	May 23, 2007
+ * linux/drivers/ide/pci/aec62xx.c		Version 0.24	May 24, 2007
  *
  * Copyright (C) 1999-2002	Andre Hedrick <[email protected]>
  * Copyright (C) 2007		MontaVista Software, Inc. <[email protected]>

@@ -140,25 +140,10 @@ static int aec6260_tune_chipset (ide_dri
 	return(ide_config_drive_speed(drive, speed));
 }
 
-static int aec62xx_tune_chipset (ide_drive_t *drive, u8 speed)
-{
-	switch (HWIF(drive)->pci_dev->device) {
-		case PCI_DEVICE_ID_ARTOP_ATP865:
-		case PCI_DEVICE_ID_ARTOP_ATP865R:
-		case PCI_DEVICE_ID_ARTOP_ATP860:
-		case PCI_DEVICE_ID_ARTOP_ATP860R:
-			return ((int) aec6260_tune_chipset(drive, speed));
-		case PCI_DEVICE_ID_ARTOP_ATP850UF:
-			return ((int) aec6210_tune_chipset(drive, speed));
-		default:
-			return -1;
-	}
-}
-
 static void aec62xx_tune_drive (ide_drive_t *drive, u8 pio)
 {
 	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);
-	(void) aec62xx_tune_chipset(drive, pio + XFER_PIO_0);
+	(void) HWIF(drive)->speedproc(drive, pio + XFER_PIO_0);
 }
 
 static int aec62xx_config_drive_xfer_rate (ide_drive_t *drive)

@@ -225,10 +210,13 @@ static void __devinit init_hwif_aec62xx(
 	unsigned long flags;
 
 	hwif->tuneproc = &aec62xx_tune_drive;
-	hwif->speedproc = &aec62xx_tune_chipset;
 
-	if (dev->device == PCI_DEVICE_ID_ARTOP_ATP850UF && hwif->mate)
-		hwif->mate->serialized = hwif->serialized = 1;
+	if (dev->device == PCI_DEVICE_ID_ARTOP_ATP850UF) {
+		if(hwif->mate)
+			hwif->mate->serialized = hwif->serialized = 1;
+		hwif->speedproc = &aec6210_tune_chipset;
+	} else
+		hwif->speedproc = &aec6260_tune_chipset;
 
 	if (!hwif->dma_base) {
 		hwif->drives[0].autotune = hwif->drives[1].autotune = 1;

Bartlomiej Zolnierkiewicz

2007-05-28 21:05 UTC #55 lore raw

Re: [PATCH pata-2.6] aec62xx: kill speedproc() method wrapper (take 2)

Quoted 8 lines · Sergei Shtylyov

There's no reason to have the speedproc() method wrapper for the two quite different chip families, so just get rid of it.
Signed-off-by Sergei Shtylyov <[email protected]>
---
Tthis version of the patch properly installs the speedproc() method. Again, the patch has only been compile tested...

applied after fixing the following trivial reject (not a problem but I thought I would let you know in case I missed something):

***************
*** 1,5 ****
/*
- * linux/drivers/ide/pci/aec62xx.c Version 0.23 May 23, 2007
*
* Copyright (C) 1999-2002 Andre Hedrick <[email protected]>
* Copyright (C) 2007 MontaVista Software, Inc. <[email protected]>
--- 1,5 ----
/*
+ * linux/drivers/ide/pci/aec62xx.c Version 0.24 May 24, 2007
*
* Copyright (C) 1999-2002 Andre Hedrick <[email protected]>
* Copyright (C) 2007 MontaVista Software, Inc. <[email protected]>

I don't know why but it was:

linux/drivers/ide/pci/aec62xx.c Version 0.23 Apr 27, 2007

Sergei Shtylyov

author

2007-06-05 15:14 UTC #56 lore raw

Re: [PATCH pata-2.6] aec62xx: kill speedproc() method wrapper (take 2)

Hello.

Quoted 11 lines · Bartlomiej Zolnierkiewicz

Quoted 8 lines
There's no reason to have the speedproc() method wrapper for the two quite different chip families, so just get rid of it.
Signed-off-by Sergei Shtylyov <[email protected]>
---
Tthis version of the patch properly installs the speedproc() method. Again, the patch has only been compile tested...
applied after fixing the following trivial reject (not a problem but I thought I would let you know in case I missed something):

Yes, you did. I've also recast the patch killing init_dma() method. Do I
need to resend it?

Quoted 14 lines

***************
*** 1,5 ****
/*

drivers/ide/pci/aec62xx.c +1 -2

-  * linux/drivers/ide/pci/aec62xx.c            Version 0.23    May 23, 2007
   *
   * Copyright (C) 1999-2002    Andre Hedrick <[email protected]>
   * Copyright (C) 2007         MontaVista Software, Inc. <[email protected]>
--- 1,5 ----
  /*
+  * linux/drivers/ide/pci/aec62xx.c            Version 0.24    May 24, 2007
   *
   * Copyright (C) 1999-2002    Andre Hedrick <[email protected]>
   * Copyright (C) 2007         MontaVista Software, Inc. <[email protected]>

Quoted 3 lines

I don't know why but it was:
linux/drivers/ide/pci/aec62xx.c Version 0.23 Apr 27, 2007

MBR, Sergei

Bartlomiej Zolnierkiewicz

2007-06-08 13:09 UTC #57 lore raw

Re: [PATCH pata-2.6] aec62xx: kill speedproc() method wrapper (take 2)

Quoted 13 lines · Sergei Shtylyov

Hello.
Quoted 8 lines · Bartlomiej Zolnierkiewicz
Quoted 8 lines
There's no reason to have the speedproc() method wrapper for the two quite different chip families, so just get rid of it.
Signed-off-by Sergei Shtylyov <[email protected]>
---
Tthis version of the patch properly installs the speedproc() method. Again, the patch has only been compile tested...

Quoted 5 lines

Quoted 2 lines
applied after fixing the following trivial reject (not a problem but I thought I would let you know in case I missed something):
Yes, you did. I've also recast the patch killing init_dma() method. Do I
need to resend it?

Please do.

Sergei Shtylyov

author

2007-06-09 10:03 UTC #58 lore raw

Re: [PATCH pata-2.6] aec62xx: kill speedproc() method wrapper (take 2)

Hello.

Quoted 9 lines · Bartlomiej Zolnierkiewicz

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
There's no reason to have the speedproc() method wrapper for the two quite different chip families, so just get rid of it.
Signed-off-by Sergei Shtylyov <[email protected]>
---
Tthis version of the patch properly installs the speedproc() method. Again, the patch has only been compile tested...

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
applied after fixing the following trivial reject (not a problem but I thought I would let you know in case I missed something):
Yes, you did. I've also recast the patch killing init_dma() method. Do I
need to resend it?
Please do.

Just have resent it privately.

MBR, Sergei

Sergei Shtylyov

author

2007-11-09 11:06 UTC #59 lore raw

PATCH 1/1 cmd64x: don't clear the other channels interrupt

In reply to Sergei Shtylyov

Make sure to not clear the other IDE channel's interrupt when clearing an IDE interrupt via the MRDMODE register.

Thanks to Bart for finding a coding mistake.

---
The patch is against the Linus tree.

 drivers/ide/pci/cmd64x.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

drivers/ide/pci/cmd64x.c +3 -2

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/cmd64x.c		Version 1.50	May 10, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.51	Nov 8, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Due to massive hardware bugs, UltraDMA is only supported

@@ -339,7 +339,8 @@ static int cmd648_ide_dma_end (ide_drive
 	u8  mrdmode		= inb(hwif->dma_master + 0x01);
 
 	/* clear the interrupt bit */
-	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+	outb((mrdmode & ~(MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1)) | irq_mask,
+	     hwif->dma_master + 0x01);
 
 	return err;
 }

Bartlomiej Zolnierkiewicz

2007-11-11 21:46 UTC #60 lore raw

Re: [PATCH] cmd64x: don't clear the other channels interrupt

Quoted 4 lines · Sergei Shtylyov

Make sure to not clear the other IDE channel's interrupt when clearing an IDE interrupt via the MRDMODE register.
Thanks to Bart for finding a coding mistake.

Could you resend with "Signed-off-by:" and "Tested-by:" tags?

[ A reference to the problematic commit and crediting Martin for
patiently bisecting the issue would score an extra points. :) ]

Quoted 29 lines

---
The patch is against the Linus tree.

 drivers/ide/pci/cmd64x.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================

drivers/ide/pci/cmd64x.c +3 -2

@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/cmd64x.c		Version 1.50	May 10, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.51	Nov 8, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Due to massive hardware bugs, UltraDMA is only supported

@@ -339,7 +339,8 @@ static int cmd648_ide_dma_end (ide_drive
 	u8  mrdmode		= inb(hwif->dma_master + 0x01);
 
 	/* clear the interrupt bit */
-	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+	outb((mrdmode & ~(MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1)) | irq_mask,
+	     hwif->dma_master + 0x01);
 
 	return err;
 }

Bartlomiej Zolnierkiewicz

2007-11-13 21:07 UTC #61 lore raw

Re: [PATCH] cmd64x: don't clear the other channels interrupt

Quoted 10 lines · Bartlomiej Zolnierkiewicz

Quoted 4 lines · Sergei Shtylyov
Make sure to not clear the other IDE channel's interrupt when clearing an IDE interrupt via the MRDMODE register.
Thanks to Bart for finding a coding mistake.
Could you resend with "Signed-off-by:" and "Tested-by:" tags?
[ A reference to the problematic commit and crediting Martin for
patiently bisecting the issue would score an extra points. :) ]

Well, I dealt with it myself and just added "From:" (together with "Tested-by:"-s and an enhanced patch description)...

For the future please always run your patches through scripts/checkpatch.pl before posting, this prevents missing "Signed-off-by:" (and few other issues :).

Thanks,
Bart

Martin Rogge

2007-11-15 19:23 UTC #62 lore raw

Re: [PATCH] cmd64x: don't clear the other channels interrupt

Quoted 15 lines · On Tuesday 13 November 2007 21:58:36 Bartlomiej Zolnierkiewicz wrote:

Quoted 10 lines · Bartlomiej Zolnierkiewicz
Quoted 4 lines · Sergei Shtylyov
Make sure to not clear the other IDE channel's interrupt when clearing an IDE interrupt via the MRDMODE register.
Thanks to Bart for finding a coding mistake.
Could you resend with "Signed-off-by:" and "Tested-by:" tags?
[ A reference to the problematic commit and crediting Martin for
patiently bisecting the issue would score an extra points. :) ]
Well, I dealt with it myself and just added "From:" (together with "Tested-by:"-s and an enhanced patch description)...

Quoted 6 lines

For the future please always run your patches through scripts/checkpatch.pl before posting, this prevents missing "Signed-off-by:" (and few other issues :).
Thanks,
Bart

Thank you all for help and resolution. And all the bisecting, with or without git, I didn't really mind doing since it is for the greater good. I've even learnt something new since my linux-m68k days. :)

cu Martin

Sergei Shtylyov

author

2007-11-16 13:34 UTC #63 lore raw

Re: [PATCH] cmd64x: don't clear the other channels interrupt

In reply to Bartlomiej Zolnierkiewicz

Quoted 5 lines · Bartlomiej Zolnierkiewicz

Quoted 4 lines
Quoted 4 lines
Make sure to not clear the other IDE channel's interrupt when clearing an IDE interrupt via the MRDMODE register.
Thanks to Bart for finding a coding mistake.

Quoted 7 lines

Quoted 4 lines
Could you resend with "Signed-off-by:" and "Tested-by:" tags?
[ A reference to the problematic commit and crediting Martin for
patiently bisecting the issue would score an extra points. :) ]
Well, I dealt with it myself and just added "From:" (together with "Tested-by:"-s and an enhanced patch description)...

Sorry, I somehow haven't noticed your mail in time... :-/

Quoted 2 lines

For the future please always run your patches through scripts/checkpatch.pl before posting, this prevents missing

Aye aye, sir! :-)

Quoted 1 line

"Signed-off-by:" (and few other issues :).

Like not crediting testers? ;-)

Quoted 2 lines

Thanks,
Bart

WBR, Sergei

Sergei Shtylyov

author

2007-02-26 20:32 UTC #64 lore raw

PATCH 1/1 (pata-2.6 fix queue) cmd64x: fix recovery time calculation (take 2)

In reply to Sergei Shtylyov

Signed-off-by Sergei Shtylyov <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
Just discovered a missed rename in this patch -- it's always a good idea to enable a debugging option when compiling. :-)
The later patches should be still applying without issues...

 drivers/ide/pci/cmd64x.c |   49 +++++++++++++++++++++--------------------------
 1 files changed, 22 insertions(+), 27 deletions(-)

drivers/ide/pci/cmd64x.c +22 ~~-27~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.42	Feb 6, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.43	Feb 8, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -262,53 +262,48 @@ static void program_drive_counts (ide_dr
 	local_irq_restore(flags);
 }
 
+static u8 quantize_timing(int timing, int quant)
+{
+	return (timing + quant - 1) / quant;
+}
+
 /*
  * This routine selects drive's best PIO mode, calculates setup/active/recovery
  * counts, and then writes them into the chipset registers.
  */
 static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted)
 {
-	int setup_time, active_time, recovery_time;
-	int clock_time, pio_mode, cycle_time;
-	u8 recovery_count2, cycle_count;
-	int setup_count, active_count, recovery_count;
-	int bus_speed = system_bus_clock();
-	ide_pio_data_t  d;
+	int setup_time, active_time, cycle_time;
+	u8  cycle_count, setup_count, active_count, recovery_count;
+	u8  pio_mode;
+	int clock_time = 1000 / system_bus_clock();
+	ide_pio_data_t pio;
 
-	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &d);
-	cycle_time = d.cycle_time;
+	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &pio);
+	cycle_time = pio.cycle_time;
 
-	/*
-	 * I copied all this complicated stuff from cmd640.c and made a few
-	 * minor changes.  For now I am just going to pray that it is correct.
-	 */
 	setup_time  = ide_pio_timings[pio_mode].setup_time;
 	active_time = ide_pio_timings[pio_mode].active_time;
-	recovery_time = cycle_time - (setup_time + active_time);
-	clock_time = 1000 / bus_speed;
-	cycle_count = (cycle_time + clock_time - 1) / clock_time;
-
-	setup_count = (setup_time + clock_time - 1) / clock_time;
-
-	active_count = (active_time + clock_time - 1) / clock_time;
-
-	recovery_count = (recovery_time + clock_time - 1) / clock_time;
-	recovery_count2 = cycle_count - (setup_count + active_count);
-	if (recovery_count2 > recovery_count)
-		recovery_count = recovery_count2;
+
+	setup_count  = quantize_timing( setup_time, clock_time);
+	cycle_count  = quantize_timing( cycle_time, clock_time);
+	active_count = quantize_timing(active_time, clock_time);
+
+	recovery_count = cycle_count - active_count;
+	/* program_drive_counts() takes care of zero recovery cycles */
 	if (recovery_count > 16) {
 		active_count += recovery_count - 16;
 		recovery_count = 16;
 	}
 	if (active_count > 16)
-		active_count = 16; /* maximum allowed by cmd646 */
+		active_count = 16; /* maximum allowed by cmd64x */
 
 	program_drive_counts (drive, setup_count, active_count, recovery_count);
 
 	cmdprintk("%s: PIO mode wanted %d, selected %d (%dns)%s, "
 		"clocks=%d/%d/%d\n",
 		drive->name, mode_wanted, pio_mode, cycle_time,
-		d.overridden ? " (overriding vendor mode)" : "",
+		pio.overridden ? " (overriding vendor mode)" : "",
 		setup_count, active_count, recovery_count);
 
 	return pio_mode;

Bartlomiej Zolnierkiewicz

2007-03-02 22:20 UTC #65 lore raw

Re: [PATCH] (pata-2.6 fix queue) cmd64x: fix recovery time calculation (take 2)

Quoted 12 lines · Sergei Shtylyov

The driver wrongly takes the address setup time into account when calculating the PIO recovery time -- this leads to slight overclocking of the PIO modes 0 and 1 (so, the prayers failed to help, as usual :-). Rework the code to be calculating recovery clock count as a difference between the total cycle count and the active count (we don't need to calculate the recovery time itself since it's not specified for the PIO modes 0 to 2, and for modes 3 and 4 this formula gives enough recovery time anyway in the chip's supported PCI frequency range).
This patch has been inspired by reading the datasheets and looking at what the libata driver does; it has been compile-tested only (as usual :-) but anyway, the new code gives the same or longer recovery times than the old one...

Quoted 7 lines

Signed-off-by Sergei Shtylyov <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
Just discovered a missed rename in this patch -- it's always a good idea to enable a debugging option when compiling. :-)
The later patches should be still applying without issues...

I replaced the old patch with this one, thanks

Sergei Shtylyov

author

2007-03-03 20:17 UTC #66 lore raw

PATCH 1/1 cmd64x: fix recovery time calculation (take 3)

In reply to Sergei Shtylyov

[PATCH] cmd64x: fix recovery time calculation

Signed-off-by Sergei Shtylyov <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
This patch has been changed in accordance to the pending reordering as DMA support removal and addition patches are going to be merged. In addition, I've put quantize_timing() to its proper place to avoid moving it later...

 drivers/ide/pci/cmd64x.c |   49 +++++++++++++++++++++--------------------------
 1 files changed, 22 insertions(+), 27 deletions(-)

drivers/ide/pci/cmd64x.c +22 ~~-27~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.41	Feb 3, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.42	Feb 8, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -189,6 +189,11 @@ static int cmd64x_get_info (char *buffer
 
 #endif	/* defined(DISPLAY_CMD64X_TIMINGS) && defined(CONFIG_PROC_FS) */
 
+static u8 quantize_timing(int timing, int quant)
+{
+	return (timing + quant - 1) / quant;
+}
+
 /*
  * This routine writes the prepared setup/active/recovery counts
  * for a drive into the cmd646 chipset registers to active them.

@@ -268,47 +273,37 @@ static void program_drive_counts (ide_dr
  */
 static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted)
 {
-	int setup_time, active_time, recovery_time;
-	int clock_time, pio_mode, cycle_time;
-	u8 recovery_count2, cycle_count;
-	int setup_count, active_count, recovery_count;
-	int bus_speed = system_bus_clock();
-	ide_pio_data_t  d;
+	int setup_time, active_time, cycle_time;
+	u8  cycle_count, setup_count, active_count, recovery_count;
+	u8  pio_mode;
+	int clock_time = 1000 / system_bus_clock();
+	ide_pio_data_t pio;
 
-	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &d);
-	cycle_time = d.cycle_time;
+	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &pio);
+	cycle_time = pio.cycle_time;
 
-	/*
-	 * I copied all this complicated stuff from cmd640.c and made a few
-	 * minor changes.  For now I am just going to pray that it is correct.
-	 */
 	setup_time  = ide_pio_timings[pio_mode].setup_time;
 	active_time = ide_pio_timings[pio_mode].active_time;
-	recovery_time = cycle_time - (setup_time + active_time);
-	clock_time = 1000 / bus_speed;
-	cycle_count = (cycle_time + clock_time - 1) / clock_time;
-
-	setup_count = (setup_time + clock_time - 1) / clock_time;
-
-	active_count = (active_time + clock_time - 1) / clock_time;
-
-	recovery_count = (recovery_time + clock_time - 1) / clock_time;
-	recovery_count2 = cycle_count - (setup_count + active_count);
-	if (recovery_count2 > recovery_count)
-		recovery_count = recovery_count2;
+
+	setup_count  = quantize_timing( setup_time, clock_time);
+	cycle_count  = quantize_timing( cycle_time, clock_time);
+	active_count = quantize_timing(active_time, clock_time);
+
+	recovery_count = cycle_count - active_count;
+	/* program_drive_counts() takes care of zero recovery cycles */
 	if (recovery_count > 16) {
 		active_count += recovery_count - 16;
 		recovery_count = 16;
 	}
 	if (active_count > 16)
-		active_count = 16; /* maximum allowed by cmd646 */
+		active_count = 16; /* maximum allowed by cmd64x */
 
 	program_drive_counts (drive, setup_count, active_count, recovery_count);
 
 	cmdprintk("%s: PIO mode wanted %d, selected %d (%dns)%s, "
 		"clocks=%d/%d/%d\n",
 		drive->name, mode_wanted, pio_mode, cycle_time,
-		d.overridden ? " (overriding vendor mode)" : "",
+		pio.overridden ? " (overriding vendor mode)" : "",
 		setup_count, active_count, recovery_count);
 
 	return pio_mode;

Bartlomiej Zolnierkiewicz

2007-03-15 22:12 UTC #67 lore raw

Re: [PATCH pata-2.6 fix queue] cmd64x: fix recovery time calculation (take 3)

Quoted 14 lines · Sergei Shtylyov

[PATCH] cmd64x: fix recovery time calculation
The driver wrongly takes the address setup time into account when calculating the PIO recovery time -- this leads to slight overclocking of the PIO modes 0 and 1 (so, the prayers failed to help, as usual :-). Rework the code to be calculating recovery clock count as a difference between the total cycle count and the active count (we don't need to calculate the recovery time itself since it's not specified for the PIO modes 0 to 2, and for modes 3 and 4 this formula gives enough recovery time anyway in the chip's supported PCI frequency range).
This patch has been inspired by reading the datasheets and looking at what the libata driver does; it has been compile-tested only (as usual :-) but anyway, the new code gives the same or longer recovery times than the old one...

Quoted 7 lines

Signed-off-by Sergei Shtylyov <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
This patch has been changed in accordance to the pending reordering as DMA support removal and addition patches are going to be merged. In addition, I've put quantize_timing() to its proper place to avoid moving it later...

thanks, I replaced the old patch with this one

Sergei Shtylyov

author

2007-02-27 21:49 UTC #68 lore raw

PATCH 1/1 (pata-2.6 fix queue) cmd64x: fix primary channel address setup time

In reply to Sergei Shtylyov

The driver used to merge the address setup timing for the secondary channel but this must have been done for both channels actually despite the primary channel had separate registers for each drive (I'afraid that this is too common mistake in both hardware and software -- address setup timing must be the same accross an IDE channel). Thus, do a number of changes to the program_drive_counts():

- store the address setup count in the 'drive_data' to select the slowest one for the primary channel's drives, and write the selected value into *both* ARRTIM0 and ARTTIM1 registers (just writing 0 to their reserved bits 0 to 5);

- when writing to ARTTIM23 register for the secondary channel, preserve the interrupt status bit; elminate the local_irq_{save|restore}() calls as there is *no* race with the interrupt handler;

- make drwtim_regs[] single-dimensional, indexing it with drive->dn, eliminate now useless index variables;

- replace 'temp_b' variable with 'arttim' and 'drwtim';

- rename {setup|recovery}_counts[] into more fitting {setup|recovery}_values[];

- replace the multiple HWIF() invocation with a single one...

And in addition to those changes also do remove:

- needless and misplaced timing registers initialization in the init_chipset() method;

- meaningless register #define's...

Signed-off-by Sergei Shtylyov <[email protected]>

---
Warning: this has only been compile-tested, as usual.

I've finally decided to make this a separate patch, so the MWDMA support restoring patch is delayed again. :-<

 drivers/ide/pci/cmd64x.c |  106 +++++++++++++++++------------------------------
 1 files changed, 40 insertions(+), 66 deletions(-)

drivers/ide/pci/cmd64x.c +40 ~~-66~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,6 +1,6 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.47	Feb 16, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.48	Feb 26, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
  *           Note, this driver is not used at all on other systems because

@@ -55,9 +55,6 @@
 #define   ARTTIM23_DIS_RA2	0x04
 #define   ARTTIM23_DIS_RA3	0x08
 #define   ARTTIM23_INTR_CH1	0x10
-#define ARTTIM2		0x57
-#define ARTTIM3		0x57
-#define DRWTIM23	0x58
 #define DRWTIM2		0x58
 #define BRST		0x59
 #define DRWTIM3		0x5b

@@ -172,73 +169,62 @@ static int cmd64x_get_info (char *buffer
  * This routine writes the prepared setup/active/recovery counts
  * for a drive into the cmd646 chipset registers to active them.
  */
-static void program_drive_counts (ide_drive_t *drive, int setup_count, int active_count, int recovery_count)
+static void program_drive_counts (ide_drive_t *drive, int setup_count,
+				  int active_count, int recovery_count)
 {
-	unsigned long flags;
-	struct pci_dev *dev = HWIF(drive)->pci_dev;
-	ide_drive_t *drives = HWIF(drive)->drives;
-	u8 temp_b;
-	static const u8 setup_counts[] = {0x40, 0x40, 0x40, 0x80, 0, 0xc0};
-	static const u8 recovery_counts[] =
+	ide_hwif_t *hwif	= HWIF(drive);
+	struct pci_dev *dev	= hwif->pci_dev;
+	ide_drive_t *drives	= hwif->drives;
+	u8 drwtim, arttim	= 0;
+	static const u8 setup_values[] = {0x40, 0x40, 0x40, 0x80, 0, 0xc0};
+	static const u8 recovery_values[] =
 		{15, 15, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 0};
-	static const u8 arttim_regs[2][2] = {
-			{ ARTTIM0, ARTTIM1 },
-			{ ARTTIM23, ARTTIM23 }
-		};
-	static const u8 drwtim_regs[2][2] = {
-			{ DRWTIM0, DRWTIM1 },
-			{ DRWTIM2, DRWTIM3 }
-		};
-	int channel = (int) HWIF(drive)->channel;
-	int slave = (drives != drive);  /* Is this really the best way to determine this?? */
+	static const u8 drwtim_regs[] = {DRWTIM0, DRWTIM1, DRWTIM2, DRWTIM3};
 
 	cmdprintk("program_drive_count parameters = s(%d),a(%d),r(%d),p(%d)\n",
 		setup_count, active_count, recovery_count, drive->present);
+
 	/*
-	 * Set up address setup count registers.
-	 * Primary interface has individual count/timing registers for
-	 * each drive.  Secondary interface has one common set of registers,
-	 * for address setup so we merge these timings, using the slowest
-	 * value.
+	 * The primary channel has individual address setup timing registers
+	 * for each drive, while the secondary channel has one common register.
+	 * We must merge these timings in any case and use the slowest value.
 	 */
-	if (channel) {
-		drive->drive_data = setup_count;
-		setup_count = max(drives[0].drive_data,
-					drives[1].drive_data);
-		cmdprintk("Secondary interface, setup_count = %d\n",
-					setup_count);
-	}
+	drive->drive_data = setup_count;
+	setup_count = max(drives[0].drive_data,	drives[1].drive_data);
+	cmdprintk("Slowest address setup count = %d\n", setup_count);
 
 	/*
 	 * Convert values to internal chipset representation
 	 */
-	setup_count = (setup_count > 5) ? 0xc0 : (int) setup_counts[setup_count];
+	setup_count = (setup_count > 5) ? 0xc0 : setup_values[setup_count];
 	active_count &= 0xf; /* Remember, max value is 16 */
-	recovery_count = (int) recovery_counts[recovery_count];
-
+	recovery_count = recovery_values[recovery_count];
 	cmdprintk("Final values = %d,%d,%d\n",
 		setup_count, active_count, recovery_count);
 
 	/*
-	 * Now that everything is ready, program the new timings
+	 * Program the address setup clocks into the ARTTIM registers.
+	 * Avoid clearing the secondary channel's interrupt bit.
 	 */
-	local_irq_save(flags);
-	/*
-	 * Program the address_setup clocks into ARTTIM reg,
-	 * and then the active/recovery counts into the DRWTIM reg
-	 */
-	(void) pci_read_config_byte(dev, arttim_regs[channel][slave], &temp_b);
-	(void) pci_write_config_byte(dev, arttim_regs[channel][slave],
-		((u8) setup_count) | (temp_b & 0x3f));
-	(void) pci_write_config_byte(dev, drwtim_regs[channel][slave],
-		(u8) ((active_count << 4) | recovery_count));
-	cmdprintk ("Write %x to %x\n",
-		((u8) setup_count) | (temp_b & 0x3f),
-		arttim_regs[channel][slave]);
-	cmdprintk ("Write %x to %x\n",
-		(u8) ((active_count << 4) | recovery_count),
-		drwtim_regs[channel][slave]);
-	local_irq_restore(flags);
+	if (hwif->channel) {
+		(void) pci_read_config_byte (dev, ARTTIM23, &arttim);
+		arttim &= ~ARTTIM23_INTR_CH1;
+		arttim &= ~0xc0;
+		arttim |= setup_count;
+		(void) pci_write_config_byte(dev, ARTTIM23, arttim);
+		cmdprintk ("Write %x to %x\n", arttim, ARTTIM23);
+	} else {
+		arttim = setup_count;
+		(void) pci_write_config_byte(dev, ARTTIM0, arttim);
+		cmdprintk ("Write %x to %x\n", arttim, ARTTIM0);
+		(void) pci_write_config_byte(dev, ARTTIM1, arttim);
+		cmdprintk ("Write %x to %x\n", arttim, ARTTIM1);
+	}
+
+	/* Program the active/recovery counts into the DRWTIM register */
+	drwtim = (active_count << 4) | recovery_count;
+	(void) pci_write_config_byte(dev, drwtim_regs[drive->dn], drwtim);
+	cmdprintk ("Write %x to %x\n", drwtim, drwtim_regs[drive->dn]);
 }
 
 static u8 quantize_timing(int timing, int quant)

@@ -575,18 +561,6 @@ static unsigned int __devinit init_chips
 	 */
 	(void) pci_write_config_byte(dev, MRDMODE, mrdmode | 0x02);
 
-	/* Set reasonable active/recovery/address-setup values. */
-	(void) pci_write_config_byte(dev, ARTTIM0,  0x40);
-	(void) pci_write_config_byte(dev, DRWTIM0,  0x3f);
-	(void) pci_write_config_byte(dev, ARTTIM1,  0x40);
-	(void) pci_write_config_byte(dev, DRWTIM1,  0x3f);
-#ifdef __i386__
-	(void) pci_write_config_byte(dev, ARTTIM23, 0x1c);
-#else
-	(void) pci_write_config_byte(dev, ARTTIM23, 0x5c);
-#endif
-	(void) pci_write_config_byte(dev, DRWTIM23, 0x3f);
-	(void) pci_write_config_byte(dev, DRWTIM3,  0x3f);
 #ifdef CONFIG_PPC
 	(void) pci_write_config_byte(dev, UDIDETCR0, 0xf0);
 #endif /* CONFIG_PPC */

Sergei Shtylyov

author

2007-02-28 13:27 UTC #69 lore raw

Re: [PATCH] (pata-2.6 fix queue) cmd64x: fix primary channel address setup time

Quoted 5 lines · Hello, I wrote:

The driver used to merge the address setup timing for the secondary channel but this must have been done for both channels actually despite the primary channel had separate registers for each drive (I'afraid that this is too common mistake in both hardware and software -- address setup timing must be the same accross an IDE channel). Thus, do a number of changes to the program_drive_counts():

Oops, looks like it was another mind glitch of mine and was fixing a
non-issue -- if I just looked into the spece I would have read there that the hardware itself handles this for the primary channel (the question is WTF they didn't do the same for the secondary one?). So, simpley disregard this patch. I'll send the MWDMA patch soon.

MBR, Sergei

Sergei Shtylyov

author

2007-02-28 20:52 UTC #70 lore raw

PATCH 1/1 (pata-2.6 fix queue) cmd64x: add back MWDMA support

In reply to Sergei Shtylyov

Add back the multiword DMA support (I think nobody will miss single-word DMA). In order to do it, a number of changes had to be done:

- rename program_drive_counts() to program_cycle_times(), pass to it cycle's total/active times instead of the clock counts, and convert them into the active/recovery clocks there instead of cmd64x_tune_pio() -- this causes quantize_timing() to also move;

- contrarywise, move all the code handling the address setup timing into cmd64x_tune_pio(), so that setting MWDMA mode wouldn't change address setup;

- add MWDMA cases to the speedproc() method and handle them by just calling program_cycle_times();

- set hwif->mdwma_mask in the init_hwif() method.

In addition to those changes, do the following:

- when writing to ARTTIM23 register for the secondary channel, preserve the interrupt status bit; eliminate the local_irq_{save|restore}() around this code as there is *no* actual race with interrupt handler;

- make {arttim|drwtim}_regs[] single-dimensional, indexed with drive->dn;

- rename {setup|recovery}_counts[] into more fitting {setup|recovery}_values[];

While at it, also do remove:

- needless and misplaced timing registers initialization in the init_chipset() method;

- meaningless register "aliases";

- meaningless comment about the driver being used only on SPARC Ultra. :-)

Signed-off-by Sergei Shtylyov <[email protected]>

---
Warning: this has only been compile-tested, as usual, so *needs* real testing.

Note that this implementation doesn't take care of properly merging MWDMA and PIO timings which share the same registers (well, that's not done by the most IDE drivers anyway).

Still have no idea about why PPC needs to explicitly disable UltraDMA on the primary channel -- it should be disabled by default...

 drivers/ide/pci/cmd64x.c |  194 +++++++++++++++++++++--------------------------
 1 files changed, 88 insertions(+), 106 deletions(-)

drivers/ide/pci/cmd64x.c +88 ~~-106~~

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
@@ -1,10 +1,8 @@
 /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16
  *
- * linux/drivers/ide/pci/cmd64x.c		Version 1.47	Feb 16, 2007
+ * linux/drivers/ide/pci/cmd64x.c		Version 1.50	Feb 27, 2007
  *
  * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines.
- *           Note, this driver is not used at all on other systems because
- *           there the "BIOS" has done all of the following already.
  *           Due to massive hardware bugs, UltraDMA is only supported
  *           on the 646U2 and not on the 646U.
  *

@@ -55,9 +53,6 @@
 #define   ARTTIM23_DIS_RA2	0x04
 #define   ARTTIM23_DIS_RA3	0x08
 #define   ARTTIM23_INTR_CH1	0x10
-#define ARTTIM2		0x57
-#define ARTTIM3		0x57
-#define DRWTIM23	0x58
 #define DRWTIM2		0x58
 #define BRST		0x59
 #define DRWTIM3		0x5b

@@ -168,122 +163,111 @@ static int cmd64x_get_info (char *buffer
 
 #endif	/* defined(DISPLAY_CMD64X_TIMINGS) && defined(CONFIG_PROC_FS) */
 
+static u8 quantize_timing (int timing, int quant)
+{
+	return (timing + quant - 1) / quant;
+}
+
 /*
- * This routine writes the prepared setup/active/recovery counts
- * for a drive into the cmd646 chipset registers to active them.
+ * This routine calculates active/recovery counts and then writes them into
+ * the chipset registers.
  */
-static void program_drive_counts (ide_drive_t *drive, int setup_count, int active_count, int recovery_count)
+static void program_cycle_times (ide_drive_t *drive, int cycle_time, int active_time)
 {
-	unsigned long flags;
-	struct pci_dev *dev = HWIF(drive)->pci_dev;
-	ide_drive_t *drives = HWIF(drive)->drives;
-	u8 temp_b;
-	static const u8 setup_counts[] = {0x40, 0x40, 0x40, 0x80, 0, 0xc0};
-	static const u8 recovery_counts[] =
+	struct pci_dev *dev	= HWIF(drive)->pci_dev;
+	int clock_time		= 1000 / system_bus_clock();
+	u8  cycle_count, active_count, recovery_count, drwtim;
+	static const u8 recovery_values[] =
 		{15, 15, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 0};
-	static const u8 arttim_regs[2][2] = {
-			{ ARTTIM0, ARTTIM1 },
-			{ ARTTIM23, ARTTIM23 }
-		};
-	static const u8 drwtim_regs[2][2] = {
-			{ DRWTIM0, DRWTIM1 },
-			{ DRWTIM2, DRWTIM3 }
-		};
-	int channel = (int) HWIF(drive)->channel;
-	int slave = (drives != drive);  /* Is this really the best way to determine this?? */
+	static const u8 drwtim_regs[4] = {DRWTIM0, DRWTIM1, DRWTIM2, DRWTIM3};
 
-	cmdprintk("program_drive_count parameters = s(%d),a(%d),r(%d),p(%d)\n",
-		setup_count, active_count, recovery_count, drive->present);
-	/*
-	 * Set up address setup count registers.
-	 * Primary interface has individual count/timing registers for
-	 * each drive.  Secondary interface has one common set of registers,
-	 * for address setup so we merge these timings, using the slowest
-	 * value.
-	 */
-	if (channel) {
-		drive->drive_data = setup_count;
-		setup_count = max(drives[0].drive_data,
-					drives[1].drive_data);
-		cmdprintk("Secondary interface, setup_count = %d\n",
-					setup_count);
-	}
+	cmdprintk("program_cycle_times parameters: total=%d, active=%d\n",
+		  cycle_time, active_time);
+
+	cycle_count	= quantize_timing( cycle_time, clock_time);
+	active_count	= quantize_timing(active_time, clock_time);
+	recovery_count	= cycle_count - active_count;
 
 	/*
-	 * Convert values to internal chipset representation
+	 * In case we've got too long recovery phase, try to lengthen
+	 * the active phase
 	 */
-	setup_count = (setup_count > 5) ? 0xc0 : (int) setup_counts[setup_count];
-	active_count &= 0xf; /* Remember, max value is 16 */
-	recovery_count = (int) recovery_counts[recovery_count];
+	if (recovery_count > 16) {
+		active_count += recovery_count - 16;
+		recovery_count = 16;
+	}
+	if (active_count > 16)		/* shouldn't actually happen... */
+	 	active_count = 16;
 
-	cmdprintk("Final values = %d,%d,%d\n",
-		setup_count, active_count, recovery_count);
+	cmdprintk("Final counts: total=%d, active=%d, recovery=%d\n",
+		  cycle_count, active_count, recovery_count);
 
 	/*
-	 * Now that everything is ready, program the new timings
-	 */
-	local_irq_save(flags);
-	/*
-	 * Program the address_setup clocks into ARTTIM reg,
-	 * and then the active/recovery counts into the DRWTIM reg
+	 * Convert values to internal chipset representation
 	 */
-	(void) pci_read_config_byte(dev, arttim_regs[channel][slave], &temp_b);
-	(void) pci_write_config_byte(dev, arttim_regs[channel][slave],
-		((u8) setup_count) | (temp_b & 0x3f));
-	(void) pci_write_config_byte(dev, drwtim_regs[channel][slave],
-		(u8) ((active_count << 4) | recovery_count));
-	cmdprintk ("Write %x to %x\n",
-		((u8) setup_count) | (temp_b & 0x3f),
-		arttim_regs[channel][slave]);
-	cmdprintk ("Write %x to %x\n",
-		(u8) ((active_count << 4) | recovery_count),
-		drwtim_regs[channel][slave]);
-	local_irq_restore(flags);
-}
+	recovery_count = recovery_values[recovery_count];
+ 	active_count &= 0x0f;
 
-static u8 quantize_timing(int timing, int quant)
-{
-	return (timing + quant - 1) / quant;
+	/* Program the active/recovery counts into the DRWTIM register */
+	drwtim = (active_count << 4) | recovery_count;
+	(void) pci_write_config_byte(dev, drwtim_regs[drive->dn], drwtim);
+	cmdprintk ("Write %x to %x\n", drwtim, drwtim_regs[drive->dn]);
 }
 
 /*
- * This routine selects drive's best PIO mode, calculates setup/active/recovery
- * counts, and then writes them into the chipset registers.
+ * This routine selects drive's best PIO mode and writes into the chipset
+ * registers setup/active/recovery timings.
  */
 static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted)
 {
-	int setup_time, active_time, cycle_time;
-	u8  cycle_count, setup_count, active_count, recovery_count;
-	u8  pio_mode;
-	int clock_time = 1000 / system_bus_clock();
+	ide_hwif_t *hwif	= HWIF(drive);
+	struct pci_dev *dev	= hwif->pci_dev;
 	ide_pio_data_t pio;
+	u8 pio_mode, setup_count, arttim = 0;
+	static const u8 setup_values[] = {0x40, 0x40, 0x40, 0x80, 0, 0xc0};
+	static const u8 arttim_regs[4] = {ARTTIM0, ARTTIM1, ARTTIM23, ARTTIM23};
 
 	pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &pio);
-	cycle_time = pio.cycle_time;
 
-	setup_time  = ide_pio_timings[pio_mode].setup_time;
-	active_time = ide_pio_timings[pio_mode].active_time;
+	program_cycle_times(drive, pio.cycle_time,
+			    ide_pio_timings[pio_mode].active_time);
 
-	setup_count  = quantize_timing( setup_time, clock_time);
-	cycle_count  = quantize_timing( cycle_time, clock_time);
-	active_count = quantize_timing(active_time, clock_time);
+	setup_count = quantize_timing(ide_pio_timings[pio_mode].setup_time,
+				      1000 / system_bus_clock());
+
+	/*
+	 * The primary channel has individual address setup timing registers
+	 * for each drive and the hardware selects the slowest timing itself.
+	 * The secondary channel has one common register and we have to select
+	 * the slowest address setup timing ourselves.
+	 */
+	if (hwif->channel) {
+		ide_drive_t *drives = hwif->drives;
+
+		drive->drive_data = setup_count;
+		setup_count = max(drives[0].drive_data, drives[1].drive_data);
 
-	recovery_count = cycle_count - active_count;
-	/* program_drive_counts() takes care of zero recovery cycles */
-	if (recovery_count > 16) {
-		active_count += recovery_count - 16;
-		recovery_count = 16;
 	}
-	if (active_count > 16)
-		active_count = 16; /* maximum allowed by cmd64x */
 
-	program_drive_counts (drive, setup_count, active_count, recovery_count);
+	if (setup_count > 5)		/* shouldn't actually happen... */
+		setup_count = 5;
+	cmdprintk("Final address setup count = %d\n", setup_count);
 
-	cmdprintk("%s: PIO mode wanted %d, selected %d (%dns)%s, "
-		"clocks=%d/%d/%d\n",
-		drive->name, mode_wanted, pio_mode, cycle_time,
-		pio.overridden ? " (overriding vendor mode)" : "",
-		setup_count, active_count, recovery_count);
+	/*
+	 * Program the address setup clocks into the ARTTIM registers.
+	 * Avoid clearing the secondary channel's interrupt bit.
+	 */
+	(void) pci_read_config_byte (dev, arttim_regs[drive->dn], &arttim);
+	if (hwif->channel)
+		arttim &= ~ARTTIM23_INTR_CH1;
+	arttim &= ~0xc0;
+	arttim |= setup_values[setup_count];
+	(void) pci_write_config_byte(dev, arttim_regs[drive->dn], arttim);
+	cmdprintk ("Write %x to %x\n", arttim, arttim_regs[drive->dn]);
+
+	cmdprintk("%s: PIO mode wanted %d, selected %d (%d ns)%s\n",
+		  drive->name, mode_wanted, pio_mode, pio.cycle_time,
+		  pio.overridden ? " (overriding vendor mode)" : "");
 
 	return pio_mode;
 }

@@ -388,6 +372,15 @@ static int cmd64x_tune_chipset (ide_driv
 	case XFER_UDMA_0:
 		regU |= unit ? 0xC2 : 0x31;
 		break;
+	case XFER_MW_DMA_2:
+		program_cycle_times(drive, 120, 70);
+		break;
+	case XFER_MW_DMA_1:
+		program_cycle_times(drive, 150, 80);
+		break;
+	case XFER_MW_DMA_0:
+		program_cycle_times(drive, 480, 215);
+		break;
 	case XFER_PIO_5:
 	case XFER_PIO_4:
 	case XFER_PIO_3:

@@ -575,18 +568,6 @@ static unsigned int __devinit init_chips
 	 */
 	(void) pci_write_config_byte(dev, MRDMODE, mrdmode | 0x02);
 
-	/* Set reasonable active/recovery/address-setup values. */
-	(void) pci_write_config_byte(dev, ARTTIM0,  0x40);
-	(void) pci_write_config_byte(dev, DRWTIM0,  0x3f);
-	(void) pci_write_config_byte(dev, ARTTIM1,  0x40);
-	(void) pci_write_config_byte(dev, DRWTIM1,  0x3f);
-#ifdef __i386__
-	(void) pci_write_config_byte(dev, ARTTIM23, 0x1c);
-#else
-	(void) pci_write_config_byte(dev, ARTTIM23, 0x5c);
-#endif
-	(void) pci_write_config_byte(dev, DRWTIM23, 0x3f);
-	(void) pci_write_config_byte(dev, DRWTIM3,  0x3f);
 #ifdef CONFIG_PPC
 	(void) pci_write_config_byte(dev, UDIDETCR0, 0xf0);
 #endif /* CONFIG_PPC */

@@ -639,6 +620,7 @@ static void __devinit init_hwif_cmd64x(i
 	hwif->atapi_dma = 1;
 
 	hwif->ultra_mask = 0x3f;
+	hwif->mwdma_mask = 0x07;
 
 	if (dev->device == PCI_DEVICE_ID_CMD_643)
 		hwif->ultra_mask = 0x80;

Bartlomiej Zolnierkiewicz

2007-03-02 22:20 UTC #71 lore raw

Re: [PATCH] (pata-2.6 fix queue) cmd64x: add back MWDMA support

Hi,

Quoted 28 lines · Sergei Shtylyov

Add back the multiword DMA support (I think nobody will miss single-word DMA). In order to do it, a number of changes had to be done:
- rename program_drive_counts() to program_cycle_times(), pass to it cycle's total/active times instead of the clock counts, and convert them into the active/recovery clocks there instead of cmd64x_tune_pio() -- this causes quantize_timing() to also move;
- contrarywise, move all the code handling the address setup timing into cmd64x_tune_pio(), so that setting MWDMA mode wouldn't change address setup;
- add MWDMA cases to the speedproc() method and handle them by just calling program_cycle_times();
- set hwif->mdwma_mask in the init_hwif() method.
In addition to those changes, do the following:
- when writing to ARTTIM23 register for the secondary channel, preserve the interrupt status bit; eliminate the local_irq_{save|restore}() around this code as there is *no* actual race with interrupt handler;
- make {arttim|drwtim}_regs[] single-dimensional, indexed with drive->dn;
- rename {setup|recovery}_counts[] into more fitting {setup|recovery}_values[];
While at it, also do remove:

Quoted 8 lines

- needless and misplaced timing registers initialization in the init_chipset() method;
- meaningless register "aliases";
- meaningless comment about the driver being used only on SPARC Ultra. :-)
Signed-off-by Sergei Shtylyov <[email protected]>

looks fine, applied

Quoted 9 lines

---
Warning: this has only been compile-tested, as usual, so *needs* real testing.
Note that this implementation doesn't take care of properly merging MWDMA and PIO timings which share the same registers (well, that's not done by the most IDE drivers anyway).
Still have no idea about why PPC needs to explicitly disable UltraDMA on the primary channel -- it should be disabled by default...

No idea, probably looking at cmd64x driver version from the time that this quirk was introduced would give some answers...

Thanks,
Bart