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

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

[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 <bzolnier@gmail.com>
---
 drivers/ide/pci/siimage.c |   20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Index: b/drivers/ide/pci/siimage.c
===================================================================
--- a/drivers/ide/pci/siimage.c
+++ 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 <andre@linux-ide.org>
  * Copyright (C) 2003		Red Hat <alan@redhat.com>
@@ -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:

Re: [PATCH] siimage: fix ->set_pio_mode method to select PIO data transfer

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

> * 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 <bzolnier@gmail.com>

Acked-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
> @@ -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 <andre@linux-ide.org>
>   * Copyright (C) 2003		Red Hat <alan@redhat.com>
> @@ -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);
>  	}
>  }

    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

Re: [PATCH] siimage: fix ->set_pio_mode method to select PIO data transfer

[Bartlomiej Zolnierkiewicz]

On Saturday 14 July 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
> > * 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 <bzolnier@gmail.com>
> 
> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

added

> > Index: b/drivers/ide/pci/siimage.c
> > ===================================================================
> > --- a/drivers/ide/pci/siimage.c
> > +++ 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 <andre@linux-ide.org>
> >   * Copyright (C) 2003		Red Hat <alan@redhat.com>
> > @@ -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);
> >  	}
> >  }
> 
>     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?

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

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

>     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