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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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?

Regards,
Angelo