[PATCH 1/4] clocksource: dw_apb_timer: Set clockevent any-possible-CPU mask

[PATCH 1/4] clocksource: dw_apb_timer: Set clockevent any-possible-CPU mask

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Currently the DW APB Timer driver binds all clockevent timers to
CPU #0. This isn't good for multiple reasons. First of all seeing
the device is placed on APB bus (which makes it accessible from any
CPU core), accessible over MMIO and having the DYNIRQ flag set we
can be sure that manually binding the timer to any CPU just isn't
correct. By doing so we just set an extra limitation on device usage.
This also doesn't reflect the device actual capability, since by
setting the IRQ affinity we can make it virtually local to any CPU.
Secondly imagine if you had a real CPU-local timer with the same
rating and the same CPU-affinity. In this case if DW APB timer was
registered first, then due to the clockevent framework tick-timer
selection procedure we'll end up with the real CPU-local timer being
left unselected for clock-events tracking. But on most of the platforms
(MIPS/ARM/etc) such timers are normally embedded into the CPU core and
are accessible with much better performance then devices placed on APB.
For instance in MIPS architectures there is r4k-timer, which is
CPU-local, assigned with the same rating, and normally its
clockevent device is registered after the platform-specific one.

So in order to fix all of these issues lets set the DW APB clockevent
timer cpumask to be 'cpu_possible_mask'. By doing so the clockevent
framework would prefer to select the real CPU-local timer instead
of DW APB one. Otherwise if there is no other than DW APB device for
clockevents tracking then it will be selected.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/clocksource/dw_apb_timer.c    | 18 +++++++-----------
 drivers/clocksource/dw_apb_timer_of.c |  3 +--
 include/linux/dw_apb_timer.h          |  2 +-
 3 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index 654766538f93..f213bcb4cf71 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -106,6 +106,7 @@ static irqreturn_t dw_apb_clockevent_irq(int irq, void *data)
 		dw_ced->eoi(&dw_ced->timer);
 
 	evt->event_handler(evt);
+
 	return IRQ_HANDLED;
 }
 
@@ -123,8 +124,7 @@ static int apbt_shutdown(struct clock_event_device *evt)
 	struct dw_apb_clock_event_device *dw_ced = ced_to_dw_apb_ced(evt);
 	u32 ctrl;
 
-	pr_debug("%s CPU %d state=shutdown\n", __func__,
-		 cpumask_first(evt->cpumask));
+	pr_debug("%s state=shutdown\n", __func__);
 
 	ctrl = apbt_readl(&dw_ced->timer, APBTMR_N_CONTROL);
 	ctrl &= ~APBTMR_CONTROL_ENABLE;
@@ -137,8 +137,7 @@ static int apbt_set_oneshot(struct clock_event_device *evt)
 	struct dw_apb_clock_event_device *dw_ced = ced_to_dw_apb_ced(evt);
 	u32 ctrl;
 
-	pr_debug("%s CPU %d state=oneshot\n", __func__,
-		 cpumask_first(evt->cpumask));
+	pr_debug("%s state=oneshot\n", __func__);
 
 	ctrl = apbt_readl(&dw_ced->timer, APBTMR_N_CONTROL);
 	/*
@@ -170,8 +169,7 @@ static int apbt_set_periodic(struct clock_event_device *evt)
 	unsigned long period = DIV_ROUND_UP(dw_ced->timer.freq, HZ);
 	u32 ctrl;
 
-	pr_debug("%s CPU %d state=periodic\n", __func__,
-		 cpumask_first(evt->cpumask));
+	pr_debug("%s state=periodic\n", __func__);
 
 	ctrl = apbt_readl(&dw_ced->timer, APBTMR_N_CONTROL);
 	ctrl |= APBTMR_CONTROL_MODE_PERIODIC;
@@ -194,8 +192,7 @@ static int apbt_resume(struct clock_event_device *evt)
 {
 	struct dw_apb_clock_event_device *dw_ced = ced_to_dw_apb_ced(evt);
 
-	pr_debug("%s CPU %d state=resume\n", __func__,
-		 cpumask_first(evt->cpumask));
+	pr_debug("%s state=resume\n", __func__);
 
 	apbt_enable_int(&dw_ced->timer);
 	return 0;
@@ -222,7 +219,6 @@ static int apbt_next_event(unsigned long delta,
 /**
  * dw_apb_clockevent_init() - use an APB timer as a clock_event_device
  *
- * @cpu:	The CPU the events will be targeted at.
  * @name:	The name used for the timer and the IRQ for it.
  * @rating:	The rating to give the timer.
  * @base:	I/O base for the timer registers.
@@ -237,7 +233,7 @@ static int apbt_next_event(unsigned long delta,
  * releasing the IRQ.
  */
 struct dw_apb_clock_event_device *
-dw_apb_clockevent_init(int cpu, const char *name, unsigned rating,
+dw_apb_clockevent_init(const char *name, unsigned int rating,
 		       void __iomem *base, int irq, unsigned long freq)
 {
 	struct dw_apb_clock_event_device *dw_ced =
@@ -257,7 +253,7 @@ dw_apb_clockevent_init(int cpu, const char *name, unsigned rating,
 	dw_ced->ced.max_delta_ticks = 0x7fffffff;
 	dw_ced->ced.min_delta_ns = clockevent_delta2ns(5000, &dw_ced->ced);
 	dw_ced->ced.min_delta_ticks = 5000;
-	dw_ced->ced.cpumask = cpumask_of(cpu);
+	dw_ced->ced.cpumask = cpu_possible_mask;
 	dw_ced->ced.features = CLOCK_EVT_FEAT_PERIODIC |
 				CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ;
 	dw_ced->ced.set_state_shutdown = apbt_shutdown;
diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index 8c28b127759f..0a2505b323d7 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -73,8 +73,7 @@ static void __init add_clockevent(struct device_node *event_timer)
 
 	timer_get_base_and_rate(event_timer, &iobase, &rate);
 
-	ced = dw_apb_clockevent_init(0, event_timer->name, 300, iobase, irq,
-				     rate);
+	ced = dw_apb_clockevent_init(event_timer->name, 300, iobase, irq, rate);
 	if (!ced)
 		panic("Unable to initialise clockevent device");
 
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index 14f072edbca5..2f3597a199bb 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -40,7 +40,7 @@ void dw_apb_clockevent_resume(struct dw_apb_clock_event_device *dw_ced);
 void dw_apb_clockevent_stop(struct dw_apb_clock_event_device *dw_ced);
 
 struct dw_apb_clock_event_device *
-dw_apb_clockevent_init(int cpu, const char *name, unsigned rating,
+dw_apb_clockevent_init(const char *name, unsigned int rating,
 		       void __iomem *base, int irq, unsigned long freq);
 struct dw_apb_clocksource *
 dw_apb_clocksource_init(unsigned rating, const char *name, void __iomem *base,
-- 
2.25.1

[PATCH 3/4] clocksource: mips-gic-timer: Register as sched_clock

[Sergey.Semin]

From: Paul Burton <paulburton@kernel.org>

The MIPS GIC timer is well suited for use as sched_clock, so register it
as such.

Whilst the existing gic_read_count() function matches the prototype
needed by sched_clock_register() already, we split it into 2 functions
in order to remove the need to evaluate the mips_cm_is64 condition
within each call since sched_clock should be as fast as possible.

Signed-off-by: Paul Burton <paulburton@kernel.org>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/clocksource/mips-gic-timer.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index 37671a5d4ed9..8239ff99cfe4 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -16,6 +16,7 @@
 #include <linux/notifier.h>
 #include <linux/of_irq.h>
 #include <linux/percpu.h>
+#include <linux/sched_clock.h>
 #include <linux/smp.h>
 #include <linux/time.h>
 #include <asm/mips-cps.h>
@@ -24,13 +25,10 @@ static DEFINE_PER_CPU(struct clock_event_device, gic_clockevent_device);
 static int gic_timer_irq;
 static unsigned int gic_frequency;
 
-static u64 notrace gic_read_count(void)
+static u64 notrace gic_read_count_2x32(void)
 {
 	unsigned int hi, hi2, lo;
 
-	if (mips_cm_is64)
-		return read_gic_counter();
-
 	do {
 		hi = read_gic_counter_32h();
 		lo = read_gic_counter_32l();
@@ -40,6 +38,19 @@ static u64 notrace gic_read_count(void)
 	return (((u64) hi) << 32) + lo;
 }
 
+static u64 notrace gic_read_count_64(void)
+{
+	return read_gic_counter();
+}
+
+static u64 notrace gic_read_count(void)
+{
+	if (mips_cm_is64)
+		return gic_read_count_64();
+
+	return gic_read_count_2x32();
+}
+
 static int gic_next_event(unsigned long delta, struct clock_event_device *evt)
 {
 	int cpu = cpumask_first(evt->cpumask);
@@ -228,6 +239,10 @@ static int __init gic_clocksource_of_init(struct device_node *node)
 	/* And finally start the counter */
 	clear_gic_config(GIC_CONFIG_COUNTSTOP);
 
+	sched_clock_register(mips_cm_is64 ?
+			     gic_read_count_64 : gic_read_count_2x32,
+			     64, gic_frequency);
+
 	return 0;
 }
 TIMER_OF_DECLARE(mips_gic_timer, "mti,gic-timer",
-- 
2.25.1

[PATCH 4/4] clocksource: mips-gic-timer: Set limitations on clocksource/sched-clocks usage

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Currently neither clocksource nor scheduler clock kernel framework
support the clocks with variable frequency. Needless to say how many
problems may cause the sudden base clocks frequency change. In a
simplest case the system time will either slow down or speed up.
Since on CM2.5 and earlier MIPS GIC timer is synchronously clocked
with CPU we must set some limitations on using it for these frameworks
if CPU frequency may change. First of all it's not safe to have the
MIPS GIC used for scheduler timings. So we shouldn't proceed with
the clocks registration in the sched-subsystem. Secondly we must
significantly decrease the MIPS GIC clocksource rating. This will let
the system to use it only as a last resort.

Note CM3.x-based systems may also experience the problems with MIPS GIC
if they the CPU-frequency change activated for the whole CPU cluster
instead of using the individual CPC core clocks divider.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/clocksource/mips-gic-timer.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index 8239ff99cfe4..5eb241b8b28d 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -185,7 +185,10 @@ static int __init __gic_clocksource_init(void)
 	gic_clocksource.mask = CLOCKSOURCE_MASK(count_width);
 
 	/* Calculate a somewhat reasonable rating value. */
-	gic_clocksource.rating = 200 + gic_frequency / 10000000;
+	if (mips_cm_revision() >= CM_REV_CM3 || !IS_ENABLED(CONFIG_CPU_FREQ))
+		gic_clocksource.rating = 200 + gic_frequency / 10000000;
+	else
+		gic_clocksource.rating = 99;
 
 	ret = clocksource_register_hz(&gic_clocksource, gic_frequency);
 	if (ret < 0)
@@ -239,9 +242,11 @@ static int __init gic_clocksource_of_init(struct device_node *node)
 	/* And finally start the counter */
 	clear_gic_config(GIC_CONFIG_COUNTSTOP);
 
-	sched_clock_register(mips_cm_is64 ?
-			     gic_read_count_64 : gic_read_count_2x32,
-			     64, gic_frequency);
+	if (mips_cm_revision() >= CM_REV_CM3 || !IS_ENABLED(CONFIG_CPU_FREQ)) {
+		sched_clock_register(mips_cm_is64 ?
+				     gic_read_count_64 : gic_read_count_2x32,
+				     64, gic_frequency);
+	}
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 0/4] clocksource: Fix MIPS GIC and DW APB Timer for Baikal-T1 SoC support

[Sergey Semin]

On Fri, Mar 06, 2020 at 03:56:00PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <fancer.lancer@gmail.com>
> 
> Aside from MIPS-specific r4k timer Baikal-T1 chip also provides a functionality
> of two another timers: embedded into the MIPS GIC timer and three external DW
> timers available over APB bus. But we can't use them before the corresponding
> drivers are properly fixed. First of all DW APB Timer shouldn't be bound to a
> single CPU, since as being accessible over APB they are external with respect
> to all possible CPUs. Secondly there might be more than just two DW APB Timers
> in the system (Baikal-T1 has three of them), so permit the driver to use one of
> them as a clocksource and the rest - for clockevents. Thirdly it's possible to
> use MIPS GIC timer as a clocksource so register it in the corresponding
> subsystem (the patch has been found in the Paul Burton MIPS repo so I left the
> original Signed-off-by attribute). Finally in the same way as r4k timer the
> MIPS GIC timer should be used with care when CPUFREQ config is enabled since in
> case of CM2 the timer counting depends on the CPU reference clock frequency
> while the clocksource subsystem currently doesn't support the timers with
> non-stable clock.
> 
> This patchset is rebased and tested on the mainline Linux kernel 5.6-rc4:
> commit 98d54f81e36b ("Linux 5.6-rc4").
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-kernel@vger.kernel.org
> 
> Paul Burton (1):
>   clocksource: mips-gic-timer: Register as sched_clock
> 
> Serge Semin (3):
>   clocksource: dw_apb_timer: Set clockevent any-possible-CPU mask
>   clocksource: dw_apb_timer_of: Fix missing clockevent timers
>   clocksource: mips-gic-timer: Set limitations on
>     clocksource/sched-clocks usage
> 
>  drivers/clocksource/dw_apb_timer.c    | 18 +++++++---------
>  drivers/clocksource/dw_apb_timer_of.c |  9 +++-----
>  drivers/clocksource/mips-gic-timer.c  | 30 ++++++++++++++++++++++-----
>  include/linux/dw_apb_timer.h          |  2 +-
>  4 files changed, 36 insertions(+), 23 deletions(-)
> 
> -- 
> 2.25.1
> 

Folks,

It appears our corporate email server changes the Message-Id field of
messages passing through it. Due to that the emails threading gets to be
broken. I'll resubmit the properly structured patchset as soon as our system
administrator fixes the problem. Sorry for the inconvenience cause by it.

Regards,
-Sergey