[Intel-gfx] [PATCH 30/49] drm/i915/bxt: add display initialize/uninitialize sequence

Imre Deak imre.deak at intel.com
Tue Apr 7 07:07:25 PDT 2015


On to, 2015-04-02 at 19:32 +0300, Ville Syrjälä wrote:
> On Tue, Mar 17, 2015 at 11:39:56AM +0200, Imre Deak wrote:
> > From: Vandana Kannan <vandana.kannan at intel.com>
> > 
> > Add display clock/PHY initialization sequence as per BSpec.
> > 
> > Until GOP/VBIOS provides an upper limit value for CDCLK, comparing clock
> > value with 624 MHz and returning 0 in case it exceeds.
> > 
> > Note that the CD clock and PHY initialization/uninitialization are done
> > at their current place only for simplicity, in a future patch - when more
> > of the runtime PM features will be enabled - these will be moved to
> > power well#1 and modeset encoder enabling/disabling hooks respectively.
> > This also means that atm dynamic power gating power well #2 is
> > effectively disabled.
> 
> OK, I've gone through the PHY stuff a bit now, and skipped the cdclk
> stuff this time.
> 
> > 
> > v1: Added function definitions in header files
> > v2: Imre's review comments addressed
> > - Moved CDCLK related definitions to i915_reg.h
> > - Removed defintions for CDCLK frequency
> > - Split uninit_cdclk() by adding a phy_uninit function
> > - Calculate freq and decimal based on input frequency
> > - Program SSA precharge based on input frequency
> > - Use wait_for 1ms instead 200us udelay for DE PLL locking
> > - Removed initial value for divider, freq, decimal, ratio.
> > - Replaced polling loops with wait_for
> > - Parameterized latency optim setting
> > - Fix the parts where DE PLL has to be disabled.
> > - Call CDCLK selection from mode set
> > 
> > v3: (imre)
> > - add note about the plan to move the cdclk/phy init to a better place
> > - take rps.hw_lock around pcode access
> > - fix DDI PHY timeout value
> > - squash in Vandana's "PORT_CL2CM_DW6_A BUN fix",
> >   "DDI PHY programming register defn", "Do ddi_phy_init always",
> >   "Check CDCLK upper limit" patches
> > - move PHY register macros next to the corresponding CHV/VLV macros
> > - move DE PLL register macros here from another patch since they are
> >   used here first
> > - add BXT_ prefix to CDCLK flags
> > - s/COMMON_RESET/COMMON_RESET_DIS/ and clarify related code comments
> > - fix incorrect read value for the RMW of BXT_PHY_CTL_FAMILY_DDI
> > - fix using GT_DISPLAY_EDP_POWER_ON vs. GT_DISPLAY_DDI_POWER_ON
> >   when powering on DDI ports
> > - fix incorrect port when setting BXT_PORT_TX_DW14_LN for DDI ports
> > - add missing masking when programming CDCLK_FREQ_DECIMAL
> > - add missing powering on for DDI-C port, rename OCL2_LDOFUSE_PWR_EN
> >   to OCL2_LDOFUSE_PWR_DIS to reduce confusion
> > - add note about mismatch with bspec in the PORT_REF_DW6 fields
> > - factor out PHY init code to a new function, so we can call it for
> >   PHY_A and PHY_BC, instead of open-coding the same
> > 
> > Signed-off-by: Vandana Kannan <vandana.kannan at intel.com> (v2)
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      | 126 +++++++++++++++
> >  drivers/gpu/drm/i915/intel_ddi.c     | 291 +++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c |  75 +++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> >  4 files changed, 496 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index b4474d3..a3579c0 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1120,6 +1120,110 @@ enum skl_disp_power_wells {
> >  #define   DPIO_FRC_LATENCY_SHFIT	8
> >  #define CHV_TX_DW14(ch, lane) _TXLANE(ch, lane, 0xb8)
> >  #define   DPIO_UPAR_SHIFT		30
> > +
> > +/* BXT PHY registers */
> > +enum bxt_phy {
> > +	BXT_PHY_A,
> > +	BXT_PHY_BC
> > +};
> 
> We have enum dpio_phy already. Although here we have defined 0 to be the
> single channel PHY and 1 is the two channel PHY,  whereas on CHV it's
> the other way around. I'm going to suggest we flip BXT over to use the
> CHV scheme to avoid any surpises later if we actualy try to unify the
> code.

Yea, I haven't noticed it, will use that instead.

> > +
> > +#define BXT_PHY(phy, a, b)		((a) + (phy) * ((b) - (a)))
> 
> This seems to be just another _PIPE(), should at least have an
> underscore so that people don't confuse it with something they are
> supposed to use.

Ok, I can rewrite this to
#define _BXT_PHY(phy, a, b)	_PIPE(phy, a, b)

> 
> > +
> > +#define BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR	0x138090
> 
> We can drop the _0_2_0_GTTMMADR suffix, we've never included it for any
> other platforms either.

Ok.

> > +#define   _EDP_POWER_ON				(1 << 1)
> > +#define   _DDI_POWER_ON				(1 << 0)
> > +#define   GT_DISPLAY_POWER_ON(phy)		BXT_PHY(phy, _EDP_POWER_ON, \
> > +							_DDI_POWER_ON)
> 
> Using a _PIPE() type of macro for register bits seems a bit unusual.
> I'd just open code it as (1 << (phy)) or something. That also work
> better if we the PHYs around so that PHY0 is the dual channel PHY.

Ok, will rewrite it.

> > +
> > +#define _PHY_CTL_FAMILY_EDP		0x64C80
> > +#define _PHY_CTL_FAMILY_DDI		0x64C90
> > +#define   COMMON_RESET_DIS		(1 << 31)
> > +#define BXT_PHY_CTL_FAMILY(phy)		BXT_PHY(phy, _PHY_CTL_FAMILY_EDP, \
> > +						     _PHY_CTL_FAMILY_DDI)
> > +
> > +/* BXT PHY common lane registers */
> > +#define _PORT_CL1CM_DW0_A		0x162000
> > +#define _PORT_CL1CM_DW0_BC		0x6C000
> > +#define   PHY_POWER_GOOD		(1 << 16)
> > +#define BXT_PORT_CL1CM_DW0(phy)		BXT_PHY(phy, _PORT_CL1CM_DW0_A,	\
> > +						     _PORT_CL1CM_DW0_BC)
> 
> I'm a bit sad these are not sharing the CHV reg defines, or even
> resemble them in any way.

I agree this isn't ideal. I've already refactored some parts in the
original version of this patch (see the commit version log) to bring it
closer to CHV, but there is definitely more to do. I'd say it's better
to do this after moving the cdckl/phy init code to the modeset/power
well enable time.

> > +#define _PORT_CL1CM_DW9_A		0x162024
> > +#define _PORT_CL1CM_DW9_BC		0x6C024
> > +#define   IREF0RC_OFFSET_SHIFT		8
> > +#define   IREF0RC_OFFSET_MASK		(0xFF << IREF0RC_OFFSET_SHIFT)
> > +#define BXT_PORT_CL1CM_DW9(phy)		BXT_PHY(phy, _PORT_CL1CM_DW9_A,	\
> > +						     _PORT_CL1CM_DW9_BC)
> > +
> > +#define _PORT_CL1CM_DW10_A		0x162028
> > +#define _PORT_CL1CM_DW10_BC		0x6C028
> > +#define   IREF1RC_OFFSET_SHIFT		8
> > +#define   IREF1RC_OFFSET_MASK		(0xFF << IREF1RC_OFFSET_SHIFT)
> > +#define BXT_PORT_CL1CM_DW10(phy)	BXT_PHY(phy, _PORT_CL1CM_DW10_A, \
> > +						     _PORT_CL1CM_DW10_BC)
> > +
> > +#define _PORT_CL1CM_DW28_A		0x162070
> > +#define _PORT_CL1CM_DW28_BC		0x6C070
> > +#define   OCL1_POWER_DOWN_EN		(1 << 23)
> > +#define   DW28_OLDO_DYN_PWR_DOWN_EN	(1 << 22)
> > +#define   SUS_CLK_CONFIG		0x3
> > +#define BXT_PORT_CL1CM_DW28(phy)	BXT_PHY(phy, _PORT_CL1CM_DW28_A, \
> > +						     _PORT_CL1CM_DW28_BC)
> > +
> > +#define _PORT_CL1CM_DW30_A		0x162078
> > +#define _PORT_CL1CM_DW30_BC		0x6C078
> > +#define   OCL2_LDOFUSE_PWR_DIS		(1 << 6)
> > +#define BXT_PORT_CL1CM_DW30(phy)	BXT_PHY(phy, _PORT_CL1CM_DW30_A, \
> > +						     _PORT_CL1CM_DW30_BC)
> > +
> > +/* Defined for PHY_BC only */
> > +#define BXT_PORT_CL2CM_DW6_BC		0x6C358
> > +#define   DW6_OLDO_DYN_PWR_DOWN_EN	(1 << 28)
> > +
> > +/* BXT PHY Ref registers */
> > +#define _PORT_REF_DW3_A			0x16218C
> > +#define _PORT_REF_DW3_BC		0x6C18C
> > +#define   GRC_DONE			(1 << 22)
> > +#define BXT_PORT_REF_DW3(phy)		BXT_PHY(phy, _PORT_REF_DW3_A,	\
> > +						     _PORT_REF_DW3_BC)
> > +
> > +#define _PORT_REF_DW6_A			0x162198
> > +#define _PORT_REF_DW6_BC		0x6C198
> > +/*
> > + * FIXME: BSpec disagrees on the following two fields, check them with
> > + * HW/documentation people.
> > + */
> 
> CHV configdb also disagrees.

Ok, thanks. Before fixing this, I want to wait for the HW to test
things.

> > +#define   GRC_CODE_SHIFT		23
> > +#define   GRC_CODE_MASK			(0x1FF << GRC_CODE_SHIFT)
> > +#define   GRC_CODE_FAST_SHIFT		16
> > +#define   GRC_CODE_FAST_MASK		(0x7F << GRC_CODE_FAST_SHIFT)
> > +#define   GRC_CODE_SLOW_SHIFT		8
> > +#define   GRC_CODE_SLOW_MASK		(0xFF << GRC_CODE_SLOW_SHIFT)
> > +#define   GRC_CODE_NOM_MASK		0xFF
> > +#define BXT_PORT_REF_DW6(phy)		BXT_PHY(phy, _PORT_REF_DW6_A,	\
> > +						     _PORT_REF_DW6_BC)
> > +
> > +#define _PORT_REF_DW8_A			0x1621A0
> > +#define _PORT_REF_DW8_BC		0x6C1A0
> > +#define   GRC_DIS			(1 << 15)
> > +#define   GRC_RDY_OVRD			(1 << 1)
> > +#define BXT_PORT_REF_DW8(phy)		BXT_PHY(phy, _PORT_REF_DW8_A,	\
> > +						     _PORT_REF_DW8_BC)
> > +
> > +/* BXT PHY TX registers */
> > +#define BXT_LANE_OFFSET(lane)           (((lane) >> 1) * 0x200 +	\
> > +					 ((lane) & 1) * 0x80)
> > +
> > +#define _PORT_TX_DW14_LN0_A		0x162538
> > +#define _PORT_TX_DW14_LN0_B		0x6C538
> > +#define _PORT_TX_DW14_LN0_C		0x6C938
> > +#define   LATENCY_OPTIM_SHIFT		30
> > +#define   LATENCY_OPTIM			(1 << LATENCY_OPTIM_SHIFT)
> > +#define BXT_PORT_TX_DW14_LN(port, lane)	(_PORT3(port, _PORT_TX_DW14_LN0_A,   \
> > +						      _PORT_TX_DW14_LN0_B,   \
> > +						      _PORT_TX_DW14_LN0_C) + \
> > +					 BXT_LANE_OFFSET(lane))
> > +
> >  /*
> >   * Fence registers
> >   */
> > @@ -5326,6 +5430,9 @@ enum skl_disp_power_wells {
> >  #define  DISP_FBC_WM_DIS		(1<<15)
> >  #define DISP_ARB_CTL2	0x45004
> >  #define  DISP_DATA_PARTITION_5_6	(1<<6)
> > +#define DBUF_CTL	0x45008
> > +#define  DBUF_POWER_REQUEST		(1<<31)
> > +#define  DBUF_POWER_STATE		(1<<30)
> >  #define GEN7_MSG_CTL	0x45010
> >  #define  WAIT_FOR_PCH_RESET_ACK		(1<<1)
> >  #define  WAIT_FOR_PCH_FLR_ACK		(1<<0)
> <snip>
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index a203d9d..789682d 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1957,6 +1957,294 @@ static void skl_shared_dplls_init(struct drm_i915_private *dev_priv)
> >  	}
> >  }
> >  
> > +static void bxt_init_phy(struct drm_i915_private *dev_priv, enum bxt_phy phy)
> > +{
> > +	enum port port;
> > +	uint32_t val;
> > +
> > +	val = I915_READ(BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR);
> > +	val |= GT_DISPLAY_POWER_ON(phy);
> > +	I915_WRITE(BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR, val);
> > +
> > +	/* Considering 10ms timeout until BSpec is updated */
> > +	if (wait_for(I915_READ(BXT_PORT_CL1CM_DW0(phy)) & PHY_POWER_GOOD, 10))
> > +		DRM_ERROR("timeout during PHY#%d power on\n", phy);
> > +
> > +	/* Program latency optim setting */
> > +	for (port =  (phy == BXT_PHY_A ? PORT_A : PORT_B);
> > +	     port <= (phy == BXT_PHY_A ? PORT_A : PORT_C); port++) {
> > +		int lane;
> > +
> > +		for (lane = 0; lane < 4; lane++) {
> > +			val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> > +			val &= ~LATENCY_OPTIM;
> > +			if (lane == 1)
> 
> Should be != 1

Thanks for catching it, this is a fall-out from my refactoring. Will fix
it.

> > +				val |= LATENCY_OPTIM;
> > +
> > +			I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
> > +		}
> > +	}
> > +
> > +	/* Program PLL Rcomp code offset */
> > +	val = I915_READ(BXT_PORT_CL1CM_DW9(phy));
> > +	val &= ~IREF0RC_OFFSET_MASK;
> > +	val |= 0xE4 << IREF0RC_OFFSET_SHIFT;
> > +	I915_WRITE(BXT_PORT_CL1CM_DW9(phy), val);
> > +
> > +	val = I915_READ(BXT_PORT_CL1CM_DW10(phy));
> > +	val &= ~IREF1RC_OFFSET_MASK;
> > +	val |= 0xE4 << IREF1RC_OFFSET_SHIFT;
> > +	I915_WRITE(BXT_PORT_CL1CM_DW10(phy), val);
> > +
> > +	/* Program power gating */
> > +	val = I915_READ(BXT_PORT_CL1CM_DW28(phy));
> > +	val |= OCL1_POWER_DOWN_EN | DW28_OLDO_DYN_PWR_DOWN_EN |
> > +		SUS_CLK_CONFIG;
> > +	I915_WRITE(BXT_PORT_CL1CM_DW28(phy), val);
> > +
> > +	if (phy == BXT_PHY_BC) {
> > +		val = I915_READ(BXT_PORT_CL2CM_DW6_BC);
> > +		val |= DW6_OLDO_DYN_PWR_DOWN_EN;
> > +		I915_WRITE(BXT_PORT_CL2CM_DW6_BC, val);
> > +	}
> > +
> > +	val = I915_READ(BXT_PORT_CL1CM_DW30(phy));
> > +	val &= ~OCL2_LDOFUSE_PWR_DIS;
> > +	/*
> > +	 * On PHY_A disable power on the second channel, since no port is
> > +	 * connected there. On PHY_BC both channels have a port, so leave it
> > +	 * enabled.
> > +	 * Note that port C is only connected on BXT-P, so on BXT0/1 we should
> > +	 * power down the second channel on PHY_BC as well.
> > +	 */
> > +	if (phy == BXT_PHY_A)
> > +		val |= OCL2_LDOFUSE_PWR_DIS;
> > +	I915_WRITE(BXT_PORT_CL1CM_DW30(phy), val);
> > +
> > +	if (phy == BXT_PHY_BC) {
> > +		uint32_t grc_code;
> > +		/*
> > +		 * PHY_BC isn't connected to an RCOMP resistor so copy over
> > +		 * the corresponding calibrated value from PHY_A, and disable
> > +		 * the automatic calibration on PHY_BC.
> > +		 */
> > +		if (wait_for(I915_READ(BXT_PORT_REF_DW3(BXT_PHY_A)) & GRC_DONE,
> > +			     10))
> > +			DRM_ERROR("timeout waiting for PHY#0 GRC\n");
> > +
> > +		val = I915_READ(BXT_PORT_REF_DW6(BXT_PHY_A));
> > +		val = (val & GRC_CODE_MASK) >> GRC_CODE_SHIFT;
> > +		grc_code = val << GRC_CODE_FAST_SHIFT |
> > +			   val << GRC_CODE_SLOW_SHIFT |
> > +			   val;
> > +		I915_WRITE(BXT_PORT_REF_DW6(BXT_PHY_BC), grc_code);
> > +
> > +		val = I915_READ(BXT_PORT_REF_DW8(BXT_PHY_BC));
> > +		val |= GRC_DIS | GRC_RDY_OVRD;
> > +		I915_WRITE(BXT_PORT_REF_DW8(BXT_PHY_BC), val);
> > +	}
> > +
> > +	/* Release common_reset */
> > +	val = I915_READ(BXT_PHY_CTL_FAMILY(phy));
> > +	val |= COMMON_RESET_DIS;
> > +	I915_WRITE(BXT_PHY_CTL_FAMILY(phy), val);
> > +}
> 
> I suppose we'll want to model this thing as a power well like on CHV
> eventually, but I guess we can start off with initializing it once.

Yep, sounds good, but I'd also prefer to do this as a follow-up.

> > +
> > +void bxt_ddi_phy_init(struct drm_device *dev)
> > +{
> > +	/* Enable PHY_A first since it provides Rcomp for PHY_BC */
> > +	bxt_init_phy(dev->dev_private, BXT_PHY_A);
> > +	bxt_init_phy(dev->dev_private, BXT_PHY_BC);
> > +}
> > +
> > +static void bxt_ddi_phy_uninit(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	uint32_t temp;
> > +
> > +	temp = I915_READ(BXT_PHY_CTL_FAMILY(BXT_PHY_A));
> > +	I915_WRITE(BXT_PHY_CTL_FAMILY(BXT_PHY_A), temp & ~COMMON_RESET_DIS);
> > +
> > +	temp = I915_READ(BXT_PHY_CTL_FAMILY(BXT_PHY_BC));
> > +	I915_WRITE(BXT_PHY_CTL_FAMILY(BXT_PHY_BC), temp & ~COMMON_RESET_DIS);
> > +
> > +	I915_WRITE(BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR, 0);
> > +}
> 
> This should really be per-phy to avoid confusion when comparing with the
> init function.

Ok, will rewrite it.

> > +
> <snip>
> > +
> > +void bxt_init_cdclk(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	/* NDE_RSTWRN_OPT RST PCH Handshake En must always be 0b on BXT
> > +	 * or else the reset will hang because there is no PCH to respond.
> > +	 * Move the handshake programming to initialization sequence.
> > +	 * Previously was left up to BIOS.
> > +	 */
> > +	u32 temp = I915_READ(HSW_NDE_RSTWRN_OPT);
> > +
> > +	temp &= ~RESET_PCH_HANDSHAKE_ENABLE;
> > +	I915_WRITE(HSW_NDE_RSTWRN_OPT, temp);
> > +
> > +	/* Enable PG1 for cdclk */
> > +	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> > +
> > +	/* check if cd clock is enabled */
> > +	if (I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_PLL_ENABLE) {
> > +		DRM_DEBUG_KMS("Display already initialized\n");
> > +		return;
> > +	}
> > +
> > +	/* FIXME:- The initial CDCLK needs to be read from VBT.
> > +	 * Need to make this change after VBT has changes for BXT.
> > +	 */
> > +	bxt_select_cdclk_freq(dev, 624000);
> > +
> > +	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> > +	udelay(10);
> > +
> > +	if (!(I915_READ(DBUF_CTL) & DBUF_POWER_STATE))
> > +		DRM_ERROR("DBuf power enable timeout!\n");
> > +}
> 
> This code seems like power well territory again. Just stuffing it into
> PG1 would seem like a good enough solution judging by the fact that we
> hold the PG1 reference as long as we have the DBUF power request enabled.
> 
> Or are there actual uses for having PG1 enabled w/o this thing?

I don't think so, bspec requires this power request line to be asserted
for all display internal functionality. So I agree moving it to the
power well code is a good plan (as a follow-up).

> > +
> > +void bxt_uninit_cdclk(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	bxt_ddi_phy_uninit(dev);
> > +
> > +	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) & ~DBUF_POWER_REQUEST);
> > +	udelay(10);
> > +
> > +	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> > +		DRM_ERROR("DBuf power disable timeout!\n");
> > +
> > +	bxt_select_cdclk_freq(dev, 0);
> > +
> > +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> > +}
> > +
> >  void intel_ddi_pll_init(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -1973,6 +2261,9 @@ void intel_ddi_pll_init(struct drm_device *dev)
> >  	if (IS_SKYLAKE(dev)) {
> >  		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> >  			DRM_ERROR("LCPLL1 is disabled\n");
> > +	} else if (IS_BROXTON(dev)) {
> > +		bxt_init_cdclk(dev);
> > +		bxt_ddi_phy_init(dev);
> >  	} else {
> >  		/*
> >  		 * The LCPLL register should be turned on by the BIOS. For now




More information about the Intel-gfx mailing list