[PATCHv2,02/22] drm/bridge: tc358767: reset voltage-swing & pre-emphasis

Message ID 20190326103146.24795-3-tomi.valkeinen@ti.com
State Superseded
Headers show
Series
  • drm/bridge: tc358767: DP support
Related show

Commit Message

Tomi Valkeinen March 26, 2019, 10:31 a.m.
We need to reset DPCD voltage-swing & pre-emphasis before starting the
link training, as otherwise tc358767 will use the previous values as
minimums.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Andrzej Hajda April 15, 2019, 7:20 a.m. | #1
On 26.03.2019 11:31, Tomi Valkeinen wrote:
> We need to reset DPCD voltage-swing & pre-emphasis before starting the
> link training, as otherwise tc358767 will use the previous values as
> minimums.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 7031c4f52c57..11a50f7bb4be 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -956,6 +956,12 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	if (ret < 0)
>  		goto err_dpcd_write;
>  
> +	// Reset voltage-swing & pre-emphasis
> +	tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0;
> +	ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);
> +	if (ret < 0)
> +		goto err_dpcd_write;
> +
>  	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
>  	if (ret)
>  		goto err;
Laurent Pinchart April 20, 2019, 8:30 p.m. | #2
Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:26PM +0200, Tomi Valkeinen wrote:
> We need to reset DPCD voltage-swing & pre-emphasis before starting the
> link training, as otherwise tc358767 will use the previous values as
> minimums.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 7031c4f52c57..11a50f7bb4be 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -956,6 +956,12 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	if (ret < 0)
>  		goto err_dpcd_write;
>  
> +	// Reset voltage-swing & pre-emphasis

The driver uses C-style comments, I think it would be best to stick to
them to avoid a style mismatch.

> +	tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0;

You may want to wrap the line.

> +	ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);

What branch does this series apply to ? DP_TRAINING_LANE0_SET isn't
defined in Linus' or Dave's master branches.

> +	if (ret < 0)
> +		goto err_dpcd_write;
> +

I can't comment on this as I don't have access to the device
documentation :-(

>  	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
>  	if (ret)
>  		goto err;
Tomi Valkeinen April 26, 2019, 2:14 p.m. | #3
On 20/04/2019 23:30, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 26, 2019 at 12:31:26PM +0200, Tomi Valkeinen wrote:
>> We need to reset DPCD voltage-swing & pre-emphasis before starting the
>> link training, as otherwise tc358767 will use the previous values as
>> minimums.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358767.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 7031c4f52c57..11a50f7bb4be 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -956,6 +956,12 @@ static int tc_main_link_setup(struct tc_data *tc)
>>  	if (ret < 0)
>>  		goto err_dpcd_write;
>>  
>> +	// Reset voltage-swing & pre-emphasis
> 
> The driver uses C-style comments, I think it would be best to stick to
> them to avoid a style mismatch.

Oops. Yep. I often use c++ comments when hacking/developing as they're
often easier to use. Sometimes I miss converting them to c comments...

> 
>> +	tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0;
> 
> You may want to wrap the line.

Well, I personally don't think wrapping at 80 is a good idea. Something
like 120 is more sensible and makes the code more readable.

I can wrap it if you insist =)

>> +	ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);
> 
> What branch does this series apply to ? DP_TRAINING_LANE0_SET isn't
> defined in Linus' or Dave's master branches.

It's there. At least v5.0 has it.

>> +	if (ret < 0)
>> +		goto err_dpcd_write;
>> +
> 
> I can't comment on this as I don't have access to the device
> documentation :-(

Hmm, comment on what?

>>  	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
>>  	if (ret)
>>  		goto err;
> 

 Tomi
Laurent Pinchart April 26, 2019, 11:46 p.m. | #4
Hi Tomi,

On Fri, Apr 26, 2019 at 05:14:17PM +0300, Tomi Valkeinen wrote:
> On 20/04/2019 23:30, Laurent Pinchart wrote:
> > On Tue, Mar 26, 2019 at 12:31:26PM +0200, Tomi Valkeinen wrote:
> >> We need to reset DPCD voltage-swing & pre-emphasis before starting the
> >> link training, as otherwise tc358767 will use the previous values as
> >> minimums.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/bridge/tc358767.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> >> index 7031c4f52c57..11a50f7bb4be 100644
> >> --- a/drivers/gpu/drm/bridge/tc358767.c
> >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> >> @@ -956,6 +956,12 @@ static int tc_main_link_setup(struct tc_data *tc)
> >>  	if (ret < 0)
> >>  		goto err_dpcd_write;
> >>  
> >> +	// Reset voltage-swing & pre-emphasis
> > 
> > The driver uses C-style comments, I think it would be best to stick to
> > them to avoid a style mismatch.
> 
> Oops. Yep. I often use c++ comments when hacking/developing as they're
> often easier to use. Sometimes I miss converting them to c comments...
> 
> >> +	tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0;
> > 
> > You may want to wrap the line.
> 
> Well, I personally don't think wrapping at 80 is a good idea. Something
> like 120 is more sensible and makes the code more readable.
> 
> I can wrap it if you insist =)

I don't mind going over 80 when it makes the code more readable, but
when it's easy to wrap, 80 is a nice value, and matches most of the
kernel code :-)

> >> +	ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);
> > 
> > What branch does this series apply to ? DP_TRAINING_LANE0_SET isn't
> > defined in Linus' or Dave's master branches.
> 
> It's there. At least v5.0 has it.

My bad, I thought it was a driver-specific macro.

> >> +	if (ret < 0)
> >> +		goto err_dpcd_write;
> >> +
> > 
> > I can't comment on this as I don't have access to the device
> > documentation :-(
> 
> Hmm, comment on what?

On the overall change. But now that I've realized this isn't specific to
the tc358767, your change seems fine to me.

> >>  	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
> >>  	if (ret)
> >>  		goto err;
Andrey Smirnov April 26, 2019, 11:54 p.m. | #5
On Fri, Apr 26, 2019 at 7:14 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 20/04/2019 23:30, Laurent Pinchart wrote:
> > Hi Tomi,
> >
> > Thank you for the patch.
> >
> > On Tue, Mar 26, 2019 at 12:31:26PM +0200, Tomi Valkeinen wrote:
> >> We need to reset DPCD voltage-swing & pre-emphasis before starting the
> >> link training, as otherwise tc358767 will use the previous values as
> >> minimums.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/bridge/tc358767.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> >> index 7031c4f52c57..11a50f7bb4be 100644
> >> --- a/drivers/gpu/drm/bridge/tc358767.c
> >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> >> @@ -956,6 +956,12 @@ static int tc_main_link_setup(struct tc_data *tc)
> >>      if (ret < 0)
> >>              goto err_dpcd_write;
> >>
> >> +    // Reset voltage-swing & pre-emphasis
> >
> > The driver uses C-style comments, I think it would be best to stick to
> > them to avoid a style mismatch.
>
> Oops. Yep. I often use c++ comments when hacking/developing as they're
> often easier to use. Sometimes I miss converting them to c comments...
>
> >
> >> +    tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0;
> >
> > You may want to wrap the line.
>
> Well, I personally don't think wrapping at 80 is a good idea.

Trying to read two files side by side on a 13" laptop screen might
change your mind :-) +1 on wrapping the code around 80 character from
me.

Thanks,
Andrey Smirnov

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7031c4f52c57..11a50f7bb4be 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -956,6 +956,12 @@  static int tc_main_link_setup(struct tc_data *tc)
 	if (ret < 0)
 		goto err_dpcd_write;
 
+	// Reset voltage-swing & pre-emphasis
+	tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0;
+	ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);
+	if (ret < 0)
+		goto err_dpcd_write;
+
 	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
 	if (ret)
 		goto err;