[Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable functionality

[Ville Syrjälä]

On Wed, Sep 19, 2018 at 05:31:37PM +0000, Kulkarni, Vandita wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Sent: Friday, September 14, 2018 9:37 PM
> > To: Kulkarni, Vandita <vandita.kulkarni at intel.com>
> > Cc: intel-gfx at lists.freedesktop.org; Nikula, Jani <jani.nikula at intel.com>;
> > Zanoni, Paulo R <paulo.r.zanoni at intel.com>
> > Subject: Re: [Intel-gfx] [RFC 1/3] drm/i915/icl: Restructure ICL DPLL enable
> > functionality
> > 
> > On Fri, Sep 14, 2018 at 12:24:12PM +0530, Vandita Kulkarni wrote:
> > > From: Madhav Chauhan <madhav.chauhan at intel.com>
> > >
> > > In Gen11, DPLL 0 and 1 are shared between DDI and DSI.
> > > Most of the steps for enabling DPLL are common across DDI and DSI.
> > > This patch makes icl_dpll_enable() generic which will be used by all
> > > the encoders.
> > >
> > > Signed-off-by: Madhav Chauhan <madhav.chauhan at intel.com>
> > > Signed-off-by: Vandita Kulkarni <vandita.kulkarni at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c      | 12 ++++++++++++
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 19 ++-----------------
> > > drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
> > >  3 files changed, 15 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index cd01a09..2942a24 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2810,6 +2810,18 @@ static void intel_ddi_clk_select(struct
> > intel_encoder *encoder,
> > >  	mutex_lock(&dev_priv->dpll_lock);
> > >
> > >  	if (IS_ICELAKE(dev_priv)) {
> > > +		enum intel_dpll_id id = pll->info->id;
> > > +		i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> > > +
> > > +		val = I915_READ(enable_reg);
> > > +		val |= PLL_ENABLE;
> > > +		I915_WRITE(enable_reg, val);
> > > +
> > > +		/* TODO: wait times missing from the spec. */
> > > +		if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK,
> > > +					    PLL_LOCK, 5))
> > > +			DRM_ERROR("PLL %d not locked\n", id);
> > > +
> > 
> > I don't really see why this can't stay in the dpll mgr.
> This was part of icl_pll_enable, which gets called for all hdmi, dp and mipi dsi.
> But for mipi dsi we have couple of more things to do before we enable pll.
> 
> In order to keep it unchanged for other encoders , pll enable was moved here.
> Another way could be have an encoder check in icl_pll_enable function and do those extra steps for mipi dsi and then enable the pll.
> Please let me know what you think would be a better way to do.

Hard to judge what is best without knowing what the differences are.

> 
> Thanks,
> Vandita 
> > 
> > >  		if (port >= PORT_C)
> > >  			I915_WRITE(DDI_CLK_SEL(port),
> > >  				   icl_pll_to_ddi_pll_sel(encoder, pll)); diff --git
> > > a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index e6cac92..36ed155 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -2930,7 +2930,7 @@ static bool icl_calc_mg_pll_state(struct
> > intel_crtc_state *crtc_state,
> > >  	return pll;
> > >  }
> > >
> > > -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> > >  {
> > >  	switch (id) {
> > >  	default:
> > > @@ -3119,22 +3119,7 @@ static void icl_pll_enable(struct drm_i915_private
> > *dev_priv,
> > >  	default:
> > >  		MISSING_CASE(id);
> > >  	}
> > > -
> > > -	/*
> > > -	 * DVFS pre sequence would be here, but in our driver the cdclk code
> > > -	 * paths should already be setting the appropriate voltage, hence we do
> > > -	 * nothign here.
> > > -	 */
> > > -
> > > -	val = I915_READ(enable_reg);
> > > -	val |= PLL_ENABLE;
> > > -	I915_WRITE(enable_reg, val);
> > > -
> > > -	if (intel_wait_for_register(dev_priv, enable_reg, PLL_LOCK, PLL_LOCK,
> > > -				    1)) /* 600us actually. */
> > > -		DRM_ERROR("PLL %d not locked\n", id);
> > > -
> > > -	/* DVFS post sequence would be here. See the comment above. */
> > > +	/* Encoder specific PLL enable steps are added in encoder file */
> > >  }
> > >
> > >  static void icl_pll_disable(struct drm_i915_private *dev_priv, diff
> > > --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > index bf0de8a..9e89265 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > @@ -345,5 +345,5 @@ void intel_dpll_dump_hw_state(struct
> > > drm_i915_private *dev_priv,  int icl_calc_dp_combo_pll_link(struct
> > drm_i915_private *dev_priv,
> > >  			       uint32_t pll_id);
> > >  int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
> > > -
> > > +i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id);
> > >  #endif /* _INTEL_DPLL_MGR_H_ */
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel