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

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

[Paulo Zanoni]

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.

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;
 	}
 
-	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

✗ Fi.CI.BAT: failure for drm/i915/cnp+: update to the new RAWCLK_FREQ recommendations

[Patchwork]

== Series Details ==

Series: drm/i915/cnp+: update to the new RAWCLK_FREQ recommendations
URL   : https://patchwork.freedesktop.org/series/50893/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4975 -> Patchwork_10432 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10432 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10432, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50893/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10432:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_objects:
      fi-cfl-8109u:       NOTRUN -> INCOMPLETE

    
== Known issues ==

  Here are the changes found in Patchwork_10432 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-blb-e6850:       NOTRUN -> INCOMPLETE (fdo#107718)

    igt@kms_frontbuffer_tracking@basic:
      fi-icl-u2:          SKIP -> FAIL (fdo#103167)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      fi-cfl-8109u:       INCOMPLETE (fdo#108126, fdo#107187) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@pm_rpm@basic-pci-d3-state:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS

    igt@pm_rpm@module-reload:
      fi-glk-j4005:       DMESG-WARN (fdo#106097, fdo#107726) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#107187 https://bugs.freedesktop.org/show_bug.cgi?id=107187
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107726 https://bugs.freedesktop.org/show_bug.cgi?id=107726
  fdo#108126 https://bugs.freedesktop.org/show_bug.cgi?id=108126


== Participating hosts (43 -> 40) ==

  Additional (2): fi-skl-guc fi-icl-u 
  Missing    (5): fi-ctg-p8600 fi-bsw-cyan fi-ilk-m540 fi-byt-squawks fi-gdg-551 


== Build changes ==

    * Linux: CI_DRM_4975 -> Patchwork_10432

  CI_DRM_4975: 2079843dea0b57ecf37532761a95e3f0752d1260 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4674: 93871c6fb3c25e5d350c9faf36ded917174214de @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10432: 5d2244b200a18c2edbcec4a47aae27961345c0bb @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

5d2244b200a1 drm/i915/cnp+: update to the new RAWCLK_FREQ recommendations

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10432/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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

Re: [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@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

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

[Rodrigo Vivi]

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

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

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 04:09:19PM -0700, Rodrigo Vivi wrote:
> 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.

If I'm changing a set of magic numbers derived from some formula
I would do the calculations anyway because the spec could be wrong.
So IMO it's better to do the calculations in the first place as
that requires you to understand what those magic numbers mean.
Having the formula in the code makes that easier as then you
don't have to derive it from the spec every time.

To me blindly copy pasting magic numbers from a spec seems like
a job for an automaton rather than an engineer. I definitely
don't enjoy doing that kind of work, which means I won't be all
that diligent when doing it. And that increases the chance of
human error in the process. If we can make due with a single
magic number rather than four there's less opportunity to
get it wrong.

Just my 2c. I presume not everyone will be convinced.

> 
> 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

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

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

[Rodrigo Vivi]

On Thu, Oct 18, 2018 at 03:52:21PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 16, 2018 at 04:09:19PM -0700, Rodrigo Vivi wrote:
> > 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.
> 
> If I'm changing a set of magic numbers derived from some formula
> I would do the calculations anyway because the spec could be wrong.
> So IMO it's better to do the calculations in the first place as
> that requires you to understand what those magic numbers mean.
> Having the formula in the code makes that easier as then you
> don't have to derive it from the spec every time.

this is a good point. And Paulo told he is open to send another
version

> 
> To me blindly copy pasting magic numbers from a spec seems like
> a job for an automaton rather than an engineer.

bummer... you are spoiling some ideas I have here to make bspec's svn
changes to check our code and file a jira for us ;)

> I definitely
> don't enjoy doing that kind of work, which means I won't be all
> that diligent when doing it.

also good point...

> And that increases the chance of
> human error in the process. If we can make due with a single
> magic number rather than four there's less opportunity to
> get it wrong.

although I bet this varies from person to person...

but I see what you mean ;)

> 
> Just my 2c. I presume not everyone will be convinced.
> 
> > 
> > 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
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> 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

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

[Paulo Zanoni]

BSpec was updated and now there's no more "subtract 1" to the
Microsecond Counter Divider field.

It seems this should help fixing some GMBUS issues. I'm not aware of
any specific open bug that could be solved by this patch.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_cdclk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 8d74276029e6..810670976e86 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -2660,7 +2660,7 @@ static int cnp_rawclk(struct drm_i915_private *dev_priv)
 		fraction = 200;
 	}
 
-	rawclk = CNP_RAWCLK_DIV((divider / 1000) - 1);
+	rawclk = CNP_RAWCLK_DIV(divider / 1000);
 	if (fraction)
 		rawclk |= CNP_RAWCLK_FRAC(DIV_ROUND_CLOSEST(1000,
 							    fraction) - 1);
@@ -2676,12 +2676,12 @@ static int icp_rawclk(struct drm_i915_private *dev_priv)
 
 	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;
 	}
