TODO: atiixp.c: fix ->tuneproc for pio == 255 (Noticed by Sergei)

Sergei Shtylyov wrote:
>>               hwif->speedproc(drive, speed);
>
> Well, well, the tuneproc() method can't ahndle auto-tunuing here (255)...

TODO: sis5513.c: "sis5513_tune_drive(drive, 5);" (Noticed by Sergei)

Bartlomiej Zolnierkiewicz wrote:
> Sergei Shtylyov wrote:
>>> -     if (ide_use_fast_pio(drive)) {
>>> +     if (ide_use_fast_pio(drive))
>>>              sis5513_tune_drive(drive, 5);

>>    Ugh, PIO fallback effectively always tries to set mode 4 here (thanks
>> it's not 5). Mess.

> Yep, but it seems to be even more complicated since config_art_rwp_pio()
> is a mess^2 - chipset is programmed to the best PIO mode while the
> device is set to PIO4... *sigh*...

TODO: it821x smart mode mode setting

On 1/12/07, Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> On 1/12/07, Alan <alan@lxorguk.ukuu.org.uk> wrote:
> > > It seems that it821x_tune_chipset() is buggy since it sends SET FEATURES
> > > command even when in smart mode.  Shouldn't there be "don't tune" flag
> > > in it812x_fixups() to tell it821x_tune_chipset() to not send SET FEATURES
> > > commands?
> >
> > It's itdev->smart but falls through to ide_config_drive_speed while it
> > should probably just copy bits of the code from it. Thats all fixed in
> > the libata driver which allows chip specific mode setup.
> 
> ide_config_drive_speed() does the following things:
> 1. disables host DMA
> 2. sends SET FEATURES command (in polling mode)
> 3. re-enables host DMA (only if DMA mode was set)
> 4. updates drive->id->dma_{ultra,mword,1word}
> 5. updates drive->{current,init}_speed
> 
> 1. and 2. should really go to higher layers
> 3. obviously shouldn't be done for itdev->smart
> 4. also shouldn't be done for itdev>smart
>    (we are not changing speed mode)
> 5. should be done once outside of it821x_tune_chipset()
> 
> Therefore current TODO looks like:
> * moving DMA fiddling code out of ide_config_drive_speed()
> * setting drive->{current,init} speed in it821x_fixups()
> * fixing it821x not to call ide_config_drive_speed() for itdev->smart
> 
> Fixing user space generated requests requires more work
> (i.e. adding ->set_mode method)...

TODO: piix READ/WRITE LONG requires prefetch to be off (Alan knows details)

TODO: sl82c105

Sergei Shtylyov wrote:
>>       DBG(("sl82c105_ide_dma_off_quietly(drive:%s)\n", drive->name));
>>
>> -     rc = __ide_dma_off_quietly(drive);
>> +     ide_dma_off_quietly(drive);
>>       if (drive->pio_speed)
> 
>    Should always be non-zero since explicitly initialized.

TODO: ide-dma ide_get_mode_mask()/ide_max_dma_mode()

Sergei Shtylyov wrote:
>    I didn't quite like the array/loop approach but well, that's my taste (I'd
> rather put the mode-from-mask evaluation to the function and call it thrice)...

The mode-from-mask approach is indeed nicer.  If you send me a patch to
use the mode-from-mask evaluation I'll happily apply it. :)

However the array/loop approach is also definitively an improvement
over the current code.

TODO: serverworks/siimage - simplify UDMA filter (set masks during initialization)

Sergei Shtylyov wrote:
>> -static u8 svwks_ratemask (ide_drive_t *drive)
>> +static u8 svwks_filter_udma_mask(ide_drive_t *drive)
>>  {
>>       struct pci_dev *dev     = HWIF(drive)->pci_dev;
>> -     u8 mode = 0;
>> +     u8 mask = 0;
> 
>    Hm, this looks like it needs rework...

...some time later. 8)

>>       if (!svwks_revision)
>>               pci_read_config_byte(dev, PCI_REVISION_ID, &svwks_revision);
>>
>>       if (dev->device == PCI_DEVICE_ID_SERVERWORKS_HT1000IDE)
>> -             return 2;
>> +             return 0x1f;
>>       if (dev->device == PCI_DEVICE_ID_SERVERWORKS_OSB4IDE) {
>>               u32 reg = 0;
>>               if (isa_dev)
>> @@ -86,25 +86,31 @@ static u8 svwks_ratemask (ide_drive_t *d
>>               if(drive->media == ide_disk)
>>                       return 0;
>>               /* Check the OSB4 DMA33 enable bit */
>> -             return ((reg & 0x00004000) == 0x00004000) ? 1 : 0;
>> +             return ((reg & 0x00004000) == 0x00004000) ? 0x07 : 0;
>>       } else if (svwks_revision < SVWKS_CSB5_REVISION_NEW) {
>> -             return 1;
>> +             return 0x07;
>>       } else if (svwks_revision >= SVWKS_CSB5_REVISION_NEW) {
>> -             u8 btr = 0;
>> +             u8 btr = 0, mode;
>>               pci_read_config_byte(dev, 0x5A, &btr);
>>               mode = btr & 0x3;
>> -             if (!eighty_ninty_three(drive))
>> -                     mode = min(mode, (u8)1);
>> +
>>               /* If someone decides to do UDMA133 on CSB5 the same
>>                  issue will bite so be inclusive */
>>               if (mode > 2 && check_in_drive_lists(drive, svwks_bad_ata100))
>>                       mode = 2;
>> +
>> +             switch(mode) {
>> +             case 2:  mask = 0x1f; break;
>> +             case 1:  mask = 0x07; break;
>> +             default: mask = 0x00; break;
>> +             }
>>       }
>>       if (((dev->device == PCI_DEVICE_ID_SERVERWORKS_CSB6IDE) ||
>>            (dev->device == PCI_DEVICE_ID_SERVERWORKS_CSB6IDE2)) &&
>>           (!(PCI_FUNC(dev->devfn) & 1)))
>> -             mode = 2;
>> -     return mode;
>> +             mask = 0x1f;
>> +
>> +     return mask;
>>  }
> 
>    Hm, what was the problem with setting the proper masks based on PCI
> device ID in the init. code?
> That'd have greatly simplified the filter...

I don't see a problem with doing it during initialization but simplifying
this code wasn't the goal of this patch and can be done in the future.

>> Index: b/drivers/ide/pci/siimage.c
>> ===================================================================
>> --- a/drivers/ide/pci/siimage.c
>> +++ b/drivers/ide/pci/siimage.c
>> @@ -116,45 +116,41 @@ static inline unsigned long siimage_seld
>>  }
>>
>>  /**
>> - *   siimage_ratemask        -       Compute available modes
>> - *   @drive: IDE drive
>> + *   sil_filter_udma_mask    -       compute UDMA mask
>> + *   @drive: IDE device
>> + *
>> + *   Compute the available UDMA speeds for the device on the interface.
>>   *
>> - *   Compute the available speeds for the devices on the interface.
>>   *   For the CMD680 this depends on the clocking mode (scsc), for the
>> - *   SI3312 SATA controller life is a bit simpler. Enforce UDMA33
>> - *   as a limit if there is no 80pin cable present.
>> + *   SI3112 SATA controller life is a bit simpler.
>>   */
>> -
>> -static byte siimage_ratemask (ide_drive_t *drive)
>> +
>> +static u8 sil_filter_udma_mask(ide_drive_t *drive)
>>  {
>> -     ide_hwif_t *hwif        = HWIF(drive);
>> -     u8 mode = 0, scsc = 0;
>> +     ide_hwif_t *hwif = drive->hwif;
>>       unsigned long base = (unsigned long) hwif->hwif_data;
>> +     u8 mask = 0, scsc = 0;
>>
>>       if (hwif->mmio)
>>               scsc = hwif->INB(base + 0x4A);
>>       else
>>               pci_read_config_byte(hwif->pci_dev, 0x8A, &scsc);
>>
>> -     if(is_sata(hwif))
>> -     {
>> -             if(strstr(drive->id->model, "Maxtor"))
>> -                     return 3;
>> -             return 4;
>> +     if (is_sata(hwif)) {
>> +             mask = strstr(drive->id->model, "Maxtor") ? 0x3f : 0x7f;
>> +             goto out;
>>       }
>> -
>> +
>>       if ((scsc & 0x30) == 0x10)      /* 133 */
>> -             mode = 4;
>> +             mask = 0x7f;
>>       else if ((scsc & 0x30) == 0x20) /* 2xPCI */
>> -             mode = 4;
>> +             mask = 0x7f;
>>       else if ((scsc & 0x30) == 0x00) /* 100 */
>> -             mode = 3;
>> +             mask = 0x3f;
>>       else    /* Disabled ? */
>>               BUG();
> 
>    Most probably this is doable at init. time also...

ditto

TODO: aec62xx

Sergei Shtylyov wrote:
>>       hwif->mwdma_mask = 0x07;
>>       hwif->swdma_mask = 0x07;
> 
>    Hm, caught another nit: this driver doesn't actually support single-word
> DMA modes... :-)

TODO: alim15x3

Sergei Shtylyov wrote:
>> Index: b/drivers/ide/pci/alim15x3.c
>> ===================================================================
>> --- a/drivers/ide/pci/alim15x3.c
>> +++ b/drivers/ide/pci/alim15x3.c
>> @@ -765,8 +765,17 @@ static void __devinit init_hwif_common_a
>>
>>       hwif->atapi_dma = 1;
>>
>> -     if (m5229_revision > 0x20)
>> -             hwif->ultra_mask = 0x7f;
>> +     if (m5229_revision <= 0x20)
>> +             hwif->ultra_mask = 0x00; /* no udma */
>> +     else if (m5229_revision < 0xC2)
>> +             hwif->ultra_mask = 0x07; /* udma0-2 */
>> +     else if (m5229_revision == 0xC2 || m5229_revision == 0xC3)
>> +             hwif->ultra_mask = 0x1f; /* udma0-4 */
>> +     else if (m5229_revision == 0xC4)
>> +             hwif->ultra_mask = 0x3f; /* udma0-5 */
>> +     else
>> +             hwif->ultra_mask = 0x7f; /* udma0-6 */
>> +
>>       hwif->mwdma_mask = 0x07;
>>       hwif->swdma_mask = 0x07;
> 
>    Ugh, I'm not seeing any *actual* support for MW/SW DMA in this driver...
> And PIO setting via speedproc() method is broken -- it passes to tuneproc()
> method mode number not biased by -XFER_PIO_0 beforehand.  Will cook up some
> patch, maybe... :-/

TODO: IORDY for PIO modes < 3

On Saturday 23 June 2007, Sergei Shtylyov wrote:

> is not quite correct. As have been noted before, when you set a PIO mode using 
> Set Transfer Mode subcode of the Set Features command, you're always selecting 
> the flow control mode, i.e. using IORDY. So, the last condition in this if 

TODO: pdc202xx_* vs IORDY for PIO modes < 3

On Sunday 24 June 2007, Sergei Shtylyov wrote:

>     Well, some drivers (like pdc202xx_*) don't do the IORDY thing right for 
> PIO modes < 3 as well...