sata_mv: fix 8-port timeouts on 508x/6081 chips

Fix a longstanding bug for the 8-port Marvell Sata controllers (508x/6081), where accesses to the upper 4 ports would cause lost-interrupts / timeouts for the lower 4-ports. With this patch, the 6081 boards should finally be reliable enough for mainstream use with Linux.

This patch is for 2.6.29, but should also get reviewed/released for the -stable branches of 2.6.28, 2.6.27, and 2.6.26.

Reposting, as I got no response/acknowledgement on the first attempt.

@@ -883,7 +883,7 @@

		struct mv_host_priv *hpriv = ap->host->private_data;
		int hardport = mv_hardport_from_port(ap->port_no);
		void __iomem *hc_mmio = mv_hc_base_from_port(
-					mv_host_base(ap->host), hardport);
+					mv_host_base(ap->host), ap->port_no);
		u32 hc_irq_cause, ipending;

		/* clear EDMA event indicators, if any */

Remove silly read-modify-write sequences when clearing interrupts in hc_irq_cause. This gets rid of unneeded MMIO reads, resulting in a slight performance boost when switching between EDMA and non-EDMA modes (eg. for cache flushes).

@@ -35,8 +35,6 @@
 *
 * --> Investigate problems with PCI Message Signalled Interrupts (MSI).
 *
- * --> Cache frequently-accessed registers in mv_port_priv to reduce overhead.
- *
 * --> Develop a low-power-consumption strategy, and implement it.
 *
 * --> [Experiment, low priority] Investigate interrupt coalescing.

@@ -884,18 +882,14 @@

		int hardport = mv_hardport_from_port(ap->port_no);
		void __iomem *hc_mmio = mv_hc_base_from_port(
					mv_host_base(ap->host), ap->port_no);

- u32 hc_irq_cause, ipending;
+ u32 hc_irq_cause;

		/* 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 | DMA_IRQ) << hardport;
- if (hc_irq_cause & ipending) {
- writelfl(hc_irq_cause & ~ipending,
- hc_mmio + HC_IRQ_CAUSE_OFS);
- }
+ /* clear pending irq events */
+ hc_irq_cause = ~((DEV_IRQ | DMA_IRQ) << hardport);
+ writelfl(hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);

		mv_edma_cfg(ap, want_ncq);

@@ -2821,8 +2815,7 @@

	writel(0, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);

	/* clear pending irq events */
-	hc_irq_cause = readl(hc_mmio + HC_IRQ_CAUSE_OFS);
-	hc_irq_cause &= ~((DEV_IRQ | DMA_IRQ) << hardport);
+	hc_irq_cause = ~((DEV_IRQ | DMA_IRQ) << hardport);
	writelfl(hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);

	mv_enable_port_irqs(ap, ERR_IRQ);

Remove unneeded nsect restriction from GenII NCQ path, and improve comments to explain why this is not a problem.

@@ -349,8 +349,6 @@
 

	EDMA_HALTCOND_OFS	= 0x60,		/* GenIIe halt conditions */

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

	/* Host private flags (hp_flags) */
	MV_HP_FLAG_MSI		= (1 << 0),
	MV_HP_ERRATA_50XXB0	= (1 << 1),

@@ -1093,20 +1091,12 @@
*
* Gen-II does not support NCQ over a port multiplier
* (no FIS-based switching).
- *
- * 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 (sata_pmp_attached(adev->link->ap)) {
			adev->flags &= ~ATA_DFLAG_NCQ;
			ata_dev_printk(adev, KERN_INFO,
				"NCQ disabled for command-based switching\n");

- } else if (adev->max_sectors > GEN_II_NCQ_MAX_SECTORS) {
- adev->max_sectors = GEN_II_NCQ_MAX_SECTORS;
- ata_dev_printk(adev, KERN_INFO,
- "max_sectors limited to %u for NCQ\n",

-				adev->max_sectors);
		}
	}
}

@@ -1444,7 +1434,8 @@
* only 11 bytes...so we must pick and choose required
* registers based on the command. So, we drop feature and
* hob_feature for [RW] DMA commands, but they are needed for
- * NCQ. NCQ will drop hob_nsect.
+ * NCQ. NCQ will drop hob_nsect, which is not needed there
+ * (nsect is used only for the tag; feat/hob_feat hold true nsect).

	 */
	switch (tf->command) {
	case ATA_CMD_READ:

Enable reliable use of Message Switched Interrupts (MSI) in sata_mv by masking further chip interrupts within the main interrupt handler. Based upon a suggestion by Grant Grundler.

@@ -33,8 +33,6 @@
 *
 * --> ATAPI support (Marvell claims the 60xx/70xx chips can do it).
 *
- * --> Investigate problems with PCI Message Signalled Interrupts (MSI).
- *
 * --> Develop a low-power-consumption strategy, and implement it.
 *
 * --> [Experiment, low priority] Investigate interrupt coalescing.

@@ -2199,9 +2197,15 @@

	struct ata_host *host = dev_instance;
	struct mv_host_priv *hpriv = host->private_data;
	unsigned int handled = 0;
+	int using_msi = hpriv->hp_flags & MV_HP_FLAG_MSI;
	u32 main_irq_cause, pending_irqs;

	spin_lock(&host->lock);

+
+ /* for MSI: block new interrupts while in here */
+ if (using_msi)
+ writel(0, hpriv->main_irq_mask_addr);
+

	main_irq_cause = readl(hpriv->main_irq_cause_addr);
	pending_irqs   = main_irq_cause & hpriv->main_irq_mask;
	/*
@@ -2215,6 +2219,11 @@
			handled = mv_host_intr(host, pending_irqs);
	}
	spin_unlock(&host->lock);

+
+ /* for MSI: unblock interupts; anything pending will hit us now */
+ if (using_msi)
+ writel(hpriv->main_irq_mask, hpriv->main_irq_mask_addr);
+

	return IRQ_RETVAL(handled);
}

Bug fix and cleanup for MSI support in sata_mv:

1. Set MV_HP_FLAG_MSI when MSI is enabled.
2. Remove redundant call of pci_intx().
3. Ensure that pci_disable_msi() is called on device removal.

Tested and working with Marvell 7042 PCIe hosts, though off by default. Use of MSI still requires "msi=1" modparm for sata_mv.

@@ -3276,13 +3276,14 @@

#ifdef CONFIG_PCI
static int mv_pci_init_one(struct pci_dev *pdev,

			   const struct pci_device_id *ent);

+static void mv_pci_remove_one(struct pci_dev *pdev);

static struct pci_driver mv_pci_driver = {
	.name			= DRV_NAME,
	.id_table		= mv_pci_tbl,
	.probe			= mv_pci_init_one,

- .remove = ata_pci_remove_one,
+ .remove = mv_pci_remove_one,
};

/*
@@ -3367,6 +3368,26 @@
}

/**
+ * mv_pci_remove_one - ensure MSI gets disabled on device removal
+ * @pdev: PCI device
+ *
+ * LOCKING:
+ * Inherited from caller.
+ */
+static void mv_pci_remove_one(struct pci_dev *pdev) +{
+ struct device *dev = &pdev->dev;
+ struct ata_host *host = dev_get_drvdata(dev);
+ struct mv_host_priv *hpriv = host->private_data;
+
+ if (hpriv->hp_flags & MV_HP_FLAG_MSI) {
+ devm_free_irq(host->dev, pdev->irq, host);
+ pci_disable_msi(pdev);
+ }
+ ata_pci_remove_one(pdev);
+}
+
+/**

 *      mv_pci_init_one - handle a positive probe of a PCI Marvell host
 *      @pdev: PCI device found
 *      @ent: PCI device ID entry for the matched host
@@ -3424,8 +3445,8 @@
		return rc;

	/* Enable interrupts */

- if (msi && pci_enable_msi(pdev))
- pci_intx(pdev, 1);
+ if (msi && 0 == pci_enable_msi(pdev))
+ hpriv->hp_flags |= MV_HP_FLAG_MSI;

	mv_dump_pci_cfg(pdev, 0x68);
	mv_print_info(host);

Bump version number on sata_mv, update list of chips supported, and (finally!) remove the "EXPERIMENTAL" designation.

@@ -68,7 +68,7 @@

#include <linux/libata.h>

#define DRV_NAME "sata_mv"
-#define DRV_VERSION "1.24"
+#define DRV_VERSION "1.25"

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

@@ -112,11 +112,11 @@

If unsure, say N.

config SATA_MV
- tristate "Marvell SATA support (HIGHLY EXPERIMENTAL)"
- depends on EXPERIMENTAL
+ tristate "Marvell SATA support"
help
This option enables support for the Marvell Serial ATA family.

-	  Currently supports 88SX[56]0[48][01] chips.
+	  Currently supports 88SX[56]0[48][01] PCI(-X) chips,
+	  as well as the newer [67]042 PCI/PCIe and SOC devices.

If unsure, say N.

This is a HW bug workaround so we don't end up with completions pending but no interrupts pending.

Please add:

    /* Unmasking any Cause bit which is set will generate an interrupt. */

Please drop patches 04,05,06 from this series.
These will be reissued as (v2) patches shortly.

Thanks.

Enable reliable use of Message-Signaled Interrupts (MSI) in sata_mv by masking further chip interrupts within the main interrupt handler.

Based upon a suggestion by Grant Grundler.
MSI is working reliably in all of my test systems here now.

---

This replaces the originally posted [PATCH 05/04] version.

