Re: [PATCH v2 4/4] clocksource/drivers/timer-mediatek: Make timer-mediatek become loadable module

From: AngeloGioacchino Del Regno
Date: Thu Feb 16 2023 - 06:36:31 EST


Il 16/02/23 12:23, Matthias Brugger ha scritto:


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

Right. I completely forgot about those platforms, even though my intention was
to actually try and migrate them once the CPUX was picked. My bad.

Well, I think that this module conversion will take quite a while, so there
should be no need to rush... I'll send a series later with the split *and* a
conversion of those platforms, so that we will see a removal of that
mediatek_timer_init() function.

Some encouraging words ahead: I'm totally confident that the conversion will
Just Work, because the MT6795 SoC had the same handling for CPUXGPT as the
older MT6589/7623/8127/8135... as that SoC had two implementations initially,
one as ARM, one as ARM64.

Whatever - let's see what I can come up with, then.

Cheers,
Angelo