Re: [PATCH v6 3/6] clk: spacemit: Add clock support for SpacemiT K1 SoC
From: Alex Elder
Date: Thu Apr 10 2025 - 08:30:50 EST
On 4/9/25 10:47 PM, Inochi Amaoto wrote:
On Thu, Apr 10, 2025 at 01:55:49AM +0000, Yixun Lan wrote:
Hi Inochi,
On 09:20 Thu 10 Apr , Inochi Amaoto wrote:
On Wed, Apr 09, 2025 at 08:10:53PM -0500, Alex Elder wrote:
On 4/9/25 7:57 PM, Inochi Amaoto wrote:
diff --git a/drivers/clk/spacemit/Kconfig b/drivers/clk/spacemit/Kconfig
new file mode 100644
index 000000000000..4c4df845b3cb
--- /dev/null
+++ b/drivers/clk/spacemit/Kconfig
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config SPACEMIT_CCU
+ tristate "Clock support for SpacemiT SoCs"
I don't know the answer to this, but... Should this be a Boolean
rather than tristate? Can a SpacemiT K1 SoC function without the
clock driver built in to the kernel?
I agree to make it a Boolean, we've already made pinctrl driver Boolean
and pinctrl depend on clk, besides, the SoC is unlikely functional
without clock built in as it's such critical..
I disagree. The kernel is only for spacemit only, and the pinctrl
Sorry for a mistake, this first "only" should be "not".
This is a general problem. You can't make a bootable
SpacemiT kernel unless you define this as built-in (at
least, that's what Yixun is saying).
Why not putting the module in the initramfs? I have tested
this in quite a lot of boards (Allwinner, rockchip, sophgo,
starfive and etc.), all of them work well.
it works, but not optimal, why delay clk initialzation at modules load stage?
IMO, it brings more overhead for using initramfs..
but there is always tradeoff and bikeshedding..
But we'd really rather *only* build it in to the kernel
for SpacemiT builds. You clearly want to minimize what
must be built in, but what if this is indeed required?
What goes in defconfig?
As defconfig is more like for a minimum example system. It
is OK to put a y in the defconfig. But for a custom system,
you do give a choice for the builder to remove your module
in non spacemit system.
I get your meaning here to remove/disable at run time stage, while
we do provide compile time option, if don't want spacemit system
just disable CONFIG_ARCH_SPACEMIT I mentioned, clk/pinctrl will be gone
I think this is not suitable for the most generic case, Especially
for distribution kernel. They prefer to set almost everything as
module, and load necessary module in initramfs, but the thing is as
you said, it is a tradeoff. So I will wait and see whether there
is any new voice for it.
I was the one who suggested it might be made Boolean, *if*
this code was actually required for a defconfig kernel on
a SpacemiT K1 platform. Yes I know needed modules can be
placed in the initramfs image, but I guess it's almost a
philosophical question of what exactly a defconfig kernel
is supposed to do: boot successfully on all supported
platforms without an initramdisk; or with one that includes
required modules.
I don't honestly care that much, so leaving it as a
tristate is fine.
That begs the question of what goes into the defconfig
file, which currently includes CONFIG_ARCH_SPACEMIT=y.
I'd like to see that be the only thing there, and have
various SpacemiT modules define default values that
depend on ARCH_SPACEMIT (or _K1) in their Kconfig
file. Like:
config SPACEMIT_K1_CCU
tristate "Support for SpacemiT K1 SoC"
depends on ARCH_SPACEMIT || COMPILE_TEST
default m if ARCH_SPACEMIT
I *think* Haylen said that's what he's going to do. You
could make it "default ARCH_SPACEMIT" too, though that
builds it in to the kernel.
-Alex
Regards,
Inochi