[Letux-kernel] jz4730-i2c - clocksource

[H. Nikolaus Schaller]

> Am 02.03.2021 um 17:08 schrieb H. Nikolaus Schaller <hns at goldelico.com>:
> 
> Hi Paul,
> I may have found something new... I did simply loop
> and collect the timer values and the sched_clock()
> in an array with later printk(). So that data collection
> is not slowed down by printk().
> 
> The interesting thing is that the timer register overflows
> and restarts - and at the same moment the sched_clock()
> jumps back. Which means that there was no cevt triggered
> snapshot in between.
> 
> My idea now was: what happens if the cevt interrupts
> are not processed correctly?
> 
> Now, ingenic-timer does:
> 
> static int ingenic_tcu_setup_cevt(unsigned int cpu)
> {
> 	struct ingenic_tcu *tcu = ingenic_tcu;
> 	struct ingenic_tcu_timer *timer = &tcu->timers[cpu];
> 	unsigned int timer_virq;
> 	struct irq_domain *domain;
> 	unsigned long rate;
> 	int err;
> 
> ...
> 	domain = irq_find_host(tcu->np);
> 	if (!domain) {
> 		err = -ENODEV;
> 		goto err_clk_disable;
> 	}
> 
> 	timer_virq = irq_create_mapping(domain, timer->channel);
> 	if (!timer_virq) {
> 		err = -EINVAL;
> 		goto err_clk_disable;
> 	}
> 
> ...
> 
> 	err = request_irq(timer_virq, ingenic_tcu_cevt_cb, IRQF_TIMER,
> 			  timer->name, timer);
> 
> 
> while ingenic-sysost.c does:
> 
> static int __init ingenic_ost_percpu_timer_init(struct device_node *np,
> 					 struct ingenic_ost *ost)
> {
> 	unsigned int timer_virq, channel = OST_CLK_PERCPU_TIMER;
> 	unsigned long rate;
> 	int err;
> 
> ...
> 
> 	timer_virq = of_irq_get(np, 0);
> 	if (!timer_virq) {
> 		err = -EINVAL;
> 		goto err_clk_disable;
> 	}
> 
> 	snprintf(ost->name, sizeof(ost->name), "OST percpu timer");
> 
> 	err = request_irq(timer_virq, ingenic_ost_cevt_cb, IRQF_TIMER,
> 			  ost->name, &ost->cevt);
> 
> So the cevt interrupt is setup differently!
> 
> Interesting is that irq_create_mapping expects some irq_hw_number_t hwirq
> but I wonder why passing the timer->channel number should be the
> right value. Well, the magic "domain" could do some translation.
> 
> The sysost uses a direct device tree based approach. Maybe nobody did care
> about OF conversion since this feature wasn't used in the jz4740 setup...
> 
> If my finding is correct this would mean that the ingenic_tcu_cevt_cb()
> is not called by the timer expiry interrupt but some other interrupt
> which happens to be fairly regular.
> 
> This would mean again that all cevt based timings are wrong and may
> come too early or too late.
> 
> I'll give it a try...

unexpected result:

[    0.000000] random: get_random_bytes called from start_kernel+0x720/0x960 with crng_init=0
[    0.000000] clocksource: ingenic-timer: mask: 0xffff max_cycles: 0xffff, max_idle_ns: 7910990 ns
[    0.000000] ingenic_tcu_init: Unable to start CPU timers: -22
[    0.000000] Failed to initialize '/timer at 10002000': -22
[    0.000000] timer_probe: no matching timers found

Maybe the code is now trying the wrong irq definition from device tree.

		interrupt-parent = <&intc>;
		interrupts = <24 23 22>;

of_irq_get(np, 0) takes line #24 which is OST0.
Ok, we should take the channel into account.

	timer_virq = of_irq_get(np, timer->channel);

Basically it turns out that of_irq_get() is a shortcut for irq_find_host+irq_create_mapping

But it only works if we remove

//		interrupt-controller;
//		#interrupt-cells = <1>;

from the TCU node...

But there is no difference..

So my theory with bad IRQ setup was again a cul-de-sac.

But it gave me another idea: I can try to log the relationship between

ingenic_tcu_cevt_set_next() and ingenic_tcu_cevt_cb()

and the OST0/1 values.

BR,
Nikolaus


> BTW: I have replaced all regmap code by direct readl/writel and IMHO
> there is a very strong argument to bypass regmap: it takes an unknown
> amount of processor cycles to read through a regmap and that it is
> used for a timer with sub-microsecond precision... Here we should
> save every unnecessary instruction to get it as fast as possible.
> 
> iMHO the same would be wise to be done for the jz4740...
> 
> BR,
> Nikolaus
>