On Mon, Sep 28, 2015 at 10:31:49PM +0300, Imre Deak wrote:
> On Thu, 2015-09-24 at 23:29 +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > As with the cdclk, read out czclk from CCK as well. This gives us the
> > real current value and avoids having to decode fuses and whatnot.
> >
> > Also store it in kHz under dev_priv like we do for cdlck since it's not
> > just an rps related clock, and having it in kHz is more
> > standard/convenient for some things.
> >
> > Imre also pointed out that we currently fail to read czclk on VLV, which
> > means the PFI credit programming isn't working as expected.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Yep, much better to have a generic way to read out all the CCK clocks,
> removes a lot of special casing. It's also true that these clocks can
> change, though let's note that czclk is fixed. Looks ok to me:
> Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> Below some nitpicks about naming.
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++--------------
> > drivers/gpu/drm/i915/intel_pm.c | 25 ++--------
> > 4 files changed, 60 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2b5d587..77c9312 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1146,7 +1146,6 @@ struct intel_gen6_power_mgmt {
> > u8 efficient_freq; /* AKA RPe. Pre-determined balanced frequency */
> > u8 rp1_freq; /* "less than" RP0 power/freqency */
> > u8 rp0_freq; /* Non-overclocked max frequency. */
> > - u32 cz_freq;
> >
> > u8 up_threshold; /* Current %busy required to uplock */
> > u8 down_threshold; /* Current %busy required to downclock */
> > @@ -1810,6 +1809,7 @@ struct drm_i915_private {
> > unsigned int cdclk_freq, max_cdclk_freq;
> > unsigned int max_dotclk_freq;
> > unsigned int hpll_freq;
> > + unsigned int czclk_freq;
> >
> > /**
> > * wq - Driver workqueue for GEM.
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index f22fb80..83218032 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -728,6 +728,7 @@ enum skl_disp_power_wells {
> > #define DSI_PLL_N1_DIV_MASK (3 << 16)
> > #define DSI_PLL_M1_DIV_SHIFT 0
> > #define DSI_PLL_M1_DIV_MASK (0x1ff << 0)
> > +#define CCK_CZ_CLOCK_CONTROL 0x62
> > #define CCK_DISPLAY_CLOCK_CONTROL 0x6b
> > #define CCK_TRUNK_FORCE_ON (1 << 17)
> > #define CCK_TRUNK_FORCE_OFF (1 << 16)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8567b46..d668897 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -132,6 +132,42 @@ struct intel_limit {
> > intel_p2_t p2;
> > };
> >
> > +/* returns HPLL frequency in kHz */
> > +static int valleyview_get_vco(struct drm_i915_private *dev_priv)
>
> This wasn't added in this patch, but I would call it get_cck_ref or
> similar. VCO is too generic and HPLL isn't accurate, since the 800MHz
> clock is a bypass clock directly from iCLK.
800Mhz on VLV and 1600MHz on CHV I believe. I'm not sure the bypass is
ever used though. CHV clock HAS has a note that the bypass option had
a lot of jitter, so that that may be the reason for not using it. VLV
north clock HAS doesn't have such a note, but it does go on to say that
the plan was to use HPLL (except maybe with SKU200). Also there's
another note in the VLV docs saying HPLL bypass+GPLL is a debug only
configuration, and we always use GPLL so that would imply we always use
HPLL too.
Looking at the fuses on a few BYT and BSW machines, they both use the
HPLL @ 1600MHz. The BYT is SKU266 and BSW is SKU320, so in theory I
think both could use the bypass clock but neither does.
But yeah, the current function name isn't all that good. I think
vlv_get_hpll_freq() or something would be OK, since even the bypass
clock is still called "HPLL bypass clock". And the way it's drawn in
the docs is often as a bigger HPLL block which contains both the HPLL
proper, the bypass path, and the mux to select between the two.
>
> > +{
> > + int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
> > +
> > + /* Obtain SKU information */
> > + mutex_lock(&dev_priv->sb_lock);
> > + hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
> > + CCK_FUSE_HPLL_FREQ_MASK;
> > + mutex_unlock(&dev_priv->sb_lock);
> > +
> > + return vco_freq[hpll_freq] * 1000;
> > +}
> > +
> > +static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
> > + const char *name, u32 reg)
>
> I would have called it simply get_cck_clock for the above reason.
I have some more patches sitting in branch that split this
into vlv_get_cck_clock() and vlv_get_cck_clock_hpll(), like so:
vlv_get_cck_clock(reg, ref)
{
// do the math
}
vlv_get_cck_clock_hpll(reg)
{
if (!hpll_freq)
hpll_freq = valleyview_get_vco()
return vlv_get_cck_clock(reg, hpll_freq);
}
Simply because in the normal case the HPLL output is the clock
that gets divider down. But there is at least one special case
(GPLL ref clock) where that isn't the case.
>
> > +{
> > + u32 val;
> > + int divider;
> > +
> > + if (dev_priv->hpll_freq == 0)
> > + dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
> > +
> > + mutex_lock(&dev_priv->sb_lock);
> > + val = vlv_cck_read(dev_priv, reg);
> + mutex_unlock(&dev_priv->sb_lock);
> > +
> > + divider = val & CCK_FREQUENCY_VALUES;
> > +
> > + WARN((val & CCK_FREQUENCY_STATUS) !=
> > + (divider << CCK_FREQUENCY_STATUS_SHIFT),
> > + "%s change in progress\n", name);
> > +
> > + return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> > +}
> > +
> > int
> > intel_pch_rawclk(struct drm_device *dev)
> > {
> > @@ -175,6 +211,17 @@ int intel_hrawclk(struct drm_device *dev)
> > }
> > }
> >
> > +static void intel_update_czclk(struct drm_i915_private *dev_priv)
> > +{
> > + if (!IS_VALLEYVIEW(dev_priv))
> > + return;
> > +
> > + dev_priv->czclk_freq = vlv_get_cck_clock_hpll(dev_priv, "czclk",
> > + CCK_CZ_CLOCK_CONTROL);
> > +
> > + DRM_DEBUG_DRIVER("CZ clock rate: %d kHz\n", dev_priv->czclk_freq);
> > +}
> > +
> > static inline u32 /* units of 100MHz */
> > intel_fdi_link_freq(struct drm_device *dev)
> > {
> > @@ -5749,20 +5796,6 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv)
> > DRM_ERROR("DBuf power enable timeout\n");
> > }
> >
> > -/* returns HPLL frequency in kHz */
> > -static int valleyview_get_vco(struct drm_i915_private *dev_priv)
> > -{
> > - int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
> > -
> > - /* Obtain SKU information */
> > - mutex_lock(&dev_priv->sb_lock);
> > - hpll_freq = vlv_cck_read(dev_priv, CCK_FUSE_REG) &
> > - CCK_FUSE_HPLL_FREQ_MASK;
> > - mutex_unlock(&dev_priv->sb_lock);
> > -
> > - return vco_freq[hpll_freq] * 1000;
> > -}
> > -
> > /* Adjust CDclk dividers to allow high res or save power if possible */
> > static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> > {
> > @@ -5983,7 +6016,7 @@ static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> > else
> > default_credits = PFI_CREDIT(8);
> >
> > - if (DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 1000) >= dev_priv->rps.cz_freq) {
> > + if (dev_priv->cdclk_freq >= dev_priv->czclk_freq) {
> > /* CHV suggested value is 31 or 63 */
> > if (IS_CHERRYVIEW(dev_priv))
> > credits = PFI_CREDIT_63;
> > @@ -6715,24 +6748,8 @@ static int haswell_get_display_clock_speed(struct drm_device *dev)
> >
> > static int valleyview_get_display_clock_speed(struct drm_device *dev)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - u32 val;
> > - int divider;
> > -
> > - if (dev_priv->hpll_freq == 0)
> > - dev_priv->hpll_freq = valleyview_get_vco(dev_priv);
> > -
> > - mutex_lock(&dev_priv->sb_lock);
> > - val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
> > - mutex_unlock(&dev_priv->sb_lock);
> > -
> > - divider = val & CCK_FREQUENCY_VALUES;
> > -
> > - WARN((val & CCK_FREQUENCY_STATUS) !=
> > - (divider << CCK_FREQUENCY_STATUS_SHIFT),
> > - "cdclk change in progress\n");
> > -
> > - return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
> > + return vlv_get_cck_clock_hpll(to_i915(dev), "cdclk",
> > + CCK_DISPLAY_CLOCK_CONTROL);
> > }
> >
> > static int ilk_get_display_clock_speed(struct drm_device *dev)
> > @@ -13323,8 +13340,6 @@ static void intel_shared_dpll_init(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > - intel_update_cdclk(dev);
> > -
> > if (HAS_DDI(dev))
> > intel_ddi_pll_init(dev);
> > else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
> > @@ -14836,6 +14851,9 @@ void intel_modeset_init(struct drm_device *dev)
> > }
> > }
> >
> > + intel_update_czclk(dev_priv);
> > + intel_update_cdclk(dev);
> > +
> > intel_shared_dpll_init(dev);
> >
> > /* Just disable it once at startup */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ab5ac5e..896a95c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5482,25 +5482,10 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
> > mutex_unlock(&dev_priv->sb_lock);
> >
> > switch ((val >> 2) & 0x7) {
> > - case 0:
> > - case 1:
> > - dev_priv->rps.cz_freq = 200;
> > - dev_priv->mem_freq = 1600;
> > - break;
> > - case 2:
> > - dev_priv->rps.cz_freq = 267;
> > - dev_priv->mem_freq = 1600;
> > - break;
> > case 3:
> > - dev_priv->rps.cz_freq = 333;
> > dev_priv->mem_freq = 2000;
> > break;
> > - case 4:
> > - dev_priv->rps.cz_freq = 320;
> > - dev_priv->mem_freq = 1600;
> > - break;
> > - case 5:
> > - dev_priv->rps.cz_freq = 400;
> > + default:
> > dev_priv->mem_freq = 1600;
> > break;
> > }
> > @@ -7327,7 +7312,7 @@ static int vlv_gpu_freq_div(unsigned int czclk_freq)
> >
> > static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> > {
> > - int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->mem_freq, 4);
> > + int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> >
> > div = vlv_gpu_freq_div(czclk_freq);
> > if (div < 0)
> > @@ -7338,7 +7323,7 @@ static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >
> > static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
> > {
> > - int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->mem_freq, 4);
> > + int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> >
> > mul = vlv_gpu_freq_div(czclk_freq);
> > if (mul < 0)
> > @@ -7349,7 +7334,7 @@ static int byt_freq_opcode(struct drm_i915_private *dev_priv, int val)
> >
> > static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
> > {
> > - int div, czclk_freq = dev_priv->rps.cz_freq;
> > + int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> >
> > div = vlv_gpu_freq_div(czclk_freq) / 2;
> > if (div < 0)
> > @@ -7360,7 +7345,7 @@ static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >
> > static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
> > {
> > - int mul, czclk_freq = dev_priv->rps.cz_freq;
> > + int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
> >
> > mul = vlv_gpu_freq_div(czclk_freq) / 2;
> > if (mul < 0)
>
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
