Re: [PATCH 3/5] drm/i915/cnl+: Verify combo PHY HW state during PHY uninit

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

On Fri, Nov 02, 2018 at 11:25:58PM +0200, Souza, Jose wrote:
> On Fri, 2018-11-02 at 20:07 +0200, Imre Deak wrote:
> > Verify on CNL, ICL that the combo PHY HW state stayed intact after
> > PHY
> > initialization.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_combo_phy.c | 103
> > ++++++++++++++++++++++++++++++++-
> >  1 file changed, 101 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_combo_phy.c
> > b/drivers/gpu/drm/i915/intel_combo_phy.c
> > index 13184ae5a217..1522e2a25390 100644
> > --- a/drivers/gpu/drm/i915/intel_combo_phy.c
> > +++ b/drivers/gpu/drm/i915/intel_combo_phy.c
> > @@ -52,8 +52,8 @@ static const struct cnl_procmon {
> >   * registers, that's why we call the ICL macros even though the
> > function has CNL
> >   * on its name.
> >   */
> > -static void cnl_set_procmon_ref_values(struct drm_i915_private
> > *dev_priv,
> > -				       enum port port)
> > +static const struct cnl_procmon *
> > +cnl_get_procmon_ref_values(struct drm_i915_private *dev_priv, enum
> > port port)
> >  {
> >  	const struct cnl_procmon *procmon;
> >  	u32 val;
> > @@ -80,6 +80,17 @@ static void cnl_set_procmon_ref_values(struct
> > drm_i915_private *dev_priv,
> >  		break;
> >  	}
> >  
> > +	return procmon;
> > +}
> > +
> > +static void cnl_set_procmon_ref_values(struct drm_i915_private
> > *dev_priv,
> > +				       enum port port)
> > +{
> > +	const struct cnl_procmon *procmon;
> > +	u32 val;
> > +
> > +	procmon = cnl_get_procmon_ref_values(dev_priv, port);
> > +
> >  	val = I915_READ(ICL_PORT_COMP_DW1(port));
> >  	val &= ~((0xff << 16) | 0xff);
> >  	val |= procmon->dw1;
> > @@ -89,6 +100,63 @@ static void cnl_set_procmon_ref_values(struct
> > drm_i915_private *dev_priv,
> >  	I915_WRITE(ICL_PORT_COMP_DW10(port), procmon->dw10);
> >  }
> >  
> > +static bool check_phy_reg(struct drm_i915_private *dev_priv,
> > +			  enum port port, i915_reg_t reg, u32 mask,
> > +			  u32 expected_val)
> > +{
> > +	u32 val = I915_READ(reg);
> > +
> > +	if ((val & mask) != expected_val) {
> > +		DRM_DEBUG_DRIVER("Port-%c combo PHY reg %08x state
> > mismatch: "
> > +				 "current %08x mask %08x expected
> > %08x\n",
> > +				 port_name(port),
> > +				 reg.reg, val, mask, expected_val);
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static bool cnl_verify_procmon_ref_values(struct drm_i915_private
> > *dev_priv,
> > +					  enum port port)
> > +{
> > +	const struct cnl_procmon *procmon;
> > +	bool ret;
> > +
> > +	procmon = cnl_get_procmon_ref_values(dev_priv, port);
> > +
> > +	ret = check_phy_reg(dev_priv, port, ICL_PORT_COMP_DW1(port),
> > +			    (0xff << 16) | 0xff, procmon->dw1);
> > +	ret &= check_phy_reg(dev_priv, port, ICL_PORT_COMP_DW9(port),
> > +			     -1U, procmon->dw9);
> > +	ret &= check_phy_reg(dev_priv, port, ICL_PORT_COMP_DW10(port),
> > +			     -1U, procmon->dw10);
> > +
> > +	return ret;
> > +}
> > +
> > +static bool cnl_combo_phy_enabled(struct drm_i915_private *dev_priv)
> > +{
> > +	return !(I915_READ(CHICKEN_MISC_2) & CNL_COMP_PWR_DOWN) &&
> > +		(I915_READ(CNL_PORT_COMP_DW0) & COMP_INIT);
> 
> 
> Minor but would be better add parenthesis in the first part:
> 
> return (!(I915_READ(CHICKEN_MISC_2) & CNL_COMP_PWR_DOWN)) &&
> (I915_READ(CNL_PORT_COMP_DW0) & COMP_INIT);

That's overdoing it imo. Consider that you wouldn't do the same for
!a && b

> 
> > +}
> > +
> > +static bool cnl_combo_phy_verify_state(struct drm_i915_private
> > *dev_priv)
> > +{
> > +	enum port port = PORT_A;
> > +	bool ret;
> > +
> > +	if (!cnl_combo_phy_enabled(dev_priv))
> > +		return false;
> > +
> > +	ret = cnl_verify_procmon_ref_values(dev_priv, port);
> > +
> > +	ret &= check_phy_reg(dev_priv, port, CNL_PORT_CL1CM_DW5,
> > +			     CL_POWER_DOWN_ENABLE,
> > CL_POWER_DOWN_ENABLE);
> > +
> > +	return ret;
> > +}
> > +
> >  void cnl_combo_phys_init(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 val;
> > @@ -113,11 +181,38 @@ void cnl_combo_phys_uninit(struct
> > drm_i915_private *dev_priv)
> >  {
> >  	u32 val;
> >  
> > +	if (!cnl_combo_phy_verify_state(dev_priv))
> > +		DRM_WARN("Combo PHY HW state changed unexpectedly.\n");
> > +
> >  	val = I915_READ(CHICKEN_MISC_2);
> >  	val |= CNL_COMP_PWR_DOWN;
> >  	I915_WRITE(CHICKEN_MISC_2, val);
> >  }
> >  
> > +static bool icl_combo_phy_enabled(struct drm_i915_private *dev_priv,
> > +				  enum port port)
> > +{
> > +	return !(I915_READ(ICL_PHY_MISC(port)) &
> > +		 ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN) &&
> > +		(I915_READ(ICL_PORT_COMP_DW0(port)) & COMP_INIT);
> 
> Same in here:
> 
> return (!(I915_READ(ICL_PHY_MISC(port)) &
> ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN)) &&
> (I915_READ(ICL_PORT_COMP_DW0(port)) & COMP_INIT);
> 
> Other than that:
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> 
> > +}
> > +
> > +static bool icl_combo_phy_verify_state(struct drm_i915_private
> > *dev_priv,
> > +				       enum port port)
> > +{
> > +	bool ret;
> > +
> > +	if (!icl_combo_phy_enabled(dev_priv, port))
> > +		return false;
> > +
> > +	ret = cnl_verify_procmon_ref_values(dev_priv, port);
> > +
> > +	ret &= check_phy_reg(dev_priv, port, ICL_PORT_CL_DW5(port),
> > +			     CL_POWER_DOWN_ENABLE,
> > CL_POWER_DOWN_ENABLE);
> > +
> > +	return ret;
> > +}
> > +
> >  void icl_combo_phys_init(struct drm_i915_private *dev_priv)
> >  {
> >  	enum port port;
> > @@ -148,6 +243,10 @@ void icl_combo_phys_uninit(struct
> > drm_i915_private *dev_priv)
> >  	for (port = PORT_A; port <= PORT_B; port++) {
> >  		u32 val;
> >  
> > +		if (!icl_combo_phy_verify_state(dev_priv, port))
> > +			DRM_WARN("Port-%c combo PHY HW state changed
> > unexpectedly\n",
> > +				 port_name(port));
> > +
> >  		val = I915_READ(ICL_PHY_MISC(port));
> >  		val |= ICL_PHY_MISC_DE_IO_COMP_PWR_DOWN;
> >  		I915_WRITE(ICL_PHY_MISC(port), val);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx