1. Patchew
  2. linux
  3. [PATCH v1 0/6] Add module support for Arm64 Exynos MCT driver
  4. View patch

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

Will McVicker posted 6 patches 8 months, 3 weeks ago
There is a newer version of this series
[PATCH v1 2/6] clocksource/drivers/exynos_mct: Don't register as a sched_clock on arm64
Posted by Will McVicker 8 months, 3 weeks ago 
When using the Exynos MCT as a sched_clock, accessing the timer value
via the MCT register is extremely slow. To improve performance on Arm64
SoCs, use the Arm architected timer instead for timekeeping.

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

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

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

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index da09f467a6bb..05c50f2f7a7e 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -219,12 +219,12 @@ static struct clocksource mct_frc = {
 	.resume		= exynos4_frc_resume,
 };
 
+#if defined(CONFIG_ARM)
 static u64 notrace exynos4_read_sched_clock(void)
 {
 	return exynos4_read_count_32();
 }
 
-#if defined(CONFIG_ARM)
 static struct delay_timer exynos4_delay_timer;
 
 static cycles_t exynos4_read_current_timer(void)
@@ -250,12 +250,13 @@ static int __init exynos4_clocksource_init(bool frc_shared)
 	exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
 	exynos4_delay_timer.freq = clk_rate;
 	register_current_timer_delay(&exynos4_delay_timer);
+
+	sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
 #endif
 
 	if (clocksource_register_hz(&mct_frc, clk_rate))
 		panic("%s: can't register clocksource\n", mct_frc.name);
 
-	sched_clock_register(exynos4_read_sched_clock, 32, clk_rate);
 
 	return 0;
 }
-- 
2.49.0.472.ge94155a9ec-goog
Re: [PATCH v1 2/6] clocksource/drivers/exynos_mct: Don't register as a sched_clock on arm64
Posted by John Stultz 8 months, 3 weeks ago 
On Mon, Mar 31, 2025 at 4:00 PM 'Will McVicker' via kernel-team
<kernel-team@android.com> wrote:
>
> When using the Exynos MCT as a sched_clock, accessing the timer value
> via the MCT register is extremely slow. To improve performance on Arm64
> SoCs, use the Arm architected timer instead for timekeeping.

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

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

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

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

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

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

Otherwise, this looks ok to me.

thanks
-john
Re: [PATCH v1 2/6] clocksource/drivers/exynos_mct: Don't register as a sched_clock on arm64
Posted by William McVicker 8 months, 3 weeks ago 
On 03/31/2025, John Stultz wrote:
> On Mon, Mar 31, 2025 at 4:00 PM 'Will McVicker' via kernel-team
> <kernel-team@android.com> wrote:
> >
> > When using the Exynos MCT as a sched_clock, accessing the timer value
> > via the MCT register is extremely slow. To improve performance on Arm64
> > SoCs, use the Arm architected timer instead for timekeeping.
> 
> This probably needs some further expansion to explain why we don't
> want to use it for sched_clock but continue to register the MCT as a
> clocksource (ie: why not disable MCT entirely?).

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

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

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

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

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

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

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

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

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

Thanks for taking the time to review!

Regards,
Will
Re: [PATCH v1 2/6] clocksource/drivers/exynos_mct: Don't register as a sched_clock on arm64
Posted by Youngmin Nam 8 months, 3 weeks ago 
Hi Will.

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

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

Exactly right.

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

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.