On Sat, Sep 12, 2009 at 5:38 PM, Jeff Garzik <jgarzik@xxxxxxxxx> wrote: > On 09/12/2009 08:17 PM, Jung-Ik (John) Lee wrote: >> >> On Sat, Sep 12, 2009 at 9:49 AM, Robert Hancock<hancockrwd@xxxxxxxxx> >> wrote: >>> >>> On 09/12/2009 04:59 AM, Jung-Ik (John) Lee wrote: >>> >>> (snip) >>> >>> Looks mostly reasonable to me, other than a few issues: >>> >>>> +static void atp867x_set_piomode(struct ata_port *ap, struct ata_device >>>> *adev) >>>> +{ >>>> + struct ata_device *peer = ata_dev_pair(adev); >>>> + struct atp867x_priv *dp = ap->private_data; >>>> + u8 speed = adev->pio_mode; >>>> + struct ata_timing t, p; >>>> + int T, UT; >>>> + u8 b; >>>> + >>>> + T = 1000000000 / 33333; >>>> + UT = T/4; >>>> + >>>> + switch (speed) { >>>> + case XFER_PIO_4: >>>> + case XFER_PIO_3: >>>> + case XFER_PIO_2: >>>> + case XFER_PIO_1: >>>> + case XFER_PIO_0: >>>> + case XFER_PIO_SLOW: >>>> + break; >>>> + default: >>>> + printk(KERN_WARNING "ATP867X: Unsupported speed %#x." >>>> + " Default to XFER_PIO_0.\n", (unsigned)speed); >>>> + speed = XFER_PIO_0; >>>> + } >>>> + >>>> + ata_timing_compute(adev, speed,&t, T, UT); >>>> + if (peer&& peer->pio_mode) { >>>> + ata_timing_compute(peer, peer->pio_mode,&p, T, UT); >>>> + ata_timing_merge(&p,&t,&t, ATA_TIMING_8BIT); >>>> + } >>>> + >>>> + b = inb(dp->dma_mode); >>>> + if (adev->devno& 1) >>>> + b = (b& ~ATP867X_IO_DMAMODE_SLAVE_MASK); >>>> + else >>>> + b = (b& ~ATP867X_IO_DMAMODE_MSTR_MASK); >>>> + outb(b, dp->dma_mode); >>>> + >>>> +#ifdef ATP867X_NO_HACK_PIOMODE >>>> + b = atp867x_get_active_clocks_shifted(t.active) | >>>> + atp867x_get_recover_clocks_shifted(t.recover); >>>> +#else >>>> + /* >>>> + * magic value that works (from doc 6.4, 6.6.9) >>>> + */ >>>> + b = 0x31; >>>> +#endif >>> >>> What's the purpose of this ifdef? >> >> The magic value part must go. I'll update the patch. >> >>> >>>> + if (dp->pci66mhz) >>>> + b += 0x10; >>>> + >>>> + if (adev->devno& 1) >>>> + outb(b, dp->slave_piospd); >>>> + else >>>> + outb(b, dp->mstr_piospd); >>>> + >>>> + /* >>>> + * use the same value for comand timing as for PIO timimg >>>> + */ >>>> + outb(b, dp->eightb_piospd); >>>> +} >>>> + >>>> +static int atp867x_cable_detect(struct ata_port *ap) >>>> +{ >>>> + return ATA_CBL_PATA40_SHORT; >>>> +} >>> >>> Doesn't the controller have a way to do proper 80-wire cable detection? >> >> No programmatic way. libata.force should be used for other configurations. > > Ideally you would use ATA_CBL_PATA_UNK / ata_cable_unknown, but it seems > that ATA_CBL_PATA_UNK has grown rather useless in the current kernel. > > We should probably (a) update libata-core/libata-sff to do something > intelligent with ATA_CBL_PATA_UNK, and (b) use ATA_CBL_PATA_UNK in > pata_atp867x. > > But yes, libata.force works as well. It just seems like we are losing an > opportunity to accept an existing BIOS-configured 80-wire cable > configuration by hardcoding ATA_CBL_PATA40_SHORT. Right. But from my experience relying on BIOS just doesn't seem to work well in all cases though. > > Also, a specific note -- atp867x_cable_detect() should be moved into > drivers/ata/libata-core.c and made generic. We have ata_cable_xxx versions > for everything except 40-short, it seems. Sounds right to me. > > Jeff > > > > > -- 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