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

[khsieh]

On 2020-10-20 15:15, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2020-10-20 09:59:59)
> > No need to check LINK_STATuS_UPDATED bit before
>
> LINK_STATUS_UPDATED?
>
> > return 6 bytes of link status during link training.
>
> Why?
>
> > This patch also fix phy compliance test link rate
> > conversion error.
>
> How?
>
> > Signed-off-by: Kuogee Hsieh <khsieh@xxxxxxxxxxxxxx>
> > ---
>
> Any Fixes: tag?
>
> >  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 6bdaec778c4c..76e891c91c6e 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;
>
> Could this be positive if the len is greater than 0 but not
> DP_LINK_STATUS_SIZE? Maybe the check should be len < 0? We certainly
> don't want to return some smaller size from this function, right?

no, it should be exactly the byte number requested to read.
otherwise, it should be failed and will re read at next run.

> >         }
> >
> > -       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 c811da515fb3..58d65daae3b3 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_link.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> > @@ -773,7 +773,8 @@ static int 
> > dp_link_process_link_training_request(struct dp_link_private *link)
> >                         link->request.test_lane_count);
> >
> >         link->dp_link.link_params.num_lanes = 
> > link->request.test_lane_count;
> > -       link->dp_link.link_params.rate = link->request.test_link_rate;
> > +       link->dp_link.link_params.rate =
> > +               
> > drm_dp_bw_code_to_link_rate(link->request.test_link_rate);
>
> Why are we storing bw_code in test_link_rate? This looks very 
> confusing.

Test_link_rate contains link rate from dpcd read. it need to be convert 
to real
rate by timing 2.7Mb before start phy compliance test.

> >         return 0;
> >  }