@@ -33,8 +33,6 @@
 *
 * --> ATAPI support (Marvell claims the 60xx/70xx chips can do it).
 *
- * --> Investigate problems with PCI Message Signalled Interrupts (MSI).
- *
 * --> Develop a low-power-consumption strategy, and implement it.
 *
 * --> [Experiment, low priority] Investigate interrupt coalescing.

@@ -70,7 +68,7 @@

#include <linux/libata.h>

#define DRV_NAME "sata_mv"
-#define DRV_VERSION "1.24"
+#define DRV_VERSION "1.25"

enum {
	/* BAR's are enumerated in terms of pci_resource_start() terms */
@@ -2199,9 +2197,15 @@
	struct ata_host *host = dev_instance;
	struct mv_host_priv *hpriv = host->private_data;
	unsigned int handled = 0;
+	int using_msi = hpriv->hp_flags & MV_HP_FLAG_MSI;
	u32 main_irq_cause, pending_irqs;

	spin_lock(&host->lock);

+
+ /* for MSI: block new interrupts while in here */
+ if (using_msi)
+ writel(0, hpriv->main_irq_mask_addr);
+

	main_irq_cause = readl(hpriv->main_irq_cause_addr);
	pending_irqs   = main_irq_cause & hpriv->main_irq_mask;
	/*
@@ -2215,6 +2219,11 @@
			handled = mv_host_intr(host, pending_irqs);
	}
	spin_unlock(&host->lock);

+
+ /* for MSI: unmask; interrupt cause bits will retrigger now */
+ if (using_msi)
+ writel(hpriv->main_irq_mask, hpriv->main_irq_mask_addr);
+

	return IRQ_RETVAL(handled);
}

@@ -3414,9 +3423,9 @@

	if (rc)
		return rc;

- /* Enable interrupts */
- if (msi && pci_enable_msi(pdev))
- pci_intx(pdev, 1);
+ /* Enable message-switched interrupts, if requested */
+ if (msi && 0 == pci_enable_msi(pdev))
+ hpriv->hp_flags |= MV_HP_FLAG_MSI;

	mv_dump_pci_cfg(pdev, 0x68);
	mv_print_info(host);

Update Kconfig for sata_mv with full list of chips supported, and (finally!) remove the "EXPERIMENTAL" designations.

---

This replaces the patch "[PATCH 06/06] sata_mv: remove experimental".

@@ -112,11 +112,11 @@

If unsure, say N.

config SATA_MV
- tristate "Marvell SATA support (HIGHLY EXPERIMENTAL)"
- depends on EXPERIMENTAL
+ tristate "Marvell SATA support"
help
This option enables support for the Marvell Serial ATA family.

-	  Currently supports 88SX[56]0[48][01] chips.
+	  Currently supports 88SX[56]0[48][01] PCI(-X) chips,
+	  as well as the newer [67]042 PCI-X/PCIe and SOC devices.

If unsure, say N.

Excellent.

thanks,
grant

Hello, Mark.

Generally looks good, but...

tejun

..

..

Heh.. Myself, I would normally just write it this way:

    if (msi && !pci_enable_msi(pdev))

But the "coding correctness police" might point out that it reads funny (negatives and all that).

But whatever way Jeff likes it, really.

-ml

..

.. perhaps keeping in mind that every little nit-picky revision like that requires me to go back to the deities at Marvell for re-approval before I can again present it for your blessings here.

Cheers

Hey, we can con the system then by you submitting the original patch and me following up with a nit picking patch right away, so if changing the patch is too much of a hassle, please just go ahead with the current patch. :-)

Thanks.

tejun

Tejun / Alan,

I'm now venturing into the labyrinth that is presented when one tries to combine a nice FIS-based controller/driver with old school BMDMA for ATAPI only.

To preserve my own sanity, I'm opting for basic PIO-only support for ATAPI to begin with. Then I'll add the DMA functions to speed things up afterward.

This patch (below) seems to work 100% okay for me here, apart from the sata_mv chipset errata warnings it generates (another thing I hope to tidy up soon-ish).

Question: is this the "right" way to force PIO-only, or is there a better mechanism that I'm missing.

It just seems.. too easy this way. :)

Thanks.

* * *

Add basic ATAPI support to sata_mv using PIO + polling. DMA support will be added later.

@@ -120,7 +120,7 @@

	MV_FLAG_IRQ_COALESCE	= (1 << 29),  /* IRQ coalescing capability */

	MV_COMMON_FLAGS		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_NO_ATAPI |
+				  ATA_FLAG_MMIO |
				  ATA_FLAG_PIO_POLLING,

	MV_6XXX_FLAGS		= MV_FLAG_IRQ_COALESCE,

@@ -502,6 +502,7 @@
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 void mv_dev_config(struct ata_device *dev);

static void mv5_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio,

			   unsigned int port);

@@ -576,6 +577,7 @@
.error_handler = ata_std_error_handler, /* avoid SFF EH */
.post_internal_cmd = ATA_OP_NULL,

+ .dev_config = mv_dev_config,

	.scr_read		= mv5_scr_read,
	.scr_write		= mv5_scr_write,

@@ -597,7 +599,7 @@

static struct ata_port_operations mv_iie_ops = {
	.inherits		= &mv6_ops,
-	.dev_config		= ATA_OP_NULL,
+	.dev_config             = mv_dev_config,
	.qc_prep		= mv_qc_prep_iie,
};

@@ -1082,6 +1084,21 @@

		return -EINVAL;
}

+static void mv_dev_config(struct ata_device *adev) +{
+ /*
+ * Support ATAPI devices via PIO-only for now.
+ * DMA support is planned for later.
+ */
+ if (adev->class == ATA_DEV_ATAPI) {
+ adev->xfer_shift = ATA_SHIFT_PIO;
+ adev->flags |= ATA_DFLAG_PIO;
+ ata_dev_printk(adev, KERN_INFO, DRV_NAME
+ ": ATAPI DMA not yet implemented;"
+ " using (slow) PIO for now\n");
+ }
+}
+
static void mv6_dev_config(struct ata_device *adev)

{
	/*
@@ -1097,6 +1114,7 @@
				"NCQ disabled for command-based switching\n");
		}
	}

+ mv_dev_config(adev);
}

static int mv_qc_defer(struct ata_queued_cmd *qc)

(resending, with different email address for Alan).

Tejun / Alan,

I'm now venturing into the labyrinth that is presented when one tries to combine a nice FIS-based controller/driver with old school BMDMA for ATAPI only.

To preserve my own sanity, I'm opting for basic PIO-only support for ATAPI to begin with. Then I'll add the DMA functions to speed things up afterward.

This patch (below) seems to work 100% okay for me here, apart from the sata_mv chipset errata warnings it generates (another thing I hope to tidy up soon-ish).

Question: is this the "right" way to force PIO-only, or is there a better mechanism that I'm missing.

It just seems.. too easy this way. :)

Thanks.

* * *

Add basic ATAPI support to sata_mv using PIO + polling. DMA support will be added later.

@@ -120,7 +120,7 @@

	MV_FLAG_IRQ_COALESCE	= (1 << 29),  /* IRQ coalescing capability */

	MV_COMMON_FLAGS		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_NO_ATAPI |
+				  ATA_FLAG_MMIO |
				  ATA_FLAG_PIO_POLLING,

	MV_6XXX_FLAGS		= MV_FLAG_IRQ_COALESCE,

@@ -502,6 +502,7 @@
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 void mv_dev_config(struct ata_device *dev);

static void mv5_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio,

			   unsigned int port);

@@ -576,6 +577,7 @@
.error_handler = ata_std_error_handler, /* avoid SFF EH */
.post_internal_cmd = ATA_OP_NULL,

+ .dev_config = mv_dev_config,

	.scr_read		= mv5_scr_read,
	.scr_write		= mv5_scr_write,

@@ -597,7 +599,7 @@

static struct ata_port_operations mv_iie_ops = {
	.inherits		= &mv6_ops,
-	.dev_config		= ATA_OP_NULL,
+	.dev_config             = mv_dev_config,
	.qc_prep		= mv_qc_prep_iie,
};

@@ -1082,6 +1084,21 @@

		return -EINVAL;
}

+static void mv_dev_config(struct ata_device *adev) +{
+ /*
+ * Support ATAPI devices via PIO-only for now.
+ * DMA support is planned for later.
+ */
+ if (adev->class == ATA_DEV_ATAPI) {
+ adev->xfer_shift = ATA_SHIFT_PIO;
+ adev->flags |= ATA_DFLAG_PIO;
+ ata_dev_printk(adev, KERN_INFO, DRV_NAME
+ ": ATAPI DMA not yet implemented;"
+ " using (slow) PIO for now\n");
+ }
+}
+
static void mv6_dev_config(struct ata_device *adev)

{
	/*
@@ -1097,6 +1114,7 @@
				"NCQ disabled for command-based switching\n");
		}
	}

+ mv_dev_config(adev);
}

static int mv_qc_defer(struct ata_queued_cmd *qc)

If you want to force ATAPI into PIO modes only then use the ->mode_filter() functionality to drop out the non PIO modes. See pata_hpt37x.c for an example where it does it for some drives only

If you want to just force some types of command into using PIO (eg by direction, length etc) then use ->check_atapi_dma(qc) - see pata_it821x for a worked example.

Alan

Alan Cox wrote:
..

..

Perfect. That hook will let me add things very gradually.

Thanks!

applied 1-3