Re: [Qemu-devel] [PATCH] hw/pl031: Actually raise interrupt on timer expiry

[Peter Maydell <peter.maydell@linaro.org>]

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