Re: [PATCH v7 03/15] clocksource: New RISC-V SBI timer driver

[Thomas Gleixner]

On Mon, 31 Jul 2017, Palmer Dabbelt wrote:

> +void timer_riscv_init(int cpu_id,
> +		      unsigned long riscv_timebase,
> +		      unsigned long long (*rdtime)(struct clocksource *),
> +		      int (*next)(unsigned long, struct clock_event_device*))
> +{
> +	struct clocksource *cs = &per_cpu(clock_source, cpu_id);
> +	struct clock_event_device *ce = &per_cpu(clock_event, cpu_id);

per_cpu_ptr() please

> +	*cs = (struct clocksource) {
> +		.name = "riscv_clocksource",
> +		.rating = 300,
> +		.read = rdtime,
> +		.mask = CLOCKSOURCE_MASK(BITS_PER_LONG),
> +		.flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +	};

Hmm, why do you try to register clocksources per cpu. The core code will
only use the first one registered and the others are just ballast. You
should just have that once at boot time and not on a per cpu basis.

> +	clocksource_register_hz(cs, riscv_timebase);

Clock events are per CPU though.

> +	*ce = (struct clock_event_device){
> +		.name = "riscv_timer_clockevent",
> +		.features = CLOCK_EVT_FEAT_ONESHOT,
> +		.rating = 300,
> +		.cpumask = cpumask_of(cpu_id),
> +		.set_next_event = next,
> +		.set_state_oneshot  = NULL,
> +		.set_state_shutdown = NULL,
> +	};

I'm not a big fan of that. What's wrong with just making it:

static DEFINE_PER_CPU(struct clock_event_device, clock_event) = {
	.name		= "riscv_timer_clockevent",
	.features	= CLOCK_EVT_FEAT_ONESHOT,
	.rating		= 300,
};

and here just do:

	ce->cpumask = cpumask_of(cpu_id);
	ce->set_next_event = next;

> +	clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff);

Hmm?

> +/*
> + * Looks up the clocksource or clock_even_device that cooresponds the given
> + * hart.
> + */
> +struct clocksource *timer_riscv_source(int cpuid);
> +struct clock_event_device *timer_riscv_device(int cpu_id);

What for? You register and forget them ....

Thanks,

	tglx