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

From: Joseph Lo
Date: Mon Feb 18 2019 - 04:01:35 EST

Next message: Michal Hocko: "Re: [LKP] efad4e475c [ 40.308255] Oops: 0000 [#1] PREEMPT SMP PTI"
Previous message: Daniel Vetter: "Re: [PATCH] drm/drm_vm: Mark expected switch fall-throughs"
Next in thread: Daniel Lezcano: "Re: [PATCH V6 2/7] clocksource: tegra: add Tegra210 timer support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 wake-up
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>
---
v6:
* refine the timer defines
* add ack tag from Jon.
v5:
* add ack tag from Thierry
v4:
* merge timer-tegra210.c in previous version into timer-tegra20.c
v3:
* use timer-of API
v2:
* add error clean-up code
---
drivers/clocksource/Kconfig | 2 +-
drivers/clocksource/timer-tegra20.c | 371 ++++++++++++++++++++--------
include/linux/cpuhotplug.h | 1 +
3 files changed, 270 insertions(+), 104 deletions(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index a9e26f6a81a1..6af78534a285 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -131,7 +131,7 @@ config SUN5I_HSTIMER
config TEGRA_TIMER
bool "Tegra timer driver" if COMPILE_TEST
select CLKSRC_MMIO
- depends on ARM

This will break because the delay functions are defined in
arch/arm/include/asm/delay.h and the 01.org will try to compile the
driver on x86.

You may want to add 'depends on ARM && ARM64'

OK, I think it's 'depends on ARM || ARM64'.
Will fix.

+ select TIMER_OF
help
Enables support for the Tegra driver.

[snip]

-
static struct timespec64 persistent_ts;
static u64 persistent_ms, last_persistent_ms;

Did you check the above changes are still relevant after commit
39232ed5a1793f67 and after doing a change similar to
commit 1569557549697207e523 ?

Yes, just check both commits. I think it's okay to use the same. But need another patch to do that, this patch only adds new support for Tegra210. Doesn't touch the original code.

static struct delay_timer tegra_delay_timer;

[snip]

+#ifdef CONFIG_ARM64
+static DEFINE_PER_CPU(struct timer_of, tegra_to) = {
+ .flags = TIMER_OF_CLOCK | TIMER_OF_BASE,
+
+ .clkevt = {
+ .name = "tegra_timer",
+ .rating = 460,
+ .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,

CLOCK_EVT_FEAT_DYNIRQ ?

Yes, good catch.

+ .set_next_event = tegra_timer_set_next_event,
+ .set_state_shutdown = tegra_timer_shutdown,
+ .set_state_periodic = tegra_timer_set_periodic,
+ .set_state_oneshot = tegra_timer_shutdown,
+ .tick_resume = tegra_timer_shutdown,
+ },
+};

[snip]

-static unsigned long tegra_delay_timer_read_counter_long(void)
+static int tegra_timer_suspend(void)
{
- return readl(timer_reg_base + TIMERUS_CNTR_1US);
+#ifdef CONFIG_ARM64

Please do not add those #ifdef but function stubs.

+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ struct timer_of *to = per_cpu_ptr(&tegra_to, cpu);
+ void __iomem *reg_base = timer_of_base(to);
+
+ writel(TIMER_PCR_INTR_CLR, reg_base + TIMER_PCR);
+ }
+#else
+ void __iomem *reg_base = timer_of_base(&tegra_to);
+
+ writel(TIMER_PCR_INTR_CLR, reg_base + TIMER_PCR);
+#endif
+
+ return 0;
}
-static irqreturn_t tegra_timer_interrupt(int irq, void *dev_id)
+static void tegra_timer_resume(void)
{
- struct clock_event_device *evt = (struct clock_event_device *)dev_id;
- timer_writel(1<<30, TIMER3_BASE + TIMER_PCR);
- evt->event_handler(evt);
- return IRQ_HANDLED;
+ writel(usec_config, timer_reg_base + TIMERUS_USEC_CFG);
}
-static struct irqaction tegra_timer_irq = {
- .name = "timer0",
- .flags = IRQF_TIMER | IRQF_TRIGGER_HIGH,
- .handler = tegra_timer_interrupt,
- .dev_id = &tegra_clockevent,
+static struct syscore_ops tegra_timer_syscore_ops = {
+ .suspend = tegra_timer_suspend,
+ .resume = tegra_timer_resume,
};

It will be nicer to use the suspend/resume callbacks defined in the
clockevent structure, so you can use generic as there are multiple
clockevents defined for the tegra210, thus multiple timer-of
encapsulating them. When the suspend/resume callbacks are called, they
have the clock_event pointer and you can use it to retrieve the timer-of
and then the base address. At the end, the callbacks will end up the
same for tegra20 and tegra210.

Very good suggestion, will follow up.

-static int __init tegra20_init_timer(struct device_node *np)
+static int tegra_timer_init(struct device_node *np, struct timer_of *to)

[snip]

+ 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 + TIMER_BASE_FOR_CPU(cpu);
+ 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",
+ __func__, cpu);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ irq_set_status_flags(cpu_to->clkevt.irq, IRQ_NOAUTOEN);
+ ret = request_irq(cpu_to->clkevt.irq, tegra_timer_isr,
+ IRQF_TIMER | IRQF_NOBALANCING,
+ cpu_to->clkevt.name, &cpu_to->clkevt);
+ if (ret) {
+ pr_err("%s: cannot setup irq %d for CPU%d\n",
+ __func__, cpu_to->clkevt.irq, cpu);
+ ret = -EINVAL;
+ 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?

+
+ cpuhp_setup_state(CPUHP_AP_TEGRA_TIMER_STARTING,
+ "AP_TEGRA_TIMER_STARTING", tegra_timer_setup,
+ tegra_timer_stop);
+
+ return ret;
+
+out_irq:
+ for_each_possible_cpu(cpu) {
+ struct timer_of *cpu_to;
+
+ cpu_to = per_cpu_ptr(&tegra_to, cpu);
+ if (cpu_to->clkevt.irq) {
+ free_irq(cpu_to->clkevt.irq, &cpu_to->clkevt);
+ irq_dispose_mapping(cpu_to->clkevt.irq);
+ }
}
+out:
+ timer_of_cleanup(to);
+ return ret;
+}
+TIMER_OF_DECLARE(tegra210_timer, "nvidia,tegra210-timer", tegra210_timer_init);
+#else /* CONFIG_ARM */

Don't use the macro to select one or another. Just define the functions
and let the init postcalls to free the memory.

Okay, I think I can move 'TIMER_OF_DECLARE' out of the ifdef. They will be something like below. And change tegraxxx_init_timer to tegra_init_timer.

TIMER_OF_DECLARE(tegra210_timer, "nvidia,tegra210-timer", tegra_timer_init);
TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra_timer_init);

Is that ok?

Thanks for reviewing,
Joseph

+static int __init tegra20_init_timer(struct device_node *np)
+{
+ int ret = 0;
+
+ ret = tegra_timer_init(np, &tegra_to);
+ if (ret < 0)
+ goto out;
- sched_clock_register(tegra_read_sched_clock, 32, 1000000);
+ tegra_to.of_base.base = timer_reg_base + TIMER_BASE_FOR_CPU(0);
+ tegra_to.of_clk.rate = 1000000; /* microsecond timer */
+ sched_clock_register(tegra_read_sched_clock, 32,
+ timer_of_rate(&tegra_to));
ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
- "timer_us", 1000000, 300, 32,
- clocksource_mmio_readl_up);
+ "timer_us", timer_of_rate(&tegra_to),
+ 300, 32, clocksource_mmio_readl_up);
if (ret) {
pr_err("Failed to register clocksource\n");
- return ret;
+ goto out;
}
tegra_delay_timer.read_current_timer =
tegra_delay_timer_read_counter_long;
- tegra_delay_timer.freq = 1000000;
+ tegra_delay_timer.freq = timer_of_rate(&tegra_to);
register_current_timer_delay(&tegra_delay_timer);
- ret = setup_irq(tegra_timer_irq.irq, &tegra_timer_irq);
- if (ret) {
- pr_err("Failed to register timer IRQ: %d\n", ret);
- return ret;
- }
+ clockevents_config_and_register(&tegra_to.clkevt,
+ timer_of_rate(&tegra_to),
+ 0x1,
+ 0x1fffffff);
- tegra_clockevent.cpumask = cpu_possible_mask;
- tegra_clockevent.irq = tegra_timer_irq.irq;
- clockevents_config_and_register(&tegra_clockevent, 1000000,
- 0x1, 0x1fffffff);
+ return ret;
+out:
+ timer_of_cleanup(&tegra_to);
- return 0;
+ return ret;
}
TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
@@ -261,3 +425,4 @@ static int __init tegra20_init_rtc(struct device_node *np)
return register_persistent_clock(tegra_read_persistent_clock64);
}
TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
+#endif
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index fd586d0301e7..e78281d07b70 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -121,6 +121,7 @@ enum cpuhp_state {
CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
CPUHP_AP_ARM_TWD_STARTING,
CPUHP_AP_QCOM_TIMER_STARTING,
+ CPUHP_AP_TEGRA_TIMER_STARTING,
CPUHP_AP_ARMADA_TIMER_STARTING,
CPUHP_AP_MARCO_TIMER_STARTING,
CPUHP_AP_MIPS_GIC_TIMER_STARTING,

Next message: Michal Hocko: "Re: [LKP] efad4e475c [ 40.308255] Oops: 0000 [#1] PREEMPT SMP PTI"
Previous message: Daniel Vetter: "Re: [PATCH] drm/drm_vm: Mark expected switch fall-throughs"
Next in thread: Daniel Lezcano: "Re: [PATCH V6 2/7] clocksource: tegra: add Tegra210 timer support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]