On 19/11/2018 17:09, Manivannan Sadhasivam wrote:

Please use the relaxed accessors throughout this driver. There is zero
reason to use the non-relaxed versions here.

Now, I'm pretty sure this thing isn't correct.

	<timer = 0x00000000ffffffff>
	lo = 0xffffffff;
	<tick, timer = 0x0000000100000000>
	hi = 0x00000001;

Bingo. You cannot read a 64bit counter with only two 32bit accesses.

+
+	return ((u64)hi << 32) | lo;
+}
+
+static struct clocksource rda_clocksource = {
+	.name           = "rda-timer",
+	.rating         = 400,
+	.read           = rda_hwtimer_read,
+	.mask           = CLOCKSOURCE_MASK(64),

This is a 64bit counter? See below.

+	.flags          = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static int rda_ostimer_start(bool periodic, u64 cycles)
+{
+	u32 ctrl, load_l;
+
+	load_l = (u32)cycles;
+	ctrl = ((cycles >> 32) & 0xffffff);
+	ctrl |= RDA_OSTIMER_CTRL_LOAD | RDA_OSTIMER_CTRL_ENABLE;
+	if (periodic)
+		ctrl |= RDA_OSTIMER_CTRL_REPEAT;
+
+	/* Enable ostimer interrupt first */
+	writel(RDA_TIMER_IRQ_MASK_SET_OSTIMER,
+	       rda_timer_base + RDA_TIMER_IRQ_MASK_SET);

Is it masking or enabling the interrupt?

+
+	/* Write low 32 bits first, high 24 bits are with ctrl */

You're saying that you can only write 56 bits? This contradicts the 64bt
counter thing above.

+	writel(load_l, rda_timer_base + RDA_OSTIMER_LOADVAL_L);
+	writel(ctrl, rda_timer_base + RDA_OSTIMER_CTRL);
+
+	return 0;
+}
+
+static int rda_ostimer_stop(void)
+{
+	/* Disable ostimer interrupt first */
+	writel(0, rda_timer_base + RDA_TIMER_IRQ_MASK_SET);
+
+	writel(0, rda_timer_base + RDA_OSTIMER_CTRL);
+
+	return 0;
+}
+
+static int rda_ostimer_set_state_shutdown(struct clock_event_device *evt)
+{
+	rda_ostimer_stop();
+
+	return 0;
+}
+
+static int rda_ostimer_set_state_oneshot(struct clock_event_device *evt)
+{
+	rda_ostimer_stop();
+
+	return 0;
+}
+
+static int rda_ostimer_set_state_periodic(struct clock_event_device *evt)
+{
+	unsigned long cycles_per_jiffy;
+
+	rda_ostimer_stop();
+
+	cycles_per_jiffy = ((unsigned long long)NSEC_PER_SEC / HZ *
+			     evt->mult) >> evt->shift;
+	rda_ostimer_start(true, cycles_per_jiffy);
+
+	return 0;
+}
+
+static int rda_ostimer_tick_resume(struct clock_event_device *evt)
+{
+	return 0;
+}
+
+static int rda_ostimer_set_next_event(unsigned long evt,
+				      struct clock_event_device *ev)
+{
+	rda_ostimer_start(false, evt);
+
+	return 0;
+}
+
+static struct clock_event_device rda_clockevent = {
+	.name			= "rda-ostimer",
+	.rating			= 250,
+	.features		= CLOCK_EVT_FEAT_PERIODIC |
+				  CLOCK_EVT_FEAT_ONESHOT |
+				  CLOCK_EVT_FEAT_DYNIRQ,
+	.set_state_shutdown	= rda_ostimer_set_state_shutdown,
+	.set_state_oneshot	= rda_ostimer_set_state_oneshot,
+	.set_state_periodic	= rda_ostimer_set_state_periodic,
+	.tick_resume		= rda_ostimer_tick_resume,
+	.set_next_event		= rda_ostimer_set_next_event,
+};
+
+static irqreturn_t rda_ostimer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+
+	/* clear timer int */
+	writel(RDA_TIMER_IRQ_CLR_OSTIMER, rda_timer_base + RDA_TIMER_IRQ_CLR);
+
+	if (evt->event_handler)
+		evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static int __init rda_timer_init(struct device_node *node)
+{
+	unsigned long rate = 2000000;
+	int ostimer_irq, ret;
+
+	rda_timer_base = of_io_request_and_map(node, 0, "rda-timer");
+	if (IS_ERR(rda_timer_base)) {
+		pr_err("Can't map timer registers");
+		return PTR_ERR(rda_timer_base);
+	}
+
+	ostimer_irq = of_irq_get_byname(node, "ostimer");
+	if (ostimer_irq <= 0) {
+		pr_err("Can't parse ostimer IRQ");
+		return -EINVAL;

Leaking IO space.

+	}
+
+	clocksource_register_hz(&rda_clocksource, rate);
+
+	ret = request_irq(ostimer_irq, rda_ostimer_interrupt, IRQF_TIMER,
+			  "rda-ostimer", &rda_clockevent);
+	if (ret) {
+		pr_err("failed to request irq %d\n", ostimer_irq);
+		return ret;

Same here.

+	}
+
+	irq_force_affinity(ostimer_irq, cpumask_of(0));

Why?

+
+	rda_clockevent.cpumask = cpumask_of(0);
+	rda_clockevent.irq = ostimer_irq;
+	clockevents_config_and_register(&rda_clockevent, rate,
+					0x2, 0xffffffff);
+
+	return 0;
+}
+
+TIMER_OF_DECLARE(rda8810pl, "rda,8810pl-timer", rda_timer_init);

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...