Quoting Kuogee Hsieh (2021-05-12 16:29:59)
> irq_hpd interrupt should be handled after dongle plugged in and
> before dongle unplugged. Hence irq_hpd interrupt is enabled at
> the end of the plugin handle and disabled at the beginning of
> unplugged handle. Current irq_hpd with sink_count = 0 is wrongly
> handled same as the dongle unplugged which tears down the mainlink
> and disables the phy. This patch fixes this problem by only tearing
> down the mainlink but keeping phy enabled at irq_hpd with
> sink_count = 0 handle so that next irq_hpe with sink_count =1 can be
s/irq_hpe/irq/hpd/
> handled by setup mainlink only.
>
> Changes in v2:
> -- add ctrl->phy_Power_count
>
> Changes in v3:
> -- del ctrl->phy_Power_count
> -- add phy_power_off to dp_ctrl_off_link_stream()
>
> Signed-off-by: Kuogee Hsieh <khsieh@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/dp/dp_catalog.c | 5 ++--
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 54 +++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 ++
> drivers/gpu/drm/msm/dp/dp_display.c | 46 ++++++++++++++++++++++++-------
> 4 files changed, 94 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index b1a9b1b..f4f53f2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -582,10 +582,9 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>
> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>
> - /* enable HPD interrupts */
> + /* enable HPD plug and unplug interrupts */
> dp_catalog_hpd_config_intr(dp_catalog,
> - DP_DP_HPD_PLUG_INT_MASK | DP_DP_IRQ_HPD_INT_MASK
> - | DP_DP_HPD_UNPLUG_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK, true);
> + DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
>
> /* Configure REFTIMER and enable it */
> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 8d59eb9..41c122c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1811,6 +1811,60 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
> return ret;
> }
>
> +int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
> +{
> + struct dp_ctrl_private *ctrl;
> + struct dp_io *dp_io;
> + struct phy *phy;
> + int ret = 0;
Please don't assign ret and then reassign it without testing it first.
> +
> + if (!dp_ctrl)
Does this ever happen?
> + return -EINVAL;
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> + dp_io = &ctrl->parser->io;
> + phy = dp_io->phy;
> +
> + dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
> +
> + ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, false);
> + if (ret)
> + DRM_ERROR("Failed to disable pixel clocks. ret=%d\n", ret);
Why do we continue instead of returning with a failure?
> +
> + ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false);
> + if (ret)
> + DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret);
Same, why do we continue?
> +
> + phy_power_off(phy);
> + phy_exit(phy);
> +
> + /* aux channel down, reinit phy */
So we need to call exit/init to reinitialize the phy? I wonder if there
could be some phy_reinit() function that combines those.
> + phy_init(phy);
> +
> + DRM_DEBUG_DP("DP off link/stream done\n");
> + return ret;
Except here we return with a failure.
> +}
> +
> +void dp_ctrl_off_phy(struct dp_ctrl *dp_ctrl)
> +{
> + struct dp_ctrl_private *ctrl;
> + struct dp_io *dp_io;
> + struct phy *phy;
> +
> + if (!dp_ctrl)
Does this ever happen?
> + return;
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> + dp_io = &ctrl->parser->io;
> + phy = dp_io->phy;
> +
> + dp_catalog_ctrl_reset(ctrl->catalog);
> +
> + phy_exit(phy);
> +
> + DRM_DEBUG_DP("DP off phy done\n");
> +}
> +
> int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
> {
> struct dp_ctrl_private *ctrl;
