[edk2,v2,1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

Message ID 1520901090-96694-2-git-send-email-heyi.guo@linaro.org
State New
Headers show
Series
  • ArmPkg/TimerDxe: Add ISB for timer compare value reload
Related show

Commit Message

gary guo March 13, 2018, 12:31 a.m.
If timer interrupt is level sensitive, reloading timer compare
register has a side effect of clearing GIC pending status, so a "ISB"
is needed to make sure this instruction is executed before enabling
CPU IRQ, or else we may get spurious timer interrupts.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---

Notes:
    v2:
    - Use ISB instead of DSB [Marc]
    - Update commit message accordingly.

 ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Marc Zyngier March 13, 2018, 9:33 a.m. | #1
On 13/03/18 00:31, Heyi Guo wrote:
> If timer interrupt is level sensitive, reloading timer compare

> register has a side effect of clearing GIC pending status, so a "ISB"

> is needed to make sure this instruction is executed before enabling

> CPU IRQ, or else we may get spurious timer interrupts.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> Cc: Leif Lindholm <leif.lindholm@linaro.org>

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Cc: Marc Zyngier <marc.zyngier@arm.com>

> ---

> 

> Notes:

>     v2:

>     - Use ISB instead of DSB [Marc]

>     - Update commit message accordingly.

> 

>  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> index 33d7c922221f..32abee8726a0 100644

> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> @@ -337,6 +337,7 @@ TimerInterruptHandler (

>  

>      // Set next compare value

>      ArmGenericTimerSetCompareVal (CompareValue);

> +    ArmInstructionSynchronizationBarrier ();

>      ArmGenericTimerEnableTimer ();

>    }


Sorry for being pedantic here, but it would make more sense if ISB was
placed after the enabling of the timer. Otherwise, you only force the
update of the comparator. But on the other hand, the timer was never
disabled the first place, in which case you'd wonder why you're trying
to enable it again.

So either you leave the ISB here and remove the enable call, or move the
ISB after the enable.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo March 14, 2018, 12:25 a.m. | #2
On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote:
> On 13/03/18 00:31, Heyi Guo wrote:

> > If timer interrupt is level sensitive, reloading timer compare

> > register has a side effect of clearing GIC pending status, so a "ISB"

> > is needed to make sure this instruction is executed before enabling

> > CPU IRQ, or else we may get spurious timer interrupts.

> > 

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > Cc: Leif Lindholm <leif.lindholm@linaro.org>

> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > Cc: Marc Zyngier <marc.zyngier@arm.com>

> > ---

> > 

> > Notes:

> >     v2:

> >     - Use ISB instead of DSB [Marc]

> >     - Update commit message accordingly.

> > 

> >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +

> >  1 file changed, 1 insertion(+)

> > 

> > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> > index 33d7c922221f..32abee8726a0 100644

> > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> > @@ -337,6 +337,7 @@ TimerInterruptHandler (

> >  

> >      // Set next compare value

> >      ArmGenericTimerSetCompareVal (CompareValue);

> > +    ArmInstructionSynchronizationBarrier ();

> >      ArmGenericTimerEnableTimer ();

> >    }

> 

> Sorry for being pedantic here, but it would make more sense if ISB was

> placed after the enabling of the timer. Otherwise, you only force the

> update of the comparator. But on the other hand, the timer was never

> disabled the first place, in which case you'd wonder why you're trying

> to enable it again.

Yes, I also had such question and hesitated at this place :)
> 

> So either you leave the ISB here and remove the enable call, or move the

> ISB after the enable.


If we are going to remove the enable call, is it better to be changed in a
separate patch? It seems not related with adding ISB, though it is only a
one-line change.

Thanks and regards,
Heyi

> 

> Thanks,

> 

> 	M.

> -- 

> Jazz is not dead. It just smells funny...

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Marc Zyngier March 14, 2018, 7:45 a.m. | #3
On Wed, 14 Mar 2018 00:25:09 +0000,
Guo Heyi wrote:
> 

> On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote:

> > On 13/03/18 00:31, Heyi Guo wrote:

> > > If timer interrupt is level sensitive, reloading timer compare

> > > register has a side effect of clearing GIC pending status, so a "ISB"

> > > is needed to make sure this instruction is executed before enabling

> > > CPU IRQ, or else we may get spurious timer interrupts.

> > > 

> > > Contributed-under: TianoCore Contribution Agreement 1.1

> > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>

> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > > Cc: Marc Zyngier <marc.zyngier@arm.com>

> > > ---

> > > 

> > > Notes:

> > >     v2:

> > >     - Use ISB instead of DSB [Marc]

> > >     - Update commit message accordingly.

> > > 

> > >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +

> > >  1 file changed, 1 insertion(+)

> > > 

> > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> > > index 33d7c922221f..32abee8726a0 100644

> > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> > > @@ -337,6 +337,7 @@ TimerInterruptHandler (

> > >  

> > >      // Set next compare value

> > >      ArmGenericTimerSetCompareVal (CompareValue);

> > > +    ArmInstructionSynchronizationBarrier ();

> > >      ArmGenericTimerEnableTimer ();

> > >    }

> > 

> > Sorry for being pedantic here, but it would make more sense if ISB was

> > placed after the enabling of the timer. Otherwise, you only force the

> > update of the comparator. But on the other hand, the timer was never

> > disabled the first place, in which case you'd wonder why you're trying

> > to enable it again.

> Yes, I also had such question and hesitated at this place :)

> > 

> > So either you leave the ISB here and remove the enable call, or move the

> > ISB after the enable.

> 

> If we are going to remove the enable call, is it better to be changed in a

> separate patch? It seems not related with adding ISB, though it is only a

> one-line change.


I guess a separate patch doesn't hurt, but that's for Ard and Leif to
decide.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 14, 2018, 2:50 p.m. | #4
On 14 March 2018 at 07:45, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Wed, 14 Mar 2018 00:25:09 +0000,

> Guo Heyi wrote:

>>

>> On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote:

>> > On 13/03/18 00:31, Heyi Guo wrote:

>> > > If timer interrupt is level sensitive, reloading timer compare

>> > > register has a side effect of clearing GIC pending status, so a "ISB"

>> > > is needed to make sure this instruction is executed before enabling

>> > > CPU IRQ, or else we may get spurious timer interrupts.

>> > >

>> > > Contributed-under: TianoCore Contribution Agreement 1.1

>> > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

>> > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

>> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>

>> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> > > Cc: Marc Zyngier <marc.zyngier@arm.com>

>> > > ---

>> > >

>> > > Notes:

>> > >     v2:

>> > >     - Use ISB instead of DSB [Marc]

>> > >     - Update commit message accordingly.

>> > >

>> > >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +

>> > >  1 file changed, 1 insertion(+)

>> > >

>> > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c

>> > > index 33d7c922221f..32abee8726a0 100644

>> > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c

>> > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c

>> > > @@ -337,6 +337,7 @@ TimerInterruptHandler (

>> > >

>> > >      // Set next compare value

>> > >      ArmGenericTimerSetCompareVal (CompareValue);

>> > > +    ArmInstructionSynchronizationBarrier ();

>> > >      ArmGenericTimerEnableTimer ();

>> > >    }

>> >

>> > Sorry for being pedantic here, but it would make more sense if ISB was

>> > placed after the enabling of the timer. Otherwise, you only force the

>> > update of the comparator. But on the other hand, the timer was never

>> > disabled the first place, in which case you'd wonder why you're trying

>> > to enable it again.

>> Yes, I also had such question and hesitated at this place :)

>> >

>> > So either you leave the ISB here and remove the enable call, or move the

>> > ISB after the enable.

>>

>> If we are going to remove the enable call, is it better to be changed in a

>> separate patch? It seems not related with adding ISB, though it is only a

>> one-line change.

>

> I guess a separate patch doesn't hurt, but that's for Ard and Leif to

> decide.

>


Yes, please.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo March 15, 2018, 7:11 a.m. | #5
Hi Marc and Ard,

I found the timer re-enable code was added by Ard for special reason:

commit b1a633434ddc5fc28de817debd963f7845fb78c7
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Thu Sep 18 21:16:47 2014 +0000

    ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling

So this line of code cannot be removed and I will add an ISB after enabling
timer.

Another strange thing is that we got lots of "Spurious GIC interrupt" error
messages when timer enable code was removed, though at last it could boot to EFI
shell. Please let me know if you have any idea about the possible reason of this
issue.

Regards,
Heyi

On Wed, Mar 14, 2018 at 02:50:41PM +0000, Ard Biesheuvel wrote:
> On 14 March 2018 at 07:45, Marc Zyngier <marc.zyngier@arm.com> wrote:

> > On Wed, 14 Mar 2018 00:25:09 +0000,

> > Guo Heyi wrote:

> >>

> >> On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote:

> >> > On 13/03/18 00:31, Heyi Guo wrote:

> >> > > If timer interrupt is level sensitive, reloading timer compare

> >> > > register has a side effect of clearing GIC pending status, so a "ISB"

> >> > > is needed to make sure this instruction is executed before enabling

> >> > > CPU IRQ, or else we may get spurious timer interrupts.

> >> > >

> >> > > Contributed-under: TianoCore Contribution Agreement 1.1

> >> > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> >> > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> >> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>

> >> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> > > Cc: Marc Zyngier <marc.zyngier@arm.com>

> >> > > ---

> >> > >

> >> > > Notes:

> >> > >     v2:

> >> > >     - Use ISB instead of DSB [Marc]

> >> > >     - Update commit message accordingly.

> >> > >

> >> > >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +

> >> > >  1 file changed, 1 insertion(+)

> >> > >

> >> > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> >> > > index 33d7c922221f..32abee8726a0 100644

> >> > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> >> > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c

> >> > > @@ -337,6 +337,7 @@ TimerInterruptHandler (

> >> > >

> >> > >      // Set next compare value

> >> > >      ArmGenericTimerSetCompareVal (CompareValue);

> >> > > +    ArmInstructionSynchronizationBarrier ();

> >> > >      ArmGenericTimerEnableTimer ();

> >> > >    }

> >> >

> >> > Sorry for being pedantic here, but it would make more sense if ISB was

> >> > placed after the enabling of the timer. Otherwise, you only force the

> >> > update of the comparator. But on the other hand, the timer was never

> >> > disabled the first place, in which case you'd wonder why you're trying

> >> > to enable it again.

> >> Yes, I also had such question and hesitated at this place :)

> >> >

> >> > So either you leave the ISB here and remove the enable call, or move the

> >> > ISB after the enable.

> >>

> >> If we are going to remove the enable call, is it better to be changed in a

> >> separate patch? It seems not related with adding ISB, though it is only a

> >> one-line change.

> >

> > I guess a separate patch doesn't hurt, but that's for Ard and Leif to

> > decide.

> >

> 

> Yes, please.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 15, 2018, 7:30 a.m. | #6
On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote:
> Hi Marc and Ard,

>

> I found the timer re-enable code was added by Ard for special reason:

>

> commit b1a633434ddc5fc28de817debd963f7845fb78c7

> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Date:   Thu Sep 18 21:16:47 2014 +0000

>

>     ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling

>

> So this line of code cannot be removed and I will add an ISB after enabling

> timer.

>


I'm not sure. IIUC, the KVM issue that required this has been fixed
long ago, and I don't want to carry this forever. Marc?

> Another strange thing is that we got lots of "Spurious GIC interrupt" error

> messages when timer enable code was removed, though at last it could boot to EFI

> shell. Please let me know if you have any idea about the possible reason of this

> issue.

>


No idea, sorry.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Marc Zyngier March 15, 2018, 9:40 a.m. | #7
On 15/03/18 07:30, Ard Biesheuvel wrote:
> On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote:

>> Hi Marc and Ard,

>>

>> I found the timer re-enable code was added by Ard for special reason:

>>

>> commit b1a633434ddc5fc28de817debd963f7845fb78c7

>> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> Date:   Thu Sep 18 21:16:47 2014 +0000

>>

>>     ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling

>>

>> So this line of code cannot be removed and I will add an ISB after enabling

>> timer.

>>

> 

> I'm not sure. IIUC, the KVM issue that required this has been fixed

> long ago, and I don't want to carry this forever. Marc?


This has been fixed quite a while ago:

commit f120cd6533d21075ab103ae6c225b1697853660d
Author: Marc Zyngier <marc.zyngier@arm.com>
Date:   Mon Jun 23 13:59:13 2014 +0100

    KVM: arm/arm64: timer: Allow the timer to control the active state

    In order to remove the crude hack where we sneak the masked bit
    into the timer's control register, make use of the phys_irq_map
    API control the active state of the interrupt.

    This causes some limited changes to allow for potential error
    propagation.

    Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

    Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>


>> Another strange thing is that we got lots of "Spurious GIC interrupt" error

>> messages when timer enable code was removed, though at last it could boot to EFI

>> shell. Please let me know if you have any idea about the possible reason of this

>> issue.

>>

> 

> No idea, sorry.


Me neither. You'd have to dump the control register before and after,
and work out what is being changed exactly.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 15, 2018, 9:52 a.m. | #8
On 15 March 2018 at 09:40, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 15/03/18 07:30, Ard Biesheuvel wrote:

>> On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote:

>>> Hi Marc and Ard,

>>>

>>> I found the timer re-enable code was added by Ard for special reason:

>>>

>>> commit b1a633434ddc5fc28de817debd963f7845fb78c7

>>> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>> Date:   Thu Sep 18 21:16:47 2014 +0000

>>>

>>>     ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling

>>>

>>> So this line of code cannot be removed and I will add an ISB after enabling

>>> timer.

>>>

>>

>> I'm not sure. IIUC, the KVM issue that required this has been fixed

>> long ago, and I don't want to carry this forever. Marc?

>

> This has been fixed quite a while ago:

>

> commit f120cd6533d21075ab103ae6c225b1697853660d

> Author: Marc Zyngier <marc.zyngier@arm.com>

> Date:   Mon Jun 23 13:59:13 2014 +0100

>

>     KVM: arm/arm64: timer: Allow the timer to control the active state

>

>     In order to remove the crude hack where we sneak the masked bit

>     into the timer's control register, make use of the phys_irq_map

>     API control the active state of the interrupt.

>

>     This causes some limited changes to allow for potential error

>     propagation.

>

>     Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

>     Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

>


Does this mean we can lose this as well?

https://github.com/tianocore/edk2/blob/master/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c#L31


>>> Another strange thing is that we got lots of "Spurious GIC interrupt" error

>>> messages when timer enable code was removed, though at last it could boot to EFI

>>> shell. Please let me know if you have any idea about the possible reason of this

>>> issue.

>>>

>>

>> No idea, sorry.

>

> Me neither. You'd have to dump the control register before and after,

> and work out what is being changed exactly.

>

> Thanks,

>

>         M.

> --

> Jazz is not dead. It just smells funny...

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Marc Zyngier March 15, 2018, 10:02 a.m. | #9
On 15/03/18 09:52, Ard Biesheuvel wrote:
> On 15 March 2018 at 09:40, Marc Zyngier <marc.zyngier@arm.com> wrote:

>> On 15/03/18 07:30, Ard Biesheuvel wrote:

>>> On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote:

>>>> Hi Marc and Ard,

>>>>

>>>> I found the timer re-enable code was added by Ard for special reason:

>>>>

>>>> commit b1a633434ddc5fc28de817debd963f7845fb78c7

>>>> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>>> Date:   Thu Sep 18 21:16:47 2014 +0000

>>>>

>>>>     ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling

>>>>

>>>> So this line of code cannot be removed and I will add an ISB after enabling

>>>> timer.

>>>>

>>>

>>> I'm not sure. IIUC, the KVM issue that required this has been fixed

>>> long ago, and I don't want to carry this forever. Marc?

>>

>> This has been fixed quite a while ago:

>>

>> commit f120cd6533d21075ab103ae6c225b1697853660d

>> Author: Marc Zyngier <marc.zyngier@arm.com>

>> Date:   Mon Jun 23 13:59:13 2014 +0100

>>

>>     KVM: arm/arm64: timer: Allow the timer to control the active state

>>

>>     In order to remove the crude hack where we sneak the masked bit

>>     into the timer's control register, make use of the phys_irq_map

>>     API control the active state of the interrupt.

>>

>>     This causes some limited changes to allow for potential error

>>     propagation.

>>

>>     Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

>>     Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

>>

> 

> Does this mean we can lose this as well?

> 

> https://github.com/tianocore/edk2/blob/master/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c#L31


Indeed, you should be able to remove this as well.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
index 33d7c922221f..32abee8726a0 100644
--- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
+++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
@@ -337,6 +337,7 @@  TimerInterruptHandler (
 
     // Set next compare value
     ArmGenericTimerSetCompareVal (CompareValue);
+    ArmInstructionSynchronizationBarrier ();
     ArmGenericTimerEnableTimer ();
   }