Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

[Jeff Garzik <jgarzik@pobox.com>]

On 09/12/2009 08:17 PM, Jung-Ik (John) Lee wrote:
> On Sat, Sep 12, 2009 at 9:49 AM, Robert Hancock<hancockrwd@gmail.com>  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.

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.

	Jeff