[PATCH V4] Quirk for IVB graphics FLR errata

[PATCH V4] Quirk for IVB graphics FLR errata

[Hao, Xudong]

(Re-rend.)

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 v3:
- add X86 configuration to avoid compile problem in other architecture.

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 |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 6476547..63e8b70 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3069,11 +3069,70 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)
 	return 0;
 }
 
+#ifdef CONFIG_X86
+
+#include "../gpu/drm/i915/i915_reg.h"
+#define MSG_CTL	0x45010
+
+static const int op_timeout = 10;	/* set timeout 10 seconds */
+static int reset_ivb_igd(struct pci_dev *dev, int probe) {
+	u8 *mmio_base;
+	u32 val;
+	cycles_t cyc_op_timeout = tsc_khz*op_timeout*1000;
+
+	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 */
+	*((u32 *)(mmio_base + MSG_CTL)) = 0x00000002;
+	/* 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.
+	 */
+	*((u32 *)(mmio_base + SOUTH_CHICKEN2)) = 0x00000005;
+	val = *((u32 *)(mmio_base + PCH_PP_CONTROL)) & 0xfffffffe;
+	*((u32 *)(mmio_base + PCH_PP_CONTROL)) = val;
+	do {
+		cycles_t start_time = get_cycles();
+		while (1) {
+			val = *((u32 *)(mmio_base + PCH_PP_STATUS));
+			if (((val & 0x80000000) == 0)
+				&& ((val & 0x30000000) == 0))
+				break;
+			if (cyc_op_timeout < (get_cycles() - start_time))
+				break;
+			cpu_relax();
+		}
+	} while (0);
+	*((u32 *)(mmio_base + 0xd0100)) = 0x00000002;
+
+	iounmap(pci_resource_start(dev, 0));
+	return 0;
+}
+#else
+static int reset_ivb_igd(struct pci_dev *dev, int probe) { }
+#endif	/* CONFIG_X86 */
+
 #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 }

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

[Matthew Wilcox]

On Wed, Apr 11, 2012 at 06:03:43AM +0000, Hao, Xudong wrote:
> +#ifdef CONFIG_X86
> +
> +#include "../gpu/drm/i915/i915_reg.h"
> +#define MSG_CTL	0x45010

There's a strange mixture of constants from i915_reg.h, defined constants
here and bare constants below.  I don't mind which get used, but the
mixture is bizarre.

> +static const int op_timeout = 10;	/* set timeout 10 seconds */
> +static int reset_ivb_igd(struct pci_dev *dev, int probe) {
> +	u8 *mmio_base;
> +	u32 val;
> +	cycles_t cyc_op_timeout = tsc_khz*op_timeout*1000;

Style: use spaces around the multiply operator:
	cycles_t cyc_op_timeout = tsc_khz * op_timeout * 1000;

> +	if (probe)
> +		return 0;
> +
> +	mmio_base = ioremap_nocache(pci_resource_start(dev, 0),
> +				 pci_resource_len(dev, 0));

mmio_base should have type void __iomem *.

> +	if (!mmio_base)
> +		return -ENOMEM;
> +
> +	/* Work Around */
> +	*((u32 *)(mmio_base + MSG_CTL)) = 0x00000002;

Why are you not using writel() here? (and readl() in other places)

> +	/* 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.
> +	 */
> +	*((u32 *)(mmio_base + SOUTH_CHICKEN2)) = 0x00000005;
> +	val = *((u32 *)(mmio_base + PCH_PP_CONTROL)) & 0xfffffffe;
> +	*((u32 *)(mmio_base + PCH_PP_CONTROL)) = val;
> +	do {
> +		cycles_t start_time = get_cycles();
> +		while (1) {
> +			val = *((u32 *)(mmio_base + PCH_PP_STATUS));
> +			if (((val & 0x80000000) == 0)
> +				&& ((val & 0x30000000) == 0))
> +				break;
> +			if (cyc_op_timeout < (get_cycles() - start_time))
> +				break;
> +			cpu_relax();
> +		}
> +	} while (0);
> +	*((u32 *)(mmio_base + 0xd0100)) = 0x00000002;
> +
> +	iounmap(pci_resource_start(dev, 0));
> +	return 0;
> +}
> +#else
> +static int reset_ivb_igd(struct pci_dev *dev, int probe) { }
> +#endif	/* CONFIG_X86 */
> +
>  #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 }
> 
> 
> --
> 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

-- 
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 V4] Quirk for IVB graphics FLR errata

[Hao, Xudong]

> -----Original Message-----
> From: Matthew Wilcox [mailto:matthew@wil.cx]
> Sent: Wednesday, April 11, 2012 8:23 PM
> To: Hao, Xudong
> Cc: Bjorn Helgaas; linux-pci@vger.kernel.org
> Subject: Re: [PATCH V4] Quirk for IVB graphics FLR errata
> 
> On Wed, Apr 11, 2012 at 06:03:43AM +0000, Hao, Xudong wrote:
> > +#ifdef CONFIG_X86
> > +
> > +#include "../gpu/drm/i915/i915_reg.h"
> > +#define MSG_CTL	0x45010
> 
> There's a strange mixture of constants from i915_reg.h, defined constants
> here and bare constants below.  I don't mind which get used, but the mixture
> is bizarre.
> 
> > +static const int op_timeout = 10;	/* set timeout 10 seconds */
> > +static int reset_ivb_igd(struct pci_dev *dev, int probe) {
> > +	u8 *mmio_base;
> > +	u32 val;
> > +	cycles_t cyc_op_timeout = tsc_khz*op_timeout*1000;
> 
> Style: use spaces around the multiply operator:
> 	cycles_t cyc_op_timeout = tsc_khz * op_timeout * 1000;
> 

I'll modify it to follow code style.

> > +	if (probe)
> > +		return 0;
> > +
> > +	mmio_base = ioremap_nocache(pci_resource_start(dev, 0),
> > +				 pci_resource_len(dev, 0));
> 
> mmio_base should have type void __iomem *.
> 
Yes, it should use void __iomem *, I'll change it.

> > +	if (!mmio_base)
> > +		return -ENOMEM;
> > +
> > +	/* Work Around */
> > +	*((u32 *)(mmio_base + MSG_CTL)) = 0x00000002;
> 
> Why are you not using writel() here? (and readl() in other places)
> 

I'll use writel() and readl() in next version patch, thanks for your code review and comments.

> > +	/* 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.
> > +	 */
> > +	*((u32 *)(mmio_base + SOUTH_CHICKEN2)) = 0x00000005;
> > +	val = *((u32 *)(mmio_base + PCH_PP_CONTROL)) & 0xfffffffe;
> > +	*((u32 *)(mmio_base + PCH_PP_CONTROL)) = val;
> > +	do {
> > +		cycles_t start_time = get_cycles();
> > +		while (1) {
> > +			val = *((u32 *)(mmio_base + PCH_PP_STATUS));
> > +			if (((val & 0x80000000) == 0)
> > +				&& ((val & 0x30000000) == 0))
> > +				break;
> > +			if (cyc_op_timeout < (get_cycles() - start_time))
> > +				break;
> > +			cpu_relax();
> > +		}
> > +	} while (0);
> > +	*((u32 *)(mmio_base + 0xd0100)) = 0x00000002;
> > +
> > +	iounmap(pci_resource_start(dev, 0));
> > +	return 0;
> > +}
> > +#else
> > +static int reset_ivb_igd(struct pci_dev *dev, int probe) { }
> > +#endif	/* CONFIG_X86 */
> > +
> >  #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 }
> >
> >
> > --
> > 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
> 
> --
> 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 V4] Quirk for IVB graphics FLR errata

[Don Dutile]

On 04/11/2012 02:03 AM, Hao, Xudong wrote:
> (Re-rend.)
>
> 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 v3:
> - add X86 configuration to avoid compile problem in other architecture.
>
> 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 |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 59 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 6476547..63e8b70 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3069,11 +3069,70 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)
>   	return 0;
>   }
>
> +#ifdef CONFIG_X86
> +
> +#include "../gpu/drm/i915/i915_reg.h"
> +#define MSG_CTL	0x45010
> +
> +static const int op_timeout = 10;	/* set timeout 10 seconds */
> +static int reset_ivb_igd(struct pci_dev *dev, int probe) {
> +	u8 *mmio_base;
> +	u32 val;
> +	cycles_t cyc_op_timeout = tsc_khz*op_timeout*1000;
> +
Don't we know how to do a 10sec timeout w/o tying it to tsc_khz?
--> other arch compile problem source???

> +	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 */
> +	*((u32 *)(mmio_base + MSG_CTL)) = 0x00000002;
> +	/* 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.
> +	 */
> +	*((u32 *)(mmio_base + SOUTH_CHICKEN2)) = 0x00000005;
> +	val = *((u32 *)(mmio_base + PCH_PP_CONTROL))&  0xfffffffe;
> +	*((u32 *)(mmio_base + PCH_PP_CONTROL)) = val;
above should use readl() & writel()
.... and once this & above fixed, then it'll work on other arches???

> +	do {
> +		cycles_t start_time = get_cycles();
> +		while (1) {
> +			val = *((u32 *)(mmio_base + PCH_PP_STATUS));
> +			if (((val&  0x80000000) == 0)
> +				&&  ((val&  0x30000000) == 0))
> +				break;
> +			if (cyc_op_timeout<  (get_cycles() - start_time))
> +				break;
> +			cpu_relax();
> +		}
> +	} while (0);
> +	*((u32 *)(mmio_base + 0xd0100)) = 0x00000002;
> +
> +	iounmap(pci_resource_start(dev, 0));
> +	return 0;
> +}
> +#else
> +static int reset_ivb_igd(struct pci_dev *dev, int probe) { }
> +#endif	/* CONFIG_X86 */
> +
>   #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 }
>
>
> --
> 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

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

[Hao, Xudong]

> -----Original Message-----
> From: Don Dutile [mailto:ddutile@redhat.com]
> Sent: Wednesday, April 11, 2012 10:28 PM
> To: Hao, Xudong
> Cc: Bjorn Helgaas; linux-pci@vger.kernel.org
> Subject: Re: [PATCH V4] Quirk for IVB graphics FLR errata
> 
> On 04/11/2012 02:03 AM, Hao, Xudong wrote:
> > (Re-rend.)
> >
> > 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 v3:
> > - add X86 configuration to avoid compile problem in other architecture.
> >
> > 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 |   59
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 files changed, 59 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index
> > 6476547..63e8b70 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3069,11 +3069,70 @@ static int reset_intel_82599_sfp_virtfn(struct
> pci_dev *dev, int probe)
> >   	return 0;
> >   }
> >
> > +#ifdef CONFIG_X86
> > +
> > +#include "../gpu/drm/i915/i915_reg.h"
> > +#define MSG_CTL	0x45010
> > +
> > +static const int op_timeout = 10;	/* set timeout 10 seconds */
> > +static int reset_ivb_igd(struct pci_dev *dev, int probe) {
> > +	u8 *mmio_base;
> > +	u32 val;
> > +	cycles_t cyc_op_timeout = tsc_khz*op_timeout*1000;
> > +
> Don't we know how to do a 10sec timeout w/o tying it to tsc_khz?
> --> other arch compile problem source???
> 

Of course we have other timeout, this quirk function is only used by IvyBridge mobile which is x86 arch, so I choose tsc_khz.
I have add "#ifdef CONFIG_X86" for avoid other arch compile problem.

> > +	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 */
> > +	*((u32 *)(mmio_base + MSG_CTL)) = 0x00000002;
> > +	/* 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.
> > +	 */
> > +	*((u32 *)(mmio_base + SOUTH_CHICKEN2)) = 0x00000005;
> > +	val = *((u32 *)(mmio_base + PCH_PP_CONTROL))&  0xfffffffe;
> > +	*((u32 *)(mmio_base + PCH_PP_CONTROL)) = val;
> above should use readl() & writel()

Thanks, I will use readl() and writel() in next version patch.

> .... and once this & above fixed, then it'll work on other arches???
> 

Other arches do not call this function at all, so it will not impact other arches.

> > +	do {
> > +		cycles_t start_time = get_cycles();
> > +		while (1) {
> > +			val = *((u32 *)(mmio_base + PCH_PP_STATUS));
> > +			if (((val&  0x80000000) == 0)
> > +				&&  ((val&  0x30000000) == 0))
> > +				break;
> > +			if (cyc_op_timeout<  (get_cycles() - start_time))
> > +				break;
> > +			cpu_relax();
> > +		}
> > +	} while (0);
> > +	*((u32 *)(mmio_base + 0xd0100)) = 0x00000002;
> > +
> > +	iounmap(pci_resource_start(dev, 0));
> > +	return 0;
> > +}
> > +#else
> > +static int reset_ivb_igd(struct pci_dev *dev, int probe) { }
> > +#endif	/* CONFIG_X86 */
> > +
> >   #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 }
> >
> >
> > --
> > 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

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

[Matthew Wilcox]

On Wed, Apr 11, 2012 at 10:28:13AM -0400, Don Dutile wrote:
>> +	cycles_t cyc_op_timeout = tsc_khz*op_timeout*1000;
> Don't we know how to do a 10sec timeout w/o tying it to tsc_khz?

Right, this code should of course be using jiffies and msleep.

> --> other arch compile problem source???

Well, this device is part of the x86 CPU.  It's never going to be found
as part of any other architecture.  Why force other architectures to
carry this quirk around?


-- 
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 V4] Quirk for IVB graphics FLR errata

[Don Dutile]

On 04/12/2012 12:06 AM, Matthew Wilcox wrote:
> On Wed, Apr 11, 2012 at 10:28:13AM -0400, Don Dutile wrote:
>>> +	cycles_t cyc_op_timeout = tsc_khz*op_timeout*1000;
>> Don't we know how to do a 10sec timeout w/o tying it to tsc_khz?
>
> Right, this code should of course be using jiffies and msleep.
>
>> -->  other arch compile problem source???
>
> Well, this device is part of the x86 CPU.  It's never going to be found
> as part of any other architecture.  Why force other architectures to
> carry this quirk around?
>
>

Well, the trend to include more IO into chipsets tied to an arch
will probably increase over time, so such conditional quirks will
increase as well.
Sounds like the quirk tables need an arch-hook (linked list) to check
& traverse.  Then such code can go into arch/<arch>/pci/quirks.c .

I was under the impression Linux prefers not to have ifdef <arch> in
common code modules, and to split it out under arch/<> .

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

[Hao, Xudong]

> -----Original Message-----
> From: Don Dutile [mailto:ddutile@redhat.com]
> Sent: Thursday, April 12, 2012 11:20 PM
> To: Matthew Wilcox
> Cc: Hao, Xudong; Bjorn Helgaas; linux-pci@vger.kernel.org
> Subject: Re: [PATCH V4] Quirk for IVB graphics FLR errata
> 
> On 04/12/2012 12:06 AM, Matthew Wilcox wrote:
> > On Wed, Apr 11, 2012 at 10:28:13AM -0400, Don Dutile wrote:
> >>> +	cycles_t cyc_op_timeout = tsc_khz*op_timeout*1000;
> >> Don't we know how to do a 10sec timeout w/o tying it to tsc_khz?
> >
> > Right, this code should of course be using jiffies and msleep.
> >
> >> -->  other arch compile problem source???
> >
> > Well, this device is part of the x86 CPU.  It's never going to be
> > found as part of any other architecture.  Why force other
> > architectures to carry this quirk around?
> >
> >
> 
> Well, the trend to include more IO into chipsets tied to an arch will probably
> increase over time, so such conditional quirks will increase as well.
> Sounds like the quirk tables need an arch-hook (linked list) to check & traverse.
> Then such code can go into arch/<arch>/pci/quirks.c .
> 

If using jiffies instead of tsc, the ifdef <x86> can be removed here, we already have device ID to limit the quirk workaround.
So I do want to change this patch file directory, I will change tsc to jiffies.

> I was under the impression Linux prefers not to have ifdef <arch> in common
> code modules, and to split it out under arch/<> .
> 

Agreed and learned.

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

[Bjorn Helgaas]

On Thu, Apr 12, 2012 at 9:19 AM, Don Dutile <ddutile@redhat.com> wrote:
> On 04/12/2012 12:06 AM, Matthew Wilcox wrote:

>>> -->  other arch compile problem source???
>>
>>
>> Well, this device is part of the x86 CPU.  It's never going to be found
>> as part of any other architecture.  Why force other architectures to
>> carry this quirk around?
>
> Well, the trend to include more IO into chipsets tied to an arch
> will probably increase over time, so such conditional quirks will
> increase as well.
> Sounds like the quirk tables need an arch-hook (linked list) to check
> & traverse.  Then such code can go into arch/<arch>/pci/quirks.c .

We do have arch/x86/pci/fixup.c already.  I agree it'd be nice if it
had the same name as the generic quirks.c.  Other than that, do you
think there's an advantage to adding some sort of explicit arch hook,
or is it sufficient that DECLARE_PCI_FIXUP...() uses the linker to
collect all the quirks (both generic and arch-specific)?

Bjorn

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

[Don Dutile]

On 04/12/2012 09:48 PM, Bjorn Helgaas wrote:
> On Thu, Apr 12, 2012 at 9:19 AM, Don Dutile<ddutile@redhat.com>  wrote:
>> On 04/12/2012 12:06 AM, Matthew Wilcox wrote:
>
>>>> -->    other arch compile problem source???
>>>
>>>
>>> Well, this device is part of the x86 CPU.  It's never going to be found
>>> as part of any other architecture.  Why force other architectures to
>>> carry this quirk around?
>>
>> Well, the trend to include more IO into chipsets tied to an arch
>> will probably increase over time, so such conditional quirks will
>> increase as well.
>> Sounds like the quirk tables need an arch-hook (linked list) to check
>> &  traverse.  Then such code can go into arch/<arch>/pci/quirks.c .
>
> We do have arch/x86/pci/fixup.c already.  I agree it'd be nice if it
> had the same name as the generic quirks.c.  Other than that, do you
> think there's an advantage to adding some sort of explicit arch hook,
> or is it sufficient that DECLARE_PCI_FIXUP...() uses the linker to
> collect all the quirks (both generic and arch-specific)?
>
> Bjorn
> --
> 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

