Re: [PATCH] drm/i915/cnp+: update to the new RAWCLK_FREQ recommendations

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

On Thu, Oct 11, 2018 at 05:40:45PM -0700, Paulo Zanoni wrote:
> These are the new recommended values provided by our spec (18 -> 19
> and 23 -> 24). It seems this should help fixing GMBUS issues. Since
> we're doing pretty much the same thing for both CNP and ICP now, unify
> the functions using the ICP version since it's more straightforward by
> just matching the values listed in BSpec instead of recalculating
> them.

IMO calculating would be better. Would be trivial to see that the values
are at least consistent. With four magic numbers you have no choice but
to dig up the spec, which is painful. I don't like needless pain that
much.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  1 -
>  drivers/gpu/drm/i915/intel_cdclk.c | 37 ++++++-------------------------------
>  2 files changed, 6 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 20785417953d..ffd564a8d339 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7832,7 +7832,6 @@ enum {
>  #define  CNP_RAWCLK_DIV(div)	((div) << 16)
>  #define  CNP_RAWCLK_FRAC_MASK	(0xf << 26)
>  #define  CNP_RAWCLK_FRAC(frac)	((frac) << 26)
> -#define  ICP_RAWCLK_DEN(den)	((den) << 26)
>  #define  ICP_RAWCLK_NUM(num)	((num) << 11)
>  
>  #define PCH_DPLL_TMR_CFG        _MMIO(0xc6208)
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 29075c763428..17d3f13d89db 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -2660,48 +2660,25 @@ void intel_update_cdclk(struct drm_i915_private *dev_priv)
>  }
>  
>  static int cnp_rawclk(struct drm_i915_private *dev_priv)
> -{
> -	u32 rawclk;
> -	int divider, fraction;
> -
> -	if (I915_READ(SFUSE_STRAP) & SFUSE_STRAP_RAW_FREQUENCY) {
> -		/* 24 MHz */
> -		divider = 24000;
> -		fraction = 0;
> -	} else {
> -		/* 19.2 MHz */
> -		divider = 19000;
> -		fraction = 200;
> -	}
> -
> -	rawclk = CNP_RAWCLK_DIV((divider / 1000) - 1);
> -	if (fraction)
> -		rawclk |= CNP_RAWCLK_FRAC(DIV_ROUND_CLOSEST(1000,
> -							    fraction) - 1);
> -
> -	I915_WRITE(PCH_RAWCLK_FREQ, rawclk);
> -	return divider + fraction;
> -}
> -
> -static int icp_rawclk(struct drm_i915_private *dev_priv)
>  {
>  	u32 rawclk;
>  	int divider, numerator, denominator, frequency;
>  
>  	if (I915_READ(SFUSE_STRAP) & SFUSE_STRAP_RAW_FREQUENCY) {
>  		frequency = 24000;
> -		divider = 23;
> +		divider = 24;
>  		numerator = 0;
>  		denominator = 0;
>  	} else {
>  		frequency = 19200;
> -		divider = 18;
> +		divider = 19;
>  		numerator = 1;
>  		denominator = 4;

These numbers are extremely confusing. The hardware wants the numerator
as is but then it wants denominator-1. Which is a but odd. I think I'd
move the -1 for the denominator into the macro (or the invocation of the
macro, like cnp had). Alternatively we could at least write this as 5-1
here, so that the reader has at least some chance to figure out what
this means.

>  	}
>  
> -	rawclk = CNP_RAWCLK_DIV(divider) | ICP_RAWCLK_NUM(numerator) |
> -		 ICP_RAWCLK_DEN(denominator);
> +	rawclk = CNP_RAWCLK_DIV(divider) | CNP_RAWCLK_FRAC(denominator);
> +	if (HAS_PCH_ICP(dev_priv))
> +		rawclk |= ICP_RAWCLK_NUM(numerator);
>  
>  	I915_WRITE(PCH_RAWCLK_FREQ, rawclk);
>  	return frequency;
> @@ -2754,9 +2731,7 @@ static int g4x_hrawclk(struct drm_i915_private *dev_priv)
>   */
>  void intel_update_rawclk(struct drm_i915_private *dev_priv)
>  {
> -	if (HAS_PCH_ICP(dev_priv))
> -		dev_priv->rawclk_freq = icp_rawclk(dev_priv);
> -	else if (HAS_PCH_CNP(dev_priv))
> +	if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv))
>  		dev_priv->rawclk_freq = cnp_rawclk(dev_priv);
>  	else if (HAS_PCH_SPLIT(dev_priv))
>  		dev_priv->rawclk_freq = pch_rawclk(dev_priv);
> -- 
> 2.14.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx