| Message ID | 20201109170601.21557-7-nikhil.nd@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tidss: Use new connector model for tidss | expand |
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@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. Tomi
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@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. Nikhil D > Tomi > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
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@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. Tomi
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@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.. Nikhil D > > Tomi > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> -----Original Message----- > From: Nikhil Devshatwar <nikhil.nd@ti.com> > Sent: Tuesday, November 10, 2020 7:23 PM > To: Tomi Valkeinen <tomi.valkeinen@ti.com>; Swapnil Kashinath Jakhade > <sjakhade@cadence.com>; Yuti Suresh Amonkar <yamonkar@cadence.com> > Cc: dri-devel@lists.freedesktop.org; Swapnil Kashinath Jakhade > <sjakhade@cadence.com>; Sekhar Nori <nsekhar@ti.com>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Yuti Suresh Amonkar > <yamonkar@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@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
On 14:32-20201110, Swapnil Jakhade wrote: > > > > -----Original Message----- > > From: Nikhil Devshatwar <nikhil.nd@ti.com> > > Sent: Tuesday, November 10, 2020 7:23 PM > > To: Tomi Valkeinen <tomi.valkeinen@ti.com>; Swapnil Kashinath Jakhade > > <sjakhade@cadence.com>; Yuti Suresh Amonkar <yamonkar@cadence.com> > > Cc: dri-devel@lists.freedesktop.org; Swapnil Kashinath Jakhade > > <sjakhade@cadence.com>; Sekhar Nori <nsekhar@ti.com>; Laurent Pinchart > > <laurent.pinchart@ideasonboard.com>; Yuti Suresh Amonkar > > <yamonkar@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@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. So do you think something should change now? I assume that you will modify this part when supporting HDCP. Also, please provide your ack if this patch is the right fix. Nikhil D > > Swapnil > > > > > Nikhil D > > > > > > Tomi > > > > > > -- > > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > > > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Nikhil, > -----Original Message----- > From: Nikhil Devshatwar <nikhil.nd@ti.com> > Sent: Monday, November 9, 2020 10:36 PM > To: dri-devel@lists.freedesktop.org; Tomi Valkeinen > <tomi.valkeinen@ti.com> > Cc: Sekhar Nori <nsekhar@ti.com>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Swapnil Kashinath Jakhade > <sjakhade@cadence.com>; Yuti Suresh Amonkar <yamonkar@cadence.com> > Subject: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt > enable/disable > > EXTERNAL MAIL > > > 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@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); > } > Can we do similar change at other places in driver too? Other than that: Reviewed-by: Swapnil Jakhade <sjakhade@cadence.com> Thanks & regards, Swapnil > static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = { > -- > 2.17.1
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 = {
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@ti.com> --- drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)