From: Thomas Gleixner [mailto:tglx@linutronix.de] 
Sent: Monday, October 31, 2016 8:13 PM

quoted

 
-static unsigned long nps_timer_rate;
+static unsigned long nps_timer1_freq;

This should be either in the previous patch or seperate.

Will fix in V4

....

quoted

@@ -101,3 +101,215 @@ static int __init nps_setup_clocksource(struct 

device_node *node)
 
 CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
 		       nps_setup_clocksource);
+CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clk_src, "ezchip,nps400-timer1",
+		       nps_setup_clocksource);

What's the point of this change?

I do this as preparation for next patch where we use timer0 for clockevent while timer1 is kept for clocksource. 
I realize that it is not aligned with binding document, so I will move this to next patch.

quoted

+/*
+ * Arm the timer to interrupt after @cycles  */ static void 
+nps_clkevent_timer_event_setup(unsigned int cycles) {
+	write_aux_reg(NPS_REG_TIMER0_LIMIT, cycles);
+	write_aux_reg(NPS_REG_TIMER0_CNT, 0);   /* start from 0 */

Please do not use tail comments. They break the reading flow.

Will fix in V4

quoted

+
+	write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH); 
+}
+
+static void nps_clkevent_rm_thread(bool remove_thread)

I have a hard time to understand why a remove_thread function needs a remove_thread boolean argument. Commenting things like this would be more helpful than commenting the obvious.

Sorry for that, will be commented in V4

quoted

+{
+	unsigned int cflags;
+	unsigned int enabled_threads;
+	unsigned long flags;
+	int thread;
+
+	local_irq_save(flags);

That's pointless. Both call sites have interrupts disabled.

Will be fixed in V4

...

quoted

+
+static void nps_clkevent_add_thread(bool set_event) {
+	int thread;
+	unsigned int cflags, enabled_threads;
+	unsigned long flags;
+
+	local_irq_save(flags);

Ditto.

Will be removed as well at V4

...

quoted

+static int nps_clkevent_set_next_event(unsigned long delta,
+				       struct clock_event_device *dev) {
+	struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
+	struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
+
+	nps_clkevent_add_thread(true);
+	chip->irq_unmask(&desc->irq_data);

Oh no. You are not supposed to fiddle in the guts of the interrupts from a random driver. Can you please explain what you are trying to do and why >the existing interfaces are not sufficient?

This function is assigned and used by several hooks at clock_event_device type.
Main purpose is to support oneshot timer mode and in general support NOHZ_FULL and finally TASK_ISOLATION.
The idea for this is borrowed from arch/tile timer driver that just like us aiming to support the TASK_ISOLATION.
Will it be better to replace these with irq_percpu_enable()/irq_percpu_disable() which seem to achieve quiet the same affect?
...

quoted

+static int nps_clkevent_set_periodic(struct clock_event_device *dev) 
+{
+	nps_clkevent_add_thread(false);
+	if (read_aux_reg(CTOP_AUX_THREAD_ID) == 0)
+		nps_clkevent_timer_event_setup(nps_timer0_freq / HZ);

So how is that enabling interrupts again?

I guess that in our system we never switch back to periodic.
Time infrastructure starts as periodic (I set CLOCK_EVT_FEAT_PERIODIC) and when timer is ready
state is switched to oneshot mode and never goes back to periodic.
I might be missing here, but couldn't find any place where CLOCK_EVT_STATE_PERIODIC is set but in tick_setup_periodic().
Should I call here to enable interrupts anyway?

quoted

+
+static irqreturn_t timer_irq_handler(int irq, void *dev_id) {
+	/*
+	 * Note that generic IRQ core could have passed @evt for @dev_id if
+	 * irq_set_chip_and_handler() asked for handle_percpu_devid_irq()

And why are you not doing that?

Will do that in V4

quoted

+static int __init nps_setup_clockevent(struct device_node *node) {
+	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
+	struct clk *clk;
+	int ret;
+
+	nps_timer0_irq = irq_of_parse_and_map(node, 0);
+	if (nps_timer0_irq <= 0) {
+		pr_err("clockevent: missing irq");
+		return -EINVAL;
+	}
+
+	nps_get_timer_clk(node, &nps_timer0_freq, clk);
+
+	/* Needs apriori irq_set_percpu_devid() done in intc map function */
+	ret = request_percpu_irq(nps_timer0_irq, timer_irq_handler,
+				 "Timer0 (per-cpu-tick)", evt);

This is wrong. the dev_id argument wants to be a __per_cpu pointer. So it should be &nps_clockevent_device and not a pointer to the cpu local one.

Will fix that in V4

quoted

+	if (ret) {
+		pr_err("Couldn't request irq\n");
+		clk_disable_unprepare(clk);
+		return ret;
+	}
+
+	ret = cpuhp_setup_state(CPUHP_AP_NPS_TIMER_STARTING,
+				"AP_NPS_TIMER_STARTING",

Please make this "clockevents/nps:starting"

Sorry but I can't see any similar thing all around.
Could you explain why you prefer this format for the string?

quoted

+				nps_timer_starting_cpu,
+				nps_timer_dying_cpu);
+	if (ret) {
+		pr_err("Failed to setup hotplug state");
+		clk_disable_unprepare(clk);

You leave the irq requested....

Will fix that in V4.

Many thanks for all comments.
Waiting for your answers on my few questions above.
-Noam