| Message ID | 20190826225540.11987-2-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New cdclk values for gen11+ | expand |
On Mon, Aug 26, 2019 at 03:55:39PM -0700, Matt Roper wrote: > The bspec has just recently been updated with new cdclk values that > require the use of a /2 CD2X divider rather than a /1 divider. Once we > add the divider selection logic to ICL+ cdclk programming, we have > pretty much the same logic we were already using on CNL, so it's simpler > to drop icl_set_cdclk() completely and reuse cnl_set_cdclk() on gen11+ > platforms as well. > > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 90 +++++++++------------- > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 36 insertions(+), 55 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 939088c7d814..a56ccd0930e0 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -1659,10 +1659,23 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv, > cnl_cdclk_pll_enable(dev_priv, vco); > > val = divider | skl_cdclk_decimal(cdclk); > - if (pipe == INVALID_PIPE) > - val |= BXT_CDCLK_CD2X_PIPE_NONE; > - else > - val |= BXT_CDCLK_CD2X_PIPE(pipe); > + > + if (INTEL_GEN(dev_priv) >= 12) { > + if (pipe == INVALID_PIPE) > + val |= ICL_CDCLK_CD2X_PIPE_NONE; > + else > + val |= BXT_CDCLK_CD2X_PIPE(pipe); Hmm. So the mess made here with icl is now fixed. Cool. Sad they didn't fix it on icl as well. However I have a feeling this may end up confusing people, so maybe we should just add TGL_CDCLK_CD2X_PIPE()? > + } else if (INTEL_GEN(dev_priv) >= 11) { > + if (pipe == INVALID_PIPE) > + val |= ICL_CDCLK_CD2X_PIPE_NONE; > + else > + val |= ICL_CDCLK_CD2X_PIPE(pipe); > + } else { > + if (pipe == INVALID_PIPE) > + val |= BXT_CDCLK_CD2X_PIPE_NONE; > + else > + val |= BXT_CDCLK_CD2X_PIPE(pipe); > + } > I915_WRITE(CDCLK_CTL, val); > > if (pipe != INVALID_PIPE) > @@ -1813,51 +1826,6 @@ static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk) > return dev_priv->cdclk.hw.ref * ratio; > } > > -static void icl_set_cdclk(struct drm_i915_private *dev_priv, > - const struct intel_cdclk_state *cdclk_state, > - enum pipe pipe) > -{ > - unsigned int cdclk = cdclk_state->cdclk; > - unsigned int vco = cdclk_state->vco; > - int ret; > - > - ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL, > - SKL_CDCLK_PREPARE_FOR_CHANGE, > - SKL_CDCLK_READY_FOR_CHANGE, > - SKL_CDCLK_READY_FOR_CHANGE, 3); > - if (ret) { > - DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n", > - ret); > - return; > - } > - > - if (dev_priv->cdclk.hw.vco != 0 && > - dev_priv->cdclk.hw.vco != vco) > - cnl_cdclk_pll_disable(dev_priv); > - > - if (dev_priv->cdclk.hw.vco != vco) > - cnl_cdclk_pll_enable(dev_priv, vco); > - > - /* > - * On ICL CD2X_DIV can only be 1, so we'll never end up changing the > - * divider here synchronized to a pipe while CDCLK is on, nor will we > - * need the corresponding vblank wait. > - */ > - I915_WRITE(CDCLK_CTL, ICL_CDCLK_CD2X_PIPE_NONE | > - skl_cdclk_decimal(cdclk)); > - > - sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, > - cdclk_state->voltage_level); > - > - intel_update_cdclk(dev_priv); > - > - /* > - * Can't read out the voltage level :( > - * Let's just assume everything is as expected. > - */ > - dev_priv->cdclk.hw.voltage_level = cdclk_state->voltage_level; > -} > - > static u8 icl_calc_voltage_level(struct drm_i915_private *dev_priv, int cdclk) > { > if (IS_ELKHARTLAKE(dev_priv)) { > @@ -1881,6 +1849,7 @@ static void icl_get_cdclk(struct drm_i915_private *dev_priv, > struct intel_cdclk_state *cdclk_state) > { > u32 val; > + int div; > > cdclk_state->bypass = 50000; > > @@ -1914,10 +1883,21 @@ static void icl_get_cdclk(struct drm_i915_private *dev_priv, > > cdclk_state->vco = (val & BXT_DE_PLL_RATIO_MASK) * cdclk_state->ref; > > - val = I915_READ(CDCLK_CTL); > - WARN_ON((val & BXT_CDCLK_CD2X_DIV_SEL_MASK) != 0); > + val = I915_READ(CDCLK_CTL) & BXT_CDCLK_CD2X_DIV_SEL_MASK; > + switch (val) { > + case BXT_CDCLK_CD2X_DIV_SEL_1: > + div = 2; > + break; > + case BXT_CDCLK_CD2X_DIV_SEL_2: > + div = 4; > + break; > + default: > + MISSING_CASE(val); > + div = 2; > + break; > + } > > - cdclk_state->cdclk = cdclk_state->vco / 2; > + cdclk_state->cdclk = cdclk_state->vco / div; cnl uses DIV_ROUND_CLOSEST(). In general this function looks rather different to cnl_get_cdclk() even though the actual differences are minimal. Some unification of style might be nice. Not sure if reusing the cnl function on icl might actually make sense. Would require a few ifs. > > out: > /* > @@ -1963,7 +1943,7 @@ static void icl_init_cdclk(struct drm_i915_private *dev_priv) > icl_calc_voltage_level(dev_priv, > sanitized_state.cdclk); > > - icl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE); > + cnl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE); > } > > static void icl_uninit_cdclk(struct drm_i915_private *dev_priv) > @@ -1975,7 +1955,7 @@ static void icl_uninit_cdclk(struct drm_i915_private *dev_priv) > cdclk_state.voltage_level = icl_calc_voltage_level(dev_priv, > cdclk_state.cdclk); > > - icl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); > + cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); > } > > static void cnl_init_cdclk(struct drm_i915_private *dev_priv) > @@ -2810,7 +2790,7 @@ void intel_update_rawclk(struct drm_i915_private *dev_priv) > void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) > { > if (INTEL_GEN(dev_priv) >= 11) { > - dev_priv->display.set_cdclk = icl_set_cdclk; > + dev_priv->display.set_cdclk = cnl_set_cdclk; > dev_priv->display.modeset_calc_cdclk = icl_modeset_calc_cdclk; > } else if (IS_CANNONLAKE(dev_priv)) { > dev_priv->display.set_cdclk = cnl_set_cdclk; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index a142aa3d74e3..958dfdfb4e10 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -9671,6 +9671,7 @@ enum skl_power_gate { > #define BXT_CDCLK_CD2X_PIPE(pipe) ((pipe) << 20) > #define CDCLK_DIVMUX_CD_OVERRIDE (1 << 19) > #define BXT_CDCLK_CD2X_PIPE_NONE BXT_CDCLK_CD2X_PIPE(3) > +#define ICL_CDCLK_CD2X_PIPE(pipe) (_PICK(pipe, 0, 2, 6) << 19) Could also do eg. ((((pipe) << 1) | ((pipe) & ~1)) << 19) if we want to avoid the _PICK(). Doesn't really matter I suppose. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > #define ICL_CDCLK_CD2X_PIPE_NONE (7 << 19) > #define BXT_CDCLK_SSA_PRECHARGE_ENABLE (1 << 16) > #define CDCLK_FREQ_DECIMAL_MASK (0x7ff) > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index 939088c7d814..a56ccd0930e0 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -1659,10 +1659,23 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv, cnl_cdclk_pll_enable(dev_priv, vco); val = divider | skl_cdclk_decimal(cdclk); - if (pipe == INVALID_PIPE) - val |= BXT_CDCLK_CD2X_PIPE_NONE; - else - val |= BXT_CDCLK_CD2X_PIPE(pipe); + + if (INTEL_GEN(dev_priv) >= 12) { + if (pipe == INVALID_PIPE) + val |= ICL_CDCLK_CD2X_PIPE_NONE; + else + val |= BXT_CDCLK_CD2X_PIPE(pipe); + } else if (INTEL_GEN(dev_priv) >= 11) { + if (pipe == INVALID_PIPE) + val |= ICL_CDCLK_CD2X_PIPE_NONE; + else + val |= ICL_CDCLK_CD2X_PIPE(pipe); + } else { + if (pipe == INVALID_PIPE) + val |= BXT_CDCLK_CD2X_PIPE_NONE; + else + val |= BXT_CDCLK_CD2X_PIPE(pipe); + } I915_WRITE(CDCLK_CTL, val); if (pipe != INVALID_PIPE) @@ -1813,51 +1826,6 @@ static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk) return dev_priv->cdclk.hw.ref * ratio; } -static void icl_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state, - enum pipe pipe) -{ - unsigned int cdclk = cdclk_state->cdclk; - unsigned int vco = cdclk_state->vco; - int ret; - - ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL, - SKL_CDCLK_PREPARE_FOR_CHANGE, - SKL_CDCLK_READY_FOR_CHANGE, - SKL_CDCLK_READY_FOR_CHANGE, 3); - if (ret) { - DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n", - ret); - return; - } - - if (dev_priv->cdclk.hw.vco != 0 && - dev_priv->cdclk.hw.vco != vco) - cnl_cdclk_pll_disable(dev_priv); - - if (dev_priv->cdclk.hw.vco != vco) - cnl_cdclk_pll_enable(dev_priv, vco); - - /* - * On ICL CD2X_DIV can only be 1, so we'll never end up changing the - * divider here synchronized to a pipe while CDCLK is on, nor will we - * need the corresponding vblank wait. - */ - I915_WRITE(CDCLK_CTL, ICL_CDCLK_CD2X_PIPE_NONE | - skl_cdclk_decimal(cdclk)); - - sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, - cdclk_state->voltage_level); - - intel_update_cdclk(dev_priv); - - /* - * Can't read out the voltage level :( - * Let's just assume everything is as expected. - */ - dev_priv->cdclk.hw.voltage_level = cdclk_state->voltage_level; -} - static u8 icl_calc_voltage_level(struct drm_i915_private *dev_priv, int cdclk) { if (IS_ELKHARTLAKE(dev_priv)) { @@ -1881,6 +1849,7 @@ static void icl_get_cdclk(struct drm_i915_private *dev_priv, struct intel_cdclk_state *cdclk_state) { u32 val; + int div; cdclk_state->bypass = 50000; @@ -1914,10 +1883,21 @@ static void icl_get_cdclk(struct drm_i915_private *dev_priv, cdclk_state->vco = (val & BXT_DE_PLL_RATIO_MASK) * cdclk_state->ref; - val = I915_READ(CDCLK_CTL); - WARN_ON((val & BXT_CDCLK_CD2X_DIV_SEL_MASK) != 0); + val = I915_READ(CDCLK_CTL) & BXT_CDCLK_CD2X_DIV_SEL_MASK; + switch (val) { + case BXT_CDCLK_CD2X_DIV_SEL_1: + div = 2; + break; + case BXT_CDCLK_CD2X_DIV_SEL_2: + div = 4; + break; + default: + MISSING_CASE(val); + div = 2; + break; + } - cdclk_state->cdclk = cdclk_state->vco / 2; + cdclk_state->cdclk = cdclk_state->vco / div; out: /* @@ -1963,7 +1943,7 @@ static void icl_init_cdclk(struct drm_i915_private *dev_priv) icl_calc_voltage_level(dev_priv, sanitized_state.cdclk); - icl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE); + cnl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE); } static void icl_uninit_cdclk(struct drm_i915_private *dev_priv) @@ -1975,7 +1955,7 @@ static void icl_uninit_cdclk(struct drm_i915_private *dev_priv) cdclk_state.voltage_level = icl_calc_voltage_level(dev_priv, cdclk_state.cdclk); - icl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); + cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); } static void cnl_init_cdclk(struct drm_i915_private *dev_priv) @@ -2810,7 +2790,7 @@ void intel_update_rawclk(struct drm_i915_private *dev_priv) void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) { if (INTEL_GEN(dev_priv) >= 11) { - dev_priv->display.set_cdclk = icl_set_cdclk; + dev_priv->display.set_cdclk = cnl_set_cdclk; dev_priv->display.modeset_calc_cdclk = icl_modeset_calc_cdclk; } else if (IS_CANNONLAKE(dev_priv)) { dev_priv->display.set_cdclk = cnl_set_cdclk; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index a142aa3d74e3..958dfdfb4e10 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -9671,6 +9671,7 @@ enum skl_power_gate { #define BXT_CDCLK_CD2X_PIPE(pipe) ((pipe) << 20) #define CDCLK_DIVMUX_CD_OVERRIDE (1 << 19) #define BXT_CDCLK_CD2X_PIPE_NONE BXT_CDCLK_CD2X_PIPE(3) +#define ICL_CDCLK_CD2X_PIPE(pipe) (_PICK(pipe, 0, 2, 6) << 19) #define ICL_CDCLK_CD2X_PIPE_NONE (7 << 19) #define BXT_CDCLK_SSA_PRECHARGE_ENABLE (1 << 16) #define CDCLK_FREQ_DECIMAL_MASK (0x7ff)
The bspec has just recently been updated with new cdclk values that require the use of a /2 CD2X divider rather than a /1 divider. Once we add the divider selection logic to ICL+ cdclk programming, we have pretty much the same logic we were already using on CNL, so it's simpler to drop icl_set_cdclk() completely and reuse cnl_set_cdclk() on gen11+ platforms as well. Cc: José Roberto de Souza <jose.souza@intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/display/intel_cdclk.c | 90 +++++++++------------- drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 36 insertions(+), 55 deletions(-)