Re: [edk2-devel] [PATCH] PL031: Actually disable interrupts

["Laszlo Ersek" <lersek@redhat.com>]

On 07/10/19 19:13, Leif Lindholm wrote:
> On Wed, Jul 10, 2019 at 04:53:11PM +0200, Alexander Graf via Groups.Io wrote:
>> The PL031 interrupt mask register (IMSC) is not very clearly documented
>> in the PL031 specification. However, bit 0 (RTCIMSC) indicates whether
>> interrupts are enabled, not disabled.
> 
> 3.3.5. Interrupt Mask Set or Clear register, RTCIMSC
> ... Writing 1 sets the mask. ...
> 
> 3.6. Interrupts
> ... This interrupt is enabled or disabled by changing the mask bit in
> RTCIMSC. To enable the interrupt, set bit[0] HIGH. ...
> 
> *boggle*

Heh :)

Alex, out of interest, what were the symptoms of this issue on your end?
Spurious interrupt confusing the firmware's exception handler, perhaps?

Thanks!
Laszlo

> 
>> So before this commit, we were actually *enabling* interrupts for the RTC.
>>
>> This patch changes the logic to instead disable interrupts when they
>> are not disabled already.
> 
> Thanks for finding/fixing this.
> 
>> Signed-off-by: Alexander Graf <graf@amazon.com>
> 
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> I took the liberty to change the subject line to
> "ArmPlatformPkg: Actually disable PL031 interrupts"
> (We tend to start with the top-level directory.)
> 
> Pushed as 8df52631e53c.
> 
> /
>     Leif
> 
>> ---
>>  .../Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c     | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
>> index b630a5cfbf..75c95985d4 100644
>> --- a/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
>> +++ b/ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c
>> @@ -80,8 +80,8 @@ InitializePL031 (
>>    }
>>  
>>    // Ensure interrupts are masked. We do not want RTC interrupts in UEFI
>> -  if ((MmioRead32 (mPL031RtcBase + PL031_RTC_IMSC_IRQ_MASK_SET_CLEAR_REGISTER) & PL031_SET_IRQ_MASK) != PL031_SET_IRQ_MASK) {
>> -    MmioOr32 (mPL031RtcBase + PL031_RTC_IMSC_IRQ_MASK_SET_CLEAR_REGISTER, PL031_SET_IRQ_MASK);
>> +  if ((MmioRead32 (mPL031RtcBase + PL031_RTC_IMSC_IRQ_MASK_SET_CLEAR_REGISTER) & PL031_SET_IRQ_MASK) != 0) {
>> +    MmioWrite32 (mPL031RtcBase + PL031_RTC_IMSC_IRQ_MASK_SET_CLEAR_REGISTER, 0);
>>    }
>>  
>>    // Clear any existing interrupts
>> -- 
>> 2.17.1
>>
>>
>>
>>
> 
> 
>