Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 [EMAIL PROTECTED]
[...]
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
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
On Saturday 25 August 2007, Sergei Shtylyov wrote:
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 [EMAIL PROTECTED]
[...]
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. ;)
Bart
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 [EMAIL PROTECTED]
[...]
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
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 =
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 [EMAIL PROTECTED]
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
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 [EMAIL PROTECTED]
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)
Re: [PATCH 2/2] hpt366: UltraDMA filtering for SATA cards
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 [EMAIL PROTECTED]
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.cVersion 1.11Aug 4, 2007
+ * linux/drivers/ide/pci/hpt366.cVersion 1.12Aug 5, 2007
*
* Copyright (C) 1999-2003 Andre Hedrick [EMAIL PROTECTED]
* 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
