Re: [PATCH 4/6] drm/i915: Make the LPT iclkip 20MHz case more generic

[Imre Deak <imre.deak@intel.com>]

On ke, 2016-02-17 at 21:41 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The reason for spcial casing 20MHz in the iclkip calculations is that
> it would overflow the 7 bit divisor value. Let's rewrite the special
> case to check for just that, and bump up auxdiv when needed. This
> makes
> the code work for freqeuencies close to but not exactly 20MHz. The
> real
> lower limit for auxdiv=0 is actually:
> 172800000/(0x7f+2)*64)=~20930 kHz, and below that we must resort to
> auxdiv=1.
> 
> Actually this is all very theoretical since we limit the dotclock to
> min 25MHz with CRT on all platforms. 25Mhz is actually the documented
> limit in Bspec, so it seems we ought to never need to worry about the
> auxdiv=1 case. But no harm in having it.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yep, fewer special casing is better. Btw, the spec could have just as
well describe how to calculate aux_div instead of providing special
cases and tables, I wonder why this is done at places. The patch looks
good:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++---------
> ----------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a3c959cd8b3b..5e7b22a31098 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3957,37 +3957,35 @@ static void lpt_disable_iclkip(struct
> drm_i915_private *dev_priv)
>  /* Program iCLKIP clock to the desired frequency */
>  static void lpt_program_iclkip(struct drm_crtc *crtc)
>  {
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	int clock = to_intel_crtc(crtc)->config-
> >base.adjusted_mode.crtc_clock;
>  	u32 divsel, phaseinc, auxdiv, phasedir = 0;
>  	u32 temp;
>  
>  	lpt_disable_iclkip(dev_priv);
>  
> -	/* 20MHz is a corner case which is out of range for the 7-
> bit divisor */
> -	if (clock == 20000) {
> -		auxdiv = 1;
> -		divsel = 0x41;
> -		phaseinc = 0x20;
> -	} else {
> -		/* The iCLK virtual clock root frequency is in MHz,
> -		 * but the adjusted_mode->crtc_clock in in KHz. To
> get the
> -		 * divisors, it is necessary to divide one by
> another, so we
> -		 * convert the virtual clock precision to KHz here
> for higher
> -		 * precision.
> -		 */
> +	/* The iCLK virtual clock root frequency is in MHz,
> +	 * but the adjusted_mode->crtc_clock in in KHz. To get the
> +	 * divisors, it is necessary to divide one by another, so we
> +	 * convert the virtual clock precision to KHz here for
> higher
> +	 * precision.
> +	 */
> +	for (auxdiv = 0; auxdiv < 2; auxdiv++) {
>  		u32 iclk_virtual_root_freq = 172800 * 1000;
>  		u32 iclk_pi_range = 64;
> -		u32 desired_divisor, msb_divisor_value, pi_value;
> +		u32 desired_divisor;
>  
> -		desired_divisor =
> DIV_ROUND_CLOSEST(iclk_virtual_root_freq, clock);
> -		msb_divisor_value = desired_divisor / iclk_pi_range;
> -		pi_value = desired_divisor % iclk_pi_range;
> +		desired_divisor =
> DIV_ROUND_CLOSEST(iclk_virtual_root_freq,
> +						    clock <<
> auxdiv);
> +		divsel = (desired_divisor / iclk_pi_range) - 2;
> +		phaseinc = desired_divisor % iclk_pi_range;
>  
> -		auxdiv = 0;
> -		divsel = msb_divisor_value - 2;
> -		phaseinc = pi_value;
> +		/*
> +		 * Near 20MHz is a corner case which is
> +		 * out of range for the 7-bit divisor
> +		 */
> +		if (divsel <= 0x7f)
> +			break;
>  	}
>  
>  	/* This should not happen with any sane values */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx