diff mbox series

[PATCHv2,15/22] drm/bridge: tc358767: clean-up link training

Message ID 20190326103146.24795-16-tomi.valkeinen@ti.com
State Superseded
Headers show
Series drm/bridge: tc358767: DP support | expand

Commit Message

Tomi Valkeinen March 26, 2019, 10:31 a.m. UTC 
The current link training code does unnecessary retry-loops, and does
extra writes to the registers. It is easier to follow the flow and
ensure it's similar to Toshiba's documentation if we deal with LT inside
tc_main_link_enable() function.

This patch adds tc_wait_link_training() which handles waiting for the LT
phase to finish, and does the necessary LT register setups in
tc_main_link_enable, without extra loops.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 129 +++++++++++++-----------------
 1 file changed, 57 insertions(+), 72 deletions(-)

Comments

Andrzej Hajda April 15, 2019, 9:54 a.m. UTC | #1 
On 26.03.2019 11:31, Tomi Valkeinen wrote:
> The current link training code does unnecessary retry-loops, and does
> extra writes to the registers. It is easier to follow the flow and
> ensure it's similar to Toshiba's documentation if we deal with LT inside
> tc_main_link_enable() function.
>
> This patch adds tc_wait_link_training() which handles waiting for the LT
> phase to finish, and does the necessary LT register setups in
> tc_main_link_enable, without extra loops.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>


Btw, there is other patchset which touches the same things, have you
decided which one should go first? yours or Andrey's?


 --
Regards
Andrzej


> ---
>  drivers/gpu/drm/bridge/tc358767.c | 129 +++++++++++++-----------------
>  1 file changed, 57 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 220408db82f7..1c61f6205e43 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -740,83 +740,25 @@ static int tc_set_video_mode(struct tc_data *tc,
>  	return ret;
>  }
>  
> -static int tc_link_training(struct tc_data *tc, int pattern)
> +static int tc_wait_link_training(struct tc_data *tc, u32 *error)
>  {
> -	const char * const *errors;
> -	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
> -		      DP0_SRCCTRL_AUTOCORRECT;
> -	int timeout;
> -	int retry;
> +	u32 timeout = 1000;
>  	u32 value;
>  	int ret;
>  
> -	if (pattern == DP_TRAINING_PATTERN_1) {
> -		srcctrl |= DP0_SRCCTRL_TP1;
> -		errors = training_pattern1_errors;
> -	} else {
> -		srcctrl |= DP0_SRCCTRL_TP2;
> -		errors = training_pattern2_errors;
> -	}
> -
> -	/* Set DPCD 0x102 for Training Part 1 or 2 */
> -	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | pattern);
> -
> -	tc_write(DP0_LTLOOPCTRL,
> -		 (0x0f << 28) |	/* Defer Iteration Count */
> -		 (0x0f << 24) |	/* Loop Iteration Count */
> -		 (0x0d << 0));	/* Loop Timer Delay */
> -
> -	retry = 5;
>  	do {
> -		/* Set DP0 Training Pattern */
> -		tc_write(DP0_SRCCTRL, srcctrl);
> -
> -		/* Enable DP0 to start Link Training */
> -		tc_write(DP0CTL, DP_EN);
> -
> -		/* wait */
> -		timeout = 1000;
> -		do {
> -			tc_read(DP0_LTSTAT, &value);
> -			udelay(1);
> -		} while ((!(value & LT_LOOPDONE)) && (--timeout));
> -		if (timeout == 0) {
> -			dev_err(tc->dev, "Link training timeout!\n");
> -		} else {
> -			int pattern = (value >> 11) & 0x3;
> -			int error = (value >> 8) & 0x7;
> -
> -			dev_dbg(tc->dev,
> -				"Link training phase %d done after %d uS: %s\n",
> -				pattern, 1000 - timeout, errors[error]);
> -			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
> -				break;
> -			if (pattern == DP_TRAINING_PATTERN_2) {
> -				value &= LT_CHANNEL1_EQ_BITS |
> -					 LT_INTERLANE_ALIGN_DONE |
> -					 LT_CHANNEL0_EQ_BITS;
> -				/* in case of two lanes */
> -				if ((tc->link.base.num_lanes == 2) &&
> -				    (value == (LT_CHANNEL1_EQ_BITS |
> -					       LT_INTERLANE_ALIGN_DONE |
> -					       LT_CHANNEL0_EQ_BITS)))
> -					break;
> -				/* in case of one line */
> -				if ((tc->link.base.num_lanes == 1) &&
> -				    (value == (LT_INTERLANE_ALIGN_DONE |
> -					       LT_CHANNEL0_EQ_BITS)))
> -					break;
> -			}
> -		}
> -		/* restart */
> -		tc_write(DP0CTL, 0);
> -		usleep_range(10, 20);
> -	} while (--retry);
> -	if (retry == 0) {
> -		dev_err(tc->dev, "Failed to finish training phase %d\n",
> -			pattern);
> +		udelay(1);
> +		tc_read(DP0_LTSTAT, &value);
> +	} while ((!(value & LT_LOOPDONE)) && (--timeout));
> +
> +	if (timeout == 0) {
> +		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
> +		ret = -ETIMEDOUT;
> +		goto err;
>  	}
>  
> +	*error = (value >> 8) & 0x7;
> +
>  	return 0;
>  err:
>  	return ret;
> @@ -832,6 +774,7 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	u32 value;
>  	int ret;
>  	u8 tmp[8];
> +	u32 error;
>  
>  	/* display mode should be set at this point */
>  	if (!tc->mode)
> @@ -950,14 +893,56 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	if (ret < 0)
>  		goto err_dpcd_write;
>  
> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
> +	/* LINK TRAINING PATTERN 1 */
> +
> +	/* Set DPCD 0x102 for Training Pattern 1 */
> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
> +
> +	tc_write(DP0_LTLOOPCTRL,
> +		 (15 << 28) |	/* Defer Iteration Count */
> +		 (15 << 24) |	/* Loop Iteration Count */
> +		 (0xd << 0));	/* Loop Timer Delay */
> +
> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
> +		 DP0_SRCCTRL_TP1);
> +
> +	/* Enable DP0 to start Link Training */
> +	tc_write(DP0CTL,
> +		 ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
> +		 DP_EN);
> +
> +	/* wait */
> +	ret = tc_wait_link_training(tc, &error);
>  	if (ret)
>  		goto err;
>  
> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
> +	if (error) {
> +		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
> +			training_pattern1_errors[error]);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	/* LINK TRAINING PATTERN 2 */
> +
> +	/* Set DPCD 0x102 for Training Pattern 2 */
> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
> +
> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
> +		 DP0_SRCCTRL_TP2);
> +
> +	/* wait */
> +	ret = tc_wait_link_training(tc, &error);
>  	if (ret)
>  		goto err;
>  
> +	if (error) {
> +		dev_err(tc->dev, "Link training phase 2 failed: %s\n",
> +			training_pattern2_errors[error]);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
>  	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
>  	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
>
Tomi Valkeinen April 15, 2019, 11:03 a.m. UTC | #2 
On 15/04/2019 12:54, Andrzej Hajda wrote:
> On 26.03.2019 11:31, Tomi Valkeinen wrote:
>> The current link training code does unnecessary retry-loops, and does
>> extra writes to the registers. It is easier to follow the flow and
>> ensure it's similar to Toshiba's documentation if we deal with LT inside
>> tc_main_link_enable() function.
>>
>> This patch adds tc_wait_link_training() which handles waiting for the LT
>> phase to finish, and does the necessary LT register setups in
>> tc_main_link_enable, without extra loops.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> 
> Btw, there is other patchset which touches the same things, have you
> decided which one should go first? yours or Andrey's?

Yes, we discussed that in "[PATCH v2 05/15] drm/bridge: tc358767: Drop
custom tc_write()/tc_read() accessors" thread. Let's merge mine first,
and Andrey rebases on top of that.

 Tomi
Laurent Pinchart April 20, 2019, 10:13 p.m. UTC | #3 
Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:39PM +0200, Tomi Valkeinen wrote:
> The current link training code does unnecessary retry-loops, and does
> extra writes to the registers. It is easier to follow the flow and
> ensure it's similar to Toshiba's documentation if we deal with LT inside
> tc_main_link_enable() function.
> 
> This patch adds tc_wait_link_training() which handles waiting for the LT
> phase to finish, and does the necessary LT register setups in
> tc_main_link_enable, without extra loops.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 129 +++++++++++++-----------------
>  1 file changed, 57 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 220408db82f7..1c61f6205e43 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -740,83 +740,25 @@ static int tc_set_video_mode(struct tc_data *tc,
>  	return ret;
>  }
>  
> -static int tc_link_training(struct tc_data *tc, int pattern)
> +static int tc_wait_link_training(struct tc_data *tc, u32 *error)
>  {
> -	const char * const *errors;
> -	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
> -		      DP0_SRCCTRL_AUTOCORRECT;
> -	int timeout;
> -	int retry;
> +	u32 timeout = 1000;
>  	u32 value;
>  	int ret;
>  
> -	if (pattern == DP_TRAINING_PATTERN_1) {
> -		srcctrl |= DP0_SRCCTRL_TP1;
> -		errors = training_pattern1_errors;
> -	} else {
> -		srcctrl |= DP0_SRCCTRL_TP2;
> -		errors = training_pattern2_errors;
> -	}
> -
> -	/* Set DPCD 0x102 for Training Part 1 or 2 */
> -	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | pattern);
> -
> -	tc_write(DP0_LTLOOPCTRL,
> -		 (0x0f << 28) |	/* Defer Iteration Count */
> -		 (0x0f << 24) |	/* Loop Iteration Count */
> -		 (0x0d << 0));	/* Loop Timer Delay */
> -
> -	retry = 5;
>  	do {
> -		/* Set DP0 Training Pattern */
> -		tc_write(DP0_SRCCTRL, srcctrl);
> -
> -		/* Enable DP0 to start Link Training */
> -		tc_write(DP0CTL, DP_EN);
> -
> -		/* wait */
> -		timeout = 1000;
> -		do {
> -			tc_read(DP0_LTSTAT, &value);
> -			udelay(1);
> -		} while ((!(value & LT_LOOPDONE)) && (--timeout));
> -		if (timeout == 0) {
> -			dev_err(tc->dev, "Link training timeout!\n");
> -		} else {
> -			int pattern = (value >> 11) & 0x3;
> -			int error = (value >> 8) & 0x7;
> -
> -			dev_dbg(tc->dev,
> -				"Link training phase %d done after %d uS: %s\n",
> -				pattern, 1000 - timeout, errors[error]);
> -			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
> -				break;
> -			if (pattern == DP_TRAINING_PATTERN_2) {
> -				value &= LT_CHANNEL1_EQ_BITS |
> -					 LT_INTERLANE_ALIGN_DONE |
> -					 LT_CHANNEL0_EQ_BITS;
> -				/* in case of two lanes */
> -				if ((tc->link.base.num_lanes == 2) &&
> -				    (value == (LT_CHANNEL1_EQ_BITS |
> -					       LT_INTERLANE_ALIGN_DONE |
> -					       LT_CHANNEL0_EQ_BITS)))
> -					break;
> -				/* in case of one line */
> -				if ((tc->link.base.num_lanes == 1) &&
> -				    (value == (LT_INTERLANE_ALIGN_DONE |
> -					       LT_CHANNEL0_EQ_BITS)))
> -					break;
> -			}
> -		}
> -		/* restart */
> -		tc_write(DP0CTL, 0);
> -		usleep_range(10, 20);
> -	} while (--retry);
> -	if (retry == 0) {
> -		dev_err(tc->dev, "Failed to finish training phase %d\n",
> -			pattern);
> +		udelay(1);
> +		tc_read(DP0_LTSTAT, &value);
> +	} while ((!(value & LT_LOOPDONE)) && (--timeout));
> +
> +	if (timeout == 0) {
> +		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
> +		ret = -ETIMEDOUT;
> +		goto err;

You can return -ETIMEDOUT directly.

>  	}
>  
> +	*error = (value >> 8) & 0x7;

How about returning the status through the return value instead ? The
status will never be negative, so it won't clash with -ETIMEDOUT.

> +
>  	return 0;
>  err:
>  	return ret;
> @@ -832,6 +774,7 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	u32 value;
>  	int ret;
>  	u8 tmp[8];
> +	u32 error;
>  
>  	/* display mode should be set at this point */
>  	if (!tc->mode)
> @@ -950,14 +893,56 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	if (ret < 0)
>  		goto err_dpcd_write;
>  
> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
> +	/* LINK TRAINING PATTERN 1 */
> +
> +	/* Set DPCD 0x102 for Training Pattern 1 */
> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
> +
> +	tc_write(DP0_LTLOOPCTRL,
> +		 (15 << 28) |	/* Defer Iteration Count */
> +		 (15 << 24) |	/* Loop Iteration Count */
> +		 (0xd << 0));	/* Loop Timer Delay */

While at it, could you define macros for these bits ?

> +
> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
> +		 DP0_SRCCTRL_TP1);

Let's break long lines (here and in other locations in this patch).

> +
> +	/* Enable DP0 to start Link Training */
> +	tc_write(DP0CTL,
> +		 ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
> +		 DP_EN);
> +
> +	/* wait */
> +	ret = tc_wait_link_training(tc, &error);
>  	if (ret)
>  		goto err;
>  
> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
> +	if (error) {
> +		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
> +			training_pattern1_errors[error]);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	/* LINK TRAINING PATTERN 2 */

Do phase 1 and phase 2 correspond to clock recovery and channel
equalization ? If so I would mention that instead of just training
pattern 1 and 2.

> +
> +	/* Set DPCD 0x102 for Training Pattern 2 */
> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
> +
> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
> +		 DP0_SRCCTRL_TP2);
> +

This channel equalization sequence of link training has a retry loop of
5 iterations. It that performed internally by the TC358767 ?

> +	/* wait */
> +	ret = tc_wait_link_training(tc, &error);
>  	if (ret)
>  		goto err;
>  
> +	if (error) {
> +		dev_err(tc->dev, "Link training phase 2 failed: %s\n",
> +			training_pattern2_errors[error]);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
>  	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
>  	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
>
Tomi Valkeinen May 3, 2019, 8:37 a.m. UTC | #4 
On 21/04/2019 01:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 26, 2019 at 12:31:39PM +0200, Tomi Valkeinen wrote:
>> The current link training code does unnecessary retry-loops, and does
>> extra writes to the registers. It is easier to follow the flow and
>> ensure it's similar to Toshiba's documentation if we deal with LT inside
>> tc_main_link_enable() function.
>>
>> This patch adds tc_wait_link_training() which handles waiting for the LT
>> phase to finish, and does the necessary LT register setups in
>> tc_main_link_enable, without extra loops.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358767.c | 129 +++++++++++++-----------------
>>  1 file changed, 57 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 220408db82f7..1c61f6205e43 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -740,83 +740,25 @@ static int tc_set_video_mode(struct tc_data *tc,
>>  	return ret;
>>  }
>>  
>> -static int tc_link_training(struct tc_data *tc, int pattern)
>> +static int tc_wait_link_training(struct tc_data *tc, u32 *error)
>>  {
>> -	const char * const *errors;
>> -	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
>> -		      DP0_SRCCTRL_AUTOCORRECT;
>> -	int timeout;
>> -	int retry;
>> +	u32 timeout = 1000;
>>  	u32 value;
>>  	int ret;
>>  
>> -	if (pattern == DP_TRAINING_PATTERN_1) {
>> -		srcctrl |= DP0_SRCCTRL_TP1;
>> -		errors = training_pattern1_errors;
>> -	} else {
>> -		srcctrl |= DP0_SRCCTRL_TP2;
>> -		errors = training_pattern2_errors;
>> -	}
>> -
>> -	/* Set DPCD 0x102 for Training Part 1 or 2 */
>> -	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | pattern);
>> -
>> -	tc_write(DP0_LTLOOPCTRL,
>> -		 (0x0f << 28) |	/* Defer Iteration Count */
>> -		 (0x0f << 24) |	/* Loop Iteration Count */
>> -		 (0x0d << 0));	/* Loop Timer Delay */
>> -
>> -	retry = 5;
>>  	do {
>> -		/* Set DP0 Training Pattern */
>> -		tc_write(DP0_SRCCTRL, srcctrl);
>> -
>> -		/* Enable DP0 to start Link Training */
>> -		tc_write(DP0CTL, DP_EN);
>> -
>> -		/* wait */
>> -		timeout = 1000;
>> -		do {
>> -			tc_read(DP0_LTSTAT, &value);
>> -			udelay(1);
>> -		} while ((!(value & LT_LOOPDONE)) && (--timeout));
>> -		if (timeout == 0) {
>> -			dev_err(tc->dev, "Link training timeout!\n");
>> -		} else {
>> -			int pattern = (value >> 11) & 0x3;
>> -			int error = (value >> 8) & 0x7;
>> -
>> -			dev_dbg(tc->dev,
>> -				"Link training phase %d done after %d uS: %s\n",
>> -				pattern, 1000 - timeout, errors[error]);
>> -			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
>> -				break;
>> -			if (pattern == DP_TRAINING_PATTERN_2) {
>> -				value &= LT_CHANNEL1_EQ_BITS |
>> -					 LT_INTERLANE_ALIGN_DONE |
>> -					 LT_CHANNEL0_EQ_BITS;
>> -				/* in case of two lanes */
>> -				if ((tc->link.base.num_lanes == 2) &&
>> -				    (value == (LT_CHANNEL1_EQ_BITS |
>> -					       LT_INTERLANE_ALIGN_DONE |
>> -					       LT_CHANNEL0_EQ_BITS)))
>> -					break;
>> -				/* in case of one line */
>> -				if ((tc->link.base.num_lanes == 1) &&
>> -				    (value == (LT_INTERLANE_ALIGN_DONE |
>> -					       LT_CHANNEL0_EQ_BITS)))
>> -					break;
>> -			}
>> -		}
>> -		/* restart */
>> -		tc_write(DP0CTL, 0);
>> -		usleep_range(10, 20);
>> -	} while (--retry);
>> -	if (retry == 0) {
>> -		dev_err(tc->dev, "Failed to finish training phase %d\n",
>> -			pattern);
>> +		udelay(1);
>> +		tc_read(DP0_LTSTAT, &value);
>> +	} while ((!(value & LT_LOOPDONE)) && (--timeout));
>> +
>> +	if (timeout == 0) {
>> +		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
>> +		ret = -ETIMEDOUT;
>> +		goto err;
> 
> You can return -ETIMEDOUT directly.

Yep.

>>  	}
>>  
>> +	*error = (value >> 8) & 0x7;
> 
> How about returning the status through the return value instead ? The
> status will never be negative, so it won't clash with -ETIMEDOUT.

Hmm, yes, I think that makes sense here.

>> +
>>  	return 0;
>>  err:
>>  	return ret;
>> @@ -832,6 +774,7 @@ static int tc_main_link_enable(struct tc_data *tc)
>>  	u32 value;
>>  	int ret;
>>  	u8 tmp[8];
>> +	u32 error;
>>  
>>  	/* display mode should be set at this point */
>>  	if (!tc->mode)
>> @@ -950,14 +893,56 @@ static int tc_main_link_enable(struct tc_data *tc)
>>  	if (ret < 0)
>>  		goto err_dpcd_write;
>>  
>> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
>> +	/* LINK TRAINING PATTERN 1 */
>> +
>> +	/* Set DPCD 0x102 for Training Pattern 1 */
>> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
>> +
>> +	tc_write(DP0_LTLOOPCTRL,
>> +		 (15 << 28) |	/* Defer Iteration Count */
>> +		 (15 << 24) |	/* Loop Iteration Count */
>> +		 (0xd << 0));	/* Loop Timer Delay */
> 
> While at it, could you define macros for these bits ?

For the shifts? We don't have macros for almost any of the shifts. I'd
rather leave such changes for later. This one is just a copy of the
existing code.

>> +
>> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
>> +		 DP0_SRCCTRL_TP1);
> 
> Let's break long lines (here and in other locations in this patch).

Ok.

>> +
>> +	/* Enable DP0 to start Link Training */
>> +	tc_write(DP0CTL,
>> +		 ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
>> +		 DP_EN);
>> +
>> +	/* wait */
>> +	ret = tc_wait_link_training(tc, &error);
>>  	if (ret)
>>  		goto err;
>>  
>> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
>> +	if (error) {
>> +		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
>> +			training_pattern1_errors[error]);
>> +		ret = -ENODEV;
>> +		goto err;
>> +	}
>> +
>> +	/* LINK TRAINING PATTERN 2 */
> 
> Do phase 1 and phase 2 correspond to clock recovery and channel
> equalization ? If so I would mention that instead of just training
> pattern 1 and 2.

Yes. All the docs talk about pattern 1 and 2, including DP 1.1a doc. But
I agree, probably these comments should talk about clock recovery and
channel eq.

>> +
>> +	/* Set DPCD 0x102 for Training Pattern 2 */
>> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
>> +
>> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
>> +		 DP0_SRCCTRL_TP2);
>> +
> 
> This channel equalization sequence of link training has a retry loop of
> 5 iterations. It that performed internally by the TC358767 ?

Yes, that is my understanding. It's the "Loop Iteration Count" in
DP0_LTLOOPCTRL.

 Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 220408db82f7..1c61f6205e43 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -740,83 +740,25 @@  static int tc_set_video_mode(struct tc_data *tc,
 	return ret;
 }
 
-static int tc_link_training(struct tc_data *tc, int pattern)
+static int tc_wait_link_training(struct tc_data *tc, u32 *error)
 {
-	const char * const *errors;
-	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
-		      DP0_SRCCTRL_AUTOCORRECT;
-	int timeout;
-	int retry;
+	u32 timeout = 1000;
 	u32 value;
 	int ret;
 
-	if (pattern == DP_TRAINING_PATTERN_1) {
-		srcctrl |= DP0_SRCCTRL_TP1;
-		errors = training_pattern1_errors;
-	} else {
-		srcctrl |= DP0_SRCCTRL_TP2;
-		errors = training_pattern2_errors;
-	}
-
-	/* Set DPCD 0x102 for Training Part 1 or 2 */
-	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | pattern);
-
-	tc_write(DP0_LTLOOPCTRL,
-		 (0x0f << 28) |	/* Defer Iteration Count */
-		 (0x0f << 24) |	/* Loop Iteration Count */
-		 (0x0d << 0));	/* Loop Timer Delay */
-
-	retry = 5;
 	do {
-		/* Set DP0 Training Pattern */
-		tc_write(DP0_SRCCTRL, srcctrl);
-
-		/* Enable DP0 to start Link Training */
-		tc_write(DP0CTL, DP_EN);
-
-		/* wait */
-		timeout = 1000;
-		do {
-			tc_read(DP0_LTSTAT, &value);
-			udelay(1);
-		} while ((!(value & LT_LOOPDONE)) && (--timeout));
-		if (timeout == 0) {
-			dev_err(tc->dev, "Link training timeout!\n");
-		} else {
-			int pattern = (value >> 11) & 0x3;
-			int error = (value >> 8) & 0x7;
-
-			dev_dbg(tc->dev,
-				"Link training phase %d done after %d uS: %s\n",
-				pattern, 1000 - timeout, errors[error]);
-			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
-				break;
-			if (pattern == DP_TRAINING_PATTERN_2) {
-				value &= LT_CHANNEL1_EQ_BITS |
-					 LT_INTERLANE_ALIGN_DONE |
-					 LT_CHANNEL0_EQ_BITS;
-				/* in case of two lanes */
-				if ((tc->link.base.num_lanes == 2) &&
-				    (value == (LT_CHANNEL1_EQ_BITS |
-					       LT_INTERLANE_ALIGN_DONE |
-					       LT_CHANNEL0_EQ_BITS)))
-					break;
-				/* in case of one line */
-				if ((tc->link.base.num_lanes == 1) &&
-				    (value == (LT_INTERLANE_ALIGN_DONE |
-					       LT_CHANNEL0_EQ_BITS)))
-					break;
-			}
-		}
-		/* restart */
-		tc_write(DP0CTL, 0);
-		usleep_range(10, 20);
-	} while (--retry);
-	if (retry == 0) {
-		dev_err(tc->dev, "Failed to finish training phase %d\n",
-			pattern);
+		udelay(1);
+		tc_read(DP0_LTSTAT, &value);
+	} while ((!(value & LT_LOOPDONE)) && (--timeout));
+
+	if (timeout == 0) {
+		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
+		ret = -ETIMEDOUT;
+		goto err;
 	}
 
+	*error = (value >> 8) & 0x7;
+
 	return 0;
 err:
 	return ret;
@@ -832,6 +774,7 @@  static int tc_main_link_enable(struct tc_data *tc)
 	u32 value;
 	int ret;
 	u8 tmp[8];
+	u32 error;
 
 	/* display mode should be set at this point */
 	if (!tc->mode)
@@ -950,14 +893,56 @@  static int tc_main_link_enable(struct tc_data *tc)
 	if (ret < 0)
 		goto err_dpcd_write;
 
-	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
+	/* LINK TRAINING PATTERN 1 */
+
+	/* Set DPCD 0x102 for Training Pattern 1 */
+	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
+
+	tc_write(DP0_LTLOOPCTRL,
+		 (15 << 28) |	/* Defer Iteration Count */
+		 (15 << 24) |	/* Loop Iteration Count */
+		 (0xd << 0));	/* Loop Timer Delay */
+
+	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
+		 DP0_SRCCTRL_TP1);
+
+	/* Enable DP0 to start Link Training */
+	tc_write(DP0CTL,
+		 ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
+		 DP_EN);
+
+	/* wait */
+	ret = tc_wait_link_training(tc, &error);
 	if (ret)
 		goto err;
 
-	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
+	if (error) {
+		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
+			training_pattern1_errors[error]);
+		ret = -ENODEV;
+		goto err;
+	}
+
+	/* LINK TRAINING PATTERN 2 */
+
+	/* Set DPCD 0x102 for Training Pattern 2 */
+	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
+
+	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
+		 DP0_SRCCTRL_TP2);
+
+	/* wait */
+	ret = tc_wait_link_training(tc, &error);
 	if (ret)
 		goto err;
 
+	if (error) {
+		dev_err(tc->dev, "Link training phase 2 failed: %s\n",
+			training_pattern2_errors[error]);
+		ret = -ENODEV;
+		goto err;
+	}
+
 	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
 	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);