| Message ID | 1341825639-23475-8-git-send-email-monstr@monstr.eu |
|---|---|
| State | Accepted |
| Delegated to: | Michal Simek |
| Headers | show |
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 */
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 */
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
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
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
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
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 */
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(-)