PL031: Actually disable interrupts

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

Posted by Alexander Graf via Groups.Io 4 years, 9 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43483): https://edk2.groups.io/g/devel/message/43483
Mute This Topic: https://groups.io/mt/32416951/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Leif Lindholm 4 years, 9 months ago

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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43489): https://edk2.groups.io/g/devel/message/43489
Mute This Topic: https://groups.io/mt/32416951/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Laszlo Ersek 4 years, 9 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43614): https://edk2.groups.io/g/devel/message/43614
Mute This Topic: https://groups.io/mt/32416951/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Alexander Graf via Groups.Io 4 years, 9 months ago


> 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43616): https://edk2.groups.io/g/devel/message/43616
Mute This Topic: https://groups.io/mt/32416951/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-