Hi, Sergei Shtylyov wrote: > > Hello. > > Bartlomiej Zolnierkiewicz wrote: > >>> [Finally forgot to stamp MV's copyright on the driver, so here's take #2...] > >>> The driver's tuneproc() method fails to set the drive's own speed -- fix >>> this >>> by renaming the function to cmd64x_tune_pio(), making it return the mode >>> set, >>> and "wrapping" the new tuneproc() method around it; while at it, also get >>> rid >>> of the non-working prefetch control code, remove redundant PIO5 mode >>> limitation, >>> and make cmdprintk() give more sensible mode info. Also, get rid of the >>> broken >>> config_chipset_for_pio() which always tried to set PIO4 instead auto-tuning >>> PIO. >>> Note that removing a call from config_chipset_for_dma() breaks "rudimentary" >>> MWDMA2 support which can only work because of the timing registers being >>> pre- >>> setup for PIO4 since the code in the speedproc() method which sets up the >>> chip >>> for SW/MW DMA is completely bogus (!) and I'm going to remove it for the >>> time >>> being in the next patch. >>> Oh, and add the missing PIO5 support to the speedproc() method while at it. >>> :-) > >> Generally looks fine, some comments below. > >>> Warning: compile tested only -- getting to the real hardware isn't > that easy... > >>> Signed-off-by: Sergei Shtylyov <[EMAIL PROTECTED]> > >>> drivers/ide/pci/cmd64x.c | 95 > ++++++++++++++++------------------------------- >>> 1 files changed, 34 insertions(+), 61 deletions(-) > >>> Index: linux-2.6/drivers/ide/pci/cmd64x.c >>> =================================================================== >>> --- linux-2.6.orig/drivers/ide/pci/cmd64x.c >>> +++ linux-2.6/drivers/ide/pci/cmd64x.c >>> @@ -1,6 +1,6 @@ >>> /* $Id: cmd64x.c,v 1.21 2000/01/30 23:23:16 >>> * >>> - * linux/drivers/ide/pci/cmd64x.c Version 1.30 Sept 10, 2002 >>> + * linux/drivers/ide/pci/cmd64x.c Version 1.41 Feb 3, 2007 >>> * >>> * cmd64x.c: Enable interrupts at initialization time on Ultra/PCI machines. >>> * Note, this driver is not used at all on other systems because >>> @@ -12,6 +12,7 @@ >>> * Copyright (C) 1998 David S. Miller (davem@redhat.com) >>> * >>> * Copyright (C) 1999-2002 Andre Hedrick <[EMAIL PROTECTED]> >>> + * Copyright (C) 2007 MontaVista Software, Inc. <[EMAIL >>> PROTECTED]> >>> */ >>> >>> #include <linux/module.h> >>> @@ -262,43 +263,25 @@ static void program_drive_counts (ide_dr >>> } >>> >>> /* >>> - * Attempts to set the interface PIO mode. >>> - * The preferred method of selecting PIO modes (e.g. mode 4) is >>> - * "echo 'piomode:4' > /proc/ide/hdx/settings". Special cases are >>> - * 8: prefetch off, 9: prefetch on, 255: auto-select best mode. >>> - * Called with 255 at boot time. >>> + * This routine selects drive's best PIO mode, calculates >>> setup/active/recovery >>> + * counts, and writes them into the chipset registers. >>> */ >>> - >>> -static void cmd64x_tuneproc (ide_drive_t *drive, u8 mode_wanted) >>> +static u8 cmd64x_tune_pio (ide_drive_t *drive, u8 mode_wanted) >>> { >>> int setup_time, active_time, recovery_time; >>> int clock_time, pio_mode, cycle_time; >>> u8 recovery_count2, cycle_count; >>> int setup_count, active_count, recovery_count; >>> int bus_speed = system_bus_clock(); >>> - /*byte b;*/ >>> ide_pio_data_t d; >>> >>> - switch (mode_wanted) { >>> - case 8: /* set prefetch off */ >>> - case 9: /* set prefetch on */ >>> - mode_wanted &= 1; >>> - /*set_prefetch_mode(index, mode_wanted);*/ >>> - cmdprintk("%s: %sabled cmd640 prefetch\n", >>> - drive->name, mode_wanted ? "en" : "dis"); >>> - return; >>> - } >>> - >>> - mode_wanted = ide_get_best_pio_mode (drive, mode_wanted, 5, &d); >>> - pio_mode = d.pio_mode; >>> + pio_mode = ide_get_best_pio_mode(drive, mode_wanted, 5, &d); >>> cycle_time = d.cycle_time; > >> After this change if an unaware user passes "8" or "9" to enable/disable >> prefetch (user doesn't know that it has never worked) it will result >> in PIO5 being programmed. > > Yes. :-) > >> I don't think that this is a real world issue but for paranoia reasons >> please add pio == 8/9 check to cmd64x_tune_drive(), something like: > >> /* >> * letf-over from prefetch setting (pio == 8/9), >> * needed to prevent PIO5 from being programmed >> */ >> if (pio == 8 || pio == 9) >> return; > > OK, I'll probably just leave the prefetch code where it was. > >> This will vanish when core code will do filtering of user space >> PIO mode change requests... > >> [ ... ] > >>> +/* >>> + * Attempts to set drive's PIO mode. >>> + * The preferred method of selecting PIO modes (e.g. mode 4) is >>> + * "echo 'piomode:4' > /proc/ide/hdx/settings". >>> + * Special case is 255: auto-select best mode (used at boot time). >>> + */ > >> The preferred method is to let the driver do the tuning and if for some >> reason somebody wants to change the PIO mode, the ioctl interface is >> preferred over the deprecated "/proc/ide/hdx/settings". > >> Therefore please remove this comment while at it. > > Will do. > >>> +static void cmd64x_tune_drive (ide_drive_t *drive, u8 pio) >>> +{ >>> + pio = cmd64x_tune_pio(drive, pio); >>> + (void) ide_config_drive_speed(drive, XFER_PIO_0 + pio); >>> } > >>> static u8 cmd64x_ratemask (ide_drive_t *drive) >>> @@ -387,22 +375,6 @@ static u8 cmd64x_ratemask (ide_drive_t * >>> return mode; >>> } >>> >>> -static void config_cmd64x_chipset_for_pio (ide_drive_t *drive, u8 >>> set_speed) >>> -{ >>> - u8 speed = 0x00; >>> - u8 set_pio = ide_get_best_pio_mode(drive, 4, 5, NULL); >>> - >>> - cmd64x_tuneproc(drive, set_pio); >>> - speed = XFER_PIO_0 + set_pio; >>> - if (set_speed) >>> - (void) ide_config_drive_speed(drive, speed); >>> -} >>> - >>> -static void config_chipset_for_pio (ide_drive_t *drive, u8 set_speed) >>> -{ >>> - config_cmd64x_chipset_for_pio(drive, set_speed); >>> -} > >> [ ... ] > >>> @@ -461,8 +436,6 @@ static int config_chipset_for_dma (ide_d >>> { >>> u8 speed = ide_dma_speed(drive, cmd64x_ratemask(drive)); >>> >>> - config_chipset_for_pio(drive, !speed); >>> - > >> While this was always incorrectly setting PIO4, the PIO4 is "the usual" case >> and for this driver we need to program PIO explicitly even when using >> DMA. > > Hm, why it's *so* special, i.e. why almost all the other drivers can get > away without it (the majority seems to have autotune set *only* if > hwif->dma_base is seen as 0 in the init_hwif() method? :-/
This is really a bug and drivers get away with it because for many systems BIOS will setup the correct PIO mode and/or ->speeproc method also sets the PIO mode on the chipset while setting DMA. In cmd64x case getting PIO setup right matters a bit more since there are many add-on cards using CMD/SiI chipsets and they can also be used on non-x86 systems (I don't think that there are non-x86 BIOS/firmwares for these cards). >> The core code doesn't program PIO mode unless told to (->autotune flag == 1) >> so after the above change PIO mode won't be programmed et all. > >> I think that we now need to set ->autotune unconditionally in >> init_hwif_cmd64x(). > > No problem. This actually seems the right thing to do in all drivers, > just like the libata core does. :-) Yes. Thanks, Bart - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html