Re: [PATCH 1/5] ide: add short cables support

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

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, &reg48h);
> >  		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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/