[PATCH 0/3] More Haswell VGA fixes

[PATCH 0/3] More Haswell VGA fixes

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

Here are more Haswell VGA fixes that I would like to see on Kernel 3.8. The
first 2 patches are needed to make VGA work after suspend, the 3rd patch is
needed to prevent black screen on some specific machines.

These patches are currently on top of drm-intel-fixes + the following 2 patches
which are already on the mailing list:
  - drm/i915: fix hsw_fdi_link_train "retry" code
  - drm/i915: reject modes the LPT FDI receiver can't handle

I'd love to get these 5 patches on Kernel 3.8 since they're all important fixes
that prevent black screens.

Thanks,
Paulo

Paulo Zanoni (3):
  drm/i915: add support for mPHY destination on intel_sbi_{read,write}
  drm/i915: add lpt_init_pch_refclk
  drm/i915: set the LPT FDI RX polarity reversal bit when needed

 drivers/gpu/drm/i915/i915_drv.c      |   3 +-
 drivers/gpu/drm/i915/i915_drv.h      |   9 +-
 drivers/gpu/drm/i915/i915_reg.h      |  11 +-
 drivers/gpu/drm/i915/intel_crt.c     |   8 ++
 drivers/gpu/drm/i915/intel_ddi.c     |   2 +
 drivers/gpu/drm/i915/intel_display.c | 247 +++++++++++++++++++++++++++++------
 6 files changed, 236 insertions(+), 44 deletions(-)

-- 
1.7.11.7

[PATCH 1/3] drm/i915: add support for mPHY destination on intel_sbi_{read, write}

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This way we should be able to write mPHY registers using the Sideband
Interface in the next commit. Also fixed some syntax oddities in the
related code.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  5 +++
 drivers/gpu/drm/i915/i915_reg.h      |  4 +++
 drivers/gpu/drm/i915/intel_display.c | 65 +++++++++++++++++-------------------
 3 files changed, 40 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 79589bb..8513e1c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -382,6 +382,11 @@ enum intel_pch {
 	PCH_LPT,	/* Lynxpoint PCH */
 };
 
+enum intel_sbi_destination {
+	SBI_ICLK,
+	SBI_MPHY,
+};
+
 #define QUIRK_PIPEA_FORCE (1<<0)
 #define QUIRK_LVDS_SSC_DISABLE (1<<1)
 #define QUIRK_INVERT_BRIGHTNESS (1<<2)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 97fbd9d..0760425 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4534,6 +4534,10 @@
 #define SBI_ADDR			0xC6000
 #define SBI_DATA			0xC6004
 #define SBI_CTL_STAT			0xC6008
+#define  SBI_CTL_DEST_ICLK		(0x0<<16)
+#define  SBI_CTL_DEST_MPHY		(0x1<<16)
+#define  SBI_CTL_OP_IORD		(0x2<<8)
+#define  SBI_CTL_OP_IOWR		(0x3<<8)
 #define  SBI_CTL_OP_CRRD		(0x6<<8)
 #define  SBI_CTL_OP_CRWR		(0x7<<8)
 #define  SBI_RESPONSE_FAIL		(0x1<<1)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8d86a39..1f1c7a6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1506,24 +1506,26 @@ static void intel_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 
 /* SBI access */
 static void
-intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value)
+intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
+		enum intel_sbi_destination destination)
 {
 	unsigned long flags;
+	u32 tmp;
 
 	spin_lock_irqsave(&dev_priv->dpio_lock, flags);
-	if (wait_for((I915_READ(SBI_CTL_STAT) & SBI_BUSY) == 0,
-				100)) {
+	if (wait_for((I915_READ(SBI_CTL_STAT) & SBI_BUSY) == 0, 100)) {
 		DRM_ERROR("timeout waiting for SBI to become ready\n");
 		goto out_unlock;
 	}
 
-	I915_WRITE(SBI_ADDR,
-			(reg << 16));
-	I915_WRITE(SBI_DATA,
-			value);
-	I915_WRITE(SBI_CTL_STAT,
-			SBI_BUSY |
-			SBI_CTL_OP_CRWR);
+	I915_WRITE(SBI_ADDR, (reg << 16));
+	I915_WRITE(SBI_DATA, value);
+
+	if (destination == SBI_ICLK)
+		tmp = SBI_CTL_DEST_ICLK | SBI_CTL_OP_CRWR;
+	else
+		tmp = SBI_CTL_DEST_MPHY | SBI_CTL_OP_IOWR;
+	I915_WRITE(SBI_CTL_STAT, SBI_BUSY | tmp);
 
 	if (wait_for((I915_READ(SBI_CTL_STAT) & (SBI_BUSY | SBI_RESPONSE_FAIL)) == 0,
 				100)) {
@@ -1536,23 +1538,25 @@ out_unlock:
 }
 
 static u32
-intel_sbi_read(struct drm_i915_private *dev_priv, u16 reg)
+intel_sbi_read(struct drm_i915_private *dev_priv, u16 reg,
+	       enum intel_sbi_destination destination)
 {
 	unsigned long flags;
 	u32 value = 0;
 
 	spin_lock_irqsave(&dev_priv->dpio_lock, flags);
-	if (wait_for((I915_READ(SBI_CTL_STAT) & SBI_BUSY) == 0,
-				100)) {
+	if (wait_for((I915_READ(SBI_CTL_STAT) & SBI_BUSY) == 0, 100)) {
 		DRM_ERROR("timeout waiting for SBI to become ready\n");
 		goto out_unlock;
 	}
 
-	I915_WRITE(SBI_ADDR,
-			(reg << 16));
-	I915_WRITE(SBI_CTL_STAT,
-			SBI_BUSY |
-			SBI_CTL_OP_CRRD);
+	I915_WRITE(SBI_ADDR, (reg << 16));
+
+	if (destination == SBI_ICLK)
+		value = SBI_CTL_DEST_ICLK | SBI_CTL_OP_CRRD;
+	else
+		value = SBI_CTL_DEST_MPHY | SBI_CTL_OP_IORD;
+	I915_WRITE(SBI_CTL_STAT, value | SBI_BUSY);
 
 	if (wait_for((I915_READ(SBI_CTL_STAT) & (SBI_BUSY | SBI_RESPONSE_FAIL)) == 0,
 				100)) {
@@ -3024,8 +3028,9 @@ static void lpt_program_iclkip(struct drm_crtc *crtc)
 
 	/* Disable SSCCTL */
 	intel_sbi_write(dev_priv, SBI_SSCCTL6,
-				intel_sbi_read(dev_priv, SBI_SSCCTL6) |
-					SBI_SSCCTL_DISABLE);
+			intel_sbi_read(dev_priv, SBI_SSCCTL6, SBI_ICLK) |
+				SBI_SSCCTL_DISABLE,
+			SBI_ICLK);
 
 	/* 20MHz is a corner case which is out of range for the 7-bit divisor */
 	if (crtc->mode.clock == 20000) {
@@ -3066,33 +3071,25 @@ static void lpt_program_iclkip(struct drm_crtc *crtc)
 			phaseinc);
 
 	/* Program SSCDIVINTPHASE6 */
-	temp = intel_sbi_read(dev_priv, SBI_SSCDIVINTPHASE6);
+	temp = intel_sbi_read(dev_priv, SBI_SSCDIVINTPHASE6, SBI_ICLK);
 	temp &= ~SBI_SSCDIVINTPHASE_DIVSEL_MASK;
 	temp |= SBI_SSCDIVINTPHASE_DIVSEL(divsel);
 	temp &= ~SBI_SSCDIVINTPHASE_INCVAL_MASK;
 	temp |= SBI_SSCDIVINTPHASE_INCVAL(phaseinc);
 	temp |= SBI_SSCDIVINTPHASE_DIR(phasedir);
 	temp |= SBI_SSCDIVINTPHASE_PROPAGATE;
-
-	intel_sbi_write(dev_priv,
-			SBI_SSCDIVINTPHASE6,
-			temp);
+	intel_sbi_write(dev_priv, SBI_SSCDIVINTPHASE6, temp, SBI_ICLK);
 
 	/* Program SSCAUXDIV */
-	temp = intel_sbi_read(dev_priv, SBI_SSCAUXDIV6);
+	temp = intel_sbi_read(dev_priv, SBI_SSCAUXDIV6, SBI_ICLK);
 	temp &= ~SBI_SSCAUXDIV_FINALDIV2SEL(1);
 	temp |= SBI_SSCAUXDIV_FINALDIV2SEL(auxdiv);
-	intel_sbi_write(dev_priv,
-			SBI_SSCAUXDIV6,
-			temp);
-
+	intel_sbi_write(dev_priv, SBI_SSCAUXDIV6, temp, SBI_ICLK);
 
 	/* Enable modulator and associated divider */
-	temp = intel_sbi_read(dev_priv, SBI_SSCCTL6);
+	temp = intel_sbi_read(dev_priv, SBI_SSCCTL6, SBI_ICLK);
 	temp &= ~SBI_SSCCTL_DISABLE;
-	intel_sbi_write(dev_priv,
-			SBI_SSCCTL6,
-			temp);
+	intel_sbi_write(dev_priv, SBI_SSCCTL6, temp, SBI_ICLK);
 
 	/* Wait for initialization time */
 	udelay(24);
-- 
1.7.11.7

Re: [PATCH 1/3] drm/i915: add support for mPHY destination on intel_sbi_{read, write}

[Damien Lespiau]

On Sat, Dec 1, 2012 at 2:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> This way we should be able to write mPHY registers using the Sideband
> Interface in the next commit. Also fixed some syntax oddities in the
> related code.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

(I'd have put the destination parameter first, talk about bike-shedding!)

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

[PATCH 2/3] drm/i915: add lpt_init_pch_refclk

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We need this code to init the PCH SSC refclk and the FDI registers.
The BIOS does this too and that's why VGA worked before this patch,
until you tried to suspend the machine...

This patch implements the "Sequence to enable CLKOUT_DP for FDI usage
and configure PCH FDI/IO" from our documentation.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |   3 +-
 drivers/gpu/drm/i915/i915_drv.h      |   2 +-
 drivers/gpu/drm/i915/i915_reg.h      |   6 +-
 drivers/gpu/drm/i915/intel_display.c | 182 +++++++++++++++++++++++++++++++++--
 4 files changed, 183 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a129218..530db83 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -554,8 +554,7 @@ static int __i915_drm_thaw(struct drm_device *dev)
 
 	/* KMS EnterVT equivalent */
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
-		if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
-			ironlake_init_pch_refclk(dev);
+		intel_init_pch_refclk(dev);
 
 		mutex_lock(&dev->struct_mutex);
 		dev_priv->mm.suspended = 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8513e1c..65213bc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1662,7 +1662,7 @@ extern void intel_modeset_setup_hw_state(struct drm_device *dev,
 extern bool intel_fbc_enabled(struct drm_device *dev);
 extern void intel_disable_fbc(struct drm_device *dev);
 extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
-extern void ironlake_init_pch_refclk(struct drm_device *dev);
+extern void intel_init_pch_refclk(struct drm_device *dev);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void intel_detect_pch(struct drm_device *dev);
 extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0760425..acf768d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3843,7 +3843,9 @@
 #define  FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2)))
 #define  FDI_BC_BIFURCATION_SELECT	(1 << 12)
 #define SOUTH_CHICKEN2		0xc2004
-#define  DPLS_EDP_PPS_FIX_DIS	(1<<0)
+#define  FDI_MPHY_IOSFSB_RESET_STATUS	(1<<13)
+#define  FDI_MPHY_IOSFSB_RESET_CTL	(1<<12)
+#define  DPLS_EDP_PPS_FIX_DIS		(1<<0)
 
 #define _FDI_RXA_CHICKEN         0xc200c
 #define _FDI_RXB_CHICKEN         0xc2010
@@ -4555,10 +4557,12 @@
 #define   SBI_SSCDIVINTPHASE_PROPAGATE		(1<<0)
 #define  SBI_SSCCTL				0x020c
 #define  SBI_SSCCTL6				0x060C
+#define   SBI_SSCCTL_PATHALT			(1<<3)
 #define   SBI_SSCCTL_DISABLE			(1<<0)
 #define  SBI_SSCAUXDIV6				0x0610
 #define   SBI_SSCAUXDIV_FINALDIV2SEL(x)		((x)<<4)
 #define  SBI_DBUFF0				0x2a00
+#define   SBI_DBUFF0_ENABLE			(1<<0)
 
 /* LPT PIXCLK_GATE */
 #define PIXCLK_GATE			0xC6020
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1f1c7a6..e87f4cc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4864,10 +4864,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	return ret;
 }
 
-/*
- * Initialize reference clocks when the driver loads
- */
-void ironlake_init_pch_refclk(struct drm_device *dev)
+static void ironlake_init_pch_refclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_mode_config *mode_config = &dev->mode_config;
@@ -4981,6 +4978,180 @@ void ironlake_init_pch_refclk(struct drm_device *dev)
 	}
 }
 
+/* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */
+static void lpt_init_pch_refclk(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct intel_encoder *encoder;
+	bool has_vga = false;
+	bool is_sdv = false;
+	u32 tmp;
+
+	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
+		switch (encoder->type) {
+		case INTEL_OUTPUT_ANALOG:
+			has_vga = true;
+			break;
+		}
+	}
+
+	if (!has_vga)
+		return;
+
+	if (IS_HASWELL(dev) && (dev->pci_device & 0xFF00) == 0x0C00)
+		is_sdv = true;
+
+	tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+	tmp &= ~SBI_SSCCTL_DISABLE;
+	tmp |= SBI_SSCCTL_PATHALT;
+	intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
+
+	udelay(24);
+
+	tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+	tmp &= ~SBI_SSCCTL_PATHALT;
+	intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
+
+	if (!is_sdv) {
+		tmp = I915_READ(SOUTH_CHICKEN2);
+		tmp |= FDI_MPHY_IOSFSB_RESET_CTL;
+		I915_WRITE(SOUTH_CHICKEN2, tmp);
+
+		if (wait_for_atomic_us(I915_READ(SOUTH_CHICKEN2) &
+				       FDI_MPHY_IOSFSB_RESET_STATUS, 100))
+			DRM_ERROR("FDI mPHY resert assert timeout\n");
+
+		tmp = I915_READ(SOUTH_CHICKEN2);
+		tmp &= ~FDI_MPHY_IOSFSB_RESET_CTL;
+		I915_WRITE(SOUTH_CHICKEN2, tmp);
+
+		if (wait_for_atomic_us((I915_READ(SOUTH_CHICKEN2) &
+				        FDI_MPHY_IOSFSB_RESET_STATUS) == 0,
+				       100))
+			DRM_ERROR("FDI mPHY reset de-assert timeout\n");
+	}
+
+	tmp = intel_sbi_read(dev_priv, 0x8008, SBI_MPHY);
+	tmp &= ~(0xFF << 24);
+	tmp |= (0x12 << 24);
+	intel_sbi_write(dev_priv, 0x8008, tmp, SBI_MPHY);
+
+	if (!is_sdv) {
+		tmp = intel_sbi_read(dev_priv, 0x808C, SBI_MPHY);
+		tmp &= ~(0x3 << 6);
+		tmp |= (1 << 6) | (1 << 0);
+		intel_sbi_write(dev_priv, 0x808C, tmp, SBI_MPHY);
+	}
+
+	if (is_sdv) {
+		tmp = intel_sbi_read(dev_priv, 0x800C, SBI_MPHY);
+		tmp |= 0x7FFF;
+		intel_sbi_write(dev_priv, 0x800C, tmp, SBI_MPHY);
+	}
+
+	tmp = intel_sbi_read(dev_priv, 0x2008, SBI_MPHY);
+	tmp |= (1 << 11);
+	intel_sbi_write(dev_priv, 0x2008, tmp, SBI_MPHY);
+
+	tmp = intel_sbi_read(dev_priv, 0x2108, SBI_MPHY);
+	tmp |= (1 << 11);
+	intel_sbi_write(dev_priv, 0x2108, tmp, SBI_MPHY);
+
+	if (is_sdv) {
+		tmp = intel_sbi_read(dev_priv, 0x2038, SBI_MPHY);
+		tmp |= (0x3F << 24) | (0xF << 20) | (0xF << 16);
+		intel_sbi_write(dev_priv, 0x2038, tmp, SBI_MPHY);
+
+		tmp = intel_sbi_read(dev_priv, 0x2138, SBI_MPHY);
+		tmp |= (0x3F << 24) | (0xF << 20) | (0xF << 16);
+		intel_sbi_write(dev_priv, 0x2138, tmp, SBI_MPHY);
+
+		tmp = intel_sbi_read(dev_priv, 0x203C, SBI_MPHY);
+		tmp |= (0x3F << 8);
+		intel_sbi_write(dev_priv, 0x203C, tmp, SBI_MPHY);
+
+		tmp = intel_sbi_read(dev_priv, 0x213C, SBI_MPHY);
+		tmp |= (0x3F << 8);
+		intel_sbi_write(dev_priv, 0x213C, tmp, SBI_MPHY);
+	}
+
+	tmp = intel_sbi_read(dev_priv, 0x206C, SBI_MPHY);
+	tmp |= (1 << 24) | (1 << 21) | (1 << 18);
+	intel_sbi_write(dev_priv, 0x206C, tmp, SBI_MPHY);
+
+	tmp = intel_sbi_read(dev_priv, 0x216C, SBI_MPHY);
+	tmp |= (1 << 24) | (1 << 21) | (1 << 18);
+	intel_sbi_write(dev_priv, 0x216C, tmp, SBI_MPHY);
+
+	if (!is_sdv) {
+		tmp = intel_sbi_read(dev_priv, 0x2080, SBI_MPHY);
+		tmp &= ~(7 << 13);
+		tmp |= (5 << 13);
+		intel_sbi_write(dev_priv, 0x2080, tmp, SBI_MPHY);
+
+		tmp = intel_sbi_read(dev_priv, 0x2180, SBI_MPHY);
+		tmp &= ~(7 << 13);
+		tmp |= (5 << 13);
+		intel_sbi_write(dev_priv, 0x2180, tmp, SBI_MPHY);
+	}
+
+	tmp = intel_sbi_read(dev_priv, 0x208C, SBI_MPHY);
+	tmp &= ~0xFF;
+	tmp |= 0x1C;
+	intel_sbi_write(dev_priv, 0x208C, tmp, SBI_MPHY);
+
+	tmp = intel_sbi_read(dev_priv, 0x218C, SBI_MPHY);
+	tmp &= ~0xFF;
+	tmp |= 0x1C;
+	intel_sbi_write(dev_priv, 0x218C, tmp, SBI_MPHY);
+
+	tmp = intel_sbi_read(dev_priv, 0x2098, SBI_MPHY);
+	tmp &= ~(0xFF << 16);
+	tmp |= (0x1C << 16);
+	intel_sbi_write(dev_priv, 0x2098, tmp, SBI_MPHY);
+
+	tmp = intel_sbi_read(dev_priv, 0x2198, SBI_MPHY);
+	tmp &= ~(0xFF << 16);
+	tmp |= (0x1C << 16);
+	intel_sbi_write(dev_priv, 0x2198, tmp, SBI_MPHY);
+
+	if (!is_sdv) {
+		tmp = intel_sbi_read(dev_priv, 0x20C4, SBI_MPHY);
+		tmp |= (1 << 27);
+		intel_sbi_write(dev_priv, 0x20C4, tmp, SBI_MPHY);
+
+		tmp = intel_sbi_read(dev_priv, 0x21C4, SBI_MPHY);
+		tmp |= (1 << 27);
+		intel_sbi_write(dev_priv, 0x21C4, tmp, SBI_MPHY);
+
+		tmp = intel_sbi_read(dev_priv, 0x20EC, SBI_MPHY);
+		tmp &= ~(0xF << 28);
+		tmp |= (4 << 28);
+		intel_sbi_write(dev_priv, 0x20EC, tmp, SBI_MPHY);
+
+		tmp = intel_sbi_read(dev_priv, 0x21EC, SBI_MPHY);
+		tmp &= ~(0xF << 28);
+		tmp |= (4 << 28);
+		intel_sbi_write(dev_priv, 0x21EC, tmp, SBI_MPHY);
+	}
+
+	tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
+	tmp |= SBI_DBUFF0_ENABLE;
+	intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
+}
+
+/*
+ * Initialize reference clocks when the driver loads
+ */
+void intel_init_pch_refclk(struct drm_device *dev)
+{
+	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
+		ironlake_init_pch_refclk(dev);
+	else if (HAS_PCH_LPT(dev))
+		lpt_init_pch_refclk(dev);
+}
+
 static int ironlake_get_refclk(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -8385,8 +8556,7 @@ static void intel_setup_outputs(struct drm_device *dev)
 			intel_encoder_clones(encoder);
 	}
 
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
-		ironlake_init_pch_refclk(dev);
+	intel_init_pch_refclk(dev);
 
 	drm_helper_move_panel_connectors_to_head(dev);
 }
-- 
1.7.11.7

Re: [PATCH 2/3] drm/i915: add lpt_init_pch_refclk

[Damien Lespiau]

On Sat, Dec 1, 2012 at 2:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> We need this code to init the PCH SSC refclk and the FDI registers.
> The BIOS does this too and that's why VGA worked before this patch,
> until you tried to suspend the machine...
>
> This patch implements the "Sequence to enable CLKOUT_DP for FDI usage
> and configure PCH FDI/IO" from our documentation.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +       if (IS_HASWELL(dev) && (dev->pci_device & 0xFF00) == 0x0C00)
> +               is_sdv = true;

Maybe have a IS_SDV() or IS_HSW_SDV() macro like we have a IS_ULT()
one? The check should be on the PCH revision but hopefully there's a
1:1 correlation?

> +               if (wait_for_atomic_us(I915_READ(SOUTH_CHICKEN2) &
> +                                      FDI_MPHY_IOSFSB_RESET_STATUS, 100))
> +                       DRM_ERROR("FDI mPHY resert assert timeout\n");

s/resert/reset/

> +
> +               tmp = I915_READ(SOUTH_CHICKEN2);
> +               tmp &= ~FDI_MPHY_IOSFSB_RESET_CTL;
> +               I915_WRITE(SOUTH_CHICKEN2, tmp);
> +
> +               if (wait_for_atomic_us((I915_READ(SOUTH_CHICKEN2) &
> +                                       FDI_MPHY_IOSFSB_RESET_STATUS) == 0,
> +                                      100))
> +                       DRM_ERROR("FDI mPHY reset de-assert timeout\n");
> +       }

When either of those two waits error out, we carry on the sequence. We
should probably fail and disable the VGA output (one of those error
paths that never get to be tested, yey! I don't know how well we
support connectors disappearing)?

> +       tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
> +       tmp |= SBI_DBUFF0_ENABLE;
> +       intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);

Th ULT path is missing there.

I double checked the programming, looks good. I'll follow up with an
amended patch for the 2 first bikesheds, leave the error path alone,
and add the ULT path as a separate patch.

-- 
Damien

Re: [PATCH 2/3] drm/i915: add lpt_init_pch_refclk

[Paulo Zanoni]

Hi

2012/12/8 Damien Lespiau <damien.lespiau@intel.com>:
> On Sat, Dec 1, 2012 at 2:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> We need this code to init the PCH SSC refclk and the FDI registers.
>> The BIOS does this too and that's why VGA worked before this patch,
>> until you tried to suspend the machine...
>>
>> This patch implements the "Sequence to enable CLKOUT_DP for FDI usage
>> and configure PCH FDI/IO" from our documentation.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
>> +       if (IS_HASWELL(dev) && (dev->pci_device & 0xFF00) == 0x0C00)
>> +               is_sdv = true;
>
> Maybe have a IS_SDV() or IS_HSW_SDV() macro like we have a IS_ULT()
> one? The check should be on the PCH revision but hopefully there's a
> 1:1 correlation?

We should just remove this code in the future, maybe even now... I did
not care too much about creating a macro since this is going to be
killed.

>
>> +               if (wait_for_atomic_us(I915_READ(SOUTH_CHICKEN2) &
>> +                                      FDI_MPHY_IOSFSB_RESET_STATUS, 100))
>> +                       DRM_ERROR("FDI mPHY resert assert timeout\n");
>
> s/resert/reset/
>
>> +
>> +               tmp = I915_READ(SOUTH_CHICKEN2);
>> +               tmp &= ~FDI_MPHY_IOSFSB_RESET_CTL;
>> +               I915_WRITE(SOUTH_CHICKEN2, tmp);
>> +
>> +               if (wait_for_atomic_us((I915_READ(SOUTH_CHICKEN2) &
>> +                                       FDI_MPHY_IOSFSB_RESET_STATUS) == 0,
>> +                                      100))
>> +                       DRM_ERROR("FDI mPHY reset de-assert timeout\n");
>> +       }
>
> When either of those two waits error out, we carry on the sequence. We
> should probably fail and disable the VGA output (one of those error
> paths that never get to be tested, yey! I don't know how well we
> support connectors disappearing)?

We have this "pattern" in many places: the doc tells us "wait for this
bit or wait at least XXXms", then we wait using wait_for and print
DRM_ERROR in case we timeout. In theory, even if we hit the timeout,
we should be fine since we've already waited for the amount of time
the spec tells us to wait... I'm not really sure what's the best thing
to do here, but it really seems we're probably never hit the "fail"
path.

>
>> +       tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
>> +       tmp |= SBI_DBUFF0_ENABLE;
>> +       intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
>
> Th ULT path is missing there.

The ULT will never reach this point, there's not VGA.

>
> I double checked the programming, looks good. I'll follow up with an
> amended patch for the 2 first bikesheds, leave the error path alone,
> and add the ULT path as a separate patch.
>
> --
> Damien



-- 
Paulo Zanoni

Re: [PATCH 2/3] drm/i915: add lpt_init_pch_refclk

[Daniel Vetter]

On Sun, Dec 09, 2012 at 07:35:13PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/12/8 Damien Lespiau <damien.lespiau@intel.com>:
> > On Sat, Dec 1, 2012 at 2:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> We need this code to init the PCH SSC refclk and the FDI registers.
> >> The BIOS does this too and that's why VGA worked before this patch,
> >> until you tried to suspend the machine...
> >>
> >> This patch implements the "Sequence to enable CLKOUT_DP for FDI usage
> >> and configure PCH FDI/IO" from our documentation.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> >> +       if (IS_HASWELL(dev) && (dev->pci_device & 0xFF00) == 0x0C00)
> >> +               is_sdv = true;
> >
> > Maybe have a IS_SDV() or IS_HSW_SDV() macro like we have a IS_ULT()
> > one? The check should be on the PCH revision but hopefully there's a
> > 1:1 correlation?
> 
> We should just remove this code in the future, maybe even now... I did
> not care too much about creating a macro since this is going to be
> killed.

Added a comment to remind us of that.

> >
> >> +               if (wait_for_atomic_us(I915_READ(SOUTH_CHICKEN2) &
> >> +                                      FDI_MPHY_IOSFSB_RESET_STATUS, 100))
> >> +                       DRM_ERROR("FDI mPHY resert assert timeout\n");
> >
> > s/resert/reset/
> >
> >> +
> >> +               tmp = I915_READ(SOUTH_CHICKEN2);
> >> +               tmp &= ~FDI_MPHY_IOSFSB_RESET_CTL;
> >> +               I915_WRITE(SOUTH_CHICKEN2, tmp);
> >> +
> >> +               if (wait_for_atomic_us((I915_READ(SOUTH_CHICKEN2) &
> >> +                                       FDI_MPHY_IOSFSB_RESET_STATUS) == 0,
> >> +                                      100))
> >> +                       DRM_ERROR("FDI mPHY reset de-assert timeout\n");
> >> +       }
> >
> > When either of those two waits error out, we carry on the sequence. We
> > should probably fail and disable the VGA output (one of those error
> > paths that never get to be tested, yey! I don't know how well we
> > support connectors disappearing)?
> 
> We have this "pattern" in many places: the doc tells us "wait for this
> bit or wait at least XXXms", then we wait using wait_for and print
> DRM_ERROR in case we timeout. In theory, even if we hit the timeout,
> we should be fine since we've already waited for the amount of time
> the spec tells us to wait... I'm not really sure what's the best thing
> to do here, but it really seems we're probably never hit the "fail"
> path.

Imo this is ok. If the hw is in a non-cooperative mood, there's not much
we can do than detect it and hope for the best.

> >> +       tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
> >> +       tmp |= SBI_DBUFF0_ENABLE;
> >> +       intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
> >
> > Th ULT path is missing there.
> 
> The ULT will never reach this point, there's not VGA.

Added a comment.

Another bikeshed: Can we have less magic here, or are the docs as opaque
as the code here? I've merged it for now, but usually Dave' doesn't like
this much magic in the code, so a fixup patch would be nice. Imo just
giving the register some names should be good enough.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/3] drm/i915: add lpt_init_pch_refclk

[Damien Lespiau]

On Mon, Dec 10, 2012 at 9:36 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> >> +       tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
>> >> +       tmp |= SBI_DBUFF0_ENABLE;
>> >> +       intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
>> >
>> > Th ULT path is missing there.
>>
>> The ULT will never reach this point, there's not VGA.
>
> Added a comment.

To go to the bottom of this, I wanted to understand why you can enable
the PCH SSC.

It is recommended to use the internal SSC source on ULT platforms. The
PCH SSC source is then only used for clock bending. Fortunately the
PLL Select bits in SPLL_CTL paper over this difference between the
internal and PCH SSC sources, selecting the right one based on a fused
bit.

And Paulo does check if we have VGA at the start, so indeed, all is
good (I believe)!

-- 
Damien

Re: [PATCH 2/3] drm/i915: add lpt_init_pch_refclk

[Damien Lespiau]

On Sun, Dec 9, 2012 at 9:35 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Hi
>
> 2012/12/8 Damien Lespiau <damien.lespiau@intel.com>:
>> Maybe have a IS_SDV() or IS_HSW_SDV() macro like we have a IS_ULT()
>> one? The check should be on the PCH revision but hopefully there's a
>> 1:1 correlation?
>
> We should just remove this code in the future, maybe even now... I did
> not care too much about creating a macro since this is going to be
> killed.

On the other hand, it's easy to grep for IS_SDV() rather than other
more complex tests. We could even expand the macro to other platforms
and have a single way to mark SDV code paths.

-- 
Damien

[PATCH 3/3] drm/i915: set the LPT FDI RX polarity reversal bit when needed

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

If we fail to set the bit when needed we get some nice FDI link
training failures (AKA "black screen on VGA output").

While we don't really know how to properly choose whether we need to
set the bit or not (VBT?), just read the initial value set by the BIOS
and store it for later usage.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 2 ++
 drivers/gpu/drm/i915/i915_reg.h  | 1 +
 drivers/gpu/drm/i915/intel_crt.c | 8 ++++++++
 drivers/gpu/drm/i915/intel_ddi.c | 2 ++
 4 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 65213bc..557843d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -915,6 +915,8 @@ typedef struct drm_i915_private {
 	bool hw_contexts_disabled;
 	uint32_t hw_context_size;
 
+	bool fdi_rx_polarity_reversed;
+
 	struct i915_suspend_saved_registers regfile;
 
 	/* Old dri1 support infrastructure, beware the dragons ya fools entering
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index acf768d..3f75cfa 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3917,6 +3917,7 @@
 #define  FDI_FS_ERRC_ENABLE		(1<<27)
 #define  FDI_FE_ERRC_ENABLE		(1<<26)
 #define  FDI_DP_PORT_WIDTH_X8           (7<<19)
+#define  FDI_RX_POLARITY_REVERSED_LPT	(1<<16)
 #define  FDI_8BPC                       (0<<16)
 #define  FDI_10BPC                      (1<<16)
 #define  FDI_6BPC                       (2<<16)
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 3084d01..fe20bf7 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -798,4 +798,12 @@ void intel_crt_init(struct drm_device *dev)
 	crt->force_hotplug_required = 0;
 
 	dev_priv->hotplug_supported_mask |= CRT_HOTPLUG_INT_STATUS;
+
+	/*
+	 * TODO: find a proper way to discover whether we need to set the
+	 * polarity reversal bit or not, instead of relying on the BIOS.
+	 */
+	if (HAS_PCH_LPT(dev))
+		dev_priv->fdi_rx_polarity_reversed =
+		     !!(I915_READ(_FDI_RXA_CTL) & FDI_RX_POLARITY_REVERSED_LPT);
 }
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 3264cb4..4bad0f7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -180,6 +180,8 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
 	/* Enable the PCH Receiver FDI PLL */
 	rx_ctl_val = FDI_RX_PLL_ENABLE | FDI_RX_ENHANCE_FRAME_ENABLE |
 		     ((intel_crtc->fdi_lanes - 1) << 19);
+	if (dev_priv->fdi_rx_polarity_reversed)
+		rx_ctl_val |= FDI_RX_POLARITY_REVERSED_LPT;
 	I915_WRITE(_FDI_RXA_CTL, rx_ctl_val);
 	POSTING_READ(_FDI_RXA_CTL);
 	udelay(220);
-- 
1.7.11.7

Re: [PATCH 3/3] drm/i915: set the LPT FDI RX polarity reversal bit when needed

[Damien Lespiau]

On Sat, Dec 1, 2012 at 2:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> If we fail to set the bit when needed we get some nice FDI link
> training failures (AKA "black screen on VGA output").
>
> While we don't really know how to properly choose whether we need to
> set the bit or not (VBT?), just read the initial value set by the BIOS
> and store it for later usage.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Sad that we did not catch that before. It's not just the polarity
(support for exchanging the - and + wires on a differential pair), but
also the FDI lane reversal (0 becomes 3, 1 becomes 2) setting that we
need to preserve. There's a fused bit for the DMI lane reversal and
FDI defaults to following that DMI lane reversal configuration, but we
also have a bit to override that behaviour in the FDI RX register, we
need to preserve this bit  as well.

I think this is the same story for DDI_BUF_CTL, DDI ports support lane reversal.

I could not find either where these settings are stored, preserving
what the BIOS does for those seems the best option so far.

-- 
Damien

Re: [PATCH 3/3] drm/i915: set the LPT FDI RX polarity reversal bit when needed

[Paulo Zanoni]

Hi

2012/12/8 Damien Lespiau <damien.lespiau@intel.com>:
> On Sat, Dec 1, 2012 at 2:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> If we fail to set the bit when needed we get some nice FDI link
>> training failures (AKA "black screen on VGA output").
>>
>> While we don't really know how to properly choose whether we need to
>> set the bit or not (VBT?), just read the initial value set by the BIOS
>> and store it for later usage.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Sad that we did not catch that before. It's not just the polarity
> (support for exchanging the - and + wires on a differential pair), but
> also the FDI lane reversal (0 becomes 3, 1 becomes 2) setting that we
> need to preserve. There's a fused bit for the DMI lane reversal and
> FDI defaults to following that DMI lane reversal configuration, but we
> also have a bit to override that behaviour in the FDI RX register, we
> need to preserve this bit  as well.

They're different problems and require different patches. This one
fixes the "FDI RX polarity" problem, not "lane reversal".

>
> I think this is the same story for DDI_BUF_CTL, DDI ports support lane reversal.
>
> I could not find either where these settings are stored, preserving
> what the BIOS does for those seems the best option so far.
>
> --
> Damien



-- 
Paulo Zanoni

Re: [PATCH 3/3] drm/i915: set the LPT FDI RX polarity reversal bit when needed

[Daniel Vetter]

On Sun, Dec 09, 2012 at 07:37:40PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/12/8 Damien Lespiau <damien.lespiau@intel.com>:
> > On Sat, Dec 1, 2012 at 2:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> If we fail to set the bit when needed we get some nice FDI link
> >> training failures (AKA "black screen on VGA output").
> >>
> >> While we don't really know how to properly choose whether we need to
> >> set the bit or not (VBT?), just read the initial value set by the BIOS
> >> and store it for later usage.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Sad that we did not catch that before. It's not just the polarity
> > (support for exchanging the - and + wires on a differential pair), but
> > also the FDI lane reversal (0 becomes 3, 1 becomes 2) setting that we
> > need to preserve. There's a fused bit for the DMI lane reversal and
> > FDI defaults to following that DMI lane reversal configuration, but we
> > also have a bit to override that behaviour in the FDI RX register, we
> > need to preserve this bit  as well.
> 
> They're different problems and require different patches. This one
> fixes the "FDI RX polarity" problem, not "lane reversal".

Picked up for -fixes (together with patch 1), thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch