[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z+yWUj5ZLftPrbht@perf>
Date: Wed, 2 Apr 2025 10:43:46 +0900
From: Youngmin Nam <youngmin.nam@...sung.com>
To: William McVicker <willmcvicker@...gle.com>
Cc: John Stultz <jstultz@...gle.com>, Catalin Marinas
<catalin.marinas@....com>, Will Deacon <will@...nel.org>, Peter Griffin
<peter.griffin@...aro.org>, André Draszik
<andre.draszik@...aro.org>, Tudor Ambarus <tudor.ambarus@...aro.org>, Rob
Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
Dooley <conor+dt@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>, Daniel
Lezcano <daniel.lezcano@...aro.org>, Thomas Gleixner <tglx@...utronix.de>,
Saravana Kannan <saravanak@...gle.com>, Krzysztof Kozlowski
<krzk@...nel.org>, kernel-team@...roid.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org, Donghoon Yu
<hoony.yu@...sung.com>, Youngmin Nam <youngmin.nam@...sung.com>
Subject: Re: [PATCH v1 2/6] clocksource/drivers/exynos_mct: Don't register
as a sched_clock on arm64
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@...roid.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@...sung.com>
> > > Signed-off-by: Youngmin Nam <youngmin.nam@...sung.com>
> > > [Original commit from https://android.googlesource.com/kernel/gs/+/630817f7080e92c5e0216095ff52f6eb8dd00727
> > > Signed-off-by: Will McVicker <willmcvicker@...gle.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@...sung.com>
Powered by blists - more mailing lists