Re: [PATCH] (2.6.20-rc7) cmd64x: fix PIO mode setup (take 2)

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

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 <sshtylyov@ru.mvista.com>
> 
>>> 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 <andre@linux-ide.org>
>>> + * Copyright (C) 2007                MontaVista Software, Inc. <source@mvista.com>
>>>  */
>>>
>>> #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