drm/bridge: it6505: fix hibernate to resume no display issue
Commit Message
From: kuro chung <kuro.chung@ite.com.tw>
ITE added a FIFO reset bit for input video. When system power resume,
the TTL input of it6505 may get some noise before video signal stable
and the hardware function reset is required.
But the input FIFO reset will also trigger error interrupts of output module rising.
Thus, it6505 have to wait a period can clear those expected error interrupts
caused by manual hardware reset in one interrupt handler calling to avoid interrupt looping.
Signed-off-by: Allen Chen <allen.chen@ite.corp-partner.google.com>
BUG=None
TEST=None
---
drivers/gpu/drm/bridge/ite-it6505.c | 53 ++++++++++++++++++++++++-----
1 file changed, 44 insertions(+), 9 deletions(-)
Comments
Hi Kuro,
On Wed, Feb 21, 2024 at 3:53 PM kuro <kuro.chung@ite.com.tw> wrote:
>
> From: kuro chung <kuro.chung@ite.com.tw>
>
> ITE added a FIFO reset bit for input video. When system power resume,
> the TTL input of it6505 may get some noise before video signal stable
> and the hardware function reset is required.
> But the input FIFO reset will also trigger error interrupts of output module rising.
> Thus, it6505 have to wait a period can clear those expected error interrupts
> caused by manual hardware reset in one interrupt handler calling to avoid interrupt looping.
>
> Signed-off-by: Allen Chen <allen.chen@ite.corp-partner.google.com>
IIUC you should also sign this off with your own account, and don't
include Allen if he is not involved in the patch development.corp
account here
>
> BUG=None
> TEST=None
Please remove these two lines for upstream review.
>
> ---
> drivers/gpu/drm/bridge/ite-it6505.c | 53 ++++++++++++++++++++++++-----
> 1 file changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index b53da9bb65a16..86277968fab93 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -1318,6 +1318,8 @@ static void it6505_video_reset(struct it6505 *it6505)
> it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_VID_MUTE, EN_VID_MUTE);
> it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_VID_CTRL_PKT, 0x00);
> it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, VIDEO_RESET);
> + it6505_set_bits(it6505, REG_VID_BUS_CTRL1, TX_FIFO_RESET, 0x02);
> + it6505_set_bits(it6505, REG_VID_BUS_CTRL1, TX_FIFO_RESET, 0x00);
> it6505_set_bits(it6505, REG_501_FIFO_CTRL, RST_501_FIFO, RST_501_FIFO);
> it6505_set_bits(it6505, REG_501_FIFO_CTRL, RST_501_FIFO, 0x00);
> it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, 0x00);
> @@ -2480,10 +2482,6 @@ static void it6505_irq_video_fifo_error(struct it6505 *it6505)
> struct device *dev = &it6505->client->dev;
>
> DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(&it6505->link_works);
> - it6505_stop_hdcp(it6505);
> - it6505_video_reset(it6505);
> }
>
> static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
> @@ -2491,10 +2489,6 @@ static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
> struct device *dev = &it6505->client->dev;
>
> DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
> - it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> - flush_work(&it6505->link_works);
> - it6505_stop_hdcp(it6505);
> - it6505_video_reset(it6505);
> }
Do we need to keep these two functions if they do nothing other than logging?
>
> static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
> @@ -2502,6 +2496,45 @@ static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
> return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
> }
>
> +static bool it6505_is_video_error_int(const int *int_status)
> +{
> + if ((it6505_test_bit(BIT_INT_VID_FIFO_ERROR, (unsigned int *)int_status)) || (it6505_test_bit(BIT_INT_IO_FIFO_OVERFLOW, (unsigned int *)int_status)))
> + return 1;
> + return 0;
> +}
Maybe just:
return it6505_test_bit(BIT_INT_VID_FIFO_ERROR, (unsigned int
*)int_status) || it6505_test_bit(BIT_INT_IO_FIFO_OVERFLOW, (unsigned
int *)int_status)
> +
> +static void it6505_irq_video_error_handler(struct it6505 *it6505)
> +{
> + struct device *dev = &it6505->client->dev;
> + int int_status[3] = {0};
> + int reg_0d;
> +
> + it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> + flush_work(&it6505->link_works);
> + it6505_stop_hdcp(it6505);
> + it6505_video_reset(it6505);
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video...");
> +
Can you add some code comments here to explain why we need to clear
the interrupt bits here?
> + for (i = 0; i < 10; i++) {
> + usleep_range(10000, 11000);
> + int_status[2] = it6505_read(it6505, INT_STATUS_03);
> + reg_0d = it6505_read(it6505, REG_SYSTEM_STS);
> + it6505_write(it6505, INT_STATUS_03, int_status[2]);
If we clear all interrupts like this, won't we risk missing other
interrupts here? E.g., if an HPD interrupt is fired here, it will be
cleared without being handled.
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "reg08 = 0x%02x", int_status[2]);
> + DRM_DEV_DEBUG_DRIVER(dev, "reg0D = 0x%02x", reg_0d);
> +
> + if ((reg_0d & VIDEO_STB) && (reg_0d >= 0))
> + break;
> +
> + if (it6505_is_video_error_int(int_status)) {
> + it6505_video_reset(it6505);
> + DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video (%d)", i);
> + }
> + }
> +}
Why do we need a for-loop here, and why 10?
> +
> static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> {
> struct it6505 *it6505 = data;
> @@ -2522,7 +2555,7 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> { BIT_INT_VID_FIFO_ERROR, it6505_irq_video_fifo_error },
> { BIT_INT_IO_FIFO_OVERFLOW, it6505_irq_io_latch_fifo_overflow },
> };
> - int int_status[3], i;
> + int int_status[3], i, reg_0d;
>
> if (it6505->enable_drv_hold || !it6505->powered)
> return IRQ_HANDLED;
> @@ -2550,6 +2583,8 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
> if (it6505_test_bit(irq_vec[i].bit, (unsigned int *)int_status))
> irq_vec[i].handler(it6505);
> }
> + if (it6505_is_video_error_int(int_status))
> + it6505_irq_video_error_handler(it6505);
> }
>
> pm_runtime_put_sync(dev);
> --
> 2.25.1
>
Regards,
Pin-yen
@@ -1318,6 +1318,8 @@ static void it6505_video_reset(struct it6505 *it6505)
it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_VID_MUTE, EN_VID_MUTE);
it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_VID_CTRL_PKT, 0x00);
it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, VIDEO_RESET);
+ it6505_set_bits(it6505, REG_VID_BUS_CTRL1, TX_FIFO_RESET, 0x02);
+ it6505_set_bits(it6505, REG_VID_BUS_CTRL1, TX_FIFO_RESET, 0x00);
it6505_set_bits(it6505, REG_501_FIFO_CTRL, RST_501_FIFO, RST_501_FIFO);
it6505_set_bits(it6505, REG_501_FIFO_CTRL, RST_501_FIFO, 0x00);
it6505_set_bits(it6505, REG_RESET_CTRL, VIDEO_RESET, 0x00);
@@ -2480,10 +2482,6 @@ static void it6505_irq_video_fifo_error(struct it6505 *it6505)
struct device *dev = &it6505->client->dev;
DRM_DEV_DEBUG_DRIVER(dev, "video fifo overflow interrupt");
- it6505->auto_train_retry = AUTO_TRAIN_RETRY;
- flush_work(&it6505->link_works);
- it6505_stop_hdcp(it6505);
- it6505_video_reset(it6505);
}
static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
@@ -2491,10 +2489,6 @@ static void it6505_irq_io_latch_fifo_overflow(struct it6505 *it6505)
struct device *dev = &it6505->client->dev;
DRM_DEV_DEBUG_DRIVER(dev, "IO latch fifo overflow interrupt");
- it6505->auto_train_retry = AUTO_TRAIN_RETRY;
- flush_work(&it6505->link_works);
- it6505_stop_hdcp(it6505);
- it6505_video_reset(it6505);
}
static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
@@ -2502,6 +2496,45 @@ static bool it6505_test_bit(unsigned int bit, const unsigned int *addr)
return 1 & (addr[bit / BITS_PER_BYTE] >> (bit % BITS_PER_BYTE));
}
+static bool it6505_is_video_error_int(const int *int_status)
+{
+ if ((it6505_test_bit(BIT_INT_VID_FIFO_ERROR, (unsigned int *)int_status)) || (it6505_test_bit(BIT_INT_IO_FIFO_OVERFLOW, (unsigned int *)int_status)))
+ return 1;
+ return 0;
+}
+
+static void it6505_irq_video_error_handler(struct it6505 *it6505)
+{
+ struct device *dev = &it6505->client->dev;
+ int int_status[3] = {0};
+ int reg_0d;
+
+ it6505->auto_train_retry = AUTO_TRAIN_RETRY;
+ flush_work(&it6505->link_works);
+ it6505_stop_hdcp(it6505);
+ it6505_video_reset(it6505);
+
+ DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video...");
+
+ for (i = 0; i < 10; i++) {
+ usleep_range(10000, 11000);
+ int_status[2] = it6505_read(it6505, INT_STATUS_03);
+ reg_0d = it6505_read(it6505, REG_SYSTEM_STS);
+ it6505_write(it6505, INT_STATUS_03, int_status[2]);
+
+ DRM_DEV_DEBUG_DRIVER(dev, "reg08 = 0x%02x", int_status[2]);
+ DRM_DEV_DEBUG_DRIVER(dev, "reg0D = 0x%02x", reg_0d);
+
+ if ((reg_0d & VIDEO_STB) && (reg_0d >= 0))
+ break;
+
+ if (it6505_is_video_error_int(int_status)) {
+ it6505_video_reset(it6505);
+ DRM_DEV_DEBUG_DRIVER(dev, "Video Error reset wait video (%d)", i);
+ }
+ }
+}
+
static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
{
struct it6505 *it6505 = data;
@@ -2522,7 +2555,7 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
{ BIT_INT_VID_FIFO_ERROR, it6505_irq_video_fifo_error },
{ BIT_INT_IO_FIFO_OVERFLOW, it6505_irq_io_latch_fifo_overflow },
};
- int int_status[3], i;
+ int int_status[3], i, reg_0d;
if (it6505->enable_drv_hold || !it6505->powered)
return IRQ_HANDLED;
@@ -2550,6 +2583,8 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
if (it6505_test_bit(irq_vec[i].bit, (unsigned int *)int_status))
irq_vec[i].handler(it6505);
}
+ if (it6505_is_video_error_int(int_status))
+ it6505_irq_video_error_handler(it6505);
}
pm_runtime_put_sync(dev);