[PATCH 3/3] ARM: twd_smp: add clock api support

[robherring2@gmail.com (Rob Herring)]

On 09/30/2010 10:27 PM, Colin Cross wrote:
> On Thu, Sep 30, 2010 at 8:14 PM, Rob Herring<robherring2@gmail.com>  wrote:
>> On 09/30/2010 09:30 PM, Colin Cross wrote:
>>>
>>> On Thu, Sep 30, 2010 at 7:03 PM, Rob Herring<robherring2@gmail.com>
>>>   wrote:
>>>>
>>>> On 09/30/2010 07:49 PM, Colin Cross wrote:
>>>>>
>>>>> On Thu, Sep 30, 2010 at 3:49 PM, Rob Herring<robherring2@gmail.com>
>>>>>   wrote:
>>>>>>
>>>>>> From: Rob Herring<rob.herring@smooth-stone.com>
>>>>>>
>>>>>> The private timer freq is currently dynamically detected
>>>>>> using jiffies count to determine the rate. This method adds
>>>>>> a delay to boot-up, so use the clock api instead to get the
>>>>>> clock rate.
>>>>>>
>>>>>> Signed-off-by: Rob Herring<rob.herring@smooth-stone.com>
>>>>>> ---
>>>>>>   arch/arm/include/asm/smp_twd.h |    2 ++
>>>>>>   arch/arm/kernel/smp_twd.c      |    7 +++++++
>>>>>>   2 files changed, 9 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/include/asm/smp_twd.h
>>>>>> b/arch/arm/include/asm/smp_twd.h
>>>>>> index 634f357..bafad52 100644
>>>>>> --- a/arch/arm/include/asm/smp_twd.h
>>>>>> +++ b/arch/arm/include/asm/smp_twd.h
>>>>>> @@ -19,11 +19,13 @@
>>>>>>   #define TWD_TIMER_CONTROL_IT_ENABLE    (1<<      2)
>>>>>>
>>>>>>   struct clock_event_device;
>>>>>> +struct clk;
>>>>>>
>>>>>>   extern void __iomem *twd_base;
>>>>>>
>>>>>>   void twd_timer_stop(void);
>>>>>>   int twd_timer_ack(void);
>>>>>>   void twd_timer_setup(struct clock_event_device *);
>>>>>> +void twd_timer_init(void __iomem *base, struct clk *clk);
>>>>>>
>>>>>>   #endif
>>>>>> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
>>>>>> index 35882fb..1a3c959 100644
>>>>>> --- a/arch/arm/kernel/smp_twd.c
>>>>>> +++ b/arch/arm/kernel/smp_twd.c
>>>>>> @@ -17,6 +17,7 @@
>>>>>>   #include<linux/clockchips.h>
>>>>>>   #include<linux/irq.h>
>>>>>>   #include<linux/io.h>
>>>>>> +#include<linux/clk.h>
>>>>>>
>>>>>>   #include<asm/smp_twd.h>
>>>>>>   #include<asm/hardware/gic.h>
>>>>>> @@ -151,6 +152,12 @@ void __cpuinit twd_timer_setup(struct
>>>>>> clock_event_device *clk)
>>>>>>         clockevents_register_device(clk);
>>>>>>   }
>>>>>>
>>>>>> +void __init twd_timer_init(void __iomem *base, struct clk *clk)
>>>>>> +{
>>>>>> +       twd_base = base;
>>>>>> +       twd_timer_rate = clk_get_rate(clk);
>>>>>> +}
>>>>>> +
>>>>>>   #ifdef CONFIG_HOTPLUG_CPU
>>>>>>   /*
>>>>>>   * take a local timer down
>>>>>
>>>>> The local timers run off the PERIPHCLK in the CPU, which is specified
>>>>> as the CPU clock divided by an implementation defined integer>= 2.
>>>>> If you take the divider value as a parameter to twd_timer_init, the
>>>>> clock that is passed in can be the cpu's clock.
>>>>
>>>> That is an implementation detail of the A9. Future designs could be
>>>> completely asynchronous. Using the clock api works either way.
>>>
>>> True.  That could still be handled by passing a divider of 1.  For all
>>> current implementations, this api will be used with clock that is a
>>> constant divider of an existing clock.  Taking a divider value would
>>> avoid having to create a new clock that may not be similar to any
>>> other clock in the device.
>>
>> If I pass in a divider, what clock rate do I divide with it? If I only have
>> the divider, then I'm dependent on knowing CLK freq (the cpu freq in A9
>> case). Ultimately I have to know the timer clock rate to setup a clock_event
>> device. I'm open to passing in the clock rate directly, but having the clk
>> ptr is more flexible. Other examples of timer init like i.MX use the same
>> arguments.
>>
>> The point of this api is to avoid spending 5 jiffies on the primary cpu to
>> calculate the rate. That is a significant chunk of the kernel boot time.
>> Calling this function is entirely optional and the old way still works.
>
> I like the idea of passing in the clk pointer - it would simplify
> problems I've been having with rescaling the localtimer when the cpu
> frequency changes.  However, it would reduce the amount of
> machine-specific code necessary if I could pass in the
> already-existing clock (cpu clock, for A9), as well as the value of
> the divider that is present inside cpu (4 for Tegra2).

You are assuming all hardware would have a fixed divider. There is no 
requirement that the CLK:PERIPHCLK ratio is fixed. Well designed clock 
controller h/w would support CLK changing while maintaining PERIPHCLK 
rate or at least support changing the ratio.

You will need to get the clock rate every set_next_event and somehow 
sync cpu freq changes to when no timers are active on all cores. Or just 
accept the timer inaccuracy for 1 time. Good luck with that... ;)

What is the problem of defining another clock in the platform? The 
watchdogs if implemented also need that clock.

Rob