[PATCH] PL031: Actually disable interrupts

[PATCH] PL031: Actually disable interrupts

[Alexander Graf]

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.

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.

Signed-off-by: Alexander Graf <graf@amazon.com>
---
 .../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

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

[Leif Lindholm]

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*

> 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
> 
> 
> 
> 

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

[Laszlo Ersek]

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
>>
>>
>>
>>
> 
> 
> 

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

[Alexander Graf]

> Am 11.07.2019 um 19:07 schrieb 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?

No symptoms that I've encountered. I just stumbled over it while studying the device and its respective UEFI code :).

But yes, you would see a spurious interrupt once the RTC wraps around to 0, so in 2038. Not that that would matter, as by that time you lost the only wall clock reference available on your ARM system anyway :).


Alex