Re: [GLK MIPI DSI V5 5/8] drm/i915/glk: Add DSI PLL divider range for glk

From: Jani Nikula <jani.nikula@intel.com>
To: Madhav Chauhan <madhav.chauhan@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: ander.conselvan.de.oliveira@intel.com, Deepak M <m.deepak@intel.com>
Subject: Re: [GLK MIPI DSI V5 5/8] drm/i915/glk: Add DSI PLL divider range for glk
Date: Thu, 16 Feb 2017 17:13:03 +0200	[thread overview]
Message-ID: <87inoamao0.fsf@intel.com> (raw)
In-Reply-To: <1487078180-15147-6-git-send-email-madhav.chauhan@intel.com>

On Tue, 14 Feb 2017, Madhav Chauhan <madhav.chauhan@intel.com> wrote:
> From: Deepak M <m.deepak@intel.com>
>
> PLL divider range for GLK is different than that of
> BXT, hence adding the GLK range check in this patch.
>
> v2: Code restructure using min and max ratio variables (Ander)
>
> Signed-off-by: Deepak M <m.deepak@intel.com>
> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  4 ++++
>  drivers/gpu/drm/i915/intel_dsi_pll.c | 24 +++++++++++++++++-------
>  2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c09f665..2cd7ff7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8348,10 +8348,12 @@ enum {
>  #define  BXT_DSI_PLL_PVD_RATIO_SHIFT	16
>  #define  BXT_DSI_PLL_PVD_RATIO_MASK	(3 << BXT_DSI_PLL_PVD_RATIO_SHIFT)
>  #define  BXT_DSI_PLL_PVD_RATIO_1	(1 << BXT_DSI_PLL_PVD_RATIO_SHIFT)
> +#define  BXT_DSIC_16X_BY1		(0 << 10)
>  #define  BXT_DSIC_16X_BY2		(1 << 10)
>  #define  BXT_DSIC_16X_BY3		(2 << 10)
>  #define  BXT_DSIC_16X_BY4		(3 << 10)
>  #define  BXT_DSIC_16X_MASK		(3 << 10)
> +#define  BXT_DSIA_16X_BY1		(0 << 8)
>  #define  BXT_DSIA_16X_BY2		(1 << 8)
>  #define  BXT_DSIA_16X_BY3		(2 << 8)
>  #define  BXT_DSIA_16X_BY4		(3 << 8)
> @@ -8361,6 +8363,8 @@ enum {
>  
>  #define BXT_DSI_PLL_RATIO_MAX		0x7D
>  #define BXT_DSI_PLL_RATIO_MIN		0x22
> +#define GLK_DSI_PLL_RATIO_MAX		0x6F
> +#define GLK_DSI_PLL_RATIO_MIN		0x22
>  #define BXT_DSI_PLL_RATIO_MASK		0xFF
>  #define BXT_REF_CLOCK_KHZ		19200
>  
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
> index 61440e5..e6383cb 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -430,11 +430,12 @@ static void bxt_dsi_program_clocks(struct drm_device *dev, enum port port,
>  	I915_WRITE(BXT_MIPI_CLOCK_CTL, tmp);
>  }
>  
> -static int bxt_compute_dsi_pll(struct intel_encoder *encoder,
> +static int gen9lp_compute_dsi_pll(struct intel_encoder *encoder,
>  			       struct intel_crtc_state *config)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> -	u8 dsi_ratio;
> +	u8 dsi_ratio, dsi_ratio_min, dsi_ratio_max;
>  	u32 dsi_clk;
>  
>  	dsi_clk = dsi_clk_from_pclk(intel_dsi->pclk, intel_dsi->pixel_format,
> @@ -446,11 +447,20 @@ static int bxt_compute_dsi_pll(struct intel_encoder *encoder,
>  	 * round 'up' the result
>  	 */
>  	dsi_ratio = DIV_ROUND_UP(dsi_clk * 2, BXT_REF_CLOCK_KHZ);
> -	if (dsi_ratio < BXT_DSI_PLL_RATIO_MIN ||
> -	    dsi_ratio > BXT_DSI_PLL_RATIO_MAX) {
> +
> +	if (IS_BROXTON(dev_priv)) {
> +		dsi_ratio_min = BXT_DSI_PLL_RATIO_MIN;
> +		dsi_ratio_max = BXT_DSI_PLL_RATIO_MAX;
> +	} else if (IS_GEMINILAKE(dev_priv)) {

This else if causes

 drivers/gpu/drm/i915/intel_dsi_pll.c: In function ‘intel_compute_dsi_pll’:
 drivers/gpu/drm/i915/intel_dsi_pll.c:513:45: error: ‘dsi_ratio_max’ may be
 used uninitialized in this function [-Werror=maybe-uninitialized]
   if (dsi_ratio < dsi_ratio_min || dsi_ratio > dsi_ratio_max) {
					      ^
 drivers/gpu/drm/i915/intel_dsi_pll.c:492:31: note: ‘dsi_ratio_max’ was
 declared here
   u8 dsi_ratio, dsi_ratio_min, dsi_ratio_max;
				^

And things like this in our driver generally beg the question, what
*other* platforms than the mentioned ones could run this code.

Just change that to an else.

BR,
Jani.

> +		dsi_ratio_min = GLK_DSI_PLL_RATIO_MIN;
> +		dsi_ratio_max = GLK_DSI_PLL_RATIO_MAX;
> +	}
> +
> +	if (dsi_ratio < dsi_ratio_min || dsi_ratio > dsi_ratio_max) {
>  		DRM_ERROR("Cant get a suitable ratio from DSI PLL ratios\n");
>  		return -ECHRNG;
> -	}
> +	} else
> +		DRM_DEBUG_KMS("DSI PLL calculation is Done!!\n");
>  
>  	/*
>  	 * Program DSI ratio and Select MIPIC and MIPIA PLL output as 8x
> @@ -462,7 +472,7 @@ static int bxt_compute_dsi_pll(struct intel_encoder *encoder,
>  	/* As per recommendation from hardware team,
>  	 * Prog PVD ratio =1 if dsi ratio <= 50
>  	 */
> -	if (dsi_ratio <= 50)
> +	if (IS_BROXTON(dev_priv) && dsi_ratio <= 50)
>  		config->dsi_pll.ctrl |= BXT_DSI_PLL_PVD_RATIO_1;
>  
>  	return 0;
> @@ -522,7 +532,7 @@ int intel_compute_dsi_pll(struct intel_encoder *encoder,
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		return vlv_compute_dsi_pll(encoder, config);
>  	else if (IS_GEN9_LP(dev_priv))
> -		return bxt_compute_dsi_pll(encoder, config);
> +		return gen9lp_compute_dsi_pll(encoder, config);
>  
>  	return -ENODEV;
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx