Re: [PATCH v4 05/17] arch_timer: Move to generic sched_clockframework

[Will Deacon]

Hi Stephen,

On Fri, Jul 19, 2013 at 12:21:18AM +0100, Stephen Boyd wrote:
> Register with the generic sched_clock framework now that it
> supports 64 bits. This fixes two problems with the current
> sched_clock support for machines using the architected timers.
> First off, we don't subtract the start value from subsequent
> sched_clock calls so we can potentially start off with
> sched_clock returning gigantic numbers. Second, there is no
> support for suspend/resume handling so problems such as discussed
> in 6a4dae5 (ARM: 7565/1: sched: stop sched_clock() during
> suspend, 2012-10-23) can happen without this patch. Finally, it
> allows us to move the sched_clock setup into drivers clocksource
> out of the arch ports.

Sorry, this one slipped through the cracks.

> diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
> index 221f07b..1791f12 100644
> --- a/arch/arm/kernel/arch_timer.c
> +++ b/arch/arm/kernel/arch_timer.c
> @@ -11,7 +11,6 @@
>  #include <linux/init.h>
>  #include <linux/types.h>
>  #include <linux/errno.h>
> -#include <linux/sched_clock.h>
>  
>  #include <asm/delay.h>
>  
> @@ -22,13 +21,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 +40,5 @@ 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/Kconfig b/arch/arm64/Kconfig
> index 9737e97..e32b471 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -14,6 +14,7 @@ config ARM64
>  	select GENERIC_IOMAP
>  	select GENERIC_IRQ_PROBE
>  	select GENERIC_IRQ_SHOW
> +	select GENERIC_SCHED_CLOCK
>  	select GENERIC_SMP_IDLE_THREAD
>  	select GENERIC_TIME_VSYSCALL
>  	select HARDIRQS_SW_RESEND
> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
> index 03dc371..29c39d5 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;
> -}
> -
>  void __init time_init(void)
>  {
>  	u32 arch_timer_rate;
> @@ -78,9 +71,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 053d846..2facf14 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -17,6 +17,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/of_irq.h>
>  #include <linux/io.h>
> +#include <linux/sched_clock.h>
>  
>  #include <asm/arch_timer.h>
>  #include <asm/virt.h>
> @@ -281,6 +282,9 @@ static int __init arch_timer_register(void)
>  	timecounter_init(&timecounter, &cyclecounter,
>  			 arch_counter_get_cntvct());
>  
> +	/* 56 bits minimum, so we assume worst case rollover */
> +	sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate);

We've got the same assumption elsewhere in the kernel (e.g. vdso) but at
some point we should probably deal with >56 bits in case somebody makes a
high-frequency timer with more significant bits.

However, I think this patch is going in the right direction:

  Acked-by: Will Deacon <will.deacon@xxxxxxx>

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/