Hi Kirill,
thanks for looking into this!
A few comments below - mostly on style issues.
I have tested this on my SPARCstation 5 - it could
boot.
Can I do more to test that this is actually working as expected?
Sam
> --- a/arch/sparc/include/asm/timer_32.h
> +++ b/arch/sparc/include/asm/timer_32.h
> @@ -8,10 +8,23 @@
> #ifndef _SPARC_TIMER_H
> #define _SPARC_TIMER_H
>
> +#include <linux/irqreturn.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <asm-generic/percpu.h>
Looks like you add more includes than strictly required for the .h file.
> --- a/arch/sparc/kernel/irq.h
> +++ b/arch/sparc/kernel/irq.h
> @@ -40,15 +40,20 @@ struct sun4m_irq_global {
> extern struct sun4m_irq_percpu __iomem *sun4m_irq_percpu[SUN4M_NCPUS];
> extern struct sun4m_irq_global __iomem *sun4m_irq_global;
>
> +#define USES_TIMER_CS (1 << 0)
> +#define USES_TIMER_CE (1 << 1)
> +#define PERCPU_CE_CAN_ONESHOT (1 << 2)
A comment about what each flag indicate would be good.
> +#define USECS_PER_JIFFY (1000000/HZ)
> +#define TICK_TIMER_LIMIT ((100*1000000/4)/HZ)
Spaces around operators please.
I miss:
#define SPARC32_NSEC_PER_CYC_SHIFT 9UL
As I assume this is what the hardcoded "9" is about.
Used the sparc64 name as template.
> +static u32 pcic_cycles_offset(void)
> {
> + u32 value, count;
> +
> + value = readl(pcic0.pcic_regs+PCI_SYS_COUNTER);
Spaces around operators.
> + count = ((count/HZ)*USECS_PER_JIFFY) / (TICK_TIMER_LIMIT/HZ);
Spaces..
In several spots below - I will stop complining here.
> /* Errm.. not sure how to do this.. */
> }
>
> -static void __init sun4c_init_timers(irq_handler_t counter_fn)
> +static void __init sun4c_init_timers(void)
> {
> const struct linux_prom_irqs *prom_irqs;
> struct device_node *dp;
> @@ -207,12 +207,15 @@ static void __init sun4c_init_timers(irq_handler_t counter_fn)
> * level 14 timer limit since we are letting the prom handle
> * them until we have a real console driver so L1-A works.
> */
> - sbus_writel((((1000000/HZ) + 1) << 10), &sun4c_timers->l10_limit);
> + timer_cs_period = 2000000/HZ;
Where does 2000000 come from? Is looks like it deserve a constant.
> + sparc_irq_config.features |= USES_TIMER_CS;
> + sparc_irq_config.features |= USES_TIMER_CE;
Could we name USES_xxx FEATURE_xxx as it is then more obvious that is is
used as flags in the sparc_irq_config.features field?
> +#ifdef CONFIG_SMP
> + timer_cs_period = 4000000; /* 2 seconds */
> +#else
This value also deserve a descriptive constant.
> static int cpu_tick[NR_CPUS];
> static char led_mask[] = { 0xe, 0xd, 0xb, 0x7, 0xb, 0xd };
>
> @@ -378,28 +378,15 @@ void smp4d_percpu_timer_interrupt(struct pt_regs *regs)
> show_leds(cpu);
> }
>
> - profile_tick(CPU_PROFILING);
> + ce = &per_cpu(percpu_ce, cpu);
>
> - if (!--prof_counter(cpu)) {
> - int user = user_mode(regs);
> + irq_enter();
> + ce->event_handler(ce);
> + irq_exit();
On sparc64 we check if ce->event_handler is NULL before we do the call.
Something we should do on sparc32 too?
> - irq_exit();
> -
> - prof_counter(cpu) = prof_multiplier(cpu);
> - }
> set_irq_regs(old_regs);
> }
>
> diff --git a/arch/sparc/kernel/sun4m_irq.c b/arch/sparc/kernel/sun4m_irq.c
> index e611651..06d3910 100644
> --- a/arch/sparc/kernel/sun4m_irq.c
> +++ b/arch/sparc/kernel/sun4m_irq.c
> @@ -318,9 +318,6 @@ struct sun4m_timer_global {
>
> static struct sun4m_timer_global __iomem *timers_global;
>
> -
> -unsigned int lvl14_resolution = (((1000000/HZ) + 1) << 10);
> -
> static void sun4m_clear_clock_irq(void)
> {
> sbus_readl(&timers_global->l10_limit);
> @@ -369,10 +366,11 @@ void sun4m_clear_profile_irq(int cpu)
>
> static void sun4m_load_profile_irq(int cpu, unsigned int limit)
> {
> - sbus_writel(limit, &timers_percpu[cpu]->l14_limit);
> + unsigned int value = limit ? (limit + 1) << 9 : 0;
> + sbus_writel(value, &timers_percpu[cpu]->l14_limit);
> }
>
> -static void __init sun4m_init_timers(irq_handler_t counter_fn)
> +static void __init sun4m_init_timers(void)
> {
> struct device_node *dp = of_find_node_by_name(NULL, "counter");
> int i, err, len, num_cpu_timers;
> @@ -402,13 +400,21 @@ static void __init sun4m_init_timers(irq_handler_t counter_fn)
> /* Every per-cpu timer works in timer mode */
> sbus_writel(0x00000000, &timers_global->timer_config);
>
> - sbus_writel((((1000000/HZ) + 1) << 10), &timers_global->l10_limit);
> +#ifdef CONFIG_SMP
> + timer_cs_period = 4000000; /* 2 seconds */
> + sparc_irq_config.features |= PERCPU_CE_CAN_ONESHOT;
> +#else
> + timer_cs_period = 2000000/HZ; /* 1/HZ */
> + sparc_irq_config.features |= USES_TIMER_CE;
> +#endif
> + sparc_irq_config.features |= USES_TIMER_CS;
> + sbus_writel(((timer_cs_period + 1) << 9), &timers_global->l10_limit);
>
> master_l10_counter = &timers_global->l10_count;
>
> irq = sun4m_build_device_irq(NULL, SUN4M_TIMER_IRQ);
>
> - err = request_irq(irq, counter_fn, IRQF_TIMER, "timer", NULL);
> + err = request_irq(irq, timer_interrupt, IRQF_TIMER, "timer", NULL);
> if (err) {
> printk(KERN_ERR "sun4m_init_timers: Register IRQ error %d.\n",
> err);
> diff --git a/arch/sparc/kernel/sun4m_smp.c b/arch/sparc/kernel/sun4m_smp.c
> index 5947686..d98d307 100644
> --- a/arch/sparc/kernel/sun4m_smp.c
> +++ b/arch/sparc/kernel/sun4m_smp.c
> @@ -11,6 +11,7 @@
>
> #include <asm/cacheflush.h>
> #include <asm/tlbflush.h>
> +#include <asm/timer.h>
>
> #include "irq.h"
> #include "kernel.h"
> @@ -30,7 +31,6 @@ swap_ulong(volatile unsigned long *ptr, unsigned long val)
> }
>
> static void smp4m_ipi_init(void);
> -static void smp_setup_percpu_timer(void);
>
> void __cpuinit smp4m_callin(void)
> {
> @@ -41,8 +41,7 @@ void __cpuinit smp4m_callin(void)
>
> notify_cpu_starting(cpuid);
>
> - /* Get our local ticker going. */
> - smp_setup_percpu_timer();
> + register_percpu_ce(cpuid);
>
> calibrate_delay();
> smp_store_cpu_info(cpuid);
> @@ -86,7 +85,7 @@ void __cpuinit smp4m_callin(void)
> void __init smp4m_boot_cpus(void)
> {
> smp4m_ipi_init();
> - smp_setup_percpu_timer();
> + sun4m_unmask_profile_irq();
> local_flush_cache_all();
> }
>
The comments you raised in private mail should be refelced as comment
in the code too I think.
"
In case of sun4m's oneshot mode, profile irq is zeroed in smp4m_percpu_timer_interrupt().
It maybe needless (double, triple etc overflow does nothing)
"
> @@ -259,37 +258,25 @@ void smp4m_cross_call_irq(void)
> void smp4m_percpu_timer_interrupt(struct pt_regs *regs)
> {
> struct pt_regs *old_regs;
> + struct clock_event_device *ce;
> int cpu = smp_processor_id();
>
> old_regs = set_irq_regs(regs);
>
> - sun4m_clear_profile_irq(cpu);
> + ce = &per_cpu(percpu_ce, cpu);
>
> - profile_tick(CPU_PROFILING);
> + if (ce->mode & CLOCK_EVT_MODE_PERIODIC)
> + sun4m_clear_profile_irq(cpu);
> + else
> + load_profile_irq(cpu, 0);
>
> - if (!--prof_counter(cpu)) {
> - int user = user_mode(regs);
> + irq_enter();
> + ce->event_handler(ce);
> + irq_exit();
>
> - irq_enter();
> - update_process_times(user);
> - irq_exit();
> -
> - prof_counter(cpu) = prof_multiplier(cpu);
> - }
> set_irq_regs(old_regs);
> }
>
> -static void __cpuinit smp_setup_percpu_timer(void)
> -{
> - int cpu = smp_processor_id();
> -
> - prof_counter(cpu) = prof_multiplier(cpu) = 1;
> - load_profile_irq(cpu, lvl14_resolution);
> -
> - if (cpu == boot_cpu_id)
> - sun4m_unmask_profile_irq();
> -}
> -
> static void __init smp4m_blackbox_id(unsigned *addr)
> {
> int rd = *addr & 0x3e000000;
> diff --git a/arch/sparc/kernel/time_32.c b/arch/sparc/kernel/time_32.c
> index 1060e06..f541ac7 100644
> --- a/arch/sparc/kernel/time_32.c
> +++ b/arch/sparc/kernel/time_32.c
> @@ -26,6 +26,8 @@
> #include <linux/rtc.h>
> #include <linux/rtc/m48t59.h>
> #include <linux/timex.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> #include <linux/init.h>
> #include <linux/pci.h>
> #include <linux/ioport.h>
> @@ -45,9 +47,24 @@
> #include <asm/page.h>
> #include <asm/pcic.h>
> #include <asm/irq_regs.h>
> +#include <asm/setup.h>
>
> #include "irq.h"
>
> +static __cacheline_aligned_in_smp DEFINE_SEQLOCK(timer_cs_lock);
> +static __volatile__ u64 timer_cs_internal_counter = 0;
Init to zero is worthless. We do it during startup anyway.
> +/* Tick period in cycles */
> +unsigned int timer_cs_period;
> +static char timer_cs_enabled = 0;
Init to zero is worthless.
> +u32 (*get_cycles_offset)(void);
> +
> +static struct clock_event_device timer_ce;
> +static char timer_ce_enabled = 0;
Init to zero is worthless.
> +
> +#ifdef CONFIG_SMP
> +DEFINE_PER_CPU(struct clock_event_device, percpu_ce);
> +#endif
I would prefer we keep naming consistent with sparc64 naming.
Here we name it sparc64_events - but I can see why you used another name too.
Why is this only SMP?
> +
> DEFINE_SPINLOCK(rtc_lock);
> EXPORT_SYMBOL(rtc_lock);
>
> @@ -76,36 +93,165 @@ EXPORT_SYMBOL(profile_pc);
>
> __volatile__ unsigned int *master_l10_counter;
>
> -u32 (*do_arch_gettimeoffset)(void);
> -
> int update_persistent_clock(struct timespec now)
> {
> return set_rtc_mmss(now.tv_sec);
> }
>
> -/*
> - * timer_interrupt() needs to keep up the real-time clock,
> - * as well as call the "xtime_update()" routine every clocktick
> - */
> +irqreturn_t notrace timer_interrupt(int dummy, void *dev_id)
> +{
> + if (timer_cs_enabled) {
> + write_seqlock(&timer_cs_lock);
> + timer_cs_internal_counter ++;
Drop space before ++
> + clear_clock_irq();
> + write_sequnlock(&timer_cs_lock);
> + } else
> + clear_clock_irq();
>
> -#define TICK_SIZE (tick_nsec / 1000)
> + if (timer_ce_enabled)
> + timer_ce.event_handler(&timer_ce);
>
> -static irqreturn_t timer_interrupt(int dummy, void *dev_id)
> + return IRQ_HANDLED;
> +}
> +
> +static void timer_ce_set_mode(enum clock_event_mode mode,
> + struct clock_event_device *evt)
> {
> -#ifndef CONFIG_SMP
> - profile_tick(CPU_PROFILING);
> -#endif
> + switch (mode) {
> + case CLOCK_EVT_MODE_PERIODIC:
> + case CLOCK_EVT_MODE_RESUME:
> + timer_ce_enabled = 1;
> + break;
> + case CLOCK_EVT_MODE_SHUTDOWN:
> + timer_ce_enabled = 0;
> + break;
> + default:
> + break;
> + }
> + smp_mb();
> +}
>
> - clear_clock_irq();
> +static __init void setup_timer_ce(void)
> +{
> + struct clock_event_device *ce = &timer_ce;
>
> - xtime_update(1);
> + BUG_ON(smp_processor_id() != boot_cpu_id);
>
> -#ifndef CONFIG_SMP
> - update_process_times(user_mode(get_irq_regs()));
> -#endif
> - return IRQ_HANDLED;
> + ce->name = "timer_ce";
> + ce->rating = 100;
> + ce->features = CLOCK_EVT_FEAT_PERIODIC;
> + ce->set_mode = timer_ce_set_mode;
> + ce->cpumask = cpu_possible_mask;
> + ce->shift = 32;
> + ce->mult = div_sc(2000000, NSEC_PER_SEC, ce->shift);
> +
> + clockevents_register_device(ce);
> +}
> +
> +static u32 sbus_cycles_offset(void)
> +{
> + unsigned int val, offset;
> +
> + val = *master_l10_counter;
> + offset = (val >> 9) & 0x3fffff;
> +
> + /* Limit hit? */
> + if (val & 0x80000000)
> + offset += timer_cs_period;
> +
> + return offset;
> +}
> +
> +static cycle_t timer_cs_read(struct clocksource *cs)
> +{
> + unsigned int seq, offset;
> + u64 cycles;
> +
> + do {
> + seq = read_seqbegin(&timer_cs_lock);
> +
> + cycles = timer_cs_internal_counter;
> + offset = get_cycles_offset();
> + } while (read_seqretry(&timer_cs_lock, seq));
> +
> + /* Count absolute cycles */
> + cycles *= timer_cs_period;
> + cycles += offset;
> +
> + return cycles;
> +}
> +
> +static struct clocksource timer_cs = {
> + .name = "timer_cs",
> + .rating = 100,
> + .read = timer_cs_read,
> + .mask = CLOCKSOURCE_MASK(64),
> + .shift = 2,
> + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +static __init int setup_timer_cs(void)
> +{
> + timer_cs_enabled = 1;
> + /* Clock rate is 2MHz */
> + timer_cs.mult = clocksource_hz2mult(2000000, timer_cs.shift);
> +
> + return clocksource_register(&timer_cs);
> +}
> +
> +#ifdef CONFIG_SMP
> +static void percpu_ce_setup(enum clock_event_mode mode,
> + struct clock_event_device *evt)
> +{
> + int cpu = __first_cpu(evt->cpumask);
> +
> + switch (mode) {
> + case CLOCK_EVT_MODE_PERIODIC:
> + load_profile_irq(cpu, 2000000/HZ);
> + break;
> + case CLOCK_EVT_MODE_ONESHOT:
> + case CLOCK_EVT_MODE_SHUTDOWN:
> + case CLOCK_EVT_MODE_UNUSED:
> + load_profile_irq(cpu, 0);
> + break;
> + default:
> + break;
> + }
> }
>
> +static int percpu_ce_set_next_event(unsigned long delta,
> + struct clock_event_device *evt)
> +{
> + int cpu = __first_cpu(evt->cpumask);
> + unsigned int next = (unsigned int)delta;
> +
> + load_profile_irq(cpu, next);
> + return 0;
> +}
> +
> +void register_percpu_ce(int cpu)
> +{
> + struct clock_event_device *ce = &per_cpu(percpu_ce, cpu);
> + unsigned int features = CLOCK_EVT_FEAT_PERIODIC;
> +
> + if (sparc_irq_config.features & PERCPU_CE_CAN_ONESHOT)
> + features |= CLOCK_EVT_FEAT_ONESHOT;
> +
> + ce->name = "percpu_ce";
> + ce->rating = 200;
> + ce->features = features;
> + ce->set_mode = percpu_ce_setup;
> + ce->set_next_event = percpu_ce_set_next_event;
> + ce->cpumask = cpumask_of(cpu);
> + ce->shift = 32;
> + ce->mult = div_sc(2000000, NSEC_PER_SEC, ce->shift);
> + ce->max_delta_ns = clockevent_delta2ns(2000000, ce);
> + ce->min_delta_ns = clockevent_delta2ns(100, ce);
> +
> + clockevents_register_device(ce);
> +}
> +#endif
> +
> static unsigned char mostek_read_byte(struct device *dev, u32 ofs)
> {
> struct platform_device *pdev = to_platform_device(dev);
> @@ -196,42 +342,36 @@ static int __init clock_init(void)
> */
> fs_initcall(clock_init);
>
> -
> -u32 sbus_do_gettimeoffset(void)
> -{
> - unsigned long val = *master_l10_counter;
> - unsigned long usec = (val >> 10) & 0x1fffff;
> -
> - /* Limit hit? */
> - if (val & 0x80000000)
> - usec += 1000000 / HZ;
> -
> - return usec * 1000;
> -}
> -
> -
> -u32 arch_gettimeoffset(void)
> +static void __init sparc32_late_time_init(void)
> {
> - if (unlikely(!do_arch_gettimeoffset))
> - return 0;
> - return do_arch_gettimeoffset();
> + if (sparc_irq_config.features & USES_TIMER_CE)
> + setup_timer_ce();
> + if (sparc_irq_config.features & USES_TIMER_CS)
> + setup_timer_cs();
> +#ifdef CONFIG_SMP
> + register_percpu_ce(smp_processor_id());
> +#endif
> }
>
> static void __init sbus_time_init(void)
> {
> - do_arch_gettimeoffset = sbus_do_gettimeoffset;
> -
> - btfixup();
> + get_cycles_offset = sbus_cycles_offset;
>
> - sparc_irq_config.init_timers(timer_interrupt);
> + sparc_irq_config.init_timers();
> }
>
> void __init time_init(void)
> {
> + btfixup();
> +
> + sparc_irq_config.features = 0;
> +
> if (pcic_present())
> pci_time_init();
> else
> sbus_time_init();
> +
> + late_time_init = sparc32_late_time_init;
> }
In the next patch please include relevant LEON people,
so they can try to check how to enable this on LEON.
And submit it as a new mail - so it get attention.
Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
