[PATCH 3/6] siimage: PIO mode setup fixes

[PATCH 3/6] siimage: PIO mode setup fixes

[Bartlomiej Zolnierkiewicz]

* Add sil_tuneproc() wrapper for siimage_tuneproc() which also sets
  PIO mode on the device.

* Add missing ide_get_best_pio_mode() call to sil_tuneproc() so
  "pio" == 255 (autotune) is handled correctly (previously PIO0 was used)
  and "pio" values > 4 && < 255 are filtered to PIO4 (instead of PIO0).

* Add code limiting maximum PIO mode according to the pair device capabilities
  to sil_tuneproc().

* Convert users of config_siimage_chipset_for_pio() to use sil_tune_pio() and
  sil_tuneproc().  This fixes PIO fallback in siimage_config_drive_for_dma() to
  use max PIO mode available instead of PIO4 (config_siimage_chipset_for_pio()
  used wrong arguments for ide_get_best_pio_mode() and as a results always
  tried to set PIO4).

* Remove no longer needed siimage_taskfile_timing()
  and config_siimage_chipset_for_pio().

* Bump driver version.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/pci/siimage.c |  108 +++++++++++++++-------------------------------
 1 file changed, 36 insertions(+), 72 deletions(-)

Index: b/drivers/ide/pci/siimage.c
===================================================================
--- a/drivers/ide/pci/siimage.c
+++ b/drivers/ide/pci/siimage.c
@@ -1,9 +1,10 @@
 /*
- * linux/drivers/ide/pci/siimage.c		Version 1.12	Mar 10 2007
+ * linux/drivers/ide/pci/siimage.c		Version 1.15	Jun 16 2007
  *
  * Copyright (C) 2001-2002	Andre Hedrick <andre@linux-ide.org>
  * Copyright (C) 2003		Red Hat <alan@redhat.com>
  * Copyright (C) 2007		MontaVista Software, Inc.
+ * Copyright (C) 2007		Bartlomiej Zolnierkiewicz
  *
  *  May be copied or modified under the terms of the GNU General Public License
  *
@@ -31,6 +32,9 @@
  *  unplugging/replugging the virtual CD interface when the DRAC is reset.
  *  This often causes drivers/ide/siimage to panic but is ok with the rather
  *  smarter code in libata.
+ *
+ * TODO:
+ * - VDMA support
  */
 
 #include <linux/types.h>
@@ -160,54 +164,39 @@ out:
 }
 
 /**
- *	siimage_taskfile_timing	-	turn timing data to a mode
- *	@hwif: interface to query
- *
- *	Read the timing data for the interface and return the 
- *	mode that is being used.
- */
- 
-static byte siimage_taskfile_timing (ide_hwif_t *hwif)
-{
-	u16 timing	= 0x328a;
-	unsigned long addr = siimage_selreg(hwif, 2);
-
-	if (hwif->mmio)
-		timing = hwif->INW(addr);
-	else
-		pci_read_config_word(hwif->pci_dev, addr, &timing);
-
-	switch (timing) {
-		case 0x10c1:	return 4;
-		case 0x10c3:	return 3;
-		case 0x1104:
-		case 0x1281:	return 2;
-		case 0x2283:	return 1;
-		case 0x328a:
-		default:	return 0;
-	}
-}
-
-/**
- *	simmage_tuneproc	-	tune a drive
+ *	sil_tune_pio	-	tune a drive
  *	@drive: drive to tune
- *	@mode_wanted: the target operating mode
+ *	@pio: the desired PIO mode
  *
  *	Load the timing settings for this device mode into the
  *	controller. If we are in PIO mode 3 or 4 turn on IORDY
  *	monitoring (bit 9). The TF timing is bits 31:16
  */
- 
-static void siimage_tuneproc (ide_drive_t *drive, byte mode_wanted)
+
+static void sil_tune_pio(ide_drive_t *drive, u8 pio)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	ide_drive_t *pair	= &hwif->drives[1 - drive->select.b.unit];
 	u32 speedt		= 0;
 	u16 speedp		= 0;
 	unsigned long addr	= siimage_seldev(drive, 0x04);
 	unsigned long tfaddr	= siimage_selreg(hwif, 0x02);
-	
+
+	/*
+	 * Compute the best PIO mode we can for a given device. We must
+	 * pick a speed that does not cause problems with the other device
+	 * on the cable.
+	 */
+	if (pair) {
+		u8 pair_pio = ide_get_best_pio_mode(pair, 255, 4, NULL);
+
+		/* trim PIO to the slowest of the master/slave */
+		if (pair_pio < pio)
+			pio = pair_pio;
+	}
+
 	/* cheat for now and use the docs */
-	switch (mode_wanted) {
+	switch (pio) {
 	case 4:
 		speedp = 0x10c1;
 		speedt = 0x10c1;
@@ -235,7 +224,7 @@ static void siimage_tuneproc (ide_drive_
 		hwif->OUTW(speedp, addr);
 		hwif->OUTW(speedt, tfaddr);
 		/* Now set up IORDY */
-		if(mode_wanted == 3 || mode_wanted == 4)
+		if (pio == 3 || pio == 4)
 			hwif->OUTW(hwif->INW(tfaddr-2)|0x200, tfaddr-2);
 		else
 			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);
@@ -245,42 +234,17 @@ static void siimage_tuneproc (ide_drive_
 		pci_read_config_word(hwif->pci_dev, tfaddr-2, &speedp);
 		speedp &= ~0x200;
 		/* Set IORDY for mode 3 or 4 */
-		if(mode_wanted == 3 || mode_wanted == 4)
+		if (pio == 3 || pio == 4)
 			speedp |= 0x200;
 		pci_write_config_word(hwif->pci_dev, tfaddr-2, speedp);
 	}
 }
 
-/**
- *	config_siimage_chipset_for_pio	-	set drive timings
- *	@drive: drive to tune
- *	@speed we want
- *
- *	Compute the best pio mode we can for a given device. Also honour
- *	the timings for the driver when dealing with mixed devices. Some
- *	of this is ugly but its all wrapped up here
- *
- *	The SI680 can also do VDMA - we need to start using that
- *
- *	FIXME: we use the BIOS channel timings to avoid driving the task
- *	files too fast at the disk. We need to compute the master/slave
- *	drive PIO mode properly so that we can up the speed on a hotplug
- *	system.
- */
- 
-static void config_siimage_chipset_for_pio (ide_drive_t *drive, byte set_speed)
+static void sil_tuneproc(ide_drive_t *drive, u8 pio)
 {
-	u8 channel_timings	= siimage_taskfile_timing(HWIF(drive));
-	u8 speed = 0, set_pio	= ide_get_best_pio_mode(drive, 4, 5, NULL);
-
-	/* WARNING PIO timing mess is going to happen b/w devices, argh */
-	if ((channel_timings != set_pio) && (set_pio > channel_timings))
-		set_pio = channel_timings;
-
-	siimage_tuneproc(drive, set_pio);
-	speed = XFER_PIO_0 + set_pio;
-	if (set_speed)
-		(void) ide_config_drive_speed(drive, speed);
+	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);
+	sil_tune_pio(drive, pio);
+	(void)ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
 
 /**
@@ -335,7 +299,7 @@ static int siimage_tune_chipset (ide_dri
 		case XFER_PIO_2:
 		case XFER_PIO_1:
 		case XFER_PIO_0:
-			siimage_tuneproc(drive, (speed - XFER_PIO_0));
+			sil_tune_pio(drive, speed - XFER_PIO_0);
 			mode |= ((unit) ? 0x10 : 0x01);
 			break;
 		case XFER_MW_DMA_2:
@@ -343,7 +307,7 @@ static int siimage_tune_chipset (ide_dri
 		case XFER_MW_DMA_0:
 			multi = dma[speed - XFER_MW_DMA_0];
 			mode |= ((unit) ? 0x20 : 0x02);
-			config_siimage_chipset_for_pio(drive, 0);
+			sil_tune_pio(drive, 4); /* FIXME */
 			break;
 		case XFER_UDMA_6:
 		case XFER_UDMA_5:
@@ -356,7 +320,7 @@ static int siimage_tune_chipset (ide_dri
 			ultra |= ((scsc) ? (ultra6[speed - XFER_UDMA_0]) :
 					   (ultra5[speed - XFER_UDMA_0]));
 			mode |= ((unit) ? 0x30 : 0x03);
-			config_siimage_chipset_for_pio(drive, 0);
+			sil_tune_pio(drive, 4); /* FIXME */
 			break;
 		default:
 			return 1;
@@ -390,7 +354,7 @@ static int siimage_config_drive_for_dma 
 		return 0;
 
 	if (ide_use_fast_pio(drive))
-		config_siimage_chipset_for_pio(drive, 1);
+		sil_tuneproc(drive, 255);
 
 	return -1;
 }
@@ -961,7 +925,7 @@ static void __devinit init_hwif_siimage(
 	
 	hwif->resetproc = &siimage_reset;
 	hwif->speedproc = &siimage_tune_chipset;
-	hwif->tuneproc	= &siimage_tuneproc;
+	hwif->tuneproc	= &sil_tuneproc;
 	hwif->reset_poll = &siimage_reset_poll;
 	hwif->pre_reset = &siimage_pre_reset;
 	hwif->udma_filter = &sil_udma_filter;

Re: [PATCH 3/6] siimage: PIO mode setup fixes

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

    A lot to argue about here...

> * Add sil_tuneproc() wrapper for siimage_tuneproc() which also sets
>   PIO mode on the device.

    Planning on the global prefix change? :-)

> * Add code limiting maximum PIO mode according to the pair device capabilities
>   to sil_tuneproc().

    Ugh... that part is terrible. :-/
    Actually, we only need to limit the taskfile, not the data transfers --
unlike it was done before.

> * Remove no longer needed siimage_taskfile_timing()
>   and config_siimage_chipset_for_pio().

    Yeah, time to get rid of that garbage. :-)

> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

    Note that PIO setup keeps being somewhat borken IODRY-wise even with this
patch as sil_tune_pio() only controls taskfile IORDY sampling -- the Right
Thing could only be done via speedproc() method...

> Index: b/drivers/ide/pci/siimage.c
> ===================================================================
> --- a/drivers/ide/pci/siimage.c
> +++ b/drivers/ide/pci/siimage.c
[...]
> -/**
> - *	simmage_tuneproc	-	tune a drive
> + *	sil_tune_pio	-	tune a drive
>   *	@drive: drive to tune
> - *	@mode_wanted: the target operating mode
> + *	@pio: the desired PIO mode
>   *
>   *	Load the timing settings for this device mode into the
>   *	controller. If we are in PIO mode 3 or 4 turn on IORDY
>   *	monitoring (bit 9). The TF timing is bits 31:16
>   */
> - 
> -static void siimage_tuneproc (ide_drive_t *drive, byte mode_wanted)
> +
> +static void sil_tune_pio(ide_drive_t *drive, u8 pio)
>  {
>  	ide_hwif_t *hwif	= HWIF(drive);
> +	ide_drive_t *pair	= &hwif->drives[1 - drive->select.b.unit];

    I'd suggest simpler [drive->dn ^ 1]...

>  	u32 speedt		= 0;
>  	u16 speedp		= 0;
>  	unsigned long addr	= siimage_seldev(drive, 0x04);
>  	unsigned long tfaddr	= siimage_selreg(hwif, 0x02);
> -	
> +
> +	/*
> +	 * Compute the best PIO mode we can for a given device. We must
> +	 * pick a speed that does not cause problems with the other device
> +	 * on the cable.
> +	 */
> +	if (pair) {

    Huh? It can *not* really be NULL.

> +		u8 pair_pio = ide_get_best_pio_mode(pair, 255, 4, NULL);

    I'm not quite sure it's safe enough to trim to the maximum supported mode 
of the other drive -- the current mode would be the safest bet... well, after 
referring to the ATA spec., it's considered safe enough there.

> +
> +		/* trim PIO to the slowest of the master/slave */
> +		if (pair_pio < pio)
> +			pio = pair_pio;

    No need to trim the *data* PIO mode.

> +	}
> +
>  	/* cheat for now and use the docs */
> -	switch (mode_wanted) {
> +	switch (pio) {
>  	case 4:
>  		speedp = 0x10c1;
>  		speedt = 0x10c1;

    What I envisioned was putting speedt into drive->drive_data, calculating 
the maximum value for 2 drives and using it to actually program the taskfile 
timing. Either that or put PIO mode there, and add the second switch to 
calculate the taskfile timings after getting the minimum PIO mode for 2 drives 
(but that's not as neat).

> @@ -235,7 +224,7 @@ static void siimage_tuneproc (ide_drive_
>  		hwif->OUTW(speedp, addr);
>  		hwif->OUTW(speedt, tfaddr);
>  		/* Now set up IORDY */
> -		if(mode_wanted == 3 || mode_wanted == 4)
> +		if (pio == 3 || pio == 4)

    Why not just (pio > 2)?

>  			hwif->OUTW(hwif->INW(tfaddr-2)|0x200, tfaddr-2);

    Erm, the same comments about taskfile IORDY as before: it should be 
selected if the drive supports it. In fact, if either of 2 drives do.

>  		else
>  			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);

    This is wrong logic: thus we may turn off IORDY although the 2nd drive may 
support it.

> @@ -245,42 +234,17 @@ static void siimage_tuneproc (ide_drive_
>  		pci_read_config_word(hwif->pci_dev, tfaddr-2, &speedp);
>  		speedp &= ~0x200;
>  		/* Set IORDY for mode 3 or 4 */
> -		if(mode_wanted == 3 || mode_wanted == 4)
> +		if (pio == 3 || pio == 4)
>  			speedp |= 0x200;
>  		pci_write_config_word(hwif->pci_dev, tfaddr-2, speedp);
>  	}
>  }

    Same here...

[...]
> @@ -335,7 +299,7 @@ static int siimage_tune_chipset (ide_dri
>  		case XFER_PIO_2:
>  		case XFER_PIO_1:
>  		case XFER_PIO_0:
> -			siimage_tuneproc(drive, (speed - XFER_PIO_0));
> +			sil_tune_pio(drive, speed - XFER_PIO_0);
>  			mode |= ((unit) ? 0x10 : 0x01);

    The last line enables IORDY sampling for data transfers.

>  			break;
>  		case XFER_MW_DMA_2:
> @@ -343,7 +307,7 @@ static int siimage_tune_chipset (ide_dri
>  		case XFER_MW_DMA_0:
>  			multi = dma[speed - XFER_MW_DMA_0];
>  			mode |= ((unit) ? 0x20 : 0x02);

    ... and this line also enables IORDY. And the one in UltraDMA case group too.

> -			config_siimage_chipset_for_pio(drive, 0);
> +			sil_tune_pio(drive, 4); /* FIXME */

    Why we still need this nonsense here...

>  			break;
>  		case XFER_UDMA_6:
>  		case XFER_UDMA_5:
> @@ -356,7 +320,7 @@ static int siimage_tune_chipset (ide_dri
>  			ultra |= ((scsc) ? (ultra6[speed - XFER_UDMA_0]) :
>  					   (ultra5[speed - XFER_UDMA_0]));
>  			mode |= ((unit) ? 0x30 : 0x03);
> -			config_siimage_chipset_for_pio(drive, 0);
> +			sil_tune_pio(drive, 4); /* FIXME */

    ... and here? If we so desperately want to setup PIO data/taskfile
timings, it's better to do via setting the 'autotune' field unconditionally.

>  			break;
>  		default:
>  			return 1;

MBR, Sergei

Re: [PATCH 3/6] siimage: PIO mode setup fixes

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday 26 June 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
>     A lot to argue about here...
> 
> > * Add sil_tuneproc() wrapper for siimage_tuneproc() which also sets
> >   PIO mode on the device.
> 
>     Planning on the global prefix change? :-)

Yep.

> > * Add code limiting maximum PIO mode according to the pair device capabilities
> >   to sil_tuneproc().
> 
>     Ugh... that part is terrible. :-/
>     Actually, we only need to limit the taskfile, not the data transfers --
> unlike it was done before.

Fixed.

> > * Remove no longer needed siimage_taskfile_timing()
> >   and config_siimage_chipset_for_pio().
> 
>     Yeah, time to get rid of that garbage. :-)
> 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
>     Note that PIO setup keeps being somewhat borken IODRY-wise even with this
> patch as sil_tune_pio() only controls taskfile IORDY sampling -- the Right
> Thing could only be done via speedproc() method...

After rehashing the datasheet I see the source of the issue:

IORDY is controlled in two registers and moreover it is always enabled
if MDMA or UDMA transfer modes are selected.

> > Index: b/drivers/ide/pci/siimage.c
> > ===================================================================
> > --- a/drivers/ide/pci/siimage.c
> > +++ b/drivers/ide/pci/siimage.c
> [...]
> > -/**
> > - *	simmage_tuneproc	-	tune a drive
> > + *	sil_tune_pio	-	tune a drive
> >   *	@drive: drive to tune
> > - *	@mode_wanted: the target operating mode
> > + *	@pio: the desired PIO mode
> >   *
> >   *	Load the timing settings for this device mode into the
> >   *	controller. If we are in PIO mode 3 or 4 turn on IORDY
> >   *	monitoring (bit 9). The TF timing is bits 31:16
> >   */
> > - 
> > -static void siimage_tuneproc (ide_drive_t *drive, byte mode_wanted)
> > +
> > +static void sil_tune_pio(ide_drive_t *drive, u8 pio)
> >  {
> >  	ide_hwif_t *hwif	= HWIF(drive);
> > +	ide_drive_t *pair	= &hwif->drives[1 - drive->select.b.unit];
> 
>     I'd suggest simpler [drive->dn ^ 1]...

Fixed.

> >  	u32 speedt		= 0;
> >  	u16 speedp		= 0;
> >  	unsigned long addr	= siimage_seldev(drive, 0x04);
> >  	unsigned long tfaddr	= siimage_selreg(hwif, 0x02);
> > -	
> > +
> > +	/*
> > +	 * Compute the best PIO mode we can for a given device. We must
> > +	 * pick a speed that does not cause problems with the other device
> > +	 * on the cable.
> > +	 */
> > +	if (pair) {
> 
>     Huh? It can *not* really be NULL.

It was supposed to be pair->present, fixed.  Thanks!

> > +		u8 pair_pio = ide_get_best_pio_mode(pair, 255, 4, NULL);
> 
>     I'm not quite sure it's safe enough to trim to the maximum supported mode 
> of the other drive -- the current mode would be the safest bet... well, after 
> referring to the ATA spec., it's considered safe enough there.
> 
> > +
> > +		/* trim PIO to the slowest of the master/slave */
> > +		if (pair_pio < pio)
> > +			pio = pair_pio;
> 
>     No need to trim the *data* PIO mode.

Fixed.

> > +	}
> > +
> >  	/* cheat for now and use the docs */
> > -	switch (mode_wanted) {
> > +	switch (pio) {
> >  	case 4:
> >  		speedp = 0x10c1;
> >  		speedt = 0x10c1;
> 
>     What I envisioned was putting speedt into drive->drive_data, calculating 
> the maximum value for 2 drives and using it to actually program the taskfile 
> timing. Either that or put PIO mode there, and add the second switch to 
> calculate the taskfile timings after getting the minimum PIO mode for 2 drives 
> (but that's not as neat).

I did something similar to the second solution (should be sufficient for now)
but improvenments are welcomed.

> > @@ -235,7 +224,7 @@ static void siimage_tuneproc (ide_drive_
> >  		hwif->OUTW(speedp, addr);
> >  		hwif->OUTW(speedt, tfaddr);
> >  		/* Now set up IORDY */
> > -		if(mode_wanted == 3 || mode_wanted == 4)
> > +		if (pio == 3 || pio == 4)
> 
>     Why not just (pio > 2)?

Fixed.

> >  			hwif->OUTW(hwif->INW(tfaddr-2)|0x200, tfaddr-2);
> 
>     Erm, the same comments about taskfile IORDY as before: it should be 
> selected if the drive supports it. In fact, if either of 2 drives do.
> 
> >  		else
> >  			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);
> 
>     This is wrong logic: thus we may turn off IORDY although the 2nd drive may 
> support it.

Indeed, but since I don't want to be selfish and keep all bugfixes to myself
I'm giving other people opportunity to fix it. :-)

ditto for ->speedproc vs IORDY problems

> > @@ -245,42 +234,17 @@ static void siimage_tuneproc (ide_drive_
> >  		pci_read_config_word(hwif->pci_dev, tfaddr-2, &speedp);
> >  		speedp &= ~0x200;
> >  		/* Set IORDY for mode 3 or 4 */
> > -		if(mode_wanted == 3 || mode_wanted == 4)
> > +		if (pio == 3 || pio == 4)
> >  			speedp |= 0x200;
> >  		pci_write_config_word(hwif->pci_dev, tfaddr-2, speedp);
> >  	}
> >  }
> 
>     Same here...

Fixed.

> [...]
> > @@ -335,7 +299,7 @@ static int siimage_tune_chipset (ide_dri
> >  		case XFER_PIO_2:
> >  		case XFER_PIO_1:
> >  		case XFER_PIO_0:
> > -			siimage_tuneproc(drive, (speed - XFER_PIO_0));
> > +			sil_tune_pio(drive, speed - XFER_PIO_0);
> >  			mode |= ((unit) ? 0x10 : 0x01);
> 
>     The last line enables IORDY sampling for data transfers.
> 
> >  			break;
> >  		case XFER_MW_DMA_2:
> > @@ -343,7 +307,7 @@ static int siimage_tune_chipset (ide_dri
> >  		case XFER_MW_DMA_0:
> >  			multi = dma[speed - XFER_MW_DMA_0];
> >  			mode |= ((unit) ? 0x20 : 0x02);
> 
>     ... and this line also enables IORDY. And the one in UltraDMA case group too.
> 
> > -			config_siimage_chipset_for_pio(drive, 0);
> > +			sil_tune_pio(drive, 4); /* FIXME */
> 
>     Why we still need this nonsense here...

I was _really_ hoping that /* FIXME */ would make this clear. ;-)

> >  			break;
> >  		case XFER_UDMA_6:
> >  		case XFER_UDMA_5:
> > @@ -356,7 +320,7 @@ static int siimage_tune_chipset (ide_dri
> >  			ultra |= ((scsc) ? (ultra6[speed - XFER_UDMA_0]) :
> >  					   (ultra5[speed - XFER_UDMA_0]));
> >  			mode |= ((unit) ? 0x30 : 0x03);
> > -			config_siimage_chipset_for_pio(drive, 0);
> > +			sil_tune_pio(drive, 4); /* FIXME */
> 
>     ... and here? If we so desperately want to setup PIO data/taskfile
> timings, it's better to do via setting the 'autotune' field unconditionally.

I had a follow-up patch doing exactly that (done later than this patch).
I integrated it into current patch since there was a need for respin...

take 2 follows:

[PATCH] siimage: PIO mode setup fixes (take 2)

* Add sil_tuneproc() wrapper for siimage_tuneproc() which also sets
  PIO mode on the device.

* Add missing ide_get_best_pio_mode() call to sil_tuneproc() so
  "pio" == 255 (autotune) is handled correctly (previously PIO0 was used)
  and "pio" values > 4 && < 255 are filtered to PIO4 (instead of PIO0).

* Add code limiting maximum PIO mode according to the pair device capabilities
  to sil_tuneproc().

* Convert users of config_siimage_chipset_for_pio() to use sil_tune_pio() and
  sil_tuneproc().  This fixes PIO fallback in siimage_config_drive_for_dma() to
  use max PIO mode available instead of PIO4 (config_siimage_chipset_for_pio()
  used wrong arguments for ide_get_best_pio_mode() and as a results always
  tried to set PIO4).

* Remove no longer needed siimage_taskfile_timing()
  and config_siimage_chipset_for_pio().

* Enable ->autotune unconditionally and remove PIO tuning for UDMA/MDMA modes
  from siimage_speedproc()

* Bump driver version.

v2:
* Fix issues noticed by Sergei:
  - correct pair device check
  - trim only taskfile PIO to the slowest of the master/slave
  - enable ->autotune unconditionally and remove PIO tuning for UDMA/MDMA modes
    from siimage_speedproc()
  - add TODO item for IORDY bugs
  - minor cleanups

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Reviewed-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
---
 drivers/ide/pci/siimage.c |  137 +++++++++++++---------------------------------
 1 file changed, 39 insertions(+), 98 deletions(-)

Index: b/drivers/ide/pci/siimage.c
===================================================================
--- a/drivers/ide/pci/siimage.c
+++ b/drivers/ide/pci/siimage.c
@@ -1,9 +1,10 @@
 /*
- * linux/drivers/ide/pci/siimage.c		Version 1.12	Mar 10 2007
+ * linux/drivers/ide/pci/siimage.c		Version 1.15	Jun 29 2007
  *
  * Copyright (C) 2001-2002	Andre Hedrick <andre@linux-ide.org>
  * Copyright (C) 2003		Red Hat <alan@redhat.com>
  * Copyright (C) 2007		MontaVista Software, Inc.
+ * Copyright (C) 2007		Bartlomiej Zolnierkiewicz
  *
  *  May be copied or modified under the terms of the GNU General Public License
  *
@@ -31,6 +32,10 @@
  *  unplugging/replugging the virtual CD interface when the DRAC is reset.
  *  This often causes drivers/ide/siimage to panic but is ok with the rather
  *  smarter code in libata.
+ *
+ * TODO:
+ * - IORDY fixes
+ * - VDMA support
  */
 
 #include <linux/types.h>
@@ -160,82 +165,45 @@ out:
 }
 
 /**
- *	siimage_taskfile_timing	-	turn timing data to a mode
- *	@hwif: interface to query
- *
- *	Read the timing data for the interface and return the 
- *	mode that is being used.
- */
- 
-static byte siimage_taskfile_timing (ide_hwif_t *hwif)
-{
-	u16 timing	= 0x328a;
-	unsigned long addr = siimage_selreg(hwif, 2);
-
-	if (hwif->mmio)
-		timing = hwif->INW(addr);
-	else
-		pci_read_config_word(hwif->pci_dev, addr, &timing);
-
-	switch (timing) {
-		case 0x10c1:	return 4;
-		case 0x10c3:	return 3;
-		case 0x1104:
-		case 0x1281:	return 2;
-		case 0x2283:	return 1;
-		case 0x328a:
-		default:	return 0;
-	}
-}
-
-/**
- *	simmage_tuneproc	-	tune a drive
+ *	sil_tune_pio	-	tune a drive
  *	@drive: drive to tune
- *	@mode_wanted: the target operating mode
+ *	@pio: the desired PIO mode
  *
  *	Load the timing settings for this device mode into the
  *	controller. If we are in PIO mode 3 or 4 turn on IORDY
  *	monitoring (bit 9). The TF timing is bits 31:16
  */
- 
-static void siimage_tuneproc (ide_drive_t *drive, byte mode_wanted)
+
+static void sil_tune_pio(ide_drive_t *drive, u8 pio)
 {
+	const u16 tf_speed[]	= { 0x328a, 0x2283, 0x1281, 0x10c3, 0x10c1 };
+	const u16 data_speed[]	= { 0x328a, 0x2283, 0x1104, 0x10c3, 0x10c1 };
+
 	ide_hwif_t *hwif	= HWIF(drive);
+	ide_drive_t *pair	= &hwif->drives[drive->dn ^ 1];
 	u32 speedt		= 0;
 	u16 speedp		= 0;
 	unsigned long addr	= siimage_seldev(drive, 0x04);
 	unsigned long tfaddr	= siimage_selreg(hwif, 0x02);
-	
-	/* cheat for now and use the docs */
-	switch (mode_wanted) {
-	case 4:
-		speedp = 0x10c1;
-		speedt = 0x10c1;
-		break;
-	case 3:
-		speedp = 0x10c3;
-		speedt = 0x10c3;
-		break;
-	case 2:
-		speedp = 0x1104;
-		speedt = 0x1281;
-		break;
-	case 1:
-		speedp = 0x2283;
-		speedt = 0x2283;
-		break;
-	case 0:
-	default:
-		speedp = 0x328a;
-		speedt = 0x328a;
-		break;
+	u8 tf_pio		= pio;
+
+	/* trim *taskfile* PIO to the slowest of the master/slave */
+	if (pair->present) {
+		u8 pair_pio = ide_get_best_pio_mode(pair, 255, 4, NULL);
+
+		if (pair_pio < tf_pio)
+			tf_pio = pair_pio;
 	}
 
+	/* cheat for now and use the docs */
+	speedp = data_speed[pio];
+	speedt = tf_speed[tf_pio];
+
 	if (hwif->mmio) {
 		hwif->OUTW(speedp, addr);
 		hwif->OUTW(speedt, tfaddr);
 		/* Now set up IORDY */
-		if(mode_wanted == 3 || mode_wanted == 4)
+		if (pio > 2)
 			hwif->OUTW(hwif->INW(tfaddr-2)|0x200, tfaddr-2);
 		else
 			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);
@@ -245,42 +213,17 @@ static void siimage_tuneproc (ide_drive_
 		pci_read_config_word(hwif->pci_dev, tfaddr-2, &speedp);
 		speedp &= ~0x200;
 		/* Set IORDY for mode 3 or 4 */
-		if(mode_wanted == 3 || mode_wanted == 4)
+		if (pio > 2)
 			speedp |= 0x200;
 		pci_write_config_word(hwif->pci_dev, tfaddr-2, speedp);
 	}
 }
 
-/**
- *	config_siimage_chipset_for_pio	-	set drive timings
- *	@drive: drive to tune
- *	@speed we want
- *
- *	Compute the best pio mode we can for a given device. Also honour
- *	the timings for the driver when dealing with mixed devices. Some
- *	of this is ugly but its all wrapped up here
- *
- *	The SI680 can also do VDMA - we need to start using that
- *
- *	FIXME: we use the BIOS channel timings to avoid driving the task
- *	files too fast at the disk. We need to compute the master/slave
- *	drive PIO mode properly so that we can up the speed on a hotplug
- *	system.
- */
- 
-static void config_siimage_chipset_for_pio (ide_drive_t *drive, byte set_speed)
+static void sil_tuneproc(ide_drive_t *drive, u8 pio)
 {
-	u8 channel_timings	= siimage_taskfile_timing(HWIF(drive));
-	u8 speed = 0, set_pio	= ide_get_best_pio_mode(drive, 4, 5, NULL);
-
-	/* WARNING PIO timing mess is going to happen b/w devices, argh */
-	if ((channel_timings != set_pio) && (set_pio > channel_timings))
-		set_pio = channel_timings;
-
-	siimage_tuneproc(drive, set_pio);
-	speed = XFER_PIO_0 + set_pio;
-	if (set_speed)
-		(void) ide_config_drive_speed(drive, speed);
+	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);
+	sil_tune_pio(drive, pio);
+	(void)ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
 
 /**
@@ -335,7 +278,7 @@ static int siimage_tune_chipset (ide_dri
 		case XFER_PIO_2:
 		case XFER_PIO_1:
 		case XFER_PIO_0:
-			siimage_tuneproc(drive, (speed - XFER_PIO_0));
+			sil_tune_pio(drive, speed - XFER_PIO_0);
 			mode |= ((unit) ? 0x10 : 0x01);
 			break;
 		case XFER_MW_DMA_2:
@@ -343,7 +286,6 @@ static int siimage_tune_chipset (ide_dri
 		case XFER_MW_DMA_0:
 			multi = dma[speed - XFER_MW_DMA_0];
 			mode |= ((unit) ? 0x20 : 0x02);
-			config_siimage_chipset_for_pio(drive, 0);
 			break;
 		case XFER_UDMA_6:
 		case XFER_UDMA_5:
@@ -356,7 +298,6 @@ static int siimage_tune_chipset (ide_dri
 			ultra |= ((scsc) ? (ultra6[speed - XFER_UDMA_0]) :
 					   (ultra5[speed - XFER_UDMA_0]));
 			mode |= ((unit) ? 0x30 : 0x03);
-			config_siimage_chipset_for_pio(drive, 0);
 			break;
 		default:
 			return 1;
@@ -390,7 +331,7 @@ static int siimage_config_drive_for_dma 
 		return 0;
 
 	if (ide_use_fast_pio(drive))
-		config_siimage_chipset_for_pio(drive, 1);
+		sil_tuneproc(drive, 255);
 
 	return -1;
 }
@@ -961,7 +902,7 @@ static void __devinit init_hwif_siimage(
 	
 	hwif->resetproc = &siimage_reset;
 	hwif->speedproc = &siimage_tune_chipset;
-	hwif->tuneproc	= &siimage_tuneproc;
+	hwif->tuneproc	= &sil_tuneproc;
 	hwif->reset_poll = &siimage_reset_poll;
 	hwif->pre_reset = &siimage_pre_reset;
 	hwif->udma_filter = &sil_udma_filter;
@@ -976,11 +917,11 @@ static void __devinit init_hwif_siimage(
 			first = 0;
 		}
 	}
-	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 == 0)
 		return;
-	}
 
 	hwif->ultra_mask = 0x7f;
 	hwif->mwdma_mask = 0x07;

Re: [PATCH 3/6] siimage: PIO mode setup fixes

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>    A lot to argue about here...

>>>* Add sil_tuneproc() wrapper for siimage_tuneproc() which also sets
>>>  PIO mode on the device.

>>    Planning on the global prefix change? :-)

> Yep.

    Well, it didn't work out with 'ata_'... ;-)

>>>* Add code limiting maximum PIO mode according to the pair device capabilities
>>>  to sil_tuneproc().

>>    Ugh... that part is terrible. :-/
>>    Actually, we only need to limit the taskfile, not the data transfers --
>>unlike it was done before.

> Fixed.

    Let's see... :-)

>>    Note that PIO setup keeps being somewhat borken IODRY-wise even with this
>>patch as sil_tune_pio() only controls taskfile IORDY sampling -- the Right
>>Thing could only be done via speedproc() method...

> After rehashing the datasheet I see the source of the issue:

> IORDY is controlled in two registers and moreover it is always enabled
> if MDMA or UDMA transfer modes are selected.

    Yeah. I drafted some patch for pata_sil.c but Alan fixed it first and in a 
more simple way -- libata calls PIO/DMA setting methods in the strict 
sequence, and that allowed to bypass some checks...

>>>Index: b/drivers/ide/pci/siimage.c
>>>===================================================================
>>>--- a/drivers/ide/pci/siimage.c
>>>+++ b/drivers/ide/pci/siimage.c
>>[...]
>>>+	}
>>>+
>>> 	/* cheat for now and use the docs */
>>>-	switch (mode_wanted) {
>>>+	switch (pio) {
>>> 	case 4:
>>> 		speedp = 0x10c1;
>>> 		speedt = 0x10c1;

>>    What I envisioned was putting speedt into drive->drive_data, calculating 
>>the maximum value for 2 drives and using it to actually program the taskfile 
>>timing. Either that or put PIO mode there, and add the second switch to 
>>calculate the taskfile timings after getting the minimum PIO mode for 2 drives 
>>(but that's not as neat).

> I did something similar to the second solution (should be sufficient for now)
> but improvenments are welcomed.

    Thanks, looks good.

>>> 			hwif->OUTW(hwif->INW(tfaddr-2)|0x200, tfaddr-2);

>>    Erm, the same comments about taskfile IORDY as before: it should be 
>>selected if the drive supports it. In fact, if either of 2 drives do.

>>> 		else
>>> 			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);

>>    This is wrong logic: thus we may turn off IORDY although the 2nd drive may 
>>support it.

> Indeed, but since I don't want to be selfish and keep all bugfixes to myself
> I'm giving other people opportunity to fix it. :-)

> ditto for ->speedproc vs IORDY problems

    Wow, drivers/ide/ is a land of opportunity. B-)

>>[...]

>>>@@ -335,7 +299,7 @@ static int siimage_tune_chipset (ide_dri
>>> 		case XFER_PIO_2:
>>> 		case XFER_PIO_1:
>>> 		case XFER_PIO_0:
>>>-			siimage_tuneproc(drive, (speed - XFER_PIO_0));
>>>+			sil_tune_pio(drive, speed - XFER_PIO_0);
>>> 			mode |= ((unit) ? 0x10 : 0x01);

>>    The last line enables IORDY sampling for data transfers.

>>> 			break;
>>> 		case XFER_MW_DMA_2:
>>>@@ -343,7 +307,7 @@ static int siimage_tune_chipset (ide_dri
>>> 		case XFER_MW_DMA_0:
>>> 			multi = dma[speed - XFER_MW_DMA_0];
>>> 			mode |= ((unit) ? 0x20 : 0x02);

>>    ... and this line also enables IORDY. And the one in UltraDMA case group too.

>>>-			config_siimage_chipset_for_pio(drive, 0);
>>>+			sil_tune_pio(drive, 4); /* FIXME */
>>
>>    Why we still need this nonsense here...

> I was _really_ hoping that /* FIXME */ would make this clear. ;-)

    You were under/over-etimatating me. ;-)

>>>@@ -356,7 +320,7 @@ static int siimage_tune_chipset (ide_dri
>>> 			ultra |= ((scsc) ? (ultra6[speed - XFER_UDMA_0]) :
>>> 					   (ultra5[speed - XFER_UDMA_0]));
>>> 			mode |= ((unit) ? 0x30 : 0x03);
>>>-			config_siimage_chipset_for_pio(drive, 0);
>>>+			sil_tune_pio(drive, 4); /* FIXME */

>>    ... and here? If we so desperately want to setup PIO data/taskfile
>>timings, it's better to do via setting the 'autotune' field unconditionally.

> I had a follow-up patch doing exactly that (done later than this patch).
> I integrated it into current patch since there was a need for respin...

    Thanks, looks better now. :-)

> take 2 follows:

> [PATCH] siimage: PIO mode setup fixes (take 2)

> * Add sil_tuneproc() wrapper for siimage_tuneproc() which also sets
>   PIO mode on the device.

> * Add missing ide_get_best_pio_mode() call to sil_tuneproc() so
>   "pio" == 255 (autotune) is handled correctly (previously PIO0 was used)
>   and "pio" values > 4 && < 255 are filtered to PIO4 (instead of PIO0).

> * Add code limiting maximum PIO mode according to the pair device capabilities
>   to sil_tuneproc().

> * Convert users of config_siimage_chipset_for_pio() to use sil_tune_pio() and
>   sil_tuneproc().  This fixes PIO fallback in siimage_config_drive_for_dma() to
>   use max PIO mode available instead of PIO4 (config_siimage_chipset_for_pio()
>   used wrong arguments for ide_get_best_pio_mode() and as a results always
>   tried to set PIO4).

> * Remove no longer needed siimage_taskfile_timing()
>   and config_siimage_chipset_for_pio().

> * Enable ->autotune unconditionally and remove PIO tuning for UDMA/MDMA modes
>   from siimage_speedproc()

> * Bump driver version.

> v2:
> * Fix issues noticed by Sergei:
>   - correct pair device check
>   - trim only taskfile PIO to the slowest of the master/slave
>   - enable ->autotune unconditionally and remove PIO tuning for UDMA/MDMA modes
>     from siimage_speedproc()
>   - add TODO item for IORDY bugs
>   - minor cleanups

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

> Index: b/drivers/ide/pci/siimage.c
> ===================================================================
> --- a/drivers/ide/pci/siimage.c
> +++ b/drivers/ide/pci/siimage.c
[...]
> @@ -31,6 +32,10 @@
>   *  unplugging/replugging the virtual CD interface when the DRAC is reset.
>   *  This often causes drivers/ide/siimage to panic but is ok with the rather
>   *  smarter code in libata.
> + *
> + * TODO:
> + * - IORDY fixes
> + * - VDMA support
>   */

    Not sure if VDMA support would be worth it...

> -/**
> - *	simmage_tuneproc	-	tune a drive
> + *	sil_tune_pio	-	tune a drive
>   *	@drive: drive to tune
> - *	@mode_wanted: the target operating mode
> + *	@pio: the desired PIO mode
>   *
>   *	Load the timing settings for this device mode into the
>   *	controller. If we are in PIO mode 3 or 4 turn on IORDY
>   *	monitoring (bit 9). The TF timing is bits 31:16
>   */
> - 
> -static void siimage_tuneproc (ide_drive_t *drive, byte mode_wanted)
> +
> +static void sil_tune_pio(ide_drive_t *drive, u8 pio)
>  {
> +	const u16 tf_speed[]	= { 0x328a, 0x2283, 0x1281, 0x10c3, 0x10c1 };
> +	const u16 data_speed[]	= { 0x328a, 0x2283, 0x1104, 0x10c3, 0x10c1 };

    Yeah, that's better than switch! :-)

> +
>  	ide_hwif_t *hwif	= HWIF(drive);
> +	ide_drive_t *pair	= &hwif->drives[drive->dn ^ 1];
>  	u32 speedt		= 0;
>  	u16 speedp		= 0;
>  	unsigned long addr	= siimage_seldev(drive, 0x04);
>  	unsigned long tfaddr	= siimage_selreg(hwif, 0x02);
> -	
> -	/* cheat for now and use the docs */
> -	switch (mode_wanted) {
> -	case 4:
> -		speedp = 0x10c1;
> -		speedt = 0x10c1;
> -		break;
> -	case 3:
> -		speedp = 0x10c3;
> -		speedt = 0x10c3;
> -		break;
> -	case 2:
> -		speedp = 0x1104;
> -		speedt = 0x1281;
> -		break;
> -	case 1:
> -		speedp = 0x2283;
> -		speedt = 0x2283;
> -		break;
> -	case 0:
> -	default:
> -		speedp = 0x328a;
> -		speedt = 0x328a;
> -		break;
> +	u8 tf_pio		= pio;
> +
> +	/* trim *taskfile* PIO to the slowest of the master/slave */
> +	if (pair->present) {
> +		u8 pair_pio = ide_get_best_pio_mode(pair, 255, 4, NULL);
> +
> +		if (pair_pio < tf_pio)
> +			tf_pio = pair_pio;
>  	}
>  
> +	/* cheat for now and use the docs */
> +	speedp = data_speed[pio];
> +	speedt = tf_speed[tf_pio];
> +
>  	if (hwif->mmio) {
>  		hwif->OUTW(speedp, addr);
>  		hwif->OUTW(speedt, tfaddr);
>  		/* Now set up IORDY */
> -		if(mode_wanted == 3 || mode_wanted == 4)
> +		if (pio > 2)

    Not tf_pio? This IORDY bit is for taskfile accesses...

>  			hwif->OUTW(hwif->INW(tfaddr-2)|0x200, tfaddr-2);
>  		else
>  			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);
> @@ -245,42 +213,17 @@ static void siimage_tuneproc (ide_drive_
>  		pci_read_config_word(hwif->pci_dev, tfaddr-2, &speedp);
>  		speedp &= ~0x200;
>  		/* Set IORDY for mode 3 or 4 */
> -		if(mode_wanted == 3 || mode_wanted == 4)
> +		if (pio > 2)

    Same question here.
    However... this logic should rely on both drives' PIO modes, so would be 
broken either way until this is fixed...

>  			speedp |= 0x200;
>  		pci_write_config_word(hwif->pci_dev, tfaddr-2, speedp);
>  	}
>  }

MBR, Sergei

Re: [PATCH 3/6] siimage: PIO mode setup fixes

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>>>* Add sil_tuneproc() wrapper for siimage_tuneproc() which also sets
>>>>> PIO mode on the device.

>>>>   Planning on the global prefix change? :-)

>>>Yep.

>>    Well, it didn't work out with 'ata_'... ;-)

> Because of bad libata taking over our precioussssss namespace... ;-)

    Thievessss! 8-)

> Fortunately pata_sil680 driver uses "sil680_" prefix.

    We'll make Coxsss pay. 8-)

>>>>   Note that PIO setup keeps being somewhat borken IODRY-wise even with this
>>>>patch as sil_tune_pio() only controls taskfile IORDY sampling -- the Right
>>>>Thing could only be done via speedproc() method...

>>>After rehashing the datasheet I see the source of the issue:

>>>IORDY is controlled in two registers and moreover it is always enabled
>>>if MDMA or UDMA transfer modes are selected.

>>    Yeah. I drafted some patch for pata_sil.c but Alan fixed it first and in a 
>>more simple way -- libata calls PIO/DMA setting methods in the strict 
>>sequence, and that allowed to bypass some checks...

> I fail to see how this helps in this case, care to explain?

    The whole reason for that resetproc() ceases to exist, no? At least that's 
what I was able to grasp from looing at the older versions... And it's mean to 
reset both channels just to be able to downgrade from UDMA to MWDMA (not a 
thing routinely done anyway), so this needs to go.
    Well, I'm seeing that it's also called from the ide_dma_timeout() and even 
ide_dma_lostirq() methods; not sure how much necessary it is -- at least for 
the latter case this seems just wrong...

>>>+	/* cheat for now and use the docs */
>>>+	speedp = data_speed[pio];
>>>+	speedt = tf_speed[tf_pio];
>>>+
>>> 	if (hwif->mmio) {
>>> 		hwif->OUTW(speedp, addr);
>>> 		hwif->OUTW(speedt, tfaddr);
>>> 		/* Now set up IORDY */
>>>-		if(mode_wanted == 3 || mode_wanted == 4)
>>>+		if (pio > 2)

>>    Not tf_pio? This IORDY bit is for taskfile accesses...

> Would this be really OK (tf_pio takes minimum PIO mode)?

    Well, probably not -- a slow mate drive could force IORDY disabled this 
way... So, just 'pio' seems more correct.

> Thanks,
> Bart

MBR, Sergei

Re: [PATCH 3/6] siimage: PIO mode setup fixes

[Bartlomiej Zolnierkiewicz]

On Wednesday 04 July 2007, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>    A lot to argue about here...
> 
> >>>* Add sil_tuneproc() wrapper for siimage_tuneproc() which also sets
> >>>  PIO mode on the device.
> 
> >>    Planning on the global prefix change? :-)
> 
> > Yep.
> 
>     Well, it didn't work out with 'ata_'... ;-)

Because of bad libata taking over our precioussssss namespace... ;-)

Fortunately pata_sil680 driver uses "sil680_" prefix.

> >>>* Add code limiting maximum PIO mode according to the pair device capabilities
> >>>  to sil_tuneproc().
> 
> >>    Ugh... that part is terrible. :-/
> >>    Actually, we only need to limit the taskfile, not the data transfers --
> >>unlike it was done before.
> 
> > Fixed.
> 
>     Let's see... :-)
> 
> >>    Note that PIO setup keeps being somewhat borken IODRY-wise even with this
> >>patch as sil_tune_pio() only controls taskfile IORDY sampling -- the Right
> >>Thing could only be done via speedproc() method...
> 
> > After rehashing the datasheet I see the source of the issue:
> 
> > IORDY is controlled in two registers and moreover it is always enabled
> > if MDMA or UDMA transfer modes are selected.
> 
>     Yeah. I drafted some patch for pata_sil.c but Alan fixed it first and in a 
> more simple way -- libata calls PIO/DMA setting methods in the strict 
> sequence, and that allowed to bypass some checks...

I fail to see how this helps in this case, care to explain?

> > +	/* cheat for now and use the docs */
> > +	speedp = data_speed[pio];
> > +	speedt = tf_speed[tf_pio];
> > +
> >  	if (hwif->mmio) {
> >  		hwif->OUTW(speedp, addr);
> >  		hwif->OUTW(speedt, tfaddr);
> >  		/* Now set up IORDY */
> > -		if(mode_wanted == 3 || mode_wanted == 4)
> > +		if (pio > 2)
> 
>     Not tf_pio? This IORDY bit is for taskfile accesses...

Would this be really OK (tf_pio takes minimum PIO mode)?

> >  			hwif->OUTW(hwif->INW(tfaddr-2)|0x200, tfaddr-2);
> >  		else
> >  			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);
> > @@ -245,42 +213,17 @@ static void siimage_tuneproc (ide_drive_
> >  		pci_read_config_word(hwif->pci_dev, tfaddr-2, &speedp);
> >  		speedp &= ~0x200;
> >  		/* Set IORDY for mode 3 or 4 */
> > -		if(mode_wanted == 3 || mode_wanted == 4)
> > +		if (pio > 2)
> 
>     Same question here.
>     However... this logic should rely on both drives' PIO modes, so would be 
> broken either way until this is fixed...

Yep.

Thanks,
Bart

Re: [PATCH 3/6] siimage: PIO mode setup fixes

[Jeff Garzik]

Sergei Shtylyov wrote:
>    Not sure if VDMA support would be worth it...


IMO, it is useless.  It requires per-IRQ handling of each DRQ block, 
which just adds complexity to existing "PIO-ish" data paths.  You have 
to set up and tear down DMA engine for each DRQ block.

VDMA would be far more useful if it handled the entire transfer in one 
go, like a normal BMDMA transaction.

	Jeff