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