-----Original Message-----Hello Zev, before the patch, do you met issue with irq handler? [continuous incoming?]
From: Joel Stanley <joel@xxxxxxxxx>
Sent: Wednesday, December 23, 2020 9:07 AM
To: Zev Weiss <zev@xxxxxxxxxxxxxxxxx>; Ryan Chen
<ryan_chen@xxxxxxxxxxxxxx>
Cc: Eddie James <eajames@xxxxxxxxxxxxx>; Mauro Carvalho Chehab
<mchehab@xxxxxxxxxx>; Andrew Jeffery <andrew@xxxxxxxx>;
linux-media@xxxxxxxxxxxxxxx; OpenBMC Maillist <openbmc@xxxxxxxxxxxxxxxx>;
Linux ARM <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; linux-aspeed
<linux-aspeed@xxxxxxxxxxxxxxxx>; Linux Kernel Mailing List
<linux-kernel@xxxxxxxxxxxxxxx>; Jae Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH 2/3] aspeed-video: clear spurious interrupt bits
unconditionally
On Tue, 22 Dec 2020 at 19:14, Zev Weiss <zev@xxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Dec 21, 2020 at 10:47:37PM CST, Joel Stanley wrote:
> >On Tue, 15 Dec 2020 at 02:46, Zev Weiss <zev@xxxxxxxxxxxxxxxxx> wrote:
> >>
> >> Instead of testing and conditionally clearing them one by one, we
> >> can instead just unconditionally clear them all at once.
> >>
> >> Signed-off-by: Zev Weiss <zev@xxxxxxxxxxxxxxxxx>
> >
> >I had a poke at the assembly and it looks like GCC is clearing the
> >bits unconditionally anyway, so removing the tests provides no change.
> >
> >Combining them is a good further optimization.
> >
> >Reviewed-by: Joel Stanley <joel@xxxxxxxxx>
> >
> >A question unrelated to this patch: Do you know why the driver
> >doesn't clear the status bits in the interrupt handler? I would
> >expect it to write the value of sts back to the register to ack the
> >pending interrupt.
> >
>
> No, I don't, and I was sort of wondering the same thing actually --
> I'm not deeply familiar with this hardware or driver though, so I was
> a bit hesitant to start messing with things. (Though maybe doing so
> would address the "stickiness" aspect when it does manifest.) Perhaps
> Eddie or Jae can shed some light here?
I think you're onto something here - this would be why the status bits seem to
stick until the device is reset.
Until Aspeed can clarify if this is a hardware or software issue, I suggest we ack
the bits and log a message when we see them, instead of always ignoring them
without taking any action.
Can you write a patch that changes the interrupt handler to ack status bits as it
handles each of them?
In aspeed_video_irq handler should only handle enable interrupt expected.
u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
+ sts &= aspeed_video_read(video, VE_INTERRUPT_CTRL);
Ryan