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@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
