On Tue, Mar 18, 2025 at 04:32:09PM +0800, Hermes Wu wrote:
> 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.
>
> Since INT_LINK_TRAIN_FAIL is checked when auto training, it is remove
> from it6505_int_threaded_handler()
>
> In order to reset HW link auto training state,
> bits FORCE_RETRAIN and MANUAL_TRAIN at REG_TRAIN_CTRL1 must be set
> at the same time.
I've been looking at this patch for some time. I think there is too much
going on for a single commit. I'd suggest splitting out the
INT_VID_FIFO_ERROR handling and MANUAL_TRAIN changes to separate
commits. Then dropping of unused BIT_INT_LINK_TRAIN_FAIL will also go to
a separate commit that it deserves.
Basically, if at any point you feel that the commit message starts to
look like 'Perform A. Then drop B. C should be set', this is a
description for three commits, not just one.
>
> Signed-off-by: Hermes Wu <hermes...@ite.com.tw>
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 61
> +++++++++++++++++++++++++------------
> 1 file changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> b/drivers/gpu/drm/bridge/ite-it6505.c
> index
> 88ef76a37fe6accacdd343839ff2569b31b18ceb..dc1179c8338e27866f6adda4ef8fb2950336221b
> 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -821,7 +821,7 @@ static void it6505_int_mask_enable(struct it6505 *it6505)
> it6505_write(it6505, INT_MASK_02, BIT(INT_AUX_CMD_FAIL) |
> BIT(INT_HDCP_KSV_CHECK) | BIT(INT_AUDIO_FIFO_ERROR));
>
> - it6505_write(it6505, INT_MASK_03, BIT(INT_LINK_TRAIN_FAIL) |
> + it6505_write(it6505, INT_MASK_03,
> BIT(INT_VID_FIFO_ERROR) | BIT(INT_IO_LATCH_FIFO_OVERFLOW));
> }
>
> @@ -1800,30 +1800,62 @@ 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;
> + struct device *dev = it6505->dev;
> + int int03;
> + 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));
> + int03 = it6505_read(it6505, INT_STATUS_03);
>
> - mutex_lock(&it6505->aux_lock);
> it6505_set_bits(it6505, REG_TRAIN_CTRL0,
> FORCE_CR_DONE | FORCE_EQ_DONE, 0x00);
> - it6505_write(it6505, REG_TRAIN_CTRL1, FORCE_RETRAIN);
> +
> + /* reset link state machine and re start training*/
> + it6505_write(it6505, REG_TRAIN_CTRL1,
> + 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)) {
> + /* 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 (%x)!",
> + int03);
> + }
> +
> + 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;
> }
>
> @@ -2377,7 +2409,7 @@ static void it6505_stop_link_train(struct it6505
> *it6505)
> {
> it6505->link_state = LINK_IDLE;
> cancel_work_sync(&it6505->link_works);
> - it6505_write(it6505, REG_TRAIN_CTRL1, FORCE_RETRAIN);
> + it6505_write(it6505, REG_TRAIN_CTRL1, FORCE_RETRAIN | MANUAL_TRAIN);
> }
>
> static void it6505_link_train_ok(struct it6505 *it6505)
> @@ -2691,14 +2723,6 @@ static void it6505_irq_audio_fifo_error(struct it6505
> *it6505)
> it6505_enable_audio(it6505);
> }
>
> -static void it6505_irq_link_train_fail(struct it6505 *it6505)
> -{
> - struct device *dev = it6505->dev;
> -
> - DRM_DEV_DEBUG_DRIVER(dev, "link training fail interrupt");
> - schedule_work(&it6505->link_works);
> -}
> -
> static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
> {
> return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
> @@ -2763,7 +2787,6 @@ static irqreturn_t it6505_int_threaded_handler(int
> unused, void *data)
> { BIT_INT_AUX_CMD_FAIL, it6505_irq_aux_cmd_fail },
> { BIT_INT_HDCP_KSV_CHECK, it6505_irq_hdcp_ksv_check },
> { BIT_INT_AUDIO_FIFO_ERROR, it6505_irq_audio_fifo_error },
> - { BIT_INT_LINK_TRAIN_FAIL, it6505_irq_link_train_fail },
> };
> int int_status[3], i;
>
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
