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

[Souza, Jose]

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 at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: José Roberto de Souza <jose.souza at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Signed-off-by: Imre Deak <imre.deak at 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);

> +}
> +
> +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 at 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);