[PATCH 00/12] sata_mv NCQ support

[PATCH 00/12] sata_mv NCQ support

[Mark Lord]

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

[PATCH 01/14] sata_mv EH fixes

[Mark Lord]

sata_mv EH fixes.

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

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 10:40:11.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 11:11:26.000000000 -0500
@@ -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;

[PATCH 02/14] sata_mv Mask transient IRQs.

[Mark Lord]

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.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 11:11:26.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 11:17:42.000000000 -0500
@@ -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),

[PATCH 03/14] sata_mv Clear queue indexes on chip restart

[Mark Lord]

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.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 11:17:42.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 11:23:05.000000000 -0500
@@ -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);

[PATCH 04/14] sata_mv rename base to port_mmio

[Mark Lord]

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.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 11:23:05.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 11:26:33.000000000 -0500
@@ -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)));
 }
 
 /**

[PATCH 05/14] sata_mv Fix EDMA configuration

[Mark Lord]

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.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 12:06:25.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 12:12:37.000000000 -0500
@@ -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;
 

[PATCH 06/14] sata_mv Add want_ncq parameter for EDMA configuration

[Mark Lord]

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

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 12:12:37.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 12:07:16.000000000 -0500
@@ -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;
 

[PATCH 07/14] sata_mv Use hqtag instead of ioid

[Mark Lord]

sata_mv Use hqtag instead of ioid.

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

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 12:07:16.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 12:35:14.000000000 -0500
@@ -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);
 

[PATCH 08/14] sata_mv Ignore response status LSB on NCQ

[Mark Lord]

sata_mv Ignore response status LSB on NCQ.

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

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 12:35:14.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 12:35:43.000000000 -0500
@@ -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;
 		}

[PATCH 09/14] sata_mv Restrict max_sectors to 8-bits on GenII NCQ

[Mark Lord]

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.. ?).

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 12:35:43.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 12:40:10.000000000 -0500
@@ -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)
 {

[PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables

[Mark Lord]

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.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 12:40:10.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 14:17:39.000000000 -0500
@@ -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");

[PATCH 11/12] sata_mv Introduce per-tag SG tables

[Mark Lord]

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.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 14:17:39.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 14:46:16.000000000 -0500
@@ -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;

[PATCH 12/14] sata_mv Enable NCQ operation

[Mark Lord]

sata_mv Enable NCQ operation.

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

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 14:46:16.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 14:49:28.000000000 -0500
@@ -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;

[PATCH 13/14] sata_mv No soft resets

[Mark Lord]

sata_mv No soft resets.

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

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 14:49:28.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 14:51:53.000000000 -0500
@@ -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)

[PATCH 14/14] sata_mv Comments and version bump

[Mark Lord]

sata_mv Comments and version bump.

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

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 15:09:31.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 15:41:17.000000000 -0500
@@ -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);

Re: [PATCH 00/12] sata_mv NCQ support

[Mark Lord]

Mark Lord wrote:
> Here is a set of 12 patches that gradually add working NCQ support to 
> sata_mv.
..

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

[PATCH 15/14] sata_mv NCQ support

[Mark Lord]

sata_mv Add missing qc_defer op.

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

Signed-off-by: Mark Lord <mlord@pobox.com>
---
Who says a 14-patch series cannot have a 15th patch?  :)

--- old/drivers/ata/sata_mv.c	2008-01-24 18:55:12.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 18:57:21.000000000 -0500
@@ -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,

Re: [PATCH 15/14] sata_mv NCQ support

[Tejun Heo]

Mark Lord wrote:
> sata_mv Add missing qc_defer op.
> 
> This is necessary now that we support NCQ in the driver.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>

Acked-by: Tejun Heo <htejun@gmail.com>

-- 
tejun

re: [PATCH 14/14] sata_mv Comments and version bump (v.2)

[Mark Lord]

Mark Lord wrote:
> sata_mv Comments and version bump.
> 
> Remove some obsolete comments, and bump up the driver version number.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
..

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

> -    int want_ncq = ata_is_ncq(protocol);
> +    int want_ncq = (protocol == ATA_PROT_NCQ); 
..

Reposting it again here, without that extra fragment:
* * * * *

sata_mv Comments and version bump.

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

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/ata/sata_mv.c	2008-01-24 15:09:31.000000000 -0500
+++ new/drivers/ata/sata_mv.c	2008-01-24 15:41:17.000000000 -0500
@@ -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 */

Re: [PATCH 01/14] sata_mv EH fixes

[Jeff Garzik]

Mark Lord wrote:
> sata_mv EH fixes.
> 
> A hard reset is necessary after hotplug events.
> Only clear the error irq bits that were set on entry.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- old/drivers/ata/sata_mv.c    2008-01-24 10:40:11.000000000 -0500
> +++ new/drivers/ata/sata_mv.c    2008-01-24 11:11:26.000000000 -0500
> @@ -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);

ACK

Re: [PATCH 02/14] sata_mv Mask transient IRQs.

[Jeff Garzik]

Mark Lord wrote:
> 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.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- old/drivers/ata/sata_mv.c    2008-01-24 11:11:26.000000000 -0500
> +++ new/drivers/ata/sata_mv.c    2008-01-24 11:17:42.000000000 -0500
> @@ -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 |

Overall operational changes -- ACK

However, I do not want to remove definitions for unchecked hardware 
bits, because the documentation is not public.

	Jeff

Re: [PATCH 04/14] sata_mv rename base to port_mmio

[Jeff Garzik]

Mark Lord wrote:
> 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.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- old/drivers/ata/sata_mv.c    2008-01-24 11:23:05.000000000 -0500
> +++ new/drivers/ata/sata_mv.c    2008-01-24 11:26:33.000000000 -0500
> @@ -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)));

ACK patches 3-4

Re: [PATCH 00/12] sata_mv NCQ support

[Jeff Garzik]

Mark Lord wrote:
> 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.

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.

Re: [PATCH 05/14] sata_mv Fix EDMA configuration

[Jeff Garzik]

Mark Lord wrote:
> 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.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- old/drivers/ata/sata_mv.c    2008-01-24 12:06:25.000000000 -0500
> +++ new/drivers/ata/sata_mv.c    2008-01-24 12:12:37.000000000 -0500
> @@ -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 */

You no longer clear bit 9 for Gen-I, which looks wrong.

everything else looks OK

Re: [PATCH 06/14] sata_mv Add want_ncq parameter for EDMA configuration

[Jeff Garzik]

Mark Lord wrote:
> 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).
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>

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

Re: [PATCH 07/14] sata_mv Use hqtag instead of ioid

[Jeff Garzik]

Mark Lord wrote:
> sata_mv Use hqtag instead of ioid.
> 
> Simplify tag handling by using the cid/hqtag field instead of ioid,
> as recommended by Marvell.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- old/drivers/ata/sata_mv.c    2008-01-24 12:07:16.000000000 -0500
> +++ new/drivers/ata/sata_mv.c    2008-01-24 12:35:14.000000000 -0500
> @@ -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),
> 

don't delete details of otherwise undocumented [publicly] hardware

everything else OK

Re: [PATCH 09/14] sata_mv Restrict max_sectors to 8-bits on GenII NCQ

[Jeff Garzik]

Mark Lord wrote:
> 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.. ?).
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- old/drivers/ata/sata_mv.c    2008-01-24 12:35:43.000000000 -0500
> +++ new/drivers/ata/sata_mv.c    2008-01-24 12:40:10.000000000 -0500
> @@ -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)
> {

ACK patches 8-9

Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables

[Jeff Garzik]

Mark Lord wrote:
> 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.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- old/drivers/ata/sata_mv.c    2008-01-24 12:40:10.000000000 -0500
> +++ new/drivers/ata/sata_mv.c    2008-01-24 14:17:39.000000000 -0500
> @@ -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);

Sorry, I would far rather waste a tiny bit of memory than this.

The main objection is that DMA allocation should /always/ be associated 
with a struct device, as that is where most of the connectivity with the 
DMA mask/boundary/etc. and private IOMMU hooks live.

The amount of memory used in the pre-existing configuration is small, it 
is already allocated on a per-device basis, and it is statically 
allocated -- meaning no worry about allocations failing inside of 
interrupts or similar nastiness.

	Jeff

Re: [PATCH 11/12] sata_mv Introduce per-tag SG tables

[Jeff Garzik]

Mark Lord wrote:
> 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.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>

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

Re: [PATCH 12/14] sata_mv Enable NCQ operation

[Jeff Garzik]

Mark Lord wrote:
> sata_mv Enable NCQ operation.
> 
> Final changes to actually turn on NCQ in the driver for GEN II/IIE 
> hardware.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>

ACK

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

Re: [PATCH 13/14] sata_mv No soft resets

[Jeff Garzik]

Mark Lord wrote:
> sata_mv No soft resets.
> 
> Soft resets rarely have significant effect with these chips,
> so always do a hard reset instead.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- old/drivers/ata/sata_mv.c    2008-01-24 14:49:28.000000000 -0500
> +++ new/drivers/ata/sata_mv.c    2008-01-24 14:51:53.000000000 -0500
> @@ -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);
> }

Can you give a bit more explanation?

In general I agree, but SRST delivery does seem to work as intended from 
a device perspective, so I would like to narrow down the conditions 
where SRST must be avoided

Re: [PATCH 15/14] sata_mv NCQ support

[Jeff Garzik]

Mark Lord wrote:
> sata_mv Add missing qc_defer op.
> 
> This is necessary now that we support NCQ in the driver.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> ---
> Who says a 14-patch series cannot have a 15th patch?  :)
> 
> --- old/drivers/ata/sata_mv.c    2008-01-24 18:55:12.000000000 -0500
> +++ new/drivers/ata/sata_mv.c    2008-01-24 18:57:21.000000000 -0500
> @@ -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,

ACK; I would combine this with the NCQ-enable patch near the end of the 
series (patch #12 in this series)

Re: [PATCH 02/14] sata_mv Mask transient IRQs.

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> 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.
>>
>> Signed-off-by: Mark Lord <mlord@pobox.com>
>>
>> --- old/drivers/ata/sata_mv.c    2008-01-24 11:11:26.000000000 -0500
>> +++ new/drivers/ata/sata_mv.c    2008-01-24 11:17:42.000000000 -0500
>> @@ -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 |
> 
> Overall operational changes -- ACK
> 
> However, I do not want to remove definitions for unchecked hardware 
> bits, because the documentation is not public.
..

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

Cheers

Re: [PATCH 05/14] sata_mv Fix EDMA configuration

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> 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.
>>
>> Signed-off-by: Mark Lord <mlord@pobox.com>
>>
>> --- old/drivers/ata/sata_mv.c    2008-01-24 12:06:25.000000000 -0500
>> +++ new/drivers/ata/sata_mv.c    2008-01-24 12:12:37.000000000 -0500
>> @@ -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 */
> 
> You no longer clear bit 9 for Gen-I, which looks wrong.
..

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

Cheers

Re: [PATCH 07/14] sata_mv Use hqtag instead of ioid

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> sata_mv Use hqtag instead of ioid.
>>
>> Simplify tag handling by using the cid/hqtag field instead of ioid,
>> as recommended by Marvell.
>>
>> Signed-off-by: Mark Lord <mlord@pobox.com>
>>
>> --- old/drivers/ata/sata_mv.c    2008-01-24 12:07:16.000000000 -0500
>> +++ new/drivers/ata/sata_mv.c    2008-01-24 12:35:14.000000000 -0500
>> @@ -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),
>>
> 
> don't delete details of otherwise undocumented [publicly] hardware
..

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

Or resubmit ?

Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
..
>> 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);
> 
> Sorry, I would far rather waste a tiny bit of memory than this.
..

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

Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables

[Mark Lord]

Jeff Garzik wrote:
..
> The amount of memory used in the pre-existing configuration is small, it 
> is already allocated on a per-device basis, and it is statically 
> allocated -- meaning no worry about allocations failing inside of 
> interrupts or similar nastiness.
..

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

Cheers

Re: [PATCH 13/14] sata_mv No soft resets

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> sata_mv No soft resets.
>>
>> Soft resets rarely have significant effect with these chips,
>> so always do a hard reset instead.
>>
>> Signed-off-by: Mark Lord <mlord@pobox.com>
>>
>> --- old/drivers/ata/sata_mv.c    2008-01-24 14:49:28.000000000 -0500
>> +++ new/drivers/ata/sata_mv.c    2008-01-24 14:51:53.000000000 -0500
>> @@ -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);
>> }
> 
> Can you give a bit more explanation?
..

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

Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables

[Mark Lord]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
> ..
>>> 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);
>>
>> Sorry, I would far rather waste a tiny bit of memory than this.
> ..
> 
> 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.
..

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

Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables

[Jeff Garzik]

Mark Lord wrote:
> 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).


(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

Re: [PATCH 10/14] sata_mv Use DMA memory pools for hardware memory tables

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Just say exactly what you require here.
..
>.. repost the whole thing ..

..

Okay, will do in a couple of days.

-ml