Re: [PATCH 05/17] drm/i915/icl: compute the MG PLL registers

[Manasi Navare <manasi.d.navare@intel.com>]

On Thu, Feb 22, 2018 at 12:55:07AM -0300, Paulo Zanoni wrote:
> This implements the "MG PLL Programming" sequence from our spec. The
> biggest problem was that the spec assumes real numbers, so we had to
> adjust some numbers and alculations due to the fact that the Kernel
> prefers to deal with integers.
> 
> I recommend grabbing some coffee, a pen and paper before reviewing
> this patch.
> 
> v2:
>  - Correctly identify DP encoders after upstream change.
>  - Small checkpatch issues.
>  - Rebase.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 217 +++++++++++++++++++++++++++++++++-
>  1 file changed, 216 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 5d7bacc80688..9a2965e0b883 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2514,11 +2514,226 @@ static enum intel_dpll_id icl_port_to_mg_pll_id(enum port port)
>  	return port - PORT_C + DPLL_ID_ICL_MGPLL1;
>  }
>  
> +static bool icl_mg_pll_find_divisors(int clock_khz, bool is_dp, bool use_ssc,
> +				     uint32_t *target_dco_khz,
> +				     struct intel_dpll_hw_state *state)
> +{
> +	uint32_t dco_min_freq, dco_max_freq;
> +	int div1_vals[] = {7, 5, 3, 2};
> +	unsigned int i;
> +	int div2;
> +
> +	dco_min_freq = is_dp ? 8100000 : use_ssc ? 8000000 : 7992000;
> +	dco_max_freq = is_dp ? 8100000 : 10000000;
> +
> +	for (i = 0; i < ARRAY_SIZE(div1_vals); i++) {
> +		int div1 = div1_vals[i];
> +
> +		for (div2 = 10; div2 > 0; div2--) {
> +			int dco = div1 * div2 * clock_khz * 5;
> +			int a_divratio, tlinedrv, inputsel, hsdiv;
> +
> +			if (dco < dco_min_freq || dco > dco_max_freq)
> +				continue;
> +
> +			if (div2 >= 2) {
> +				a_divratio = is_dp ? 10 : 5;
> +				tlinedrv = 2;
> +			} else {
> +				a_divratio = 5;
> +				tlinedrv = 0;
> +			}
> +			inputsel = is_dp ? 0 : 1;
> +
> +			switch (div1) {
> +			default:
> +				MISSING_CASE(div1);
> +			case 2:
> +				hsdiv = 0;
> +				break;
> +			case 3:
> +				hsdiv = 1;
> +				break;
> +			case 5:
> +				hsdiv = 2;
> +				break;
> +			case 7:
> +				hsdiv = 3;
> +				break;
> +			}
> +
> +			*target_dco_khz = dco;
> +
> +			state->mg_refclkin_ctl = MG_REFCLKIN_CTL_OD_2_MUX(1);
> +
> +			state->mg_clktop2_coreclkctl1 =
> +				MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO(a_divratio);
> +
> +			state->mg_clktop2_hsclkctl =
> +				MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL(tlinedrv) |
> +				MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL(inputsel) |
> +				MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO(hsdiv) |
> +				MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO(div2);
> +
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
> +/*
> + * The specification for this function uses real numbers, so the math had to be
> + * adapted to integer-only calculation, that's why it looks so different.
> + */
>  static bool icl_calc_mg_pll_state(struct intel_crtc_state *crtc_state,
>  				  struct intel_encoder *encoder, int clock,
>  				  struct intel_dpll_hw_state *pll_state)
>  {
> -	/* TODO */
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	int refclk_khz = dev_priv->cdclk.hw.ref;
> +	uint32_t dco_khz, m1div, m2div_int, m2div_rem, m2div_frac;
> +	uint32_t iref_ndiv, iref_trim, iref_pulse_w;
> +	uint32_t prop_coeff, int_coeff;
> +	uint32_t tdc_targetcnt, feedfwgain;
> +	uint64_t ssc_stepsize, ssc_steplen, ssc_steplog;
> +	uint64_t tmp;
> +	bool use_ssc = false;
> +	bool is_dp = !intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI);
> +
> +	if (!icl_mg_pll_find_divisors(clock, is_dp, use_ssc, &dco_khz,
> +				      pll_state)) {
> +		DRM_DEBUG_KMS("Failed to find divisors for clock %d\n", clock);
> +		return false;
> +	}
> +
> +	m1div = 2;
> +	m2div_int = dco_khz / (refclk_khz * m1div);
> +	if (m2div_int > 255) {
> +		m1div = 4;
> +		m2div_int = dco_khz / (refclk_khz * m1div);
> +		if (m2div_int > 255) {
> +			DRM_DEBUG_KMS("Failed to find mdiv for clock %d\n",
> +				      clock);
> +			return false;
> +		}
> +	}
> +	m2div_rem = dco_khz % (refclk_khz * m1div);
> +
> +	tmp = (uint64_t)m2div_rem * (1 << 22);
> +	do_div(tmp, refclk_khz * m1div);
> +	m2div_frac = tmp;
> +
> +	switch (refclk_khz) {
> +	case 19200:
> +		iref_ndiv = 1;
> +		iref_trim = 28;
> +		iref_pulse_w = 1;
> +		break;
> +	case 24000:
> +		iref_ndiv = 1;
> +		iref_trim = 25;
> +		iref_pulse_w = 2;
> +		break;
> +	case 38400:
> +		iref_ndiv = 2;
> +		iref_trim = 28;
> +		iref_pulse_w = 1;
> +		break;
> +	default:
> +		MISSING_CASE(refclk_khz);
> +		return false;
> +	}
> +
> +	/*
> +	 * tdc_res = 0.000003
> +	 * tdc_targetcnt = int(2 / (tdc_res * 8 * 50 * 1.1) / refclk_mhz + 0.5)
> +	 *
> +	 * The multiplication by 1000 is due to refclk MHz to KHz conversion.
> +	 * 0.000003 * 8 * 50 * 1.1 = 0.00132, also known as 132 / 100000.
> +	 * The 0.5 transformed to 5 results in a multiplication by 10 and the
> +	 * last division by 10.
> +	 */

Just a nit here in the comment above since that definitely got me confused with the
refclk in Mhz to Khz conversion. So in the equation below, we are converting the value in Khz to Mhz
so its a division by 1000 that becomes a multiplier by 1000 in the numerator. So I think the comment should
be updated to clarify this.

Apart from this, double checked all the calculations from the spec and look good. So

Reviewed-by; Manasi Navare <manasi.d.navare@intel.com>

Manasi

> +	tdc_targetcnt = (2 * 1000 * 100000 * 10 / (132 * refclk_khz) + 5) / 10;
> +
> +	/*
> +	 * Here we divide dco_khz by 10 in order to allow the dividend to fit in
> +	 * 32 bits. That's not a problem since we round the division down
> +	 * anyway.
> +	 */
> +	feedfwgain = (use_ssc || m2div_rem > 0) ?
> +		m1div * 1000000 * 100 / (dco_khz * 3 / 10) : 0;
> +
> +	if (dco_khz >= 9000000) {
> +		prop_coeff = 5;
> +		int_coeff = 10;
> +	} else {
> +		prop_coeff = 4;
> +		int_coeff = 8;
> +	}
> +
> +	if (use_ssc) {
> +		tmp = (uint64_t)dco_khz * 47 * 32;
> +		do_div(tmp, refclk_khz * m1div * 10000);
> +		ssc_stepsize = tmp;
> +
> +		tmp = (uint64_t)dco_khz * 1000;
> +		ssc_steplen = DIV_ROUND_UP_ULL(tmp, 32 * 2 * 32);
> +	} else {
> +		ssc_stepsize = 0;
> +		ssc_steplen = 0;
> +	}
> +	ssc_steplog = 4;
> +
> +	pll_state->mg_pll_div0 = (m2div_rem > 0 ? MG_PLL_DIV0_FRACNEN_H : 0) |
> +				  MG_PLL_DIV0_FBDIV_FRAC(m2div_frac) |
> +				  MG_PLL_DIV0_FBDIV_INT(m2div_int);
> +
> +	pll_state->mg_pll_div1 = MG_PLL_DIV1_IREF_NDIVRATIO(iref_ndiv) |
> +				 MG_PLL_DIV1_DITHER_DIV_2 |
> +				 MG_PLL_DIV1_NDIVRATIO(1) |
> +				 MG_PLL_DIV1_FBPREDIV(m1div);
> +
> +	pll_state->mg_pll_lf = MG_PLL_LF_TDCTARGETCNT(tdc_targetcnt) |
> +			       MG_PLL_LF_AFCCNTSEL_512 |
> +			       MG_PLL_LF_GAINCTRL(1) |
> +			       MG_PLL_LF_INT_COEFF(int_coeff) |
> +			       MG_PLL_LF_PROP_COEFF(prop_coeff);
> +
> +	pll_state->mg_pll_frac_lock = MG_PLL_FRAC_LOCK_TRUELOCK_CRIT_32 |
> +				      MG_PLL_FRAC_LOCK_EARLYLOCK_CRIT_32 |
> +				      MG_PLL_FRAC_LOCK_LOCKTHRESH(10) |
> +				      MG_PLL_FRAC_LOCK_DCODITHEREN |
> +				      MG_PLL_FRAC_LOCK_FEEDFWRDGAIN(feedfwgain);
> +	if (use_ssc || m2div_rem > 0)
> +		pll_state->mg_pll_frac_lock |= MG_PLL_FRAC_LOCK_FEEDFWRDCAL_EN;
> +
> +	pll_state->mg_pll_ssc = (use_ssc ? MG_PLL_SSC_EN : 0) |
> +				MG_PLL_SSC_TYPE(2) |
> +				MG_PLL_SSC_STEPLENGTH(ssc_steplen) |
> +				MG_PLL_SSC_STEPNUM(ssc_steplog) |
> +				MG_PLL_SSC_FILEN |
> +				MG_PLL_SSC_STEPSIZE(ssc_stepsize);
> +
> +	pll_state->mg_pll_tdc_coldst_bias = MG_PLL_TDC_COLDST_COLDSTART;
> +
> +	if (refclk_khz != 38400) {
> +		pll_state->mg_pll_tdc_coldst_bias |=
> +			MG_PLL_TDC_COLDST_IREFINT_EN |
> +			MG_PLL_TDC_COLDST_REFBIAS_START_PULSE_W(iref_pulse_w) |
> +			MG_PLL_TDC_COLDST_COLDSTART |
> +			MG_PLL_TDC_TDCCOVCCORR_EN |
> +			MG_PLL_TDC_TDCSEL(3);
> +
> +		pll_state->mg_pll_bias = MG_PLL_BIAS_BIAS_GB_SEL(3) |
> +					 MG_PLL_BIAS_INIT_DCOAMP(0x3F) |
> +					 MG_PLL_BIAS_BIAS_BONUS(10) |
> +					 MG_PLL_BIAS_BIASCAL_EN |
> +					 MG_PLL_BIAS_CTRIM(12) |
> +					 MG_PLL_BIAS_VREF_RDAC(4) |
> +					 MG_PLL_BIAS_IREFTRIM(iref_trim);
> +	}
> +
>  	return true;
>  }
>  
> -- 
> 2.14.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx