[PATCH v2 0/7] Add module support for Arm64 Exynos MCT driver

[PATCH v2 0/7] 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 is used as the wakeup source for the arch_timer to ensure the device
can wakeup from the "c2" idle state.

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

Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Donghoon Yu <hoony.yu@samsung.com>
Cc: Hosung Kim <hosung0.kim@samsung.com>
Cc: kernel-team@android.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: Rob Herring <robh@kernel.org>
Cc: Saravana Kannan <saravanak@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Youngmin Nam <youngmin.nam@samsung.com>
Cc: Peter Griffin <peter.griffin@linaro.org>
Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
Cc: André Draszik <andre.draszik@linaro.org>
Cc: Will Deacon <will@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: linux-samsung-soc@vger.kernel.org

---
Changes in v2:
- Re-worked patch v1 5 based on Rob Herring's review to use the compatible data
  for retrieving the mct_init function pointer.
- Updated the Kconfig logic to disallow building the Exynos MCT driver as
  a module for ARM32 configurations based on Krzysztof Kozlowski's findings.
- Added comments and clarified commit messages in patches 1 and 2 based on
  reviews from John Stultz and Youngmin Nam.
- Fixed an issue found during testing that resulted in the device getting
  stuck on boot. This is included in v2 as patch 5.
- Collected *-by tags
- Rebased to the latest linux-next/master.

---
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 (4):
  of/irq: Export of_irq_count for modules
  clocksource/drivers/exynos_mct: Don't register as a sched_clock on
    arm64
  clocksource/drivers/exynos_mct: Fix uninitialized irq name warning
  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             | 73 ++++++++++++++++----
 drivers/of/irq.c                             |  1 +
 5 files changed, 67 insertions(+), 14 deletions(-)

-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v2 1/7] 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>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
---
 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 v2 2/7] clocksource/drivers/exynos_mct: Don't register as a sched_clock on arm64

[Will McVicker]

To use the MCT as a sched_clock, the timer value has to be accessed vi
an MCT register which is extremely slow. To improve performance on Arm64
SoCs, use the Arm architected timer as the default clocksource. Note, we
can't completely disable the MCT on Arm64 since it needs to be used as
the wakeup source for the arch_timer to exit the "c2" idle state.

Since ARM SoCs don't have an architectured timer, the MCT will continue
to be the default clocksource. Detailed discussion on this topic can be
found at [1].

[1] https://lore.kernel.org/linux-samsung-soc/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>
Reviewed-by: Youngmin Nam <youngmin.nam@samsung.com>
---
 drivers/clocksource/exynos_mct.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index da09f467a6bb..96361d5dc57d 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -219,12 +219,18 @@ static struct clocksource mct_frc = {
 	.resume		= exynos4_frc_resume,
 };
 
+/*
+ * Since ARM devices do not have an architected timer, they need to continue
+ * using the MCT as the main clocksource for timekeeping, sched_clock, and the
+ * delay timer. For AARCH64 SoCs, the architected timer is the preferred
+ * clocksource due to it's superior performance.
+ */
+#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 +256,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 v2 3/7] clocksource/drivers/exynos_mct: Set local timer interrupts as percpu

[Will McVicker]

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

To allow the CPU to handle it's own clock events, we need to set the
IRQF_PERCPU flag. This prevents the local timer interrupts from
migrating to other CPUs.

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 96361d5dc57d..a5ef7d64b1c2 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -596,7 +596,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 v2 4/7] 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 v2 5/7] clocksource/drivers/exynos_mct: Fix uninitialized irq name warning

[Will McVicker]

The Exynos MCT driver doesn't set the clocksource name until the CPU
hotplug state is setup which happens after the IRQs are requested. This
results in an empty IRQ name which leads to the below warning at
proc_create() time. When this happens, the userdata partition fails to
mount and the device gets stuck in an endless loop printing the error:

  root '/dev/disk/by-partlabel/userdata' doesn't exist or does not contain a /dev.

To fix this, we just need to initialize the name before requesting the
IRQs.

Warning from Pixel 6 kernel log:

[  T430] name len 0
[  T430] WARNING: CPU: 6 PID: 430 at fs/proc/generic.c:407 __proc_create+0x258/0x2b4
[  T430] Modules linked in: dwc3_exynos(E+)
[  T430]  ufs_exynos(E+) phy_exynos_ufs(E)
[  T430]  phy_exynos5_usbdrd(E) exynos_usi(E+) exynos_mct(E+) s3c2410_wdt(E)
[  T430]  arm_dsu_pmu(E) simplefb(E)
[  T430] CPU: 6 UID: 0 PID: 430 Comm: (udev-worker) Tainted:
         ... 6.14.0-next-20250331-4k-00008-g59adf909e40e #1 ...
[  T430] Tainted: [W]=WARN, [E]=UNSIGNED_MODULE
[  T430] Hardware name: Raven (DT)
[...]
[  T430] Call trace:
[  T430]  __proc_create+0x258/0x2b4 (P)
[  T430]  proc_mkdir+0x40/0xa0
[  T430]  register_handler_proc+0x118/0x140
[  T430]  __setup_irq+0x460/0x6d0
[  T430]  request_threaded_irq+0xcc/0x1b0
[  T430]  mct_init_dt+0x244/0x604 [exynos_mct ...]
[  T430]  mct_init_spi+0x18/0x34 [exynos_mct ...]
[  T430]  exynos4_mct_probe+0x30/0x4c [exynos_mct ...]
[  T430]  platform_probe+0x6c/0xe4
[  T430]  really_probe+0xf4/0x38c
[...]
[  T430]  driver_register+0x6c/0x140
[  T430]  __platform_driver_register+0x28/0x38
[  T430]  exynos4_mct_driver_init+0x24/0xfe8 [exynos_mct ...]
[  T430]  do_one_initcall+0x84/0x3c0
[  T430]  do_init_module+0x58/0x208
[  T430]  load_module+0x1de0/0x2500
[  T430]  init_module_from_file+0x8c/0xdc

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 drivers/clocksource/exynos_mct.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index a5ef7d64b1c2..62febeb4e1de 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -465,8 +465,6 @@ static int exynos4_mct_starting_cpu(unsigned int cpu)
 		per_cpu_ptr(&percpu_mct_tick, cpu);
 	struct clock_event_device *evt = &mevt->evt;
 
-	snprintf(mevt->name, sizeof(mevt->name), "mct_tick%d", cpu);
-
 	evt->name = mevt->name;
 	evt->cpumask = cpumask_of(cpu);
 	evt->set_next_event = exynos4_tick_set_next_event;
@@ -567,6 +565,14 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
 	for (i = MCT_L0_IRQ; i < nr_irqs; i++)
 		mct_irqs[i] = irq_of_parse_and_map(np, i);
 
+	for_each_possible_cpu(cpu) {
+		struct mct_clock_event_device *mevt =
+		    per_cpu_ptr(&percpu_mct_tick, cpu);
+
+		snprintf(mevt->name, sizeof(mevt->name), "mct_tick%d",
+			 cpu);
+	}
+
 	if (mct_int_type == MCT_INT_PPI) {
 
 		err = request_percpu_irq(mct_irqs[MCT_L0_IRQ],
-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v2 6/7] 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 | 49 +++++++++++++++++++++++++++-----
 2 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 487c85259967..e89373827c3a 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" if ARM64
+	default y if ARCH_EXYNOS || COMPILE_TEST
 	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 62febeb4e1de..8943274378be 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>
 
@@ -241,7 +243,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
@@ -511,7 +513,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;
 
@@ -539,7 +541,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)
@@ -652,7 +654,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};
@@ -700,15 +702,48 @@ 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 *dev = &pdev->dev;
+	int (*mct_init)(struct device_node *np);
+
+	mct_init = of_device_get_match_data(dev);
+	if (!mct_init)
+		return -EINVAL;
+
+	return mct_init(dev->of_node);
+}
+
+static const struct of_device_id exynos4_mct_match_table[] = {
+	{ .compatible = "samsung,exynos4210-mct", .data = &mct_init_spi, },
+	{ .compatible = "samsung,exynos4412-mct", .data = &mct_init_ppi, },
+	{}
+};
+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 v2 7/7] 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 v2 2/7] clocksource/drivers/exynos_mct: Don't register as a sched_clock on arm64

[John Stultz]

On Wed, Apr 2, 2025 at 4:34 PM 'Will McVicker' via kernel-team
<kernel-team@android.com> wrote:
>
> To use the MCT as a sched_clock, the timer value has to be accessed vi
> an MCT register which is extremely slow. To improve performance on Arm64
> SoCs, use the Arm architected timer as the default clocksource. Note, we

Nit:  sched_clock is sort of separate from the "default clocksource",
and after this patch we still register MCT as a clocksource, so this
doesn't sound quite right.

I'd probably reword this slightly to:
"The MCT register is unfortunately very slow to access, but importantly
does not halt in the c2 idle state.  So for ARM64, we can improve
performance by not registering the MCT for sched_clock, allowing the
system to use the faster ARM architected timer for sched_clock instead.

The MCT is still registered as a clocksource, and a clockevent in order
to be a wakeup source for the arch_timer to exit the "c2" idle state.

Since ARM32 SoCs don't have an architected timer, the MCT must continue
to be used for sched_clock. Detailed discussion on this topic can be
found at [1]. "

> can't completely disable the MCT on Arm64 since it needs to be used as
> the wakeup source for the arch_timer to exit the "c2" idle state.
>
> Since ARM SoCs don't have an architectured timer, the MCT will continue
> to be the default clocksource. Detailed discussion on this topic can be
> found at [1].
>
> [1] https://lore.kernel.org/linux-samsung-soc/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>
> Reviewed-by: Youngmin Nam <youngmin.nam@samsung.com>

Otherwise, looks good.
Acked-by: John Stultz <jstultz@google.com>

thanks
-john

Re: [PATCH v2 0/7] Add module support for Arm64 Exynos MCT driver

[Youngmin Nam]

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

On Wed, Apr 02, 2025 at 04:33:51PM -0700, Will McVicker wrote:
> 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 is used as the wakeup source for the arch_timer to ensure the device
> can wakeup from the "c2" idle state.
> 
> 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://protect2.fireeye.com/v1/url?k=d287bb1b-b30cae21-d2863054-74fe4860008a-f0cb7ae29f3b1b85&q=1&e=4e8467a4-13da-4dd4-a8fd-4ddfc38e89b4&u=https%3A%2F%2Fgit.codelinaro.org%2Flinaro%2Fgooglelt%2Fpixelscripts%2F-%2Fblob%2Fclo%2Fmain%2FREADME.md%3Fref_type%3Dheads
> 
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Donghoon Yu <hoony.yu@samsung.com>
> Cc: Hosung Kim <hosung0.kim@samsung.com>
> Cc: kernel-team@android.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Youngmin Nam <youngmin.nam@samsung.com>
> Cc: Peter Griffin <peter.griffin@linaro.org>
> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
> Cc: André Draszik <andre.draszik@linaro.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: linux-samsung-soc@vger.kernel.org
> 
> ---
> Changes in v2:
> - Re-worked patch v1 5 based on Rob Herring's review to use the compatible data
>   for retrieving the mct_init function pointer.
> - Updated the Kconfig logic to disallow building the Exynos MCT driver as
>   a module for ARM32 configurations based on Krzysztof Kozlowski's findings.
> - Added comments and clarified commit messages in patches 1 and 2 based on
>   reviews from John Stultz and Youngmin Nam.
> - Fixed an issue found during testing that resulted in the device getting
>   stuck on boot. This is included in v2 as patch 5.
> - Collected *-by tags
> - Rebased to the latest linux-next/master.
> 
> ---
> 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 (4):
>   of/irq: Export of_irq_count for modules
>   clocksource/drivers/exynos_mct: Don't register as a sched_clock on
>     arm64
>   clocksource/drivers/exynos_mct: Fix uninitialized irq name warning
>   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             | 73 ++++++++++++++++----
>  drivers/of/irq.c                             |  1 +
>  5 files changed, 67 insertions(+), 14 deletions(-)
> 
> -- 
> 2.49.0.472.ge94155a9ec-goog
> 
> 

Hi Will.

I tested this series on a E850-96(Exynos3830 based) board and it's working as a moudle.

# dmesg | grep mct
[7.376224] clocksource: mct-frc: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 73510017198 ns

# lsmod | grep exynos_mct
exynos_mct             12288  0

# cat /sys/devices/system/clocksource/clocksource0/current_clocksource
arch_sys_counter
# cat /sys/devices/system/clockevents/clockevent0/current_device
arch_sys_timer

# cat /proc/interrupts 
        CPU0    CPU1    CPU2    CPU3    CPU4    CPU5    CPU6    CPU7
 12:    2566    2752    2467    4026    3372    2822    2115    3227 GIC-0  27 Level     arch_timer
...
 77:       0       0       0       0       0       0       0       0 GIC-0 235 Level     mct_comp_irq
 78:       0       0       0       0       0       0       0       0 GIC-0 239 Level     mct_tick0
 79:       0       0       0       0       0       0       0       0 GIC-0 240 Level     mct_tick1
 80:       0       0       0       0       0       0       0       0 GIC-0 241 Level     mct_tick2
 81:       0       0       0       0       0       0       0       0 GIC-0 242 Level     mct_tick3
 82:       0       0       0       0       0       0       0       0 GIC-0 243 Level     mct_tick4
 83:       0       0       0       0       0       0       0       0 GIC-0 244 Level     mct_tick5
 84:       0       0       0       0       0       0       0       0 GIC-0 245 Level     mct_tick6
 85:       0       0       0       0       0       0       0       0 GIC-0 246 Level     mct_tick7

Reviewed-by: Youngmin Nam <youngmin.nam@samsung.com>
Tested-by: Youngmin Nam <youngmin.nam@samsung.com>

Thanks,
Youngmin

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

Re: [PATCH v2 0/7] Add module support for Arm64 Exynos MCT driver

[Krzysztof Kozlowski]

On 04/04/2025 03:11, Youngmin Nam wrote:
>>
>> -- 
>> 2.49.0.472.ge94155a9ec-goog
>>
>>
> 
> Hi Will.
> 
> I tested this series on a E850-96(Exynos3830 based) board and it's working as a moudle.

Hi,

On which kernel did you apply these patches for testing?

> 
> # dmesg | grep mct
> [7.376224] clocksource: mct-frc: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 73510017198 ns
> 
> # lsmod | grep exynos_mct
> exynos_mct             12288  0
> 
> # cat /sys/devices/system/clocksource/clocksource0/current_clocksource
> arch_sys_counter
> # cat /sys/devices/system/clockevents/clockevent0/current_device
> arch_sys_timer
> 
> # cat /proc/interrupts 
>         CPU0    CPU1    CPU2    CPU3    CPU4    CPU5    CPU6    CPU7
>  12:    2566    2752    2467    4026    3372    2822    2115    3227 GIC-0  27 Level     arch_timer
> ...
>  77:       0       0       0       0       0       0       0       0 GIC-0 235 Level     mct_comp_irq
>  78:       0       0       0       0       0       0       0       0 GIC-0 239 Level     mct_tick0
>  79:       0       0       0       0       0       0       0       0 GIC-0 240 Level     mct_tick1
>  80:       0       0       0       0       0       0       0       0 GIC-0 241 Level     mct_tick2
>  81:       0       0       0       0       0       0       0       0 GIC-0 242 Level     mct_tick3
>  82:       0       0       0       0       0       0       0       0 GIC-0 243 Level     mct_tick4
>  83:       0       0       0       0       0       0       0       0 GIC-0 244 Level     mct_tick5
>  84:       0       0       0       0       0       0       0       0 GIC-0 245 Level     mct_tick6
>  85:       0       0       0       0       0       0       0       0 GIC-0 246 Level     mct_tick7
> 
> Reviewed-by: Youngmin Nam <youngmin.nam@samsung.com>

This means you reviewed *every* patch.

> Tested-by: Youngmin Nam <youngmin.nam@samsung.com>

Best regards,
Krzysztof

Re: [PATCH v2 0/7] Add module support for Arm64 Exynos MCT driver

[Youngmin Nam]

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

On Fri, Apr 04, 2025 at 08:02:41AM +0200, Krzysztof Kozlowski wrote:
> On 04/04/2025 03:11, Youngmin Nam wrote:
> >>
> >> -- 
> >> 2.49.0.472.ge94155a9ec-goog
> >>
> >>
> > 
> > Hi Will.
> > 
> > I tested this series on a E850-96(Exynos3830 based) board and it's working as a moudle.
> 
> Hi,
> 
> On which kernel did you apply these patches for testing?
> 

Hi,

I tested it on the latest torvalds tree.

# uname -r
6.14.0-12893-g26d035c84315-dirty

> > 
> > # dmesg | grep mct
> > [7.376224] clocksource: mct-frc: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 73510017198 ns
> > 
> > # lsmod | grep exynos_mct
> > exynos_mct             12288  0
> > 
> > # cat /sys/devices/system/clocksource/clocksource0/current_clocksource
> > arch_sys_counter
> > # cat /sys/devices/system/clockevents/clockevent0/current_device
> > arch_sys_timer
> > 
> > # cat /proc/interrupts 
> >         CPU0    CPU1    CPU2    CPU3    CPU4    CPU5    CPU6    CPU7
> >  12:    2566    2752    2467    4026    3372    2822    2115    3227 GIC-0  27 Level     arch_timer
> > ...
> >  77:       0       0       0       0       0       0       0       0 GIC-0 235 Level     mct_comp_irq
> >  78:       0       0       0       0       0       0       0       0 GIC-0 239 Level     mct_tick0
> >  79:       0       0       0       0       0       0       0       0 GIC-0 240 Level     mct_tick1
> >  80:       0       0       0       0       0       0       0       0 GIC-0 241 Level     mct_tick2
> >  81:       0       0       0       0       0       0       0       0 GIC-0 242 Level     mct_tick3
> >  82:       0       0       0       0       0       0       0       0 GIC-0 243 Level     mct_tick4
> >  83:       0       0       0       0       0       0       0       0 GIC-0 244 Level     mct_tick5
> >  84:       0       0       0       0       0       0       0       0 GIC-0 245 Level     mct_tick6
> >  85:       0       0       0       0       0       0       0       0 GIC-0 246 Level     mct_tick7
> > 
> > Reviewed-by: Youngmin Nam <youngmin.nam@samsung.com>
> 
> This means you reviewed *every* patch.
> 

Yes, I did.

> > Tested-by: Youngmin Nam <youngmin.nam@samsung.com>
> 
> Best regards,
> Krzysztof
> 

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

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

[William McVicker]

On 04/02/2025, John Stultz wrote:
> On Wed, Apr 2, 2025 at 4:34 PM 'Will McVicker' via kernel-team
> <kernel-team@android.com> wrote:
> >
> > To use the MCT as a sched_clock, the timer value has to be accessed vi
> > an MCT register which is extremely slow. To improve performance on Arm64
> > SoCs, use the Arm architected timer as the default clocksource. Note, we
> 
> Nit:  sched_clock is sort of separate from the "default clocksource",
> and after this patch we still register MCT as a clocksource, so this
> doesn't sound quite right.
> 
> I'd probably reword this slightly to:
> "The MCT register is unfortunately very slow to access, but importantly
> does not halt in the c2 idle state.  So for ARM64, we can improve
> performance by not registering the MCT for sched_clock, allowing the
> system to use the faster ARM architected timer for sched_clock instead.
> 
> The MCT is still registered as a clocksource, and a clockevent in order
> to be a wakeup source for the arch_timer to exit the "c2" idle state.
> 
> Since ARM32 SoCs don't have an architected timer, the MCT must continue
> to be used for sched_clock. Detailed discussion on this topic can be
> found at [1]. "

Thanks John for the suggestion! I'll give more time for review feedback before
sending the v3 with this update.

Regards,
Will

> 
> > can't completely disable the MCT on Arm64 since it needs to be used as
> > the wakeup source for the arch_timer to exit the "c2" idle state.
> >
> > Since ARM SoCs don't have an architectured timer, the MCT will continue
> > to be the default clocksource. Detailed discussion on this topic can be
> > found at [1].
> >
> > [1] https://lore.kernel.org/linux-samsung-soc/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>
> > Reviewed-by: Youngmin Nam <youngmin.nam@samsung.com>
> 
> Otherwise, looks good.
> Acked-by: John Stultz <jstultz@google.com>
> 
> thanks
> -john

Re: [PATCH v2 0/7] Add module support for Arm64 Exynos MCT driver

[William McVicker]

On 04/04/2025, Youngmin Nam wrote:
> On Wed, Apr 02, 2025 at 04:33:51PM -0700, Will McVicker wrote:
> > 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 is used as the wakeup source for the arch_timer to ensure the device
> > can wakeup from the "c2" idle state.
> > 
> > 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://protect2.fireeye.com/v1/url?k=d287bb1b-b30cae21-d2863054-74fe4860008a-f0cb7ae29f3b1b85&q=1&e=4e8467a4-13da-4dd4-a8fd-4ddfc38e89b4&u=https%3A%2F%2Fgit.codelinaro.org%2Flinaro%2Fgooglelt%2Fpixelscripts%2F-%2Fblob%2Fclo%2Fmain%2FREADME.md%3Fref_type%3Dheads
> > 
> > Cc: Alim Akhtar <alim.akhtar@samsung.com>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Donghoon Yu <hoony.yu@samsung.com>
> > Cc: Hosung Kim <hosung0.kim@samsung.com>
> > Cc: kernel-team@android.com
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Saravana Kannan <saravanak@google.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Youngmin Nam <youngmin.nam@samsung.com>
> > Cc: Peter Griffin <peter.griffin@linaro.org>
> > Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
> > Cc: André Draszik <andre.draszik@linaro.org>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: Krzysztof Kozlowski <krzk@kernel.org>
> > Cc: linux-samsung-soc@vger.kernel.org
> > 
> > ---
> > Changes in v2:
> > - Re-worked patch v1 5 based on Rob Herring's review to use the compatible data
> >   for retrieving the mct_init function pointer.
> > - Updated the Kconfig logic to disallow building the Exynos MCT driver as
> >   a module for ARM32 configurations based on Krzysztof Kozlowski's findings.
> > - Added comments and clarified commit messages in patches 1 and 2 based on
> >   reviews from John Stultz and Youngmin Nam.
> > - Fixed an issue found during testing that resulted in the device getting
> >   stuck on boot. This is included in v2 as patch 5.
> > - Collected *-by tags
> > - Rebased to the latest linux-next/master.
> > 
> > ---
> > 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 (4):
> >   of/irq: Export of_irq_count for modules
> >   clocksource/drivers/exynos_mct: Don't register as a sched_clock on
> >     arm64
> >   clocksource/drivers/exynos_mct: Fix uninitialized irq name warning
> >   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             | 73 ++++++++++++++++----
> >  drivers/of/irq.c                             |  1 +
> >  5 files changed, 67 insertions(+), 14 deletions(-)
> > 
> > -- 
> > 2.49.0.472.ge94155a9ec-goog
> > 
> > 
> 
> Hi Will.
> 
> I tested this series on a E850-96(Exynos3830 based) board and it's working as a moudle.
> 
> # dmesg | grep mct
> [7.376224] clocksource: mct-frc: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 73510017198 ns
> 
> # lsmod | grep exynos_mct
> exynos_mct             12288  0
> 
> # cat /sys/devices/system/clocksource/clocksource0/current_clocksource
> arch_sys_counter
> # cat /sys/devices/system/clockevents/clockevent0/current_device
> arch_sys_timer
> 
> # cat /proc/interrupts 
>         CPU0    CPU1    CPU2    CPU3    CPU4    CPU5    CPU6    CPU7
>  12:    2566    2752    2467    4026    3372    2822    2115    3227 GIC-0  27 Level     arch_timer
> ...
>  77:       0       0       0       0       0       0       0       0 GIC-0 235 Level     mct_comp_irq
>  78:       0       0       0       0       0       0       0       0 GIC-0 239 Level     mct_tick0
>  79:       0       0       0       0       0       0       0       0 GIC-0 240 Level     mct_tick1
>  80:       0       0       0       0       0       0       0       0 GIC-0 241 Level     mct_tick2
>  81:       0       0       0       0       0       0       0       0 GIC-0 242 Level     mct_tick3
>  82:       0       0       0       0       0       0       0       0 GIC-0 243 Level     mct_tick4
>  83:       0       0       0       0       0       0       0       0 GIC-0 244 Level     mct_tick5
>  84:       0       0       0       0       0       0       0       0 GIC-0 245 Level     mct_tick6
>  85:       0       0       0       0       0       0       0       0 GIC-0 246 Level     mct_tick7
> 
> Reviewed-by: Youngmin Nam <youngmin.nam@samsung.com>
> Tested-by: Youngmin Nam <youngmin.nam@samsung.com>
> 
> Thanks,
> Youngmin

Thanks Youngmin for the reviews and testing!

Regards,
Will

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[Daniel Lezcano]

Hi Will,

On Wed, Apr 02, 2025 at 04:33:57PM -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.

From a previous thread where there is no answer:

https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/

I don't feel comfortable with changing the clocksource / clockevent drivers to
a module for the reasons explained in the aforementionned thread.

Before this could be accepted, I really need a strong acked-by from Thomas

Thanks

  -- Daniel

> 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 | 49 +++++++++++++++++++++++++++-----
>  2 files changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 487c85259967..e89373827c3a 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" if ARM64
> +	default y if ARCH_EXYNOS || COMPILE_TEST
>  	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 62febeb4e1de..8943274378be 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>
>  
> @@ -241,7 +243,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
> @@ -511,7 +513,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;
>  
> @@ -539,7 +541,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)
> @@ -652,7 +654,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};
> @@ -700,15 +702,48 @@ 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 *dev = &pdev->dev;
> +	int (*mct_init)(struct device_node *np);
> +
> +	mct_init = of_device_get_match_data(dev);
> +	if (!mct_init)
> +		return -EINVAL;
> +
> +	return mct_init(dev->of_node);
> +}
> +
> +static const struct of_device_id exynos4_mct_match_table[] = {
> +	{ .compatible = "samsung,exynos4210-mct", .data = &mct_init_spi, },
> +	{ .compatible = "samsung,exynos4412-mct", .data = &mct_init_ppi, },
> +	{}
> +};
> +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
> 

-- 

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

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

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[William McVicker]

On 04/15/2025, Daniel Lezcano wrote:
> Hi Will,

Hi Daniel,

> 
> On Wed, Apr 02, 2025 at 04:33:57PM -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.
> 
> From a previous thread where there is no answer:
> 
> https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
> 
> I don't feel comfortable with changing the clocksource / clockevent drivers to
> a module for the reasons explained in the aforementionned thread.
> 
> Before this could be accepted, I really need a strong acked-by from Thomas

Thanks for the response! I'll copy-and-paste your replies from that previous
thread and try to address your concerns.

>   * the GKI approach is to have an update for the 'mainline' kernel and
> let the different SoC vendors deal with their drivers. I'm afraid this
> will prevent driver fixes to be carry on upstream because they will stay
> in the OoT kernels

I can't speak for that specific thread or their intent, but I can speak to this
thread and our intent.

This whole patch series is about upstreaming the downstream changes. So saying
this will prevent others from upstreaming changes is punishing the folks who
are actually trying to upstream changes. I don't think that's a fair way to
handle this.

Also, rejecting this series will not prevent people from upstreaming their
changes, it'll just make it more unlikely because they now have to deal with
upstreaming more changes that were rejected in the past. That's daunting for
someone who doesn't do upstreaming often. I'm telling this from experience
dealing with SoC vendors and asking them to upstream stuff.

With that said, let me try to address some of your technical concerns.

> * the core code may not be prepared for that, so loading / unloading
> the modules with active timers may result into some issues

We had the same concern for irqchip drivers. We can easily disable unloading
for these clocksource modules just like we did for irqchip by making them
permanent modules.

> * it may end up with some interactions with cpuidle at boot time and
> the broadcast timer

If I'm understanding this correctly, no driver is guaranteed to probe at
initialization time regardless of whether it is built-in or a module. Taking
a look at the other clocksource drivers, I found that the following drivers are
all calling `clocksource_register_hz()` and `clockevents_config_and_register()`
at probe time.

  timer-sun5i.c
  sh_tmu.c
  sh_cmt.c
  timer-tegra186.c
  timer-stm32-lp.c (only calls clockevents_config_and_register())

So this concern is unrelated to building these drivers are modules. Please let
me know if I'm missing something here.

>  * the timekeeping may do jump in the past [if and] when switching the
> clocksource

Can you clarify how this relates to modules? IIUC, the clocksource can be
changed anytime by writing to:

 /sys/devices/system/clocksource/clocksource0/current_clocksource

If there's a bug related to timekeeping and changing the clocksource, then that
should be handled separately from the modularization code.

For ARM64 in general, the recommendation is to use the ARM architected timer
which is not a module and is used for scheduling and timekeeping. While the
Exynos MCT driver can functionally be used as the primary clocksource, it's not
recommended due to performance issues. So building the MCT driver as a kernel
module really shouldn't be an issue and has been thoroughly testing on several
generations of Pixel devices which is why we are trying to upstream our
downstream technical debt (so we can directly using the upstream version of the
Exynos MCT driver).

Thanks,
Will

[...]

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[John Stultz]

On Tue, Apr 15, 2025 at 9:50 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Wed, Apr 02, 2025 at 04:33:57PM -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.
>
> From a previous thread where there is no answer:
>
> https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
>
> I don't feel comfortable with changing the clocksource / clockevent drivers to
> a module for the reasons explained in the aforementionned thread.

I wasn't CC'ed on that, but to address a few of your points:

> I have some concerns about this kind of changes:
>
>   * the core code may not be prepared for that, so loading / unloading
> the modules with active timers may result into some issues

That's a fair concern, but permanent modules (which are loaded but not
unloaded) shouldn't suffer this issue. I recognize having modules be
fully unloadable is generally cleaner and preferred, but I also see
the benefit of allowing permanent modules to be one-way loaded so a
generic/distro kernel shared between lots of different platforms
doesn't need to be bloated with drivers that aren't used everywhere.
Obviously any single driver doesn't make a huge difference, but all
the small drivers together does add up.

>  * it may end up with some interactions with cpuidle at boot time and
> the broadcast timer

Do you have more details as to your concerns here? I know there can be
cases of issues if the built in clockevent drivers are problematic and
the working ones don't load until later, you can have races where if
the system goes into idle before the module loads it could stall out
(there was a recent issue with an older iMac TSC halting in idle and
it not reliably getting disqualified before it got stuck in idle).  In
those cases I could imagine folks reasonably arguing for including the
working clock as a built in, but I'm not sure I'd say forcing
everything to be built in is the better approach.

> * the timekeeping may do jump in the past [if and] when switching the
> clocksource

? It shouldn't. We've had tests in kselftest that switch between
clocksources checking for inconsistencies for awhile, so if such a
jump occurred it would be considered a bug.

>  * the GKI approach is to have an update for the 'mainline' kernel and
> let the different SoC vendors deal with their drivers. I'm afraid this
> will prevent driver fixes to be carry on upstream because they will stay
> in the OoT kernels

I'm not sure I understand this point?  Could you expand on it a bit?
While I very much can understand concerns and potential downsides of
the GKI approach, I'm not sure how that applies to the submission
here, as the benefit would apply to classic distro kernels as much as
GKI.

I realize in the time since I started this reply, Will has already
covered much of the above! So apologies for being redundant.  That
said, there are some non-modularization changes in this series that
should be considered even if the modularization logic is a continued
sticking point.
-john

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[Daniel Lezcano]

On Tue, Apr 15, 2025 at 02:25:43PM -0700, William McVicker wrote:
> On 04/15/2025, Daniel Lezcano wrote:
> > Hi Will,
> 
> Hi Daniel,
> 
> > 
> > On Wed, Apr 02, 2025 at 04:33:57PM -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.
> > 
> > From a previous thread where there is no answer:
> > 
> > https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
> > 
> > I don't feel comfortable with changing the clocksource / clockevent drivers to
> > a module for the reasons explained in the aforementionned thread.
> > 
> > Before this could be accepted, I really need a strong acked-by from Thomas
> 
> Thanks for the response! I'll copy-and-paste your replies from that previous
> thread and try to address your concerns.
> 
> >   * the GKI approach is to have an update for the 'mainline' kernel and
> > let the different SoC vendors deal with their drivers. I'm afraid this
> > will prevent driver fixes to be carry on upstream because they will stay
> > in the OoT kernels
> 
> I can't speak for that specific thread or their intent, but I can speak to this
> thread and our intent.
> 
> This whole patch series is about upstreaming the downstream changes. So saying
> this will prevent others from upstreaming changes is punishing the folks who
> are actually trying to upstream changes. I don't think that's a fair way to
> handle this.
> 
> Also, rejecting this series will not prevent people from upstreaming their
> changes, it'll just make it more unlikely because they now have to deal with
> upstreaming more changes that were rejected in the past. That's daunting for
> someone who doesn't do upstreaming often. I'm telling this from experience
> dealing with SoC vendors and asking them to upstream stuff.
> 
> With that said, let me try to address some of your technical concerns.

I won't reject the series based on my opinion. Answering the technical concerns
will prevail.

Why is it needed to convert the timer into a module ?
 
> > * the core code may not be prepared for that, so loading / unloading
> > the modules with active timers may result into some issues
> 
> We had the same concern for irqchip drivers. We can easily disable unloading
> for these clocksource modules just like we did for irqchip by making them
> permanent modules.

In the clockevent / clocksource initialization process, depending on the
platform, some are needed very early and other can be loaded later.

For example, the usual configuration is the architected timers are initialized
very early, then the external timer is loaded a bit later. And when this one is
loaded it does not take over the architected timers. It acts as a "broadcast"
timer to program the next timer event when the current CPU is going to an idle
state where the local timer is stopped.

Other cases are the architected timers are not desired and the 'external' timer
is used in place when it is loaded with a higher rating. Some configuration can
mimic local timers by settting a per CPU timer.

Some platforms could be without the architected timers, so the 'external' timer
is used.

Let's imagine the system started, the timers are running and then we load a
module with a timer replacing the current ones. Does it work well ?

Are we sure, the timer modularization is compatible with all the timer use cases ?
 
> > * it may end up with some interactions with cpuidle at boot time and
> > the broadcast timer
> 
> If I'm understanding this correctly, no driver is guaranteed to probe at
> initialization time regardless of whether it is built-in or a module. Taking
> a look at the other clocksource drivers, I found that the following drivers are
> all calling `clocksource_register_hz()` and `clockevents_config_and_register()`
> at probe time.
> 
>   timer-sun5i.c
>   sh_tmu.c
>   sh_cmt.c
>   timer-tegra186.c
>   timer-stm32-lp.c (only calls clockevents_config_and_register())
> 
> So this concern is unrelated to building these drivers are modules. Please let
> me know if I'm missing something here.

We would have to check each platform individually to answer this question.

The interaction between cpuidle and the timer module is about not having a
broadcast timer when cpuidle initializes and then having it later when the
module is loaded. Did you check the deep idle states are used after loading the
module ?

> >  * the timekeeping may do jump in the past [if and] when switching the
> > clocksource
> 
> Can you clarify how this relates to modules? IIUC, the clocksource can be
> changed anytime by writing to:
> 
>  /sys/devices/system/clocksource/clocksource0/current_clocksource

The clocksource counter is stopped when it is not the current one. So when you
switch it, the new clocksource counter could be different and can make you jump
back in time.
 
> If there's a bug related to timekeeping and changing the clocksource, then that
> should be handled separately from the modularization code.

I disagree :)

The whole point is the time framework may not be totally immune against the
timer modularization. It is about identifying the corner cases where the timer
driver modularization can have an impact and set the scene to support it.
 
> For ARM64 in general, the recommendation is to use the ARM architected timer
> which is not a module and is used for scheduling and timekeeping. While the
> Exynos MCT driver can functionally be used as the primary clocksource, it's not
> recommended due to performance issues. So building the MCT driver as a kernel
> module really shouldn't be an issue and has been thoroughly testing on several
> generations of Pixel devices which is why we are trying to upstream our
> downstream technical debt (so we can directly using the upstream version of the
> Exynos MCT driver).

The discussion is not about only the Exynos MCT but as you are not the first
one asking to convert the timer driver to a module, we should check what could
be the impact on the time framework and the system in general.

Others proposed to convert to module and I asked to investigate the impact.
Nobody came back with a clear answer and there is no feedback from Thomas.


> [...]

-- 

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

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

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[Daniel Lezcano]

On Tue, Apr 15, 2025 at 05:48:41PM -0700, John Stultz wrote:
> On Tue, Apr 15, 2025 at 9:50 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> > On Wed, Apr 02, 2025 at 04:33:57PM -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.
> >
> > From a previous thread where there is no answer:
> >
> > https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
> >
> > I don't feel comfortable with changing the clocksource / clockevent drivers to
> > a module for the reasons explained in the aforementionned thread.
> 
> I wasn't CC'ed on that, but to address a few of your points:
> 
> > I have some concerns about this kind of changes:
> >
> >   * the core code may not be prepared for that, so loading / unloading
> > the modules with active timers may result into some issues
> 
> That's a fair concern, but permanent modules (which are loaded but not
> unloaded) shouldn't suffer this issue. I recognize having modules be
> fully unloadable is generally cleaner and preferred, but I also see
> the benefit of allowing permanent modules to be one-way loaded so a
> generic/distro kernel shared between lots of different platforms
> doesn't need to be bloated with drivers that aren't used everywhere.
> Obviously any single driver doesn't make a huge difference, but all
> the small drivers together does add up.

Yes, I agree.

So the whole clockevent / clocksource drivers policy would have to be making
impossible to unload a module once it is loaded.

Do you have any ideas how to ensure that the converted drivers follow this
rule without putting more burden on the maintainer?

> >  * it may end up with some interactions with cpuidle at boot time and
> > the broadcast timer
> 
> Do you have more details as to your concerns here? I know there can be
> cases of issues if the built in clockevent drivers are problematic and
> the working ones don't load until later, you can have races where if
> the system goes into idle before the module loads it could stall out
> (there was a recent issue with an older iMac TSC halting in idle and
> it not reliably getting disqualified before it got stuck in idle). 

Yes, that is that kind of issue I suspect.

> In
> those cases I could imagine folks reasonably arguing for including the
> working clock as a built in, but I'm not sure I'd say forcing
> everything to be built in is the better approach.

When the first driver converted as a module will be accepted, I'm pretty sure
there will be a wave of patches to convert more drivers into modules. 

What tool / use cases / tests can we put in place to ensure it is not breaking
the existing platforms for the different configurations?

> > * the timekeeping may do jump in the past [if and] when switching the
> > clocksource
> 
> ? It shouldn't. We've had tests in kselftest that switch between
> clocksources checking for inconsistencies for awhile, so if such a
> jump occurred it would be considered a bug.

But in the context of modules, the current clocksource counter is running but
what about the clocksource's counter related to the module which will be
started when the driver is loaded and then switches to the new clocksource. Is
it possible in this case there is a time drift between the clocksource which
was started at boot time and the one started later when the module is loaded ?
 
> >  * the GKI approach is to have an update for the 'mainline' kernel and
> > let the different SoC vendors deal with their drivers. I'm afraid this
> > will prevent driver fixes to be carry on upstream because they will stay
> > in the OoT kernels
> 
> I'm not sure I understand this point?  Could you expand on it a bit?
> While I very much can understand concerns and potential downsides of
> the GKI approach, I'm not sure how that applies to the submission
> here, as the benefit would apply to classic distro kernels as much as
> GKI.

Ok let's consider my comment as out of the technical aspects of the changes. I
can clarify it but it does not enter into consideration for the module
conversion. It is an overall feeling about the direction of all in modules for
GKI policy. I'm a little worried about changes not carried on mainline because
it is no longer an obstacle to keep OoT drivers. The core kernel is mainline
but the drivers can be vendor provided as module. I understand it is already
the case but the time framework is the base brick of the system, so there is
the risk a platform is supported with less than the minimum functionality.


-- 

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

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

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[John Stultz]

On Wed, Apr 16, 2025 at 6:46 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Tue, Apr 15, 2025 at 05:48:41PM -0700, John Stultz wrote:
> > On Tue, Apr 15, 2025 at 9:50 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> > > I have some concerns about this kind of changes:
> > >
> > >   * the core code may not be prepared for that, so loading / unloading
> > > the modules with active timers may result into some issues
> >
> > That's a fair concern, but permanent modules (which are loaded but not
> > unloaded) shouldn't suffer this issue. I recognize having modules be
> > fully unloadable is generally cleaner and preferred, but I also see
> > the benefit of allowing permanent modules to be one-way loaded so a
> > generic/distro kernel shared between lots of different platforms
> > doesn't need to be bloated with drivers that aren't used everywhere.
> > Obviously any single driver doesn't make a huge difference, but all
> > the small drivers together does add up.
>
> Yes, I agree.
>
> So the whole clockevent / clocksource drivers policy would have to be making
> impossible to unload a module once it is loaded.
>
> Do you have any ideas how to ensure that the converted drivers follow this
> rule without putting more burden on the maintainer?

Permanent modules just don't have a module_exit() hook, so that is
pretty easy to look for.
Obviously, I don't want to add more burden to the maintainership.

From a given clockevent driver (or maybe a function pointer), we could
check on the registration by calling __module_address(addr) [thanks to
Sami Tolvanen for pointing that function out to me] on one of the
function pointers provided, and check that there isn't a module->exit
pointer.

For clocksources we already have an owner pointer in the clocksource
struct that the driver is supposed to set if it's a module, but
clocksources already handle the get/puts needed to prevent modules
unloading under us.

> > > * the timekeeping may do jump in the past [if and] when switching the
> > > clocksource
> >
> > ? It shouldn't. We've had tests in kselftest that switch between
> > clocksources checking for inconsistencies for awhile, so if such a
> > jump occurred it would be considered a bug.
>
> But in the context of modules, the current clocksource counter is running but
> what about the clocksource's counter related to the module which will be
> started when the driver is loaded and then switches to the new clocksource. Is
> it possible in this case there is a time drift between the clocksource which
> was started at boot time and the one started later when the module is loaded ?

The clocksource code already has support for modules, and will do the
module_get and call enable hooks before switching to the new
clocksource. So the clocksource from the module has to be functioning
and running before timekeeping switches to using it.

By drift, its true changing clocksources can change the underlying
crystal so NTP has to begin again to determine any long-term frequency
adjustment needed, but we signal that properly.  And there should be
no time inconsistency during the switch as we accumulate everything
from the current clocksource and read a new base value from the new
clocksource. If there is, it's a bug.

> > >  * the GKI approach is to have an update for the 'mainline' kernel and
> > > let the different SoC vendors deal with their drivers. I'm afraid this
> > > will prevent driver fixes to be carry on upstream because they will stay
> > > in the OoT kernels
> >
> > I'm not sure I understand this point?  Could you expand on it a bit?
> > While I very much can understand concerns and potential downsides of
> > the GKI approach, I'm not sure how that applies to the submission
> > here, as the benefit would apply to classic distro kernels as much as
> > GKI.
>
> Ok let's consider my comment as out of the technical aspects of the changes. I
> can clarify it but it does not enter into consideration for the module
> conversion. It is an overall feeling about the direction of all in modules for
> GKI policy. I'm a little worried about changes not carried on mainline because
> it is no longer an obstacle to keep OoT drivers. The core kernel is mainline
> but the drivers can be vendor provided as module. I understand it is already
> the case but the time framework is the base brick of the system, so there is
> the risk a platform is supported with less than the minimum functionality.

So separately from this patch submission, I agree that the GKI
approach does not enforce vendor participation upstream. But there is
no rule *anywhere* that makes folks participate, and with the old
vendor trees it was definitely worse.

The GKI does result in vendors having a common interest in the
*actual* common portions of the kernel to be working well, so we can
make sure things like bug fixes, etc are submitted upstream first.
That is a clear benefit over separate vendor trees, but it's not a
magic tool to get everyone submitting all of their code upstream.

Trying to cajole upstream participation via barriers (not supporting
modular drivers upstream to try to enforce vendors submit patches to
add built in drivers for support) won't really work because they will
just be enabled as modules anyway out of tree. And it's hard to argue
against, as there isn't really a technical benefit to the GKI
requiring lots of SoC specific hardware support be built in.  So it
only ends up being another reason to not bother with upstream.

<Sorry, I'm getting a bit soap-boxy here>
I personally think the best tool we have to improve participation and
collaboration with the community is to do what we can to make it a
positive/beneficial/worthwhile experience. Every time I've submitted
patches and had bugs pointed out or fixes suggested, is a huge value
to me, and I have tremendous appreciation for folks sharing their
knowledge and expertise. And every time a patch I send gets merged or
a bug I reported ends up being fixed, there is a real sense of pride
in the contribution made to such an important project. Then, to get to
a point of a maintainership, and to be able to consider these amazing
influential developers as (relative)peers feels like such a career
accomplishment!  As an individual, those moments feel awesome and
motivate me over and over to want to reach out and share patches or
issues or thoughts.

And yes, there are organizations that focus more on how to exploit the
community for their benefit without contributing, and I get the
protective reaction that maintainers have to that. But I also know
there is *a lot* of amazing expertise inside the heads of
*individuals* who don't participate because they don't feel the "us vs
them" combative interactions are worth it. I think we/the community
are missing out, and those folks are the ones we should be trying to
welcome and include in order to build up our community.

Maintainers and the community need to keep high technical standards
and make the right long term choices, and developers won't agree all
the time on what those are, and I think that's all fine! But I think
if we want to grow the community and have more participation (as well
as growing folks into maintainers), I think we'll have more success
focusing on the honey than the vinegar.

thanks
-john

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[John Stultz]

On Wed, Apr 16, 2025 at 12:48 PM John Stultz <jstultz@google.com> wrote:
> On Wed, Apr 16, 2025 at 6:46 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> > So the whole clockevent / clocksource drivers policy would have to be making
> > impossible to unload a module once it is loaded.
> >
> > Do you have any ideas how to ensure that the converted drivers follow this
> > rule without putting more burden on the maintainer?
>
> Permanent modules just don't have a module_exit() hook, so that is
> pretty easy to look for.
> Obviously, I don't want to add more burden to the maintainership.
>
> From a given clockevent driver (or maybe a function pointer), we could
> check on the registration by calling __module_address(addr) [thanks to
> Sami Tolvanen for pointing that function out to me] on one of the
> function pointers provided, and check that there isn't a module->exit
> pointer.

Saravana also pointed out to me another approach that the irqchip code
uses: macros to populate an owner field with THIS_MODULE so that one
can easily get to the module struct
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/irqchip.h#n41

thanks
-john

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

[Peter Griffin]

Hi Will,

On Thu, 3 Apr 2025 at 00:34, Will McVicker <willmcvicker@google.com> 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>
> ---

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>
Tested-by: Peter Griffin <peter.griffin@linaro.org>

Tested using gs101-oriole with linux-next plus this series. With these
MCT timer changes and another series I will send shortly for
exynos-pmu to program the correct hint to ACPM wakeup from c2 idle
state is then functional.


>  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
>

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

[Peter Griffin]

On Thu, 3 Apr 2025 at 00:34, Will McVicker <willmcvicker@google.com> wrote:
>
> From: Hosung Kim <hosung0.kim@samsung.com>
>
> To allow the CPU to handle it's own clock events, we need to set the
> IRQF_PERCPU flag. This prevents the local timer interrupts from
> migrating to other CPUs.
>
> 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>
> ---

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>

Re: [PATCH v2 5/7] clocksource/drivers/exynos_mct: Fix uninitialized irq name warning

[Peter Griffin]

Hi Will,

On Thu, 3 Apr 2025 at 00:34, Will McVicker <willmcvicker@google.com> wrote:
>
> The Exynos MCT driver doesn't set the clocksource name until the CPU
> hotplug state is setup which happens after the IRQs are requested. This
> results in an empty IRQ name which leads to the below warning at
> proc_create() time. When this happens, the userdata partition fails to
> mount and the device gets stuck in an endless loop printing the error:
>
>   root '/dev/disk/by-partlabel/userdata' doesn't exist or does not contain a /dev.
>
> To fix this, we just need to initialize the name before requesting the
> IRQs.
>
> Warning from Pixel 6 kernel log:
>
> [  T430] name len 0
> [  T430] WARNING: CPU: 6 PID: 430 at fs/proc/generic.c:407 __proc_create+0x258/0x2b4
> [  T430] Modules linked in: dwc3_exynos(E+)
> [  T430]  ufs_exynos(E+) phy_exynos_ufs(E)
> [  T430]  phy_exynos5_usbdrd(E) exynos_usi(E+) exynos_mct(E+) s3c2410_wdt(E)
> [  T430]  arm_dsu_pmu(E) simplefb(E)
> [  T430] CPU: 6 UID: 0 PID: 430 Comm: (udev-worker) Tainted:
>          ... 6.14.0-next-20250331-4k-00008-g59adf909e40e #1 ...
> [  T430] Tainted: [W]=WARN, [E]=UNSIGNED_MODULE
> [  T430] Hardware name: Raven (DT)
> [...]
> [  T430] Call trace:
> [  T430]  __proc_create+0x258/0x2b4 (P)
> [  T430]  proc_mkdir+0x40/0xa0
> [  T430]  register_handler_proc+0x118/0x140
> [  T430]  __setup_irq+0x460/0x6d0
> [  T430]  request_threaded_irq+0xcc/0x1b0
> [  T430]  mct_init_dt+0x244/0x604 [exynos_mct ...]
> [  T430]  mct_init_spi+0x18/0x34 [exynos_mct ...]
> [  T430]  exynos4_mct_probe+0x30/0x4c [exynos_mct ...]
> [  T430]  platform_probe+0x6c/0xe4
> [  T430]  really_probe+0xf4/0x38c
> [...]
> [  T430]  driver_register+0x6c/0x140
> [  T430]  __platform_driver_register+0x28/0x38
> [  T430]  exynos4_mct_driver_init+0x24/0xfe8 [exynos_mct ...]
> [  T430]  do_one_initcall+0x84/0x3c0
> [  T430]  do_init_module+0x58/0x208
> [  T430]  load_module+0x1de0/0x2500
> [  T430]  init_module_from_file+0x8c/0xdc
>
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---

You could additionally consider adding a Fixes: tag and CC stable if
you want this to land in LTS tree's.

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>

>  drivers/clocksource/exynos_mct.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index a5ef7d64b1c2..62febeb4e1de 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -465,8 +465,6 @@ static int exynos4_mct_starting_cpu(unsigned int cpu)
>                 per_cpu_ptr(&percpu_mct_tick, cpu);
>         struct clock_event_device *evt = &mevt->evt;
>
> -       snprintf(mevt->name, sizeof(mevt->name), "mct_tick%d", cpu);
> -
>         evt->name = mevt->name;
>         evt->cpumask = cpumask_of(cpu);
>         evt->set_next_event = exynos4_tick_set_next_event;
> @@ -567,6 +565,14 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
>         for (i = MCT_L0_IRQ; i < nr_irqs; i++)
>                 mct_irqs[i] = irq_of_parse_and_map(np, i);
>
> +       for_each_possible_cpu(cpu) {
> +               struct mct_clock_event_device *mevt =
> +                   per_cpu_ptr(&percpu_mct_tick, cpu);
> +
> +               snprintf(mevt->name, sizeof(mevt->name), "mct_tick%d",
> +                        cpu);
> +       }
> +
>         if (mct_int_type == MCT_INT_PPI) {
>
>                 err = request_percpu_irq(mct_irqs[MCT_L0_IRQ],
> --
> 2.49.0.472.ge94155a9ec-goog
>

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[Peter Griffin]

Hi Daniel,

[..]

> > > On Wed, Apr 02, 2025 at 04:33:57PM -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.
> > >
> > > From a previous thread where there is no answer:

Aside from the timer modularization part here which is proving a bit
contentious are
you OK with queueing the other parts of this MCT series?

As I need some of the MCT driver changes to enable CPUIdle on Pixel 6 upstream.
Exiting from c2 idle state is broken for gs101 without some of these
MCT changes.

[..]

> > >
> > > https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
> > >
> > > I don't feel comfortable with changing the clocksource / clockevent drivers to
> > > a module for the reasons explained in the aforementionned thread.
> > >
> > > Before this could be accepted, I really need a strong acked-by from Thomas
> >
> > Thanks for the response! I'll copy-and-paste your replies from that previous
> > thread and try to address your concerns.
> >
> > >   * the GKI approach is to have an update for the 'mainline' kernel and
> > > let the different SoC vendors deal with their drivers. I'm afraid this
> > > will prevent driver fixes to be carry on upstream because they will stay
> > > in the OoT kernels
> >
> > I can't speak for that specific thread or their intent, but I can speak to this
> > thread and our intent.
> >
> > This whole patch series is about upstreaming the downstream changes. So saying
> > this will prevent others from upstreaming changes is punishing the folks who
> > are actually trying to upstream changes. I don't think that's a fair way to
> > handle this.
> >
> > Also, rejecting this series will not prevent people from upstreaming their
> > changes, it'll just make it more unlikely because they now have to deal with
> > upstreaming more changes that were rejected in the past. That's daunting for
> > someone who doesn't do upstreaming often. I'm telling this from experience
> > dealing with SoC vendors and asking them to upstream stuff.
> >
> > With that said, let me try to address some of your technical concerns.
>
> I won't reject the series based on my opinion. Answering the technical concerns
> will prevail.
>
> Why is it needed to convert the timer into a module ?

MCT is hardware very specific to Exynos based SoCs. Forcing this
built-in is just
bloat for many other systems that will never use it.

The aim of this series is to upstream the downstream OOT code with the goal of
then switching over to the upstream version of the driver.

I agree with your points though that we should be sure the timer framework can
handle this, and we aren't introducing subtle bugs.

>
> > > * the core code may not be prepared for that, so loading / unloading
> > > the modules with active timers may result into some issues
> >
> > We had the same concern for irqchip drivers. We can easily disable unloading
> > for these clocksource modules just like we did for irqchip by making them
> > permanent modules.
>
> In the clockevent / clocksource initialization process, depending on the
> platform, some are needed very early and other can be loaded later.
>
> For example, the usual configuration is the architected timers are initialized
> very early, then the external timer is loaded a bit later. And when this one is
> loaded it does not take over the architected timers. It acts as a "broadcast"
> timer to program the next timer event when the current CPU is going to an idle
> state where the local timer is stopped.
>
> Other cases are the architected timers are not desired and the 'external' timer
> is used in place when it is loaded with a higher rating. Some configuration can
> mimic local timers by settting a per CPU timer.
>
> Some platforms could be without the architected timers, so the 'external' timer
> is used.
>
> Let's imagine the system started, the timers are running and then we load a
> module with a timer replacing the current ones. Does it work well ?
>
> Are we sure, the timer modularization is compatible with all the timer use cases ?

It's a good question. We can say it's been used as a module on
multiple generations
of Pixel devices in production without issues. Whether that covers all
timer use cases
I can't say.

>
> > > * it may end up with some interactions with cpuidle at boot time and
> > > the broadcast timer
> >
> > If I'm understanding this correctly, no driver is guaranteed to probe at
> > initialization time regardless of whether it is built-in or a module. Taking
> > a look at the other clocksource drivers, I found that the following drivers are
> > all calling `clocksource_register_hz()` and `clockevents_config_and_register()`
> > at probe time.
> >
> >   timer-sun5i.c
> >   sh_tmu.c
> >   sh_cmt.c
> >   timer-tegra186.c
> >   timer-stm32-lp.c (only calls clockevents_config_and_register())
> >
> > So this concern is unrelated to building these drivers are modules. Please let
> > me know if I'm missing something here.
>
> We would have to check each platform individually to answer this question.
>
> The interaction between cpuidle and the timer module is about not having a
> broadcast timer when cpuidle initializes and then having it later when the
> module is loaded. Did you check the deep idle states are used after loading the
> module ?

For gs101 / Oriole deep idle states are used after loading the module as if the
MCT timer *isn't* used then the CPU can't wake up from c2 and eventually there
are no CPUs left leading to a hang.

>
> The discussion is not about only the Exynos MCT but as you are not the first
> one asking to convert the timer driver to a module, we should check what could
> be the impact on the time framework and the system in general.
>
> Others proposed to convert to module and I asked to investigate the impact.
> Nobody came back with a clear answer and there is no feedback from Thomas.

Hopefully the above answers go a little bit towards giving a degree of
confidence
that this is a safe change :)

regards,

Peter.

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[Daniel Lezcano]

On Tue, Apr 15, 2025 at 05:48:41PM -0700, John Stultz wrote:
> On Tue, Apr 15, 2025 at 9:50 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> > On Wed, Apr 02, 2025 at 04:33:57PM -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.
> >
> > From a previous thread where there is no answer:
> >
> > https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
> >
> > I don't feel comfortable with changing the clocksource / clockevent drivers to
> > a module for the reasons explained in the aforementionned thread.
> 
> I wasn't CC'ed on that, but to address a few of your points:
> 
> > I have some concerns about this kind of changes:
> >
> >   * the core code may not be prepared for that, so loading / unloading
> > the modules with active timers may result into some issues
> 
> That's a fair concern, but permanent modules (which are loaded but not
> unloaded) shouldn't suffer this issue. I recognize having modules be
> fully unloadable is generally cleaner and preferred, but I also see
> the benefit of allowing permanent modules to be one-way loaded so a
> generic/distro kernel shared between lots of different platforms
> doesn't need to be bloated with drivers that aren't used everywhere.
> Obviously any single driver doesn't make a huge difference, but all
> the small drivers together does add up.

Perhaps using module_platform_driver_probe() should do the trick with
some scripts updated for my git hooks to check
module_platform_driver() is not used.

[ ... ]

-- 

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

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

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[William McVicker]

On 05/13/2025, Daniel Lezcano wrote:
> On Tue, Apr 15, 2025 at 05:48:41PM -0700, John Stultz wrote:
> > On Tue, Apr 15, 2025 at 9:50 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> > > On Wed, Apr 02, 2025 at 04:33:57PM -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.
> > >
> > > From a previous thread where there is no answer:
> > >
> > > https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
> > >
> > > I don't feel comfortable with changing the clocksource / clockevent drivers to
> > > a module for the reasons explained in the aforementionned thread.
> > 
> > I wasn't CC'ed on that, but to address a few of your points:
> > 
> > > I have some concerns about this kind of changes:
> > >
> > >   * the core code may not be prepared for that, so loading / unloading
> > > the modules with active timers may result into some issues
> > 
> > That's a fair concern, but permanent modules (which are loaded but not
> > unloaded) shouldn't suffer this issue. I recognize having modules be
> > fully unloadable is generally cleaner and preferred, but I also see
> > the benefit of allowing permanent modules to be one-way loaded so a
> > generic/distro kernel shared between lots of different platforms
> > doesn't need to be bloated with drivers that aren't used everywhere.
> > Obviously any single driver doesn't make a huge difference, but all
> > the small drivers together does add up.
> 
> Perhaps using module_platform_driver_probe() should do the trick with
> some scripts updated for my git hooks to check
> module_platform_driver() is not used.

Using `module_platform_driver_probe()` won't work as that still defines
a `module_exit()` hook. If you want to automatically handle this in code, then
the best approach is to follow what Saravana did in [1] for irqchip drivers.
Basically by using `builtin_platform_driver(drv_name##_driver)`, you will only
define the `module_init()` hook when the driver is compiled as a module which
ensures you always get a permanent module.

[1] https://lore.kernel.org/linux-arm-kernel/20200718000637.3632841-1-saravanak@google.com/

Regards,
Will

[...]

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[Daniel Lezcano]

Hi William,

On 15/05/2025 01:16, William McVicker wrote:
> On 05/13/2025, Daniel Lezcano wrote:
>> On Tue, Apr 15, 2025 at 05:48:41PM -0700, John Stultz wrote:
>>> On Tue, Apr 15, 2025 at 9:50 AM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>> On Wed, Apr 02, 2025 at 04:33:57PM -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.
>>>>
>>>>  From a previous thread where there is no answer:
>>>>
>>>> https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
>>>>
>>>> I don't feel comfortable with changing the clocksource / clockevent drivers to
>>>> a module for the reasons explained in the aforementionned thread.
>>>
>>> I wasn't CC'ed on that, but to address a few of your points:
>>>
>>>> I have some concerns about this kind of changes:
>>>>
>>>>    * the core code may not be prepared for that, so loading / unloading
>>>> the modules with active timers may result into some issues
>>>
>>> That's a fair concern, but permanent modules (which are loaded but not
>>> unloaded) shouldn't suffer this issue. I recognize having modules be
>>> fully unloadable is generally cleaner and preferred, but I also see
>>> the benefit of allowing permanent modules to be one-way loaded so a
>>> generic/distro kernel shared between lots of different platforms
>>> doesn't need to be bloated with drivers that aren't used everywhere.
>>> Obviously any single driver doesn't make a huge difference, but all
>>> the small drivers together does add up.
>>
>> Perhaps using module_platform_driver_probe() should do the trick with
>> some scripts updated for my git hooks to check
>> module_platform_driver() is not used.
> 
> Using `module_platform_driver_probe()` won't work as that still defines
> a `module_exit()` hook. If you want to automatically handle this in code, then
> the best approach is to follow what Saravana did in [1] for irqchip drivers.
> Basically by using `builtin_platform_driver(drv_name##_driver)`, you will only
> define the `module_init()` hook when the driver is compiled as a module which
> ensures you always get a permanent module.
> 
> [1] https://lore.kernel.org/linux-arm-kernel/20200718000637.3632841-1-saravanak@google.com/

Thanks for the pointer and the heads up regarding the module_exit() 
problem with module_platform_driver_probe().

After digging into the timekeeping framework it appears if the owner of 
the clockevent device is set to THIS_MODULE, then the framework 
automatically grabs a reference preventing unloading the module when 
this one is registered.

IMO it was not heavily tested but for me it is enough to go forward with 
the module direction regarding the drivers.

One point though, the condition:

+#ifdef MODULE
[ ... ]
+static const struct of_device_id exynos4_mct_match_table[] = {
+	{ .compatible = "samsung,exynos4210-mct", .data = &mct_init_spi, },
+	{ .compatible = "samsung,exynos4412-mct", .data = &mct_init_ppi, },
+	{}
+};
+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

  is not acceptable as is. We don't want to do the same in all the 
drivers. Furthermore, we should not assume if the modules are enabled in 
the kernel that implies the driver is compiled as a module.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

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

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[William McVicker]

On 05/23/2025, Daniel Lezcano wrote:
> 
> Hi William,
> 
> On 15/05/2025 01:16, William McVicker wrote:
> > On 05/13/2025, Daniel Lezcano wrote:
> > > On Tue, Apr 15, 2025 at 05:48:41PM -0700, John Stultz wrote:
> > > > On Tue, Apr 15, 2025 at 9:50 AM Daniel Lezcano
> > > > <daniel.lezcano@linaro.org> wrote:
> > > > > On Wed, Apr 02, 2025 at 04:33:57PM -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.
> > > > > 
> > > > >  From a previous thread where there is no answer:
> > > > > 
> > > > > https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
> > > > > 
> > > > > I don't feel comfortable with changing the clocksource / clockevent drivers to
> > > > > a module for the reasons explained in the aforementionned thread.
> > > > 
> > > > I wasn't CC'ed on that, but to address a few of your points:
> > > > 
> > > > > I have some concerns about this kind of changes:
> > > > > 
> > > > >    * the core code may not be prepared for that, so loading / unloading
> > > > > the modules with active timers may result into some issues
> > > > 
> > > > That's a fair concern, but permanent modules (which are loaded but not
> > > > unloaded) shouldn't suffer this issue. I recognize having modules be
> > > > fully unloadable is generally cleaner and preferred, but I also see
> > > > the benefit of allowing permanent modules to be one-way loaded so a
> > > > generic/distro kernel shared between lots of different platforms
> > > > doesn't need to be bloated with drivers that aren't used everywhere.
> > > > Obviously any single driver doesn't make a huge difference, but all
> > > > the small drivers together does add up.
> > > 
> > > Perhaps using module_platform_driver_probe() should do the trick with
> > > some scripts updated for my git hooks to check
> > > module_platform_driver() is not used.
> > 
> > Using `module_platform_driver_probe()` won't work as that still defines
> > a `module_exit()` hook. If you want to automatically handle this in code, then
> > the best approach is to follow what Saravana did in [1] for irqchip drivers.
> > Basically by using `builtin_platform_driver(drv_name##_driver)`, you will only
> > define the `module_init()` hook when the driver is compiled as a module which
> > ensures you always get a permanent module.
> > 
> > [1] https://lore.kernel.org/linux-arm-kernel/20200718000637.3632841-1-saravanak@google.com/
> 
> Thanks for the pointer and the heads up regarding the module_exit() problem
> with module_platform_driver_probe().
> 
> After digging into the timekeeping framework it appears if the owner of the
> clockevent device is set to THIS_MODULE, then the framework automatically
> grabs a reference preventing unloading the module when this one is
> registered.
> 
> IMO it was not heavily tested but for me it is enough to go forward with the
> module direction regarding the drivers.

Great! Thanks for looking into that. I'll add that in the next revision and
verify we can't unload the module.

> 
> One point though, the condition:
> 
> +#ifdef MODULE
> [ ... ]
> +static const struct of_device_id exynos4_mct_match_table[] = {
> +	{ .compatible = "samsung,exynos4210-mct", .data = &mct_init_spi, },
> +	{ .compatible = "samsung,exynos4412-mct", .data = &mct_init_ppi, },
> +	{}
> +};
> +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
> 
>  is not acceptable as is. We don't want to do the same in all the drivers.

Are you suggesting we create a new timer macro to handle if we want to use
TIMER_OF_DECLARE() or builtin_platform_driver()?

> Furthermore, we should not assume if the modules are enabled in the kernel
> that implies the driver is compiled as a module.

Ah yes, that's right. The ifdef should be checking
CONFIG_CLKSRC_EXYNOS_MCT_MODULE.

Thanks,
Will

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[Saravana Kannan]

On Fri, May 23, 2025 at 10:06 AM William McVicker
<willmcvicker@google.com> wrote:
>
> On 05/23/2025, Daniel Lezcano wrote:
> >
> > Hi William,
> >
> > On 15/05/2025 01:16, William McVicker wrote:
> > > On 05/13/2025, Daniel Lezcano wrote:
> > > > On Tue, Apr 15, 2025 at 05:48:41PM -0700, John Stultz wrote:
> > > > > On Tue, Apr 15, 2025 at 9:50 AM Daniel Lezcano
> > > > > <daniel.lezcano@linaro.org> wrote:
> > > > > > On Wed, Apr 02, 2025 at 04:33:57PM -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.
> > > > > >
> > > > > >  From a previous thread where there is no answer:
> > > > > >
> > > > > > https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
> > > > > >
> > > > > > I don't feel comfortable with changing the clocksource / clockevent drivers to
> > > > > > a module for the reasons explained in the aforementionned thread.
> > > > >
> > > > > I wasn't CC'ed on that, but to address a few of your points:
> > > > >
> > > > > > I have some concerns about this kind of changes:
> > > > > >
> > > > > >    * the core code may not be prepared for that, so loading / unloading
> > > > > > the modules with active timers may result into some issues
> > > > >
> > > > > That's a fair concern, but permanent modules (which are loaded but not
> > > > > unloaded) shouldn't suffer this issue. I recognize having modules be
> > > > > fully unloadable is generally cleaner and preferred, but I also see
> > > > > the benefit of allowing permanent modules to be one-way loaded so a
> > > > > generic/distro kernel shared between lots of different platforms
> > > > > doesn't need to be bloated with drivers that aren't used everywhere.
> > > > > Obviously any single driver doesn't make a huge difference, but all
> > > > > the small drivers together does add up.
> > > >
> > > > Perhaps using module_platform_driver_probe() should do the trick with
> > > > some scripts updated for my git hooks to check
> > > > module_platform_driver() is not used.
> > >
> > > Using `module_platform_driver_probe()` won't work as that still defines
> > > a `module_exit()` hook. If you want to automatically handle this in code, then
> > > the best approach is to follow what Saravana did in [1] for irqchip drivers.
> > > Basically by using `builtin_platform_driver(drv_name##_driver)`, you will only
> > > define the `module_init()` hook when the driver is compiled as a module which
> > > ensures you always get a permanent module.
> > >
> > > [1] https://lore.kernel.org/linux-arm-kernel/20200718000637.3632841-1-saravanak@google.com/
> >
> > Thanks for the pointer and the heads up regarding the module_exit() problem
> > with module_platform_driver_probe().
> >
> > After digging into the timekeeping framework it appears if the owner of the
> > clockevent device is set to THIS_MODULE, then the framework automatically
> > grabs a reference preventing unloading the module when this one is
> > registered.
> >
> > IMO it was not heavily tested but for me it is enough to go forward with the
> > module direction regarding the drivers.
>
> Great! Thanks for looking into that. I'll add that in the next revision and
> verify we can't unload the module.

Daniel, is the module_get() done when someone uses the clock source or
during registration? Also, we either want to support modules that can
be unloaded or we don't. In that case, it's better to make it explicit
in the macros too. It's clear and it's set where it matters. Not
hidden deep inside the code -- I tried to find the answer to my
question above and it wasn't clear (showing that it's not obvious).

>
> >
> > One point though, the condition:
> >
> > +#ifdef MODULE
> > [ ... ]
> > +static const struct of_device_id exynos4_mct_match_table[] = {
> > +     { .compatible = "samsung,exynos4210-mct", .data = &mct_init_spi, },
> > +     { .compatible = "samsung,exynos4412-mct", .data = &mct_init_ppi, },
> > +     {}
> > +};
> > +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
> >
> >  is not acceptable as is. We don't want to do the same in all the drivers.
>
> Are you suggesting we create a new timer macro to handle if we want to use
> TIMER_OF_DECLARE() or builtin_platform_driver()?

One you convert a driver to tristate, there's no reason to continue
using TIMER_OF_DECLARE. Just always do the "module" approach. If it
gets built in, it'll just initialize early?

What am I missing?

Thanks,
Saravana

>
> > Furthermore, we should not assume if the modules are enabled in the kernel
> > that implies the driver is compiled as a module.
>
> Ah yes, that's right. The ifdef should be checking
> CONFIG_CLKSRC_EXYNOS_MCT_MODULE.
>
> Thanks,
> Will

Re: [PATCH v2 6/7] clocksource/drivers/exynos_mct: Add module support

[Daniel Lezcano]

On 23/05/2025 20:06, Saravana Kannan wrote:
> On Fri, May 23, 2025 at 10:06 AM William McVicker
> <willmcvicker@google.com> wrote:
>>
>> On 05/23/2025, Daniel Lezcano wrote:
>>>
>>> Hi William,
>>>
>>> On 15/05/2025 01:16, William McVicker wrote:
>>>> On 05/13/2025, Daniel Lezcano wrote:
>>>>> On Tue, Apr 15, 2025 at 05:48:41PM -0700, John Stultz wrote:
>>>>>> On Tue, Apr 15, 2025 at 9:50 AM Daniel Lezcano
>>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>> On Wed, Apr 02, 2025 at 04:33:57PM -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.
>>>>>>>
>>>>>>>   From a previous thread where there is no answer:
>>>>>>>
>>>>>>> https://lore.kernel.org/all/c1e8abec-680c-451d-b5df-f687291aa413@linaro.org/
>>>>>>>
>>>>>>> I don't feel comfortable with changing the clocksource / clockevent drivers to
>>>>>>> a module for the reasons explained in the aforementionned thread.
>>>>>>
>>>>>> I wasn't CC'ed on that, but to address a few of your points:
>>>>>>
>>>>>>> I have some concerns about this kind of changes:
>>>>>>>
>>>>>>>     * the core code may not be prepared for that, so loading / unloading
>>>>>>> the modules with active timers may result into some issues
>>>>>>
>>>>>> That's a fair concern, but permanent modules (which are loaded but not
>>>>>> unloaded) shouldn't suffer this issue. I recognize having modules be
>>>>>> fully unloadable is generally cleaner and preferred, but I also see
>>>>>> the benefit of allowing permanent modules to be one-way loaded so a
>>>>>> generic/distro kernel shared between lots of different platforms
>>>>>> doesn't need to be bloated with drivers that aren't used everywhere.
>>>>>> Obviously any single driver doesn't make a huge difference, but all
>>>>>> the small drivers together does add up.
>>>>>
>>>>> Perhaps using module_platform_driver_probe() should do the trick with
>>>>> some scripts updated for my git hooks to check
>>>>> module_platform_driver() is not used.
>>>>
>>>> Using `module_platform_driver_probe()` won't work as that still defines
>>>> a `module_exit()` hook. If you want to automatically handle this in code, then
>>>> the best approach is to follow what Saravana did in [1] for irqchip drivers.
>>>> Basically by using `builtin_platform_driver(drv_name##_driver)`, you will only
>>>> define the `module_init()` hook when the driver is compiled as a module which
>>>> ensures you always get a permanent module.
>>>>
>>>> [1] https://lore.kernel.org/linux-arm-kernel/20200718000637.3632841-1-saravanak@google.com/
>>>
>>> Thanks for the pointer and the heads up regarding the module_exit() problem
>>> with module_platform_driver_probe().
>>>
>>> After digging into the timekeeping framework it appears if the owner of the
>>> clockevent device is set to THIS_MODULE, then the framework automatically
>>> grabs a reference preventing unloading the module when this one is
>>> registered.
>>>
>>> IMO it was not heavily tested but for me it is enough to go forward with the
>>> module direction regarding the drivers.
>>
>> Great! Thanks for looking into that. I'll add that in the next revision and
>> verify we can't unload the module.
> 
> Daniel, is the module_get() done when someone uses the clock source or
> during registration? Also, we either want to support modules that can
> be unloaded or we don't. In that case, it's better to make it explicit
> in the macros too. It's clear and it's set where it matters. Not
> hidden deep inside the code -- 

Why do you want to unload ?

That is another aspect and the time framework is not totally ready for 
that. So I would consider for the moment to load only.

> I tried to find the answer to my
> question above and it wasn't clear (showing that it's not obvious).

Globally the idea would be to take a ref to the module when the 
clockevent or the clocksource is in use and release the ref when it is 
unused. That needs an extra function unregister_clockevent_device() and 
a verification of the current time core code to check if the ref is 
get/put correctly which is, after investigating a bit, not correct at 
the first glance.

>>> One point though, the condition:
>>>
>>> +#ifdef MODULE
>>> [ ... ]
>>> +static const struct of_device_id exynos4_mct_match_table[] = {
>>> +     { .compatible = "samsung,exynos4210-mct", .data = &mct_init_spi, },
>>> +     { .compatible = "samsung,exynos4412-mct", .data = &mct_init_ppi, },
>>> +     {}
>>> +};
>>> +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
>>>
>>>   is not acceptable as is. We don't want to do the same in all the drivers.
>>
>> Are you suggesting we create a new timer macro to handle if we want to use
>> TIMER_OF_DECLARE() or builtin_platform_driver()?
> 
> One you convert a driver to tristate, there's no reason to continue
> using TIMER_OF_DECLARE. Just always do the "module" approach. If it
> gets built in, it'll just initialize early?
> 
> What am I missing?

TIMER_OF_DECLARE relies on a mechanism building an array at compile 
time. It is called very early in the boot process.

What would be nice is to introduce something like 
TIMER_OF_MODULE_DECLARE() where builtin means TIMER_OF_DECLARE and 
module means module_platform_driver()



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

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