[Intel-gfx] [PATCH] drm/i915/cnp+: update to the new RAWCLK_FREQ recommendations

[Paulo Zanoni]

Em Sex, 2018-10-12 às 15:42 +0300, Ville Syrjälä escreveu:
> 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.

Then I guess our workloads are different. IMHO the code is trivial when
we can easily see that we set the exact magic values that the spec
tells us to set.  With the formula, you have to do the calculations for
both frequencies, then you take those values and compare against the
spec, which is an extra step. Developing without the spec open is
something that I never even consider.

Anyway, I can submit another version with the cnl model, no problem.

> 
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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,
> > -							    fracti
> > on) - 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
>