Re: [PATCH] drm/msm/dp: do not initialize phy until plugin interrupt received

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Kuogee Hsieh (2021-11-05 10:28:11)
> From: Kuogee Hsieh <khsieh@xxxxxxxxxxxxxx>
>
> Combo phy supports both USB and DP simultaneously. There may has a
> possible conflict during phy initialization phase between USB and
> DP driver which may cause USB phy timeout when USB tries to power
> up its phy. This patch has the DP driver not initialize its phy
> during DP driver initialization phase. Instead DP driver only enable
> required regulators and clocks so that it is able to receive HPD
> interrupts after completion of initialization phase. DP driver will
> initialize its phy when HPD plug-in interrupt received.
> This patch also provides a positive side effects which balance regulator
> enable count since regulator only enabled at initialize phase and resume
> and disabled at followed suspend.
>

Any Fixes: tag?

> Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c    | 97 ++++++++++++++++++++++---------------
>  drivers/gpu/drm/msm/dp/dp_ctrl.h    |  9 ++--
>  drivers/gpu/drm/msm/dp/dp_display.c | 71 ++++++++++++++++++++-------
>  3 files changed, 119 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 7ec155d..e0e5dc9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1364,7 +1364,41 @@ static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl)
>         return ret;
>  }
>
> -int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
> +void dp_ctrl_irq_enable(struct dp_ctrl *dp_ctrl, bool flip)
> +{
> +       struct dp_ctrl_private *ctrl;
> +
> +       if (!dp_ctrl) {
> +               DRM_ERROR("Invalid input data\n");
> +               return;
> +       }
> +
> +       ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> +       ctrl->dp_ctrl.orientation = flip;
> +
> +       dp_catalog_ctrl_reset(ctrl->catalog);
> +
> +       dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
> +}
> +
> +void dp_ctrl_irq_disable(struct dp_ctrl *dp_ctrl)
> +{
> +       struct dp_ctrl_private *ctrl;
> +
> +       if (!dp_ctrl) {

Please lets stop adding NULL pointer checks. They should be pushed as
high as possible in the call chain so that we don't need them down deep
in the driver.

> +               DRM_ERROR("Invalid input data\n");
> +               return;
> +       }
> +
> +       ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> +       dp_catalog_ctrl_reset(ctrl->catalog);
> +
> +       dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
> +}
> +
> +void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
>  {
>         struct dp_ctrl_private *ctrl;
>         struct dp_io *dp_io;
> @@ -1372,34 +1406,24 @@ int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
>
>         if (!dp_ctrl) {
>                 DRM_ERROR("Invalid input data\n");
> -               return -EINVAL;
> +               return;
>         }
>
>         ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>         dp_io = &ctrl->parser->io;
>         phy = dp_io->phy;
>
> -       ctrl->dp_ctrl.orientation = flip;
> -
> -       if (reset)
> -               dp_catalog_ctrl_reset(ctrl->catalog);
> +       DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> +               (u32)(uintptr_t)phy, phy->init_count, phy->power_count);

Use %p instead of casting to a u32.

