[PATCH] hw/timer/a9gtimer: Clear pending interrupt, after the clear of Event flag

Vaclav Vanc posted 1 patch 3 years, 10 months ago
Test docker-mingw@fedora passed
Test checkpatch passed
Test asan passed
Test docker-quick@centos7 passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200610084551.28222-1-vav@sysgo.com
Maintainers: Peter Maydell <peter.maydell@linaro.org>
hw/timer/a9gtimer.c | 3 +++
1 file changed, 3 insertions(+)
[PATCH] hw/timer/a9gtimer: Clear pending interrupt, after the clear of Event flag
Posted by Vaclav Vanc 3 years, 10 months ago
A9 Global Timer is used with Edge triggered interrupts (This is true
at least for Zynq and i.MX6 processors).
When Event Flag is cleared in Interrupt Status Register and new interrupt
was supposed to be scheduled, interrupt request is never cleared.
Since interrupt in GIC is configured as Edge triggered, new interrupts
are not registered (because interrupt is stuck at pending and GIC thinks
it was already serviced). As a result interrupts from timer does not work
anymore.

Note: This happens only when interrupt was not serviced before the next
interrupt is suppose to be scheduled. This happens for example during
the increased load of the host system.

Interrupt is now always cleared when Event Flag is cleared.
This is in accordance to A9 Global Timer documentation.

Signed-off-by: Vaclav Vanc <vav@sysgo.com>
---
 hw/timer/a9gtimer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
index 7233068a37..732889105e 100644
--- a/hw/timer/a9gtimer.c
+++ b/hw/timer/a9gtimer.c
@@ -206,6 +206,9 @@ static void a9_gtimer_write(void *opaque, hwaddr addr, uint64_t value,
     case R_INTERRUPT_STATUS:
         a9_gtimer_update(s, false);
         gtb->status &= ~value;
+        if (gtb->status == 0) {
+            qemu_set_irq(gtb->irq, 0);
+        }
         break;
     case R_COMPARATOR_HI:
         shift = 32;
-- 
2.20.1


Re: [PATCH] hw/timer/a9gtimer: Clear pending interrupt, after the clear of Event flag
Posted by Peter Maydell 3 years, 10 months ago
On Wed, 10 Jun 2020 at 09:47, Vaclav Vanc <vav@sysgo.com> wrote:
>
> A9 Global Timer is used with Edge triggered interrupts (This is true
> at least for Zynq and i.MX6 processors).
> When Event Flag is cleared in Interrupt Status Register and new interrupt
> was supposed to be scheduled, interrupt request is never cleared.
> Since interrupt in GIC is configured as Edge triggered, new interrupts
> are not registered (because interrupt is stuck at pending and GIC thinks
> it was already serviced). As a result interrupts from timer does not work
> anymore.
>
> Note: This happens only when interrupt was not serviced before the next
> interrupt is suppose to be scheduled. This happens for example during
> the increased load of the host system.
>
> Interrupt is now always cleared when Event Flag is cleared.
> This is in accordance to A9 Global Timer documentation.
>
> Signed-off-by: Vaclav Vanc <vav@sysgo.com>
> ---
>  hw/timer/a9gtimer.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
> index 7233068a37..732889105e 100644
> --- a/hw/timer/a9gtimer.c
> +++ b/hw/timer/a9gtimer.c
> @@ -206,6 +206,9 @@ static void a9_gtimer_write(void *opaque, hwaddr addr, uint64_t value,
>      case R_INTERRUPT_STATUS:
>          a9_gtimer_update(s, false);
>          gtb->status &= ~value;
> +        if (gtb->status == 0) {
> +            qemu_set_irq(gtb->irq, 0);
> +        }
>          break;

Hi; thanks for this patch. I can see the situation you're trying to address,
but I can't find anything in the docs that convinces me that this change
is the right way to fix it.

The problem we have is that the a9_gtimer_update() function (which is
going to get called after this code at the end of the a9_gtimer_write()
function) updates the gtb->status bit based on whether the timer has
currently passed the compare value. So effectively we do the "has the
count gone past the compare value" check not only when the timer expires
but also at every register write. My best guess is that the real hardware
only does an expiry-check when it does a counter value update. If that's
the case then in the situation you outline the guest clearing the event
flag should mean that the interrupt is not re-asserted until the counter
next increments past the compare value (ie not for a little while) whereas
with your patch it will be instantly re-asserted.

(Unfortunately the A9 TRM is not clear on the behaviour here. It would
probably be possible to write some test code to check the real h/w
behaviour.)

thanks
-- PMM

Re: [PATCH] hw/timer/a9gtimer: Clear pending interrupt, after the clear of Event flag
Posted by Václav Vanc 3 years, 10 months ago
On 6/15/20 1:04 PM, Peter Maydell wrote:
> On Wed, 10 Jun 2020 at 09:47, Vaclav Vanc <vav@sysgo.com> wrote:
>>
>> A9 Global Timer is used with Edge triggered interrupts (This is true
>> at least for Zynq and i.MX6 processors).
>> When Event Flag is cleared in Interrupt Status Register and new interrupt
>> was supposed to be scheduled, interrupt request is never cleared.
>> Since interrupt in GIC is configured as Edge triggered, new interrupts
>> are not registered (because interrupt is stuck at pending and GIC thinks
>> it was already serviced). As a result interrupts from timer does not work
>> anymore.
>>
>> Note: This happens only when interrupt was not serviced before the next
>> interrupt is suppose to be scheduled. This happens for example during
>> the increased load of the host system.
>>
>> Interrupt is now always cleared when Event Flag is cleared.
>> This is in accordance to A9 Global Timer documentation.
>>
>> Signed-off-by: Vaclav Vanc <vav@sysgo.com>
>> ---
>>   hw/timer/a9gtimer.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
>> index 7233068a37..732889105e 100644
>> --- a/hw/timer/a9gtimer.c
>> +++ b/hw/timer/a9gtimer.c
>> @@ -206,6 +206,9 @@ static void a9_gtimer_write(void *opaque, hwaddr addr, uint64_t value,
>>       case R_INTERRUPT_STATUS:
>>           a9_gtimer_update(s, false);
>>           gtb->status &= ~value;
>> +        if (gtb->status == 0) {
>> +            qemu_set_irq(gtb->irq, 0);
>> +        }
>>           break;
> 
> Hi; thanks for this patch. I can see the situation you're trying to address,
> but I can't find anything in the docs that convinces me that this change
> is the right way to fix it.
> 
Hi, thanks for reviewing the patch.

> The problem we have is that the a9_gtimer_update() function (which is
> going to get called after this code at the end of the a9_gtimer_write()
> function) updates the gtb->status bit based on whether the timer has
> currently passed the compare value. So effectively we do the "has the
> count gone past the compare value" check not only when the timer expires
> but also at every register write. My best guess is that the real hardware
> only does an expiry-check when it does a counter value update. If that's
> the case then in the situation you outline the guest clearing the event
> flag should mean that the interrupt is not re-asserted until the counter
> next increments past the compare value (ie not for a little while) whereas
> with your patch it will be instantly re-asserted.

We must distinguish between two cases:
1, Auto-increment is disabled.
I just run some test on SABRE Lite (i.MX6) board.
I had auto-increment disabled, I verified, that GIC is configured for 
Edge interrupts. Once count went past the compare value I got the 
interrupt. I did not touch Timer registers, just signal EOI to GIC and 
surprisingly, I got a another interrupt. If I stopped the timer 
interrupts stopped coming (Status was still set to 1).

 From this behavior I assume, that every time the Timer is incremented 
(and Timer value is past the compare value) an EDGE interrupt (that 
means actual X->0->1 transition) is asserted. This is really interesting 
from HW point of view. This would mean, that a9_gtimer_update function 
should generate the pulse and not level on compare event.

2, Auto-increment is enabled.
This is actually what the patch was aiming for.
First call of a9_gtimer_update will potentially update the compare 
value, then the interrupt and event flag is de-asserted. Second call of 
a9_gtimer_update will just update the timer (or re-assert the interrupt 
if the Timer hit the compare value in the meantime, which is fine).

Let me know I you want me to run some more tests.

> 
> (Unfortunately the A9 TRM is not clear on the behaviour here. It would
> probably be possible to write some test code to check the real h/w
> behaviour.)
>  > thanks
> -- PMM
> 

Best Regards,
Ing. Václav Vanc
Project Engineer

SYSGO s.r.o.
Embedding Innovations
Research and Development Center Prague
Zeleny pruh 1560/99 I CZ-14000 Praha 4
Phone: +420 222138 828, +49 6136 9948 828
Fax: +420 244911174
E-mail: vaclav.vanc@sysgo.com
_________________________________________________________________________________

Web: https://www.sysgo.com
Blog: https://www.sysgo.com/blog
Events: https://www.sysgo.com/events
Newsletter: https://www.sysgo.com/newsletter
_________________________________________________________________________________

Handelsregister/Commercial Registry: HRB Mainz 90 HRB 48884
Geschäftsführung/Managing Directors: Etienne Butery (CEO), Kai Sablotny 
(COO)
USt-Id-Nr./VAT-Id-No.: DE 149062328

The protection of your personal data is important to us. Under the 
following link you can see the information in accordance with article 13 
GDPR: https://www.sysgo.com/privacy_policy

Re: [PATCH] hw/timer/a9gtimer: Clear pending interrupt, after the clear of Event flag
Posted by Peter Maydell 3 years, 10 months ago
On Tue, 16 Jun 2020 at 08:04, Václav Vanc <vav@sysgo.com> wrote:
>
> On 6/15/20 1:04 PM, Peter Maydell wrote:
> We must distinguish between two cases:
> 1, Auto-increment is disabled.
> I just run some test on SABRE Lite (i.MX6) board.
> I had auto-increment disabled, I verified, that GIC is configured for
> Edge interrupts. Once count went past the compare value I got the
> interrupt. I did not touch Timer registers, just signal EOI to GIC and
> surprisingly, I got a another interrupt. If I stopped the timer
> interrupts stopped coming (Status was still set to 1).
>
>  From this behavior I assume, that every time the Timer is incremented
> (and Timer value is past the compare value) an EDGE interrupt (that
> means actual X->0->1 transition) is asserted. This is really interesting
> from HW point of view. This would mean, that a9_gtimer_update function
> should generate the pulse and not level on compare event.

That's interesting. Which version of the Cortex-A9 does this
board have? The TRM documents that the comparator behaviour
changed in r2p0...

thanks
-- PMM

Re: [PATCH] hw/timer/a9gtimer: Clear pending interrupt, after the clear of Event flag
Posted by Václav Vanc 3 years, 9 months ago

On 6/26/20 4:46 PM, Peter Maydell wrote:
> On Tue, 16 Jun 2020 at 08:04, Václav Vanc <vav@sysgo.com> wrote:
>>
>> On 6/15/20 1:04 PM, Peter Maydell wrote:
>> We must distinguish between two cases:
>> 1, Auto-increment is disabled.
>> I just run some test on SABRE Lite (i.MX6) board.
>> I had auto-increment disabled, I verified, that GIC is configured for
>> Edge interrupts. Once count went past the compare value I got the
>> interrupt. I did not touch Timer registers, just signal EOI to GIC and
>> surprisingly, I got a another interrupt. If I stopped the timer
>> interrupts stopped coming (Status was still set to 1).
>>
>>   From this behavior I assume, that every time the Timer is incremented
>> (and Timer value is past the compare value) an EDGE interrupt (that
>> means actual X->0->1 transition) is asserted. This is really interesting
>> from HW point of view. This would mean, that a9_gtimer_update function
>> should generate the pulse and not level on compare event.
> 
> That's interesting. Which version of the Cortex-A9 does this
> board have? The TRM documents that the comparator behaviour
> changed in r2p0...
> 
> thanks
> -- PMM
> 

It is "SABRE Lite (i.MX6)" board with Cortex-A9 r2p10.
Behavior is also the same for "Xilinx Zynq-7000 SoC ZC702 Evaluation 
Kit" with Cortex-A9 r3p0.

Unfortunately I do not have any board with older revision of Cortex-A9.

Best Regards,
Ing. Václav Vanc
Project Engineer

SYSGO s.r.o.
Embedding Innovations
Research and Development Center Prague
Zeleny pruh 1560/99 I CZ-14000 Praha 4
Phone: +420 222138 828, +49 6136 9948 828
Fax: +420 244911174
E-mail: vaclav.vanc@sysgo.com
_________________________________________________________________________________

Web: https://www.sysgo.com
Blog: https://www.sysgo.com/blog
Events: https://www.sysgo.com/events
Newsletter: https://www.sysgo.com/newsletter
_________________________________________________________________________________

Handelsregister/Commercial Registry: HRB Mainz 90 HRB 48884
Geschäftsführung/Managing Directors: Etienne Butery (CEO), Kai Sablotny 
(COO)
USt-Id-Nr./VAT-Id-No.: DE 149062328

The protection of your personal data is important to us. Under the 
following link you can see the information in accordance with article 13 
GDPR: https://www.sysgo.com/privacy_policy