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

Mon Oct 1 06:38:18 UTC 2018

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Wednesday, September 26, 2018 7:57 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 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.
As per the bspec,
The below two steps are the extra ones that we do before enabling the PLL
1. Configure both DSI_ESC_CLK_DIV and DPHY_ESC_CLK_DIV registers. Both registers must be programmed with the same value.
2. Read back both DPHY_ESC_CLK_DIV, ignoring the data value, this is to ensure write completed before the next step.

And these steps below
Configure DPCLKA_CFGCR0 to map the DPLL to the DDI/DSI.
Make sure that the DDI clocks are not gated at this point.  The DSI enabling sequence will gate the DDI clocks at a later point within the sequence.
are done before pll enable in case of pll enable sequence for mipi dsi.

Thanks
Vandita
> 
> >
> > 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