Skip to contentSkip to conversation

NVIDIA Tegra clocksource driver improvements

PATCH Superseded Dmitry Osipenko 3 participants 15 messages May 24, 2019 – Jun 3, 2019

Subsystems (2)
Tegra architecture support Clocksource, clockevent drivers

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Dmitry Osipenko

author
#0 lore raw

v3 cover NVIDIA Tegra clocksource driver improvements

Hello,

This series primarily unifies the driver code across all Tegra SoC generations. In a result the clocksources are allocated per-CPU on older Tegra's and have a higher rating than the arch-timer, the newer Tegra210 is getting support for microsecond clocksource and the driver's code is getting much cleaner. Note that arch-timer usage is discouraged on all Tegra's due to the time jitter caused by the CPU frequency scaling.

The series was extensively tested on Tegra20 and Tegra30.

Changelog:

v3: Fixed compilation on ARM64. Turned out that it doesn't have the
delay-timer, thanks to Nicolas Chauvet for the report.

Added new "Support COMPILE_TEST universally" patch for better compile-test coverage.

v2: Rebased on recent linux-next. Now all of #ifdef's are removed from the
code due to the recent patch that generalized persistent clocksource.

Couple other minor cosmetic changes.

Dmitry Osipenko (8):
clocksource/drivers/tegra: Support per-CPU timers on all Tegra's clocksource/drivers/tegra: Unify timer code clocksource/drivers/tegra: Reset hardware state on init clocksource/drivers/tegra: Replace readl/writel with relaxed versions clocksource/drivers/tegra: Release all IRQ's on request_irq() error clocksource/drivers/tegra: Minor code clean up clocksource/drivers/tegra: Use SPDX identifier clocksource/drivers/tegra: Support COMPILE_TEST universally

 drivers/clocksource/Kconfig         |   4 +-
 drivers/clocksource/timer-tegra20.c | 276 +++++++++++++---------------
 2 files changed, 127 insertions(+), 153 deletions(-)
Signature · 1 line
2.21.0
author
#1 lore raw

PATCH v3 1/8 clocksource/drivers/tegra: Support per-CPU timers on all Tegra's

Assign TMR1-4 per-CPU core on 32bit Tegra's in a way it is done for Tegra210. In a result each core can handle its own timer events, less code is unique to ARM64 and Tegra's clock events driver now has higher rating on all Tegra's, replacing the ARM's TWD timer which isn't very accurate due to the clock rate jitter caused by CPU frequency scaling.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/clocksource/timer-tegra20.c | 120 ++++++++++------------------
 1 file changed, 43 insertions(+), 77 deletions(-)
drivers/clocksource/timer-tegra20.c +43 -77
index 919b3568c495..58e8bb6deac9 100644
@@ -49,13 +49,18 @@
 #define TIMER_PCR_INTR_CLR	BIT(30)
 
 #ifdef CONFIG_ARM
-#define TIMER_CPU0		0x50 /* TIMER3 */
+#define TIMER_CPU0		0x00 /* TIMER1 */
+#define TIMER_CPU2		0x50 /* TIMER3 */
+#define TIMER1_IRQ_IDX		0
+#define IRQ_IDX_FOR_CPU(cpu)	(TIMER1_IRQ_IDX + cpu)
+#define TIMER_BASE_FOR_CPU(cpu)	\
+	(((cpu) & 1) * 8 + ((cpu) < 2 ? TIMER_CPU0 : TIMER_CPU2))
 #else
 #define TIMER_CPU0		0x90 /* TIMER10 */
 #define TIMER10_IRQ_IDX		10
 #define IRQ_IDX_FOR_CPU(cpu)	(TIMER10_IRQ_IDX + cpu)
-#endif
 #define TIMER_BASE_FOR_CPU(cpu) (TIMER_CPU0 + (cpu) * 8)
+#endif
 
 static u32 usec_config;
 static void __iomem *timer_reg_base;

@@ -118,7 +123,6 @@ static void tegra_timer_resume(struct clock_event_device *evt)
 	writel(usec_config, timer_reg_base + TIMERUS_USEC_CFG);
 }
 
-#ifdef CONFIG_ARM64
 static DEFINE_PER_CPU(struct timer_of, tegra_to) = {
 	.flags = TIMER_OF_CLOCK | TIMER_OF_BASE,
 

@@ -159,33 +163,8 @@ static int tegra_timer_stop(unsigned int cpu)
 
 	return 0;
 }
-#else /* CONFIG_ARM */
-static struct timer_of tegra_to = {
-	.flags = TIMER_OF_CLOCK | TIMER_OF_BASE | TIMER_OF_IRQ,
-
-	.clkevt = {
-		.name = "tegra_timer",
-		.rating	= 300,
-		.features = CLOCK_EVT_FEAT_ONESHOT |
-			    CLOCK_EVT_FEAT_PERIODIC |
-			    CLOCK_EVT_FEAT_DYNIRQ,
-		.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,
-		.suspend = tegra_timer_suspend,
-		.resume = tegra_timer_resume,
-		.cpumask = cpu_possible_mask,
-	},
-
-	.of_irq = {
-		.index = 2,
-		.flags = IRQF_TIMER | IRQF_TRIGGER_HIGH,
-		.handler = tegra_timer_isr,
-	},
-};
 
+#ifdef CONFIG_ARM
 static u64 notrace tegra_read_sched_clock(void)
 {
 	return readl(timer_reg_base + TIMERUS_CNTR_1US);

@@ -222,10 +201,12 @@ static struct clocksource suspend_rtc_clocksource = {
 };
 #endif
 
-static int tegra_timer_common_init(struct device_node *np, struct timer_of *to)
+static int tegra_init_timer(struct device_node *np, bool tegra20)
 {
-	int ret = 0;
+	struct timer_of *to;
+	int cpu, ret;
 
+	to = this_cpu_ptr(&tegra_to);
 	ret = timer_of_init(np, to);
 	if (ret < 0)
 		goto out;

@@ -267,29 +248,19 @@ static int tegra_timer_common_init(struct device_node *np, struct timer_of *to)
 		goto out;
 	}
 
-	writel(usec_config, timer_of_base(to) + TIMERUS_USEC_CFG);
-
-out:
-	return ret;
-}
-
-#ifdef CONFIG_ARM64
-static int __init tegra_init_timer(struct device_node *np)
-{
-	int cpu, ret = 0;
-	struct timer_of *to;
-
-	to = this_cpu_ptr(&tegra_to);
-	ret = tegra_timer_common_init(np, to);
-	if (ret < 0)
-		goto out;
+	writel(usec_config, timer_reg_base + TIMERUS_USEC_CFG);
 
 	for_each_possible_cpu(cpu) {
-		struct timer_of *cpu_to;
+		struct timer_of *cpu_to = per_cpu_ptr(&tegra_to, cpu);
+
+		/*
+		 * TIMER1-9 are fixed to 1MHz, TIMER10-13 are running off the
+		 * parent clock.
+		 */
+		if (tegra20)
+			cpu_to->of_clk.rate = 1000000;
 
-		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));

@@ -331,43 +302,39 @@ static int __init tegra_init_timer(struct device_node *np)
 	timer_of_cleanup(to);
 	return ret;
 }
+
+#ifdef CONFIG_ARM64
+static int __init tegra210_init_timer(struct device_node *np)
+{
+	return tegra_init_timer(np, false);
+}
+TIMER_OF_DECLARE(tegra210_timer, "nvidia,tegra210-timer", tegra210_init_timer);
 #else /* CONFIG_ARM */
-static int __init tegra_init_timer(struct device_node *np)
+static int __init tegra20_init_timer(struct device_node *np)
 {
-	int ret = 0;
+	struct timer_of *to;
+	int err;
 
-	ret = tegra_timer_common_init(np, &tegra_to);
-	if (ret < 0)
-		goto out;
+	err = tegra_init_timer(np, true);
+	if (err)
+		return err;
 
-	tegra_to.of_base.base = timer_reg_base + TIMER_BASE_FOR_CPU(0);
-	tegra_to.of_clk.rate = 1000000; /* microsecond timer */
+	to = this_cpu_ptr(&tegra_to);
 
 	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", timer_of_rate(&tegra_to),
+			     timer_of_rate(to));
+	err = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
+				    "timer_us", timer_of_rate(to),
 				    300, 32, clocksource_mmio_readl_up);
-	if (ret) {
-		pr_err("Failed to register clocksource\n");
-		goto out;
-	}
+	if (err)
+		pr_err("Failed to register clocksource: %d\n", err);
 
 	tegra_delay_timer.read_current_timer =
 			tegra_delay_timer_read_counter_long;
-	tegra_delay_timer.freq = timer_of_rate(&tegra_to);
+	tegra_delay_timer.freq = timer_of_rate(to);
 	register_current_timer_delay(&tegra_delay_timer);
 
-	clockevents_config_and_register(&tegra_to.clkevt,
-					timer_of_rate(&tegra_to),
-					0x1,
-					0x1fffffff);
-
-	return ret;
-out:
-	timer_of_cleanup(&tegra_to);
-
-	return ret;
+	return 0;
 }
 
 static int __init tegra20_init_rtc(struct device_node *np)

@@ -383,6 +350,5 @@ static int __init tegra20_init_rtc(struct device_node *np)
 	return 0;
 }
 TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
+TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
 #endif
-TIMER_OF_DECLARE(tegra210_timer, "nvidia,tegra210-timer", tegra_init_timer);
-TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra_init_timer);
Signature · 1 line
2.21.0
author
#2 lore raw

PATCH v3 2/8 clocksource/drivers/tegra: Unify timer code

In reply to Dmitry Osipenko

Tegra132 is 64bit platform and it has the tegra20-timer hardware unit. Right now the corresponding timer code isn't compiled for ARM64, remove ifdef'iness from the code and compile tegra20-timer for both 32 and 64 bit platforms. Also note that like the older generations, Tegra210 has the microseconds counter, hence the timer_us clocksource is now made available for Tegra210 as well.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/clocksource/timer-tegra20.c | 111 +++++++++++++++-------------
 1 file changed, 60 insertions(+), 51 deletions(-)
