| Message ID | 20181012004045.13535-1-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 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
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 > >
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
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
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
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);
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(-)