Re: [PATCH v4 10/13] drm: zynqmp_dp: Use AUX IRQs instead of polling

Tomi Valkeinen
2024-04-24T12:30:56Z

On 23/04/2024 20:18, Sean Anderson wrote:
> Instead of polling the status register for the AUX status, just enable
> the IRQs and signal a completion.
>
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
>

This one seems to cause a hang when I unload the modules. I didn't debug
it further yet, but most likely we get an AUX interrupt when disabling
the hardware, and the driver hasn't disabled the IRQ handler.

Tomi

> (no changes since v3)
>
> Changes in v3:
> - New
>
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 35 +++++++++++++++++++++++---------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 9d61b6b8f2d4..863668642190 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -285,6 +285,7 @@ struct zynqmp_dp_config {
> * @next_bridge: The downstream bridge
> * @config: IP core configuration from DTS
> * @aux: aux channel
> + * @aux_done: Completed when we get an AUX reply or timeout
> * @phy: PHY handles for DP lanes
> * @num_lanes: number of enabled phy lanes
> * @hpd_work: hot plug detection worker
> @@ -305,6 +306,7 @@ struct zynqmp_dp {
> struct drm_bridge bridge;
> struct work_struct hpd_work;
> struct work_struct hpd_irq_work;
> + struct completion aux_done;
> struct mutex lock;
>
> struct drm_bridge *next_bridge;
> @@ -941,12 +943,15 @@ static int zynqmp_dp_aux_cmd_submit(struct zynqmp_dp *dp, u32 cmd, u16 addr,
> u8 *buf, u8 bytes, u8 *reply)
> {
> bool is_read = (cmd & AUX_READ_BIT) ? true : false;
> + unsigned long time_left;
> u32 reg, i;
>
> reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
> if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REQUEST)
> return -EBUSY;
>
> + reinit_completion(&dp->aux_done);
> +
> zynqmp_dp_write(dp, ZYNQMP_DP_AUX_ADDRESS, addr);
> if (!is_read)
> for (i = 0; i < bytes; i++)
> @@ -961,17 +966,14 @@ static int zynqmp_dp_aux_cmd_submit(struct zynqmp_dp *dp, u32 cmd, u16 addr,
> zynqmp_dp_write(dp, ZYNQMP_DP_AUX_COMMAND, reg);
>
> /* Wait for reply to be delivered upto 2ms */
> - for (i = 0; ; i++) {
> - reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
> - if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY)
> - break;
> + time_left = wait_for_completion_timeout(&dp->aux_done,
> + msecs_to_jiffies(2));
> + if (!time_left)
> + return -ETIMEDOUT;
>
> - if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT ||
> - i == 2)
> - return -ETIMEDOUT;
> -
> - usleep_range(1000, 1100);
> - }
> + reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
> + if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT)
> + return -ETIMEDOUT;
>
> reg = zynqmp_dp_read(dp, ZYNQMP_DP_AUX_REPLY_CODE);
> if (reply)
> @@ -1055,6 +1057,9 @@ static int zynqmp_dp_aux_init(struct zynqmp_dp *dp)
> (w << ZYNQMP_DP_AUX_CLK_DIVIDER_AUX_FILTER_SHIFT) |
> (rate / (1000 * 1000)));
>
> + zynqmp_dp_write(dp, ZYNQMP_DP_INT_EN, ZYNQMP_DP_INT_REPLY_RECEIVED |
> + ZYNQMP_DP_INT_REPLY_TIMEOUT);
> +
> dp->aux.name = "ZynqMP DP AUX";
> dp->aux.dev = dp->dev;
> dp->aux.drm_dev = dp->bridge.dev;
> @@ -1072,6 +1077,9 @@ static int zynqmp_dp_aux_init(struct zynqmp_dp *dp)
> static void zynqmp_dp_aux_cleanup(struct zynqmp_dp *dp)
> {
> drm_dp_aux_unregister(&dp->aux);
> +
> + zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_REPLY_RECEIVED |
> + ZYNQMP_DP_INT_REPLY_TIMEOUT);
> }
>
> /* -----------------------------------------------------------------------------
> @@ -1685,6 +1693,12 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
> if (status & ZYNQMP_DP_INT_HPD_IRQ)
> schedule_work(&dp->hpd_irq_work);
>
> + if (status & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY)
> + complete(&dp->aux_done);
> +
> + if (status & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT)
> + complete(&dp->aux_done);
> +
> return IRQ_HANDLED;
> }
>
> @@ -1708,6 +1722,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
> dp->dpsub = dpsub;
> dp->status = connector_status_disconnected;
> mutex_init(&dp->lock);
> + init_completion(&dp->aux_done);
>
> INIT_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
> INIT_WORK(&dp->hpd_irq_work, zynqmp_dp_hpd_irq_work_func);