Re: [PATCH 8/9] drm/i915: Add location of the Rcomp resistor to bxt_ddi_phy_info

[Imre Deak <imre.deak@intel.com>]

On ke, 2016-10-05 at 15:09 +0300, Ander Conselvan de Oliveira wrote:
> Use struct bxt_ddi_phy_info to hold information of where the Rcomp
> resistor is located, instead of hard coding it in the init sequence.
> 
> Note that this moves the enabling of the phy with the Rcomp resistor out
> of the power well enable code. That should be safe since
> bxt_ddi_phy_init() is called while the power domains lock is held, and
> that is the only way that function gets called, so there is no
> possibility of a concurrent phy enable caused by a power domain get
> call.
> 
> Signed-off-by: Ander Conselvan de Oliveira 
> ---
>  drivers/gpu/drm/i915/intel_dpio_phy.c   | 76 +++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 15 -------
>  2 files changed, 59 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpio_phy.c b/drivers/gpu/drm/i915/intel_dpio_phy.c
> index 66d750a..e8a75fd 100644
> --- a/drivers/gpu/drm/i915/intel_dpio_phy.c
> +++ b/drivers/gpu/drm/i915/intel_dpio_phy.c
> @@ -124,6 +124,13 @@ struct bxt_ddi_phy_info {
>  	bool dual_channel;
>  
>  	/**
> +	 * @rcomp_phy: If -1, indicates this phy has its own rcomp resistor.
> +	 * Otherwise the GRC value will be copied from the phy indicated by
> +	 * this field.
> +	 */
> +	enum dpio_phy rcomp_phy;
> +
> +	/**
>  	 * @channel: struct containing per channel information.
>  	 */
>  	struct {
> @@ -137,6 +144,7 @@ struct bxt_ddi_phy_info {
>  static const struct bxt_ddi_phy_info bxt_ddi_phy_info[] = {
>  	[DPIO_PHY0] = {
>  		.dual_channel = true,
> +		.rcomp_phy = DPIO_PHY1,
>  
>  		.channel = {
>  			[DPIO_CH0] = { .port = PORT_B },
> @@ -145,6 +153,7 @@ static const struct bxt_ddi_phy_info bxt_ddi_phy_info[] = {
>  	},
>  	[DPIO_PHY1] = {
>  		.dual_channel = false,
> +		.rcomp_phy = -1,
>  
>  		.channel = {
>  			[DPIO_CH0] = { .port = PORT_A },
> @@ -152,6 +161,7 @@ static const struct bxt_ddi_phy_info bxt_ddi_phy_info[] = {
>  	},
>  };
>  
> +static u32 bxt_phy_port_mask(const struct bxt_ddi_phy_info *phy_info)
>  {
>  	return (phy_info->dual_channel * BIT(phy_info->channel[DPIO_CH1].port)) |
>  		BIT(phy_info->channel[DPIO_CH0].port);
> @@ -199,6 +209,7 @@ void bxt_ddi_phy_set_signal_level(struct drm_i915_private *dev_priv,
>  bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
>  			    enum dpio_phy phy)
>  {
> +	const struct bxt_ddi_phy_info *phy_info = &bxt_ddi_phy_info[phy];
>  	enum port port;
>  
>  	if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy)))
> @@ -212,9 +223,10 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
>  		return false;
>  	}
>  
> -	if (phy == DPIO_PHY1 &&
> -	    !(I915_READ(BXT_PORT_REF_DW3(DPIO_PHY1)) & GRC_DONE)) {
> -		DRM_DEBUG_DRIVER("DDI PHY 1 powered, but GRC isn't done\n");
> +	if (phy_info->rcomp_phy == -1 &&
> +	    !(I915_READ(BXT_PORT_REF_DW3(phy)) & GRC_DONE)) {
> +		DRM_DEBUG_DRIVER("DDI PHY %d powered, but GRC isn't done\n",
> +				 phy);
>  
>  		return false;
>  	}
> @@ -259,14 +271,15 @@ static void bxt_phy_wait_grc_done(struct drm_i915_private *dev_priv,
>  		DRM_ERROR("timeout waiting for PHY%d GRC\n", phy);
>  }
>  
> -void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
> +static void _bxt_ddi_phy_init(struct drm_i915_private *dev_priv,
> +			      enum dpio_phy phy)
>  {
>  	const struct bxt_ddi_phy_info *phy_info = &bxt_ddi_phy_info[phy];
>  	u32 val;
>  
>  	if (bxt_ddi_phy_is_enabled(dev_priv, phy)) {
>  		/* Still read out the GRC value for state verification */
> -		if (phy == DPIO_PHY0)
> +		if (phy_info->rcomp_phy != -1)
>  			dev_priv->bxt_phy_grc = bxt_get_grc(dev_priv, phy);
>  
>  		if (bxt_ddi_phy_verify_state(dev_priv, phy)) {
> @@ -336,30 +349,32 @@ void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
>  		val |= OCL2_LDOFUSE_PWR_DIS;
>  	I915_WRITE(BXT_PORT_CL1CM_DW30(phy), val);
>  
> -	if (phy == DPIO_PHY0) {
> +	if (phy_info->rcomp_phy != -1) {
>  		uint32_t grc_code;
>  		/*
>  		 * PHY0 isn't connected to an RCOMP resistor so copy over
>  		 * the corresponding calibrated value from PHY1, and disable
>  		 * the automatic calibration on PHY0.
>  		 */
> -		val = dev_priv->bxt_phy_grc = bxt_get_grc(dev_priv, DPIO_PHY1);
> +		val = dev_priv->bxt_phy_grc = bxt_get_grc(dev_priv,
> +							  phy_info->rcomp_phy);
>  		grc_code = val << GRC_CODE_FAST_SHIFT |
>  			   val << GRC_CODE_SLOW_SHIFT |
>  			   val;
> -		I915_WRITE(BXT_PORT_REF_DW6(DPIO_PHY0), grc_code);
> +		I915_WRITE(BXT_PORT_REF_DW6(phy), grc_code);
>  
> -		val = I915_READ(BXT_PORT_REF_DW8(DPIO_PHY0));
> +		val = I915_READ(BXT_PORT_REF_DW8(phy));
>  		val |= GRC_DIS | GRC_RDY_OVRD;
> -		I915_WRITE(BXT_PORT_REF_DW8(DPIO_PHY0), val);
> +		I915_WRITE(BXT_PORT_REF_DW8(phy), val);
>  	}
>  
>  	val = I915_READ(BXT_PHY_CTL_FAMILY(phy));
>  	val |= COMMON_RESET_DIS;
>  	I915_WRITE(BXT_PHY_CTL_FAMILY(phy), val);
>  
> -	if (phy == DPIO_PHY1)
> -		bxt_phy_wait_grc_done(dev_priv, DPIO_PHY1);
> +	if (phy_info->rcomp_phy == -1)
> +		bxt_phy_wait_grc_done(dev_priv, phy);
> +
>  }
>  
>  void bxt_ddi_phy_uninit(struct drm_i915_private *dev_priv, enum dpio_phy phy)
> @@ -375,6 +390,33 @@ void bxt_ddi_phy_uninit(struct drm_i915_private *dev_priv, enum dpio_phy phy)
>  	I915_WRITE(BXT_P_CR_GT_DISP_PWRON, val);
>  }
>  
> +/* This function assumes it is called from an intel_display_power_get() call,
> + * which would serialize any other attempts to enable the phy with the Rcomp
> + * resistor.
> + */

You could replace the above with a mutex locked assert.

> +void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
> +{
> +	const struct bxt_ddi_phy_info *phy_info = &bxt_ddi_phy_info[phy];
> +	enum dpio_phy rcomp_phy = phy_info->rcomp_phy;
> +	bool was_enabled;
> +
> +	if (rcomp_phy != -1) {
> +		was_enabled = bxt_ddi_phy_is_enabled(dev_priv, rcomp_phy);
> +
> +		/*
> +		 * We need to copy the GRC calibration value from rcomp_phy,
> +		 * so make sure it's powered up.
> +		 */
> +		if (!was_enabled)
> +			_bxt_ddi_phy_init(dev_priv, rcomp_phy);
> +	}
> +
> +	_bxt_ddi_phy_init(dev_priv, phy);
> +
> +	if (rcomp_phy != -1 && !was_enabled)
> +		bxt_ddi_phy_uninit(dev_priv, phy_info->rcomp_phy);
> +}

Nitpick: An alternative would be to add the power well dependency as a
custom power well field for the cmn power wells. We'd avoid the
unnecessary HW access in bxt_ddi_phy_is_enabled().

> +
>  static bool __printf(6, 7)
>  __phy_reg_verify_state(struct drm_i915_private *dev_priv, enum dpio_phy phy,
>  		       i915_reg_t reg, u32 mask, u32 expected,
> @@ -441,7 +483,7 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
>  	 * at least on stepping A this bit is read-only and fixed at 0.
>  	 */
>  
> -	if (phy == DPIO_PHY0) {
> +	if (phy_info->rcomp_phy != -1) {
>  		u32 grc_code = dev_priv->bxt_phy_grc;
>  
>  		grc_code = grc_code << GRC_CODE_FAST_SHIFT |
> @@ -449,12 +491,12 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
>  			   grc_code;
>  		mask = GRC_CODE_FAST_MASK | GRC_CODE_SLOW_MASK |
>  		       GRC_CODE_NOM_MASK;
> -		ok &= _CHK(BXT_PORT_REF_DW6(DPIO_PHY0), mask, grc_code,
> -			    "BXT_PORT_REF_DW6(%d)", DPIO_PHY0);
> +		ok &= _CHK(BXT_PORT_REF_DW6(phy), mask, grc_code,
> +			    "BXT_PORT_REF_DW6(%d)", phy);
>  
>  		mask = GRC_DIS | GRC_RDY_OVRD;
> -		ok &= _CHK(BXT_PORT_REF_DW8(DPIO_PHY0), mask, mask,
> -			    "BXT_PORT_REF_DW8(%d)", DPIO_PHY0);
> +		ok &= _CHK(BXT_PORT_REF_DW8(phy), mask, mask,
> +			    "BXT_PORT_REF_DW8(%d)", phy);
>  	}
>  
>  	return ok;
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index d41fd46..3b38998 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -849,22 +849,7 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
>  static void bxt_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>  					   struct i915_power_well *power_well)
>  {
> -	enum skl_disp_power_wells power_well_id = power_well->id;
> -	struct i915_power_well *cmn_a_well = NULL;
> -
> -	if (power_well_id == BXT_DPIO_CMN_BC) {
> -		/*
> -		 * We need to copy the GRC calibration value from the eDP PHY,
> -		 * so make sure it's powered up.
> -		 */
> -		cmn_a_well = lookup_power_well(dev_priv, BXT_DPIO_CMN_A);
> -		intel_power_well_get(dev_priv, cmn_a_well);
> -	}
> -
>  	bxt_ddi_phy_init(dev_priv, power_well->data);
> -
> -	if (cmn_a_well)
> -		intel_power_well_put(dev_priv, cmn_a_well);
>  }
>  
>  static void bxt_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx