* [PATCH 0/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload @ 2018-03-12 6:53 Heyi Guo 2018-03-12 6:53 ` [PATCH 1/1] " Heyi Guo 0 siblings, 1 reply; 8+ messages in thread From: Heyi Guo @ 2018-03-12 6:53 UTC (permalink / raw) To: edk2-devel; +Cc: Heyi Guo, Leif Lindholm, Ard Biesheuvel, Marc Zyngier After rebasing to edk2 commit 5e3719a, we found D05 would hang after printing a lot of "Spurious interrupt" messages. The issue would gone away if we restored the removal of "enable interrupt source": gInterrupt->EnableInterruptSource (gInterrupt, Source); It can also be fixed if we add a "DSB" after reloading timer compare value, and we think that it makes sense to do that. Cc: Leif Lindholm <leif.lindholm@linaro.org> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Marc Zyngier <marc.zyngier@arm.com> Heyi Guo (1): ArmPkg/TimerDxe: Add DSB for timer compare value reload ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 + 1 file changed, 1 insertion(+) -- 2.7.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload 2018-03-12 6:53 [PATCH 0/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload Heyi Guo @ 2018-03-12 6:53 ` Heyi Guo 2018-03-12 9:46 ` Marc Zyngier 0 siblings, 1 reply; 8+ messages in thread From: Heyi Guo @ 2018-03-12 6:53 UTC (permalink / raw) To: edk2-devel; +Cc: Heyi Guo, Yi Li, Leif Lindholm, Ard Biesheuvel, Marc Zyngier Resetting timer compare register has a side effect of clearing GIC pending status, if timer interrupt is level sensitive, so a "DSB SY" is needed to make sure this change effect is synchronized. 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> --- 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..b732a2ac1b64 100644 --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c @@ -337,6 +337,7 @@ TimerInterruptHandler ( // Set next compare value ArmGenericTimerSetCompareVal (CompareValue); + ArmDataSynchronizationBarrier (); ArmGenericTimerEnableTimer (); } -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload 2018-03-12 6:53 ` [PATCH 1/1] " Heyi Guo @ 2018-03-12 9:46 ` Marc Zyngier 2018-03-12 10:26 ` Guo Heyi 0 siblings, 1 reply; 8+ messages in thread From: Marc Zyngier @ 2018-03-12 9:46 UTC (permalink / raw) To: Heyi Guo, edk2-devel; +Cc: Yi Li, Leif Lindholm, Ard Biesheuvel On 12/03/18 06:53, Heyi Guo wrote: > Resetting timer compare register has a side effect of clearing GIC > pending status, if timer interrupt is level sensitive, so a "DSB SY" > is needed to make sure this change effect is synchronized. > > 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> > --- > 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..b732a2ac1b64 100644 > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > @@ -337,6 +337,7 @@ TimerInterruptHandler ( > > // Set next compare value > ArmGenericTimerSetCompareVal (CompareValue); > + ArmDataSynchronizationBarrier (); > ArmGenericTimerEnableTimer (); > } > > Which HW platform is that on? DSB on its own doesn't have any effect on inputs to the GIC, only on the synchronization at the GIC system register level (see the GICv3 architecture specification, 8.1.6). I don't believe this is required. You could stick an ISB instead to ensure that the write to the CNTVCTL_EL0 is executed, but DSB feels pretty odd, unless this is a workaround for a platform erratum. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload 2018-03-12 9:46 ` Marc Zyngier @ 2018-03-12 10:26 ` Guo Heyi 2018-03-12 10:38 ` Guo Heyi 0 siblings, 1 reply; 8+ messages in thread From: Guo Heyi @ 2018-03-12 10:26 UTC (permalink / raw) To: Marc Zyngier; +Cc: Heyi Guo, edk2-devel, Yi Li, Leif Lindholm, Ard Biesheuvel Thanks, I can try if ISB works. The issue was observed on Hisilicon D05 platform. Regards, Heyi On Mon, Mar 12, 2018 at 09:46:41AM +0000, Marc Zyngier wrote: > On 12/03/18 06:53, Heyi Guo wrote: > > Resetting timer compare register has a side effect of clearing GIC > > pending status, if timer interrupt is level sensitive, so a "DSB SY" > > is needed to make sure this change effect is synchronized. > > > > 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> > > --- > > 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..b732a2ac1b64 100644 > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > > @@ -337,6 +337,7 @@ TimerInterruptHandler ( > > > > // Set next compare value > > ArmGenericTimerSetCompareVal (CompareValue); > > + ArmDataSynchronizationBarrier (); > > ArmGenericTimerEnableTimer (); > > } > > > > > > Which HW platform is that on? > > DSB on its own doesn't have any effect on inputs to the GIC, only on the > synchronization at the GIC system register level (see the GICv3 > architecture specification, 8.1.6). > > I don't believe this is required. You could stick an ISB instead to > ensure that the write to the CNTVCTL_EL0 is executed, but DSB feels > pretty odd, unless this is a workaround for a platform erratum. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload 2018-03-12 10:26 ` Guo Heyi @ 2018-03-12 10:38 ` Guo Heyi 2018-03-12 10:38 ` Ard Biesheuvel 0 siblings, 1 reply; 8+ messages in thread From: Guo Heyi @ 2018-03-12 10:38 UTC (permalink / raw) To: Marc Zyngier Cc: Marc Zyngier, edk2-devel, Yi Li, Leif Lindholm, Ard Biesheuvel Hi Marc, I just tested with an ISB and it also worked for our platform. So is it acceptable to add an ISB after reloading timer compare value? Regards, Heyi On Mon, Mar 12, 2018 at 06:26:43PM +0800, Guo Heyi wrote: > Thanks, I can try if ISB works. The issue was observed on Hisilicon D05 platform. > > Regards, > > Heyi > > On Mon, Mar 12, 2018 at 09:46:41AM +0000, Marc Zyngier wrote: > > On 12/03/18 06:53, Heyi Guo wrote: > > > Resetting timer compare register has a side effect of clearing GIC > > > pending status, if timer interrupt is level sensitive, so a "DSB SY" > > > is needed to make sure this change effect is synchronized. > > > > > > 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> > > > --- > > > 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..b732a2ac1b64 100644 > > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c > > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c > > > @@ -337,6 +337,7 @@ TimerInterruptHandler ( > > > > > > // Set next compare value > > > ArmGenericTimerSetCompareVal (CompareValue); > > > + ArmDataSynchronizationBarrier (); > > > ArmGenericTimerEnableTimer (); > > > } > > > > > > > > > > Which HW platform is that on? > > > > DSB on its own doesn't have any effect on inputs to the GIC, only on the > > synchronization at the GIC system register level (see the GICv3 > > architecture specification, 8.1.6). > > > > I don't believe this is required. You could stick an ISB instead to > > ensure that the write to the CNTVCTL_EL0 is executed, but DSB feels > > pretty odd, unless this is a workaround for a platform erratum. > > > > Thanks, > > > > M. > > -- > > Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload 2018-03-12 10:38 ` Guo Heyi @ 2018-03-12 10:38 ` Ard Biesheuvel 2018-03-12 10:48 ` Marc Zyngier 0 siblings, 1 reply; 8+ messages in thread From: Ard Biesheuvel @ 2018-03-12 10:38 UTC (permalink / raw) To: Guo Heyi; +Cc: Marc Zyngier, edk2-devel@lists.01.org, Yi Li, Leif Lindholm On 12 March 2018 at 10:38, Guo Heyi <heyi.guo@linaro.org> wrote: > Hi Marc, > > I just tested with an ISB and it also worked for our platform. > > So is it acceptable to add an ISB after reloading timer compare value? > I am fine with that if Marc is. -- Ard. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload 2018-03-12 10:38 ` Ard Biesheuvel @ 2018-03-12 10:48 ` Marc Zyngier 2018-03-13 0:34 ` Guo Heyi 0 siblings, 1 reply; 8+ messages in thread From: Marc Zyngier @ 2018-03-12 10:48 UTC (permalink / raw) To: Ard Biesheuvel, Guo Heyi; +Cc: edk2-devel@lists.01.org, Yi Li, Leif Lindholm On 12/03/18 10:38, Ard Biesheuvel wrote: > On 12 March 2018 at 10:38, Guo Heyi <heyi.guo@linaro.org> wrote: >> Hi Marc, >> >> I just tested with an ISB and it also worked for our platform. >> >> So is it acceptable to add an ISB after reloading timer compare value? >> > > I am fine with that if Marc is. An ISB makes a lot more sense to me. You can put my ack on a modified version of that patch. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload 2018-03-12 10:48 ` Marc Zyngier @ 2018-03-13 0:34 ` Guo Heyi 0 siblings, 0 replies; 8+ messages in thread From: Guo Heyi @ 2018-03-13 0:34 UTC (permalink / raw) To: Marc Zyngier Cc: Ard Biesheuvel, Guo Heyi, edk2-devel@lists.01.org, Yi Li, Leif Lindholm On Mon, Mar 12, 2018 at 10:48:59AM +0000, Marc Zyngier wrote: > On 12/03/18 10:38, Ard Biesheuvel wrote: > > On 12 March 2018 at 10:38, Guo Heyi <heyi.guo@linaro.org> wrote: > >> Hi Marc, > >> > >> I just tested with an ISB and it also worked for our platform. > >> > >> So is it acceptable to add an ISB after reloading timer compare value? > >> > > > > I am fine with that if Marc is. > > An ISB makes a lot more sense to me. You can put my ack on a modified > version of that patch. Hi Marc, I've sent v2 patch using ISB. Please help to check the updated commit message to see if my understanding is correct. Thanks, Heyi > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-03-13 0:27 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-12 6:53 [PATCH 0/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload Heyi Guo 2018-03-12 6:53 ` [PATCH 1/1] " Heyi Guo 2018-03-12 9:46 ` Marc Zyngier 2018-03-12 10:26 ` Guo Heyi 2018-03-12 10:38 ` Guo Heyi 2018-03-12 10:38 ` Ard Biesheuvel 2018-03-12 10:48 ` Marc Zyngier 2018-03-13 0:34 ` Guo Heyi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox