Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20201030232310.11100-1-khsieh@codeaurora.org>
References: <20201030232310.11100-1-khsieh@codeaurora.org>
Subject: Re: [PATCH v2] drm/msm/dp: skip checking LINK_STATUS_UPDATED bit
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     tanmay@codeaurora.org, abhinavk@codeaurora.org,
        aravindh@codeaurora.org, khsieh@codeaurora.org,
        rnayak@codeaurora.org, airlied@linux.ie, daniel@ffwll.ch,
        linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
        freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org
To:     Kuogee Hsieh <khsieh@codeaurora.org>, robdclark@gmail.com,
        sean@poorly.run
Date:   Mon, 02 Nov 2020 12:53:04 -0800
Message-ID: <160435038402.884498.10403326257118588679@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9.1
Precedence: bulk

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.=20

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.
>=20
> Chanegs in V2:
> -- revise commit text
>=20
> Fixes: fd4a29bed29b (drm/msm/dp: DisplayPort PHY compliance tests fixup)
>=20

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

> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
>  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(-)
>=20
> 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_c=
trl_private *ctrl,
>  static int dp_ctrl_read_link_status(struct dp_ctrl_private *ctrl,
>                                     u8 *link_status)
>  {
> -       int len =3D 0;
> -       u32 const offset =3D DP_LANE_ALIGN_STATUS_UPDATED - DP_LANE0_1_ST=
ATUS;
> -       u32 link_status_read_max_retries =3D 100;
> -
> -       while (--link_status_read_max_retries) {
> -               len =3D drm_dp_dpcd_read_link_status(ctrl->aux,
> -                       link_status);
> -               if (len !=3D DP_LINK_STATUS_SIZE) {
> -                       DRM_ERROR("DP link status read failed, err: %d\n"=
, len);
> -                       return len;
> -               }
> +       int ret =3D 0, len;
> =20
> -               if (!(link_status[offset] & DP_LINK_STATUS_UPDATED))
> -                       return 0;
> +       len =3D drm_dp_dpcd_read_link_status(ctrl->aux, link_status);
> +       if (len !=3D DP_LINK_STATUS_SIZE) {
> +               DRM_ERROR("DP link status read failed, err: %d\n", len);
> +               ret =3D len;

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

>         }
> =20
> -       return -ETIMEDOUT;
> +       return ret;
>  }
> =20
>  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_L=
INK_STATUS_SIZE], int r)
>   */
>  static int dp_link_process_link_status_update(struct dp_link_private *li=
nk)
>  {
> -       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_lan=
es) &&
> -                       drm_dp_channel_eq_ok(link->link_status,
> -                                       link->dp_link.link_params.num_lan=
es)))
> -               return -EINVAL;
> +       bool channel_eq_done =3D drm_dp_channel_eq_ok(link->link_status,
> +                       link->dp_link.link_params.num_lanes);
> +
> +       bool clock_recovery_done =3D drm_dp_clock_recovery_ok(link->link_=
status,
> +                       link->dp_link.link_params.num_lanes);
> =20
>         DRM_DEBUG_DP("channel_eq_done =3D %d, clock_recovery_done =3D %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;
> =20
>         return 0;
>  }
>=20
> base-commit: 03a9adc88c206b3857ce95f4f4d3b185d429fa31

What is this commit?