[PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable

[Swapnil Kashinath Jakhade]

> -----Original Message-----
> From: Nikhil Devshatwar <nikhil.nd at ti.com>
> Sent: Tuesday, November 10, 2020 7:23 PM
> To: Tomi Valkeinen <tomi.valkeinen at ti.com>; Swapnil Kashinath Jakhade
> <sjakhade at cadence.com>; Yuti Suresh Amonkar <yamonkar at cadence.com>
> Cc: dri-devel at lists.freedesktop.org; Swapnil Kashinath Jakhade
> <sjakhade at cadence.com>; Sekhar Nori <nsekhar at ti.com>; Laurent Pinchart
> <laurent.pinchart at ideasonboard.com>; Yuti Suresh Amonkar
> <yamonkar at cadence.com>
> Subject: Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt
> enable/disable
> 
> EXTERNAL MAIL
> 
> 
> On 14:27-20201110, Tomi Valkeinen wrote:
> > On 10/11/2020 12:27, Nikhil Devshatwar wrote:
> > > On 11:21-20201110, Tomi Valkeinen wrote:
> > >> On 09/11/2020 19:06, Nikhil Devshatwar wrote:
> > >>> When removing the tidss driver, there is a warning reported by
> > >>> kernel about an unhandled interrupt for mhdp driver.
> > >>>
> > >>> [   43.238895] irq 31: nobody cared (try booting with the "irqpoll"
> option)
> > >>> ... [snipped backtrace]
> > >>> [   43.330735] handlers:
> > >>> [   43.333020] [<000000005367c4f9>] irq_default_primary_handler
> threaded [<000000007e02b601>]
> > >>> cdns_mhdp_irq_handler [cdns_mhdp8546]
> > >>> [   43.344607] Disabling IRQ #31
> > >>>
> > >>> This happens because as part of cdns_mhdp_bridge_hpd_disable,
> > >>> driver tries to disable the interrupts. While disabling the
> > >>> SW_EVENT interrupts, it accidentally enables the MBOX interrupts,
> > >>> which are not handled by the driver.
> > >>>
> > >>> Fix this with a read-modify-write to update only required bits.
> > >>> Do the same for enabling interrupts as well.
> > >>>
> > >>> Signed-off-by: Nikhil Devshatwar <nikhil.nd at ti.com>
> > >>> ---
> > >>>  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 7 +++++--
> > >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > >>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > >>> index 2cd809eed827..6beccd2a408e 100644
> > >>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > >>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > >>> @@ -2146,7 +2146,8 @@ static void
> > >>> cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
> > >>>
> > >>>  	/* Enable SW event interrupts */
> > >>>  	if (mhdp->bridge_attached)
> > >>> -		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
> > >>> +		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
> > >>> +		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
> > >>>  		       mhdp->regs + CDNS_APB_INT_MASK);  }
> > >>>
> > >>> @@ -2154,7 +2155,9 @@ static void
> > >>> cdns_mhdp_bridge_hpd_disable(struct drm_bridge *bridge)  {
> > >>>  	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> > >>>
> > >>> -	writel(CDNS_APB_INT_MASK_SW_EVENT_INT, mhdp->regs +
> CDNS_APB_INT_MASK);
> > >>> +	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
> > >>> +	       CDNS_APB_INT_MASK_SW_EVENT_INT,
> > >>> +	       mhdp->regs + CDNS_APB_INT_MASK);
> > >>>  }
> > >>>
> > >>>  static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
> > >>
> > >> Good catch. I wonder why we need the above functions... We already
> > >> enable and disable the interrupts when attaching/detaching the
> > >> driver. And I think we want to get the interrupt even if we won't report
> HPD (but I think we always do report it), as we need the interrupts to track
> the link status.
> > >>
> > >
> > > I read from the code that there is TODO for handling the mailbox
> > > interrupts in the driver. Once that is supported, you will be able
> > > to explictily enable/disable interrupts for SW_EVENTS (like hotplug)
> > > as well as mailbox events. This enabling specific bits in the
> > > interrupt status.
> >
> > But SW_EVENTS is not the same as HPD, at least in theory. If we
> > disable SW_EVENT_INT in hpd_disable(), we lose all SW_EVENT interrupts.
> 
> I am not sure, what exactly is covered in the SW events apart from the
> hotplug.
> 
> Swapnil, Yuti, Please fill in..

hpd_enable/hpd_disable callbacks were implemented as a part of supporting
DRM_BRIDGE_OP_HPD bridge operation. The existing implementation could
work with current features set supported by MHDP driver. But Tomi's point is
valid, as there are some HDCP interrupts which are part of SW_EVENT interrupts
and this might not be the control to just enable/disable HPD.

Swapnil

> 
> Nikhil D
> >
> >  Tomi
> >
> > --
> > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki