Re: [Intel-gfx] [PATCH] drm/i915/display: Increase AUX timeout for Type-C

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

Hi Suraj,

Thanks for the patch, please find my comments inline:


On 4/3/2023 1:31 PM, Suraj Kandpal wrote:
> Type-C PHYs are taking longer than expected for Aux IO Power Enabling.
> Workaround: Increase the timeout.
>
> WA: 14017271110

Lets use Wa_#### as per convention.

Also I am wondering if we should keep the original number 14017248603.

> Bspec: 55480
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>   .../drm/i915/display/intel_display_power_well.c   | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> index 62bafcbc7937..357617b9b725 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> @@ -252,6 +252,7 @@ static void hsw_wait_for_power_well_enable(struct drm_i915_private *dev_priv,
>   					   bool timeout_expected)
>   {
>   	const struct i915_power_well_regs *regs = power_well->desc->ops->regs;
> +	enum phy phy = icl_aux_pw_to_phy(dev_priv, power_well);
>   	int pw_idx = i915_power_well_instance(power_well)->hsw.idx;
>   
>   	/*
> @@ -264,6 +265,20 @@ static void hsw_wait_for_power_well_enable(struct drm_i915_private *dev_priv,
>   		return;
>   	}
>   
> +	/*
> +	 * WA: 14017271110

Wa_<number>:adlp


> +	 * Type-C Phy are taking longer than expected for AUX IO Power Enabling.
> +	 * Increase timeout to 500ms.
> +	 */
> +	if (IS_ALDERLAKE_P(dev_priv) && intel_phy_is_tc(dev_priv, phy)) {
> +		if (intel_de_wait_for_set(dev_priv, regs->driver,
> +					  HSW_PWR_WELL_CTL_STATE(pw_idx), 500)) {
> +			drm_dbg_kms(&dev_priv->drm, "%s power well enable timeout\n",
> +				    intel_power_well_name(power_well));
> +			return;
> +		}
> +	}
> +

Though not part of this workaround, as per Bspec:49294, the power well 
enable timeout value for Gen 12 seems to be 1.5 ms : ~2ms

May be have another change for that as well?

In my opinion, we can have an enable_timeout_ms which is set to 
different values based on platform and just

use that in intel_de_wait _for_set.


Regards,

Ankit

>   	/* Timeout for PW1:10 us, AUX:not specified, other PWs:20 us. */
>   	if (intel_de_wait_for_set(dev_priv, regs->driver,
>   				  HSW_PWR_WELL_CTL_STATE(pw_idx), 1)) {