siimage: fix ->set_pio_mode method to select PIO data transfer

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 3 messages Jul 13, 2007 – Jul 18, 2007 libata

Share & tools

Bartlomiej Zolnierkiewicz

author

2007-07-13 21:31 UTC #0 lore raw

PATCH 1/1 siimage: fix ->set_pio_mode method to select PIO data transfer

* Remember to select PIO data transfer (with IORDY monitored) in sil_tune_pio() (->set_pio_mode method) so the controller is always programmed correctly for PIO transfers (this is important if DMA is not going to be used).

* Don't set DMA/UDMA timings for PIO modes in siimage_tune_chipset().

* Bump driver version.

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

---
 drivers/ide/pci/siimage.c |   20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

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

Index: b/drivers/ide/pci/siimage.c
===================================================================
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/siimage.c		Version 1.15	Jun 29 2007
+ * linux/drivers/ide/pci/siimage.c		Version 1.16	Jul 13 2007
  *
  * Copyright (C) 2001-2002	Andre Hedrick <[email protected]>
  * Copyright (C) 2003		Red Hat <[email protected]>

@@ -185,7 +185,12 @@ static void sil_tune_pio(ide_drive_t *dr
 	u16 speedp		= 0;
 	unsigned long addr	= siimage_seldev(drive, 0x04);
 	unsigned long tfaddr	= siimage_selreg(hwif, 0x02);
+	unsigned long base	= (unsigned long)hwif->hwif_data;
 	u8 tf_pio		= pio;
+	u8 addr_mask		= hwif->channel ? (hwif->mmio ? 0xF4 : 0x84)
+						: (hwif->mmio ? 0xB4 : 0x80);
+	u8 mode			= 0;
+	u8 unit			= drive->select.b.unit;
 
 	/* trim *taskfile* PIO to the slowest of the master/slave */
 	if (pair->present) {

@@ -207,6 +212,11 @@ static void sil_tune_pio(ide_drive_t *dr
 			hwif->OUTW(hwif->INW(tfaddr-2)|0x200, tfaddr-2);
 		else
 			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);
+
+		mode = hwif->INB(base + addr_mask);
+		mode &= ~(unit ? 0x30 : 0x03);
+		mode |= (unit ? 0x10 : 0x01);
+		hwif->OUTB(mode, base + addr_mask);
 	} else {
 		pci_write_config_word(hwif->pci_dev, addr, speedp);
 		pci_write_config_word(hwif->pci_dev, tfaddr, speedt);

@@ -216,6 +226,11 @@ static void sil_tune_pio(ide_drive_t *dr
 		if (pio > 2)
 			speedp |= 0x200;
 		pci_write_config_word(hwif->pci_dev, tfaddr-2, speedp);
+
+		pci_read_config_byte(hwif->pci_dev, addr_mask, &mode);
+		mode &= ~(unit ? 0x30 : 0x03);
+		mode |= (unit ? 0x10 : 0x01);
+		pci_write_config_byte(hwif->pci_dev, addr_mask, mode);
 	}
 }

@@ -275,8 +290,7 @@ static int siimage_tune_chipset(ide_driv
 		case XFER_PIO_1:
 		case XFER_PIO_0:
 			sil_tune_pio(drive, speed - XFER_PIO_0);
-			mode |= ((unit) ? 0x10 : 0x01);
-			break;
+			return ide_config_drive_speed(drive, speed);
 		case XFER_MW_DMA_2:
 		case XFER_MW_DMA_1:
 		case XFER_MW_DMA_0:

Sergei Shtylyov

Acked-by on v1 1/1

2007-07-14 17:33 UTC #1 lore raw

Quoted 9 lines · Bartlomiej Zolnierkiewicz

* Remember to select PIO data transfer (with IORDY monitored) in sil_tune_pio() (->set_pio_mode method) so the controller is always programmed correctly for PIO transfers (this is important if DMA is not going to be used).
* Don't set DMA/UDMA timings for PIO modes in siimage_tune_chipset().
* Bump driver version.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Acked-by Sergei Shtylyov <[email protected]>

Quoted 37 lines

Index: b/drivers/ide/pci/siimage.c
===================================================================

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

@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/siimage.c		Version 1.15	Jun 29 2007
+ * linux/drivers/ide/pci/siimage.c		Version 1.16	Jul 13 2007
  *
  * Copyright (C) 2001-2002	Andre Hedrick <[email protected]>
  * Copyright (C) 2003		Red Hat <[email protected]>

@@ -185,7 +185,12 @@ static void sil_tune_pio(ide_drive_t *dr
 	u16 speedp		= 0;
 	unsigned long addr	= siimage_seldev(drive, 0x04);
 	unsigned long tfaddr	= siimage_selreg(hwif, 0x02);
+	unsigned long base	= (unsigned long)hwif->hwif_data;
 	u8 tf_pio		= pio;
+	u8 addr_mask		= hwif->channel ? (hwif->mmio ? 0xF4 : 0x84)
+						: (hwif->mmio ? 0xB4 : 0x80);
+	u8 mode			= 0;
+	u8 unit			= drive->select.b.unit;
 
 	/* trim *taskfile* PIO to the slowest of the master/slave */
 	if (pair->present) {

@@ -207,6 +212,11 @@ static void sil_tune_pio(ide_drive_t *dr
 			hwif->OUTW(hwif->INW(tfaddr-2)|0x200, tfaddr-2);
 		else
 			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);
+
+		mode = hwif->INB(base + addr_mask);
+		mode &= ~(unit ? 0x30 : 0x03);
+		mode |= (unit ? 0x10 : 0x01);
+		hwif->OUTB(mode, base + addr_mask);
 	} else {
 		pci_write_config_word(hwif->pci_dev, addr, speedp);
 		pci_write_config_word(hwif->pci_dev, tfaddr, speedt);

drivers/ide/pci/siimage.c static void sil_tune_pio(ide_drive_t *dr · line 235

Quoted 10 lines

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

@@ -216,6 +226,11 @@ static void sil_tune_pio(ide_drive_t *dr
 		if (pio > 2)
 			speedp |= 0x200;
 		pci_write_config_word(hwif->pci_dev, tfaddr-2, speedp);
+
+		pci_read_config_byte(hwif->pci_dev, addr_mask, &mode);
+		mode &= ~(unit ? 0x30 : 0x03);
+		mode |= (unit ? 0x10 : 0x01);
+		pci_write_config_byte(hwif->pci_dev, addr_mask, mode);
 	}
 }

First, there's a possibility of disabling DMA mode if an invalid PIO mode
is passed -- ide_config_drive_speed() won't call dma_off_quietly() in this
case. Well, due to the way IDE core works now, one has to re-tune DMA after setting any PIO mode anyway, so maybe this simple code would indeed be enough...
Second, although being correct (only for the drives supporting IORDY,
that is), this code is asymmetric to what's done for the taskfile IORDY.

A side note: pio/mmio code patchs certalinly could use some reorg here, to
extract the common code from them... at least that's what was on my mind when I intended to work on this driver. :-)

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2007-07-18 21:07 UTC #2 lore raw

Quoted 6 lines · Sergei Shtylyov

Quoted 3 lines · Bartlomiej Zolnierkiewicz
* Remember to select PIO data transfer (with IORDY monitored) in sil_tune_pio() (->set_pio_mode method) so the controller is always programmed correctly for PIO transfers (this is important if DMA is not going to be used).

Quoted 7 lines

Quoted 5 lines
* Don't set DMA/UDMA timings for PIO modes in siimage_tune_chipset().
* Bump driver version.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
Acked-by Sergei Shtylyov <[email protected]>

added

Quoted 42 lines

Index: b/drivers/ide/pci/siimage.c
===================================================================

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

@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/siimage.c		Version 1.15	Jun 29 2007
+ * linux/drivers/ide/pci/siimage.c		Version 1.16	Jul 13 2007
  *
  * Copyright (C) 2001-2002	Andre Hedrick <[email protected]>
  * Copyright (C) 2003		Red Hat <[email protected]>

@@ -185,7 +185,12 @@ static void sil_tune_pio(ide_drive_t *dr
 	u16 speedp		= 0;
 	unsigned long addr	= siimage_seldev(drive, 0x04);
 	unsigned long tfaddr	= siimage_selreg(hwif, 0x02);
+	unsigned long base	= (unsigned long)hwif->hwif_data;
 	u8 tf_pio		= pio;
+	u8 addr_mask		= hwif->channel ? (hwif->mmio ? 0xF4 : 0x84)
+						: (hwif->mmio ? 0xB4 : 0x80);
+	u8 mode			= 0;
+	u8 unit			= drive->select.b.unit;
 
 	/* trim *taskfile* PIO to the slowest of the master/slave */
 	if (pair->present) {

@@ -207,6 +212,11 @@ static void sil_tune_pio(ide_drive_t *dr
 			hwif->OUTW(hwif->INW(tfaddr-2)|0x200, tfaddr-2);
 		else
 			hwif->OUTW(hwif->INW(tfaddr-2)&~0x200, tfaddr-2);
+
+		mode = hwif->INB(base + addr_mask);
+		mode &= ~(unit ? 0x30 : 0x03);
+		mode |= (unit ? 0x10 : 0x01);
+		hwif->OUTB(mode, base + addr_mask);
 	} else {
 		pci_write_config_word(hwif->pci_dev, addr, speedp);
 		pci_write_config_word(hwif->pci_dev, tfaddr, speedt);

@@ -216,6 +226,11 @@ static void sil_tune_pio(ide_drive_t *dr
 		if (pio > 2)
 			speedp |= 0x200;
 		pci_write_config_word(hwif->pci_dev, tfaddr-2, speedp);
+
+		pci_read_config_byte(hwif->pci_dev, addr_mask, &mode);

Quoted 8 lines

drivers/ide/pci/siimage.c static void sil_tune_pio(ide_drive_t *dr · line 235
Quoted 5 lines
drivers/ide/pci/siimage.c +3 -0
+		mode &= ~(unit ? 0x30 : 0x03);
+		mode |= (unit ? 0x10 : 0x01);
+		pci_write_config_byte(hwif->pci_dev, addr_mask, mode);
 	}
 }
First, there's a possibility of disabling DMA mode if an invalid PIO mode
is passed -- ide_config_drive_speed() won't call dma_off_quietly() in this

#ifdef CONFIG_BLK_DEV_IDEDMA
        if (speed >= XFER_SW_DMA_0)
                hwif->dma_host_on(drive);
        else if (hwif->ide_dma_check)   /* check if host supports DMA */
                hwif->dma_off_quietly(drive);

#endif

It looks like it will?

Quoted 2 lines

case. Well, due to the way IDE core works now, one has to re-tune DMA after setting any PIO mode anyway, so maybe this simple code would indeed be enough...

Yep.

Quoted 2 lines

Second, although being correct (only for the drives supporting IORDY,
that is), this code is asymmetric to what's done for the taskfile IORDY.

Improvements are welcomed. :)

Quoted 3 lines

A side note: pio/mmio code patchs certalinly could use some reorg here, to
extract the common code from them... at least that's what was on my mind when I intended to work on this driver. :-)

Certainly, I would even say that it is worth adding sil_io{read,write}8/16/32 since none of the methods containing pci/mmio split are performance critical.

Thanks,
Bart