Re: [PATCH 30/49] drm/i915/bxt: add display initialize/uninitialize sequence

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Mar 17, 2015 at 11:39:56AM +0200, Imre Deak wrote:
> From: Vandana Kannan <vandana.kannan@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@intel.com> (v2)
> Signed-off-by: Imre Deak <imre.deak@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.

> +
> +#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.

> +
> +#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.

> +#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.

> +
> +#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.

> +
> +#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.

> +#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

> +				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.

> +
> +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.

> +
<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?

> +
> +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
-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx