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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Oct 16, 2018 at 04:00:26PM -0700, Paulo Zanoni wrote:
> 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.

I am assumed lazy, so for me it is much better if I can put something
side by side and compare. So having it matching with whatever/however
spec is telling us is always better than calculations.

In case spec changes and we get notification it is easier to get and
change values directly.

> 
> Anyway, I can submit another version with the cnl model, no problem.
> 
> > 
> > > 
> > > 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,
> > > -							    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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx