Re: [PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode
Hi, On Friday 20 April 2007, Sergei Shtylyov wrote: > Hello, I wrote: > > 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_udma_filter(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); > > Hm, found a buglet in my former filtering rewrite -- the condition in the > preceding if stmt should be a reverse one, and speed limitation to > XFER_UDMA_3 should have been left under it. With the current code, > XFER_UDMA_2 limitation wouldn't have been applied if the same drive is not in > both 'bad_ata66_4' and 'bad_ata66_3' lists -- this, however, actually is not > the case since WDC AC310200R drive is in both these lists (maybe I wrote it > this way because of this fact :-). IIRC I've noticed this during the review of the filtering rewrite but I though that it was meant to be this way. :) > +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) > > >>> This case 0x01 will *never* be hit for HPT370 chip with the new > >>> code, therefore the filter won't get applied. > > >> Oh, and for HPT36x chips used with 40c cable too (unless they're > >> artificaially reduced to UltraDMA/33 by the driver #define's). > > > It will still get applied since the code always resorts to looking up > > the 'bad_ata33' list for HPT36x/370. > > I've got a bit muddled in my own code -- not sure if it got much clearer > > after I'd untangled hpt3xx_ratemask() / hpt3xx_ratefilter() puzzle. :-) > > Yeah, I'm definitely having trouble understanding my own code after some > months have passed... :-/ The filtering code badly needs more comments/documentation and it was already true for the old code (before your rewrite). Bart - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode
Hello, I wrote: 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_udma_filter(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); Hm, found a buglet in my former filtering rewrite -- the condition in the preceding if stmt should be a reverse one, and speed limitation to XFER_UDMA_3 should have been left under it. With the current code, XFER_UDMA_2 limitation wouldn't have been applied if the same drive is not in both 'bad_ata66_4' and 'bad_ata66_3' lists -- this, however, actually is not the case since WDC AC310200R drive is in both these lists (maybe I wrote it this way because of this fact :-). +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) This case 0x01 will *never* be hit for HPT370 chip with the new code, therefore the filter won't get applied. Oh, and for HPT36x chips used with 40c cable too (unless they're artificaially reduced to UltraDMA/33 by the driver #define's). It will still get applied since the code always resorts to looking up the 'bad_ata33' list for HPT36x/370. I've got a bit muddled in my own code -- not sure if it got much clearer after I'd untangled hpt3xx_ratemask() / hpt3xx_ratefilter() puzzle. :-) Yeah, I'm definitely having trouble understanding my own code after some months have passed... :-/ MBR, Sergei - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode
Hello, I wrote: [PATCH] ide: rework the code for selecting the best DMA transfer mode Depends on the "ide: fix UDMA/MWDMA/SWDMA masks" patch. I'm now trying to rewrite hpt366.c to benefit more from these patches... and alas, this very patch seems to be breaking filtering (at least) in this driver. :-] Ignore me, I've seemingly misundertood the code. :-< 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_udma_filter(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) This case 0x01 will *never* be hit for HPT370 chip with the new code, therefore the filter won't get applied. Oh, and for HPT36x chips used with 40c cable too (unless they're artificaially reduced to UltraDMA/33 by the driver #define's). It will still get applied since the code always resorts to looking up the 'bad_ata33' list for HPT36x/370. I've got a bit muddled in my own code -- not sure if it got much clearer after I'd untangled hpt3xx_ratemask() / hpt3xx_ratefilter() puzzle. :-) @@ -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; Well, that case 0x00 should never actually happen. Yeah, that's true, 'case 0x00' (and even 'default') labels could have been removed. MBR, Sergei - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode
Hello, I wrote: Bartlomiej Zolnierkiewicz wrote: [PATCH] ide: rework the code for selecting the best DMA transfer mode Depends on the "ide: fix UDMA/MWDMA/SWDMA masks" patch. I'm now trying to rewrite hpt366.c to benefit more from these patches... and alas, this very patch seems to be breaking filtering (at least) in this driver. :-] 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->udma_filter) +mask &= hwif->udma_filter(drive); + +if ((mask & 0x78) && (eighty_ninty_three(drive) == 0)) +mask &= 0x07; Note the subtle difference between the old and new behavior: the old driver code was applying UltraDMA filter *after* the cable type limit, and the new code does it *before*. Was there any particular reason to change that order? Ah, I see. Cable-reduced mask can't be passed back to driver's filter. This needs changing. MBR, Sergei - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode
Sergei Shtylyov wrote: Hello. Bartlomiej Zolnierkiewicz wrote: [PATCH] ide: rework the code for selecting the best DMA transfer mode Depends on the "ide: fix UDMA/MWDMA/SWDMA masks" patch. I'm now trying to rewrite hpt366.c to benefit more from these patches... and alas, this very patch seems to be breaking filtering (at least) in this driver. :-] 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->udma_filter) +mask &= hwif->udma_filter(drive); + +if ((mask & 0x78) && (eighty_ninty_three(drive) == 0)) +mask &= 0x07; Note the subtle difference between the old and new behavior: the old driver code was applying UltraDMA filter *after* the cable type limit, and the new code does it *before*. Was there any particular reason to change that order? [...] 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_udma_filter(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) This case 0x01 will *never* be hit for HPT370 chip with the new code, therefore the filter won't get applied. Oh, and for HPT36x chips used with 40c cable too (unless they're artificaially reduced to UltraDMA/33 by the driver #define's). @@ -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; Well, that case 0x00 should never actually happen. } -return speed; +return mask; } [...] @@ -1270,6 +1272,7 @@ static void __devinit init_hwif_hpt366(i hwif->intrproc= &hpt3xx_intrproc; hwif->maskproc= &hpt3xx_maskproc; hwif->busproc= &hpt3xx_busproc; +hwif->udma_filter= &hpt3xx_udma_filter; In fact, we only need a filter for HPT36x/370 chips -- I'll address it in my patch. /* * HPT3xxN chips have some complications: MBR, Sergei - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [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 Depends on the "ide: fix UDMA/MWDMA/SWDMA masks" patch. I'm now trying to rewrite hpt366.c to benefit more from these patches... and alas, this very patch seems to be breaking filtering (at least) in this driver. :-] 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->udma_filter) + mask &= hwif->udma_filter(drive); + + if ((mask & 0x78) && (eighty_ninty_three(drive) == 0)) + mask &= 0x07; Note the subtle difference between the old and new behavior: the old driver code was applying UltraDMA filter *after* the cable type limit, and the new code does it *before*. + 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); + void ide_dma_verbose(ide_drive_t *drive) { struct hd_driveid *id = drive->id; [...] 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_udma_filter(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;
Re: [PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode
[PATCH] ide: rework the code for selecting the best DMA transfer mode Depends on the "ide: fix UDMA/MWDMA/SWDMA masks" patch. * add ide_hwif_t.udma_filter hook for filtering UDMA mask (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() v2: * rename ->filter_udma_mask to ->udma_filter [ Suggested by Sergei Shtylyov <[EMAIL PROTECTED]>. ] Signed-off-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> --- drivers/ide/arm/icside.c |2 drivers/ide/cris/ide-cris.c|2 drivers/ide/ide-dma.c | 74 drivers/ide/ide-iops.c |2 drivers/ide/ide-lib.c | 125 + drivers/ide/ide.c |1 drivers/ide/pci/aec62xx.c | 32 -- drivers/ide/pci/alim15x3.c | 76 +--- drivers/ide/pci/atiixp.c | 20 -- drivers/ide/pci/cmd64x.c | 65 - drivers/ide/pci/cs5535.c | 20 -- drivers/ide/pci/hpt34x.c |9 -- drivers/ide/pci/hpt366.c | 67 +++-- drivers/ide/pci/it8213.c | 20 -- drivers/ide/pci/it821x.c | 20 -- drivers/ide/pci/jmicron.c | 21 -- drivers/ide/pci/pdc202xx_new.c | 14 drivers/ide/pci/pdc202xx_old.c | 27 drivers/ide/pci/piix.c | 66 - drivers/ide/pci/serverworks.c | 31 ++ drivers/ide/pci/siimage.c | 45 ++ drivers/ide/pci/sis5513.c | 14 drivers/ide/pci/slc90e66.c | 13 drivers/ide/pci/tc86c001.c |9 -- drivers/ide/pci/triflex.c |4 - include/linux/ide.h| 10 +-- 26 files changed, 234 insertions(+), 555 deletions(-) Index: b/drivers/ide/arm/icside.c === --- a/drivers/ide/arm/icside.c +++ b/drivers/ide/arm/icside.c @@ -347,7 +347,7 @@ static int icside_dma_check(ide_drive_t * Enable DMA on any drive that has multiword DMA */ if (id->field_valid & 2) { - xfer_mode = ide_dma_speed(drive, 0); + xfer_mode = ide_max_dma_mode(drive); goto out; } Index: b/drivers/ide/cris/ide-cris.c === --- a/drivers/ide/cris/ide-cris.c +++ b/drivers/ide/cris/ide-cris.c @@ -1006,7 +1006,7 @@ static int cris_ide_build_dmatable (ide_ static int cris_config_drive_for_dma (ide_drive_t *drive) { - u8 speed = ide_dma_speed(drive, 1); + u8 speed = ide_max_dma_mode(drive); if (!speed) return 0; 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->udma_filter) + mask &= hwif->udma_filter(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_g
Re: [PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode
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 = in
Re: [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
[PATCH 14/15] ide: rework the code for selecting the best DMA transfer mode
[PATCH] ide: rework the code for selecting the best DMA transfer mode Depends on the "ide: fix UDMA/MWDMA/SWDMA masks" patch. * add ide_hwif_t.filter_udma_mask hook for filtering UDMA mask (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() Signed-off-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> --- drivers/ide/arm/icside.c |2 drivers/ide/cris/ide-cris.c|2 drivers/ide/ide-dma.c | 74 drivers/ide/ide-iops.c |2 drivers/ide/ide-lib.c | 125 + drivers/ide/ide.c |1 drivers/ide/pci/aec62xx.c | 32 -- drivers/ide/pci/alim15x3.c | 76 +--- drivers/ide/pci/atiixp.c | 20 -- drivers/ide/pci/cmd64x.c | 66 - drivers/ide/pci/cs5535.c | 20 -- drivers/ide/pci/hpt34x.c |9 -- drivers/ide/pci/hpt366.c | 67 +++-- drivers/ide/pci/it8213.c | 20 -- drivers/ide/pci/it821x.c | 20 -- drivers/ide/pci/jmicron.c | 21 -- drivers/ide/pci/pdc202xx_new.c | 14 drivers/ide/pci/pdc202xx_old.c | 27 drivers/ide/pci/piix.c | 66 - drivers/ide/pci/serverworks.c | 31 ++ drivers/ide/pci/siimage.c | 45 ++ drivers/ide/pci/sis5513.c | 14 drivers/ide/pci/slc90e66.c | 13 drivers/ide/pci/tc86c001.c |9 -- drivers/ide/pci/triflex.c |4 - include/linux/ide.h| 10 +-- 26 files changed, 235 insertions(+), 555 deletions(-) Index: b/drivers/ide/arm/icside.c === --- a/drivers/ide/arm/icside.c +++ b/drivers/ide/arm/icside.c @@ -347,7 +347,7 @@ static int icside_dma_check(ide_drive_t * Enable DMA on any drive that has multiword DMA */ if (id->field_valid & 2) { - xfer_mode = ide_dma_speed(drive, 0); + xfer_mode = ide_max_dma_mode(drive); goto out; } Index: b/drivers/ide/cris/ide-cris.c === --- a/drivers/ide/cris/ide-cris.c +++ b/drivers/ide/cris/ide-cris.c @@ -1006,7 +1006,7 @@ static int cris_ide_build_dmatable (ide_ static int cris_config_drive_for_dma (ide_drive_t *drive) { - u8 speed = ide_dma_speed(drive, 1); + u8 speed = ide_max_dma_mode(drive); if (!speed) return 0; 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; +