sata_mv NCQ support

Here is a set of 12 patches that gradually add working NCQ support to sata_mv.

The driver still has issues afterwards, but nothing new that wasn't broken already. I am working on additional patches to correct the interrupt and error handling problems that still exist in the driver, as well as PMP and ATAPI support (later).

Patches are against Jeff's #upstream git from this morning.

Cheers

Mark

sata_mv 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;

sata_mv Mask transient IRQs.

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),

..

Sure thing. But the patch above does NOT remove any.

Cheers

sata_mv Clear queue indexes on chip restart.

Force in/out queue pointer indexes back to zero on any (re)init. This ensures things are in a consistent state after any chip/dev errors.

@@ -792,7 +792,7 @@

	/*
	 * initialize request queue
	 */

- index = (pp->req_idx & MV_MAX_Q_DEPTH_MASK) << EDMA_REQ_Q_PTR_SHIFT;
+ index = pp->req_idx = 0;

	WARN_ON(pp->crqb_dma & 0x3ff);
	writel((pp->crqb_dma >> 16) >> 16, port_mmio + EDMA_REQ_Q_BASE_HI_OFS);
@@ -808,7 +808,7 @@
	/*
	 * initialize response queue
	 */

- index = (pp->resp_idx & MV_MAX_Q_DEPTH_MASK) << EDMA_RSP_Q_PTR_SHIFT;
+ index = pp->resp_idx = 0;

	WARN_ON(pp->crpb_dma & 0xff);
	writel((pp->crpb_dma >> 16) >> 16, port_mmio + EDMA_RSP_Q_BASE_HI_OFS);

sata_mv rename base to port_mmio.

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)));
}

/**

sata_mv Fix EDMA configuration.

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;

..

Sure it does. It clears the *whole* register by default, and then turns on only the bits we want.

Cheers

sata_mv Add want_ncq parameter for EDMA configuration.

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 = ata_is_ncq(protocol);
+
+ 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;

ACK, though from a personal taste perspective, I would probably have combined this with other NCQ changes further down the line

Not a complaint... just an observation

sata_mv Use hqtag instead of ioid.

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

@@ -132,15 +132,12 @@
 

	CRQB_FLAG_READ		= (1 << 0),
	CRQB_TAG_SHIFT		= 1,
-	CRQB_IOID_SHIFT		= 6,	/* CRQB Gen-II/IIE IO Id shift */
	CRQB_HOSTQ_SHIFT	= 17,	/* CRQB Gen-II/IIE HostQueTag shift */
	CRQB_CMD_ADDR_SHIFT	= 8,
	CRQB_CMD_CS		= (0x2 << 11),
	CRQB_CMD_LAST		= (1 << 15),

	CRPB_FLAG_STATUS_SHIFT	= 8,

- CRPB_IOID_SHIFT_6 = 5, /* CRPB Gen-II IO Id shift */
- CRPB_IOID_SHIFT_7 = 7, /* CRPB Gen-IIE IO Id shift */

	EPRD_FLAG_END_OF_TBL	= (1 << 31),

@@ -1252,7 +1249,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 +1341,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 +1582,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);

..

Okay. Do you want to just nuke the first fragment of that patch and keep the rest, then?

Or resubmit ?

sata_mv Ignore response status LSB on NCQ.

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;
		}

sata_mv Restrict max_sectors to 8-bits on GenII NCQ.

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)
{

sata_mv Use DMA memory pools for hardware memory tables.

Create module-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 */

@@ -701,6 +699,33 @@
*/
static int msi; /* Use PCI msi; either zero (off, default) or non-zero */

+/*
+ * These consistent DMA memory pools are owned by this module,
+ * and shared among all device instances. This gives us guaranteed
+ * alignment for hardware-accessed data structures, and low memory
+ * waste in accomplishing the alignment.
+ */
+static struct dma_pool *mv_crpb_pool;
+static struct dma_pool *mv_crqb_pool;
+static struct dma_pool *mv_sg_tbl_pool;
+
+static void mv_free_pool_items(struct ata_port *ap)
+{
+ struct mv_port_priv *pp = ap->private_data;
+
+ if (pp->sg_tbl) {
+ dma_pool_free(mv_sg_tbl_pool, pp->sg_tbl, pp->sg_tbl_dma);
+ pp->sg_tbl = NULL;
+ }
+ if (pp->crqb) {
+ dma_pool_free(mv_crqb_pool, pp->crqb, pp->crqb_dma);
+ pp->crpb = NULL;
+ }
+ if (pp->crqb) {
+ dma_pool_free(mv_crqb_pool, pp->crqb, pp->crqb_dma);
+ pp->crqb = NULL;
+ }
+}

/* move to PCI layer or libata core? */
static int pci_go_64(struct pci_dev *pdev)
@@ -1110,8 +1135,6 @@

	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;

@@ -1119,37 +1142,21 @@

	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);
-

	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;
-
- /* Third item:
- * Table of scatter-gather descriptors (ePRD), 16 bytes each
- */
- pp->sg_tbl = mem;
- pp->sg_tbl_dma = mem_dma;
+ pp->crqb = dma_pool_alloc(mv_crqb_pool, GFP_KERNEL, &pp->crqb_dma);
+ if (!pp->crqb)
+ goto out_alloc_failed;
+
+ pp->crpb = dma_pool_alloc(mv_crpb_pool, GFP_KERNEL, &pp->crpb_dma);
+ if (!pp->crpb)
+ goto out_alloc_failed;
+
+ pp->sg_tbl = dma_pool_alloc(mv_sg_tbl_pool, GFP_KERNEL, &pp->sg_tbl_dma);
+ if (!pp->sg_tbl)
+ goto out_alloc_failed;

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

@@ -1165,6 +1172,10 @@

	 */
	ap->private_data = pp;
	return 0;

+
+out_alloc_failed:
+ mv_free_pool_items(ap);
+ return -ENOMEM;
}

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

{
	mv_stop_dma(ap);

+ mv_free_pool_items(ap);
}

/**
@@ -2825,14 +2837,39 @@

				 IS_GEN_I(hpriv) ? &mv5_sht : &mv6_sht);
}

+static void mv_destroy_pools(void)
+{
+ if (mv_sg_tbl_pool)
+ dma_pool_destroy(mv_sg_tbl_pool);
+ if (mv_crpb_pool)
+ dma_pool_destroy(mv_crpb_pool);
+ if (mv_crqb_pool)
+ dma_pool_destroy(mv_crqb_pool);
+}
+
static int __init mv_init(void)
{
+ /* Ideally, a device (second parameter) would "own" these pools.
+ * But for maximum memory efficiency, we really need one global
+ * set of each, shared among all like devices. As below.
+ */
+ mv_crqb_pool = dma_pool_create("mv_crqb_q", NULL, MV_CRQB_Q_SZ,
+ MV_CRQB_Q_SZ, 0);
+ mv_crpb_pool = dma_pool_create("mv_crpb_q", NULL, MV_CRPB_Q_SZ,
+ MV_CRPB_Q_SZ, 0);
+ mv_sg_tbl_pool = dma_pool_create("mv_sg_tbl", NULL, MV_SG_TBL_SZ,
+ MV_SG_TBL_SZ, 0);
+ if (!mv_crqb_pool || !mv_crpb_pool || !mv_sg_tbl_pool) {
+ mv_destroy_pools();
+ return -ENOMEM;
+ }

	return pci_register_driver(&mv_pci_driver);
}

static void __exit mv_exit(void)

{
	pci_unregister_driver(&mv_pci_driver);

+ mv_destroy_pools();
}

MODULE_AUTHOR("Brett Russ");

..

..

The waste amounts to perhaps half a page, per port. The chips have either 4 or 8 ports.

But whatever. I think libata should actually provide the pools anyway, since most drivers need hardware aligned DMA memory of very similar requirements.

The pre-existing scheme in the driver is insufficient, as it does not guarantee correct alignment. Right now it does appear to work by accident, but there's no alignment guarantee in the interface unless pools are used.

When we allocate 32 SG tables *per port*, this becomes much more of a bother, so pools are needed there to avoid waste.

Having the pools per-port rather than per-chip multiplies any waste by factors of 4 or 8. I prefer not wasting memory, myself.

Cheers

..

Actually, it currently does one set of pools for the driver.

An only slightly worse solution might be to do one set of pools per chip, as much of the time there's likely only a single chip anyway.

And per-chip means we'll have a struct device to pass to the pools interface.

So I'll change the driver to do it that way.

I can do this as a supplemental patch immediately, which will minimize the need for retest/rebuild of other changes.

Or re-issue patches 10,11 as a single new patch, and possibly reissue patches 12,13,14,15 but only if they no longer apply cleanly.

Just say exactly what you require here.
And keep in mind that any change I make incurs a 2-day wait while the Marvell folks vet it again (not my choice).

Thanks

(about to go to sleep, so the rest must wait)

Bandwidth and mailing list archives are cheap, emailing is scriptable and its less confusion to simply discuss, revise, then repost the whole thing. Keeping track of the latest revision of each of 15 patches is an exercise in complexity management :)

When its just one or two patches, replying with an updated

	"[PATCH v2] ..."

generally works, but that doesn't scale well.

(and, more practically, I made a guess that the patch series would need some revisions somewhere, assumed it would be reposted post-discussion, and deleted it locally after review)

Jeff

..

..

Okay, will do in a couple of days.

-ml

Jeff Garzik wrote:
..

..

Note that there's also "no worry about allocations failing inside of interrupts or similar nastiness" with the patch posted, either.

Cheers

sata_mv Introduce per-tag SG tables.

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.

@@ -395,8 +395,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;
@@ -472,6 +472,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,

@@ -711,12 +715,23 @@

static void mv_free_pool_items(struct ata_port *ap) {

+	struct mv_host_priv *hpriv = ap->host->private_data;
	struct mv_port_priv *pp = ap->private_data;

+ int tag;

- if (pp->sg_tbl) {
- dma_pool_free(mv_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(mv_sg_tbl_pool, pp->sg_tbl[tag],
+ pp->sg_tbl_dma[tag]);
+ pp->sg_tbl[tag] = NULL;
+ }

	}
+
	if (pp->crqb) {
		dma_pool_free(mv_crqb_pool, pp->crqb, pp->crqb_dma);
		pp->crpb = NULL;
@@ -1136,7 +1151,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)
@@ -1153,10 +1168,21 @@
	pp->crpb = dma_pool_alloc(mv_crpb_pool, GFP_KERNEL, &pp->crpb_dma);
	if (!pp->crpb)
		goto out_alloc_failed;

-
- pp->sg_tbl = dma_pool_alloc(mv_sg_tbl_pool, GFP_KERNEL, &pp->sg_tbl_dma);
- if (!pp->sg_tbl)
- goto out_alloc_failed;
+ /*
+ * 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(mv_sg_tbl_pool,
+ GFP_KERNEL, &pp->sg_tbl_dma[tag]);
+ if (!pp->sg_tbl[tag])
+ goto out_alloc_failed;
+ } 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);

@@ -1209,7 +1235,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);
@@ -1279,9 +1305,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];
@@ -1372,8 +1398,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;

ACK (though it obviously must be revised per comments on patch #10)

sata_mv Enable NCQ operation.

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

@@ -499,7 +499,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,
@@ -617,26 +618,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,
+				  MV_FLAG_DUAL_HC | ATA_FLAG_NCQ,
		.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,
@@ -1291,7 +1295,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
@@ -1327,13 +1332,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
@@ -1382,7 +1385,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
@@ -1448,7 +1452,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.
@@ -1459,12 +1464,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;

ACK

though personally I would merge the ->dev_config() patch into this one

sata_mv No soft resets.

Soft resets rarely have significant effect with these chips, so always do a hard reset instead.

@@ -2414,8 +2414,7 @@
 

static void mv_error_handler(struct ata_port *ap)
{
- ata_do_eh(ap, mv_prereset, ata_std_softreset,
- mv_hardreset, mv_postreset);
+ ata_do_eh(ap, mv_prereset, NULL, mv_hardreset, mv_postreset);
}

static void mv_post_int_cmd(struct ata_queued_cmd *qc)

..

When I hit the existing EH, the system locks up.

When I remove the ata_std_softreset from that line, it no longer locks up.

It still does have other issues, though, so you could drop that patch for now, and I'll reissue it when I get round to fixing that particularly horribly broken part of the driver later.

Cheers

sata_mv Comments and version bump.

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

@@ -29,8 +29,6 @@
  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)
-
  5) Investigate problems with PCI Message Signalled Interrupts (MSI).
 
  6) Add port multiplier support (intermediate)

@@ -53,8 +51,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 +69,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 */

@@ -885,7 +881,7 @@
static void mv_start_dma(struct ata_port *ap, void __iomem *port_mmio,
struct mv_port_priv *pp, u8 protocol)
{
- int want_ncq = ata_is_ncq(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);

..

Mmm.. something weird happened with that original patch, in that it somehow also had this:

@@ -29,8 +29,6 @@
  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)
-
  5) Investigate problems with PCI Message Signalled Interrupts (MSI).
 
  6) Add port multiplier support (intermediate)

@@ -53,8 +51,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.
-

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

..

Well, 14 patches, not 12.
But they're mostly very small. :)

sata_mv Add missing qc_defer op.

This is necessary now that we support NCQ in the driver.

---
Who says a 14-patch series cannot have a 15th patch? :)

@@ -558,6 +558,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,
@@ -586,6 +587,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,

Patch process note:

Please don't duplicate the one-line summary found in the email subject line, in the email body. That causes all standard automated tools to copy that line twice into the permanent kernel changelog -- provided I don't go back and hand-edit each of the 15 patches.