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@xxxxxxxxx>
>
> 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@xxxxxxxxx>
Reviewed-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxx>
* Copyright (C) 2003 Red Hat <alan@xxxxxxxxxx>
* 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;
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
