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

[Youngmin Nam <youngmin.nam@samsung.com>]

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

Hi Will.

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

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

Exactly right.

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

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

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