>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.barysh...@oss.qualcomm.com>
>Sent: Monday, March 31, 2025 2:44 AM
>To: Hermes Wu (吳佳宏) <hermes...@ite.com.tw>
>Cc: Andrzej Hajda <andrzej.ha...@intel.com>; Neil Armstrong
><neil.armstr...@linaro.org>; Robert Foss <rf...@kernel.org>; Laurent Pinchart
><laurent.pinch...@ideasonboard.com>; Jonas Karlman <jo...@kwiboo.se>; Jernej
>Skrabec <jernej.skra...@gmail.com>; Maarten Lankhorst
><maarten.lankho...@linux.intel.com>; Maxime Ripard <mrip...@kernel.org>;
>Thomas Zimmermann <tzimmerm...@suse.de>; David Airlie <airl...@gmail.com>;
>Simona Vetter <sim...@ffwll.ch>; Pet Weng (翁玉芬) <pet.w...@ite.com.tw>; Kenneth
>Hung (洪家倫) <kenneth.h...@ite.com.tw>; treapk...@chromium.org;
>dri-devel@lists.freedesktop.org; linux-ker...@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...@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..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