drivers/clocksource/timer-tegra20.c +60 -51
index 58e8bb6deac9..57e7aa2b80a3 100644
@@ -30,10 +30,6 @@
 
 #include "timer-of.h"
 
-#ifdef CONFIG_ARM
-#include <asm/mach/time.h>
-#endif
-
 #define RTC_SECONDS            0x08
 #define RTC_SHADOW_SECONDS     0x0c
 #define RTC_MILLISECONDS       0x10

@@ -48,25 +44,17 @@
 #define TIMER_PCR		0x4
 #define TIMER_PCR_INTR_CLR	BIT(30)
 
-#ifdef CONFIG_ARM
-#define TIMER_CPU0		0x00 /* TIMER1 */
-#define TIMER_CPU2		0x50 /* TIMER3 */
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x08
+#define TIMER3_BASE		0x50
+#define TIMER4_BASE		0x58
+#define TIMER10_BASE		0x90
+
 #define TIMER1_IRQ_IDX		0
-#define IRQ_IDX_FOR_CPU(cpu)	(TIMER1_IRQ_IDX + cpu)
-#define TIMER_BASE_FOR_CPU(cpu)	\
-	(((cpu) & 1) * 8 + ((cpu) < 2 ? TIMER_CPU0 : TIMER_CPU2))
-#else
-#define TIMER_CPU0		0x90 /* TIMER10 */
 #define TIMER10_IRQ_IDX		10
-#define IRQ_IDX_FOR_CPU(cpu)	(TIMER10_IRQ_IDX + cpu)
-#define TIMER_BASE_FOR_CPU(cpu) (TIMER_CPU0 + (cpu) * 8)
-#endif
 
 static u32 usec_config;
 static void __iomem *timer_reg_base;
-#ifdef CONFIG_ARM
-static struct delay_timer tegra_delay_timer;
-#endif
 
 static int tegra_timer_set_next_event(unsigned long cycles,
 					 struct clock_event_device *evt)

@@ -164,17 +152,23 @@ static int tegra_timer_stop(unsigned int cpu)
 	return 0;
 }
 
-#ifdef CONFIG_ARM
 static u64 notrace tegra_read_sched_clock(void)
 {
 	return readl(timer_reg_base + TIMERUS_CNTR_1US);
 }
 
+#ifdef CONFIG_ARM
 static unsigned long tegra_delay_timer_read_counter_long(void)
 {
 	return readl(timer_reg_base + TIMERUS_CNTR_1US);
 }
 
+static struct delay_timer tegra_delay_timer = {
+	.read_current_timer = tegra_delay_timer_read_counter_long,
+	.freq = 1000000,
+};
+#endif
+
 static struct timer_of suspend_rtc_to = {
 	.flags = TIMER_OF_BASE | TIMER_OF_CLOCK,
 };

@@ -199,9 +193,34 @@ static struct clocksource suspend_rtc_clocksource = {
 	.mask	= CLOCKSOURCE_MASK(32),
 	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
 };
-#endif
 
-static int tegra_init_timer(struct device_node *np, bool tegra20)
+static inline unsigned int tegra_base_for_cpu(int cpu, bool tegra20)
+{
+	if (tegra20) {
+		switch (cpu) {
+		case 0:
+			return TIMER1_BASE;
+		case 1:
+			return TIMER2_BASE;
+		case 2:
+			return TIMER3_BASE;
+		default:
+			return TIMER4_BASE;
+		}
+	}
+
+	return TIMER10_BASE + cpu * 8;
+}
+
+static inline unsigned int tegra_irq_idx_for_cpu(int cpu, bool tegra20)
+{
+	if (tegra20)
+		return TIMER1_IRQ_IDX + cpu;
+
+	return TIMER10_IRQ_IDX + cpu;
+}
+
+static int __init tegra_init_timer(struct device_node *np, bool tegra20)
 {
 	struct timer_of *to;
 	int cpu, ret;

@@ -252,6 +271,8 @@ static int tegra_init_timer(struct device_node *np, bool tegra20)
 
 	for_each_possible_cpu(cpu) {
 		struct timer_of *cpu_to = per_cpu_ptr(&tegra_to, cpu);
+		unsigned int base = tegra_base_for_cpu(cpu, tegra20);
+		unsigned int idx = tegra_irq_idx_for_cpu(cpu, tegra20);
 
 		/*
 		 * TIMER1-9 are fixed to 1MHz, TIMER10-13 are running off the

@@ -260,10 +281,10 @@ static int tegra_init_timer(struct device_node *np, bool tegra20)
 		if (tegra20)
 			cpu_to->of_clk.rate = 1000000;
 
-		cpu_to->of_base.base = timer_reg_base + TIMER_BASE_FOR_CPU(cpu);
+		cpu_to = per_cpu_ptr(&tegra_to, cpu);
+		cpu_to->of_base.base = timer_reg_base + base;
 		cpu_to->clkevt.cpumask = cpumask_of(cpu);
-		cpu_to->clkevt.irq =
-			irq_of_parse_and_map(np, IRQ_IDX_FOR_CPU(cpu));
+		cpu_to->clkevt.irq = irq_of_parse_and_map(np, idx);
 		if (!cpu_to->clkevt.irq) {
 			pr_err("%s: can't map IRQ for CPU%d\n",
 			       __func__, cpu);

@@ -283,6 +304,18 @@ static int tegra_init_timer(struct device_node *np, bool tegra20)
 		}
 	}
 
+	sched_clock_register(tegra_read_sched_clock, 32, 1000000);
+
+	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
+				    "timer_us", 1000000,
+				    300, 32, clocksource_mmio_readl_up);
+	if (ret)
+		pr_err("failed to register clocksource: %d\n", ret);
+
+#ifdef CONFIG_ARM
+	register_current_timer_delay(&tegra_delay_timer);
+#endif
+
 	cpuhp_setup_state(CPUHP_AP_TEGRA_TIMER_STARTING,
 			  "AP_TEGRA_TIMER_STARTING", tegra_timer_setup,
 			  tegra_timer_stop);

@@ -303,39 +336,17 @@ static int tegra_init_timer(struct device_node *np, bool tegra20)
 	return ret;
 }
 
-#ifdef CONFIG_ARM64
 static int __init tegra210_init_timer(struct device_node *np)
 {
 	return tegra_init_timer(np, false);
 }
 TIMER_OF_DECLARE(tegra210_timer, "nvidia,tegra210-timer", tegra210_init_timer);
-#else /* CONFIG_ARM */
+
 static int __init tegra20_init_timer(struct device_node *np)
 {
-	struct timer_of *to;
-	int err;
-
-	err = tegra_init_timer(np, true);
-	if (err)
-		return err;
-
-	to = this_cpu_ptr(&tegra_to);
-
-	sched_clock_register(tegra_read_sched_clock, 32,
-			     timer_of_rate(to));
-	err = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
-				    "timer_us", timer_of_rate(to),
-				    300, 32, clocksource_mmio_readl_up);
-	if (err)
-		pr_err("Failed to register clocksource: %d\n", err);
-
-	tegra_delay_timer.read_current_timer =
-			tegra_delay_timer_read_counter_long;
-	tegra_delay_timer.freq = timer_of_rate(to);
-	register_current_timer_delay(&tegra_delay_timer);
-
-	return 0;
+	return tegra_init_timer(np, true);
 }
+TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
 
 static int __init tegra20_init_rtc(struct device_node *np)
 {

@@ -350,5 +361,3 @@ static int __init tegra20_init_rtc(struct device_node *np)
 	return 0;
 }
 TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
-TIMER_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
-#endif
Signature · 1 line
2.21.0
author
#3 lore raw

PATCH v3 3/8 clocksource/drivers/tegra: Reset hardware state on init

In reply to Dmitry Osipenko

Reset timer's hardware state to ensure that initially it is in a predictable state.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/clocksource/timer-tegra20.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/clocksource/timer-tegra20.c +3 -0
index 57e7aa2b80a3..739f83fdb318 100644
@@ -132,6 +132,9 @@ static int tegra_timer_setup(unsigned int cpu)
 {
 	struct timer_of *to = per_cpu_ptr(&tegra_to, cpu);
 
+	writel(0, timer_of_base(to) + TIMER_PTV);
+	writel(TIMER_PCR_INTR_CLR, timer_of_base(to) + TIMER_PCR);
+
 	irq_force_affinity(to->clkevt.irq, cpumask_of(cpu));
 	enable_irq(to->clkevt.irq);
Signature · 1 line
2.21.0
author
#4 lore raw

PATCH v3 4/8 clocksource/drivers/tegra: Replace readl/writel with relaxed versions

In reply to Dmitry Osipenko

The readl/writel functions are inserting memory barrier to ensure that outstanding memory writes are completed, this results in L2 cache syncing being done on Tegra20 and Tegra30 which isn't a very cheap operation. Replace all readl/writel occurrences in the code with the relaxed versions since there is no need for the memory-access syncing.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/clocksource/timer-tegra20.c | 35 +++++++++++++++--------------
 1 file changed, 18 insertions(+), 17 deletions(-)
drivers/clocksource/timer-tegra20.c +18 -17
index 739f83fdb318..55e9b3e1fbeb 100644
@@ -61,9 +61,9 @@ static int tegra_timer_set_next_event(unsigned long cycles,
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 
-	writel(TIMER_PTV_EN |
-	       ((cycles > 1) ? (cycles - 1) : 0), /* n+1 scheme */
-	       reg_base + TIMER_PTV);
+	writel_relaxed(TIMER_PTV_EN |
+		       ((cycles > 1) ? (cycles - 1) : 0), /* n+1 scheme */
+		       reg_base + TIMER_PTV);
 
 	return 0;
 }

@@ -72,7 +72,7 @@ static int tegra_timer_shutdown(struct clock_event_device *evt)
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 
-	writel(0, reg_base + TIMER_PTV);
+	writel_relaxed(0, reg_base + TIMER_PTV);
 
 	return 0;
 }

@@ -81,9 +81,9 @@ static int tegra_timer_set_periodic(struct clock_event_device *evt)
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 
-	writel(TIMER_PTV_EN | TIMER_PTV_PER |
-	       ((timer_of_rate(to_timer_of(evt)) / HZ) - 1),
-	       reg_base + TIMER_PTV);
+	writel_relaxed(TIMER_PTV_EN | TIMER_PTV_PER |
+		       ((timer_of_rate(to_timer_of(evt)) / HZ) - 1),
+		       reg_base + TIMER_PTV);
 
 	return 0;
 }

@@ -93,7 +93,7 @@ static irqreturn_t tegra_timer_isr(int irq, void *dev_id)
 	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 
-	writel(TIMER_PCR_INTR_CLR, reg_base + TIMER_PCR);
+	writel_relaxed(TIMER_PCR_INTR_CLR, reg_base + TIMER_PCR);
 	evt->event_handler(evt);
 
 	return IRQ_HANDLED;

@@ -103,12 +103,12 @@ static void tegra_timer_suspend(struct clock_event_device *evt)
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 
-	writel(TIMER_PCR_INTR_CLR, reg_base + TIMER_PCR);
+	writel_relaxed(TIMER_PCR_INTR_CLR, reg_base + TIMER_PCR);
 }
 
 static void tegra_timer_resume(struct clock_event_device *evt)
 {
-	writel(usec_config, timer_reg_base + TIMERUS_USEC_CFG);
+	writel_relaxed(usec_config, timer_reg_base + TIMERUS_USEC_CFG);
 }
 
 static DEFINE_PER_CPU(struct timer_of, tegra_to) = {

@@ -132,8 +132,8 @@ static int tegra_timer_setup(unsigned int cpu)
 {
 	struct timer_of *to = per_cpu_ptr(&tegra_to, cpu);
 
-	writel(0, timer_of_base(to) + TIMER_PTV);
-	writel(TIMER_PCR_INTR_CLR, timer_of_base(to) + TIMER_PCR);
+	writel_relaxed(0, timer_of_base(to) + TIMER_PTV);
+	writel_relaxed(TIMER_PCR_INTR_CLR, timer_of_base(to) + TIMER_PCR);
 
 	irq_force_affinity(to->clkevt.irq, cpumask_of(cpu));
 	enable_irq(to->clkevt.irq);

@@ -157,13 +157,13 @@ static int tegra_timer_stop(unsigned int cpu)
 
 static u64 notrace tegra_read_sched_clock(void)
 {
-	return readl(timer_reg_base + TIMERUS_CNTR_1US);
+	return readl_relaxed(timer_reg_base + TIMERUS_CNTR_1US);
 }
 
 #ifdef CONFIG_ARM
 static unsigned long tegra_delay_timer_read_counter_long(void)
 {
-	return readl(timer_reg_base + TIMERUS_CNTR_1US);
+	return readl_relaxed(timer_reg_base + TIMERUS_CNTR_1US);
 }
 
 static struct delay_timer tegra_delay_timer = {

@@ -184,8 +184,9 @@ static struct timer_of suspend_rtc_to = {
  */
 static u64 tegra_rtc_read_ms(struct clocksource *cs)
 {
-	u32 ms = readl(timer_of_base(&suspend_rtc_to) + RTC_MILLISECONDS);
-	u32 s = readl(timer_of_base(&suspend_rtc_to) + RTC_SHADOW_SECONDS);
+	void __iomem *reg_base = timer_of_base(&suspend_rtc_to);
+	u32 ms = readl_relaxed(reg_base + RTC_MILLISECONDS);
+	u32 s = readl_relaxed(reg_base + RTC_SHADOW_SECONDS);
 	return (u64)s * MSEC_PER_SEC + ms;
 }
 

@@ -270,7 +271,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20)
 		goto out;
 	}
 
-	writel(usec_config, timer_reg_base + TIMERUS_USEC_CFG);
+	writel_relaxed(usec_config, timer_reg_base + TIMERUS_USEC_CFG);
 
 	for_each_possible_cpu(cpu) {
 		struct timer_of *cpu_to = per_cpu_ptr(&tegra_to, cpu);
Signature · 1 line
2.21.0
author
#5 lore raw

PATCH v3 5/8 clocksource/drivers/tegra: Release all IRQ's on request_irq() error

In reply to Dmitry Osipenko

Release all requested IRQ's on the request error to properly clean up allocated resources.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/clocksource/timer-tegra20.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/clocksource/timer-tegra20.c +3 -2
index 55e9b3e1fbeb..18b81d814b3b 100644
@@ -293,7 +293,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20)
 			pr_err("%s: can't map IRQ for CPU%d\n",
 			       __func__, cpu);
 			ret = -EINVAL;
-			goto out;
+			goto out_irq;
 		}
 
 		irq_set_status_flags(cpu_to->clkevt.irq, IRQ_NOAUTOEN);

@@ -303,7 +303,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20)
 		if (ret) {
 			pr_err("%s: cannot setup irq %d for CPU%d\n",
 				__func__, cpu_to->clkevt.irq, cpu);
-			ret = -EINVAL;
+			irq_dispose_mapping(cpu_to->clkevt.irq);
+			cpu_to->clkevt.irq = 0;
 			goto out_irq;
 		}
 	}
Signature · 1 line
2.21.0
author
#6 lore raw

PATCH v3 6/8 clocksource/drivers/tegra: Minor code clean up

In reply to Dmitry Osipenko

Correct typo and use proper upper casing for acronyms in the comments, use common style for error messages, prepend error messages with "tegra-timer:", add error message for cpuhp_setup_state() failure and clean up whitespaces in the code to fix checkpatch warnings.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/clocksource/timer-tegra20.c | 43 ++++++++++++++++-------------
 1 file changed, 24 insertions(+), 19 deletions(-)
drivers/clocksource/timer-tegra20.c +24 -19
index 18b81d814b3b..12784a82fd57 100644
@@ -15,6 +15,8 @@
  *
  */
 
+#define pr_fmt(fmt)	"tegra-timer: " fmt
+
 #include <linux/clk.h>
 #include <linux/clockchips.h>
 #include <linux/cpu.h>

@@ -30,13 +32,13 @@
 
 #include "timer-of.h"
 
-#define RTC_SECONDS            0x08
-#define RTC_SHADOW_SECONDS     0x0c
-#define RTC_MILLISECONDS       0x10
+#define RTC_SECONDS		0x08
+#define RTC_SHADOW_SECONDS	0x0c
+#define RTC_MILLISECONDS	0x10
 
-#define TIMERUS_CNTR_1US 0x10
-#define TIMERUS_USEC_CFG 0x14
-#define TIMERUS_CNTR_FREEZE 0x4c
+#define TIMERUS_CNTR_1US	0x10
+#define TIMERUS_USEC_CFG	0x14
+#define TIMERUS_CNTR_FREEZE	0x4c
 
 #define TIMER_PTV		0x0
 #define TIMER_PTV_EN		BIT(31)

@@ -57,7 +59,7 @@ static u32 usec_config;
 static void __iomem *timer_reg_base;
 
 static int tegra_timer_set_next_event(unsigned long cycles,
-					 struct clock_event_device *evt)
+				      struct clock_event_device *evt)
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 

@@ -178,15 +180,17 @@ static struct timer_of suspend_rtc_to = {
 
 /*
  * tegra_rtc_read - Reads the Tegra RTC registers
- * Care must be taken that this funciton is not called while the
+ * Care must be taken that this function is not called while the
  * tegra_rtc driver could be executing to avoid race conditions
  * on the RTC shadow register
  */
 static u64 tegra_rtc_read_ms(struct clocksource *cs)
 {
 	void __iomem *reg_base = timer_of_base(&suspend_rtc_to);
+
 	u32 ms = readl_relaxed(reg_base + RTC_MILLISECONDS);
 	u32 s = readl_relaxed(reg_base + RTC_SHADOW_SECONDS);
+
 	return (u64)s * MSEC_PER_SEC + ms;
 }
 

@@ -231,7 +235,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20)
 
 	to = this_cpu_ptr(&tegra_to);
 	ret = timer_of_init(np, to);
-	if (ret < 0)
+	if (ret)
 		goto out;
 
 	timer_reg_base = timer_of_base(to);

@@ -290,8 +294,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20)
 		cpu_to->clkevt.cpumask = cpumask_of(cpu);
 		cpu_to->clkevt.irq = irq_of_parse_and_map(np, idx);
 		if (!cpu_to->clkevt.irq) {
-			pr_err("%s: can't map IRQ for CPU%d\n",
-			       __func__, cpu);
+			pr_err("failed to map irq for cpu%d\n", cpu);
 			ret = -EINVAL;
 			goto out_irq;
 		}

@@ -301,8 +304,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20)
 				  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);
+			pr_err("failed to set up irq for cpu%d: %d\n",
+			       cpu, ret);
 			irq_dispose_mapping(cpu_to->clkevt.irq);
 			cpu_to->clkevt.irq = 0;
 			goto out_irq;

@@ -321,11 +324,14 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20)
 	register_current_timer_delay(&tegra_delay_timer);
 #endif
 
-	cpuhp_setup_state(CPUHP_AP_TEGRA_TIMER_STARTING,
-			  "AP_TEGRA_TIMER_STARTING", tegra_timer_setup,
-			  tegra_timer_stop);
+	ret = cpuhp_setup_state(CPUHP_AP_TEGRA_TIMER_STARTING,
+				"AP_TEGRA_TIMER_STARTING", tegra_timer_setup,
+				tegra_timer_stop);
+	if (ret)
+		pr_err("failed to set up cpu hp state: %d\n", ret);
 
 	return ret;
+
 out_irq:
 	for_each_possible_cpu(cpu) {
 		struct timer_of *cpu_to;

@@ -338,6 +344,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20)
 	}
 out:
 	timer_of_cleanup(to);
+
 	return ret;
 }
 

@@ -361,8 +368,6 @@ static int __init tegra20_init_rtc(struct device_node *np)
 	if (ret)
 		return ret;
 
-	clocksource_register_hz(&suspend_rtc_clocksource, 1000);
-
-	return 0;
+	return clocksource_register_hz(&suspend_rtc_clocksource, 1000);
 }
 TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
Signature · 1 line
2.21.0
author
#7 lore raw

PATCH v3 7/8 clocksource/drivers/tegra: Use SPDX identifier

In reply to Dmitry Osipenko

Use SPDX tag for the license identification instead of a free form text to aid license-checking automation and for brevity.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/clocksource/timer-tegra20.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)
drivers/clocksource/timer-tegra20.c +2 -13
index 12784a82fd57..1a3ee928e9a5 100644
@@ -1,18 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (C) 2010 Google, Inc.
- *
- * Author:
- *	Colin Cross <[email protected]>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
+ * Author: Colin Cross <[email protected]>
  */
 
 #define pr_fmt(fmt)	"tegra-timer: " fmt
Signature · 1 line
2.21.0
author
#8 lore raw

PATCH v3 8/8 clocksource/drivers/tegra: Support COMPILE_TEST universally

In reply to Dmitry Osipenko

Remove build dependency on ARM for compile-testing to allow non-arch specific build-bots (like Intel's test robot) to compile the driver and report about problems.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/clocksource/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/clocksource/Kconfig +2 -2
index 3300739edce4..8c21e557181b 100644
@@ -137,10 +137,10 @@ config SUN5I_HSTIMER
 	  Enables support the Sun5i timer.
 
 config TEGRA_TIMER
-	bool "Tegra timer driver" if COMPILE_TEST
+	bool "Tegra timer driver"
 	select CLKSRC_MMIO
 	select TIMER_OF
-	depends on ARM || ARM64
+	depends on ARCH_TEGRA || COMPILE_TEST
 	help
 	  Enables support for the Tegra driver.
Signature · 1 line
2.21.0
#9 lore raw
In reply to Dmitry Osipenko
Quoted 8 lines · Dmitry Osipenko

Hello,

This series primarily unifies the driver code across all Tegra SoC generations. In a result the clocksources are allocated per-CPU on older Tegra's and have a higher rating than the arch-timer, the newer Tegra210 is getting support for microsecond clocksource and the driver's code is getting much cleaner. Note that arch-timer usage is discouraged on all Tegra's due to the time jitter caused by the CPU frequency scaling.

I think the limitations are more as follows:

Chip timer suffers cpu dvfs jitter can wakeup from cc7
T20 us-timer No Yes
T20 twd timer Yes No?
T30 us-timer No Yes
T30 twd timer Yes No?
T114 us-timer No Yes
T114 arch timer No Yes
T124 us-timer No Yes
T124 arch timer No Yes
T210 us-timer No Yes
T210 arch timer No No
T210 clk_m timer No Yes

right?

Peter.

author
#10 lore raw

31.05.2019 11:26, Peter De Schrijver пишет:

Quoted 25 lines · Dmitry Osipenko
Quoted 8 lines

Hello,

This series primarily unifies the driver code across all Tegra SoC generations. In a result the clocksources are allocated per-CPU on older Tegra's and have a higher rating than the arch-timer, the newer Tegra210 is getting support for microsecond clocksource and the driver's code is getting much cleaner. Note that arch-timer usage is discouraged on all Tegra's due to the time jitter caused by the CPU frequency scaling.

I think the limitations are more as follows:

Chip timer suffers cpu dvfs jitter can wakeup from cc7
T20 us-timer No Yes
T20 twd timer Yes No?
T30 us-timer No Yes
T30 twd timer Yes No?
T114 us-timer No Yes
T114 arch timer No Yes
T124 us-timer No Yes
T124 arch timer No Yes
T210 us-timer No Yes
T210 arch timer No No
T210 clk_m timer No Yes

Quoted 1 line

right?

Doesn't arch timer run off the CPU clock? If yes (that's what I assumed), then it should be affected by the DVFS. Otherwise I'll lower the clocksource's rating for T114/124/132.

TWD can't wake CPU from the power-down state, so it's a solid "No" for TWD in the "can wakeup from cc7" column.

#11 lore raw
Quoted 26 lines · Dmitry Osipenko

31.05.2019 11:26, Peter De Schrijver пишет:

Quoted 25 lines · Dmitry Osipenko
Quoted 8 lines

Hello,

This series primarily unifies the driver code across all Tegra SoC generations. In a result the clocksources are allocated per-CPU on older Tegra's and have a higher rating than the arch-timer, the newer Tegra210 is getting support for microsecond clocksource and the driver's code is getting much cleaner. Note that arch-timer usage is discouraged on all Tegra's due to the time jitter caused by the CPU frequency scaling.

I think the limitations are more as follows:

Chip timer suffers cpu dvfs jitter can wakeup from cc7
T20 us-timer No Yes
T20 twd timer Yes No?
T30 us-timer No Yes
T30 twd timer Yes No?
T114 us-timer No Yes
T114 arch timer No Yes
T124 us-timer No Yes
T124 arch timer No Yes
T210 us-timer No Yes
T210 arch timer No No
T210 clk_m timer No Yes

