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

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

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