diff mbox series

[2/5] drm/i915/icl: use a function pointer for pll_write when enabling

Message ID 20190306012636.18619-3-lucas.demarchi@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/icl: split pll functions | expand

Commit Message

Lucas De Marchi March 6, 2019, 1:26 a.m. UTC 
This allows us to share the icl_pll_enable() between the different types
of PLL while allowing the caller to differentiate how to write the
registers.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Ville Syrjälä March 8, 2019, 8:19 p.m. UTC | #1 
On Tue, Mar 05, 2019 at 05:26:33PM -0800, Lucas De Marchi wrote:
> This allows us to share the icl_pll_enable() between the different types
> of PLL while allowing the caller to differentiate how to write the
> registers.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 3b3de99756d6..5511bc23ea3d 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -3118,9 +3118,10 @@ static void icl_mg_pll_write(struct drm_i915_private *dev_priv,
>  
>  static void icl_pll_enable(struct drm_i915_private *dev_priv,
>  			   struct intel_shared_dpll *pll,
> -			   i915_reg_t enable_reg)
> +			   i915_reg_t enable_reg,
> +			   void (*pll_write)(struct drm_i915_private *dev_priv,
> +					     struct intel_shared_dpll *pll))
>  {
> -	const enum intel_dpll_id id = pll->info->id;
>  	u32 val;
>  
>  	val = I915_READ(enable_reg);
> @@ -3133,12 +3134,9 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>  	 */
>  	if (intel_wait_for_register(dev_priv, enable_reg, PLL_POWER_STATE,
>  				    PLL_POWER_STATE, 1))
> -		DRM_ERROR("PLL %d Power not enabled\n", id);
> +		DRM_ERROR("PLL %d Power not enabled\n", pll->info->id);
>  
> -	if (intel_dpll_is_combophy(id) || id == DPLL_ID_ICL_TBTPLL)
> -		icl_dpll_write(dev_priv, pll);
> -	else
> -		icl_mg_pll_write(dev_priv, pll);
> +	pll_write(dev_priv, pll);

Hmm. Would it be cleaner to just exract the pll power
enable/disable and pll enable/disable parts into small helpers?
It looks like like glk/cnl also follow this same pattern, so
there may be a chance to reuse the code on those platforms
as well.

>  
>  	/*
>  	 * DVFS pre sequence would be here, but in our driver the cdclk code
> @@ -3152,7 +3150,7 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>  
>  	if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK,
>  				    1)) /* 600us actually. */
> -		DRM_ERROR("PLL %d not locked\n", id);
> +		DRM_ERROR("PLL %d not locked\n", pll->info->id);
>  
>  	/* DVFS post sequence would be here. See the comment above. */
>  }
> @@ -3162,7 +3160,7 @@ static void combo_pll_enable(struct drm_i915_private *dev_priv,
>  {
>  	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(pll->info->id);
>  
> -	icl_pll_enable(dev_priv, pll, enable_reg);
> +	icl_pll_enable(dev_priv, pll, enable_reg, icl_dpll_write);
>  }
>  
>  static void mg_pll_enable(struct drm_i915_private *dev_priv,
> @@ -3171,7 +3169,7 @@ static void mg_pll_enable(struct drm_i915_private *dev_priv,
>  	i915_reg_t enable_reg =
>  		MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id));
>  
> -	icl_pll_enable(dev_priv, pll, enable_reg);
> +	icl_pll_enable(dev_priv, pll, enable_reg, icl_mg_pll_write);
>  }
>  
>  static void icl_pll_disable(struct drm_i915_private *dev_priv,
> -- 
> 2.20.1
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 3b3de99756d6..5511bc23ea3d 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -3118,9 +3118,10 @@  static void icl_mg_pll_write(struct drm_i915_private *dev_priv,
 
 static void icl_pll_enable(struct drm_i915_private *dev_priv,
 			   struct intel_shared_dpll *pll,
-			   i915_reg_t enable_reg)
+			   i915_reg_t enable_reg,
+			   void (*pll_write)(struct drm_i915_private *dev_priv,
+					     struct intel_shared_dpll *pll))
 {
-	const enum intel_dpll_id id = pll->info->id;
 	u32 val;
 
 	val = I915_READ(enable_reg);
@@ -3133,12 +3134,9 @@  static void icl_pll_enable(struct drm_i915_private *dev_priv,
 	 */
 	if (intel_wait_for_register(dev_priv, enable_reg, PLL_POWER_STATE,
 				    PLL_POWER_STATE, 1))
-		DRM_ERROR("PLL %d Power not enabled\n", id);
+		DRM_ERROR("PLL %d Power not enabled\n", pll->info->id);
 
-	if (intel_dpll_is_combophy(id) || id == DPLL_ID_ICL_TBTPLL)
-		icl_dpll_write(dev_priv, pll);
-	else
-		icl_mg_pll_write(dev_priv, pll);
+	pll_write(dev_priv, pll);
 
 	/*
 	 * DVFS pre sequence would be here, but in our driver the cdclk code
@@ -3152,7 +3150,7 @@  static void icl_pll_enable(struct drm_i915_private *dev_priv,
 
 	if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK,
 				    1)) /* 600us actually. */
-		DRM_ERROR("PLL %d not locked\n", id);
+		DRM_ERROR("PLL %d not locked\n", pll->info->id);
 
 	/* DVFS post sequence would be here. See the comment above. */
 }
@@ -3162,7 +3160,7 @@  static void combo_pll_enable(struct drm_i915_private *dev_priv,
 {
 	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(pll->info->id);
 
-	icl_pll_enable(dev_priv, pll, enable_reg);
+	icl_pll_enable(dev_priv, pll, enable_reg, icl_dpll_write);
 }
 
 static void mg_pll_enable(struct drm_i915_private *dev_priv,
@@ -3171,7 +3169,7 @@  static void mg_pll_enable(struct drm_i915_private *dev_priv,
 	i915_reg_t enable_reg =
 		MG_PLL_ENABLE(icl_pll_id_to_tc_port(pll->info->id));
 
-	icl_pll_enable(dev_priv, pll, enable_reg);
+	icl_pll_enable(dev_priv, pll, enable_reg, icl_mg_pll_write);
 }
 
 static void icl_pll_disable(struct drm_i915_private *dev_priv,