From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephan Linz <linz@li-pro.net>
Date: Tue, 10 Jul 2012 20:36:13 +0200
Subject: [U-Boot] [PATCH 08/12] microblaze: timer: Prepare for
 device-tree initialization
In-Reply-To: <4FFBE4D9.1000809@monstr.eu>
References: <1341825639-23475-1-git-send-email-monstr@monstr.eu>
	<1341825639-23475-8-git-send-email-monstr@monstr.eu>
	<1341860778.3219.64.camel@keto>  <4FFBE4D9.1000809@monstr.eu>
Message-ID: <1341945373.4010.64.camel@keto>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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