lkml.org 
[lkml]   [2008]   [Aug]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: Kernel Summit request for Discussion of future of ATA (libata) and IDE
    Alan Cox wrote:
    >> I guess that bit doesn't really make any difference with remotely modern
    >> drives, then.. Could we make that ata_id_has_dword_io check always
    >> return true if ata_id_is_ata returns true and only check word 48 if not?
    >
    > I suspect (but need to dig further) that ata_id_has_dword_io should only
    > be called by pata_legacy.
    >
    >> I saw Willy Tarreau's patch from February for this, I agree that we
    >> should likely use a separate data_xfer method for 32-bit transfer (or if
    >> enough controllers should support 32-bit, then just make it be the
    >> default and make a separate 16-bit only function for those that don't),
    >> rather than punting the decision to the user with hdparm.
    >
    > Definitely.
    >
    >> You mentioned in the thread for Willy's patch that "some
    >> controllers have quirky rules for 32bit xfers" - any details anywhere?
    >
    > There are two main ones
    >
    > - Some controllers only support 32bit I/O for a multiple of 32bit values
    > [sometimes 'unless the fifo is disabled']. I'd have to go back over the
    > docs but I think the AMD may be one of those
    > - Some controllers (VLB generally) require a magic sequence before the
    > transfer. You'll see that in the pata_legacy bits.

    Here's my first cut at it. Compile tested only. This sets most controllers
    to use 32-bit PIO except for those which could potentially be on a real ISA
    or other 16-bit bus. It's a bit non-obvious what to do with some of the
    drivers, so input is welcome.

    This implementation doesn't check the ata_id_has_dword_io at all, since it
    would only make a difference on controllers where we don't really want to
    use it anyway.

    It seems like regardless of whether we do 32-bit by default or not the 32-bit
    data_xfer function should be added to libata core as we have several drivers
    which duplicate the same code currently..

    Signed-off-by: Robert Hancock <hancockr@shaw.ca>

    diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
    index 304fdc6..acb65a9 100644
    --- a/drivers/ata/libata-sff.c
    +++ b/drivers/ata/libata-sff.c
    @@ -673,13 +673,14 @@ static inline void ata_tf_to_host(struct ata_port *ap,
    }

    /**
    - * ata_sff_data_xfer - Transfer data by PIO
    + * ata_sff_data_xfer_16bit - Transfer data by PIO
    * @dev: device to target
    * @buf: data buffer
    * @buflen: buffer length
    * @rw: read/write
    *
    - * Transfer data from/to the device data register by PIO.
    + * Transfer data from/to the device data register by PIO using only
    + * 16-bit transfers.
    *
    * LOCKING:
    * Inherited from caller.
    @@ -687,7 +688,7 @@ static inline void ata_tf_to_host(struct ata_port *ap,
    * RETURNS:
    * Bytes consumed.
    */
    -unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
    +unsigned int ata_sff_data_xfer_16bit(struct ata_device *dev, unsigned char *buf,
    unsigned int buflen, int rw)
    {
    struct ata_port *ap = dev->link->ap;
    @@ -719,6 +720,51 @@ unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
    }

    /**
    + * ata_sff_data_xfer - Transfer data by PIO
    + * @dev: device to target
    + * @buf: data buffer
    + * @buflen: buffer length
    + * @rw: read/write
    + *
    + * Transfer data from/to the device data register by PIO.
    + *
    + * LOCKING:
    + * Inherited from caller.
    + *
    + * RETURNS:
    + * Bytes consumed.
    + */
    +unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
    + unsigned int buflen, int rw)
    +{
    + struct ata_port *ap = dev->link->ap;
    + void __iomem *data_addr = ap->ioaddr.data_addr;
    + unsigned int words = buflen >> 2;
    + unsigned int slop = buflen & 3;
    +
    + /* Transfer multiple of 4 bytes */
    + if (rw == READ)
    + ioread32_rep(data_addr, buf, words);
    + else
    + iowrite32_rep(data_addr, buf, words);
    +
    + /* Transfer trailing 1 byte, if any. */
    + if (unlikely(slop)) {
    + __le32 pad = 0;
    + if (rw == READ) {
    + pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
    + memcpy(buf + buflen - slop, &pad, slop);
    + } else {
    + memcpy(&pad, buf + buflen - slop, slop);
    + iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
    + }
    + buflen += 4 - slop;
    + }
    +
    + return buflen;
    +}
    +
    +/**
    * ata_sff_data_xfer_noirq - Transfer data by PIO
    * @dev: device to target
    * @buf: data buffer
    @@ -748,6 +794,36 @@ unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev, unsigned char *buf,
    }

    /**
    + * ata_sff_data_xfer_noirq_16bit - Transfer data by PIO
    + * @dev: device to target
    + * @buf: data buffer
    + * @buflen: buffer length
    + * @rw: read/write
    + *
    + * Transfer data from/to the device data register by PIO. Do the
    + * transfer with interrupts disabled using only 16-bit transfers.
    + *
    + * LOCKING:
    + * Inherited from caller.
    + *
    + * RETURNS:
    + * Bytes consumed.
    + */
    +unsigned int ata_sff_data_xfer_noirq_16bit(struct ata_device *dev,
    + unsigned char *buf,
    + unsigned int buflen, int rw)
    +{
    + unsigned long flags;
    + unsigned int consumed;
    +
    + local_irq_save(flags);
    + consumed = ata_sff_data_xfer_16bit(dev, buf, buflen, rw);
    + local_irq_restore(flags);
    +
    + return consumed;
    +}
    +
    +/**
    * ata_pio_sector - Transfer a sector of data.
    * @qc: Command on going
    *
    @@ -2827,7 +2903,9 @@ EXPORT_SYMBOL_GPL(ata_sff_tf_load);
    EXPORT_SYMBOL_GPL(ata_sff_tf_read);
    EXPORT_SYMBOL_GPL(ata_sff_exec_command);
    EXPORT_SYMBOL_GPL(ata_sff_data_xfer);
    +EXPORT_SYMBOL_GPL(ata_sff_data_xfer_16bit);
    EXPORT_SYMBOL_GPL(ata_sff_data_xfer_noirq);
    +EXPORT_SYMBOL_GPL(ata_sff_data_xfer_noirq_16bit);
    EXPORT_SYMBOL_GPL(ata_sff_irq_on);
    EXPORT_SYMBOL_GPL(ata_sff_irq_clear);
    EXPORT_SYMBOL_GPL(ata_sff_hsm_move);
    diff --git a/drivers/ata/pata_at32.c b/drivers/ata/pata_at32.c
    index 82fb6e2..aa90b19 100644
    --- a/drivers/ata/pata_at32.c
    +++ b/drivers/ata/pata_at32.c
    @@ -173,6 +173,7 @@ static struct scsi_host_template at32_sht = {
    static struct ata_port_operations at32_port_ops = {
    .inherits = &ata_sff_port_ops,
    .cable_detect = ata_cable_40wire,
    + .sff_data_xfer = ata_sff_data_xfer_16bit,
    .set_piomode = pata_at32_set_piomode,
    };

    diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
    index cf9e984..a1f09ca 100644
    --- a/drivers/ata/pata_icside.c
    +++ b/drivers/ata/pata_icside.c
    @@ -336,7 +336,7 @@ static struct ata_port_operations pata_icside_port_ops = {
    .inherits = &ata_sff_port_ops,
    /* no need to build any PRD tables for DMA */
    .qc_prep = ata_noop_qc_prep,
    - .sff_data_xfer = ata_sff_data_xfer_noirq,
    + .sff_data_xfer = ata_sff_data_xfer_noirq_16bit,
    .bmdma_setup = pata_icside_bmdma_setup,
    .bmdma_start = pata_icside_bmdma_start,
    .bmdma_stop = pata_icside_bmdma_stop,
    diff --git a/drivers/ata/pata_isapnp.c b/drivers/ata/pata_isapnp.c
    index 6a111ba..6f38f76 100644
    --- a/drivers/ata/pata_isapnp.c
    +++ b/drivers/ata/pata_isapnp.c
    @@ -26,6 +26,7 @@ static struct scsi_host_template isapnp_sht = {
    static struct ata_port_operations isapnp_port_ops = {
    .inherits = &ata_sff_port_ops,
    .cable_detect = ata_cable_40wire,
    + .sff_data_xfer = ata_sff_data_xfer_16bit,
    };

    /**
    diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
    index bc037ff..d919934 100644
    --- a/drivers/ata/pata_legacy.c
    +++ b/drivers/ata/pata_legacy.c
    @@ -226,12 +226,12 @@ static const struct ata_port_operations legacy_base_port_ops = {

    static struct ata_port_operations simple_port_ops = {
    .inherits = &legacy_base_port_ops,
    - .sff_data_xfer = ata_sff_data_xfer_noirq,
    + .sff_data_xfer = ata_sff_data_xfer_noirq_16bit,
    };

    static struct ata_port_operations legacy_port_ops = {
    .inherits = &legacy_base_port_ops,
    - .sff_data_xfer = ata_sff_data_xfer_noirq,
    + .sff_data_xfer = ata_sff_data_xfer_noirq_16bit,
    .set_mode = legacy_set_mode,
    };

    @@ -286,39 +286,18 @@ static void pdc20230_set_piomode(struct ata_port *ap, struct ata_device *adev)
    static unsigned int pdc_data_xfer_vlb(struct ata_device *dev,
    unsigned char *buf, unsigned int buflen, int rw)
    {
    - if (ata_id_has_dword_io(dev->id)) {
    - struct ata_port *ap = dev->link->ap;
    - int slop = buflen & 3;
    - unsigned long flags;
    + unsigned long flags;

    - local_irq_save(flags);
    + local_irq_save(flags);

    - /* Perform the 32bit I/O synchronization sequence */
    - ioread8(ap->ioaddr.nsect_addr);
    - ioread8(ap->ioaddr.nsect_addr);
    - ioread8(ap->ioaddr.nsect_addr);
    + /* Perform the 32bit I/O synchronization sequence */
    + ioread8(ap->ioaddr.nsect_addr);
    + ioread8(ap->ioaddr.nsect_addr);
    + ioread8(ap->ioaddr.nsect_addr);

    - /* Now the data */
    - if (rw == READ)
    - ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
    - else
    - iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
    -
    - if (unlikely(slop)) {
    - __le32 pad;
    - if (rw == READ) {
    - pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
    - memcpy(buf + buflen - slop, &pad, slop);
    - } else {
    - memcpy(&pad, buf + buflen - slop, slop);
    - iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
    - }
    - buflen += 4 - slop;
    - }
    - local_irq_restore(flags);
    - } else
    - buflen = ata_sff_data_xfer_noirq(dev, buf, buflen, rw);
    + buflen = ata_sff_data_xfer(dev, buf, buflen, rw);

    + local_irq_restore(flags);
    return buflen;
    }

    @@ -733,33 +712,6 @@ static unsigned int qdi_qc_issue(struct ata_queued_cmd *qc)
    return ata_sff_qc_issue(qc);
    }

    -static unsigned int vlb32_data_xfer(struct ata_device *adev, unsigned char *buf,
    - unsigned int buflen, int rw)
    -{
    - struct ata_port *ap = adev->link->ap;
    - int slop = buflen & 3;
    -
    - if (ata_id_has_dword_io(adev->id)) {
    - if (rw == WRITE)
    - iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
    - else
    - ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
    -
    - if (unlikely(slop)) {
    - __le32 pad;
    - if (rw == WRITE) {
    - memcpy(&pad, buf + buflen - slop, slop);
    - iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
    - } else {
    - pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
    - memcpy(buf + buflen - slop, &pad, slop);
    - }
    - }
    - return (buflen + 3) & ~3;
    - } else
    - return ata_sff_data_xfer(adev, buf, buflen, rw);
    -}
    -
    static int qdi_port(struct platform_device *dev,
    struct legacy_probe *lp, struct legacy_data *ld)
    {
    @@ -773,19 +725,19 @@ static struct ata_port_operations qdi6500_port_ops = {
    .inherits = &legacy_base_port_ops,
    .set_piomode = qdi6500_set_piomode,
    .qc_issue = qdi_qc_issue,
    - .sff_data_xfer = vlb32_data_xfer,
    + .sff_data_xfer = ata_sff_data_xfer,
    };

    static struct ata_port_operations qdi6580_port_ops = {
    .inherits = &legacy_base_port_ops,
    .set_piomode = qdi6580_set_piomode,
    - .sff_data_xfer = vlb32_data_xfer,
    + .sff_data_xfer = ata_sff_data_xfer,
    };

    static struct ata_port_operations qdi6580dp_port_ops = {
    .inherits = &legacy_base_port_ops,
    .set_piomode = qdi6580dp_set_piomode,
    - .sff_data_xfer = vlb32_data_xfer,
    + .sff_data_xfer = ata_sff_data_xfer,
    };

    static DEFINE_SPINLOCK(winbond_lock);
    @@ -856,7 +808,7 @@ static int winbond_port(struct platform_device *dev,
    static struct ata_port_operations winbond_port_ops = {
    .inherits = &legacy_base_port_ops,
    .set_piomode = winbond_set_piomode,
    - .sff_data_xfer = vlb32_data_xfer,
    + .sff_data_xfer = ata_sff_data_xfer,
    };

    static struct legacy_controller controllers[] = {
    diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
    index a9e8273..0ab2c8e 100644
    --- a/drivers/ata/pata_mpc52xx.c
    +++ b/drivers/ata/pata_mpc52xx.c
    @@ -265,6 +265,7 @@ static struct ata_port_operations mpc52xx_ata_port_ops = {
    .cable_detect = ata_cable_40wire,
    .set_piomode = mpc52xx_ata_set_piomode,
    .post_internal_cmd = ATA_OP_NULL,
    + .sff_data_xfer = ata_sff_data_xfer_16bit,
    };

    static int __devinit
    diff --git a/drivers/ata/pata_pcmcia.c b/drivers/ata/pata_pcmcia.c
    index 41b4361..8cb4923 100644
    --- a/drivers/ata/pata_pcmcia.c
    +++ b/drivers/ata/pata_pcmcia.c
    @@ -133,7 +133,7 @@ static struct scsi_host_template pcmcia_sht = {

    static struct ata_port_operations pcmcia_port_ops = {
    .inherits = &ata_sff_port_ops,
    - .sff_data_xfer = ata_sff_data_xfer_noirq,
    + .sff_data_xfer = ata_sff_data_xfer_noirq_16bit,
    .cable_detect = ata_cable_40wire,
    .set_mode = pcmcia_set_mode,
    };
    diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
    index 8f65ad6..3310eb0 100644
    --- a/drivers/ata/pata_platform.c
    +++ b/drivers/ata/pata_platform.c
    @@ -52,7 +52,7 @@ static struct scsi_host_template pata_platform_sht = {

    static struct ata_port_operations pata_platform_port_ops = {
    .inherits = &ata_sff_port_ops,
    - .sff_data_xfer = ata_sff_data_xfer_noirq,
    + .sff_data_xfer = ata_sff_data_xfer_noirq_16bit,
    .cable_detect = ata_cable_unknown,
    .set_mode = pata_platform_set_mode,
    .port_start = ATA_OP_NULL,
    diff --git a/drivers/ata/pata_qdi.c b/drivers/ata/pata_qdi.c
    index 63b7a1c..36cc778 100644
    --- a/drivers/ata/pata_qdi.c
    +++ b/drivers/ata/pata_qdi.c
    @@ -124,35 +124,6 @@ static unsigned int qdi_qc_issue(struct ata_queued_cmd *qc)
    return ata_sff_qc_issue(qc);
    }

    -static unsigned int qdi_data_xfer(struct ata_device *dev, unsigned char *buf,
    - unsigned int buflen, int rw)
    -{
    - if (ata_id_has_dword_io(dev->id)) {
    - struct ata_port *ap = dev->link->ap;
    - int slop = buflen & 3;
    -
    - if (rw == READ)
    - ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
    - else
    - iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
    -
    - if (unlikely(slop)) {
    - __le32 pad;
    - if (rw == READ) {
    - pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
    - memcpy(buf + buflen - slop, &pad, slop);
    - } else {
    - memcpy(&pad, buf + buflen - slop, slop);
    - iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
    - }
    - buflen += 4 - slop;
    - }
    - } else
    - buflen = ata_sff_data_xfer(dev, buf, buflen, rw);
    -
    - return buflen;
    -}
    -
    static struct scsi_host_template qdi_sht = {
    ATA_PIO_SHT(DRV_NAME),
    };
    @@ -160,7 +131,6 @@ static struct scsi_host_template qdi_sht = {
    static struct ata_port_operations qdi6500_port_ops = {
    .inherits = &ata_sff_port_ops,
    .qc_issue = qdi_qc_issue,
    - .sff_data_xfer = qdi_data_xfer,
    .cable_detect = ata_cable_40wire,
    .set_piomode = qdi6500_set_piomode,
    };
    diff --git a/drivers/ata/pata_winbond.c b/drivers/ata/pata_winbond.c
    index a7606b0..56f5f8e 100644
    --- a/drivers/ata/pata_winbond.c
    +++ b/drivers/ata/pata_winbond.c
    @@ -92,42 +92,12 @@ static void winbond_set_piomode(struct ata_port *ap, struct ata_device *adev)
    }


    -static unsigned int winbond_data_xfer(struct ata_device *dev,
    - unsigned char *buf, unsigned int buflen, int rw)
    -{
    - struct ata_port *ap = dev->link->ap;
    - int slop = buflen & 3;
    -
    - if (ata_id_has_dword_io(dev->id)) {
    - if (rw == READ)
    - ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
    - else
    - iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
    -
    - if (unlikely(slop)) {
    - __le32 pad;
    - if (rw == READ) {
    - pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
    - memcpy(buf + buflen - slop, &pad, slop);
    - } else {
    - memcpy(&pad, buf + buflen - slop, slop);
    - iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
    - }
    - buflen += 4 - slop;
    - }
    - } else
    - buflen = ata_sff_data_xfer(dev, buf, buflen, rw);
    -
    - return buflen;
    -}
    -
    static struct scsi_host_template winbond_sht = {
    ATA_PIO_SHT(DRV_NAME),
    };

    static struct ata_port_operations winbond_port_ops = {
    .inherits = &ata_sff_port_ops,
    - .sff_data_xfer = winbond_data_xfer,
    .cable_detect = ata_cable_40wire,
    .set_piomode = winbond_set_piomode,
    };
    diff --git a/include/linux/libata.h b/include/linux/libata.h
    index 06b8033..643328a 100644
    --- a/include/linux/libata.h
    +++ b/include/linux/libata.h
    @@ -1481,8 +1481,12 @@ extern void ata_sff_exec_command(struct ata_port *ap,
    const struct ata_taskfile *tf);
    extern unsigned int ata_sff_data_xfer(struct ata_device *dev,
    unsigned char *buf, unsigned int buflen, int rw);
    +extern unsigned int ata_sff_data_xfer_16bit(struct ata_device *dev,
    + unsigned char *buf, unsigned int buflen, int rw);
    extern unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev,
    unsigned char *buf, unsigned int buflen, int rw);
    +extern unsigned int ata_sff_data_xfer_noirq_16bit(struct ata_device *dev,
    + unsigned char *buf, unsigned int buflen, int rw);
    extern u8 ata_sff_irq_on(struct ata_port *ap);
    extern void ata_sff_irq_clear(struct ata_port *ap);
    extern int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,


    \
     
     \ /
      Last update: 2008-08-06 02:25    [W:0.048 / U:1.584 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site