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

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

[Alison Wang]

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.

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/014400.html

Signed-off-by: Chris Kilgour <techie@whiterocker.com>
Signed-off-by: Alison Wang <alison.wang@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] [PATCH] arm: ls1021a: Ensure LS1021 ARM Generic Timer CompareValue Set 64-bit

[Mark Rutland]

Hi,

Isn't this the same patch as a couple of days ago [2], which I replied
to [3]?

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.

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/014400.html
> Signed-off-by: Chris Kilgour <techie@whiterocker.com>
> Signed-off-by: Alison Wang <alison.wang@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
> 

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

[Huan Wang]

Hi, Mark

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland at arm.com]
> Sent: Wednesday, July 15, 2015 5:14 PM
> To: Wang Huan-B18965
> Cc: Sun York-R58495; u-boot at lists.denx.de; Wang Huan-B18965;
> marc.zyngier at arm.com
> Subject: Re: [U-Boot] [PATCH] arm: ls1021a: Ensure LS1021 ARM Generic
> Timer CompareValue Set 64-bit
> 
> Hi,
> 
> Isn't this the same patch as a couple of days ago [2], which I replied
> to [3]?
[Alison Wang] Sorry, I missed that patch. I thought Kilgour would not send the patch.

> 
> 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.
> 
> 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@whiterocker.com>
> > Signed-off-by: Alison Wang <alison.wang@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
> >

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

[Huan Wang]

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? How about removing the following code?

/* Set PL1 Physical Comp Value */
val = TIMER_COMP_VAL;
asm("mcrr p15, 2, %Q0, %R0, c14" : : "r" (val));

> > 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@whiterocker.com>
> > > Signed-off-by: Alison Wang <alison.wang@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
> > >

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

[Marc Zyngier]

Alison,

On 17/07/15 11:01, 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? How about removing the following code?
> 
> /* Set PL1 Physical Comp Value */
> val = TIMER_COMP_VAL;
> asm("mcrr p15, 2, %Q0, %R0, c14" : : "r" (val));

There is two aspects to it:

- if you're not using the timer at all, there is no point writing to the
comparator.

- but whether you're using it or not, it is good practice to turn it off
before jumping into the OS: this OS may run non-secure and will then be
unable to turn the secure timer off.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Mark Rutland]

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.

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

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@whiterocker.com>
> > > > Signed-off-by: Alison Wang <alison.wang@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
> > > >
> 

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

[Huan Wang]

Hi, Marc,

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

There is two aspects to it:

- if you're not using the timer at all, there is no point writing to the
comparator.

- but whether you're using it or not, it is good practice to turn it off
before jumping into the OS: this OS may run non-secure and will then be
unable to turn the secure timer off.

[Alison Wang] Yes, I understand. Thanks for your explanation.

Thanks,

        M.
--
Jazz is not dead. It just smells funny...

[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@whiterocker.com>
> > > > Signed-off-by: Alison Wang <alison.wang@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
> > > >
>

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

[Alexander Stein]

On Wednesday 15 July 2015 15:13:05, 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.
> 
> 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.

initcall_run_list is the function "hiding" or not "hiding" this problem when 
calling timer_init. It is using r4 and r5 for it's loop variables. On gcc-4.8 
and gcc-4.9 the usage of those two registers are switched. So a newer gcc uses 
a slightly different register allocation. While this function is perfectly 
fine, depending on the r4 register timer_init uses a different value for the 
upper 32-bit of CNTP_CVAL resulting in the different behavior.
I've compared two u-boots objdumps showing and not showing this problem which 
_only_ differ in those 2 register usages.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein
SYS TEC electronic GmbH
alexander.stein at systec-electronic.com

Legal and Commercial Address:
Am Windrad 2
08468 Heinsdorfergrund
Germany

Office: +49 (0) 3765 38600-0
Fax:    +49 (0) 3765 38600-4100
 
Managing Directors:
	Director Technology/CEO: Dipl.-Phys. Siegmar Schmidt;
	Director Commercial Affairs/COO: Dipl. Ing. (FH) Armin von Collrepp
Commercial Registry:
	Amtsgericht Chemnitz, HRB 28082; USt.-Id Nr. DE150534010

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

[York Sun]

On 07/15/2015 12:13 AM, 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.
> 
> 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/014400.html
> 
> Signed-off-by: Chris Kilgour <techie@whiterocker.com>
> Signed-off-by: Alison Wang <alison.wang@freescale.com>
> ---

Applied to fsl-qoriq master. Thanks.

York