sata_mv ncq EH fixes

A hard reset is necessary after hotplug events. Only clear the error irq bits that were set on entry.

@@ -1437,6 +1437,7 @@

		ata_ehi_hotplugged(ehi);
		ata_ehi_push_desc(ehi, edma_err_cause & EDMA_ERR_DEV_DCON ?
			"dev disconnect" : "dev connect");
+		action |= ATA_EH_HARDRESET;
	}

	if (IS_GEN_I(hpriv)) {
@@ -1465,7 +1466,7 @@
	}

	/* Clear EDMA now that SERR cleanup done */

- writelfl(0, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
+ writelfl(~edma_err_cause, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);

	if (!err_mask) {
		err_mask = AC_ERR_OTHER;

The chips can handle many transient errors internally without a software IRQ. We now mask/ignore those interrupts here. This is necessary for NCQ, later on.

@@ -170,7 +170,7 @@
 

	PCIE_IRQ_CAUSE_OFS	= 0x1900,
	PCIE_IRQ_MASK_OFS	= 0x1910,

- PCIE_UNMASK_ALL_IRQS = 0x70a, /* assorted bits */
+ PCIE_UNMASK_ALL_IRQS = 0x40a, /* assorted bits */

	HC_MAIN_IRQ_CAUSE_OFS	= 0x1d60,
	HC_MAIN_IRQ_MASK_OFS	= 0x1d64,
@@ -241,17 +241,36 @@
	EDMA_ERR_BIST_ASYNC	= (1 << 8),	/* BIST FIS or Async Notify */
	EDMA_ERR_TRANS_IRQ_7	= (1 << 8),	/* Gen IIE transprt layer irq */
	EDMA_ERR_CRQB_PAR	= (1 << 9),	/* CRQB parity error */

- EDMA_ERR_CRPB_PAR = (1 << 10), /* CRPB parity error */
- EDMA_ERR_INTRL_PAR = (1 << 11), /* internal parity error */
- EDMA_ERR_IORDY = (1 << 12), /* IORdy timeout */
- EDMA_ERR_LNK_CTRL_RX = (0xf << 13), /* link ctrl rx error */
- EDMA_ERR_LNK_CTRL_RX_2 = (1 << 15),
- EDMA_ERR_LNK_DATA_RX = (0xf << 17), /* link data rx error */
- EDMA_ERR_LNK_CTRL_TX = (0x1f << 21), /* link ctrl tx error */
- EDMA_ERR_LNK_DATA_TX = (0x1f << 26), /* link data tx error */
- EDMA_ERR_TRANS_PROTO = (1 << 31), /* transport protocol error */
- EDMA_ERR_OVERRUN_5 = (1 << 5),
- EDMA_ERR_UNDERRUN_5 = (1 << 6),
+ EDMA_ERR_CRPB_PAR = (1 << 10), /* CRPB parity error */
+ EDMA_ERR_INTRL_PAR = (1 << 11), /* internal parity error */
+ EDMA_ERR_IORDY = (1 << 12), /* IORdy timeout */
+
+ EDMA_ERR_LNK_CTRL_RX = (0xf << 13), /* link ctrl rx error */
+ EDMA_ERR_LNK_CTRL_RX_0 = (1 << 13), /* transient: CRC err */
+ EDMA_ERR_LNK_CTRL_RX_1 = (1 << 14), /* transient: FIFO err */
+ EDMA_ERR_LNK_CTRL_RX_2 = (1 << 15), /* fatal: caught SYNC */
+ EDMA_ERR_LNK_CTRL_RX_3 = (1 << 16), /* transient: FIS rx err */
+
+ EDMA_ERR_LNK_DATA_RX = (0xf << 17), /* link data rx error */
+
+ EDMA_ERR_LNK_CTRL_TX = (0x1f << 21), /* link ctrl tx error */
+ EDMA_ERR_LNK_CTRL_TX_0 = (1 << 21), /* transient: CRC err */
+ EDMA_ERR_LNK_CTRL_TX_1 = (1 << 22), /* transient: FIFO err */
+ EDMA_ERR_LNK_CTRL_TX_2 = (1 << 23), /* transient: caught SYNC */
+ EDMA_ERR_LNK_CTRL_TX_3 = (1 << 24), /* transient: caught DMAT */
+ EDMA_ERR_LNK_CTRL_TX_4 = (1 << 25), /* transient: FIS collision */
+
+ EDMA_ERR_LNK_DATA_TX = (0x1f << 26), /* link data tx error */
+
+ EDMA_ERR_TRANS_PROTO = (1 << 31), /* transport protocol error */
+ EDMA_ERR_OVERRUN_5 = (1 << 5),
+ EDMA_ERR_UNDERRUN_5 = (1 << 6),
+
+ EDMA_ERR_IRQ_TRANSIENT = EDMA_ERR_LNK_CTRL_RX_0 |

+				  EDMA_ERR_LNK_CTRL_RX_1 |
+				  EDMA_ERR_LNK_CTRL_RX_3 |
+				  EDMA_ERR_LNK_CTRL_TX,
+
	EDMA_EH_FREEZE		= EDMA_ERR_D_PAR |
				  EDMA_ERR_PRD_PAR |
				  EDMA_ERR_DEV_DCON |
@@ -1716,18 +1735,19 @@
	struct ata_host *host = dev_instance;
	unsigned int hc, handled = 0, n_hcs;
	void __iomem *mmio = host->iomap[MV_PRIMARY_BAR];

- u32 irq_stat;
+ u32 irq_stat, irq_mask;

+	spin_lock(&host->lock);
	irq_stat = readl(mmio + HC_MAIN_IRQ_CAUSE_OFS);

+ irq_mask = readl(mmio + HC_MAIN_IRQ_MASK_OFS);

	/* check the cases where we either have nothing pending or have read
	 * a bogus register value which can indicate HW removal or PCI fault
	 */

- if (!irq_stat || (0xffffffffU == irq_stat))
- return IRQ_NONE;
+ if (!(irq_stat & irq_mask) || (0xffffffffU == irq_stat))
+ goto out_unlock;

	n_hcs = mv_get_hc_count(host->ports[0]->flags);

- spin_lock(&host->lock);

	if (unlikely(irq_stat & PCI_ERR)) {
		mv_pci_error(host, mmio);
@@ -2428,8 +2448,8 @@
	writelfl(readl(port_mmio + serr_ofs), port_mmio + serr_ofs);
	writelfl(0, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);

- /* unmask all EDMA error interrupts */
- writelfl(~0, port_mmio + EDMA_ERR_IRQ_MASK_OFS);
+ /* unmask all non-transient EDMA error interrupts */
+ writelfl(~EDMA_ERR_IRQ_TRANSIENT, port_mmio + EDMA_ERR_IRQ_MASK_OFS);

	VPRINTK("EDMA cfg=0x%08x EDMA IRQ err cause/mask=0x%08x/0x%08x\n",
		readl(port_mmio + EDMA_CFG_OFS),

Use naming consistent with elsewhere in this driver.
This will keep things less confusing when we later add "hc_mmio" in this function.

@@ -834,19 +834,19 @@
 *      LOCKING:
 *      Inherited from caller.
 */
-static void mv_start_dma(void __iomem *base, struct mv_host_priv *hpriv,
+static void mv_start_dma(void __iomem *port_mmio, struct mv_host_priv *hpriv,

struct mv_port_priv *pp)

{
	if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN)) {
		/* clear EDMA event indicators, if any */

- writelfl(0, base + EDMA_ERR_IRQ_CAUSE_OFS);
+ writelfl(0, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);

- mv_set_edma_ptrs(base, hpriv, pp);
+ mv_set_edma_ptrs(port_mmio, hpriv, pp);

- writelfl(EDMA_EN, base + EDMA_CMD_OFS);

+		writelfl(EDMA_EN, port_mmio + EDMA_CMD_OFS);
		pp->pp_flags |= MV_PP_FLAG_EDMA_EN;
	}

- WARN_ON(!(EDMA_EN & readl(base + EDMA_CMD_OFS)));
+ WARN_ON(!(EDMA_EN & readl(port_mmio + EDMA_CMD_OFS)));
}

/**

Simplify and fix EDMA configuration setup to match Marvell specificiations. The chip documentation gives a specific (re)init sequence, which we now follow.

@@ -210,6 +210,7 @@

	/* SATA registers */
	SATA_STATUS_OFS		= 0x300,  /* ctrl, err regs follow status */
	SATA_ACTIVE_OFS		= 0x350,
+	SATA_FIS_IRQ_CAUSE_OFS	= 0x364,
	PHY_MODE3		= 0x310,
	PHY_MODE4		= 0x314,
	PHY_MODE2		= 0x330,

@@ -222,11 +223,11 @@

	/* Port registers */
	EDMA_CFG_OFS		= 0,

- EDMA_CFG_Q_DEPTH = 0, /* queueing disabled */
- EDMA_CFG_NCQ = (1 << 5),
- EDMA_CFG_NCQ_GO_ON_ERR = (1 << 14), /* continue on error */
- EDMA_CFG_RD_BRST_EXT = (1 << 11), /* read burst 512B */
- EDMA_CFG_WR_BUFF_LEN = (1 << 13), /* write buffer 512B */
+ EDMA_CFG_Q_DEPTH = 0x1f, /* max device queue depth */
+ EDMA_CFG_NCQ = (1 << 5), /* for R/W FPDMA queued */
+ EDMA_CFG_NCQ_GO_ON_ERR = (1 << 14), /* continue on error */
+ EDMA_CFG_RD_BRST_EXT = (1 << 11), /* read burst 512B */
+ EDMA_CFG_WR_BUFF_LEN = (1 << 13), /* write buffer 512B */

	EDMA_ERR_IRQ_CAUSE_OFS	= 0x8,
	EDMA_ERR_IRQ_MASK_OFS	= 0xc,
@@ -470,6 +471,8 @@
static void mv_reset_pci_bus(struct pci_dev *pdev, void __iomem *mmio);
static void mv_channel_reset(struct mv_host_priv *hpriv, void __iomem *mmio,
			     unsigned int port_no);

+static void mv_edma_cfg(struct ata_port *ap, struct mv_host_priv *hpriv,
+ void __iomem *port_mmio);

static struct scsi_host_template mv5_sht = {
	.module			= THIS_MODULE,

@@ -834,13 +837,33 @@
* LOCKING:
* Inherited from caller.
*/
-static void mv_start_dma(void __iomem *port_mmio, struct mv_host_priv *hpriv, +static void mv_start_dma(struct ata_port *ap, void __iomem *port_mmio,
struct mv_port_priv *pp)

{
	if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN)) {

+ struct mv_host_priv *hpriv = ap->host->private_data;
+ int hard_port = mv_hardport_from_port(ap->port_no);
+ void __iomem *hc_mmio = mv_hc_base_from_port(
+ ap->host->iomap[MV_PRIMARY_BAR], hard_port);
+ u32 hc_irq_cause, ipending;
+

		/* clear EDMA event indicators, if any */
		writelfl(0, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);

+ /* clear EDMA interrupt indicator, if any */
+ hc_irq_cause = readl(hc_mmio + HC_IRQ_CAUSE_OFS);
+ ipending = (DEV_IRQ << hard_port) |
+ (CRPB_DMA_DONE << hard_port);
+ if (hc_irq_cause & ipending) {
+ writelfl(hc_irq_cause & ~ipending,
+ hc_mmio + HC_IRQ_CAUSE_OFS);
+ }
+
+ mv_edma_cfg(ap, hpriv, port_mmio);
+
+ /* clear FIS IRQ Cause */
+ writelfl(0, port_mmio + SATA_FIS_IRQ_CAUSE_OFS);
+

		mv_set_edma_ptrs(port_mmio, hpriv, pp);

		writelfl(EDMA_EN, port_mmio + EDMA_CMD_OFS);

@@ -1025,30 +1048,22 @@
static void mv_edma_cfg(struct ata_port *ap, struct mv_host_priv *hpriv,
void __iomem *port_mmio)
{
- u32 cfg = readl(port_mmio + EDMA_CFG_OFS);
+ u32 cfg;

	/* set up non-NCQ EDMA configuration */

- cfg &= ~(1 << 9); /* disable eQue */
+ cfg = EDMA_CFG_Q_DEPTH; /* always 0x1f for *all* chips */

- if (IS_GEN_I(hpriv)) {
- cfg &= ~0x1f; /* clear queue depth */
+ if (IS_GEN_I(hpriv))

		cfg |= (1 << 8);	/* enab config burst size mask */

- }

- else if (IS_GEN_II(hpriv)) {
- cfg &= ~0x1f; /* clear queue depth */
+ else if (IS_GEN_II(hpriv))

		cfg |= EDMA_CFG_RD_BRST_EXT | EDMA_CFG_WR_BUFF_LEN;

- cfg &= ~(EDMA_CFG_NCQ | EDMA_CFG_NCQ_GO_ON_ERR); /* clear NCQ */
- }

	else if (IS_GEN_IIE(hpriv)) {
		cfg |= (1 << 23);	/* do not mask PM field in rx'd FIS */
		cfg |= (1 << 22);	/* enab 4-entry host queue cache */
-		cfg &= ~(1 << 19);	/* dis 128-entry queue (for now?) */
		cfg |= (1 << 18);	/* enab early completion */
		cfg |= (1 << 17);	/* enab cut-through (dis stor&forwrd) */
-		cfg &= ~(1 << 16);	/* dis FIS-based switching (for now) */
-		cfg &= ~(EDMA_CFG_NCQ);	/* clear NCQ */
	}

	writelfl(cfg, port_mmio + EDMA_CFG_OFS);
@@ -1370,7 +1385,6 @@
	struct ata_port *ap = qc->ap;
	void __iomem *port_mmio = mv_ap_base(ap);
	struct mv_port_priv *pp = ap->private_data;
-	struct mv_host_priv *hpriv = ap->host->private_data;
	u32 in_index;

	if (qc->tf.protocol != ATA_PROT_DMA) {
@@ -1382,7 +1396,7 @@
		return ata_qc_issue_prot(qc);
	}

- mv_start_dma(port_mmio, hpriv, pp);
+ mv_start_dma(ap, port_mmio, pp);

	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;

An extra EDMA config bit is required for NCQ operation. So set/clear it as needed, and cache current setting in port_priv. For now though, it will always be "off" (0).

@@ -331,6 +331,7 @@
 

	/* Port private flags (pp_flags) */
	MV_PP_FLAG_EDMA_EN	= (1 << 0),	/* is EDMA engine enabled? */
+	MV_PP_FLAG_NCQ_EN	= (1 << 1),	/* is EDMA set up for NCQ? */
	MV_PP_FLAG_HAD_A_RESET	= (1 << 2),	/* 1st hard reset complete? */
};

@@ -471,8 +472,9 @@
static void mv_reset_pci_bus(struct pci_dev *pdev, void __iomem *mmio); static void mv_channel_reset(struct mv_host_priv *hpriv, void __iomem *mmio,

			     unsigned int port_no);

-static void mv_edma_cfg(struct ata_port *ap, struct mv_host_priv *hpriv,
- void __iomem *port_mmio);
+static void mv_edma_cfg(struct mv_port_priv *pp, struct mv_host_priv *hpriv,
+ void __iomem *port_mmio, int want_ncq);
+static int __mv_stop_dma(struct ata_port *ap);

static struct scsi_host_template mv5_sht = {
	.module			= THIS_MODULE,

@@ -838,8 +840,15 @@
* Inherited from caller.
*/
static void mv_start_dma(struct ata_port *ap, void __iomem *port_mmio,
- struct mv_port_priv *pp)
+ struct mv_port_priv *pp, u8 protocol)
{
+ int want_ncq = (protocol == ATA_PROT_NCQ);
+
+ if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
+ int using_ncq = ((pp->pp_flags & MV_PP_FLAG_NCQ_EN) != 0);
+ if (want_ncq != using_ncq)
+ __mv_stop_dma(ap);
+ }

	if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN)) {
		struct mv_host_priv *hpriv = ap->host->private_data;
		int hard_port = mv_hardport_from_port(ap->port_no);
@@ -859,7 +868,7 @@
				 hc_mmio + HC_IRQ_CAUSE_OFS);
		}

- mv_edma_cfg(ap, hpriv, port_mmio);
+ mv_edma_cfg(pp, hpriv, port_mmio, want_ncq);

		/* clear FIS IRQ Cause */
		writelfl(0, port_mmio + SATA_FIS_IRQ_CAUSE_OFS);
@@ -1045,8 +1054,8 @@
		return -EINVAL;
}

-static void mv_edma_cfg(struct ata_port *ap, struct mv_host_priv *hpriv,
- void __iomem *port_mmio)
+static void mv_edma_cfg(struct mv_port_priv *pp, struct mv_host_priv *hpriv,
+ void __iomem *port_mmio, int want_ncq)

{
	u32 cfg;

@@ -1066,6 +1075,12 @@

		cfg |= (1 << 17);	/* enab cut-through (dis stor&forwrd) */
	}

+ if (want_ncq) {
+ cfg |= EDMA_CFG_NCQ;
+ pp->pp_flags |= MV_PP_FLAG_NCQ_EN;
+ } else
+ pp->pp_flags &= ~MV_PP_FLAG_NCQ_EN;
+

	writelfl(cfg, port_mmio + EDMA_CFG_OFS);
}

@@ -1128,7 +1143,7 @@

	spin_lock_irqsave(&ap->host->lock, flags);

- mv_edma_cfg(ap, hpriv, port_mmio);
+ mv_edma_cfg(pp, hpriv, port_mmio, 0);

	mv_set_edma_ptrs(port_mmio, hpriv, pp);

@@ -1396,7 +1411,7 @@

		return ata_qc_issue_prot(qc);
	}

- mv_start_dma(ap, port_mmio, pp);
+ mv_start_dma(ap, port_mmio, pp, qc->tf.protocol);

	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;

Simplify tag handling by using the cid/hqtag field instead of ioid, as recommended by Marvell.

@@ -1252,7 +1252,6 @@

		flags |= CRQB_FLAG_READ;
	WARN_ON(MV_MAX_Q_DEPTH <= qc->tag);
	flags |= qc->tag << CRQB_TAG_SHIFT;

- flags |= qc->tag << CRQB_IOID_SHIFT; /* 50xx appears to ignore this*/

	/* get current queue index from software */
	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;

@@ -1345,8 +1344,7 @@

	WARN_ON(MV_MAX_Q_DEPTH <= qc->tag);
	flags |= qc->tag << CRQB_TAG_SHIFT;

- flags |= qc->tag << CRQB_IOID_SHIFT; /* "I/O Id" is -really-
- what we use as our tag */
+ flags |= qc->tag << CRQB_HOSTQ_SHIFT;

	/* get current queue index from software */
	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;

@@ -1587,13 +1585,8 @@
* support for queueing. this works transparently for
* queued and non-queued modes.
*/
- else if (IS_GEN_II(hpriv))
- tag = (le16_to_cpu(pp->crpb[out_index].id)
- >> CRPB_IOID_SHIFT_6) & 0x3f;
-
- else /* IS_GEN_IIE */
- tag = (le16_to_cpu(pp->crpb[out_index].id)
- >> CRPB_IOID_SHIFT_7) & 0x3f;
+ else
+ tag = le16_to_cpu(pp->crpb[out_index].id) & 0x1f;

		qc = ata_qc_from_tag(ap, tag);

The lower 8 bits of response status are not valid for NCQ.

@@ -1587,13 +1587,12 @@
 

		qc = ata_qc_from_tag(ap, tag);

- /* lower 8 bits of status are EDMA_ERR_IRQ_CAUSE_OFS
- * bits (WARNING: might not necessarily be associated
- * with this command), which -should- be clear
- * if all is well
+ /* For non-NCQ mode, the lower 8 bits of status
+ * are from EDMA_ERR_IRQ_CAUSE_OFS,
+ * which should be zero if all went well.

		 */
		status = le16_to_cpu(pp->crpb[out_index].flags);
-		if (unlikely(status & 0xff)) {
+		if ((status & 0xff) && !(pp->pp_flags & MV_PP_FLAG_NCQ_EN)) {
			mv_err_intr(ap, qc);
			return;
		}

The GenII chips have only 8-bits for the sector_count field when performing NCQ. Add a dev_config method to restrict this when necessary, taking care not to override any other restriction already in place (likely none, but someday.. ?).

@@ -446,6 +446,7 @@

static void mv_post_int_cmd(struct ata_queued_cmd *qc);
static void mv_eh_freeze(struct ata_port *ap);
static void mv_eh_thaw(struct ata_port *ap);
+static void mv6_dev_config(struct ata_device *dev);
static int mv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);

static void mv5_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio, @@ -538,6 +539,7 @@
};

static const struct ata_port_operations mv6_ops = {
+ .dev_config = mv6_dev_config,

	.tf_load		= ata_tf_load,
	.tf_read		= ata_tf_read,
	.check_status		= ata_check_status,
@@ -1051,6 +1053,17 @@
		return -EINVAL;
}

+static void mv6_dev_config(struct ata_device *adev)
+{
+ /*
+ * We don't have hob_nsect when doing NCQ commands on Gen-II.
+ * See mv_qc_prep() for more info.
+ */
+ if (adev->flags & ATA_DFLAG_NCQ)
+ if (adev->max_sectors > ATA_MAX_SECTORS)
+ adev->max_sectors = ATA_MAX_SECTORS;
+}
+
static void mv_edma_cfg(struct mv_port_priv *pp, struct mv_host_priv *hpriv,
void __iomem *port_mmio, int want_ncq)
{

Create host-owned DMA memory pools, for use in allocating/freeing per-port command/response queues and SG tables. This gives us a way to guarantee we meet the hardware address alignment requirements, and also reduces memory that might otherwise be wasted on alignment gaps.

@@ -107,14 +107,12 @@
 

	/* CRQB needs alignment on a 1KB boundary. Size == 1KB
	 * CRPB needs alignment on a 256B boundary. Size == 256B
-	 * SG count of 176 leads to MV_PORT_PRIV_DMA_SZ == 4KB
	 * ePRD (SG) entries need alignment on a 16B boundary. Size == 16B
	 */
	MV_CRQB_Q_SZ		= (32 * MV_MAX_Q_DEPTH),
	MV_CRPB_Q_SZ		= (8 * MV_MAX_Q_DEPTH),
-	MV_MAX_SG_CT		= 176,
+	MV_MAX_SG_CT		= 256,
	MV_SG_TBL_SZ		= (16 * MV_MAX_SG_CT),

- MV_PORT_PRIV_DMA_SZ = (MV_CRQB_Q_SZ + MV_CRPB_Q_SZ + MV_SG_TBL_SZ),

	MV_PORTS_PER_HC		= 4,
	/* == (port / MV_PORTS_PER_HC) to determine HC from 0-7 port */
@@ -421,6 +419,14 @@
	u32			irq_cause_ofs;
	u32			irq_mask_ofs;
	u32			unmask_all_irqs;

+ /*
+ * These consistent DMA memory pools give us guaranteed
+ * alignment for hardware-accessed data structures,
+ * and less memory waste in accomplishing the alignment.
+ */
+ struct dma_pool *crqb_pool;
+ struct dma_pool *crpb_pool;
+ struct dma_pool *sg_tbl_pool;
};

struct mv_hw_ops {
@@ -1097,6 +1103,25 @@

	writelfl(cfg, port_mmio + EDMA_CFG_OFS);
}

+static void mv_port_free_dma_mem(struct ata_port *ap) +{
+ struct mv_host_priv *hpriv = ap->host->private_data;
+ struct mv_port_priv *pp = ap->private_data;
+
+ if (pp->crqb) {
+ dma_pool_free(hpriv->crqb_pool, pp->crqb, pp->crqb_dma);
+ pp->crqb = NULL;
+ }
+ if (pp->crpb) {
+ dma_pool_free(hpriv->crpb_pool, pp->crpb, pp->crpb_dma);
+ pp->crpb = NULL;
+ }
+ if (pp->sg_tbl) {
+ dma_pool_free(hpriv->sg_tbl_pool, pp->sg_tbl, pp->sg_tbl_dma);
+ pp->sg_tbl = NULL;
+ }
+}
+
/**
* mv_port_start - Port specific init/start routine.
* @ap: ATA channel to manipulate
@@ -1113,51 +1138,36 @@

	struct mv_host_priv *hpriv = ap->host->private_data;
	struct mv_port_priv *pp;
	void __iomem *port_mmio = mv_ap_base(ap);
-	void *mem;
-	dma_addr_t mem_dma;
	unsigned long flags;
	int rc;

	pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
	if (!pp)
		return -ENOMEM;

-
- mem = dmam_alloc_coherent(dev, MV_PORT_PRIV_DMA_SZ, &mem_dma,
- GFP_KERNEL);
- if (!mem)
- return -ENOMEM;
- memset(mem, 0, MV_PORT_PRIV_DMA_SZ);
+ ap->private_data = pp;

	rc = ata_pad_alloc(ap, dev);
	if (rc)
		return rc;

- /* First item in chunk of DMA memory:
- * 32-slot command request table (CRQB), 32 bytes each in size
- */
- pp->crqb = mem;
- pp->crqb_dma = mem_dma;
- mem += MV_CRQB_Q_SZ;
- mem_dma += MV_CRQB_Q_SZ;
-
- /* Second item:
- * 32-slot command response table (CRPB), 8 bytes each in size
- */
- pp->crpb = mem;
- pp->crpb_dma = mem_dma;
- mem += MV_CRPB_Q_SZ;
- mem_dma += MV_CRPB_Q_SZ;
+ pp->crqb = dma_pool_alloc(hpriv->crqb_pool, GFP_KERNEL, &pp->crqb_dma);
+ if (!pp->crqb)
+ return -ENOMEM;
+ memset(pp->crqb, 0, MV_CRQB_Q_SZ);

- /* Third item:
- * Table of scatter-gather descriptors (ePRD), 16 bytes each
- */
- pp->sg_tbl = mem;
- pp->sg_tbl_dma = mem_dma;
+ pp->crpb = dma_pool_alloc(hpriv->crpb_pool, GFP_KERNEL, &pp->crpb_dma);
+ if (!pp->crpb)
+ goto out_port_free_dma_mem;
+ memset(pp->crpb, 0, MV_CRPB_Q_SZ);
+
+ pp->sg_tbl = dma_pool_alloc(hpriv->sg_tbl_pool, GFP_KERNEL,
+ &pp->sg_tbl_dma);
+ if (!pp->sg_tbl)
+ goto out_port_free_dma_mem;

	spin_lock_irqsave(&ap->host->lock, flags);

	mv_edma_cfg(pp, hpriv, port_mmio, 0);
-
	mv_set_edma_ptrs(port_mmio, hpriv, pp);

	spin_unlock_irqrestore(&ap->host->lock, flags);

@@ -1166,8 +1176,11 @@
* we'll be unable to send non-data, PIO, etc due to restricted access
* to shadow regs.
*/

-	ap->private_data = pp;
	return 0;

+
+out_port_free_dma_mem:
+ mv_port_free_dma_mem(ap);
+ return -ENOMEM;
}

/**
@@ -1182,6 +1195,7 @@
static void mv_port_stop(struct ata_port *ap)

{
	mv_stop_dma(ap);

+ mv_port_free_dma_mem(ap);
}

/**
@@ -2765,6 +2779,26 @@

	       scc_s, (MV_HP_FLAG_MSI & hpriv->hp_flags) ? "MSI" : "INTx");
}

+static int mv_create_dma_pools(struct mv_host_priv *hpriv, struct device *dev) +{
+ hpriv->crqb_pool = dmam_pool_create("crqb_q", dev, MV_CRQB_Q_SZ,
+ MV_CRQB_Q_SZ, 0);
+ if (!hpriv->crqb_pool)
+ return -ENOMEM;
+
+ hpriv->crpb_pool = dmam_pool_create("crpb_q", dev, MV_CRPB_Q_SZ,
+ MV_CRPB_Q_SZ, 0);
+ if (!hpriv->crpb_pool)
+ return -ENOMEM;
+
+ hpriv->sg_tbl_pool = dmam_pool_create("sg_tbl", dev, MV_SG_TBL_SZ,
+ MV_SG_TBL_SZ, 0);
+ if (!hpriv->sg_tbl_pool)
+ return -ENOMEM;
+
+ return 0;
+}
+
/**
* mv_init_one - handle a positive probe of a Marvell host
* @pdev: PCI device found
@@ -2810,6 +2844,10 @@

	if (rc)
		return rc;

+ rc = mv_create_dma_pools(hpriv, &pdev->dev);
+ if (rc)
+ return rc;
+

	/* initialize adapter */
	rc = mv_init_host(host, board_idx);
	if (rc)

ACK patches 1-13

applied patches 1-9 to #upstream. patch #10 failed, with git-am reporting it as a corrupt patch.

..

That's weird. I can save the email from linux-ide here, and apply as a patch (after 01-09) with no issues at all.

Jeff got mail reader problems?

Here it is again, in case it got corrupted in transit to you.

* * * *

In preparation for supporting NCQ, we must allocate separate SG tables for each command tag, rather than just a single table per port as before.

Gen-I hardware cannot do NCQ, though, so we still allocate just a single table for that, but populate it in all 32 slots to avoid special-cases elsewhere in hotter paths of the code.

@@ -398,8 +398,8 @@

	dma_addr_t		crqb_dma;
	struct mv_crpb		*crpb;
	dma_addr_t		crpb_dma;

- struct mv_sg *sg_tbl;
- dma_addr_t sg_tbl_dma;
+ struct mv_sg *sg_tbl[MV_MAX_Q_DEPTH];
+ dma_addr_t sg_tbl_dma[MV_MAX_Q_DEPTH];

	unsigned int		req_idx;
	unsigned int		resp_idx;
@@ -483,6 +483,10 @@
			void __iomem *port_mmio, int want_ncq);

static int __mv_stop_dma(struct ata_port *ap);

+/* .sg_tablesize is (MV_MAX_SG_CT / 2) in the structures below
+ * because we have to allow room for worst case splitting of
+ * PRDs for 64K boundaries in mv_fill_sg().
+ */

static struct scsi_host_template mv5_sht = {
	.module			= THIS_MODULE,
	.name			= DRV_NAME,
@@ -1107,6 +1111,7 @@
{
	struct mv_host_priv *hpriv = ap->host->private_data;
	struct mv_port_priv *pp = ap->private_data;

+ int tag;

	if (pp->crqb) {
		dma_pool_free(hpriv->crqb_pool, pp->crqb, pp->crqb_dma);
@@ -1116,9 +1121,18 @@
		dma_pool_free(hpriv->crpb_pool, pp->crpb, pp->crpb_dma);
		pp->crpb = NULL;
	}

- if (pp->sg_tbl) {
- dma_pool_free(hpriv->sg_tbl_pool, pp->sg_tbl, pp->sg_tbl_dma);
- pp->sg_tbl = NULL;
+ /*
+ * For GEN_I, there's no NCQ, so we have only a single sg_tbl.
+ * For later hardware, we have one unique sg_tbl per NCQ tag.
+ */
+ for (tag = 0; tag < MV_MAX_Q_DEPTH; ++tag) {
+ if (pp->sg_tbl[tag]) {
+ if (tag == 0 || !IS_GEN_I(hpriv))
+ dma_pool_free(hpriv->sg_tbl_pool,
+ pp->sg_tbl[tag],
+ pp->sg_tbl_dma[tag]);
+ pp->sg_tbl[tag] = NULL;
+ }

	}
}

@@ -1139,7 +1153,7 @@

	struct mv_port_priv *pp;
	void __iomem *port_mmio = mv_ap_base(ap);
	unsigned long flags;

- int rc;
+ int tag, rc;

	pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
	if (!pp)
@@ -1160,10 +1174,21 @@
		goto out_port_free_dma_mem;
	memset(pp->crpb, 0, MV_CRPB_Q_SZ);

- pp->sg_tbl = dma_pool_alloc(hpriv->sg_tbl_pool, GFP_KERNEL,
- &pp->sg_tbl_dma);
- if (!pp->sg_tbl)
- goto out_port_free_dma_mem;
+ /*
+ * For GEN_I, there's no NCQ, so we only allocate a single sg_tbl.
+ * For later hardware, we need one unique sg_tbl per NCQ tag.
+ */
+ for (tag = 0; tag < MV_MAX_Q_DEPTH; ++tag) {
+ if (tag == 0 || !IS_GEN_I(hpriv)) {
+ pp->sg_tbl[tag] = dma_pool_alloc(hpriv->sg_tbl_pool,
+ GFP_KERNEL, &pp->sg_tbl_dma[tag]);
+ if (!pp->sg_tbl[tag])
+ goto out_port_free_dma_mem;
+ } else {
+ pp->sg_tbl[tag] = pp->sg_tbl[0];
+ pp->sg_tbl_dma[tag] = pp->sg_tbl_dma[0];
+ }
+ }

	spin_lock_irqsave(&ap->host->lock, flags);

@@ -1213,7 +1238,7 @@

	struct mv_sg *mv_sg, *last_sg = NULL;
	unsigned int si;

- mv_sg = pp->sg_tbl;

+	mv_sg = pp->sg_tbl[qc->tag];
	for_each_sg(qc->sg, sg, qc->n_elem, si) {
		dma_addr_t addr = sg_dma_address(sg);
		u32 sg_len = sg_dma_len(sg);
@@ -1283,9 +1308,9 @@
	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;

	pp->crqb[in_index].sg_addr =
-		cpu_to_le32(pp->sg_tbl_dma & 0xffffffff);
+		cpu_to_le32(pp->sg_tbl_dma[qc->tag] & 0xffffffff);
	pp->crqb[in_index].sg_addr_hi =
-		cpu_to_le32((pp->sg_tbl_dma >> 16) >> 16);
+		cpu_to_le32((pp->sg_tbl_dma[qc->tag] >> 16) >> 16);
	pp->crqb[in_index].ctrl_flags = cpu_to_le16(flags);

	cw = &pp->crqb[in_index].ata_cmd[0];
@@ -1376,8 +1401,8 @@
	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;

	crqb = (struct mv_crqb_iie *) &pp->crqb[in_index];
-	crqb->addr = cpu_to_le32(pp->sg_tbl_dma & 0xffffffff);
-	crqb->addr_hi = cpu_to_le32((pp->sg_tbl_dma >> 16) >> 16);
+	crqb->addr = cpu_to_le32(pp->sg_tbl_dma[qc->tag] & 0xffffffff);
+	crqb->addr_hi = cpu_to_le32((pp->sg_tbl_dma[qc->tag] >> 16) >> 16);
	crqb->flags = cpu_to_le32(flags);

	tf = &qc->tf;

Nope, not corrupted in transit or on this side. It falls into a familiar pattern:

* git-am(1) fails
* patch(1) succeeds
* when applying patch, patch(1) drops a .orig turd

So while patch(1) succeeds because patch(1) is highly forgiving and git-am(1) is more strict, something was definitely strange on that incoming email. patch(1) lets you know by giving you a .orig file, something not normally created if the patch operation was 100% sound.

ISTR Linus or Junio explaining why git-am(1) was more strict and why it was a good thing... As I did in this case, I usually just run patch(1), look carefully at the result using 'git diff HEAD', and then commit/resolve the changes.

Jeff

..

Okay, I figured it out. There's a 1-line offset difference between what is used in patch 10, and where stuff actually is/was in sata_mv.c .

I probably changed an unrelated line and rediff'd an earlier patch without rediff'ing everything that followed.

Because patch is smart and can handle it.

But since git-am plays dumb (both good and bad), I'll be more careful about rediffing the entire series next time round.

Meanwhile, no further action required here.

Cheers

ACK :)

And thanks for rounding out the NCQ work. sata_mv has needed love and attention for a while (well, really, its entire life).

Jeff

..

Well, it's going to be getting plenty of TLC over the next few months.

In the short term, my plan is to submit further small patches to fix the IRQ and error-handling in sata_mv, as bug fixes for 2.6.25.

Note that hot plug/unplug will begin to work correctly once the IRQ/EH code gets fixed (it sort of works already, but sometimes kills the machine).

There are also some errata that need to be addressed in the 2.6.25 timeframe.

In particular, there's an NCQ EH errata for the 60x1 chips, and a tricky issue about HOB access not working correctly on most versions of the chips.

Bigger stuff that I'm deferring for 2.6.26:

-- Port multiplier support (though this does look rather simple..) -- power management support
-- ATAPI
-- IRQ Coalescing
-- Target Mode support (interfaces yet to be defined)
-- TCQ support: would be good in general for libata on smart hosts,
but I'm not sure of the impact on libata EH processing.

How's that all sound to you guys?

Cheers

I'm interested to see this :)

Most "modern" SATA has some form of this, but I've yet to see any benefit. I've dealt with interrupt (packet) rates of well over 500k/sec in network land, and IMO the overhead in storage, even with tiny operations, is really small in comparison.

So, I'm not sure its worth the latency penalty... at least as turned on by default.

I would assume this would be along the lines of the SCSI target mode stuff.

Agreed, it would be nice to support host queueing controllers.

However, specifically for TCQ, it was rather poorly conceived. For most controllers (mv, broadcom/svw, others) an error will stop the DMA engine, and you perform recovery in software. All well and good, but figuring out all the states possible during recovery is non-trivial (I looked into it years ago). Its just messy.

Jeff

..

..

Heh.. me too, actually. :)

..

I agree. It should default to off, and perhaps have some /sys/ attributes to enable/tune it. Or something like that.

The theoretical value is apparently for situations like writing to RAID1. The write isn't really "complete" until all drives ACK it,
so with IRQ coalescing it may behave more like NAPI than one I/O per IRQ. And NAPI is apparently a win under heavy load for network, so.. we'll see.

The vendor wants it in the driver, and I think it will be good to have it there so we can play with and tune it -- to find out for real whether it has worthwhile value or not. But yes, the default should be "off", I think.

..

Ah, now there's a starting point. Thanks.

..

So is NCQ EH, but we manage it. I wonder how similar (or not) the two are?

I've done host-based TCQ several times now, and EH can be as simple as:

"when something goes wrong, just reset everything, and then re-issue the commands one at a time, doing per-command EH normally. Then resume TCQ."

That's dumb, but works extremely reliably.

Cheers

Eeeek.. :-)

I'm skeptical about the benefit of IRQ coalescing on storage controllers. Coalescing improves performance when there are many small requests to complete and if you put a lot of small non-consecutive requests to a disk, it gets really really really slow and IRQ coalescing just doesn't matter at all. The only way to achieve high number of completions is to issue small commands to consecutive addresses which is just silly. In storage, high volume transfer is achieved through request coalescing not completion coalescing and this is true for even SDDs.

It would be great if we can make a cheap SATA analyzer out of it.

How many devices with working TCQ support are out there? Early raptors?
If the controller handles all that releasing and state transitions, I
think is shouldn't be too difficult. You'll probably need to add TCQ support to ata_eh_autopsy or roll your own autopsy function but that should be about it for EH. Heck, just freezing on any sign of problem and let EH reset the drive and retry should work for most of the cases although things like media errors won't be reported properly.

Oh, you were thinking the same thing. :-) It can be made more reliable by simply falling back to non-TCQ if error repeats itself. e.g. Media error -> TCQ freeze -> reset -> media error -> TCQ freeze -> reset and turn off TCQ -> media error gets handled and reported. It isn't pretty but should work for initial implementation.

Thanks.

tejun

To add a bit, I was thinking about writes regarding SDDs. Completion coalescing makes more sense for reads from SDDs if log based filesystem is used on it because then read operations are scattered all over the disk and the device doesn't have to seek. Limited queue size compared to network interfaces will make completion coalescing less effective but yeah, I think it will be worth playing with it on SDD + lfs.

Thanks.

tejun

..

One cool thing with the Marvell cores, is that they actually implement "transaction based" IRQ coalescing, whereby a number of related I/O commands (say, all the RAID5 member commands generated by a single R/W request) can be tagged together, generating an interrupt only when they all complete (or after a timeout if something goes wrong).

We don't have anything resembling an appropriate abstraction for that yet, so I doubt that we could really take advantage of it.

I think one thought in general for IRQ coalescing, is that with largish
storage arrays it may cut down the number of IRQ entry/exit cycles considerably under heavy load, and perhaps slightly improve L1 cache occupancy of the IRQ handler paths as well. Dunno, but it could be fun
to wire it in there so we can experiment and (in)validate such theories.

..

Yeah. It'll have software latency added in, but the chip really looks like it can do the analyzer job just fine. I wonder if any of the existing analyzer products already use these chips..

..

..

Raptors, everything ever made by Hitachi, some Maxtor, some Seagate (I think), and even the odd Western Digital drive. And in the PATA space, there were WD and IBM/Hitachi drives with it. The PDC ADMA and QStor chips were designed for TCQ (and NCQ for QStor).

Still though, I don't think it's a huge priority, since all modern drives now implement NCQ when it matters.

Cheers

Promise SATA controllers have this feature too,
though sata_promise doesn't make use of it.

In preparation for supporting NCQ, we must allocate separate SG tables for each command tag, rather than just a single table per port as before.

Gen-I hardware cannot do NCQ, though, so we still allocate just a single table for that, but populate it in all 32 slots to avoid special-cases elsewhere in hotter paths of the code.

@@ -398,8 +398,8 @@

	dma_addr_t		crqb_dma;
	struct mv_crpb		*crpb;
	dma_addr_t		crpb_dma;

- struct mv_sg *sg_tbl;
- dma_addr_t sg_tbl_dma;
+ struct mv_sg *sg_tbl[MV_MAX_Q_DEPTH];
+ dma_addr_t sg_tbl_dma[MV_MAX_Q_DEPTH];

	unsigned int		req_idx;
	unsigned int		resp_idx;
@@ -483,6 +483,10 @@
			void __iomem *port_mmio, int want_ncq);

static int __mv_stop_dma(struct ata_port *ap);

+/* .sg_tablesize is (MV_MAX_SG_CT / 2) in the structures below
+ * because we have to allow room for worst case splitting of
+ * PRDs for 64K boundaries in mv_fill_sg().
+ */

static struct scsi_host_template mv5_sht = {
	.module			= THIS_MODULE,
	.name			= DRV_NAME,
@@ -1107,6 +1111,7 @@
{
	struct mv_host_priv *hpriv = ap->host->private_data;
	struct mv_port_priv *pp = ap->private_data;

+ int tag;

	if (pp->crqb) {
		dma_pool_free(hpriv->crqb_pool, pp->crqb, pp->crqb_dma);
@@ -1116,9 +1121,18 @@
		dma_pool_free(hpriv->crpb_pool, pp->crpb, pp->crpb_dma);
		pp->crpb = NULL;
	}

- if (pp->sg_tbl) {
- dma_pool_free(hpriv->sg_tbl_pool, pp->sg_tbl, pp->sg_tbl_dma);
- pp->sg_tbl = NULL;
+ /*
+ * For GEN_I, there's no NCQ, so we have only a single sg_tbl.
+ * For later hardware, we have one unique sg_tbl per NCQ tag.
+ */
+ for (tag = 0; tag < MV_MAX_Q_DEPTH; ++tag) {
+ if (pp->sg_tbl[tag]) {
+ if (tag == 0 || !IS_GEN_I(hpriv))
+ dma_pool_free(hpriv->sg_tbl_pool,
+ pp->sg_tbl[tag],
+ pp->sg_tbl_dma[tag]);
+ pp->sg_tbl[tag] = NULL;
+ }

	}
}

@@ -1139,7 +1153,7 @@

	struct mv_port_priv *pp;
	void __iomem *port_mmio = mv_ap_base(ap);
	unsigned long flags;

- int rc;
+ int tag, rc;

	pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
	if (!pp)
@@ -1160,10 +1174,21 @@
		goto out_port_free_dma_mem;
	memset(pp->crpb, 0, MV_CRPB_Q_SZ);

- pp->sg_tbl = dma_pool_alloc(hpriv->sg_tbl_pool, GFP_KERNEL,
- &pp->sg_tbl_dma);
- if (!pp->sg_tbl)
- goto out_port_free_dma_mem;
+ /*
+ * For GEN_I, there's no NCQ, so we only allocate a single sg_tbl.
+ * For later hardware, we need one unique sg_tbl per NCQ tag.
+ */
+ for (tag = 0; tag < MV_MAX_Q_DEPTH; ++tag) {
+ if (tag == 0 || !IS_GEN_I(hpriv)) {
+ pp->sg_tbl[tag] = dma_pool_alloc(hpriv->sg_tbl_pool,
+ GFP_KERNEL, &pp->sg_tbl_dma[tag]);
+ if (!pp->sg_tbl[tag])
+ goto out_port_free_dma_mem;
+ } else {
+ pp->sg_tbl[tag] = pp->sg_tbl[0];
+ pp->sg_tbl_dma[tag] = pp->sg_tbl_dma[0];
+ }
+ }

	spin_lock_irqsave(&ap->host->lock, flags);

@@ -1213,7 +1238,7 @@

	struct mv_sg *mv_sg, *last_sg = NULL;
	unsigned int si;

- mv_sg = pp->sg_tbl;

+	mv_sg = pp->sg_tbl[qc->tag];
	for_each_sg(qc->sg, sg, qc->n_elem, si) {
		dma_addr_t addr = sg_dma_address(sg);
		u32 sg_len = sg_dma_len(sg);
@@ -1283,9 +1308,9 @@
	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;

	pp->crqb[in_index].sg_addr =
-		cpu_to_le32(pp->sg_tbl_dma & 0xffffffff);
+		cpu_to_le32(pp->sg_tbl_dma[qc->tag] & 0xffffffff);
	pp->crqb[in_index].sg_addr_hi =
-		cpu_to_le32((pp->sg_tbl_dma >> 16) >> 16);
+		cpu_to_le32((pp->sg_tbl_dma[qc->tag] >> 16) >> 16);
	pp->crqb[in_index].ctrl_flags = cpu_to_le16(flags);

	cw = &pp->crqb[in_index].ata_cmd[0];
@@ -1376,8 +1401,8 @@
	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;

	crqb = (struct mv_crqb_iie *) &pp->crqb[in_index];
-	crqb->addr = cpu_to_le32(pp->sg_tbl_dma & 0xffffffff);
-	crqb->addr_hi = cpu_to_le32((pp->sg_tbl_dma >> 16) >> 16);
+	crqb->addr = cpu_to_le32(pp->sg_tbl_dma[qc->tag] & 0xffffffff);
+	crqb->addr_hi = cpu_to_le32((pp->sg_tbl_dma[qc->tag] >> 16) >> 16);
	crqb->flags = cpu_to_le32(flags);

	tf = &qc->tf;

applied 10-13

Final changes to actually turn on NCQ in the driver for GEN_II/IIE hardware.

@@ -510,7 +510,8 @@

	.name			= DRV_NAME,
	.ioctl			= ata_scsi_ioctl,
	.queuecommand		= ata_scsi_queuecmd,
-	.can_queue		= ATA_DEF_QUEUE,
+	.change_queue_depth	= ata_scsi_change_queue_depth,
+	.can_queue		= MV_MAX_Q_DEPTH - 1,
	.this_id		= ATA_SHT_THIS_ID,
	.sg_tablesize		= MV_MAX_SG_CT / 2,
	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
@@ -572,6 +573,7 @@
	.post_internal_cmd	= mv_post_int_cmd,
	.freeze			= mv_eh_freeze,
	.thaw			= mv_eh_thaw,

+ .qc_defer = ata_std_qc_defer,

	.scr_read		= mv_scr_read,
	.scr_write		= mv_scr_write,
@@ -600,6 +602,7 @@
	.post_internal_cmd	= mv_post_int_cmd,
	.freeze			= mv_eh_freeze,
	.thaw			= mv_eh_thaw,

+ .qc_defer = ata_std_qc_defer,

	.scr_read		= mv_scr_read,
	.scr_write		= mv_scr_write,
@@ -628,26 +631,29 @@
		.port_ops	= &mv5_ops,
	},
	{  /* chip_604x */
-		.flags		= MV_COMMON_FLAGS | MV_6XXX_FLAGS,
+		.flags		= MV_COMMON_FLAGS | MV_6XXX_FLAGS |
+				  ATA_FLAG_NCQ,
		.pio_mask	= 0x1f,	/* pio0-4 */
		.udma_mask	= ATA_UDMA6,
		.port_ops	= &mv6_ops,
	},
	{  /* chip_608x */
		.flags		= MV_COMMON_FLAGS | MV_6XXX_FLAGS |
-				  MV_FLAG_DUAL_HC,
+				  ATA_FLAG_NCQ | MV_FLAG_DUAL_HC,
		.pio_mask	= 0x1f,	/* pio0-4 */
		.udma_mask	= ATA_UDMA6,
		.port_ops	= &mv6_ops,
	},
	{  /* chip_6042 */
-		.flags		= MV_COMMON_FLAGS | MV_6XXX_FLAGS,
+		.flags		= MV_COMMON_FLAGS | MV_6XXX_FLAGS |
+				  ATA_FLAG_NCQ,
		.pio_mask	= 0x1f,	/* pio0-4 */
		.udma_mask	= ATA_UDMA6,
		.port_ops	= &mv_iie_ops,
	},
	{  /* chip_7042 */
-		.flags		= MV_COMMON_FLAGS | MV_6XXX_FLAGS,
+		.flags		= MV_COMMON_FLAGS | MV_6XXX_FLAGS |
+				  ATA_FLAG_NCQ,
		.pio_mask	= 0x1f,	/* pio0-4 */
		.udma_mask	= ATA_UDMA6,
		.port_ops	= &mv_iie_ops,
@@ -1261,7 +1267,8 @@
	u16 flags = 0;
	unsigned in_index;

- if (qc->tf.protocol != ATA_PROT_DMA)
+ if ((qc->tf.protocol != ATA_PROT_DMA) &&
+ (qc->tf.protocol != ATA_PROT_NCQ))

		return;

	/* Fill in command request block
@@ -1297,13 +1304,11 @@
	case ATA_CMD_WRITE_FUA_EXT:
		mv_crqb_pack_cmd(cw++, tf->hob_nsect, ATA_REG_NSECT, 0);
		break;
-#ifdef LIBATA_NCQ		/* FIXME: remove this line when NCQ added */
	case ATA_CMD_FPDMA_READ:
	case ATA_CMD_FPDMA_WRITE:
		mv_crqb_pack_cmd(cw++, tf->hob_feature, ATA_REG_FEATURE, 0);
		mv_crqb_pack_cmd(cw++, tf->feature, ATA_REG_FEATURE, 0);
		break;
-#endif				/* FIXME: remove this line when NCQ added */
	default:
		/* The only other commands EDMA supports in non-queued and
		 * non-NCQ mode are: [RW] STREAM DMA and W DMA FUA EXT, none
@@ -1352,7 +1357,8 @@
	unsigned in_index;
	u32 flags = 0;

- if (qc->tf.protocol != ATA_PROT_DMA)
+ if ((qc->tf.protocol != ATA_PROT_DMA) &&
+ (qc->tf.protocol != ATA_PROT_NCQ))

		return;

	/* Fill in Gen IIE command request block
@@ -1418,7 +1424,8 @@
	struct mv_port_priv *pp = ap->private_data;
	u32 in_index;

- if (qc->tf.protocol != ATA_PROT_DMA) {
+ if ((qc->tf.protocol != ATA_PROT_DMA) &&
+ (qc->tf.protocol != ATA_PROT_NCQ)) {
/* We're about to send a non-EDMA capable command to the
* port. Turn off EDMA so there won't be problems accessing
* shadow block, etc registers.
@@ -1429,12 +1436,6 @@

	mv_start_dma(ap, port_mmio, pp, qc->tf.protocol);

- in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;
-
- /* until we do queuing, the queue should be empty at this point */
- WARN_ON(in_index != ((readl(port_mmio + EDMA_REQ_Q_OUT_PTR_OFS)
- >> EDMA_REQ_Q_PTR_SHIFT) & MV_MAX_Q_DEPTH_MASK));
-

	pp->req_idx++;

	in_index = (pp->req_idx & MV_MAX_Q_DEPTH_MASK) << EDMA_REQ_Q_PTR_SHIFT;

This driver currently has no need for the .post_internal_cmd op.
So get rid of it, to save unnecessary transitions between EDMA and non-EDMA modes.

@@ -452,7 +452,6 @@

static void mv_qc_prep_iie(struct ata_queued_cmd *qc); static unsigned int mv_qc_issue(struct ata_queued_cmd *qc); static void mv_error_handler(struct ata_port *ap); -static void mv_post_int_cmd(struct ata_queued_cmd *qc); static void mv_eh_freeze(struct ata_port *ap); static void mv_eh_thaw(struct ata_port *ap); static void mv6_dev_config(struct ata_device *dev); @@ -541,7 +540,6 @@

	.irq_on			= ata_irq_on,

	.error_handler		= mv_error_handler,
-	.post_internal_cmd	= mv_post_int_cmd,
	.freeze			= mv_eh_freeze,
	.thaw			= mv_eh_thaw,

@@ -570,7 +568,6 @@

	.irq_on			= ata_irq_on,

	.error_handler		= mv_error_handler,
-	.post_internal_cmd	= mv_post_int_cmd,
	.freeze			= mv_eh_freeze,
	.thaw			= mv_eh_thaw,
	.qc_defer		= ata_std_qc_defer,
@@ -599,7 +596,6 @@
	.irq_on			= ata_irq_on,

	.error_handler		= mv_error_handler,
-	.post_internal_cmd	= mv_post_int_cmd,
	.freeze			= mv_eh_freeze,
	.thaw			= mv_eh_thaw,
	.qc_defer		= ata_std_qc_defer,
@@ -2424,11 +2420,6 @@
		  mv_hardreset, mv_postreset);
}

-static void mv_post_int_cmd(struct ata_queued_cmd *qc) -{
- mv_stop_dma(qc->ap);
-}
-
static void mv_eh_freeze(struct ata_port *ap)

{
	void __iomem *mmio = ap->host->iomap[MV_PRIMARY_BAR];

Remove some obsolete comments, and bump up the driver version number.

@@ -29,7 +29,13 @@
  I distinctly remember a couple workarounds (one related to PCI-X)
  are still needed.
 
-  4) Add NCQ support (easy to intermediate, once new-EH support appears)
+  2) Improve/fix IRQ and error handling sequences.
+
+  3) ATAPI support (Marvell claims the 60xx/70xx chips can do it).
+
+  4) Think about TCQ support here, and for libata in general
+  with controllers that suppport it via host-queuing hardware
+  (a software-only implementation could be a nightmare).
 
  5) Investigate problems with PCI Message Signalled Interrupts (MSI).
 

@@ -53,8 +59,6 @@
  Target mode, for those without docs, is the ability to directly
  connect two SATA controllers.
 
-  13) Verify that 7042 is fully supported.  I only have a 6042.
-

*/

@@ -73,7 +77,7 @@
#include <linux/libata.h>

#define DRV_NAME "sata_mv"
-#define DRV_VERSION "1.01"
+#define DRV_VERSION "1.20"

enum {
	/* BAR's are enumerated in terms of pci_resource_start() terms */