Re: [PATCH 3/3] drm/i915/icl: decouple dpll ids from type

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

On Fri, Feb 22, 2019 at 03:23:24PM -0800, Lucas De Marchi wrote:
> Use the first 3 bits of dpll_info.platform_flags to mark the type of the
> PLL instead of relying on the IDs. This is more future proof for
> allowing the same set of functions to be reused, even if the IDs change.
> 
> The warning about PLL id not corresponding to a combo phy in intel_display
> was removed as I don't think it should ever trigger.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  3 --
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 54 +++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  1 -
>  3 files changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b1d63c32ca94..a2be35118dd5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9592,9 +9592,6 @@ static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
>  		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
>  		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
>  		id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> -
> -		if (WARN_ON(!intel_dpll_is_combophy(id)))
> -			return;
>  	} else if (intel_port_is_tc(dev_priv, port)) {
>  		id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv, port));
>  	} else {
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index e4ec73d415d9..cdb4463bab5d 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2639,6 +2639,13 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>  	return link_clock;
>  }
>  
> +enum icl_dpll_flags {
> +	ICL_DPLL_TYPE_COMBOPHY,
> +	ICL_DPLL_TYPE_TBT,
> +	ICL_DPLL_TYPE_MG,
> +	_ICL_DPLL_TYPE_MASK = 0x7,
> +};
> +
>  static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
>  {
>  	return id - DPLL_ID_ICL_MGPLL1;
> @@ -2649,9 +2656,9 @@ enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port)
>  	return tc_port + DPLL_ID_ICL_MGPLL1;
>  }
>  
> -bool intel_dpll_is_combophy(enum intel_dpll_id id)
> +static enum icl_dpll_flags icl_dpll_type(const struct dpll_info *info)
>  {
> -	return id == DPLL_ID_ICL_DPLL0 || id == DPLL_ID_ICL_DPLL1;
> +	return info->platform_flags & _ICL_DPLL_TYPE_MASK;
>  }
>  
>  static bool icl_mg_pll_find_divisors(int clock_khz, bool is_dp, bool use_ssc,
> @@ -2956,14 +2963,22 @@ icl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  	return pll;
>  }
>  
> -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> +static i915_reg_t
> +icl_pll_info_to_enable_reg(const struct dpll_info *info)
>  {
> -	if (intel_dpll_is_combophy(id))
> -		return CNL_DPLL_ENABLE(id);
> -	else if (id == DPLL_ID_ICL_TBTPLL)
> -		return TBT_PLL_ENABLE;
> +	enum icl_dpll_flags type = icl_dpll_type(info);
>  
> -	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
> +	switch (type) {
> +	case ICL_DPLL_TYPE_COMBOPHY:
> +		return CNL_DPLL_ENABLE(info->id);
> +	case ICL_DPLL_TYPE_TBT:
> +		return TBT_PLL_ENABLE;
> +	case ICL_DPLL_TYPE_MG:
> +		return MG_PLL_ENABLE(icl_pll_id_to_tc_port(info->id));
> +	default:
> +		MISSING_CASE(type);
> +		return CNL_DPLL_ENABLE(info->id);
> +	}
>  }

This seems a rather roundabout way of doing things when we already have
the vfuncs.

How about just:

mg_pll_enable()
{
	icl_pll_enable(MG_REG);
}

combo_pll_enable()
{
	icl_pll_enable(COMBO_REG);
}

or something along those lines.


>  
>  static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
> @@ -3042,7 +3057,7 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>  	if (!wakeref)
>  		return false;
>  
> -	val = I915_READ(icl_pll_id_to_enable_reg(id));
> +	val = I915_READ(icl_pll_info_to_enable_reg(pll->info));
>  	if (!(val & PLL_ENABLE))
>  		goto out;
>  
> @@ -3120,7 +3135,8 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>  			   struct intel_shared_dpll *pll)
>  {
>  	const enum intel_dpll_id id = pll->info->id;
> -	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> +	enum icl_dpll_flags type = icl_dpll_type(pll->info);
> +	i915_reg_t enable_reg = icl_pll_info_to_enable_reg(pll->info);
>  	u32 val;
>  
>  	val = I915_READ(enable_reg);
> @@ -3135,7 +3151,7 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>  				    PLL_POWER_STATE, 1))
>  		DRM_ERROR("PLL %d Power not enabled\n", id);
>  
> -	if (intel_dpll_is_combophy(id) || id == DPLL_ID_ICL_TBTPLL)
> +	if (type == ICL_DPLL_TYPE_COMBOPHY || type == ICL_DPLL_TYPE_TBT)
>  		icl_dpll_write(dev_priv, pll);
>  	else
>  		icl_mg_pll_write(dev_priv, pll);
> @@ -3161,7 +3177,7 @@ static void icl_pll_disable(struct drm_i915_private *dev_priv,
>  			    struct intel_shared_dpll *pll)
>  {
>  	const enum intel_dpll_id id = pll->info->id;
> -	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> +	i915_reg_t enable_reg = icl_pll_info_to_enable_reg(pll->info);
>  	u32 val;
>  
>  	/* The first steps are done by intel_ddi_post_disable(). */
> @@ -3230,13 +3246,13 @@ static const struct intel_shared_dpll_funcs mg_pll_funcs = {
>  };
>  
>  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", &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 },
> +	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0, 0, ICL_DPLL_TYPE_COMBOPHY },
> +	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1, 0, ICL_DPLL_TYPE_COMBOPHY },
> +	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0, ICL_DPLL_TYPE_TBT },
> +	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0, ICL_DPLL_TYPE_MG },
> +	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0, ICL_DPLL_TYPE_MG },
> +	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0, ICL_DPLL_TYPE_MG },
> +	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0, ICL_DPLL_TYPE_MG },
>  	{ },
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index b1fdce1be942..12ffe83598aa 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -352,6 +352,5 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>  			       u32 pll_id);
>  int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
>  enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port);
> -bool intel_dpll_is_combophy(enum intel_dpll_id id);
>  
>  #endif /* _INTEL_DPLL_MGR_H_ */
> -- 
> 2.20.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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