lkml.org 
[lkml]   [2009]   [Sep]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers
    On Sat, Sep 12, 2009 at 5:38 PM, Jeff Garzik <jgarzik@pobox.com> 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@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.

    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-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/

    \
     
     \ /
      Last update: 2009-09-13 02:55    [W:0.045 / U:0.644 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site