On 12/11/13 10:00, Ivan Khoronzhuk wrote:

[...]

+#include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#define TIMER_NAME			"timer64-event"

Why not have keystone in the name? Or should this file be renamed?

+
+static inline u32 keystone_timer_readl(unsigned long rg)
+{
+	return readl(timer.base + rg);
+}
+
+static inline void keystone_timer_writel(u32 val, unsigned long rg)
+{
+	writel(val, timer.base + rg);
+}

It's probably better to use the relaxed variants here to avoid any
memory barriers that aren't necessary.

+
+static irqreturn_t keystone_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = &timer.event_dev;
+
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}

Why not just pass the evt pointer via dev_id? That would be faster and
we want this function to be as fast as possible.

+
+static int keystone_set_next_event(unsigned long cycles,
+				  struct clock_event_device *evt)
+{
+	return keystone_timer_config(cycles, evt->mode);
+}
+
+static void keystone_set_mode(enum clock_event_mode mode,
+			     struct clock_event_device *evt)
+{
+	u64 period;
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		period = timer.clk_rate / (HZ);

Why not just store the period instead of calculating it here?

+		keystone_timer_config(period, CLOCK_EVT_MODE_PERIODIC);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_ONESHOT:
+		keystone_timer_config(0, CLOCK_EVT_MODE_SHUTDOWN);
+	default:
+		break;
+	}
+}
+
+static void __init keystone_timer_init(struct device_node *np)
+{

[...]

+
+	/* enable timer interrupts */
+	keystone_timer_writel(INTCTLSTAT_ENINT_MASK, INTCTLSTAT);
+
+	keystone_timer_config(0, CLOCK_EVT_MODE_UNUSED);
+
+	/* init irqaction */
+	irqaction->flags = IRQF_TIMER;
+	irqaction->handler = keystone_timer_interrupt;
+	irqaction->name = TIMER_NAME;
+	irqaction->dev_id = (void *)&timer;
+	error = setup_irq(irq, irqaction);
+	if (error) {
+		pr_err("%s: failed to setup irq\n", __func__);
+		goto err;
+	}

Why not use request_irq() here? This isn't called really early, is it?

+
+	/* setup clockevent */
+	event_dev->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
+	event_dev->set_next_event = keystone_set_next_event;
+	event_dev->set_mode = keystone_set_mode;
+	event_dev->cpumask = cpu_all_mask;
+	event_dev->owner = THIS_MODULE;
+	event_dev->name = TIMER_NAME;

Please assign the irq here too.

+
+	clockevents_config_and_register(event_dev, rate, 1, ULONG_MAX);
+
+	pr_info("keystone timer clock @%lu Hz\n", rate);
+	return;
+err:
+	clk_put(clk);
+	iounmap(timer.base);
+}
+
+CLOCKSOURCE_OF_DECLARE(keystone_timer, "ti,keystone-timer",
+					keystone_timer_init);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation