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

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

On Tue, Feb 26, 2019 at 11:15:44AM -0800, Lucas De Marchi wrote:
> 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@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.

There is always pending work somewhere. We can't stop for that. Also
if there's more of this stuff about to be submitted upstream we 
should clean it up before it lands.

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

The first part is already sorted by CI checking the whole thing.

The second part should be easy to fix.

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

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