Re: [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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.

> 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.

> 
> >This patch looks good to me. It'll conflict with my series though, but
> >no biggie.
> >Reviewed-by: Ville Syrjälä <ville.syrjala@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@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >-- 
> >Ville Syrjälä
> >Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx