Re: [RFC PATCH 1/5] arm:omap1/2/3/4:Convert 32k-Sync clocksource driver to platform_driver

[Marc Zyngier <marc.zyngier@arm.com>]

On 18/01/12 14:38, Hiremath, Vaibhav wrote:
> On Wed, Jan 18, 2012 at 19:55:51, Marc Zyngier wrote:
>> On 18/01/12 14:11, Russell King - ARM Linux wrote:
>>> On Wed, Jan 18, 2012 at 01:34:55PM +0000, Hiremath, Vaibhav wrote:
>>>> On Wed, Jan 18, 2012 at 17:29:52, Russell King - ARM Linux wrote:
>>>>> On Wed, Jan 18, 2012 at 04:58:02PM +0530, Vaibhav Hiremath wrote:
>>>>>> Convert counter_32k clocksource driver to standard platform_driver
>>>>>> and move it drivers/clocksource/ directory.
>>>>>>
>>>>>> Also, rename it to more generic name "omap-32k.c".
>>>>>
>>>>> NAK.  sched_clock is supposed to be available early.  Platform device
>>>>> driver initialization is FAR too late.
>>>>>
>>>> Russell,
>>>>
>>>> Sched_clock is available very early during boot sequence. Initially gp-timer 
>>>> (dmtimer) will get registered as a clocksource. Please refer to the file
>>>> mach-omap2/timer.c
>>>>
>>>> 32k_sync timer (omap-32k.c) will come get registered during arch_init.
>>>>
>>>> Just FYI, the way I tested it is, I used kernel parameter 
>>>> "clocksourse=counter-32k", the switch from gp-timer to 32k timer
>>>> will happen once it gets registered.
>>>
>>> So please delete the sched_clock code from the 32k timer stuff you've
>>> moved to a platform driver.  It will cause sched_clock to reset to zero,
>>> and that's bad news.
>>>
>>> Only one sched_clock() should ever be registered, and that should only be
>>> registered very early at boot time.
>>
>> The kernel will WARN if two sched_clock() are registered. I hope this
>> will be enough for people not to persist with such a thing...
>>
> Marc,
> 
> This code-snippet is not registering multiple sched_clock() functions,
> Here we have 2 timers
>   - gp-timer (available on all devices)
>   - 32k-sync timer (may not available on all devices, like AM33xx)
> 
> So we are registering both the timers as a clocksource and kernel
> Chooses one based on kernel-parameter or rating.
> From sched_clock perspective, we have only one function exported in
> mach-omap2/timer.c, which in-turn calls omap-32k.c timer api.
> 
> Something like this,
> 
> 
> unsigned long long notrace sched_clock(void)
> {
> 	U32 cyc;
> 
> 	/* First call 32k-sync timer sched_clock() */
> 	/* return 0 means, the device doesn't have 32k-sync timer available */
> 	cyc = sched_clock_32k();
> 	if (cyc)
> 		return cyc;
> 
> 	/* Fall back to gp-timer approach */
> }
> 
> So there is and will not be any warning from kernel here.

Can't you instead use the ARM sched_clock framework? Please have a look
at what's currently in mainline (we basically register a read() function
with the sched_clock framework).

A per-platform sched_clock() is a step backward for the single zImage
work, and I really want to avoid this.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...