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

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

Commit Message

Tomi Valkeinen March 26, 2019, 10:31 a.m.
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(-)

Comments

Andrzej Hajda April 15, 2019, 8:53 a.m. | #1
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
Laurent Pinchart April 20, 2019, 10:06 p.m. | #2
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;
Tomi Valkeinen May 3, 2019, 11 a.m. | #3
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

Patch

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;