[PATCH v5] Quirk for IVB graphics FLR errata

[PATCH v5] Quirk for IVB graphics FLR errata

[Hao, Xudong]

For IvyBridge Mobile platform, a system hang may occur if a FLR(Function Level Reset) is asserted to internal graphics.

This quirk patch is workaround for the IVB FLR errata issue.
We are disabling the FLR reset handshake between the PCH and CPU display, then manually powering down the panel power sequencing and resetting the PCH display.

Changes from v4: (based on Matthew and Don's comments)
- using jiffies to set timeout instead of tsc.
- correct type mmio_base variable
- using readl() and writel() to access io.
- correct code style, use spaces around the multiply operator.

Signed-off-by: Xudong Hao <xudong.hao@intel.com>
Signed-off-by: Kay, Allen M <allen.m.kay@intel.com>
Reviewed-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
 drivers/pci/quirks.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 4bf7102..136d3c4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -29,6 +29,7 @@
 #include <linux/sched.h>
 #include <linux/ktime.h>
 #include <asm/dma.h>	/* isa_dma_bridge_buggy */
+#include <asm/io.h>
 #include "pci.h"
 
 /*
@@ -3085,11 +3086,63 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)
 	return 0;
 }
 
+#include "../gpu/drm/i915/i915_reg.h"
+#define MSG_CTL	0x45010
+#define IGD_OPERATION_TIMEOUT 10000     /* set timeout 10 seconds */
+
+static int reset_ivb_igd(struct pci_dev *dev, int probe) {
+	void __iomem *mmio_base;
+	unsigned long timeout;
+	u32 val;
+
+	if (probe)
+		return 0;
+
+	mmio_base = ioremap_nocache(pci_resource_start(dev, 0),
+				 pci_resource_len(dev, 0));
+	if (!mmio_base)
+		return -ENOMEM;
+
+	/* Work Around */
+	writel(0x00000002, mmio_base + MSG_CTL);
+	/* Clobbering SOUTH_CHICKEN2 register is fine only if the next
+	 * driver loaded sets the right bits. However, this's a reset and
+	 * the bits have been set by i915 previously, so we clobber
+	 * SOUTH_CHICKEN2 register directly here.
+	 */
+	writel(0x00000005, mmio_base + SOUTH_CHICKEN2);
+	val = readl(mmio_base + PCH_PP_CONTROL) & 0xfffffffe;
+	writel(val, mmio_base + PCH_PP_CONTROL);
+	do {
+		timeout = jiffies + msecs_to_jiffies(IGD_OPERATION_TIMEOUT);
+		while (1) {
+			val = readl(mmio_base + PCH_PP_STATUS);
+			if (((val & 0x80000000) == 0)
+				&& ((val & 0x30000000) == 0))
+				break;
+			if (time_after(jiffies, timeout))
+				break;
+			cpu_relax();
+		}
+	} while (0);
+	writel(0x00000002, mmio_base + 0xd0100);
+
+	iounmap(pci_resource_start(dev, 0));
+	return 0;
+}
+
 #define PCI_DEVICE_ID_INTEL_82599_SFP_VF   0x10ed
+#define PCI_DEVICE_ID_INTEL_IVB_M_VGA      0x0156
+#define PCI_DEVICE_ID_INTEL_IVB_M2_VGA     0x0166
 
 static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
 		 reset_intel_82599_sfp_virtfn },
+	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IVB_M_VGA,
+		reset_ivb_igd },
+	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IVB_M2_VGA,
+		reset_ivb_igd },
 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
 		reset_intel_generic_dev },
 	{ 0 }
--
1.6.0.rc1

Re: [PATCH v5] Quirk for IVB graphics FLR errata

[Matthew Wilcox]

On Fri, Apr 13, 2012 at 08:22:18AM +0000, Hao, Xudong wrote:
> Changes from v4: (based on Matthew and Don's comments)
> - using jiffies to set timeout instead of tsc.
> - correct type mmio_base variable
> - using readl() and writel() to access io.
> - correct code style, use spaces around the multiply operator.

Much better!  Just a couple of minor nits ...

> +	val = readl(mmio_base + PCH_PP_CONTROL) & 0xfffffffe;
> +	writel(val, mmio_base + PCH_PP_CONTROL);
> +	do {
> +		timeout = jiffies + msecs_to_jiffies(IGD_OPERATION_TIMEOUT);
> +		while (1) {

Personally, I'd write this as:

	while (time_before(jiffies, timeout)) {
		...
	}

> +			val = readl(mmio_base + PCH_PP_STATUS);
> +			if (((val & 0x80000000) == 0)
> +				&& ((val & 0x30000000) == 0))

Is there a reason why this shouldn't be:

		if ((val & 0xB0000000) == 0)

> +				break;
> +			if (time_after(jiffies, timeout))
> +				break;
> +			cpu_relax();
> +		}
> +	} while (0);

Why do we need this to be in a do { } while (0) loop?

Putting those three suggestions together, I think it should look like this:

	writel(val, mmio_base + PCH_PP_CONTROL);
	timeout = jiffies + msecs_to_jiffies(IGD_OPERATION_TIMEOUT);
	while (time_before(jiffies, timeout)) {
		val = readl(mmio_base + PCH_PP_STATUS);
		if ((val & 0xB0000000) == 0)
			break;
		cpu_relax();
	}
	writel(0x00000002, mmio_base + 0xd0100);

With that change, please add:

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

RE: [PATCH v5] Quirk for IVB graphics FLR errata

[Hao, Xudong]

> -----Original Message-----
> From: Matthew Wilcox [mailto:matthew@wil.cx]
> Sent: Sunday, April 15, 2012 4:41 AM
> To: Hao, Xudong
> Cc: Bjorn Helgaas; linux-pci@vger.kernel.org; Don Dutile
> Subject: Re: [PATCH v5] Quirk for IVB graphics FLR errata
> 
> On Fri, Apr 13, 2012 at 08:22:18AM +0000, Hao, Xudong wrote:
> > Changes from v4: (based on Matthew and Don's comments)
> > - using jiffies to set timeout instead of tsc.
> > - correct type mmio_base variable
> > - using readl() and writel() to access io.
> > - correct code style, use spaces around the multiply operator.
> 
> Much better!  Just a couple of minor nits ...
> 
> > +	val = readl(mmio_base + PCH_PP_CONTROL) & 0xfffffffe;
> > +	writel(val, mmio_base + PCH_PP_CONTROL);
> > +	do {
> > +		timeout = jiffies + msecs_to_jiffies(IGD_OPERATION_TIMEOUT);
> > +		while (1) {
> 
> Personally, I'd write this as:
> 
> 	while (time_before(jiffies, timeout)) {
> 		...
> 	}
> 
> > +			val = readl(mmio_base + PCH_PP_STATUS);
> > +			if (((val & 0x80000000) == 0)
> > +				&& ((val & 0x30000000) == 0))
> 
> Is there a reason why this shouldn't be:
> 
> 		if ((val & 0xB0000000) == 0)
> 
> > +				break;
> > +			if (time_after(jiffies, timeout))
> > +				break;
> > +			cpu_relax();
> > +		}
> > +	} while (0);
> 
> Why do we need this to be in a do { } while (0) loop?
> 
> Putting those three suggestions together, I think it should look like this:
> 
> 	writel(val, mmio_base + PCH_PP_CONTROL);
> 	timeout = jiffies + msecs_to_jiffies(IGD_OPERATION_TIMEOUT);
> 	while (time_before(jiffies, timeout)) {
> 		val = readl(mmio_base + PCH_PP_STATUS);
> 		if ((val & 0xB0000000) == 0)
> 			break;
> 		cpu_relax();
> 	}
> 	writel(0x00000002, mmio_base + 0xd0100);
> 

The code looks simple and reasonable, I'll modify with changes and add your name in Signed-off-by.

> With that change, please add:
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> 
> --
> Matthew Wilcox				Intel Open Source Technology Centre
> "Bill, look, we understand that you're interested in selling us this operating
> system, but compare it to ours.  We can't possibly take such a retrograde
> step."