Re: [PATCH 3/3] drm/i915: add ICP support to cnp_rawclk() and kill icp_rawclk()

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Seg, 2018-11-12 às 16:54 +0200, Ville Syrjälä escreveu:
> On Fri, Nov 09, 2018 at 04:23:50PM -0800, Paulo Zanoni wrote:
> > I think I'm probably the one who argued in favor of having separate
> > implementations for both PCHs, but the calculations are actually
> > the
> > same, the clocks are the same and the only difference is that on
> > ICP
> > we write the numerator to the register.
> > 
> > I have previously suggested to kill cnp_rawclk() and keep the
> > icp_rawclk() style, but Ville gave some good arguments that what's
> > in
> > this patch may be the better choice.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 37 ++++++++----------------
> > -------------
> >  1 file changed, 8 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 928671936286..60437675354e 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -2661,36 +2661,17 @@ static int cnp_rawclk(struct
> > drm_i915_private *dev_priv)
> >  	}
> >  
> >  	rawclk = CNP_RAWCLK_DIV(divider / 1000);
> > -	if (fraction)
> > -		rawclk |= CNP_RAWCLK_DEN(DIV_ROUND_CLOSEST(1000,
> > -							   fractio
> > n) - 1);
> > +	if (fraction) {
> > +		int numerator = 1000;
> >  
> > -	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 = 24;
> > -		numerator = 0;
> > -		denominator = 0;
> > -	} else {
> > -		frequency = 19200;
> > -		divider = 19;
> > -		numerator = 1;
> > -		denominator = 4;
> > +		rawclk |=
> > CNP_RAWCLK_DEN(DIV_ROUND_CLOSEST(numerator,
> > +							   fractio
> > n) - 1);
> > +		if (HAS_PCH_ICP(dev_priv))
> > +			rawclk |= ICP_RAWCLK_NUM(numerator /
> > 1000);
> 
> Maybe
> int numerator = 1;
> ...
> DIV_ROUND_CLOSEST(numerator * 1000, ... ?

Will do. I guess I tried to keep the logic of "local variables should
be in KHz and conversion to MHz goes inside the reg-writing macros",
but I don't feel it is better or worse than the suggestion.


> 
> The fixed numerator is OK since we only have to support 19.2 for
> now. If in the future we get more frequencies we may want to make
> this more flexible. But not much point in worrying about that now.
> 
> Series is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks a lot. Sorry for the huge delay in the follow-up.

> 
> >  	}
> >  
> > -	rawclk = CNP_RAWCLK_DIV(divider) |
> > ICP_RAWCLK_NUM(numerator) |
> > -		 CNP_RAWCLK_DEN(denominator);
> > -
> >  	I915_WRITE(PCH_RAWCLK_FREQ, rawclk);
> > -	return frequency;
> > +	return divider + fraction;
> >  }
> >  
> >  static int pch_rawclk(struct drm_i915_private *dev_priv)
> > @@ -2740,9 +2721,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