Re: [PATCH 5/9] drm/bridge: tc358767: Simplify polling in tc_link_training()

[Andrey Smirnov]

On Mon, Mar 4, 2019 at 4:30 AM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Andrey,
>
> Thank you for the patch.
>
> On Tue, Feb 26, 2019 at 11:36:05AM -0800, Andrey Smirnov wrote:
> > Replace explicit polling in tc_link_training() with equivalent call to
> > regmap_read_poll_timeout() for simplicity. No functional change
> > intended (not including slightly altered debug output).
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> > Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
> > Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Cc: Chris Healy <cphealy@xxxxxxxxx>
> > Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 6455e6484722..ea30cec4a0c3 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -735,7 +735,6 @@ static int tc_link_training(struct tc_data *tc, int pattern)
> >       const char * const *errors;
> >       u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
> >                     DP0_SRCCTRL_AUTOCORRECT;
> > -     int timeout;
> >       int retry;
> >       u32 value;
> >       int ret;
> > @@ -765,20 +764,17 @@ static int tc_link_training(struct tc_data *tc, int pattern)
> >               tc_write(DP0CTL, DP_EN);
> >
> >               /* wait */
> > -             timeout = 1000;
> > -             do {
> > -                     tc_read(DP0_LTSTAT, &value);
> > -                     udelay(1);
> > -             } while ((!(value & LT_LOOPDONE)) && (--timeout));
> > -             if (timeout == 0) {
> > +             ret = regmap_read_poll_timeout(tc->regmap, DP0_LTSTAT, value,
> > +                                            value & LT_LOOPDONE, 1, 1000);
> > +             if (ret) {
> >                       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]);
> > +                             "Link training phase %d done: %s\n",
> > +                             pattern, errors[error]);
>
> It's probably not a big deal in this specific case, but in general it
> can be useful to know how long the poll took.

I don't disagree, but bear in mind that the way time is measured in
original loop assumes that tc_read, an I2C transaction over 100KHz
bus, takes insignificant amount of time compared to 1 uS delay. I
think original debug statement does a bit of a false advertising when
it presents a number of polling loop iterations as if it is time it
took to establish a link in microseconds.

> Any hope to enhance regmap_read_poll_timeout() to return either the elapsed time or the
> remaining timeout instead of 0 on success ?
>

I'd rather not go there. That'll take convincing Mark Brown to accept
that semantics change, then fixing all of the callers across the tree
via a separate patch series.

What if instead we just add an extra debug statement before link
training starts, so that duration of the process can be discerned from
logging timestamps? This does require user doing a bit of math by
hand, but it's actually more accurate timing info compared to original
and it doesn't require any API modification.

Thanks,
Andrey Smirnov