>
> -       DRM_DEBUG_DP("flip=%d\n", flip);
>         dp_catalog_ctrl_phy_reset(ctrl->catalog);
>         phy_init(phy);
> -       dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
>
> -       return 0;
> +       DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> +               (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
>  }
>
> -/**
> - * dp_ctrl_host_deinit() - Uninitialize DP controller
> - * @dp_ctrl: Display Port Driver data
> - *
> - * Perform required steps to uninitialize DP controller
> - * and its resources.
> - */
> -void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
> +void dp_ctrl_phy_exit(struct dp_ctrl *dp_ctrl)
>  {
>         struct dp_ctrl_private *ctrl;
>         struct dp_io *dp_io;
> @@ -1414,10 +1438,14 @@ void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
>         dp_io = &ctrl->parser->io;
>         phy = dp_io->phy;
>
> -       dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
> +       DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> +               (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> +
> +       dp_catalog_ctrl_phy_reset(ctrl->catalog);
>         phy_exit(phy);
>
> -       DRM_DEBUG_DP("Host deinitialized successfully\n");
> +       DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> +               (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
>  }
>
>  static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
> @@ -1895,8 +1923,14 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
>                 return ret;
>         }
>
> +       DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> +               (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> +
>         phy_power_off(phy);
>
> +       DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> +               (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> +
>         /* aux channel down, reinit phy */
>         phy_exit(phy);
>         phy_init(phy);
> @@ -1905,23 +1939,6 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
>         return ret;
>  }
>
> -void dp_ctrl_off_phy(struct dp_ctrl *dp_ctrl)
> -{
> -       struct dp_ctrl_private *ctrl;
> -       struct dp_io *dp_io;
> -       struct phy *phy;
> -
> -       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;
> @@ -1949,10 +1966,14 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>                 DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret);
>         }
>
> +       DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> +               (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> +
>         phy_power_off(phy);
> -       phy_exit(phy);
>
> -       DRM_DEBUG_DP("DP off done\n");
> +       DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> +               (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> +

These debug prints make it really hard to see what's actually changing.
Can you please add them after making the fix in a different patch? So
first patch fixes the problem, and the second patch adds more logging to
track the counts for each phy pointer.

>         return ret;
>  }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 2363a2d..c1e4b1b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -19,12 +19,9 @@ struct dp_ctrl {
>         u32 pixel_rate;
>  };
>
> -int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset);
> -void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl);
>  int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>  int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
>  int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
> -void dp_ctrl_off_phy(struct dp_ctrl *dp_ctrl);
>  int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
>  void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
>  void dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
> @@ -34,4 +31,10 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
>                         struct dp_power *power, struct dp_catalog *catalog,
>                         struct dp_parser *parser);
>
> +void dp_ctrl_irq_enable(struct dp_ctrl *dp_ctrl, bool flip);

This just looks odd. Why does irq enable care about flip? Is there a
better name for this function?

> +void dp_ctrl_irq_disable(struct dp_ctrl *dp_ctrl);
> +void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl);
> +void dp_ctrl_phy_exit(struct dp_ctrl *dp_ctrl);
> +void dp_ctrl_irq_phy_exit(struct dp_ctrl *dp_ctrl);
> +
>  #endif /* _DP_CTRL_H_ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index e41dd40..17d2d5c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -84,6 +84,7 @@ struct dp_display_private {
>
>         /* state variables */
>         bool core_initialized;
> +       bool phy_initialized;
>         bool hpd_irq_on;
>         bool audio_supported;
>
> @@ -387,7 +388,7 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
>         return rc;
>  }
>
> -static void dp_display_host_init(struct dp_display_private *dp, int reset)
> +static void dp_display_host_init(struct dp_display_private *dp)
>  {
>         bool flip = false;
>
> @@ -400,12 +401,37 @@ static void dp_display_host_init(struct dp_display_private *dp, int reset)
>         if (dp->usbpd->orientation == ORIENTATION_CC2)
>                 flip = true;
>
> -       dp_power_init(dp->power, flip);
> -       dp_ctrl_host_init(dp->ctrl, flip, reset);
> +       dp_power_init(dp->power, false);
> +       dp_ctrl_irq_enable(dp->ctrl, flip);
> +
> +       if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP)

Can we get a comment here why it's safe to initialize the phy for eDP
but not for DP? And maybe a pointer to where the phy is initialized for
DP?

> +               dp_ctrl_phy_init(dp->ctrl);
> +
>         dp_aux_init(dp->aux);
>         dp->core_initialized = true;
>  }
>
> +static void dp_display_host_phy_init(struct dp_display_private *dp)
> +{
> +       DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
> +                       dp->core_initialized, dp->phy_initialized);
> +       if (dp->phy_initialized == false) {

The preferred style is

	if (!dp->phy_initialized)

> +               dp_ctrl_phy_init(dp->ctrl);
> +               dp->phy_initialized = true;
> +       }
> +}
> +
> +static void dp_display_host_phy_exit(struct dp_display_private *dp)
> +{
> +       DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
> +                       dp->core_initialized, dp->phy_initialized);
> +
> +       if (dp->phy_initialized == true) {
> +               dp_ctrl_phy_exit(dp->ctrl);
> +               dp->phy_initialized = false;
> +       }
> +}
> +
>  static void dp_display_host_deinit(struct dp_display_private *dp)
>  {
>         if (!dp->core_initialized) {
> @@ -413,7 +439,7 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
>                 return;
>         }
>
> -       dp_ctrl_host_deinit(dp->ctrl);
> +       dp_ctrl_irq_disable(dp->ctrl);
>         dp_aux_deinit(dp->aux);
>         dp_power_deinit(dp->power);
>
> @@ -424,7 +450,7 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
>  {
>         struct dp_display_private *dp = dev_get_dp_display_private(dev);
>
> -       dp_display_host_init(dp, true);
> +       dp_display_host_phy_init(dp);
>
>         return dp_display_process_hpd_high(dp);
>  }
> @@ -551,7 +577,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
>                 dp->hpd_state = ST_DISCONNECTED;
>
>                 if (ret == -ECONNRESET) { /* cable unplugged */
> -                       dp->core_initialized = false;
> +                       dp->phy_initialized = false;
>                 }
>
>         } else {
> @@ -623,9 +649,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>         if (state == ST_DISCONNECTED) {
>                 /* triggered by irq_hdp with sink_count = 0 */
>                 if (dp->link->sink_count == 0) {
> -                       dp_ctrl_off_phy(dp->ctrl);
> +                       dp_display_host_phy_exit(dp);
>                         hpd->hpd_high = 0;
> -                       dp->core_initialized = false;
>                 }
>                 mutex_unlock(&dp->event_mutex);
>                 return 0;
> @@ -716,7 +741,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
>
>         ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
>         if (ret == -ECONNRESET) { /* cable unplugged */
> -               dp->core_initialized = false;
> +               dp->phy_initialized = false;
>         }
>         DRM_DEBUG_DP("hpd_state=%d\n", state);
>
> @@ -918,12 +943,19 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
>
>         dp_display->audio_enabled = false;
>
> -       /* triggered by irq_hpd with sink_count = 0 */
>         if (dp->link->sink_count == 0) {
> +               /*
> +                * irq_hpd with sink_count = 0
> +                * hdmi unplugged out of dongle
> +                */
>                 dp_ctrl_off_link_stream(dp->ctrl);
>         } else {
> +               /*
> +                * unplugged interrupt
> +                * dongle unplugged out of DUT
> +                */
>                 dp_ctrl_off(dp->ctrl);
> -               dp->core_initialized = false;
> +               dp_display_host_phy_exit(dp);
>         }
>
>         dp_power_panel_on(dp->power, false);
> @@ -1059,7 +1091,7 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
>  static void dp_display_config_hpd(struct dp_display_private *dp)
>  {
>
> -       dp_display_host_init(dp, true);
> +       dp_display_host_init(dp);
>         dp_catalog_ctrl_hpd_config(dp->catalog);
>
>         /* Enable interrupt first time
> @@ -1338,7 +1370,7 @@ static int dp_pm_resume(struct device *dev)
>         dp->hpd_state = ST_DISCONNECTED;
>
>         /* turn on dp ctrl/phy */
> -       dp_display_host_init(dp, true);
> +       dp_display_host_init(dp);
>
>         dp_catalog_ctrl_hpd_config(dp->catalog);
>
> @@ -1346,12 +1378,15 @@ static int dp_pm_resume(struct device *dev)
>          * set sink to normal operation mode -- D0
>          * before dpcd read

Does this comment still apply?

>          */
> -       dp_link_psm_config(dp->link, &dp->panel->link_info, false);
> -
>         if (dp_catalog_link_is_connected(dp->catalog)) {
> +               dp_display_host_phy_init(dp);
> +
> +               dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>                 sink_count = drm_dp_read_sink_count(dp->aux);
>                 if (sink_count < 0)
>                         sink_count = 0;
> +
> +               dp_display_host_phy_exit(dp);
>         }
>
>         dp->link->sink_count = sink_count;



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux