| Message ID | 403610A45A2B5242BD291EDAE8B37D300FD0FF14@SHSMSX102.ccr.corp.intel.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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>
> -----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." -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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