Re: [PATCH V6 2/7] clocksource: tegra: add Tegra210 timer support

From: Joseph Lo
Date: Tue Feb 19 2019 - 04:01:10 EST

On 2/18/19 5:39 PM, Daniel Lezcano wrote:
On 18/02/2019 10:01, Joseph Lo wrote:
On 2/15/19 11:14 PM, Daniel Lezcano wrote:
On 01/02/2019 17:16, Joseph Lo wrote:
Add support for the Tegra210 timer that runs at oscillator clock
(TMR10-TMR13). We need these timers to work as clock event device and to
replace the ARMv8 architected timer due to it can't survive across the
power cycle of the CPU core or CPUPORESET signal. So it can't be a
source when CPU suspends in power down state.

Also convert the original driver to use timer-of API.

Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: linux-kernel@xxxxxxxxxxxxxxx
Signed-off-by: Joseph Lo <josephl@xxxxxxxxxx>
Acked-by: Thierry Reding <treding@xxxxxxxxxx>
Acked-by: Jon Hunter <jonathanh@xxxxxxxxxx>
 * refine the timer defines
 * add ack tag from Jon.
 * add ack tag from Thierry
 * merge timer-tegra210.c in previous version into timer-tegra20.c
 * use timer-of API
 * add error clean-up code
+ÂÂÂ for_each_possible_cpu(cpu) {
+ÂÂÂÂÂÂÂ struct timer_of *cpu_to;
+ÂÂÂÂÂÂÂ cpu_to = per_cpu_ptr(&tegra_to, cpu);
+ÂÂÂÂÂÂÂ cpu_to->of_base.base = timer_reg_base +
+ÂÂÂÂÂÂÂ cpu_to->of_clk.rate = timer_of_rate(to);
+ÂÂÂÂÂÂÂ cpu_to->clkevt.cpumask = cpumask_of(cpu);
+ÂÂÂÂÂÂÂ cpu_to->clkevt.irq =
+ÂÂÂÂÂÂÂÂÂÂÂ irq_of_parse_and_map(np, IRQ_IDX_FOR_CPU(cpu));
+ÂÂÂÂÂÂÂ if (!cpu_to->clkevt.irq) {
+ÂÂÂÂÂÂÂÂÂÂÂ pr_err("%s: can't map IRQ for CPU%d\n",
+ÂÂÂÂÂÂÂÂÂÂÂ goto out;
+ÂÂÂÂÂÂÂ irq_set_status_flags(cpu_to->clkevt.irq, IRQ_NOAUTOEN);
+ÂÂÂÂÂÂÂ ret = request_irq(cpu_to->clkevt.irq, tegra_timer_isr,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ cpu_to->, &cpu_to->clkevt);
+ÂÂÂÂÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂÂÂÂÂ pr_err("%s: cannot setup irq %d for CPU%d\n",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __func__, cpu_to->clkevt.irq, cpu);
+ÂÂÂÂÂÂÂÂÂÂÂ goto out_irq;
+ÂÂÂ }

You should configure the timer in the tegra_timer_setup() function
instead of using this cpu loop.

I think I still need to leave 'irq_of_parse_and_map' and 'request_irq'
here. Is that ok?

Perhaps you can store the np pointer in the private data structure of
timer-of and let the timer_of API to retrieve the irq in the cpuhp

irq_of_parse_and_map will be called by timer-of.

I'm not sure irq_set_status_flags really operates on the irq because it
is called after request_irq.

I did some experiments today. The 'irq_of_parse_and_map', 'request_irq' and 'setup_irq' are not able to run in the atomic section that tegra_timer_setup would be triggered in.

So I think I still need to leave the IRQ configuration code here in the loop. Should I move others to 'tegra_timer_setup' or just keep as the same in this patch?