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

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

[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, 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 <sshtylyov@ru.mvista.com>

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

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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) {

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

[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. :-)

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

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

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

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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 (davem@redhat.com)
  *
  * Copyright (C) 1999-2002	Andre Hedrick <andre@linux-ide.org>
+ * Copyright (C) 2007		MontaVista Software, Inc. <source@mvista.com>
  */
 
 #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) {

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

[Bartlomiej Zolnierkiewicz]

Hi,

Sergei Shtylyov wrote:
> [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.

> Warning: compile tested only -- getting to the real hardware isn't that easy...
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
>  drivers/ide/pci/cmd64x.c |   95 ++++++++++++++++-------------------------------
>  1 files changed, 34 insertions(+), 61 deletions(-)
> 
> Index: linux-2.6/drivers/ide/pci/cmd64x.c
> ===================================================================
> --- linux-2.6.orig/drivers/ide/pci/cmd64x.c
> +++ 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 (davem@redhat.com)
>   *
>   * Copyright (C) 1999-2002	Andre Hedrick <andre@linux-ide.org>
> + * Copyright (C) 2007		MontaVista Software, Inc. <source@mvista.com>
>   */
>  
>  #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;

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...

[ ... ]
> +/*
> + * 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.

> +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);
> -}

[ ... ]

> @@ -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

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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>[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.

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

>>Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

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

>>Index: linux-2.6/drivers/ide/pci/cmd64x.c
>>===================================================================
>>--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
>>+++ 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 (davem@redhat.com)
>>  *
>>  * Copyright (C) 1999-2002	Andre Hedrick <andre@linux-ide.org>
>>+ * Copyright (C) 2007		MontaVista Software, Inc. <source@mvista.com>
>>  */
>> 
>> #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;

> 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. :-)

> 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.

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

> [ ... ]

>>+/*
>>+ * 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.

    Will do.

>>+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);
>>-}

> [ ... ]

>>@@ -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.

    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? :-/

> 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

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

[Sergei Shtylyov]

Hello, I wrote:

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

>> 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? :-/

>> 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. :-)

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

MBR, Sergei

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

[Bartlomiej Zolnierkiewicz]

Sergei Shtylyov wrote:
> 
> Hello, I wrote:
> 
>>>> Index: linux-2.6/drivers/ide/pci/cmd64x.c
>>>> ===================================================================
>>>> --- linux-2.6.orig/drivers/ide/pci/cmd64x.c
>>>> +++ linux-2.6/drivers/ide/pci/cmd64x.c
> 
>>> 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? :-/
> 
>>> 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. :-)

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

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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

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

>>>>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? :-/

>>>>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. :-)

> 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...

> Bart

MBR, Sergei

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

[Bartlomiej Zolnierkiewicz]

Hi,

Sergei Shtylyov wrote:
> 
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
>>> [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.
> 
>>> Warning: compile tested only -- getting to the real hardware isn't
> that easy...
> 
>>> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
>>> drivers/ide/pci/cmd64x.c |   95
> ++++++++++++++++-------------------------------
>>> 1 files changed, 34 insertions(+), 61 deletions(-)
> 
>>> Index: linux-2.6/drivers/ide/pci/cmd64x.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/ide/pci/cmd64x.c
>>> +++ 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 (davem@redhat.com)
>>>  *
>>>  * Copyright (C) 1999-2002   Andre Hedrick <andre@linux-ide.org>
>>> + * Copyright (C) 2007                MontaVista Software, Inc. <source@mvista.com>
>>>  */
>>>
>>> #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;
> 
>> 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. :-)
> 
>> 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.
> 
>> This will vanish when core code will do filtering of user space
>> PIO mode change requests...
> 
>> [ ... ]
> 
>>> +/*
>>> + * 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.
> 
>    Will do.
> 
>>> +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);
>>> -}
> 
>> [ ... ]
> 
>>> @@ -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.
> 
>    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).

>> 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

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

[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 <sshtylyov@ru.mvista.com>

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

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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 (davem@redhat.com)
  *
  * Copyright (C) 1999-2002	Andre Hedrick <andre@linux-ide.org>
+ * Copyright (C) 2007		MontaVista Software, Inc. <source@mvista.com>
  */
 
 #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;
 

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

[Mikael Pettersson]

Sergei Shtylyov writes:
 > 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

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

[Bartlomiej Zolnierkiewicz]

On Tuesday 06 February 2007 22:11, Mikael Pettersson wrote:
> Sergei Shtylyov writes:
>  > 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.

Thanks!  I updated the patch description accordingly.

Bart

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

[Bartlomiej Zolnierkiewicz]

Sergei Shtylyov wrote:
> 
> 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 <sshtylyov@ru.mvista.com>

thanks for updating the patch

applied

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

[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 <sshtylyov@ru.mvista.com>

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

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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);
 

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

[Bartlomiej Zolnierkiewicz]

On Friday 09 February 2007 23:29, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>

applied

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

[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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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 */

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

[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 <sshtylyov@ru.mvista.com>

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

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

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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[] = {

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

[Bartlomiej Zolnierkiewicz]

On Wednesday 14 February 2007 23:35, Sergei Shtylyov wrote:
> 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 ]

> 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 <sshtylyov@ru.mvista.com>

applied

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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>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. :-<

> [ I corrected this in the applied patch ]

>>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 <sshtylyov@ru.mvista.com>

> applied

    Thanks!

MBR, Sergei

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

[Bartlomiej Zolnierkiewicz]

On Tuesday 20 February 2007, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>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

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

[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 <sshtylyov@ru.mvista.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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[] = {

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

[Bartlomiej Zolnierkiewicz]

On Saturday 14 April 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> ---
> This is an update due to patch order changes.  Has also been tested on PCI-649.

applied

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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 */

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

[Bartlomiej Zolnierkiewicz]

On Thursday 15 February 2007 14:53, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>

applied

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

[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 <sshtylyov@ru.mvista.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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 */

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

[Bartlomiej Zolnierkiewicz]

On Saturday 14 April 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> ---
> This is an update due to patch order changes.  Has also been tested on PCI-649.

applied

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

[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 <sshtylyov@ru.mvista.com>

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

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

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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");

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

[Bartlomiej Zolnierkiewicz]

On Thursday 15 February 2007 20:17, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>

applied

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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);

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

[Bartlomiej Zolnierkiewicz]

On Saturday 14 April 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>
> 
> ---
> 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

[PATCH] (pata-2.6 fix queue) cmd64x:

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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;

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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;

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

[Bartlomiej Zolnierkiewicz]

On Friday 16 February 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>

applied

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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;
 	}
 
 

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

[Bartlomiej Zolnierkiewicz]

On Saturday 14 April 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>
> 
> ---
> 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

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

[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 <sshtylyov@ru.mvista.com>

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

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

Index: linux-2.6/drivers/ide/pci/hpt366.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/hpt366.c
+++ 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 <andre@linux-ide.org>
  * 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 = {

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

[Bartlomiej Zolnierkiewicz]

On Wednesday 18 April 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>

applied

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/aec62xx.c
+++ 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 <andre@linux-ide.org>
+ * Copyright (C) 2007		MontaVista Software, Inc. <source@mvista.com>
  *
  */
 
@@ -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;

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

[Bartlomiej Zolnierkiewicz]

On Sunday 22 April 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>
> 
> ---
> 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

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

[Sergei Shtylyov]

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

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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)

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

[Bartlomiej Zolnierkiewicz]

On Thursday 10 May 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>

applied

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/aec62xx.c
+++ 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 <andre@linux-ide.org>
  * Copyright (C) 2007		MontaVista Software, Inc. <source@mvista.com>
@@ -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[] = {

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

[Bartlomiej Zolnierkiewicz]

On Friday 11 May 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>

applied

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/aec62xx.c
+++ 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 <andre@linux-ide.org>
  * Copyright (C) 2007		MontaVista Software, Inc. <source@mvista.com>
@@ -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}},

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

[Bartlomiej Zolnierkiewicz]

On Friday 11 May 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>

applied

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/aec62xx.c
+++ 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 <andre@linux-ide.org>
  * Copyright (C) 2007		MontaVista Software, Inc. <source@mvista.com>
@@ -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;
 	}
 

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

[Bartlomiej Zolnierkiewicz]

On Friday 11 May 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>

applied

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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>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 <sshtylyov@ru.mvista.com>

> 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

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

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday 16 May 2007, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>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 <sshtylyov@ru.mvista.com>
> 
> > 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

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

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:
>>Bartlomiej Zolnierkiewicz wrote:

>>>>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 <sshtylyov@ru.mvista.com>

>>>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.

> 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.

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

    Probably doesn't worth it...

> Thanks,
> Bart

WBR, Sergei

[PATCH pata-2.6] aec62xx: remove init_dma() method (take 2)

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/aec62xx.c
+++ 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 <andre@linux-ide.org>
  * Copyright (C) 2007		MontaVista Software, Inc. <source@mvista.com>
@@ -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}},

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/aec62xx.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/aec62xx.c
+++ 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 <andre@linux-ide.org>
  * Copyright (C) 2007		MontaVista Software, Inc. <source@mvista.com>
@@ -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;

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

[Bartlomiej Zolnierkiewicz]

On Thursday 24 May 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>
> 
> ---
> 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 <andre@linux-ide.org>
   * Copyright (C) 2007         MontaVista Software, Inc. <source@mvista.com>
--- 1,5 ----
  /*
+  * linux/drivers/ide/pci/aec62xx.c            Version 0.24    May 24, 2007
   *
   * Copyright (C) 1999-2002    Andre Hedrick <andre@linux-ide.org>
   * Copyright (C) 2007         MontaVista Software, Inc. <source@mvista.com>

I don't know why but it was:

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

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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>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 <sshtylyov@ru.mvista.com>

>>---
>>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?

> ***************
> *** 1,5 ****
>   /*
> -  * linux/drivers/ide/pci/aec62xx.c            Version 0.23    May 23, 2007
>    *
>    * Copyright (C) 1999-2002    Andre Hedrick <andre@linux-ide.org>
>    * Copyright (C) 2007         MontaVista Software, Inc. <source@mvista.com>
> --- 1,5 ----
>   /*
> +  * linux/drivers/ide/pci/aec62xx.c            Version 0.24    May 24, 2007
>    *
>    * Copyright (C) 1999-2002    Andre Hedrick <andre@linux-ide.org>
>    * Copyright (C) 2007         MontaVista Software, Inc. <source@mvista.com>
> 
> I don't know why but it was:
> 
> linux/drivers/ide/pci/aec62xx.c             Version 0.23    Apr 27, 2007

MBR, Sergei

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

[Bartlomiej Zolnierkiewicz]

On Tuesday 05 June 2007, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>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 <sshtylyov@ru.mvista.com>
> 
> >>---
> >>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?

Please do.

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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>>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 <sshtylyov@ru.mvista.com>

>>>>---
>>>>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?

> Please do.

    Just have resent it privately.

MBR, Sergei

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

[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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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;
 }

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

[Bartlomiej Zolnierkiewicz]

On Friday 09 November 2007, Sergei Shtylyov wrote:
> 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. :) ]

> ---
> 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
> ===================================================================
> --- linux-2.6.orig/drivers/ide/pci/cmd64x.c
> +++ 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;
>  }
> 
> 

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

[Bartlomiej Zolnierkiewicz]

On Sunday 11 November 2007, Bartlomiej Zolnierkiewicz wrote:
> On Friday 09 November 2007, Sergei Shtylyov wrote:
> > 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

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

[Martin Rogge]

On Tuesday 13 November 2007 21:58:36 Bartlomiej Zolnierkiewicz wrote:
> On Sunday 11 November 2007, Bartlomiej Zolnierkiewicz wrote:
> > On Friday 09 November 2007, Sergei Shtylyov wrote:
> > > 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

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

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

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>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)...

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

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

    Aye aye, sir! :-)

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

    Like not crediting testers? ;-)

> Thanks,
> Bart

WBR, Sergei

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

[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 <sshtylyov@ru.mvista.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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;

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

[Bartlomiej Zolnierkiewicz]

On Monday 26 February 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> ---
> 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

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

[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...

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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;

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

[Bartlomiej Zolnierkiewicz]

On Saturday 03 March 2007, Sergei Shtylyov wrote:
> [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...
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> ---
> 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

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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 */

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

[Sergei Shtylyov]

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

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

[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 <sshtylyov@ru.mvista.com>

---
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(-)

Index: linux-2.6/drivers/ide/pci/cmd64x.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/cmd64x.c
+++ 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;

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

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday 28 February 2007, Sergei Shtylyov wrote:
> 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 <sshtylyov@ru.mvista.com>

looks fine, applied

> ---
> 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