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

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

On Mon, Aug 31, 2020 at 07:03:47PM +0000, Srivatsa, Anusha wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Monday, August 31, 2020 6:42 AM
> > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register
> > lookup
> > 
> > On Fri, Aug 28, 2020 at 02:58:32PM -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.
> > >
> > > Suggested-by: Matt Roper <matthew.d.roper@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 | 27
> > > ++++++++++++-------
> > >  1 file changed, 17 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 81ab975fe4f0..388136618bb7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > @@ -147,6 +147,20 @@ void assert_shared_dpll(struct drm_i915_private
> > *dev_priv,
> > >  			pll->info->name, onoff(state), onoff(cur_state));  }
> > >
> > > +static
> > > +i915_reg_t intel_get_pll_enable_reg(struct drm_i915_private *dev_priv,
> > > +				    struct intel_shared_dpll *pll)
> > 
> > combo_pll_enable_reg() ?
> Actually want to avoid mentioning combo in the name. We might have platforms that do not have combo phys. We still want this function to be one place where platforms gets the PLL_ENABLE register.

There's no point in mixing up different PHY types in a single function.

>  
> > 
> > > +{
> > > +
> > > +	if (IS_ELKHARTLAKE(dev_priv)) {
> > > +		if (pll->info->id == DPLL_ID_EHL_DPLL4)
> > > +			return MG_PLL_ENABLE(0);
> > > +	}
> > 
> > Ugly nested if.
>  Will change it.
> 
> Anusha 
> > > +
> > > +	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 +3856,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_get_pll_enable_reg(dev_priv, pll);
> > >
> > >  	return icl_pll_get_hw_state(dev_priv, pll, hw_state, enable_reg);  }
> > > @@ -4045,11 +4054,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_get_pll_enable_reg(dev_priv, pll);
> > >
> > >  	if (IS_ELKHARTLAKE(dev_priv) &&
> > >  	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> > > -		enable_reg = MG_PLL_ENABLE(0);
> > >
> > >  		/*
> > >  		 * We need to disable DC states when this DPLL is enabled.
> > > @@ -4157,11 +4165,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_get_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);
> > >
> > >  		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
> > 
> > --
> > 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