On Wed, Mar 26, 2025 at 01:34:15PM +0800, Hermes Wu via B4 Relay wrote:
> From: Hermes Wu <hermes...@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...@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
>
>
--
With best wishes
Dmitry
