[PATCH v1 0/6] Add module support for Arm64 Exynos MCT driver

[PATCH v1 0/6] Add module support for Arm64 Exynos MCT driver

[Will McVicker]

This series adds support to build the Arm64 Exynos MCT driver as a module. This
is only possible on Arm64 SoCs since they can use the Arm architected timer as
the clocksource. Once the Exynos MCT module is loaded and the device probes,
the MCT driver is used as the wakeup source for the arch_timer.

These patches are originally from the downstream Pixel 6 (gs101) kernel found
at [1] and have been adapted for upstream. Not only has the Exynos MCT driver
been shipping as a module in the field with Android, but I've also tested this
seris with the upstream kernel on my Pixel 6 Pro.

Thanks,
Will

Note1, instructions to build and flash a Pixel 6 device with the upstream kernel
can be found at [2].

Note2, this series is based off of linux-next/master commit 405e2241def8 ("Add
linux-next specific files for 20250331").

[1] https://android.googlesource.com/kernel/gs/+log/refs/heads/android-gs-raviole-5.10-android12-d1
[2] https://git.codelinaro.org/linaro/googlelt/pixelscripts/-/blob/clo/main/README.md?ref_type=heads

Donghoon Yu (1):
  clocksource/drivers/exynos_mct: Add module support

Hosung Kim (1):
  clocksource/drivers/exynos_mct: Set local timer interrupts as percpu

Will Deacon (1):
  arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes

Will McVicker (3):
  of/irq: Export of_irq_count for modules
  clocksource/drivers/exynos_mct: Don't register as a sched_clock on
    arm64
  arm64: exynos: Drop select CLKSRC_EXYNOS_MCT

 arch/arm64/Kconfig.platforms                 |  1 -
 arch/arm64/boot/dts/exynos/google/gs101.dtsi |  3 ++
 drivers/clocksource/Kconfig                  |  3 +-
 drivers/clocksource/exynos_mct.c             | 55 ++++++++++++++++----
 drivers/of/irq.c                             |  1 +
 5 files changed, 51 insertions(+), 12 deletions(-)

-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v1 1/6] of/irq: Export of_irq_count for modules

[Will McVicker]

Need to export `of_irq_count` in preparation for modularizing the Exynos
MCT driver which uses this API for setting up the timer IRQs.

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index f8ad79b9b1c9..5adda1dac3cf 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -519,6 +519,7 @@ int of_irq_count(struct device_node *dev)
 
 	return nr;
 }
+EXPORT_SYMBOL_GPL(of_irq_count);
 
 /**
  * of_irq_to_resource_table - Fill in resource table with node's IRQ info
-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v1 2/6] clocksource/drivers/exynos_mct: Don't register as a sched_clock on arm64

[Will McVicker]

When using the Exynos MCT as a sched_clock, accessing the timer value
via the MCT register is extremely slow. To improve performance on Arm64
SoCs, use the Arm architected timer instead for timekeeping.

Note, ARM32 SoCs don't have an architectured timer and therefore
will continue to use the MCT timer. Detailed discussion on this topic
can be found at [1].

[1] https://lore.kernel.org/all/1400188079-21832-1-git-send-email-chirantan@chromium.org/

Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
[Original commit from https://android.googlesource.com/kernel/gs/+/630817f7080e92c5e0216095ff52f6eb8dd00727
Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 drivers/clocksource/exynos_mct.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index da09f467a6bb..05c50f2f7a7e 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -219,12 +219,12 @@ static struct clocksource mct_frc = {
 	.resume		= exynos4_frc_resume,
 };
 
+#if defined(CONFIG_ARM)
 static u64 notrace exynos4_read_sched_clock(void)
 {
 	return exynos4_read_count_32();
 }
 
-#if defined(CONFIG_ARM)
 static struct delay_timer exynos4_delay_timer;
 
 static cycles_t exynos4_read_current_timer(void)
@@ -250,12 +250,13 @@ static int __init exynos4_clocksource_init(bool frc_shared)
 	exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
 	exynos4_delay_timer.freq = clk_rate;
 	register_current_timer_delay(&exynos4_delay_timer);
+
+	sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
 #endif
 
 	if (clocksource_register_hz(&mct_frc, clk_rate))
 		panic("%s: can't register clocksource\n", mct_frc.name);
 
-	sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
 
 	return 0;
 }
-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v1 3/6] clocksource/drivers/exynos_mct: Set local timer interrupts as percpu

[Will McVicker]

From: Hosung Kim <hosung0.kim@samsung.com>

The MCT local timers can be used as a per-cpu event timer. To prevent
the timer interrupts from migrating to other CPUs, set the flag
IRQF_PERCPU.

Signed-off-by: Hosung Kim <hosung0.kim@samsung.com>
[Original commit from https://android.googlesource.com/kernel/gs/+/03267fad19f093bac979ca78309483e9eb3a8d16]
Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 drivers/clocksource/exynos_mct.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 05c50f2f7a7e..21ded37137d7 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -590,7 +590,8 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
 			irq_set_status_flags(mct_irq, IRQ_NOAUTOEN);
 			if (request_irq(mct_irq,
 					exynos4_mct_tick_isr,
-					IRQF_TIMER | IRQF_NOBALANCING,
+					IRQF_TIMER | IRQF_NOBALANCING |
+					IRQF_PERCPU,
 					pcpu_mevt->name, pcpu_mevt)) {
 				pr_err("exynos-mct: cannot register IRQ (cpu%d)\n",
 									cpu);
-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v1 4/6] arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes

[Will McVicker]

From: Will Deacon <willdeacon@google.com>

In preparation for switching to the architected timer as the primary
clockevents device, mark the cpuidle nodes with the 'local-timer-stop'
property to indicate that an alternative clockevents device must be
used for waking up from the "c2" idle state.

Signed-off-by: Will Deacon <willdeacon@google.com>
[Original commit from https://android.googlesource.com/kernel/gs/+/a896fd98638047989513d05556faebd28a62b27c]
Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 arch/arm64/boot/dts/exynos/google/gs101.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
index 3de3a758f113..fd0badf24e6f 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
+++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
@@ -155,6 +155,7 @@ ananke_cpu_sleep: cpu-ananke-sleep {
 				idle-state-name = "c2";
 				compatible = "arm,idle-state";
 				arm,psci-suspend-param = <0x0010000>;
+				local-timer-stop;
 				entry-latency-us = <70>;
 				exit-latency-us = <160>;
 				min-residency-us = <2000>;
@@ -164,6 +165,7 @@ enyo_cpu_sleep: cpu-enyo-sleep {
 				idle-state-name = "c2";
 				compatible = "arm,idle-state";
 				arm,psci-suspend-param = <0x0010000>;
+				local-timer-stop;
 				entry-latency-us = <150>;
 				exit-latency-us = <190>;
 				min-residency-us = <2500>;
@@ -173,6 +175,7 @@ hera_cpu_sleep: cpu-hera-sleep {
 				idle-state-name = "c2";
 				compatible = "arm,idle-state";
 				arm,psci-suspend-param = <0x0010000>;
+				local-timer-stop;
 				entry-latency-us = <235>;
 				exit-latency-us = <220>;
 				min-residency-us = <3500>;
-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v1 5/6] clocksource/drivers/exynos_mct: Add module support

[Will McVicker]

From: Donghoon Yu <hoony.yu@samsung.com>

On Arm64 platforms the Exynos MCT driver can be built as a module. On
boot (and even after boot) the arch_timer is used as the clocksource and
tick timer. Once the MCT driver is loaded, it can be used as the wakeup
source for the arch_timer.

Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
[Original commit from https://android.googlesource.com/kernel/gs/+/8a52a8288ec7d88ff78f0b37480dbb0e9c65bbfd]
Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 drivers/clocksource/Kconfig      |  3 +-
 drivers/clocksource/exynos_mct.c | 47 +++++++++++++++++++++++++++-----
 2 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 487c85259967..e5d9d8383607 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -443,7 +443,8 @@ config ATMEL_TCB_CLKSRC
 	  Support for Timer Counter Blocks on Atmel SoCs.
 
 config CLKSRC_EXYNOS_MCT
-	bool "Exynos multi core timer driver" if COMPILE_TEST
+	tristate "Exynos multi core timer driver"
+	default y if ARCH_EXYNOS
 	depends on ARM || ARM64
 	depends on ARCH_ARTPEC || ARCH_EXYNOS || COMPILE_TEST
 	help
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 21ded37137d7..da4460d8a0ba 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -15,9 +15,11 @@
 #include <linux/cpu.h>
 #include <linux/delay.h>
 #include <linux/percpu.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
+#include <linux/platform_device.h>
 #include <linux/clocksource.h>
 #include <linux/sched_clock.h>
 
@@ -235,7 +237,7 @@ static cycles_t exynos4_read_current_timer(void)
 }
 #endif
 
-static int __init exynos4_clocksource_init(bool frc_shared)
+static int exynos4_clocksource_init(bool frc_shared)
 {
 	/*
 	 * When the frc is shared, the main processor should have already
@@ -507,7 +509,7 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
 	return 0;
 }
 
-static int __init exynos4_timer_resources(struct device_node *np)
+static int exynos4_timer_resources(struct device_node *np)
 {
 	struct clk *mct_clk, *tick_clk;
 
@@ -535,7 +537,7 @@ static int __init exynos4_timer_resources(struct device_node *np)
  * @local_idx: array mapping CPU numbers to local timer indices
  * @nr_local: size of @local_idx array
  */
-static int __init exynos4_timer_interrupts(struct device_node *np,
+static int exynos4_timer_interrupts(struct device_node *np,
 					   unsigned int int_type,
 					   const u32 *local_idx,
 					   size_t nr_local)
@@ -640,7 +642,7 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
 	return err;
 }
 
-static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
+static int mct_init_dt(struct device_node *np, unsigned int int_type)
 {
 	bool frc_shared = of_property_read_bool(np, "samsung,frc-shared");
 	u32 local_idx[MCT_NR_LOCAL] = {0};
@@ -688,15 +690,46 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
 	return exynos4_clockevent_init();
 }
 
-
-static int __init mct_init_spi(struct device_node *np)
+static int mct_init_spi(struct device_node *np)
 {
 	return mct_init_dt(np, MCT_INT_SPI);
 }
 
-static int __init mct_init_ppi(struct device_node *np)
+static int mct_init_ppi(struct device_node *np)
 {
 	return mct_init_dt(np, MCT_INT_PPI);
 }
+
+#ifdef MODULE
+static int exynos4_mct_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	if (of_machine_is_compatible("samsung,exynos4412-mct"))
+		return mct_init_ppi(np);
+
+	return mct_init_spi(np);
+}
+
+static const struct of_device_id exynos4_mct_match_table[] = {
+	{ .compatible = "samsung,exynos4210-mct" },
+	{ .compatible = "samsung,exynos4412-mct" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, exynos4_mct_match_table);
+
+static struct platform_driver exynos4_mct_driver = {
+	.probe		= exynos4_mct_probe,
+	.driver		= {
+		.name	= "exynos-mct",
+		.of_match_table = exynos4_mct_match_table,
+	},
+};
+module_platform_driver(exynos4_mct_driver);
+#else
 TIMER_OF_DECLARE(exynos4210, "samsung,exynos4210-mct", mct_init_spi);
 TIMER_OF_DECLARE(exynos4412, "samsung,exynos4412-mct", mct_init_ppi);
+#endif
+
+MODULE_DESCRIPTION("Exynos Multi Core Timer Driver");
+MODULE_LICENSE("GPL");
-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v1 6/6] arm64: exynos: Drop select CLKSRC_EXYNOS_MCT

[Will McVicker]

Since the Exynos MCT driver can be built as a module for some Arm64 SoCs
like gs101, drop force-selecting it as a built-in driver by ARCH_EXYNOS
and instead depend on `default y if ARCH_EXYNOS` to select it
automatically. This allows platforms like Android to build the driver as
a module if desired.

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 arch/arm64/Kconfig.platforms | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 8b76821f190f..325279193e2c 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -109,7 +109,6 @@ config ARCH_BLAIZE
 config ARCH_EXYNOS
 	bool "Samsung Exynos SoC family"
 	select COMMON_CLK_SAMSUNG
-	select CLKSRC_EXYNOS_MCT
 	select EXYNOS_PM_DOMAINS if PM_GENERIC_DOMAINS
 	select EXYNOS_PMU
 	select PINCTRL
-- 
2.49.0.472.ge94155a9ec-goog

Re: [PATCH v1 2/6] clocksource/drivers/exynos_mct: Don't register as a sched_clock on arm64

[John Stultz]

On Mon, Mar 31, 2025 at 4:00 PM 'Will McVicker' via kernel-team
<kernel-team@android.com> wrote:
>
> When using the Exynos MCT as a sched_clock, accessing the timer value
> via the MCT register is extremely slow. To improve performance on Arm64
> SoCs, use the Arm architected timer instead for timekeeping.

This probably needs some further expansion to explain why we don't
want to use it for sched_clock but continue to register the MCT as a
clocksource (ie: why not disable MCT entirely?).

> Note, ARM32 SoCs don't have an architectured timer and therefore
> will continue to use the MCT timer. Detailed discussion on this topic
> can be found at [1].
>
> [1] https://lore.kernel.org/all/1400188079-21832-1-git-send-email-chirantan@chromium.org/

That's a pretty deep thread (more so with the duplicate messages, as
you used the "all" instead of a specific list). It might be good to
have a bit more of a summary here in the commit message, so folks
don't have to dig too deeply themselves.

> Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> [Original commit from https://android.googlesource.com/kernel/gs/+/630817f7080e92c5e0216095ff52f6eb8dd00727
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---
>  drivers/clocksource/exynos_mct.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index da09f467a6bb..05c50f2f7a7e 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -219,12 +219,12 @@ static struct clocksource mct_frc = {
>         .resume         = exynos4_frc_resume,
>  };
>
> +#if defined(CONFIG_ARM)

I'd probably suggest adding a comment here explaining why this is kept
on ARM and not on AARCH64 as well.

>  static u64 notrace exynos4_read_sched_clock(void)
>  {
>         return exynos4_read_count_32();
>  }
>
> -#if defined(CONFIG_ARM)
>  static struct delay_timer exynos4_delay_timer;
>
>  static cycles_t exynos4_read_current_timer(void)
> @@ -250,12 +250,13 @@ static int __init exynos4_clocksource_init(bool frc_shared)
>         exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
>         exynos4_delay_timer.freq = clk_rate;
>         register_current_timer_delay(&exynos4_delay_timer);
> +
> +       sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
>  #endif
>
>         if (clocksource_register_hz(&mct_frc, clk_rate))
>                 panic("%s: can't register clocksource\n", mct_frc.name);
>
> -       sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
>
>         return 0;

Otherwise, this looks ok to me.

thanks
-john

Re: [PATCH v1 3/6] clocksource/drivers/exynos_mct: Set local timer interrupts as percpu

[John Stultz]

On Mon, Mar 31, 2025 at 4:00 PM 'Will McVicker' via kernel-team
<kernel-team@android.com> wrote:
>
> From: Hosung Kim <hosung0.kim@samsung.com>
>
> The MCT local timers can be used as a per-cpu event timer. To prevent

Can be used, or are used?  If it's an option, is this change important
in both cases?

> the timer interrupts from migrating to other CPUs, set the flag
> IRQF_PERCPU.

Might be work expanding this a bit to clarify why the interrupts
migrating to other cpus is undesired.

> Signed-off-by: Hosung Kim <hosung0.kim@samsung.com>
> [Original commit from https://android.googlesource.com/kernel/gs/+/03267fad19f093bac979ca78309483e9eb3a8d16]
> Signed-off-by: Will McVicker <willmcvicker@google.com>

thanks!
-john

Re: [PATCH v1 5/6] clocksource/drivers/exynos_mct: Add module support

[Rob Herring]

On Mon, Mar 31, 2025 at 04:00:27PM -0700, Will McVicker wrote:
> From: Donghoon Yu <hoony.yu@samsung.com>
> 
> On Arm64 platforms the Exynos MCT driver can be built as a module. On
> boot (and even after boot) the arch_timer is used as the clocksource and
> tick timer. Once the MCT driver is loaded, it can be used as the wakeup
> source for the arch_timer.
> 
> Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> [Original commit from https://android.googlesource.com/kernel/gs/+/8a52a8288ec7d88ff78f0b37480dbb0e9c65bbfd]
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---
>  drivers/clocksource/Kconfig      |  3 +-
>  drivers/clocksource/exynos_mct.c | 47 +++++++++++++++++++++++++++-----
>  2 files changed, 42 insertions(+), 8 deletions(-)

[...]

> +#ifdef MODULE
> +static int exynos4_mct_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	if (of_machine_is_compatible("samsung,exynos4412-mct"))

Your root node compatible has "samsung,exynos4412-mct"!?

In any case, add a data ptr to of_device_id table and then use the match 
data rather than comparing compatible strings again.

> +		return mct_init_ppi(np);
> +
> +	return mct_init_spi(np);
> +}
> +
> +static const struct of_device_id exynos4_mct_match_table[] = {
> +	{ .compatible = "samsung,exynos4210-mct" },
> +	{ .compatible = "samsung,exynos4412-mct" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, exynos4_mct_match_table);

Re: [PATCH v1 1/6] of/irq: Export of_irq_count for modules

[Rob Herring]

On Mon, Mar 31, 2025 at 04:00:23PM -0700, Will McVicker wrote:
> Need to export `of_irq_count` in preparation for modularizing the Exynos
> MCT driver which uses this API for setting up the timer IRQs.
> 
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---
>  drivers/of/irq.c | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v1 5/6] clocksource/drivers/exynos_mct: Add module support

[Krzysztof Kozlowski]

On 01/04/2025 01:00, Will McVicker wrote:
> From: Donghoon Yu <hoony.yu@samsung.com>
> 
> On Arm64 platforms the Exynos MCT driver can be built as a module. On
> boot (and even after boot) the arch_timer is used as the clocksource and
> tick timer. Once the MCT driver is loaded, it can be used as the wakeup
> source for the arch_timer.
> 
> Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> [Original commit from https://android.googlesource.com/kernel/gs/+/8a52a8288ec7d88ff78f0b37480dbb0e9c65bbfd]
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---
>  drivers/clocksource/Kconfig      |  3 +-
>  drivers/clocksource/exynos_mct.c | 47 +++++++++++++++++++++++++++-----
>  2 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 487c85259967..e5d9d8383607 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -443,7 +443,8 @@ config ATMEL_TCB_CLKSRC
>  	  Support for Timer Counter Blocks on Atmel SoCs.
>  
>  config CLKSRC_EXYNOS_MCT
> -	bool "Exynos multi core timer driver" if COMPILE_TEST
> +	tristate "Exynos multi core timer driver"
> +	default y if ARCH_EXYNOS
>  	depends on ARM || ARM64
>  	depends on ARCH_ARTPEC || ARCH_EXYNOS || COMPILE_TEST
I am not sure if you actually tested it as module. On arm I cannot build
it even:

ERROR: modpost: "register_current_timer_delay"
[drivers/clocksource/exynos_mct.ko] undefined!
ERROR: modpost: "sched_clock_register"
[drivers/clocksource/exynos_mct.ko] undefined!

Best regards,
Krzysztof

Re: [PATCH v1 5/6] clocksource/drivers/exynos_mct: Add module support

[William McVicker]

On 04/01/2025, Krzysztof Kozlowski wrote:
> On 01/04/2025 01:00, Will McVicker wrote:
> > From: Donghoon Yu <hoony.yu@samsung.com>
> > 
> > On Arm64 platforms the Exynos MCT driver can be built as a module. On
> > boot (and even after boot) the arch_timer is used as the clocksource and
> > tick timer. Once the MCT driver is loaded, it can be used as the wakeup
> > source for the arch_timer.
> > 
> > Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
> > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> > [Original commit from https://android.googlesource.com/kernel/gs/+/8a52a8288ec7d88ff78f0b37480dbb0e9c65bbfd]
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > ---
> >  drivers/clocksource/Kconfig      |  3 +-
> >  drivers/clocksource/exynos_mct.c | 47 +++++++++++++++++++++++++++-----
> >  2 files changed, 42 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 487c85259967..e5d9d8383607 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -443,7 +443,8 @@ config ATMEL_TCB_CLKSRC
> >  	  Support for Timer Counter Blocks on Atmel SoCs.
> >  
> >  config CLKSRC_EXYNOS_MCT
> > -	bool "Exynos multi core timer driver" if COMPILE_TEST
> > +	tristate "Exynos multi core timer driver"
> > +	default y if ARCH_EXYNOS
> >  	depends on ARM || ARM64
> >  	depends on ARCH_ARTPEC || ARCH_EXYNOS || COMPILE_TEST
> I am not sure if you actually tested it as module. On arm I cannot build
> it even:
> 
> ERROR: modpost: "register_current_timer_delay"
> [drivers/clocksource/exynos_mct.ko] undefined!
> ERROR: modpost: "sched_clock_register"
> [drivers/clocksource/exynos_mct.ko] undefined!

I tested with the gs101 ARM64 configuration. You're right it won't work with
ARM32. Thanks for catching this! Since ARM32 architectures don't have the
arch_timer, I'm not sure if we can actually support Exynos MCT as a module as
you wouldn't have any available clocksource during boot. I'll update the
Kconfig for v2 to handle this and make sure it works for ARM32. I'm guessing
it'll work with something like:

config CLKSRC_EXYNOS_MCT
	tristate "Exynos multi core timer driver" if ARM64


Regards,
Will

[...]

Re: [PATCH v1 5/6] clocksource/drivers/exynos_mct: Add module support

[William McVicker]

On 03/31/2025, Rob Herring wrote:
> On Mon, Mar 31, 2025 at 04:00:27PM -0700, Will McVicker wrote:
> > From: Donghoon Yu <hoony.yu@samsung.com>
> > 
> > On Arm64 platforms the Exynos MCT driver can be built as a module. On
> > boot (and even after boot) the arch_timer is used as the clocksource and
> > tick timer. Once the MCT driver is loaded, it can be used as the wakeup
> > source for the arch_timer.
> > 
> > Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
> > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> > [Original commit from https://android.googlesource.com/kernel/gs/+/8a52a8288ec7d88ff78f0b37480dbb0e9c65bbfd]
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > ---
> >  drivers/clocksource/Kconfig      |  3 +-
> >  drivers/clocksource/exynos_mct.c | 47 +++++++++++++++++++++++++++-----
> >  2 files changed, 42 insertions(+), 8 deletions(-)
> 
> [...]
> 
> > +#ifdef MODULE
> > +static int exynos4_mct_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +
> > +	if (of_machine_is_compatible("samsung,exynos4412-mct"))
> 
> Your root node compatible has "samsung,exynos4412-mct"!?
> 
> In any case, add a data ptr to of_device_id table and then use the match 
> data rather than comparing compatible strings again.

Ah yes, you're right. Thanks for the suggestion! I'll update on v2.

Regards,
Will

[...]

Re: [PATCH v1 3/6] clocksource/drivers/exynos_mct: Set local timer interrupts as percpu

[William McVicker]

On 03/31/2025, John Stultz wrote:
> On Mon, Mar 31, 2025 at 4:00 PM 'Will McVicker' via kernel-team
> <kernel-team@android.com> wrote:
> >
> > From: Hosung Kim <hosung0.kim@samsung.com>
> >
> > The MCT local timers can be used as a per-cpu event timer. To prevent
> 
> Can be used, or are used?  If it's an option, is this change important
> in both cases?
> 
> > the timer interrupts from migrating to other CPUs, set the flag
> > IRQF_PERCPU.
> 
> Might be work expanding this a bit to clarify why the interrupts
> migrating to other cpus is undesired.

Let me dig into this further to figure out if the IP has a limitation where the
interrupts need to be handled by the CPU the timer was triggered on or if this
is just an optimization.

Any chance you know this @Youngmin?

Thanks,
Will

> 
> > Signed-off-by: Hosung Kim <hosung0.kim@samsung.com>
> > [Original commit from https://android.googlesource.com/kernel/gs/+/03267fad19f093bac979ca78309483e9eb3a8d16]
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> 
> thanks!
> -john

Re: [PATCH v1 2/6] clocksource/drivers/exynos_mct: Don't register as a sched_clock on arm64

[William McVicker]

On 03/31/2025, John Stultz wrote:
> On Mon, Mar 31, 2025 at 4:00 PM 'Will McVicker' via kernel-team
> <kernel-team@android.com> wrote:
> >
> > When using the Exynos MCT as a sched_clock, accessing the timer value
> > via the MCT register is extremely slow. To improve performance on Arm64
> > SoCs, use the Arm architected timer instead for timekeeping.
> 
> This probably needs some further expansion to explain why we don't
> want to use it for sched_clock but continue to register the MCT as a
> clocksource (ie: why not disable MCT entirely?).

Using the MCT as a sched_clock was originally added for Exynos4 SoCs to improve
the gettimeofday() syscalls on ChromeOS. For ARM32 this is the best they can do
without the Arm architected timer. ChromeOS perf data can be found in [1,2]

[1] https://lore.kernel.org/linux-samsung-soc/CAJFHJrrgWGc4XGQB0ysLufAg3Wouz-aYXu97Sy2Kp=HzK+akVQ@mail.gmail.com/
[2] https://lore.kernel.org/linux-samsung-soc/CAASgrz2Nr69tpfC8ka9gbs2OvjLEGsvgAj4vBCFxhsamuFum7w@mail.gmail.com/

I think it's valid to still register the MCT as a clocksource to make it
available in case someone decides they want to use it, but by default it
doesn't make sense to use it as the default clocksource on Exynos-based ARM64
systems with arch_timer support. However, we can't disable the Exynos MCT
entirely on ARM64 because we need it as the wakeup source for the arch_timer to
support waking up from the "c2" idle state, which is discussed in [3].

[3] https://lore.kernel.org/linux-arm-kernel/20210608154341.10794-1-will@kernel.org/

> 
> > Note, ARM32 SoCs don't have an architectured timer and therefore
> > will continue to use the MCT timer. Detailed discussion on this topic
> > can be found at [1].
> >
> > [1] https://lore.kernel.org/all/1400188079-21832-1-git-send-email-chirantan@chromium.org/
> 
> That's a pretty deep thread (more so with the duplicate messages, as
> you used the "all" instead of a specific list). It might be good to
> have a bit more of a summary here in the commit message, so folks
> don't have to dig too deeply themselves.

Ah, sorry about the bad link. The above points should be a good summary of that
conversation with regards to this patch.

> 
> > Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
> > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> > [Original commit from https://android.googlesource.com/kernel/gs/+/630817f7080e92c5e0216095ff52f6eb8dd00727
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > ---
> >  drivers/clocksource/exynos_mct.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> > index da09f467a6bb..05c50f2f7a7e 100644
> > --- a/drivers/clocksource/exynos_mct.c
> > +++ b/drivers/clocksource/exynos_mct.c
> > @@ -219,12 +219,12 @@ static struct clocksource mct_frc = {
> >         .resume         = exynos4_frc_resume,
> >  };
> >
> > +#if defined(CONFIG_ARM)
> 
> I'd probably suggest adding a comment here explaining why this is kept
> on ARM and not on AARCH64 as well.

Sure, I can add my comments above here in v2.

> 
> >  static u64 notrace exynos4_read_sched_clock(void)
> >  {
> >         return exynos4_read_count_32();
> >  }
> >
> > -#if defined(CONFIG_ARM)
> >  static struct delay_timer exynos4_delay_timer;
> >
> >  static cycles_t exynos4_read_current_timer(void)
> > @@ -250,12 +250,13 @@ static int __init exynos4_clocksource_init(bool frc_shared)
> >         exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
> >         exynos4_delay_timer.freq = clk_rate;
> >         register_current_timer_delay(&exynos4_delay_timer);
> > +
> > +       sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
> >  #endif
> >
> >         if (clocksource_register_hz(&mct_frc, clk_rate))
> >                 panic("%s: can't register clocksource\n", mct_frc.name);
> >
> > -       sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
> >
> >         return 0;
> 
> Otherwise, this looks ok to me.
> 
> thanks
> -john

Thanks for taking the time to review!

Regards,
Will

Re: [PATCH v1 2/6] clocksource/drivers/exynos_mct: Don't register as a sched_clock on arm64

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 4560 bytes --]

Hi Will.

I'm really glad to see our work on Pixel being upstreamed.

On Tue, Apr 01, 2025 at 09:50:31AM -0700, William McVicker wrote:
> On 03/31/2025, John Stultz wrote:
> > On Mon, Mar 31, 2025 at 4:00 PM 'Will McVicker' via kernel-team
> > <kernel-team@android.com> wrote:
> > >
> > > When using the Exynos MCT as a sched_clock, accessing the timer value
> > > via the MCT register is extremely slow. To improve performance on Arm64
> > > SoCs, use the Arm architected timer instead for timekeeping.
> > 
> > This probably needs some further expansion to explain why we don't
> > want to use it for sched_clock but continue to register the MCT as a
> > clocksource (ie: why not disable MCT entirely?).
> 
> Using the MCT as a sched_clock was originally added for Exynos4 SoCs to improve
> the gettimeofday() syscalls on ChromeOS. For ARM32 this is the best they can do
> without the Arm architected timer. ChromeOS perf data can be found in [1,2]
> 
> [1] https://lore.kernel.org/linux-samsung-soc/CAJFHJrrgWGc4XGQB0ysLufAg3Wouz-aYXu97Sy2Kp=HzK+akVQ@mail.gmail.com/
> [2] https://lore.kernel.org/linux-samsung-soc/CAASgrz2Nr69tpfC8ka9gbs2OvjLEGsvgAj4vBCFxhsamuFum7w@mail.gmail.com/
> 
> I think it's valid to still register the MCT as a clocksource to make it
> available in case someone decides they want to use it, but by default it
> doesn't make sense to use it as the default clocksource on Exynos-based ARM64
> systems with arch_timer support. However, we can't disable the Exynos MCT
> entirely on ARM64 because we need it as the wakeup source for the arch_timer to
> support waking up from the "c2" idle state, which is discussed in [3].
> 
> [3] https://lore.kernel.org/linux-arm-kernel/20210608154341.10794-1-will@kernel.org/
> 

Exactly right.

> > 
> > > Note, ARM32 SoCs don't have an architectured timer and therefore
> > > will continue to use the MCT timer. Detailed discussion on this topic
> > > can be found at [1].
> > >
> > > [1] https://lore.kernel.org/all/1400188079-21832-1-git-send-email-chirantan@chromium.org/
> > 
> > That's a pretty deep thread (more so with the duplicate messages, as
> > you used the "all" instead of a specific list). It might be good to
> > have a bit more of a summary here in the commit message, so folks
> > don't have to dig too deeply themselves.
> 
> Ah, sorry about the bad link. The above points should be a good summary of that
> conversation with regards to this patch.
> 
> > 
> > > Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
> > > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> > > [Original commit from https://android.googlesource.com/kernel/gs/+/630817f7080e92c5e0216095ff52f6eb8dd00727
> > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > > ---
> > >  drivers/clocksource/exynos_mct.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> > > index da09f467a6bb..05c50f2f7a7e 100644
> > > --- a/drivers/clocksource/exynos_mct.c
> > > +++ b/drivers/clocksource/exynos_mct.c
> > > @@ -219,12 +219,12 @@ static struct clocksource mct_frc = {
> > >         .resume         = exynos4_frc_resume,
> > >  };
> > >
> > > +#if defined(CONFIG_ARM)
> > 
> > I'd probably suggest adding a comment here explaining why this is kept
> > on ARM and not on AARCH64 as well.
> 
> Sure, I can add my comments above here in v2.
> 
> > 
> > >  static u64 notrace exynos4_read_sched_clock(void)
> > >  {
> > >         return exynos4_read_count_32();
> > >  }
> > >
> > > -#if defined(CONFIG_ARM)
> > >  static struct delay_timer exynos4_delay_timer;
> > >
> > >  static cycles_t exynos4_read_current_timer(void)
> > > @@ -250,12 +250,13 @@ static int __init exynos4_clocksource_init(bool frc_shared)
> > >         exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
> > >         exynos4_delay_timer.freq = clk_rate;
> > >         register_current_timer_delay(&exynos4_delay_timer);
> > > +
> > > +       sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
> > >  #endif
> > >
> > >         if (clocksource_register_hz(&mct_frc, clk_rate))
> > >                 panic("%s: can't register clocksource\n", mct_frc.name);
> > >
> > > -       sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
> > >
> > >         return 0;
> > 
> > Otherwise, this looks ok to me.
> > 
> > thanks
> > -john
> 
> Thanks for taking the time to review!
> 
> Regards,
> Will
> 

Along with John's comment,
Reviewed-by:: Youngmin Nam <youngmin.nam@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v1 3/6] clocksource/drivers/exynos_mct: Set local timer interrupts as percpu

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]

On Tue, Apr 01, 2025 at 09:36:19AM -0700, William McVicker wrote:
> On 03/31/2025, John Stultz wrote:
> > On Mon, Mar 31, 2025 at 4:00 PM 'Will McVicker' via kernel-team
> > <kernel-team@android.com> wrote:
> > >
> > > From: Hosung Kim <hosung0.kim@samsung.com>
> > >
> > > The MCT local timers can be used as a per-cpu event timer. To prevent
> > 
> > Can be used, or are used?  If it's an option, is this change important
> > in both cases?
> > 
> > > the timer interrupts from migrating to other CPUs, set the flag
> > > IRQF_PERCPU.
> > 
> > Might be work expanding this a bit to clarify why the interrupts
> > migrating to other cpus is undesired.
> 
> Let me dig into this further to figure out if the IP has a limitation where the
> interrupts need to be handled by the CPU the timer was triggered on or if this
> is just an optimization.
> 
> Any chance you know this @Youngmin?
> 
> Thanks,
> Will
> 

Hi Will.

Yes. In downstream, we’ve been using MCT as the clock event timer instead of the ARM timer.
Setting this flag allows each CPU to handle its own clock events, such as scheduling interrupts.

> > 
> > > Signed-off-by: Hosung Kim <hosung0.kim@samsung.com>
> > > [Original commit from https://android.googlesource.com/kernel/gs/+/03267fad19f093bac979ca78309483e9eb3a8d16]
> > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > 
> > thanks!
> > -john
> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v1 4/6] arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]

On Mon, Mar 31, 2025 at 04:00:26PM -0700, Will McVicker wrote:
> From: Will Deacon <willdeacon@google.com>
> 
> In preparation for switching to the architected timer as the primary
> clockevents device, mark the cpuidle nodes with the 'local-timer-stop'
> property to indicate that an alternative clockevents device must be
> used for waking up from the "c2" idle state.
> 
> Signed-off-by: Will Deacon <willdeacon@google.com>
> [Original commit from https://android.googlesource.com/kernel/gs/+/a896fd98638047989513d05556faebd28a62b27c]
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---
>  arch/arm64/boot/dts/exynos/google/gs101.dtsi | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> index 3de3a758f113..fd0badf24e6f 100644
> --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> @@ -155,6 +155,7 @@ ananke_cpu_sleep: cpu-ananke-sleep {
>  				idle-state-name = "c2";
>  				compatible = "arm,idle-state";
>  				arm,psci-suspend-param = <0x0010000>;
> +				local-timer-stop;
>  				entry-latency-us = <70>;
>  				exit-latency-us = <160>;
>  				min-residency-us = <2000>;
> @@ -164,6 +165,7 @@ enyo_cpu_sleep: cpu-enyo-sleep {
>  				idle-state-name = "c2";
>  				compatible = "arm,idle-state";
>  				arm,psci-suspend-param = <0x0010000>;
> +				local-timer-stop;
>  				entry-latency-us = <150>;
>  				exit-latency-us = <190>;
>  				min-residency-us = <2500>;
> @@ -173,6 +175,7 @@ hera_cpu_sleep: cpu-hera-sleep {
>  				idle-state-name = "c2";
>  				compatible = "arm,idle-state";
>  				arm,psci-suspend-param = <0x0010000>;
> +				local-timer-stop;
>  				entry-latency-us = <235>;
>  				exit-latency-us = <220>;
>  				min-residency-us = <3500>;
> -- 
> 2.49.0.472.ge94155a9ec-goog
> 
Hi Will.

Are you using this property in production?
If so, have you noticed any performance improvements?

Thanks.
Youngmin

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v1 5/6] clocksource/drivers/exynos_mct: Add module support

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

On Tue, Apr 01, 2025 at 09:27:09AM -0700, William McVicker wrote:
> On 04/01/2025, Krzysztof Kozlowski wrote:
> > On 01/04/2025 01:00, Will McVicker wrote:
> > > From: Donghoon Yu <hoony.yu@samsung.com>
> > > 
> > > On Arm64 platforms the Exynos MCT driver can be built as a module. On
> > > boot (and even after boot) the arch_timer is used as the clocksource and
> > > tick timer. Once the MCT driver is loaded, it can be used as the wakeup
> > > source for the arch_timer.
> > > 
> > > Signed-off-by: Donghoon Yu <hoony.yu@samsung.com>
> > > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> > > [Original commit from https://android.googlesource.com/kernel/gs/+/8a52a8288ec7d88ff78f0b37480dbb0e9c65bbfd]
> > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > > ---
> > >  drivers/clocksource/Kconfig      |  3 +-
> > >  drivers/clocksource/exynos_mct.c | 47 +++++++++++++++++++++++++++-----
> > >  2 files changed, 42 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > index 487c85259967..e5d9d8383607 100644
> > > --- a/drivers/clocksource/Kconfig
> > > +++ b/drivers/clocksource/Kconfig
> > > @@ -443,7 +443,8 @@ config ATMEL_TCB_CLKSRC
> > >  	  Support for Timer Counter Blocks on Atmel SoCs.
> > >  
> > >  config CLKSRC_EXYNOS_MCT
> > > -	bool "Exynos multi core timer driver" if COMPILE_TEST
> > > +	tristate "Exynos multi core timer driver"
> > > +	default y if ARCH_EXYNOS
> > >  	depends on ARM || ARM64
> > >  	depends on ARCH_ARTPEC || ARCH_EXYNOS || COMPILE_TEST
> > I am not sure if you actually tested it as module. On arm I cannot build
> > it even:
> > 
> > ERROR: modpost: "register_current_timer_delay"
> > [drivers/clocksource/exynos_mct.ko] undefined!
> > ERROR: modpost: "sched_clock_register"
> > [drivers/clocksource/exynos_mct.ko] undefined!
> 
> I tested with the gs101 ARM64 configuration. You're right it won't work with
> ARM32. Thanks for catching this! Since ARM32 architectures don't have the
> arch_timer, I'm not sure if we can actually support Exynos MCT as a module as
> you wouldn't have any available clocksource during boot. I'll update the
> Kconfig for v2 to handle this and make sure it works for ARM32. I'm guessing
> it'll work with something like:
> 
> config CLKSRC_EXYNOS_MCT
> 	tristate "Exynos multi core timer driver" if ARM64
> 
> 
> Regards,
> Will
> 
> [...]
> 

Thanks for working on upstreaming the MCT driver modularization.
I'll take another look at your v2 patch

Thanks.
Youngmin

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v1 4/6] arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes

[William McVicker]

Hi Youngmin,

On 04/02/2025, Youngmin Nam wrote:
> On Mon, Mar 31, 2025 at 04:00:26PM -0700, Will McVicker wrote:
> > From: Will Deacon <willdeacon@google.com>
> > 
> > In preparation for switching to the architected timer as the primary
> > clockevents device, mark the cpuidle nodes with the 'local-timer-stop'
> > property to indicate that an alternative clockevents device must be
> > used for waking up from the "c2" idle state.
> > 
> > Signed-off-by: Will Deacon <willdeacon@google.com>
> > [Original commit from https://android.googlesource.com/kernel/gs/+/a896fd98638047989513d05556faebd28a62b27c]
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > ---
> >  arch/arm64/boot/dts/exynos/google/gs101.dtsi | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > index 3de3a758f113..fd0badf24e6f 100644
> > --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > @@ -155,6 +155,7 @@ ananke_cpu_sleep: cpu-ananke-sleep {
> >  				idle-state-name = "c2";
> >  				compatible = "arm,idle-state";
> >  				arm,psci-suspend-param = <0x0010000>;
> > +				local-timer-stop;
> >  				entry-latency-us = <70>;
> >  				exit-latency-us = <160>;
> >  				min-residency-us = <2000>;
> > @@ -164,6 +165,7 @@ enyo_cpu_sleep: cpu-enyo-sleep {
> >  				idle-state-name = "c2";
> >  				compatible = "arm,idle-state";
> >  				arm,psci-suspend-param = <0x0010000>;
> > +				local-timer-stop;
> >  				entry-latency-us = <150>;
> >  				exit-latency-us = <190>;
> >  				min-residency-us = <2500>;
> > @@ -173,6 +175,7 @@ hera_cpu_sleep: cpu-hera-sleep {
> >  				idle-state-name = "c2";
> >  				compatible = "arm,idle-state";
> >  				arm,psci-suspend-param = <0x0010000>;
> > +				local-timer-stop;
> >  				entry-latency-us = <235>;
> >  				exit-latency-us = <220>;
> >  				min-residency-us = <3500>;
> > -- 
> > 2.49.0.472.ge94155a9ec-goog
> > 
> Hi Will.
> 
> Are you using this property in production?
> If so, have you noticed any performance improvements?

On Pixel 6, I have only recently switched to using the arch_timer as the
default clocksource. I haven't noticed any major perf improvements to the main
benchmarks, but also haven't seen any regressions. Based on the ChromeOS perf
analysis in [1,2], there was a significant perf difference found.

[1] https://lore.kernel.org/linux-samsung-soc/CAJFHJrrgWGc4XGQB0ysLufAg3Wouz-aYXu97Sy2Kp=HzK+akVQ@mail.gmail.com/
[2] https://lore.kernel.org/linux-samsung-soc/CAASgrz2Nr69tpfC8ka9gbs2OvjLEGsvgAj4vBCFxhsamuFum7w@mail.gmail.com/

If it helps, I found that Pixel 8 and 9 devices (didn't check Pixel 7)
are already using the arch_timer with this 'local-timer-stop' as the default
clocksource in the production kernel.

Thanks,
Will

[...]

Re: [PATCH v1 3/6] clocksource/drivers/exynos_mct: Set local timer interrupts as percpu

[William McVicker]

On 04/02/2025, Youngmin Nam wrote:
> On Tue, Apr 01, 2025 at 09:36:19AM -0700, William McVicker wrote:
> > On 03/31/2025, John Stultz wrote:
> > > On Mon, Mar 31, 2025 at 4:00 PM 'Will McVicker' via kernel-team
> > > <kernel-team@android.com> wrote:
> > > >
> > > > From: Hosung Kim <hosung0.kim@samsung.com>
> > > >
> > > > The MCT local timers can be used as a per-cpu event timer. To prevent
> > > 
> > > Can be used, or are used?  If it's an option, is this change important
> > > in both cases?
> > > 
> > > > the timer interrupts from migrating to other CPUs, set the flag
> > > > IRQF_PERCPU.
> > > 
> > > Might be work expanding this a bit to clarify why the interrupts
> > > migrating to other cpus is undesired.
> > 
> > Let me dig into this further to figure out if the IP has a limitation where the
> > interrupts need to be handled by the CPU the timer was triggered on or if this
> > is just an optimization.
> > 
> > Any chance you know this @Youngmin?
> > 
> > Thanks,
> > Will
> > 
> 
> Hi Will.
> 
> Yes. In downstream, we’ve been using MCT as the clock event timer instead of the ARM timer.
> Setting this flag allows each CPU to handle its own clock events, such as scheduling interrupts.

Thanks for the explanation! I'll integrate this into the commit text.

Regards,
Will

> 
> > > 
> > > > Signed-off-by: Hosung Kim <hosung0.kim@samsung.com>
> > > > [Original commit from https://android.googlesource.com/kernel/gs/+/03267fad19f093bac979ca78309483e9eb3a8d16]
> > > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > > 
> > > thanks!
> > > -john
> > 
> > 

Re: [PATCH v1 4/6] arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 3397 bytes --]

On Wed, Apr 02, 2025 at 02:59:31PM -0700, William McVicker wrote:
> Hi Youngmin,
> 
> On 04/02/2025, Youngmin Nam wrote:
> > On Mon, Mar 31, 2025 at 04:00:26PM -0700, Will McVicker wrote:
> > > From: Will Deacon <willdeacon@google.com>
> > > 
> > > In preparation for switching to the architected timer as the primary
> > > clockevents device, mark the cpuidle nodes with the 'local-timer-stop'
> > > property to indicate that an alternative clockevents device must be
> > > used for waking up from the "c2" idle state.
> > > 
> > > Signed-off-by: Will Deacon <willdeacon@google.com>
> > > [Original commit from https://android.googlesource.com/kernel/gs/+/a896fd98638047989513d05556faebd28a62b27c]
> > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > > ---
> > >  arch/arm64/boot/dts/exynos/google/gs101.dtsi | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > > index 3de3a758f113..fd0badf24e6f 100644
> > > --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > > +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > > @@ -155,6 +155,7 @@ ananke_cpu_sleep: cpu-ananke-sleep {
> > >  				idle-state-name = "c2";
> > >  				compatible = "arm,idle-state";
> > >  				arm,psci-suspend-param = <0x0010000>;
> > > +				local-timer-stop;
> > >  				entry-latency-us = <70>;
> > >  				exit-latency-us = <160>;
> > >  				min-residency-us = <2000>;
> > > @@ -164,6 +165,7 @@ enyo_cpu_sleep: cpu-enyo-sleep {
> > >  				idle-state-name = "c2";
> > >  				compatible = "arm,idle-state";
> > >  				arm,psci-suspend-param = <0x0010000>;
> > > +				local-timer-stop;
> > >  				entry-latency-us = <150>;
> > >  				exit-latency-us = <190>;
> > >  				min-residency-us = <2500>;
> > > @@ -173,6 +175,7 @@ hera_cpu_sleep: cpu-hera-sleep {
> > >  				idle-state-name = "c2";
> > >  				compatible = "arm,idle-state";
> > >  				arm,psci-suspend-param = <0x0010000>;
> > > +				local-timer-stop;
> > >  				entry-latency-us = <235>;
> > >  				exit-latency-us = <220>;
> > >  				min-residency-us = <3500>;
> > > -- 
> > > 2.49.0.472.ge94155a9ec-goog
> > > 
> > Hi Will.
> > 
> > Are you using this property in production?
> > If so, have you noticed any performance improvements?
> 
> On Pixel 6, I have only recently switched to using the arch_timer as the
> default clocksource. I haven't noticed any major perf improvements to the main
> benchmarks, but also haven't seen any regressions. Based on the ChromeOS perf
> analysis in [1,2], there was a significant perf difference found.
> 
> [1] https://lore.kernel.org/linux-samsung-soc/CAJFHJrrgWGc4XGQB0ysLufAg3Wouz-aYXu97Sy2Kp=HzK+akVQ@mail.gmail.com/
> [2] https://lore.kernel.org/linux-samsung-soc/CAASgrz2Nr69tpfC8ka9gbs2OvjLEGsvgAj4vBCFxhsamuFum7w@mail.gmail.com/
> 
> If it helps, I found that Pixel 8 and 9 devices (didn't check Pixel 7)
> are already using the arch_timer with this 'local-timer-stop' as the default
> clocksource in the production kernel.
> 
> Thanks,
> Will
> 
> [...]
> 

Hi Will,

Thanks for sharing the status of Pixel devices.

I agree that using the arch_timer as a clock source device brings significant benefits.
The links you shared are definitely related to that.

However, I would also like to know whether arch_timer is used as a clock event device in Pixel production.

Thanks,
Youngmin

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v1 4/6] arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes

[William McVicker]

On 04/03/2025, Youngmin Nam wrote:
> On Wed, Apr 02, 2025 at 02:59:31PM -0700, William McVicker wrote:
> > Hi Youngmin,
> > 
> > On 04/02/2025, Youngmin Nam wrote:
> > > On Mon, Mar 31, 2025 at 04:00:26PM -0700, Will McVicker wrote:
> > > > From: Will Deacon <willdeacon@google.com>
> > > > 
> > > > In preparation for switching to the architected timer as the primary
> > > > clockevents device, mark the cpuidle nodes with the 'local-timer-stop'
> > > > property to indicate that an alternative clockevents device must be
> > > > used for waking up from the "c2" idle state.
> > > > 
> > > > Signed-off-by: Will Deacon <willdeacon@google.com>
> > > > [Original commit from https://android.googlesource.com/kernel/gs/+/a896fd98638047989513d05556faebd28a62b27c]
> > > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > > > ---
> > > >  arch/arm64/boot/dts/exynos/google/gs101.dtsi | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > > > index 3de3a758f113..fd0badf24e6f 100644
> > > > --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > > > +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > > > @@ -155,6 +155,7 @@ ananke_cpu_sleep: cpu-ananke-sleep {
> > > >  				idle-state-name = "c2";
> > > >  				compatible = "arm,idle-state";
> > > >  				arm,psci-suspend-param = <0x0010000>;
> > > > +				local-timer-stop;
> > > >  				entry-latency-us = <70>;
> > > >  				exit-latency-us = <160>;
> > > >  				min-residency-us = <2000>;
> > > > @@ -164,6 +165,7 @@ enyo_cpu_sleep: cpu-enyo-sleep {
> > > >  				idle-state-name = "c2";
> > > >  				compatible = "arm,idle-state";
> > > >  				arm,psci-suspend-param = <0x0010000>;
> > > > +				local-timer-stop;
> > > >  				entry-latency-us = <150>;
> > > >  				exit-latency-us = <190>;
> > > >  				min-residency-us = <2500>;
> > > > @@ -173,6 +175,7 @@ hera_cpu_sleep: cpu-hera-sleep {
> > > >  				idle-state-name = "c2";
> > > >  				compatible = "arm,idle-state";
> > > >  				arm,psci-suspend-param = <0x0010000>;
> > > > +				local-timer-stop;
> > > >  				entry-latency-us = <235>;
> > > >  				exit-latency-us = <220>;
> > > >  				min-residency-us = <3500>;
> > > > -- 
> > > > 2.49.0.472.ge94155a9ec-goog
> > > > 
> > > Hi Will.
> > > 
> > > Are you using this property in production?
> > > If so, have you noticed any performance improvements?
> > 
> > On Pixel 6, I have only recently switched to using the arch_timer as the
> > default clocksource. I haven't noticed any major perf improvements to the main
> > benchmarks, but also haven't seen any regressions. Based on the ChromeOS perf
> > analysis in [1,2], there was a significant perf difference found.
> > 
> > [1] https://lore.kernel.org/linux-samsung-soc/CAJFHJrrgWGc4XGQB0ysLufAg3Wouz-aYXu97Sy2Kp=HzK+akVQ@mail.gmail.com/
> > [2] https://lore.kernel.org/linux-samsung-soc/CAASgrz2Nr69tpfC8ka9gbs2OvjLEGsvgAj4vBCFxhsamuFum7w@mail.gmail.com/
> > 
> > If it helps, I found that Pixel 8 and 9 devices (didn't check Pixel 7)
> > are already using the arch_timer with this 'local-timer-stop' as the default
> > clocksource in the production kernel.
> > 
> > Thanks,
> > Will
> > 
> > [...]
> > 
> 
> Hi Will,
> 
> Thanks for sharing the status of Pixel devices.
> 
> I agree that using the arch_timer as a clock source device brings significant benefits.
> The links you shared are definitely related to that.
> 
> However, I would also like to know whether arch_timer is used as a clock event device in Pixel production.

For Pixel 8 and 9, the arch_timer is used as both the clocksource and
clockevent device which is what my series is proposing for Pixel 6 upstream.
The MCT device is solely being used as the alternative clockevents device for
waking up from the "c2" state. The reason for using the arch_timer as the
clockevents device is because we were seeing hrtimer stability issues where
a 10ms interval timer would delay about 300ms-1s before starting the callback.
This resulted in several media-related latency issues.

Thanks,
Will

[...]

Re: [PATCH v1 4/6] arm64: dts: exynos: gs101: Add 'local-timer-stop' to cpuidle nodes

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 4461 bytes --]

On Thu, Apr 03, 2025 at 11:39:23AM -0700, William McVicker wrote:
> On 04/03/2025, Youngmin Nam wrote:
> > On Wed, Apr 02, 2025 at 02:59:31PM -0700, William McVicker wrote:
> > > Hi Youngmin,
> > > 
> > > On 04/02/2025, Youngmin Nam wrote:
> > > > On Mon, Mar 31, 2025 at 04:00:26PM -0700, Will McVicker wrote:
> > > > > From: Will Deacon <willdeacon@google.com>
> > > > > 
> > > > > In preparation for switching to the architected timer as the primary
> > > > > clockevents device, mark the cpuidle nodes with the 'local-timer-stop'
> > > > > property to indicate that an alternative clockevents device must be
> > > > > used for waking up from the "c2" idle state.
> > > > > 
> > > > > Signed-off-by: Will Deacon <willdeacon@google.com>
> > > > > [Original commit from https://android.googlesource.com/kernel/gs/+/a896fd98638047989513d05556faebd28a62b27c]
> > > > > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > > > > ---
> > > > >  arch/arm64/boot/dts/exynos/google/gs101.dtsi | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm64/boot/dts/exynos/google/gs101.dtsi b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > > > > index 3de3a758f113..fd0badf24e6f 100644
> > > > > --- a/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > > > > +++ b/arch/arm64/boot/dts/exynos/google/gs101.dtsi
> > > > > @@ -155,6 +155,7 @@ ananke_cpu_sleep: cpu-ananke-sleep {
> > > > >  				idle-state-name = "c2";
> > > > >  				compatible = "arm,idle-state";
> > > > >  				arm,psci-suspend-param = <0x0010000>;
> > > > > +				local-timer-stop;
> > > > >  				entry-latency-us = <70>;
> > > > >  				exit-latency-us = <160>;
> > > > >  				min-residency-us = <2000>;
> > > > > @@ -164,6 +165,7 @@ enyo_cpu_sleep: cpu-enyo-sleep {
> > > > >  				idle-state-name = "c2";
> > > > >  				compatible = "arm,idle-state";
> > > > >  				arm,psci-suspend-param = <0x0010000>;
> > > > > +				local-timer-stop;
> > > > >  				entry-latency-us = <150>;
> > > > >  				exit-latency-us = <190>;
> > > > >  				min-residency-us = <2500>;
> > > > > @@ -173,6 +175,7 @@ hera_cpu_sleep: cpu-hera-sleep {
> > > > >  				idle-state-name = "c2";
> > > > >  				compatible = "arm,idle-state";
> > > > >  				arm,psci-suspend-param = <0x0010000>;
> > > > > +				local-timer-stop;
> > > > >  				entry-latency-us = <235>;
> > > > >  				exit-latency-us = <220>;
> > > > >  				min-residency-us = <3500>;
> > > > > -- 
> > > > > 2.49.0.472.ge94155a9ec-goog
> > > > > 
> > > > Hi Will.
> > > > 
> > > > Are you using this property in production?
> > > > If so, have you noticed any performance improvements?
> > > 
> > > On Pixel 6, I have only recently switched to using the arch_timer as the
> > > default clocksource. I haven't noticed any major perf improvements to the main
> > > benchmarks, but also haven't seen any regressions. Based on the ChromeOS perf
> > > analysis in [1,2], there was a significant perf difference found.
> > > 
> > > [1] https://lore.kernel.org/linux-samsung-soc/CAJFHJrrgWGc4XGQB0ysLufAg3Wouz-aYXu97Sy2Kp=HzK+akVQ@mail.gmail.com/
> > > [2] https://lore.kernel.org/linux-samsung-soc/CAASgrz2Nr69tpfC8ka9gbs2OvjLEGsvgAj4vBCFxhsamuFum7w@mail.gmail.com/
> > > 
> > > If it helps, I found that Pixel 8 and 9 devices (didn't check Pixel 7)
> > > are already using the arch_timer with this 'local-timer-stop' as the default
> > > clocksource in the production kernel.
> > > 
> > > Thanks,
> > > Will
> > > 
> > > [...]
> > > 
> > 
> > Hi Will,
> > 
> > Thanks for sharing the status of Pixel devices.
> > 
> > I agree that using the arch_timer as a clock source device brings significant benefits.
> > The links you shared are definitely related to that.
> > 
> > However, I would also like to know whether arch_timer is used as a clock event device in Pixel production.
> 
> For Pixel 8 and 9, the arch_timer is used as both the clocksource and
> clockevent device which is what my series is proposing for Pixel 6 upstream.
> The MCT device is solely being used as the alternative clockevents device for
> waking up from the "c2" state. The reason for using the arch_timer as the
> clockevents device is because we were seeing hrtimer stability issues where
> a 10ms interval timer would delay about 300ms-1s before starting the callback.
> This resulted in several media-related latency issues.
> 
> Thanks,
> Will
> 
> [...]
> 

Thank you for sharing your valuable experience. That will be helpful to us.

Thanks,
Youngmin

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]