I didn't understand that DECLARE_PCI_FIXUP...() uses the linker to
collect all the quirks.  if so, that WFM.
I do think the arch/<>/pci/fixup.c module should be renamed to quirks.c
so it's association with drivers/pci/quirks.c is (more) obvious.
fixup.c gives the impression it may be more like bios-related fixup,
and not quirk-related fixups.

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

[Hao, Xudong]

> -----Original Message-----
> From: Hao, Xudong
> Sent: Friday, April 13, 2012 9:41 AM
> To: 'Don Dutile'; Matthew Wilcox
> Cc: Bjorn Helgaas; linux-pci@vger.kernel.org
> Subject: RE: [PATCH V4] Quirk for IVB graphics FLR errata
> 
> > -----Original Message-----
> > From: Don Dutile [mailto:ddutile@redhat.com]
> > Sent: Thursday, April 12, 2012 11:20 PM
> > To: Matthew Wilcox
> > Cc: Hao, Xudong; Bjorn Helgaas; linux-pci@vger.kernel.org
> > Subject: Re: [PATCH V4] Quirk for IVB graphics FLR errata
> >
> > On 04/12/2012 12:06 AM, Matthew Wilcox wrote:
> > > On Wed, Apr 11, 2012 at 10:28:13AM -0400, Don Dutile wrote:
> > >>> +	cycles_t cyc_op_timeout = tsc_khz*op_timeout*1000;
> > >> Don't we know how to do a 10sec timeout w/o tying it to tsc_khz?
> > >
> > > Right, this code should of course be using jiffies and msleep.
> > >
> > >> -->  other arch compile problem source???
> > >
> > > Well, this device is part of the x86 CPU.  It's never going to be
> > > found as part of any other architecture.  Why force other
> > > architectures to carry this quirk around?
> > >
> > >
> >
> > Well, the trend to include more IO into chipsets tied to an arch will
> > probably increase over time, so such conditional quirks will increase as well.
> > Sounds like the quirk tables need an arch-hook (linked list) to check &
> traverse.
> > Then such code can go into arch/<arch>/pci/quirks.c .
> >
> 
> If using jiffies instead of tsc, the ifdef <x86> can be removed here, we already
> have device ID to limit the quirk workaround.
> So I do want to change this patch file directory, I will change tsc to jiffies.

A mistake here, I means I do NOT want to change the patch directory, according to device ID, other arch/device will not execute this quirk.

> 
> > I was under the impression Linux prefers not to have ifdef <arch> in
> > common code modules, and to split it out under arch/<> .
> >
> 
> Agreed and learned.

[PATCH V4] 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 v3:
- add X86 configuration to avoid compile problem in other architecture.

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 |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 6476547..63e8b70 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3069,11 +3069,70 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)
 	return 0;
 }
 
+#ifdef CONFIG_X86
+
+#include "../gpu/drm/i915/i915_reg.h"
+#define MSG_CTL	0x45010
+
+static const int op_timeout = 10;	/* set timeout 10 seconds */
+static int reset_ivb_igd(struct pci_dev *dev, int probe) {
+	u8 *mmio_base;
+	u32 val;
+	cycles_t cyc_op_timeout = tsc_khz*op_timeout*1000;
+
+	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 */
+	*((u32 *)(mmio_base + MSG_CTL)) = 0x00000002;
+	/* 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.
+	 */
+	*((u32 *)(mmio_base + SOUTH_CHICKEN2)) = 0x00000005;
+	val = *((u32 *)(mmio_base + PCH_PP_CONTROL)) & 0xfffffffe;
+	*((u32 *)(mmio_base + PCH_PP_CONTROL)) = val;
+	do {
+		cycles_t start_time = get_cycles();
+		while (1) {
+			val = *((u32 *)(mmio_base + PCH_PP_STATUS));
+			if (((val & 0x80000000) == 0)
+				&& ((val & 0x30000000) == 0))
+				break;
+			if (cyc_op_timeout < (get_cycles() - start_time))
+				break;
+			cpu_relax();
+		}
+	} while (0);
+	*((u32 *)(mmio_base + 0xd0100)) = 0x00000002;
+
+	iounmap(pci_resource_start(dev, 0));
+	return 0;
+}
+#else
+static int reset_ivb_igd(struct pci_dev *dev, int probe) { }
+#endif	/* CONFIG_X86 */
+
 #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