| Message ID | 4F3B95AA.8090007@suse.de |
|---|---|
| State | Rejected |
| Headers | show |
On 15 February 2012 11:23, Andreas Färber <afaerber@suse.de> wrote: > Am 14.02.2012 18:40, schrieb Peter Maydell: >> Fix a typo in pl031_interrupt() which meant we were setting a bit >> in the interrupt mask rather than the interrupt status register >> and thus not actually raising an interrupt. This fix allows the >> rtctest program from the kernel's Documentation/rtc.txt to pass >> rather than hanging. >> > > Reported-by: Daniel Forsgren <daniel.forsgren@enea.com> > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> Looks like our PL031 has always had this bug since it was added >> in 2007... Daniel Forsgren reported this, suggested the fix and >> pointed me at the test program. Thanks! > > Down here the credit for the find gets lost. > >> https://bugs.launchpad.net/qemu-linaro/+bug/931940 >> >> hw/pl031.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/hw/pl031.c b/hw/pl031.c >> index 8416a60..f06b5ae 100644 >> --- a/hw/pl031.c >> +++ b/hw/pl031.c >> @@ -76,7 +76,7 @@ static void pl031_interrupt(void * opaque) >> { >> pl031_state *s = (pl031_state *)opaque; >> >> - s->im = 1; >> + s->is = 1; >> DPRINTF("Alarm raised\n"); >> pl031_update(s); >> } > > So on RTC_ICR write s->is = 0; but it was never set elsewhere, so > RTC_RIS would always return 0. > > Acked-by: Andreas Färber <afaerber@suse.de> > > However, to facilitate future review of these non-telling fields I > propose the following documentation patch as a follow-up: > +/** > + * pl031_state: > + * @im: Interrupt mask. > + * @is: Interrupt state. > + */ If they're that cryptic we should just rename them to int_mask and int_status. Alternatively there's a good argument that 'im' should be 'imsc' since that's what the PL031 TRM calls that register. By that logic 'is' would be 'ris'. -- PMM
diff --git a/hw/pl031.c b/hw/pl031.c index 8416a60..a20c625 100644 --- a/hw/pl031.c +++ b/hw/pl031.c @@ -32,6 +32,11 @@ do { printf("pl031: " fmt , ## __VA_ARGS__); } while (0) #define RTC_MIS 0x18 /* Masked interrupt status register */ #define RTC_ICR 0x1c /* Interrupt clear register */ +/** + * pl031_state: + * @im: Interrupt mask. + * @is: Interrupt state. + */ typedef struct { SysBusDevice busdev; MemoryRegion iomem;