* [PATCH 1/7] drm/i915: remove SDV support from lpt_pch_init_refclk
2013-07-12 17:19 [PATCH 0/7] HSW/LPT clocking code additional sequences Paulo Zanoni
@ 2013-07-12 17:19 ` Paulo Zanoni
2013-07-13 5:11 ` Ben Widawsky
2013-07-12 17:19 ` [PATCH 2/7] drm/i915: extract FDI mPHY functions from lpt_init_pch_refclk Paulo Zanoni
` (6 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-12 17:19 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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 <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 104 ++++++++++++-----------------------
1 file changed, 34 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c79addd..5821ffc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 1/7] drm/i915: remove SDV support from lpt_pch_init_refclk
2013-07-12 17:19 ` [PATCH 1/7] drm/i915: remove SDV support from lpt_pch_init_refclk Paulo Zanoni
@ 2013-07-13 5:11 ` Ben Widawsky
0 siblings, 0 replies; 30+ messages in thread
From: Ben Widawsky @ 2013-07-13 5:11 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Jul 12, 2013 at 02:19:36PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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 <paulo.r.zanoni@intel.com>
Was hoping to get through the first 3 before I got too tired, but I
didn't make it. This one is:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Will try to finish up some more this weekend.
> ---
> drivers/gpu/drm/i915/intel_display.c | 104 ++++++++++++-----------------------
> 1 file changed, 34 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c79addd..5821ffc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/7] drm/i915: extract FDI mPHY functions from lpt_init_pch_refclk
2013-07-12 17:19 [PATCH 0/7] HSW/LPT clocking code additional sequences Paulo Zanoni
2013-07-12 17:19 ` [PATCH 1/7] drm/i915: remove SDV support from lpt_pch_init_refclk Paulo Zanoni
@ 2013-07-12 17:19 ` Paulo Zanoni
2013-07-18 21:51 ` Paulo Zanoni
2013-07-12 17:19 ` [PATCH 3/7] drm/i915: extract lpt_enable_clkout_dp " Paulo Zanoni
` (5 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-12 17:19 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Because lpt_init_pch_refclk implements the "Sequence to enable
CLKOUT_DP for FDI usage and configure PCH FDI I/O", which is very
similar to "Sequence to enable CLKOUT_DP" and "Sequence to enable
CLKOUT_DP without spread". With the extracted functions we can more
easily implement the two missing sequences.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 75 +++++++++++++++++++++---------------
1 file changed, 44 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5821ffc..753317d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5163,38 +5163,9 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
BUG_ON(val != final);
}
-/* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */
-static void lpt_init_pch_refclk(struct drm_device *dev)
+static void lpt_reset_fdi_mphy(struct drm_i915_private *dev_priv)
{
- 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;
- 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;
-
- mutex_lock(&dev_priv->dpio_lock);
-
- 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);
+ uint32_t tmp;
tmp = I915_READ(SOUTH_CHICKEN2);
tmp |= FDI_MPHY_IOSFSB_RESET_CTL;
@@ -5211,6 +5182,11 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
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");
+}
+
+static void lpt_program_fdi_mphy(struct drm_i915_private *dev_priv)
+{
+ uint32_t tmp;
tmp = intel_sbi_read(dev_priv, 0x8008, SBI_MPHY);
tmp &= ~(0xFF << 24);
@@ -5280,6 +5256,43 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
tmp &= ~(0xF << 28);
tmp |= (4 << 28);
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_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;
+ 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;
+
+ mutex_lock(&dev_priv->dpio_lock);
+
+ 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);
+
+ 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);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 2/7] drm/i915: extract FDI mPHY functions from lpt_init_pch_refclk
2013-07-12 17:19 ` [PATCH 2/7] drm/i915: extract FDI mPHY functions from lpt_init_pch_refclk Paulo Zanoni
@ 2013-07-18 21:51 ` Paulo Zanoni
0 siblings, 0 replies; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-18 21:51 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Because lpt_init_pch_refclk implements the "Sequence to enable
CLKOUT_DP for FDI usage and configure PCH FDI I/O", which is very
similar to "Sequence to enable CLKOUT_DP" and "Sequence to enable
CLKOUT_DP without spread". With the extracted functions we can more
easily implement the two missing sequences.
v2: Rebase (WaMPhyProgramming:hsw comment).
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 79 ++++++++++++++++++++----------------
1 file changed, 45 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2dab208..bc586ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5164,41 +5164,9 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
BUG_ON(val != final);
}
-/*
- * Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O.
- * WaMPhyProgramming:hsw
- */
-static void lpt_init_pch_refclk(struct drm_device *dev)
+static void lpt_reset_fdi_mphy(struct drm_i915_private *dev_priv)
{
- 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;
- 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;
-
- mutex_lock(&dev_priv->dpio_lock);
-
- 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);
+ uint32_t tmp;
tmp = I915_READ(SOUTH_CHICKEN2);
tmp |= FDI_MPHY_IOSFSB_RESET_CTL;
@@ -5215,6 +5183,12 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
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");
+}
+
+/* WaMPhyProgramming:hsw */
+static void lpt_program_fdi_mphy(struct drm_i915_private *dev_priv)
+{
+ uint32_t tmp;
tmp = intel_sbi_read(dev_priv, 0x8008, SBI_MPHY);
tmp &= ~(0xFF << 24);
@@ -5284,6 +5258,43 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
tmp &= ~(0xF << 28);
tmp |= (4 << 28);
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_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;
+ 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;
+
+ mutex_lock(&dev_priv->dpio_lock);
+
+ 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);
+
+ 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);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/7] drm/i915: extract lpt_enable_clkout_dp from lpt_init_pch_refclk
2013-07-12 17:19 [PATCH 0/7] HSW/LPT clocking code additional sequences Paulo Zanoni
2013-07-12 17:19 ` [PATCH 1/7] drm/i915: remove SDV support from lpt_pch_init_refclk Paulo Zanoni
2013-07-12 17:19 ` [PATCH 2/7] drm/i915: extract FDI mPHY functions from lpt_init_pch_refclk Paulo Zanoni
@ 2013-07-12 17:19 ` Paulo Zanoni
2013-07-19 6:54 ` Daniel Vetter
2013-07-12 17:19 ` [PATCH 4/7] drm/i915: extend lpt_enable_clkout_dp Paulo Zanoni
` (4 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-12 17:19 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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 <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 38 +++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 753317d..f4c5263 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5259,24 +5259,10 @@ static void lpt_program_fdi_mphy(struct drm_i915_private *dev_priv)
}
/* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */
-static void lpt_init_pch_refclk(struct drm_device *dev)
+static void lpt_enable_clkout_dp(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;
- 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;
+ uint32_t tmp;
mutex_lock(&dev_priv->dpio_lock);
@@ -5302,6 +5288,26 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
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;
+ struct intel_encoder *encoder;
+ bool has_vga = false;
+
+ 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;
+
+ lpt_enable_clkout_dp(dev);
+}
+
/*
* Initialize reference clocks when the driver loads
*/
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 3/7] drm/i915: extract lpt_enable_clkout_dp from lpt_init_pch_refclk
2013-07-12 17:19 ` [PATCH 3/7] drm/i915: extract lpt_enable_clkout_dp " Paulo Zanoni
@ 2013-07-19 6:54 ` Daniel Vetter
0 siblings, 0 replies; 30+ messages in thread
From: Daniel Vetter @ 2013-07-19 6:54 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Jul 12, 2013 at 02:19:38PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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 <paulo.r.zanoni@intel.com>
I've merged the first three patches in this series to dinq, thanks for
patches and review.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 4/7] drm/i915: extend lpt_enable_clkout_dp
2013-07-12 17:19 [PATCH 0/7] HSW/LPT clocking code additional sequences Paulo Zanoni
` (2 preceding siblings ...)
2013-07-12 17:19 ` [PATCH 3/7] drm/i915: extract lpt_enable_clkout_dp " Paulo Zanoni
@ 2013-07-12 17:19 ` Paulo Zanoni
2013-07-18 22:40 ` Ben Widawsky
2013-07-12 17:19 ` [PATCH 5/7] drm/i915: disable CLKOUT_DP when it's not needed Paulo Zanoni
` (3 subsequent siblings)
7 siblings, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-12 17:19 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Now it implements 3 different sequences from BSpec and also has
support for ULT.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 2 ++
drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++-----------
2 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index dc3d6a7..be6164f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -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)
/* 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 f4c5263..5f3b636 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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;
+ WARN(with_fdi && !with_spread, "FDI requires downspread\n");
+
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);
}
/*
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 4/7] drm/i915: extend lpt_enable_clkout_dp
2013-07-12 17:19 ` [PATCH 4/7] drm/i915: extend lpt_enable_clkout_dp Paulo Zanoni
@ 2013-07-18 22:40 ` Ben Widawsky
2013-07-19 15:04 ` Paulo Zanoni
0 siblings, 1 reply; 30+ messages in thread
From: Ben Widawsky @ 2013-07-18 22:40 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Jul 12, 2013 at 02:19:39PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Now it implements 3 different sequences from BSpec and also has
> support for ULT.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++-----------
> 2 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index dc3d6a7..be6164f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -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
>
> /* 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 f4c5263..5f3b636 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
>
> + 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?
> 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);
> }
>
> /*
> --
> 1.8.1.2
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/7] drm/i915: extend lpt_enable_clkout_dp
2013-07-18 22:40 ` Ben Widawsky
@ 2013-07-19 15:04 ` Paulo Zanoni
2013-07-19 21:53 ` [PATCH 1/5] " Paulo Zanoni
0 siblings, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-19 15:04 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, Paulo Zanoni
2013/7/18 Ben Widawsky <ben@bwidawsk.net>:
> On Fri, Jul 12, 2013 at 02:19:39PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Now it implements 3 different sequences from BSpec and also has
>> support for ULT.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 2 ++
>> drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++-----------
>> 2 files changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index dc3d6a7..be6164f 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -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
Our docs have changed considerably since I originally wrote this
patch. They have joined both registers and a single definition makes
sense now. I also just noticed the docs say 0 is Enable and 1 is
Disable, even though the documentation for the Sequences suggests that
1 enables stuff and 0 disables stuff :(
>
>>
>> /* 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 f4c5263..5f3b636 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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;
>>
>> + WARN(with_fdi && !with_spread, "FDI requires downspread\n");
>> +
>
> WARN_ON(with_fdi && IS_ULT(dev))?
Yeah, but I noticed I'm using IS_ULT checks where I should be checking
for LPT-LP.
> Should we proceed after the WARN, or break out early?
Either way we'll end up with possibly broken PCH clocks.... I'll try
to adjust the arguments to something sane and print the warn.
>
>> 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);
>> }
>>
>> /*
>> --
>> 1.8.1.2
> --
> Ben Widawsky, Intel Open Source Technology Center
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 1/5] drm/i915: extend lpt_enable_clkout_dp
2013-07-19 15:04 ` Paulo Zanoni
@ 2013-07-19 21:53 ` Paulo Zanoni
0 siblings, 0 replies; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-19 21:53 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 3 ++-
drivers/gpu/drm/i915/intel_display.c | 43 +++++++++++++++++++++++++-----------
2 files changed, 32 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e20f093..0dfcbad 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -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
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c9e9ad0..3f31ca0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5260,11 +5260,23 @@ 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;
+ uint32_t reg, tmp;
+
+ 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"))
+ with_fdi = false;
mutex_lock(&dev_priv->dpio_lock);
@@ -5275,17 +5287,22 @@ 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);
+ 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);
mutex_unlock(&dev_priv->dpio_lock);
}
@@ -5307,7 +5324,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);
}
/*
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 5/7] drm/i915: disable CLKOUT_DP when it's not needed
2013-07-12 17:19 [PATCH 0/7] HSW/LPT clocking code additional sequences Paulo Zanoni
` (3 preceding siblings ...)
2013-07-12 17:19 ` [PATCH 4/7] drm/i915: extend lpt_enable_clkout_dp Paulo Zanoni
@ 2013-07-12 17:19 ` Paulo Zanoni
2013-07-12 18:23 ` Daniel Vetter
2013-07-18 22:54 ` Ben Widawsky
2013-07-12 17:19 ` [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL Paulo Zanoni
` (2 subsequent siblings)
7 siblings, 2 replies; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-12 17:19 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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 <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++++++++++++++----
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5f3b636..059c9a8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 5/7] drm/i915: disable CLKOUT_DP when it's not needed
2013-07-12 17:19 ` [PATCH 5/7] drm/i915: disable CLKOUT_DP when it's not needed Paulo Zanoni
@ 2013-07-12 18:23 ` Daniel Vetter
2013-07-12 18:24 ` Paulo Zanoni
2013-07-18 22:54 ` Ben Widawsky
1 sibling, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2013-07-12 18:23 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Jul 12, 2013 at 02:19:40PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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 <paulo.r.zanoni@intel.com>
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
> ---
> drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++++++++++++++----
> 1 file changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5f3b636..059c9a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 5/7] drm/i915: disable CLKOUT_DP when it's not needed
2013-07-12 18:23 ` Daniel Vetter
@ 2013-07-12 18:24 ` Paulo Zanoni
0 siblings, 0 replies; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-12 18:24 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Paulo Zanoni
2013/7/12 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Jul 12, 2013 at 02:19:40PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> 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 <paulo.r.zanoni@intel.com>
>
> 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.
Yeah, that's a good idea. And for that, we'll need this series merged :)
> -Daniel
>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++++++++++++++----
>> 1 file changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 5f3b636..059c9a8 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/7] drm/i915: disable CLKOUT_DP when it's not needed
2013-07-12 17:19 ` [PATCH 5/7] drm/i915: disable CLKOUT_DP when it's not needed Paulo Zanoni
2013-07-12 18:23 ` Daniel Vetter
@ 2013-07-18 22:54 ` Ben Widawsky
2013-07-19 21:54 ` [PATCH 2/5] " Paulo Zanoni
1 sibling, 1 reply; 30+ messages in thread
From: Ben Widawsky @ 2013-07-18 22:54 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Jul 12, 2013 at 02:19:40PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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 <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++++++++++++++++----
> 1 file changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5f3b636..059c9a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
> + }
Apologies if this is stupid, I'm new to this part of the doc, shouldn't
it be:
if (tmp & ~SBI_SSCCTL_DISABLE == 0)
> + 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);
> }
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 2/5] drm/i915: disable CLKOUT_DP when it's not needed
2013-07-18 22:54 ` Ben Widawsky
@ 2013-07-19 21:54 ` Paulo Zanoni
0 siblings, 0 replies; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-19 21:54 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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).
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++++++++++++++++++++++++----
1 file changed, 32 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3f31ca0..1b41178 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
}
/*
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL
2013-07-12 17:19 [PATCH 0/7] HSW/LPT clocking code additional sequences Paulo Zanoni
` (4 preceding siblings ...)
2013-07-12 17:19 ` [PATCH 5/7] drm/i915: disable CLKOUT_DP when it's not needed Paulo Zanoni
@ 2013-07-12 17:19 ` Paulo Zanoni
2013-07-18 21:53 ` Paulo Zanoni
2013-07-18 23:26 ` Ben Widawsky
2013-07-12 17:19 ` [PATCH 7/7] drm/i915: add some assertions to hsw_disable_lcpll Paulo Zanoni
2013-07-18 23:33 ` [PATCH 0/7] HSW/LPT clocking code additional sequences Ben Widawsky
7 siblings, 2 replies; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-12 17:19 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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 <paulo.r.zanoni@intel.com>
---
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(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index be6164f..8e5a5ec 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -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)
+
+#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
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 059c9a8..ffb08bf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
+
+ udelay(1);
+
+ val = I915_READ(LCPLL_CTL);
+ if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
+ DRM_ERROR("Switching to FCLK failed\n");
+ }
+
+ 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);
+
+ udelay(2);
+
+ val = I915_READ(D_COMP);
+ if (val & D_COMP_RCOMP_IN_PROGRESS)
+ 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_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);
+
+ 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, 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;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5dfc1a0..15989d1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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__ */
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL
2013-07-12 17:19 ` [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL Paulo Zanoni
@ 2013-07-18 21:53 ` Paulo Zanoni
2013-07-18 23:26 ` Ben Widawsky
1 sibling, 0 replies; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-18 21:53 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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.
v2: - Rebase.
- Fix D_COMP wait timeout.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 7 +++
drivers/gpu/drm/i915/intel_display.c | 92 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_drv.h | 3 ++
3 files changed, 102 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bfa6f7f..b57d564 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5007,7 +5007,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
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0741e1f..915da2d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5924,6 +5924,98 @@ 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);
+
+ udelay(1);
+
+ val = I915_READ(LCPLL_CTL);
+ if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
+ DRM_ERROR("Switching to FCLK failed\n");
+ }
+
+ 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);
+
+ 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_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);
+
+ 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, 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;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 31087ff..3fbe80b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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__ */
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL
2013-07-12 17:19 ` [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL Paulo Zanoni
2013-07-18 21:53 ` Paulo Zanoni
@ 2013-07-18 23:26 ` Ben Widawsky
2013-07-18 23:33 ` Ben Widawsky
2013-07-19 18:22 ` Paulo Zanoni
1 sibling, 2 replies; 30+ messages in thread
From: Ben Widawsky @ 2013-07-18 23:26 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Jul 12, 2013 at 02:19:41PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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?
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> 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(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index be6164f..8e5a5ec 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -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
> +
> +#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
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 059c9a8..ffb08bf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
> +
> + 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)?
> + }
> +
> + 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);
> +
> + udelay(2);
ndelay(100)?
> +
> + val = I915_READ(D_COMP);
> + if (val & D_COMP_RCOMP_IN_PROGRESS)
> + DRM_ERROR("D_COMP RCOMP still in progress\n");
wait_for(..., 1)?
> +
> + 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);
> +
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.
> + 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.
> + val = I915_READ(LCPLL_CTL);
> + val &= ~LCPLL_PLL_DISABLE;
> + I915_WRITE(LCPLL_CTL, val);
> + POSTING_READ(LCPLL_CTL);
^ unnecessary POSTING_READ - but meh
> +
> + 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;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5dfc1a0..15989d1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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?
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL
2013-07-18 23:26 ` Ben Widawsky
@ 2013-07-18 23:33 ` Ben Widawsky
2013-07-19 16:57 ` Paulo Zanoni
2013-07-19 18:22 ` Paulo Zanoni
1 sibling, 1 reply; 30+ messages in thread
From: Ben Widawsky @ 2013-07-18 23:33 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Thu, Jul 18, 2013 at 04:26:42PM -0700, Ben Widawsky wrote:
> On Fri, Jul 12, 2013 at 02:19:41PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > 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?
>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> > 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(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index be6164f..8e5a5ec 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -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.
>
> > +
> > +#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
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 059c9a8..ffb08bf 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -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);
> > +
> > + 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)?
>
> > + }
> > +
> > + 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);
> > +
> > + udelay(2);
>
> ndelay(100)?
>
> > +
> > + val = I915_READ(D_COMP);
> > + if (val & D_COMP_RCOMP_IN_PROGRESS)
> > + DRM_ERROR("D_COMP RCOMP still in progress\n");
>
> wait_for(..., 1)?
>
> > +
> > + 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);
> > +
>
> 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.
>
> > + 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.
>
>
> > + val = I915_READ(LCPLL_CTL);
> > + val &= ~LCPLL_PLL_DISABLE;
> > + I915_WRITE(LCPLL_CTL, val);
> > + POSTING_READ(LCPLL_CTL);
> ^ unnecessary POSTING_READ - but meh
> > +
> > + 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;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 5dfc1a0..15989d1 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -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?
>
> --
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL
2013-07-18 23:33 ` Ben Widawsky
@ 2013-07-19 16:57 ` Paulo Zanoni
0 siblings, 0 replies; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-19 16:57 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, Paulo Zanoni
2013/7/18 Ben Widawsky <ben@bwidawsk.net>:
> On Thu, Jul 18, 2013 at 04:26:42PM -0700, Ben Widawsky wrote:
>> On Fri, Jul 12, 2013 at 02:19:41PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > 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?
>>
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > ---
>> > 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(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index be6164f..8e5a5ec 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -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.
I looked at the LCPLL_CTL register definition. Bit 22 is called
"Display Power Down Allow" and value 1 means "Allow". Bit 19 is called
"CD Source Fclk" and value 1 means "Done".
>
>>
>> > +
>> > +#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
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 059c9a8..ffb08bf 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -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);
>> > +
>> > + 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)?
>>
>> > + }
>> > +
>> > + 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);
>> > +
>> > + udelay(2);
>>
>> ndelay(100)?
>>
>> > +
>> > + val = I915_READ(D_COMP);
>> > + if (val & D_COMP_RCOMP_IN_PROGRESS)
>> > + DRM_ERROR("D_COMP RCOMP still in progress\n");
>>
>> wait_for(..., 1)?
>>
>> > +
>> > + 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);
>> > +
>>
>> 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.
>>
>> > + 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.
>>
>>
>> > + val = I915_READ(LCPLL_CTL);
>> > + val &= ~LCPLL_PLL_DISABLE;
>> > + I915_WRITE(LCPLL_CTL, val);
>> > + POSTING_READ(LCPLL_CTL);
>> ^ unnecessary POSTING_READ - but meh
>> > +
>> > + 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;
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 5dfc1a0..15989d1 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -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?
>>
>> --
>> Ben Widawsky, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL
2013-07-18 23:26 ` Ben Widawsky
2013-07-18 23:33 ` Ben Widawsky
@ 2013-07-19 18:22 ` Paulo Zanoni
2013-07-19 21:56 ` [PATCH 3/5] " Paulo Zanoni
1 sibling, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-19 18:22 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, Paulo Zanoni
2013/7/18 Ben Widawsky <ben@bwidawsk.net>:
> On Fri, Jul 12, 2013 at 02:19:41PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> 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?
I'll merge this patch with the next one when sending, so we'll have
our assertions.
>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>> 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(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index be6164f..8e5a5ec 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -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
>
>> +
>> +#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
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 059c9a8..ffb08bf 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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);
>> +
>> + 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)?
Done.
>
>> + }
>> +
>> + 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);
>> +
>> + udelay(2);
>
> ndelay(100)?
>
>> +
>> + val = I915_READ(D_COMP);
>> + if (val & D_COMP_RCOMP_IN_PROGRESS)
>> + DRM_ERROR("D_COMP RCOMP still in progress\n");
>
> wait_for(..., 1)?
You reviewed v1. In v2 this wait_for was already there. But I re-added
the ndelay(100).
>
>> +
>> + 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);
>> +
>
> 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.
Makes sense. Done.
>
>> + 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.
Done.
>
>
>> + val = I915_READ(LCPLL_CTL);
>> + val &= ~LCPLL_PLL_DISABLE;
>> + I915_WRITE(LCPLL_CTL, val);
>> + POSTING_READ(LCPLL_CTL);
> ^ unnecessary POSTING_READ - but meh
Yeah, we have the wait_for which does the job. Done.
>> +
>> + 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;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 5dfc1a0..15989d1 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -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?
For Package C8 we want to switch to fclk and allow power down, but
BSpec chapter "Display Sequences for LCPLL disabling" defines a
sequence that doesn't switch to fclk nor allow power down. On that
case, the graphics card will be completely disabled. I don't really
know if we'll need it, but at least we have the code for it :)
>
> --
> Ben Widawsky, Intel Open Source Technology Center
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 3/5] drm/i915: add functions to disable and restore LCPLL
2013-07-19 18:22 ` Paulo Zanoni
@ 2013-07-19 21:56 ` Paulo Zanoni
2013-07-19 21:58 ` [PATCH 5/5] drm/i915: add HAS_LP_PCH check Paulo Zanoni
0 siblings, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-19 21:56 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
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(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0dfcbad..6caa748 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -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
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1b41178..c35a613 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5922,6 +5922,142 @@ 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;
+ 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))
+ 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;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 31087ff..3fbe80b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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__ */
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 5/5] drm/i915: add HAS_LP_PCH check
2013-07-19 21:56 ` [PATCH 3/5] " Paulo Zanoni
@ 2013-07-19 21:58 ` Paulo Zanoni
2013-07-22 17:10 ` Ben Widawsky
0 siblings, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-19 21:58 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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 <paulo.r.zanoni@intel.com>
---
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(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2885265..262c3d4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c35a613..1d3c805 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 43031ec..7643b16 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 5/5] drm/i915: add HAS_LP_PCH check
2013-07-19 21:58 ` [PATCH 5/5] drm/i915: add HAS_LP_PCH check Paulo Zanoni
@ 2013-07-22 17:10 ` Ben Widawsky
2013-07-22 22:44 ` Paulo Zanoni
0 siblings, 1 reply; 30+ messages in thread
From: Ben Widawsky @ 2013-07-22 17:10 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Jul 19, 2013 at 06:58:57PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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 <paulo.r.zanoni@intel.com>
1-4 is:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
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 <ben@bwidawsk.net>
> ---
> 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(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2885265..262c3d4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -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)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c35a613..1d3c805 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 43031ec..7643b16 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 5/5] drm/i915: add HAS_LP_PCH check
2013-07-22 17:10 ` Ben Widawsky
@ 2013-07-22 22:44 ` Paulo Zanoni
0 siblings, 0 replies; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-22 22:44 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, Paulo Zanoni
2013/7/22 Ben Widawsky <ben@bwidawsk.net>:
> On Fri, Jul 19, 2013 at 06:58:57PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> 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 <paulo.r.zanoni@intel.com>
>
> 1-4 is:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>
> 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...
I was thinking in the future where other PCHs may also have this
LP/non-LP difference, so we'd use the HAS_LP_PCH macro for them. If we
want the macro to be PCH-specific we should probably even drop the
macro and keep using dev_priv->pch_id (drop this patch).
>
> With that this too is:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 2885265..262c3d4 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -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)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index c35a613..1d3c805 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 43031ec..7643b16 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -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
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 7/7] drm/i915: add some assertions to hsw_disable_lcpll
2013-07-12 17:19 [PATCH 0/7] HSW/LPT clocking code additional sequences Paulo Zanoni
` (5 preceding siblings ...)
2013-07-12 17:19 ` [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL Paulo Zanoni
@ 2013-07-12 17:19 ` Paulo Zanoni
2013-07-18 23:32 ` Ben Widawsky
2013-07-18 23:33 ` [PATCH 0/7] HSW/LPT clocking code additional sequences Ben Widawsky
7 siblings, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-12 17:19 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
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 <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++++++
2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8e5a5ec..9556dff 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -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 */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ffb08bf..9055f60 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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");
+ 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");
+}
+
/*
* 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;
+ assert_can_disable_lcpll(dev_priv);
+
val = I915_READ(LCPLL_CTL);
if (switch_to_fclk) {
--
1.8.1.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 7/7] drm/i915: add some assertions to hsw_disable_lcpll
2013-07-12 17:19 ` [PATCH 7/7] drm/i915: add some assertions to hsw_disable_lcpll Paulo Zanoni
@ 2013-07-18 23:32 ` Ben Widawsky
2013-07-19 18:42 ` Paulo Zanoni
0 siblings, 1 reply; 30+ messages in thread
From: Ben Widawsky @ 2013-07-18 23:32 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Jul 12, 2013 at 02:19:42PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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 <paulo.r.zanoni@intel.com>
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).
> ---
> drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
> drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8e5a5ec..9556dff 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -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 */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ffb08bf..9055f60 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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");
> + 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.
> +}
> +
> /*
> * 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;
>
> + 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.
> val = I915_READ(LCPLL_CTL);
>
> if (switch_to_fclk) {
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 7/7] drm/i915: add some assertions to hsw_disable_lcpll
2013-07-18 23:32 ` Ben Widawsky
@ 2013-07-19 18:42 ` Paulo Zanoni
0 siblings, 0 replies; 30+ messages in thread
From: Paulo Zanoni @ 2013-07-19 18:42 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, Paulo Zanoni
2013/7/18 Ben Widawsky <ben@bwidawsk.net>:
> On Fri, Jul 12, 2013 at 02:19:42PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> 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 <paulo.r.zanoni@intel.com>
>
> 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).
Originally this code was part of other function. I agree that the way
things look now, it should be on the same patch.
>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++
>> drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++++++
>> 2 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 8e5a5ec..9556dff 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -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 */
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ffb08bf..9055f60 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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");
>> + 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
The audio registers go away when we disable the power well, which is
part of the assertion.
> eDP HPD
> Other CPU/PCH interrupts
I guess these ones deserve to be part of the assertion, but checking
interrupts is dangerous and racy...I'll try to find a good solution.
>
> You've worked with this code longer than I have, so maybe you can
> explain why you've skipped them.
>
>> +}
>> +
>> /*
>> * 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;
>>
>> + 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.
This function should should be the last thing on a big sequence that
involves disabling all the outputs, rearranging the interrupts, etc.
If we detect a problem here, we can't just return, the caller would
have to unwind everything it did. Anyway, this is really something we
don't expect and should never happen. I was kinda assuming the callers
would have to make sure they do everything before they call
hsw_diable_lcpll, and then these assertions are just double-checking.
Suggestions welcome :)
>
>> val = I915_READ(LCPLL_CTL);
>>
>> if (switch_to_fclk) {
>> --
>> 1.8.1.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/7] HSW/LPT clocking code additional sequences
2013-07-12 17:19 [PATCH 0/7] HSW/LPT clocking code additional sequences Paulo Zanoni
` (6 preceding siblings ...)
2013-07-12 17:19 ` [PATCH 7/7] drm/i915: add some assertions to hsw_disable_lcpll Paulo Zanoni
@ 2013-07-18 23:33 ` Ben Widawsky
7 siblings, 0 replies; 30+ messages in thread
From: Ben Widawsky @ 2013-07-18 23:33 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Fri, Jul 12, 2013 at 02:19:35PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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(-)
>
With the comments addressed in patches 1-5, they are:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
I'll wait for feedback before slapping tags on 6 & 7.
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 30+ messages in thread