diff mbox series

[v6,03/15] drm/bridge: tc358767: Simplify polling in tc_link_training()

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

Commit Message

Andrey Smirnov June 19, 2019, 5:27 a.m. UTC 
Replace explicit polling in tc_link_training() with equivalent call to
tc_poll_timeout() for simplicity. No functional change intended (not
including slightly altered debug output).

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Cory Tusar <cory.tusar@zii.aero>
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 | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Tomi Valkeinen Dec. 4, 2019, 6:27 p.m. UTC | #1 
Hi Andrey,

On 19/06/2019 08:27, Andrey Smirnov wrote:

> @@ -748,22 +748,19 @@ static int tc_set_video_mode(struct tc_data *tc,
>   
>   static int tc_wait_link_training(struct tc_data *tc)
>   {
> -	u32 timeout = 1000;
>   	u32 value;
>   	int ret;
>   
> -	do {
> -		udelay(1);
> -		tc_read(DP0_LTSTAT, &value);
> -	} while ((!(value & LT_LOOPDONE)) && (--timeout));
> -
> -	if (timeout == 0) {
> +	ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
> +			      LT_LOOPDONE, 1, 1000);

This seems to break DP at least with some monitors for me. I think it's just a timeout problem, as 
increasing the values helps.

Using ktime, I can see that during link training, the first call takes ~2ms, the second ~7ms. I 
think this worked before, as udelay(1) takes much longer than 1 us.

We have 1000us limit in a few other places too, which I don't see causing issues, but might need 
increasing too.

Also, 1us sleep_us may be a bit too small to be sane. If the loops take milliseconds, probably 100us 
or even more would make sense.

This didn't cause any issues with your display?

  Tomi
Tomi Valkeinen Dec. 4, 2019, 7:13 p.m. UTC | #2 
On 04/12/2019 20:27, Tomi Valkeinen wrote:
> Hi Andrey,
> 
> On 19/06/2019 08:27, Andrey Smirnov wrote:
> 
>> @@ -748,22 +748,19 @@ static int tc_set_video_mode(struct tc_data *tc,
>>   static int tc_wait_link_training(struct tc_data *tc)
>>   {
>> -    u32 timeout = 1000;
>>       u32 value;
>>       int ret;
>> -    do {
>> -        udelay(1);
>> -        tc_read(DP0_LTSTAT, &value);
>> -    } while ((!(value & LT_LOOPDONE)) && (--timeout));
>> -
>> -    if (timeout == 0) {
>> +    ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
>> +                  LT_LOOPDONE, 1, 1000);
> 
> This seems to break DP at least with some monitors for me. I think it's just a timeout problem, as 
> increasing the values helps.
> 
> Using ktime, I can see that during link training, the first call takes ~2ms, the second ~7ms. I 
> think this worked before, as udelay(1) takes much longer than 1 us.

Also the timeout in tc_aux_link_setup takes ~500us for me, and max is 1000us. So it works, but I 
think it's a bit tight.

  Tomi
Andrey Smirnov Dec. 5, 2019, 4:06 p.m. UTC | #3 
On Wed, Dec 4, 2019 at 10:27 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Hi Andrey,
>
> On 19/06/2019 08:27, Andrey Smirnov wrote:
>
> > @@ -748,22 +748,19 @@ static int tc_set_video_mode(struct tc_data *tc,
> >
> >   static int tc_wait_link_training(struct tc_data *tc)
> >   {
> > -     u32 timeout = 1000;
> >       u32 value;
> >       int ret;
> >
> > -     do {
> > -             udelay(1);
> > -             tc_read(DP0_LTSTAT, &value);
> > -     } while ((!(value & LT_LOOPDONE)) && (--timeout));
> > -
> > -     if (timeout == 0) {
> > +     ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
> > +                           LT_LOOPDONE, 1, 1000);
>
> This seems to break DP at least with some monitors for me. I think it's just a timeout problem, as
> increasing the values helps.
>
> Using ktime, I can see that during link training, the first call takes ~2ms, the second ~7ms. I
> think this worked before, as udelay(1) takes much longer than 1 us.
>
> We have 1000us limit in a few other places too, which I don't see causing issues, but might need
> increasing too.
>
> Also, 1us sleep_us may be a bit too small to be sane. If the loops take milliseconds, probably 100us
> or even more would make sense.
>
> This didn't cause any issues with your display?
>

Hmm, not that I know of. Your reasoning makes sense, though. If
increasing the timeout helps, I am all for it. And, yeah, I agree,
this is probably not the only place that could use an increased
timeout.

Thanks,
Andrey Smirnov
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index f463ef6d4271..31f5045e7e42 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -748,22 +748,19 @@  static int tc_set_video_mode(struct tc_data *tc,
 
 static int tc_wait_link_training(struct tc_data *tc)
 {
-	u32 timeout = 1000;
 	u32 value;
 	int ret;
 
-	do {
-		udelay(1);
-		tc_read(DP0_LTSTAT, &value);
-	} while ((!(value & LT_LOOPDONE)) && (--timeout));
-
-	if (timeout == 0) {
+	ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
+			      LT_LOOPDONE, 1, 1000);
+	if (ret) {
 		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
-		return -ETIMEDOUT;
+		return ret;
 	}
 
-	return (value >> 8) & 0x7;
+	tc_read(DP0_LTSTAT, &value);
 
+	return (value >> 8) & 0x7;
 err:
 	return ret;
 }