[PATCH 01/04] sata_mv: fix 8-port timeouts on 508x/6081 chips

[PATCH 01/04] sata_mv: fix 8-port timeouts on 508x/6081 chips

[Mark Lord]

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.

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

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

--- old/drivers/ata/sata_mv.c	2009-01-16 11:11:16.000000000 -0500
+++ linux/drivers/ata/sata_mv.c	2009-01-16 11:52:17.000000000 -0500
@@ -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 */

[PATCH 02/04] sata_mv: don't read hc_irq_cause

[Mark Lord]

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

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

--- old/drivers/ata/sata_mv.c	2009-01-16 11:52:17.000000000 -0500
+++ linux/drivers/ata/sata_mv.c	2009-01-19 16:47:24.000000000 -0500
@@ -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);

[PATCH 03/04] sata_mv: remove bogus nsect restriction

[Mark Lord]

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

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

--- old/drivers/ata/sata_mv.c	2009-01-19 17:15:48.000000000 -0500
+++ linux/drivers/ata/sata_mv.c	2009-01-19 17:18:31.000000000 -0500
@@ -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:

[PATCH 04/04] sata_mv: msi masking

[Mark Lord]

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.

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

--- old/drivers/ata/sata_mv.c	2009-01-19 17:18:31.000000000 -0500
+++ linux/drivers/ata/sata_mv.c	2009-01-19 17:12:51.000000000 -0500
@@ -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);
 }
 

[PATCH 05/04] sata_mv: msi fix and cleanup

[Mark Lord]

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.

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

--- old/drivers/ata/sata_mv.c	2009-01-19 17:12:51.000000000 -0500
+++ linux/drivers/ata/sata_mv.c	2009-01-20 13:06:06.000000000 -0500
@@ -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);

[PATCH 06/06] sata_mv: remove experimental designation

[Mark Lord]

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

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

--- old/drivers/ata/sata_mv.c	2009-01-20 13:06:06.000000000 -0500
+++ linux/drivers/ata/sata_mv.c	2009-01-20 13:42:32.000000000 -0500
@@ -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 */
--- old/drivers/ata/Kconfig	2009-01-20 13:44:28.000000000 -0500
+++ linux/drivers/ata/Kconfig	2009-01-20 13:44:33.000000000 -0500
@@ -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.
 

Re: [PATCH 04/04] sata_mv: msi masking

[Grant Grundler]

On Mon, Jan 19, 2009 at 3:07 PM, Mark Lord <liml@rtr.ca> wrote:
> 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.

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

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

Please add:
Reviewed-by: Grant Grundler <grundler@google.com>


> --- old/drivers/ata/sata_mv.c   2009-01-19 17:18:31.000000000 -0500
> +++ linux/drivers/ata/sata_mv.c 2009-01-19 17:12:51.000000000 -0500
> @@ -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 */

Better comment would be:
    /* Unmasking any Cause bit which is set will generate an interrupt. */

And since we've masked all bits above, this effectively applies to all bits
in the mask. This is true for either type of interrupt (Line vs MSI).
We only require this for MSI though.

FYI (everyone else but Mark), MSI only brings two benefits to this driver:
1) ability to spread interrupt load across lots of CPUs if we have lots
    of controllers in a machine.
2) Get an exclusive IRQ (avoids spurious calls to IRQ handlers)

thanks,
grant

ps. Kudos to Mark for excellent work to date on this driver.
I'm impressed with how well it's working now compared
to the crap we had a year or two ago.

> +       if (using_msi)
> +               writel(hpriv->main_irq_mask, hpriv->main_irq_mask_addr);
> +
>        return IRQ_RETVAL(handled);
> }

Re: [PATCH 04/04] sata_mv: msi masking

[Mark Lord]

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

Thanks.

[PATCH 04/05] sata_mv: msi masking fix (v2)

[Mark Lord]

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.

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

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

--- old/drivers/ata/sata_mv.c	2009-01-21 10:16:59.000000000 -0500
+++ linux/drivers/ata/sata_mv.c	2009-01-21 10:22:28.000000000 -0500
@@ -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);

[PATCH 05/05] sata_mv: no longer experimental (v2)

[Mark Lord]

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

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

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

--- old/drivers/ata/Kconfig	2009-01-21 10:16:16.000000000 -0500
+++ linux/drivers/ata/Kconfig	2009-01-21 10:25:35.000000000 -0500
@@ -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.
 

Re: [PATCH 04/05] sata_mv: msi masking fix (v2)

[Grant Grundler]

On Wed, Jan 21, 2009 at 7:31 AM, Mark Lord <liml@rtr.ca> wrote:
> 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.

Excellent.

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

Reviewed-by: Grant Grundler <grundler@google.com>

thanks,
grant

> ---
>
> This replaces the originally posted [PATCH 05/04] version.
>
> --- old/drivers/ata/sata_mv.c   2009-01-21 10:16:59.000000000 -0500
> +++ linux/drivers/ata/sata_mv.c 2009-01-21 10:22:28.000000000 -0500
> @@ -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);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 04/05] sata_mv: msi masking fix (v2)

[Tejun Heo]

Hello, Mark.

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

Generally looks good, but...

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

I just don't like CONSTANT OP EXPRESSION construct.  The only possible
upside it has is that it would make the compiler fail if '=' is used
instead of '==' but compiler has been smart enough to whine about
LVALUE = EXPRESSION for a very long time now.  How about plain
!pci_enable_msi(pdev) or pci_enable_msi(pdev) == 0?

Thanks.

-- 
tejun

Re: [PATCH 04/05] sata_mv: msi masking fix (v2)

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
..
>> +    /* Enable message-switched interrupts, if requested */
>> +    if (msi && 0 == pci_enable_msi(pdev))
>> +        hpriv->hp_flags |= MV_HP_FLAG_MSI;
> 
> I just don't like CONSTANT OP EXPRESSION construct.
..

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

Re: [PATCH 04/05] sata_mv: msi masking fix (v2)

[Mark Lord]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
> ..
>>> +    /* Enable message-switched interrupts, if requested */
>>> +    if (msi && 0 == pci_enable_msi(pdev))
>>> +        hpriv->hp_flags |= MV_HP_FLAG_MSI;
>>
>> I just don't like CONSTANT OP EXPRESSION construct.
> ..
> 
> 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.
..

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

Re: [PATCH 04/05] sata_mv: msi masking fix (v2)

[Tejun Heo]

Mark Lord wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>> Mark Lord wrote:
>> ..
>>>> +    /* Enable message-switched interrupts, if requested */
>>>> +    if (msi && 0 == pci_enable_msi(pdev))
>>>> +        hpriv->hp_flags |= MV_HP_FLAG_MSI;
>>>
>>> I just don't like CONSTANT OP EXPRESSION construct.
>> ..
>>
>> 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.
> ..
> 
> .. 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.

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

sata_mv: basic PIO-only ATAPI support?

[Mark Lord]

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.

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

--- old/drivers/ata/sata_mv.c	2009-01-23 11:20:57.000000000 -0500
+++ linux/drivers/ata/sata_mv.c	2009-01-23 12:02:20.000000000 -0500
@@ -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)

sata_mv: basic PIO-only ATAPI support?

[Mark Lord]

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

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

--- old/drivers/ata/sata_mv.c	2009-01-23 11:20:57.000000000 -0500
+++ linux/drivers/ata/sata_mv.c	2009-01-23 12:02:20.000000000 -0500
@@ -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)

Re: sata_mv: basic PIO-only ATAPI support?

[Alan Cox]

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

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

Re: sata_mv: basic PIO-only ATAPI support?

[Mark Lord]

Alan Cox wrote:
..
> 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.
..

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

Thanks!

Re: [PATCH 01/04] sata_mv: fix 8-port timeouts on 508x/6081 chips

[Jeff Garzik]

Mark Lord wrote:
> 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.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com> ---
> 
> Reposting, as I got no response/acknowledgement on the first attempt.

applied 1-3

Re: [PATCH 04/05] sata_mv: msi masking fix (v2)

[Jeff Garzik]

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

applied, after moving the '0' equality test to follow the 
pci_enable_msi() call...  :)

Re: [PATCH 05/05] sata_mv: no longer experimental (v2)

[Jeff Garzik]

Mark Lord wrote:
> Update Kconfig for sata_mv with full list of chips supported,
> and (finally!) remove the "EXPERIMENTAL" designations.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> ---
> 
> This replaces the patch "[PATCH 06/06] sata_mv: remove experimental".
> 
> --- old/drivers/ata/Kconfig    2009-01-21 10:16:16.000000000 -0500
> +++ linux/drivers/ata/Kconfig    2009-01-21 10:25:35.000000000 -0500
> @@ -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.

applied.

three cheers!