Re: [PATCH v2] drm/msm/dp: skip checking LINK_STATUS_UPDATED bit

From: Stephen Boyd
Date: Mon Nov 02 2020 - 15:54:29 EST


Quoting Kuogee Hsieh (2020-10-30 16:23:10)
> Some dongle will not clear LINK_STATUS_UPDATED bit after
> DPCD read which cause link training failed. This patch

$ git grep 'this patch' -- Documentation/process/submitting-patches.rst

> just read 6 bytes of DPCD link status from sink and return
> without checking LINK_STATUS_UPDATED bit.
> Link rate read back from sink need to be convert into
> really rate by timing 2.7Mb.

This last sentence doesn't make sense to me, sorry. What is being said?

> For example 0x0A is equivalent
> to 2.7Gb. This patch also convert link rate correctly to fix
> phy compliance test link rate error.
>
> Chanegs in V2:
> -- revise commit text
>
> Fixes: fd4a29bed29b (drm/msm/dp: DisplayPort PHY compliance tests fixup)
>

Shouldn't be any space here between SoB and Fixes tag.

> Signed-off-by: Kuogee Hsieh <khsieh@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 20 ++++++--------------
> drivers/gpu/drm/msm/dp/dp_link.c | 24 +++++++++++-------------
> 2 files changed, 17 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 904698dfc7f7..844ba756a2c6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1061,23 +1061,15 @@ static bool dp_ctrl_train_pattern_set(struct dp_ctrl_private *ctrl,
> static int dp_ctrl_read_link_status(struct dp_ctrl_private *ctrl,
> u8 *link_status)
> {
> - int len = 0;
> - u32 const offset = DP_LANE_ALIGN_STATUS_UPDATED - DP_LANE0_1_STATUS;
> - u32 link_status_read_max_retries = 100;
> -
> - while (--link_status_read_max_retries) {
> - len = drm_dp_dpcd_read_link_status(ctrl->aux,
> - link_status);
> - if (len != DP_LINK_STATUS_SIZE) {
> - DRM_ERROR("DP link status read failed, err: %d\n", len);
> - return len;
> - }
> + int ret = 0, len;
>
> - if (!(link_status[offset] & DP_LINK_STATUS_UPDATED))
> - return 0;
> + len = drm_dp_dpcd_read_link_status(ctrl->aux, link_status);
> + if (len != DP_LINK_STATUS_SIZE) {
> + DRM_ERROR("DP link status read failed, err: %d\n", len);
> + ret = len;

So if this returns the integer 2 it's OK? Shouldn't it return some error
value?

> }
>
> - return -ETIMEDOUT;
> + return ret;
> }
>
> static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
> index 49d7fad36fc4..64a002d100c7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -943,20 +944,17 @@ static u8 get_link_status(const u8 link_status[DP_LINK_STATUS_SIZE], int r)
> */
> static int dp_link_process_link_status_update(struct dp_link_private *link)
> {
> - if (!(get_link_status(link->link_status,
> - DP_LANE_ALIGN_STATUS_UPDATED) &
> - DP_LINK_STATUS_UPDATED) ||
> - (drm_dp_clock_recovery_ok(link->link_status,
> - link->dp_link.link_params.num_lanes) &&
> - drm_dp_channel_eq_ok(link->link_status,
> - link->dp_link.link_params.num_lanes)))
> - return -EINVAL;
> + bool channel_eq_done = drm_dp_channel_eq_ok(link->link_status,
> + link->dp_link.link_params.num_lanes);
> +
> + bool clock_recovery_done = drm_dp_clock_recovery_ok(link->link_status,
> + link->dp_link.link_params.num_lanes);
>
> DRM_DEBUG_DP("channel_eq_done = %d, clock_recovery_done = %d\n",
> - drm_dp_clock_recovery_ok(link->link_status,
> - link->dp_link.link_params.num_lanes),
> - drm_dp_clock_recovery_ok(link->link_status,
> - link->dp_link.link_params.num_lanes));
> + channel_eq_done, clock_recovery_done);
> +
> + if (channel_eq_done && clock_recovery_done)
> + return -EINVAL;
>
> return 0;
> }
>
> base-commit: 03a9adc88c206b3857ce95f4f4d3b185d429fa31

What is this commit?