On Fri, 2024-09-06 at 13:50 +0100, Peter Maydell wrote: > On Mon, 2 Sept 2024 at 13:32, Jan Klötzke <jan.kloet...@kernkonzept.com> > wrote: > > > > Level triggered interrupts are pending when either the interrupt line > > is asserted or the interrupt was made pending by a GICD_ISPENDRn write. > > Making a level triggered interrupt pending by software persists until > > either the interrupt is acknowledged or cleared by writing > > GICD_ICPENDRn. As long as the interrupt line is asserted, the interrupt > > is pending in any case. > > > > This logic is transparently implemented in gic_test_pending(). The > > function combines the "pending" irq_state flag (used for edge triggered > > interrupts and software requests) and the line status (tracked in the > > "level" field). Now, writing GICD_ISENABLERn incorrectly set the > > pending flag if the line of a level triggered interrupt was asserted. > > This keeps the interrupt pending even if the line is de-asserted after > > some time. > > > > Fix this by simply removing the code. The pending status is fully > > handled by gic_test_pending() and does not need any special treatment > > when enabling the level interrupt. > > > > Signed-off-by: Jan Klötzke <jan.kloet...@kernkonzept.com> > > Thanks for this patch. I agree that this is wrong for the > GICv2 -- I think this is a bit we missed in commit 8d999995e45c > back in 2013 where we fixed most other places that were not > correctly making this distinction of "pending because of > ISPENDR write" and "pending because level triggered and > line is held high". > > However I think for consistency with that commit, we should > retain the current behaviour here for the s->revision == REV_11MPCORE > case. (This is basically saying "we don't really know exactly > how the 11MPCore GIC behaved and we don't much care to try to > find out, so leave it alone", which is the stance we were > already taking in 2013...) In particular, notice that > gic_test_pending() only does the "pending if level triggered > and held high" logic for the not-REV_11MPCORE case.
Right. Thanks for catching this. I'll send a V2 shortly. Actually, I tried to make sense out of the ARM11 MPCore TRM but gave up. At least, I could not come with a consistent idea how the hardware is supposed to behave. Keeping the old behavior really looks like the most sensible option here. Thanks, Jan -- Jan Klötzke, jan.kloet...@kernkonzept.com, +49 351 41883238 Kernkonzept GmbH, Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth
