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.
> }
>
> + *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);
>
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
