| Message ID | 20181110002350.24362-3-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/cnp+: update to the new RAWCLK_FREQ recommendations | expand |
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, > - fraction) - 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, > + fraction) - 1); > + if (HAS_PCH_ICP(dev_priv)) > + rawclk |= ICP_RAWCLK_NUM(numerator / 1000); Maybe int numerator = 1; ... DIV_ROUND_CLOSEST(numerator * 1000, ... ? 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> > } > > - 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
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 > >
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, - fraction) - 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, + fraction) - 1); + if (HAS_PCH_ICP(dev_priv)) + rawclk |= ICP_RAWCLK_NUM(numerator / 1000); } - 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);
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(-)