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

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

[Sergei Shtylyov]

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>

---
This is against the current Linus tree and unfortunately I was able to only
compile test it since that tree gives MODPOST warning and dies early.
Bob, please test it if/when you'll be able to and report the results...

 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
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/hpt366.c		Version 1.11	Aug 4, 2007
+ * linux/drivers/ide/pci/hpt366.c		Version 1.12	Aug 5, 2007
  *
  * Copyright (C) 1999-2003		Andre Hedrick <andre@linux-ide.org>
  * Portions Copyright (C) 2001	        Sun Microsystems, Inc.
@@ -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) {
-	case HPT370A:
-		if (!HPT370_ALLOW_ATA100_5 ||
-		    check_in_drive_list(drive, bad_ata100_5))
-			return 0x1f;
-		else
-			return 0x3f;
-	case HPT370:
-		if (!HPT370_ALLOW_ATA100_5 ||
-		    check_in_drive_list(drive, bad_ata100_5))
-			mask = 0x1f;
-		else
-			mask = 0x3f;
-		break;
 	case HPT36x:
 		if (!HPT366_ALLOW_ATA66_4 ||
 		    check_in_drive_list(drive, bad_ata66_4))
@@ -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;
+		break;
+	case HPT370A:
+		if (!HPT370_ALLOW_ATA100_5 ||
+		    check_in_drive_list(drive, bad_ata100_5))
+			return 0x1f;
+		else
+			return 0x3f;
+	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))
+			return 0x71;
+		/* fall thru */
 	default:
 		return 0x7f;
 	}
