[Intel-gfx] [PATCH 32/67] drm/i915/cnl: DDI - PLL mapping

[Ander Conselvan De Oliveira]

On Fri, 2017-04-07 at 18:12 -0300, Paulo Zanoni wrote:
> Em Qui, 2017-04-06 às 12:15 -0700, Rodrigo Vivi escreveu:
> > One of the steps for PLL (un)initialization is to (un)map
> > the correspondent DDI that is actually using that PLL.
> > 
> > So, let's do this step following the places already stablished
> > and used so far, although spec put this as part of PLL
> > initialization sequences.
> > 
> > v2: Use proper prefix on bits names as suggested by Ander.
> > v3: Add missed "~". Without that the logic was inverted
> >     so we were disabling interrupts.
> >     Credits-to: Clinton
> >     Credits-to: Art
> > v4: Spec is getting updated to do DDI -> PLL mapping
> >     and clock on in 2 separated reg writes. (Paulo)
> >     Also update bits definitions to use space
> >     (1 << 1) instead of (1<<1). (Paulo)
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Cc: Art Runyan <arthur.j.runyan at intel.com>
> > Cc: Clint Taylor <clinton.a.taylor at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Kahola, Mika <mika.kahola at intel.com>
> > Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira at intel.co
> > m>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Reviewed-by: Kahola, Mika <mika.kahola at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |  9 +++++++++
> >  drivers/gpu/drm/i915/intel_ddi.c | 23 ++++++++++++++++++++---
> >  2 files changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 3cfc65f..dcb8e21 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -8150,6 +8150,15 @@ enum {
> >  #define DPLL_CFGCR1(id)	_MMIO_PIPE((id) - SKL_DPLL1,
> > _DPLL1_CFGCR1, _DPLL2_CFGCR1)
> >  #define DPLL_CFGCR2(id)	_MMIO_PIPE((id) - SKL_DPLL1,
> > _DPLL1_CFGCR2, _DPLL2_CFGCR2)
> >  
> > +/*
> > + * CNL Clocks
> > + */
> > +#define DPCLKA_CFGCR0				_MMIO(0x6C200)
> > +#define  DPCLKA_CFGCR0_DDI_CLK_OFF(port)	(1 << ((port)+10))
> > +#define  DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port)	(3 <<
> > ((port)*2))
> > +#define  DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port)	((port)*2)
> > +#define  DPCLKA_CFGCR0_DDI_CLK_SEL(pll, port)	((pll) <<
> > ((port)*2))
> > +
> >  /* BXT display engine PLL */
> >  #define BXT_DE_PLL_CTL			_MMIO(0x6d000)
> >  #define   BXT_DE_PLL_RATIO(x)		(x)	/*
> > {60,65,100} * 19.2MHz */
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 0914ad9..2a901bf 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1621,13 +1621,27 @@ static void intel_ddi_clk_select(struct
> > intel_encoder *encoder,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder-
> > > base.dev);
> > 
> >  	enum port port = intel_ddi_get_encoder_port(encoder);
> > +	uint32_t val;
> >  
> >  	if (WARN_ON(!pll))
> >  		return;
> >  
> > -	if (IS_GEN9_BC(dev_priv)) {
> > -		uint32_t val;
> > +	if (IS_CANNONLAKE(dev_priv)) {
> > +		/* Configure DPCLKA_CFGCR0 to map the DPLL to the
> > DDI. */
> > +		val = I915_READ(DPCLKA_CFGCR0);
> > +		val |= DPCLKA_CFGCR0_DDI_CLK_SEL(pll->id, port);
> > +		I915_WRITE(DPCLKA_CFGCR0, val);
> 
> A question to the Atomic Lords: don't we need some sort of locking
> around this register since it's used by all ports/clocks? I suppose
> dev_priv->dpll_lock would do...
> 
> Maybe the same would apply for gen9_bc.

If there are modesets happening in parallel for different crtcs, then some
locking is needed. dpll_lock seems like the right call, that's what's used to
avoid the same problem with the enable/disable hooks.

Btw, I think this patch shows why something like [1] might be a good idea.

[1] https://patchwork.freedesktop.org/patch/113598/
> 
> >  
> > +		/*
> > +		 * Configure DPCLKA_CFGCR0 to turn on the clock for
> > the DDI.
> > +		 * This step and the step before must be done with
> > separate
> > +		 * register writes.
> > +		 */
> > +		val = I915_READ(DPCLKA_CFGCR0);
> > +		val &= ~(DPCLKA_CFGCR0_DDI_CLK_OFF(port) |
> > +			 DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port));
> > +		I915_WRITE(DPCLKA_CFGCR0, val);
> > +	} else if (IS_GEN9_BC(dev_priv)) {
> >  		/* DDI -> PLL mapping  */
> >  		val = I915_READ(DPLL_CTRL2);
> >  
> > @@ -1763,7 +1777,10 @@ static void intel_ddi_post_disable(struct
> > intel_encoder *intel_encoder,
> >  	if (dig_port)
> >  		intel_display_power_put(dev_priv, dig_port-
> > > ddi_io_power_domain);
> > 
> >  
> > -	if (IS_GEN9_BC(dev_priv))
> > +	if (IS_CANNONLAKE(dev_priv))
> > +		I915_WRITE(DPCLKA_CFGCR0, I915_READ(DPCLKA_CFGCR0) |
> > +			   DPCLKA_CFGCR0_DDI_CLK_OFF(port));
> > +	else if (IS_GEN9_BC(dev_priv))
> >  		I915_WRITE(DPLL_CTRL2, (I915_READ(DPLL_CTRL2) |
> >  					DPLL_CTRL2_DDI_CLK_OFF(port)
> > ));
> >  	else if (INTEL_GEN(dev_priv) < 9)
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx