| Message ID | 1366313410-16692-1-git-send-email-robherring2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/18/13 12:30, Rob Herring wrote: > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index 122ff05..17ed8e4 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -266,6 +266,15 @@ static struct notifier_block arch_timer_cpu_nb __cpuinitdata = { > .notifier_call = arch_timer_cpu_notify, > }; > > +static u64 sched_clock_mult __read_mostly; > + > +unsigned long long notrace arch_timer_sched_clock(void) > +{ > + return arch_timer_read_counter() * sched_clock_mult; > +} > +unsigned long long sched_clock(void) \ > + __attribute__((weak, alias("arch_timer_sched_clock"))); I'm still lost, how does this prevent the timer in ARM's 32 bit sched_clock code from getting setup in sched_clock_postinit()? That print is still there right? Who owns sched_clock() in multi-target builds? Why can't we play along with the sched_clock code that lives in arm? Maybe we should resurrect those clocksource sched_clock patches again. Or maybe we should add support for setup_sched_clock_64() in arm's sched clock code.
On 04/18/2013 07:00 PM, Stephen Boyd wrote: > On 04/18/13 12:30, Rob Herring wrote: >> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c >> index 122ff05..17ed8e4 100644 >> --- a/drivers/clocksource/arm_arch_timer.c >> +++ b/drivers/clocksource/arm_arch_timer.c >> @@ -266,6 +266,15 @@ static struct notifier_block arch_timer_cpu_nb __cpuinitdata = { >> .notifier_call = arch_timer_cpu_notify, >> }; >> >> +static u64 sched_clock_mult __read_mostly; >> + >> +unsigned long long notrace arch_timer_sched_clock(void) >> +{ >> + return arch_timer_read_counter() * sched_clock_mult; >> +} >> +unsigned long long sched_clock(void) \ >> + __attribute__((weak, alias("arch_timer_sched_clock"))); > > I'm still lost, how does this prevent the timer in ARM's 32 bit > sched_clock code from getting setup in sched_clock_postinit()? That > print is still there right? Who owns sched_clock() in multi-target builds? For arm64, it does not define sched_clock, so it will get arch_timer_sched_clock. For arm, sched_clock is defined in arch/arm/kernel/sched_clock.c and the weak alias is not used. The arm sched_clock function just calls a function pointer which defaults to sched_clock_32 (which is the original arm sched_clock implementation). If the arch timer is present, then the function pointer is set to arch_timer_sched_clock and any calls to setup_sched_clock and the sched_clock_postinit have no effect. Otherwise, the functionality is basically unchanged for <=32-bit sched_clock implementations. > Why can't we play along with the sched_clock code that lives in arm? > Maybe we should resurrect those clocksource sched_clock patches again. > Or maybe we should add support for setup_sched_clock_64() in arm's sched > clock code. That's what I originally had which Russell objected to. The needs for the arch timer is a bit different since we don't need to deal with wrapping. And we need the same boot time offset and suspend handling in both arm and arm64. Rob
On Thu, Apr 18, 2013 at 08:30:09PM +0100, Rob Herring wrote: > From: Rob Herring <rob.herring@calxeda.com> > > In preparation to fix initial time and suspend/resume handling, unify > the sched_clock init and implementation for arch timer on arm and arm64. > > Signed-off-by: Rob Herring <rob.herring@calxeda.com> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Stephen Boyd <sboyd@codeaurora.org> For arm64: Acked-by: Catalin Marinas <catalin.marinas@arm.com>
On 04/18/13 18:37, Rob Herring wrote: > On 04/18/2013 07:00 PM, Stephen Boyd wrote: >> On 04/18/13 12:30, Rob Herring wrote: >>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c >>> index 122ff05..17ed8e4 100644 >>> --- a/drivers/clocksource/arm_arch_timer.c >>> +++ b/drivers/clocksource/arm_arch_timer.c >>> @@ -266,6 +266,15 @@ static struct notifier_block arch_timer_cpu_nb __cpuinitdata = { >>> .notifier_call = arch_timer_cpu_notify, >>> }; >>> >>> +static u64 sched_clock_mult __read_mostly; >>> + >>> +unsigned long long notrace arch_timer_sched_clock(void) >>> +{ >>> + return arch_timer_read_counter() * sched_clock_mult; >>> +} >>> +unsigned long long sched_clock(void) \ >>> + __attribute__((weak, alias("arch_timer_sched_clock"))); >> I'm still lost, how does this prevent the timer in ARM's 32 bit >> sched_clock code from getting setup in sched_clock_postinit()? That >> print is still there right? I still see this: [ 0.000000] sched_clock: ARM arch timer >56 bits at 19200kHz, resolution 52ns [ 0.000000] Architected cp15 timer running at 19.20MHz (virt). [ 0.000000] Switching to timer-based delay loop [ 0.000000] sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every 4294967286ms >> Who owns sched_clock() in multi-target builds? > For arm64, it does not define sched_clock, so it will get > arch_timer_sched_clock. > > For arm, sched_clock is defined in arch/arm/kernel/sched_clock.c and the > weak alias is not used. The arm sched_clock function just calls a > function pointer which defaults to sched_clock_32 (which is the original > arm sched_clock implementation). If the arch timer is present, then the > function pointer is set to arch_timer_sched_clock and any calls to > setup_sched_clock and the sched_clock_postinit have no effect. > Otherwise, the functionality is basically unchanged for <=32-bit > sched_clock implementations. Ok. I was missing the part where the function pointer is assigned. > >> Why can't we play along with the sched_clock code that lives in arm? >> Maybe we should resurrect those clocksource sched_clock patches again. >> Or maybe we should add support for setup_sched_clock_64() in arm's sched >> clock code. > That's what I originally had which Russell objected to. The needs for > the arch timer is a bit different since we don't need to deal with > wrapping. And we need the same boot time offset and suspend handling in > both arm and arm64. > I would much rather we play along with arm's sched_clock code. If we can add support for 64 bits alongside the 32 bit stuff in the same file we should be able to generalize the entire code to generic kernel code and use it on even more arches. I'll try to put something together today.
This is what I was thinking. I don't see why we can't move this to generic code and have arm64 use it too. Those patches will follow once I find an arm64 compiler. First two patches should probably go in even if the 64 bit stuff doesn't go in at the same time. Stephen Boyd (4): ARM: sched_clock: Remove unused needs_suspend member ARM: sched_clock: Return suspended count earlier ARM: sched_clock: Add support for >32 bit sched_clock ARM: arch_timer: Move to setup_sched_clock_64() arch/arm/include/asm/sched_clock.h | 5 +- arch/arm/kernel/arch_timer.c | 14 +---- arch/arm/kernel/sched_clock.c | 111 ++++++++++++++++++++++++++----------- 3 files changed, 84 insertions(+), 46 deletions(-)
On Saturday 20 April 2013, Stephen Boyd wrote: > This is what I was thinking. I don't see why we can't move this to generic code > and have arm64 use it too. Those patches will follow once I find an arm64 > compiler. I don't have enough background to review the patches, but I know that you can find an arm64 compiler in Ubuntu Raring. If you are running that on one of your sysstems, just 'apt-get install gcc-aarch64-linux-gnu'. Arnd
Hi Stephen, On Sat, Apr 20, 2013 at 01:29:02AM +0100, Stephen Boyd wrote: > This is what I was thinking. I don't see why we can't move this to generic > code and have arm64 use it too. Those patches will follow once I find an > arm64 compiler. > > First two patches should probably go in even if the 64 bit stuff doesn't go in > at the same time. > > Stephen Boyd (4): > ARM: sched_clock: Remove unused needs_suspend member > ARM: sched_clock: Return suspended count earlier > ARM: sched_clock: Add support for >32 bit sched_clock > ARM: arch_timer: Move to setup_sched_clock_64() > > arch/arm/include/asm/sched_clock.h | 5 +- > arch/arm/kernel/arch_timer.c | 14 +---- > arch/arm/kernel/sched_clock.c | 111 ++++++++++++++++++++++++++----------- > 3 files changed, 84 insertions(+), 46 deletions(-) I wanted to look at the series with more context, but I don't seem to be able to apply patch 2 and beyond to my tree, and I couldn't figure out what tree this series was based on. What do I need to use as the base for this series? Thanks, Mark.
On 04/22/13 08:34, Mark Rutland wrote: > Hi Stephen, > > On Sat, Apr 20, 2013 at 01:29:02AM +0100, Stephen Boyd wrote: >> This is what I was thinking. I don't see why we can't move this to generic >> code and have arm64 use it too. Those patches will follow once I find an >> arm64 compiler. >> >> First two patches should probably go in even if the 64 bit stuff doesn't go in >> at the same time. >> >> Stephen Boyd (4): >> ARM: sched_clock: Remove unused needs_suspend member >> ARM: sched_clock: Return suspended count earlier >> ARM: sched_clock: Add support for >32 bit sched_clock >> ARM: arch_timer: Move to setup_sched_clock_64() >> >> arch/arm/include/asm/sched_clock.h | 5 +- >> arch/arm/kernel/arch_timer.c | 14 +---- >> arch/arm/kernel/sched_clock.c | 111 ++++++++++++++++++++++++++----------- >> 3 files changed, 84 insertions(+), 46 deletions(-) > I wanted to look at the series with more context, but I don't seem to be able > to apply patch 2 and beyond to my tree, and I couldn't figure out what tree > this series was based on. > > What do I need to use as the base for this series? These are based on next-20130419.
On Mon, Apr 22, 2013 at 04:36:14PM +0100, Stephen Boyd wrote: > On 04/22/13 08:34, Mark Rutland wrote: > > Hi Stephen, > > > > On Sat, Apr 20, 2013 at 01:29:02AM +0100, Stephen Boyd wrote: > >> This is what I was thinking. I don't see why we can't move this to generic > >> code and have arm64 use it too. Those patches will follow once I find an > >> arm64 compiler. > >> > >> First two patches should probably go in even if the 64 bit stuff doesn't go in > >> at the same time. > >> > >> Stephen Boyd (4): > >> ARM: sched_clock: Remove unused needs_suspend member > >> ARM: sched_clock: Return suspended count earlier > >> ARM: sched_clock: Add support for >32 bit sched_clock > >> ARM: arch_timer: Move to setup_sched_clock_64() > >> > >> arch/arm/include/asm/sched_clock.h | 5 +- > >> arch/arm/kernel/arch_timer.c | 14 +---- > >> arch/arm/kernel/sched_clock.c | 111 ++++++++++++++++++++++++++----------- > >> 3 files changed, 84 insertions(+), 46 deletions(-) > > I wanted to look at the series with more context, but I don't seem to be able > > to apply patch 2 and beyond to my tree, and I couldn't figure out what tree > > this series was based on. > > > > What do I need to use as the base for this series? > > These are based on next-20130419. Cheers! Mark.
On 04/19/2013 05:29 PM, Stephen Boyd wrote: > This is what I was thinking. I don't see why we can't move this to generic code and have arm64 use it too. Those patches will follow once I find an arm64 > compiler. I think moving this to generic code sounds like a good idea. You could probably also prototype and test the 64bit code with x86_64, using the TSC counter. thanks -john
On 04/22/2013 12:00 PM, John Stultz wrote: > On 04/19/2013 05:29 PM, Stephen Boyd wrote: >> This is what I was thinking. I don't see why we can't move this to >> generic code and have arm64 use it too. Those patches will follow once >> I find an arm64 >> compiler. > > I think moving this to generic code sounds like a good idea. You could > probably also prototype and test the 64bit code with x86_64, using the > TSC counter. I agree this should all be common, but I'd like to see the common version first. That is not going to make it for 3.10. For 3.10, the immediate need is to fix suspend and initial time for the arch timer. I think this should be fixed locally in arch timer code for 3.10. The alternative is to revert linux-next commit 023796b9be3a77481cd5 (ARM: arch_timer: use full 64-bit counter for sched_clock) which will cause the arch timer to not be used as sched_clock if another higher frequency sched_clock is registered. Rob
On 04/22/13 13:46, Rob Herring wrote: > On 04/22/2013 12:00 PM, John Stultz wrote: >> On 04/19/2013 05:29 PM, Stephen Boyd wrote: >>> This is what I was thinking. I don't see why we can't move this to >>> generic code and have arm64 use it too. Those patches will follow once >>> I find an arm64 >>> compiler. >> I think moving this to generic code sounds like a good idea. You could >> probably also prototype and test the 64bit code with x86_64, using the >> TSC counter. > I agree this should all be common, but I'd like to see the common > version first. That is not going to make it for 3.10. For 3.10, the > immediate need is to fix suspend and initial time for the arch timer. I > think this should be fixed locally in arch timer code for 3.10. The > alternative is to revert linux-next commit 023796b9be3a77481cd5 (ARM: > arch_timer: use full 64-bit counter for sched_clock) which will cause > the arch timer to not be used as sched_clock if another higher frequency > sched_clock is registered. This would make sense to me if we were already in the 3.10-rc1 or rc2 phase, but this code isn't even in Linus' tree yet. Why can't we just fix it properly before sending off to Linus? Obviously this is up to the maintainers to decide, so if we can't fix it properly with this patch series I propose we revert like you say.
diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c index 59dcdce..df6825e 100644 --- a/arch/arm/kernel/arch_timer.c +++ b/arch/arm/kernel/arch_timer.c @@ -22,13 +22,6 @@ static unsigned long arch_timer_read_counter_long(void) return arch_timer_read_counter(); } -static u32 sched_clock_mult __read_mostly; - -static unsigned long long notrace arch_timer_sched_clock(void) -{ - return arch_timer_read_counter() * sched_clock_mult; -} - static struct delay_timer arch_delay_timer; static void __init arch_timer_delay_timer_register(void) @@ -48,11 +41,6 @@ int __init arch_timer_arch_init(void) arch_timer_delay_timer_register(); - /* Cache the sched_clock multiplier to save a divide in the hot path. */ - sched_clock_mult = NSEC_PER_SEC / arch_timer_rate; sched_clock_func = arch_timer_sched_clock; - pr_info("sched_clock: ARM arch timer >56 bits at %ukHz, resolution %uns\n", - arch_timer_rate / 1000, sched_clock_mult); - return 0; } diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c index a551f88..3a369aa 100644 --- a/arch/arm64/kernel/time.c +++ b/arch/arm64/kernel/time.c @@ -61,13 +61,6 @@ unsigned long profile_pc(struct pt_regs *regs) EXPORT_SYMBOL(profile_pc); #endif -static u64 sched_clock_mult __read_mostly; - -unsigned long long notrace sched_clock(void) -{ - return arch_timer_read_counter() * sched_clock_mult; -} - int read_current_timer(unsigned long *timer_value) { *timer_value = arch_timer_read_counter(); @@ -84,9 +77,6 @@ void __init time_init(void) if (!arch_timer_rate) panic("Unable to initialise architected timer.\n"); - /* Cache the sched_clock multiplier to save a divide in the hot path. */ - sched_clock_mult = NSEC_PER_SEC / arch_timer_rate; - /* Calibrate the delay loop directly */ lpj_fine = arch_timer_rate / HZ; } diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 122ff05..17ed8e4 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -266,6 +266,15 @@ static struct notifier_block arch_timer_cpu_nb __cpuinitdata = { .notifier_call = arch_timer_cpu_notify, }; +static u64 sched_clock_mult __read_mostly; + +unsigned long long notrace arch_timer_sched_clock(void) +{ + return arch_timer_read_counter() * sched_clock_mult; +} +unsigned long long sched_clock(void) \ + __attribute__((weak, alias("arch_timer_sched_clock"))); + static int __init arch_timer_register(void) { int err; @@ -318,6 +327,11 @@ static int __init arch_timer_register(void) /* Immediately configure the timer on the boot CPU */ arch_timer_setup(this_cpu_ptr(arch_timer_evt)); + /* Cache the sched_clock multiplier to save a divide in the hot path. */ + sched_clock_mult = NSEC_PER_SEC / arch_timer_rate; + pr_info("sched_clock: ARM arch timer >56 bits at %ukHz, resolution %lluns\n", + arch_timer_rate / 1000, sched_clock_mult); + return 0; out_free_irq: diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h index e6c9c4c..ded7c77 100644 --- a/include/clocksource/arm_arch_timer.h +++ b/include/clocksource/arm_arch_timer.h @@ -34,6 +34,7 @@ extern u32 arch_timer_get_rate(void); extern u64 (*arch_timer_read_counter)(void); extern struct timecounter *arch_timer_get_timecounter(void); +extern unsigned long long notrace arch_timer_sched_clock(void); #else