sata_mv: The last big set for 2.6.26

Jeff,

Here's my final series of (12) patches, to complete the overhaul of interrupt and error handling for sata_mv. With these patches in place, I actually *would* trust sata_mv with my data. :)

So perhaps another patch may follow to remove the "HIGHLY EXPERIMENTAL" label.

The only other stuff planned for 2.6.26, are some small errata workarounds for various chipset "features", as I get to them. Not many, but a few for sure.

Please apply for 2.6.26. Thanks.

More cosmetic changes; no code changes.

-- try and improve consistency of naming.
-- add missing _OFS to tails of register offset definitions.
-- rename mv_setup_ifctl() to mv_setup_ifcfg(), since that's what it really does. -- remove/move some dead comments

@@ -91,9 +91,9 @@

	MV_IRQ_COAL_TIME_THRESHOLD	= (MV_IRQ_COAL_REG_BASE + 0xd0),

	MV_SATAHC0_REG_BASE	= 0x20000,

- MV_FLASH_CTL = 0x1046c,
- MV_GPIO_PORT_CTL = 0x104f0,
- MV_RESET_CFG = 0x180d8,
+ MV_FLASH_CTL_OFS = 0x1046c,
+ MV_GPIO_PORT_CTL_OFS = 0x104f0,
+ MV_RESET_CFG_OFS = 0x180d8,

	MV_PCI_REG_SZ		= MV_MAJOR_REG_AREA_SZ,
	MV_SATAHC_REG_SZ	= MV_MAJOR_REG_AREA_SZ,
@@ -147,18 +147,21 @@
	/* PCI interface registers */

	PCI_COMMAND_OFS		= 0xc00,

+ PCI_COMMAND_MRDTRIG = (1 << 7), /* PCI Master Read Trigger */

	PCI_MAIN_CMD_STS_OFS	= 0xd30,
	STOP_PCI_MASTER		= (1 << 2),
	PCI_MASTER_EMPTY	= (1 << 3),
	GLOB_SFT_RST		= (1 << 4),

- MV_PCI_MODE = 0xd00,
+ MV_PCI_MODE_OFS = 0xd00,
+ MV_PCI_MODE_MASK = 0x30,
+

	MV_PCI_EXP_ROM_BAR_CTL	= 0xd2c,
	MV_PCI_DISC_TIMER	= 0xd04,
	MV_PCI_MSI_TRIGGER	= 0xc38,
	MV_PCI_SERR_MASK	= 0xc28,
-	MV_PCI_XBAR_TMOUT	= 0x1d04,
+	MV_PCI_XBAR_TMOUT_OFS	= 0x1d04,
	MV_PCI_ERR_LOW_ADDRESS	= 0x1d40,
	MV_PCI_ERR_HIGH_ADDRESS	= 0x1d44,
	MV_PCI_ERR_ATTRIBUTE	= 0x1d48,
@@ -225,16 +228,18 @@
	PHY_MODE4		= 0x314,
	PHY_MODE2		= 0x330,
	SATA_IFCTL_OFS		= 0x344,
+	SATA_TESTCTL_OFS	= 0x348,
	SATA_IFSTAT_OFS		= 0x34c,
	VENDOR_UNIQUE_FIS_OFS	= 0x35c,

- FIS_CFG_OFS = 0x360,
- FIS_CFG_SINGLE_SYNC = (1 << 16), /* SYNC on DMA activation */
+ FISCFG_OFS = 0x360,
+ FISCFG_WAIT_DEV_ERR = (1 << 8), /* wait for host on DevErr */
+ FISCFG_SINGLE_SYNC = (1 << 16), /* SYNC on DMA activation */

	MV5_PHY_MODE		= 0x74,

- MV5_LT_MODE = 0x30,
- MV5_PHY_CTL = 0x0C,
- SATA_INTERFACE_CFG = 0x050,
+ MV5_LTMODE_OFS = 0x30,
+ MV5_PHY_CTL_OFS = 0x0C,
+ SATA_INTERFACE_CFG_OFS = 0x050,

	MV_M2_PREAMP_MASK	= 0x7e0,

@@ -332,10 +337,16 @@

	EDMA_CMD_OFS		= 0x28,		/* EDMA command register */
	EDMA_EN			= (1 << 0),	/* enable EDMA */
	EDMA_DS			= (1 << 1),	/* disable EDMA; self-negated */

- ATA_RST = (1 << 2), /* reset trans/link/phy */
+ EDMA_RESET = (1 << 2), /* reset eng/trans/link/phy */
+
+ EDMA_STATUS_OFS = 0x30, /* EDMA engine status */
+ EDMA_STATUS_CACHE_EMPTY = (1 << 6), /* GenIIe command cache empty */
+ EDMA_STATUS_IDLE = (1 << 7), /* GenIIe EDMA enabled/idle */

- EDMA_IORDY_TMOUT = 0x34,
- EDMA_ARB_CFG = 0x38,
+ EDMA_IORDY_TMOUT_OFS = 0x34,
+ EDMA_ARB_CFG_OFS = 0x38,
+
+ EDMA_HALTCOND_OFS = 0x60, /* GenIIe halt conditions */

	GEN_II_NCQ_MAX_SECTORS	= 256,		/* max sects/io on Gen2 w/NCQ */

@@ -359,6 +370,7 @@
#define IS_GEN_I(hpriv) ((hpriv)->hp_flags & MV_HP_GEN_I) #define IS_GEN_II(hpriv) ((hpriv)->hp_flags & MV_HP_GEN_II) #define IS_GEN_IIE(hpriv) ((hpriv)->hp_flags & MV_HP_GEN_IIE) +#define IS_PCIE(hpriv) ((hpriv)->hp_flags & MV_HP_PCIE) #define HAS_PCI(host) (!((host)->ports[0]->flags & MV_FLAG_SOC))

#define WINDOW_CTRL(i) (0x20030 + ((i) << 4))
@@ -1059,22 +1071,22 @@

static void mv_config_fbs(void __iomem *port_mmio, int enable_fbs) {
- u32 old_fcfg, new_fcfg, old_ltmode, new_ltmode;

+	u32 old_fiscfg, new_fiscfg, old_ltmode, new_ltmode;
	/*
	 * Various bit settings required for operation
	 * in FIS-based switching (fbs) mode on GenIIe:
	 */
-	old_fcfg   = readl(port_mmio + FIS_CFG_OFS);
+	old_fiscfg = readl(port_mmio + FISCFG_OFS);
	old_ltmode = readl(port_mmio + LTMODE_OFS);
	if (enable_fbs) {
-		new_fcfg   = old_fcfg   |  FIS_CFG_SINGLE_SYNC;
+		new_fiscfg = old_fiscfg |  FISCFG_SINGLE_SYNC;
		new_ltmode = old_ltmode |  LTMODE_BIT8;
	} else { /* disable fbs */
-		new_fcfg   = old_fcfg   & ~FIS_CFG_SINGLE_SYNC;
+		new_fiscfg = old_fiscfg & ~FISCFG_SINGLE_SYNC;
		new_ltmode = old_ltmode & ~LTMODE_BIT8;
	}
-	if (new_fcfg != old_fcfg)
-		writelfl(new_fcfg, port_mmio + FIS_CFG_OFS);
+	if (new_fiscfg != old_fiscfg)
+		writelfl(new_fiscfg, port_mmio + FISCFG_OFS);
	if (new_ltmode != old_ltmode)
		writelfl(new_ltmode, port_mmio + LTMODE_OFS);
}

@@ -1894,7 +1906,7 @@

static void mv5_reset_flash(struct mv_host_priv *hpriv, void __iomem *mmio) {
- writel(0x0fcfffff, mmio + MV_FLASH_CTL);
+ writel(0x0fcfffff, mmio + MV_FLASH_CTL_OFS);
}

static void mv5_read_preamp(struct mv_host_priv *hpriv, int idx, @@ -1913,7 +1925,7 @@

{
	u32 tmp;

- writel(0, mmio + MV_GPIO_PORT_CTL);
+ writel(0, mmio + MV_GPIO_PORT_CTL_OFS);

	/* FIXME: handle MV_HP_ERRATA_50XXB2 errata */

@@ -1931,14 +1943,14 @@

	int fix_apm_sq = (hpriv->hp_flags & MV_HP_ERRATA_50XXB0);

	if (fix_apm_sq) {
-		tmp = readl(phy_mmio + MV5_LT_MODE);
+		tmp = readl(phy_mmio + MV5_LTMODE_OFS);
		tmp |= (1 << 19);

- writel(tmp, phy_mmio + MV5_LT_MODE);
+ writel(tmp, phy_mmio + MV5_LTMODE_OFS);

- tmp = readl(phy_mmio + MV5_PHY_CTL);

+		tmp = readl(phy_mmio + MV5_PHY_CTL_OFS);
		tmp &= ~0x3;
		tmp |= 0x1;
-		writel(tmp, phy_mmio + MV5_PHY_CTL);
+		writel(tmp, phy_mmio + MV5_PHY_CTL_OFS);
	}

	tmp = readl(phy_mmio + MV5_PHY_MODE);
@@ -1956,11 +1968,6 @@
{
	void __iomem *port_mmio = mv_port_base(mmio, port);

- /*
- * The datasheet warns against setting ATA_RST when EDMA is active
- * (but doesn't say what the problem might be). So we first try
- * to disable the EDMA engine before doing the ATA_RST operation.
- */

	mv_reset_channel(hpriv, mmio, port);

	ZERO(0x028);	/* command */
@@ -1975,7 +1982,7 @@
	ZERO(0x024);	/* respq outp */
	ZERO(0x020);	/* respq inp */
	ZERO(0x02c);	/* test control */

- writel(0xbc, port_mmio + EDMA_IORDY_TMOUT);
+ writel(0xbc, port_mmio + EDMA_IORDY_TMOUT_OFS);
}
#undef ZERO

@@ -2021,13 +2028,13 @@

	struct mv_host_priv *hpriv = host->private_data;
	u32 tmp;

- tmp = readl(mmio + MV_PCI_MODE);

+	tmp = readl(mmio + MV_PCI_MODE_OFS);
	tmp &= 0xff00ffff;

- writel(tmp, mmio + MV_PCI_MODE);
+ writel(tmp, mmio + MV_PCI_MODE_OFS);

	ZERO(MV_PCI_DISC_TIMER);
	ZERO(MV_PCI_MSI_TRIGGER);
-	writel(0x000100ff, mmio + MV_PCI_XBAR_TMOUT);
+	writel(0x000100ff, mmio + MV_PCI_XBAR_TMOUT_OFS);
	ZERO(PCI_HC_MAIN_IRQ_MASK_OFS);
	ZERO(MV_PCI_SERR_MASK);
	ZERO(hpriv->irq_cause_ofs);

@@ -2045,10 +2052,10 @@

	mv5_reset_flash(hpriv, mmio);

- tmp = readl(mmio + MV_GPIO_PORT_CTL);

+	tmp = readl(mmio + MV_GPIO_PORT_CTL_OFS);
	tmp &= 0x3;
	tmp |= (1 << 5) | (1 << 6);

- writel(tmp, mmio + MV_GPIO_PORT_CTL);
+ writel(tmp, mmio + MV_GPIO_PORT_CTL_OFS);
}

/**
@@ -2121,7 +2128,7 @@

	void __iomem *port_mmio;
	u32 tmp;

- tmp = readl(mmio + MV_RESET_CFG);

+	tmp = readl(mmio + MV_RESET_CFG_OFS);
	if ((tmp & (1 << 0)) == 0) {
		hpriv->signal[idx].amps = 0x7 << 8;
		hpriv->signal[idx].pre = 0x1 << 5;

@@ -2137,7 +2144,7 @@

static void mv6_enable_leds(struct mv_host_priv *hpriv, void __iomem *mmio) {
- writel(0x00000060, mmio + MV_GPIO_PORT_CTL);
+ writel(0x00000060, mmio + MV_GPIO_PORT_CTL_OFS);
}

static void mv6_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio, @@ -2235,11 +2242,6 @@

{
	void __iomem *port_mmio = mv_port_base(mmio, port);

- /*
- * The datasheet warns against setting ATA_RST when EDMA is active
- * (but doesn't say what the problem might be). So we first try
- * to disable the EDMA engine before doing the ATA_RST operation.
- */

	mv_reset_channel(hpriv, mmio, port);

	ZERO(0x028);		/* command */
@@ -2254,7 +2256,7 @@
	ZERO(0x024);		/* respq outp */
	ZERO(0x020);		/* respq inp */
	ZERO(0x02c);		/* test control */

- writel(0xbc, port_mmio + EDMA_IORDY_TMOUT);
+ writel(0xbc, port_mmio + EDMA_IORDY_TMOUT_OFS);
}

#undef ZERO
@@ -2297,38 +2299,39 @@

	return;
}

-static void mv_setup_ifctl(void __iomem *port_mmio, int want_gen2i) +static void mv_setup_ifcfg(void __iomem *port_mmio, int want_gen2i) {
- u32 ifctl = readl(port_mmio + SATA_INTERFACE_CFG);
+ u32 ifcfg = readl(port_mmio + SATA_INTERFACE_CFG_OFS);

- ifctl = (ifctl & 0xf7f) | 0x9b1000; /* from chip spec */
+ ifcfg = (ifcfg & 0xf7f) | 0x9b1000; /* from chip spec */

	if (want_gen2i)

- ifctl |= (1 << 7); /* enable gen2i speed */
- writelfl(ifctl, port_mmio + SATA_INTERFACE_CFG);
+ ifcfg |= (1 << 7); /* enable gen2i speed */
+ writelfl(ifcfg, port_mmio + SATA_INTERFACE_CFG_OFS);
}

-/*
- * Caller must ensure that EDMA is not active,
- * by first doing mv_stop_edma() where needed.
- */
static void mv_reset_channel(struct mv_host_priv *hpriv, void __iomem *mmio,
unsigned int port_no)

{
	void __iomem *port_mmio = mv_port_base(mmio, port_no);

+ /*
+ * The datasheet warns against setting EDMA_RESET when EDMA is active
+ * (but doesn't say what the problem might be). So we first try
+ * to disable the EDMA engine before doing the EDMA_RESET operation.
+ */

	mv_stop_edma_engine(port_mmio);

- writelfl(ATA_RST, port_mmio + EDMA_CMD_OFS);
+ writelfl(EDMA_RESET, port_mmio + EDMA_CMD_OFS);

	if (!IS_GEN_I(hpriv)) {
-		/* Enable 3.0gb/s link speed */
-		mv_setup_ifctl(port_mmio, 1);
+		/* Enable 3.0gb/s link speed: this survives EDMA_RESET */
+		mv_setup_ifcfg(port_mmio, 1);
	}
	/*

- * Strobing ATA_RST here causes a hard reset of the SATA transport,
+ * Strobing EDMA_RESET here causes a hard reset of the SATA transport,
* link, and physical layers. It resets all SATA interface registers
* (except for SATA_INTERFACE_CFG), and issues a COMRESET to the dev. */
- writelfl(ATA_RST, port_mmio + EDMA_CMD_OFS);

+	writelfl(EDMA_RESET, port_mmio + EDMA_CMD_OFS);
	udelay(25);	/* allow reset propagation */
	writelfl(0, port_mmio + EDMA_CMD_OFS);

@@ -2392,7 +2395,7 @@

		sata_scr_read(link, SCR_STATUS, &sstatus);
		if (!IS_GEN_I(hpriv) && ++attempts >= 5 && sstatus == 0x121) {
			/* Force 1.5gb/s link speed and try again */

- mv_setup_ifctl(mv_ap_base(ap), 0);
+ mv_setup_ifcfg(mv_ap_base(ap), 0);
if (time_after(jiffies + HZ, deadline))
extra = HZ; /* only extend it once, max */
}
@@ -2590,6 +2593,7 @@

				" and avoid the final two gigabytes on"
				" all RocketRAID BIOS initialized drives.\n");
		}
+		/* drop through */
	case chip_6042:
		hpriv->ops = &mv6xxx_ops;
		hp_flags |= MV_HP_GEN_IIE;

Some of the GenIIe EDMA optimizations should not be used for non-PCI (SOC) devices, and nor for certain configurations of conventional PCI (non PCI-X, PCIe) buses.

Logic taken/simplified from that in the Marvell proprietary driver.

@@ -361,6 +361,7 @@

	MV_HP_GEN_II		= (1 << 7),	/* Generation II: 60xx */
	MV_HP_GEN_IIE		= (1 << 8),	/* Generation IIE: 6042/7042 */
	MV_HP_PCIE		= (1 << 9),	/* PCIe bus/regs: 7042 */

+ MV_HP_CUT_THROUGH = (1 << 10), /* can use EDMA cut-through */

	/* Port private flags (pp_flags) */
	MV_PP_FLAG_EDMA_EN	= (1 << 0),	/* is EDMA engine enabled? */
@@ -1110,8 +1111,10 @@
	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 << 18); /* enab early completion */
- cfg |= (1 << 17); /* enab cut-through (dis stor&forwrd) */
+ if (HAS_PCI(ap->host))
+ cfg |= (1 << 18); /* enab early completion */
+ if (hpriv->hp_flags & MV_HP_CUT_THROUGH)
+ cfg |= (1 << 17); /* enab cut-thru (dis stor&forwrd) */

		if (want_ncq && sata_pmp_attached(ap)) {
			cfg |= EDMA_CFG_EDMA_FBS; /* FIS-based switching */
@@ -2496,6 +2499,34 @@
		readl(port_mmio + EDMA_ERR_IRQ_MASK_OFS));
}

+static unsigned int mv_in_pcix_mode(struct ata_host *host) +{
+ struct mv_host_priv *hpriv = host->private_data;
+ void __iomem *mmio = hpriv->base;
+ u32 reg;
+
+ if (!HAS_PCI(host) || !IS_PCIE(hpriv))
+ return 0; /* not PCI-X capable */
+ reg = readl(mmio + MV_PCI_MODE_OFS);
+ if ((reg & MV_PCI_MODE_MASK) == 0)
+ return 0; /* conventional PCI mode */
+ return 1; /* chip is in PCI-X mode */
+}
+
+static int mv_pci_cut_through_okay(struct ata_host *host) +{
+ struct mv_host_priv *hpriv = host->private_data;
+ void __iomem *mmio = hpriv->base;
+ u32 reg;
+
+ if (!mv_in_pcix_mode(host)) {
+ reg = readl(mmio + PCI_COMMAND_OFS);
+ if (reg & PCI_COMMAND_MRDTRIG)
+ return 0; /* not okay */
+ }
+ return 1; /* okay */
+}
+
static int mv_chip_id(struct ata_host *host, unsigned int board_idx)

{
	struct pci_dev *pdev = to_pci_dev(host->dev);
@@ -2563,7 +2594,7 @@
		break;

	case chip_7042:
-		hp_flags |= MV_HP_PCIE;
+		hp_flags |= MV_HP_PCIE | MV_HP_CUT_THROUGH;
		if (pdev->vendor == PCI_VENDOR_ID_TTI &&
		    (pdev->device == 0x2300 || pdev->device == 0x2310))
		{
@@ -2597,6 +2628,8 @@
	case chip_6042:
		hpriv->ops = &mv6xxx_ops;
		hp_flags |= MV_HP_GEN_IIE;

+ if (board_idx == chip_6042 && mv_pci_cut_through_okay(host))
+ hp_flags |= MV_HP_CUT_THROUGH;

		switch (pdev->revision) {
		case 0x0:

When performing EH, it is recommended to wait for the EDMA engine to empty out requests-in-progress before disabling EDMA.

Introduce code to poll the EDMA_STATUS register for idle/empty bits before disabling EDMA. For non-EH operation, this will normally exit without delay, other than the register read.

A later series of patches may focus on eliminating this and various other register reads (when possible) throughout the driver, but for now we're focussing on solid reliablity.

@@ -888,6 +888,25 @@

	}
}

+static void mv_wait_for_edma_empty_idle(struct ata_port *ap) +{
+ void __iomem *port_mmio = mv_ap_base(ap);
+ const u32 empty_idle = (EDMA_STATUS_CACHE_EMPTY | EDMA_STATUS_IDLE);
+ const int per_loop = 5, timeout = (15 * 1000 / per_loop);
+ int i;
+
+ /*
+ * Wait for the EDMA engine to finish transactions in progress.
+ */
+ for (i = 0; i < timeout; ++i) {
+ u32 edma_stat = readl(port_mmio + EDMA_STATUS_OFS);
+ if ((edma_stat & empty_idle) == empty_idle)
+ break;
+ udelay(per_loop);
+ }
+ /* ata_port_printk(ap, KERN_INFO, "%s: %u+ usecs\n", __func__, i); */
+}
+
/**
* mv_stop_edma_engine - Disable eDMA engine
* @port_mmio: io base address
@@ -920,6 +939,7 @@

	if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN))
		return 0;
	pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
+	mv_wait_for_edma_empty_idle(ap);
	if (mv_stop_edma_engine(port_mmio)) {
		ata_port_printk(ap, KERN_ERR, "Unable to stop eDMA\n");
		return -EIO;

The EDMA engine cannot tolerate a mix of NCQ/non-NCQ commands, and cannot be used for PIO at all. So we need to prevent libata from trying to feed us such mixtures.

Introduce mv_qc_defer() for this purpose, and use it for all chip versions.

@@ -492,6 +492,7 @@

static int mv5_scr_write(struct ata_port *ap, unsigned int sc_reg_in, u32 val); static int mv_port_start(struct ata_port *ap);
static void mv_port_stop(struct ata_port *ap);
+static int mv_qc_defer(struct ata_queued_cmd *qc);
static void mv_qc_prep(struct ata_queued_cmd *qc);
static void mv_qc_prep_iie(struct ata_queued_cmd *qc);
static unsigned int mv_qc_issue(struct ata_queued_cmd *qc);
@@ -561,6 +562,7 @@

static struct ata_port_operations mv5_ops = {
	.inherits		= &ata_sff_port_ops,

+ .qc_defer = mv_qc_defer,

	.qc_prep		= mv_qc_prep,
	.qc_issue		= mv_qc_issue,

@@ -579,7 +581,6 @@

static struct ata_port_operations mv6_ops = {
	.inherits		= &mv5_ops,
-	.qc_defer		= sata_pmp_qc_defer_cmd_switch,
	.dev_config             = mv6_dev_config,
	.scr_read		= mv_scr_read,
	.scr_write		= mv_scr_write,

@@ -592,7 +593,6 @@

static struct ata_port_operations mv_iie_ops = {
	.inherits		= &mv6_ops,
-	.qc_defer		= ata_std_qc_defer, /* FIS-based switching */
	.dev_config		= ATA_OP_NULL,
	.qc_prep		= mv_qc_prep_iie,
};
@@ -1090,6 +1090,45 @@
	}
}

+static int mv_qc_defer(struct ata_queued_cmd *qc)
+{
+ struct ata_link *link = qc->dev->link;
+ struct ata_port *ap = link->ap;
+ struct mv_port_priv *pp = ap->private_data;
+
+ /*
+ * If the port is completely idle, then allow the new qc.
+ */
+ if (ap->nr_active_links == 0)
+ return 0;
+
+ if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
+ /*
+ * The port is operating in host queuing mode (EDMA).
+ * It can accomodate a new qc if the qc protocol
+ * is compatible with the current host queue mode.
+ */
+ if (pp->pp_flags & MV_PP_FLAG_NCQ_EN) {
+ /*
+ * The host queue (EDMA) is in NCQ mode.
+ * If the new qc is also an NCQ command,
+ * then allow the new qc.
+ */
+ if (qc->tf.protocol == ATA_PROT_NCQ)
+ return 0;
+ } else {
+ /*
+ * The host queue (EDMA) is in non-NCQ, DMA mode.
+ * If the new qc is also a non-NCQ, DMA command,
+ * then allow the new qc.
+ */
+ if (qc->tf.protocol == ATA_PROT_DMA)
+ return 0;
+ }
+ }
+ return ATA_DEFER_PORT;
+}
+
static void mv_config_fbs(void __iomem *port_mmio, int enable_fbs)

{
	u32 old_fiscfg, new_fiscfg, old_ltmode, new_ltmode;

Part 1 of workaround for errata "sata#25" for the 60x1 series (the second half of this errata workaround is still in development.

Bit22 of the GPIO port has to be set "on" when in NCQ mode.

@@ -1151,6 +1151,21 @@

		writelfl(new_ltmode, port_mmio + LTMODE_OFS);
}

+static void mv_60x1_errata_sata25(struct ata_port *ap, int want_ncq) +{
+ struct mv_host_priv *hpriv = ap->host->private_data;
+ u32 old, new;
+
+ /* workaround for 88SX60x1 FEr SATA#25 (part 1) */
+ old = readl(hpriv->base + MV_GPIO_PORT_CTL_OFS);
+ if (want_ncq)
+ new = old | (1 << 22);
+ else
+ new = old & ~(1 << 22);
+ if (new != old)
+ writel(new, hpriv->base + MV_GPIO_PORT_CTL_OFS);
+}
+
static void mv_edma_cfg(struct ata_port *ap, int want_ncq)

{
	u32 cfg;
@@ -1164,10 +1179,11 @@
	if (IS_GEN_I(hpriv))
		cfg |= (1 << 8);	/* enab config burst size mask */

- else if (IS_GEN_II(hpriv))

+	else if (IS_GEN_II(hpriv)) {
		cfg |= EDMA_CFG_RD_BRST_EXT | EDMA_CFG_WR_BUFF_LEN;

+ mv_60x1_errata_sata25(ap, want_ncq);

- else if (IS_GEN_IIE(hpriv)) {

+	} 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 */
		if (HAS_PCI(ap->host))

Rearrange mv_config_fbs() to more closely follow the (corrected) datasheet recommendations for NCQ and FIS-based switching (FBS).

Also, maintain a port flag to let us know when FBS is enabled. We will make more use of that flag later in this patch series.

---
The final patch in this series builds further on top of this prep work.

@@ -366,6 +366,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_FBS_EN = (1 << 2), /* is EDMA set up for FBS? */
};

#define IS_GEN_I(hpriv) ((hpriv)->hp_flags & MV_HP_GEN_I) @@ -1129,26 +1130,31 @@

	return ATA_DEFER_PORT;
}

-static void mv_config_fbs(void __iomem *port_mmio, int enable_fbs) +static void mv_config_fbs(void __iomem *port_mmio, int want_ncq, int want_fbs) {
- u32 old_fiscfg, new_fiscfg, old_ltmode, new_ltmode;
- /*
- * Various bit settings required for operation
- * in FIS-based switching (fbs) mode on GenIIe:
- */
- old_fiscfg = readl(port_mmio + FISCFG_OFS);
- old_ltmode = readl(port_mmio + LTMODE_OFS);
- if (enable_fbs) {

-		new_fiscfg = old_fiscfg |  FISCFG_SINGLE_SYNC;
-		new_ltmode = old_ltmode |  LTMODE_BIT8;

- } else { /* disable fbs */
- new_fiscfg = old_fiscfg & ~FISCFG_SINGLE_SYNC;
- new_ltmode = old_ltmode & ~LTMODE_BIT8;
+ u32 new_fiscfg, old_fiscfg;
+ u32 new_ltmode, old_ltmode;
+ u32 new_haltcond, old_haltcond;
+
+ old_fiscfg = readl(port_mmio + FISCFG_OFS);
+ old_ltmode = readl(port_mmio + LTMODE_OFS);
+ old_haltcond = readl(port_mmio + EDMA_HALTCOND_OFS);
+
+ new_fiscfg = old_fiscfg & ~(FISCFG_SINGLE_SYNC | FISCFG_WAIT_DEV_ERR);
+ new_ltmode = old_ltmode & ~LTMODE_BIT8;
+ new_haltcond = old_haltcond | EDMA_ERR_DEV;
+
+ if (want_fbs) {
+ new_fiscfg = old_fiscfg | FISCFG_SINGLE_SYNC;

+		new_ltmode = old_ltmode | LTMODE_BIT8;
	}
+
	if (new_fiscfg != old_fiscfg)
		writelfl(new_fiscfg, port_mmio + FISCFG_OFS);
	if (new_ltmode != old_ltmode)
		writelfl(new_ltmode, port_mmio + LTMODE_OFS);

+ if (new_haltcond != old_haltcond)
+ writelfl(new_haltcond, port_mmio + EDMA_HALTCOND_OFS);
}

static void mv_60x1_errata_sata25(struct ata_port *ap, int want_ncq) @@ -1175,6 +1181,7 @@

	/* set up non-NCQ EDMA configuration */
	cfg = EDMA_CFG_Q_DEPTH;		/* always 0x1f for *all* chips */

+ pp->pp_flags &= ~MV_PP_FLAG_FBS_EN;

	if (IS_GEN_I(hpriv))
		cfg |= (1 << 8);	/* enab config burst size mask */
@@ -1184,19 +1191,30 @@
		mv_60x1_errata_sata25(ap, want_ncq);

	} else if (IS_GEN_IIE(hpriv)) {

+ int want_fbs = sata_pmp_attached(ap);
+ /*
+ * Possible future enhancement:
+ *
+ * The chip can use FBS with non-NCQ, if we allow it,
+ * But first we need to have the error handling in place
+ * for this mode (datasheet section 7.3.15.4.2.3).
+ * So disallow non-NCQ FBS for now.
+ */
+ want_fbs &= want_ncq;
+
+ mv_config_fbs(port_mmio, want_ncq, want_fbs);
+
+ if (want_fbs) {
+ pp->pp_flags |= MV_PP_FLAG_FBS_EN;
+ cfg |= EDMA_CFG_EDMA_FBS; /* FIS-based switching */
+ }
+

		cfg |= (1 << 23);	/* do not mask PM field in rx'd FIS */
		cfg |= (1 << 22);	/* enab 4-entry host queue cache */
		if (HAS_PCI(ap->host))
			cfg |= (1 << 18);	/* enab early completion */
		if (hpriv->hp_flags & MV_HP_CUT_THROUGH)
			cfg |= (1 << 17); /* enab cut-thru (dis stor&forwrd) */

-
- if (want_ncq && sata_pmp_attached(ap)) {
- cfg |= EDMA_CFG_EDMA_FBS; /* FIS-based switching */
- mv_config_fbs(port_mmio, 1);
- } else {
- mv_config_fbs(port_mmio, 0);
- }

	}

	if (want_ncq) {

Sigh. Undo some earlier changes to mv_port_intr(), so that we now read/clear SError again in all cases.

Arrange the top of the function to be as close as possible
to what we need for a later update (in this series) for ERR_DEV handling.

Fix things so that libata-eh can attempt a READ_LOG_EXT_10H in response to a failed NCQ command, by just doing a local mv_eh_freeze() rather than ata_port_freeze().

This will now fully handle NCQ errors much of the time, but more fixes are needed for FBS/PMP, and for certain chip errata.

@@ -1627,7 +1627,7 @@
 *      LOCKING:
 *      Inherited from caller.
 */
-static void mv_err_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
+static void mv_err_intr(struct ata_port *ap)

{
	void __iomem *port_mmio = mv_ap_base(ap);
	u32 edma_err_cause, eh_freeze_mask, serr = 0;
@@ -1635,24 +1635,33 @@
	struct mv_host_priv *hpriv = ap->host->private_data;
	unsigned int action = 0, err_mask = 0;
	struct ata_eh_info *ehi = &ap->link.eh_info;

-
- ata_ehi_clear_desc(ehi);
+ struct ata_queued_cmd *qc;
+ int abort = 0;

	/*

- * Read and clear the err_cause bits. This won't actually
- * clear for some errors (eg. SError), but we will be doing
- * a hard reset in those cases regardless, which *will* clear it.
+ * Read and clear the SError and err_cause bits.

	 */
+	sata_scr_read(&ap->link, SCR_ERROR, &serr);
+	sata_scr_write_flush(&ap->link, SCR_ERROR, serr);
+
	edma_err_cause = readl(port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
	writelfl(~edma_err_cause, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);

- ata_ehi_push_desc(ehi, "edma_err_cause=%08x", edma_err_cause);
+ ata_port_printk(ap, KERN_INFO, "%s: err_cause=%08x pp_flags=0x%x\n",
+ __func__, edma_err_cause, pp->pp_flags);

+ qc = mv_get_active_qc(ap);
+ ata_ehi_clear_desc(ehi);
+ ata_ehi_push_desc(ehi, "edma_err_cause=%08x pp_flags=%08x",

+			  edma_err_cause, pp->pp_flags);
	/*
	 * All generations share these EDMA error cause bits:
	 */
-	if (edma_err_cause & EDMA_ERR_DEV)
+	if (edma_err_cause & EDMA_ERR_DEV) {
		err_mask |= AC_ERR_DEV;

+ action |= ATA_EH_RESET;
+ ata_ehi_push_desc(ehi, "dev error");
+ }
if (edma_err_cause & (EDMA_ERR_D_PAR | EDMA_ERR_PRD_PAR |
EDMA_ERR_CRQB_PAR | EDMA_ERR_CRPB_PAR | EDMA_ERR_INTRL_PAR)) {
@@ -1684,13 +1693,6 @@

			ata_ehi_push_desc(ehi, "EDMA self-disable");
		}
		if (edma_err_cause & EDMA_ERR_SERR) {

- /*
- * Ensure that we read our own SCR, not a pmp link SCR:
- */
- ap->ops->scr_read(ap, SCR_ERROR, &serr);
- /*
- * Don't clear SError here; leave it for libata-eh:
- */

			ata_ehi_push_desc(ehi, "SError=%08x", serr);
			err_mask |= AC_ERR_ATA_BUS;
			action |= ATA_EH_RESET;
@@ -1710,10 +1712,29 @@
	else
		ehi->err_mask |= err_mask;

- if (edma_err_cause & eh_freeze_mask)
+ if (err_mask == AC_ERR_DEV) {
+ /*
+ * Cannot do ata_port_freeze() here,
+ * because it would kill PIO access,
+ * which is needed for further diagnosis.
+ */
+ mv_eh_freeze(ap);
+ abort = 1;
+ } else if (edma_err_cause & eh_freeze_mask) {
+ /*
+ * Note to self: ata_port_freeze() calls ata_port_abort()
+ */

		ata_port_freeze(ap);

- else
- ata_port_abort(ap);
+ } else {
+ abort = 1;
+ }
+
+ if (abort) {
+ if (qc)
+ ata_link_abort(qc->dev->link);
+ else
+ ata_port_abort(ap);
+ }
}

static void mv_process_crpb_response(struct ata_port *ap, @@ -1740,8 +1761,9 @@

			}
		}
		ata_status = edma_status >> CRPB_FLAG_STATUS_SHIFT;

- qc->err_mask |= ac_err_mask(ata_status);
- ata_qc_complete(qc);
+ if (!ac_err_mask(ata_status))
+ ata_qc_complete(qc);
+ /* else: leave it for mv_err_intr() */

	} else {
		ata_port_printk(ap, KERN_ERR, "%s: no qc for tag=%d\n",
				__func__, tag);
@@ -1845,7 +1867,7 @@
		 * Handle chip-reported errors, or continue on to handle PIO.
		 */
		if (unlikely(port_cause & ERR_IRQ)) {
-			mv_err_intr(ap, mv_get_active_qc(ap));
+			mv_err_intr(ap);
		} else if (hc_irq_cause & (DEV_IRQ << hardport)) {
			if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN)) {
				struct ata_queued_cmd *qc = mv_get_active_qc(ap);

Remove the unwanted reads of hc_irq_cause from mv_host_intr(), thereby removing a bug whereby we were not always reading it when needed..

@@ -1818,48 +1818,61 @@

static int mv_host_intr(struct ata_host *host, u32 main_irq_cause)

{
	struct mv_host_priv *hpriv = host->private_data;
-	void __iomem *mmio = hpriv->base, *hc_mmio = NULL;
-	u32 hc_irq_cause = 0;
+	void __iomem *mmio = hpriv->base, *hc_mmio;
	unsigned int handled = 0, port;

	for (port = 0; port < hpriv->n_ports; port++) {
		struct ata_port *ap = host->ports[port];
		struct mv_port_priv *pp;

- unsigned int shift, hardport, port_cause;
- /*
- * When we move to the second hc, flag our cached
- * copies of hc_mmio (and hc_irq_cause) as invalid again.
- */
- if (port == MV_PORTS_PER_HC)
- hc_mmio = NULL;
- /*
- * Do nothing if port is not interrupting or is disabled:
- */
+ unsigned int p, shift, hardport, port_cause;
+

		MV_PORT_TO_SHIFT_AND_HARDPORT(port, shift, hardport);
-		port_cause = (main_irq_cause >> shift) & (DONE_IRQ | ERR_IRQ);
-		if (!port_cause || !ap || (ap->flags & ATA_FLAG_DISABLED))
-			continue;
		/*

- * Each hc within the host has its own hc_irq_cause register.
- * We defer reading it until we know we need it, right now:
- *
- * FIXME later: we don't really need to read this register
- * (some logic changes required below if we go that way),
- * because it doesn't tell us anything new. But we do need
- * to write to it, outside the top of this loop,
- * to reset the interrupt triggers for next time.
+ * Each hc within the host has its own hc_irq_cause register,
+ * where the interrupting ports bits get ack'd.

		 */

- if (!hc_mmio) {
+ if (hardport == 0) { /* first port on this hc ? */
+ u32 hc_cause = (main_irq_cause >> shift) & HC0_IRQ_PEND;
+ u32 port_mask, ack_irqs;
+ /*
+ * Skip this entire hc if nothing pending for any ports
+ */
+ if (!hc_cause) {
+ port += MV_PORTS_PER_HC - 1;
+ continue;
+ }
+ /*
+ * We don't need/want to read the hc_irq_cause register,
+ * because doing so hurts performance, and
+ * main_irq_cause already gives us everything we need.
+ *
+ * But we do have to *write* to the hc_irq_cause to ack
+ * the ports that we are handling this time through.
+ *
+ * This requires that we create a bitmap for those
+ * ports which interrupted us, and use that bitmap
+ * to ack (only) those ports via hc_irq_cause.
+ */
+ ack_irqs = 0;
+ for (p = 0; p < MV_PORTS_PER_HC; ++p) {
+ if ((port + p) >= hpriv->n_ports)
+ break;
+ port_mask = (DONE_IRQ | ERR_IRQ) << (p * 2);
+ if (hc_cause & port_mask)
+ ack_irqs |= (DMA_IRQ | DEV_IRQ) << p;
+ }

			hc_mmio = mv_hc_base_from_port(mmio, port);
-			hc_irq_cause = readl(hc_mmio + HC_IRQ_CAUSE_OFS);
-			writelfl(~hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
+			writelfl(~ack_irqs, hc_mmio + HC_IRQ_CAUSE_OFS);
			handled = 1;
		}
+		port_cause = (main_irq_cause >> shift) & (DONE_IRQ | ERR_IRQ);
+		if (!port_cause)
+			continue;
		/*
		 * Process completed CRPB response(s) before other events.
		 */
		pp = ap->private_data;
-		if (hc_irq_cause & (DMA_IRQ << hardport)) {
+		if (port_cause & DONE_IRQ) {
			if (pp->pp_flags & MV_PP_FLAG_EDMA_EN)
				mv_process_crpb_entries(ap, pp);
		}
@@ -1868,15 +1881,15 @@
		 */
		if (unlikely(port_cause & ERR_IRQ)) {
			mv_err_intr(ap);
-		} else if (hc_irq_cause & (DEV_IRQ << hardport)) {
+		} else {
			if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN)) {
				struct ata_queued_cmd *qc = mv_get_active_qc(ap);
				if (qc) {
					ata_sff_host_intr(ap, qc);
					continue;
				}
+				mv_unexpected_intr(ap);
			}
-			mv_unexpected_intr(ap);
		}
	}
	return handled;

Separate out the inner loop body of mv_host_intr() into it's own function called mv_port_intr().

This should help maintainabilty.

---
Ignore the checkpatch.pl "braces {} are not necessary" warning for now. A subsequent patch installs more code within them.

@@ -1591,25 +1591,22 @@

	return qc;
}

-static void mv_unexpected_intr(struct ata_port *ap)
+static void mv_unexpected_intr(struct ata_port *ap, int edma_was_enabled) {

-	struct mv_port_priv *pp = ap->private_data;
	struct ata_eh_info *ehi = &ap->link.eh_info;

- char *when = "";
+ char *when = "idle";

- /*
- * We got a device interrupt from something that
- * was supposed to be using EDMA or polling.
- */

	ata_ehi_clear_desc(ehi);

- if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
- when = " while EDMA enabled";
+ if (!ap || (ap->flags & ATA_FLAG_DISABLED)) {
+ when = "disabled";
+ } else if (edma_was_enabled) {

+		when = "EDMA enabled";
	} else {
		struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->link.active_tag);
		if (qc && (qc->tf.flags & ATA_TFLAG_POLLING))
-			when = " while polling";
+			when = "polling";
	}
-	ata_ehi_push_desc(ehi, "unexpected device interrupt%s", when);
+	ata_ehi_push_desc(ehi, "unexpected device interrupt while %s", when);
	ehi->err_mask |= AC_ERR_OTHER;
	ehi->action   |= ATA_EH_RESET;
	ata_port_freeze(ap);
@@ -1807,6 +1804,42 @@
			 port_mmio + EDMA_RSP_Q_OUT_PTR_OFS);
}

+static void mv_port_intr(struct ata_port *ap, u32 port_cause) +{
+ struct mv_port_priv *pp;
+ int edma_was_enabled;
+
+ if (!ap || (ap->flags & ATA_FLAG_DISABLED)) {
+ mv_unexpected_intr(ap, 0);
+ return;
+ }
+ /*
+ * Grab a snapshot of the EDMA_EN flag setting,
+ * so that we have a consistent view for this port,
+ * even if something we call of our routines changes it.
+ */
+ pp = ap->private_data;
+ edma_was_enabled = (pp->pp_flags & MV_PP_FLAG_EDMA_EN);
+ /*
+ * Process completed CRPB response(s) before other events.
+ */
+ if (edma_was_enabled && (port_cause & DONE_IRQ)) {
+ mv_process_crpb_entries(ap, pp);
+ }
+ /*
+ * Handle chip-reported errors, or continue on to handle PIO.
+ */
+ if (unlikely(port_cause & ERR_IRQ)) {
+ mv_err_intr(ap);
+ } else if (!edma_was_enabled) {
+ struct ata_queued_cmd *qc = mv_get_active_qc(ap);
+ if (qc)
+ ata_sff_host_intr(ap, qc);
+ else
+ mv_unexpected_intr(ap, edma_was_enabled);
+ }
+}
+
/**
* mv_host_intr - Handle all interrupts on the given host controller
* @host: host specific structure
@@ -1823,7 +1856,6 @@

	for (port = 0; port < hpriv->n_ports; port++) {
		struct ata_port *ap = host->ports[port];
-		struct mv_port_priv *pp;
		unsigned int p, shift, hardport, port_cause;

		MV_PORT_TO_SHIFT_AND_HARDPORT(port, shift, hardport);
@@ -1865,32 +1897,12 @@
			writelfl(~ack_irqs, hc_mmio + HC_IRQ_CAUSE_OFS);
			handled = 1;
		}
-		port_cause = (main_irq_cause >> shift) & (DONE_IRQ | ERR_IRQ);
-		if (!port_cause)
-			continue;
		/*
-		 * Process completed CRPB response(s) before other events.
+		 * Handle interrupts signalled for this port:
		 */

- pp = ap->private_data;
- if (port_cause & DONE_IRQ) {
- if (pp->pp_flags & MV_PP_FLAG_EDMA_EN)
- mv_process_crpb_entries(ap, pp);
- }
- /*
- * Handle chip-reported errors, or continue on to handle PIO.
- */
- if (unlikely(port_cause & ERR_IRQ)) {
- mv_err_intr(ap);
- } else {
- if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN)) {
- struct ata_queued_cmd *qc = mv_get_active_qc(ap);
- if (qc) {
- ata_sff_host_intr(ap, qc);
- continue;
- }
- mv_unexpected_intr(ap);
- }
- }
+ port_cause = (main_irq_cause >> shift) & (DONE_IRQ | ERR_IRQ);
+ if (port_cause)

+			mv_port_intr(ap, port_cause);
	}
	return handled;
}

Export ata_eh_analyze_ncq_error() for subsequent use by sata_mv, as suggested by Tejun.

---
checkpatch.pl complains about the new EXPORT not following the function, but it is done here the same as for all other libata-eh functions.

@@ -6292,6 +6292,7 @@

EXPORT_SYMBOL_GPL(ata_eh_thaw_port);
EXPORT_SYMBOL_GPL(ata_eh_qc_complete);
EXPORT_SYMBOL_GPL(ata_eh_qc_retry);
+EXPORT_SYMBOL_GPL(ata_eh_analyze_ncq_error);
EXPORT_SYMBOL_GPL(ata_do_eh);
EXPORT_SYMBOL_GPL(ata_std_error_handler);

@@ -1357,7 +1357,7 @@
 *	LOCKING:
 *	Kernel thread context (may sleep).
 */
-static void ata_eh_analyze_ncq_error(struct ata_link *link)
+void ata_eh_analyze_ncq_error(struct ata_link *link)

{
	struct ata_port *ap = link->ap;
	struct ata_eh_context *ehc = &link->eh_context;

@@ -1039,6 +1039,7 @@
 

extern void ata_eh_qc_complete(struct ata_queued_cmd *qc); extern void ata_eh_qc_retry(struct ata_queued_cmd *qc); +extern void ata_eh_analyze_ncq_error(struct ata_link *link);

extern void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
ata_reset_fn_t softreset, ata_reset_fn_t hardreset,

Introduce a new "delayed error handling" mechanism in sata_mv, to enable us to eventually deal with multiple simultaneous NCQ failures on a single host link when a PM is present.

This involves a port flag (MV_PP_FLAG_DELAYED_EH) to prevent new commands being queued, and a pmp bitmap to indicate which pmp links had NCQ errors.

The new mv_pmp_error_handler() uses those values to invoke ata_eh_analyze_ncq_error() on each failed link, prior to freezing the port and passing control to sata_pmp_error_handler().

This is based upon a strategy suggested by Tejun.

For now, we just implement the delayed mechanism.
The next patch in this series will add the multiple-NCQ EH code to take advantage of it.

@@ -367,6 +367,7 @@

	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_FBS_EN	= (1 << 2),	/* is EDMA set up for FBS? */

+ MV_PP_FLAG_DELAYED_EH = (1 << 3), /* delayed dev err handling */
};

#define IS_GEN_I(hpriv) ((hpriv)->hp_flags & MV_HP_GEN_I) @@ -447,6 +448,7 @@

	unsigned int		resp_idx;

	u32			pp_flags;

+ unsigned int delayed_eh_pmp_map;
};

struct mv_port_signal {
@@ -542,6 +544,7 @@

				unsigned long deadline);
static int  mv_softreset(struct ata_link *link, unsigned int *class,
				unsigned long deadline);

+static void mv_pmp_error_handler(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
@@ -589,7 +592,7 @@
.pmp_hardreset = mv_pmp_hardreset,
.pmp_softreset = mv_softreset,
.softreset = mv_softreset,
- .error_handler = sata_pmp_error_handler,
+ .error_handler = mv_pmp_error_handler,
};

static struct ata_port_operations mv_iie_ops = {
@@ -1098,6 +1101,12 @@

	struct mv_port_priv *pp = ap->private_data;

	/*

+ * Don't allow new commands if we're in a delayed EH state
+ * for NCQ and/or FIS-based switching.
+ */
+ if (pp->pp_flags & MV_PP_FLAG_DELAYED_EH)
+ return ATA_DEFER_PORT;
+ /*

	 * If the port is completely idle, then allow the new qc.
	 */
	if (ap->nr_active_links == 0)
@@ -1591,6 +1600,33 @@
	return qc;
}

+static void mv_pmp_error_handler(struct ata_port *ap)
+{
+ unsigned int pmp, pmp_map;
+ struct mv_port_priv *pp = ap->private_data;
+
+ if (pp->pp_flags & MV_PP_FLAG_DELAYED_EH) {
+ /*
+ * Perform NCQ error analysis on failed PMPs
+ * before we freeze the port entirely.
+ *
+ * The failed PMPs are marked earlier by mv_pmp_eh_prep().
+ */
+ pmp_map = pp->delayed_eh_pmp_map;
+ pp->pp_flags &= ~MV_PP_FLAG_DELAYED_EH;
+ for (pmp = 0; pmp_map != 0; pmp++) {
+ unsigned int this_pmp = (1 << pmp);
+ if (pmp_map & this_pmp) {
+ struct ata_link *link = &ap->pmp_link[pmp];
+ pmp_map &= ~this_pmp;
+ ata_eh_analyze_ncq_error(link);
+ }
+ }
+ ata_port_freeze(ap);
+ }
+ sata_pmp_error_handler(ap);
+}
+
static void mv_unexpected_intr(struct ata_port *ap, int edma_was_enabled)

{
	struct ata_eh_info *ehi = &ap->link.eh_info;

Convert sata_mv's EH for FIS-based switching (FBS) over to the sequence recommended by Marvell. This enables us to catch/analyze multiple failed links on a port-multiplier when using NCQ.

To do this, we clear the ERR_DEV bit in the EDMA Halt-Conditions register, so that the EDMA engine doesn't self-disable on the first NCQ error.

Our EH code sets the MV_PP_FLAG_DELAYED_EH flag to prevent new commands being queued while we await completion of all outstanding NCQ commands on all links of the failed PM.

The SATA Test Control register tells us which links have failed, so we must only wait for any other active links to finish up before we stop the EDMA and run the .error_handler afterward.

The patch also includes skeleton code for handling of non-NCQ FBS operation. This is more for documentation purposes right now, as that mode is not yet enabled in sata_mv.

@@ -545,6 +545,8 @@

static int mv_softreset(struct ata_link *link, unsigned int *class,

				unsigned long deadline);

static void mv_pmp_error_handler(struct ata_port *ap); +static void mv_process_crpb_entries(struct ata_port *ap,
+ struct mv_port_priv *pp);

/* .sg_tablesize is (MV_MAX_SG_CT / 2) in the structures below
* because we have to allow room for worst case splitting of
@@ -1156,6 +1158,10 @@

	if (want_fbs) {
		new_fiscfg = old_fiscfg | FISCFG_SINGLE_SYNC;
		new_ltmode = old_ltmode | LTMODE_BIT8;
+		if (want_ncq)
+			new_haltcond &= ~EDMA_ERR_DEV;
+		else
+			new_fiscfg |=  FISCFG_WAIT_DEV_ERR;
	}

	if (new_fiscfg != old_fiscfg)
@@ -1627,6 +1633,154 @@
	sata_pmp_error_handler(ap);
}

+static unsigned int mv_get_err_pmp_map(struct ata_port *ap) +{
+ void __iomem *port_mmio = mv_ap_base(ap);
+
+ return readl(port_mmio + SATA_TESTCTL_OFS) >> 16;
+}
+
+static int mv_count_pmp_links(unsigned int pmp_map)
+{
+ unsigned int link_count = 0;
+
+ while (pmp_map) {
+ link_count += (pmp_map & 1);
+ pmp_map >>= 1;
+ }
+ return link_count;
+}
+
+static void mv_pmp_eh_prep(struct ata_port *ap, unsigned int pmp_map) +{
+ struct ata_eh_info *ehi;
+ unsigned int pmp;
+
+ /*
+ * Initialize EH info for PMPs which saw device errors
+ */
+ ehi = &ap->link.eh_info;
+ for (pmp = 0; pmp_map != 0; pmp++) {
+ unsigned int this_pmp = (1 << pmp);
+ if (pmp_map & this_pmp) {
+ struct ata_link *link = &ap->pmp_link[pmp];
+
+ pmp_map &= ~this_pmp;
+ ehi = &link->eh_info;
+ ata_ehi_clear_desc(ehi);
+ ata_ehi_push_desc(ehi, "dev err");
+ ehi->err_mask |= AC_ERR_DEV;
+ ehi->action |= ATA_EH_RESET;
+ ata_link_abort(link);
+ }
+ }
+}
+
+static int mv_handle_fbs_ncq_dev_err(struct ata_port *ap)
+{
+ struct mv_port_priv *pp = ap->private_data;
+ int failed_links;
+ unsigned int old_map, new_map;
+
+ /*
+ * Device error during FBS+NCQ operation:
+ *
+ * Set a port flag to prevent further I/O being enqueued.
+ * Leave the EDMA running to drain outstanding commands from this port.
+ * Perform the post-mortem/EH only when all responses are complete.
+ * Follow recovery sequence from 6042/7042 datasheet (7.3.15.4.2.2).
+ */
+ if (!(pp->pp_flags & MV_PP_FLAG_DELAYED_EH)) {
+ pp->pp_flags |= MV_PP_FLAG_DELAYED_EH;
+ pp->delayed_eh_pmp_map = 0;
+ }
+ old_map = pp->delayed_eh_pmp_map;
+ new_map = old_map | mv_get_err_pmp_map(ap);
+
+ if (old_map != new_map) {
+ pp->delayed_eh_pmp_map = new_map;
+ mv_pmp_eh_prep(ap, new_map & ~old_map);
+ }
+ failed_links = mv_count_pmp_links(new_map);
+
+ ata_port_printk(ap, KERN_INFO, "%s: pmp_map=%04x qc_map=%04x "
+ "failed_links=%d nr_active_links=%d\n",
+ __func__, pp->delayed_eh_pmp_map,
+ ap->qc_active, failed_links,
+ ap->nr_active_links);
+
+ if (ap->nr_active_links <= failed_links) {
+ mv_process_crpb_entries(ap, pp);
+ mv_stop_edma(ap);
+ mv_eh_freeze(ap);
+ ata_port_printk(ap, KERN_INFO, "%s: done\n", __func__);
+ return 1; /* handled */
+ }
+ ata_port_printk(ap, KERN_INFO, "%s: waiting\n", __func__);
+ return 1; /* handled */
+}
+
+static int mv_handle_fbs_non_ncq_dev_err(struct ata_port *ap) +{
+ /*
+ * Possible future enhancement:
+ *
+ * FBS+non-NCQ operation is not yet implemented.
+ * See related notes in mv_edma_cfg().
+ *
+ * Device error during FBS+non-NCQ operation:
+ *
+ * We need to snapshot the shadow registers for each failed command.
+ * Follow recovery sequence from 6042/7042 datasheet (7.3.15.4.2.3).
+ */
+ return 0; /* not handled */
+}
+
+static int mv_handle_dev_err(struct ata_port *ap, u32 edma_err_cause) +{
+ struct mv_port_priv *pp = ap->private_data;
+
+ if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN))
+ return 0; /* EDMA was not active: not handled */
+ if (!(pp->pp_flags & MV_PP_FLAG_FBS_EN))
+ return 0; /* FBS was not active: not handled */
+
+ if (!(edma_err_cause & EDMA_ERR_DEV))
+ return 0; /* non DEV error: not handled */
+ edma_err_cause &= ~EDMA_ERR_IRQ_TRANSIENT;
+ if (edma_err_cause & ~(EDMA_ERR_DEV | EDMA_ERR_SELF_DIS))
+ return 0; /* other problems: not handled */
+
+ if (pp->pp_flags & MV_PP_FLAG_NCQ_EN) {
+ /*
+ * EDMA should NOT have self-disabled for this case.
+ * If it did, then something is wrong elsewhere,
+ * and we cannot handle it here.
+ */
+ if (edma_err_cause & EDMA_ERR_SELF_DIS) {
+ ata_port_printk(ap, KERN_WARNING,
+ "%s: err_cause=0x%x pp_flags=0x%x\n",
+ __func__, edma_err_cause, pp->pp_flags);
+ return 0; /* not handled */
+ }
+ return mv_handle_fbs_ncq_dev_err(ap);
+ } else {
+ /*
+ * EDMA should have self-disabled for this case.
+ * If it did not, then something is wrong elsewhere,
+ * and we cannot handle it here.
+ */
+ if (!(edma_err_cause & EDMA_ERR_SELF_DIS)) {
+ ata_port_printk(ap, KERN_WARNING,
+ "%s: err_cause=0x%x pp_flags=0x%x\n",
+ __func__, edma_err_cause, pp->pp_flags);
+ return 0; /* not handled */
+ }
+ return mv_handle_fbs_non_ncq_dev_err(ap);
+ }
+ return 0; /* not handled */
+}
+
static void mv_unexpected_intr(struct ata_port *ap, int edma_was_enabled)

{
	struct ata_eh_info *ehi = &ap->link.eh_info;
@@ -1683,6 +1837,15 @@
	ata_port_printk(ap, KERN_INFO, "%s: err_cause=%08x pp_flags=0x%x\n",
			__func__, edma_err_cause, pp->pp_flags);

+ if (edma_err_cause & EDMA_ERR_DEV) {
+ /*
+ * Device errors during FIS-based switching operation
+ * require special handling.
+ */
+ if (mv_handle_dev_err(ap, edma_err_cause))
+ return;
+ }
+

	qc = mv_get_active_qc(ap);
	ata_ehi_clear_desc(ehi);
	ata_ehi_push_desc(ehi, "edma_err_cause=%08x pp_flags=%08x",
@@ -1861,6 +2024,8 @@
	 */
	if (edma_was_enabled && (port_cause & DONE_IRQ)) {
		mv_process_crpb_entries(ap, pp);
+		if (pp->pp_flags & MV_PP_FLAG_DELAYED_EH)
+			mv_handle_fbs_ncq_dev_err(ap);
	}
	/*
	 * Handle chip-reported errors, or continue on to handle PIO.

By the way. If anyone can point out an existing EXPORTed "count number of one bits" in a word function, then I'll very happily submit a followup patch to remove this (below) and use that instead.

..

include/asm-ia64/bitops.h:hweight64()

grundler <2186>fgrep hweight include/asm/* include/asm/bitops_32.h:#include <asm-generic/bitops/hweight.h> include/asm/bitops_64.h:#include <asm-generic/bitops/hweight.h>

yup.

hth,
grant

On Fri, May 2, 2008 at 9:52 AM, Grant Grundler <[email protected]> wrote:
...

Sorry - forgot to include this part:

grundler <2189>fgrep hweight64 lib/* Binary file lib/bitmap.o matches
Binary file lib/built-in.o matches lib/hweight.c:unsigned long hweight64(__u64 w) lib/hweight.c:EXPORT_SYMBOL(hweight64); Binary file lib/hweight.o matches

thanks,
grant

..

"hweight" ?? That's a nice "obvious" name for "count number of set bits". :)

I'll post a follow-up patch to convert sata_mv to use hweight16() .

Thanks

It is if you ever do any signal processing work ;)

H for Hamming.

Alan

Some tidying as suggested by Grant Grundler.

Nuke local bit-counting function from sata_mv in favour of using hweight16(). Also add a short explanation for the 15msec timeout used when waiting for empty/idle.

---
This makes 13 patches now, rather than just 12.

@@ -903,6 +903,10 @@
 

	/*
	 * Wait for the EDMA engine to finish transactions in progress.

+ * No idea what a good "timeout" value might be, but measurements
+ * indicate that it often requires hundreds of microseconds
+ * with two drives in-use. So we use the 15msec value above
+ * as a rough guess at what even more drives might require.

	 */
	for (i = 0; i < timeout; ++i) {
		u32 edma_stat = readl(port_mmio + EDMA_STATUS_OFS);
@@ -1640,17 +1644,6 @@
	return readl(port_mmio + SATA_TESTCTL_OFS) >> 16;
}

-static int mv_count_pmp_links(unsigned int pmp_map)
-{
- unsigned int link_count = 0;
-
- while (pmp_map) {
- link_count += (pmp_map & 1);
- pmp_map >>= 1;
- }
- return link_count;
-}
-
static void mv_pmp_eh_prep(struct ata_port *ap, unsigned int pmp_map)

{
	struct ata_eh_info *ehi;
@@ -1701,7 +1694,7 @@
		pp->delayed_eh_pmp_map = new_map;
		mv_pmp_eh_prep(ap, new_map & ~old_map);
	}

- failed_links = mv_count_pmp_links(new_map);
+ failed_links = hweight16(new_map);

	ata_port_printk(ap, KERN_INFO, "%s: pmp_map=%04x qc_map=%04x "
			"failed_links=%d nr_active_links=%d\n",

...

NAK that. I'll repost a V2 of this patch shortly, with an #include <linux/bitops.h> for cross-platform safety.

Cheers

Some tidying as suggested by Grant Grundler.

Nuke local bit-counting function from sata_mv in favour of using hweight16(). Also add a short explanation for the 15msec timeout used when waiting for empty/idle.

---
Respin/repost of original patch 13: added #include <linux/bitops.h> for safety.

@@ -65,6 +65,7 @@

#include <linux/platform_device.h>
#include <linux/ata_platform.h>
#include <linux/mbus.h>
+#include <linux/bitops.h>
#include <scsi/scsi_host.h>
#include <scsi/scsi_cmnd.h>
#include <scsi/scsi_device.h>
@@ -903,6 +904,10 @@

	/*
	 * Wait for the EDMA engine to finish transactions in progress.

+ * No idea what a good "timeout" value might be, but measurements
+ * indicate that it often requires hundreds of microseconds
+ * with two drives in-use. So we use the 15msec value above
+ * as a rough guess at what even more drives might require.

	 */
	for (i = 0; i < timeout; ++i) {
		u32 edma_stat = readl(port_mmio + EDMA_STATUS_OFS);
@@ -1640,17 +1645,6 @@
	return readl(port_mmio + SATA_TESTCTL_OFS) >> 16;
}

-static int mv_count_pmp_links(unsigned int pmp_map)
-{
- unsigned int link_count = 0;
-
- while (pmp_map) {
- link_count += (pmp_map & 1);
- pmp_map >>= 1;
- }
- return link_count;
-}
-
static void mv_pmp_eh_prep(struct ata_port *ap, unsigned int pmp_map)

{
	struct ata_eh_info *ehi;
@@ -1701,7 +1695,7 @@
		pp->delayed_eh_pmp_map = new_map;
		mv_pmp_eh_prep(ap, new_map & ~old_map);
	}

- failed_links = mv_count_pmp_links(new_map);
+ failed_links = hweight16(new_map);

	ata_port_printk(ap, KERN_INFO, "%s: pmp_map=%04x qc_map=%04x "
			"failed_links=%d nr_active_links=%d\n",

Anything related to EH can do as many MMIO reads as necessary. I'm just not going to worry about it since once we've entered EH, any latency "guarantees" are already lost.

I reccomend setting per_loop to 10 or 20 at least. 5us is not very long. It takes about 1us to do an MMIO read. Each additional MMIO read will exacerbate the problem of DMA not finishing since MMIO reads interfere with DMA streams.

Also can you add a comment why 15ms is right amount of time to wait?

.e.g. 4 ports/SATAHC w/31 (NCQ) IOs @ 128KB == ~15MB of transfer time. PCI-e 4X can transfer 1MB/s (roughly).
So this wouldn't be true for PCI-X devices running at 66Mhz. Or if the ports are only running at 1.5Gb/s, etc.

BTW, I have no idea when EDMA stops on it's own or if we could safely get EDMA to stop "prematurely" (e.g. disable PCI Bus Master).

hth,
grant

..

..

I would, if I knew. That's just a random number. My own timings with a handful of drives here indicated that it takes a few hundred microseconds worst case, so I just picked a bigger number to start with.

No rationale == no comment in the original.
But if you have a good theory for what that number ought to be, then go for it -- just patch it and stick an explanation in.

Thanks.

..

I don't know what you're talking about there.

???

...

Ok. Often there is a rationale and the original author just didn't include it.

I'd need to study the EDMA engine docs more carefully than I have to date.

I don't either. :)
Just trying to explain I was making up the example.

thanks,
grant

..

..

Well, when we hit the disable bit, it stops fairly quickly. The (existing) code still polls it there, but generally it does stop without much fuss.

Another way to stop it is to just go and hit EDMA_RESET (ATA_RST), which immediately kills everything. The datasheet recommends against it, but doesn't say why (disruption to packets in flight and possibly messing up drive write cache and the like).

But the current code now tries for a more graceful shutdown, with firm time limits.

I particularly like the new NCQ error-handling for FBS, which allows activity on the non-failed links to quiesce before beginning the EH/reset/recovery sequence. Much less disruptive to the non-failing drives that way.

There's still an errata or two with some of the chips that may prevent the full EH from working there sometimes. The READ_LOG_EXT_10H may fail, meaning we might not report the failed LBA number back up the stack. With a bit of luck, we'll tack that down once we learn the workaround.

Cheers

Mark,
In order to enable cut-thru, the docs say "MRdTrig" bit in PCI Command Register ()xC00) must also be clear on 6042. This _might_ also apply to 7042. I didn't see any code in this patch (Perhaps a different one?) to make sure "MRdTrig" bit is clear.

hth,
grant

Grant Grundler wrote:
..

..

Were you looking for this, from the same patch #02:

..

..

And, no, this does not apply to the 7042 (PCI Express version).

Cheers

Yup - exactly. Sorry, I missed that.

I was expecting MRDTRIG to be cleared if the cut-through was going to be enabled. Instead, MRDTRIG is a gating factor. I don't know if you want to (or can) change that though.

Yes - seems obvious from the code once it's pointed out.

thanks,
grant

bleh. from the department of redundant redundancies :) Those types of constants are usually either register bits or register offsets, and from
reading the code it is always clear which is which. IMO, _OFS is
redundant.

That's in the realm of maintainer discretion, though, so I applied the patch and merely request that you take my comment under advisement

..

Heh.. that's actually my opinion, too.

But the original authors seemed to use _OFS in many places, so I'm just trying to create some consistency from it all. ;)

Cheers

applied 1-13