Patchwork [U-Boot,08/12] microblaze: timer: Prepare for device-tree initialization

login
register
mail settings
Submitter Michal Simek
Date July 9, 2012, 9:20 a.m.
Message ID <1341825639-23475-8-git-send-email-monstr@monstr.eu>
Download mbox | patch
Permalink /patch/169725/
State Accepted
Delegated to: Michal Simek
Headers show

Comments

Michal Simek - July 9, 2012, 9:20 a.m.
microblaze: Fix CONFIG_SYS_HZ usage in board config

Do not use hardcoded value. Use CONFIG_SYS_HZ instead.
Separate static configuration to single block.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---
 arch/microblaze/cpu/timer.c                    |   69 ++++++++++++-----------
 arch/microblaze/include/asm/microblaze_timer.h |    3 +
 arch/microblaze/lib/board.c                    |    5 --
 include/configs/microblaze-generic.h           |   12 +----
 4 files changed, 41 insertions(+), 48 deletions(-)
Stephan Linz - July 9, 2012, 6:26 p.m.
Hi Michal,

Am Montag, den 09.07.2012, 11:20 +0200 schrieb Michal Simek: 
> microblaze: Fix CONFIG_SYS_HZ usage in board config
> 
> Do not use hardcoded value. Use CONFIG_SYS_HZ instead.
> Separate static configuration to single block.
> 
> Signed-off-by: Michal Simek <monstr@monstr.eu>
> ---
>  arch/microblaze/cpu/timer.c                    |   69 ++++++++++++-----------
>  arch/microblaze/include/asm/microblaze_timer.h |    3 +
>  arch/microblaze/lib/board.c                    |    5 --
>  include/configs/microblaze-generic.h           |   12 +----
>  4 files changed, 41 insertions(+), 48 deletions(-)
> 
> diff --git a/arch/microblaze/cpu/timer.c b/arch/microblaze/cpu/timer.c
> index cc6b897..dfaaaf5 100644
> --- a/arch/microblaze/cpu/timer.c
> +++ b/arch/microblaze/cpu/timer.c
> @@ -27,42 +27,30 @@
>  #include <asm/microblaze_intc.h>
>  
>  volatile int timestamp = 0;
> +microblaze_timer_t *tmr;
>  
> -#ifdef CONFIG_SYS_TIMER_0
>  ulong get_timer (ulong base)
>  {
> -	return (timestamp - base);
> +	if (tmr)
> +		return timestamp - base;
> +	return timestamp++ - base;
>  }
> -#else
> -ulong get_timer (ulong base)
> -{
> -	return (timestamp++ - base);
> -}
> -#endif
>  
> -#ifdef CONFIG_SYS_TIMER_0
>  void __udelay(unsigned long usec)
>  {
> -	int i;
> +	u32 i;
>  
> -	i = get_timer(0);
> -	while ((get_timer(0) - i) < (usec / 1000))
> -		;
> +	if (tmr) {
> +		i = get_timer(0);
> +		while ((get_timer(0) - i) < (usec / 1000))
> +			;
> +	} else {
> +		for (i = 0; i < (usec * XILINX_CLOCK_FREQ / 10000000); i++)
> +			;
> +	}
>  }
> -#else
> -void __udelay(unsigned long usec)
> -{
> -	unsigned int i;
>  
> -	for (i = 0; i < (usec * CONFIG_XILINX_CLOCK_FREQ / 10000000); i++)
> -		;
> -}
> -#endif
> -
> -#ifdef CONFIG_SYS_TIMER_0
> -microblaze_timer_t *tmr = (microblaze_timer_t *) (CONFIG_SYS_TIMER_0_ADDR);
> -
> -void timer_isr (void *arg)
> +static void timer_isr(void *arg)
>  {
>  	timestamp++;
>  	tmr->control = tmr->control | TIMER_INTERRUPT;
> @@ -70,15 +58,30 @@ void timer_isr (void *arg)
>  
>  int timer_init (void)
>  {
> -	tmr->loadreg = CONFIG_SYS_TIMER_0_PRELOAD;
> -	tmr->control = TIMER_INTERRUPT | TIMER_RESET;
> -	tmr->control =
> -	    TIMER_ENABLE | TIMER_ENABLE_INTR | TIMER_RELOAD | TIMER_DOWN_COUNT;
> -	timestamp = 0;
> -	install_interrupt_handler (CONFIG_SYS_TIMER_0_IRQ, timer_isr, (void *)tmr);
> +	u32 irq = 0;

should be   int irq = -1;   (see below)

> +	u32 preload = 0;
> +	u32 ret = 0;
> +
> +#if defined(CONFIG_SYS_TIMER_0_ADDR) && defined(CONFIG_SYS_INTC_0_NUM)
> +	preload = XILINX_CLOCK_FREQ / CONFIG_SYS_HZ;
> +	irq = CONFIG_SYS_TIMER_0_IRQ;
> +	tmr = (microblaze_timer_t *) (CONFIG_SYS_TIMER_0_ADDR);
> +#endif
> +
> +	if (tmr && irq && preload) {

it should be:

if (tmr && irq >= 0 && preload) {

irq valid --> irq >= 0
irq invalid --> irq == -1


br,
Stephan

> +		tmr->loadreg = preload;
> +		tmr->control = TIMER_INTERRUPT | TIMER_RESET;
> +		tmr->control = TIMER_ENABLE | TIMER_ENABLE_INTR |\
> +					TIMER_RELOAD | TIMER_DOWN_COUNT;
> +		timestamp = 0;
> +		ret = install_interrupt_handler (irq, timer_isr, (void *)tmr);
> +		if (ret)
> +			tmr = NULL;
> +	}
> +
> +	/* No problem if timer is not found/initialized */
>  	return 0;
>  }
> -#endif
>  
>  /*
>   * This function is derived from PowerPC code (read timebase as long long).
> diff --git a/arch/microblaze/include/asm/microblaze_timer.h b/arch/microblaze/include/asm/microblaze_timer.h
> index 844c8db..28e8b02 100644
> --- a/arch/microblaze/include/asm/microblaze_timer.h
> +++ b/arch/microblaze/include/asm/microblaze_timer.h
> @@ -39,3 +39,6 @@ typedef volatile struct microblaze_timer_t {
>  	int loadreg; /* load register TLR */
>  	int counter; /* timer/counter register */
>  } microblaze_timer_t;
> +
> +int timer_init(void);
> +
> diff --git a/arch/microblaze/lib/board.c b/arch/microblaze/lib/board.c
> index 8cf4266..ddbc862 100644
> --- a/arch/microblaze/lib/board.c
> +++ b/arch/microblaze/lib/board.c
> @@ -40,9 +40,6 @@ DECLARE_GLOBAL_DATA_PTR;
>  #ifdef CONFIG_SYS_GPIO_0
>  extern int gpio_init (void);
>  #endif
> -#ifdef CONFIG_SYS_TIMER_0
> -extern int timer_init (void);
> -#endif
>  #ifdef CONFIG_SYS_FSL_2
>  extern void fsl_init2 (void);
>  #endif
> @@ -72,9 +69,7 @@ init_fnc_t *init_sequence[] = {
>  	gpio_init,
>  #endif
>  	interrupts_init,
> -#ifdef CONFIG_SYS_TIMER_0
>  	timer_init,
> -#endif
>  #ifdef CONFIG_SYS_FSL_2
>  	fsl_init2,
>  #endif
> diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h
> index 44934eb..9f90107 100644
> --- a/include/configs/microblaze-generic.h
> +++ b/include/configs/microblaze-generic.h
> @@ -102,19 +102,11 @@
>  #endif
>  
>  /* timer */
> -#ifdef XILINX_TIMER_BASEADDR
> -# if (XILINX_TIMER_IRQ != -1)
> -#  define CONFIG_SYS_TIMER_0		1
> +#if defined(XILINX_TIMER_BASEADD) && defined(XILINX_TIMER_IRQ)
>  #  define CONFIG_SYS_TIMER_0_ADDR	XILINX_TIMER_BASEADDR
>  #  define CONFIG_SYS_TIMER_0_IRQ	XILINX_TIMER_IRQ
> -#  define FREQUENCE	XILINX_CLOCK_FREQ
> -#  define CONFIG_SYS_TIMER_0_PRELOAD	( FREQUENCE/1000 )
> -# endif
> -#elif XILINX_CLOCK_FREQ
> -# define CONFIG_XILINX_CLOCK_FREQ	XILINX_CLOCK_FREQ
> -#else
> -# error BAD CLOCK FREQ
>  #endif
> +
>  /* FSL */
>  /* #define	CONFIG_SYS_FSL_2 */
>  /* #define	FSL_INTR_2	1 */
Stephan Linz - July 9, 2012, 7:06 p.m.
Am Montag, den 09.07.2012, 11:20 +0200 schrieb Michal Simek: 
> microblaze: Fix CONFIG_SYS_HZ usage in board config
> 
> Do not use hardcoded value. Use CONFIG_SYS_HZ instead.
> Separate static configuration to single block.
> 
> Signed-off-by: Michal Simek <monstr@monstr.eu>
> ---
>  arch/microblaze/cpu/timer.c                    |   69 ++++++++++++-----------
>  arch/microblaze/include/asm/microblaze_timer.h |    3 +
>  arch/microblaze/lib/board.c                    |    5 --
>  include/configs/microblaze-generic.h           |   12 +----
>  4 files changed, 41 insertions(+), 48 deletions(-)
> 
> diff --git a/arch/microblaze/cpu/timer.c b/arch/microblaze/cpu/timer.c
> index cc6b897..dfaaaf5 100644
> --- a/arch/microblaze/cpu/timer.c
> +++ b/arch/microblaze/cpu/timer.c
> @@ -27,42 +27,30 @@
>  #include <asm/microblaze_intc.h>
>  
>  volatile int timestamp = 0;
> +microblaze_timer_t *tmr;
>  
> -#ifdef CONFIG_SYS_TIMER_0
>  ulong get_timer (ulong base)
>  {
> -	return (timestamp - base);
> +	if (tmr)
> +		return timestamp - base;
> +	return timestamp++ - base;
>  }
> -#else
> -ulong get_timer (ulong base)
> -{
> -	return (timestamp++ - base);
> -}
> -#endif
>  
> -#ifdef CONFIG_SYS_TIMER_0
>  void __udelay(unsigned long usec)
>  {
> -	int i;
> +	u32 i;
>  
> -	i = get_timer(0);
> -	while ((get_timer(0) - i) < (usec / 1000))
> -		;
> +	if (tmr) {
> +		i = get_timer(0);
> +		while ((get_timer(0) - i) < (usec / 1000))
> +			;

Hi Michal,

> +	} else {
> +		for (i = 0; i < (usec * XILINX_CLOCK_FREQ / 10000000); i++)
> +			;

this part should be enclosed by   #ifdef XILINX_CLOCK_FREQ


br,
Stephan Linz

> +	}
>  }
> -#else
> -void __udelay(unsigned long usec)
> -{
> -	unsigned int i;
>  
> -	for (i = 0; i < (usec * CONFIG_XILINX_CLOCK_FREQ / 10000000); i++)
> -		;
> -}
> -#endif
> -
> -#ifdef CONFIG_SYS_TIMER_0
> -microblaze_timer_t *tmr = (microblaze_timer_t *) (CONFIG_SYS_TIMER_0_ADDR);
> -
> -void timer_isr (void *arg)
> +static void timer_isr(void *arg)
>  {
>  	timestamp++;
>  	tmr->control = tmr->control | TIMER_INTERRUPT;
> @@ -70,15 +58,30 @@ void timer_isr (void *arg)
>  
>  int timer_init (void)
>  {
> -	tmr->loadreg = CONFIG_SYS_TIMER_0_PRELOAD;
> -	tmr->control = TIMER_INTERRUPT | TIMER_RESET;
> -	tmr->control =
> -	    TIMER_ENABLE | TIMER_ENABLE_INTR | TIMER_RELOAD | TIMER_DOWN_COUNT;
> -	timestamp = 0;
> -	install_interrupt_handler (CONFIG_SYS_TIMER_0_IRQ, timer_isr, (void *)tmr);
> +	u32 irq = 0;
> +	u32 preload = 0;
> +	u32 ret = 0;
> +
> +#if defined(CONFIG_SYS_TIMER_0_ADDR) && defined(CONFIG_SYS_INTC_0_NUM)
> +	preload = XILINX_CLOCK_FREQ / CONFIG_SYS_HZ;
> +	irq = CONFIG_SYS_TIMER_0_IRQ;
> +	tmr = (microblaze_timer_t *) (CONFIG_SYS_TIMER_0_ADDR);
> +#endif
> +
> +	if (tmr && irq && preload) {
> +		tmr->loadreg = preload;
> +		tmr->control = TIMER_INTERRUPT | TIMER_RESET;
> +		tmr->control = TIMER_ENABLE | TIMER_ENABLE_INTR |\
> +					TIMER_RELOAD | TIMER_DOWN_COUNT;
> +		timestamp = 0;
> +		ret = install_interrupt_handler (irq, timer_isr, (void *)tmr);
> +		if (ret)
> +			tmr = NULL;
> +	}
> +
> +	/* No problem if timer is not found/initialized */
>  	return 0;
>  }
> -#endif
>  
>  /*
>   * This function is derived from PowerPC code (read timebase as long long).
> diff --git a/arch/microblaze/include/asm/microblaze_timer.h b/arch/microblaze/include/asm/microblaze_timer.h
> index 844c8db..28e8b02 100644
> --- a/arch/microblaze/include/asm/microblaze_timer.h
> +++ b/arch/microblaze/include/asm/microblaze_timer.h
> @@ -39,3 +39,6 @@ typedef volatile struct microblaze_timer_t {
>  	int loadreg; /* load register TLR */
>  	int counter; /* timer/counter register */
>  } microblaze_timer_t;
> +
> +int timer_init(void);
> +
> diff --git a/arch/microblaze/lib/board.c b/arch/microblaze/lib/board.c
> index 8cf4266..ddbc862 100644
> --- a/arch/microblaze/lib/board.c
> +++ b/arch/microblaze/lib/board.c
> @@ -40,9 +40,6 @@ DECLARE_GLOBAL_DATA_PTR;
>  #ifdef CONFIG_SYS_GPIO_0
>  extern int gpio_init (void);
>  #endif
> -#ifdef CONFIG_SYS_TIMER_0
> -extern int timer_init (void);
> -#endif
>  #ifdef CONFIG_SYS_FSL_2
>  extern void fsl_init2 (void);
>  #endif
> @@ -72,9 +69,7 @@ init_fnc_t *init_sequence[] = {
>  	gpio_init,
>  #endif
>  	interrupts_init,
> -#ifdef CONFIG_SYS_TIMER_0
>  	timer_init,
> -#endif
>  #ifdef CONFIG_SYS_FSL_2
>  	fsl_init2,
>  #endif
> diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h
> index 44934eb..9f90107 100644
> --- a/include/configs/microblaze-generic.h
> +++ b/include/configs/microblaze-generic.h
> @@ -102,19 +102,11 @@
>  #endif
>  
>  /* timer */
> -#ifdef XILINX_TIMER_BASEADDR
> -# if (XILINX_TIMER_IRQ != -1)
> -#  define CONFIG_SYS_TIMER_0		1
> +#if defined(XILINX_TIMER_BASEADD) && defined(XILINX_TIMER_IRQ)
>  #  define CONFIG_SYS_TIMER_0_ADDR	XILINX_TIMER_BASEADDR
>  #  define CONFIG_SYS_TIMER_0_IRQ	XILINX_TIMER_IRQ
> -#  define FREQUENCE	XILINX_CLOCK_FREQ
> -#  define CONFIG_SYS_TIMER_0_PRELOAD	( FREQUENCE/1000 )
> -# endif
> -#elif XILINX_CLOCK_FREQ
> -# define CONFIG_XILINX_CLOCK_FREQ	XILINX_CLOCK_FREQ
> -#else
> -# error BAD CLOCK FREQ
>  #endif
> +
>  /* FSL */
>  /* #define	CONFIG_SYS_FSL_2 */
>  /* #define	FSL_INTR_2	1 */
Michal Simek - July 10, 2012, 8:11 a.m.
On 07/09/2012 08:26 PM, Stephan Linz wrote:
> Hi Michal,
>
> Am Montag, den 09.07.2012, 11:20 +0200 schrieb Michal Simek:
>> microblaze: Fix CONFIG_SYS_HZ usage in board config
>>
>> Do not use hardcoded value. Use CONFIG_SYS_HZ instead.
>> Separate static configuration to single block.
>>
>> Signed-off-by: Michal Simek<monstr@monstr.eu>
>> ---
>>   arch/microblaze/cpu/timer.c                    |   69 ++++++++++++-----------
>>   arch/microblaze/include/asm/microblaze_timer.h |    3 +
>>   arch/microblaze/lib/board.c                    |    5 --
>>   include/configs/microblaze-generic.h           |   12 +----
>>   4 files changed, 41 insertions(+), 48 deletions(-)
>>
>> diff --git a/arch/microblaze/cpu/timer.c b/arch/microblaze/cpu/timer.c
>> index cc6b897..dfaaaf5 100644
>> --- a/arch/microblaze/cpu/timer.c
>> +++ b/arch/microblaze/cpu/timer.c
>> @@ -27,42 +27,30 @@
>>   #include<asm/microblaze_intc.h>
>>
>>   volatile int timestamp = 0;
>> +microblaze_timer_t *tmr;
>>
>> -#ifdef CONFIG_SYS_TIMER_0
>>   ulong get_timer (ulong base)
>>   {
>> -	return (timestamp - base);
>> +	if (tmr)
>> +		return timestamp - base;
>> +	return timestamp++ - base;
>>   }
>> -#else
>> -ulong get_timer (ulong base)
>> -{
>> -	return (timestamp++ - base);
>> -}
>> -#endif
>>
>> -#ifdef CONFIG_SYS_TIMER_0
>>   void __udelay(unsigned long usec)
>>   {
>> -	int i;
>> +	u32 i;
>>
>> -	i = get_timer(0);
>> -	while ((get_timer(0) - i)<  (usec / 1000))
>> -		;
>> +	if (tmr) {
>> +		i = get_timer(0);
>> +		while ((get_timer(0) - i)<  (usec / 1000))
>> +			;
>> +	} else {
>> +		for (i = 0; i<  (usec * XILINX_CLOCK_FREQ / 10000000); i++)
>> +			;
>> +	}
>>   }
>> -#else
>> -void __udelay(unsigned long usec)
>> -{
>> -	unsigned int i;
>>
>> -	for (i = 0; i<  (usec * CONFIG_XILINX_CLOCK_FREQ / 10000000); i++)
>> -		;
>> -}
>> -#endif
>> -
>> -#ifdef CONFIG_SYS_TIMER_0
>> -microblaze_timer_t *tmr = (microblaze_timer_t *) (CONFIG_SYS_TIMER_0_ADDR);
>> -
>> -void timer_isr (void *arg)
>> +static void timer_isr(void *arg)
>>   {
>>   	timestamp++;
>>   	tmr->control = tmr->control | TIMER_INTERRUPT;
>> @@ -70,15 +58,30 @@ void timer_isr (void *arg)
>>
>>   int timer_init (void)
>>   {
>> -	tmr->loadreg = CONFIG_SYS_TIMER_0_PRELOAD;
>> -	tmr->control = TIMER_INTERRUPT | TIMER_RESET;
>> -	tmr->control =
>> -	    TIMER_ENABLE | TIMER_ENABLE_INTR | TIMER_RELOAD | TIMER_DOWN_COUNT;
>> -	timestamp = 0;
>> -	install_interrupt_handler (CONFIG_SYS_TIMER_0_IRQ, timer_isr, (void *)tmr);
>> +	u32 irq = 0;
>
> should be   int irq = -1;   (see below)
>
>> +	u32 preload = 0;
>> +	u32 ret = 0;
>> +
>> +#if defined(CONFIG_SYS_TIMER_0_ADDR)&&  defined(CONFIG_SYS_INTC_0_NUM)
>> +	preload = XILINX_CLOCK_FREQ / CONFIG_SYS_HZ;
>> +	irq = CONFIG_SYS_TIMER_0_IRQ;
>> +	tmr = (microblaze_timer_t *) (CONFIG_SYS_TIMER_0_ADDR);
>> +#endif
>> +
>> +	if (tmr&&  irq&&  preload) {
>
> it should be:
>
> if (tmr&&  irq>= 0&&  preload) {
>
> irq valid -->  irq>= 0
> irq invalid -->  irq == -1


Correct - will be in the v2.

Thanks,
Michal
Michal Simek - July 10, 2012, 8:16 a.m.
On 07/09/2012 09:06 PM, Stephan Linz wrote:
> Am Montag, den 09.07.2012, 11:20 +0200 schrieb Michal Simek:
>> microblaze: Fix CONFIG_SYS_HZ usage in board config
>>
>> Do not use hardcoded value. Use CONFIG_SYS_HZ instead.
>> Separate static configuration to single block.
>>
>> Signed-off-by: Michal Simek<monstr@monstr.eu>
>> ---
>>   arch/microblaze/cpu/timer.c                    |   69 ++++++++++++-----------
>>   arch/microblaze/include/asm/microblaze_timer.h |    3 +
>>   arch/microblaze/lib/board.c                    |    5 --
>>   include/configs/microblaze-generic.h           |   12 +----
>>   4 files changed, 41 insertions(+), 48 deletions(-)
>>
>> diff --git a/arch/microblaze/cpu/timer.c b/arch/microblaze/cpu/timer.c
>> index cc6b897..dfaaaf5 100644
>> --- a/arch/microblaze/cpu/timer.c
>> +++ b/arch/microblaze/cpu/timer.c
>> @@ -27,42 +27,30 @@
>>   #include<asm/microblaze_intc.h>
>>
>>   volatile int timestamp = 0;
>> +microblaze_timer_t *tmr;
>>
>> -#ifdef CONFIG_SYS_TIMER_0
>>   ulong get_timer (ulong base)
>>   {
>> -	return (timestamp - base);
>> +	if (tmr)
>> +		return timestamp - base;
>> +	return timestamp++ - base;
>>   }
>> -#else
>> -ulong get_timer (ulong base)
>> -{
>> -	return (timestamp++ - base);
>> -}
>> -#endif
>>
>> -#ifdef CONFIG_SYS_TIMER_0
>>   void __udelay(unsigned long usec)
>>   {
>> -	int i;
>> +	u32 i;
>>
>> -	i = get_timer(0);
>> -	while ((get_timer(0) - i)<  (usec / 1000))
>> -		;
>> +	if (tmr) {
>> +		i = get_timer(0);
>> +		while ((get_timer(0) - i)<  (usec / 1000))
>> +			;
>
> Hi Michal,
>
>> +	} else {
>> +		for (i = 0; i<  (usec * XILINX_CLOCK_FREQ / 10000000); i++)
>> +			;
>
> this part should be enclosed by   #ifdef XILINX_CLOCK_FREQ

It was intentional because XILINX_CLOCK_FREQ must be define.
Maybe it could be handle by

#ifndef XILINX_CLOCK_FREQ
# error Please define XILINX_CLOCK_FREQ
#endif

Thanks,
Michal
Stephan Linz - July 10, 2012, 6:36 p.m.
Am Dienstag, den 10.07.2012, 10:16 +0200 schrieb Michal Simek: 
> On 07/09/2012 09:06 PM, Stephan Linz wrote:
> > Am Montag, den 09.07.2012, 11:20 +0200 schrieb Michal Simek:
> >> microblaze: Fix CONFIG_SYS_HZ usage in board config
> >>
> >> Do not use hardcoded value. Use CONFIG_SYS_HZ instead.
> >> Separate static configuration to single block.
> >>
> >> Signed-off-by: Michal Simek<monstr@monstr.eu>
> >> ---
> >>   arch/microblaze/cpu/timer.c                    |   69 ++++++++++++-----------
> >>   arch/microblaze/include/asm/microblaze_timer.h |    3 +
> >>   arch/microblaze/lib/board.c                    |    5 --
> >>   include/configs/microblaze-generic.h           |   12 +----
> >>   4 files changed, 41 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/arch/microblaze/cpu/timer.c b/arch/microblaze/cpu/timer.c
> >> index cc6b897..dfaaaf5 100644
> >> --- a/arch/microblaze/cpu/timer.c
> >> +++ b/arch/microblaze/cpu/timer.c
> >> @@ -27,42 +27,30 @@
> >>   #include<asm/microblaze_intc.h>
> >>
> >>   volatile int timestamp = 0;
> >> +microblaze_timer_t *tmr;
> >>
> >> -#ifdef CONFIG_SYS_TIMER_0
> >>   ulong get_timer (ulong base)
> >>   {
> >> -	return (timestamp - base);
> >> +	if (tmr)
> >> +		return timestamp - base;
> >> +	return timestamp++ - base;
> >>   }
> >> -#else
> >> -ulong get_timer (ulong base)
> >> -{
> >> -	return (timestamp++ - base);
> >> -}
> >> -#endif
> >>
> >> -#ifdef CONFIG_SYS_TIMER_0
> >>   void __udelay(unsigned long usec)
> >>   {
> >> -	int i;
> >> +	u32 i;
> >>
> >> -	i = get_timer(0);
> >> -	while ((get_timer(0) - i)<  (usec / 1000))
> >> -		;
> >> +	if (tmr) {
> >> +		i = get_timer(0);
> >> +		while ((get_timer(0) - i)<  (usec / 1000))
> >> +			;
> >
> > Hi Michal,
> >
> >> +	} else {
> >> +		for (i = 0; i<  (usec * XILINX_CLOCK_FREQ / 10000000); i++)
> >> +			;
> >
> > this part should be enclosed by   #ifdef XILINX_CLOCK_FREQ
> 
> It was intentional because XILINX_CLOCK_FREQ must be define.

Hm, it's a catch-22 situation in between compilation and run time.

With a prober fdt setup you will implicitly fetch this value from dts
(as tmr->loadreg) and you will never reach this code. In the case of a
wrong dts content, for example a missing "clock-frequency" entry, this
code snipped could be a fall back solution. But what is wrong and what
is right?

I think you should make a hard cut here and make a strict distinguish
between the old (undef CONFIG_OF_CONTROL) and the new timer setup. Your
current code base leads to a hardware description with overdetermined
definition of timer clock frequency:

first one here: board/xilinx/microblaze-generic/xparameters.h
second one here: board/xilinx/microblaze-generic/dts/microblaze.dts


> Maybe it could be handle by
> 
> #ifndef XILINX_CLOCK_FREQ
> # error Please define XILINX_CLOCK_FREQ
> #endif

better would be:

#if !defined(CONFIG_OF_CONTROL) && !defined(XILINX_CLOCK_FREQ)
# error Please define XILINX_CLOCK_FREQ
#endif


br,
Stephan
Michal Simek - July 11, 2012, 6:07 a.m.
On 07/10/2012 08:36 PM, Stephan Linz wrote:
> Am Dienstag, den 10.07.2012, 10:16 +0200 schrieb Michal Simek:
>> On 07/09/2012 09:06 PM, Stephan Linz wrote:
>>> Am Montag, den 09.07.2012, 11:20 +0200 schrieb Michal Simek:
>>>> microblaze: Fix CONFIG_SYS_HZ usage in board config
>>>>
>>>> Do not use hardcoded value. Use CONFIG_SYS_HZ instead.
>>>> Separate static configuration to single block.
>>>>
>>>> Signed-off-by: Michal Simek<monstr@monstr.eu>
>>>> ---
>>>>    arch/microblaze/cpu/timer.c                    |   69 ++++++++++++-----------
>>>>    arch/microblaze/include/asm/microblaze_timer.h |    3 +
>>>>    arch/microblaze/lib/board.c                    |    5 --
>>>>    include/configs/microblaze-generic.h           |   12 +----
>>>>    4 files changed, 41 insertions(+), 48 deletions(-)
>>>>
>>>> diff --git a/arch/microblaze/cpu/timer.c b/arch/microblaze/cpu/timer.c
>>>> index cc6b897..dfaaaf5 100644
>>>> --- a/arch/microblaze/cpu/timer.c
>>>> +++ b/arch/microblaze/cpu/timer.c
>>>> @@ -27,42 +27,30 @@
>>>>    #include<asm/microblaze_intc.h>
>>>>
>>>>    volatile int timestamp = 0;
>>>> +microblaze_timer_t *tmr;
>>>>
>>>> -#ifdef CONFIG_SYS_TIMER_0
>>>>    ulong get_timer (ulong base)
>>>>    {
>>>> -	return (timestamp - base);
>>>> +	if (tmr)
>>>> +		return timestamp - base;
>>>> +	return timestamp++ - base;
>>>>    }
>>>> -#else
>>>> -ulong get_timer (ulong base)
>>>> -{
>>>> -	return (timestamp++ - base);
>>>> -}
>>>> -#endif
>>>>
>>>> -#ifdef CONFIG_SYS_TIMER_0
>>>>    void __udelay(unsigned long usec)
>>>>    {
>>>> -	int i;
>>>> +	u32 i;
>>>>
>>>> -	i = get_timer(0);
>>>> -	while ((get_timer(0) - i)<   (usec / 1000))
>>>> -		;
>>>> +	if (tmr) {
>>>> +		i = get_timer(0);
>>>> +		while ((get_timer(0) - i)<   (usec / 1000))
>>>> +			;
>>>
>>> Hi Michal,
>>>
>>>> +	} else {
>>>> +		for (i = 0; i<   (usec * XILINX_CLOCK_FREQ / 10000000); i++)
>>>> +			;
>>>
>>> this part should be enclosed by   #ifdef XILINX_CLOCK_FREQ
>>
>> It was intentional because XILINX_CLOCK_FREQ must be define.
>
> Hm, it's a catch-22 situation in between compilation and run time.
>
> With a prober fdt setup you will implicitly fetch this value from dts
> (as tmr->loadreg) and you will never reach this code. In the case of a
> wrong dts content, for example a missing "clock-frequency" entry, this
> code snipped could be a fall back solution. But what is wrong and what
> is right?

Correct - u-boot is able to work without timer and it is fall back solution.
My point was that XILINX_CLOCK_FREQ will be setup to any default value
just for sure.

>
> I think you should make a hard cut here and make a strict distinguish
> between the old (undef CONFIG_OF_CONTROL) and the new timer setup. Your
> current code base leads to a hardware description with overdetermined
> definition of timer clock frequency:

Not going to do hard cut. We can do it in future that we will use only
DTS initialization but it is long way to go.


>
> first one here: board/xilinx/microblaze-generic/xparameters.h

It should be define here for non FDT platform
and my preference is completely remove xparameters for FDT platform.

> second one here: board/xilinx/microblaze-generic/dts/microblaze.dts

For FDT platform with timer connected to intc.

>
>
>> Maybe it could be handle by
>>
>> #ifndef XILINX_CLOCK_FREQ
>> # error Please define XILINX_CLOCK_FREQ
>> #endif
>
> better would be:
>
> #if !defined(CONFIG_OF_CONTROL)&&  !defined(XILINX_CLOCK_FREQ)
> # error Please define XILINX_CLOCK_FREQ
> #endif

Still make sense to define default value for CONFIG_OF_CONTROL
for that fall back solution which should be there.

There is also one more case for system which contains several timers
where some of them have IRQ connected and some not.
This parsing just read the first timer with selected compatible property.
But this is different topic.

Thanks,
Michal

Patch

diff --git a/arch/microblaze/cpu/timer.c b/arch/microblaze/cpu/timer.c
index cc6b897..dfaaaf5 100644
--- a/arch/microblaze/cpu/timer.c
+++ b/arch/microblaze/cpu/timer.c
@@ -27,42 +27,30 @@ 
 #include <asm/microblaze_intc.h>
 
 volatile int timestamp = 0;
+microblaze_timer_t *tmr;
 
-#ifdef CONFIG_SYS_TIMER_0
 ulong get_timer (ulong base)
 {
-	return (timestamp - base);
+	if (tmr)
+		return timestamp - base;
+	return timestamp++ - base;
 }
-#else
-ulong get_timer (ulong base)
-{
-	return (timestamp++ - base);
-}
-#endif
 
-#ifdef CONFIG_SYS_TIMER_0
 void __udelay(unsigned long usec)
 {
-	int i;
+	u32 i;
 
-	i = get_timer(0);
-	while ((get_timer(0) - i) < (usec / 1000))
-		;
+	if (tmr) {
+		i = get_timer(0);
+		while ((get_timer(0) - i) < (usec / 1000))
+			;
+	} else {
+		for (i = 0; i < (usec * XILINX_CLOCK_FREQ / 10000000); i++)
+			;
+	}
 }
-#else
-void __udelay(unsigned long usec)
-{
-	unsigned int i;
 
-	for (i = 0; i < (usec * CONFIG_XILINX_CLOCK_FREQ / 10000000); i++)
-		;
-}
-#endif
-
-#ifdef CONFIG_SYS_TIMER_0
-microblaze_timer_t *tmr = (microblaze_timer_t *) (CONFIG_SYS_TIMER_0_ADDR);
-
-void timer_isr (void *arg)
+static void timer_isr(void *arg)
 {
 	timestamp++;
 	tmr->control = tmr->control | TIMER_INTERRUPT;
@@ -70,15 +58,30 @@  void timer_isr (void *arg)
 
 int timer_init (void)
 {
-	tmr->loadreg = CONFIG_SYS_TIMER_0_PRELOAD;
-	tmr->control = TIMER_INTERRUPT | TIMER_RESET;
-	tmr->control =
-	    TIMER_ENABLE | TIMER_ENABLE_INTR | TIMER_RELOAD | TIMER_DOWN_COUNT;
-	timestamp = 0;
-	install_interrupt_handler (CONFIG_SYS_TIMER_0_IRQ, timer_isr, (void *)tmr);
+	u32 irq = 0;
+	u32 preload = 0;
+	u32 ret = 0;
+
+#if defined(CONFIG_SYS_TIMER_0_ADDR) && defined(CONFIG_SYS_INTC_0_NUM)
+	preload = XILINX_CLOCK_FREQ / CONFIG_SYS_HZ;
+	irq = CONFIG_SYS_TIMER_0_IRQ;
+	tmr = (microblaze_timer_t *) (CONFIG_SYS_TIMER_0_ADDR);
+#endif
+
+	if (tmr && irq && preload) {
+		tmr->loadreg = preload;
+		tmr->control = TIMER_INTERRUPT | TIMER_RESET;
+		tmr->control = TIMER_ENABLE | TIMER_ENABLE_INTR |\
+					TIMER_RELOAD | TIMER_DOWN_COUNT;
+		timestamp = 0;
+		ret = install_interrupt_handler (irq, timer_isr, (void *)tmr);
+		if (ret)
+			tmr = NULL;
+	}
+
+	/* No problem if timer is not found/initialized */
 	return 0;
 }
-#endif
 
 /*
  * This function is derived from PowerPC code (read timebase as long long).
diff --git a/arch/microblaze/include/asm/microblaze_timer.h b/arch/microblaze/include/asm/microblaze_timer.h
index 844c8db..28e8b02 100644
--- a/arch/microblaze/include/asm/microblaze_timer.h
+++ b/arch/microblaze/include/asm/microblaze_timer.h
@@ -39,3 +39,6 @@  typedef volatile struct microblaze_timer_t {
 	int loadreg; /* load register TLR */
 	int counter; /* timer/counter register */
 } microblaze_timer_t;
+
+int timer_init(void);
+
diff --git a/arch/microblaze/lib/board.c b/arch/microblaze/lib/board.c
index 8cf4266..ddbc862 100644
--- a/arch/microblaze/lib/board.c
+++ b/arch/microblaze/lib/board.c
@@ -40,9 +40,6 @@  DECLARE_GLOBAL_DATA_PTR;
 #ifdef CONFIG_SYS_GPIO_0
 extern int gpio_init (void);
 #endif
-#ifdef CONFIG_SYS_TIMER_0
-extern int timer_init (void);
-#endif
 #ifdef CONFIG_SYS_FSL_2
 extern void fsl_init2 (void);
 #endif
@@ -72,9 +69,7 @@  init_fnc_t *init_sequence[] = {
 	gpio_init,
 #endif
 	interrupts_init,
-#ifdef CONFIG_SYS_TIMER_0
 	timer_init,
-#endif
 #ifdef CONFIG_SYS_FSL_2
 	fsl_init2,
 #endif
diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h
index 44934eb..9f90107 100644
--- a/include/configs/microblaze-generic.h
+++ b/include/configs/microblaze-generic.h
@@ -102,19 +102,11 @@ 
 #endif
 
 /* timer */
-#ifdef XILINX_TIMER_BASEADDR
-# if (XILINX_TIMER_IRQ != -1)
-#  define CONFIG_SYS_TIMER_0		1
+#if defined(XILINX_TIMER_BASEADD) && defined(XILINX_TIMER_IRQ)
 #  define CONFIG_SYS_TIMER_0_ADDR	XILINX_TIMER_BASEADDR
 #  define CONFIG_SYS_TIMER_0_IRQ	XILINX_TIMER_IRQ
-#  define FREQUENCE	XILINX_CLOCK_FREQ
-#  define CONFIG_SYS_TIMER_0_PRELOAD	( FREQUENCE/1000 )
-# endif
-#elif XILINX_CLOCK_FREQ
-# define CONFIG_XILINX_CLOCK_FREQ	XILINX_CLOCK_FREQ
-#else
-# error BAD CLOCK FREQ
 #endif
+
 /* FSL */
 /* #define	CONFIG_SYS_FSL_2 */
 /* #define	FSL_INTR_2	1 */