On 08/15, Domenico Andreoli wrote:

+
+static inline void __iomem *to_load(struct bcm4760_timer *timer)
+{
+	return timer->base + TIMER_LOAD_OFFSET;
+}
+
+static inline void __iomem *to_control(struct bcm4760_timer *timer)
+{
+	return timer->base + TIMER_CONTROL_OFFSET;
+}
+
+static inline void __iomem *to_intclr(struct bcm4760_timer *timer)
+{
+	return timer->base + TIMER_INTCLR_OFFSET;
+}
+
+static inline void __iomem *to_ris(struct bcm4760_timer *timer)
+{
+	return timer->base + TIMER_RIS_OFFSET;
+}
+
+static inline void __iomem *to_mis(struct bcm4760_timer *timer)
+{
+	return timer->base + TIMER_MIS_OFFSET;
+}

Style Nit: This is new. Usually people either make a
<my_driver>_{readl,writel}() function that takes the struct and
an offset or they just add the offset directly in their
readl/writel calls. Can you do that? Probably save some lines of
code.

+static irqreturn_t bcm4760_timer_interrupt(int irq, void *dev_id)
+{
+	struct bcm4760_timer *timer = dev_id;
+	void (*event_handler)(struct clock_event_device *);
+
+	/* check the (masked) interrupt status */
+	if (!readl_relaxed(to_mis(timer)))
+		return IRQ_NONE;
+
+	/* clear the timer interrupt */
+	writel_relaxed(1, to_intclr(timer));
+
+	event_handler = ACCESS_ONCE(timer->evt.event_handler);
+	if (event_handler)
+		event_handler(&timer->evt);

This is unfortunate. Do you have a pending timer interrupt left
by the bootloader?

+
+	return IRQ_HANDLED;
+}
+
+static void __init bcm4760_init_time(struct device_node *node)
+{
+	void __iomem *base;
+	u32 freq = 24000000;

Why have freq in the DT binding at all then?

+	int irq;
+	struct bcm4760_timer *timer;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("Can't remap timer registers");
+
+	timer = kzalloc(sizeof(*timer), GFP_KERNEL);
+	if (!timer)
+		panic("Can't allocate timer struct\n");
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("Can't parse timer IRQ");
+
+	timer->base = base;
+	timer->evt.name = node->name;
+	timer->evt.rating = 300;
+	timer->evt.features = CLOCK_EVT_FEAT_ONESHOT;
+	timer->evt.set_mode = bcm4760_timer_set_mode;
+	timer->evt.set_next_event = bcm4760_timer_set_next_event;
+	timer->evt.cpumask = cpumask_of(0);
+	timer->act.name = node->name;
+	timer->act.flags = IRQF_TIMER | IRQF_SHARED;
+	timer->act.dev_id = timer;
+	timer->act.handler = bcm4760_timer_interrupt;
+
+	if (setup_irq(irq, &timer->act))
+		panic("Can't set up timer IRQ\n");
+
+	clockevents_config_and_register(&timer->evt, freq, 0xf, 0xffffffff);

If you switch this registration and the setup_irq() call you
shouldn't need the ACCESS_ONCE() and that check in the irq handler.
Please switch the order and or clear the interrupt before
registering the clockevent and remove the checks in the interrupt
handler.

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