Re: [PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, May 24, 2016 at 12:27:50PM +0300, Imre Deak wrote:
> If the CDCLK PLL isn't locked we can just assume that it's off resulting
> in fully re-initializing both CDCLK PLL and CDCLK dividers. This way the
> CDCLK PLL sanitization added in the following patch can be done on BXT
> the same way as it's done on SKL.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c1e666b..b8e5995 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5461,14 +5461,14 @@ skl_dpll0_update(struct drm_i915_private *dev_priv)
>  	u32 val;
>  
>  	dev_priv->cdclk_pll.ref = 24000;
> +	dev_priv->cdclk_pll.vco = 0;
>  
>  	val = I915_READ(LCPLL1_CTL);
> -	if ((val & LCPLL_PLL_ENABLE) == 0) {
> -		dev_priv->cdclk_pll.vco = 0;
> +	if ((val & LCPLL_PLL_ENABLE) == 0)
>  		return;
> -	}
>  
> -	WARN_ON((val & LCPLL_PLL_LOCK) == 0);
> +	if (WARN_ON((val & LCPLL_PLL_LOCK) == 0))
> +		return;
>  
>  	val = I915_READ(DPLL_CTRL1);
>  
> @@ -5690,9 +5690,10 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  	if ((I915_READ(SWF_ILK(0x18)) & 0x00FFFFFF) == 0)
>  		goto sanitize;
>  
> +	intel_update_cdclk(dev_priv->dev);
>  	/* Is PLL enabled and locked ? */
> -	if ((I915_READ(LCPLL1_CTL) & (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK)) !=
> -	    (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK))
> +	if (!dev_priv->cdclk_pll.vco ||

== 0 please. I find that more pleasing to the eye when we end up mixing
with == anyway on the next line.

Actually is there any extra benefit from the cdclk_freq check? As
in would vco==0 be sufficient on its own?

> +	    dev_priv->cdclk_freq == dev_priv->cdclk_pll.ref)
>  		goto sanitize;
>  
>  	if ((I915_READ(DPLL_CTRL1) & (DPLL_CTRL1_HDMI_MODE(SKL_DPLL0) |

Maybe toss out this DPLL_CTRL1 check that I added as well then, and have
skl_dpll0_update() set the vco to 0 when it's crap. If we ever actually
hit this in the real world, we'll get the warn, and then we perhaps get
to rethink this stuff, but for now simpler seems better.

> @@ -5701,8 +5702,6 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  	    DPLL_CTRL1_OVERRIDE(SKL_DPLL0))
>  		goto sanitize;
>  
> -	intel_update_cdclk(dev_priv->dev);
> -
>  	/* DPLL okay; verify the cdclock
>  	 *
>  	 * Noticed in some instances that the freq selection is correct but
> @@ -6608,14 +6607,14 @@ static void bxt_de_pll_update(struct drm_i915_private *dev_priv)
>  	u32 val;
>  
>  	dev_priv->cdclk_pll.ref = 19200;
> +	dev_priv->cdclk_pll.vco = 0;
>  
>  	val = I915_READ(BXT_DE_PLL_ENABLE);
> -	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) {
> -		dev_priv->cdclk_pll.vco = 0;
> +	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0)
>  		return;
> -	}
>  
> -	WARN_ON((val & BXT_DE_PLL_LOCK) == 0);
> +	if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0))
> +		return;
>  
>  	val = I915_READ(BXT_DE_PLL_CTL);
>  	dev_priv->cdclk_pll.vco = (val & BXT_DE_PLL_RATIO_MASK) *
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx