2008-12-15 02:17:20

by Robert Hancock

[permalink] [raw]
Subject: [PATCH] sata_sil: add Large Block Transfer support

This implements support for the Large Block Transfer feature found in Silicon
Image 311x SATA controllers. This allows transferring bigger contiguous chunks
of data from system memory and avoids the 64KB boundary restriction of standard
SFF controllers.

This is based on a patch from Jeff Garzik (from the sii-lbt branch of
libata-dev) but includes a few bug fixes: Since the bmdma2 register does not
implement the status bits, the original bmdma register must be used except
where the bmdma2 register is required. As well, the DMA boundary should be
31-bit instead of 32-bit since the top bit of the length field is still
required for the PRD end-of-table flag, and the sg_tablesize can be set to
the full size of the allocated PRD table since no splitting of entries
will be required.

Signed-off-by: Robert Hancock <[email protected]>

---

Obviously not 2.6.28 material, but could potentially head into .29.
I've done some testing with a DVD drive connected to this controller
and verified that reading off an entire DVD returns correct data
(and that the controller is actually getting requests that cross
64K boundaries). More testing would certainly be useful..

diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index 031d7b7..92c7315 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -46,7 +46,9 @@
#include <linux/libata.h>

#define DRV_NAME "sata_sil"
-#define DRV_VERSION "2.3"
+#define DRV_VERSION "2.4"
+
+#define SIL_DMA_BOUNDARY 0x7fffffffUL

enum {
SIL_MMIO_BAR = 5,
@@ -118,6 +120,10 @@ static void sil_dev_config(struct ata_device *dev);
static int sil_scr_read(struct ata_link *link, unsigned int sc_reg, u32 *val);
static int sil_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val);
static int sil_set_mode(struct ata_link *link, struct ata_device **r_failed);
+static void sil_qc_prep(struct ata_queued_cmd *qc);
+static void sil_bmdma_setup(struct ata_queued_cmd *qc);
+static void sil_bmdma_start(struct ata_queued_cmd *qc);
+static void sil_bmdma_stop(struct ata_queued_cmd *qc);
static void sil_freeze(struct ata_port *ap);
static void sil_thaw(struct ata_port *ap);

@@ -167,13 +173,22 @@ static struct pci_driver sil_pci_driver = {
};

static struct scsi_host_template sil_sht = {
- ATA_BMDMA_SHT(DRV_NAME),
+ ATA_BASE_SHT(DRV_NAME),
+ /** These controllers support Large Block Transfer which allows
+ transfer chunks up to 2GB and which cross 64KB boundaries,
+ therefore the DMA limits are more relaxed than standard ATA SFF. */
+ .dma_boundary = SIL_DMA_BOUNDARY,
+ .sg_tablesize = ATA_MAX_PRD
};

static struct ata_port_operations sil_ops = {
.inherits = &ata_bmdma_port_ops,
.dev_config = sil_dev_config,
.set_mode = sil_set_mode,
+ .bmdma_setup = sil_bmdma_setup,
+ .bmdma_start = sil_bmdma_start,
+ .bmdma_stop = sil_bmdma_stop,
+ .qc_prep = sil_qc_prep,
.freeze = sil_freeze,
.thaw = sil_thaw,
.scr_read = sil_scr_read,
@@ -249,6 +264,84 @@ module_param(slow_down, int, 0444);
MODULE_PARM_DESC(slow_down, "Sledgehammer used to work around random problems, by limiting commands to 15 sectors (0=off, 1=on)");


+static void sil_bmdma_stop(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ void __iomem *mmio_base = ap->host->iomap[SIL_MMIO_BAR];
+ void __iomem *bmdma2 = mmio_base + sil_port[ap->port_no].bmdma2;
+
+ /* clear start/stop bit - can safely always write 0 */
+ writeb(0, bmdma2);
+
+ /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
+ ata_sff_dma_pause(ap);
+}
+
+static void sil_bmdma_setup(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc->ap;
+ void __iomem *bmdma = ap->ioaddr.bmdma_addr;
+
+ /* load PRD table addr. */
+ mb(); /* make sure PRD table writes are visible to controller */
+ writel(ap->prd_dma, bmdma + ATA_DMA_TABLE_OFS);
+
+ /* issue r/w command */
+ ap->ops->sff_exec_command(ap, &qc->tf);
+}
+
+static void sil_bmdma_start(struct ata_queued_cmd *qc)
+{
+ unsigned int rw = (qc->tf.flags & ATA_TFLAG_WRITE);
+ struct ata_port *ap = qc->ap;
+ void __iomem *mmio_base = ap->host->iomap[SIL_MMIO_BAR];
+ void __iomem *bmdma2 = mmio_base + sil_port[ap->port_no].bmdma2;
+ u8 dmactl = ATA_DMA_START;
+
+ /* set transfer direction, start host DMA transaction
+ Note: For Large Block Transfer to work, the DMA must be started
+ using the bmdma2 register. */
+ if (!rw)
+ dmactl |= ATA_DMA_WR;
+ writeb(dmactl, bmdma2);
+}
+
+/* The way God intended PCI IDE scatter/gather lists to look and behave... */
+static void sil_fill_sg(struct ata_queued_cmd *qc)
+{
+ struct scatterlist *sg;
+ struct ata_port *ap = qc->ap;
+ struct ata_prd *prd, *last_prd = NULL;
+ unsigned int si;
+
+ prd = &ap->prd[0];
+ for_each_sg(qc->sg, sg, qc->n_elem, si) {
+ /* Note h/w doesn't support 64-bit, so we unconditionally
+ * truncate dma_addr_t to u32.
+ */
+ u32 addr = (u32) sg_dma_address(sg);
+ u32 sg_len = sg_dma_len(sg);
+
+ prd->addr = cpu_to_le32(addr);
+ prd->flags_len = cpu_to_le32(sg_len);
+ VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", pi, addr, sg_len);
+
+ last_prd = prd;
+ prd++;
+ }
+
+ if (likely(last_prd))
+ last_prd->flags_len |= cpu_to_le32(ATA_PRD_EOT);
+}
+
+static void sil_qc_prep(struct ata_queued_cmd *qc)
+{
+ if (!(qc->flags & ATA_QCFLAG_DMAMAP))
+ return;
+
+ sil_fill_sg(qc);
+}
+
static unsigned char sil_get_device_cache_line(struct pci_dev *pdev)
{
u8 cache_line = 0;


2008-12-22 10:20:07

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] sata_sil: add Large Block Transfer support

Hello,

Robert Hancock wrote:
> Obviously not 2.6.28 material, but could potentially head into .29.
> I've done some testing with a DVD drive connected to this controller
> and verified that reading off an entire DVD returns correct data
> (and that the controller is actually getting requests that cross
> 64K boundaries). More testing would certainly be useful..

Ah... nice. It would be great to have this in -next for some time.

> +static void sil_bmdma_stop(struct ata_queued_cmd *qc)
> +{
> + struct ata_port *ap = qc->ap;
> + void __iomem *mmio_base = ap->host->iomap[SIL_MMIO_BAR];
> + void __iomem *bmdma2 = mmio_base + sil_port[ap->port_no].bmdma2;
> +
> + /* clear start/stop bit - can safely always write 0 */
> + writeb(0, bmdma2);

ioread/iowrite?

> + /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
> + ata_sff_dma_pause(ap);
> +}
> +
> +static void sil_bmdma_setup(struct ata_queued_cmd *qc)
> +{
> + struct ata_port *ap = qc->ap;
> + void __iomem *bmdma = ap->ioaddr.bmdma_addr;
> +
> + /* load PRD table addr. */
> + mb(); /* make sure PRD table writes are visible to controller */

I know it's not specific to this change but does mb() really make
sense here? I don't think we need any barrier here.

> + writel(ap->prd_dma, bmdma + ATA_DMA_TABLE_OFS);
> +
> + /* issue r/w command */
> + ap->ops->sff_exec_command(ap, &qc->tf);
> +}

Thanks.

--
tejun

2008-12-23 04:36:19

by Robert Hancock

[permalink] [raw]
Subject: Re: [PATCH] sata_sil: add Large Block Transfer support

Tejun Heo wrote:
> Hello,
>
> Robert Hancock wrote:
>> Obviously not 2.6.28 material, but could potentially head into .29.
>> I've done some testing with a DVD drive connected to this controller
>> and verified that reading off an entire DVD returns correct data
>> (and that the controller is actually getting requests that cross
>> 64K boundaries). More testing would certainly be useful..
>
> Ah... nice. It would be great to have this in -next for some time.
>
>> +static void sil_bmdma_stop(struct ata_queued_cmd *qc)
>> +{
>> + struct ata_port *ap = qc->ap;
>> + void __iomem *mmio_base = ap->host->iomap[SIL_MMIO_BAR];
>> + void __iomem *bmdma2 = mmio_base + sil_port[ap->port_no].bmdma2;
>> +
>> + /* clear start/stop bit - can safely always write 0 */
>> + writeb(0, bmdma2);
>
> ioread/iowrite?

We know the register's always MMIO on this controller, so it's slightly
more optimal to avoid the conditional in there.

>
>> + /* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
>> + ata_sff_dma_pause(ap);
>> +}
>> +
>> +static void sil_bmdma_setup(struct ata_queued_cmd *qc)
>> +{
>> + struct ata_port *ap = qc->ap;
>> + void __iomem *bmdma = ap->ioaddr.bmdma_addr;
>> +
>> + /* load PRD table addr. */
>> + mb(); /* make sure PRD table writes are visible to controller */
>
> I know it's not specific to this change but does mb() really make
> sense here? I don't think we need any barrier here.

Not sure. Documentation/memory-barriers.txt seems rather unfortunately
vague on whether MMIO writes are strongly ordered with respect to memory
writes. I seem to recall some debate on this a while ago, did it ever
get resolved?

>
>> + writel(ap->prd_dma, bmdma + ATA_DMA_TABLE_OFS);
>> +
>> + /* issue r/w command */
>> + ap->ops->sff_exec_command(ap, &qc->tf);
>> +}
>
> Thanks.
>

2008-12-23 04:40:45

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH] sata_sil: add Large Block Transfer support

Hello, Robert.

Robert Hancock wrote:
> Tejun Heo wrote:
>>
>> ioread/iowrite?
>
> We know the register's always MMIO on this controller, so it's slightly
> more optimal to avoid the conditional in there.

Yeah, by a small margin but I think the consensus was to use
ioread/write no matter what if it's mapped using iomap. I don't think
we guarantee iomapped address == mmio address after all.

>> I know it's not specific to this change but does mb() really make
>> sense here? I don't think we need any barrier here.
>
> Not sure. Documentation/memory-barriers.txt seems rather unfortunately
> vague on whether MMIO writes are strongly ordered with respect to memory
> writes. I seem to recall some debate on this a while ago, did it ever
> get resolved?

I remember that thread too but don't really any definite conclusion.
Eh... I hate barriers lying around without exact explanation. :-(

Thanks.

--
tejun

2008-12-23 10:10:36

by Alan

[permalink] [raw]
Subject: Re: [PATCH] sata_sil: add Large Block Transfer support

> > We know the register's always MMIO on this controller, so it's slightly
> > more optimal to avoid the conditional in there.
>
> Yeah, by a small margin but I think the consensus was to use
> ioread/write no matter what if it's mapped using iomap. I don't think
> we guarantee iomapped address == mmio address after all.

This was discussed before - using writel/readl on iomap is wrong. It may
happen to work on some platforms but shouldn't be expected to work on all.