Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler

[khsieh]

On 2021-01-13 12:23, Stephen Boyd wrote:
> Quoting khsieh@xxxxxxxxxxxxxx (2021-01-13 09:48:25)
> > On 2021-01-11 11:54, Stephen Boyd wrote:
> > > Quoting Kuogee Hsieh (2021-01-07 12:30:25)
> > >> There is HPD unplug interrupts missed at scenario of an irq_hpd
> > >> followed by unplug interrupts with around 10 ms in between.
> > >> Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
> > >> irq_hpd handler should not issues either aux or sw reset to avoid
> > >> following unplug interrupt be cleared accidentally.
> > >
> > > So the problem is that we're resetting the DP aux phy in the middle of
> > > the HPD state machine transitioning states?
> > >
> > yes, after reset aux, hw clear pending hpd interrupts
> > >>
> > >> Signed-off-by: Kuogee Hsieh <khsieh@xxxxxxxxxxxxxx>
> > >> ---
> > >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >> index 44f0c57..9c0ce98 100644
> > >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >> @@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct
> > >> dp_catalog *dp_catalog)
> > >>         return 0;
> > >>  }
> > >>
> > >> +/**
> > >> + * dp_catalog_aux_reset() - reset AUX controller
> > >> + *
> > >> + * @aux: DP catalog structure
> > >> + *
> > >> + * return: void
> > >> + *
> > >> + * This function reset AUX controller
> > >> + *
> > >> + * NOTE: reset AUX controller will also clear any pending HPD related
> > >> interrupts
> > >> + *
> > >> + */
> > >>  void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
> > >>  {
> > >>         u32 aux_ctrl;
> > >> @@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog
> > >> *dp_catalog,
> > >>         return 0;
> > >>  }
> > >>
> > >> +/**
> > >> + * dp_catalog_ctrl_reset() - reset DP controller
> > >> + *
> > >> + * @aux: DP catalog structure
> > >
> > > It's called dp_catalog though.
> > registers access are through dp_catalog_xxxx
>
> Agreed. The variable is not called 'aux' though, it's called
> 'dp_catalog'.
>
> > >
> > >> + *
> > >> + * return: void
> > >> + *
> > >> + * This function reset DP controller
> > >
> > > resets the
> > >
> > >> + *
> > >> + * NOTE: reset DP controller will also clear any pending HPD related
> > >> interrupts
> > >> + *
> > >> + */
> > >>  void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
>
> Right here.

fixed at v2
> > >>  {
> > >>         u32 sw_reset;
> > >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > >> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > >> index e3462f5..f96c415 100644
> > >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > >> @@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct
> > >> dp_ctrl_private *ctrl,
> > >>          * transitioned to PUSH_IDLE. In order to start transmitting
> > >>          * a link training pattern, we have to first do soft reset.
> > >>          */
> > >> -       dp_catalog_ctrl_reset(ctrl->catalog);
> > >> +       if (*training_step != DP_TRAINING_NONE)
> > >
> > > Can we check for the positive value instead? i.e.
> > > DP_TRAINING_1/DP_TRAINING_2
> > >
>
> Any answer?

fixed at v2