HSW/LPT clocking code additional sequences

From: Paulo Zanoni <[email protected]>

The machines that fall in the "is_sdv" case are some very early pre-production steppings. This patch may break VGA output after suspend/resume on these machines.

Even the documentation for the is_sdv cases was removed from BSpec.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 104 ++++++++++++-----------------------
 1 file changed, 34 insertions(+), 70 deletions(-)

drivers/gpu/drm/i915/intel_display.c +34 -70

index c79addd..5821ffc 100644
@@ -5170,7 +5170,6 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 	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) {

@@ -5186,10 +5185,6 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 
 	mutex_lock(&dev_priv->dpio_lock);
 
-	/* XXX: Rip out SDV support once Haswell ships for real. */
-	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;

@@ -5201,36 +5196,27 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 	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);
+	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 reset assert timeout\n");
+	if (wait_for_atomic_us(I915_READ(SOUTH_CHICKEN2) &
+			       FDI_MPHY_IOSFSB_RESET_STATUS, 100))
+		DRM_ERROR("FDI mPHY reset assert timeout\n");
 
-		tmp = I915_READ(SOUTH_CHICKEN2);
-		tmp &= ~FDI_MPHY_IOSFSB_RESET_CTL;
-		I915_WRITE(SOUTH_CHICKEN2, tmp);
+	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");
-	}
+	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, 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);

@@ -5239,24 +5225,6 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 	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);

@@ -5265,17 +5233,15 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 	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, 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, 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;

@@ -5297,25 +5263,23 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 	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, 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, 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, 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, 0x21EC, SBI_MPHY);
+	tmp &= ~(0xF << 28);
+	tmp |= (4 << 28);
+	intel_sbi_write(dev_priv, 0x21EC, tmp, SBI_MPHY);
 
 	/* ULT uses SBI_GEN0, but ULT doesn't have VGA, so we don't care. */
 	tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);

Signature · 6 lines

1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Paulo Zanoni <[email protected]>

The next step is to modify lpt_enable_clkout_dp to enable support for "Sequence to enable CLKOUT_DP" and "Sequence to enable CLKOUT_DP without spread".

Signed-off-by Paulo Zanoni <[email protected]>

From: Paulo Zanoni <[email protected]>

Now it implements 3 different sequences from BSpec and also has support for ULT.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h      |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++-----------
 2 files changed, 31 insertions(+), 12 deletions(-)

drivers/gpu/drm/i915/i915_reg.h +0 -0

index dc3d6a7..be6164f 100644

drivers/gpu/drm/i915/i915_reg.h +0 -0

 /* LPT PIXCLK_GATE */
 #define PIXCLK_GATE			0xC6020

drivers/gpu/drm/i915/intel_display.c +4 -2

index f4c5263..5f3b636 100644
@@ -5258,12 +5258,20 @@ static void lpt_program_fdi_mphy(struct drm_i915_private *dev_priv)
 	intel_sbi_write(dev_priv, 0x21EC, tmp, SBI_MPHY);
 }
 
-/* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */
-static void lpt_enable_clkout_dp(struct drm_device *dev)
+/* Implements 3 different sequences from BSpec chapter "Display iCLK
+ * Programming" based on the parameters passed:
+ * - Sequence to enable CLKOUT_DP
+ * - Sequence to enable CLKOUT_DP without spread

drivers/gpu/drm/i915/intel_display.c +19 -10

 	mutex_lock(&dev_priv->dpio_lock);
 
 	tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);

@@ -5273,17 +5281,26 @@ static void lpt_enable_clkout_dp(struct drm_device *dev)
 
 	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 (with_spread) {
+		tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+		tmp &= ~SBI_SSCCTL_PATHALT;
+		intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
 
-	lpt_reset_fdi_mphy(dev_priv);
-	lpt_program_fdi_mphy(dev_priv);
+		if (with_fdi) {
+			lpt_reset_fdi_mphy(dev_priv);
+			lpt_program_fdi_mphy(dev_priv);
+		}
+	}
 
-	/* ULT uses SBI_GEN0, but ULT doesn't have VGA, so we don't care. */
-	tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
-	tmp |= SBI_DBUFF0_ENABLE;
-	intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
+	if (IS_ULT(dev)) {
+		tmp = intel_sbi_read(dev_priv, SBI_GEN0, SBI_ICLK);
+		tmp |= SBI_GEN0_ENABLE;
+		intel_sbi_write(dev_priv, SBI_GEN0, tmp, SBI_ICLK);
+	} else {
+		tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
+		tmp |= SBI_DBUFF0_ENABLE;
+		intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
+	}
 
 	mutex_unlock(&dev_priv->dpio_lock);
 }

@@ -5305,7 +5322,7 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 	if (!has_vga)
 		return;
 
-	lpt_enable_clkout_dp(dev);
+	lpt_enable_clkout_dp(dev, true, true);
 }
 
 /*

Signature · 1 line

1.8.1.2

Quoted 11 lines

From: Paulo Zanoni <[email protected]>

Now it implements 3 different sequences from BSpec and also has support for ULT.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h      |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++-----------
 2 files changed, 31 insertions(+), 12 deletions(-)

drivers/gpu/drm/i915/i915_reg.h line 4867

Quoted 10 lines

drivers/gpu/drm/i915/i915_reg.h +2 -0

index dc3d6a7..be6164f 100644
@@ -4863,6 +4863,8 @@
 #define   SBI_SSCAUXDIV_FINALDIV2SEL(x)              ((x)<<4)
 #define  SBI_DBUFF0                          0x2a00
 #define   SBI_DBUFF0_ENABLE                  (1<<0)
+#define  SBI_GEN0                            0x1f00
+#define   SBI_GEN0_ENABLE                    (1<<0)

bikeshed: I wouldn't haved bothered defining SBI_GEN0_ENABLE

Quoted 26 lines

drivers/gpu/drm/i915/i915_reg.h +0 -0

 /* LPT PIXCLK_GATE */
 #define PIXCLK_GATE                  0xC6020

drivers/gpu/drm/i915/intel_display.c +8 -2

index f4c5263..5f3b636 100644
@@ -5258,12 +5258,20 @@ static void lpt_program_fdi_mphy(struct drm_i915_private *dev_priv)
      intel_sbi_write(dev_priv, 0x21EC, tmp, SBI_MPHY);
 }
 
-/* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */
-static void lpt_enable_clkout_dp(struct drm_device *dev)
+/* Implements 3 different sequences from BSpec chapter "Display iCLK
+ * Programming" based on the parameters passed:
+ * - Sequence to enable CLKOUT_DP
+ * - Sequence to enable CLKOUT_DP without spread
+ * - Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O
+ */
+static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread,
+                              bool with_fdi)
 {
      struct drm_i915_private *dev_priv = dev->dev_private;
      uint32_t tmp;
 

drivers/gpu/drm/i915/intel_display.c static void lpt_program_fdi_mphy(struct drm_i915_private *dev_priv) · line 5274

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +2 -0

+     WARN(with_fdi && !with_spread, "FDI requires downspread\n");
+

WARN_ON(with_fdi && IS_ULT(dev))?

Should we proceed after the WARN, or break out early?

Quoted 50 lines

drivers/gpu/drm/i915/intel_display.c +19 -11

      mutex_lock(&dev_priv->dpio_lock);
 
      tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);

@@ -5273,17 +5281,26 @@ static void lpt_enable_clkout_dp(struct drm_device *dev)
 
      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 (with_spread) {
+             tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+             tmp &= ~SBI_SSCCTL_PATHALT;
+             intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
 
-     lpt_reset_fdi_mphy(dev_priv);
-     lpt_program_fdi_mphy(dev_priv);
+             if (with_fdi) {
+                     lpt_reset_fdi_mphy(dev_priv);
+                     lpt_program_fdi_mphy(dev_priv);
+             }
+     }
 
-     /* ULT uses SBI_GEN0, but ULT doesn't have VGA, so we don't care. */
-     tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
-     tmp |= SBI_DBUFF0_ENABLE;
-     intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
+     if (IS_ULT(dev)) {
+             tmp = intel_sbi_read(dev_priv, SBI_GEN0, SBI_ICLK);
+             tmp |= SBI_GEN0_ENABLE;
+             intel_sbi_write(dev_priv, SBI_GEN0, tmp, SBI_ICLK);
+     } else {
+             tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
+             tmp |= SBI_DBUFF0_ENABLE;
+             intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
+     }
 
      mutex_unlock(&dev_priv->dpio_lock);
 }

@@ -5305,7 +5322,7 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
      if (!has_vga)
              return;
 
-     lpt_enable_clkout_dp(dev);
+     lpt_enable_clkout_dp(dev, true, true);
 }
 
 /*
--

Quoted 1 line

1.8.1.2

--
Ben Widawsky, Intel Open Source Technology Center

From: Paulo Zanoni <[email protected]>

We currently don't support HDMI clock bending nor use SSC for DP or HDMI on Haswell, so the only case where we need CLKOUT_DP is for VGA.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_display.c +34 -4

index 5f3b636..059c9a8 100644
@@ -5305,6 +5305,36 @@ static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread,
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
+/* Sequence to disable CLKOUT_DP */
+static void lpt_disable_clkout_dp(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t tmp;
+
+	mutex_lock(&dev_priv->dpio_lock);
+
+	if (IS_ULT(dev_priv->dev)) {
+		tmp = intel_sbi_read(dev_priv, SBI_GEN0, SBI_ICLK);
+		tmp &= ~SBI_GEN0_ENABLE;
+		intel_sbi_write(dev_priv, SBI_GEN0, tmp, SBI_ICLK);
+	} else {
+		tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
+		tmp &= ~SBI_DBUFF0_ENABLE;
+		intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
+	}
+
+	tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+	if (!(tmp & SBI_SSCCTL_PATHALT)) {
+		tmp |= SBI_SSCCTL_PATHALT;
+		intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
+		udelay(32);
+	}
+	tmp |= SBI_SSCCTL_DISABLE;
+	intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
+
+	mutex_unlock(&dev_priv->dpio_lock);
+}
+
 static void lpt_init_pch_refclk(struct drm_device *dev)
 {
 	struct drm_mode_config *mode_config = &dev->mode_config;

@@ -5319,10 +5349,10 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 		}
 	}
 
-	if (!has_vga)
-		return;
-
-	lpt_enable_clkout_dp(dev, true, true);
+	if (has_vga)
+		lpt_enable_clkout_dp(dev, true, true);
+	else
+		lpt_disable_clkout_dp(dev);
 }
 
 /*

Signature · 6 lines

1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 5 lines

From: Paulo Zanoni <[email protected]>

We currently don't support HDMI clock bending nor use SSC for DP or HDMI on Haswell, so the only case where we need CLKOUT_DP is for VGA.

Quoted 1 line

Signed-off-by Paulo Zanoni <[email protected]>

Just an aside: One of the many plans on my todo is to move the refclk updates into the global_modeset_resources callback and only enable the refclocks we actually need for the given configuration. Entirely different patch series though.

-Daniel

Quoted 63 lines

---
 drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_display.c +34 -5

index 5f3b636..059c9a8 100644
@@ -5305,6 +5305,36 @@ static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread,
      mutex_unlock(&dev_priv->dpio_lock);
 }
 
+/* Sequence to disable CLKOUT_DP */
+static void lpt_disable_clkout_dp(struct drm_device *dev)
+{
+     struct drm_i915_private *dev_priv = dev->dev_private;
+     uint32_t tmp;
+
+     mutex_lock(&dev_priv->dpio_lock);
+
+     if (IS_ULT(dev_priv->dev)) {
+             tmp = intel_sbi_read(dev_priv, SBI_GEN0, SBI_ICLK);
+             tmp &= ~SBI_GEN0_ENABLE;
+             intel_sbi_write(dev_priv, SBI_GEN0, tmp, SBI_ICLK);
+     } else {
+             tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
+             tmp &= ~SBI_DBUFF0_ENABLE;
+             intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
+     }
+
+     tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+     if (!(tmp & SBI_SSCCTL_PATHALT)) {
+             tmp |= SBI_SSCCTL_PATHALT;
+             intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
+             udelay(32);
+     }
+     tmp |= SBI_SSCCTL_DISABLE;
+     intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
+
+     mutex_unlock(&dev_priv->dpio_lock);
+}
+
 static void lpt_init_pch_refclk(struct drm_device *dev)
 {
      struct drm_mode_config *mode_config = &dev->mode_config;

@@ -5319,10 +5349,10 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
              }
      }
 
-     if (!has_vga)
-             return;
-
-     lpt_enable_clkout_dp(dev, true, true);
+     if (has_vga)
+             lpt_enable_clkout_dp(dev, true, true);
+     else
+             lpt_disable_clkout_dp(dev);
 }
 
 /*
--

1.8.1.2

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

From: Paulo Zanoni <[email protected]>

We currently don't support HDMI clock bending nor use SSC for DP or HDMI on Haswell, so the only case where we need CLKOUT_DP is for VGA.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_display.c +14 -0

index 5f3b636..059c9a8 100644
@@ -5305,6 +5305,36 @@ static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread,
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
+/* Sequence to disable CLKOUT_DP */
+static void lpt_disable_clkout_dp(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t tmp;
+
+	mutex_lock(&dev_priv->dpio_lock);
+
+	if (IS_ULT(dev_priv->dev)) {
+		tmp = intel_sbi_read(dev_priv, SBI_GEN0, SBI_ICLK);
+		tmp &= ~SBI_GEN0_ENABLE;
+		intel_sbi_write(dev_priv, SBI_GEN0, tmp, SBI_ICLK);
+	} else {
+		tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);

drivers/gpu/drm/i915/intel_display.c +10 -4

+	tmp |= SBI_SSCCTL_DISABLE;
+	intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
+
+	mutex_unlock(&dev_priv->dpio_lock);
+}
+
 static void lpt_init_pch_refclk(struct drm_device *dev)
 {
 	struct drm_mode_config *mode_config = &dev->mode_config;

@@ -5319,10 +5349,10 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 		}
 	}
 
-	if (!has_vga)
-		return;
-
-	lpt_enable_clkout_dp(dev, true, true);
+	if (has_vga)
+		lpt_enable_clkout_dp(dev, true, true);
+	else
+		lpt_disable_clkout_dp(dev);
 }

From: Paulo Zanoni <[email protected]>

For now there are no callers, but these functions are going to be needed for the code that allows Package C8+. Other future features may also require this code.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h      |  7 +++
 drivers/gpu/drm/i915/intel_display.c | 95 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 3 files changed, 105 insertions(+)

drivers/gpu/drm/i915/i915_reg.h +0 -0

index be6164f..8e5a5ec 100644

drivers/gpu/drm/i915/i915_reg.h +5 -0

+
+#define D_COMP				(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+#define  D_COMP_RCOMP_IN_PROGRESS	(1<<9)
+#define  D_COMP_COMP_FORCE		(1<<8)
+#define  D_COMP_COMP_DISABLE		(1<<0)
 
 /* Pipe WM_LINETIME - watermark line time */
 #define PIPE_WM_LINETIME_A		0x45270

drivers/gpu/drm/i915/intel_display.c +15 -0

index 059c9a8..ffb08bf 100644
@@ -5922,6 +5922,101 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	return true;
 }
 
+/*
+ * This function implements pieces of two sequences from BSpec:
+ * - Sequence for display software to disable LCPLL
+ * - Sequence for display software to allow package C8+
+ * The steps implemented here are just the steps that actually touch the LCPLL
+ * register. Callers should take care of disabling all the display engine
+ * functions, doing the mode unset, fixing interrupts, etc.
+ */
+void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
+		       bool switch_to_fclk, bool allow_power_down)
+{
+	uint32_t val;
+
+	val = I915_READ(LCPLL_CTL);
+

drivers/gpu/drm/i915/intel_display.c +5 -0

+	}
+
+	val |= LCPLL_PLL_DISABLE;
+	I915_WRITE(LCPLL_CTL, val);
+	POSTING_READ(LCPLL_CTL);

drivers/gpu/drm/i915/intel_display.c +9 -0

+
+	if (allow_power_down) {
+		val = I915_READ(LCPLL_CTL);
+		val |= LCPLL_POWER_DOWN_ALLOW;
+		I915_WRITE(LCPLL_CTL, val);
+		POSTING_READ(LCPLL_CTL);
+	}
+}
+

drivers/gpu/drm/i915/intel_display.c +18 -0

+
+	if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK, 5))
+		DRM_ERROR("LCPLL not locked yet\n");
+
+	if (val & LCPLL_CD_SOURCE_FCLK) {
+		val = I915_READ(LCPLL_CTL);
+		val &= ~LCPLL_CD_SOURCE_FCLK;
+		I915_WRITE(LCPLL_CTL, val);
+		POSTING_READ(LCPLL_CTL);
+
+		udelay(1);
+
+		val = I915_READ(LCPLL_CTL);
+		if (val & LCPLL_CD_SOURCE_FCLK_DONE)
+			DRM_ERROR("Switching back to LCPLL failed\n");
+	}
+}
+
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
 	bool enable = false;

drivers/gpu/drm/i915/intel_drv.h +0 -0

index 5dfc1a0..15989d1 100644

Quoted 6 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

For now there are no callers, but these functions are going to be needed for the code that allows Package C8+. Other future features may also require this code.

The thing that's missing from the patches is any sort of assertions about things being on before the disable sequence. Is this something we don't need to address?

Quoted 7 lines

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h      |  7 +++
 drivers/gpu/drm/i915/intel_display.c | 95 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 3 files changed, 105 insertions(+)

drivers/gpu/drm/i915/i915_reg.h line 4935

Quoted 11 lines

drivers/gpu/drm/i915/i915_reg.h +2 -0

index be6164f..8e5a5ec 100644
@@ -4930,7 +4930,14 @@
 #define  LCPLL_CLK_FREQ_450		(0<<26)
 #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
 #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
+#define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
 #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
+#define  LCPLL_CD_SOURCE_FCLK_DONE	(1<<19)

Hmm... the doc I am looking at says

Quoted 32 lines

drivers/gpu/drm/i915/i915_reg.h +5 -0

+
+#define D_COMP				(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+#define  D_COMP_RCOMP_IN_PROGRESS	(1<<9)
+#define  D_COMP_COMP_FORCE		(1<<8)
+#define  D_COMP_COMP_DISABLE		(1<<0)
 
 /* Pipe WM_LINETIME - watermark line time */
 #define PIPE_WM_LINETIME_A		0x45270

drivers/gpu/drm/i915/intel_display.c +15 -0

index 059c9a8..ffb08bf 100644
@@ -5922,6 +5922,101 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	return true;
 }
 
+/*
+ * This function implements pieces of two sequences from BSpec:
+ * - Sequence for display software to disable LCPLL
+ * - Sequence for display software to allow package C8+
+ * The steps implemented here are just the steps that actually touch the LCPLL
+ * register. Callers should take care of disabling all the display engine
+ * functions, doing the mode unset, fixing interrupts, etc.
+ */
+void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
+		       bool switch_to_fclk, bool allow_power_down)
+{
+	uint32_t val;
+
+	val = I915_READ(LCPLL_CTL);
+

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5949

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+	if (switch_to_fclk) {
+		val |= LCPLL_CD_SOURCE_FCLK;
+		I915_WRITE(LCPLL_CTL, val);
+		POSTING_READ(LCPLL_CTL);
+
+		udelay(1);
+
+		val = I915_READ(LCPLL_CTL);
+		if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
+			DRM_ERROR("Switching to FCLK failed\n");

wait_for_us(..., 1)?

Quoted 5 lines

drivers/gpu/drm/i915/intel_display.c +5 -0

+	}
+
+	val |= LCPLL_PLL_DISABLE;
+	I915_WRITE(LCPLL_CTL, val);
+	POSTING_READ(LCPLL_CTL);

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5964

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+
+	if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
+		DRM_ERROR("LCPLL still locked\n");
+
+	val = I915_READ(D_COMP);
+	val |= D_COMP_COMP_DISABLE;
+	I915_WRITE(D_COMP, val);
+	POSTING_READ(D_COMP);
+
+	udelay(2);

ndelay(100)?

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5968

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +4 -0

+
+	val = I915_READ(D_COMP);
+	if (val & D_COMP_RCOMP_IN_PROGRESS)
+		DRM_ERROR("D_COMP RCOMP still in progress\n");

wait_for(..., 1)?

Quoted 9 lines

drivers/gpu/drm/i915/intel_display.c +9 -0

+
+	if (allow_power_down) {
+		val = I915_READ(LCPLL_CTL);
+		val |= LCPLL_POWER_DOWN_ALLOW;
+		I915_WRITE(LCPLL_CTL, val);
+		POSTING_READ(LCPLL_CTL);
+	}
+}
+

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5987

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+/*
+ * Fully restores LCPLL, disallowing power down and switching back to LCPLL
+ * source.
+ */
+void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
+{
+	uint32_t val;
+
+	val = I915_READ(LCPLL_CTL);
+

I think we could potentially exit early here if the PLL is already locked, and we're on CDclk. And indeed, I've already seen this case occur, but I'm not sure I will ever see that case again.

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5997

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+	if (val & LCPLL_POWER_DOWN_ALLOW) {
+		val &= ~LCPLL_POWER_DOWN_ALLOW;
+		I915_WRITE(LCPLL_CTL, val);
+	}
+
+	val = I915_READ(D_COMP);
+	val |= D_COMP_COMP_FORCE;
+	val &= ~D_COMP_COMP_DISABLE;
+	I915_WRITE(D_COMP, val);
+

I think you need a posting read here. I am not sure we're allowed to read LCPLL_CTL until we know the write has landed.

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 6001

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +4 -0

+	val = I915_READ(LCPLL_CTL);
+	val &= ~LCPLL_PLL_DISABLE;
+	I915_WRITE(LCPLL_CTL, val);
+	POSTING_READ(LCPLL_CTL);

^ unnecessary POSTING_READ - but meh

Quoted 26 lines

drivers/gpu/drm/i915/intel_display.c +18 -0

+
+	if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK, 5))
+		DRM_ERROR("LCPLL not locked yet\n");
+
+	if (val & LCPLL_CD_SOURCE_FCLK) {
+		val = I915_READ(LCPLL_CTL);
+		val &= ~LCPLL_CD_SOURCE_FCLK;
+		I915_WRITE(LCPLL_CTL, val);
+		POSTING_READ(LCPLL_CTL);
+
+		udelay(1);
+
+		val = I915_READ(LCPLL_CTL);
+		if (val & LCPLL_CD_SOURCE_FCLK_DONE)
+			DRM_ERROR("Switching back to LCPLL failed\n");
+	}
+}
+
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
 	bool enable = false;

drivers/gpu/drm/i915/intel_drv.h +0 -0

index 5dfc1a0..15989d1 100644

drivers/gpu/drm/i915/intel_drv.h extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, · line 839

Quoted 8 lines

drivers/gpu/drm/i915/intel_drv.h +3 -0

@@ -832,5 +832,8 @@ extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 						 enum transcoder pch_transcoder,
 						 bool enable);
+extern void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
+			      bool switch_to_fclk, bool allow_power_down);
+extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
 
 #endif /* __INTEL_DRV_H__ */

I'm a bit torn as to whether or not it makes sense to extract the pure LCPLL disable from hsw_disable_lcpll. Did you think about this, could you explain the reason you decided against it? (I'm a bit partial since that was the way I had written it).

Does it every make sense to switch to fclk and not allow_power_down?

Signature · 5 lines

Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 19 lines · Paulo Zanoni

Quoted 6 lines

From: Paulo Zanoni <[email protected]>

For now there are no callers, but these functions are going to be needed for the code that allows Package C8+. Other future features may also require this code.

The thing that's missing from the patches is any sort of assertions about things being on before the disable sequence. Is this something we don't need to address?

Quoted 7 lines

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h      |  7 +++
 drivers/gpu/drm/i915/intel_display.c | 95 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 3 files changed, 105 insertions(+)

Quoted 13 lines

drivers/gpu/drm/i915/i915_reg.h line 4935

Quoted 11 lines

drivers/gpu/drm/i915/i915_reg.h +2 -0

index be6164f..8e5a5ec 100644
@@ -4930,7 +4930,14 @@
 #define  LCPLL_CLK_FREQ_450                (0<<26)
 #define  LCPLL_CD_CLOCK_DISABLE            (1<<25)
 #define  LCPLL_CD2X_CLOCK_DISABLE  (1<<23)
+#define  LCPLL_POWER_DOWN_ALLOW            (1<<22)
 #define  LCPLL_CD_SOURCE_FCLK              (1<<21)
+#define  LCPLL_CD_SOURCE_FCLK_DONE (1<<19)

Hmm... the doc I am looking at says

Oops. The doc I was looking at had some different names for things, was what I wanted to say.

Quoted 155 lines

Quoted 33 lines

drivers/gpu/drm/i915/i915_reg.h +5 -0

+
+#define D_COMP                             (MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+#define  D_COMP_RCOMP_IN_PROGRESS  (1<<9)
+#define  D_COMP_COMP_FORCE         (1<<8)
+#define  D_COMP_COMP_DISABLE               (1<<0)
 
 /* Pipe WM_LINETIME - watermark line time */
 #define PIPE_WM_LINETIME_A         0x45270

drivers/gpu/drm/i915/intel_display.c +15 -0

index 059c9a8..ffb08bf 100644
@@ -5922,6 +5922,101 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
    return true;
 }
 
+/*
+ * This function implements pieces of two sequences from BSpec:
+ * - Sequence for display software to disable LCPLL
+ * - Sequence for display software to allow package C8+
+ * The steps implemented here are just the steps that actually touch the LCPLL
+ * register. Callers should take care of disabling all the display engine
+ * functions, doing the mode unset, fixing interrupts, etc.
+ */
+void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
+                  bool switch_to_fclk, bool allow_power_down)
+{
+   uint32_t val;
+
+   val = I915_READ(LCPLL_CTL);
+

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5949

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+   if (switch_to_fclk) {
+           val |= LCPLL_CD_SOURCE_FCLK;
+           I915_WRITE(LCPLL_CTL, val);
+           POSTING_READ(LCPLL_CTL);
+
+           udelay(1);
+
+           val = I915_READ(LCPLL_CTL);
+           if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
+                   DRM_ERROR("Switching to FCLK failed\n");

wait_for_us(..., 1)?

Quoted 5 lines

drivers/gpu/drm/i915/intel_display.c +5 -0

+   }
+
+   val |= LCPLL_PLL_DISABLE;
+   I915_WRITE(LCPLL_CTL, val);
+   POSTING_READ(LCPLL_CTL);

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5964

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+
+   if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
+           DRM_ERROR("LCPLL still locked\n");
+
+   val = I915_READ(D_COMP);
+   val |= D_COMP_COMP_DISABLE;
+   I915_WRITE(D_COMP, val);
+   POSTING_READ(D_COMP);
+
+   udelay(2);

ndelay(100)?

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5968

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +4 -0

+
+   val = I915_READ(D_COMP);
+   if (val & D_COMP_RCOMP_IN_PROGRESS)
+           DRM_ERROR("D_COMP RCOMP still in progress\n");

wait_for(..., 1)?

Quoted 9 lines

drivers/gpu/drm/i915/intel_display.c +9 -0

+
+   if (allow_power_down) {
+           val = I915_READ(LCPLL_CTL);
+           val |= LCPLL_POWER_DOWN_ALLOW;
+           I915_WRITE(LCPLL_CTL, val);
+           POSTING_READ(LCPLL_CTL);
+   }
+}
+

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5987

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+/*
+ * Fully restores LCPLL, disallowing power down and switching back to LCPLL
+ * source.
+ */
+void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
+{
+   uint32_t val;
+
+   val = I915_READ(LCPLL_CTL);
+

I think we could potentially exit early here if the PLL is already locked, and we're on CDclk. And indeed, I've already seen this case occur, but I'm not sure I will ever see that case again.

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5997

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+   if (val & LCPLL_POWER_DOWN_ALLOW) {
+           val &= ~LCPLL_POWER_DOWN_ALLOW;
+           I915_WRITE(LCPLL_CTL, val);
+   }
+
+   val = I915_READ(D_COMP);
+   val |= D_COMP_COMP_FORCE;
+   val &= ~D_COMP_COMP_DISABLE;
+   I915_WRITE(D_COMP, val);
+

I think you need a posting read here. I am not sure we're allowed to read LCPLL_CTL until we know the write has landed.

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 6001

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +4 -0

+   val = I915_READ(LCPLL_CTL);
+   val &= ~LCPLL_PLL_DISABLE;
+   I915_WRITE(LCPLL_CTL, val);
+   POSTING_READ(LCPLL_CTL);

^ unnecessary POSTING_READ - but meh

Quoted 26 lines

drivers/gpu/drm/i915/intel_display.c +18 -0

+
+   if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK, 5))
+           DRM_ERROR("LCPLL not locked yet\n");
+
+   if (val & LCPLL_CD_SOURCE_FCLK) {
+           val = I915_READ(LCPLL_CTL);
+           val &= ~LCPLL_CD_SOURCE_FCLK;
+           I915_WRITE(LCPLL_CTL, val);
+           POSTING_READ(LCPLL_CTL);
+
+           udelay(1);
+
+           val = I915_READ(LCPLL_CTL);
+           if (val & LCPLL_CD_SOURCE_FCLK_DONE)
+                   DRM_ERROR("Switching back to LCPLL failed\n");
+   }
+}
+
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
    bool enable = false;

drivers/gpu/drm/i915/intel_drv.h +0 -0

index 5dfc1a0..15989d1 100644

drivers/gpu/drm/i915/intel_drv.h extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, · line 839

Quoted 8 lines

drivers/gpu/drm/i915/intel_drv.h +3 -0

@@ -832,5 +832,8 @@ extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
                                             enum transcoder pch_transcoder,
                                             bool enable);
+extern void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
+                         bool switch_to_fclk, bool allow_power_down);
+extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
 
 #endif /* __INTEL_DRV_H__ */

I'm a bit torn as to whether or not it makes sense to extract the pure LCPLL disable from hsw_disable_lcpll. Did you think about this, could you explain the reason you decided against it? (I'm a bit partial since that was the way I had written it).

Quoted 8 lines

Does it every make sense to switch to fclk and not allow_power_down?

--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ben Widawsky, Intel Open Source Technology Center

Quoted 6 lines

From: Paulo Zanoni <[email protected]>

For now there are no callers, but these functions are going to be needed for the code that allows Package C8+. Other future features may also require this code.

The thing that's missing from the patches is any sort of assertions about things being on before the disable sequence. Is this something we don't need to address?

Quoted 13 lines

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h      |  7 +++
 drivers/gpu/drm/i915/intel_display.c | 95 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 3 files changed, 105 insertions(+)

drivers/gpu/drm/i915/i915_reg.h +0 -0

index be6164f..8e5a5ec 100644

drivers/gpu/drm/i915/i915_reg.h line 4935

Quoted 6 lines

drivers/gpu/drm/i915/i915_reg.h +2 -0

@@ -4930,7 +4930,14 @@
 #define  LCPLL_CLK_FREQ_450          (0<<26)
 #define  LCPLL_CD_CLOCK_DISABLE              (1<<25)
 #define  LCPLL_CD2X_CLOCK_DISABLE    (1<<23)
+#define  LCPLL_POWER_DOWN_ALLOW              (1<<22)
 #define  LCPLL_CD_SOURCE_FCLK                (1<<21)
+#define  LCPLL_CD_SOURCE_FCLK_DONE   (1<<19)

Hmm... the doc I am looking at says

Quoted 35 lines

drivers/gpu/drm/i915/i915_reg.h +5 -0

+
+#define D_COMP                               (MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+#define  D_COMP_RCOMP_IN_PROGRESS    (1<<9)
+#define  D_COMP_COMP_FORCE           (1<<8)
+#define  D_COMP_COMP_DISABLE         (1<<0)
 
 /* Pipe WM_LINETIME - watermark line time */
 #define PIPE_WM_LINETIME_A           0x45270

drivers/gpu/drm/i915/intel_display.c +19 -0

index 059c9a8..ffb08bf 100644
@@ -5922,6 +5922,101 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
      return true;
 }
 
+/*
+ * This function implements pieces of two sequences from BSpec:
+ * - Sequence for display software to disable LCPLL
+ * - Sequence for display software to allow package C8+
+ * The steps implemented here are just the steps that actually touch the LCPLL
+ * register. Callers should take care of disabling all the display engine
+ * functions, doing the mode unset, fixing interrupts, etc.
+ */
+void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
+                    bool switch_to_fclk, bool allow_power_down)
+{
+     uint32_t val;
+
+     val = I915_READ(LCPLL_CTL);
+
+     if (switch_to_fclk) {
+             val |= LCPLL_CD_SOURCE_FCLK;
+             I915_WRITE(LCPLL_CTL, val);
+             POSTING_READ(LCPLL_CTL);

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5949

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.c +6 -0

+
+             udelay(1);
+
+             val = I915_READ(LCPLL_CTL);
+             if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
+                     DRM_ERROR("Switching to FCLK failed\n");

wait_for_us(..., 1)?

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.c +5 -0

+     }
+
+     val |= LCPLL_PLL_DISABLE;
+     I915_WRITE(LCPLL_CTL, val);
+     POSTING_READ(LCPLL_CTL);

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5964

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+
+     if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
+             DRM_ERROR("LCPLL still locked\n");
+
+     val = I915_READ(D_COMP);
+     val |= D_COMP_COMP_DISABLE;
+     I915_WRITE(D_COMP, val);
+     POSTING_READ(D_COMP);
+
+     udelay(2);

ndelay(100)?

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5968

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +4 -0

+
+     val = I915_READ(D_COMP);
+     if (val & D_COMP_RCOMP_IN_PROGRESS)
+             DRM_ERROR("D_COMP RCOMP still in progress\n");

wait_for(..., 1)?

Quoted 16 lines

drivers/gpu/drm/i915/intel_display.c +15 -0

+
+     if (allow_power_down) {
+             val = I915_READ(LCPLL_CTL);
+             val |= LCPLL_POWER_DOWN_ALLOW;
+             I915_WRITE(LCPLL_CTL, val);
+             POSTING_READ(LCPLL_CTL);
+     }
+}
+
+/*
+ * Fully restores LCPLL, disallowing power down and switching back to LCPLL
+ * source.
+ */
+void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
+{

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5939

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +4 -0

+     uint32_t val;
+
+     val = I915_READ(LCPLL_CTL);
+

I think we could potentially exit early here if the PLL is already locked, and we're on CDclk. And indeed, I've already seen this case occur, but I'm not sure I will ever see that case again.

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5997

Quoted 11 lines

drivers/gpu/drm/i915/intel_display.c +10 -0

+     if (val & LCPLL_POWER_DOWN_ALLOW) {
+             val &= ~LCPLL_POWER_DOWN_ALLOW;
+             I915_WRITE(LCPLL_CTL, val);
+     }
+
+     val = I915_READ(D_COMP);
+     val |= D_COMP_COMP_FORCE;
+     val &= ~D_COMP_COMP_DISABLE;
+     I915_WRITE(D_COMP, val);
+

I think you need a posting read here. I am not sure we're allowed to read LCPLL_CTL until we know the write has landed.

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 6001

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.c +4 -0

+     val = I915_READ(LCPLL_CTL);
+     val &= ~LCPLL_PLL_DISABLE;
+     I915_WRITE(LCPLL_CTL, val);
+     POSTING_READ(LCPLL_CTL);

^ unnecessary POSTING_READ - but meh

Quoted 33 lines

drivers/gpu/drm/i915/intel_display.c +18 -0

+
+     if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK, 5))
+             DRM_ERROR("LCPLL not locked yet\n");
+
+     if (val & LCPLL_CD_SOURCE_FCLK) {
+             val = I915_READ(LCPLL_CTL);
+             val &= ~LCPLL_CD_SOURCE_FCLK;
+             I915_WRITE(LCPLL_CTL, val);
+             POSTING_READ(LCPLL_CTL);
+
+             udelay(1);
+
+             val = I915_READ(LCPLL_CTL);
+             if (val & LCPLL_CD_SOURCE_FCLK_DONE)
+                     DRM_ERROR("Switching back to LCPLL failed\n");
+     }
+}
+
 static void haswell_modeset_global_resources(struct drm_device *dev)
 {
      bool enable = false;

drivers/gpu/drm/i915/intel_drv.h +3 -0

index 5dfc1a0..15989d1 100644
@@ -832,5 +832,8 @@ extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
                                               enum transcoder pch_transcoder,
                                               bool enable);
+extern void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
+                           bool switch_to_fclk, bool allow_power_down);
+extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
 

drivers/gpu/drm/i915/intel_drv.h extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev, · line 839

Quoted 1 line

drivers/gpu/drm/i915/intel_drv.h +0 -0

 #endif /* __INTEL_DRV_H__ */

I'm a bit torn as to whether or not it makes sense to extract the pure LCPLL disable from hsw_disable_lcpll. Did you think about this, could you explain the reason you decided against it? (I'm a bit partial since that was the way I had written it).

Does it every make sense to switch to fclk and not allow_power_down?

--
Ben Widawsky, Intel Open Source Technology Center

From: Paulo Zanoni <[email protected]>

We have 2 possible LPT PCHs: the normal version, which contains the pixel path (FDI, transcoders, VGA, etc), and the LP version, which comes with ULT machines and doesn't contain the pixel path. Both models return true for HAS_PCH_LPT.

We already have a few places where we explicitly check for LPT-LP, so add a HAS_LP_PCH check to simplify the code.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h      | 2 ++
 drivers/gpu/drm/i915/intel_display.c | 9 +++------
 drivers/gpu/drm/i915/intel_pm.c      | 4 ++--
 3 files changed, 7 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 2885265..262c3d4 100644
@@ -1578,6 +1578,8 @@ struct drm_i915_file_private {
 #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
 #define HAS_PCH_NOP(dev) (INTEL_PCH_TYPE(dev) == PCH_NOP)
 #define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE)
+#define HAS_LP_PCH(dev_priv) ((dev_priv)->pch_id == \
+				INTEL_PCH_LPT_LP_DEVICE_ID_TYPE)
 
 #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)
 

drivers/gpu/drm/i915/intel_display.c +3 -6

index c35a613..1d3c805 100644
@@ -5274,8 +5274,7 @@ static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread,
 
 	if (WARN(with_fdi && !with_spread, "FDI requires downspread\n"))
 		with_spread = true;
-	if (WARN(dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE &&
-		 with_fdi, "LP PCH doesn't have FDI\n"))
+	if (WARN(HAS_LP_PCH(dev_priv) && with_fdi, "LP PCH doesn't have FDI\n"))
 		with_fdi = false;
 
 	mutex_lock(&dev_priv->dpio_lock);

@@ -5298,8 +5297,7 @@ static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread,
 		}
 	}
 
-	reg = (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) ?
-	       SBI_GEN0 : SBI_DBUFF0;
+	reg = HAS_LP_PCH(dev_priv) ? SBI_GEN0 : SBI_DBUFF0;
 	tmp = intel_sbi_read(dev_priv, reg, SBI_ICLK);
 	tmp |= SBI_GEN0_CFG_BUFFENABLE_DISABLE;
 	intel_sbi_write(dev_priv, reg, tmp, SBI_ICLK);

@@ -5315,8 +5313,7 @@ static void lpt_disable_clkout_dp(struct drm_device *dev)
 
 	mutex_lock(&dev_priv->dpio_lock);
 
-	reg = (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) ?
-	       SBI_GEN0 : SBI_DBUFF0;
+	reg = HAS_LP_PCH(dev_priv) ? SBI_GEN0 : SBI_DBUFF0;
 	tmp = intel_sbi_read(dev_priv, reg, SBI_ICLK);
 	tmp &= ~SBI_GEN0_CFG_BUFFENABLE_DISABLE;
 	intel_sbi_write(dev_priv, reg, tmp, SBI_ICLK);

drivers/gpu/drm/i915/intel_pm.c +2 -2

index 43031ec..7643b16 100644
@@ -4650,7 +4650,7 @@ static void lpt_init_clock_gating(struct drm_device *dev)
 	 * TODO: this bit should only be enabled when really needed, then
 	 * disabled when not needed anymore in order to save power.
 	 */
-	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE)
+	if (HAS_LP_PCH(dev_priv))
 		I915_WRITE(SOUTH_DSPCLK_GATE_D,
 			   I915_READ(SOUTH_DSPCLK_GATE_D) |
 			   PCH_LP_PARTITION_LEVEL_DISABLE);

@@ -4665,7 +4665,7 @@ static void lpt_suspend_hw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+	if (HAS_LP_PCH(dev_priv)) {
 		uint32_t val = I915_READ(SOUTH_DSPCLK_GATE_D);
 
 		val &= ~PCH_LP_PARTITION_LEVEL_DISABLE;

Signature · 6 lines

1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 11 lines

From: Paulo Zanoni <[email protected]>

We have 2 possible LPT PCHs: the normal version, which contains the pixel path (FDI, transcoders, VGA, etc), and the LP version, which comes with ULT machines and doesn't contain the pixel path. Both models return true for HAS_PCH_LPT.

We already have a few places where we explicitly check for LPT-LP, so add a HAS_LP_PCH check to simplify the code.

Signed-off-by Paulo Zanoni <[email protected]>

1-4 is:

Reviewed-by Ben Widawsky <[email protected]>

This patch I believe should have come first in the series (we discussed on IRC), but meh.

Also, I think the name HAS_LP_PCH is no good since the LP is "Low Power." It really should be HAS_PCH_LPT_LP. I realize that's redundant within the functions since they are lpt only, but this is defined in the primary header file...

With that this too is:

Reviewed-by Ben Widawsky <[email protected]>

Quoted 78 lines

---
 drivers/gpu/drm/i915/i915_drv.h      | 2 ++
 drivers/gpu/drm/i915/intel_display.c | 9 +++------
 drivers/gpu/drm/i915/intel_pm.c      | 4 ++--
 3 files changed, 7 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +2 -0

index 2885265..262c3d4 100644
@@ -1578,6 +1578,8 @@ struct drm_i915_file_private {
 #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
 #define HAS_PCH_NOP(dev) (INTEL_PCH_TYPE(dev) == PCH_NOP)
 #define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE)
+#define HAS_LP_PCH(dev_priv) ((dev_priv)->pch_id == \
+                             INTEL_PCH_LPT_LP_DEVICE_ID_TYPE)
 
 #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)
 

drivers/gpu/drm/i915/intel_display.c +3 -6

index c35a613..1d3c805 100644
@@ -5274,8 +5274,7 @@ static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread,
 
      if (WARN(with_fdi && !with_spread, "FDI requires downspread\n"))
              with_spread = true;
-     if (WARN(dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE &&
-              with_fdi, "LP PCH doesn't have FDI\n"))
+     if (WARN(HAS_LP_PCH(dev_priv) && with_fdi, "LP PCH doesn't have FDI\n"))
              with_fdi = false;
 
      mutex_lock(&dev_priv->dpio_lock);

@@ -5298,8 +5297,7 @@ static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread,
              }
      }
 
-     reg = (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) ?
-            SBI_GEN0 : SBI_DBUFF0;
+     reg = HAS_LP_PCH(dev_priv) ? SBI_GEN0 : SBI_DBUFF0;
      tmp = intel_sbi_read(dev_priv, reg, SBI_ICLK);
      tmp |= SBI_GEN0_CFG_BUFFENABLE_DISABLE;
      intel_sbi_write(dev_priv, reg, tmp, SBI_ICLK);

@@ -5315,8 +5313,7 @@ static void lpt_disable_clkout_dp(struct drm_device *dev)
 
      mutex_lock(&dev_priv->dpio_lock);
 
-     reg = (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) ?
-            SBI_GEN0 : SBI_DBUFF0;
+     reg = HAS_LP_PCH(dev_priv) ? SBI_GEN0 : SBI_DBUFF0;
      tmp = intel_sbi_read(dev_priv, reg, SBI_ICLK);
      tmp &= ~SBI_GEN0_CFG_BUFFENABLE_DISABLE;
      intel_sbi_write(dev_priv, reg, tmp, SBI_ICLK);

drivers/gpu/drm/i915/intel_pm.c +2 -3

index 43031ec..7643b16 100644
@@ -4650,7 +4650,7 @@ static void lpt_init_clock_gating(struct drm_device *dev)
       * TODO: this bit should only be enabled when really needed, then
       * disabled when not needed anymore in order to save power.
       */
-     if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE)
+     if (HAS_LP_PCH(dev_priv))
              I915_WRITE(SOUTH_DSPCLK_GATE_D,
                         I915_READ(SOUTH_DSPCLK_GATE_D) |
                         PCH_LP_PARTITION_LEVEL_DISABLE);

@@ -4665,7 +4665,7 @@ static void lpt_suspend_hw(struct drm_device *dev)
 {
      struct drm_i915_private *dev_priv = dev->dev_private;
 
-     if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+     if (HAS_LP_PCH(dev_priv)) {
              uint32_t val = I915_READ(SOUTH_DSPCLK_GATE_D);
 
              val &= ~PCH_LP_PARTITION_LEVEL_DISABLE;
--

1.8.1.2

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ben Widawsky, Intel Open Source Technology Center

From: Paulo Zanoni <[email protected]>

Most of the hardware needs to be disabled before LCPLL is disabled, so let's add a function to assert some of items listed in the "Display Sequences for LCPLL disabling" documentation.

The idea is that hsw_disable_lcpll should not disable the hardware, the callers need to take care of calling hsw_disable_lcpll only once everything is already disabled.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h      |  8 ++++++++
 drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

drivers/gpu/drm/i915/i915_reg.h +8 -0

index 8e5a5ec..9556dff 100644
@@ -2195,6 +2195,8 @@
 #define BLC_PWM_CPU_CTL2	0x48250
 #define BLC_PWM_CPU_CTL		0x48254
 
+#define HSW_BLC_PWM2_CTL	0x48350
+
 /* PCH CTL1 is totally different, all but the below bits are reserved. CTL2 is
  * like the normal CTL from gen4 and earlier. Hooray for confusing naming. */
 #define BLC_PWM_PCH_CTL1	0xc8250

@@ -2203,6 +2205,12 @@
 #define   BLM_PCH_POLARITY			(1 << 29)
 #define BLC_PWM_PCH_CTL2	0xc8254
 
+#define UTIL_PIN_CTL		0x48400
+#define   UTIL_PIN_ENABLE	(1 << 31)
+
+#define PCH_GTC_CTL		0xe7000
+#define   PCH_GTC_ENABLE	(1 << 31)
+
 /* TV port control */
 #define TV_CTL			0x68000
 /** Enables the TV encoder */

drivers/gpu/drm/i915/intel_display.c +14 -0

index ffb08bf..9055f60 100644
@@ -5922,6 +5922,32 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	return true;
 }
 
+static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
+	struct intel_crtc *crtc;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
+		WARN(crtc->base.enabled, "CRTC for pipe %c enabled\n",
+		     pipe_name(crtc->pipe));
+
+	WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
+	WARN(plls->spll_refcount, "SPLL enabled\n");
+	WARN(plls->wrpll1_refcount, "WRPLL1 enabled\n");
+	WARN(plls->wrpll2_refcount, "WRPLL2 enabled\n");

drivers/gpu/drm/i915/intel_display.c +0 -0

 	val = I915_READ(LCPLL_CTL);
 
 	if (switch_to_fclk) {

Signature · 6 lines

1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 10 lines

From: Paulo Zanoni <[email protected]>

Most of the hardware needs to be disabled before LCPLL is disabled, so let's add a function to assert some of items listed in the "Display Sequences for LCPLL disabling" documentation.

The idea is that hsw_disable_lcpll should not disable the hardware, the callers need to take care of calling hsw_disable_lcpll only once everything is already disabled.

Quoted 1 line

Signed-off-by Paulo Zanoni <[email protected]>

I don't see a reason to separate this patch from the previous one. It makes review easier, but it's weird bisect wise. The correct order, if you wanted to do them as separate patches would be to introduce the assertions, and then add the functionality (I think).

Quoted 59 lines

---
 drivers/gpu/drm/i915/i915_reg.h      |  8 ++++++++
 drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

drivers/gpu/drm/i915/i915_reg.h +8 -0

index 8e5a5ec..9556dff 100644
@@ -2195,6 +2195,8 @@
 #define BLC_PWM_CPU_CTL2     0x48250
 #define BLC_PWM_CPU_CTL              0x48254
 
+#define HSW_BLC_PWM2_CTL     0x48350
+
 /* PCH CTL1 is totally different, all but the below bits are reserved. CTL2 is
  * like the normal CTL from gen4 and earlier. Hooray for confusing naming. */
 #define BLC_PWM_PCH_CTL1     0xc8250

@@ -2203,6 +2205,12 @@
 #define   BLM_PCH_POLARITY                   (1 << 29)
 #define BLC_PWM_PCH_CTL2     0xc8254
 
+#define UTIL_PIN_CTL         0x48400
+#define   UTIL_PIN_ENABLE    (1 << 31)
+
+#define PCH_GTC_CTL          0xe7000
+#define   PCH_GTC_ENABLE     (1 << 31)
+
 /* TV port control */
 #define TV_CTL                       0x68000
 /** Enables the TV encoder */

drivers/gpu/drm/i915/intel_display.c +19 -0

index ffb08bf..9055f60 100644
@@ -5922,6 +5922,32 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
      return true;
 }
 
+static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
+{
+     struct drm_device *dev = dev_priv->dev;
+     struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
+     struct intel_crtc *crtc;
+
+     list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
+             WARN(crtc->base.enabled, "CRTC for pipe %c enabled\n",
+                  pipe_name(crtc->pipe));
+
+     WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
+     WARN(plls->spll_refcount, "SPLL enabled\n");
+     WARN(plls->wrpll1_refcount, "WRPLL1 enabled\n");
+     WARN(plls->wrpll2_refcount, "WRPLL2 enabled\n");
+     WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
+     WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE,
+          "CPU PWM1 enabled\n");
+     WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE,
+          "CPU PWM2 enabled\n");

drivers/gpu/drm/i915/intel_display.c static bool ironlake_get_pipe_config(struct intel_crtc *crtc, · line 5948

Quoted 5 lines

drivers/gpu/drm/i915/intel_display.c +5 -0

+     WARN(I915_READ(BLC_PWM_PCH_CTL1) & BLM_PCH_PWM_ENABLE,
+          "PCH PWM1 enabled\n");
+     WARN(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE,
+          "Utility pin enabled\n");
+     WARN(I915_READ(PCH_GTC_CTL) & PCH_GTC_ENABLE, "PCH GTC enabled\n");

Looking at probably the same list you've looked at, I don't see:
Audio controller

eDP HPD
Other CPU/PCH interrupts

You've worked with this code longer than I have, so maybe you can explain why you've skipped them.

Quoted 9 lines

drivers/gpu/drm/i915/intel_display.c +2 -0

+}
+
 /*
  * This function implements pieces of two sequences from BSpec:
  * - Sequence for display software to disable LCPLL

@@ -5935,6 +5961,8 @@ void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
 {
      uint32_t val;
 

drivers/gpu/drm/i915/intel_display.c void hsw_disable_lcpll(struct drm_i915_private *dev_priv, · line 5965

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +2 -0

+     assert_can_disable_lcpll(dev_priv);
+

Do we want to proceed if the above fails? I was sort of under the impression that hard hangs can occur as a result of some functions still being enabled when we change clocks. I'm fine with continuing on since we have the WARNS, just wondering if you've thought about it.

Quoted 11 lines

drivers/gpu/drm/i915/intel_display.c +0 -1

      val = I915_READ(LCPLL_CTL);
 
      if (switch_to_fclk) {
--

1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ben Widawsky, Intel Open Source Technology Center

From: Paulo Zanoni <[email protected]>

Hi

On the code that allows Package C8+ we need to disable/reenable the PCH reference clocks and also disable/reenable LCPLL. This series implements the sequences that are going to be needed. I have local patches that use this code and they seem to work (we survive going to C10 and back to C7), and I also sent earlier versions of these patches to the mailing list, so open-source code that uses these functions already exists on the intel-gfx mailing list. The goal here is allow people to review/merge this while the final PC8+ patches are not yet 100% reviewer-compliant.

On the first 3 patches we massage our code so we can reuse it for PC8+ code. On patch 4 we extend the current code to also implement one of the sequences required by PC8+. On patch 5 we implement the equivalent disable sequence, also needed by PC8+. On patch 6 we implement the functions to disable and restore LCPLL, and on patch 7 we add some assertions that are going to be used.

If you want to review patches 1-3 all you need is C experience. For patches 4 and 5 you need to read the "Display iCLK programming" chapter of BSpec. For patches 6 and 7 you need to read "Display sequences for LCPLL disabling" and "Display sequences for Package C8".

Cheers,
Paulo

Paulo Zanoni (7):
drm/i915: remove SDV support from lpt_pch_init_refclk drm/i915: extract FDI mPHY functions from lpt_init_pch_refclk drm/i915: extract lpt_enable_clkout_dp from lpt_init_pch_refclk drm/i915: extend lpt_enable_clkout_dp
drm/i915: disable CLKOUT_DP when it's not needed drm/i915: add functions to disable and restore LCPLL drm/i915: add some assertions to hsw_disable_lcpll

 drivers/gpu/drm/i915/i915_reg.h      |  17 ++
 drivers/gpu/drm/i915/intel_display.c | 359 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 3 files changed, 276 insertions(+), 103 deletions(-)