diff mbox series

[v2,3/5] drm/bridge: it6505: modify DP link auto training

Message ID 20250326-fix-link-training-v2-3-756c8306f500@ite.com.tw (mailing list archive)
State New
Headers show
Series drm/bridge: it6505: fix DP link traning and improve compatibility | expand

Commit Message

Hermes Wu March 26, 2025, 5:34 a.m. UTC 
From: Hermes Wu <Hermes.wu@ite.com.tw>

IT6505 supports HW link training which will write DPCD and check
training status automatically.

In the case that driver set link rate at 2.7G and HW fail to training,
it will change link configuration and try 1.65G. And this will cause
INT_VID_FIFO_ERROR triggered when link clock is changed.

When video error occurs, video logic is reset and link training restart,
this will cause endless auto link training.

Modify link auto training with disable INT_VID_FIFO_ERROR to avoid loop
and check INT_LINK_TRAIN_FAIL event to abort wait training done.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 38 +++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

Comments

Dmitry Baryshkov March 30, 2025, 6:43 p.m. UTC | #1 
On Wed, Mar 26, 2025 at 01:34:15PM +0800, Hermes Wu via B4 Relay wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> IT6505 supports HW link training which will write DPCD and check
> training status automatically.
> 
> In the case that driver set link rate at 2.7G and HW fail to training,
> it will change link configuration and try 1.65G. And this will cause
> INT_VID_FIFO_ERROR triggered when link clock is changed.
> 
> When video error occurs, video logic is reset and link training restart,
> this will cause endless auto link training.
> 
> Modify link auto training with disable INT_VID_FIFO_ERROR to avoid loop
> and check INT_LINK_TRAIN_FAIL event to abort wait training done.
> 
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 38 +++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 0607f99446b37c82b41a376c2f4e10c7565d1b61..e0e13e737763fb801fd1cd803734a0d6ae1dd812 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -1800,11 +1800,20 @@ static void it6505_link_training_setup(struct it6505 *it6505)
>  
>  static bool it6505_link_start_auto_train(struct it6505 *it6505)
>  {
> -	int timeout = 500, link_training_state;
> +	int link_training_state;
>  	bool state = false;
>  	int int03;
> +	struct device *dev = it6505->dev;
> +	unsigned long timeout;
> +
> +	guard(mutex)(&it6505->aux_lock);

Please don't mix functional changes and refactoring.

> +	/* Disable FIFO error interrupt trigger  */
> +	/* to prevent training fail loop issue   */
> +	it6505_set_bits(it6505, INT_MASK_03, BIT(INT_VID_FIFO_ERROR), 0);
> +
> +	it6505_write(it6505, INT_STATUS_03,
> +		     BIT(INT_LINK_TRAIN_FAIL) | BIT(INT_VID_FIFO_ERROR));
>  
> -	mutex_lock(&it6505->aux_lock);
>  	it6505_set_bits(it6505, REG_TRAIN_CTRL0,
>  			FORCE_CR_DONE | FORCE_EQ_DONE, 0x00);
>  	/* reset link state machine and re start training*/
> @@ -1812,32 +1821,41 @@ static bool it6505_link_start_auto_train(struct it6505 *it6505)
>  		     FORCE_RETRAIN | MANUAL_TRAIN);
>  	it6505_write(it6505, REG_TRAIN_CTRL1, AUTO_TRAIN);
>  
> -	while (timeout > 0) {
> +	timeout = jiffies + msecs_to_jiffies(100) + 1;
> +	for (;;) {
>  		usleep_range(1000, 2000);
>  		link_training_state = it6505_read(it6505, REG_LINK_TRAIN_STS);
>  		int03 = it6505_read(it6505, INT_STATUS_03);
>  		if (int03 & BIT(INT_LINK_TRAIN_FAIL)) {
> -			it6505_write(it6505, INT_STATUS_03,
> -				     BIT(INT_LINK_TRAIN_FAIL));
> -
>  			DRM_DEV_DEBUG_DRIVER(dev,
>  					     "INT_LINK_TRAIN_FAIL(%x)!",
>  					      int03);
>  
> +			/* Ignore INT_VID_FIFO_ERROR when auto training fail*/
> +			it6505_write(it6505, INT_STATUS_03,
> +				     BIT(INT_LINK_TRAIN_FAIL) |
> +				     BIT(INT_VID_FIFO_ERROR));

Why can't it stay where it was?

> +
> +			if (int03 & BIT(INT_VID_FIFO_ERROR))
> +				DRM_DEV_DEBUG_DRIVER(dev,
> +						     "video fifo error when training fail");

Isn't the first message enough?

> +
>  			break;
>  		}
>  
>  		if (link_training_state > 0 &&
>  		    (link_training_state & LINK_STATE_NORP)) {
>  			state = true;
> -			goto unlock;
> +			break;
>  		}
>  
> -		timeout--;
> +		if (time_after(jiffies, timeout))
> +			break;
>  	}
> -unlock:
> -	mutex_unlock(&it6505->aux_lock);
>  
> +	/* recover interrupt trigger*/
> +	it6505_set_bits(it6505, INT_MASK_03,
> +			BIT(INT_VID_FIFO_ERROR), BIT(INT_VID_FIFO_ERROR));
>  	return state;
>  }
>  
> 
> -- 
> 2.34.1
> 
>
Hermes Wu March 31, 2025, 2:31 a.m. UTC | #2 
>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> 
>Sent: Monday, March 31, 2025 2:44 AM
>To: Hermes Wu (吳佳宏) <Hermes.Wu@ite.com.tw>
>Cc: Andrzej Hajda <andrzej.hajda@intel.com>; Neil Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Laurent Pinchart <Laurent.pinchart@ideasonboard.com>; Jonas Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Pet Weng (翁玉芬) <Pet.Weng@ite.com.tw>; Kenneth Hung (洪家倫) <Kenneth.Hung@ite.com.tw>; treapking@chromium.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH v2 3/5] drm/bridge: it6505: modify DP link auto training
>
>On Wed, Mar 26, 2025 at 01:34:15PM +0800, Hermes Wu via B4 Relay wrote:
>> From: Hermes Wu <Hermes.wu@ite.com.tw>
>> 
>> IT6505 supports HW link training which will write DPCD and check 
>> training status automatically.
>> 
>> In the case that driver set link rate at 2.7G and HW fail to training, 
>> it will change link configuration and try 1.65G. And this will cause 
>> INT_VID_FIFO_ERROR triggered when link clock is changed.
>> 
>> When video error occurs, video logic is reset and link training 
>> restart, this will cause endless auto link training.
>> 
>> Modify link auto training with disable INT_VID_FIFO_ERROR to avoid 
>> loop and check INT_LINK_TRAIN_FAIL event to abort wait training done.
>> 
>> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
>> ---
>>  drivers/gpu/drm/bridge/ite-it6505.c | 38 
>> +++++++++++++++++++++++++++----------
>>  1 file changed, 28 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c 
>> b/drivers/gpu/drm/bridge/ite-it6505.c
>> index 
>> 0607f99446b37c82b41a376c2f4e10c7565d1b61..e0e13e737763fb801fd1cd803734
>> a0d6ae1dd812 100644
>> --- a/drivers/gpu/drm/bridge/ite-it6505.c
>> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
>> @@ -1800,11 +1800,20 @@ static void it6505_link_training_setup(struct 
>> it6505 *it6505)
>>  
>>  static bool it6505_link_start_auto_train(struct it6505 *it6505)  {
>> -	int timeout = 500, link_training_state;
>> +	int link_training_state;
>>  	bool state = false;
>>  	int int03;
>> +	struct device *dev = it6505->dev;
>> +	unsigned long timeout;
>> +
>> +	guard(mutex)(&it6505->aux_lock);
>
>Please don't mix functional changes and refactoring.
>
>> +	/* Disable FIFO error interrupt trigger  */
>> +	/* to prevent training fail loop issue   */
>> +	it6505_set_bits(it6505, INT_MASK_03, BIT(INT_VID_FIFO_ERROR), 0);
>> +
>> +	it6505_write(it6505, INT_STATUS_03,
>> +		     BIT(INT_LINK_TRAIN_FAIL) | BIT(INT_VID_FIFO_ERROR));
>>  
>> -	mutex_lock(&it6505->aux_lock);
>>  	it6505_set_bits(it6505, REG_TRAIN_CTRL0,
>>  			FORCE_CR_DONE | FORCE_EQ_DONE, 0x00);
>>  	/* reset link state machine and re start training*/ @@ -1812,32 
>> +1821,41 @@ static bool it6505_link_start_auto_train(struct it6505 *it6505)
>>  		     FORCE_RETRAIN | MANUAL_TRAIN);
>>  	it6505_write(it6505, REG_TRAIN_CTRL1, AUTO_TRAIN);
>>  
>> -	while (timeout > 0) {
>> +	timeout = jiffies + msecs_to_jiffies(100) + 1;
>> +	for (;;) {
>>  		usleep_range(1000, 2000);
>>  		link_training_state = it6505_read(it6505, REG_LINK_TRAIN_STS);
>>  		int03 = it6505_read(it6505, INT_STATUS_03);
>>  		if (int03 & BIT(INT_LINK_TRAIN_FAIL)) {
>> -			it6505_write(it6505, INT_STATUS_03,
>> -				     BIT(INT_LINK_TRAIN_FAIL));
>> -
>>  			DRM_DEV_DEBUG_DRIVER(dev,
>>  					     "INT_LINK_TRAIN_FAIL(%x)!",
>>  					      int03);
>>  
>> +			/* Ignore INT_VID_FIFO_ERROR when auto training fail*/
>> +			it6505_write(it6505, INT_STATUS_03,
>> +				     BIT(INT_LINK_TRAIN_FAIL) |
>> +				     BIT(INT_VID_FIFO_ERROR));
>
>Why can't it stay where it was?

Do you mean stay at original INT_LINK_TRAIN_FAIL clear and just add INT_VID_FIFO_ERROR?


>> +
>> +			if (int03 & BIT(INT_VID_FIFO_ERROR))
>> +				DRM_DEV_DEBUG_DRIVER(dev,
>> +						     "video fifo error when training fail");
>
>Isn't the first message enough?

It just leave for checking if INT_VID_FIFO_ERROR happen,
The first message has shown int03 value, and this message will remove next patch.
>> +
>>  			break;
>>  		}
>>  
>>  		if (link_training_state > 0 &&
>>  		    (link_training_state & LINK_STATE_NORP)) {
>>  			state = true;
>> -			goto unlock;
>> +			break;
>>  		}
>>  
>> -		timeout--;
>> +		if (time_after(jiffies, timeout))
>> +			break;
>>  	}
>> -unlock:
>> -	mutex_unlock(&it6505->aux_lock);
>>  
>> +	/* recover interrupt trigger*/
>> +	it6505_set_bits(it6505, INT_MASK_03,
>> +			BIT(INT_VID_FIFO_ERROR), BIT(INT_VID_FIFO_ERROR));
>>  	return state;
>>  }
>>  
>> 
>> --
>> 2.34.1
>> 
>> 
>
>--
>With best wishes
>Dmitry
>

BR,
Hermes
Dmitry Baryshkov March 31, 2025, 7:30 a.m. UTC | #3 
On Mon, Mar 31, 2025 at 02:31:30AM +0000, Hermes.Wu@ite.com.tw wrote:
> >-----Original Message-----
> >From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> 
> >Sent: Monday, March 31, 2025 2:44 AM
> >To: Hermes Wu (吳佳宏) <Hermes.Wu@ite.com.tw>
> >Cc: Andrzej Hajda <andrzej.hajda@intel.com>; Neil Armstrong <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; Laurent Pinchart <Laurent.pinchart@ideasonboard.com>; Jonas Karlman <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Pet Weng (翁玉芬) <Pet.Weng@ite.com.tw>; Kenneth Hung (洪家倫) <Kenneth.Hung@ite.com.tw>; treapking@chromium.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org
> >Subject: Re: [PATCH v2 3/5] drm/bridge: it6505: modify DP link auto training
> >
> >On Wed, Mar 26, 2025 at 01:34:15PM +0800, Hermes Wu via B4 Relay wrote:
> >> From: Hermes Wu <Hermes.wu@ite.com.tw>
> >> 
> >> IT6505 supports HW link training which will write DPCD and check 
> >> training status automatically.
> >> 
> >> In the case that driver set link rate at 2.7G and HW fail to training, 
> >> it will change link configuration and try 1.65G. And this will cause 
> >> INT_VID_FIFO_ERROR triggered when link clock is changed.
> >> 
> >> When video error occurs, video logic is reset and link training 
> >> restart, this will cause endless auto link training.
> >> 
> >> Modify link auto training with disable INT_VID_FIFO_ERROR to avoid 
> >> loop and check INT_LINK_TRAIN_FAIL event to abort wait training done.
> >> 
> >> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> >> ---
> >>  drivers/gpu/drm/bridge/ite-it6505.c | 38 
> >> +++++++++++++++++++++++++++----------
> >>  1 file changed, 28 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c 
> >> b/drivers/gpu/drm/bridge/ite-it6505.c
> >> index 
> >> 0607f99446b37c82b41a376c2f4e10c7565d1b61..e0e13e737763fb801fd1cd803734
> >> a0d6ae1dd812 100644
> >> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> >> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> >> @@ -1800,11 +1800,20 @@ static void it6505_link_training_setup(struct 
> >> it6505 *it6505)
> >>  
> >>  static bool it6505_link_start_auto_train(struct it6505 *it6505)  {
> >> -	int timeout = 500, link_training_state;
> >> +	int link_training_state;
> >>  	bool state = false;
> >>  	int int03;
> >> +	struct device *dev = it6505->dev;
> >> +	unsigned long timeout;
> >> +
> >> +	guard(mutex)(&it6505->aux_lock);
> >
> >Please don't mix functional changes and refactoring.
> >
> >> +	/* Disable FIFO error interrupt trigger  */
> >> +	/* to prevent training fail loop issue   */
> >> +	it6505_set_bits(it6505, INT_MASK_03, BIT(INT_VID_FIFO_ERROR), 0);
> >> +
> >> +	it6505_write(it6505, INT_STATUS_03,
> >> +		     BIT(INT_LINK_TRAIN_FAIL) | BIT(INT_VID_FIFO_ERROR));
> >>  
> >> -	mutex_lock(&it6505->aux_lock);
> >>  	it6505_set_bits(it6505, REG_TRAIN_CTRL0,
> >>  			FORCE_CR_DONE | FORCE_EQ_DONE, 0x00);
> >>  	/* reset link state machine and re start training*/ @@ -1812,32 
> >> +1821,41 @@ static bool it6505_link_start_auto_train(struct it6505 *it6505)
> >>  		     FORCE_RETRAIN | MANUAL_TRAIN);
> >>  	it6505_write(it6505, REG_TRAIN_CTRL1, AUTO_TRAIN);
> >>  
> >> -	while (timeout > 0) {
> >> +	timeout = jiffies + msecs_to_jiffies(100) + 1;
> >> +	for (;;) {
> >>  		usleep_range(1000, 2000);
> >>  		link_training_state = it6505_read(it6505, REG_LINK_TRAIN_STS);
> >>  		int03 = it6505_read(it6505, INT_STATUS_03);
> >>  		if (int03 & BIT(INT_LINK_TRAIN_FAIL)) {
> >> -			it6505_write(it6505, INT_STATUS_03,
> >> -				     BIT(INT_LINK_TRAIN_FAIL));
> >> -
> >>  			DRM_DEV_DEBUG_DRIVER(dev,
> >>  					     "INT_LINK_TRAIN_FAIL(%x)!",
> >>  					      int03);
> >>  
> >> +			/* Ignore INT_VID_FIFO_ERROR when auto training fail*/
> >> +			it6505_write(it6505, INT_STATUS_03,
> >> +				     BIT(INT_LINK_TRAIN_FAIL) |
> >> +				     BIT(INT_VID_FIFO_ERROR));
> >
> >Why can't it stay where it was?
> 
> Do you mean stay at original INT_LINK_TRAIN_FAIL clear and just add INT_VID_FIFO_ERROR?

Yes.

> 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 0607f99446b37c82b41a376c2f4e10c7565d1b61..e0e13e737763fb801fd1cd803734a0d6ae1dd812 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -1800,11 +1800,20 @@  static void it6505_link_training_setup(struct it6505 *it6505)
 
 static bool it6505_link_start_auto_train(struct it6505 *it6505)
 {
-	int timeout = 500, link_training_state;
+	int link_training_state;
 	bool state = false;
 	int int03;
+	struct device *dev = it6505->dev;
+	unsigned long timeout;
+
+	guard(mutex)(&it6505->aux_lock);
+	/* Disable FIFO error interrupt trigger  */
+	/* to prevent training fail loop issue   */
+	it6505_set_bits(it6505, INT_MASK_03, BIT(INT_VID_FIFO_ERROR), 0);
+
+	it6505_write(it6505, INT_STATUS_03,
+		     BIT(INT_LINK_TRAIN_FAIL) | BIT(INT_VID_FIFO_ERROR));
 
-	mutex_lock(&it6505->aux_lock);
 	it6505_set_bits(it6505, REG_TRAIN_CTRL0,
 			FORCE_CR_DONE | FORCE_EQ_DONE, 0x00);
 	/* reset link state machine and re start training*/
@@ -1812,32 +1821,41 @@  static bool it6505_link_start_auto_train(struct it6505 *it6505)
 		     FORCE_RETRAIN | MANUAL_TRAIN);
 	it6505_write(it6505, REG_TRAIN_CTRL1, AUTO_TRAIN);
 
-	while (timeout > 0) {
+	timeout = jiffies + msecs_to_jiffies(100) + 1;
+	for (;;) {
 		usleep_range(1000, 2000);
 		link_training_state = it6505_read(it6505, REG_LINK_TRAIN_STS);
 		int03 = it6505_read(it6505, INT_STATUS_03);
 		if (int03 & BIT(INT_LINK_TRAIN_FAIL)) {
-			it6505_write(it6505, INT_STATUS_03,
-				     BIT(INT_LINK_TRAIN_FAIL));
-
 			DRM_DEV_DEBUG_DRIVER(dev,
 					     "INT_LINK_TRAIN_FAIL(%x)!",
 					      int03);
 
+			/* Ignore INT_VID_FIFO_ERROR when auto training fail*/
+			it6505_write(it6505, INT_STATUS_03,
+				     BIT(INT_LINK_TRAIN_FAIL) |
+				     BIT(INT_VID_FIFO_ERROR));
+
+			if (int03 & BIT(INT_VID_FIFO_ERROR))
+				DRM_DEV_DEBUG_DRIVER(dev,
+						     "video fifo error when training fail");
+
 			break;
 		}
 
 		if (link_training_state > 0 &&
 		    (link_training_state & LINK_STATE_NORP)) {
 			state = true;
-			goto unlock;
+			break;
 		}
 
-		timeout--;
+		if (time_after(jiffies, timeout))
+			break;
 	}
-unlock:
-	mutex_unlock(&it6505->aux_lock);
 
+	/* recover interrupt trigger*/
+	it6505_set_bits(it6505, INT_MASK_03,
+			BIT(INT_VID_FIFO_ERROR), BIT(INT_VID_FIFO_ERROR));
 	return state;
 }