[U-Boot] v2015.10-rc2: timer not initialized on Kirkwood

Hi Simon,

On 02.09.2015 17:49, Simon Guinot wrote:
> While testing U-Boot v2015.10-rc2 on the Kirkwood-based LaCie boards
> I noticed that the autoboot counter is not decrementing. It stays stuck
> at '3' endlessly. After some digging, I found out that this regression
> is due to the commit: ade741b3896b1a3872ff74437f04b50762d05849
> "arm: mvebu: Call timer_init early before PHY and DDR init".

Sorry about that. Unfortunately I don't have a Kirkwood target here. So 
I can't test any of the MVEBU changes on this platform.

> With this commit it appears that the timer initialization is skipped
> on Kirkwood boards. As a consequence the timer is not ticking and then
> all the features relying on the timer are most likely broken.
>
> On the Kirkwood boards, the timer_init function is only called from
> from the ARM init_sequence. SPL support is disabled. The problem is
> that the patch introduces a static init_done variable (to prevent
> multiple timer initializations). But while debugging the timer_init
> function (via JTAG), I noticed that the init_done initial value is not
> zero. So the function exists without initializing the timer. A possible
> explanation is that timer_init is called before the U-Boot relocation,
> when the BSS segment is still not available...

Hmmm. I thought that BSS should be available and cleared at that time. 
But this does not seem to be the case.

> Maybe we should use an initialized variable instead ?

Could you please test the small attached patch? If this fixes the 
problem for you on your Kirkwood board?

If yes, I'll submit it officially to the list for inclusion.

Thanks,
Stefan

Hi Stefan,

On Thu, Sep 03, 2015 at 07:28:01AM +0200, Stefan Roese wrote:
> Hi Simon,
> 
> On 02.09.2015 17:49, Simon Guinot wrote:
> >While testing U-Boot v2015.10-rc2 on the Kirkwood-based LaCie boards
> >I noticed that the autoboot counter is not decrementing. It stays stuck
> >at '3' endlessly. After some digging, I found out that this regression
> >is due to the commit: ade741b3896b1a3872ff74437f04b50762d05849
> >"arm: mvebu: Call timer_init early before PHY and DDR init".
> 
> Sorry about that. Unfortunately I don't have a Kirkwood target here.
> So I can't test any of the MVEBU changes on this platform.
> 
> >With this commit it appears that the timer initialization is skipped
> >on Kirkwood boards. As a consequence the timer is not ticking and then
> >all the features relying on the timer are most likely broken.
> >
> >On the Kirkwood boards, the timer_init function is only called from
> >from the ARM init_sequence. SPL support is disabled. The problem is
> >that the patch introduces a static init_done variable (to prevent
> >multiple timer initializations). But while debugging the timer_init
> >function (via JTAG), I noticed that the init_done initial value is not
> >zero. So the function exists without initializing the timer. A possible
> >explanation is that timer_init is called before the U-Boot relocation,
> >when the BSS segment is still not available...
> 
> Hmmm. I thought that BSS should be available and cleared at that
> time. But this does not seem to be the case.
> 
> >Maybe we should use an initialized variable instead ?
> 
> Could you please test the small attached patch? If this fixes the
> problem for you on your Kirkwood board?

I can confirm that the attached patch does fix the issue.

Thanks for the quick reply.

Simon

> 
> If yes, I'll submit it officially to the list for inclusion.
> 
> Thanks,
> Stefan
> 

> >From 9525d96509e36ae534a15f55afd2eae30f9e000a Mon Sep 17 00:00:00 2001
> From: Stefan Roese <sr@denx.de>
> Date: Thu, 3 Sep 2015 07:22:42 +0200
> Subject: [PATCH] arm: mvebu: timer.c: Explicitly move "init_done" var to data
>  section
> 
> As reported by Simon Guinot, commit ade741b3
> "arm: mvebu: Call timer_init early before PHY and DDR init" breaks
> Kirkwood platforms. As the static variable "init_done" is not
> available at that early boot time. This patch moves it to explicitly
> to the data section, making it available at that time.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Reported-by: Simon Guinot <simon.guinot@sequanux.org>
> Cc: Luka Perkov <luka.perkov@sartura.hr>
> ---
>  arch/arm/mach-mvebu/timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-mvebu/timer.c b/arch/arm/mach-mvebu/timer.c
> index c516c41..5449a89 100644
> --- a/arch/arm/mach-mvebu/timer.c
> +++ b/arch/arm/mach-mvebu/timer.c
> @@ -41,7 +41,7 @@
>  #define timestamp			gd->arch.tbl
>  #define lastdec				gd->arch.lastinc
>  
> -static int init_done;
> +static int init_done __attribute__((section(".data"))) = 0;
>  
>  /* Timer reload and current value registers */
>  struct kwtmr_val {
> -- 
> 2.5.1
>