Re: [PATCH v2 09/15] drm/i915: add VLV DSI PLL Calculations

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

On Fri, Aug 16, 2013 at 03:35:57PM +0300, Jani Nikula wrote:
> From: ymohanma <yogesh.mohan.marimuthu@intel.com>
> 
> v2:
>  - Grab dpio_lock mutex in vlv_enable_dsi_pll().
>  - Add and call vlv_disable_dsi_pll().
> 
> Signed-off-by: ymohanma <yogesh.mohan.marimuthu@intel.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile        |    1 +
>  drivers/gpu/drm/i915/i915_reg.h      |   10 ++
>  drivers/gpu/drm/i915/intel_dsi.c     |    7 +
>  drivers/gpu/drm/i915/intel_dsi.h     |    3 +
>  drivers/gpu/drm/i915/intel_dsi_pll.c |  310 ++++++++++++++++++++++++++++++++++
>  5 files changed, 331 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/intel_dsi_pll.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 5864c5b..65e60d2 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -23,6 +23,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
>  	  intel_lvds.o \
>  	  intel_dsi.o \
>  	  intel_dsi_cmd.o \
> +	  intel_dsi_pll.o \
>  	  intel_bios.o \
>  	  intel_ddi.o \
>  	  intel_dp.o \
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9879619..5feed04 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -390,6 +390,16 @@
>  #define   FB_FMAX_VMIN_FREQ_LO_SHIFT		27
>  #define   FB_FMAX_VMIN_FREQ_LO_MASK		0xf8000000
>  
> +/* vlv2 north clock has */
> +#define CCK_REG_DSI_PLL_FUSE			0x44
> +#define CCK_REG_DSI_PLL_CONTROL			0x48
> +#define  DSI_PLL_VCO_EN				(1 << 31)
> +#define  DSI_PLL_LDO_GATE			(1 << 30)
> +#define  DSI_PLL_P1_POST_DIV_SHIFT		17
> +#define  DSI_PLL_P1_POST_DIV_MASK		(0x1ff << 17)

All the clock gate/mux bits missing here. Maybe add names for them as
well.

> +#define  DSI_PLL_LOCK				(1 << 0)
> +#define CCK_REG_DSI_PLL_DIVIDER			0x4c

And maybe add the names for the bits here too.

> +
>  /*
>   * DPIO - a special bus for various display related registers to hide behind
>   *
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index d7eddbd..352ff46 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -83,6 +83,8 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>  static void intel_dsi_pre_pll_enable(struct intel_encoder *encoder)
>  {
>  	DRM_DEBUG_KMS("\n");
> +
> +	vlv_enable_dsi_pll(encoder);
>  }
>  
>  static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> @@ -161,6 +163,8 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>  static void intel_dsi_post_disable(struct intel_encoder *encoder)
>  {
>  	DRM_DEBUG_KMS("\n");
> +
> +	vlv_disable_dsi_pll(encoder);
>  }
>  
>  static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
> @@ -284,6 +288,9 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
>  
>  	DRM_DEBUG_KMS("pipe %d\n", pipe);
>  
> +	/* Update the DSI PLL */
> +	vlv_enable_dsi_pll(intel_encoder);
> +
>  	/* escape clock divider, 20MHz, shared for A and C. device ready must be
>  	 * off when doing this! txclkesc? */
>  	tmp = I915_READ(MIPI_CTRL(0));
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index f308269..c7765f3 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -96,4 +96,7 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>  	return container_of(encoder, struct intel_dsi, base.base);
>  }
>  
> +extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
> +extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
> +
>  #endif /* _INTEL_DSI_H */
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
> new file mode 100644
> index 0000000..c50a90e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -0,0 +1,310 @@
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + *	Shobhit Kumar <shobhit.kumar@intel.com>
> + *	Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include "intel_drv.h"
> +#include "i915_drv.h"
> +#include "intel_dsi.h"
> +
> +#define DSI_HSS_PACKET_SIZE		4
> +#define DSI_HSE_PACKET_SIZE		4
> +#define DSI_HSA_PACKET_EXTRA_SIZE	6
> +#define DSI_HBP_PACKET_EXTRA_SIZE	6
> +#define DSI_HACTIVE_PACKET_EXTRA_SIZE	6
> +#define DSI_HFP_PACKET_EXTRA_SIZE	6
> +#define DSI_EOTP_PACKET_SIZE		4
> +
> +struct dsi_clock_table {
> +	u32 freq;
> +	u8 m;
> +	u8 p;
> +};
> +
> +struct dsi_mnp {
> +	u32 dsi_pll_ctrl;
> +	u32 dsi_pll_div;
> +};
> +
> +static u32 lfsr_converts[] = {

+const

> +	426, 469, 234, 373, 442, 221, 110, 311, 411,		/* 62 - 70 */
> +	461, 486, 243, 377, 188, 350, 175, 343, 427, 213,	/* 71 - 80 */
> +	106, 53, 282, 397, 354, 227, 113, 56, 284, 142,		/* 81 - 90 */
> +	71, 35							/* 91 - 92 */
> +};

Can't find this table in my specs. Presumably it's correct if the code
manages to generate the correct clocks.

> +
> +struct dsi_clock_table dsi_clk_tbl[] = {

+static const 

> +	{300, 72, 6}, {313, 75, 6}, {323, 78, 6}, {333, 80, 6},
> +	{343, 82, 6}, {353, 85, 6}, {363, 87, 6}, {373, 90, 6},
> +	{383, 92, 6}, {390, 78, 5}, {393, 79, 5}, {400, 80, 5},
> +	{401, 80, 5}, {402, 80, 5}, {403, 81, 5}, {404, 81, 5},
> +	{405, 81, 5}, {406, 81, 5}, {407, 81, 5}, {408, 82, 5},
> +	{409, 82, 5}, {410, 82, 5}, {411, 82, 5}, {412, 82, 5},
> +	{413, 83, 5}, {414, 83, 5}, {415, 83, 5}, {416, 83, 5},
> +	{417, 83, 5}, {418, 84, 5}, {419, 84, 5}, {420, 84, 5},
> +	{430, 86, 5}, {440, 88, 5}, {450, 90, 5}, {460, 92, 5},
> +	{470, 75, 4}, {480, 77, 4}, {490, 78, 4}, {500, 80, 4},
> +	{510, 82, 4}, {520, 83, 4}, {530, 85, 4}, {540, 86, 4},
> +	{550, 88, 4}, {560, 90, 4}, {570, 91, 4}, {580, 70, 3},
> +	{590, 71, 3}, {600, 72, 3}, {610, 73, 3}, {620, 74, 3},
> +	{630, 76, 3}, {640, 77, 3}, {650, 78, 3}, {660, 79, 3},
> +	{670, 80, 3}, {680, 82, 3}, {690, 83, 3}, {700, 84, 3},
> +	{710, 85, 3}, {720, 86, 3}, {730, 88, 3}, {740, 89, 3},
> +	{750, 90, 3}, {760, 91, 3}, {770, 92, 3}, {780, 62, 2},
> +	{790, 63, 2}, {800, 64, 2}, {880, 70, 2}, {900, 72, 2},
> +	{1000, 80, 2},		/* dsi clock frequency in Mhz*/
> +};
> +
> +static u32 dsi_rr_formula(struct drm_display_mode *mode,
> +			  int pixel_format, int video_mode_format,
> +			  int lane_count, bool eotp)
> +{
> +	u32 bpp;
> +	u32 hactive, vactive, hfp, hsync, hbp, vfp, vsync, vbp;
> +	u32 hsync_bytes, hbp_bytes, hactive_bytes, hfp_bytes;
> +	u32 bytes_per_line, bytes_per_frame;
> +	u32 num_frames;
> +	u32 bytes_per_x_frames, bytes_per_x_frames_x_lanes;
> +	u32 dsi_bit_clock_hz;
> +	u32 dsi_clk;
> +
> +	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;
> +	}
> +
> +	hactive = mode->hdisplay;
> +	vactive = mode->vdisplay;
> +	hfp = mode->hsync_start - mode->hdisplay;
> +	hsync = mode->hsync_end - mode->hsync_start;
> +	hbp = mode->htotal - mode->hsync_end;
> +
> +	vfp = mode->vsync_start - mode->vdisplay;
> +	vsync = mode->vsync_end - mode->vsync_start;
> +	vbp = mode->vtotal - mode->vsync_end;
> +
> +	hsync_bytes = DIV_ROUND_UP(hsync * bpp, 8);
> +	hbp_bytes = DIV_ROUND_UP(hbp * bpp, 8);
> +	hactive_bytes = DIV_ROUND_UP(hactive * bpp, 8);
> +	hfp_bytes = DIV_ROUND_UP(hfp * bpp, 8);
> +
> +	bytes_per_line = DSI_HSS_PACKET_SIZE + hsync_bytes +
> +		DSI_HSA_PACKET_EXTRA_SIZE + DSI_HSE_PACKET_SIZE +
> +		hbp_bytes + DSI_HBP_PACKET_EXTRA_SIZE +
> +		hactive_bytes + DSI_HACTIVE_PACKET_EXTRA_SIZE +
> +		hfp_bytes + DSI_HFP_PACKET_EXTRA_SIZE;
> +
> +	/*
> +	 * XXX: Need to accurately calculate LP to HS transition timeout and add
> +	 * it to bytes_per_line/bytes_per_frame.
> +	 */
> +
> +	if (eotp && video_mode_format == VIDEO_MODE_BURST)
> +		bytes_per_line += DSI_EOTP_PACKET_SIZE;
> +
> +	bytes_per_frame = vsync * bytes_per_line + vbp * bytes_per_line +
> +		vactive * bytes_per_line + vfp * bytes_per_line;
> +
> +	if (eotp &&
> +	    (video_mode_format == VIDEO_MODE_NON_BURST_WITH_SYNC_PULSE ||
> +	     video_mode_format == VIDEO_MODE_NON_BURST_WITH_SYNC_EVENTS))
> +		bytes_per_frame += DSI_EOTP_PACKET_SIZE;
> +
> +	num_frames = (mode->clock * 1000) / (mode->htotal * mode->vtotal);

drm_mode_vrefresh() perhaps. That would round to nearest instead of down.

> +	bytes_per_x_frames = num_frames * bytes_per_frame;
> +
> +	bytes_per_x_frames_x_lanes = bytes_per_x_frames / lane_count;
> +
> +	/* the dsi clock is divided by 2 in the hardware to get dsi ddr clock */
> +	dsi_bit_clock_hz = bytes_per_x_frames_x_lanes * 8;
> +	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);

We want it in kHz for the PLL calculations, so we lose a bit of precision
when we clearly don't need to.

> +
> +	if (eotp && video_mode_format == VIDEO_MODE_BURST)
> +		dsi_clk *= 2;

I wonder what's the magic here? Why does the generation of EoT packets
in burst mode warrant doubled clock? We already added the actual
overhead from the EoT packets before.

> +
> +	return dsi_clk;
> +}
> +
> +static int mnp_from_clk_table(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
> +{
> +	unsigned int i;
> +	u8 m;
> +	u8 n;
> +	u8 p;
> +	u32 m_seed;
> +
> +	if (dsi_clk < 300 || dsi_clk > 1000)
> +		return -ECHRNG;
> +
> +	for (i = 0; i <= ARRAY_SIZE(dsi_clk_tbl); i++) {
> +		if (dsi_clk_tbl[i].freq > dsi_clk)
> +			break;
> +	}
> +
> +	m = dsi_clk_tbl[i].m;
> +	p = dsi_clk_tbl[i].p;
> +	m_seed = lfsr_converts[m - 62];
> +	n = 1;
> +	dsi_mnp->dsi_pll_ctrl = (1 << (17 + p - 2)) | (1 << 8);

The clock gating stuff is hardcoded here. I'd probably move it out from
this function so that we can play with the PLL bypass/second DSI output
a bit easier at some point. A FIXME might be nice to let people know
we still have things to implement there.

> +	dsi_mnp->dsi_pll_div = ((n - 1) << 16) | m_seed;
> +
> +	return 0;
> +}
> +
> +static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
> +{
> +	u32 m, n, p;
> +	u32 ref_clk;
> +	u32 error;
> +	u32 tmp_error;
> +	u32 target_dsi_clk;
> +	u32 calc_dsi_clk;
> +	u32 calc_m;
> +	u32 calc_p;
> +	u32 m_seed;
> +
> +	if (dsi_clk < 300 || dsi_clk > 1150) {

Hmm. North clock HAS says the range is 100-1000, display cluster HAS
says 300-1000. Can't see 1150 in any of my docs.

> +		DRM_ERROR("DSI CLK Out of Range\n");
> +		return -ECHRNG;
> +	}
> +
> +	ref_clk = 25000;
> +	target_dsi_clk = dsi_clk * 1000;
> +	error = 0xFFFFFFFF;
> +	tmp_error = 0xFFFFFFFF;

No need to init tmp_error.

> +	calc_m = 0;
> +	calc_p = 0;
> +
> +	for (m = 62; m <= 92; m++) {
> +		for (p = 2; p <= 6; p++) {

Hmm. Full range for P1 is 2-10 accoring to north clock HAS.
Display cluster HAS has the 2-6 range.

> +
> +			calc_dsi_clk = (m * ref_clk) / p;

We're not actually checking that VCO freq is within limits.
Again north clock HAS says 100-1000 is also the VCO limit
(sounds fishy), display cluster HAS says 1550-2300.

1150 is 2300/2, so maybe that's where the 1150 came from. Not
sure how the 6 came about. 300*7 < 2300 so letting P go up to
7 would still keep the VCO freq in range.

Oh well, I suppose it's all good enough.

> +			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;
> +				}
> +			}

Could use 'continue' to avoid deep nesting.

> +		}
> +	}
> +
> +	m_seed = lfsr_converts[calc_m - 62];
> +	n = 1;

Matches display cluster HAS it seems.

> +	dsi_mnp->dsi_pll_ctrl = (1 << (17 + calc_p - 2)) | (1 << 8);

Hardcoded clock gating again.

> +	dsi_mnp->dsi_pll_div = ((n - 1) << 16) | m_seed;
> +
> +	return 0;
> +}
> +
> +static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> +	int ret;
> +	struct dsi_mnp dsi_mnp;
> +	u32 dsi_clk;
> +
> +	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
> +				 intel_dsi->video_mode_format,
> +				 intel_dsi->lane_count, !intel_dsi->eot_disable);
> +
> +#if 0
> +	ret = mnp_from_clk_table(dsi_clk, &dsi_mnp);
> +#else
> +	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
> +#endif
> +
> +	if (ret) {
> +		DRM_DEBUG_KMS("dsi_calc_mnp failed\n");
> +		return;
> +	}
> +
> +	DRM_DEBUG_KMS("dsi pll div %08x, ctrl %08x\n",
> +		      dsi_mnp.dsi_pll_div, dsi_mnp.dsi_pll_ctrl);
> +
> +	vlv_cck_write(dev_priv, CCK_REG_DSI_PLL_CONTROL, 0);
> +	vlv_cck_write(dev_priv, CCK_REG_DSI_PLL_DIVIDER, dsi_mnp.dsi_pll_div);
> +	vlv_cck_write(dev_priv, CCK_REG_DSI_PLL_CONTROL, dsi_mnp.dsi_pll_ctrl);
> +}
> +
> +void vlv_enable_dsi_pll(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	int pipe = intel_crtc->pipe;
> +	u32 tmp;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	mutex_lock(&dev_priv->dpio_lock);
> +
> +	vlv_configure_dsi_pll(encoder);
> +
> +	/* wait 0.5us after ungating before enabling again */
> +	udelay(1 * 1000);

Says 0.5us and then spins for 1ms. Quite a large difference.

> +
> +	tmp = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL);
> +	tmp |= DSI_PLL_VCO_EN;
> +	vlv_cck_write(dev_priv, CCK_REG_DSI_PLL_CONTROL, tmp);
> +
> +	mutex_unlock(&dev_priv->dpio_lock);
> +
> +	if (wait_for(I915_READ(PIPECONF(pipe)) & PIPECONF_DSI_PLL_LOCKED, 20)) {

Should be hardcoded PIPE_A here I think. Or we could use the lock bit
in CCK_REG_DSI_PLL_CONTROL, but PIPECONF should have less overhead and
we don't need to hold the dpio_lock.

> +		DRM_ERROR("DSI PLL lock failed\n");
> +		return;
> +	}
> +
> +	DRM_DEBUG_KMS("DSI PLL locked\n");
> +}
> +
> +void vlv_disable_dsi_pll(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> +	u32 tmp;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	mutex_lock(&dev_priv->dpio_lock);
> +
> +	tmp = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL);
> +	tmp &= ~DSI_PLL_VCO_EN;

Also power gate the LDO?

> +	vlv_cck_write(dev_priv, CCK_REG_DSI_PLL_CONTROL, tmp);
> +
> +	mutex_unlock(&dev_priv->dpio_lock);
> +}
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC