On Thu, Nov 1, 2012 at 1:35 AM, Jingoo Han <jg1.han@xxxxxxxxxxx> wrote: > On Thursday, November 01, 2012 1:55 AM Sean Paul wrote >> >> Clean up some of the SW training code to make it more clear and reduce >> duplicate code. >> >> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx> >> --- >> drivers/video/exynos/exynos_dp_core.c | 279 +++++++++++++-------------------- >> 1 files changed, 112 insertions(+), 167 deletions(-) >> >> Thanks for the pointer. There are still places where the code can be either >> simplified, or duplication removed. > > Removing duplication is good, but don't change the Link training sequence. > Link training sequence is very sensitive and tricky. > I definitely appreciate how tricky it is :) I didn't actually change any of the functionality from the original code. I noticed you made a couple of functional changes in your clean-up patch (http://www.spinics.net/lists/linux-fbdev/msg06849.html). I assumed that these functional changes were no-ops since bug fixes would have gone in separate patches. I've also done a fair bit of testing to ensure it works. > I will modify your patch and I will submit new patch. > More comments below. > Best regards, > Jingoo Han > >> >> Below is a rebased patch for your review. >> >> Sean >> >> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c >> index 44820f2..b126e8a 100644 >> --- a/drivers/video/exynos/exynos_dp_core.c >> +++ b/drivers/video/exynos/exynos_dp_core.c >> @@ -276,7 +276,7 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp) >> >> /* Set sink to D0 (Sink Not Ready) mode. */ >> retval = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE, >> - DPCD_SET_POWER_STATE_D0); >> + DPCD_SET_POWER_STATE_D0); >> if (retval) >> return retval; >> >> @@ -301,17 +301,18 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp) >> exynos_dp_set_training_pattern(dp, TRAINING_PTN1); >> >> /* Set RX training pattern */ >> - exynos_dp_write_byte_to_dpcd(dp, >> - DPCD_ADDR_TRAINING_PATTERN_SET, >> - DPCD_SCRAMBLING_DISABLED | >> - DPCD_TRAINING_PATTERN_1); >> + retval = exynos_dp_write_byte_to_dpcd(dp, >> + DPCD_ADDR_TRAINING_PATTERN_SET, >> + DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_1); >> + if (retval) >> + return retval; >> >> for (lane = 0; lane < lane_count; lane++) >> buf[lane] = DPCD_PRE_EMPHASIS_PATTERN2_LEVEL0 | >> DPCD_VOLTAGE_SWING_PATTERN1_LEVEL0; >> - retval = exynos_dp_write_bytes_to_dpcd(dp, >> - DPCD_ADDR_TRAINING_LANE0_SET, >> - lane_count, buf); >> + >> + retval = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_TRAINING_LANE0_SET, >> + lane_count, buf); >> >> return retval; >> } >> @@ -337,18 +338,17 @@ static int exynos_dp_clock_recovery_ok(u8 link_status[2], int lane_count) >> return 0; >> } >> >> -static int exynos_dp_channel_eq_ok(u8 link_align[3], int lane_count) >> +static int exynos_dp_channel_eq_ok(u8 link_status[2], u8 link_align, >> + int lane_count) >> { >> int lane; >> - u8 lane_align; >> u8 lane_status; >> >> - lane_align = link_align[2]; >> - if ((lane_align & DPCD_INTERLANE_ALIGN_DONE) == 0) >> + if ((link_align & DPCD_INTERLANE_ALIGN_DONE) == 0) >> return -EINVAL; >> >> for (lane = 0; lane < lane_count; lane++) { >> - lane_status = exynos_dp_get_lane_status(link_align, lane); >> + lane_status = exynos_dp_get_lane_status(link_status, lane); >> lane_status &= DPCD_CHANNEL_EQ_BITS; >> if (lane_status != DPCD_CHANNEL_EQ_BITS) >> return -EINVAL; >> @@ -432,22 +432,47 @@ static void exynos_dp_reduce_link_rate(struct exynos_dp_device *dp) >> dp->link_train.lt_state = FAILED; >> } >> >> +static void exynos_dp_get_adjust_training_lane(struct exynos_dp_device *dp, >> + u8 adjust_request[2]) >> +{ >> + int lane, lane_count; >> + u8 voltage_swing, pre_emphasis, training_lane; >> + >> + lane_count = dp->link_train.lane_count; >> + for (lane = 0; lane < lane_count; lane++) { >> + voltage_swing = exynos_dp_get_adjust_request_voltage( >> + adjust_request, lane); >> + pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis( >> + adjust_request, lane); >> + training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) | >> + DPCD_PRE_EMPHASIS_SET(pre_emphasis); >> + >> + if (voltage_swing == VOLTAGE_LEVEL_3) >> + training_lane |= DPCD_MAX_SWING_REACHED; >> + if (pre_emphasis == PRE_EMPHASIS_LEVEL_3) >> + training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED; >> + >> + dp->link_train.training_lane[lane] = training_lane; >> + } >> +} >> + >> static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp) >> { >> - u8 link_status[2]; >> int lane, lane_count, retval; >> - >> - u8 adjust_request[2]; >> - u8 voltage_swing; >> - u8 pre_emphasis; >> - u8 training_lane; >> + u8 voltage_swing, pre_emphasis, training_lane; >> + u8 link_status[2], adjust_request[2]; >> >> usleep_range(100, 101); >> >> lane_count = dp->link_train.lane_count; >> >> retval = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS, >> - 2, link_status); >> + 2, link_status); >> + if (retval) >> + return retval; >> + >> + retval = exynos_dp_read_bytes_from_dpcd(dp, >> + DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request); >> if (retval) >> return retval; >> >> @@ -455,43 +480,9 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp) >> /* set training pattern 2 for EQ */ >> exynos_dp_set_training_pattern(dp, TRAINING_PTN2); >> >> - for (lane = 0; lane < lane_count; lane++) { >> - retval = exynos_dp_read_bytes_from_dpcd(dp, >> - DPCD_ADDR_ADJUST_REQUEST_LANE0_1, >> - 2, adjust_request); >> - if (retval) >> - return retval; >> - >> - voltage_swing = exynos_dp_get_adjust_request_voltage( >> - adjust_request, lane); >> - pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis( >> - adjust_request, lane); >> - training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) | >> - DPCD_PRE_EMPHASIS_SET(pre_emphasis); >> - >> - if (voltage_swing == VOLTAGE_LEVEL_3) >> - training_lane |= DPCD_MAX_SWING_REACHED; >> - if (pre_emphasis == PRE_EMPHASIS_LEVEL_3) >> - training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED; >> - >> - dp->link_train.training_lane[lane] = training_lane; >> - >> - exynos_dp_set_lane_link_training(dp, >> - dp->link_train.training_lane[lane], >> - lane); >> - } >> - > > Please don't move it to back. > I assume you're talking about the adjust_request read here? I noticed this was changed in your original clean-up patch (http://www.spinics.net/lists/linux-fbdev/msg06849.html), but assumed it was a no-op. What bug does it fix? According to the flowcharts in the exynos5250 datasheet (figure 49-10 & 49-11), this should be done *before* setting training pattern 2. Your alteration to my patch will read it after. I also noticed that you added back exynos_dp_get_adjust_training_lane call here, along with setting DPCD_ADDR_TRAINING_LANE0_SET. You'll notice that this same code is run in the else path of this function. Hence, I removed the duplication and put it all at the bottom. This improves readability, matches the flowchart more closely, and removes duplication. I'd urge you to please read my patch more carefully and ask questions if you have any. Thanks! Sean >> retval = exynos_dp_write_byte_to_dpcd(dp, >> DPCD_ADDR_TRAINING_PATTERN_SET, >> - DPCD_SCRAMBLING_DISABLED | >> - DPCD_TRAINING_PATTERN_2); >> - if (retval) >> - return retval; >> - >> - retval = exynos_dp_write_bytes_to_dpcd(dp, >> - DPCD_ADDR_TRAINING_LANE0_SET, >> - lane_count, >> - dp->link_train.training_lane); >> + DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_2); >> if (retval) >> return retval; >> >> @@ -501,73 +492,49 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp) >> for (lane = 0; lane < lane_count; lane++) { >> training_lane = exynos_dp_get_lane_link_training( >> dp, lane); >> - retval = exynos_dp_read_bytes_from_dpcd(dp, >> - DPCD_ADDR_ADJUST_REQUEST_LANE0_1, >> - 2, adjust_request); >> - if (retval) >> - return retval; >> - >> voltage_swing = exynos_dp_get_adjust_request_voltage( >> adjust_request, lane); >> pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis( >> adjust_request, lane); >> >> - if (voltage_swing == VOLTAGE_LEVEL_3 || >> - pre_emphasis == PRE_EMPHASIS_LEVEL_3) { >> - dev_err(dp->dev, "voltage or pre emphasis reached max level\n"); >> - goto reduce_link_rate; >> - } >> - >> - if ((DPCD_VOLTAGE_SWING_GET(training_lane) == >> - voltage_swing) && >> - (DPCD_PRE_EMPHASIS_GET(training_lane) == >> - pre_emphasis)) { >> + if (DPCD_VOLTAGE_SWING_GET(training_lane) == >> + voltage_swing && >> + DPCD_PRE_EMPHASIS_GET(training_lane) == >> + pre_emphasis) >> dp->link_train.cr_loop[lane]++; >> - if (dp->link_train.cr_loop[lane] == MAX_CR_LOOP) { >> - dev_err(dp->dev, "CR Max loop\n"); >> - goto reduce_link_rate; >> - } >> - } >> >> - training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) | >> - DPCD_PRE_EMPHASIS_SET(pre_emphasis); >> - >> - if (voltage_swing == VOLTAGE_LEVEL_3) >> - training_lane |= DPCD_MAX_SWING_REACHED; >> - if (pre_emphasis == PRE_EMPHASIS_LEVEL_3) >> - training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED; >> - >> - dp->link_train.training_lane[lane] = training_lane; >> - >> - exynos_dp_set_lane_link_training(dp, >> - dp->link_train.training_lane[lane], lane); >> + if (dp->link_train.cr_loop[lane] == MAX_CR_LOOP || >> + voltage_swing == VOLTAGE_LEVEL_3 || >> + pre_emphasis == PRE_EMPHASIS_LEVEL_3) { >> + dev_err(dp->dev, "CR Max reached (%d,%d,%d)\n", >> + dp->link_train.cr_loop[lane], >> + voltage_swing, pre_emphasis); >> + exynos_dp_reduce_link_rate(dp); >> + return -EIO; >> + } >> } >> + } >> + >> + exynos_dp_get_adjust_training_lane(dp, adjust_request); >> >> - retval = exynos_dp_write_bytes_to_dpcd(dp, >> - DPCD_ADDR_TRAINING_LANE0_SET, lane_count, >> - dp->link_train.training_lane); >> + for (lane = 0; lane < lane_count; lane++) { >> + exynos_dp_set_lane_link_training(dp, >> + dp->link_train.training_lane[lane], lane); >> + retval = exynos_dp_write_byte_to_dpcd(dp, >> + DPCD_ADDR_TRAINING_LANE0_SET + lane, >> + dp->link_train.training_lane[lane]); > > The following would be better. > byte's'_to_dpcd is faster than byte_to_dpcd x 4 times. > > for (lane = 0; lane < lane_count; lane++) { > exynos_dp_set_lane_link_training(dp, > dp->link_train.training_lane[lane], lane); > } > > retval = exynos_dp_write_bytes_to_dpcd(dp, > DPCD_ADDR_TRAINING_LANE0_SET, lane_count, > dp->link_train.training_lane); > Makes sense, that's a good change. >> if (retval) >> return retval; >> } >> >> return retval; >> - >> -reduce_link_rate: >> - exynos_dp_reduce_link_rate(dp); >> - return -EIO; >> } >> >> static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp) >> { >> - u8 link_status[2]; >> - u8 link_align[3]; >> int lane, lane_count, retval; >> u32 reg; >> - >> - u8 adjust_request[2]; >> - u8 voltage_swing; >> - u8 pre_emphasis; >> - u8 training_lane; >> + u8 link_align, link_status[2], adjust_request[2]; >> >> usleep_range(400, 401); >> >> @@ -578,85 +545,63 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp) >> if (retval) >> return retval; >> >> - if (exynos_dp_clock_recovery_ok(link_status, lane_count) == 0) { >> - link_align[0] = link_status[0]; >> - link_align[1] = link_status[1]; >> - >> - exynos_dp_read_byte_from_dpcd(dp, >> - DPCD_ADDR_LANE_ALIGN_STATUS_UPDATED, >> - &link_align[2]); >> - >> - for (lane = 0; lane < lane_count; lane++) { >> - retval = exynos_dp_read_bytes_from_dpcd(dp, >> - DPCD_ADDR_ADJUST_REQUEST_LANE0_1, >> - 2, adjust_request); >> - if (retval) >> - return retval; >> + if (exynos_dp_clock_recovery_ok(link_status, lane_count)) { >> + exynos_dp_reduce_link_rate(dp); >> + return -EIO; >> + } >> >> - voltage_swing = exynos_dp_get_adjust_request_voltage( >> - adjust_request, lane); >> - pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis( >> - adjust_request, lane); >> - training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) | >> - DPCD_PRE_EMPHASIS_SET(pre_emphasis); >> + retval = exynos_dp_read_bytes_from_dpcd(dp, >> + DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request); >> + if (retval) >> + return retval; >> >> - if (voltage_swing == VOLTAGE_LEVEL_3) >> - training_lane |= DPCD_MAX_SWING_REACHED; >> - if (pre_emphasis == PRE_EMPHASIS_LEVEL_3) >> - training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED; >> + retval = exynos_dp_read_byte_from_dpcd(dp, >> + DPCD_ADDR_LANE_ALIGN_STATUS_UPDATED, &link_align); >> + if (retval) >> + return retval; >> >> - dp->link_train.training_lane[lane] = training_lane; >> - } >> + exynos_dp_get_adjust_training_lane(dp, adjust_request); >> >> - if (exynos_dp_channel_eq_ok(link_align, lane_count) == 0) { >> - /* traing pattern Set to Normal */ >> - exynos_dp_training_pattern_dis(dp); >> + if (!exynos_dp_channel_eq_ok(link_status, link_align, lane_count)) { >> + /* traing pattern Set to Normal */ >> + exynos_dp_training_pattern_dis(dp); >> >> - dev_info(dp->dev, "Link Training success!\n"); >> + dev_info(dp->dev, "Link Training success!\n"); >> >> - exynos_dp_get_link_bandwidth(dp, ®); >> - dp->link_train.link_rate = reg; >> - dev_dbg(dp->dev, "final bandwidth = %.2x\n", >> - dp->link_train.link_rate); >> + exynos_dp_get_link_bandwidth(dp, ®); >> + dp->link_train.link_rate = reg; >> + dev_dbg(dp->dev, "final bandwidth = %.2x\n", >> + dp->link_train.link_rate); >> >> - exynos_dp_get_lane_count(dp, ®); >> - dp->link_train.lane_count = reg; >> - dev_dbg(dp->dev, "final lane count = %.2x\n", >> - dp->link_train.lane_count); >> + exynos_dp_get_lane_count(dp, ®); >> + dp->link_train.lane_count = reg; >> + dev_dbg(dp->dev, "final lane count = %.2x\n", >> + dp->link_train.lane_count); >> >> - /* set enhanced mode if available */ >> - exynos_dp_set_enhanced_mode(dp); >> - dp->link_train.lt_state = FINISHED; >> - } else { >> - /* not all locked */ >> - dp->link_train.eq_loop++; >> + /* set enhanced mode if available */ >> + exynos_dp_set_enhanced_mode(dp); >> + dp->link_train.lt_state = FINISHED; >> >> - if (dp->link_train.eq_loop > MAX_EQ_LOOP) { >> - dev_err(dp->dev, "EQ Max loop\n"); >> - goto reduce_link_rate; >> - } >> + return 0; >> + } >> >> - for (lane = 0; lane < lane_count; lane++) >> - exynos_dp_set_lane_link_training(dp, >> - dp->link_train.training_lane[lane], >> - lane); >> + /* not all locked */ >> + dp->link_train.eq_loop++; >> >> - retval = exynos_dp_write_bytes_to_dpcd(dp, >> - DPCD_ADDR_TRAINING_LANE0_SET, >> - lane_count, >> - dp->link_train.training_lane); >> - if (retval) >> - return retval; >> - } >> - } else { >> - goto reduce_link_rate; >> + if (dp->link_train.eq_loop > MAX_EQ_LOOP) { >> + dev_err(dp->dev, "EQ Max loop\n"); >> + exynos_dp_reduce_link_rate(dp); >> + return -EIO; >> } >> >> - return 0; >> + for (lane = 0; lane < lane_count; lane++) >> + exynos_dp_set_lane_link_training(dp, >> + dp->link_train.training_lane[lane], lane); >> >> -reduce_link_rate: >> - exynos_dp_reduce_link_rate(dp); >> - return -EIO; >> + retval = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_TRAINING_LANE0_SET, >> + lane_count, dp->link_train.training_lane); >> + >> + return retval; >> } >> >> static void exynos_dp_get_max_rx_bandwidth(struct exynos_dp_device *dp, >> -- >> 1.7.7.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