-- 
2.14.4

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

[PATCH 2/3] drm/i915: rename CNP_RAWCLK_FRAC to CNP_RAWCLK_DEN

[Paulo Zanoni]

Although CNP names this field "Counter Fraction", what we write to the
register is really the denominator for the fractional part of the
divider, not the fractional part (and the field description even says
that). The ICP spec renamed the field to "Counter Fraction
Denominator", which makes a lot more sense. Use the more complete ICL
naming because we will merge the CNP and ICP functions into a single
one, which will introduce the concept of the numerator. That will make
a lot more sense when you read the "num/frac = den" calculation.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    | 3 +--
 drivers/gpu/drm/i915/intel_cdclk.c | 6 +++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fe4b913e46ac..16f0d73bb4fe 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7907,8 +7907,7 @@ enum {
 #define  CNP_RAWCLK_DIV_MASK	(0x3ff << 16)
 #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  CNP_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 810670976e86..928671936286 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -2662,8 +2662,8 @@ static int cnp_rawclk(struct drm_i915_private *dev_priv)
 
 	rawclk = CNP_RAWCLK_DIV(divider / 1000);
 	if (fraction)
-		rawclk |= CNP_RAWCLK_FRAC(DIV_ROUND_CLOSEST(1000,
-							    fraction) - 1);
+		rawclk |= CNP_RAWCLK_DEN(DIV_ROUND_CLOSEST(1000,
+							   fraction) - 1);
 
 	I915_WRITE(PCH_RAWCLK_FREQ, rawclk);
 	return divider + fraction;
@@ -2687,7 +2687,7 @@ static int icp_rawclk(struct drm_i915_private *dev_priv)
 	}
 
 	rawclk = CNP_RAWCLK_DIV(divider) | ICP_RAWCLK_NUM(numerator) |
-		 ICP_RAWCLK_DEN(denominator);
+		 CNP_RAWCLK_DEN(denominator);
 
 	I915_WRITE(PCH_RAWCLK_FREQ, rawclk);
 	return frequency;
-- 
2.14.4

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

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

[Paulo Zanoni]

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);
 	}
 
-	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

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

[Ville Syrjälä]

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

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

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

[Paulo Zanoni]

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

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

[Paulo Zanoni]

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.

v2: Switch numerator to 1 from 1000 and adjust calculations
    accordingly (Ville).

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: 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..25e3aba9cded 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 = 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 = 24;
-		numerator = 0;
-		denominator = 0;
-	} else {
-		frequency = 19200;
-		divider = 19;
-		numerator = 1;
-		denominator = 4;
+		rawclk |= CNP_RAWCLK_DEN(DIV_ROUND_CLOSEST(numerator * 1000,
+							   fraction) - 1);
+		if (HAS_PCH_ICP(dev_priv))
+			rawclk |= ICP_RAWCLK_NUM(numerator);
 	}
 
-	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

✗ Fi.CI.BAT: failure for drm/i915/cnp+: update to the new RAWCLK_FREQ recommendations

[Patchwork]

== Series Details ==

Series: drm/i915/cnp+: update to the new RAWCLK_FREQ recommendations
URL   : https://patchwork.freedesktop.org/series/50893/
State : failure

== Summary ==

Applying: drm/i915/cnp+: update to the new RAWCLK_FREQ recommendations
Applying: drm/i915: add ICP support to cnp_rawclk() and kill icp_rawclk()
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_cdclk.c).
error: could not build fake ancestor
Patch failed at 0002 drm/i915: add ICP support to cnp_rawclk() and kill icp_rawclk()
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10432/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx