siimage: PIO mode setup fixes

PATCH In review Bartlomiej Zolnierkiewicz 3 participants 7 messages Jun 23, 2007 – Jul 6, 2007 libata

lore

Share & tools

lore

Bartlomiej Zolnierkiewicz

author

2007-06-23 17:52 UTC #0 lore raw

PATCH 3/6 siimage: PIO mode setup fixes

* 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 <[email protected]>

---
 drivers/ide/pci/siimage.c |  108 +++++++++++++++-------------------------------
 1 file changed, 36 insertions(+), 72 deletions(-)

drivers/ide/pci/siimage.c +36 ~~-72~~

Index: 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 <[email protected]>
  * Copyright (C) 2003		Red Hat <[email protected]>
  * 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;

Sergei Shtylyov

2007-06-26 19:50 UTC #1 lore raw

Bartlomiej Zolnierkiewicz wrote:

A lot to argue about here...

Quoted 2 lines

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

Planning on the global prefix change? :-)

Quoted 2 lines

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

Quoted 2 lines

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

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

Quoted 1 line

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

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

Quoted 4 lines

Index: b/drivers/ide/pci/siimage.c
===================================================================
drivers/ide/pci/siimage.c +0 -0

[...]

Quoted 8 lines

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

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

drivers/ide/pci/siimage.c out: · line 179

Quoted 10 lines

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

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

drivers/ide/pci/siimage.c out: · line 190

Quoted 12 lines

drivers/ide/pci/siimage.c +7 -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.

drivers/ide/pci/siimage.c out: · line 191

Quoted 1 line

drivers/ide/pci/siimage.c +1 -0

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

drivers/ide/pci/siimage.c out: · line 195

Quoted 4 lines

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

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

No need to trim the *data* PIO mode.

drivers/ide/pci/siimage.c out: · line 202

Quoted 8 lines

drivers/ide/pci/siimage.c +3 -1

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

drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 227

Quoted 6 lines

drivers/ide/pci/siimage.c +1 -1

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

drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 228

Quoted 1 line

drivers/ide/pci/siimage.c +0 -0
 			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.

drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 230

Quoted 2 lines

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

 		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.

drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 241

Quoted 10 lines

drivers/ide/pci/siimage.c +1 -1

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

[...]

drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 303

Quoted 7 lines

drivers/ide/pci/siimage.c +1 -1

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

drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 309

Quoted 6 lines

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

 			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.

drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 310

Quoted 2 lines

drivers/ide/pci/siimage.c +1 -1

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

Why we still need this nonsense here...

drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 323

Quoted 9 lines

drivers/ide/pci/siimage.c +1 -1

 			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.

drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 326

Quoted 3 lines

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

 			break;
 		default:
 			return 1;

MBR, Sergei

Bartlomiej Zolnierkiewicz

author Reviewed-by Sergei Shtylyov

2007-06-29 22:01 UTC #2 lore raw

Hi,

Quoted 8 lines · Sergei Shtylyov

Bartlomiej Zolnierkiewicz wrote:
A lot to argue about here...
Quoted 2 lines
* Add sil_tuneproc() wrapper for siimage_tuneproc() which also sets PIO mode on the device.
Planning on the global prefix change? :-)

Yep.

Quoted 6 lines

Quoted 2 lines
* 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.

Quoted 10 lines

Quoted 2 lines
* Remove no longer needed siimage_taskfile_timing() and config_siimage_chipset_for_pio().
Yeah, time to get rid of that garbage. :-)
Quoted 1 line
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
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.

Quoted 17 lines

Quoted 4 lines
Index: b/drivers/ide/pci/siimage.c
===================================================================
drivers/ide/pci/siimage.c +0 -0
[...]
Quoted 12 lines
-/**
- * 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
*/
-

Quoted 8 lines

drivers/ide/pci/siimage.c

Quoted 6 lines

drivers/ide/pci/siimage.c +3 -1

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

Quoted 6 lines

drivers/ide/pci/siimage.c +1 -1

 	u32 speedt		= 0;
 	u16 speedp		= 0;
 	unsigned long addr	= siimage_seldev(drive, 0x04);
 	unsigned long tfaddr	= siimage_selreg(hwif, 0x02);
-	
+

Quoted 8 lines

drivers/ide/pci/siimage.c
Quoted 6 lines
drivers/ide/pci/siimage.c +6 -0
+	/*
+	 * 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!

Quoted 6 lines

drivers/ide/pci/siimage.c
Quoted 1 line
drivers/ide/pci/siimage.c +1 -0
+		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.

Quoted 6 lines

drivers/ide/pci/siimage.c
Quoted 4 lines
drivers/ide/pci/siimage.c +4 -0
+
+		/* 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.

Quoted 10 lines

drivers/ide/pci/siimage.c

Quoted 8 lines

drivers/ide/pci/siimage.c +3 -1

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

Quoted 4 lines

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.

Quoted 8 lines

drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 227
Quoted 6 lines
drivers/ide/pci/siimage.c +1 -1
@@ -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.

Quoted 10 lines

drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 228
Quoted 1 line
drivers/ide/pci/siimage.c +0 -0
 			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.
drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 230
Quoted 2 lines
drivers/ide/pci/siimage.c +0 -0
 		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

Quoted 4 lines

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

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

Quoted 8 lines

drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 241
Quoted 6 lines
drivers/ide/pci/siimage.c +1 -1
-		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.

Quoted 20 lines

[...]
drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 303
Quoted 7 lines
drivers/ide/pci/siimage.c +1 -1
@@ -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.
drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 309
Quoted 6 lines
drivers/ide/pci/siimage.c +0 -0
 			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.

Quoted 4 lines

drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 310
Quoted 2 lines
drivers/ide/pci/siimage.c +1 -1
-			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. ;-)

Quoted 4 lines

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

 			break;
 		case XFER_UDMA_6:
 		case XFER_UDMA_5:

@@ -356,7 +320,7 @@ static int siimage_tune_chipset (ide_dri

Quoted 8 lines

drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 323
Quoted 5 lines
drivers/ide/pci/siimage.c +1 -1
 			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 code limiting maximum PIO mode according to the pair device capabilities to sil_tuneproc().

* 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 <[email protected]>

Reviewed-by Sergei Shtylyov <[email protected]>

---
 drivers/ide/pci/siimage.c |  137 +++++++++++++---------------------------------
 1 file changed, 39 insertions(+), 98 deletions(-)

drivers/ide/pci/siimage.c +39 ~~-98~~

Index: 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 <[email protected]>
  * Copyright (C) 2003		Red Hat <[email protected]>
  * 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;

Sergei Shtylyov

2007-07-04 16:14 UTC #3 lore raw

Hello.

Quoted 8 lines · Bartlomiej Zolnierkiewicz

Quoted 6 lines
A lot to argue about here...
Quoted 2 lines
* 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_'... ;-)

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
* 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... :-)

Quoted 8 lines

Quoted 3 lines
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...

Quoted 17 lines

Quoted 5 lines
Quoted 4 lines
Index: b/drivers/ide/pci/siimage.c
===================================================================
drivers/ide/pci/siimage.c +0 -0
[...]
Quoted 11 lines
Quoted 8 lines
drivers/ide/pci/siimage.c +2 -0
+	}
+
	/* cheat for now and use the docs */
drivers/ide/pci/siimage.c +1 -1
-	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).

Quoted 2 lines

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

Thanks, looks good.

Quoted 8 lines

Quoted 7 lines
Quoted 1 line
			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.
Quoted 2 lines
		else
			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);

Quoted 7 lines

Quoted 2 lines
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-)

Quoted 21 lines

Quoted 20 lines
[...]
drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 303
Quoted 7 lines
drivers/ide/pci/siimage.c +1 -1
@@ -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.
drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 309
Quoted 6 lines
drivers/ide/pci/siimage.c +0 -0
 			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.

Quoted 6 lines

Quoted 4 lines
drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 310
Quoted 2 lines
drivers/ide/pci/siimage.c +1 -1
-			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. ;-)

Quoted 10 lines

Quoted 9 lines
drivers/ide/pci/siimage.c static int siimage_tune_chipset (ide_dri · line 323
Quoted 6 lines
drivers/ide/pci/siimage.c +1 -1
@@ -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.

Quoted 2 lines

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

Quoted 44 lines

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 <[email protected]>
Reviewed-by Sergei Shtylyov <[email protected]>
Index: b/drivers/ide/pci/siimage.c
===================================================================
drivers/ide/pci/siimage.c +0 -0

[...]

drivers/ide/pci/siimage.c line 39

Quoted 9 lines

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

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

Quoted 8 lines

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

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

drivers/ide/pci/siimage.c out: · line 180

Quoted 10 lines

drivers/ide/pci/siimage.c +4 -2

  *	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! :-)

Quoted 41 lines

drivers/ide/pci/siimage.c +10 ~~-24~~

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

drivers/ide/pci/siimage.c out: · line 206

Quoted 10 lines

drivers/ide/pci/siimage.c +5 -1

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

drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 216

Quoted 9 lines

drivers/ide/pci/siimage.c +1 -1

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

drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 220

Quoted 4 lines

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

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

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2007-07-04 18:46 UTC #4 lore raw

Quoted 6 lines · Sergei Shtylyov

Hello.
Quoted 1 line · Bartlomiej Zolnierkiewicz
Quoted 1 line
A lot to argue about here...

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
* 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.

Quoted 15 lines

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
* 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... :-)
Quoted 3 lines
Quoted 3 lines
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...

Quoted 8 lines

Quoted 4 lines
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?

Quoted 4 lines

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

+	/* cheat for now and use the docs */
+	speedp = data_speed[pio];
+	speedt = tf_speed[tf_pio];
+

Quoted 8 lines

drivers/ide/pci/siimage.c out: · line 206
Quoted 6 lines
drivers/ide/pci/siimage.c +1 -1
 	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)?

Quoted 5 lines

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

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

Quoted 8 lines

drivers/ide/pci/siimage.c static void siimage_tuneproc (ide_drive_ · line 216
Quoted 4 lines
drivers/ide/pci/siimage.c +1 -1
 		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

Sergei Shtylyov

2007-07-04 18:57 UTC #5 lore raw

Quoted 10 lines · Bartlomiej Zolnierkiewicz

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
* 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-)

Quoted 1 line

Fortunately pata_sil680 driver uses "sil680_" prefix.

We'll make Coxsss pay. 8-)

Quoted 6 lines

Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
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:

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
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...

Quoted 13 lines

Quoted 4 lines
Quoted 4 lines
drivers/ide/pci/siimage.c +4 -0
+	/* cheat for now and use the docs */
+	speedp = data_speed[pio];
+	speedt = tf_speed[tf_pio];
+
Quoted 8 lines
Review
Quoted 6 lines
	if (hwif->mmio) {
		hwif->OUTW(speedp, addr);
		hwif->OUTW(speedt, tfaddr);
		/* Now set up IORDY */
(quoted diff) +1 -1
-		if(mode_wanted == 3 || mode_wanted == 4)
+		if (pio > 2)
Not tf_pio? This IORDY bit is for taskfile accesses...

Quoted 1 line

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.

Quoted 2 lines

Thanks,
Bart

MBR, Sergei

Jeff Garzik

2007-07-06 00:31 UTC #6 lore raw

In reply to Sergei Shtylyov

Quoted 1 line · Sergei Shtylyov

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