From mboxrd@z Thu Jan 1 00:00:00 1970 From: robherring2@gmail.com (Rob Herring) Date: Fri, 01 Oct 2010 12:04:42 -0500 Subject: [PATCH 3/3] ARM: twd_smp: add clock api support In-Reply-To: References: <1285886952-30888-1-git-send-email-robherring2@gmail.com> <1285886952-30888-3-git-send-email-robherring2@gmail.com> <4CA5417B.5020200@gmail.com> <4CA551FF.3080100@gmail.com> Message-ID: <4CA614AA.8090305@gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 09/30/2010 10:27 PM, Colin Cross wrote: > On Thu, Sep 30, 2010 at 8:14 PM, Rob Herring wrote: >> On 09/30/2010 09:30 PM, Colin Cross wrote: >>> >>> On Thu, Sep 30, 2010 at 7:03 PM, Rob Herring >>> wrote: >>>> >>>> On 09/30/2010 07:49 PM, Colin Cross wrote: >>>>> >>>>> On Thu, Sep 30, 2010 at 3:49 PM, Rob Herring >>>>> wrote: >>>>>> >>>>>> From: Rob Herring >>>>>> >>>>>> 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 >>>>>> --- >>>>>> 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 >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> >>>>>> #include >>>>>> #include >>>>>> @@ -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