drm/i915: extend lpt_enable_clkout_dp

PATCH In review Paulo Zanoni 2 participants 6 messages Jul 23, 2013

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Paulo Zanoni

author

2013-07-23 14:19 UTC #0 lore raw

PATCH 1/4 drm/i915: extend lpt_enable_clkout_dp

From: Paulo Zanoni <[email protected]>

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

v2: - Change IS_ULT checks for LPT-LP checks
- Add check for LPT-LP + with_fdi (Ben)
- Merge DBUFF0/GEN0 bit definitions since they're the same register (Ben)
- DBUFF0 (1<<0) is Disable, not Enable

Reviewed-by Ben Widawsky <[email protected]>

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

---
 drivers/gpu/drm/i915/i915_reg.h      |  3 ++-
 drivers/gpu/drm/i915/intel_display.c | 43 +++++++++++++++++++++++++-----------
 2 files changed, 32 insertions(+), 14 deletions(-)

Resending the remaining patches of this series on a separate thread, as requested by Daniel.

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

index e20f093..0dfcbad 100644
@@ -4942,7 +4942,8 @@
 #define  SBI_SSCAUXDIV6				0x0610
 #define   SBI_SSCAUXDIV_FINALDIV2SEL(x)		((x)<<4)
 #define  SBI_DBUFF0				0x2a00
-#define   SBI_DBUFF0_ENABLE			(1<<0)
+#define  SBI_GEN0				0x1f00
+#define   SBI_GEN0_CFG_BUFFENABLE_DISABLE	(1<<0)
 
 /* LPT PIXCLK_GATE */
 #define PIXCLK_GATE			0xC6020

drivers/gpu/drm/i915/intel_display.c +30 ~~-13~~

65 lines not on this page · raw message

Signature · 1 line

1.8.1.2

Paulo Zanoni

author

2013-07-23 14:19 UTC #1 lore raw

PATCH 2/4 drm/i915: disable CLKOUT_DP when it's not needed

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.

v2: - Replace the IS_ULT check for LPT-LP
- Simplify GEN0/DBUFF0 check due to change on the previous patch
- Also check for SBI_SSCCTL_DISABLE (Ben).

Reviewed-by Ben Widawsky <[email protected]>

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

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

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

index 7f61ac5..deee650d 100644
@@ -5307,6 +5307,34 @@ 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 reg, tmp;
+
+	mutex_lock(&dev_priv->dpio_lock);
+
+	reg = (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) ?
+	       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);
+
+	tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+	if (!(tmp & SBI_SSCCTL_DISABLE)) {
+		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;

@@ -5321,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 · 1 line

1.8.1.2

Paulo Zanoni

author

2013-07-23 14:19 UTC #2 lore raw

PATCH 3/4 drm/i915: add functions to disable and restore LCPLL

In reply to 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.

Also merge the commit which introduced assert_can_disable_lcpll and had the following commit message:

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.

v2: - Rebase.
- Fix D_COMP wait timeout.
v3: - Use wait_for_atomic_use (Ben)
- Remove/add a useless/needed POSTING_READ (Ben)
- Early return in case LCPLL is already restored (Ben)
- Add ndelay(100) (Ben)
v4: - Merge the commit that added assert_can_disable_lcpll (Ben)
- Add interrupt assertions (Ben)

Reviewed-by Ben Widawsky <[email protected]>

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

---
 drivers/gpu/drm/i915/i915_reg.h      |  15 ++++
 drivers/gpu/drm/i915/intel_display.c | 136 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 3 files changed, 154 insertions(+)

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

index 0dfcbad..6caa748 100644
@@ -2261,6 +2261,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

@@ -2269,6 +2271,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 */

@@ -5009,7 +5017,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)
+
+#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 +136 -0

142 lines not on this page · raw message

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

index 31087ff..3fbe80b 100644
@@ -838,5 +838,8 @@ extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_update(struct drm_device *dev);
+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__ */

Signature · 1 line

1.8.1.2

Daniel Vetter

2013-07-23 14:49 UTC #3 lore raw

Re: [PATCH 3/4] drm/i915: add functions to disable and restore LCPLL

Quoted 83 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.
Also merge the commit which introduced assert_can_disable_lcpll and had the following commit message:
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.
v2: - Rebase.
- Fix D_COMP wait timeout.
v3: - Use wait_for_atomic_use (Ben)
- Remove/add a useless/needed POSTING_READ (Ben)
- Early return in case LCPLL is already restored (Ben)
- Add ndelay(100) (Ben)
v4: - Merge the commit that added assert_can_disable_lcpll (Ben)
- Add interrupt assertions (Ben)
Reviewed-by Ben Widawsky <[email protected]>
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_reg.h      |  15 ++++
 drivers/gpu/drm/i915/intel_display.c | 136 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 3 files changed, 154 insertions(+)
drivers/gpu/drm/i915/i915_reg.h +15 -0
index 0dfcbad..6caa748 100644
@@ -2261,6 +2261,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
@@ -2269,6 +2271,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 */
@@ -5009,7 +5017,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)
+
+#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 +0 -0
index deee650d..0b0696a 100644
@@ -5922,6 +5922,142 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	return true;
 }
 

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

Quoted 10 lines

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

+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;
+	unsigned long irqflags;
+	uint32_t val, pch_hpd_mask;
+
+	pch_hpd_mask = SDE_PORTB_HOTPLUG_CPT | SDE_PORTC_HOTPLUG_CPT;
+	if (!HAS_LP_PCH(dev_priv))

This didn't compile, I've fixed it up.
-Daniel

Quoted 149 lines

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

+		pch_hpd_mask |= SDE_PORTD_HOTPLUG_CPT | SDE_CRT_HOTPLUG_CPT;
+
+	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");
+	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");
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+	val = I915_READ(DEIMR);
+	WARN((val & ~DE_PCH_EVENT_IVB) != val,
+	     "Unexpected DEIMR bits enabled: 0x%x\n", val);
+	val = I915_READ(SDEIMR);
+	WARN((val & ~pch_hpd_mask) != val,
+	     "Unexpected SDEIMR bits enabled: 0x%x\n", val);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
+/*
+ * 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;
+
+	assert_can_disable_lcpll(dev_priv);
+
+	val = I915_READ(LCPLL_CTL);
+
+	if (switch_to_fclk) {
+		val |= LCPLL_CD_SOURCE_FCLK;
+		I915_WRITE(LCPLL_CTL, val);
+
+		if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
+				       LCPLL_CD_SOURCE_FCLK_DONE, 1))
+			DRM_ERROR("Switching to FCLK failed\n");
+
+		val = I915_READ(LCPLL_CTL);
+	}
+
+	val |= LCPLL_PLL_DISABLE;
+	I915_WRITE(LCPLL_CTL, val);
+	POSTING_READ(LCPLL_CTL);
+
+	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);
+	ndelay(100);
+
+	if (wait_for((I915_READ(D_COMP) & D_COMP_RCOMP_IN_PROGRESS) == 0, 1))
+		DRM_ERROR("D_COMP RCOMP still in progress\n");
+
+	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)
+{
+	uint32_t val;
+
+	val = I915_READ(LCPLL_CTL);
+
+	if ((val & (LCPLL_PLL_LOCK | LCPLL_PLL_DISABLE | LCPLL_CD_SOURCE_FCLK |
+		    LCPLL_POWER_DOWN_ALLOW)) == LCPLL_PLL_LOCK)
+		return;
+
+	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);
+	I915_READ(D_COMP);
+
+	val = I915_READ(LCPLL_CTL);
+	val &= ~LCPLL_PLL_DISABLE;
+	I915_WRITE(LCPLL_CTL, val);
+
+	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);
+
+		if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
+					LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
+			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 31087ff..3fbe80b 100644
@@ -838,5 +838,8 @@ extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
 extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_disable(struct intel_dp *intel_dp);
 extern void intel_edp_psr_update(struct drm_device *dev);
+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__ */

Signature · 6 lines

1.8.1.2

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

Signature · 3 lines

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

Paulo Zanoni

author

2013-07-23 14:19 UTC #4 lore raw

PATCH 4/4 drm/i915: add HAS_LP_PCH check

In reply to Paulo Zanoni

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 8b3167e..713e7bc 100644
@@ -1575,6 +1575,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 0b0696a..04a4550 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 008e0e0..545d4ac 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 · 1 line

1.8.1.2

Daniel Vetter

2013-07-23 14:45 UTC #5 lore raw

Re: [PATCH 4/4] drm/i915: add HAS_LP_PCH check

Quoted 11 lines · Paulo Zanoni

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]>

Hm, I'm not that much convinced that this cleans up things: If we'll have more LP versions in future generations then either the check will get ugly or we have a bit a confusion. Since the scope of this chekc is fairly restricted to a bit of reference clock handling I think we're ok.

One thing we could do is trim the giant suffix a bit, i.e. s/INTEL_PCH_LPT_LP_DEVICE_ID_TYPE/INTEL_PCH_LPT_LP/. Since we're always comparing that constant against a pciid or pch_id field it's still rather clear what's going on. But like I've said, since this has fairly minimal scope imo not really worth to make a fuss about.

So punted on this one here for now, but merged the other three patches from this series.

Thanks, Daniel

Quoted 82 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 8b3167e..713e7bc 100644
@@ -1575,6 +1575,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 0b0696a..04a4550 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 008e0e0..545d4ac 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

Signature · 3 lines

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