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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2016-10-05 at 17:51 +0300, Imre Deak wrote:
> 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.

I'll do that and resend.

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

Since you gave me the option, I'll choose to ignore this one. What I like about
the current solution is that the dependency relationship between the phys is
captured in a single place, so I'd like to avoid duplicating that information in
the power well code.

I don't think the extra hardware access is performance critical, but if it were,
maybe we could track whether the phy is enabled in the phy code too?

Ander

> 
> > 
> > +
> >  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux