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@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx/ > [2] https://lore.kernel.org/linux-samsung-soc/CAASgrz2Nr69tpfC8ka9gbs2OvjLEGsvgAj4vBCFxhsamuFum7w@xxxxxxxxxxxxxx/ > > 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@xxxxxxxxxx/ > 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@xxxxxxxxxxxx/ > > > > 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@xxxxxxxxxxx> > > > Signed-off-by: Youngmin Nam <youngmin.nam@xxxxxxxxxxx> > > > [Original commit from https://android.googlesource.com/kernel/gs/+/630817f7080e92c5e0216095ff52f6eb8dd00727 > > > Signed-off-by: Will McVicker <willmcvicker@xxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxx>
