On Friday 15 June 2007, Sergei Shtylyov wrote:
> Hello.
>
> Bartlomiej Zolnierkiewicz wrote:
>
> > This patch allows users to override both host and device side cable detection
> > with "ideX=ata66" kernel parameter. Thanks to this it should be now possible
> > to use UDMA > 2 modes on systems (laptops mainly) which use short 40-pin cable
> > instead of 80-pin one.
>
> > Next patches add automatic detection of some systems using short cables.
>
> > Changes:
>
> > * Rename hwif->udma_four to hwif->cbl and make it u8.
>
> Not sure if already short word "cable" was worth further shortening. :-)
The name/usage is the same as in libata.
When we add ->cable_detect method it will make grepping easier. 8)
> > Index: b/drivers/ide/pci/alim15x3.c
> > ===================================================================
> > --- a/drivers/ide/pci/alim15x3.c
> > +++ b/drivers/ide/pci/alim15x3.c
> > @@ -594,7 +594,7 @@ out:
> > * FIXME: frobs bits that are not defined on newer ALi devicea
> > */
> >
> > -static unsigned int __devinit ata66_ali15x3 (ide_hwif_t *hwif)
> > +static u8 __devinit ata66_ali15x3(ide_hwif_t *hwif)
> > {
> > struct pci_dev *dev = hwif->pci_dev;
> > unsigned int ata66 = 0;
> > @@ -657,7 +657,7 @@ static unsigned int __devinit ata66_ali1
> >
> > local_irq_restore(flags);
> >
> > - return(ata66);
> > + return ata66 ? ATA_CBL_PATA80 : ATA_CBL_PATA40;
>
> Ahem... I'd think it was about the right time to fix the abomination which
> those ata66 and cable_80_pin[2] are, something like this:
>
> static unsigned int __devinit ata66_ali15x3 (ide_hwif_t *hwif)
> {
> struct pci_dev *dev = hwif->pci_dev;
> unsigned int cbl = ATA_CBL_PATA40;
> unsigned long flags;
> u8 tmpbyte, mask = hwif->channel ? 0x02 : 0x01;
>
> local_irq_save(flags); /* Not sure if it's necessary... */
>
> if (m5229_revision >= 0xC2) {
> /*
> * Ultra66 cable detection (from Host View)
> * m5229, 0x4a, bit0: primary, bit1: secondary
> * 0: 80 pin, 1: 40 pin
> */
> pci_read_config_byte(dev, 0x4a, &tmpbyte);
> /*
> * Allow ata66 if cable of current channel has 80 pins
> */
> cbl = (tmpbyte & mask) ? ATA_CBL_PATA40 : ATA_CBL_PATA80;
> } else {
>
> [Following code frankly speaking has no business being in this function]
patch #3/5 already takes care of cable_80_pin[2] part,
ata66 part was left as an exercise for the reader ;-)
> local_irq_restore(flags); /* Not sure if it's necessary... */
>
> return cbl;
> > }
> >
> > /**
> > Index: b/drivers/ide/pci/serverworks.c
> > ===================================================================
> > --- a/drivers/ide/pci/serverworks.c
> > +++ b/drivers/ide/pci/serverworks.c
> > @@ -329,9 +329,9 @@ static unsigned int __devinit init_chips
> > return dev->irq;
> > }
> >
> > -static unsigned int __devinit ata66_svwks_svwks (ide_hwif_t *hwif)
> > +static u8 __devinit ata66_svwks_svwks(ide_hwif_t *hwif)
> > {
> > - return 1;
> > + return ATA_CBL_PATA80;
> > }
>
> Hm, worth folding into ata66_svwks()...
Yes and no...
Alan did a very nice rewrite of cable detection code for pata_serverworks.c
driver. We may be better off just back-porting it (patches are welcomed).
> > /* On Dell PowerEdge servers with a CSB5/CSB6, the top two bits
> > @@ -341,7 +341,7 @@ static unsigned int __devinit ata66_svwk
> > * Bit 14 clear = primary IDE channel does not have 80-pin cable.
> > * Bit 14 set = primary IDE channel has 80-pin cable.
> > */
> > -static unsigned int __devinit ata66_svwks_dell (ide_hwif_t *hwif)
> > +static u8 __devinit ata66_svwks_dell(ide_hwif_t *hwif)
> > {
> > struct pci_dev *dev = hwif->pci_dev;
> > if (dev->subsystem_vendor == PCI_VENDOR_ID_DELL &&
>
> Isn't this check duplicating the check in ata66_svwks()?
Yep.
> > @@ -359,18 +359,18 @@ static unsigned int __devinit ata66_svwk
> > *
> > * WARNING: this only works on Alpine hardware!
> > */
> > -static unsigned int __devinit ata66_svwks_cobalt (ide_hwif_t *hwif)
> > +static u8 __devinit ata66_svwks_cobalt(ide_hwif_t *hwif)
> > {
> > struct pci_dev *dev = hwif->pci_dev;
> > if (dev->subsystem_vendor == PCI_VENDOR_ID_SUN &&
>
> Same question here...
Yep.
> > Index: b/drivers/ide/pci/sis5513.c
> > ===================================================================
> > --- a/drivers/ide/pci/sis5513.c
> > +++ b/drivers/ide/pci/sis5513.c
> > @@ -796,7 +796,7 @@ static unsigned int __devinit init_chips
> > return 0;
> > }
> >
> > -static unsigned int __devinit ata66_sis5513 (ide_hwif_t *hwif)
> > +static u8 __devinit ata66_sis5513(ide_hwif_t *hwif)
> > {
> > u8 ata66 = 0;
> >
> > @@ -811,7 +811,8 @@ static unsigned int __devinit ata66_sis5
> > pci_read_config_byte(hwif->pci_dev, 0x48, ®48h);
> > ata66 = (reg48h & mask) ? 0 : 1;
> > }
> > - return ata66;
> > +
> > + return ata66 ? ATA_CBL_PATA80 : ATA_CBL_PATA40;
> > }
>
> That doesn't look good as well. I think we should part with 'ata66'
> variable here completely...
Could be fixed in the follow-up patch as we should also split ata66_sis5513()
into two separate functions (one for chipset_family >= ATA_133 and one for
chipset_family >= ATA_66).
Thanks,
Bart
-
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
