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

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

 



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@xxxxxx>
>> ---
>>  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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux