Skip to contentSkip to conversation

Few more cleanups for tegra-timer

PATCH In review Dmitry Osipenko 3 participants 23 messages Jun 18, 2019 – Jun 20, 2019

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v3 cover Few more cleanups for tegra-timer

Hello,

This a followup to [0] that includes some more fixes and further prettifies the driver's code.

[0] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=111529

Changelog:

v3: Addressed request from Jon Hunter that was made in a review comment
to v2 by dropping the timer's period rounding-up in the "Set and use timer' period" patch.

Appended two new patches to this series that were already sent out and reviewed after the v2 of this series:

clocksource/drivers/tegra: Cycles can't be 0 clocksource/drivers/tegra: Set up maximum-ticks limit properly

In this two new patches I addressed review comments that were made by Thierry Reding by adding clarifying comments to the code and extending the commit messages a tad.

Corrected the "Fixes" tag in a "Restore timer rate on Tegra210" patch such that linux-next checker won't complain about the shortened commit's subject.

v2: Fixed a bug that was introduced by [0] in a newly added patch:
"Restore timer rate on Tegra210".

Fixed potential problem in regards to error handling in another new
patch: "Restore base address before cleanup".

Added new patch "Add verbose definition for 1MHz constant" as per Daniel's Lezcano recommendation.

Fixed a code typo that was made in "Remove duplicated use of per_cpu_ptr" of v1.

Dmitry Osipenko (8):
clocksource/drivers/tegra: Restore timer rate on Tegra210 clocksource/drivers/tegra: Remove duplicated use of per_cpu_ptr clocksource/drivers/tegra: Set and use timer's period clocksource/drivers/tegra: Drop unneeded typecasting in one place clocksource/drivers/tegra: Add verbose definition for 1MHz constant clocksource/drivers/tegra: Restore base address before cleanup clocksource/drivers/tegra: Cycles can't be 0 clocksource/drivers/tegra: Set up maximum-ticks limit properly

 drivers/clocksource/timer-tegra.c | 82 +++++++++++++++++++++----------
 1 file changed, 56 insertions(+), 26 deletions(-)
Signature · 1 line
2.22.0
#1 lore raw

PATCH v3 1/8 clocksource/drivers/tegra: Restore timer rate on Tegra210

The clocksource rate is initialized only for the first per-CPU clocksource and then that rate shall be replicated for the rest of clocksource's because they are initialized manually in the code.

Fixes 3be2a85a0b61 ("clocksource/drivers/tegra: Support per-CPU timers on all Tegra's")
---
 drivers/clocksource/timer-tegra.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/clocksource/timer-tegra.c +2 -0
index 1cc5847a1030..501071f09fa8 100644
@@ -277,6 +277,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 		 */
 		if (tegra20)
 			cpu_to->of_clk.rate = 1000000;
+		else
+			cpu_to->of_clk.rate = timer_of_rate(to);
 
 		cpu_to = per_cpu_ptr(&tegra_to, cpu);
 		cpu_to->of_base.base = timer_reg_base + base;
Signature · 1 line
2.22.0
Acked-by on v3 1/8
#2 lore raw

Re: [PATCH v3 1/8] clocksource/drivers/tegra: Restore timer rate on Tegra210

Quoted 11 lines · Dmitry Osipenko

The clocksource rate is initialized only for the first per-CPU clocksource and then that rate shall be replicated for the rest of clocksource's because they are initialized manually in the code.

Fixes 3be2a85a0b61 ("clocksource/drivers/tegra: Support per-CPU timers on all Tegra's")
---
 drivers/clocksource/timer-tegra.c | 2 ++
 1 file changed, 2 insertions(+)
#3 lore raw

PATCH v3 2/8 clocksource/drivers/tegra: Remove duplicated use of per_cpu_ptr

In reply to Dmitry Osipenko

It was left unnoticed by accident, which means that the code could be cleaned up a tad more.

---
 drivers/clocksource/timer-tegra.c | 42 ++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 17 deletions(-)
drivers/clocksource/timer-tegra.c +25 -17
index 501071f09fa8..ff5a4ccb5d52 100644
@@ -216,6 +216,19 @@ static inline unsigned int tegra_irq_idx_for_cpu(int cpu, bool tegra20)
 	return TIMER10_IRQ_IDX + cpu;
 }
 
+static inline unsigned long tegra_rate_for_timer(struct timer_of *to,
+						 bool tegra20)
+{
+	/*
+	 * TIMER1-9 are fixed to 1MHz, TIMER10-13 are running off the
+	 * parent clock.
+	 */
+	if (tegra20)
+		return 1000000;
+
+	return timer_of_rate(to);
+}
+
 static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 				   int rating)
 {
@@ -268,32 +281,27 @@ static int __init 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 long flags = IRQF_TIMER | IRQF_NOBALANCING;
+		unsigned long rate = tegra_rate_for_timer(to, tegra20);
 		unsigned int base = tegra_base_for_cpu(cpu, tegra20);
 		unsigned int idx = tegra_irq_idx_for_cpu(cpu, tegra20);
+		unsigned int irq = irq_of_parse_and_map(np, idx);
 
-		/*
-		 * TIMER1-9 are fixed to 1MHz, TIMER10-13 are running off the
-		 * parent clock.
-		 */
-		if (tegra20)
-			cpu_to->of_clk.rate = 1000000;
-		else
-			cpu_to->of_clk.rate = timer_of_rate(to);
-
-		cpu_to = per_cpu_ptr(&tegra_to, cpu);
-		cpu_to->of_base.base = timer_reg_base + base;
-		cpu_to->clkevt.rating = rating;
-		cpu_to->clkevt.cpumask = cpumask_of(cpu);
-		cpu_to->clkevt.irq = irq_of_parse_and_map(np, idx);
-		if (!cpu_to->clkevt.irq) {
+		if (!irq) {
 			pr_err("failed to map irq for cpu%d\n", cpu);
 			ret = -EINVAL;
 			goto out_irq;
 		}
 
+		cpu_to->clkevt.irq = irq;
+		cpu_to->clkevt.rating = rating;
+		cpu_to->clkevt.cpumask = cpumask_of(cpu);
+		cpu_to->of_base.base = timer_reg_base + base;
+		cpu_to->of_clk.rate = rate;
+
 		irq_set_status_flags(cpu_to->clkevt.irq, IRQ_NOAUTOEN);
-		ret = request_irq(cpu_to->clkevt.irq, tegra_timer_isr,
-				  IRQF_TIMER | IRQF_NOBALANCING,
+
+		ret = request_irq(cpu_to->clkevt.irq, tegra_timer_isr, flags,
 				  cpu_to->clkevt.name, &cpu_to->clkevt);
 		if (ret) {
 			pr_err("failed to set up irq for cpu%d: %d\n",
Signature · 1 line
2.22.0
Acked-by on v3 2/8
#4 lore raw

Re: [PATCH v3 2/8] clocksource/drivers/tegra: Remove duplicated use of per_cpu_ptr

Quoted 8 lines · Dmitry Osipenko

It was left unnoticed by accident, which means that the code could be cleaned up a tad more.

---
 drivers/clocksource/timer-tegra.c | 42 ++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 17 deletions(-)
#5 lore raw

PATCH v3 3/8 clocksource/drivers/tegra: Set and use timer's period

In reply to Dmitry Osipenko

The of_clk structure has a period field that is set up initially by timer_of_clk_init(), that period value need to be adjusted for a case of TIMER1-9 that are running at a fixed rate that doesn't match the clock's rate. Note that the period value is currently used only by some of the clocksource drivers internally and hence this is just a minor cleanup change that doesn't fix anything.

---
 drivers/clocksource/timer-tegra.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/clocksource/timer-tegra.c +3 -2
index ff5a4ccb5d52..e6221e070499 100644
@@ -71,9 +71,9 @@ static int tegra_timer_shutdown(struct clock_event_device *evt)
 static int tegra_timer_set_periodic(struct clock_event_device *evt)
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
+	unsigned long period = timer_of_period(to_timer_of(evt));
 
-	writel_relaxed(TIMER_PTV_EN | TIMER_PTV_PER |
-		       ((timer_of_rate(to_timer_of(evt)) / HZ) - 1),
+	writel_relaxed(TIMER_PTV_EN | TIMER_PTV_PER | (period - 1),
 		       reg_base + TIMER_PTV);
 
 	return 0;
@@ -297,6 +297,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 		cpu_to->clkevt.rating = rating;
 		cpu_to->clkevt.cpumask = cpumask_of(cpu);
 		cpu_to->of_base.base = timer_reg_base + base;
+		cpu_to->of_clk.period = rate / HZ;
 		cpu_to->of_clk.rate = rate;
 
 		irq_set_status_flags(cpu_to->clkevt.irq, IRQ_NOAUTOEN);
Signature · 1 line
2.22.0
maintainer Acked-by on v3 3/8
#6 lore raw

Re: [PATCH v3 3/8] clocksource/drivers/tegra: Set and use timer's period

Quoted 29 lines · Dmitry Osipenko

The of_clk structure has a period field that is set up initially by timer_of_clk_init(), that period value need to be adjusted for a case of TIMER1-9 that are running at a fixed rate that doesn't match the clock's rate. Note that the period value is currently used only by some of the clocksource drivers internally and hence this is just a minor cleanup change that doesn't fix anything.

---
 drivers/clocksource/timer-tegra.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/clocksource/timer-tegra.c +2 -2
index ff5a4ccb5d52..e6221e070499 100644
@@ -71,9 +71,9 @@ static int tegra_timer_shutdown(struct clock_event_device *evt)
 static int tegra_timer_set_periodic(struct clock_event_device *evt)
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt)); 
+	unsigned long period = timer_of_period(to_timer_of(evt));
 
-	writel_relaxed(TIMER_PTV_EN | TIMER_PTV_PER |
-		       ((timer_of_rate(to_timer_of(evt)) / HZ) - 1),
+	writel_relaxed(TIMER_PTV_EN | TIMER_PTV_PER | (period - 1),
 		       reg_base + TIMER_PTV);
 
 	return 0;

drivers/clocksource/timer-tegra.c static int __init tegra_init_timer(struct device_node *np, bool tegra20, · line 303

Quoted 7 lines
drivers/clocksource/timer-tegra.c +1 -0
@@ -297,6 +297,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 		cpu_to->clkevt.rating = rating;
 		cpu_to->clkevt.cpumask = cpumask_of(cpu);
 		cpu_to->of_base.base = timer_reg_base + base;
+		cpu_to->of_clk.period = rate / HZ;
 		cpu_to->of_clk.rate = rate;
 
 		irq_set_status_flags(cpu_to->clkevt.irq, IRQ_NOAUTOEN);

Cheers
Jon

Signature · 1 line
nvpublic
#7 lore raw

Re: [PATCH v3 3/8] clocksource/drivers/tegra: Set and use timer's period

18.06.2019 19:32, Jon Hunter пишет:

Quoted 37 lines · Dmitry Osipenko
Quoted 35 lines

The of_clk structure has a period field that is set up initially by timer_of_clk_init(), that period value need to be adjusted for a case of TIMER1-9 that are running at a fixed rate that doesn't match the clock's rate. Note that the period value is currently used only by some of the clocksource drivers internally and hence this is just a minor cleanup change that doesn't fix anything.

---
 drivers/clocksource/timer-tegra.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/clocksource/timer-tegra.c +3 -2
index ff5a4ccb5d52..e6221e070499 100644
@@ -71,9 +71,9 @@ static int tegra_timer_shutdown(struct clock_event_device *evt)
 static int tegra_timer_set_periodic(struct clock_event_device *evt)
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt)); 
+	unsigned long period = timer_of_period(to_timer_of(evt));
 
-	writel_relaxed(TIMER_PTV_EN | TIMER_PTV_PER |
-		       ((timer_of_rate(to_timer_of(evt)) / HZ) - 1),
+	writel_relaxed(TIMER_PTV_EN | TIMER_PTV_PER | (period - 1),
 		       reg_base + TIMER_PTV);
 
 	return 0;
@@ -297,6 +297,7 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 		cpu_to->clkevt.rating = rating;
 		cpu_to->clkevt.cpumask = cpumask_of(cpu);
 		cpu_to->of_base.base = timer_reg_base + base;
+		cpu_to->of_clk.period = rate / HZ;
 		cpu_to->of_clk.rate = rate;
 
Quoted 3 lines

drivers/clocksource/timer-tegra.c static int __init tegra_init_timer(struct device_node *np, bool tegra20, · line 303

Quoted 1 line
drivers/clocksource/timer-tegra.c +0 -0
 		irq_set_status_flags(cpu_to->clkevt.irq, IRQ_NOAUTOEN);

Thanks!

Acked-by on v3 3/8
#8 lore raw

Re: [PATCH v3 3/8] clocksource/drivers/tegra: Set and use timer's period

In reply to Dmitry Osipenko
Quoted 11 lines · Dmitry Osipenko

The of_clk structure has a period field that is set up initially by timer_of_clk_init(), that period value need to be adjusted for a case of TIMER1-9 that are running at a fixed rate that doesn't match the clock's rate. Note that the period value is currently used only by some of the clocksource drivers internally and hence this is just a minor cleanup change that doesn't fix anything.

---
 drivers/clocksource/timer-tegra.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
#9 lore raw

PATCH v3 4/8 clocksource/drivers/tegra: Drop unneeded typecasting in one place

In reply to Dmitry Osipenko

There is no need to cast void because kernel allows to do that without a warning message from a compiler.

---
 drivers/clocksource/timer-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/clocksource/timer-tegra.c +1 -1
index e6221e070499..3afa66c6730b 100644
@@ -81,7 +81,7 @@ static int tegra_timer_set_periodic(struct clock_event_device *evt)
 
 static irqreturn_t tegra_timer_isr(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+	struct clock_event_device *evt = dev_id;
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 
 	writel_relaxed(TIMER_PCR_INTR_CLR, reg_base + TIMER_PCR);
Signature · 1 line
2.22.0
Acked-by on v3 4/8
#10 lore raw

Re: [PATCH v3 4/8] clocksource/drivers/tegra: Drop unneeded typecasting in one place

Quoted 8 lines · Dmitry Osipenko

There is no need to cast void because kernel allows to do that without a warning message from a compiler.

---
 drivers/clocksource/timer-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
#11 lore raw

PATCH v3 5/8 clocksource/drivers/tegra: Add verbose definition for 1MHz constant

In reply to Dmitry Osipenko

Convert all 1MHz literals to a verbose constant for better readability.

---
 drivers/clocksource/timer-tegra.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/clocksource/timer-tegra.c +7 -5
index 3afa66c6730b..ddf5531c48a9 100644
@@ -44,6 +44,8 @@
 #define TIMER1_IRQ_IDX		0
 #define TIMER10_IRQ_IDX		10
 
+#define TIMER_1MHz		1000000
+
 static u32 usec_config;
 static void __iomem *timer_reg_base;
 
@@ -158,7 +160,7 @@ static unsigned long tegra_delay_timer_read_counter_long(void)
 
 static struct delay_timer tegra_delay_timer = {
 	.read_current_timer = tegra_delay_timer_read_counter_long,
-	.freq = 1000000,
+	.freq = TIMER_1MHz,
 };
 #endif
 
@@ -224,7 +226,7 @@ static inline unsigned long tegra_rate_for_timer(struct timer_of *to,
 	 * parent clock.
 	 */
 	if (tegra20)
-		return 1000000;
+		return TIMER_1MHz;
 
 	return timer_of_rate(to);
 }
@@ -313,11 +315,11 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 		}
 	}
 
-	sched_clock_register(tegra_read_sched_clock, 32, 1000000);
+	sched_clock_register(tegra_read_sched_clock, 32, TIMER_1MHz);
 
 	ret = clocksource_mmio_init(timer_reg_base + TIMERUS_CNTR_1US,
-				    "timer_us", 1000000,
-				    300, 32, clocksource_mmio_readl_up);
+				    "timer_us", TIMER_1MHz, 300, 32,
+				    clocksource_mmio_readl_up);
 	if (ret)
 		pr_err("failed to register clocksource: %d\n", ret);
 
Signature · 1 line
2.22.0
Acked-by on v3 5/8
#12 lore raw

Re: [PATCH v3 5/8] clocksource/drivers/tegra: Add verbose definition for 1MHz constant

Quoted 8 lines · Dmitry Osipenko

Convert all 1MHz literals to a verbose constant for better readability.

---
 drivers/clocksource/timer-tegra.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
#13 lore raw

PATCH v3 6/8 clocksource/drivers/tegra: Restore base address before cleanup

In reply to Dmitry Osipenko

We're adjusting the timer's base for each per-CPU timer to point to the actual start of the timer since device-tree defines a compound registers range that includes all of the timers. In this case the original base need to be restore before calling iounmap to unmap the proper address.

---
 drivers/clocksource/timer-tegra.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/clocksource/timer-tegra.c +2 -0
index ddf5531c48a9..2673b6e0caa8 100644
@@ -345,6 +345,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 			irq_dispose_mapping(cpu_to->clkevt.irq);
 		}
 	}
+
+	to->of_base.base = timer_reg_base;
 out:
 	timer_of_cleanup(to);
 
Signature · 1 line
2.22.0
maintainer Acked-by on v3 6/8
#14 lore raw

Re: [PATCH v3 6/8] clocksource/drivers/tegra: Restore base address before cleanup

Quoted 15 lines · Dmitry Osipenko

We're adjusting the timer's base for each per-CPU timer to point to the actual start of the timer since device-tree defines a compound registers range that includes all of the timers. In this case the original base need to be restore before calling iounmap to unmap the proper address.

---
 drivers/clocksource/timer-tegra.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/clocksource/timer-tegra.c +0 -0
index ddf5531c48a9..2673b6e0caa8 100644

drivers/clocksource/timer-tegra.c static int __init tegra_init_timer(struct device_node *np, bool tegra20, · line 351

Quoted 7 lines
drivers/clocksource/timer-tegra.c +2 -0
@@ -345,6 +345,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 			irq_dispose_mapping(cpu_to->clkevt.irq);
 		}
 	}
+
+	to->of_base.base = timer_reg_base;
 out:
 	timer_of_cleanup(to);

Cheers
Jon

Signature · 1 line
nvpublic
Acked-by on v3 6/8
#15 lore raw

Re: [PATCH v3 6/8] clocksource/drivers/tegra: Restore base address before cleanup

In reply to Dmitry Osipenko
Quoted 9 lines · Dmitry Osipenko

We're adjusting the timer's base for each per-CPU timer to point to the actual start of the timer since device-tree defines a compound registers range that includes all of the timers. In this case the original base need to be restore before calling iounmap to unmap the proper address.

---
 drivers/clocksource/timer-tegra.c | 2 ++
 1 file changed, 2 insertions(+)
#16 lore raw

PATCH v3 7/8 clocksource/drivers/tegra: Cycles can't be 0

In reply to Dmitry Osipenko

Tegra's timer uses n+1 scheme for the counter, i.e. timer will fire after one tick if 0 is loaded. The minimum and maximum numbers of oneshot ticks are defined by clockevents_config_and_register(min, max) invocation and the min value is set to 1 tick. Hence "cycles" value can't ever be 0, unless it's a bug in clocksource core.

---
 drivers/clocksource/timer-tegra.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
drivers/clocksource/timer-tegra.c +10 -3
index 2673b6e0caa8..b84324288749 100644
@@ -54,9 +54,16 @@ static int tegra_timer_set_next_event(unsigned long cycles,
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 
-	writel_relaxed(TIMER_PTV_EN |
-		       ((cycles > 1) ? (cycles - 1) : 0), /* n+1 scheme */
-		       reg_base + TIMER_PTV);
+	/*
+	 * Tegra's timer uses n+1 scheme for the counter, i.e. timer will
+	 * fire after one tick if 0 is loaded.
+	 *
+	 * The minimum and maximum numbers of oneshot ticks are defined
+	 * by clockevents_config_and_register(1, 0x1fffffff + 1) invocation
+	 * below in the code. Hence the cycles (ticks) can't be outside of
+	 * a range supportable by hardware.
+	 */
+	writel_relaxed(TIMER_PTV_EN | (cycles - 1), reg_base + TIMER_PTV);
 
 	return 0;
 }
Signature · 1 line
2.22.0
maintainer Acked-by on v3 7/8
#17 lore raw

Re: [PATCH v3 7/8] clocksource/drivers/tegra: Cycles can't be 0

Quoted 26 lines · Dmitry Osipenko

Tegra's timer uses n+1 scheme for the counter, i.e. timer will fire after one tick if 0 is loaded. The minimum and maximum numbers of oneshot ticks are defined by clockevents_config_and_register(min, max) invocation and the min value is set to 1 tick. Hence "cycles" value can't ever be 0, unless it's a bug in clocksource core.

---
 drivers/clocksource/timer-tegra.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
drivers/clocksource/timer-tegra.c +3 -3
index 2673b6e0caa8..b84324288749 100644
@@ -54,9 +54,16 @@ static int tegra_timer_set_next_event(unsigned long cycles,
 {
 	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
 
-	writel_relaxed(TIMER_PTV_EN |
-		       ((cycles > 1) ? (cycles - 1) : 0), /* n+1 scheme */
-		       reg_base + TIMER_PTV);
+	/*
+	 * Tegra's timer uses n+1 scheme for the counter, i.e. timer will
+	 * fire after one tick if 0 is loaded.

drivers/clocksource/timer-tegra.c static int tegra_timer_set_next_event(unsigned long cycles, · line 69

Quoted 10 lines
drivers/clocksource/timer-tegra.c +7 -0
+	 *
+	 * The minimum and maximum numbers of oneshot ticks are defined
+	 * by clockevents_config_and_register(1, 0x1fffffff + 1) invocation
+	 * below in the code. Hence the cycles (ticks) can't be outside of
+	 * a range supportable by hardware.
+	 */
+	writel_relaxed(TIMER_PTV_EN | (cycles - 1), reg_base + TIMER_PTV);
 
 	return 0;
 }

Cheers
Jon

Signature · 1 line
nvpublic
Acked-by on v3 7/8
#18 lore raw

Re: [PATCH v3 7/8] clocksource/drivers/tegra: Cycles can't be 0

In reply to Dmitry Osipenko
Quoted 10 lines · Dmitry Osipenko

Tegra's timer uses n+1 scheme for the counter, i.e. timer will fire after one tick if 0 is loaded. The minimum and maximum numbers of oneshot ticks are defined by clockevents_config_and_register(min, max) invocation and the min value is set to 1 tick. Hence "cycles" value can't ever be 0, unless it's a bug in clocksource core.

---
 drivers/clocksource/timer-tegra.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
#19 lore raw

Re: [PATCH v3 7/8] clocksource/drivers/tegra: Cycles can't be 0

19.06.2019 11:19, Thierry Reding пишет:

Quoted 7 lines · Dmitry Osipenko
Quoted 6 lines

Tegra's timer uses n+1 scheme for the counter, i.e. timer will fire after one tick if 0 is loaded. The minimum and maximum numbers of oneshot ticks are defined by clockevents_config_and_register(min, max) invocation and the min value is set to 1 tick. Hence "cycles" value can't ever be 0, unless it's a bug in clocksource core.

Quoted 7 lines
Quoted 4 lines
---
 drivers/clocksource/timer-tegra.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Thanks!

#20 lore raw

PATCH v3 8/8 clocksource/drivers/tegra: Set up maximum-ticks limit properly

In reply to Dmitry Osipenko

Tegra's timer has 29 bits for the counter and for the "load" register which sets counter to a load-value. The counter's value is lower than the actual value by 1 because it starts to decrement after one tick, hence the maximum number of ticks that hardware can handle equals to 29 bits + 1.

---
 drivers/clocksource/timer-tegra.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/clocksource/timer-tegra.c +9 -1
index b84324288749..355b29ff6362 100644
@@ -137,9 +137,17 @@ static int tegra_timer_setup(unsigned int cpu)
 	irq_force_affinity(to->clkevt.irq, cpumask_of(cpu));
 	enable_irq(to->clkevt.irq);
 
+	/*
+	 * Tegra's timer uses n+1 scheme for the counter, i.e. timer will
+	 * fire after one tick if 0 is loaded and thus minimum number of
+	 * ticks is 1. In result both of the clocksource's tick limits are
+	 * higher than a minimum and maximum that hardware register can
+	 * take by 1, this is then taken into account by set_next_event
+	 * callback.
+	 */
 	clockevents_config_and_register(&to->clkevt, timer_of_rate(to),
 					1, /* min */
-					0x1fffffff); /* 29 bits */
+					0x1fffffff + 1); /* max 29 bits + 1 */
 
 	return 0;
 }
Signature · 1 line
2.22.0
maintainer Acked-by on v3 8/8
#21 lore raw

Re: [PATCH v3 8/8] clocksource/drivers/tegra: Set up maximum-ticks limit properly

Quoted 25 lines · Dmitry Osipenko

Tegra's timer has 29 bits for the counter and for the "load" register which sets counter to a load-value. The counter's value is lower than the actual value by 1 because it starts to decrement after one tick, hence the maximum number of ticks that hardware can handle equals to 29 bits + 1.

---
 drivers/clocksource/timer-tegra.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/clocksource/timer-tegra.c +5 -0
index b84324288749..355b29ff6362 100644
@@ -137,9 +137,17 @@ static int tegra_timer_setup(unsigned int cpu)
 	irq_force_affinity(to->clkevt.irq, cpumask_of(cpu));
 	enable_irq(to->clkevt.irq);
 
+	/*
+	 * Tegra's timer uses n+1 scheme for the counter, i.e. timer will
+	 * fire after one tick if 0 is loaded and thus minimum number of
+	 * ticks is 1. In result both of the clocksource's tick limits are
+	 * higher than a minimum and maximum that hardware register can

drivers/clocksource/timer-tegra.c static int tegra_timer_setup(unsigned int cpu) · line 153

Quoted 10 lines
drivers/clocksource/timer-tegra.c +4 -1
+	 * take by 1, this is then taken into account by set_next_event
+	 * callback.
+	 */
 	clockevents_config_and_register(&to->clkevt, timer_of_rate(to),
 					1, /* min */
-					0x1fffffff); /* 29 bits */
+					0x1fffffff + 1); /* max 29 bits + 1 */
 
 	return 0;
 }

Cheers
Jon

Signature · 1 line
nvpublic
Acked-by on v3 8/8
#22 lore raw

Re: [PATCH v3 8/8] clocksource/drivers/tegra: Set up maximum-ticks limit properly

In reply to Dmitry Osipenko
Quoted 10 lines · Dmitry Osipenko

Tegra's timer has 29 bits for the counter and for the "load" register which sets counter to a load-value. The counter's value is lower than the actual value by 1 because it starts to decrement after one tick, hence the maximum number of ticks that hardware can handle equals to 29 bits + 1.

---
 drivers/clocksource/timer-tegra.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)