[PATCHv2 00/22] drm/bridge: tc358767: DP support

[PATCHv2 00/22] drm/bridge: tc358767: DP support

[Tomi Valkeinen]

Hi,

tc358767 bridge was originally implemented for eDP use with an embedded
panel. I've been working to add DP and HPD support, and this series is
the result. I did have a lot of issues with link training, but with
these, it's been working reliably with my devices.

Compared to v1, I dropped the "implement naive HPD handling". I managed
to modify my board to support interrupts, and in this series also
interrupt based HPD is supported with the last two patches. The other
patches should be unchanged to the v1.

While I think the HPD support works ok, I'm unsure about the DT binding.
I first tried implementing the HPD as a gpiochip, but for interrupts I
then needed irqchip, and the amount of code started to increase a lot,
whereas in my current patch the code is quite simple and short.

So, here I have a simple 'hpd-num' DT property to mark which (if any) of
the two GPIO pins is used for HPD.

I have tested this with interrupt and polling based HPD, and also with a
"simulated" eDP panel (DP monitor but defining a simple-panel in the DT
data).

 Tomi

Tomi Valkeinen (22):
  drm/bridge: tc358767: fix tc_aux_get_status error handling
  drm/bridge: tc358767: reset voltage-swing & pre-emphasis
  drm/bridge: tc358767: fix ansi 8b10b use
  drm/bridge: tc358767: cleanup spread & scrambler_dis
  drm/bridge: tc358767: remove unused swing & preemp
  drm/bridge: tc358767: cleanup aux_link_setup
  drm/bridge: tc358767: move video stream setup to tc_main_link_stream
  drm/bridge: tc358767: split stream enable/disable
  drm/bridge: tc358767: move PXL PLL enable/disable to stream
    enable/disable
  drm/bridge: tc358767: add link disable function
  drm/bridge: tc358767: ensure DP is disabled before LT
  drm/bridge: tc358767: remove unnecessary msleep
  drm/bridge: tc358767: use more reliable seq when finishing LT
  drm/bridge: tc358767: cleanup LT result check
  drm/bridge: tc358767: clean-up link training
  drm/bridge: tc358767: remove check for video mode in link enable
  drm/bridge: tc358767: use bridge mode_valid
  drm/bridge: tc358767: remove tc_connector_best_encoder
  drm/bridge: tc358767: copy the mode data, instead of storing the
    pointer
  drm/bridge: tc358767: add GPIO & interrupt registers
  drm/bridge: tc358767: add IRQ and HPD support
  dt-bindings: tc358767: add IRQ & HPD support

 .../display/bridge/toshiba,tc358767.txt       |   3 +
 drivers/gpu/drm/bridge/tc358767.c             | 558 +++++++++++-------
 2 files changed, 353 insertions(+), 208 deletions(-)

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 01/22] drm/bridge: tc358767: fix tc_aux_get_status error handling

[Tomi Valkeinen]

tc_aux_get_status() does not report AUX_TIMEOUT correctly, as it only
checks the AUX_TIMEOUT if aux is still busy. Fix this by always checking
for AUX_TIMEOUT.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 888980d4bc74..7031c4f52c57 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -286,14 +286,17 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
 	ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
 	if (ret < 0)
 		return ret;
+
 	if (value & AUX_BUSY) {
-		if (value & AUX_TIMEOUT) {
-			dev_err(tc->dev, "i2c access timeout!\n");
-			return -ETIMEDOUT;
-		}
+		dev_err(tc->dev, "aux busy!\n");
 		return -EBUSY;
 	}
 
+	if (value & AUX_TIMEOUT) {
+		dev_err(tc->dev, "aux access timeout!\n");
+		return -ETIMEDOUT;
+	}
+
 	*reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
 	return 0;
 }
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 01/22] drm/bridge: tc358767: fix tc_aux_get_status error handling

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> tc_aux_get_status() does not report AUX_TIMEOUT correctly, as it only
> checks the AUX_TIMEOUT if aux is still busy. Fix this by always checking
> for AUX_TIMEOUT.
>
> 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 | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 888980d4bc74..7031c4f52c57 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -286,14 +286,17 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
>  	ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
>  	if (ret < 0)
>  		return ret;
> +
>  	if (value & AUX_BUSY) {
> -		if (value & AUX_TIMEOUT) {
> -			dev_err(tc->dev, "i2c access timeout!\n");
> -			return -ETIMEDOUT;
> -		}
> +		dev_err(tc->dev, "aux busy!\n");
>  		return -EBUSY;
>  	}
>  
> +	if (value & AUX_TIMEOUT) {
> +		dev_err(tc->dev, "aux access timeout!\n");
> +		return -ETIMEDOUT;
> +	}
> +
>  	*reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
>  	return 0;
>  }


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 01/22] drm/bridge: tc358767: fix tc_aux_get_status error handling

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:25PM +0200, Tomi Valkeinen wrote:
> tc_aux_get_status() does not report AUX_TIMEOUT correctly, as it only
> checks the AUX_TIMEOUT if aux is still busy. Fix this by always checking
> for AUX_TIMEOUT.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 888980d4bc74..7031c4f52c57 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -286,14 +286,17 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
>  	ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
>  	if (ret < 0)
>  		return ret;
> +
>  	if (value & AUX_BUSY) {
> -		if (value & AUX_TIMEOUT) {
> -			dev_err(tc->dev, "i2c access timeout!\n");
> -			return -ETIMEDOUT;
> -		}
> +		dev_err(tc->dev, "aux busy!\n");
>  		return -EBUSY;
>  	}
>  
> +	if (value & AUX_TIMEOUT) {
> +		dev_err(tc->dev, "aux access timeout!\n");
> +		return -ETIMEDOUT;
> +	}
> +

This changes the priority between errors. Should AUX_TIMEOUT be checked
first ?

>  	*reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 01/22] drm/bridge: tc358767: fix tc_aux_get_status error handling

[Tomi Valkeinen]

On 20/04/2019 23:14, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 26, 2019 at 12:31:25PM +0200, Tomi Valkeinen wrote:
>> tc_aux_get_status() does not report AUX_TIMEOUT correctly, as it only
>> checks the AUX_TIMEOUT if aux is still busy. Fix this by always checking
>> for AUX_TIMEOUT.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358767.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 888980d4bc74..7031c4f52c57 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -286,14 +286,17 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
>>  	ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
>>  	if (ret < 0)
>>  		return ret;
>> +
>>  	if (value & AUX_BUSY) {
>> -		if (value & AUX_TIMEOUT) {
>> -			dev_err(tc->dev, "i2c access timeout!\n");
>> -			return -ETIMEDOUT;
>> -		}
>> +		dev_err(tc->dev, "aux busy!\n");
>>  		return -EBUSY;
>>  	}
>>  
>> +	if (value & AUX_TIMEOUT) {
>> +		dev_err(tc->dev, "aux access timeout!\n");
>> +		return -ETIMEDOUT;
>> +	}
>> +
> 
> This changes the priority between errors. Should AUX_TIMEOUT be checked
> first ?

BUSY is not an error, it indicates that the HW is not finished. I don't
think it makes sense to look at the error bits before the HW is done.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tomi Valkeinen]

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
+	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;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrzej Hajda]

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;


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tomi Valkeinen]

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrey Smirnov]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Tomi Valkeinen]

DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
the ANSI 8B10B bit set in DPCD, even if it should always be set.

The tc358767 driver currently respects that flag, and turns the encoding
off if the monitor does not have the bit set, which then results in the
monitor not working.

This patch makes the driver to always use ANSI 8B10B encoding, and drops
the 'coding8b10b' field which is no longer used.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 11a50f7bb4be..163c594fa6ac 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -188,7 +188,6 @@ struct tc_edp_link {
 	u8			assr;
 	int			scrambler_dis;
 	int			spread;
-	int			coding8b10b;
 	u8			swing;
 	u8			preemp;
 };
@@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
 	 * No training pattern, skew lane 1 data by two LSCLK cycles with
 	 * respect to lane 0 data, AutoCorrect Mode = 0
 	 */
-	u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
+	u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
 
 	if (tc->link.scrambler_dis)
 		reg |= DP0_SRCCTRL_SCRMBLDIS;	/* Scrambler Disabled */
-	if (tc->link.coding8b10b)
-		/* Enable 8/10B Encoder (TxData[19:16] not used) */
-		reg |= DP0_SRCCTRL_EN810B;
 	if (tc->link.spread)
 		reg |= DP0_SRCCTRL_SSCG;	/* Spread Spectrum Enable */
 	if (tc->link.base.num_lanes == 2)
@@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
 	if (ret < 0)
 		goto err_dpcd_read;
-	tc->link.coding8b10b = tmp[0] & BIT(0);
+
 	tc->link.scrambler_dis = 0;
 	/* read assr */
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
@@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
 		tc->link.base.num_lanes,
 		(tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
 		"enhanced" : "non-enhanced");
-	dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
 	dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
 		tc->link.assr, tc->assr);
 
@@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
 	/* DOWNSPREAD_CTRL */
 	tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
 	/* MAIN_LINK_CHANNEL_CODING_SET */
-	tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
+	tmp[1] =  DP_SET_ANSI_8B10B;
 	ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
 	if (ret < 0)
 		goto err_dpcd_write;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> the ANSI 8B10B bit set in DPCD, even if it should always be set.
>
> The tc358767 driver currently respects that flag, and turns the encoding
> off if the monitor does not have the bit set, which then results in the
> monitor not working.
>
> This patch makes the driver to always use ANSI 8B10B encoding, and drops
> the 'coding8b10b' field which is no longer used.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:27PM +0200, Tomi Valkeinen wrote:
> DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> the ANSI 8B10B bit set in DPCD, even if it should always be set.

Makes you wonder why the bit is present :-) I've checked the DP v1.0
specification, and even though the bit isn't documented as being always
1, 8B/10B encoding is mandatory, so this should be safe from a DP point
of view.

Without access to the TC358767 datasheet I can't tell what use cases
were intended for disabling 8B/10B encoding. Could it be related to
video sources that already provide X3.230-1994 encoded data ? In any
case this shouldn't be driven by the sink but by the source, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Andrey, as this feature was present in the initial driver version that
you authored, do you have more information about its intended use cases
?

> The tc358767 driver currently respects that flag, and turns the encoding
> off if the monitor does not have the bit set, which then results in the
> monitor not working.
> 
> This patch makes the driver to always use ANSI 8B10B encoding, and drops
> the 'coding8b10b' field which is no longer used.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 11a50f7bb4be..163c594fa6ac 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -188,7 +188,6 @@ struct tc_edp_link {
>  	u8			assr;
>  	int			scrambler_dis;
>  	int			spread;
> -	int			coding8b10b;
>  	u8			swing;
>  	u8			preemp;
>  };
> @@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
>  	 * No training pattern, skew lane 1 data by two LSCLK cycles with
>  	 * respect to lane 0 data, AutoCorrect Mode = 0
>  	 */
> -	u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
> +	u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
>  
>  	if (tc->link.scrambler_dis)
>  		reg |= DP0_SRCCTRL_SCRMBLDIS;	/* Scrambler Disabled */
> -	if (tc->link.coding8b10b)
> -		/* Enable 8/10B Encoder (TxData[19:16] not used) */
> -		reg |= DP0_SRCCTRL_EN810B;
>  	if (tc->link.spread)
>  		reg |= DP0_SRCCTRL_SSCG;	/* Spread Spectrum Enable */
>  	if (tc->link.base.num_lanes == 2)
> @@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
>  	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
>  	if (ret < 0)
>  		goto err_dpcd_read;
> -	tc->link.coding8b10b = tmp[0] & BIT(0);
> +
>  	tc->link.scrambler_dis = 0;
>  	/* read assr */
>  	ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
> @@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
>  		tc->link.base.num_lanes,
>  		(tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
>  		"enhanced" : "non-enhanced");
> -	dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
>  	dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
>  		tc->link.assr, tc->assr);
>  
> @@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	/* DOWNSPREAD_CTRL */
>  	tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
>  	/* MAIN_LINK_CHANNEL_CODING_SET */
> -	tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
> +	tmp[1] =  DP_SET_ANSI_8B10B;
>  	ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
>  	if (ret < 0)
>  		goto err_dpcd_write;

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Andrey Gusakov]

Hi Laurent,

On Sun, Apr 21, 2019 at 12:14 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomi,
>
> Thank you for the patch.
>
> On Tue, Mar 26, 2019 at 12:31:27PM +0200, Tomi Valkeinen wrote:
> > DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> > the ANSI 8B10B bit set in DPCD, even if it should always be set.
>
> Makes you wonder why the bit is present :-) I've checked the DP v1.0
> specification, and even though the bit isn't documented as being always
> 1, 8B/10B encoding is mandatory, so this should be safe from a DP point
> of view.
>
> Without access to the TC358767 datasheet I can't tell what use cases
> were intended for disabling 8B/10B encoding. Could it be related to
> video sources that already provide X3.230-1994 encoded data ? In any
> case this shouldn't be driven by the sink but by the source, so
Datasheet only describes this bit in register, without any additional
information how it should be handled.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Andrey, as this feature was present in the initial driver version that
> you authored, do you have more information about its intended use cases
> ?
During initial driver development I had one eDP display that reports 0 in Bit 0
(ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
(MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
So I had to disable 8B10 encoding on tc358767 side to make this display
work.

On other hand if there are displays that report zero bit 0 in
MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
reasonable.

May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
register after setting it and check if 8b10b actually enabled?

Andrey.

>
> > The tc358767 driver currently respects that flag, and turns the encoding
> > off if the monitor does not have the bit set, which then results in the
> > monitor not working.
> >
> > This patch makes the driver to always use ANSI 8B10B encoding, and drops
> > the 'coding8b10b' field which is no longer used.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 11a50f7bb4be..163c594fa6ac 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -188,7 +188,6 @@ struct tc_edp_link {
> >       u8                      assr;
> >       int                     scrambler_dis;
> >       int                     spread;
> > -     int                     coding8b10b;
> >       u8                      swing;
> >       u8                      preemp;
> >  };
> > @@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
> >        * No training pattern, skew lane 1 data by two LSCLK cycles with
> >        * respect to lane 0 data, AutoCorrect Mode = 0
> >        */
> > -     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
> > +     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
> >
> >       if (tc->link.scrambler_dis)
> >               reg |= DP0_SRCCTRL_SCRMBLDIS;   /* Scrambler Disabled */
> > -     if (tc->link.coding8b10b)
> > -             /* Enable 8/10B Encoder (TxData[19:16] not used) */
> > -             reg |= DP0_SRCCTRL_EN810B;
> >       if (tc->link.spread)
> >               reg |= DP0_SRCCTRL_SSCG;        /* Spread Spectrum Enable */
> >       if (tc->link.base.num_lanes == 2)
> > @@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
> >       ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
> >       if (ret < 0)
> >               goto err_dpcd_read;
> > -     tc->link.coding8b10b = tmp[0] & BIT(0);
> > +
> >       tc->link.scrambler_dis = 0;
> >       /* read assr */
> >       ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
> > @@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
> >               tc->link.base.num_lanes,
> >               (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
> >               "enhanced" : "non-enhanced");
> > -     dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
> >       dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
> >               tc->link.assr, tc->assr);
> >
> > @@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
> >       /* DOWNSPREAD_CTRL */
> >       tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
> >       /* MAIN_LINK_CHANNEL_CODING_SET */
> > -     tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
> > +     tmp[1] =  DP_SET_ANSI_8B10B;
> >       ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
> >       if (ret < 0)
> >               goto err_dpcd_write;
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Laurent Pinchart]

Hi Andrey,

On Tue, Apr 23, 2019 at 11:19:17AM +0300, Andrey Gusakov wrote:
> On Sun, Apr 21, 2019 at 12:14 AM Laurent Pinchart wrote:
> > On Tue, Mar 26, 2019 at 12:31:27PM +0200, Tomi Valkeinen wrote:
> >> DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> >> the ANSI 8B10B bit set in DPCD, even if it should always be set.
> >
> > Makes you wonder why the bit is present :-) I've checked the DP v1.0
> > specification, and even though the bit isn't documented as being always
> > 1, 8B/10B encoding is mandatory, so this should be safe from a DP point
> > of view.
> >
> > Without access to the TC358767 datasheet I can't tell what use cases
> > were intended for disabling 8B/10B encoding. Could it be related to
> > video sources that already provide X3.230-1994 encoded data ? In any
> > case this shouldn't be driven by the sink but by the source, so
> 
> Datasheet only describes this bit in register, without any additional
> information how it should be handled.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Andrey, as this feature was present in the initial driver version that
> > you authored, do you have more information about its intended use cases
> > ?
> 
> During initial driver development I had one eDP display that reports 0 in Bit 0
> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
> So I had to disable 8B10 encoding on tc358767 side to make this display
> work.

Out of curiosity, how does the eDP display recover the clock without
8B/10B encoding ?

> On other hand if there are displays that report zero bit 0 in
> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
> reasonable.
> 
> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
> register after setting it and check if 8b10b actually enabled?

This sounds like a reasonable thing to try. Tomi, do you still have
accesss to those faulty monitors ?

> >> The tc358767 driver currently respects that flag, and turns the encoding
> >> off if the monitor does not have the bit set, which then results in the
> >> monitor not working.
> >>
> >> This patch makes the driver to always use ANSI 8B10B encoding, and drops
> >> the 'coding8b10b' field which is no longer used.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
> >>  1 file changed, 3 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> >> index 11a50f7bb4be..163c594fa6ac 100644
> >> --- a/drivers/gpu/drm/bridge/tc358767.c
> >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> >> @@ -188,7 +188,6 @@ struct tc_edp_link {
> >>       u8                      assr;
> >>       int                     scrambler_dis;
> >>       int                     spread;
> >> -     int                     coding8b10b;
> >>       u8                      swing;
> >>       u8                      preemp;
> >>  };
> >> @@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
> >>        * No training pattern, skew lane 1 data by two LSCLK cycles with
> >>        * respect to lane 0 data, AutoCorrect Mode = 0
> >>        */
> >> -     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
> >> +     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
> >>
> >>       if (tc->link.scrambler_dis)
> >>               reg |= DP0_SRCCTRL_SCRMBLDIS;   /* Scrambler Disabled */
> >> -     if (tc->link.coding8b10b)
> >> -             /* Enable 8/10B Encoder (TxData[19:16] not used) */
> >> -             reg |= DP0_SRCCTRL_EN810B;
> >>       if (tc->link.spread)
> >>               reg |= DP0_SRCCTRL_SSCG;        /* Spread Spectrum Enable */
> >>       if (tc->link.base.num_lanes == 2)
> >> @@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
> >>       ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
> >>       if (ret < 0)
> >>               goto err_dpcd_read;
> >> -     tc->link.coding8b10b = tmp[0] & BIT(0);
> >> +
> >>       tc->link.scrambler_dis = 0;
> >>       /* read assr */
> >>       ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
> >> @@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
> >>               tc->link.base.num_lanes,
> >>               (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
> >>               "enhanced" : "non-enhanced");
> >> -     dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
> >>       dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
> >>               tc->link.assr, tc->assr);
> >>
> >> @@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
> >>       /* DOWNSPREAD_CTRL */
> >>       tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
> >>       /* MAIN_LINK_CHANNEL_CODING_SET */
> >> -     tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
> >> +     tmp[1] =  DP_SET_ANSI_8B10B;
> >>       ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
> >>       if (ret < 0)
> >>               goto err_dpcd_write;

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Andrey Gusakov]

Hi Laurent,

On Tue, Apr 23, 2019 at 5:56 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Andrey,
>
> On Tue, Apr 23, 2019 at 11:19:17AM +0300, Andrey Gusakov wrote:
> > On Sun, Apr 21, 2019 at 12:14 AM Laurent Pinchart wrote:
> > > On Tue, Mar 26, 2019 at 12:31:27PM +0200, Tomi Valkeinen wrote:
> > >> DP always uses ANSI 8B10B encoding. Some monitors (old?) may not have
> > >> the ANSI 8B10B bit set in DPCD, even if it should always be set.
> > >
> > > Makes you wonder why the bit is present :-) I've checked the DP v1.0
> > > specification, and even though the bit isn't documented as being always
> > > 1, 8B/10B encoding is mandatory, so this should be safe from a DP point
> > > of view.
> > >
> > > Without access to the TC358767 datasheet I can't tell what use cases
> > > were intended for disabling 8B/10B encoding. Could it be related to
> > > video sources that already provide X3.230-1994 encoded data ? In any
> > > case this shouldn't be driven by the sink but by the source, so
> >
> > Datasheet only describes this bit in register, without any additional
> > information how it should be handled.
> >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > Andrey, as this feature was present in the initial driver version that
> > > you authored, do you have more information about its intended use cases
> > > ?
> >
> > During initial driver development I had one eDP display that reports 0 in Bit 0
> > (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
> > Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
> > (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
> > So I had to disable 8B10 encoding on tc358767 side to make this display
> > work.
>
> Out of curiosity, how does the eDP display recover the clock without
> 8B/10B encoding ?
Sorry, I have no much knowledge how it works on phy level.
I thought additional 2 bit are used for DC balancing only.

Andrey.
>
> > On other hand if there are displays that report zero bit 0 in
> > MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
> > reasonable.
> >
> > May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
> > register after setting it and check if 8b10b actually enabled?
>
> This sounds like a reasonable thing to try. Tomi, do you still have
> accesss to those faulty monitors ?
>
> > >> The tc358767 driver currently respects that flag, and turns the encoding
> > >> off if the monitor does not have the bit set, which then results in the
> > >> monitor not working.
> > >>
> > >> This patch makes the driver to always use ANSI 8B10B encoding, and drops
> > >> the 'coding8b10b' field which is no longer used.
> > >>
> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > >> ---
> > >>  drivers/gpu/drm/bridge/tc358767.c | 11 +++--------
> > >>  1 file changed, 3 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > >> index 11a50f7bb4be..163c594fa6ac 100644
> > >> --- a/drivers/gpu/drm/bridge/tc358767.c
> > >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> > >> @@ -188,7 +188,6 @@ struct tc_edp_link {
> > >>       u8                      assr;
> > >>       int                     scrambler_dis;
> > >>       int                     spread;
> > >> -     int                     coding8b10b;
> > >>       u8                      swing;
> > >>       u8                      preemp;
> > >>  };
> > >> @@ -390,13 +389,10 @@ static u32 tc_srcctrl(struct tc_data *tc)
> > >>        * No training pattern, skew lane 1 data by two LSCLK cycles with
> > >>        * respect to lane 0 data, AutoCorrect Mode = 0
> > >>        */
> > >> -     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW;
> > >> +     u32 reg = DP0_SRCCTRL_NOTP | DP0_SRCCTRL_LANESKEW | DP0_SRCCTRL_EN810B;
> > >>
> > >>       if (tc->link.scrambler_dis)
> > >>               reg |= DP0_SRCCTRL_SCRMBLDIS;   /* Scrambler Disabled */
> > >> -     if (tc->link.coding8b10b)
> > >> -             /* Enable 8/10B Encoder (TxData[19:16] not used) */
> > >> -             reg |= DP0_SRCCTRL_EN810B;
> > >>       if (tc->link.spread)
> > >>               reg |= DP0_SRCCTRL_SSCG;        /* Spread Spectrum Enable */
> > >>       if (tc->link.base.num_lanes == 2)
> > >> @@ -635,7 +631,7 @@ static int tc_get_display_props(struct tc_data *tc)
> > >>       ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
> > >>       if (ret < 0)
> > >>               goto err_dpcd_read;
> > >> -     tc->link.coding8b10b = tmp[0] & BIT(0);
> > >> +
> > >>       tc->link.scrambler_dis = 0;
> > >>       /* read assr */
> > >>       ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
> > >> @@ -649,7 +645,6 @@ static int tc_get_display_props(struct tc_data *tc)
> > >>               tc->link.base.num_lanes,
> > >>               (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
> > >>               "enhanced" : "non-enhanced");
> > >> -     dev_dbg(tc->dev, "ANSI 8B/10B: %d\n", tc->link.coding8b10b);
> > >>       dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
> > >>               tc->link.assr, tc->assr);
> > >>
> > >> @@ -951,7 +946,7 @@ static int tc_main_link_setup(struct tc_data *tc)
> > >>       /* DOWNSPREAD_CTRL */
> > >>       tmp[0] = tc->link.spread ? DP_SPREAD_AMP_0_5 : 0x00;
> > >>       /* MAIN_LINK_CHANNEL_CODING_SET */
> > >> -     tmp[1] =  tc->link.coding8b10b ? DP_SET_ANSI_8B10B : 0x00;
> > >> +     tmp[1] =  DP_SET_ANSI_8B10B;
> > >>       ret = drm_dp_dpcd_write(aux, DP_DOWNSPREAD_CTRL, tmp, 2);
> > >>       if (ret < 0)
> > >>               goto err_dpcd_write;
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Tomi Valkeinen]

On 23/04/2019 17:56, Laurent Pinchart wrote:

>> During initial driver development I had one eDP display that reports 0 in Bit 0
>> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
>> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
>> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
>> So I had to disable 8B10 encoding on tc358767 side to make this display
>> work.
> 
> Out of curiosity, how does the eDP display recover the clock without
> 8B/10B encoding ?
> 
>> On other hand if there are displays that report zero bit 0 in
>> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
>> reasonable.
>>
>> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
>> register after setting it and check if 8b10b actually enabled?
> 
> This sounds like a reasonable thing to try. Tomi, do you still have
> accesss to those faulty monitors ?

On my monitor it does not seem to matter whether I write 0 or 1 to
MAIN_LINK_CHANNEL_CODING_SET, as long as I have 8b10b enabled on
TC358767 side. The writes do go through, and I can read the written bit
back.

So... I guess when we talk about eDP panels, there may be all kinds of
oddities, as they're usually meant to be used with a certain configuration.

But if everyone agrees that 8B10B is a normal, required feature of DP,
and there is an eDP panel that needs 8B10B disabled, maybe that panel
has to be handled separately as a special case? A dts entry to disable
8B10B? Or something. But it does not sound like a good idea for the
driver to try to guess this.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Laurent Pinchart]

Hi Tomi,

On Fri, May 03, 2019 at 02:43:51PM +0300, Tomi Valkeinen wrote:
> On 23/04/2019 17:56, Laurent Pinchart wrote:
> 
> >> During initial driver development I had one eDP display that reports 0 in Bit 0
> >> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
> >> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
> >> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
> >> So I had to disable 8B10 encoding on tc358767 side to make this display
> >> work.
> > 
> > Out of curiosity, how does the eDP display recover the clock without
> > 8B/10B encoding ?
> > 
> >> On other hand if there are displays that report zero bit 0 in
> >> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
> >> reasonable.
> >>
> >> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
> >> register after setting it and check if 8b10b actually enabled?
> > 
> > This sounds like a reasonable thing to try. Tomi, do you still have
> > accesss to those faulty monitors ?
> 
> On my monitor it does not seem to matter whether I write 0 or 1 to
> MAIN_LINK_CHANNEL_CODING_SET, as long as I have 8b10b enabled on
> TC358767 side. The writes do go through, and I can read the written bit
> back.
> 
> So... I guess when we talk about eDP panels, there may be all kinds of
> oddities, as they're usually meant to be used with a certain configuration.
> 
> But if everyone agrees that 8B10B is a normal, required feature of DP,
> and there is an eDP panel that needs 8B10B disabled, maybe that panel
> has to be handled separately as a special case? A dts entry to disable
> 8B10B? Or something. But it does not sound like a good idea for the
> driver to try to guess this.

As reported by Andrey, there is at least one panel that would break with
this patch. I agree 8b10b should be the default, but if the above
procedure works for all the panels we know about, is there an issue
implementing it ?

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Tomi Valkeinen]

On 03/05/2019 15:48, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Fri, May 03, 2019 at 02:43:51PM +0300, Tomi Valkeinen wrote:
>> On 23/04/2019 17:56, Laurent Pinchart wrote:
>>
>>>> During initial driver development I had one eDP display that reports 0 in Bit 0
>>>> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
>>>> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
>>>> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
>>>> So I had to disable 8B10 encoding on tc358767 side to make this display
>>>> work.
>>>
>>> Out of curiosity, how does the eDP display recover the clock without
>>> 8B/10B encoding ?
>>>
>>>> On other hand if there are displays that report zero bit 0 in
>>>> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
>>>> reasonable.
>>>>
>>>> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
>>>> register after setting it and check if 8b10b actually enabled?
>>>
>>> This sounds like a reasonable thing to try. Tomi, do you still have
>>> accesss to those faulty monitors ?
>>
>> On my monitor it does not seem to matter whether I write 0 or 1 to
>> MAIN_LINK_CHANNEL_CODING_SET, as long as I have 8b10b enabled on
>> TC358767 side. The writes do go through, and I can read the written bit
>> back.
>>
>> So... I guess when we talk about eDP panels, there may be all kinds of
>> oddities, as they're usually meant to be used with a certain configuration.
>>
>> But if everyone agrees that 8B10B is a normal, required feature of DP,
>> and there is an eDP panel that needs 8B10B disabled, maybe that panel
>> has to be handled separately as a special case? A dts entry to disable
>> 8B10B? Or something. But it does not sound like a good idea for the
>> driver to try to guess this.
> 
> As reported by Andrey, there is at least one panel that would break with
> this patch. I agree 8b10b should be the default, but if the above
> procedure works for all the panels we know about, is there an issue
> implementing it ?

Well, we don't have data for a big set of panels. I'm worried that such
a change, which, in my opinion, makes the driver guess whether to enable
or disable 8b10b, can break other panels or monitors if the guess
doesn't go right. Especially as disabling 8b10b does not sound like a
valid setup "officially".

I agree that if the panel Andrey mentioned is still used, we need to
handle it somehow. But I think explicitly handling such a case is better
than guessing.

And isn't this something that's not really TC358767 specific? If that
panel is used with other bridges, we need to deal with this case with
those bridges too? Or is TC358767 the only bridge that allows disabling
8b10b?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Laurent Pinchart]

Hi Tomi,

On Fri, May 03, 2019 at 04:17:41PM +0300, Tomi Valkeinen wrote:
> On 03/05/2019 15:48, Laurent Pinchart wrote:
> > On Fri, May 03, 2019 at 02:43:51PM +0300, Tomi Valkeinen wrote:
> >> On 23/04/2019 17:56, Laurent Pinchart wrote:
> >>
> >>>> During initial driver development I had one eDP display that reports 0 in Bit 0
> >>>> (ANSI 8B/10B) of DPCD reg 0x0006 (MAIN_LINK_CHANNEL_CODING).
> >>>> Also it does not react on setting Bit 0 (SET_ANSI 8B10B) in 0x0108
> >>>> (MAIN_LINK_CHANNEL_CODING_SET) - after reading back it was 0 again.
> >>>> So I had to disable 8B10 encoding on tc358767 side to make this display
> >>>> work.
> >>>
> >>> Out of curiosity, how does the eDP display recover the clock without
> >>> 8B/10B encoding ?
> >>>
> >>>> On other hand if there are displays that report zero bit 0 in
> >>>> MAIN_LINK_CHANNEL_CODING while needing 8b10b then this patch looks
> >>>> reasonable.
> >>>>
> >>>> May be driver should read back MAIN_LINK_CHANNEL_CODING_SET
> >>>> register after setting it and check if 8b10b actually enabled?
> >>>
> >>> This sounds like a reasonable thing to try. Tomi, do you still have
> >>> accesss to those faulty monitors ?
> >>
> >> On my monitor it does not seem to matter whether I write 0 or 1 to
> >> MAIN_LINK_CHANNEL_CODING_SET, as long as I have 8b10b enabled on
> >> TC358767 side. The writes do go through, and I can read the written bit
> >> back.
> >>
> >> So... I guess when we talk about eDP panels, there may be all kinds of
> >> oddities, as they're usually meant to be used with a certain configuration.
> >>
> >> But if everyone agrees that 8B10B is a normal, required feature of DP,
> >> and there is an eDP panel that needs 8B10B disabled, maybe that panel
> >> has to be handled separately as a special case? A dts entry to disable
> >> 8B10B? Or something. But it does not sound like a good idea for the
> >> driver to try to guess this.
> > 
> > As reported by Andrey, there is at least one panel that would break with
> > this patch. I agree 8b10b should be the default, but if the above
> > procedure works for all the panels we know about, is there an issue
> > implementing it ?
> 
> Well, we don't have data for a big set of panels. I'm worried that such
> a change, which, in my opinion, makes the driver guess whether to enable
> or disable 8b10b, can break other panels or monitors if the guess
> doesn't go right. Especially as disabling 8b10b does not sound like a
> valid setup "officially".
> 
> I agree that if the panel Andrey mentioned is still used, we need to
> handle it somehow. But I think explicitly handling such a case is better
> than guessing.

The risk may not be worth it, I agree. I would explain this in details
in the commit message though, and add a comment to the code as well, to
ease future debugging.

> And isn't this something that's not really TC358767 specific? If that
> panel is used with other bridges, we need to deal with this case with
> those bridges too? Or is TC358767 the only bridge that allows disabling
> 8b10b?

I don't know about other bridges, but I think it would need to be
handled globally, yes.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Tomi Valkeinen]

Hi Laurent, Andrey,

On 03/05/2019 20:11, Laurent Pinchart wrote:
>> I agree that if the panel Andrey mentioned is still used, we need to
>> handle it somehow. But I think explicitly handling such a case is better
>> than guessing.
> 
> The risk may not be worth it, I agree. I would explain this in details
> in the commit message though, and add a comment to the code as well, to
> ease future debugging.

Andrey, do you still have the panel that doesn't work with 8b10b? Is it
used in real life (i.e. it was not just a temporary development phase
panel)? What's the model, and is there a public datasheet?

Everywhere I look, I always see 8b10b as mandatory for all DP versions
for the main link. If the panel in question requires 8b10b to be
disabled, I would imagine that mentioned in its datasheet, as it's kind
of a big thing. I would guess that the panel doesn't work with many eDP
sources.

>> And isn't this something that's not really TC358767 specific? If that
>> panel is used with other bridges, we need to deal with this case with
>> those bridges too? Or is TC358767 the only bridge that allows disabling
>> 8b10b?
> 
> I don't know about other bridges, but I think it would need to be
> handled globally, yes.

Ok. This does sound like a relatively big work, adding a new field to
simple panel, or maybe a new DT property to the panels, and making the
bridges work use that data (even if we'd add the support only to
tc358767 for now).

I don't want to break Andrey's panel, but I have to say I'm not very
enthusiastic about this work either =).

The DP 1.0 spec does say that PRBS7 test pattern is not 8b10b encoded. I
understand this meaning that 8b10b is not used for some particular
tests, which would explain why 8b10b can be turned off in tc358767 (and
maybe that also means it can be turned off in all/most other DP sources).

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use

[Andrey Smirnov]

On Mon, May 6, 2019 at 2:59 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Hi Laurent, Andrey,
>
> On 03/05/2019 20:11, Laurent Pinchart wrote:
> >> I agree that if the panel Andrey mentioned is still used, we need to
> >> handle it somehow. But I think explicitly handling such a case is better
> >> than guessing.
> >
> > The risk may not be worth it, I agree. I would explain this in details
> > in the commit message though, and add a comment to the code as well, to
> > ease future debugging.
>
> Andrey, do you still have the panel that doesn't work with 8b10b? Is it
> used in real life (i.e. it was not just a temporary development phase
> panel)? What's the model, and is there a public datasheet?

Note that I am a different Andrey, and I can't speak about the
original panel that caused the issue. However, production units are
shipped with this panel:

https://www.data-modul.com/productfinder/sites/default/files/products/NL192108AC13-02D_specification_12023727.pdf

which seems to do pretty standard DP stuff. In all of my testing of
Tomi's patches, I haven't seen any problems so far (I still have to
test v3 though), so I think we can carefully proceed assuming that
that weird panel was just a fluke.

Thanks,
Andrey Smirnov
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 04/22] drm/bridge: tc358767: cleanup spread & scrambler_dis

[Tomi Valkeinen]

Minor cleanups:
- Use bool for boolean fields
- Use DP_MAX_DOWNSPREAD_0_5 instead of BIT(0)
- debug print down-spread and scrambler status

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 163c594fa6ac..8e53073f0e35 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -186,8 +186,8 @@ module_param_named(test, tc_test_pattern, bool, 0644);
 struct tc_edp_link {
 	struct drm_dp_link	base;
 	u8			assr;
-	int			scrambler_dis;
-	int			spread;
+	bool			scrambler_dis;
+	bool			spread;
 	u8			swing;
 	u8			preemp;
 };
@@ -626,13 +626,13 @@ static int tc_get_display_props(struct tc_data *tc)
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, tmp);
 	if (ret < 0)
 		goto err_dpcd_read;
-	tc->link.spread = tmp[0] & BIT(0); /* 0.5% down spread */
+	tc->link.spread = tmp[0] & DP_MAX_DOWNSPREAD_0_5;
 
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
 	if (ret < 0)
 		goto err_dpcd_read;
 
-	tc->link.scrambler_dis = 0;
+	tc->link.scrambler_dis = false;
 	/* read assr */
 	ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
 	if (ret < 0)
@@ -645,6 +645,9 @@ static int tc_get_display_props(struct tc_data *tc)
 		tc->link.base.num_lanes,
 		(tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
 		"enhanced" : "non-enhanced");
+	dev_dbg(tc->dev, "Downspread: %s, scrambler: %s\n",
+		tc->link.spread ? "0.5%" : "0.0%",
+		tc->link.scrambler_dis ? "disabled" : "enabled");
 	dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
 		tc->link.assr, tc->assr);
 
@@ -934,7 +937,7 @@ static int tc_main_link_setup(struct tc_data *tc)
 			dev_dbg(dev, "Failed to switch display ASSR to %d, falling back to unscrambled mode\n",
 				 tc->assr);
 			/* trying with disabled scrambler */
-			tc->link.scrambler_dis = 1;
+			tc->link.scrambler_dis = true;
 		}
 	}
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 04/22] drm/bridge: tc358767: cleanup spread & scrambler_dis

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> Minor cleanups:
> - Use bool for boolean fields
> - Use DP_MAX_DOWNSPREAD_0_5 instead of BIT(0)
> - debug print down-spread and scrambler status
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 04/22] drm/bridge: tc358767: cleanup spread & scrambler_dis

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:28PM +0200, Tomi Valkeinen wrote:
> Minor cleanups:
> - Use bool for boolean fields
> - Use DP_MAX_DOWNSPREAD_0_5 instead of BIT(0)
> - debug print down-spread and scrambler status
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/tc358767.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 163c594fa6ac..8e53073f0e35 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -186,8 +186,8 @@ module_param_named(test, tc_test_pattern, bool, 0644);
>  struct tc_edp_link {
>  	struct drm_dp_link	base;
>  	u8			assr;
> -	int			scrambler_dis;
> -	int			spread;
> +	bool			scrambler_dis;
> +	bool			spread;
>  	u8			swing;
>  	u8			preemp;
>  };
> @@ -626,13 +626,13 @@ static int tc_get_display_props(struct tc_data *tc)
>  	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, tmp);
>  	if (ret < 0)
>  		goto err_dpcd_read;
> -	tc->link.spread = tmp[0] & BIT(0); /* 0.5% down spread */
> +	tc->link.spread = tmp[0] & DP_MAX_DOWNSPREAD_0_5;
>  
>  	ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
>  	if (ret < 0)
>  		goto err_dpcd_read;
>  
> -	tc->link.scrambler_dis = 0;
> +	tc->link.scrambler_dis = false;
>  	/* read assr */
>  	ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
>  	if (ret < 0)
> @@ -645,6 +645,9 @@ static int tc_get_display_props(struct tc_data *tc)
>  		tc->link.base.num_lanes,
>  		(tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ?
>  		"enhanced" : "non-enhanced");
> +	dev_dbg(tc->dev, "Downspread: %s, scrambler: %s\n",
> +		tc->link.spread ? "0.5%" : "0.0%",
> +		tc->link.scrambler_dis ? "disabled" : "enabled");
>  	dev_dbg(tc->dev, "Display ASSR: %d, TC358767 ASSR: %d\n",
>  		tc->link.assr, tc->assr);
>  
> @@ -934,7 +937,7 @@ static int tc_main_link_setup(struct tc_data *tc)
>  			dev_dbg(dev, "Failed to switch display ASSR to %d, falling back to unscrambled mode\n",
>  				 tc->assr);
>  			/* trying with disabled scrambler */
> -			tc->link.scrambler_dis = 1;
> +			tc->link.scrambler_dis = true;
>  		}
>  	}
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 05/22] drm/bridge: tc358767: remove unused swing & preemp

[Tomi Valkeinen]

swing and preemp fields are not used. Remove them.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 8e53073f0e35..7ef8d754b4ff 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -188,8 +188,6 @@ struct tc_edp_link {
 	u8			assr;
 	bool			scrambler_dis;
 	bool			spread;
-	u8			swing;
-	u8			preemp;
 };
 
 struct tc_data {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 05/22] drm/bridge: tc358767: remove unused swing & preemp

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> swing and preemp fields are not used. Remove them.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 05/22] drm/bridge: tc358767: remove unused swing & preemp

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:29PM +0200, Tomi Valkeinen wrote:
> swing and preemp fields are not used. Remove them.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/tc358767.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 8e53073f0e35..7ef8d754b4ff 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -188,8 +188,6 @@ struct tc_edp_link {
>  	u8			assr;
>  	bool			scrambler_dis;
>  	bool			spread;
> -	u8			swing;
> -	u8			preemp;
>  };
>  
>  struct tc_data {

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 06/22] drm/bridge: tc358767: cleanup aux_link_setup

[Tomi Valkeinen]

Modify aux_link_setup so that it does not use tc->link, and thus makes
aux setup independent of the link probing.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7ef8d754b4ff..f5c232a9064e 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -542,7 +542,6 @@ static int tc_aux_link_setup(struct tc_data *tc)
 	unsigned long rate;
 	u32 value;
 	int ret;
-	u32 dp_phy_ctrl;
 
 	rate = clk_get_rate(tc->refclk);
 	switch (rate) {
@@ -567,10 +566,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
 	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
 	tc_write(SYS_PLLPARAM, value);
 
-	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
-	if (tc->link.base.num_lanes == 2)
-		dp_phy_ctrl |= PHY_2LANE;
-	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
+	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
 
 	/*
 	 * Initially PLLs are in bypass. Force PLL parameter update,
@@ -587,8 +583,9 @@ static int tc_aux_link_setup(struct tc_data *tc)
 	if (ret == -ETIMEDOUT) {
 		dev_err(tc->dev, "Timeout waiting for PHY to become ready");
 		return ret;
-	} else if (ret)
+	} else if (ret) {
 		goto err;
+	}
 
 	/* Setup AUX link */
 	tc_write(DP0_AUXCFG1, AUX_RX_FILTER_EN |
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 06/22] drm/bridge: tc358767: cleanup aux_link_setup

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> Modify aux_link_setup so that it does not use tc->link, and thus makes
> aux setup independent of the link probing.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 7ef8d754b4ff..f5c232a9064e 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -542,7 +542,6 @@ static int tc_aux_link_setup(struct tc_data *tc)
>  	unsigned long rate;
>  	u32 value;
>  	int ret;
> -	u32 dp_phy_ctrl;
>  
>  	rate = clk_get_rate(tc->refclk);
>  	switch (rate) {
> @@ -567,10 +566,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
>  	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
>  	tc_write(SYS_PLLPARAM, value);
>  
> -	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
> -	if (tc->link.base.num_lanes == 2)
> -		dp_phy_ctrl |= PHY_2LANE;
> -	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
> +	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);


Description does not explain why removal of PHY_2LANE is OK.

I guess because it is set anyway before training but that needs lurking
into the code.

With description fixed:

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


>  
>  	/*
>  	 * Initially PLLs are in bypass. Force PLL parameter update,
> @@ -587,8 +583,9 @@ static int tc_aux_link_setup(struct tc_data *tc)
>  	if (ret == -ETIMEDOUT) {
>  		dev_err(tc->dev, "Timeout waiting for PHY to become ready");
>  		return ret;
> -	} else if (ret)
> +	} else if (ret) {
>  		goto err;
> +	}
>  
>  	/* Setup AUX link */
>  	tc_write(DP0_AUXCFG1, AUX_RX_FILTER_EN |


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 06/22] drm/bridge: tc358767: cleanup aux_link_setup

[Tomi Valkeinen]

On 15/04/2019 10:38, Andrzej Hajda wrote:
> On 26.03.2019 11:31, Tomi Valkeinen wrote:
>> Modify aux_link_setup so that it does not use tc->link, and thus makes
>> aux setup independent of the link probing.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358767.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 7ef8d754b4ff..f5c232a9064e 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -542,7 +542,6 @@ static int tc_aux_link_setup(struct tc_data *tc)
>>  	unsigned long rate;
>>  	u32 value;
>>  	int ret;
>> -	u32 dp_phy_ctrl;
>>  
>>  	rate = clk_get_rate(tc->refclk);
>>  	switch (rate) {
>> @@ -567,10 +566,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
>>  	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
>>  	tc_write(SYS_PLLPARAM, value);
>>  
>> -	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
>> -	if (tc->link.base.num_lanes == 2)
>> -		dp_phy_ctrl |= PHY_2LANE;
>> -	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
>> +	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
> 
> 
> Description does not explain why removal of PHY_2LANE is OK.
> 
> I guess because it is set anyway before training but that needs lurking
> into the code.

Good point, the desc is a bit unclear. The reason is that we're setting
up only AUX here, so anything related to the data-lanes is don't-care.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 06/22] drm/bridge: tc358767: cleanup aux_link_setup

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Apr 15, 2019 at 10:52:38AM +0300, Tomi Valkeinen wrote:
> On 15/04/2019 10:38, Andrzej Hajda wrote:
> > On 26.03.2019 11:31, Tomi Valkeinen wrote:
> >> Modify aux_link_setup so that it does not use tc->link, and thus makes
> >> aux setup independent of the link probing.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/bridge/tc358767.c | 9 +++------
> >>  1 file changed, 3 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> >> index 7ef8d754b4ff..f5c232a9064e 100644
> >> --- a/drivers/gpu/drm/bridge/tc358767.c
> >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> >> @@ -542,7 +542,6 @@ static int tc_aux_link_setup(struct tc_data *tc)
> >>  	unsigned long rate;
> >>  	u32 value;
> >>  	int ret;
> >> -	u32 dp_phy_ctrl;
> >>  
> >>  	rate = clk_get_rate(tc->refclk);
> >>  	switch (rate) {
> >> @@ -567,10 +566,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
> >>  	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
> >>  	tc_write(SYS_PLLPARAM, value);
> >>  
> >> -	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
> >> -	if (tc->link.base.num_lanes == 2)
> >> -		dp_phy_ctrl |= PHY_2LANE;
> >> -	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
> >> +	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
> > 
> > 
> > Description does not explain why removal of PHY_2LANE is OK.
> > 
> > I guess because it is set anyway before training but that needs lurking
> > into the code.
> 
> Good point, the desc is a bit unclear. The reason is that we're setting
> up only AUX here, so anything related to the data-lanes is don't-care.

And also because the tc_aux_link_setup() function is called at probe
time, while the tc_main_link_setup() function is called later and will
overwrite the DP_PHY_CTRL register with bits related to the main link.

With the description fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 07/22] drm/bridge: tc358767: move video stream setup to tc_main_link_stream

[Tomi Valkeinen]

The driver currently sets the video stream registers in
tc_main_link_setup. One should be able to establish the DP link without
any video stream, so a more logical place is to configure the stream in
the tc_main_link_stream. So move them there.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index f5c232a9064e..86b272422281 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1003,15 +1003,6 @@ static int tc_main_link_setup(struct tc_data *tc)
 		return -EAGAIN;
 	}
 
-	ret = tc_set_video_mode(tc, tc->mode);
-	if (ret)
-		goto err;
-
-	/* Set M/N */
-	ret = tc_stream_clock_calc(tc);
-	if (ret)
-		goto err;
-
 	return 0;
 err_dpcd_read:
 	dev_err(tc->dev, "Failed to read DPCD: %d\n", ret);
@@ -1030,6 +1021,15 @@ static int tc_main_link_stream(struct tc_data *tc, int state)
 	dev_dbg(tc->dev, "stream: %d\n", state);
 
 	if (state) {
+		ret = tc_set_video_mode(tc, tc->mode);
+		if (ret)
+			goto err;
+
+		/* Set M/N */
+		ret = tc_stream_clock_calc(tc);
+		if (ret)
+			goto err;
+
 		value = VID_MN_GEN | DP_EN;
 		if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
 			value |= EF_EN;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 07/22] drm/bridge: tc358767: move video stream setup to tc_main_link_stream

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> The driver currently sets the video stream registers in
> tc_main_link_setup. One should be able to establish the DP link without
> any video stream, so a more logical place is to configure the stream in
> the tc_main_link_stream. So move them there.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index f5c232a9064e..86b272422281 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1003,15 +1003,6 @@ static int tc_main_link_setup(struct tc_data *tc)
>  		return -EAGAIN;
>  	}
>  
> -	ret = tc_set_video_mode(tc, tc->mode);
> -	if (ret)
> -		goto err;
> -
> -	/* Set M/N */
> -	ret = tc_stream_clock_calc(tc);
> -	if (ret)
> -		goto err;
> -
>  	return 0;
>  err_dpcd_read:
>  	dev_err(tc->dev, "Failed to read DPCD: %d\n", ret);
> @@ -1030,6 +1021,15 @@ static int tc_main_link_stream(struct tc_data *tc, int state)
>  	dev_dbg(tc->dev, "stream: %d\n", state);
>  
>  	if (state) {


Maybe would be better to simplify the conditional:

if (!state) {

    tc_write(..., 0);

    return 0;

}

...

To avoid indentation of increasing chunk of code. Anyway:

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 07/22] drm/bridge: tc358767: move video stream setup to tc_main_link_stream

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:31PM +0200, Tomi Valkeinen wrote:
> The driver currently sets the video stream registers in
> tc_main_link_setup. One should be able to establish the DP link without
> any video stream, so a more logical place is to configure the stream in
> the tc_main_link_stream. So move them there.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index f5c232a9064e..86b272422281 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1003,15 +1003,6 @@ static int tc_main_link_setup(struct tc_data *tc)
>  		return -EAGAIN;
>  	}
>  
> -	ret = tc_set_video_mode(tc, tc->mode);
> -	if (ret)
> -		goto err;
> -
> -	/* Set M/N */
> -	ret = tc_stream_clock_calc(tc);
> -	if (ret)
> -		goto err;
> -
>  	return 0;
>  err_dpcd_read:
>  	dev_err(tc->dev, "Failed to read DPCD: %d\n", ret);
> @@ -1030,6 +1021,15 @@ static int tc_main_link_stream(struct tc_data *tc, int state)
>  	dev_dbg(tc->dev, "stream: %d\n", state);
>  
>  	if (state) {
> +		ret = tc_set_video_mode(tc, tc->mode);
> +		if (ret)
> +			goto err;
> +
> +		/* Set M/N */
> +		ret = tc_stream_clock_calc(tc);
> +		if (ret)
> +			goto err;
> +

Assuming this change will have a purpose further down in the series,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

On a side note you may want to remove the err label in
tc_stream_clock_calc(), or even inline the write to DP0_VIDMNGEN1 here
directly.

>  		value = VID_MN_GEN | DP_EN;
>  		if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
>  			value |= EF_EN;

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 07/22] drm/bridge: tc358767: move video stream setup to tc_main_link_stream

[Tomi Valkeinen]

On 21/04/2019 00:25, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 26, 2019 at 12:31:31PM +0200, Tomi Valkeinen wrote:
>> The driver currently sets the video stream registers in
>> tc_main_link_setup. One should be able to establish the DP link without
>> any video stream, so a more logical place is to configure the stream in
>> the tc_main_link_stream. So move them there.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358767.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index f5c232a9064e..86b272422281 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -1003,15 +1003,6 @@ static int tc_main_link_setup(struct tc_data *tc)
>>  		return -EAGAIN;
>>  	}
>>  
>> -	ret = tc_set_video_mode(tc, tc->mode);
>> -	if (ret)
>> -		goto err;
>> -
>> -	/* Set M/N */
>> -	ret = tc_stream_clock_calc(tc);
>> -	if (ret)
>> -		goto err;
>> -
>>  	return 0;
>>  err_dpcd_read:
>>  	dev_err(tc->dev, "Failed to read DPCD: %d\n", ret);
>> @@ -1030,6 +1021,15 @@ static int tc_main_link_stream(struct tc_data *tc, int state)
>>  	dev_dbg(tc->dev, "stream: %d\n", state);
>>  
>>  	if (state) {
>> +		ret = tc_set_video_mode(tc, tc->mode);
>> +		if (ret)
>> +			goto err;
>> +
>> +		/* Set M/N */
>> +		ret = tc_stream_clock_calc(tc);
>> +		if (ret)
>> +			goto err;
>> +
> 
> Assuming this change will have a purpose further down in the series,

The purpose is mainly cleanup. The series doesn't go to the point where
the link and the stream could be enabled/disabled independently, but it
tries to separate those functionalities for clarity.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> On a side note you may want to remove the err label in
> tc_stream_clock_calc(), or even inline the write to DP0_VIDMNGEN1 here
> directly.

err label is needed by the tc_write macro. Yes, ugly. There's another
series that cleans those up.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 08/22] drm/bridge: tc358767: split stream enable/disable

[Tomi Valkeinen]

It is nicer to have enable/disable functions instead of set(bool enable)
style function.

Split tc_main_link_stream into tc_stream_enable and tc_stream_disable.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 86b272422281..bfc673bd5986 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1013,47 +1013,56 @@ static int tc_main_link_setup(struct tc_data *tc)
 	return ret;
 }
 
-static int tc_main_link_stream(struct tc_data *tc, int state)
+static int tc_stream_enable(struct tc_data *tc)
 {
 	int ret;
 	u32 value;
 
-	dev_dbg(tc->dev, "stream: %d\n", state);
+	dev_dbg(tc->dev, "stream enable\n");
 
-	if (state) {
-		ret = tc_set_video_mode(tc, tc->mode);
-		if (ret)
-			goto err;
+	ret = tc_set_video_mode(tc, tc->mode);
+	if (ret)
+		goto err;
 
-		/* Set M/N */
-		ret = tc_stream_clock_calc(tc);
-		if (ret)
-			goto err;
+	/* Set M/N */
+	ret = tc_stream_clock_calc(tc);
+	if (ret)
+		goto err;
 
-		value = VID_MN_GEN | DP_EN;
-		if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
-			value |= EF_EN;
-		tc_write(DP0CTL, value);
-		/*
-		 * VID_EN assertion should be delayed by at least N * LSCLK
-		 * cycles from the time VID_MN_GEN is enabled in order to
-		 * generate stable values for VID_M. LSCLK is 270 MHz or
-		 * 162 MHz, VID_N is set to 32768 in  tc_stream_clock_calc(),
-		 * so a delay of at least 203 us should suffice.
-		 */
-		usleep_range(500, 1000);
-		value |= VID_EN;
-		tc_write(DP0CTL, value);
-		/* Set input interface */
-		value = DP0_AUDSRC_NO_INPUT;
-		if (tc_test_pattern)
-			value |= DP0_VIDSRC_COLOR_BAR;
-		else
-			value |= DP0_VIDSRC_DPI_RX;
-		tc_write(SYSCTRL, value);
-	} else {
-		tc_write(DP0CTL, 0);
-	}
+	value = VID_MN_GEN | DP_EN;
+	if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
+		value |= EF_EN;
+	tc_write(DP0CTL, value);
+	/*
+	 * VID_EN assertion should be delayed by at least N * LSCLK
+	 * cycles from the time VID_MN_GEN is enabled in order to
+	 * generate stable values for VID_M. LSCLK is 270 MHz or
+	 * 162 MHz, VID_N is set to 32768 in  tc_stream_clock_calc(),
+	 * so a delay of at least 203 us should suffice.
+	 */
+	usleep_range(500, 1000);
+	value |= VID_EN;
+	tc_write(DP0CTL, value);
+	/* Set input interface */
+	value = DP0_AUDSRC_NO_INPUT;
+	if (tc_test_pattern)
+		value |= DP0_VIDSRC_COLOR_BAR;
+	else
+		value |= DP0_VIDSRC_DPI_RX;
+	tc_write(SYSCTRL, value);
+
+	return 0;
+err:
+	return ret;
+}
+
+static int tc_stream_disable(struct tc_data *tc)
+{
+	int ret;
+
+	dev_dbg(tc->dev, "stream disable\n");
+
+	tc_write(DP0CTL, 0);
 
 	return 0;
 err:
@@ -1078,7 +1087,7 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 		return;
 	}
 
-	ret = tc_main_link_stream(tc, 1);
+	ret = tc_stream_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link stream start error: %d\n", ret);
 		return;
@@ -1094,7 +1103,7 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
 
 	drm_panel_disable(tc->panel);
 
-	ret = tc_main_link_stream(tc, 0);
+	ret = tc_stream_disable(tc);
 	if (ret < 0)
 		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
 }
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 08/22] drm/bridge: tc358767: split stream enable/disable

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> It is nicer to have enable/disable functions instead of set(bool enable)
> style function.
>
> Split tc_main_link_stream into tc_stream_enable and tc_stream_disable.


So my comment regarding previous patch was already addresed :)


>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 08/22] drm/bridge: tc358767: split stream enable/disable

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:32PM +0200, Tomi Valkeinen wrote:
> It is nicer to have enable/disable functions instead of set(bool enable)
> style function.

When the two functions have nothing in common, yes.

> Split tc_main_link_stream into tc_stream_enable and tc_stream_disable.

Should you keep the tc_main_link_ prefix ? I suppose it is implied in a
way, as the stream is carried over the main link.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 81 +++++++++++++++++--------------
>  1 file changed, 45 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 86b272422281..bfc673bd5986 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1013,47 +1013,56 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	return ret;
>  }
>  
> -static int tc_main_link_stream(struct tc_data *tc, int state)
> +static int tc_stream_enable(struct tc_data *tc)
>  {
>  	int ret;
>  	u32 value;
>  
> -	dev_dbg(tc->dev, "stream: %d\n", state);
> +	dev_dbg(tc->dev, "stream enable\n");

Maybe "enable video stream\n" (and similarly for the tc_stream_disable()
function) ?

>  
> -	if (state) {
> -		ret = tc_set_video_mode(tc, tc->mode);
> -		if (ret)
> -			goto err;
> +	ret = tc_set_video_mode(tc, tc->mode);
> +	if (ret)
> +		goto err;

Let's return ret directly and remove the err label.

With these issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
> -		/* Set M/N */
> -		ret = tc_stream_clock_calc(tc);
> -		if (ret)
> -			goto err;
> +	/* Set M/N */
> +	ret = tc_stream_clock_calc(tc);
> +	if (ret)
> +		goto err;
>  
> -		value = VID_MN_GEN | DP_EN;
> -		if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
> -			value |= EF_EN;
> -		tc_write(DP0CTL, value);
> -		/*
> -		 * VID_EN assertion should be delayed by at least N * LSCLK
> -		 * cycles from the time VID_MN_GEN is enabled in order to
> -		 * generate stable values for VID_M. LSCLK is 270 MHz or
> -		 * 162 MHz, VID_N is set to 32768 in  tc_stream_clock_calc(),
> -		 * so a delay of at least 203 us should suffice.
> -		 */
> -		usleep_range(500, 1000);
> -		value |= VID_EN;
> -		tc_write(DP0CTL, value);
> -		/* Set input interface */
> -		value = DP0_AUDSRC_NO_INPUT;
> -		if (tc_test_pattern)
> -			value |= DP0_VIDSRC_COLOR_BAR;
> -		else
> -			value |= DP0_VIDSRC_DPI_RX;
> -		tc_write(SYSCTRL, value);
> -	} else {
> -		tc_write(DP0CTL, 0);
> -	}
> +	value = VID_MN_GEN | DP_EN;
> +	if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
> +		value |= EF_EN;
> +	tc_write(DP0CTL, value);
> +	/*
> +	 * VID_EN assertion should be delayed by at least N * LSCLK
> +	 * cycles from the time VID_MN_GEN is enabled in order to
> +	 * generate stable values for VID_M. LSCLK is 270 MHz or
> +	 * 162 MHz, VID_N is set to 32768 in  tc_stream_clock_calc(),
> +	 * so a delay of at least 203 us should suffice.
> +	 */
> +	usleep_range(500, 1000);
> +	value |= VID_EN;
> +	tc_write(DP0CTL, value);
> +	/* Set input interface */
> +	value = DP0_AUDSRC_NO_INPUT;
> +	if (tc_test_pattern)
> +		value |= DP0_VIDSRC_COLOR_BAR;
> +	else
> +		value |= DP0_VIDSRC_DPI_RX;
> +	tc_write(SYSCTRL, value);
> +
> +	return 0;
> +err:
> +	return ret;
> +}
> +
> +static int tc_stream_disable(struct tc_data *tc)
> +{
> +	int ret;
> +
> +	dev_dbg(tc->dev, "stream disable\n");
> +
> +	tc_write(DP0CTL, 0);
>  
>  	return 0;
>  err:
> @@ -1078,7 +1087,7 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>  		return;
>  	}
>  
> -	ret = tc_main_link_stream(tc, 1);
> +	ret = tc_stream_enable(tc);
>  	if (ret < 0) {
>  		dev_err(tc->dev, "main link stream start error: %d\n", ret);
>  		return;
> @@ -1094,7 +1103,7 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
>  
>  	drm_panel_disable(tc->panel);
>  
> -	ret = tc_main_link_stream(tc, 0);
> +	ret = tc_stream_disable(tc);
>  	if (ret < 0)
>  		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
>  }

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 08/22] drm/bridge: tc358767: split stream enable/disable

[Tomi Valkeinen]

On 21/04/2019 00:29, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 26, 2019 at 12:31:32PM +0200, Tomi Valkeinen wrote:
>> It is nicer to have enable/disable functions instead of set(bool enable)
>> style function.
> 
> When the two functions have nothing in common, yes.
> 
>> Split tc_main_link_stream into tc_stream_enable and tc_stream_disable.
> 
> Should you keep the tc_main_link_ prefix ? I suppose it is implied in a
> way, as the stream is carried over the main link.

It sounds a bit redundant, only making the functions names longer.

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358767.c | 81 +++++++++++++++++--------------
>>  1 file changed, 45 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 86b272422281..bfc673bd5986 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -1013,47 +1013,56 @@ static int tc_main_link_setup(struct tc_data *tc)
>>  	return ret;
>>  }
>>  
>> -static int tc_main_link_stream(struct tc_data *tc, int state)
>> +static int tc_stream_enable(struct tc_data *tc)
>>  {
>>  	int ret;
>>  	u32 value;
>>  
>> -	dev_dbg(tc->dev, "stream: %d\n", state);
>> +	dev_dbg(tc->dev, "stream enable\n");
> 
> Maybe "enable video stream\n" (and similarly for the tc_stream_disable()
> function) ?

Ok.

>>  
>> -	if (state) {
>> -		ret = tc_set_video_mode(tc, tc->mode);
>> -		if (ret)
>> -			goto err;
>> +	ret = tc_set_video_mode(tc, tc->mode);
>> +	if (ret)
>> +		goto err;
> 
> Let's return ret directly and remove the err label.

Can't remove the err label, because of the tc_write() calls...

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 08/22] drm/bridge: tc358767: split stream enable/disable

[Laurent Pinchart]

Hi Tomi,

On Fri, May 03, 2019 at 12:20:49PM +0300, Tomi Valkeinen wrote:
> On 21/04/2019 00:29, Laurent Pinchart wrote:
> > On Tue, Mar 26, 2019 at 12:31:32PM +0200, Tomi Valkeinen wrote:
> >> It is nicer to have enable/disable functions instead of set(bool enable)
> >> style function.
> > 
> > When the two functions have nothing in common, yes.
> > 
> >> Split tc_main_link_stream into tc_stream_enable and tc_stream_disable.
> > 
> > Should you keep the tc_main_link_ prefix ? I suppose it is implied in a
> > way, as the stream is carried over the main link.
> 
> It sounds a bit redundant, only making the functions names longer.

A bit, but it also makes the code a bit clearer in my opinion. Up to
you.

> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/bridge/tc358767.c | 81 +++++++++++++++++--------------
> >>  1 file changed, 45 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> >> index 86b272422281..bfc673bd5986 100644
> >> --- a/drivers/gpu/drm/bridge/tc358767.c
> >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> >> @@ -1013,47 +1013,56 @@ static int tc_main_link_setup(struct tc_data *tc)
> >>  	return ret;
> >>  }
> >>  
> >> -static int tc_main_link_stream(struct tc_data *tc, int state)
> >> +static int tc_stream_enable(struct tc_data *tc)
> >>  {
> >>  	int ret;
> >>  	u32 value;
> >>  
> >> -	dev_dbg(tc->dev, "stream: %d\n", state);
> >> +	dev_dbg(tc->dev, "stream enable\n");
> > 
> > Maybe "enable video stream\n" (and similarly for the tc_stream_disable()
> > function) ?
> 
> Ok.
> 
> >>  
> >> -	if (state) {
> >> -		ret = tc_set_video_mode(tc, tc->mode);
> >> -		if (ret)
> >> -			goto err;
> >> +	ret = tc_set_video_mode(tc, tc->mode);
> >> +	if (ret)
> >> +		goto err;
> > 
> > Let's return ret directly and remove the err label.
> 
> Can't remove the err label, because of the tc_write() calls...

:-(

I'd love to see this getting fixed. The best way I've found so far would
be

int tc_write(struct tc_data *tc, unsigned int reg, unsigned int value, int &err)
{
	unsigned int ret;

	if (err && *err)
		return *err;

	ret = do_the_write_here(..., reg, value);
	if (err)
		*err = ret;

	return ret;
}

This can be used as

	int ret = 0;

	tc_write(tc, REG0, VAL0, &ret);
	...
	tc_write(tc, REGn, VALn, &ret);

	if (ret)
		/* Error handling */

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 08/22] drm/bridge: tc358767: split stream enable/disable

[Tomi Valkeinen]

On 03/05/2019 15:55, Laurent Pinchart wrote:

>>>>  
>>>> -	if (state) {
>>>> -		ret = tc_set_video_mode(tc, tc->mode);
>>>> -		if (ret)
>>>> -			goto err;
>>>> +	ret = tc_set_video_mode(tc, tc->mode);
>>>> +	if (ret)
>>>> +		goto err;
>>>
>>> Let's return ret directly and remove the err label.
>>
>> Can't remove the err label, because of the tc_write() calls...
> 
> :-(
> 
> I'd love to see this getting fixed. The best way I've found so far would
> be

And by fixed you mean cleaned up?

Yes, it's a mess. That's why I want to get this series merged asap, so
Andrey can rebase his series, and we can proceed with all the cleanups
=). His series removes these macros that require the err label.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 09/22] drm/bridge: tc358767: move PXL PLL enable/disable to stream enable/disable

[Tomi Valkeinen]

We set up the PXL PLL inside tc_main_link_setup. This is unnecessary,
and makes tc_main_link_setup depend on the video-mode, which should not
be the case. As PXL PLL is used only for the video stream (and only when
using the HW test pattern), let's move the PXL PLL setup into
tc_stream_enable.

Also, currently the PXL PLL is only disabled if the driver if removed.
Let's disable the PXL PLL when the stream is disabled.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index bfc673bd5986..f8039149a4e8 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -877,14 +877,6 @@ static int tc_main_link_setup(struct tc_data *tc)
 	tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
 	tc_wait_pll_lock(tc);
 
-	/* PXL PLL setup */
-	if (tc_test_pattern) {
-		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
-				    1000 * tc->mode->clock);
-		if (ret)
-			goto err;
-	}
-
 	/* Reset/Enable Main Links */
 	dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
 	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
@@ -1020,6 +1012,14 @@ static int tc_stream_enable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "stream enable\n");
 
+	/* PXL PLL setup */
+	if (tc_test_pattern) {
+		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
+				    1000 * tc->mode->clock);
+		if (ret)
+			goto err;
+	}
+
 	ret = tc_set_video_mode(tc, tc->mode);
 	if (ret)
 		goto err;
@@ -1064,6 +1064,8 @@ static int tc_stream_disable(struct tc_data *tc)
 
 	tc_write(DP0CTL, 0);
 
+	tc_pxl_pll_dis(tc);
+
 	return 0;
 err:
 	return ret;
@@ -1390,8 +1392,6 @@ static int tc_remove(struct i2c_client *client)
 	drm_bridge_remove(&tc->bridge);
 	drm_dp_aux_unregister(&tc->aux);
 
-	tc_pxl_pll_dis(tc);
-
 	return 0;
 }
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 09/22] drm/bridge: tc358767: move PXL PLL enable/disable to stream enable/disable

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> We set up the PXL PLL inside tc_main_link_setup. This is unnecessary,
> and makes tc_main_link_setup depend on the video-mode, which should not
> be the case. As PXL PLL is used only for the video stream (and only when
> using the HW test pattern), let's move the PXL PLL setup into
> tc_stream_enable.
>
> Also, currently the PXL PLL is only disabled if the driver if removed.
> Let's disable the PXL PLL when the stream is disabled.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 09/22] drm/bridge: tc358767: move PXL PLL enable/disable to stream enable/disable

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:33PM +0200, Tomi Valkeinen wrote:
> We set up the PXL PLL inside tc_main_link_setup. This is unnecessary,
> and makes tc_main_link_setup depend on the video-mode, which should not
> be the case. As PXL PLL is used only for the video stream (and only when
> using the HW test pattern), let's move the PXL PLL setup into
> tc_stream_enable.
> 
> Also, currently the PXL PLL is only disabled if the driver if removed.
> Let's disable the PXL PLL when the stream is disabled.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/tc358767.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index bfc673bd5986..f8039149a4e8 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -877,14 +877,6 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
>  	tc_wait_pll_lock(tc);
>  
> -	/* PXL PLL setup */
> -	if (tc_test_pattern) {
> -		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
> -				    1000 * tc->mode->clock);
> -		if (ret)
> -			goto err;
> -	}
> -
>  	/* Reset/Enable Main Links */
>  	dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
>  	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
> @@ -1020,6 +1012,14 @@ static int tc_stream_enable(struct tc_data *tc)
>  
>  	dev_dbg(tc->dev, "stream enable\n");
>  
> +	/* PXL PLL setup */
> +	if (tc_test_pattern) {
> +		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
> +				    1000 * tc->mode->clock);
> +		if (ret)
> +			goto err;
> +	}
> +
>  	ret = tc_set_video_mode(tc, tc->mode);
>  	if (ret)
>  		goto err;
> @@ -1064,6 +1064,8 @@ static int tc_stream_disable(struct tc_data *tc)
>  
>  	tc_write(DP0CTL, 0);
>  
> +	tc_pxl_pll_dis(tc);
> +
>  	return 0;
>  err:
>  	return ret;
> @@ -1390,8 +1392,6 @@ static int tc_remove(struct i2c_client *client)
>  	drm_bridge_remove(&tc->bridge);
>  	drm_dp_aux_unregister(&tc->aux);
>  
> -	tc_pxl_pll_dis(tc);
> -
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 10/22] drm/bridge: tc358767: add link disable function

[Tomi Valkeinen]

Currently we have tc_main_link_setup(), which configures and enabled the
link, but we have no counter-part for disabling the link.

Add tc_main_link_disable, and rename tc_main_link_setup to
tc_main_link_enable.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index f8039149a4e8..fe5fd7db0247 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -822,7 +822,7 @@ static int tc_link_training(struct tc_data *tc, int pattern)
 	return ret;
 }
 
-static int tc_main_link_setup(struct tc_data *tc)
+static int tc_main_link_enable(struct tc_data *tc)
 {
 	struct drm_dp_aux *aux = &tc->aux;
 	struct device *dev = tc->dev;
@@ -837,6 +837,8 @@ static int tc_main_link_setup(struct tc_data *tc)
 	if (!tc->mode)
 		return -EINVAL;
 
+	dev_dbg(tc->dev, "link enable\n");
+
 	tc_write(DP0_SRCCTRL, tc_srcctrl(tc));
 	/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
 	tc_write(DP1_SRCCTRL,
@@ -1005,6 +1007,20 @@ static int tc_main_link_setup(struct tc_data *tc)
 	return ret;
 }
 
+static int tc_main_link_disable(struct tc_data *tc)
+{
+	int ret;
+
+	dev_dbg(tc->dev, "link disable\n");
+
+	tc_write(DP0_SRCCTRL, 0);
+	tc_write(DP0CTL, 0);
+
+	return 0;
+err:
+	return ret;
+}
+
 static int tc_stream_enable(struct tc_data *tc)
 {
 	int ret;
@@ -1083,15 +1099,16 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
-	ret = tc_main_link_setup(tc);
+	ret = tc_main_link_enable(tc);
 	if (ret < 0) {
-		dev_err(tc->dev, "main link setup error: %d\n", ret);
+		dev_err(tc->dev, "main link enable error: %d\n", ret);
 		return;
 	}
 
 	ret = tc_stream_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link stream start error: %d\n", ret);
+		tc_main_link_disable(tc);
 		return;
 	}
 
@@ -1108,6 +1125,10 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
 	ret = tc_stream_disable(tc);
 	if (ret < 0)
 		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
+
+	ret = tc_main_link_disable(tc);
+	if (ret < 0)
+		dev_err(tc->dev, "main link disable error: %d\n", ret);
 }
 
 static void tc_bridge_post_disable(struct drm_bridge *bridge)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 10/22] drm/bridge: tc358767: add link disable function

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> Currently we have tc_main_link_setup(), which configures and enabled the
enables
> link, but we have no counter-part for disabling the link.
>
> Add tc_main_link_disable, and rename tc_main_link_setup to
> tc_main_link_enable.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index f8039149a4e8..fe5fd7db0247 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -822,7 +822,7 @@ static int tc_link_training(struct tc_data *tc, int pattern)
>  	return ret;
>  }
>  
> -static int tc_main_link_setup(struct tc_data *tc)
> +static int tc_main_link_enable(struct tc_data *tc)
>  {
>  	struct drm_dp_aux *aux = &tc->aux;
>  	struct device *dev = tc->dev;
> @@ -837,6 +837,8 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	if (!tc->mode)
>  		return -EINVAL;
>  
> +	dev_dbg(tc->dev, "link enable\n");
> +
>  	tc_write(DP0_SRCCTRL, tc_srcctrl(tc));
>  	/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
>  	tc_write(DP1_SRCCTRL,
> @@ -1005,6 +1007,20 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	return ret;
>  }
>  
> +static int tc_main_link_disable(struct tc_data *tc)
> +{
> +	int ret;
> +
> +	dev_dbg(tc->dev, "link disable\n");
> +
> +	tc_write(DP0_SRCCTRL, 0);
> +	tc_write(DP0CTL, 0);


The same register is set in stream_disable, is it correct? Looks
suspicious, disabling stream and link should be different things.

Regards

Andrzej


> +
> +	return 0;
> +err:
> +	return ret;
> +}
> +
>  static int tc_stream_enable(struct tc_data *tc)
>  {
>  	int ret;
> @@ -1083,15 +1099,16 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
>  
> -	ret = tc_main_link_setup(tc);
> +	ret = tc_main_link_enable(tc);
>  	if (ret < 0) {
> -		dev_err(tc->dev, "main link setup error: %d\n", ret);
> +		dev_err(tc->dev, "main link enable error: %d\n", ret);
>  		return;
>  	}
>  
>  	ret = tc_stream_enable(tc);
>  	if (ret < 0) {
>  		dev_err(tc->dev, "main link stream start error: %d\n", ret);
> +		tc_main_link_disable(tc);
>  		return;
>  	}
>  
> @@ -1108,6 +1125,10 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
>  	ret = tc_stream_disable(tc);
>  	if (ret < 0)
>  		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
> +
> +	ret = tc_main_link_disable(tc);
> +	if (ret < 0)
> +		dev_err(tc->dev, "main link disable error: %d\n", ret);
>  }
>  
>  static void tc_bridge_post_disable(struct drm_bridge *bridge)


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 10/22] drm/bridge: tc358767: add link disable function

[Tomi Valkeinen]

On 15/04/2019 11:36, Andrzej Hajda wrote:

>> +static int tc_main_link_disable(struct tc_data *tc)
>> +{
>> +	int ret;
>> +
>> +	dev_dbg(tc->dev, "link disable\n");
>> +
>> +	tc_write(DP0_SRCCTRL, 0);
>> +	tc_write(DP0CTL, 0);
> 
> 
> The same register is set in stream_disable, is it correct? Looks
> suspicious, disabling stream and link should be different things.

Good point. It's probably not correct. With these patches, the link and
stream are always enabled and disabled together, so it shouldn't cause
any problems at the moment.

During my debugging and development, at some point I had a version where
I enabled the link right away when we got HPD high, mostly to help my
debugging. That's why I separated link and stream setup (although I
think that's a logical design in any case).

However, I never did try disabling only stream, and keeping the link up,
so it may well be non-functional. Or, well, it clearly is
non-functional, as we disable the link (DP0CTL) in tc_stream_disable()...

So this is mostly about just adding the architecture to have separate
link/stream handling, but the functionality is not there. I should
perhaps add a comment to the code to point this out.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 10/22] drm/bridge: tc358767: add link disable function

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Apr 15, 2019 at 02:39:21PM +0300, Tomi Valkeinen wrote:
> On 15/04/2019 11:36, Andrzej Hajda wrote:
> 
> >> +static int tc_main_link_disable(struct tc_data *tc)
> >> +{
> >> +	int ret;
> >> +
> >> +	dev_dbg(tc->dev, "link disable\n");
> >> +
> >> +	tc_write(DP0_SRCCTRL, 0);
> >> +	tc_write(DP0CTL, 0);
> > 
> > The same register is set in stream_disable, is it correct? Looks
> > suspicious, disabling stream and link should be different things.
> 
> Good point. It's probably not correct. With these patches, the link and
> stream are always enabled and disabled together, so it shouldn't cause
> any problems at the moment.
> 
> During my debugging and development, at some point I had a version where
> I enabled the link right away when we got HPD high, mostly to help my
> debugging. That's why I separated link and stream setup (although I
> think that's a logical design in any case).
> 
> However, I never did try disabling only stream, and keeping the link up,
> so it may well be non-functional. Or, well, it clearly is
> non-functional, as we disable the link (DP0CTL) in tc_stream_disable()...
> 
> So this is mostly about just adding the architecture to have separate
> link/stream handling, but the functionality is not there. I should
> perhaps add a comment to the code to point this out.

This makes me a bit uneasy about the rework, as the
tc_main_link_enable() function doesn't enable the link (it doesn't set
the DP_EN bit in DP0CTL), and stream disabling separately from link
disabling is broken. Is this fixable ?

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 10/22] drm/bridge: tc358767: add link disable function

[Tomi Valkeinen]

On 21/04/2019 00:39, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, Apr 15, 2019 at 02:39:21PM +0300, Tomi Valkeinen wrote:
>> On 15/04/2019 11:36, Andrzej Hajda wrote:
>>
>>>> +static int tc_main_link_disable(struct tc_data *tc)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	dev_dbg(tc->dev, "link disable\n");
>>>> +
>>>> +	tc_write(DP0_SRCCTRL, 0);
>>>> +	tc_write(DP0CTL, 0);
>>>
>>> The same register is set in stream_disable, is it correct? Looks
>>> suspicious, disabling stream and link should be different things.
>>
>> Good point. It's probably not correct. With these patches, the link and
>> stream are always enabled and disabled together, so it shouldn't cause
>> any problems at the moment.
>>
>> During my debugging and development, at some point I had a version where
>> I enabled the link right away when we got HPD high, mostly to help my
>> debugging. That's why I separated link and stream setup (although I
>> think that's a logical design in any case).
>>
>> However, I never did try disabling only stream, and keeping the link up,
>> so it may well be non-functional. Or, well, it clearly is
>> non-functional, as we disable the link (DP0CTL) in tc_stream_disable()...
>>
>> So this is mostly about just adding the architecture to have separate
>> link/stream handling, but the functionality is not there. I should
>> perhaps add a comment to the code to point this out.
> 
> This makes me a bit uneasy about the rework, as the
> tc_main_link_enable() function doesn't enable the link (it doesn't set
> the DP_EN bit in DP0CTL), and stream disabling separately from link
> disabling is broken. Is this fixable ?

tc_main_link_enable() does set DP_EN.

It's true that managing the link and stream separately isn't working,
but it wasn't there earlier either, and it still isn't, as the driver
never enables/disables the link/stream separately. We need more code and
logic to manage them separately, and this series just starts the work by
trying to organize the code better.

I'll look at tc_stream_disable(), as tc_write(DP0CTL, 0) there does not
look correct. Probably we can just clear VID_EN.

Afaics, this series should not introduce any issues, but fixes many
issues that were present. There's lots more to do, and there's another
series from Andrey that cleans up many things that I didn't touch in
this series. And after Andrey's cleanups, I think we can then start
looking at adding more logic like the separate link/stream enabling if
needed.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 11/22] drm/bridge: tc358767: ensure DP is disabled before LT

[Tomi Valkeinen]

Link training will sometimes fail if the DP link is, for some whatever
reason, enabled when tc_main_link_enable() is called.

Ensure that the DP link is disabled by setting DP0CTL to 0 as the first
thing.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index fe5fd7db0247..f628575c9de9 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -839,6 +839,8 @@ static int tc_main_link_enable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "link enable\n");
 
+	tc_write(DP0CTL, 0);
+
 	tc_write(DP0_SRCCTRL, tc_srcctrl(tc));
 	/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
 	tc_write(DP1_SRCCTRL,
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 11/22] drm/bridge: tc358767: ensure DP is disabled before LT

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> Link training will sometimes fail if the DP link is, for some whatever
> reason, enabled when tc_main_link_enable() is called.


Only tc_stream_enable enables it, does it mean that link training can
happen after tc_stream_enable?

It suggests that driver/device preforms strange things, is it true? Or
just overprotection?


Regards

Andrzej


>
> Ensure that the DP link is disabled by setting DP0CTL to 0 as the first
> thing.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index fe5fd7db0247..f628575c9de9 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -839,6 +839,8 @@ static int tc_main_link_enable(struct tc_data *tc)
>  
>  	dev_dbg(tc->dev, "link enable\n");
>  
> +	tc_write(DP0CTL, 0);
> +



>  	tc_write(DP0_SRCCTRL, tc_srcctrl(tc));
>  	/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
>  	tc_write(DP1_SRCCTRL,


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 11/22] drm/bridge: tc358767: ensure DP is disabled before LT

[Tomi Valkeinen]

On 15/04/2019 11:49, Andrzej Hajda wrote:
> On 26.03.2019 11:31, Tomi Valkeinen wrote:
>> Link training will sometimes fail if the DP link is, for some whatever
>> reason, enabled when tc_main_link_enable() is called.
> 
> 
> Only tc_stream_enable enables it, does it mean that link training can
> happen after tc_stream_enable?
> 
> It suggests that driver/device preforms strange things, is it true? Or
> just overprotection?

Just protection. I did try all kinds of things when trying to get the
link setup stable and having DP0CTL enabled before link training was one
of the problems I encountered.

In theory DP0CTL should always be disabled when we call
tc_main_link_enable, but I thought it best leave it there in case we
accidentally leave DP0CTL enabled via some error path or such.

Maybe we should have a WARN there if DP0CTL is enabled (and then clear
it), so that we might find those error cases.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 11/22] drm/bridge: tc358767: ensure DP is disabled before LT

[Laurent Pinchart]

Hi Tomi,

On Mon, Apr 15, 2019 at 02:26:20PM +0300, Tomi Valkeinen wrote:
> On 15/04/2019 11:49, Andrzej Hajda wrote:
> > On 26.03.2019 11:31, Tomi Valkeinen wrote:
> >> Link training will sometimes fail if the DP link is, for some whatever
> >> reason, enabled when tc_main_link_enable() is called.
> > 
> > Only tc_stream_enable enables it, does it mean that link training can
> > happen after tc_stream_enable?
> > 
> > It suggests that driver/device preforms strange things, is it true? Or
> > just overprotection?
> 
> Just protection. I did try all kinds of things when trying to get the
> link setup stable and having DP0CTL enabled before link training was one
> of the problems I encountered.
> 
> In theory DP0CTL should always be disabled when we call
> tc_main_link_enable, but I thought it best leave it there in case we
> accidentally leave DP0CTL enabled via some error path or such.
> 
> Maybe we should have a WARN there if DP0CTL is enabled (and then clear
> it), so that we might find those error cases.

I'd prefer a warning, as incorrect error paths (or such) could also
create lots of other issues. I don't think we should protect against one
particular issue that is supposed never to happen and then consider that
we're safe.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 12/22] drm/bridge: tc358767: remove unnecessary msleep

[Tomi Valkeinen]

For some reason the driver has a msleep(100) after writing to
DP_PHY_CTRL. Toshiba's documentation doesn't suggest any delay is
needed, and I have not seen any issues with the sleep removed.

Drop it, as msleep(100) is a rather big one.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index f628575c9de9..2a6c0c0d47a6 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -872,7 +872,6 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (tc->link.base.num_lanes == 2)
 		dp_phy_ctrl |= PHY_2LANE;
 	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
-	msleep(100);
 
 	/* PLL setup */
 	tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 12/22] drm/bridge: tc358767: remove unnecessary msleep

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> For some reason the driver has a msleep(100) after writing to
> DP_PHY_CTRL. Toshiba's documentation doesn't suggest any delay is
> needed, and I have not seen any issues with the sleep removed.
>
> Drop it, as msleep(100) is a rather big one.
>
> 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 | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index f628575c9de9..2a6c0c0d47a6 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -872,7 +872,6 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	if (tc->link.base.num_lanes == 2)
>  		dp_phy_ctrl |= PHY_2LANE;
>  	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
> -	msleep(100);
>  
>  	/* PLL setup */
>  	tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 12/22] drm/bridge: tc358767: remove unnecessary msleep

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:36PM +0200, Tomi Valkeinen wrote:
> For some reason the driver has a msleep(100) after writing to
> DP_PHY_CTRL. Toshiba's documentation doesn't suggest any delay is
> needed, and I have not seen any issues with the sleep removed.

I can't comment on this as I don't have access to the datasheet. The
code change itself is fine.

Andrey, do you remember why a sleep was added there in the first place ?

> Drop it, as msleep(100) is a rather big one.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index f628575c9de9..2a6c0c0d47a6 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -872,7 +872,6 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	if (tc->link.base.num_lanes == 2)
>  		dp_phy_ctrl |= PHY_2LANE;
>  	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
> -	msleep(100);
>  
>  	/* PLL setup */
>  	tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 12/22] drm/bridge: tc358767: remove unnecessary msleep

[Andrey Gusakov]

Hi


On Sun, Apr 21, 2019 at 12:43 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Tomi,
>
> Thank you for the patch.
>
> On Tue, Mar 26, 2019 at 12:31:36PM +0200, Tomi Valkeinen wrote:
> > For some reason the driver has a msleep(100) after writing to
> > DP_PHY_CTRL. Toshiba's documentation doesn't suggest any delay is
> > needed, and I have not seen any issues with the sleep removed.
>
> I can't comment on this as I don't have access to the datasheet. The
> code change itself is fine.
>
> Andrey, do you remember why a sleep was added there in the first place ?
I see no reason for this delay. Seems this was not cleaned-up after debug.

Reviewed-by: Andrey Gusakov <andrey.gusakov@cogentembedded.com>

>
> > Drop it, as msleep(100) is a rather big one.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index f628575c9de9..2a6c0c0d47a6 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -872,7 +872,6 @@ static int tc_main_link_enable(struct tc_data *tc)
> >       if (tc->link.base.num_lanes == 2)
> >               dp_phy_ctrl |= PHY_2LANE;
> >       tc_write(DP_PHY_CTRL, dp_phy_ctrl);
> > -     msleep(100);
> >
> >       /* PLL setup */
> >       tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
>
> --
> Regards,
>
> Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 13/22] drm/bridge: tc358767: use more reliable seq when finishing LT

[Tomi Valkeinen]

At the end of the link training, two steps have to be taken: 1)
tc358767's LT mode is disabled by a write to DP0_SRCCTRL, and 2) Remove
LT flag in DPCD 0x102.

Toshiba's documentation tells to first write the DPCD, then modify
DP0_SRCCTRL. In my testing this often causes issues, and the link
disconnects right after those steps.

If I reverse the steps, it works every time. There's a chance that this
is DP sink specific, though, but as my testing shows this sequence to be
much more reliable, let's change it.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 2a6c0c0d47a6..700e161015af 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -958,6 +958,9 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (ret)
 		goto err;
 
+	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
+	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
+
 	/* Clear DPCD 0x102 */
 	/* Note: Can Not use DP0_SNKLTCTRL (0x06E4) short cut */
 	tmp[0] = tc->link.scrambler_dis ? DP_LINK_SCRAMBLING_DISABLE : 0x00;
@@ -965,9 +968,6 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (ret < 0)
 		goto err_dpcd_write;
 
-	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
-	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
-
 	/* Wait */
 	timeout = 100;
 	do {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 13/22] drm/bridge: tc358767: use more reliable seq when finishing LT

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> At the end of the link training, two steps have to be taken: 1)
> tc358767's LT mode is disabled by a write to DP0_SRCCTRL, and 2) Remove
> LT flag in DPCD 0x102.
>
> Toshiba's documentation tells to first write the DPCD, then modify
> DP0_SRCCTRL. In my testing this often causes issues, and the link
> disconnects right after those steps.
>
> If I reverse the steps, it works every time. There's a chance that this
> is DP sink specific, though, but as my testing shows this sequence to be
> much more reliable, let's change it.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 13/22] drm/bridge: tc358767: use more reliable seq when finishing LT

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:37PM +0200, Tomi Valkeinen wrote:
> At the end of the link training, two steps have to be taken: 1)
> tc358767's LT mode is disabled by a write to DP0_SRCCTRL, and 2) Remove
> LT flag in DPCD 0x102.
> 
> Toshiba's documentation tells to first write the DPCD, then modify
> DP0_SRCCTRL. In my testing this often causes issues, and the link
> disconnects right after those steps.
> 
> If I reverse the steps, it works every time. There's a chance that this
> is DP sink specific, though, but as my testing shows this sequence to be
> much more reliable, let's change it.

As this departs from the documentation I think it deserves a comment in
the code.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 2a6c0c0d47a6..700e161015af 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -958,6 +958,9 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	if (ret)
>  		goto err;
>  
> +	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
> +
>  	/* Clear DPCD 0x102 */
>  	/* Note: Can Not use DP0_SNKLTCTRL (0x06E4) short cut */
>  	tmp[0] = tc->link.scrambler_dis ? DP_LINK_SCRAMBLING_DISABLE : 0x00;
> @@ -965,9 +968,6 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	if (ret < 0)
>  		goto err_dpcd_write;
>  
> -	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
> -	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
> -
>  	/* Wait */
>  	timeout = 100;
>  	do {

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 13/22] drm/bridge: tc358767: use more reliable seq when finishing LT

[Tomi Valkeinen]

On 21/04/2019 00:44, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 26, 2019 at 12:31:37PM +0200, Tomi Valkeinen wrote:
>> At the end of the link training, two steps have to be taken: 1)
>> tc358767's LT mode is disabled by a write to DP0_SRCCTRL, and 2) Remove
>> LT flag in DPCD 0x102.
>>
>> Toshiba's documentation tells to first write the DPCD, then modify
>> DP0_SRCCTRL. In my testing this often causes issues, and the link
>> disconnects right after those steps.
>>
>> If I reverse the steps, it works every time. There's a chance that this
>> is DP sink specific, though, but as my testing shows this sequence to be
>> much more reliable, let's change it.
> 
> As this departs from the documentation I think it deserves a comment in
> the code.

Yep. It's not exactly documentation, but just an excel sheet that they
use for testing. The actual documentation doesn't talk about this.

I'll add a comment.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 14/22] drm/bridge: tc358767: cleanup LT result check

[Tomi Valkeinen]

The driver has a loop after ending link training, where it reads the
DPCD link status and prints an error if that status is not ok.

The loop is unnecessary, as far as I can understand from DP specs, so
let's remove it. We can also print the more specific errors to help
debugging.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 700e161015af..220408db82f7 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -968,34 +968,42 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (ret < 0)
 		goto err_dpcd_write;
 
-	/* Wait */
-	timeout = 100;
-	do {
-		udelay(1);
-		/* Read DPCD 0x202-0x207 */
-		ret = drm_dp_dpcd_read_link_status(aux, tmp + 2);
-		if (ret < 0)
-			goto err_dpcd_read;
-	} while ((--timeout) &&
-		 !(drm_dp_channel_eq_ok(tmp + 2,  tc->link.base.num_lanes)));
+	/* Check link status */
+	ret = drm_dp_dpcd_read_link_status(aux, tmp);
+	if (ret < 0)
+		goto err_dpcd_read;
 
-	if (timeout == 0) {
-		/* Read DPCD 0x200-0x201 */
-		ret = drm_dp_dpcd_read(aux, DP_SINK_COUNT, tmp, 2);
-		if (ret < 0)
-			goto err_dpcd_read;
-		dev_err(dev, "channel(s) EQ not ok\n");
-		dev_info(dev, "0x0200 SINK_COUNT: 0x%02x\n", tmp[0]);
-		dev_info(dev, "0x0201 DEVICE_SERVICE_IRQ_VECTOR: 0x%02x\n",
-			 tmp[1]);
-		dev_info(dev, "0x0202 LANE0_1_STATUS: 0x%02x\n", tmp[2]);
-		dev_info(dev, "0x0204 LANE_ALIGN_STATUS_UPDATED: 0x%02x\n",
-			 tmp[4]);
-		dev_info(dev, "0x0205 SINK_STATUS: 0x%02x\n", tmp[5]);
-		dev_info(dev, "0x0206 ADJUST_REQUEST_LANE0_1: 0x%02x\n",
-			 tmp[6]);
-
-		return -EAGAIN;
+	ret = 0;
+
+	value = tmp[0] & DP_CHANNEL_EQ_BITS;
+
+	if (value != DP_CHANNEL_EQ_BITS) {
+		dev_err(tc->dev, "Lane 0 failed: %x\n", value);
+		ret = -ENODEV;
+	}
+
+	if (tc->link.base.num_lanes == 2) {
+		value = (tmp[0] >> 4) & DP_CHANNEL_EQ_BITS;
+
+		if (value != DP_CHANNEL_EQ_BITS) {
+			dev_err(tc->dev, "Lane 1 failed: %x\n", value);
+			ret = -ENODEV;
+		}
+
+		if (!(tmp[2] & DP_INTERLANE_ALIGN_DONE)) {
+			dev_err(tc->dev, "Interlane align failed\n");
+			ret = -ENODEV;
+		}
+	}
+
+	if (ret) {
+		dev_err(dev, "0x0202 LANE0_1_STATUS:            0x%02x\n", tmp[0]);
+		dev_err(dev, "0x0203 LANE2_3_STATUS             0x%02x\n", tmp[1]);
+		dev_err(dev, "0x0204 LANE_ALIGN_STATUS_UPDATED: 0x%02x\n", tmp[2]);
+		dev_err(dev, "0x0205 SINK_STATUS:               0x%02x\n", tmp[3]);
+		dev_err(dev, "0x0206 ADJUST_REQUEST_LANE0_1:    0x%02x\n", tmp[4]);
+		dev_err(dev, "0x0207 ADJUST_REQUEST_LANE2_3:    0x%02x\n", tmp[5]);
+		goto err;
 	}
 
 	return 0;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 14/22] drm/bridge: tc358767: cleanup LT result check

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> The driver has a loop after ending link training, where it reads the
> DPCD link status and prints an error if that status is not ok.
>
> The loop is unnecessary, as far as I can understand from DP specs, so
> let's remove it. We can also print the more specific errors to help
> debugging.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 14/22] drm/bridge: tc358767: cleanup LT result check

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:38PM +0200, Tomi Valkeinen wrote:
> The driver has a loop after ending link training, where it reads the
> DPCD link status and prints an error if that status is not ok.
> 
> The loop is unnecessary, as far as I can understand from DP specs, so
> let's remove it. We can also print the more specific errors to help
> debugging.

I see in tc_link_training() that the driver checks the channel EQ bits
through the TC358767 DP0_LTSTAT register. Does the chip read the link
status DPCD registers by itself through the AUX link ? If so we could
remove this check completely (unless we don't trust the TC358767 and
want to double-check). If not, where does it get the link status from ?

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 62 +++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 700e161015af..220408db82f7 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -968,34 +968,42 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	if (ret < 0)
>  		goto err_dpcd_write;
>  
> -	/* Wait */
> -	timeout = 100;
> -	do {
> -		udelay(1);
> -		/* Read DPCD 0x202-0x207 */
> -		ret = drm_dp_dpcd_read_link_status(aux, tmp + 2);
> -		if (ret < 0)
> -			goto err_dpcd_read;
> -	} while ((--timeout) &&
> -		 !(drm_dp_channel_eq_ok(tmp + 2,  tc->link.base.num_lanes)));
> +	/* Check link status */
> +	ret = drm_dp_dpcd_read_link_status(aux, tmp);
> +	if (ret < 0)
> +		goto err_dpcd_read;
>  
> -	if (timeout == 0) {
> -		/* Read DPCD 0x200-0x201 */
> -		ret = drm_dp_dpcd_read(aux, DP_SINK_COUNT, tmp, 2);
> -		if (ret < 0)
> -			goto err_dpcd_read;
> -		dev_err(dev, "channel(s) EQ not ok\n");
> -		dev_info(dev, "0x0200 SINK_COUNT: 0x%02x\n", tmp[0]);
> -		dev_info(dev, "0x0201 DEVICE_SERVICE_IRQ_VECTOR: 0x%02x\n",
> -			 tmp[1]);
> -		dev_info(dev, "0x0202 LANE0_1_STATUS: 0x%02x\n", tmp[2]);
> -		dev_info(dev, "0x0204 LANE_ALIGN_STATUS_UPDATED: 0x%02x\n",
> -			 tmp[4]);
> -		dev_info(dev, "0x0205 SINK_STATUS: 0x%02x\n", tmp[5]);
> -		dev_info(dev, "0x0206 ADJUST_REQUEST_LANE0_1: 0x%02x\n",
> -			 tmp[6]);
> -
> -		return -EAGAIN;
> +	ret = 0;
> +
> +	value = tmp[0] & DP_CHANNEL_EQ_BITS;
> +
> +	if (value != DP_CHANNEL_EQ_BITS) {
> +		dev_err(tc->dev, "Lane 0 failed: %x\n", value);
> +		ret = -ENODEV;
> +	}
> +
> +	if (tc->link.base.num_lanes == 2) {
> +		value = (tmp[0] >> 4) & DP_CHANNEL_EQ_BITS;
> +
> +		if (value != DP_CHANNEL_EQ_BITS) {
> +			dev_err(tc->dev, "Lane 1 failed: %x\n", value);
> +			ret = -ENODEV;
> +		}
> +
> +		if (!(tmp[2] & DP_INTERLANE_ALIGN_DONE)) {
> +			dev_err(tc->dev, "Interlane align failed\n");
> +			ret = -ENODEV;
> +		}
> +	}
> +
> +	if (ret) {
> +		dev_err(dev, "0x0202 LANE0_1_STATUS:            0x%02x\n", tmp[0]);
> +		dev_err(dev, "0x0203 LANE2_3_STATUS             0x%02x\n", tmp[1]);
> +		dev_err(dev, "0x0204 LANE_ALIGN_STATUS_UPDATED: 0x%02x\n", tmp[2]);
> +		dev_err(dev, "0x0205 SINK_STATUS:               0x%02x\n", tmp[3]);
> +		dev_err(dev, "0x0206 ADJUST_REQUEST_LANE0_1:    0x%02x\n", tmp[4]);
> +		dev_err(dev, "0x0207 ADJUST_REQUEST_LANE2_3:    0x%02x\n", tmp[5]);
> +		goto err;
>  	}
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 14/22] drm/bridge: tc358767: cleanup LT result check

[Tomi Valkeinen]

On 21/04/2019 01:06, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 26, 2019 at 12:31:38PM +0200, Tomi Valkeinen wrote:
>> The driver has a loop after ending link training, where it reads the
>> DPCD link status and prints an error if that status is not ok.
>>
>> The loop is unnecessary, as far as I can understand from DP specs, so
>> let's remove it. We can also print the more specific errors to help
>> debugging.
> 
> I see in tc_link_training() that the driver checks the channel EQ bits
> through the TC358767 DP0_LTSTAT register. Does the chip read the link
> status DPCD registers by itself through the AUX link ? If so we could

Yes. I'm not exactly sure at what point TC358767 reads the registers,
but I think it reads it at the end of link training loop. In theory the
link should stay up after that, but as described in the previous patch,
that was not what I saw in every case. So I'd rather have an explicit
check here at the end.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 15/22] drm/bridge: tc358767: clean-up link training

[Tomi Valkeinen]

The current link training code does unnecessary retry-loops, and does
extra writes to the registers. It is easier to follow the flow and
ensure it's similar to Toshiba's documentation if we deal with LT inside
tc_main_link_enable() function.

This patch adds tc_wait_link_training() which handles waiting for the LT
phase to finish, and does the necessary LT register setups in
tc_main_link_enable, without extra loops.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 220408db82f7..1c61f6205e43 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -740,83 +740,25 @@ static int tc_set_video_mode(struct tc_data *tc,
 	return ret;
 }
 
-static int tc_link_training(struct tc_data *tc, int pattern)
+static int tc_wait_link_training(struct tc_data *tc, u32 *error)
 {
-	const char * const *errors;
-	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
-		      DP0_SRCCTRL_AUTOCORRECT;
-	int timeout;
-	int retry;
+	u32 timeout = 1000;
 	u32 value;
 	int ret;
 
-	if (pattern == DP_TRAINING_PATTERN_1) {
-		srcctrl |= DP0_SRCCTRL_TP1;
-		errors = training_pattern1_errors;
-	} else {
-		srcctrl |= DP0_SRCCTRL_TP2;
-		errors = training_pattern2_errors;
-	}
-
-	/* Set DPCD 0x102 for Training Part 1 or 2 */
-	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | pattern);
-
-	tc_write(DP0_LTLOOPCTRL,
-		 (0x0f << 28) |	/* Defer Iteration Count */
-		 (0x0f << 24) |	/* Loop Iteration Count */
-		 (0x0d << 0));	/* Loop Timer Delay */
-
-	retry = 5;
 	do {
-		/* Set DP0 Training Pattern */
-		tc_write(DP0_SRCCTRL, srcctrl);
-
-		/* Enable DP0 to start Link Training */
-		tc_write(DP0CTL, DP_EN);
-
-		/* wait */
-		timeout = 1000;
-		do {
-			tc_read(DP0_LTSTAT, &value);
-			udelay(1);
-		} while ((!(value & LT_LOOPDONE)) && (--timeout));
-		if (timeout == 0) {
-			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]);
-			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
-				break;
-			if (pattern == DP_TRAINING_PATTERN_2) {
-				value &= LT_CHANNEL1_EQ_BITS |
-					 LT_INTERLANE_ALIGN_DONE |
-					 LT_CHANNEL0_EQ_BITS;
-				/* in case of two lanes */
-				if ((tc->link.base.num_lanes == 2) &&
-				    (value == (LT_CHANNEL1_EQ_BITS |
-					       LT_INTERLANE_ALIGN_DONE |
-					       LT_CHANNEL0_EQ_BITS)))
-					break;
-				/* in case of one line */
-				if ((tc->link.base.num_lanes == 1) &&
-				    (value == (LT_INTERLANE_ALIGN_DONE |
-					       LT_CHANNEL0_EQ_BITS)))
-					break;
-			}
-		}
-		/* restart */
-		tc_write(DP0CTL, 0);
-		usleep_range(10, 20);
-	} while (--retry);
-	if (retry == 0) {
-		dev_err(tc->dev, "Failed to finish training phase %d\n",
-			pattern);
+		udelay(1);
+		tc_read(DP0_LTSTAT, &value);
+	} while ((!(value & LT_LOOPDONE)) && (--timeout));
+
+	if (timeout == 0) {
+		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
+		ret = -ETIMEDOUT;
+		goto err;
 	}
 
+	*error = (value >> 8) & 0x7;
+
 	return 0;
 err:
 	return ret;
@@ -832,6 +774,7 @@ static int tc_main_link_enable(struct tc_data *tc)
 	u32 value;
 	int ret;
 	u8 tmp[8];
+	u32 error;
 
 	/* display mode should be set at this point */
 	if (!tc->mode)
@@ -950,14 +893,56 @@ static int tc_main_link_enable(struct tc_data *tc)
 	if (ret < 0)
 		goto err_dpcd_write;
 
-	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
+	/* LINK TRAINING PATTERN 1 */
+
+	/* Set DPCD 0x102 for Training Pattern 1 */
+	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
+
+	tc_write(DP0_LTLOOPCTRL,
+		 (15 << 28) |	/* Defer Iteration Count */
+		 (15 << 24) |	/* Loop Iteration Count */
+		 (0xd << 0));	/* Loop Timer Delay */
+
+	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
+		 DP0_SRCCTRL_TP1);
+
+	/* Enable DP0 to start Link Training */
+	tc_write(DP0CTL,
+		 ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
+		 DP_EN);
+
+	/* wait */
+	ret = tc_wait_link_training(tc, &error);
 	if (ret)
 		goto err;
 
-	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
+	if (error) {
+		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
+			training_pattern1_errors[error]);
+		ret = -ENODEV;
+		goto err;
+	}
+
+	/* LINK TRAINING PATTERN 2 */
+
+	/* Set DPCD 0x102 for Training Pattern 2 */
+	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
+
+	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
+		 DP0_SRCCTRL_TP2);
+
+	/* wait */
+	ret = tc_wait_link_training(tc, &error);
 	if (ret)
 		goto err;
 
+	if (error) {
+		dev_err(tc->dev, "Link training phase 2 failed: %s\n",
+			training_pattern2_errors[error]);
+		ret = -ENODEV;
+		goto err;
+	}
+
 	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
 	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 15/22] drm/bridge: tc358767: clean-up link training

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> The current link training code does unnecessary retry-loops, and does
> extra writes to the registers. It is easier to follow the flow and
> ensure it's similar to Toshiba's documentation if we deal with LT inside
> tc_main_link_enable() function.
>
> This patch adds tc_wait_link_training() which handles waiting for the LT
> phase to finish, and does the necessary LT register setups in
> tc_main_link_enable, without extra loops.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>


Btw, there is other patchset which touches the same things, have you
decided which one should go first? yours or Andrey's?


 --
Regards
Andrzej


> ---
>  drivers/gpu/drm/bridge/tc358767.c | 129 +++++++++++++-----------------
>  1 file changed, 57 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 220408db82f7..1c61f6205e43 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -740,83 +740,25 @@ static int tc_set_video_mode(struct tc_data *tc,
>  	return ret;
>  }
>  
> -static int tc_link_training(struct tc_data *tc, int pattern)
> +static int tc_wait_link_training(struct tc_data *tc, u32 *error)
>  {
> -	const char * const *errors;
> -	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
> -		      DP0_SRCCTRL_AUTOCORRECT;
> -	int timeout;
> -	int retry;
> +	u32 timeout = 1000;
>  	u32 value;
>  	int ret;
>  
> -	if (pattern == DP_TRAINING_PATTERN_1) {
> -		srcctrl |= DP0_SRCCTRL_TP1;
> -		errors = training_pattern1_errors;
> -	} else {
> -		srcctrl |= DP0_SRCCTRL_TP2;
> -		errors = training_pattern2_errors;
> -	}
> -
> -	/* Set DPCD 0x102 for Training Part 1 or 2 */
> -	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | pattern);
> -
> -	tc_write(DP0_LTLOOPCTRL,
> -		 (0x0f << 28) |	/* Defer Iteration Count */
> -		 (0x0f << 24) |	/* Loop Iteration Count */
> -		 (0x0d << 0));	/* Loop Timer Delay */
> -
> -	retry = 5;
>  	do {
> -		/* Set DP0 Training Pattern */
> -		tc_write(DP0_SRCCTRL, srcctrl);
> -
> -		/* Enable DP0 to start Link Training */
> -		tc_write(DP0CTL, DP_EN);
> -
> -		/* wait */
> -		timeout = 1000;
> -		do {
> -			tc_read(DP0_LTSTAT, &value);
> -			udelay(1);
> -		} while ((!(value & LT_LOOPDONE)) && (--timeout));
> -		if (timeout == 0) {
> -			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]);
> -			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
> -				break;
> -			if (pattern == DP_TRAINING_PATTERN_2) {
> -				value &= LT_CHANNEL1_EQ_BITS |
> -					 LT_INTERLANE_ALIGN_DONE |
> -					 LT_CHANNEL0_EQ_BITS;
> -				/* in case of two lanes */
> -				if ((tc->link.base.num_lanes == 2) &&
> -				    (value == (LT_CHANNEL1_EQ_BITS |
> -					       LT_INTERLANE_ALIGN_DONE |
> -					       LT_CHANNEL0_EQ_BITS)))
> -					break;
> -				/* in case of one line */
> -				if ((tc->link.base.num_lanes == 1) &&
> -				    (value == (LT_INTERLANE_ALIGN_DONE |
> -					       LT_CHANNEL0_EQ_BITS)))
> -					break;
> -			}
> -		}
> -		/* restart */
> -		tc_write(DP0CTL, 0);
> -		usleep_range(10, 20);
> -	} while (--retry);
> -	if (retry == 0) {
> -		dev_err(tc->dev, "Failed to finish training phase %d\n",
> -			pattern);
> +		udelay(1);
> +		tc_read(DP0_LTSTAT, &value);
> +	} while ((!(value & LT_LOOPDONE)) && (--timeout));
> +
> +	if (timeout == 0) {
> +		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
> +		ret = -ETIMEDOUT;
> +		goto err;
>  	}
>  
> +	*error = (value >> 8) & 0x7;
> +
>  	return 0;
>  err:
>  	return ret;
> @@ -832,6 +774,7 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	u32 value;
>  	int ret;
>  	u8 tmp[8];
> +	u32 error;
>  
>  	/* display mode should be set at this point */
>  	if (!tc->mode)
> @@ -950,14 +893,56 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	if (ret < 0)
>  		goto err_dpcd_write;
>  
> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
> +	/* LINK TRAINING PATTERN 1 */
> +
> +	/* Set DPCD 0x102 for Training Pattern 1 */
> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
> +
> +	tc_write(DP0_LTLOOPCTRL,
> +		 (15 << 28) |	/* Defer Iteration Count */
> +		 (15 << 24) |	/* Loop Iteration Count */
> +		 (0xd << 0));	/* Loop Timer Delay */
> +
> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
> +		 DP0_SRCCTRL_TP1);
> +
> +	/* Enable DP0 to start Link Training */
> +	tc_write(DP0CTL,
> +		 ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
> +		 DP_EN);
> +
> +	/* wait */
> +	ret = tc_wait_link_training(tc, &error);
>  	if (ret)
>  		goto err;
>  
> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
> +	if (error) {
> +		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
> +			training_pattern1_errors[error]);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	/* LINK TRAINING PATTERN 2 */
> +
> +	/* Set DPCD 0x102 for Training Pattern 2 */
> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
> +
> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
> +		 DP0_SRCCTRL_TP2);
> +
> +	/* wait */
> +	ret = tc_wait_link_training(tc, &error);
>  	if (ret)
>  		goto err;
>  
> +	if (error) {
> +		dev_err(tc->dev, "Link training phase 2 failed: %s\n",
> +			training_pattern2_errors[error]);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
>  	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
>  	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
>  


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 15/22] drm/bridge: tc358767: clean-up link training

[Tomi Valkeinen]

On 15/04/2019 12:54, Andrzej Hajda wrote:
> On 26.03.2019 11:31, Tomi Valkeinen wrote:
>> The current link training code does unnecessary retry-loops, and does
>> extra writes to the registers. It is easier to follow the flow and
>> ensure it's similar to Toshiba's documentation if we deal with LT inside
>> tc_main_link_enable() function.
>>
>> This patch adds tc_wait_link_training() which handles waiting for the LT
>> phase to finish, and does the necessary LT register setups in
>> tc_main_link_enable, without extra loops.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> 
> Btw, there is other patchset which touches the same things, have you
> decided which one should go first? yours or Andrey's?

Yes, we discussed that in "[PATCH v2 05/15] drm/bridge: tc358767: Drop
custom tc_write()/tc_read() accessors" thread. Let's merge mine first,
and Andrey rebases on top of that.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 15/22] drm/bridge: tc358767: clean-up link training

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:39PM +0200, Tomi Valkeinen wrote:
> The current link training code does unnecessary retry-loops, and does
> extra writes to the registers. It is easier to follow the flow and
> ensure it's similar to Toshiba's documentation if we deal with LT inside
> tc_main_link_enable() function.
> 
> This patch adds tc_wait_link_training() which handles waiting for the LT
> phase to finish, and does the necessary LT register setups in
> tc_main_link_enable, without extra loops.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 129 +++++++++++++-----------------
>  1 file changed, 57 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 220408db82f7..1c61f6205e43 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -740,83 +740,25 @@ static int tc_set_video_mode(struct tc_data *tc,
>  	return ret;
>  }
>  
> -static int tc_link_training(struct tc_data *tc, int pattern)
> +static int tc_wait_link_training(struct tc_data *tc, u32 *error)
>  {
> -	const char * const *errors;
> -	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
> -		      DP0_SRCCTRL_AUTOCORRECT;
> -	int timeout;
> -	int retry;
> +	u32 timeout = 1000;
>  	u32 value;
>  	int ret;
>  
> -	if (pattern == DP_TRAINING_PATTERN_1) {
> -		srcctrl |= DP0_SRCCTRL_TP1;
> -		errors = training_pattern1_errors;
> -	} else {
> -		srcctrl |= DP0_SRCCTRL_TP2;
> -		errors = training_pattern2_errors;
> -	}
> -
> -	/* Set DPCD 0x102 for Training Part 1 or 2 */
> -	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | pattern);
> -
> -	tc_write(DP0_LTLOOPCTRL,
> -		 (0x0f << 28) |	/* Defer Iteration Count */
> -		 (0x0f << 24) |	/* Loop Iteration Count */
> -		 (0x0d << 0));	/* Loop Timer Delay */
> -
> -	retry = 5;
>  	do {
> -		/* Set DP0 Training Pattern */
> -		tc_write(DP0_SRCCTRL, srcctrl);
> -
> -		/* Enable DP0 to start Link Training */
> -		tc_write(DP0CTL, DP_EN);
> -
> -		/* wait */
> -		timeout = 1000;
> -		do {
> -			tc_read(DP0_LTSTAT, &value);
> -			udelay(1);
> -		} while ((!(value & LT_LOOPDONE)) && (--timeout));
> -		if (timeout == 0) {
> -			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]);
> -			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
> -				break;
> -			if (pattern == DP_TRAINING_PATTERN_2) {
> -				value &= LT_CHANNEL1_EQ_BITS |
> -					 LT_INTERLANE_ALIGN_DONE |
> -					 LT_CHANNEL0_EQ_BITS;
> -				/* in case of two lanes */
> -				if ((tc->link.base.num_lanes == 2) &&
> -				    (value == (LT_CHANNEL1_EQ_BITS |
> -					       LT_INTERLANE_ALIGN_DONE |
> -					       LT_CHANNEL0_EQ_BITS)))
> -					break;
> -				/* in case of one line */
> -				if ((tc->link.base.num_lanes == 1) &&
> -				    (value == (LT_INTERLANE_ALIGN_DONE |
> -					       LT_CHANNEL0_EQ_BITS)))
> -					break;
> -			}
> -		}
> -		/* restart */
> -		tc_write(DP0CTL, 0);
> -		usleep_range(10, 20);
> -	} while (--retry);
> -	if (retry == 0) {
> -		dev_err(tc->dev, "Failed to finish training phase %d\n",
> -			pattern);
> +		udelay(1);
> +		tc_read(DP0_LTSTAT, &value);
> +	} while ((!(value & LT_LOOPDONE)) && (--timeout));
> +
> +	if (timeout == 0) {
> +		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
> +		ret = -ETIMEDOUT;
> +		goto err;

You can return -ETIMEDOUT directly.

>  	}
>  
> +	*error = (value >> 8) & 0x7;

How about returning the status through the return value instead ? The
status will never be negative, so it won't clash with -ETIMEDOUT.

> +
>  	return 0;
>  err:
>  	return ret;
> @@ -832,6 +774,7 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	u32 value;
>  	int ret;
>  	u8 tmp[8];
> +	u32 error;
>  
>  	/* display mode should be set at this point */
>  	if (!tc->mode)
> @@ -950,14 +893,56 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	if (ret < 0)
>  		goto err_dpcd_write;
>  
> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
> +	/* LINK TRAINING PATTERN 1 */
> +
> +	/* Set DPCD 0x102 for Training Pattern 1 */
> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
> +
> +	tc_write(DP0_LTLOOPCTRL,
> +		 (15 << 28) |	/* Defer Iteration Count */
> +		 (15 << 24) |	/* Loop Iteration Count */
> +		 (0xd << 0));	/* Loop Timer Delay */

While at it, could you define macros for these bits ?

> +
> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
> +		 DP0_SRCCTRL_TP1);

Let's break long lines (here and in other locations in this patch).

> +
> +	/* Enable DP0 to start Link Training */
> +	tc_write(DP0CTL,
> +		 ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
> +		 DP_EN);
> +
> +	/* wait */
> +	ret = tc_wait_link_training(tc, &error);
>  	if (ret)
>  		goto err;
>  
> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
> +	if (error) {
> +		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
> +			training_pattern1_errors[error]);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	/* LINK TRAINING PATTERN 2 */

Do phase 1 and phase 2 correspond to clock recovery and channel
equalization ? If so I would mention that instead of just training
pattern 1 and 2.

> +
> +	/* Set DPCD 0x102 for Training Pattern 2 */
> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
> +
> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
> +		 DP0_SRCCTRL_TP2);
> +

This channel equalization sequence of link training has a retry loop of
5 iterations. It that performed internally by the TC358767 ?

> +	/* wait */
> +	ret = tc_wait_link_training(tc, &error);
>  	if (ret)
>  		goto err;
>  
> +	if (error) {
> +		dev_err(tc->dev, "Link training phase 2 failed: %s\n",
> +			training_pattern2_errors[error]);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
>  	/* Clear Training Pattern, set AutoCorrect Mode = 1 */
>  	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_AUTOCORRECT);
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 15/22] drm/bridge: tc358767: clean-up link training

[Tomi Valkeinen]

On 21/04/2019 01:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 26, 2019 at 12:31:39PM +0200, Tomi Valkeinen wrote:
>> The current link training code does unnecessary retry-loops, and does
>> extra writes to the registers. It is easier to follow the flow and
>> ensure it's similar to Toshiba's documentation if we deal with LT inside
>> tc_main_link_enable() function.
>>
>> This patch adds tc_wait_link_training() which handles waiting for the LT
>> phase to finish, and does the necessary LT register setups in
>> tc_main_link_enable, without extra loops.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358767.c | 129 +++++++++++++-----------------
>>  1 file changed, 57 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 220408db82f7..1c61f6205e43 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -740,83 +740,25 @@ static int tc_set_video_mode(struct tc_data *tc,
>>  	return ret;
>>  }
>>  
>> -static int tc_link_training(struct tc_data *tc, int pattern)
>> +static int tc_wait_link_training(struct tc_data *tc, u32 *error)
>>  {
>> -	const char * const *errors;
>> -	u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
>> -		      DP0_SRCCTRL_AUTOCORRECT;
>> -	int timeout;
>> -	int retry;
>> +	u32 timeout = 1000;
>>  	u32 value;
>>  	int ret;
>>  
>> -	if (pattern == DP_TRAINING_PATTERN_1) {
>> -		srcctrl |= DP0_SRCCTRL_TP1;
>> -		errors = training_pattern1_errors;
>> -	} else {
>> -		srcctrl |= DP0_SRCCTRL_TP2;
>> -		errors = training_pattern2_errors;
>> -	}
>> -
>> -	/* Set DPCD 0x102 for Training Part 1 or 2 */
>> -	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | pattern);
>> -
>> -	tc_write(DP0_LTLOOPCTRL,
>> -		 (0x0f << 28) |	/* Defer Iteration Count */
>> -		 (0x0f << 24) |	/* Loop Iteration Count */
>> -		 (0x0d << 0));	/* Loop Timer Delay */
>> -
>> -	retry = 5;
>>  	do {
>> -		/* Set DP0 Training Pattern */
>> -		tc_write(DP0_SRCCTRL, srcctrl);
>> -
>> -		/* Enable DP0 to start Link Training */
>> -		tc_write(DP0CTL, DP_EN);
>> -
>> -		/* wait */
>> -		timeout = 1000;
>> -		do {
>> -			tc_read(DP0_LTSTAT, &value);
>> -			udelay(1);
>> -		} while ((!(value & LT_LOOPDONE)) && (--timeout));
>> -		if (timeout == 0) {
>> -			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]);
>> -			if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
>> -				break;
>> -			if (pattern == DP_TRAINING_PATTERN_2) {
>> -				value &= LT_CHANNEL1_EQ_BITS |
>> -					 LT_INTERLANE_ALIGN_DONE |
>> -					 LT_CHANNEL0_EQ_BITS;
>> -				/* in case of two lanes */
>> -				if ((tc->link.base.num_lanes == 2) &&
>> -				    (value == (LT_CHANNEL1_EQ_BITS |
>> -					       LT_INTERLANE_ALIGN_DONE |
>> -					       LT_CHANNEL0_EQ_BITS)))
>> -					break;
>> -				/* in case of one line */
>> -				if ((tc->link.base.num_lanes == 1) &&
>> -				    (value == (LT_INTERLANE_ALIGN_DONE |
>> -					       LT_CHANNEL0_EQ_BITS)))
>> -					break;
>> -			}
>> -		}
>> -		/* restart */
>> -		tc_write(DP0CTL, 0);
>> -		usleep_range(10, 20);
>> -	} while (--retry);
>> -	if (retry == 0) {
>> -		dev_err(tc->dev, "Failed to finish training phase %d\n",
>> -			pattern);
>> +		udelay(1);
>> +		tc_read(DP0_LTSTAT, &value);
>> +	} while ((!(value & LT_LOOPDONE)) && (--timeout));
>> +
>> +	if (timeout == 0) {
>> +		dev_err(tc->dev, "Link training timeout waiting for LT_LOOPDONE!\n");
>> +		ret = -ETIMEDOUT;
>> +		goto err;
> 
> You can return -ETIMEDOUT directly.

Yep.

>>  	}
>>  
>> +	*error = (value >> 8) & 0x7;
> 
> How about returning the status through the return value instead ? The
> status will never be negative, so it won't clash with -ETIMEDOUT.

Hmm, yes, I think that makes sense here.

>> +
>>  	return 0;
>>  err:
>>  	return ret;
>> @@ -832,6 +774,7 @@ static int tc_main_link_enable(struct tc_data *tc)
>>  	u32 value;
>>  	int ret;
>>  	u8 tmp[8];
>> +	u32 error;
>>  
>>  	/* display mode should be set at this point */
>>  	if (!tc->mode)
>> @@ -950,14 +893,56 @@ static int tc_main_link_enable(struct tc_data *tc)
>>  	if (ret < 0)
>>  		goto err_dpcd_write;
>>  
>> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
>> +	/* LINK TRAINING PATTERN 1 */
>> +
>> +	/* Set DPCD 0x102 for Training Pattern 1 */
>> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_1);
>> +
>> +	tc_write(DP0_LTLOOPCTRL,
>> +		 (15 << 28) |	/* Defer Iteration Count */
>> +		 (15 << 24) |	/* Loop Iteration Count */
>> +		 (0xd << 0));	/* Loop Timer Delay */
> 
> While at it, could you define macros for these bits ?

For the shifts? We don't have macros for almost any of the shifts. I'd
rather leave such changes for later. This one is just a copy of the
existing code.

>> +
>> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
>> +		 DP0_SRCCTRL_TP1);
> 
> Let's break long lines (here and in other locations in this patch).

Ok.

>> +
>> +	/* Enable DP0 to start Link Training */
>> +	tc_write(DP0CTL,
>> +		 ((tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) ? EF_EN : 0) |
>> +		 DP_EN);
>> +
>> +	/* wait */
>> +	ret = tc_wait_link_training(tc, &error);
>>  	if (ret)
>>  		goto err;
>>  
>> -	ret = tc_link_training(tc, DP_TRAINING_PATTERN_2);
>> +	if (error) {
>> +		dev_err(tc->dev, "Link training phase 1 failed: %s\n",
>> +			training_pattern1_errors[error]);
>> +		ret = -ENODEV;
>> +		goto err;
>> +	}
>> +
>> +	/* LINK TRAINING PATTERN 2 */
> 
> Do phase 1 and phase 2 correspond to clock recovery and channel
> equalization ? If so I would mention that instead of just training
> pattern 1 and 2.

Yes. All the docs talk about pattern 1 and 2, including DP 1.1a doc. But
I agree, probably these comments should talk about clock recovery and
channel eq.

>> +
>> +	/* Set DPCD 0x102 for Training Pattern 2 */
>> +	tc_write(DP0_SNKLTCTRL, DP_LINK_SCRAMBLING_DISABLE | DP_TRAINING_PATTERN_2);
>> +
>> +	tc_write(DP0_SRCCTRL, tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS | DP0_SRCCTRL_AUTOCORRECT |
>> +		 DP0_SRCCTRL_TP2);
>> +
> 
> This channel equalization sequence of link training has a retry loop of
> 5 iterations. It that performed internally by the TC358767 ?

Yes, that is my understanding. It's the "Loop Iteration Count" in
DP0_LTLOOPCTRL.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 16/22] drm/bridge: tc358767: remove check for video mode in link enable

[Tomi Valkeinen]

tc_main_link_enable() checks if videomode has been set, and fails if
there's no videomode. As tc_main_link_enable() no longer depends on the
videomode, we can drop the check.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 1c61f6205e43..ece330c05b9f 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -776,10 +776,6 @@ static int tc_main_link_enable(struct tc_data *tc)
 	u8 tmp[8];
 	u32 error;
 
-	/* display mode should be set at this point */
-	if (!tc->mode)
-		return -EINVAL;
-
 	dev_dbg(tc->dev, "link enable\n");
 
 	tc_write(DP0CTL, 0);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 16/22] drm/bridge: tc358767: remove check for video mode in link enable

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> tc_main_link_enable() checks if videomode has been set, and fails if
> there's no videomode. As tc_main_link_enable() no longer depends on the
> videomode, we can drop the check.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 16/22] drm/bridge: tc358767: remove check for video mode in link enable

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:40PM +0200, Tomi Valkeinen wrote:
> tc_main_link_enable() checks if videomode has been set, and fails if
> there's no videomode. As tc_main_link_enable() no longer depends on the
> videomode, we can drop the check.

Shouldn't you move the check to the stream enable function ? Or if it's
not needed there, explain why in the commit message ?

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 1c61f6205e43..ece330c05b9f 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -776,10 +776,6 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	u8 tmp[8];
>  	u32 error;
>  
> -	/* display mode should be set at this point */
> -	if (!tc->mode)
> -		return -EINVAL;
> -
>  	dev_dbg(tc->dev, "link enable\n");
>  
>  	tc_write(DP0CTL, 0);

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 16/22] drm/bridge: tc358767: remove check for video mode in link enable

[Tomi Valkeinen]

On 21/04/2019 01:14, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 26, 2019 at 12:31:40PM +0200, Tomi Valkeinen wrote:
>> tc_main_link_enable() checks if videomode has been set, and fails if
>> there's no videomode. As tc_main_link_enable() no longer depends on the
>> videomode, we can drop the check.
> 
> Shouldn't you move the check to the stream enable function ? Or if it's
> not needed there, explain why in the commit message ?

True. I believe it is not needed. I don't think bridge_enable should be
called at all, if there has not been a mode set before it. If there's no
mode, bridge enable presumably would fail for any bridge...

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 16/22] drm/bridge: tc358767: remove check for video mode in link enable

[Laurent Pinchart]

Hi Tomi,

On Fri, May 03, 2019 at 11:10:54AM +0300, Tomi Valkeinen wrote:
> On 21/04/2019 01:14, Laurent Pinchart wrote:
> > On Tue, Mar 26, 2019 at 12:31:40PM +0200, Tomi Valkeinen wrote:
> >> tc_main_link_enable() checks if videomode has been set, and fails if
> >> there's no videomode. As tc_main_link_enable() no longer depends on the
> >> videomode, we can drop the check.
> > 
> > Shouldn't you move the check to the stream enable function ? Or if it's
> > not needed there, explain why in the commit message ?
> 
> True. I believe it is not needed. I don't think bridge_enable should be
> called at all, if there has not been a mode set before it. If there's no
> mode, bridge enable presumably would fail for any bridge...

That's my understanding too. A quick check in the core could be useful,
and mentioning this in the commit message too. Ideally the documentation
of the bridge operations should be updated to make this clear :-)

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 17/22] drm/bridge: tc358767: use bridge mode_valid

[Tomi Valkeinen]

We have tc_connector_mode_valid() to filter out videomdoes that the
tc358767 cannot support. As it is a bridge limitation, change the code
to use drm_bridge_funcs's mode_valid instead.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index ece330c05b9f..9fbda370a4c2 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1140,10 +1140,10 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
 	return true;
 }
 
-static enum drm_mode_status tc_connector_mode_valid(struct drm_connector *connector,
-				   struct drm_display_mode *mode)
+static enum drm_mode_status tc_mode_valid(struct drm_bridge *bridge,
+					  const struct drm_display_mode *mode)
 {
-	struct tc_data *tc = connector_to_tc(connector);
+	struct tc_data *tc = bridge_to_tc(bridge);
 	u32 req, avail;
 	u32 bits_per_pixel = 24;
 
@@ -1212,7 +1212,6 @@ tc_connector_best_encoder(struct drm_connector *connector)
 
 static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
 	.get_modes = tc_connector_get_modes,
-	.mode_valid = tc_connector_mode_valid,
 	.best_encoder = tc_connector_best_encoder,
 };
 
@@ -1255,6 +1254,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
 
 static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.attach = tc_bridge_attach,
+	.mode_valid = tc_mode_valid,
 	.mode_set = tc_bridge_mode_set,
 	.pre_enable = tc_bridge_pre_enable,
 	.enable = tc_bridge_enable,
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 17/22] drm/bridge: tc358767: use bridge mode_valid

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> We have tc_connector_mode_valid() to filter out videomdoes that the
> tc358767 cannot support. As it is a bridge limitation, change the code
> to use drm_bridge_funcs's mode_valid instead.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 17/22] drm/bridge: tc358767: use bridge mode_valid

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:41PM +0200, Tomi Valkeinen wrote:
> We have tc_connector_mode_valid() to filter out videomdoes that the
> tc358767 cannot support. As it is a bridge limitation, change the code
> to use drm_bridge_funcs's mode_valid instead.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/tc358767.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index ece330c05b9f..9fbda370a4c2 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1140,10 +1140,10 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
>  	return true;
>  }
>  
> -static enum drm_mode_status tc_connector_mode_valid(struct drm_connector *connector,
> -				   struct drm_display_mode *mode)
> +static enum drm_mode_status tc_mode_valid(struct drm_bridge *bridge,
> +					  const struct drm_display_mode *mode)
>  {
> -	struct tc_data *tc = connector_to_tc(connector);
> +	struct tc_data *tc = bridge_to_tc(bridge);
>  	u32 req, avail;
>  	u32 bits_per_pixel = 24;
>  
> @@ -1212,7 +1212,6 @@ tc_connector_best_encoder(struct drm_connector *connector)
>  
>  static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
>  	.get_modes = tc_connector_get_modes,
> -	.mode_valid = tc_connector_mode_valid,
>  	.best_encoder = tc_connector_best_encoder,
>  };
>  
> @@ -1255,6 +1254,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
>  
>  static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.attach = tc_bridge_attach,
> +	.mode_valid = tc_mode_valid,
>  	.mode_set = tc_bridge_mode_set,
>  	.pre_enable = tc_bridge_pre_enable,
>  	.enable = tc_bridge_enable,

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 18/22] drm/bridge: tc358767: remove tc_connector_best_encoder

[Tomi Valkeinen]

As far as I know, drm_connector_helper_funcs.best_encoder is not needed
in a trivial case as we have here. So remove tc_connector_best_encoder.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 9fbda370a4c2..114d535c296b 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1202,17 +1202,8 @@ static void tc_connector_set_polling(struct tc_data *tc,
 			    DRM_CONNECTOR_POLL_DISCONNECT;
 }
 
-static struct drm_encoder *
-tc_connector_best_encoder(struct drm_connector *connector)
-{
-	struct tc_data *tc = connector_to_tc(connector);
-
-	return tc->bridge.encoder;
-}
-
 static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
 	.get_modes = tc_connector_get_modes,
-	.best_encoder = tc_connector_best_encoder,
 };
 
 static const struct drm_connector_funcs tc_connector_funcs = {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 18/22] drm/bridge: tc358767: remove tc_connector_best_encoder

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> As far as I know, drm_connector_helper_funcs.best_encoder is not needed
> in a trivial case as we have here. So remove tc_connector_best_encoder.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 18/22] drm/bridge: tc358767: remove tc_connector_best_encoder

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:42PM +0200, Tomi Valkeinen wrote:
> As far as I know, drm_connector_helper_funcs.best_encoder is not needed
> in a trivial case as we have here. So remove tc_connector_best_encoder.

I would add that the operation is only needed when the connector has
more than one encoder, which can't be the case here.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 9fbda370a4c2..114d535c296b 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1202,17 +1202,8 @@ static void tc_connector_set_polling(struct tc_data *tc,
>  			    DRM_CONNECTOR_POLL_DISCONNECT;
>  }
>  
> -static struct drm_encoder *
> -tc_connector_best_encoder(struct drm_connector *connector)
> -{
> -	struct tc_data *tc = connector_to_tc(connector);
> -
> -	return tc->bridge.encoder;
> -}
> -
>  static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
>  	.get_modes = tc_connector_get_modes,
> -	.best_encoder = tc_connector_best_encoder,
>  };
>  
>  static const struct drm_connector_funcs tc_connector_funcs = {

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 19/22] drm/bridge: tc358767: copy the mode data, instead of storing the pointer

[Tomi Valkeinen]

In tc_bridge_mode_set callback, we store the pointer to the given
drm_display_mode, and use the mode later. Storing a pointer in such a
way looks very suspicious to me, and I have observed odd issues where
the timings were apparently (at least mostly) zero.

Do a copy of the drm_display_mode instead to ensure we don't refer to
freed/modified data.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 114d535c296b..8732d5b05453 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -205,7 +205,7 @@ struct tc_data {
 	/* display edid */
 	struct edid		*edid;
 	/* current mode */
-	const struct drm_display_mode	*mode;
+	struct drm_display_mode	mode;
 
 	u32			rev;
 	u8			assr;
@@ -1021,12 +1021,12 @@ static int tc_stream_enable(struct tc_data *tc)
 	/* PXL PLL setup */
 	if (tc_test_pattern) {
 		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
-				    1000 * tc->mode->clock);
+				    1000 * tc->mode.clock);
 		if (ret)
 			goto err;
 	}
 
-	ret = tc_set_video_mode(tc, tc->mode);
+	ret = tc_set_video_mode(tc, &tc->mode);
 	if (ret)
 		goto err;
 
@@ -1166,7 +1166,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 
-	tc->mode = mode;
+	tc->mode = *mode;
 }
 
 static int tc_connector_get_modes(struct drm_connector *connector)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 19/22] drm/bridge: tc358767: copy the mode data, instead of storing the pointer

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> In tc_bridge_mode_set callback, we store the pointer to the given
> drm_display_mode, and use the mode later. Storing a pointer in such a
> way looks very suspicious to me, and I have observed odd issues where
> the timings were apparently (at least mostly) zero.
>
> Do a copy of the drm_display_mode instead to ensure we don't refer to
> freed/modified data.


Why not tc->bridge.encoder->crtc->state->adjusted_mode or

tc->bridge.encoder->crtc->state->mode ?


They should exists if the mode is set.


Regards

Andrzej


>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 114d535c296b..8732d5b05453 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -205,7 +205,7 @@ struct tc_data {
>  	/* display edid */
>  	struct edid		*edid;
>  	/* current mode */
> -	const struct drm_display_mode	*mode;
> +	struct drm_display_mode	mode;
>  
>  	u32			rev;
>  	u8			assr;
> @@ -1021,12 +1021,12 @@ static int tc_stream_enable(struct tc_data *tc)
>  	/* PXL PLL setup */
>  	if (tc_test_pattern) {
>  		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
> -				    1000 * tc->mode->clock);
> +				    1000 * tc->mode.clock);
>  		if (ret)
>  			goto err;
>  	}
>  
> -	ret = tc_set_video_mode(tc, tc->mode);
> +	ret = tc_set_video_mode(tc, &tc->mode);
>  	if (ret)
>  		goto err;
>  
> @@ -1166,7 +1166,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge,
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  
> -	tc->mode = mode;
> +	tc->mode = *mode;
>  }
>  
>  static int tc_connector_get_modes(struct drm_connector *connector)


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 19/22] drm/bridge: tc358767: copy the mode data, instead of storing the pointer

[Tomi Valkeinen]

On 15/04/2019 13:09, Andrzej Hajda wrote:
> On 26.03.2019 11:31, Tomi Valkeinen wrote:
>> In tc_bridge_mode_set callback, we store the pointer to the given
>> drm_display_mode, and use the mode later. Storing a pointer in such a
>> way looks very suspicious to me, and I have observed odd issues where
>> the timings were apparently (at least mostly) zero.
>>
>> Do a copy of the drm_display_mode instead to ensure we don't refer to
>> freed/modified data.
> 
> 
> Why not tc->bridge.encoder->crtc->state->adjusted_mode or
> 
> tc->bridge.encoder->crtc->state->mode ?
> 
> 
> They should exists if the mode is set.

Well, one reason was that my main concern was to get the DP output
working (as it was quite broken wrt. the link setup), and I wanted to
minimize all the other changes. This change felt the simplest way to fix
this issue and get forward, without possibly causing new problems.

Second, I'm a bit confused about DRM mode setting, and didn't want to
possibly move the driver into the wrong direction. It's not clear to me
whether the "mode" referred here is about the input or the output mode.
And, in both cases, if there's a bridge between the CRTC and the
TC358767, we would definitely be looking at the wrong mode if we peek at
the CRTC's.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 19/22] drm/bridge: tc358767: copy the mode data, instead of storing the pointer

[Andrzej Hajda]

On 15.04.2019 13:19, Tomi Valkeinen wrote:
> On 15/04/2019 13:09, Andrzej Hajda wrote:
>> On 26.03.2019 11:31, Tomi Valkeinen wrote:
>>> In tc_bridge_mode_set callback, we store the pointer to the given
>>> drm_display_mode, and use the mode later. Storing a pointer in such a
>>> way looks very suspicious to me, and I have observed odd issues where
>>> the timings were apparently (at least mostly) zero.
>>>
>>> Do a copy of the drm_display_mode instead to ensure we don't refer to
>>> freed/modified data.
>>
>> Why not tc->bridge.encoder->crtc->state->adjusted_mode or
>>
>> tc->bridge.encoder->crtc->state->mode ?
>>
>>
>> They should exists if the mode is set.
> Well, one reason was that my main concern was to get the DP output
> working (as it was quite broken wrt. the link setup), and I wanted to
> minimize all the other changes. This change felt the simplest way to fix
> this issue and get forward, without possibly causing new problems.
>
> Second, I'm a bit confused about DRM mode setting, and didn't want to
> possibly move the driver into the wrong direction. It's not clear to me
> whether the "mode" referred here is about the input or the output mode.
> And, in both cases, if there's a bridge between the CRTC and the
> TC358767, we would definitely be looking at the wrong mode if we peek at
> the CRTC's.


DRM does not support multiple intermediate modes, there is .mode as
requested by userspace and .adjusted_mode with twisted semantic, with
recent improvements[1].


[1]: https://www.kernel.org/doc/html/latest/gpu/drm-kms.html


Regards

Andrzej


>
>  Tomi
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 19/22] drm/bridge: tc358767: copy the mode data, instead of storing the pointer

[Laurent Pinchart]

Hi Andrzej,

On Mon, Apr 15, 2019 at 02:12:46PM +0200, Andrzej Hajda wrote:
> On 15.04.2019 13:19, Tomi Valkeinen wrote:
> > On 15/04/2019 13:09, Andrzej Hajda wrote:
> >> On 26.03.2019 11:31, Tomi Valkeinen wrote:
> >>> In tc_bridge_mode_set callback, we store the pointer to the given
> >>> drm_display_mode, and use the mode later. Storing a pointer in such a
> >>> way looks very suspicious to me, and I have observed odd issues where
> >>> the timings were apparently (at least mostly) zero.
> >>>
> >>> Do a copy of the drm_display_mode instead to ensure we don't refer to
> >>> freed/modified data.
> >>
> >> Why not tc->bridge.encoder->crtc->state->adjusted_mode or
> >>
> >> tc->bridge.encoder->crtc->state->mode ?
> >>
> >> They should exists if the mode is set.
> > 
> > Well, one reason was that my main concern was to get the DP output
> > working (as it was quite broken wrt. the link setup), and I wanted to
> > minimize all the other changes. This change felt the simplest way to fix
> > this issue and get forward, without possibly causing new problems.
> >
> > Second, I'm a bit confused about DRM mode setting, and didn't want to
> > possibly move the driver into the wrong direction. It's not clear to me
> > whether the "mode" referred here is about the input or the output mode.
> > And, in both cases, if there's a bridge between the CRTC and the
> > TC358767, we would definitely be looking at the wrong mode if we peek at
> > the CRTC's.
> 
> DRM does not support multiple intermediate modes,

*yet* :-) I don't know when we will need intermediate modes and bridge
states, but we'll get there.

I think this patch is safe, and I agree with Tomi that it minimizes the
impact on the driver.

> there is .mode as requested by userspace and .adjusted_mode with
> twisted semantic, with recent improvements[1].
> 
> [1]: https://www.kernel.org/doc/html/latest/gpu/drm-kms.html

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 20/22] drm/bridge: tc358767: add GPIO & interrupt registers

[Tomi Valkeinen]

Add GPIO and interrupt related registers for HPD work. Mark INTSTS_G and
GPIOI as volatile.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 8732d5b05453..8606de29c9b2 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -78,6 +78,12 @@
 #define DP0_VIDSRC_DSI_RX		(1 << 0)
 #define DP0_VIDSRC_DPI_RX		(2 << 0)
 #define DP0_VIDSRC_COLOR_BAR		(3 << 0)
+#define GPIOM			0x0540
+#define GPIOI			0x054c
+#define INTCTL_G		0x0560
+#define INTSTS_G		0x0564
+#define INT_GP0_LCNT		0x0584
+#define INT_GP1_LCNT		0x0588
 
 /* Control */
 #define DP0CTL			0x0600
@@ -1265,6 +1271,8 @@ static const struct regmap_range tc_volatile_ranges[] = {
 	regmap_reg_range(DP_PHY_CTRL, DP_PHY_CTRL),
 	regmap_reg_range(DP0_PLLCTRL, PXL_PLLCTRL),
 	regmap_reg_range(VFUEN0, VFUEN0),
+	regmap_reg_range(INTSTS_G, INTSTS_G),
+	regmap_reg_range(GPIOI, GPIOI),
 };
 
 static const struct regmap_access_table tc_volatile_table = {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 20/22] drm/bridge: tc358767: add GPIO & interrupt registers

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> Add GPIO and interrupt related registers for HPD work. Mark INTSTS_G and
> GPIOI as volatile.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 21/22] drm/bridge: tc358767: add IRQ and HPD support

[Tomi Valkeinen]

Add support for interrupt and hotplug handling. Both are optional.

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

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 8606de29c9b2..6978129428a8 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -71,6 +71,7 @@
 
 /* System */
 #define TC_IDREG		0x0500
+#define SYSSTAT			0x0508
 #define SYSCTRL			0x0510
 #define DP0_AUDSRC_NO_INPUT		(0 << 3)
 #define DP0_AUDSRC_I2S_RX		(1 << 3)
@@ -79,9 +80,16 @@
 #define DP0_VIDSRC_DPI_RX		(2 << 0)
 #define DP0_VIDSRC_COLOR_BAR		(3 << 0)
 #define GPIOM			0x0540
+#define GPIOC			0x0544
+#define GPIOO			0x0548
 #define GPIOI			0x054c
 #define INTCTL_G		0x0560
 #define INTSTS_G		0x0564
+
+#define INT_SYSERR		BIT(16)
+#define INT_GPIO_H(x)		(1 << (x == 0 ? 2 : 10))
+#define INT_GPIO_LC(x)		(1 << (x == 0 ? 3 : 11))
+
 #define INT_GP0_LCNT		0x0584
 #define INT_GP1_LCNT		0x0588
 
@@ -219,6 +227,12 @@ struct tc_data {
 	struct gpio_desc	*sd_gpio;
 	struct gpio_desc	*reset_gpio;
 	struct clk		*refclk;
+
+	/* do we have IRQ */
+	bool			have_irq;
+
+	/* HPD pin number (0 or 1) or -ENODEV */
+	int			hpd_num;
 };
 
 static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
@@ -1095,6 +1109,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
+	ret = tc_get_display_props(tc);
+	if (ret < 0) {
+		dev_err(tc->dev, "failed to read display props: %d\n", ret);
+		return;
+	}
+
 	ret = tc_main_link_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link enable error: %d\n", ret);
@@ -1200,19 +1220,42 @@ static int tc_connector_get_modes(struct drm_connector *connector)
 	return count;
 }
 
-static void tc_connector_set_polling(struct tc_data *tc,
-				     struct drm_connector *connector)
-{
-	/* TODO: add support for HPD */
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			    DRM_CONNECTOR_POLL_DISCONNECT;
-}
-
 static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
 	.get_modes = tc_connector_get_modes,
 };
 
+static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct tc_data *tc = connector_to_tc(connector);
+	bool conn;
+	u32 val;
+	int ret;
+
+	if (tc->hpd_num < 0) {
+		if (tc->panel)
+			return connector_status_connected;
+		else
+			return connector_status_unknown;
+	}
+
+	tc_read(GPIOI, &val);
+
+	conn = val & BIT(tc->hpd_num);
+
+	if (force && conn)
+		tc_get_display_props(tc);
+
+	if (conn)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
+
+err:
+	return connector_status_unknown;
+}
+
 static const struct drm_connector_funcs tc_connector_funcs = {
+	.detect = tc_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = drm_connector_cleanup,
 	.reset = drm_atomic_helper_connector_reset,
@@ -1227,7 +1270,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
 	struct drm_device *drm = bridge->dev;
 	int ret;
 
-	/* Create eDP connector */
+	/* Create DP/eDP connector */
 	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
 	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
 				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
@@ -1235,6 +1278,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
 	if (ret)
 		return ret;
 
+	/* Don't poll if don't have HPD connected */
+	if (tc->hpd_num >= 0) {
+		if (tc->have_irq)
+			tc->connector.polled = DRM_CONNECTOR_POLL_HPD;
+		else
+			tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
+					       DRM_CONNECTOR_POLL_DISCONNECT;
+	}
+
 	if (tc->panel)
 		drm_panel_attach(tc->panel, &tc->connector);
 
@@ -1301,6 +1353,43 @@ static const struct regmap_config tc_regmap_config = {
 	.val_format_endian = REGMAP_ENDIAN_LITTLE,
 };
 
+static irqreturn_t tc_irq_handler(int irq, void *arg)
+{
+	struct tc_data *tc = arg;
+	u32 val;
+	int r;
+
+	r = regmap_read(tc->regmap, INTSTS_G, &val);
+	if (r)
+		return IRQ_NONE;
+
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & INT_SYSERR) {
+		u32 stat = 0;
+
+		regmap_read(tc->regmap, SYSSTAT, &stat);
+
+		dev_err(tc->dev, "syserr %x\n", stat);
+	}
+
+	if (tc->hpd_num >= 0 && tc->bridge.dev) {
+		bool h = val & INT_GPIO_H(tc->hpd_num);
+		bool lc = val & INT_GPIO_LC(tc->hpd_num);
+
+		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num,
+			h ? "H" : "", lc ? "LC" : "");
+
+		if (h || lc)
+			drm_kms_helper_hotplug_event(tc->bridge.dev);
+	}
+
+	regmap_write(tc->regmap, INTSTS_G, val);
+
+	return IRQ_HANDLED;
+}
+
 static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
@@ -1352,6 +1441,31 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return ret;
 	}
 
+	ret = of_property_read_u32(dev->of_node, "hpd-num", &tc->hpd_num);
+	if (ret) {
+		tc->hpd_num = -ENODEV;
+	} else {
+		if (tc->hpd_num < 0 || tc->hpd_num > 1) {
+			dev_err(dev, "failed to parse HPD number\n");
+			return ret;
+		}
+	}
+
+	if (client->irq > 0) {
+		/* enable SysErr */
+		regmap_write(tc->regmap, INTCTL_G, INT_SYSERR);
+
+		ret = devm_request_threaded_irq(dev, client->irq,
+						NULL, tc_irq_handler, IRQF_ONESHOT,
+						"tc358767-irq", tc);
+		if (ret) {
+			dev_err(dev, "failed to register dp interrupt\n");
+			return ret;
+		}
+
+		tc->have_irq = true;
+	}
+
 	ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev);
 	if (ret) {
 		dev_err(tc->dev, "can not read device ID: %d\n", ret);
@@ -1365,6 +1479,22 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */
 
+	if (tc->hpd_num >= 0) {
+		u32 lcnt_reg = tc->hpd_num == 0 ? INT_GP0_LCNT : INT_GP1_LCNT;
+		u32 h_lc = INT_GPIO_H(tc->hpd_num) | INT_GPIO_LC(tc->hpd_num);
+
+		/* Set LCNT to 2ms */
+		regmap_write(tc->regmap, lcnt_reg,
+			     clk_get_rate(tc->refclk) * 2 / 1000);
+		/* We need the "alternate" mode for HPD */
+		regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_num));
+
+		if (tc->have_irq) {
+			/* enable H & LC */
+			regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc);
+		}
+	}
+
 	ret = tc_aux_link_setup(tc);
 	if (ret)
 		return ret;
@@ -1377,12 +1507,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (ret)
 		return ret;
 
-	ret = tc_get_display_props(tc);
-	if (ret)
-		goto err_unregister_aux;
-
-	tc_connector_set_polling(tc, &tc->connector);
-
 	tc->bridge.funcs = &tc_bridge_funcs;
 	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
@@ -1390,9 +1514,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	i2c_set_clientdata(client, tc);
 
 	return 0;
-err_unregister_aux:
-	drm_dp_aux_unregister(&tc->aux);
-	return ret;
 }
 
 static int tc_remove(struct i2c_client *client)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 21/22] drm/bridge: tc358767: add IRQ and HPD support

[Andrey Smirnov]

On Tue, Mar 26, 2019 at 3:33 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Add support for interrupt and hotplug handling. Both are optional.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 157 ++++++++++++++++++++++++++----
>  1 file changed, 139 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 8606de29c9b2..6978129428a8 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -71,6 +71,7 @@
>
>  /* System */
>  #define TC_IDREG               0x0500
> +#define SYSSTAT                        0x0508
>  #define SYSCTRL                        0x0510
>  #define DP0_AUDSRC_NO_INPUT            (0 << 3)
>  #define DP0_AUDSRC_I2S_RX              (1 << 3)
> @@ -79,9 +80,16 @@
>  #define DP0_VIDSRC_DPI_RX              (2 << 0)
>  #define DP0_VIDSRC_COLOR_BAR           (3 << 0)
>  #define GPIOM                  0x0540
> +#define GPIOC                  0x0544
> +#define GPIOO                  0x0548
>  #define GPIOI                  0x054c
>  #define INTCTL_G               0x0560
>  #define INTSTS_G               0x0564
> +
> +#define INT_SYSERR             BIT(16)
> +#define INT_GPIO_H(x)          (1 << (x == 0 ? 2 : 10))
> +#define INT_GPIO_LC(x)         (1 << (x == 0 ? 3 : 11))
> +
>  #define INT_GP0_LCNT           0x0584
>  #define INT_GP1_LCNT           0x0588
>
> @@ -219,6 +227,12 @@ struct tc_data {
>         struct gpio_desc        *sd_gpio;
>         struct gpio_desc        *reset_gpio;
>         struct clk              *refclk;
> +
> +       /* do we have IRQ */
> +       bool                    have_irq;
> +
> +       /* HPD pin number (0 or 1) or -ENODEV */
> +       int                     hpd_num;
>  };
>
>  static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
> @@ -1095,6 +1109,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>         struct tc_data *tc = bridge_to_tc(bridge);
>         int ret;
>
> +       ret = tc_get_display_props(tc);
> +       if (ret < 0) {
> +               dev_err(tc->dev, "failed to read display props: %d\n", ret);
> +               return;
> +       }
> +
>         ret = tc_main_link_enable(tc);
>         if (ret < 0) {
>                 dev_err(tc->dev, "main link enable error: %d\n", ret);
> @@ -1200,19 +1220,42 @@ static int tc_connector_get_modes(struct drm_connector *connector)
>         return count;
>  }
>
> -static void tc_connector_set_polling(struct tc_data *tc,
> -                                    struct drm_connector *connector)
> -{
> -       /* TODO: add support for HPD */
> -       connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> -                           DRM_CONNECTOR_POLL_DISCONNECT;
> -}
> -
>  static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
>         .get_modes = tc_connector_get_modes,
>  };
>
> +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force)
> +{
> +       struct tc_data *tc = connector_to_tc(connector);
> +       bool conn;
> +       u32 val;
> +       int ret;
> +
> +       if (tc->hpd_num < 0) {
> +               if (tc->panel)
> +                       return connector_status_connected;
> +               else
> +                       return connector_status_unknown;
> +       }
> +

The early return above causes tc_get_display_props() to never be
called for eDP case, which in turn result in tc_mode_valid() returning
MODE_BAD for every mode it is given since it depends on tc->link.base
being initialized properly. I had to change this code to:

if (tc->hpd_num < 0) {
    if (!tc->panel)
        return connector_status_unknown;

    conn = true;
} else {
    tc_read(GPIOI, &val);

    conn = val & BIT(tc->hpd_num);
}

to fix the problem.

Thanks,
Andrey Smirnov
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 21/22] drm/bridge: tc358767: add IRQ and HPD support

[Tomi Valkeinen]

On 02/04/2019 05:16, Andrey Smirnov wrote:

> The early return above causes tc_get_display_props() to never be
> called for eDP case, which in turn result in tc_mode_valid() returning
> MODE_BAD for every mode it is given since it depends on tc->link.base
> being initialized properly. I had to change this code to:
> 
> if (tc->hpd_num < 0) {
>     if (!tc->panel)
>         return connector_status_unknown;
> 
>     conn = true;
> } else {
>     tc_read(GPIOI, &val);
> 
>     conn = val & BIT(tc->hpd_num);
> }
> 
> to fix the problem.

Ah, right. There's tc_get_display_props() in tc_bridge_enable() but
that's of course too late (and maybe even not needed at all).

What you suggest here looks fine to me, so I'll change my patch
accordingly. Thanks!

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 21/22] drm/bridge: tc358767: add IRQ and HPD support

[Tomi Valkeinen]

Hi Andrey,

On 03/04/2019 14:34, Tomi Valkeinen wrote:
> On 02/04/2019 05:16, Andrey Smirnov wrote:
> 
>> The early return above causes tc_get_display_props() to never be
>> called for eDP case, which in turn result in tc_mode_valid() returning
>> MODE_BAD for every mode it is given since it depends on tc->link.base
>> being initialized properly. I had to change this code to:
>>
>> if (tc->hpd_num < 0) {
>>     if (!tc->panel)
>>         return connector_status_unknown;
>>
>>     conn = true;
>> } else {
>>     tc_read(GPIOI, &val);
>>
>>     conn = val & BIT(tc->hpd_num);
>> }
>>
>> to fix the problem.
> 
> Ah, right. There's tc_get_display_props() in tc_bridge_enable() but
> that's of course too late (and maybe even not needed at all).
> 
> What you suggest here looks fine to me, so I'll change my patch
> accordingly. Thanks!

With the change above, does the series work with your setup? Can I add
your tested-by? I'll send v3 series soon with the DT change suggested by
Rob and the above fix.

Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 21/22] drm/bridge: tc358767: add IRQ and HPD support

[Andrey Smirnov]

On Fri, Apr 12, 2019 at 1:02 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Hi Andrey,
>
> On 03/04/2019 14:34, Tomi Valkeinen wrote:
> > On 02/04/2019 05:16, Andrey Smirnov wrote:
> >
> >> The early return above causes tc_get_display_props() to never be
> >> called for eDP case, which in turn result in tc_mode_valid() returning
> >> MODE_BAD for every mode it is given since it depends on tc->link.base
> >> being initialized properly. I had to change this code to:
> >>
> >> if (tc->hpd_num < 0) {
> >>     if (!tc->panel)
> >>         return connector_status_unknown;
> >>
> >>     conn = true;
> >> } else {
> >>     tc_read(GPIOI, &val);
> >>
> >>     conn = val & BIT(tc->hpd_num);
> >> }
> >>
> >> to fix the problem.
> >
> > Ah, right. There's tc_get_display_props() in tc_bridge_enable() but
> > that's of course too late (and maybe even not needed at all).
> >
> > What you suggest here looks fine to me, so I'll change my patch
> > accordingly. Thanks!
>
> With the change above, does the series work with your setup? Can I add
> your tested-by? I'll send v3 series soon with the DT change suggested by
> Rob and the above fix.
>

Yeah, other than that, the series seemed to work as expected on my
setup. You can add

Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>

to v3.

Thanks,
Andrey Smirnov
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 21/22] drm/bridge: tc358767: add IRQ and HPD support

[Andrzej Hajda]

On 26.03.2019 11:31, Tomi Valkeinen wrote:
> Add support for interrupt and hotplug handling. Both are optional.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 157 ++++++++++++++++++++++++++----
>  1 file changed, 139 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 8606de29c9b2..6978129428a8 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -71,6 +71,7 @@
>  
>  /* System */
>  #define TC_IDREG		0x0500
> +#define SYSSTAT			0x0508
>  #define SYSCTRL			0x0510
>  #define DP0_AUDSRC_NO_INPUT		(0 << 3)
>  #define DP0_AUDSRC_I2S_RX		(1 << 3)
> @@ -79,9 +80,16 @@
>  #define DP0_VIDSRC_DPI_RX		(2 << 0)
>  #define DP0_VIDSRC_COLOR_BAR		(3 << 0)
>  #define GPIOM			0x0540
> +#define GPIOC			0x0544
> +#define GPIOO			0x0548
>  #define GPIOI			0x054c
>  #define INTCTL_G		0x0560
>  #define INTSTS_G		0x0564
> +
> +#define INT_SYSERR		BIT(16)
> +#define INT_GPIO_H(x)		(1 << (x == 0 ? 2 : 10))
> +#define INT_GPIO_LC(x)		(1 << (x == 0 ? 3 : 11))
> +
>  #define INT_GP0_LCNT		0x0584
>  #define INT_GP1_LCNT		0x0588
>  
> @@ -219,6 +227,12 @@ struct tc_data {
>  	struct gpio_desc	*sd_gpio;
>  	struct gpio_desc	*reset_gpio;
>  	struct clk		*refclk;
> +
> +	/* do we have IRQ */
> +	bool			have_irq;
> +
> +	/* HPD pin number (0 or 1) or -ENODEV */
> +	int			hpd_num;
>  };
>  
>  static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
> @@ -1095,6 +1109,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
>  
> +	ret = tc_get_display_props(tc);
> +	if (ret < 0) {
> +		dev_err(tc->dev, "failed to read display props: %d\n", ret);
> +		return;
> +	}
> +
>  	ret = tc_main_link_enable(tc);
>  	if (ret < 0) {
>  		dev_err(tc->dev, "main link enable error: %d\n", ret);
> @@ -1200,19 +1220,42 @@ static int tc_connector_get_modes(struct drm_connector *connector)
>  	return count;
>  }
>  
> -static void tc_connector_set_polling(struct tc_data *tc,
> -				     struct drm_connector *connector)
> -{
> -	/* TODO: add support for HPD */
> -	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> -			    DRM_CONNECTOR_POLL_DISCONNECT;
> -}
> -
>  static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
>  	.get_modes = tc_connector_get_modes,
>  };
>  
> +static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct tc_data *tc = connector_to_tc(connector);
> +	bool conn;
> +	u32 val;
> +	int ret;
> +
> +	if (tc->hpd_num < 0) {
> +		if (tc->panel)
> +			return connector_status_connected;
> +		else
> +			return connector_status_unknown;


Ok we have here 4 combinations:

1. noHPD + eDP.

2. noHPD + DP.

3. HPD + eDP.

4. HPD + DP.


Which ones do you want to support, disallow. It is not clear to me.

Moreover what connector_status_unknown should mean here for users?


> +	}
> +
> +	tc_read(GPIOI, &val);
> +
> +	conn = val & BIT(tc->hpd_num);
> +
> +	if (force && conn)
> +		tc_get_display_props(tc);
> +
> +	if (conn)
> +		return connector_status_connected;
> +	else
> +		return connector_status_disconnected;
> +
> +err:
> +	return connector_status_unknown;
> +}
> +
>  static const struct drm_connector_funcs tc_connector_funcs = {
> +	.detect = tc_connector_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.destroy = drm_connector_cleanup,
>  	.reset = drm_atomic_helper_connector_reset,
> @@ -1227,7 +1270,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
>  	struct drm_device *drm = bridge->dev;
>  	int ret;
>  
> -	/* Create eDP connector */
> +	/* Create DP/eDP connector */
>  	drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs);
>  	ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs,
>  				 tc->panel ? DRM_MODE_CONNECTOR_eDP :
> @@ -1235,6 +1278,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
>  	if (ret)
>  		return ret;
>  
> +	/* Don't poll if don't have HPD connected */
> +	if (tc->hpd_num >= 0) {
> +		if (tc->have_irq)
> +			tc->connector.polled = DRM_CONNECTOR_POLL_HPD;
> +		else
> +			tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
> +					       DRM_CONNECTOR_POLL_DISCONNECT;
> +	}
> +
>  	if (tc->panel)
>  		drm_panel_attach(tc->panel, &tc->connector);
>  
> @@ -1301,6 +1353,43 @@ static const struct regmap_config tc_regmap_config = {
>  	.val_format_endian = REGMAP_ENDIAN_LITTLE,
>  };
>  
> +static irqreturn_t tc_irq_handler(int irq, void *arg)
> +{
> +	struct tc_data *tc = arg;
> +	u32 val;
> +	int r;
> +
> +	r = regmap_read(tc->regmap, INTSTS_G, &val);
> +	if (r)
> +		return IRQ_NONE;
> +
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & INT_SYSERR) {
> +		u32 stat = 0;
> +
> +		regmap_read(tc->regmap, SYSSTAT, &stat);
> +
> +		dev_err(tc->dev, "syserr %x\n", stat);
> +	}
> +
> +	if (tc->hpd_num >= 0 && tc->bridge.dev) {
> +		bool h = val & INT_GPIO_H(tc->hpd_num);
> +		bool lc = val & INT_GPIO_LC(tc->hpd_num);
> +
> +		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num,
> +			h ? "H" : "", lc ? "LC" : "");


What does "h" and "lc" mean?


Regards

Andrzej


> +
> +		if (h || lc)
> +			drm_kms_helper_hotplug_event(tc->bridge.dev);
> +	}
> +
> +	regmap_write(tc->regmap, INTSTS_G, val);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>  	struct device *dev = &client->dev;
> @@ -1352,6 +1441,31 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		return ret;
>  	}
>  
> +	ret = of_property_read_u32(dev->of_node, "hpd-num", &tc->hpd_num);
> +	if (ret) {
> +		tc->hpd_num = -ENODEV;
> +	} else {
> +		if (tc->hpd_num < 0 || tc->hpd_num > 1) {
> +			dev_err(dev, "failed to parse HPD number\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (client->irq > 0) {
> +		/* enable SysErr */
> +		regmap_write(tc->regmap, INTCTL_G, INT_SYSERR);
> +
> +		ret = devm_request_threaded_irq(dev, client->irq,
> +						NULL, tc_irq_handler, IRQF_ONESHOT,
> +						"tc358767-irq", tc);
> +		if (ret) {
> +			dev_err(dev, "failed to register dp interrupt\n");
> +			return ret;
> +		}
> +
> +		tc->have_irq = true;
> +	}
> +
>  	ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev);
>  	if (ret) {
>  		dev_err(tc->dev, "can not read device ID: %d\n", ret);
> @@ -1365,6 +1479,22 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  
>  	tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */
>  
> +	if (tc->hpd_num >= 0) {
> +		u32 lcnt_reg = tc->hpd_num == 0 ? INT_GP0_LCNT : INT_GP1_LCNT;
> +		u32 h_lc = INT_GPIO_H(tc->hpd_num) | INT_GPIO_LC(tc->hpd_num);
> +
> +		/* Set LCNT to 2ms */
> +		regmap_write(tc->regmap, lcnt_reg,
> +			     clk_get_rate(tc->refclk) * 2 / 1000);
> +		/* We need the "alternate" mode for HPD */
> +		regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_num));
> +
> +		if (tc->have_irq) {
> +			/* enable H & LC */
> +			regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc);
> +		}
> +	}
> +
>  	ret = tc_aux_link_setup(tc);
>  	if (ret)
>  		return ret;
> @@ -1377,12 +1507,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	if (ret)
>  		return ret;
>  
> -	ret = tc_get_display_props(tc);
> -	if (ret)
> -		goto err_unregister_aux;
> -
> -	tc_connector_set_polling(tc, &tc->connector);
> -
>  	tc->bridge.funcs = &tc_bridge_funcs;
>  	tc->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&tc->bridge);
> @@ -1390,9 +1514,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	i2c_set_clientdata(client, tc);
>  
>  	return 0;
> -err_unregister_aux:
> -	drm_dp_aux_unregister(&tc->aux);
> -	return ret;
>  }
>  
>  static int tc_remove(struct i2c_client *client)


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 21/22] drm/bridge: tc358767: add IRQ and HPD support

[Tomi Valkeinen]

On 15/04/2019 13:42, Andrzej Hajda wrote:

> Ok we have here 4 combinations:
> 
> 1. noHPD + eDP.
> 
> 2. noHPD + DP.
> 
> 3. HPD + eDP.
> 
> 4. HPD + DP.
> 
> 
> Which ones do you want to support, disallow. It is not clear to me.

They all should work.

If there is HPD, we use it to return connected/disconnected.

If we don't have HPD:
- If there's a panel (i.e. eDP), we presume that it is always there, as
by definition it can't be disconnected.
- If there's no panel (i.e. DP), we can't know if the cable is connected
or not, so we return unknown. I think this could be improved by trying
to "ping" the monitor with an AUX transaction, but I didn't look at that.

> Moreover what connector_status_unknown should mean here for users?

The the driver does not know if there's a monitor or not.

>> +	if (tc->hpd_num >= 0 && tc->bridge.dev) {
>> +		bool h = val & INT_GPIO_H(tc->hpd_num);
>> +		bool lc = val & INT_GPIO_LC(tc->hpd_num);
>> +
>> +		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num,
>> +			h ? "H" : "", lc ? "LC" : "");
> 
> 
> What does "h" and "lc" mean?

These are from the func spec. H means high, LC means low-counter. H is
triggered immediately when the HPD line goes high, LC is triggered when
the line has been low for the counter-specified time (the counter is
configured at probe time).

These could be used to implement a more elaborate DP HPD & interrupt
handling, but for the time being the driver just takes them as "HPD may
have changed".

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 21/22] drm/bridge: tc358767: add IRQ and HPD support

[Andrzej Hajda]

On 15.04.2019 12:59, Tomi Valkeinen wrote:
> On 15/04/2019 13:42, Andrzej Hajda wrote:
>
>> Ok we have here 4 combinations:
>>
>> 1. noHPD + eDP.
>>
>> 2. noHPD + DP.
>>
>> 3. HPD + eDP.
>>
>> 4. HPD + DP.
>>
>>
>> Which ones do you want to support, disallow. It is not clear to me.
> They all should work.
>
> If there is HPD, we use it to return connected/disconnected.


OK, I though that eDP does not use HPD at all.


> If we don't have HPD:
> - If there's a panel (i.e. eDP), we presume that it is always there, as
> by definition it can't be disconnected.
> - If there's no panel (i.e. DP), we can't know if the cable is connected
> or not, so we return unknown. I think this could be improved by trying
> to "ping" the monitor with an AUX transaction, but I didn't look at that.
>
>> Moreover what connector_status_unknown should mean here for users?
> The the driver does not know if there's a monitor or not.

:)

More specifically, what user can do with such information.

OK, he can ensure monitor is connected and then force connected state.

But shall we expect existence of such configurations, I hoped we could
eliminate such combination (DP+noHPD) during probe.


>
>>> +	if (tc->hpd_num >= 0 && tc->bridge.dev) {
>>> +		bool h = val & INT_GPIO_H(tc->hpd_num);
>>> +		bool lc = val & INT_GPIO_LC(tc->hpd_num);
>>> +
>>> +		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num,
>>> +			h ? "H" : "", lc ? "LC" : "");
>>
>> What does "h" and "lc" mean?
> These are from the func spec. H means high, LC means low-counter. H is
> triggered immediately when the HPD line goes high, LC is triggered when
> the line has been low for the counter-specified time (the counter is
> configured at probe time).


It would be good to add this info somewhere, it is hard to guess what lc
means.


Regards

Andrzej


>
> These could be used to implement a more elaborate DP HPD & interrupt
> handling, but for the time being the driver just takes them as "HPD may
> have changed".
>
>  Tomi
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 21/22] drm/bridge: tc358767: add IRQ and HPD support

[Tomi Valkeinen]

Hi,

On 17/04/2019 10:32, Andrzej Hajda wrote:
> On 15.04.2019 12:59, Tomi Valkeinen wrote:
>> On 15/04/2019 13:42, Andrzej Hajda wrote:
>>
>>> Ok we have here 4 combinations:
>>>
>>> 1. noHPD + eDP.
>>>
>>> 2. noHPD + DP.
>>>
>>> 3. HPD + eDP.
>>>
>>> 4. HPD + DP.
>>>
>>>
>>> Which ones do you want to support, disallow. It is not clear to me.
>> They all should work.
>>
>> If there is HPD, we use it to return connected/disconnected.
> 
> 
> OK, I though that eDP does not use HPD at all.
> 
> 
>> If we don't have HPD:
>> - If there's a panel (i.e. eDP), we presume that it is always there, as
>> by definition it can't be disconnected.
>> - If there's no panel (i.e. DP), we can't know if the cable is connected
>> or not, so we return unknown. I think this could be improved by trying
>> to "ping" the monitor with an AUX transaction, but I didn't look at that.
>>
>>> Moreover what connector_status_unknown should mean here for users?
>> The the driver does not know if there's a monitor or not.
> 
> :)
> 
> More specifically, what user can do with such information.
> 
> OK, he can ensure monitor is connected and then force connected state.

Yes, something like that. I haven't really been testing that kind of
setups, but afaik, that's more about how DRM exposes and expects the
userspace to handle "unknown" connection status than what TC358767 does.

> But shall we expect existence of such configurations, I hoped we could
> eliminate such combination (DP+noHPD) during probe.

Eliminate how? Make HPD required for DP and fail if there's no HPD? I
guess that's an option.

Then again, the solutions the HW people come up with in the embedded
space never ceases to amaze me, so while I don't expect such
configurations, I would not be surprised to see one.

>>>> +	if (tc->hpd_num >= 0 && tc->bridge.dev) {
>>>> +		bool h = val & INT_GPIO_H(tc->hpd_num);
>>>> +		bool lc = val & INT_GPIO_LC(tc->hpd_num);
>>>> +
>>>> +		dev_dbg(tc->dev, "GPIO%d: %s %s\n", tc->hpd_num,
>>>> +			h ? "H" : "", lc ? "LC" : "");
>>>
>>> What does "h" and "lc" mean?
>> These are from the func spec. H means high, LC means low-counter. H is
>> triggered immediately when the HPD line goes high, LC is triggered when
>> the line has been low for the counter-specified time (the counter is
>> configured at probe time).
> 
> 
> It would be good to add this info somewhere, it is hard to guess what lc
> means.

Ok.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2 22/22] dt-bindings: tc358767: add IRQ & HPD support

[Tomi Valkeinen]

Add DT properties needed for IRQ and HPD support.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: devicetree@vger.kernel.org
Cc: Rob Herring <robh@kernel.org>
---

A quick summary of tc358767's HPD/GPIO support:

tc358767 has two pins that can be used as GPIOs. Those can be used for
HPD by configuring the pin as input. tc358767 supports HW detection of a
"short" HPD pulse (programmable length) which can be used to detect
DisplayPort IRQ from the monitor. Afaik, this kind of short pulse
detection is not possible with common GPIOs found in SoCs.

While I think the HPD support works ok, I'm unsure about the DT binding.
I first tried implementing the HPD as a gpiochip, but for interrupts I
then needed irqchip, and the amount of code started to increase a lot.

I decided that gpiochip + irqchip just isn't worth the effort and
complexity here, as most likely the pins will ever be used for HPD.

So, here I have a simple 'hpd-num' DT property to mark which (if any) of
the two GPIO pins is used for HPD.

Suggestions welcome.


 .../devicetree/bindings/display/bridge/toshiba,tc358767.txt    | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
index e3f6aa6a214d..79c71e7eb71f 100644
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
@@ -12,6 +12,9 @@ Optional properties:
                    (active high shutdown input)
  - reset-gpios: OF device-tree gpio specification for RSTX pin
                 (active low system reset)
+ - interrupt-parent: phandle of the parent interrupt controller
+ - interrupts: interrupt specifier for the bridge's interrupt
+ - hpd-num: TC358767 GPIO pin number to which HPD is connected to (0 or 1)
  - ports: the ports node can contain video interface port nodes to connect
    to a DPI/DSI source and to an eDP/DP sink according to [1][2]:
     - port@0: DSI input port
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 22/22] dt-bindings: tc358767: add IRQ & HPD support

[Rob Herring]

On Tue, Mar 26, 2019 at 12:31:46PM +0200, Tomi Valkeinen wrote:
> Add DT properties needed for IRQ and HPD support.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>
> ---
> 
> A quick summary of tc358767's HPD/GPIO support:
> 
> tc358767 has two pins that can be used as GPIOs. Those can be used for
> HPD by configuring the pin as input. tc358767 supports HW detection of a
> "short" HPD pulse (programmable length) which can be used to detect
> DisplayPort IRQ from the monitor. Afaik, this kind of short pulse
> detection is not possible with common GPIOs found in SoCs.
> 
> While I think the HPD support works ok, I'm unsure about the DT binding.
> I first tried implementing the HPD as a gpiochip, but for interrupts I
> then needed irqchip, and the amount of code started to increase a lot.
> 
> I decided that gpiochip + irqchip just isn't worth the effort and
> complexity here, as most likely the pins will ever be used for HPD.
> 
> So, here I have a simple 'hpd-num' DT property to mark which (if any) of
> the two GPIO pins is used for HPD.

There's similar properties for other chips where some signal has a 
selectable pin assignment.

> 
> Suggestions welcome.
> 
> 
>  .../devicetree/bindings/display/bridge/toshiba,tc358767.txt    | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
> index e3f6aa6a214d..79c71e7eb71f 100644
> --- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
> @@ -12,6 +12,9 @@ Optional properties:
>                     (active high shutdown input)
>   - reset-gpios: OF device-tree gpio specification for RSTX pin
>                  (active low system reset)
> + - interrupt-parent: phandle of the parent interrupt controller

Drop this, it is implied or in the parent.

> + - interrupts: interrupt specifier for the bridge's interrupt
> + - hpd-num: TC358767 GPIO pin number to which HPD is connected to (0 or 1)

ti,hpd-pin

>   - ports: the ports node can contain video interface port nodes to connect
>     to a DPI/DSI source and to an eDP/DP sink according to [1][2]:
>      - port@0: DSI input port
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2.1 22/22] dt-bindings: tc358767: add HPD support

[Tomi Valkeinen]

Add DT property for defining the pin used for HPD.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: devicetree@vger.kernel.org
Cc: Rob Herring <robh@kernel.org>
---

* Dropped the interrupt properties
* Renamed hpd-num to hpd-pin
* Added toshiba prefix for hpd-pin

 .../devicetree/bindings/display/bridge/toshiba,tc358767.txt      | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
index e3f6aa6a214d..583c5e9dbe6b 100644
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt
@@ -12,6 +12,7 @@ Optional properties:
                    (active high shutdown input)
  - reset-gpios: OF device-tree gpio specification for RSTX pin
                 (active low system reset)
+ - toshiba,hpd-pin: TC358767 GPIO pin number to which HPD is connected to (0 or 1)
  - ports: the ports node can contain video interface port nodes to connect
    to a DPI/DSI source and to an eDP/DP sink according to [1][2]:
     - port@0: DSI input port
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2.1 22/22] dt-bindings: tc358767: add HPD support

[Rob Herring]

On Mon, 1 Apr 2019 13:13:08 +0300, Tomi Valkeinen wrote:
> Add DT property for defining the pin used for HPD.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>
> ---
> 
> * Dropped the interrupt properties
> * Renamed hpd-num to hpd-pin
> * Added toshiba prefix for hpd-pin
> 
>  .../devicetree/bindings/display/bridge/toshiba,tc358767.txt      | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel