lkml.org 
[lkml]   [2007]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode
    Hello.

    Bartlomiej Zolnierkiewicz wrote:

    > [PATCH] ide: rework the code for selecting the best DMA transfer mode

    Here's another portion of comments...

    > Depends on the "ide: fix UDMA/MWDMA/SWDMA masks" patch.

    > * add ide_hwif_t.filter_udma_mask hook for filtering UDMA mask

    Erm, maybe a shorter method name like udma_filter would go with the others
    better. But well, that's my taste. :-)

    > (use it in alim15x3, hpt366, siimage and serverworks drivers)
    > * add ide_max_dma_mode() for finding best DMA mode for the device
    > (loosely based on some older libata-core.c code)
    > * convert ide_dma_speed() users to use ide_max_dma_mode()
    > * make ide_rate_filter() take "ide_drive_t *drive" as an argument instead
    > of "u8 mode" and teach it to how to use UDMA mask to do filtering
    > * use ide_rate_filter() in hpt366 driver
    > * remove no longer needed ide_dma_speed() and *_ratemask()
    > * unexport eighty_ninty_three()

    > Index: b/drivers/ide/ide-dma.c
    > ===================================================================
    > --- a/drivers/ide/ide-dma.c
    > +++ b/drivers/ide/ide-dma.c
    > @@ -705,6 +705,80 @@ int ide_use_dma(ide_drive_t *drive)
    >
    > EXPORT_SYMBOL_GPL(ide_use_dma);
    >
    > +static const u8 xfer_mode_bases[] = {
    > + XFER_UDMA_0,
    > + XFER_MW_DMA_0,
    > + XFER_SW_DMA_0,
    > +};
    > +
    > +static unsigned int ide_get_mode_mask(ide_drive_t *drive, u8 base)
    > +{
    > + struct hd_driveid *id = drive->id;
    > + ide_hwif_t *hwif = drive->hwif;
    > + unsigned int mask = 0;
    > +
    > + switch(base) {
    > + case XFER_UDMA_0:
    > + if ((id->field_valid & 4) == 0)
    > + break;
    > +
    > + mask = id->dma_ultra & hwif->ultra_mask;
    > +
    > + if (hwif->filter_udma_mask)
    > + mask &= hwif->filter_udma_mask(drive);
    > +
    > + if ((mask & 0x78) && (eighty_ninty_three(drive) == 0))
    > + mask &= 0x07;
    > + break;
    > + case XFER_MW_DMA_0:
    > + mask = id->dma_mword & hwif->mwdma_mask;
    > + break;
    > + case XFER_SW_DMA_0:
    > + mask = id->dma_1word & hwif->swdma_mask;
    > + break;
    > + default:
    > + BUG();
    > + break;
    > + }
    > +
    > + return mask;
    > +}
    > +
    > +/**
    > + * ide_max_dma_mode - compute DMA speed
    > + * @drive: IDE device
    > + *
    > + * Checks the drive capabilities and returns the speed to use
    > + * for the DMA transfer. Returns 0 if the drive is incapable
    > + * of DMA transfers.
    > + */
    > +
    > +u8 ide_max_dma_mode(ide_drive_t *drive)
    > +{
    > + ide_hwif_t *hwif = drive->hwif;
    > + unsigned int mask;
    > + int x, i;
    > + u8 mode = 0;
    > +
    > + if (drive->media != ide_disk && hwif->atapi_dma == 0)
    > + return 0;
    > +
    > + for (i = 0; i < ARRAY_SIZE(xfer_mode_bases); i++) {
    > + mask = ide_get_mode_mask(drive, xfer_mode_bases[i]);
    > + x = fls(mask) - 1;
    > + if (x >= 0) {
    > + mode = xfer_mode_bases[i] + x;
    > + break;
    > + }
    > + }
    > +
    > + printk(KERN_DEBUG "%s: selected mode 0x%x\n", drive->name, mode);
    > +
    > + return mode;
    > +}
    > +
    > +EXPORT_SYMBOL_GPL(ide_max_dma_mode);
    > +

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

    > Index: b/drivers/ide/ide-lib.c
    > ===================================================================
    > --- a/drivers/ide/ide-lib.c
    > +++ b/drivers/ide/ide-lib.c
    > @@ -69,123 +69,34 @@ char *ide_xfer_verbose (u8 xfer_rate)
    > EXPORT_SYMBOL(ide_xfer_verbose);
    >
    > /**
    > - * ide_dma_speed - compute DMA speed
    > - * @drive: drive
    > - * @mode: modes available
    > - *
    > - * Checks the drive capabilities and returns the speed to use
    > - * for the DMA transfer. Returns 0 if the drive is incapable
    > - * of DMA transfers.
    > - */
    > -
    > -u8 ide_dma_speed(ide_drive_t *drive, u8 mode)

    [...]

    > -EXPORT_SYMBOL(ide_dma_speed);

    Alas, my ide_dma_speed() fix is going to be oudated rather quickly... :-)

    > Index: b/drivers/ide/pci/hpt366.c
    > ===================================================================
    > --- a/drivers/ide/pci/hpt366.c
    > +++ b/drivers/ide/pci/hpt366.c
    > @@ -513,43 +513,31 @@ static int check_in_drive_list(ide_drive
    > return 0;
    > }
    >
    > -static u8 hpt3xx_ratemask(ide_drive_t *drive)
    > -{
    > - struct hpt_info *info = pci_get_drvdata(HWIF(drive)->pci_dev);
    > - u8 mode = info->max_mode;
    > -
    > - if (!eighty_ninty_three(drive) && mode)
    > - mode = min(mode, (u8)1);
    > - return mode;
    > -}
    > -
    > /*
    > * Note for the future; the SATA hpt37x we must set
    > * either PIO or UDMA modes 0,4,5
    > */
    > -
    > -static u8 hpt3xx_ratefilter(ide_drive_t *drive, u8 speed)
    > +
    > +static u8 hpt3xx_filter_udma_mask(ide_drive_t *drive)
    > {
    > struct hpt_info *info = pci_get_drvdata(HWIF(drive)->pci_dev);
    > u8 chip_type = info->chip_type;
    > - u8 mode = hpt3xx_ratemask(drive);
    > -
    > - if (drive->media != ide_disk)
    > - return min(speed, (u8)XFER_PIO_4);
    > + u8 mode = info->max_mode;
    > + u8 mask;
    >
    > switch (mode) {
    > case 0x04:
    > - speed = min_t(u8, speed, XFER_UDMA_6);
    > + mask = 0x7f;
    > break;
    > case 0x03:
    > - speed = min_t(u8, speed, XFER_UDMA_5);
    > + mask = 0x3f;
    > if (chip_type >= HPT374)
    > break;
    > if (!check_in_drive_list(drive, bad_ata100_5))
    > goto check_bad_ata33;
    > /* fall thru */
    > case 0x02:
    > - speed = min_t(u8, speed, XFER_UDMA_4);
    > + mask = 0x1f;
    >
    > /*
    > * CHECK ME, Does this need to be changed to HPT374 ??
    > @@ -560,13 +548,13 @@ static u8 hpt3xx_ratefilter(ide_drive_t
    > !check_in_drive_list(drive, bad_ata66_4))
    > goto check_bad_ata33;
    >
    > - speed = min_t(u8, speed, XFER_UDMA_3);
    > + mask = 0x0f;
    > if (HPT366_ALLOW_ATA66_3 &&
    > !check_in_drive_list(drive, bad_ata66_3))
    > goto check_bad_ata33;
    > /* fall thru */
    > case 0x01:
    > - speed = min_t(u8, speed, XFER_UDMA_2);
    > + mask = 0x07;
    >
    > check_bad_ata33:
    > if (chip_type >= HPT370A)
    > @@ -576,10 +564,10 @@ static u8 hpt3xx_ratefilter(ide_drive_t
    > /* fall thru */
    > case 0x00:
    > default:
    > - speed = min_t(u8, speed, XFER_MW_DMA_2);
    > + mask = 0x00;
    > break;
    > }
    > - return speed;
    > + return mask;
    > }

    Erm, I see. This driver will need some redesign because 'struct hpt_info'
    was fitted for the old rate filtering model. Looks like the 'max_mode' field
    should be replaced by 'ultra_mask' there...

    > static u32 get_speed_setting(u8 speed, struct hpt_info *info)
    > @@ -607,12 +595,19 @@ static int hpt36x_tune_chipset(ide_drive
    > ide_hwif_t *hwif = HWIF(drive);
    > struct pci_dev *dev = hwif->pci_dev;
    > struct hpt_info *info = pci_get_drvdata(dev);
    > - u8 speed = hpt3xx_ratefilter(drive, xferspeed);
    > + u8 speed = ide_rate_filter(drive, xferspeed);
    > u8 itr_addr = drive->dn ? 0x44 : 0x40;
    > - u32 itr_mask = speed < XFER_MW_DMA_0 ? 0x30070000 :
    > - (speed < XFER_UDMA_0 ? 0xc0070000 : 0xc03800ff);
    > - u32 new_itr = get_speed_setting(speed, info);
    > u32 old_itr = 0;
    > + u32 itr_mask, new_itr;
    > +
    > + /* TODO: move this to ide_rate_filter() [ check ->atapi_dma ] */
    > + if (drive->media != ide_disk)
    > + speed = min_t(u8, speed, XFER_PIO_4);
    > +

    When I think about it, it seems quite stupid to set a PIO mode instead of
    a requested DMA one. So, this is actually a questionable piece of code in
    this driver...

    Well, I must note the sorrow fact that both the IDE susbsytem as a whole
    doesn't keep track of PIO/DMA modes separately (having only current_mode) and
    the many drivers also fail to handle the timing registers shared by PIO/DMA
    modes correctly (well, it's actually also a hardware design issue :-).

    > + itr_mask = speed < XFER_MW_DMA_0 ? 0x30070000 :
    > + (speed < XFER_UDMA_0 ? 0xc0070000 : 0xc03800ff);
    > +
    > + new_itr = get_speed_setting(speed, info);

    Well, I liked this code where it was. But anyway, it's going to be
    replaced RSN...

    > @@ -632,12 +627,19 @@ static int hpt37x_tune_chipset(ide_drive
    > ide_hwif_t *hwif = HWIF(drive);
    > struct pci_dev *dev = hwif->pci_dev;
    > struct hpt_info *info = pci_get_drvdata(dev);
    > - u8 speed = hpt3xx_ratefilter(drive, xferspeed);
    > + u8 speed = ide_rate_filter(drive, xferspeed);
    > u8 itr_addr = 0x40 + (drive->dn * 4);
    > - u32 itr_mask = speed < XFER_MW_DMA_0 ? 0x303c0000 :
    > - (speed < XFER_UDMA_0 ? 0xc03c0000 : 0xc1c001ff);
    > - u32 new_itr = get_speed_setting(speed, info);
    > u32 old_itr = 0;
    > + u32 itr_mask, new_itr;
    > +
    > + /* TODO: move this to ide_rate_filter() [ check ->atapi_dma ] */
    > + if (drive->media != ide_disk)
    > + speed = min_t(u8, speed, XFER_PIO_4);
    > +
    > + itr_mask = speed < XFER_MW_DMA_0 ? 0x303c0000 :
    > + (speed < XFER_UDMA_0 ? 0xc03c0000 : 0xc1c001ff);
    > +
    > + new_itr = get_speed_setting(speed, info);

    Same comments here...

    > Index: b/drivers/ide/pci/serverworks.c
    > ===================================================================
    > --- a/drivers/ide/pci/serverworks.c
    > +++ b/drivers/ide/pci/serverworks.c
    > @@ -65,16 +65,16 @@ static int check_in_drive_lists (ide_dri
    > return 0;
    > }
    >
    > -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...

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

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

    MBR, Sergei

    PS: I understand that the intent wasn not to make this rewrite optimal but
    do it quick and with least affect. And I certainly may handle hpt366.c
    redesign... :-)

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-01-22 21:17    [W:0.053 / U:31.456 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site