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

[Michal Simek]

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


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian