Hi,
On Thu, Apr 21, 2022 at 9:39 AM Sankeerth Billakanti (QUIC)
<quic_sbillaka@xxxxxxxxxxx> wrote:
>
> Hi Doug,
>
> >On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
> ><quic_sbillaka@xxxxxxxxxxx> wrote:
> >>
> >> The panel-edp enables the eDP panel power during probe, get_modes and
> >> pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> >> controller are directly dependent on panel power. As eDP display can
> >> be assumed as always connected, the controller driver can skip the eDP
> >> connect and disconnect interrupts. Any disruption in the link status
> >> will be indicated via the IRQ_HPD interrupts.
> >>
> >> So, the eDP controller driver can just enable the IRQ_HPD and replug
> >> interrupts. The DP controller driver still needs to enable all the
> >> interrupts.
> >>
> >> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@xxxxxxxxxxx>
> >> ---
> >> Changes in v8:
> >> - add comment explaining the interrupt status return
> >>
> >> Changes in v7:
> >> - reordered the patch in the series
> >> - modified the return statement for isr
> >> - connector check modified to just check for eDP
> >>
> >> drivers/gpu/drm/msm/dp/dp_catalog.c | 18 ++++++++++++------
> >> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
> >> 2 files changed, 33 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> index fac815f..3a298e9 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
> >> *dp_catalog)
> >>
> >> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
> >>
> >> - /* enable HPD plug and unplug interrupts */
> >> - dp_catalog_hpd_config_intr(dp_catalog,
> >> - DP_DP_HPD_PLUG_INT_MASK |
> >DP_DP_HPD_UNPLUG_INT_MASK, true);
> >> -
> >> /* Configure REFTIMER and enable it */
> >> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> >> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); @@
> >> -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct
> >> dp_catalog *dp_catalog) {
> >> struct dp_catalog_private *catalog = container_of(dp_catalog,
> >> struct dp_catalog_private, dp_catalog);
> >> - int isr = 0;
> >> + int isr, mask;
> >>
> >> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> >> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> >> (isr & DP_DP_HPD_INT_MASK));
> >> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
> >>
> >> - return isr;
> >> + /*
> >> + * REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
> >> + * irrespective of their masked status. The HW interrupt will not be
> >> + * generated if an interrupt is masked. However, the interrupt status
> >> + * bit in the register will still be set. The eDP controller driver
> >> + * masks the plug and unplug interrupts unlike DP controller which
> >> + * unmasks all the interrupts. So, do not report the status of the
> >> + * masked interrupts.
> >> + */
> >> + return isr & (mask | ~DP_DP_HPD_INT_MASK);
> >
> >What's still missing in your comments is why you aren't just doing "return isr &
> >mask;". In other words, why is the API for HPD bits different from the API for
> >non-HPD bits? What code out there wants to know about non-HPD interrupts
> >even if they are masked?
>
> The mask register bitfields are different from the INT_STATUS register.
> The INT_STATUS register has additional bits [31:28] which indicates the HPD state machine
> status and [3:0] indicates the actual generated/set interrupt. The fields [3:0] are similar to
> the mask and ack interrupts.
>
> #define DP_DP_HPD_STATE_STATUS_CONNECTED (0x40000000)
> #define DP_DP_HPD_STATE_STATUS_PENDING (0x20000000)
> #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x00000000)
> #define DP_DP_HPD_STATE_STATUS_MASK (0xE0000000)
>
> So, earlier I returned isr & (mask | DP_DP_HPD_STATE_STATUS_MASK);
>
> Actually, there is no reason to do this. We can just do the below:
> return isr & mask;
>
> >
> >Actually, thinking about this more, my preference would be this:
> >
> >a) Rename the existing function to dp_catalog_hpd_get_intr_status_raw()
> >
> >b) Create a new function called dp_catalog_hpd_get_intr_status() whose
> >implementation is:
> >
> > return dp_catalog_hpd_get_intr_status_raw() & mask;
> >
> >Then any callers who care about the raw status can be changed to call
> >dp_catalog_hpd_get_intr_status_raw(). If no callers need
> >dp_catalog_hpd_get_intr_status_raw() then you don't actually need to
> >create this function.
> >
>
> There is no caller for raw status. All interrupts are unmasked for DP.
> While handling the aux interrupts generated during the transfer call from panel probe,
> we read that the HPD interrupt status [3:0] bit0 is set and proceed to handle connect like DP.
> We experimented to find out that the connect interrupt is not generated but just the status
> bit is set.
>
> As the interrupts are generated over a single MDSS line, the controller driver has to read all the
> interrupt status registers and handle all the set interrupt bits. So, while handling aux transfer
> interrupts, we were proceeding to handle connect interrupt also as a consequence.
Ah, I see. There aren't other _interrupts_ in this register. There's
just other status info that someone jammed into this register. Got it.
So with this comment I'm happy with my Reviewed-by:
/*
* We only want to return interrupts that are unmasked to the caller.
* However, the interrupt status field also contains other
* informational bits about the HPD state status, so we only mask
* out the part of the register that tells us about which interrupts
* are pending.
*/
-Doug
