Re: [PATCH 1/1] ArmPkg/TimerDxe: Add DSB for timer compare value reload

[Guo Heyi <heyi.guo@linaro.org>]

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