RE: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Noam Camus]

> From: Daniel Lezcano [mailto:daniel.lezcano@xxxxxxxxxx] 
> Sent: Monday, November 14, 2016 1:23 PM


>> + */
>> +static void nps_clkevent_rm_thread(bool remove_thread) {
>> +	unsigned int cflags;
>> +	unsigned int enabled_threads = 0;
>> +	int thread;
>> +
>> +	hw_schd_save(&cflags);

>I'm not used with hardware scheduling. Can you explain why this is needed here ? What >window race we want to close ?
We are using HW scheduling off/on in order to keep consistency of auxiliary registers shared among HW threads within the same core.
Example to such registers NPS_REG_TIMER0_TSI and NPS_REG_TIMER0_CTRL.
Since update procedure of these registers is not atomic we use save/restore macros to turn off/on the HW scheduling. This way we insure that no HW scheduling occurs and another HW thread (represented as another CPU) will execute in this same critical code path.
If we take for example nps_clkevent_add_thread() we can see that we are doing some read modify write to NPS_REG_TIMER0_TSI and optionally writing to NPS_REG_TIMER0_CTRL. This flow should be atomic and is protected by our save/restore macros.
Do note that interrupts are disabled at this point so we are safe from all asynchronous events. 

...
>> +static void nps_clkevent_add_thread(bool set_event) {
>> +	int thread;
>> +	unsigned int cflags, enabled_threads;
>> +
>> +	hw_schd_save(&cflags);
>> +
>> +	/* add thread to TSI1 */
>> +	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
>> +	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
>> +	enabled_threads |= (1 << thread);
>> +	write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
>> +
>> +	/* set next timer event */
>> +	if (set_event)
>> +		write_aux_reg(NPS_REG_TIMER0_CTRL,
>> +			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
>> +
>> +	hw_schd_restore(cflags);
>> +}

>Not sure the boolean parameters for *_rm_thread and *_add_thread helps to clarify the code. Depending on the race window with hw_schd_save/restore We should be able to simplify it.
I am not sure I am following you here, how race window may simplify this code?
If those routines will get no parameter I can't determine when to add or not (same as remove).
...
>> +
>> +static DEFINE_PER_CPU(struct clock_event_device, nps_clockevent_device) = {
>> +	.name				=	"NPS Timer0",
>> +	.features			=	CLOCK_EVT_FEAT_ONESHOT |
>> +						CLOCK_EVT_FEAT_PERIODIC,
>> +	.rating				=	300,
>> +	.set_next_event			=	nps_clkevent_set_next_event,
>> +	.set_state_periodic		=	nps_clkevent_set_periodic,
>> +	.set_state_oneshot		=	nps_clkevent_set_oneshot,
>> +	.set_state_oneshot_stopped	=	nps_clkevent_timer_shutdown,
>> +	.set_state_shutdown		=	nps_clkevent_timer_shutdown,

>Doesn't set_state_shutdown and set_state_oneshot_stopped need to remove the HW thread from the TSI ?
You are correct, I will fix that.

Thanks
--Noam