@@ -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;
 
 	/*
 	 * HPT3xxN chips have some complications:

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

[Bartlomiej Zolnierkiewicz]

On Sunday 05 August 2007, Sergei Shtylyov 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

> ---
> This is against the current Linus tree and unfortunately I was able to only
> compile test it since that tree gives MODPOST warning and dies early.
> Bob, please test it if/when you'll be able to and report the results...
> 
>  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
> @@ -1,5 +1,5 @@
>  /*
> - * linux/drivers/ide/pci/hpt366.c		Version 1.11	Aug 4, 2007
> + * linux/drivers/ide/pci/hpt366.c		Version 1.12	Aug 5, 2007
>   *
>   * Copyright (C) 1999-2003		Andre Hedrick <andre@linux-ide.org>
>   * Portions Copyright (C) 2001	        Sun Microsystems, Inc.
> @@ -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.

> -	case HPT370A:
> -		if (!HPT370_ALLOW_ATA100_5 ||
> -		    check_in_drive_list(drive, bad_ata100_5))
> -			return 0x1f;
> -		else
> -			return 0x3f;
> -	case HPT370:
> -		if (!HPT370_ALLOW_ATA100_5 ||
> -		    check_in_drive_list(drive, bad_ata100_5))
> -			mask = 0x1f;
> -		else
> -			mask = 0x3f;
> -		break;
>  	case HPT36x:
>  		if (!HPT366_ALLOW_ATA66_4 ||
>  		    check_in_drive_list(drive, bad_ata66_4))
> @@ -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

> +		break;
> +	case HPT370A:
> +		if (!HPT370_ALLOW_ATA100_5 ||
> +		    check_in_drive_list(drive, bad_ata100_5))
> +			return 0x1f;
> +		else
> +			return 0x3f;

ditto

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

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

IMO all HPT*_ALLOW_ATA* defines should just go away...

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), init_setup_hpt366() and
hpt366_chipsets[] (by removing udma_mask).

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

Thanks,
Bart

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

[Sergei Shtylyov]

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

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

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday 10 August 2007, Sergei Shtylyov wrote:
> 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.

If we can use this one instead it would be even better.

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

I bet that you are looking at vanilla kernel which currently misses

 101 files changed, 1880 insertions(+), 2828 deletions(-)

please look at -mm or IDE quilt tree instead. :)

ide_rate_filter() happily uses ide_find_dma_mode() nowadays (however this
hpt366 patch is for vanilla kernel which doesn't have the needed changes).

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

No strong feelings about these defines but I think that they actually make
the code less readable and also more complex because they control _both_
DPLL used (through controlling max_ultra) and the maximum UDMA mask.

Moreover they are _compile_ time options so for testing purposes we may
as well ask user to change UDMA mask etc.

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

We won't be adding a single line of new code:

- the current ->udma_filter implementation does everything needed already

- in init_chipset_hpt366() we simply would replace

		if (info->max_ultra > 6)

  with

		if (info->chip_type >= HPT374)

  (this change depends on the current HPT3xx enums order
   and on removal HPT*_ALLOW_ATA* defines)

I wouldn't be surprised if we actually _decrease_ the driver size a bit
(in addition to removal of ~35 LOC).

> > 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? ;-)

Iff you base the new patch on top of IDE quilt tree otherwise I'll have
to fix it _again_. ;-)

Thanks,
Bart

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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

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

[...]

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

> If we can use this one instead it would be even better.

    Only by wrapping it up with the argument typecast. :-)
That function calls another inline, ata_id_major_version() which is quite 
clumsy and useless for this case (does a bit scan in the word 80), so 
introducing our own may be better...

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

> I bet that you are looking at vanilla kernel which currently misses

    Of course.

>  101 files changed, 1880 insertions(+), 2828 deletions(-)

> please look at -mm or IDE quilt tree instead. :)

    Looking...

> ide_rate_filter() happily uses ide_find_dma_mode() nowadays (however this
> hpt366 patch is for vanilla kernel which doesn't have the needed changes).

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

> No strong feelings about these defines but I think that they actually make
> the code less readable and also more complex because they control _both_
> DPLL used (through controlling max_ultra) and the maximum UDMA mask.

    That's because the maximum UDMA mask depends on the DPLL frequency...

> Moreover they are _compile_ time options so for testing purposes we may
> as well ask user to change UDMA mask etc.

    ... and UltraDMA/100 is *not* reachable with 66 MHz clock (it will have to 
use the same timings as UltraDMA/66 -- so changing the mask only is just not 
enough.
    Now you can hopefully see that these #define's as they are now exist for a 
good reason... :-)

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

> We won't be adding a single line of new code:

> - the current ->udma_filter implementation does everything needed already

    Not really. It will return 0x7f for chipset not supporting it

> - in init_chipset_hpt366() we simply would replace

> 		if (info->max_ultra > 6)

    Actually,( info->max_ultra == 6)

>   with

> 		if (info->chip_type >= HPT374)

    This is just wrong -- HPT374 does not tolarate 66 MHz clock.  You probably 
meant HPT372 (or >)?

>   (this change depends on the current HPT3xx enums order
>    and on removal HPT*_ALLOW_ATA* defines)

    Heh, how about doing this (pardon for the bad... er, sed language):

	default:
		return s/0x71/drive->hwif->ultra_mask/;

without all any changes that you've proposed and being done with that fix? :-)

> I wouldn't be surprised if we actually _decrease_ the driver size a bit
> (in addition to removal of ~35 LOC).

    Decrasing .init.text section's width doesn't buy you much.

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

> :-)

    Unfortunately, it's being spent off-PC too.

>>>>@@ -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? ;-)

> Iff you base the new patch on top of IDE quilt tree otherwise I'll have
> to fix it _again_. ;-)

    I hope you haven't forgotten the basic rule: "the fixes come first"? :-)
    And why fix it again, if I'm not going to drop that part?
    I just felt your pain going thru the (already obsolete) series and fixing 
the rejects -- not only due to my patches... my patchutils are outdated. :-/

> Thanks,
> Bart

MBR, Sergei

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

[Bartlomiej Zolnierkiewicz]

On Saturday 11 August 2007, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>>>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
> 
> [...]

> >>>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.
> 
> > No strong feelings about these defines but I think that they actually make
> > the code less readable and also more complex because they control _both_
> > DPLL used (through controlling max_ultra) and the maximum UDMA mask.
> 
>     That's because the maximum UDMA mask depends on the DPLL frequency...
> 
> > Moreover they are _compile_ time options so for testing purposes we may
> > as well ask user to change UDMA mask etc.
> 
>     ... and UltraDMA/100 is *not* reachable with 66 MHz clock (it will have to 
> use the same timings as UltraDMA/66 -- so changing the mask only is just not 
> enough.
> 
>     Now you can hopefully see that these #define's as they are now exist for a 
> good reason... :-)

Yes but I still believe that there should be some cleaner solution... ;)

> >>>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.
> 
> > We won't be adding a single line of new code:
> 
> > - the current ->udma_filter implementation does everything needed already
> 
>     Not really. It will return 0x7f for chipset not supporting it

Which is 100% correct given current values of HPT*_ALLOW_ATA133_6 defines.

> > - in init_chipset_hpt366() we simply would replace
> 
> > 		if (info->max_ultra > 6)
> 
>     Actually,( info->max_ultra == 6)

Yes.
 
> >   with
> 
> > 		if (info->chip_type >= HPT374)
> 
>     This is just wrong -- HPT374 does not tolarate 66 MHz clock.  You probably 
> meant HPT372 (or >)?

Yes, ">".

> >   (this change depends on the current HPT3xx enums order
> >    and on removal HPT*_ALLOW_ATA* defines)
> 
>     Heh, how about doing this (pardon for the bad... er, sed language):
> 
> 	default:
> 		return s/0x71/drive->hwif->ultra_mask/;
> 
> without all any changes that you've proposed and being done with that fix? :-)

I hope that you meant:

	default:
		return s/0x7f/drive->hwif->ultra_mask/;

Yep, I'm fine with it.

> >>>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? ;-)
> 
> > Iff you base the new patch on top of IDE quilt tree otherwise I'll have
> > to fix it _again_. ;-)
> 
>     I hope you haven't forgotten the basic rule: "the fixes come first"? :-)

The basic rule is to keep the process steady. :-)

"The fixes come first" is the 2-nd rule and becomes a bit hard to keep up
when there is almost hundred patches in the series.

>     And why fix it again, if I'm not going to drop that part?

Indeed, there is no need to fix again...

Thanks,
Bart

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

[Sergei Shtylyov]

Hello, I wrote:

>>  101 files changed, 1880 insertions(+), 2828 deletions(-)

>> please look at -mm or IDE quilt tree instead. :)

>    Looking...

    When are you planning to push out to Linus the 
ide-mode-limiting-fixes-for-user-requested-speed-changes.patch? I'd like my 
HPT37x SATA mode filtering stuff to be atop of this one, after looking at it.

>> ide_rate_filter() happily uses ide_find_dma_mode() nowadays (however this
>> hpt366 patch is for vanilla kernel which doesn't have the needed 
>> changes).

    Yeah, it keeps being in the same vein (same bug rather :-) as the old 
code, i.e. not looking at hwif->mwdma_mask when falling back in 
ide_rate_filter()...

[...]

>> I wouldn't be surprised if we actually _decrease_ the driver size a bit
>> (in addition to removal of ~35 LOC).

>    Decrasing .init.text section's width doesn't buy you much.

    Ugh, I meant "decreasing", not decraZing (another freudian slip :-) and 
"length", not "width"

>> Thanks,
>> Bart

MBR, Sergei

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

[Bartlomiej Zolnierkiewicz]

On Saturday 11 August 2007, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >>  101 files changed, 1880 insertions(+), 2828 deletions(-)
> 
> >> please look at -mm or IDE quilt tree instead. :)
> 
> >    Looking...
> 
>     When are you planning to push out to Linus the 
> ide-mode-limiting-fixes-for-user-requested-speed-changes.patch? I'd like my 
> HPT37x SATA mode filtering stuff to be atop of this one, after looking at it.

Preferably 2.6.24 material and ide_rate_filter() FIXME (respecting device PIO
limits) still needs to be addressed before pushing all mode limiting patches
upstream.

> >> ide_rate_filter() happily uses ide_find_dma_mode() nowadays (however this
> >> hpt366 patch is for vanilla kernel which doesn't have the needed 
> >> changes).
> 
>     Yeah, it keeps being in the same vein (same bug rather :-) as the old 
> code, i.e. not looking at hwif->mwdma_mask when falling back in 
> ide_rate_filter()...

Worth fixing but deserves a separate patch.

Bart

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

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>> 101 files changed, 1880 insertions(+), 2828 deletions(-)

>>>>please look at -mm or IDE quilt tree instead. :)

>>>   Looking...

>>    When are you planning to push out to Linus the 
>>ide-mode-limiting-fixes-for-user-requested-speed-changes.patch? I'd like my 
>>HPT37x SATA mode filtering stuff to be atop of this one, after looking at it.

> Preferably 2.6.24 material and ide_rate_filter() FIXME (respecting device PIO
> limits) still needs to be addressed before pushing all mode limiting patches
> upstream.

    Maybe I'll look into this...

>>>>ide_rate_filter() happily uses ide_find_dma_mode() nowadays (however this
>>>>hpt366 patch is for vanilla kernel which doesn't have the needed 
>>>>changes).

>>    Yeah, it keeps being in the same vein (same bug rather :-) as the old 
>>code, i.e. not looking at hwif->mwdma_mask when falling back in 
>>ide_rate_filter()...

> Worth fixing but deserves a separate patch.

    It does. Unfortunately, after you said that this issue has been already
dealt width, I've dropped (and lost) the code fixing this -- will have to redo
it now. :-/
    I'm now envisioning the HPT37[24] SATA filtering work as series of n patches:

[1/4] introduce drive_is_sata() helper + minor fix to eighty_ninty_three()
[2/4] hpt366: UltraDMA mode filter for SATA cards
[3/4] fix ide_rate_filter() to respect hwif->mwdma_mask
[4/4] introduce mwdma_filter() method and use it for HPT37x-based SATA cards

> Bart

MBR, Sergei

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

[Sergei Shtylyov]

Hello, I wrote:

>>>    When are you planning to push out to Linus the 
>>> ide-mode-limiting-fixes-for-user-requested-speed-changes.patch? I'd 
>>> like my HPT37x SATA mode filtering stuff to be atop of this one, 
>>> after looking at it.

>> Preferably 2.6.24 material and ide_rate_filter() FIXME (respecting 
>> device PIO
>> limits) still needs to be addressed before pushing all mode limiting 
>> patches
>> upstream.

>    Maybe I'll look into this...

    PIO stuff seems done.

>>>>> ide_rate_filter() happily uses ide_find_dma_mode() nowadays 
>>>>> (however this
>>>>> hpt366 patch is for vanilla kernel which doesn't have the needed 
>>>>> changes).

>>>    Yeah, it keeps being in the same vein (same bug rather :-) as the 
>>> old code, i.e. not looking at hwif->mwdma_mask when falling back in 
>>> ide_rate_filter()...

>> Worth fixing but deserves a separate patch.

    Fixed (I hope).

>    It does. Unfortunately, after you said that this issue has been already
> dealt width, I've dropped (and lost) the code fixing this -- will have 
> to redo
> it now. :-/
>    I'm now envisioning the HPT37[24] SATA filtering work as series of n 
> patches:

> [1/4] introduce drive_is_sata() helper + minor fix to eighty_ninty_three()
> [2/4] hpt366: UltraDMA mode filter for SATA cards
> [3/4] fix ide_rate_filter() to respect hwif->mwdma_mask
> [4/4] introduce mwdma_filter() method and use it for HPT37x-based SATA 
> cards

    Looks like I'm done with [3/4] and almost done with [4/4]...
    "I have no more whiskey, I have to go home" %-)

>> Bart

MBR, Sergei

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

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>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
[...]
>>+	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>.

    Right in this patch, or a later cleanup?

>>+			return 0x71;
>>+		/* fall thru */
>> 	default:
>> 		return 0x7f;

MBR, Sergei

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

[Bartlomiej Zolnierkiewicz]

On Saturday 11 August 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>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
> [...]
> >>+	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>.
> 
>     Right in this patch, or a later cleanup?

I prefer doing it in this patch but later cleanup is also OK...

Thanks.
Bart

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

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

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

>>[...]

>>>>+	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>.
>>
>>    Right in this patch, or a later cleanup?

> I prefer doing it in this patch but later cleanup is also OK...

    Alas, you'll have to wait till the next weekend -- the patch doesn't seem 
to be critical though, it didn't fix the bug... :-]

> Thanks.
> Bart

MBR, Sergei

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

[Sergei Shtylyov]

Hello.

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>

[...]

> Also now that ->udma_filter is always present the initial hwif->ultra_mask

    Aha, so this method's semantics intended to *completely override* the 
ultra_mask field?!  Wouldn't it be better to make the code behave more 
consistent, i.e. in ide_get_mode_mask() do:

         unsigned int mask = 0;

         switch(base) {
         case XFER_UDMA_0:
                 if ((id->field_valid & 4) == 0)
                         break;

                 if (hwif->udma_filter)
                         mask = hwif->udma_filter(drive);
		else
                         mask = hwif->ultra_mask;

                 mask &= id->dma_ultra;
                 if ((mask & 0x78) && (eighty_ninty_three(drive) == 0))
                         mask &= 0x07;
                 break;
         case XFER_MW_DMA_0:
                 if ((id->field_valid & 2) == 0)
                         break;

                 if (hwif->mdma_filter)
                         mask = hwif->mdma_filter(drive);
		else
                         mask = hwif->mwdma_mask;
                 mask &= id->dma_mword;
                 break;

to avoid the further confusion? ;-)

> Thanks,
> Bart

WBR, Sergei

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

[Sergei Shtylyov]

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>

>>[...]

>>>Also now that ->udma_filter is always present the initial hwif->ultra_mask

>>    Aha, so this method's semantics intended to *completely override* the 
>>ultra_mask field?!  Wouldn't it be better to make the code behave more 
>>consistent, i.e. in ide_get_mode_mask() do:

>>         unsigned int mask = 0;
>>
>>         switch(base) {
>>         case XFER_UDMA_0:
>>                 if ((id->field_valid & 4) == 0)
>>                         break;
>>
>>                 if (hwif->udma_filter)
>>                         mask = hwif->udma_filter(drive);
>>		else
>>                         mask = hwif->ultra_mask;
>>
>>                 mask &= id->dma_ultra;
>>                 if ((mask & 0x78) && (eighty_ninty_three(drive) == 0))
>>                         mask &= 0x07;
>>                 break;
>>         case XFER_MW_DMA_0:
>>                 if ((id->field_valid & 2) == 0)
>>                         break;
>>
>>                 if (hwif->mdma_filter)
>>                         mask = hwif->mdma_filter(drive);
>>		else
>>                         mask = hwif->mwdma_mask;
>>                 mask &= id->dma_mword;
>>                 break;

>>to avoid the further confusion? ;-)

> Fine with me but you forgot to attach a patch. ;)

    That was only a trial ball. ;-)

> Bart

MBR, Sergei