Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

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@ru.mvista.com>

> 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