[Intel-gfx] [PATCH 3/4] drm/i915: Compute dsi_clk from pixel clock

[Jani Nikula]

On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar at intel.com> wrote:
> Minor modification to m_n_p calculations as well

That should probably be a separate patch, unless it's a requirement for
what the main subject of this patch is. The commit message does not say.

> Signed-off-by: Shobhit Kumar <shobhit.kumar at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
> index 44279b2..bf12335 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>  	71, 35							/* 91 - 92 */
>  };
>  
> +#ifdef DSI_CLK_FROM_RR
> +
>  static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  			  int pixel_format, int video_mode_format,
>  			  int lane_count, bool eotp)
> @@ -129,6 +131,40 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  	return dsi_clk;
>  }
>  
> +#else
> +
> +/* Get DSI clock from pixel clock */
> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
> +			  int pixel_format, int lane_count)
> +{
> +	u32 dsi_bit_clock_hz, dsi_clk;
> +	u32 bpp;
> +
> +	switch (pixel_format) {
> +	default:
> +	case VID_MODE_FORMAT_RGB888:
> +	case VID_MODE_FORMAT_RGB666_LOOSE:
> +		bpp = 24;
> +		break;
> +	case VID_MODE_FORMAT_RGB666:
> +		bpp = 18;
> +		break;
> +	case VID_MODE_FORMAT_RGB565:
> +		bpp = 16;
> +		break;
> +	}
> +
> +	/* DSI data rate = pixel clock * bits per pixel / lane count
> +	   pixel clock is converted from KHz to Hz */
> +	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
> +
> +	/* DSI clock rate */
> +	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
> +	return dsi_clk;
> +}
> +
> +#endif
> +
>  #ifdef MNP_FROM_TABLE
>  
>  struct dsi_clock_table {
> @@ -208,29 +244,42 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>  	ref_clk = 25000;
>  	target_dsi_clk = dsi_clk * 1000;
>  	error = 0xFFFFFFFF;
> +	tmp_error = 0xFFFFFFFF;
>  	calc_m = 0;
>  	calc_p = 0;
>  
>  	for (m = 62; m <= 92; m++) {
>  		for (p = 2; p <= 6; p++) {
> -
> +			/* Find the optimal m and p divisors
> +			with minimal error +/- the required clock */
>  			calc_dsi_clk = (m * ref_clk) / p;
> -			if (calc_dsi_clk >= target_dsi_clk) {
> +			if (calc_dsi_clk == target_dsi_clk) {
> +				calc_m = m;
> +				calc_p = p;
> +				error = 0;
> +				break;
> +			} else if (calc_dsi_clk > target_dsi_clk)
>  				tmp_error = calc_dsi_clk - target_dsi_clk;
> -				if (tmp_error < error) {
> -					error = tmp_error;
> -					calc_m = m;
> -					calc_p = p;
> -				}
> +			else
> +				tmp_error = target_dsi_clk - calc_dsi_clk;
> +

Using abs() might clean this up a bit.

> +			if (tmp_error < error) {
> +				error = tmp_error;
> +				calc_m = m;
> +				calc_p = p;
>  			}
>  		}
> +
> +		if (error == 0)
> +			break;

The above changes should be a separate patch, with rationale in the
commit message.

>  	}
>  
>  	m_seed = lfsr_converts[calc_m - 62];
>  	n = 1;
> +
>  	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
>  	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
> -		m_seed << DSI_PLL_M1_DIV_SHIFT;
> +					m_seed << DSI_PLL_M1_DIV_SHIFT;

If really needed, style/whitespace changes should also be separated.

>  
>  	return 0;
>  }
> @@ -249,11 +298,13 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	int ret;
>  	struct dsi_mnp dsi_mnp;
> -	u32 dsi_clk;
> +	u32 dsi_clk = 0;
>  
> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
> -				 intel_dsi->video_mode_format,
> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
> +	if (intel_dsi->dsi_clock_freq)
> +		dsi_clk = intel_dsi->dsi_clock_freq;

This is the third major change in the patch. Should be separate, with
rationale, or possibly bundled with a different change which starts
using it. Who is responsible for setting and clearing this?

> +	else
> +		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
> +							intel_dsi->lane_count);
>  
>  	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>  	if (ret) {
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center