Re: [PATCH v2 4/4] clocksource/drivers/timer-mediatek: Make timer-mediatek become loadable module
From: Matthias Brugger
Date: Thu Feb 16 2023 - 06:23:23 EST
On 16/02/2023 11:22, AngeloGioacchino Del Regno wrote:
Il 15/02/23 15:46, Sudeep Holla ha scritto:
On Wed, Feb 15, 2023 at 02:30:51PM +0100, AngeloGioacchino Del Regno wrote:
Both. I mean that these platforms do have architected timers, but they are
stopped
before the bootloader jumps to the kernel, or they are never started at all.
Please refer to:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/clocksource/timer-mediatek.c?h=next-20230215&id=327e93cf9a59b0d04eb3a31a7fdbf0f11cf13ecb
For a nice explanation.
Thanks for that. Well then I see no point in making these modules if you
can't have generic Image that boots on all the platform. I now tend to think
that these are made modules just because GKI demands and it *might* work
on one or 2 platforms. One we move this as modules, how will be know the
Image without these timers or with them built as modules will boot or not
on a given mediatek platform. Sorry, I initially saw some point in making
these timers as modules but if they are required for boot on some systems
then I see no point. So if that is the case, NACK for these as it just
creates more confusion after these are merged as why some Images or
even why defconfig image(if we push the config change as well) is not
booting on these platforms.
It is no longer just for system timer useful in low power CPU idle states
as I initial thought.
I think that there is still a point in modularization for this driver and I
can propose a rather simple solution, even though this may add some, rather
little, code duplication to the mix.
The platforms that I've described (like mt6795) need the system timer to be
initialized as early as possible - that's true - but that timer is always
"CPUXGPT".
On those platforms, you *still* have multiple timers:
- CPUX (short for cpuxgpt), used only as system timer;
- SYST, as another system timer implementation (additional timers) but
those are always initialized (AFAIK) from the bootloader before booting;
- GPT (General Purpose Timer).
On one SoC, you may have:
- CPUX *and* SYST
- CPUX *and* GPT
- CPUX *and* SYST *and* GPT
... where the only one that is boot critical and needs to be initialized early
is always only CPUX.
Hence this proposal: to still allow modularization of timers on MediaTek platforms,
we could eventually split the CPUX as a separated driver that *cannot be*, due to
the previously explained constraints, compiled as module, hence always built-in,
from a timer-mediatek driver that could be a module and capable of handling only
SYST and GPT timers.
In that case, we'd hence have...
- timer-mediatek-cpux.o (bool)
- timer-mediatek.c (tristate)
Counting that the CPUX timers are actually even using different `tick_resume`
and `set_state_shutdown` callbacks (doing only a IRQ clear/restore and nothing
else), the amount of duplication would be .. well, again, minimal, but then
this means that timer-mediatek-cpux would be `default y if ARCH_MEDIATEK`, or
even selected by ARCH_MEDIATEK itself.
If you think that this could be a good solution, I can send a "fast" patch to
split it out, preparing the ground for the people doing this module work.
Any considerations?
I think your proposal sounds acceptable, but we would need to make sure that all
SoCs can boot with the CPUX driver. I'm aware of some armv7 SoCs that use a kind
of hack to enable the architecture timer [1]. This, for one part, should be
moved to CPUX, if possible. For the other part it makes me wonder if really all
supported MediaTek platforms will boot with SYST/GPT being a module. I think we
will need some effort from the community to test that.
So as a resume, yes I think your approach is feasible but we should collect
tested-by tags before merging it.
Regards,
Matthias
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-mediatek/mediatek.c?h=v6.2-rc8#n16