Quoted 8 lines
Quoted 1 line

right?

Doesn't arch timer run off the CPU clock? If yes (that's what I assumed), then it should be affected by the DVFS. Otherwise I'll lower the clocksource's rating for T114/124/132.

TWD can't wake CPU from the power-down state, so it's a solid "No" for TWD in the "can wakeup from cc7" column.

Wouldn't make sense to rename the timer-tegra20.c to timer-tegra.c now ?

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
author
#12 lore raw

31.05.2019 23:31, Daniel Lezcano пишет:

Quoted 29 lines · Dmitry Osipenko
Quoted 28 lines

31.05.2019 11:26, Peter De Schrijver пишет:

Quoted 26 lines · Dmitry Osipenko
Quoted 8 lines

Hello,

This series primarily unifies the driver code across all Tegra SoC generations. In a result the clocksources are allocated per-CPU on older Tegra's and have a higher rating than the arch-timer, the newer Tegra210 is getting support for microsecond clocksource and the driver's code is getting much cleaner. Note that arch-timer usage is discouraged on all Tegra's due to the time jitter caused by the CPU frequency scaling.

I think the limitations are more as follows:

Chip timer suffers cpu dvfs jitter can wakeup from cc7
T20 us-timer No Yes
T20 twd timer Yes No?
T30 us-timer No Yes
T30 twd timer Yes No?
T114 us-timer No Yes
T114 arch timer No Yes
T124 us-timer No Yes
T124 arch timer No Yes
T210 us-timer No Yes
T210 arch timer No No
T210 clk_m timer No Yes

right?

Quoted 8 lines
Quoted 6 lines

Doesn't arch timer run off the CPU clock? If yes (that's what I assumed), then it should be affected by the DVFS. Otherwise I'll lower the clocksource's rating for T114/124/132.

TWD can't wake CPU from the power-down state, so it's a solid "No" for TWD in the "can wakeup from cc7" column.

Wouldn't make sense to rename the timer-tegra20.c to timer-tegra.c now ?

Wouldn't hurt, given the refreshment that driver is getting lately. I'll include a patch for that in the next revision, thanks.

#13 lore raw
In reply to Dmitry Osipenko
Quoted 26 lines · Dmitry Osipenko

31.05.2019 11:26, Peter De Schrijver пишет:

Quoted 25 lines · Dmitry Osipenko
Quoted 8 lines

Hello,

This series primarily unifies the driver code across all Tegra SoC generations. In a result the clocksources are allocated per-CPU on older Tegra's and have a higher rating than the arch-timer, the newer Tegra210 is getting support for microsecond clocksource and the driver's code is getting much cleaner. Note that arch-timer usage is discouraged on all Tegra's due to the time jitter caused by the CPU frequency scaling.

I think the limitations are more as follows:

Chip timer suffers cpu dvfs jitter can wakeup from cc7
T20 us-timer No Yes
T20 twd timer Yes No?
T30 us-timer No Yes
T30 twd timer Yes No?
T114 us-timer No Yes
T114 arch timer No Yes
T124 us-timer No Yes
T124 arch timer No Yes
T210 us-timer No Yes
T210 arch timer No No
T210 clk_m timer No Yes

Quoted 6 lines
Quoted 1 line

right?

Doesn't arch timer run off the CPU clock? If yes (that's what I assumed), then it should be affected by the DVFS. Otherwise I'll lower the clocksource's rating for T114/124/132.

No. It doesn't. This is the big change between A9 and later CPUs.

Peter.

Quoted 2 lines

TWD can't wake CPU from the power-down state, so it's a solid "No" for TWD in the "can wakeup from cc7" column.

author
#14 lore raw

03.06.2019 10:17, Peter De Schrijver пишет:

Quoted 27 lines · Dmitry Osipenko
Quoted 26 lines

31.05.2019 11:26, Peter De Schrijver пишет:

Quoted 25 lines · Dmitry Osipenko
Quoted 8 lines

Hello,

This series primarily unifies the driver code across all Tegra SoC generations. In a result the clocksources are allocated per-CPU on older Tegra's and have a higher rating than the arch-timer, the newer Tegra210 is getting support for microsecond clocksource and the driver's code is getting much cleaner. Note that arch-timer usage is discouraged on all Tegra's due to the time jitter caused by the CPU frequency scaling.

I think the limitations are more as follows:

Chip timer suffers cpu dvfs jitter can wakeup from cc7
T20 us-timer No Yes
T20 twd timer Yes No?
T30 us-timer No Yes
T30 twd timer Yes No?
T114 us-timer No Yes
T114 arch timer No Yes
T124 us-timer No Yes
T124 arch timer No Yes
T210 us-timer No Yes
T210 arch timer No No
T210 clk_m timer No Yes

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

right?

Doesn't arch timer run off the CPU clock? If yes (that's what I assumed), then it should be affected by the DVFS. Otherwise I'll lower the clocksource's rating for T114/124/132.

No. It doesn't. This is the big change between A9 and later CPUs.

Thank you for the clarification, I'll add a patch to lower the rating where appropriate.