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

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

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