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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
