[U-Boot] [PATCH] arm: ls1021a: Ensure LS1021 ARM Generic Timer CompareValue Set 64-bit

[Huan Wang]

Hi, Mark,

On Fri, Jul 17, 2015 at 11:01:01AM +0100, Huan Wang wrote:
> Hi, Mark,
>
> > > On Wed, Jul 15, 2015 at 08:13:05AM +0100, Alison Wang wrote:
> > > > This patch addresses a problem mentioned recently on this mailing
> > > list:
> > > > [1].
> > > >
> > > > In that posting a LS1021 based system was locking up at about 5
> > > > minutes after boot, but the problem was mysteriously related to the
> > > > toolchain used for building u-boot.  Debugging the problem reveals
> > a
> > > > stuck interrupt 29 on the GIC.
> > > >
> > > > It appears Freescale's LS1021 support in u-boot erroneously sets
> > the
> > > > 64-bit ARM generic PL1 physical time CompareValue register to all-
> > > ones
> > > > with a 32-bit value.  This causes the timer compare to fire 344
> > > > seconds after u-boot configures it.  Depending on how fast u-boot
> > > gets
> > > > the kernel booted, this amounts to about 5-minutes of Linux uptime
> > > > before locking up.
> > >
> > > If as in [2] this is an attempt to not generate interrupts that Linux
> > > doesn't expect, it would be far better to simply disable the timer
> > > interrupt before leaving U-Boot, ensuring that unexpected interrupts
> > > are never generated regardless of the width or rate of the counter.
> > >
> > > There are bits in CNTP_CTL to do this.
> > [Alison Wang] Yes, your idea is far better.
> [Alison Wang] If the CompareValue register is not written, is there any unexpected
> Interrupt?

If you don't write to CNTP_CVAL, you have the exact same problem. The
only difference is that CNTP_CVAL contains an UNKNOWN value, and so the
interrutp could trigger at any point in time.
[Alison Wang] Thanks for reminding me. It's terrible if the interrupt could
trigger at any point in time. 

> How about removing the following code?
>
> /* Set PL1 Physical Comp Value */
> val = TIMER_COMP_VAL;
> asm("mcrr p15, 2, %Q0, %R0, c14" : : "r" (val));

To stop the interrupt from firing at all you can clear CNTP_CTL.ENABLE,
which will disable the comparator. You could instead set CNTP_CTL.IMASK,
but I think clearing ENABLE is preferable because you might also save
power.
[Alison Wang] Yes, I think so. 
Thanks.

Thanks,
Mark.

>
> > > Thanks,
> > > Mark.
> > >
> > > [2] http://lists.denx.de/pipermail/u-boot/2015-July/218937.html
> > > [3] http://lists.denx.de/pipermail/u-boot/2015-July/218979.html
> > >
> > > > Apparently the bug is masked by some toolchains.  Perhaps this is
> > > > explained by default compiler options, word sizes, or binutils
> > > versions.
> > > > At any rate this patch makes the manipulation explicitly 64-bit
> > > > which alleviates the issue.
> > > >
> > > > [1]
> > > > https://lists.yoctoproject.org/pipermail/meta-freescale/2015-
> > > June/0144
> > > > 00.html
> > > > Signed-off-by: Chris Kilgour <techie at whiterocker.com>
> > > > Signed-off-by: Alison Wang <alison.wang at freescale.com>
> > > > ---
> > > >  arch/arm/cpu/armv7/ls102xa/timer.c                | 3 ++-
> > > >  arch/arm/include/asm/arch-ls102xa/immap_ls102xa.h | 2 +-
> > > >  2 files changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/arm/cpu/armv7/ls102xa/timer.c
> > > > b/arch/arm/cpu/armv7/ls102xa/timer.c
> > > > index 11b17b2..e6a32ca 100644
> > > > --- a/arch/arm/cpu/armv7/ls102xa/timer.c
> > > > +++ b/arch/arm/cpu/armv7/ls102xa/timer.c
> > > > @@ -56,7 +56,8 @@ static inline unsigned long long
> > > us_to_tick(unsigned
> > > > long long usec)  int timer_init(void)  {
> > > >         struct sctr_regs *sctr = (struct sctr_regs *)SCTR_BASE_ADDR;
> > > > -       unsigned long ctrl, val, freq;
> > > > +       unsigned long ctrl, freq;
> > > > +       unsigned long long val;
> > > >
> > > >         /* Enable System Counter */
> > > >         writel(SYS_COUNTER_CTRL_ENABLE, &sctr->cntcr); diff --git
> > > > a/arch/arm/include/asm/arch-ls102xa/immap_ls102xa.h
> > > > b/arch/arm/include/asm/arch-ls102xa/immap_ls102xa.h
> > > > index ee547fb..34854da 100644
> > > > --- a/arch/arm/include/asm/arch-ls102xa/immap_ls102xa.h
> > > > +++ b/arch/arm/include/asm/arch-ls102xa/immap_ls102xa.h
> > > > @@ -31,7 +31,7 @@
> > > >  #define RCWSR4_SRDS1_PRTCL_SHIFT       24
> > > >  #define RCWSR4_SRDS1_PRTCL_MASK                0xff000000
> > > >
> > > > -#define TIMER_COMP_VAL                 0xffffffff
> > > > +#define TIMER_COMP_VAL                 0xffffffffffffffffull
> > > >  #define ARCH_TIMER_CTRL_ENABLE         (1 << 0)
> > > >  #define SYS_COUNTER_CTRL_ENABLE                (1 << 24)
> > > >
> > > > --
> > > > 2.1.0.27.g96db324
> > > >
> > > > _______________________________________________
> > > > U-Boot mailing list
> > > > U-Boot at lists.denx.de
> > > > http://lists.denx.de/mailman/listinfo/u-boot
> > > >
>