Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Sep 01, 2020 at 11:27:58AM -0700, Anusha Srivatsa wrote:
> We currenty check for platform at multiple parts in the driver
> to grab the correct PLL. Let us begin to centralize it through a
> helper function.
> 
> v2: s/intel_get_pll_enable_reg()/intel_combo_pll_enable_reg() (Ville)
> 
> Suggested-by: Matt Roper <matthew.d.roper@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 25 +++++++++++--------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index c9013f8f766f..7440836c5e44 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -147,6 +147,18 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  			pll->info->name, onoff(state), onoff(cur_state));
>  }
>  
> +static
> +i915_reg_t intel_combo_pll_enable_reg(struct drm_i915_private *dev_priv,
> +				    struct intel_shared_dpll *pll)
> +{
> +
> +	if (IS_ELKHARTLAKE(dev_priv) && (pll->info->id == DPLL_ID_EHL_DPLL4))
> +			return MG_PLL_ENABLE(0);
> +
> +	return CNL_DPLL_ENABLE(pll->info->id);
> +
> +
> +}
>  /**
>   * intel_prepare_shared_dpll - call a dpll's prepare hook
>   * @crtc_state: CRTC, and its state, which has a shared dpll
> @@ -3842,12 +3854,7 @@ static bool combo_pll_get_hw_state(struct drm_i915_private *dev_priv,
>  				   struct intel_shared_dpll *pll,
>  				   struct intel_dpll_hw_state *hw_state)
>  {
> -	i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> -
> -	if (IS_ELKHARTLAKE(dev_priv) &&
> -	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> -		enable_reg = MG_PLL_ENABLE(0);
> -	}
> +	i915_reg_t enable_reg = intel_combo_pll_enable_reg(dev_priv, pll);
>  
>  	return icl_pll_get_hw_state(dev_priv, pll, hw_state, enable_reg);
>  }
> @@ -4045,11 +4052,10 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>  static void combo_pll_enable(struct drm_i915_private *dev_priv,
>  			     struct intel_shared_dpll *pll)
>  {
> -	i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> +	i915_reg_t enable_reg = intel_combo_pll_enable_reg(dev_priv, pll);
>  
>  	if (IS_ELKHARTLAKE(dev_priv) &&
>  	    pll->info->id == DPLL_ID_EHL_DPLL4) {

there's probably something else that we can do now with the power_{put,get}
to get rid of the, now, doubled if checks...

> -		enable_reg = MG_PLL_ENABLE(0);
>  
>  		/*
>  		 * We need to disable DC states when this DPLL is enabled.
> @@ -4157,11 +4163,10 @@ static void icl_pll_disable(struct drm_i915_private *dev_priv,
>  static void combo_pll_disable(struct drm_i915_private *dev_priv,
>  			      struct intel_shared_dpll *pll)
>  {
> -	i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> +	i915_reg_t enable_reg = intel_combo_pll_enable_reg(dev_priv, pll);
>  
>  	if (IS_ELKHARTLAKE(dev_priv) &&
>  	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> -		enable_reg = MG_PLL_ENABLE(0);
>  		icl_pll_disable(dev_priv, pll, enable_reg);

but here, at least, let's clean this function now...
move this call above and out of the if and delete the one below
and keep just the power_put inside the if...

>  
>  		intel_display_power_put(dev_priv, POWER_DOMAIN_DPLL_DC_OFF,
> -- 
> 2.25.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx