[PATCH 0/8] sata_mv interrupt/eh fixes etc..

[PATCH 0/8] sata_mv interrupt/eh fixes etc..

[Mark Lord]

Here are eight more patches for sata_mv.

The focus this time is on a revamp of the interrupt and error handling code,
and re-enabling hotplug (primary ports only) at the end.

The Marvell chips cannot support Asynchronous Notification (AN)
due to errata issues, so hotplug on PM ports is not supported for now.
Hotplug on PM ports would require Tejun's HP polling framework.

Cheers

[PATCH 1/8] sata_mv more cosmetics

[Mark Lord]

More cosmetic cleanups prior to the interrupt/error handling logic changes.

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

--- old/drivers/ata/sata_mv.c	2008-04-18 09:31:15.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:01:22.000000000 -0400
@@ -123,11 +123,11 @@
 	MV_MAX_SG_CT		= 256,
 	MV_SG_TBL_SZ		= (16 * MV_MAX_SG_CT),
 
-	MV_PORTS_PER_HC		= 4,
-	/* == (port / MV_PORTS_PER_HC) to determine HC from 0-7 port */
+	/* Determine hc from 0-7 port: hc = port >> MV_PORT_HC_SHIFT */
 	MV_PORT_HC_SHIFT	= 2,
-	/* == (port % MV_PORTS_PER_HC) to determine hard port from 0-7 port */
-	MV_PORT_MASK		= 3,
+	MV_PORTS_PER_HC		= (1 << MV_PORT_HC_SHIFT), /* 4 */
+	/* Determine hc port from 0-7 port: hardport = port & MV_PORT_MASK */
+	MV_PORT_MASK		= (MV_PORTS_PER_HC - 1),   /* 3 */
 
 	/* Host Flags */
 	MV_FLAG_DUAL_HC		= (1 << 30),  /* two SATA Host Controllers */
@@ -187,8 +187,8 @@
 	HC_MAIN_IRQ_MASK_OFS	= 0x1d64,
 	HC_SOC_MAIN_IRQ_CAUSE_OFS = 0x20020,
 	HC_SOC_MAIN_IRQ_MASK_OFS = 0x20024,
-	PORT0_ERR		= (1 << 0),	/* shift by port # */
-	PORT0_DONE		= (1 << 1),	/* shift by port # */
+	ERR_IRQ			= (1 << 0),	/* shift by port # */
+	DONE_IRQ		= (1 << 1),	/* shift by port # */
 	HC0_IRQ_PEND		= 0x1ff,	/* bits 0-8 = HC0's ports */
 	HC_SHIFT		= 9,		/* bits 9-17 = HC1's ports */
 	PCI_ERR			= (1 << 18),
@@ -214,8 +214,8 @@
 	HC_CFG_OFS		= 0,
 
 	HC_IRQ_CAUSE_OFS	= 0x14,
-	CRPB_DMA_DONE		= (1 << 0),	/* shift by port # */
-	HC_IRQ_COAL		= (1 << 4),	/* IRQ coalescing */
+	DMA_IRQ			= (1 << 0),	/* shift by port # */
+	HC_COAL_IRQ		= (1 << 4),	/* IRQ coalescing */
 	DEV_IRQ			= (1 << 8),	/* shift by port # */
 
 	/* Shadow block registers */
@@ -348,6 +348,8 @@
 	EDMA_IORDY_TMOUT	= 0x34,
 	EDMA_ARB_CFG		= 0x38,
 
+	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),
@@ -718,11 +720,6 @@
 	(void) readl(addr);	/* flush to avoid PCI posted write */
 }
 
-static inline void __iomem *mv_hc_base(void __iomem *base, unsigned int hc)
-{
-	return (base + MV_SATAHC0_REG_BASE + (hc * MV_SATAHC_REG_SZ));
-}
-
 static inline unsigned int mv_hc_from_port(unsigned int port)
 {
 	return port >> MV_PORT_HC_SHIFT;
@@ -733,6 +730,11 @@
 	return port & MV_PORT_MASK;
 }
 
+static inline void __iomem *mv_hc_base(void __iomem *base, unsigned int hc)
+{
+	return (base + MV_SATAHC0_REG_BASE + (hc * MV_SATAHC_REG_SZ));
+}
+
 static inline void __iomem *mv_hc_base_from_port(void __iomem *base,
 						 unsigned int port)
 {
@@ -833,9 +835,9 @@
 	}
 	if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN)) {
 		struct mv_host_priv *hpriv = ap->host->private_data;
-		int hard_port = mv_hardport_from_port(ap->port_no);
+		int hardport = mv_hardport_from_port(ap->port_no);
 		void __iomem *hc_mmio = mv_hc_base_from_port(
-					mv_host_base(ap->host), hard_port);
+					mv_host_base(ap->host), hardport);
 		u32 hc_irq_cause, ipending;
 
 		/* clear EDMA event indicators, if any */
@@ -843,8 +845,7 @@
 
 		/* clear EDMA interrupt indicator, if any */
 		hc_irq_cause = readl(hc_mmio + HC_IRQ_CAUSE_OFS);
-		ipending = (DEV_IRQ << hard_port) |
-				(CRPB_DMA_DONE << hard_port);
+		ipending = (DEV_IRQ | DMA_IRQ) << hardport;
 		if (hc_irq_cause & ipending) {
 			writelfl(hc_irq_cause & ~ipending,
 				 hc_mmio + HC_IRQ_CAUSE_OFS);
@@ -860,7 +861,6 @@
 		writelfl(EDMA_EN, port_mmio + EDMA_CMD_OFS);
 		pp->pp_flags |= MV_PP_FLAG_EDMA_EN;
 	}
-	WARN_ON(!(EDMA_EN & readl(port_mmio + EDMA_CMD_OFS)));
 }
 
 /**
@@ -1032,10 +1032,16 @@
 	 * See mv_qc_prep() for more info.
 	 */
 	if (adev->flags & ATA_DFLAG_NCQ) {
-		if (sata_pmp_attached(adev->link->ap))
+		if (sata_pmp_attached(adev->link->ap)) {
 			adev->flags &= ~ATA_DFLAG_NCQ;
-		else if (adev->max_sectors > ATA_MAX_SECTORS)
-			adev->max_sectors = ATA_MAX_SECTORS;
+			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);
+		}
 	}
 }
 
@@ -1489,12 +1495,11 @@
 
 	edma_err_cause = readl(port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
 
-	ata_ehi_push_desc(ehi, "edma_err 0x%08x", edma_err_cause);
+	ata_ehi_push_desc(ehi, "edma_err_cause=%08x", edma_err_cause);
 
 	/*
-	 * all generations share these EDMA error cause bits
+	 * All generations share these EDMA error cause bits:
 	 */
-
 	if (edma_err_cause & EDMA_ERR_DEV)
 		err_mask |= AC_ERR_DEV;
 	if (edma_err_cause & (EDMA_ERR_D_PAR | EDMA_ERR_PRD_PAR |
@@ -1511,23 +1516,22 @@
 		action |= ATA_EH_RESET;
 	}
 
+	/*
+	 * Gen-I has a different SELF_DIS bit,
+	 * different FREEZE bits, and no SERR bit:
+	 */
 	if (IS_GEN_I(hpriv)) {
 		eh_freeze_mask = EDMA_EH_FREEZE_5;
-
 		if (edma_err_cause & EDMA_ERR_SELF_DIS_5) {
-			pp = ap->private_data;
 			pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
 			ata_ehi_push_desc(ehi, "EDMA self-disable");
 		}
 	} else {
 		eh_freeze_mask = EDMA_EH_FREEZE;
-
 		if (edma_err_cause & EDMA_ERR_SELF_DIS) {
-			pp = ap->private_data;
 			pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
 			ata_ehi_push_desc(ehi, "EDMA self-disable");
 		}
-
 		if (edma_err_cause & EDMA_ERR_SERR) {
 			sata_scr_read(&ap->link, SCR_ERROR, &serr);
 			sata_scr_write_flush(&ap->link, SCR_ERROR, serr);
@@ -1640,6 +1644,7 @@
 		pp->resp_idx++;
 	}
 
+	/* Update the software queue position index in hardware */
 	if (work_done)
 		writelfl((pp->crpb_dma & EDMA_RSP_Q_BASE_LO_MASK) |
 			 (out_index << EDMA_RSP_Q_PTR_SHIFT),
@@ -1692,7 +1697,7 @@
 	for (port = port0; port < last_port; port++) {
 		struct ata_port *ap = host->ports[port];
 		struct mv_port_priv *pp;
-		int have_err_bits, hard_port, shift;
+		int have_err_bits, hardport, shift;
 
 		if ((!ap) || (ap->flags & ATA_FLAG_DISABLED))
 			continue;
@@ -1703,7 +1708,7 @@
 		if (port >= MV_PORTS_PER_HC)
 			shift++;	/* skip bit 8 in the HC Main IRQ reg */
 
-		have_err_bits = ((PORT0_ERR << shift) & relevant);
+		have_err_bits = ((ERR_IRQ << shift) & relevant);
 
 		if (unlikely(have_err_bits)) {
 			struct ata_queued_cmd *qc;
@@ -1716,13 +1721,13 @@
 			continue;
 		}
 
-		hard_port = mv_hardport_from_port(port); /* range 0..3 */
+		hardport = mv_hardport_from_port(port); /* range 0..3 */
 
 		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
-			if ((CRPB_DMA_DONE << hard_port) & hc_irq_cause)
+			if ((DMA_IRQ << hardport) & hc_irq_cause)
 				mv_intr_edma(ap);
 		} else {
-			if ((DEV_IRQ << hard_port) & hc_irq_cause)
+			if ((DEV_IRQ << hardport) & hc_irq_cause)
 				mv_intr_pio(ap);
 		}
 	}
@@ -1789,30 +1794,28 @@
 	struct mv_host_priv *hpriv = host->private_data;
 	unsigned int hc, handled = 0, n_hcs;
 	void __iomem *mmio = hpriv->base;
-	u32 irq_stat, irq_mask;
+	u32 main_cause, main_mask;
 
-	/* Note to self: &host->lock == &ap->host->lock == ap->lock */
 	spin_lock(&host->lock);
-
-	irq_stat = readl(hpriv->main_cause_reg_addr);
-	irq_mask = readl(hpriv->main_mask_reg_addr);
-
-	/* check the cases where we either have nothing pending or have read
-	 * a bogus register value which can indicate HW removal or PCI fault
+	main_cause = readl(hpriv->main_cause_reg_addr);
+	main_mask  = readl(hpriv->main_mask_reg_addr);
+	/*
+	 * Deal with cases where we either have nothing pending, or have read
+	 * a bogus register value which can indicate HW removal or PCI fault.
 	 */
-	if (!(irq_stat & irq_mask) || (0xffffffffU == irq_stat))
+	if (!(main_cause & main_mask) || (main_cause == 0xffffffffU))
 		goto out_unlock;
 
 	n_hcs = mv_get_hc_count(host->ports[0]->flags);
 
-	if (unlikely((irq_stat & PCI_ERR) && HAS_PCI(host))) {
+	if (unlikely((main_cause & PCI_ERR) && HAS_PCI(host))) {
 		mv_pci_error(host, mmio);
 		handled = 1;
 		goto out_unlock;	/* skip all other HC irq handling */
 	}
 
 	for (hc = 0; hc < n_hcs; hc++) {
-		u32 relevant = irq_stat & (HC0_IRQ_PEND << (hc * HC_SHIFT));
+		u32 relevant = main_cause & (HC0_IRQ_PEND << (hc * HC_SHIFT));
 		if (relevant) {
 			mv_host_intr(host, relevant, hc);
 			handled = 1;
@@ -1821,7 +1824,6 @@
 
 out_unlock:
 	spin_unlock(&host->lock);
-
 	return IRQ_RETVAL(handled);
 }
 
@@ -2406,8 +2408,8 @@
 {
 	struct mv_host_priv *hpriv = ap->host->private_data;
 	unsigned int hc = (ap->port_no > 3) ? 1 : 0;
-	u32 tmp, mask;
 	unsigned int shift;
+	u32 main_mask;
 
 	/* FIXME: handle coalescing completion events properly */
 
@@ -2415,11 +2417,10 @@
 	if (hc > 0)
 		shift++;
 
-	mask = 0x3 << shift;
-
 	/* disable assertion of portN err, done events */
-	tmp = readl(hpriv->main_mask_reg_addr);
-	writelfl(tmp & ~mask, hpriv->main_mask_reg_addr);
+	main_mask = readl(hpriv->main_mask_reg_addr);
+	main_mask &= ~((DONE_IRQ | ERR_IRQ) << shift);
+	writelfl(main_mask, hpriv->main_mask_reg_addr);
 }
 
 static void mv_eh_thaw(struct ata_port *ap)
@@ -2429,8 +2430,8 @@
 	unsigned int hc = (ap->port_no > 3) ? 1 : 0;
 	void __iomem *hc_mmio = mv_hc_base(mmio, hc);
 	void __iomem *port_mmio = mv_ap_base(ap);
-	u32 tmp, mask, hc_irq_cause;
 	unsigned int shift, hc_port_no = ap->port_no;
+	u32 main_mask, hc_irq_cause;
 
 	/* FIXME: handle coalescing completion events properly */
 
@@ -2440,20 +2441,18 @@
 		hc_port_no -= 4;
 	}
 
-	mask = 0x3 << shift;
-
 	/* clear EDMA errors on this port */
 	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 &= ~(1 << hc_port_no);	/* clear CRPB-done */
-	hc_irq_cause &= ~(1 << (hc_port_no + 8)); /* clear Device int */
+	hc_irq_cause &= ~((DEV_IRQ | DMA_IRQ) << hc_port_no);
 	writel(hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
 
 	/* enable assertion of portN err, done events */
-	tmp = readl(hpriv->main_mask_reg_addr);
-	writelfl(tmp | mask, hpriv->main_mask_reg_addr);
+	main_mask = readl(hpriv->main_mask_reg_addr);
+	main_mask |= ((DONE_IRQ | ERR_IRQ) << shift);
+	writelfl(main_mask, hpriv->main_mask_reg_addr);
 }
 
 /**
@@ -2664,19 +2663,17 @@
 
 	rc = mv_chip_id(host, board_idx);
 	if (rc)
-	goto done;
+		goto done;
 
 	if (HAS_PCI(host)) {
-		hpriv->main_cause_reg_addr = hpriv->base +
-		  HC_MAIN_IRQ_CAUSE_OFS;
-		hpriv->main_mask_reg_addr = hpriv->base + HC_MAIN_IRQ_MASK_OFS;
+		hpriv->main_cause_reg_addr = mmio + HC_MAIN_IRQ_CAUSE_OFS;
+		hpriv->main_mask_reg_addr  = mmio + HC_MAIN_IRQ_MASK_OFS;
 	} else {
-		hpriv->main_cause_reg_addr = hpriv->base +
-		  HC_SOC_MAIN_IRQ_CAUSE_OFS;
-		hpriv->main_mask_reg_addr = hpriv->base +
-		  HC_SOC_MAIN_IRQ_MASK_OFS;
+		hpriv->main_cause_reg_addr = mmio + HC_SOC_MAIN_IRQ_CAUSE_OFS;
+		hpriv->main_mask_reg_addr  = mmio + HC_SOC_MAIN_IRQ_MASK_OFS;
 	}
-	/* global interrupt mask */
+
+	/* global interrupt mask: 0 == mask everything */
 	writel(0, hpriv->main_mask_reg_addr);
 
 	n_hc = mv_get_hc_count(host->ports[0]->flags);

Re: [PATCH 1/8] sata_mv more cosmetics

[Jeff Garzik]

Mark Lord wrote:


applied patches 1-8...



> @@ -1789,30 +1794,28 @@
>     struct mv_host_priv *hpriv = host->private_data;
>     unsigned int hc, handled = 0, n_hcs;
>     void __iomem *mmio = hpriv->base;
> -    u32 irq_stat, irq_mask;
> +    u32 main_cause, main_mask;
> 
> -    /* Note to self: &host->lock == &ap->host->lock == ap->lock */
>     spin_lock(&host->lock);
> -
> -    irq_stat = readl(hpriv->main_cause_reg_addr);
> -    irq_mask = readl(hpriv->main_mask_reg_addr);
> -
> -    /* check the cases where we either have nothing pending or have read
> -     * a bogus register value which can indicate HW removal or PCI fault
> +    main_cause = readl(hpriv->main_cause_reg_addr);
> +    main_mask  = readl(hpriv->main_mask_reg_addr);
> +    /*
> +     * Deal with cases where we either have nothing pending, or have read
> +     * a bogus register value which can indicate HW removal or PCI fault.
>      */

  ...but I am sad to see this.  irq_stat and irq_mask naming make the 
driver more accessible to outsiders, because the purpose of the 
registers is immediately apparent even without having the docs at hand 
-- which is the case for everybody in the world except me and you :)

I applied the patch anyway because you are defacto maintainer of sata_mv.

However, I _request_ a reconsideration of some of these renames when 
viewed in that light.  It's your prerogative as maintainer to ignore or 
honor that request as you see fit...  We all have to balance making our 
own job easier with making the driver accessible to outsiders, 
particularly those without NDA'd docs.

	Jeff

Re: [PATCH 1/8] sata_mv more cosmetics

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
..
>> -    irq_stat = readl(hpriv->main_cause_reg_addr);
>> -    irq_mask = readl(hpriv->main_mask_reg_addr);
..
>> +    main_cause = readl(hpriv->main_cause_reg_addr);
>> +    main_mask  = readl(hpriv->main_mask_reg_addr);
..
>  ...but I am sad to see this.  irq_stat and irq_mask naming make the 
> driver more accessible to outsiders, because the purpose of the 
> registers is immediately apparent even without having the docs at hand 
> -- which is the case for everybody in the world except me and you :)
> 
> I applied the patch anyway because you are defacto maintainer of sata_mv.
> 
> However, I _request_ a reconsideration of some of these renames when 
> viewed in that light.  It's your prerogative as maintainer to ignore or 
> honor that request as you see fit...  We all have to balance making our 
> own job easier with making the driver accessible to outsiders, 
> particularly those without NDA'd docs.
..

Thanks, Jeff.
I agree, too, in principle if not in deed at the moment.

But there are just *so many* irq cause / mask registers in these chips,
(one for PCI, one for the host function, and one per-port for EDMA,
plus the SATA SError + mask, ..), that even I was getting confused
while working on the code.

So, for now, they've been renamed back to something closer to
what is used in the (NDA only) documentation.

I did consider main_irq_{cause,mask} as an even better set of names there,
but in the end went for the shortened versions to avoid hitting the
pedantic 80-character line "limits" too often.

Perhaps after we finish the major hacking around the interrupt/EH code
(yes, more to come..) we can try and fix up those names again.

I'll put it on the next TODO list update.

Cheers

Re: [PATCH 1/8] sata_mv more cosmetics

[Grant Grundler]

On Fri, Apr 25, 2008 at 6:46 AM, Mark Lord <liml@rtr.ca> wrote:
...
>  But there are just *so many* irq cause / mask registers in these chips,
>  (one for PCI, one for the host function, and one per-port for EDMA,
>  plus the SATA SError + mask, ..), that even I was getting confused
>  while working on the code.

Yeah, it's confusing because most of the bits from one "interrupt"
(quotes explained in the next paragraph) register are rolled up into one bit
and available in another register. It seems like someone thought it
would be more efficient to read one register for all possible "causes"
and then have evidence a second register read is really necessary
(because a bit was set). Unfortunately, the result was the default
programming model as suggested by Marvell asks for at least two
MMIO reads just to find out if any IO completions are pending. 'Nuf said.
Kudos to Mark for figuring out how to reduce that to one MMIO read.

WRT "Interrupt", only the main cause (register can actually
generate interrupts. All the other "interrupt" registers
are effectively status registers. If a bit gets set in the main cause
register and it's not masked, an interrupt will get generated
(subject to any coalescing rules that have been enabled).
This is important to know for MSI support where we are dealing
with exclusive edge triggered interrupts instead of PCI shared
level triggered interrupts.

>  So, for now, they've been renamed back to something closer to
>  what is used in the (NDA only) documentation.
>
>  I did consider main_irq_{cause,mask} as an even better set of names there,
>  but in the end went for the shortened versions to avoid hitting the
>  pedantic 80-character line "limits" too often.

I think you can associate any names you want with whatever the
non-public documentation is calling the registers by adding a comment
in the header files where the bit masks and register offsets are defined.
Or vice versa. Which ever way works for you.

I prefer to use the non-public register names in the code only because it
will be easier for Marvell engineers to participate in _maintaining_
this driver. I think getting involved with open source developement
is a good career developement experience. And the device drivers for
"obsolete" HW allows them to take more risks/make mistakes
without getting into serious trouble with the company.

hth,
grant

Re: [PATCH 1/8] sata_mv more cosmetics

[Jeff Garzik]

Grant Grundler wrote:
> I think you can associate any names you want with whatever the
> non-public documentation is calling the registers by adding a comment
> in the header files where the bit masks and register offsets are defined.
> Or vice versa. Which ever way works for you.
> 
> I prefer to use the non-public register names in the code only because it
> will be easier for Marvell engineers to participate in _maintaining_
> this driver. I think getting involved with open source developement
> is a good career developement experience. And the device drivers for
> "obsolete" HW allows them to take more risks/make mistakes
> without getting into serious trouble with the company.


The trouble, though, comes with following that logic in every driver, 
making the collective body less accessible to anyone not _intimately_ 
tied into the hardware _and_ possessing NDA'd docs.

Further, it is obvious with _most_ drivers in Linux that engineers at 
the hardware vendor do _not_ participate in maintaining drivers for 
their older hardware, so I also wish to be careful and avoid punishing 
the majority to help a minority.

I put a significant value in having more-readable code like

	status = mr32(IRQ_STAT);	/* read IRQ status from PPB */

rather than

	status = READ_REG(ICR5PPB);	/* read IRQ status from PPB */

because the casual reader is more likely to understand the first 
example, even though it deviates from the string of line noise some 
engineer writing Verilog at 4:00am decided was a good register name.

	Jeff

Re: [PATCH 1/8] sata_mv more cosmetics

[Mark Lord]

Jeff Garzik wrote:
> Grant Grundler wrote:
>> I think you can associate any names you want with whatever the
>> non-public documentation is calling the registers by adding a comment
>> in the header files where the bit masks and register offsets are defined.
>> Or vice versa. Which ever way works for you.
>>
>> I prefer to use the non-public register names in the code only because it
>> will be easier for Marvell engineers to participate in _maintaining_
>> this driver. I think getting involved with open source developement
>> is a good career developement experience. And the device drivers for
>> "obsolete" HW allows them to take more risks/make mistakes
>> without getting into serious trouble with the company.
> 
> 
> The trouble, though, comes with following that logic in every driver, 
> making the collective body less accessible to anyone not _intimately_ 
> tied into the hardware _and_ possessing NDA'd docs.
> 
> Further, it is obvious with _most_ drivers in Linux that engineers at 
> the hardware vendor do _not_ participate in maintaining drivers for 
> their older hardware, so I also wish to be careful and avoid punishing 
> the majority to help a minority.
> 
> I put a significant value in having more-readable code like
> 
>     status = mr32(IRQ_STAT);    /* read IRQ status from PPB */
> 
> rather than
> 
>     status = READ_REG(ICR5PPB);    /* read IRQ status from PPB */
> 
> because the casual reader is more likely to understand the first 
> example, even though it deviates from the string of line noise some 
> engineer writing Verilog at 4:00am decided was a good register name.
..

There's a patch out for this now -- just waiting for you to pick it up.
It uses "main_irq_cause/mask" now instead of merely "main_cause/mask".

This ought to be understandable at a glance to anyone,
even those of us with chipset docs.  :)

Cheers

Re: [PATCH 1/8] sata_mv more cosmetics

[Grant Grundler]

On Fri, Apr 25, 2008 at 9:57 AM, Jeff Garzik <jgarzik@pobox.com> wrote:
> Grant Grundler wrote:
>
> > I think you can associate any names you want with whatever the
> > non-public documentation is calling the registers by adding a comment
> > in the header files where the bit masks and register offsets are defined.
> > Or vice versa. Which ever way works for you.
> >
> > I prefer to use the non-public register names in the code only because it
> > will be easier for Marvell engineers to participate in _maintaining_
> > this driver. I think getting involved with open source developement
> > is a good career developement experience. And the device drivers for
> > "obsolete" HW allows them to take more risks/make mistakes
> > without getting into serious trouble with the company.
> >
>
>
>  The trouble, though, comes with following that logic in every driver,
> making the collective body less accessible to anyone not _intimately_ tied
> into the hardware _and_ possessing NDA'd docs.

As a general statement I agree but think linux drivers have issues with
accessibility anyway.

Not possessing documentation inherently makes the device driver less accessible
and no naming convention is going to fix that.  I agree different variable names
make it slightly harder for someone like yourself (well, me too to a
lesser degree)
since we are looking at lots of different drivers. My problem with
this statement is
the definition of "majority" here sounds like a few kernel maintainers. The
"industry" (HW Vendors and their customers) folks I've dealt with historically
are only interested in a few drivers (less than a hand full) since
that's all they need to solve particular problems.

Secondly, anyone trying to understand a particular device driver will have to
understand quite a bit about the HW (hopefully from comments if it's not obvious
from the code) in order to make sense of what "IRQ_STAT" register actually
provides and what the side effects are from reading it (e.g. flushing DMA that
the driver authoer knew was inflight). The only other way is to have this
understanding is to read the NDA documentation which employees
of HW vendors typically have done.

Lastly, I'm trying to _encourage_ some Marvell employees to spend a few minutes
on this driver since it's easier for them than it is for me (despite
also having access
to various vendor's NDA documentation). I'm lazy and any time I don't have
to read the documentation because someone already knows the answer
is good. While having documentation is way better than nothing, documentation
is never as good as one hopes because of "errata", omissions, ambiguity, etc.

>  Further, it is obvious with _most_ drivers in Linux that engineers at the
> hardware vendor do _not_ participate in maintaining drivers for their older
> hardware, so I also wish to be careful and avoid punishing the majority to
> help a minority.

OK - I guess I was expressing more optimism than reflecting reality. :(

>  I put a significant value in having more-readable code like
>
>         status = mr32(IRQ_STAT);        /* read IRQ status from PPB */
>
>  rather than
>
>         status = READ_REG(ICR5PPB);     /* read IRQ status from PPB */
>
>  because the casual reader is more likely to understand the first example,
> even though it deviates from the string of line noise some engineer writing
> Verilog at 4:00am decided was a good register name.

*nod*  - though to be fair, "MAIN_CAUSE" is readable and "ICR5PPB" is not.
(and mark already posted a patch renaming this to main_irq_cause).

BTW, if this is really important, you might consider adding a sentence
to Documentation/CodingStyle "Chapter 4: Naming" to encourage using
"the same register names as what similar drivers are using" and give the
above example (It's a good one.)

cheers,
grant

Re: [PATCH 1/8] sata_mv more cosmetics

[Mark Lord]

Grant Grundler wrote:
> On Fri, Apr 25, 2008 at 6:46 AM, Mark Lord <liml@rtr.ca> wrote:
> ...
>>  But there are just *so many* irq cause / mask registers in these chips,
>>  (one for PCI, one for the host function, and one per-port for EDMA,
>>  plus the SATA SError + mask, ..), that even I was getting confused
>>  while working on the code.
> 
> Yeah, it's confusing because most of the bits from one "interrupt"
> (quotes explained in the next paragraph) register are rolled up into one bit
> and available in another register. It seems like someone thought it
> would be more efficient to read one register for all possible "causes"
> and then have evidence a second register read is really necessary
> (because a bit was set). Unfortunately, the result was the default
> programming model as suggested by Marvell asks for at least two
> MMIO reads just to find out if any IO completions are pending. 'Nuf said.
> Kudos to Mark for figuring out how to reduce that to one MMIO read.
..

Readers may note that the interrupt path (for a real mv interrupt)
currently reads three registers (or four if more than one hc interrupts).

Grant was refering to a plan we have formulated to reduce that to
a single read for the non-error cases.  Not currently implemented
in the driver (though there are comments marking where to change it).

I'm leaving those optimizations for after the rest of the EH fixes are completed.
Hopefully within a week or less.

Cheers

[PATCH 2/8] sata_mv mask all interrupt coalescing bits

[Mark Lord]

Ignore *all* interrupt coalescing bits on all controllers,
not just some of each.

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:01:22.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:02:17.000000000 -0400
@@ -204,6 +204,7 @@
 	HC_MAIN_RSVD_5		= (0x1fff << 19), /* bits 31-19 */
 	HC_MAIN_RSVD_SOC	= (0x3fffffb << 6),     /* bits 31-9, 7-6 */
 	HC_MAIN_MASKED_IRQS	= (TRAN_LO_DONE | TRAN_HI_DONE |
+				   PORTS_0_3_COAL_DONE | PORTS_4_7_COAL_DONE |
 				   PORTS_0_7_COAL_DONE | GPIO_INT | TWSI_INT |
 				   HC_MAIN_RSVD),
 	HC_MAIN_MASKED_IRQS_5	= (PORTS_0_3_COAL_DONE | PORTS_4_7_COAL_DONE |

[PATCH 3/8] sata_mv simplify freeze/thaw bit-shift calculations

[Mark Lord]

Introduce the MV_PORT_TO_SHIFT_AND_HARDPORT() macro,
to centralize/simplify various scattered bits of logic
for calculating bit shifts and the like.

Some of the places that do this get it wrong, too,
so consolidating the algorithm at one place will help
keep the code correct.

For now, we use the new macro in mv_eh_{freeze,thaw}.
A subsequent patch will re-use this in the interrupt handlers

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:02:17.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:06:31.000000000 -0400
@@ -731,6 +731,24 @@
 	return port & MV_PORT_MASK;
 }
 
+/*
+ * Consolidate some rather tricky bit shift calculations.
+ * This is hot-path stuff, so not a function.
+ * Simple code, with two return values, so macro rather than inline.
+ *
+ * port is the sole input, in range 0..7.
+ * shift is one output, for use with the main_cause and main_mask registers.
+ * hardport is the other output, in range 0..3
+ *
+ * Note that port and hardport may be the same variable in some cases.
+ */
+#define MV_PORT_TO_SHIFT_AND_HARDPORT(port,shift,hardport)	\
+{								\
+	shift    = mv_hc_from_port(port) * HC_SHIFT;		\
+	hardport = mv_hardport_from_port(port);			\
+	shift   += hardport * 2;				\
+}
+
 static inline void __iomem *mv_hc_base(void __iomem *base, unsigned int hc)
 {
 	return (base + MV_SATAHC0_REG_BASE + (hc * MV_SATAHC_REG_SZ));
@@ -2408,15 +2426,13 @@
 static void mv_eh_freeze(struct ata_port *ap)
 {
 	struct mv_host_priv *hpriv = ap->host->private_data;
-	unsigned int hc = (ap->port_no > 3) ? 1 : 0;
-	unsigned int shift;
+	unsigned int shift, hardport, port = ap->port_no;
 	u32 main_mask;
 
 	/* FIXME: handle coalescing completion events properly */
 
-	shift = ap->port_no * 2;
-	if (hc > 0)
-		shift++;
+	mv_stop_edma(ap);
+	MV_PORT_TO_SHIFT_AND_HARDPORT(port,shift,hardport);
 
 	/* disable assertion of portN err, done events */
 	main_mask = readl(hpriv->main_mask_reg_addr);
@@ -2427,28 +2443,22 @@
 static void mv_eh_thaw(struct ata_port *ap)
 {
 	struct mv_host_priv *hpriv = ap->host->private_data;
-	void __iomem *mmio = hpriv->base;
-	unsigned int hc = (ap->port_no > 3) ? 1 : 0;
-	void __iomem *hc_mmio = mv_hc_base(mmio, hc);
+	unsigned int shift, hardport, port = ap->port_no;
+	void __iomem *hc_mmio = mv_hc_base_from_port(hpriv->base, port);
 	void __iomem *port_mmio = mv_ap_base(ap);
-	unsigned int shift, hc_port_no = ap->port_no;
 	u32 main_mask, hc_irq_cause;
 
 	/* FIXME: handle coalescing completion events properly */
 
-	shift = ap->port_no * 2;
-	if (hc > 0) {
-		shift++;
-		hc_port_no -= 4;
-	}
+	MV_PORT_TO_SHIFT_AND_HARDPORT(port,shift,hardport);
 
 	/* clear EDMA errors on this port */
 	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) << hc_port_no);
-	writel(hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
+	hc_irq_cause &= ~((DEV_IRQ | DMA_IRQ) << hardport);
+	writelfl(hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
 
 	/* enable assertion of portN err, done events */
 	main_mask = readl(hpriv->main_mask_reg_addr);

REPOST: [PATCH 3/8] sata_mv simplify freeze/thaw bit-shift calculations

[Mark Lord]

Here it is again, minus the checkpatch.pl complaint:

Introduce the MV_PORT_TO_SHIFT_AND_HARDPORT() macro,
to centralize/simplify various scattered bits of logic
for calculating bit shifts and the like.

Some of the places that do this get it wrong, too,
so consolidating the algorithm at one place will help
keep the code correct.

For now, we use the new macro in mv_eh_{freeze,thaw}.
A subsequent patch will re-use this in the interrupt handlers

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:02:17.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:06:31.000000000 -0400
@@ -731,6 +731,24 @@
 	return port & MV_PORT_MASK;
 }
 
+/*
+ * Consolidate some rather tricky bit shift calculations.
+ * This is hot-path stuff, so not a function.
+ * Simple code, with two return values, so macro rather than inline.
+ *
+ * port is the sole input, in range 0..7.
+ * shift is one output, for use with the main_cause and main_mask registers.
+ * hardport is the other output, in range 0..3
+ *
+ * Note that port and hardport may be the same variable in some cases.
+ */
+#define MV_PORT_TO_SHIFT_AND_HARDPORT(port, shift, hardport)	\
+{								\
+	shift    = mv_hc_from_port(port) * HC_SHIFT;		\
+	hardport = mv_hardport_from_port(port);			\
+	shift   += hardport * 2;				\
+}
+
 static inline void __iomem *mv_hc_base(void __iomem *base, unsigned int hc)
 {
 	return (base + MV_SATAHC0_REG_BASE + (hc * MV_SATAHC_REG_SZ));
@@ -2408,15 +2426,13 @@
 static void mv_eh_freeze(struct ata_port *ap)
 {
 	struct mv_host_priv *hpriv = ap->host->private_data;
-	unsigned int hc = (ap->port_no > 3) ? 1 : 0;
-	unsigned int shift;
+	unsigned int shift, hardport, port = ap->port_no;
 	u32 main_mask;
 
 	/* FIXME: handle coalescing completion events properly */
 
-	shift = ap->port_no * 2;
-	if (hc > 0)
-		shift++;
+	mv_stop_edma(ap);
+	MV_PORT_TO_SHIFT_AND_HARDPORT(port, shift, hardport);
 
 	/* disable assertion of portN err, done events */
 	main_mask = readl(hpriv->main_mask_reg_addr);
@@ -2427,28 +2443,22 @@
 static void mv_eh_thaw(struct ata_port *ap)
 {
 	struct mv_host_priv *hpriv = ap->host->private_data;
-	void __iomem *mmio = hpriv->base;
-	unsigned int hc = (ap->port_no > 3) ? 1 : 0;
-	void __iomem *hc_mmio = mv_hc_base(mmio, hc);
+	unsigned int shift, hardport, port = ap->port_no;
+	void __iomem *hc_mmio = mv_hc_base_from_port(hpriv->base, port);
 	void __iomem *port_mmio = mv_ap_base(ap);
-	unsigned int shift, hc_port_no = ap->port_no;
 	u32 main_mask, hc_irq_cause;
 
 	/* FIXME: handle coalescing completion events properly */
 
-	shift = ap->port_no * 2;
-	if (hc > 0) {
-		shift++;
-		hc_port_no -= 4;
-	}
+	MV_PORT_TO_SHIFT_AND_HARDPORT(port, shift, hardport);
 
 	/* clear EDMA errors on this port */
 	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) << hc_port_no);
-	writel(hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
+	hc_irq_cause &= ~((DEV_IRQ | DMA_IRQ) << hardport);
+	writelfl(hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
 
 	/* enable assertion of portN err, done events */
 	main_mask = readl(hpriv->main_mask_reg_addr);

[PATCH 4/8] sata_mv simplify request/response queue handling

[Mark Lord]

Try and simplify handling of the request/response queues.

Maintain the cached copies of queue indexes in a fully-masked state,
rather than having each use of them have to do the masking.

Split off handling of a single crpb response into a separate function,
to reduce complexity in the main mv_process_crpb_entries() routine.

Ignore the rarely-valid error bits from the crpb status field,
as we already handle that information in mv_err_intr().

For now, preserve the rest of the original logic.
A later patch will deal with fixing that separately.

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:06:31.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:21:28.000000000 -0400
@@ -800,7 +800,8 @@
 	/*
 	 * initialize request queue
 	 */
-	index = (pp->req_idx & MV_MAX_Q_DEPTH_MASK) << EDMA_REQ_Q_PTR_SHIFT;
+	pp->req_idx &= MV_MAX_Q_DEPTH_MASK;	/* paranoia */
+	index = pp->req_idx << EDMA_REQ_Q_PTR_SHIFT;
 
 	WARN_ON(pp->crqb_dma & 0x3ff);
 	writel((pp->crqb_dma >> 16) >> 16, port_mmio + EDMA_REQ_Q_BASE_HI_OFS);
@@ -816,7 +817,8 @@
 	/*
 	 * initialize response queue
 	 */
-	index = (pp->resp_idx & MV_MAX_Q_DEPTH_MASK) << EDMA_RSP_Q_PTR_SHIFT;
+	pp->resp_idx &= MV_MAX_Q_DEPTH_MASK;	/* paranoia */
+	index = pp->resp_idx << EDMA_RSP_Q_PTR_SHIFT;
 
 	WARN_ON(pp->crpb_dma & 0xff);
 	writel((pp->crpb_dma >> 16) >> 16, port_mmio + EDMA_RSP_Q_BASE_HI_OFS);
@@ -1308,7 +1310,7 @@
 	flags |= (qc->dev->link->pmp & 0xf) << CRQB_PMP_SHIFT;
 
 	/* get current queue index from software */
-	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;
+	in_index = pp->req_idx;
 
 	pp->crqb[in_index].sg_addr =
 		cpu_to_le32(pp->sg_tbl_dma[qc->tag] & 0xffffffff);
@@ -1400,7 +1402,7 @@
 	flags |= (qc->dev->link->pmp & 0xf) << CRQB_PMP_SHIFT;
 
 	/* get current queue index from software */
-	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;
+	in_index = pp->req_idx;
 
 	crqb = (struct mv_crqb_iie *) &pp->crqb[in_index];
 	crqb->addr = cpu_to_le32(pp->sg_tbl_dma[qc->tag] & 0xffffffff);
@@ -1467,9 +1469,8 @@
 
 	mv_start_dma(ap, port_mmio, pp, qc->tf.protocol);
 
-	pp->req_idx++;
-
-	in_index = (pp->req_idx & MV_MAX_Q_DEPTH_MASK) << EDMA_REQ_Q_PTR_SHIFT;
+	pp->req_idx = (pp->req_idx + 1) & MV_MAX_Q_DEPTH_MASK;
+	in_index = pp->req_idx << EDMA_REQ_Q_PTR_SHIFT;
 
 	/* and write the request in pointer to kick the EDMA to life */
 	writelfl((pp->crqb_dma & EDMA_REQ_Q_BASE_LO_MASK) | in_index,
@@ -1603,70 +1604,72 @@
 	ata_qc_complete(qc);
 }
 
-static void mv_intr_edma(struct ata_port *ap)
+static void mv_process_crpb_response(struct ata_port *ap,
+		struct mv_crpb *response, unsigned int tag, int ncq_enabled)
+{
+	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, tag);
+
+	if (qc) {
+		u8 ata_status;
+		u16 edma_status = le16_to_cpu(response->flags);
+		/*
+		 * edma_status from a response queue entry:
+		 *   LSB is from EDMA_ERR_IRQ_CAUSE_OFS (non-NCQ only).
+		 *   MSB is saved ATA status from command completion.
+		 */
+		if (!ncq_enabled) {
+			u8 err_cause = edma_status & 0xff & ~EDMA_ERR_DEV;
+			if (err_cause) {
+				/*
+				 * Error will be seen/handled by mv_err_intr().
+				 * So do nothing at all here.
+				 */
+				return;
+			}
+		}
+		ata_status = edma_status >> CRPB_FLAG_STATUS_SHIFT;
+		qc->err_mask |= ac_err_mask(ata_status);
+		ata_qc_complete(qc);
+	} else {
+		ata_port_printk(ap, KERN_ERR, "%s: no qc for tag=%d\n",
+				__FUNCTION__, tag);
+	}
+}
+
+static void mv_process_crpb_entries(struct ata_port *ap, struct mv_port_priv *pp)
 {
 	void __iomem *port_mmio = mv_ap_base(ap);
 	struct mv_host_priv *hpriv = ap->host->private_data;
-	struct mv_port_priv *pp = ap->private_data;
-	struct ata_queued_cmd *qc;
-	u32 out_index, in_index;
+	u32 in_index;
 	bool work_done = false;
+	int ncq_enabled = (pp->pp_flags & MV_PP_FLAG_NCQ_EN);
 
-	/* get h/w response queue pointer */
+	/* Get the hardware queue position index */
 	in_index = (readl(port_mmio + EDMA_RSP_Q_IN_PTR_OFS)
 			>> EDMA_RSP_Q_PTR_SHIFT) & MV_MAX_Q_DEPTH_MASK;
 
-	while (1) {
-		u16 status;
+	/* Process new responses from since the last time we looked */
+	while (in_index != pp->resp_idx) {
 		unsigned int tag;
+		struct mv_crpb *response = &pp->crpb[pp->resp_idx];
 
-		/* get s/w response queue last-read pointer, and compare */
-		out_index = pp->resp_idx & MV_MAX_Q_DEPTH_MASK;
-		if (in_index == out_index)
-			break;
+		pp->resp_idx = (pp->resp_idx + 1) & MV_MAX_Q_DEPTH_MASK;
 
-		/* 50xx: get active ATA command */
-		if (IS_GEN_I(hpriv))
+		if (IS_GEN_I(hpriv)) {
+			/* 50xx: no NCQ, only one command active at a time */
 			tag = ap->link.active_tag;
-
-		/* Gen II/IIE: get active ATA command via tag, to enable
-		 * support for queueing.  this works transparently for
-		 * queued and non-queued modes.
-		 */
-		else
-			tag = le16_to_cpu(pp->crpb[out_index].id) & 0x1f;
-
-		qc = ata_qc_from_tag(ap, tag);
-
-		/* For non-NCQ mode, the lower 8 bits of status
-		 * are from EDMA_ERR_IRQ_CAUSE_OFS,
-		 * which should be zero if all went well.
-		 */
-		status = le16_to_cpu(pp->crpb[out_index].flags);
-		if ((status & 0xff) && !(pp->pp_flags & MV_PP_FLAG_NCQ_EN)) {
-			mv_err_intr(ap, qc);
-			return;
-		}
-
-		/* and finally, complete the ATA command */
-		if (qc) {
-			qc->err_mask |=
-				ac_err_mask(status >> CRPB_FLAG_STATUS_SHIFT);
-			ata_qc_complete(qc);
+		} else {
+			/* Gen II/IIE: get command tag from CRPB entry */
+			tag = le16_to_cpu(response->id) & 0x1f;
 		}
-
-		/* advance software response queue pointer, to
-		 * indicate (after the loop completes) to hardware
-		 * that we have consumed a response queue entry.
-		 */
+		mv_process_crpb_response(ap, response, tag, ncq_enabled);
 		work_done = true;
-		pp->resp_idx++;
 	}
 
 	/* Update the software queue position index in hardware */
 	if (work_done)
 		writelfl((pp->crpb_dma & EDMA_RSP_Q_BASE_LO_MASK) |
-			 (out_index << EDMA_RSP_Q_PTR_SHIFT),
+			 (pp->resp_idx << EDMA_RSP_Q_PTR_SHIFT),
 			 port_mmio + EDMA_RSP_Q_OUT_PTR_OFS);
 }
 
@@ -1744,7 +1747,7 @@
 
 		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
 			if ((DMA_IRQ << hardport) & hc_irq_cause)
-				mv_intr_edma(ap);
+				mv_process_crpb_entries(ap, pp);
 		} else {
 			if ((DEV_IRQ << hardport) & hc_irq_cause)
 				mv_intr_pio(ap);

REPOST: [PATCH 4/8] sata_mv simplify request/response queue handling

[Mark Lord]

Here it is again, minus the checkpatch.pl complaint: 

Try and simplify handling of the request/response queues.

Maintain the cached copies of queue indexes in a fully-masked state,
rather than having each use of them have to do the masking.

Split off handling of a single crpb response into a separate function,
to reduce complexity in the main mv_process_crpb_entries() routine.

Ignore the rarely-valid error bits from the crpb status field,
as we already handle that information in mv_err_intr().

For now, preserve the rest of the original logic.
A later patch will deal with fixing that separately.

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:06:31.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:21:28.000000000 -0400
@@ -800,7 +800,8 @@
 	/*
 	 * initialize request queue
 	 */
-	index = (pp->req_idx & MV_MAX_Q_DEPTH_MASK) << EDMA_REQ_Q_PTR_SHIFT;
+	pp->req_idx &= MV_MAX_Q_DEPTH_MASK;	/* paranoia */
+	index = pp->req_idx << EDMA_REQ_Q_PTR_SHIFT;
 
 	WARN_ON(pp->crqb_dma & 0x3ff);
 	writel((pp->crqb_dma >> 16) >> 16, port_mmio + EDMA_REQ_Q_BASE_HI_OFS);
@@ -816,7 +817,8 @@
 	/*
 	 * initialize response queue
 	 */
-	index = (pp->resp_idx & MV_MAX_Q_DEPTH_MASK) << EDMA_RSP_Q_PTR_SHIFT;
+	pp->resp_idx &= MV_MAX_Q_DEPTH_MASK;	/* paranoia */
+	index = pp->resp_idx << EDMA_RSP_Q_PTR_SHIFT;
 
 	WARN_ON(pp->crpb_dma & 0xff);
 	writel((pp->crpb_dma >> 16) >> 16, port_mmio + EDMA_RSP_Q_BASE_HI_OFS);
@@ -1308,7 +1310,7 @@
 	flags |= (qc->dev->link->pmp & 0xf) << CRQB_PMP_SHIFT;
 
 	/* get current queue index from software */
-	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;
+	in_index = pp->req_idx;
 
 	pp->crqb[in_index].sg_addr =
 		cpu_to_le32(pp->sg_tbl_dma[qc->tag] & 0xffffffff);
@@ -1400,7 +1402,7 @@
 	flags |= (qc->dev->link->pmp & 0xf) << CRQB_PMP_SHIFT;
 
 	/* get current queue index from software */
-	in_index = pp->req_idx & MV_MAX_Q_DEPTH_MASK;
+	in_index = pp->req_idx;
 
 	crqb = (struct mv_crqb_iie *) &pp->crqb[in_index];
 	crqb->addr = cpu_to_le32(pp->sg_tbl_dma[qc->tag] & 0xffffffff);
@@ -1467,9 +1469,8 @@
 
 	mv_start_dma(ap, port_mmio, pp, qc->tf.protocol);
 
-	pp->req_idx++;
-
-	in_index = (pp->req_idx & MV_MAX_Q_DEPTH_MASK) << EDMA_REQ_Q_PTR_SHIFT;
+	pp->req_idx = (pp->req_idx + 1) & MV_MAX_Q_DEPTH_MASK;
+	in_index = pp->req_idx << EDMA_REQ_Q_PTR_SHIFT;
 
 	/* and write the request in pointer to kick the EDMA to life */
 	writelfl((pp->crqb_dma & EDMA_REQ_Q_BASE_LO_MASK) | in_index,
@@ -1603,70 +1604,72 @@
 	ata_qc_complete(qc);
 }
 
-static void mv_intr_edma(struct ata_port *ap)
+static void mv_process_crpb_response(struct ata_port *ap,
+		struct mv_crpb *response, unsigned int tag, int ncq_enabled)
+{
+	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, tag);
+
+	if (qc) {
+		u8 ata_status;
+		u16 edma_status = le16_to_cpu(response->flags);
+		/*
+		 * edma_status from a response queue entry:
+		 *   LSB is from EDMA_ERR_IRQ_CAUSE_OFS (non-NCQ only).
+		 *   MSB is saved ATA status from command completion.
+		 */
+		if (!ncq_enabled) {
+			u8 err_cause = edma_status & 0xff & ~EDMA_ERR_DEV;
+			if (err_cause) {
+				/*
+				 * Error will be seen/handled by mv_err_intr().
+				 * So do nothing at all here.
+				 */
+				return;
+			}
+		}
+		ata_status = edma_status >> CRPB_FLAG_STATUS_SHIFT;
+		qc->err_mask |= ac_err_mask(ata_status);
+		ata_qc_complete(qc);
+	} else {
+		ata_port_printk(ap, KERN_ERR, "%s: no qc for tag=%d\n",
+				__func__, tag);
+	}
+}
+
+static void mv_process_crpb_entries(struct ata_port *ap, struct mv_port_priv *pp)
 {
 	void __iomem *port_mmio = mv_ap_base(ap);
 	struct mv_host_priv *hpriv = ap->host->private_data;
-	struct mv_port_priv *pp = ap->private_data;
-	struct ata_queued_cmd *qc;
-	u32 out_index, in_index;
+	u32 in_index;
 	bool work_done = false;
+	int ncq_enabled = (pp->pp_flags & MV_PP_FLAG_NCQ_EN);
 
-	/* get h/w response queue pointer */
+	/* Get the hardware queue position index */
 	in_index = (readl(port_mmio + EDMA_RSP_Q_IN_PTR_OFS)
 			>> EDMA_RSP_Q_PTR_SHIFT) & MV_MAX_Q_DEPTH_MASK;
 
-	while (1) {
-		u16 status;
+	/* Process new responses from since the last time we looked */
+	while (in_index != pp->resp_idx) {
 		unsigned int tag;
+		struct mv_crpb *response = &pp->crpb[pp->resp_idx];
 
-		/* get s/w response queue last-read pointer, and compare */
-		out_index = pp->resp_idx & MV_MAX_Q_DEPTH_MASK;
-		if (in_index == out_index)
-			break;
+		pp->resp_idx = (pp->resp_idx + 1) & MV_MAX_Q_DEPTH_MASK;
 
-		/* 50xx: get active ATA command */
-		if (IS_GEN_I(hpriv))
+		if (IS_GEN_I(hpriv)) {
+			/* 50xx: no NCQ, only one command active at a time */
 			tag = ap->link.active_tag;
-
-		/* Gen II/IIE: get active ATA command via tag, to enable
-		 * support for queueing.  this works transparently for
-		 * queued and non-queued modes.
-		 */
-		else
-			tag = le16_to_cpu(pp->crpb[out_index].id) & 0x1f;
-
-		qc = ata_qc_from_tag(ap, tag);
-
-		/* For non-NCQ mode, the lower 8 bits of status
-		 * are from EDMA_ERR_IRQ_CAUSE_OFS,
-		 * which should be zero if all went well.
-		 */
-		status = le16_to_cpu(pp->crpb[out_index].flags);
-		if ((status & 0xff) && !(pp->pp_flags & MV_PP_FLAG_NCQ_EN)) {
-			mv_err_intr(ap, qc);
-			return;
-		}
-
-		/* and finally, complete the ATA command */
-		if (qc) {
-			qc->err_mask |=
-				ac_err_mask(status >> CRPB_FLAG_STATUS_SHIFT);
-			ata_qc_complete(qc);
+		} else {
+			/* Gen II/IIE: get command tag from CRPB entry */
+			tag = le16_to_cpu(response->id) & 0x1f;
 		}
-
-		/* advance software response queue pointer, to
-		 * indicate (after the loop completes) to hardware
-		 * that we have consumed a response queue entry.
-		 */
+		mv_process_crpb_response(ap, response, tag, ncq_enabled);
 		work_done = true;
-		pp->resp_idx++;
 	}
 
 	/* Update the software queue position index in hardware */
 	if (work_done)
 		writelfl((pp->crpb_dma & EDMA_RSP_Q_BASE_LO_MASK) |
-			 (out_index << EDMA_RSP_Q_PTR_SHIFT),
+			 (pp->resp_idx << EDMA_RSP_Q_PTR_SHIFT),
 			 port_mmio + EDMA_RSP_Q_OUT_PTR_OFS);
 }
 
@@ -1744,7 +1747,7 @@
 
 		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
 			if ((DMA_IRQ << hardport) & hc_irq_cause)
-				mv_intr_edma(ap);
+				mv_process_crpb_entries(ap, pp);
 		} else {
 			if ((DEV_IRQ << hardport) & hc_irq_cause)
 				mv_intr_pio(ap);

[PATCH 5/8] sata_mv tidy host controller interrupt handling

[Mark Lord]

Tidy up host controller interrupt handling, by moving the weirdo
bit shifting from mv_interrupt() to mv_host_intr().

This lets us take advantage of the MV_PORT_TO_SHIFT_AND_HARDPORT() macro
from an earlier patch to greatly simplify the port numbering logic.

Also, defer reading the hc_irq_cause (one per hc) until it is
actually proven to be needed.  This may save a microsecond or
so per interrupt, on average (a later patchset will further reduce
unnecessary register reads throughout the driver).

Apart from that, we still leave the actual IRQ handling logic alone.
Subsequent patches in this series will address that.

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:21:28.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:36:17.000000000 -0400
@@ -1689,50 +1689,48 @@
  *      LOCKING:
  *      Inherited from caller.
  */
-static void mv_host_intr(struct ata_host *host, u32 relevant, unsigned int hc)
+static int mv_host_intr(struct ata_host *host, u32 main_cause)
 {
 	struct mv_host_priv *hpriv = host->private_data;
-	void __iomem *mmio = hpriv->base;
-	void __iomem *hc_mmio = mv_hc_base(mmio, hc);
-	u32 hc_irq_cause;
-	int port, port0, last_port;
-
-	if (hc == 0)
-		port0 = 0;
-	else
-		port0 = MV_PORTS_PER_HC;
-
-	if (HAS_PCI(host))
-		last_port = port0 + MV_PORTS_PER_HC;
-	else
-		last_port = port0 + hpriv->n_ports;
-	/* we'll need the HC success int register in most cases */
-	hc_irq_cause = readl(hc_mmio + HC_IRQ_CAUSE_OFS);
-	if (!hc_irq_cause)
-		return;
-
-	writelfl(~hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
+	void __iomem *mmio = hpriv->base, *hc_mmio = NULL;
+	u32 hc_irq_cause = 0;
+	unsigned int handled = 0, port;
 
-	VPRINTK("ENTER, hc%u relevant=0x%08x HC IRQ cause=0x%08x\n",
-		hc, relevant, hc_irq_cause);
-
-	for (port = port0; port < last_port; port++) {
+	for (port = 0; port < hpriv->n_ports; port++) {
 		struct ata_port *ap = host->ports[port];
 		struct mv_port_priv *pp;
-		int have_err_bits, hardport, shift;
-
-		if ((!ap) || (ap->flags & ATA_FLAG_DISABLED))
+		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:
+		 */
+		MV_PORT_TO_SHIFT_AND_HARDPORT(port,shift,hardport);
+		port_cause = (main_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.
+		 */
+		if (!hc_mmio) {
+			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);
+			handled = 1;
+		}
 
-		pp = ap->private_data;
-
-		shift = port << 1;		/* (port * 2) */
-		if (port >= MV_PORTS_PER_HC)
-			shift++;	/* skip bit 8 in the HC Main IRQ reg */
-
-		have_err_bits = ((ERR_IRQ << shift) & relevant);
-
-		if (unlikely(have_err_bits)) {
+		if (unlikely(port_cause & ERR_IRQ)) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->link.active_tag);
@@ -1743,8 +1741,7 @@
 			continue;
 		}
 
-		hardport = mv_hardport_from_port(port); /* range 0..3 */
-
+		pp = ap->private_data;
 		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
 			if ((DMA_IRQ << hardport) & hc_irq_cause)
 				mv_process_crpb_entries(ap, pp);
@@ -1753,10 +1750,10 @@
 				mv_intr_pio(ap);
 		}
 	}
-	VPRINTK("EXIT\n");
+	return handled;
 }
 
-static void mv_pci_error(struct ata_host *host, void __iomem *mmio)
+static int mv_pci_error(struct ata_host *host, void __iomem *mmio)
 {
 	struct mv_host_priv *hpriv = host->private_data;
 	struct ata_port *ap;
@@ -1794,6 +1791,7 @@
 			ata_port_freeze(ap);
 		}
 	}
+	return 1;	/* handled */
 }
 
 /**
@@ -1814,8 +1812,7 @@
 {
 	struct ata_host *host = dev_instance;
 	struct mv_host_priv *hpriv = host->private_data;
-	unsigned int hc, handled = 0, n_hcs;
-	void __iomem *mmio = hpriv->base;
+	unsigned int handled = 0;
 	u32 main_cause, main_mask;
 
 	spin_lock(&host->lock);
@@ -1825,26 +1822,12 @@
 	 * Deal with cases where we either have nothing pending, or have read
 	 * a bogus register value which can indicate HW removal or PCI fault.
 	 */
-	if (!(main_cause & main_mask) || (main_cause == 0xffffffffU))
-		goto out_unlock;
-
-	n_hcs = mv_get_hc_count(host->ports[0]->flags);
-
-	if (unlikely((main_cause & PCI_ERR) && HAS_PCI(host))) {
-		mv_pci_error(host, mmio);
-		handled = 1;
-		goto out_unlock;	/* skip all other HC irq handling */
-	}
-
-	for (hc = 0; hc < n_hcs; hc++) {
-		u32 relevant = main_cause & (HC0_IRQ_PEND << (hc * HC_SHIFT));
-		if (relevant) {
-			mv_host_intr(host, relevant, hc);
-			handled = 1;
-		}
+	if ((main_cause & main_mask) && (main_cause != 0xffffffffU)) {
+		if (unlikely((main_cause & PCI_ERR) && HAS_PCI(host)))
+			handled = mv_pci_error(host, hpriv->base);
+		else
+			handled = mv_host_intr(host, main_cause);
 	}
-
-out_unlock:
 	spin_unlock(&host->lock);
 	return IRQ_RETVAL(handled);
 }

REPOST: [PATCH 5/8] sata_mv tidy host controller interrupt handling

[Mark Lord]

Here it is again, minus the checkpatch.pl complaint: 

Tidy up host controller interrupt handling, by moving the weirdo
bit shifting from mv_interrupt() to mv_host_intr().

This lets us take advantage of the MV_PORT_TO_SHIFT_AND_HARDPORT() macro
from an earlier patch to greatly simplify the port numbering logic.

Also, defer reading the hc_irq_cause (one per hc) until it is
actually proven to be needed.  This may save a microsecond or
so per interrupt, on average (a later patchset will further reduce
unnecessary register reads throughout the driver).

Apart from that, we still leave the actual IRQ handling logic alone.
Subsequent patches in this series will address that.

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:21:28.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:36:17.000000000 -0400
@@ -1689,50 +1689,48 @@
  *      LOCKING:
  *      Inherited from caller.
  */
-static void mv_host_intr(struct ata_host *host, u32 relevant, unsigned int hc)
+static int mv_host_intr(struct ata_host *host, u32 main_cause)
 {
 	struct mv_host_priv *hpriv = host->private_data;
-	void __iomem *mmio = hpriv->base;
-	void __iomem *hc_mmio = mv_hc_base(mmio, hc);
-	u32 hc_irq_cause;
-	int port, port0, last_port;
-
-	if (hc == 0)
-		port0 = 0;
-	else
-		port0 = MV_PORTS_PER_HC;
-
-	if (HAS_PCI(host))
-		last_port = port0 + MV_PORTS_PER_HC;
-	else
-		last_port = port0 + hpriv->n_ports;
-	/* we'll need the HC success int register in most cases */
-	hc_irq_cause = readl(hc_mmio + HC_IRQ_CAUSE_OFS);
-	if (!hc_irq_cause)
-		return;
-
-	writelfl(~hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
+	void __iomem *mmio = hpriv->base, *hc_mmio = NULL;
+	u32 hc_irq_cause = 0;
+	unsigned int handled = 0, port;
 
-	VPRINTK("ENTER, hc%u relevant=0x%08x HC IRQ cause=0x%08x\n",
-		hc, relevant, hc_irq_cause);
-
-	for (port = port0; port < last_port; port++) {
+	for (port = 0; port < hpriv->n_ports; port++) {
 		struct ata_port *ap = host->ports[port];
 		struct mv_port_priv *pp;
-		int have_err_bits, hardport, shift;
-
-		if ((!ap) || (ap->flags & ATA_FLAG_DISABLED))
+		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:
+		 */
+		MV_PORT_TO_SHIFT_AND_HARDPORT(port, shift, hardport);
+		port_cause = (main_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.
+		 */
+		if (!hc_mmio) {
+			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);
+			handled = 1;
+		}
 
-		pp = ap->private_data;
-
-		shift = port << 1;		/* (port * 2) */
-		if (port >= MV_PORTS_PER_HC)
-			shift++;	/* skip bit 8 in the HC Main IRQ reg */
-
-		have_err_bits = ((ERR_IRQ << shift) & relevant);
-
-		if (unlikely(have_err_bits)) {
+		if (unlikely(port_cause & ERR_IRQ)) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->link.active_tag);
@@ -1743,8 +1741,7 @@
 			continue;
 		}
 
-		hardport = mv_hardport_from_port(port); /* range 0..3 */
-
+		pp = ap->private_data;
 		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
 			if ((DMA_IRQ << hardport) & hc_irq_cause)
 				mv_process_crpb_entries(ap, pp);
@@ -1753,10 +1750,10 @@
 				mv_intr_pio(ap);
 		}
 	}
-	VPRINTK("EXIT\n");
+	return handled;
 }
 
-static void mv_pci_error(struct ata_host *host, void __iomem *mmio)
+static int mv_pci_error(struct ata_host *host, void __iomem *mmio)
 {
 	struct mv_host_priv *hpriv = host->private_data;
 	struct ata_port *ap;
@@ -1794,6 +1791,7 @@
 			ata_port_freeze(ap);
 		}
 	}
+	return 1;	/* handled */
 }
 
 /**
@@ -1814,8 +1812,7 @@
 {
 	struct ata_host *host = dev_instance;
 	struct mv_host_priv *hpriv = host->private_data;
-	unsigned int hc, handled = 0, n_hcs;
-	void __iomem *mmio = hpriv->base;
+	unsigned int handled = 0;
 	u32 main_cause, main_mask;
 
 	spin_lock(&host->lock);
@@ -1825,26 +1822,12 @@
 	 * Deal with cases where we either have nothing pending, or have read
 	 * a bogus register value which can indicate HW removal or PCI fault.
 	 */
-	if (!(main_cause & main_mask) || (main_cause == 0xffffffffU))
-		goto out_unlock;
-
-	n_hcs = mv_get_hc_count(host->ports[0]->flags);
-
-	if (unlikely((main_cause & PCI_ERR) && HAS_PCI(host))) {
-		mv_pci_error(host, mmio);
-		handled = 1;
-		goto out_unlock;	/* skip all other HC irq handling */
-	}
-
-	for (hc = 0; hc < n_hcs; hc++) {
-		u32 relevant = main_cause & (HC0_IRQ_PEND << (hc * HC_SHIFT));
-		if (relevant) {
-			mv_host_intr(host, relevant, hc);
-			handled = 1;
-		}
+	if ((main_cause & main_mask) && (main_cause != 0xffffffffU)) {
+		if (unlikely((main_cause & PCI_ERR) && HAS_PCI(host)))
+			handled = mv_pci_error(host, hpriv->base);
+		else
+			handled = mv_host_intr(host, main_cause);
 	}
-
-out_unlock:
 	spin_unlock(&host->lock);
 	return IRQ_RETVAL(handled);
 }

[PATCH 6/8] sata_mv more interrupt handling rework

[Mark Lord]

Continue fixing the interrupt handling logic.

Get rid of mv_intr_pio(), by using ata_sff_host_intr() for PIO..

Add a mv_unexpected_intr() catch-all for "impossible" scenarios,
where we get an interrupt that shouldn't have happened
(never seen in testing, but just in case..).

Rearrange the logic so that we always process completed
response queue entries before looking for other events,
This avoids having to re-issue commands that had already succeeded.

As part of this, we split out some duplicated functionality
into a new function, mv_get_active_qc().

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:36:17.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:47:23.000000000 -0400
@@ -1479,6 +1479,43 @@
 	return 0;
 }
 
+static struct ata_queued_cmd *mv_get_active_qc(struct ata_port *ap)
+{
+	struct mv_port_priv *pp = ap->private_data;
+	struct ata_queued_cmd *qc;
+
+	if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
+		return NULL;
+	qc = ata_qc_from_tag(ap, ap->link.active_tag);
+	if (qc && (qc->tf.flags & ATA_TFLAG_POLLING))
+		qc = NULL;
+	return qc;
+}
+
+static void mv_unexpected_intr(struct ata_port *ap)
+{
+	struct mv_port_priv *pp = ap->private_data;
+	struct ata_eh_info *ehi = &ap->link.eh_info;
+	char *when = "";
+
+	/*
+	 * 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";
+	} 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";
+	}
+	ata_ehi_push_desc(ehi, "unexpected device interrupt%s", when);
+	ehi->err_mask |= AC_ERR_OTHER;
+	ehi->action   |= ATA_EH_RESET;
+	ata_port_freeze(ap);
+}
+
 /**
  *      mv_err_intr - Handle error interrupts on the port
  *      @ap: ATA channel to manipulate
@@ -1582,28 +1619,6 @@
 		ata_port_abort(ap);
 }
 
-static void mv_intr_pio(struct ata_port *ap)
-{
-	struct ata_queued_cmd *qc;
-	u8 ata_status;
-
-	/* ignore spurious intr if drive still BUSY */
-	ata_status = readb(ap->ioaddr.status_addr);
-	if (unlikely(ata_status & ATA_BUSY))
-		return;
-
-	/* get active ATA command */
-	qc = ata_qc_from_tag(ap, ap->link.active_tag);
-	if (unlikely(!qc))			/* no active tag */
-		return;
-	if (qc->tf.flags & ATA_TFLAG_POLLING)	/* polling; we don't own qc */
-		return;
-
-	/* and finally, complete the ATA command */
-	qc->err_mask |= ac_err_mask(ata_status);
-	ata_qc_complete(qc);
-}
-
 static void mv_process_crpb_response(struct ata_port *ap,
 		struct mv_crpb *response, unsigned int tag, int ncq_enabled)
 {
@@ -1676,15 +1691,7 @@
 /**
  *      mv_host_intr - Handle all interrupts on the given host controller
  *      @host: host specific structure
- *      @relevant: port error bits relevant to this host controller
- *      @hc: which host controller we're to look at
- *
- *      Read then write clear the HC interrupt status then walk each
- *      port connected to the HC and see if it needs servicing.  Port
- *      success ints are reported in the HC interrupt status reg, the
- *      port error ints are reported in the higher level main
- *      interrupt status register and thus are passed in via the
- *      'relevant' argument.
+ *      @main_cause: Main interrupt cause register for the chip.
  *
  *      LOCKING:
  *      Inherited from caller.
@@ -1729,25 +1736,28 @@
 			writelfl(~hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
 			handled = 1;
 		}
-
-		if (unlikely(port_cause & ERR_IRQ)) {
-			struct ata_queued_cmd *qc;
-
-			qc = ata_qc_from_tag(ap, ap->link.active_tag);
-			if (qc && (qc->tf.flags & ATA_TFLAG_POLLING))
-				continue;
-
-			mv_err_intr(ap, qc);
-			continue;
-		}
-
+		/*
+		 * Process completed CRPB response(s) before other events.
+		 */
 		pp = ap->private_data;
-		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
-			if ((DMA_IRQ << hardport) & hc_irq_cause)
+		if (hc_irq_cause & (DMA_IRQ << hardport)) {
+			if (pp->pp_flags & MV_PP_FLAG_EDMA_EN)
 				mv_process_crpb_entries(ap, pp);
-		} else {
-			if ((DEV_IRQ << hardport) & hc_irq_cause)
-				mv_intr_pio(ap);
+		}
+		/*
+		 * 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));
+		} 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);
+				if (qc) {
+					ata_sff_host_intr(ap, qc);
+					continue;
+				}
+			}
+			mv_unexpected_intr(ap);
 		}
 	}
 	return handled;

[PATCH 7/8] sata_mv leave SError bits untouched in mv_err_intr

[Mark Lord]

Rework mv_err_intr() to leave the SError bits as-is,
so that libata-eh has a chance to see/use them.

We originally thought that clearing them here was necessary
before writing back to edma_err_cause (per the Marvell datasheets),
but we will end up reseting the chip regardless in those cases.

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:47:23.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:53:33.000000000 -0400
@@ -1519,13 +1519,11 @@
 /**
  *      mv_err_intr - Handle error interrupts on the port
  *      @ap: ATA channel to manipulate
- *      @reset_allowed: bool: 0 == don't trigger from reset here
+ *      @qc: affected command (non-NCQ), or NULL
  *
- *      In most cases, just clear the interrupt and move on.  However,
- *      some cases require an eDMA reset, which also performs a COMRESET.
- *      The SERR case requires a clear of pending errors in the SATA
- *      SERROR register.  Finally, if the port disabled DMA,
- *      update our cached copy to match.
+ *      Most cases require a full reset of the chip's state machine,
+ *      which also performs a COMRESET.
+ *      Also, if the port disabled DMA, update our cached copy to match.
  *
  *      LOCKING:
  *      Inherited from caller.
@@ -1536,21 +1534,18 @@
 	u32 edma_err_cause, eh_freeze_mask, serr = 0;
 	struct mv_port_priv *pp = ap->private_data;
 	struct mv_host_priv *hpriv = ap->host->private_data;
-	unsigned int edma_enabled = (pp->pp_flags & MV_PP_FLAG_EDMA_EN);
 	unsigned int action = 0, err_mask = 0;
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 
 	ata_ehi_clear_desc(ehi);
 
-	if (!edma_enabled) {
-		/* just a guess: do we need to do this? should we
-		 * expand this, and do it in all cases?
-		 */
-		sata_scr_read(&ap->link, SCR_ERROR, &serr);
-		sata_scr_write_flush(&ap->link, SCR_ERROR, serr);
-	}
-
+	/*
+	 * 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.
+	 */
 	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);
 
@@ -1590,16 +1585,19 @@
 			ata_ehi_push_desc(ehi, "EDMA self-disable");
 		}
 		if (edma_err_cause & EDMA_ERR_SERR) {
-			sata_scr_read(&ap->link, SCR_ERROR, &serr);
-			sata_scr_write_flush(&ap->link, SCR_ERROR, serr);
-			err_mask = AC_ERR_ATA_BUS;
+			/*
+			 * Ensure that we only 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;
 		}
 	}
 
-	/* Clear EDMA now that SERR cleanup done */
-	writelfl(~edma_err_cause, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
-
 	if (!err_mask) {
 		err_mask = AC_ERR_OTHER;
 		action |= ATA_EH_RESET;

REPOST: [PATCH 7/8] sata_mv leave SError bits untouched in mv_err_intr

[Mark Lord]

Here it is again, minus the checkpatch.pl complaint: 

Rework mv_err_intr() to leave the SError bits as-is,
so that libata-eh has a chance to see/use them.

We originally thought that clearing them here was necessary
before writing back to edma_err_cause (per the Marvell datasheets),
but we will end up reseting the chip regardless in those cases.

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:47:23.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:53:33.000000000 -0400
@@ -1519,13 +1519,11 @@
 /**
  *      mv_err_intr - Handle error interrupts on the port
  *      @ap: ATA channel to manipulate
- *      @reset_allowed: bool: 0 == don't trigger from reset here
+ *      @qc: affected command (non-NCQ), or NULL
  *
- *      In most cases, just clear the interrupt and move on.  However,
- *      some cases require an eDMA reset, which also performs a COMRESET.
- *      The SERR case requires a clear of pending errors in the SATA
- *      SERROR register.  Finally, if the port disabled DMA,
- *      update our cached copy to match.
+ *      Most cases require a full reset of the chip's state machine,
+ *      which also performs a COMRESET.
+ *      Also, if the port disabled DMA, update our cached copy to match.
  *
  *      LOCKING:
  *      Inherited from caller.
@@ -1536,21 +1534,18 @@
 	u32 edma_err_cause, eh_freeze_mask, serr = 0;
 	struct mv_port_priv *pp = ap->private_data;
 	struct mv_host_priv *hpriv = ap->host->private_data;
-	unsigned int edma_enabled = (pp->pp_flags & MV_PP_FLAG_EDMA_EN);
 	unsigned int action = 0, err_mask = 0;
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 
 	ata_ehi_clear_desc(ehi);
 
-	if (!edma_enabled) {
-		/* just a guess: do we need to do this? should we
-		 * expand this, and do it in all cases?
-		 */
-		sata_scr_read(&ap->link, SCR_ERROR, &serr);
-		sata_scr_write_flush(&ap->link, SCR_ERROR, serr);
-	}
-
+	/*
+	 * 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.
+	 */
 	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);
 
@@ -1590,16 +1585,19 @@
 			ata_ehi_push_desc(ehi, "EDMA self-disable");
 		}
 		if (edma_err_cause & EDMA_ERR_SERR) {
-			sata_scr_read(&ap->link, SCR_ERROR, &serr);
-			sata_scr_write_flush(&ap->link, SCR_ERROR, serr);
-			err_mask = AC_ERR_ATA_BUS;
+			/*
+			 * 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;
 		}
 	}
 
-	/* Clear EDMA now that SERR cleanup done */
-	writelfl(~edma_err_cause, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
-
 	if (!err_mask) {
 		err_mask = AC_ERR_OTHER;
 		action |= ATA_EH_RESET;

[PATCH 8/8] sata_mv re-enable hotplug, update TODO list

[Mark Lord]

Re-enable hotplug, now that the interrupt/error handling are mostly sane.
Also update the TODO list at the top.

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

--- old/drivers/ata/sata_mv.c	2008-04-19 13:53:33.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-04-19 13:56:32.000000000 -0400
@@ -23,46 +23,34 @@
  */
 
 /*
-  sata_mv TODO list:
-
-  1) Needs a full errata audit for all chipsets.  I implemented most
-  of the errata workarounds found in the Marvell vendor driver, but
-  I distinctly remember a couple workarounds (one related to PCI-X)
-  are still needed.
-
-  2) Improve/fix IRQ and error handling sequences.
-
-  3) ATAPI support (Marvell claims the 60xx/70xx chips can do it).
-
-  4) Think about TCQ support here, and for libata in general
-  with controllers that suppport it via host-queuing hardware
-  (a software-only implementation could be a nightmare).
-
-  5) Investigate problems with PCI Message Signalled Interrupts (MSI).
-
-  6) Cache frequently-accessed registers in mv_port_priv to reduce overhead.
-
-  7) Fix/reenable hot plug/unplug (should happen as a side-effect of (2) above).
-
-  8) Develop a low-power-consumption strategy, and implement it.
-
-  9) [Experiment, low priority] See if ATAPI can be supported using
-  "unknown FIS" or "vendor-specific FIS" support, or something creative
-  like that.
-
-  10) [Experiment, low priority] Investigate interrupt coalescing.
-  Quite often, especially with PCI Message Signalled Interrupts (MSI),
-  the overhead reduced by interrupt mitigation is quite often not
-  worth the latency cost.
-
-  11) [Experiment, Marvell value added] Is it possible to use target
-  mode to cross-connect two Linux boxes with Marvell cards?  If so,
-  creating LibATA target mode support would be very interesting.
-
-  Target mode, for those without docs, is the ability to directly
-  connect two SATA controllers.
-
-*/
+ * sata_mv TODO list:
+ *
+ * --> Errata workaround for NCQ device errors.
+ *
+ * --> More errata workarounds for PCI-X.
+ *
+ * --> Complete a full errata audit for all chipsets to identify others.
+ *
+ * --> ATAPI support (Marvell claims the 60xx/70xx chips can do it).
+ *
+ * --> 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.
+ *       Quite often, especially with PCI Message Signalled Interrupts (MSI),
+ *       the overhead reduced by interrupt mitigation is quite often not
+ *       worth the latency cost.
+ *
+ * --> [Experiment, Marvell value added] Is it possible to use target
+ *       mode to cross-connect two Linux boxes with Marvell cards?  If so,
+ *       creating LibATA target mode support would be very interesting.
+ *
+ *       Target mode, for those without docs, is the ability to directly
+ *       connect two SATA ports.
+ */
 
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -299,9 +287,7 @@
 	EDMA_ERR_IRQ_TRANSIENT  = EDMA_ERR_LNK_CTRL_RX_0 |
 				  EDMA_ERR_LNK_CTRL_RX_1 |
 				  EDMA_ERR_LNK_CTRL_RX_3 |
-				  EDMA_ERR_LNK_CTRL_TX |
-				 /* temporary, until we fix hotplug: */
-				 (EDMA_ERR_DEV_DCON | EDMA_ERR_DEV_CON),
+				  EDMA_ERR_LNK_CTRL_TX,
 
 	EDMA_EH_FREEZE		= EDMA_ERR_D_PAR |
 				  EDMA_ERR_PRD_PAR |
@@ -2120,13 +2106,6 @@
 		printk(KERN_ERR DRV_NAME ": can't clear global reset\n");
 		rc = 1;
 	}
-	/*
-	 * Temporary: wait 3 seconds before port-probing can happen,
-	 * so that we don't miss finding sleepy SilXXXX port-multipliers.
-	 * This can go away once hotplug is fully/correctly implemented.
-	 */
-	if (rc == 0)
-		msleep(3000);
 done:
 	return rc;
 }

Re: [PATCH 8/8] sata_mv re-enable hotplug, update TODO list

[Mark Lord]

Mark Lord wrote:
> Re-enable hotplug, now that the interrupt/error handling are mostly sane.
> Also update the TODO list at the top.
..

The checkpatch.pl script went totally berserk on the TODO list.
I'm ignoring it -- checkpatch.pl shouldn't be reading comments anyhow.

:)

Re: [PATCH 0/8] sata_mv interrupt/eh fixes etc..

[Mark Lord]

Mark Lord wrote:
> Here are eight more patches for sata_mv.
> 
> The focus this time is on a revamp of the interrupt and error handling code,
> and re-enabling hotplug (primary ports only) at the end.
> 
> The Marvell chips cannot support Asynchronous Notification (AN)
> due to errata issues, so hotplug on PM ports is not supported for now.
> Hotplug on PM ports would require Tejun's HP polling framework.
..

Jeff?  Are you waiting for a complete series repost or something,
or just as busy as the rest of us are right now?  :)

Thanks