Bartlomiej Zolnierkiewicz wrote:
The Marvell bridge chips used on HighPoint SATA cards do not seem to support the UltraDMA modes 1, 2, and 3 (as well as any MWDMA modes), so the driver needs to account for this in the udma_filter() method. In order to achieve that, do the following changes:
- install the method for all chips, not only HPT36x/370 (improve code formatting by killing an extra tabs while at it);
- add to the end of the 'switch' statement in hpt3xx_udma_filter() case for HPT372[AN] and HPT374 chips upon which the SATA cards are based and check there whether we're dealing with SATA drive (by looking at words 80 and 93 of the drive's identify data), reorder HPT370[A] cases for consistency...
Signed-off-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>
applied but
drivers/ide/pci/hpt366.c | 75 ++++++++++++++++++++++++++--------------------- 1 files changed, 43 insertions(+), 32 deletions(-)
Index: linux-2.6/drivers/ide/pci/hpt366.c =================================================================== --- linux-2.6.orig/drivers/ide/pci/hpt366.c +++ linux-2.6/drivers/ide/pci/hpt366.c
[...]
@@ -517,29 +517,17 @@ static int check_in_drive_list(ide_drive } /* - * Note for the future; the SATA hpt37x we must set - * either PIO or UDMA modes 0,4,5 + * The Marvell bridge chips used on the HighPoint SATA cards do not seem + * to support the UltraDMA modes 1, 2, and 3 -- as well as any MWDMA modes + * (that we should start filtering out once the IDE core allows that). */ - static u8 hpt3xx_udma_filter(ide_drive_t *drive) { struct hpt_info *info = pci_get_drvdata(HWIF(drive)->pci_dev); + struct hd_driveid *id = drive->id; u8 mask; switch (info->chip_type) {
HPT374/HPT372[NA] case could be added here so re-ordering wouldn't be needed.
I did that on purpose -- to keep an alphanumeric ordering. ;-)
@@ -551,6 +539,30 @@ static u8 hpt3xx_udma_filter(ide_drive_t check_in_drive_list(drive, bad_ata66_3)) mask = 0x07; break; + case HPT370: + if (!HPT370_ALLOW_ATA100_5 || + check_in_drive_list(drive, bad_ata100_5)) + mask = 0x1f; + else + mask = 0x3f;
ATA_UDMA* defines should be used if you insist on re-ordering
OK, recasting...
+ case HPT372 : + case HPT372A: + case HPT372N: + case HPT374 : + /* + * Check for SATA drive by verifying that the word 93 is 0 and + * the drive is ATA-5 or higher compatible. + */ + if (id->hw_config == 0 && (id->major_rev_num & 0x7fe0))
Same check as in ide-iops.c::eighty_ninty_three(). Would make sense to add ide_id_is_sata_dev() inline to <linux/ide.h>.
Actually, libata already has ata_id_is_sata() defined in <linux/ata.h> but it takes <const u16 *> argument.
+ return 0x71; + /* fall thru */ default: return 0x7f;
HPT371[N]/HPT302[N] will use the default mask which is correct but adds hidden dependency on HPT*_ALLOW_ATA_133 being always defined as "1".
No, it doesn't since all this will be AND'ed with & hwif->udma_mask... But wait, ide_rate_filter has the different code, it just sets mask to the result of the udma_filter() method... I wonder which code is correct? :-O
IMO all HPT*_ALLOW_ATA* defines should just go away...
I think it's still worth to keep 'em alive for the possible blacklist additions.
Also now that ->udma_filter is always present the initial hwif->ultra_mask doesn't matter so as well we may set it to ATA_UDMA6 (0x7f) and cleanup struct hpt_info (by removing max_ultra after fixing init_chipset_hpt366() to use info->chip_type >= HPT374 check instead),
It's all interesting but you've missed one aspect -- this will make the kernel larger while the current code keeps all this logic in the init.text section.
init_setup_hpt366() and hpt366_chipsets[] (by removing udma_mask).
I'll think about it in my copious free time (I have plenty of time spent offline now indeed :-)...
@@ -1229,25 +1241,24 @@ static unsigned int __devinit init_chips static void __devinit init_hwif_hpt366(ide_hwif_t *hwif) { - struct pci_dev *dev = hwif->pci_dev; - struct hpt_info *info = pci_get_drvdata(dev); - int serialize = HPT_SERIALIZE_IO; - u8 scr1 = 0, ata66 = hwif->channel ? 0x01 : 0x02; - u8 chip_type = info->chip_type; - u8 new_mcr, old_mcr = 0; + struct pci_dev *dev = hwif->pci_dev; + struct hpt_info *info = pci_get_drvdata(dev); + int serialize = HPT_SERIALIZE_IO; + u8 scr1 = 0, ata66 = hwif->channel ? 0x01 : 0x02; + u8 chip_type = info->chip_type; + u8 new_mcr, old_mcr = 0; /* Cache the channel's MISC. control registers' offset */ - hwif->select_data = hwif->channel ? 0x54 : 0x50; + hwif->select_data = hwif->channel ? 0x54 : 0x50; - hwif->tuneproc = &hpt3xx_tune_drive; - hwif->speedproc = &hpt3xx_tune_chipset; - hwif->quirkproc = &hpt3xx_quirkproc; - hwif->intrproc = &hpt3xx_intrproc; - hwif->maskproc = &hpt3xx_maskproc; - hwif->busproc = &hpt3xx_busproc; + hwif->tuneproc = &hpt3xx_tune_drive; + hwif->speedproc = &hpt3xx_tune_chipset; + hwif->quirkproc = &hpt3xx_quirkproc; + hwif->intrproc = &hpt3xx_intrproc; + hwif->maskproc = &hpt3xx_maskproc; + hwif->busproc = &hpt3xx_busproc; - if (chip_type <= HPT370A) - hwif->udma_filter = &hpt3xx_udma_filter; + hwif->udma_filter = &hpt3xx_udma_filter;
Uh, the only real change here consists of the three lines above, the rest is just a noise caused by removal of one tab.
Such changes are really not worth it - in this case it caused rejects in two patches from IDE quilt tree which I had to fix manually.
I hope now that you've fixed it, I may leave this part intact? ;-)
Thanks, Bart
MBR, Sergei - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
