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