[Intel-gfx] [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout
Lucas De Marchi
lucas.demarchi at intel.com
Tue Feb 26 19:15:44 UTC 2019
On Tue, Feb 26, 2019 at 04:21:01PM +0200, Ville Syrjälä wrote:
>On Mon, Feb 25, 2019 at 04:03:05PM -0800, Lucas De Marchi wrote:
>> On Mon, Feb 25, 2019 at 10:42:12PM +0200, Ville Syrjälä wrote:
>> >On Fri, Feb 22, 2019 at 03:23:22PM -0800, Lucas De Marchi wrote:
>> >> Let the MG plls have their own hooks since it shares very little with
>> >> other PLL types. It's also better so the platform info contains the info
>> >> if the PLL is for MG PHY rather than relying on the PLL ids.
>> >>
>> >> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> >
>> >There's quite a bit more cleanup to be done in this area. As a start
>> >https://patchwork.freedesktop.org/series/56354/ ;)
>>
>> I started reviewing that and even gave my r-b in some patches - first
>> patches cause several conflicts with in-flight patches though. Not sure
>> how beneficial they are.
>
>The current code is quite messy, and thus hard to follow.
>It should be cleaned up before we pile even more stuff on top.
>People had already cargo culted all the crud from the older
>implementations to the later ones.
>
>We can't let the dirt accumulate indefinitely because eventually
>the point comes where the thing either collapses under its own weight
>or someone just decides to not even look at the previous code and
>does the new thing totally differently. And that approach does not
>scale because then everyone has to keep in mind N different ways
>of doing the exact same thing. And also all the learnings from the
>old code are forgotten and we probably get more bugs as a result.
I agree with all of that. I'm talking about those particular changes and
the timing with the pending work going upstream, not generically.
>> IMO last 3 patches could be standalone
>> (particularly the one that is a bug fix, and doesn't depend on the
>> previous ones)
>
>Nothing prevents the patches from being reviewed/merged out of order.
CI running on them and they taking more time to be merged waiting for
the other patches to be reviewed.
Lucas De Marchi
>
>>
>> >This patch looks good to me. It'll conflict with my series though, but
>> >no biggie.
>> >Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>
>> thanks
>> Lucas De Marchi
>>
>> >
>> >> ---
>> >> drivers/gpu/drm/i915/intel_dpll_mgr.c | 122 ++++++++++++++++----------
>> >> 1 file changed, 74 insertions(+), 48 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> >> index 0a42d11c4c33..e4ec73d415d9 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> >> @@ -2966,6 +2966,68 @@ static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
>> >> return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
>> >> }
>> >>
>> >> +static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> >> + struct intel_shared_dpll *pll,
>> >> + struct intel_dpll_hw_state *hw_state)
>> >> +{
>> >> + const enum intel_dpll_id id = pll->info->id;
>> >> + enum tc_port tc_port = icl_pll_id_to_tc_port(id);
>> >> + intel_wakeref_t wakeref;
>> >> + bool ret = false;
>> >> + u32 val;
>> >> +
>> >> + wakeref = intel_display_power_get_if_enabled(dev_priv,
>> >> + POWER_DOMAIN_PLLS);
>> >> + if (!wakeref)
>> >> + return false;
>> >> +
>> >> + val = I915_READ(MG_PLL_ENABLE(tc_port));
>> >> + if (!(val & PLL_ENABLE))
>> >> + goto out;
>> >> +
>> >> + hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
>> >> + hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
>> >> +
>> >> + hw_state->mg_clktop2_coreclkctl1 =
>> >> + I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
>> >> + hw_state->mg_clktop2_coreclkctl1 &=
>> >> + MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
>> >> +
>> >> + hw_state->mg_clktop2_hsclkctl =
>> >> + I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
>> >> + hw_state->mg_clktop2_hsclkctl &=
>> >> + MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
>> >> + MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
>> >> + MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
>> >> + MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
>> >> +
>> >> + hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
>> >> + hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
>> >> + hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
>> >> + hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
>> >> + hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
>> >> +
>> >> + hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
>> >> + hw_state->mg_pll_tdc_coldst_bias =
>> >> + I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
>> >> +
>> >> + if (dev_priv->cdclk.hw.ref == 38400) {
>> >> + hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
>> >> + hw_state->mg_pll_bias_mask = 0;
>> >> + } else {
>> >> + hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
>> >> + hw_state->mg_pll_bias_mask = -1U;
>> >> + }
>> >> +
>> >> + hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
>> >> + hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
>> >> +
>> >> + ret = true;
>> >> +out:
>> >> + intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
>> >> + return ret;
>> >> +}
>> >> +
>> >> static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> >> struct intel_shared_dpll *pll,
>> >> struct intel_dpll_hw_state *hw_state)
>> >> @@ -2984,50 +3046,8 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> >> if (!(val & PLL_ENABLE))
>> >> goto out;
>> >>
>> >> - if (intel_dpll_is_combophy(id) ||
>> >> - id == DPLL_ID_ICL_TBTPLL) {
>> >> - hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
>> >> - hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
>> >> - } else {
>> >> - enum tc_port tc_port = icl_pll_id_to_tc_port(id);
>> >> -
>> >> - hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
>> >> - hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
>> >> -
>> >> - hw_state->mg_clktop2_coreclkctl1 =
>> >> - I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
>> >> - hw_state->mg_clktop2_coreclkctl1 &=
>> >> - MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
>> >> -
>> >> - hw_state->mg_clktop2_hsclkctl =
>> >> - I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
>> >> - hw_state->mg_clktop2_hsclkctl &=
>> >> - MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
>> >> - MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
>> >> - MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
>> >> - MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
>> >> -
>> >> - hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
>> >> - hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
>> >> - hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
>> >> - hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
>> >> - hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
>> >> -
>> >> - hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
>> >> - hw_state->mg_pll_tdc_coldst_bias =
>> >> - I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
>> >> -
>> >> - if (dev_priv->cdclk.hw.ref == 38400) {
>> >> - hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
>> >> - hw_state->mg_pll_bias_mask = 0;
>> >> - } else {
>> >> - hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
>> >> - hw_state->mg_pll_bias_mask = -1U;
>> >> - }
>> >> -
>> >> - hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
>> >> - hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
>> >> - }
>> >> + hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
>> >> + hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
>> >>
>> >> ret = true;
>> >> out:
>> >> @@ -3203,14 +3223,20 @@ static const struct intel_shared_dpll_funcs icl_pll_funcs = {
>> >> .get_hw_state = icl_pll_get_hw_state,
>> >> };
>> >>
>> >> +static const struct intel_shared_dpll_funcs mg_pll_funcs = {
>> >> + .enable = icl_pll_enable,
>> >> + .disable = icl_pll_disable,
>> >> + .get_hw_state = mg_pll_get_hw_state,
>> >> +};
>> >> +
>> >> static const struct dpll_info icl_plls[] = {
>> >> { "DPLL 0", &icl_pll_funcs, DPLL_ID_ICL_DPLL0, 0 },
>> >> { "DPLL 1", &icl_pll_funcs, DPLL_ID_ICL_DPLL1, 0 },
>> >> { "TBT PLL", &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
>> >> - { "MG PLL 1", &icl_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
>> >> - { "MG PLL 2", &icl_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
>> >> - { "MG PLL 3", &icl_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
>> >> - { "MG PLL 4", &icl_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
>> >> + { "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
>> >> + { "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
>> >> + { "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
>> >> + { "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
>> >> { },
>> >> };
>> >>
>> >> --
>> >> 2.20.0
>> >>
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx at lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>--
>Ville Syrjälä
>Intel
More information about the Intel-gfx
mailing list