Few more cleanups for tegra-timer

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 ("Support per-CPU timers on all Tegra's")

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +0 -0

index 9406855781ff..830c66e2d927 100644

Quoted 21 lines

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 ("Support per-CPU timers on all Tegra's")

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +2 -0

index 9406855781ff..830c66e2d927 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);
 

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

Quoted 2 lines

drivers/clocksource/timer-tegra.c +0 -0

 		cpu_to = per_cpu_ptr(&tegra_to, cpu);
 		cpu_to->of_base.base = timer_reg_base + base;

Thanks. This fixes a boot regression we are seeing on -next with Tegra210 (introduced by the commit referenced above). So ...

Acked-by Jon Hunter <[email protected]>

Tested-by Jon Hunter <[email protected]>

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

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +21 -15

index 830c66e2d927..810b4e7435cf 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;

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.

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +2 -2

index 810b4e7435cf..646b3530c2d2 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;

Quoted 27 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.

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +2 -2

index 810b4e7435cf..646b3530c2d2 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);
 

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

Quoted 6 lines

drivers/clocksource/timer-tegra.c +1 -0

 	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 = DIV_ROUND_UP(rate, HZ);

Any reason you made this a round-up?

14.06.2019 18:48, Jon Hunter пишет:

Quoted 29 lines · Dmitry Osipenko

Quoted 27 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.

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +2 -2

index 810b4e7435cf..646b3530c2d2 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);
 

Quoted 8 lines

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

Quoted 6 lines

drivers/clocksource/timer-tegra.c +1 -0

 	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 = DIV_ROUND_UP(rate, HZ);

Any reason you made this a round-up?

That's what timer_of_clk_init() does, I assume it should be a more correct variant.

Quoted 39 lines

14.06.2019 18:48, Jon Hunter пишет:

Quoted 37 lines · Dmitry Osipenko

Quoted 29 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.

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +2 -2

index 810b4e7435cf..646b3530c2d2 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 300

Quoted 4 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 = DIV_ROUND_UP(rate, HZ);

Any reason you made this a round-up?

Quoted 1 line

That's what timer_of_clk_init() does, I assume it should be a more correct variant.

Sounds to me like this should be 2 patches, because you are changing the value. This is not just purely cleanup IMO.

17.06.2019 13:51, Jon Hunter пишет:

Quoted 43 lines · Dmitry Osipenko

Quoted 40 lines

14.06.2019 18:48, Jon Hunter пишет:

Quoted 29 lines · Dmitry Osipenko

Quoted 27 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.

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +2 -2

index 810b4e7435cf..646b3530c2d2 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);
 

Quoted 8 lines

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

Quoted 6 lines

drivers/clocksource/timer-tegra.c +1 -0

 	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 = DIV_ROUND_UP(rate, HZ);

Any reason you made this a round-up?

That's what timer_of_clk_init() does, I assume it should be a more correct variant.

Quoted 2 lines

Sounds to me like this should be 2 patches, because you are changing the value. This is not just purely cleanup IMO.

Indeed, that could be at least mentioned in the commit message. Probably I just assumed that this is such a minor change that not worth anything. A hundred of microseconds is hardly noticeable.

I'm not really sure if this really worth a re-spin at this point. Jon, are you insisting?

Quoted 51 lines

17.06.2019 13:51, Jon Hunter пишет:

Quoted 41 lines · Dmitry Osipenko

Quoted 39 lines

14.06.2019 18:48, Jon Hunter пишет:

Quoted 37 lines · Dmitry Osipenko

Quoted 29 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.

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +2 -2

index 810b4e7435cf..646b3530c2d2 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 300

Quoted 4 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 = DIV_ROUND_UP(rate, HZ);

Any reason you made this a round-up?

Quoted 4 lines

Quoted 1 line

That's what timer_of_clk_init() does, I assume it should be a more correct variant.

Sounds to me like this should be 2 patches, because you are changing the value. This is not just purely cleanup IMO.

Indeed, that could be at least mentioned in the commit message. Probably I just assumed that this is such a minor change that not worth anything. A hundred of microseconds is hardly noticeable.

Quoted 1 line

I'm not really sure if this really worth a re-spin at this point. Jon, are you insisting?

At a minimum the changelog needs to be udpated to reflect what is going on here. Yes it may not be a massive difference, but I prefer not to change things without any rationale.

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

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +0 -0

index 646b3530c2d2..a7fa12488066 100644

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

Suggested-by Daniel Lezcano <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +4 -3

index a7fa12488066..2a428fdf702f 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);

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.

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +0 -0

index 2a428fdf702f..7be91db98bd7 100644

Quoted 19 lines

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.

Signed-off-by Dmitry Osipenko <[email protected]>

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

drivers/clocksource/timer-tegra.c +1 -0

index 2a428fdf702f..7be91db98bd7 100644
@@ -345,6 +345,8 @@ static int __init tegra_init_timer(struct device_node *np, bool tegra20,
 			irq_dispose_mapping(cpu_to->clkevt.irq);
 		}
 	}
+

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

Quoted 3 lines

drivers/clocksource/timer-tegra.c +1 -0

+	to->of_base.base = timer_reg_base;
 out:
 	timer_of_cleanup(to);

So what you are saying is that because we don't know which CPU executes the tegra_init_timer() function, then it is necessary to restore the base. IOW if it is not CPU0, then the base will be updated and hence, need to be restored. Correct?