From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bartlomiej Zolnierkiewicz Subject: Re: [PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode Date: Sat, 03 Feb 2007 00:57:51 +0100 Message-ID: <45C3CFFF.6020408@gmail.com> References: <20070119003058.14846.43637.sendpatchset@localhost.localdomain> <20070119003233.14846.5450.sendpatchset@localhost.localdomain> <45B51505.4040905@ru.mvista.com> <58cb370e0702021207o435f39cdsf3abb0d55829fc45@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from ug-out-1314.google.com ([66.249.92.170]:43065 "EHLO ug-out-1314.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1946177AbXBBXxB (ORCPT ); Fri, 2 Feb 2007 18:53:01 -0500 Received: by ug-out-1314.google.com with SMTP id 44so904776uga for ; Fri, 02 Feb 2007 15:52:59 -0800 (PST) In-Reply-To: <58cb370e0702021207o435f39cdsf3abb0d55829fc45@mail.gmail.com> Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Sergei Shtylyov Cc: linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org Hi, Sergei Shtylyov wrote: > > 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. :-) done >> (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)... 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. >> 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... :-) C'est la vie :) >> 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... yes... >> 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... This can safely vanish when core code gets fixed to correctly check device+host supported PIO/DMA modes before calling ->tuneproc/->speedproc for user space originated code change requests. As for now IMHO it is the best to just leave it alone... > 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 :-). Actually, I have an unfinished patch to fix it but it depends on many other unfinished patches... *sigh*... >> + 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... ...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 patches welcomed (I added this to the IDE tree TODO) > 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... :-) Please do. Thanks, Bart