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