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
