Re: [PATCH] drm/i915/cnl: Implement CNL display init/unit sequence

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Mon, 2017-06-05 at 18:07 +0300, Imre Deak wrote:
> On Thu, Apr 13, 2017 at 09:13:02AM -0700, Rodrigo Vivi wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Implement the CNL display init/uninit sequence as outlined in Bspec.
> > 
> > Quite similar to SKL/BXT. The main complicaiton is probably the extra
> > procmon setup we must do based on the process/voltage information we
> > can read out from some register.
> > 
> > For now we assume DMC will handle the AUX wells, and we'll just enable
> > all of them during the init sequence. Even if DMC will handle them, we
> > should perhaps trim the set of enabled wells based on which DDI ports
> > are actually present.
> 
> The above needs to be aligned with the current version of the code.
> 
> Yes, looks like DMC saves/restores the state of both AUX and DDI power
> wells. The spec says to enable the AUX wells during init, but I think
> it's ok to do this on-demand. 

Indeed. I updated the code during some rebase but forgot to update the
commit message. I will remove this chuck on next rebase.

> 
> > 
> > v2: s/skl_dbuf/gen9_dbuf/ to follow upstream
> >     bxt needed a cdclk sanitize step, so let's add it for cnl too
> > v3: s/CHICKEN_MISC_1/CHICKEN_MISC_2/ (Ander)
> > v4: Rebased by Rodrigo after Ville's cdclk rework
> > v5: Removed unecessary Aux IO forced enable/disable, Fix DW10 setup
> >     Fix procpon Mask. (Credits-to Paulo and Clint)
> >     Remove A0 workaround.
> > v6: Rebased on top of recent code (Rodrigo).
> > v7: Respect the order of sanitize_ after set_
> >     (Done by Rodrigo, Requested by Ville)
> > v8: (Rodrigo) Remove CHICKEN_MISC_2 double definition.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Clint Taylor <clinton.a.taylor@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h         |  23 +++++++
> >  drivers/gpu/drm/i915/intel_cdclk.c      | 108 ++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h        |   2 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 113 +++++++++++++++++++++++++++++++-
> >  4 files changed, 244 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8353892..9b2d8c0 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1655,6 +1655,9 @@ enum skl_disp_power_wells {
> >  #define   PHY_RESERVED			(1 << 7)
> >  #define BXT_PORT_CL1CM_DW0(phy)		_BXT_PHY((phy), _PORT_CL1CM_DW0_BC)
> >  
> > +#define CNL_PORT_CL1CM_DW5		_MMIO(0x162014)
> > +#define   CL_POWER_DOWN_ENABLE		(1 << 4)
> > +
> >  #define _PORT_CL1CM_DW9_A		0x162024
> >  #define _PORT_CL1CM_DW9_BC		0x6C024
> >  #define   IREF0RC_OFFSET_SHIFT		8
> > @@ -1687,6 +1690,25 @@ enum skl_disp_power_wells {
> >  #define BXT_PORT_CL2CM_DW6(phy)		_BXT_PHY((phy), _PORT_CL2CM_DW6_BC)
> >  #define   DW6_OLDO_DYN_PWR_DOWN_EN	(1 << 28)
> >  
> > +#define CNL_PORT_COMP_DW0		_MMIO(0x162100)
> > +#define   COMP_INIT			(1 << 31)
> > +#define CNL_PORT_COMP_DW1		_MMIO(0x162104)
> > +#define CNL_PORT_COMP_DW3		_MMIO(0x16210c)
> > +#define   PROCESS_INFO_DOT_0		(0 << 26)
> > +#define   PROCESS_INFO_DOT_1		(1 << 26)
> > +#define   PROCESS_INFO_DOT_4		(2 << 26)
> > +#define   PROCESS_INFO_MASK		(7 << 26)
> > +#define   PROCESS_INFO_SHIFT		26
> > +#define   VOLTAGE_INFO_0_85V		(0 << 24)
> > +#define   VOLTAGE_INFO_0_95V		(1 << 24)
> > +#define   VOLTAGE_INFO_1_05V		(2 << 24)
> > +#define   VOLTAGE_INFO_MASK		(3 << 24)
> > +#define   VOLTAGE_INFO_SHIFT		24
> > +#define CNL_PORT_COMP_DW8		_MMIO(0x162120)
> > +#define   PRDIC_ICOMP_DIS		(1 << 14)
> 
> DW8 looks to be unused.

I believe an early w/a for A0 was using that, but that got remove.

I will also remove this.

> 
> > +#define CNL_PORT_COMP_DW9		_MMIO(0x162124)
> > +#define CNL_PORT_COMP_DW10		_MMIO(0x162128)
> > +
> >  /* BXT PHY Ref registers */
> >  #define _PORT_REF_DW3_A			0x16218C
> >  #define _PORT_REF_DW3_BC		0x6C18C
> > @@ -6513,6 +6535,7 @@ enum {
> >  #define  KVM_CONFIG_CHANGE_NOTIFICATION_SELECT	(1 << 14)
> >  
> >  #define CHICKEN_MISC_2		_MMIO(0x42084)
> > +#define  COMP_PWR_DOWN		(1 << 23)
> >  #define  GLK_CL0_PWR_DOWN	(1 << 10)
> >  #define  GLK_CL1_PWR_DOWN	(1 << 11)
> >  #define  GLK_CL2_PWR_DOWN	(1 << 12)
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index bee4394..f9ba1e7 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1555,6 +1555,114 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> >  	intel_update_cdclk(dev_priv);
> >  }
> >  
> > +static int cnl_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
> > +{
> > +	int ratio;
> > +
> > +	if (cdclk == dev_priv->cdclk.hw.ref)
> > +		return 0;
> > +
> > +	switch (cdclk) {
> > +	default:
> > +		MISSING_CASE(cdclk);
> > +	case 168000:
> > +	case 336000:
> > +		ratio = dev_priv->cdclk.hw.ref == 19200 ? 35 : 28;
> > +		break;
> > +	case 528000:
> > +		ratio = dev_priv->cdclk.hw.ref == 19200 ? 55 : 44;
> > +		break;
> > +	}
> > +
> > +	return dev_priv->cdclk.hw.ref * ratio;
> > +}
> > +
> > +static void cnl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> > +{
> > +	u32 cdctl, expected;
> > +
> > +	intel_update_cdclk(dev_priv);
> > +
> > +	if (dev_priv->cdclk.hw.vco == 0 ||
> > +	    dev_priv->cdclk.hw.cdclk == dev_priv->cdclk.hw.ref)
> > +		goto sanitize;
> > +
> > +	/* DPLL okay; verify the cdclock
> > +	 *
> > +	 * Some BIOS versions leave an incorrect decimal frequency value and
> > +	 * set reserved MBZ bits in CDCLK_CTL at least during exiting from S4,
> > +	 * so sanitize this register.
> > +	 */
> > +	cdctl = I915_READ(CDCLK_CTL);
> > +	/*
> > +	 * Let's ignore the pipe field, since BIOS could have configured the
> > +	 * dividers both synching to an active pipe, or asynchronously
> > +	 * (PIPE_NONE).
> > +	 */
> > +	cdctl &= ~BXT_CDCLK_CD2X_PIPE_NONE;
> > +
> > +	expected = (cdctl & BXT_CDCLK_CD2X_DIV_SEL_MASK) |
> > +		   skl_cdclk_decimal(dev_priv->cdclk.hw.cdclk);
> > +
> > +	if (cdctl == expected)
> > +		/* All well; nothing to sanitize */
> > +		return;
> > +
> > +sanitize:
> > +	DRM_DEBUG_KMS("Sanitizing cdclk programmed by pre-os\n");
> > +
> > +	/* force cdclk programming */
> > +	dev_priv->cdclk.hw.cdclk = 0;
> > +
> > +	/* force full PLL disable + enable */
> > +	dev_priv->cdclk.hw.vco = -1;
> > +}
> > +
> > +/**
> > + * cnl_init_cdclk - Initialize CDCLK on CNL
> > + * @dev_priv: i915 device
> > + *
> > + * Initialize CDCLK for CNL. This is generally
> > + * done only during the display core initialization sequence,
> > + * after which the DMC will take care of turning CDCLK off/on
> > + * as needed.
> > + */
> > +
> 
> Extra w/s.

what do you mean here?

> 
> Looks ok:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

thanks

> 
> > +void cnl_init_cdclk(struct drm_i915_private *dev_priv)
> > +{
> > +	struct intel_cdclk_state cdclk_state;
> > +
> > +	cnl_sanitize_cdclk(dev_priv);
> > +
> > +	if (dev_priv->cdclk.hw.cdclk != 0 &&
> > +	    dev_priv->cdclk.hw.vco != 0)
> > +		return;
> > +
> > +	cdclk_state = dev_priv->cdclk.hw;
> > +
> > +	cdclk_state.cdclk = 168000;
> > +	cdclk_state.vco = cnl_cdclk_pll_vco(dev_priv, cdclk_state.cdclk);
> > +
> > +	cnl_set_cdclk(dev_priv, &cdclk_state);
> > +}
> > +
> > +/**
> > + * cnl_uninit_cdclk - Uninitialize CDCLK on CNL
> > + * @dev_priv: i915 device
> > + *
> > + * Uninitialize CDCLK for CNL. This is done only
> > + * during the display core uninitialization sequence.
> > + */
> > +void cnl_uninit_cdclk(struct drm_i915_private *dev_priv)
> > +{
> > +	struct intel_cdclk_state cdclk_state = dev_priv->cdclk.hw;
> > +
> > +	cdclk_state.cdclk = cdclk_state.ref;
> > +	cdclk_state.vco = 0;
> > +
> > +	cnl_set_cdclk(dev_priv, &cdclk_state);
> > +}
> > +
> >  /**
> >   * intel_cdclk_state_compare - Determine if two CDCLK states differ
> >   * @a: first CDCLK state
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 7bc0c25..a526e6e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1280,6 +1280,8 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
> >  /* intel_cdclk.c */
> >  void skl_init_cdclk(struct drm_i915_private *dev_priv);
> >  void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> > +void cnl_init_cdclk(struct drm_i915_private *dev_priv);
> > +void cnl_uninit_cdclk(struct drm_i915_private *dev_priv);
> >  void bxt_init_cdclk(struct drm_i915_private *dev_priv);
> >  void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
> >  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 1797c91..5c3c6ec 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -2698,6 +2698,111 @@ void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
> >  	mutex_unlock(&power_domains->lock);
> >  }
> >  
> > +#define CNL_PROCMON_IDX(val) \
> > +	(((val) & (PROCESS_INFO_MASK | VOLTAGE_INFO_MASK)) >> VOLTAGE_INFO_SHIFT)
> > +#define NUM_CNL_PROCMON \
> > +	(CNL_PROCMON_IDX(VOLTAGE_INFO_MASK | PROCESS_INFO_MASK) + 1)
> > +
> > +static const struct cnl_procmon {
> > +	u32 dw1, dw9, dw10;
> > +} cnl_procmon_values[NUM_CNL_PROCMON] = {
> > +	[CNL_PROCMON_IDX(VOLTAGE_INFO_0_85V | PROCESS_INFO_DOT_0)] =
> > +		{ .dw1 = 0x00 << 16, .dw9 = 0x62AB67BB, .dw10 = 0x51914F96, },
> > +	[CNL_PROCMON_IDX(VOLTAGE_INFO_0_95V | PROCESS_INFO_DOT_0)] =
> > +		{ .dw1 = 0x00 << 16, .dw9 = 0x86E172C7, .dw10 = 0x77CA5EAB, },
> > +	[CNL_PROCMON_IDX(VOLTAGE_INFO_0_95V | PROCESS_INFO_DOT_1)] =
> > +		{ .dw1 = 0x00 << 16, .dw9 = 0x93F87FE1, .dw10 = 0x8AE871C5, },
> > +	[CNL_PROCMON_IDX(VOLTAGE_INFO_1_05V | PROCESS_INFO_DOT_0)] =
> > +		{ .dw1 = 0x00 << 16, .dw9 = 0x98FA82DD, .dw10 = 0x89E46DC1, },
> > +	[CNL_PROCMON_IDX(VOLTAGE_INFO_1_05V | PROCESS_INFO_DOT_1)] =
> > +		{ .dw1 = 0x44 << 16, .dw9 = 0x9A00AB25, .dw10 = 0x8AE38FF1, },
> > +};
> > +
> > +static void cnl_display_core_init(struct drm_i915_private *dev_priv, bool resume)
> > +{
> > +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > +	const struct cnl_procmon *procmon;
> > +	struct i915_power_well *well;
> > +	u32 val;
> > +
> > +	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > +
> > +	/* 1. Enable PCH Reset Handshake */
> > +	val = I915_READ(HSW_NDE_RSTWRN_OPT);
> > +	val |= RESET_PCH_HANDSHAKE_ENABLE;
> > +	I915_WRITE(HSW_NDE_RSTWRN_OPT, val);
> > +
> > +	/* 2. Enable Comp */
> > +	val = I915_READ(CHICKEN_MISC_2);
> > +	val &= ~COMP_PWR_DOWN;
> > +	I915_WRITE(CHICKEN_MISC_2, val);
> > +
> > +	val = I915_READ(CNL_PORT_COMP_DW3);
> > +	procmon = &cnl_procmon_values[CNL_PROCMON_IDX(val)];
> > +
> > +	WARN_ON(procmon->dw10 == 0);
> > +
> > +	val = I915_READ(CNL_PORT_COMP_DW1);
> > +	val &= ~((0xff << 16) | 0xff);
> > +	val |= procmon->dw1;
> > +	I915_WRITE(CNL_PORT_COMP_DW1, val);
> > +
> > +	I915_WRITE(CNL_PORT_COMP_DW9, procmon->dw9);
> > +	I915_WRITE(CNL_PORT_COMP_DW10, procmon->dw10);
> > +
> > +	val = I915_READ(CNL_PORT_COMP_DW0);
> > +	val |= COMP_INIT;
> > +	I915_WRITE(CNL_PORT_COMP_DW0, val);
> > +
> > +	/* 3. */
> > +	val = I915_READ(CNL_PORT_CL1CM_DW5);
> > +	val |= CL_POWER_DOWN_ENABLE;
> > +	I915_WRITE(CNL_PORT_CL1CM_DW5, val);
> > +
> > +	/* 4. Enable Power Well 1 (PG1) and Aux IO Power */
> > +	mutex_lock(&power_domains->lock);
> > +	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
> > +	intel_power_well_enable(dev_priv, well);
> > +	mutex_unlock(&power_domains->lock);
> > +
> > +	/* 5. Enable CD clock */
> > +	cnl_init_cdclk(dev_priv);
> > +
> > +	/* 6. Enable DBUF */
> > +	gen9_dbuf_enable(dev_priv);
> > +}
> > +
> > +#undef CNL_PROCMON_IDX
> > +#undef NUM_CNL_PROCMON
> > +
> > +static void cnl_display_core_uninit(struct drm_i915_private *dev_priv)
> > +{
> > +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > +	struct i915_power_well *well;
> > +	u32 val;
> > +
> > +	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > +
> > +	/* 1. Disable all display engine functions -> aready done */
> > +
> > +	/* 2. Disable DBUF */
> > +	gen9_dbuf_disable(dev_priv);
> > +
> > +	/* 3. Disable CD clock */
> > +	cnl_uninit_cdclk(dev_priv);
> > +
> > +	/* 4. Disable Power Well 1 (PG1) and Aux IO Power */
> > +	mutex_lock(&power_domains->lock);
> > +	well = lookup_power_well(dev_priv, SKL_DISP_PW_1);
> > +	intel_power_well_disable(dev_priv, well);
> > +	mutex_unlock(&power_domains->lock);
> > +
> > +	/* 5. Disable Comp */
> > +	val = I915_READ(CHICKEN_MISC_2);
> > +	val |= COMP_PWR_DOWN;
> > +	I915_WRITE(CHICKEN_MISC_2, val);
> > +}
> > +
> >  static void chv_phy_control_init(struct drm_i915_private *dev_priv)
> >  {
> >  	struct i915_power_well *cmn_bc =
> > @@ -2830,7 +2935,9 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
> >  
> >  	power_domains->initializing = true;
> >  
> > -	if (IS_GEN9_BC(dev_priv)) {
> > +	if (IS_CANNONLAKE(dev_priv)) {
> > +		cnl_display_core_init(dev_priv, resume);
> > +	} else if (IS_GEN9_BC(dev_priv)) {
> >  		skl_display_core_init(dev_priv, resume);
> >  	} else if (IS_GEN9_LP(dev_priv)) {
> >  		bxt_display_core_init(dev_priv, resume);
> > @@ -2869,7 +2976,9 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
> >  	if (!i915.disable_power_well)
> >  		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> >  
> > -	if (IS_GEN9_BC(dev_priv))
> > +	if (IS_CANNONLAKE(dev_priv))
> > +		cnl_display_core_uninit(dev_priv);
> > +	else if (IS_GEN9_BC(dev_priv))
> >  		skl_display_core_uninit(dev_priv);
> >  	else if (IS_GEN9_LP(dev_priv))
> >  		bxt_display_core_uninit(dev_priv);
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx