diff mbox series

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

Message ID 20190226193609.9862-6-andrew.smirnov@gmail.com (mailing list archive)
State New, archived
Headers show
Series tc358767 driver improvements | expand

Commit Message

Andrey Smirnov Feb. 26, 2019, 7:36 p.m. UTC 
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@gmail.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Comments

Laurent Pinchart March 4, 2019, 12:30 p.m. UTC | #1 
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@gmail.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  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. Any hope to enhance
regmap_read_poll_timeout() to return either the elapsed time or the
remaining timeout instead of 0 on success ?

>  			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
>  				break;
>  			if (pattern == DP_TRAINING_PATTERN_2) {
Andrey Smirnov March 11, 2019, 6:26 p.m. UTC | #2 
On Mon, Mar 4, 2019 at 4:30 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@gmail.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Chris Healy <cphealy@gmail.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  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
Laurent Pinchart March 12, 2019, 3:15 p.m. UTC | #3 
Hi Andrey,

On Mon, Mar 11, 2019 at 11:26:20AM -0700, Andrey Smirnov wrote:
> On Mon, Mar 4, 2019 at 4:30 AM Laurent Pinchart wrote:
> > 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@gmail.com>
> >> Cc: Archit Taneja <architt@codeaurora.org>
> >> Cc: Andrzej Hajda <a.hajda@samsung.com>
> >> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> >> Cc: Chris Healy <cphealy@gmail.com>
> >> Cc: Lucas Stach <l.stach@pengutronix.de>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Cc: linux-kernel@vger.kernel.org
> >> ---
> >>  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.

That works for me.
diff mbox series

Patch

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]);
 			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
 				break;
 			if (pattern == DP_TRAINING_PATTERN_2) {