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

[William McVicker <willmcvicker@google.com>]

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