RE: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
Hi Nikhil,
> -Original Message-
> From: Nikhil Devshatwar
> Sent: Monday, November 9, 2020 10:36 PM
> To: dri-devel@lists.freedesktop.org; Tomi Valkeinen
>
> Cc: Sekhar Nori ; Laurent Pinchart
> ; Swapnil Kashinath Jakhade
> ; Yuti Suresh Amonkar
> 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] [<5367c4f9>] irq_default_primary_handler threaded
> [<7e02b601>]
> 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
> ---
> 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
Thanks & regards,
Swapnil
> static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
> --
> 2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
> -Original Message-
> From: Nikhil Devshatwar
> Sent: Tuesday, November 10, 2020 7:23 PM
> To: Tomi Valkeinen ; Swapnil Kashinath Jakhade
> ; Yuti Suresh Amonkar
> Cc: dri-devel@lists.freedesktop.org; Swapnil Kashinath Jakhade
> ; Sekhar Nori ; Laurent Pinchart
> ; Yuti Suresh Amonkar
>
> 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] [<5367c4f9>] irq_default_primary_handler
> threaded [<7e02b601>]
> > >>> 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
> > >>> ---
> > >>> 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
On 14:32-20201110, Swapnil Jakhade wrote:
>
>
> > -Original Message-
> > From: Nikhil Devshatwar
> > Sent: Tuesday, November 10, 2020 7:23 PM
> > To: Tomi Valkeinen ; Swapnil Kashinath Jakhade
> > ; Yuti Suresh Amonkar
> > Cc: dri-devel@lists.freedesktop.org; Swapnil Kashinath Jakhade
> > ; Sekhar Nori ; Laurent Pinchart
> > ; Yuti Suresh Amonkar
> >
> > 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] [<5367c4f9>] irq_default_primary_handler
> > threaded [<7e02b601>]
> > > >>> 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
> > > >>> ---
> > > >>> 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
Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
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] [<5367c4f9>] irq_default_primary_handler threaded
> >>> [<7e02b601>]
> >>> 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
> >>> ---
> >>> 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
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] [<5367c4f9>] irq_default_primary_handler threaded
>>> [<7e02b601>]
>>> 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
>>> ---
>>> 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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
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] [<5367c4f9>] irq_default_primary_handler threaded
> > [<7e02b601>]
> > 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
> > ---
> > 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 6/6] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
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] [<5367c4f9>] irq_default_primary_handler threaded
> [<7e02b601>]
> 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
> ---
> 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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
