MT8195 clock drivers were encapsulated in one single (and big) Kconfig
option: there's no reason to do that, as it is totally unnecessary to
build in all or none of them.

Split them out: keep boot-critical clocks as bool and allow choosing
non critical clocks as tristate.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/clk/mediatek/Kconfig  | 86 +++++++++++++++++++++++++++++++++++
 drivers/clk/mediatek/Makefile | 20 +++++---
 2 files changed, 99 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
index 45b7aea7648d..88937d111e98 100644
--- a/drivers/clk/mediatek/Kconfig
+++ b/drivers/clk/mediatek/Kconfig
@@ -692,6 +692,92 @@ config COMMON_CLK_MT8195
         help
           This driver supports MediaTek MT8195 clocks.
 
+config COMMON_CLK_MT8195_APUSYS
+	tristate "Clock driver for MediaTek MT8195 apusys"
+	depends on COMMON_CLK_MT8195
+	help
+	  This driver supports MediaTek MT8195 AI Processor Unit System clocks.
+
+config COMMON_CLK_MT8195_AUDSYS
+	tristate "Clock driver for MediaTek MT8195 audsys"
+	depends on COMMON_CLK_MT8195
+	help
+	  This driver supports MediaTek MT8195 audsys clocks.
+
+config COMMON_CLK_MT8195_CAMSYS
+	tristate "Clock driver for MediaTek MT8195 camsys"
+	depends on COMMON_CLK_MT8195_VPPSYS
+	help
+	  This driver supports MediaTek MT8195 camsys and camsys_raw clocks.
+
+config COMMON_CLK_MT8195_IMGSYS
+	tristate "Clock driver for MediaTek MT8195 imgsys"
+	depends on COMMON_CLK_MT8195_VPPSYS
+	help
+	  This driver supports MediaTek MT8195 imgsys and imgsys2 clocks.
+
+config COMMON_CLK_MT8195_IMP_IIC_WRAP
+	tristate "Clock driver for MediaTek MT8195 imp_iic_wrap"
+	depends on COMMON_CLK_MT8195
+	help
+	  This driver supports MediaTek MT8195 I2C/I3C clocks.
+
+config COMMON_CLK_MT8195_IPESYS
+	tristate "Clock driver for MediaTek MT8195 ipesys"
+	depends on COMMON_CLK_MT8195_IMGSYS
+	help
+	  This driver supports MediaTek MT8195 ipesys clocks.
+
+config COMMON_CLK_MT8195_MFGCFG
+	tristate "Clock driver for MediaTek MT8195 mfgcfg"
+	depends on COMMON_CLK_MT8195
+	help
+	  This driver supports MediaTek MT8195 mfgcfg clocks.
+
+config COMMON_CLK_MT8195_VDOSYS
+	tristate "Clock driver for MediaTek MT8195 vdosys"
+	depends on COMMON_CLK_MT8195
+	help
+	  This driver supports MediaTek MT8195 vdosys0/1 (multimedia) clocks.
+
+config COMMON_CLK_MT8195_MSDC
+	tristate "Clock driver for MediaTek MT8195 msdc"
+	depends on COMMON_CLK_MT8195
+	help
+	  This driver supports MediaTek MT8195 MMC and SD Controller's
+	  msdc and msdc_top clocks.
+
+config COMMON_CLK_MT8195_SCP_ADSP
+	tristate "Clock driver for MediaTek MT8195 scp_adsp"
+	depends on COMMON_CLK_MT8195
+	help
+	  This driver supports MediaTek MT8195 System Companion Processor
+	  Audio DSP clocks.
+
+config COMMON_CLK_MT8195_VPPSYS
+	tristate "Clock driver for MediaTek MT8195 vppsys"
+	depends on COMMON_CLK_MT8195
+	help
+	  This driver supports MediaTek MT8195 vppsys0/1 clocks.
+
+config COMMON_CLK_MT8195_VDECSYS
+	tristate "Clock driver for MediaTek MT8195 vdecsys"
+	depends on COMMON_CLK_MT8195_VPPSYS
+	help
+	  This driver supports MediaTek MT8195 vdecsys and vdecsys_soc clocks.
+
+config COMMON_CLK_MT8195_VENCSYS
+	tristate "Clock driver for MediaTek MT8195 vencsys"
+	depends on COMMON_CLK_MT8195_VPPSYS
+	help
+	  This driver supports MediaTek MT8195 vencsys clocks.
+
+config COMMON_CLK_MT8195_WPESYS
+	tristate "Clock driver for MediaTek MT8195 wpesys"
+	depends on COMMON_CLK_MT8195_IMGSYS
+	help
+	  This driver supports MediaTek MT8195 Warp Engine clocks.
+
 config COMMON_CLK_MT8365
 	tristate "Clock driver for MediaTek MT8365"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
index 3133ad8c2028..63351957f862 100644
--- a/drivers/clk/mediatek/Makefile
+++ b/drivers/clk/mediatek/Makefile
@@ -106,13 +106,19 @@ obj-$(CONFIG_COMMON_CLK_MT8192_SCP_ADSP) += clk-mt8192-scp_adsp.o
 obj-$(CONFIG_COMMON_CLK_MT8192_VDECSYS) += clk-mt8192-vdec.o
 obj-$(CONFIG_COMMON_CLK_MT8192_VENCSYS) += clk-mt8192-venc.o
 obj-$(CONFIG_COMMON_CLK_MT8195) += clk-mt8195-apmixedsys.o clk-mt8195-topckgen.o \
-				   clk-mt8195-peri_ao.o clk-mt8195-infra_ao.o \
-				   clk-mt8195-cam.o clk-mt8195-ccu.o clk-mt8195-img.o \
-				   clk-mt8195-ipe.o clk-mt8195-mfg.o clk-mt8195-scp_adsp.o \
-				   clk-mt8195-vdec.o clk-mt8195-vdo0.o clk-mt8195-vdo1.o \
-				   clk-mt8195-venc.o clk-mt8195-vpp0.o clk-mt8195-vpp1.o \
-				   clk-mt8195-wpe.o clk-mt8195-imp_iic_wrap.o \
-				   clk-mt8195-apusys_pll.o
+				   clk-mt8195-peri_ao.o clk-mt8195-infra_ao.o
+obj-$(CONFIG_COMMON_CLK_MT8195_APUSYS) += clk-mt8195-apusys_pll.o
+obj-$(CONFIG_COMMON_CLK_MT8195_CAMSYS) += clk-mt8195-cam.o clk-mt8195-ccu.o
+obj-$(CONFIG_COMMON_CLK_MT8195_IMGSYS) += clk-mt8195-img.o
+obj-$(CONFIG_COMMON_CLK_MT8195_IMP_IIC_WRAP) += clk-mt8195-imp_iic_wrap.o
+obj-$(CONFIG_COMMON_CLK_MT8195_IPESYS) += clk-mt8195-ipe.o
+obj-$(CONFIG_COMMON_CLK_MT8195_MFGCFG) += clk-mt8195-mfg.o
+obj-$(CONFIG_COMMON_CLK_MT8195_SCP_ADSP) += clk-mt8195-scp_adsp.o
+obj-$(CONFIG_COMMON_CLK_MT8195_VDECSYS) += clk-mt8195-vdec.o
+obj-$(CONFIG_COMMON_CLK_MT8195_VDOSYS) += clk-mt8195-vdo0.o clk-mt8195-vdo1.o
+obj-$(CONFIG_COMMON_CLK_MT8195_VENCSYS) += clk-mt8195-venc.o
+obj-$(CONFIG_COMMON_CLK_MT8195_VPPSYS) += clk-mt8195-vpp0.o clk-mt8195-vpp1.o
+obj-$(CONFIG_COMMON_CLK_MT8195_WPESYS) += clk-mt8195-wpe.o
 obj-$(CONFIG_COMMON_CLK_MT8365) += clk-mt8365.o clk-mt8365-apmixedsys.o
 obj-$(CONFIG_COMMON_CLK_MT8365_APU) += clk-mt8365-apu.o
 obj-$(CONFIG_COMMON_CLK_MT8365_CAM) += clk-mt8365-cam.o
-- 
2.39.1

On Tue, Feb 14, 2023 at 9:42 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> MT8195 clock drivers were encapsulated in one single (and big) Kconfig
> option: there's no reason to do that, as it is totally unnecessary to
> build in all or none of them.
>
> Split them out: keep boot-critical clocks as bool and allow choosing
> non critical clocks as tristate.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/clk/mediatek/Kconfig  | 86 +++++++++++++++++++++++++++++++++++
>  drivers/clk/mediatek/Makefile | 20 +++++---
>  2 files changed, 99 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
> index 45b7aea7648d..88937d111e98 100644
> --- a/drivers/clk/mediatek/Kconfig
> +++ b/drivers/clk/mediatek/Kconfig
> @@ -692,6 +692,92 @@ config COMMON_CLK_MT8195
>          help
>            This driver supports MediaTek MT8195 clocks.
>
> +config COMMON_CLK_MT8195_APUSYS
> +       tristate "Clock driver for MediaTek MT8195 apusys"
> +       depends on COMMON_CLK_MT8195
> +       help
> +         This driver supports MediaTek MT8195 AI Processor Unit System clocks.
> +
> +config COMMON_CLK_MT8195_AUDSYS
> +       tristate "Clock driver for MediaTek MT8195 audsys"
> +       depends on COMMON_CLK_MT8195
> +       help
> +         This driver supports MediaTek MT8195 audsys clocks.
> +
> +config COMMON_CLK_MT8195_CAMSYS
> +       tristate "Clock driver for MediaTek MT8195 camsys"
> +       depends on COMMON_CLK_MT8195_VPPSYS

One other thing. If a Kconfig option immediately follows its dependency,
then it gets indented nicely in menuconfig, but only if.
If other options are interspersed, then the indentation gets reset.

So could you reorder the options to follow the dependency graph?

Also how you chose the dependencies should be mentioned in the commit log.
These are pure run time dependencies, not compile time nor link/load ones.

Last, I think an argument could be made against the proliferation of
Kconfig options, as it dramatically increases the combinations of
allrandconfigs. Maybe Arnd (who IIRC frequently runs allrandconfig)
could chime in on whether this is actually a concern or not.

> +       help
> +         This driver supports MediaTek MT8195 camsys and camsys_raw clocks.
> +
> +config COMMON_CLK_MT8195_IMGSYS
> +       tristate "Clock driver for MediaTek MT8195 imgsys"
> +       depends on COMMON_CLK_MT8195_VPPSYS
> +       help
> +         This driver supports MediaTek MT8195 imgsys and imgsys2 clocks.
> +
> +config COMMON_CLK_MT8195_IMP_IIC_WRAP
> +       tristate "Clock driver for MediaTek MT8195 imp_iic_wrap"
> +       depends on COMMON_CLK_MT8195
> +       help
> +         This driver supports MediaTek MT8195 I2C/I3C clocks.
> +
> +config COMMON_CLK_MT8195_IPESYS
> +       tristate "Clock driver for MediaTek MT8195 ipesys"
> +       depends on COMMON_CLK_MT8195_IMGSYS
> +       help
> +         This driver supports MediaTek MT8195 ipesys clocks.
> +
> +config COMMON_CLK_MT8195_MFGCFG
> +       tristate "Clock driver for MediaTek MT8195 mfgcfg"
> +       depends on COMMON_CLK_MT8195
> +       help
> +         This driver supports MediaTek MT8195 mfgcfg clocks.
> +
> +config COMMON_CLK_MT8195_VDOSYS
> +       tristate "Clock driver for MediaTek MT8195 vdosys"
> +       depends on COMMON_CLK_MT8195

Not sure why this option is here, out of order?

> +       help
> +         This driver supports MediaTek MT8195 vdosys0/1 (multimedia) clocks.

[...]

ChenYu

Il 17/02/23 08:37, Chen-Yu Tsai ha scritto:
> On Tue, Feb 14, 2023 at 9:42 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> MT8195 clock drivers were encapsulated in one single (and big) Kconfig
>> option: there's no reason to do that, as it is totally unnecessary to
>> build in all or none of them.
>>
>> Split them out: keep boot-critical clocks as bool and allow choosing
>> non critical clocks as tristate.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/clk/mediatek/Kconfig  | 86 +++++++++++++++++++++++++++++++++++
>>   drivers/clk/mediatek/Makefile | 20 +++++---
>>   2 files changed, 99 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
>> index 45b7aea7648d..88937d111e98 100644
>> --- a/drivers/clk/mediatek/Kconfig
>> +++ b/drivers/clk/mediatek/Kconfig
>> @@ -692,6 +692,92 @@ config COMMON_CLK_MT8195
>>           help
>>             This driver supports MediaTek MT8195 clocks.
>>
>> +config COMMON_CLK_MT8195_APUSYS
>> +       tristate "Clock driver for MediaTek MT8195 apusys"
>> +       depends on COMMON_CLK_MT8195
>> +       help
>> +         This driver supports MediaTek MT8195 AI Processor Unit System clocks.
>> +
>> +config COMMON_CLK_MT8195_AUDSYS
>> +       tristate "Clock driver for MediaTek MT8195 audsys"
>> +       depends on COMMON_CLK_MT8195
>> +       help
>> +         This driver supports MediaTek MT8195 audsys clocks.
>> +
>> +config COMMON_CLK_MT8195_CAMSYS
>> +       tristate "Clock driver for MediaTek MT8195 camsys"
>> +       depends on COMMON_CLK_MT8195_VPPSYS
> 
> One other thing. If a Kconfig option immediately follows its dependency,
> then it gets indented nicely in menuconfig, but only if.
> If other options are interspersed, then the indentation gets reset.
> 
> So could you reorder the options to follow the dependency graph?
> 

Sure, I will!

> Also how you chose the dependencies should be mentioned in the commit log.
> These are pure run time dependencies, not compile time nor link/load ones.
> 

Right.

> Last, I think an argument could be made against the proliferation of
> Kconfig options, as it dramatically increases the combinations of
> allrandconfigs. Maybe Arnd (who IIRC frequently runs allrandconfig)
> could chime in on whether this is actually a concern or not.
> 

I understand, but I don't see any way around that.
In my opinion, we shall give flexibility, and this is the only way to achieve
that: if you don't use IMGSYS, CAMSYS, WPESYS and IPESYS you should *not* be
forced to add that to the mix, as this would result in a footprint increase
for no *final* practical reason.

It's true, today we have big storage capacities and fast machines, but we can
still see a reduction in boot times (bootloader kernel load time, other than
actual kernel boot time), even if minimal, with this added flexibility.

Save a few milliseconds here, a few milliseconds there (not necessarily on
clock drivers, expand this to others) and you start reaching a meaningful
increase in boot performance.

>> +       help
>> +         This driver supports MediaTek MT8195 camsys and camsys_raw clocks.
>> +
>> +config COMMON_CLK_MT8195_IMGSYS
>> +       tristate "Clock driver for MediaTek MT8195 imgsys"
>> +       depends on COMMON_CLK_MT8195_VPPSYS
>> +       help
>> +         This driver supports MediaTek MT8195 imgsys and imgsys2 clocks.
>> +
>> +config COMMON_CLK_MT8195_IMP_IIC_WRAP
>> +       tristate "Clock driver for MediaTek MT8195 imp_iic_wrap"
>> +       depends on COMMON_CLK_MT8195
>> +       help
>> +         This driver supports MediaTek MT8195 I2C/I3C clocks.
>> +
>> +config COMMON_CLK_MT8195_IPESYS
>> +       tristate "Clock driver for MediaTek MT8195 ipesys"
>> +       depends on COMMON_CLK_MT8195_IMGSYS
>> +       help
>> +         This driver supports MediaTek MT8195 ipesys clocks.
>> +
>> +config COMMON_CLK_MT8195_MFGCFG
>> +       tristate "Clock driver for MediaTek MT8195 mfgcfg"
>> +       depends on COMMON_CLK_MT8195
>> +       help
>> +         This driver supports MediaTek MT8195 mfgcfg clocks.
>> +
>> +config COMMON_CLK_MT8195_VDOSYS
>> +       tristate "Clock driver for MediaTek MT8195 vdosys"
>> +       depends on COMMON_CLK_MT8195
> 
> Not sure why this option is here, out of order?

My alphabet skills finally failed me, lol.
I'll fix that for v3 :-)

Thanks!
Angelo

On Tue, Feb 14, 2023 at 9:42 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> MT8195 clock drivers were encapsulated in one single (and big) Kconfig
> option: there's no reason to do that, as it is totally unnecessary to
> build in all or none of them.
>
> Split them out: keep boot-critical clocks as bool and allow choosing
> non critical clocks as tristate.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/clk/mediatek/Kconfig  | 86 +++++++++++++++++++++++++++++++++++
>  drivers/clk/mediatek/Makefile | 20 +++++---
>  2 files changed, 99 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
> index 45b7aea7648d..88937d111e98 100644
> --- a/drivers/clk/mediatek/Kconfig
> +++ b/drivers/clk/mediatek/Kconfig
> @@ -692,6 +692,92 @@ config COMMON_CLK_MT8195
>          help
>            This driver supports MediaTek MT8195 clocks.
>
> +config COMMON_CLK_MT8195_APUSYS
> +       tristate "Clock driver for MediaTek MT8195 apusys"
> +       depends on COMMON_CLK_MT8195

Would something like

          default COMMON_CLK_MT8195

help with the transition?

Otherwise we'd need to add a whole lot more stuff to arm64's defconfig,
and anyone running `make olddefconfig` would have many of their clock
drivers no longer available.

Same applies to the MT8186 split.

Seems like not all MediaTek SoCs apply this pattern, but at least MT7986,
MT8167, MT8173, MT8183 do this.

ChenYu

Il 17/02/23 05:31, Chen-Yu Tsai ha scritto:
> On Tue, Feb 14, 2023 at 9:42 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> MT8195 clock drivers were encapsulated in one single (and big) Kconfig
>> option: there's no reason to do that, as it is totally unnecessary to
>> build in all or none of them.
>>
>> Split them out: keep boot-critical clocks as bool and allow choosing
>> non critical clocks as tristate.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/clk/mediatek/Kconfig  | 86 +++++++++++++++++++++++++++++++++++
>>   drivers/clk/mediatek/Makefile | 20 +++++---
>>   2 files changed, 99 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
>> index 45b7aea7648d..88937d111e98 100644
>> --- a/drivers/clk/mediatek/Kconfig
>> +++ b/drivers/clk/mediatek/Kconfig
>> @@ -692,6 +692,92 @@ config COMMON_CLK_MT8195
>>           help
>>             This driver supports MediaTek MT8195 clocks.
>>
>> +config COMMON_CLK_MT8195_APUSYS
>> +       tristate "Clock driver for MediaTek MT8195 apusys"
>> +       depends on COMMON_CLK_MT8195
> 
> Would something like
> 
>            default COMMON_CLK_MT8195
> 
> help with the transition?
> 
> Otherwise we'd need to add a whole lot more stuff to arm64's defconfig,
> and anyone running `make olddefconfig` would have many of their clock
> drivers no longer available.
> 
> Same applies to the MT8186 split.
> 
> Seems like not all MediaTek SoCs apply this pattern, but at least MT7986,
> MT8167, MT8173, MT8183 do this.
> 
> ChenYu

Right. Since MT8195 machines have been out in the wild for a bit of time now,
I think it's worth following your advice and add `default COMMON_CLK_MT8195`
to the new configuration options.

As for MT8186, currently, there's only one board supported upstream, which
is the "unobtainable" EVB so I would rather not do that for MT8186, unless
you have any strong opinions on that.

Regards,
Angelo

On Fri, Feb 17, 2023 at 7:29 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 17/02/23 05:31, Chen-Yu Tsai ha scritto:
> > On Tue, Feb 14, 2023 at 9:42 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> >>
> >> MT8195 clock drivers were encapsulated in one single (and big) Kconfig
> >> option: there's no reason to do that, as it is totally unnecessary to
> >> build in all or none of them.
> >>
> >> Split them out: keep boot-critical clocks as bool and allow choosing
> >> non critical clocks as tristate.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >> ---
> >>   drivers/clk/mediatek/Kconfig  | 86 +++++++++++++++++++++++++++++++++++
> >>   drivers/clk/mediatek/Makefile | 20 +++++---
> >>   2 files changed, 99 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
> >> index 45b7aea7648d..88937d111e98 100644
> >> --- a/drivers/clk/mediatek/Kconfig
> >> +++ b/drivers/clk/mediatek/Kconfig
> >> @@ -692,6 +692,92 @@ config COMMON_CLK_MT8195
> >>           help
> >>             This driver supports MediaTek MT8195 clocks.
> >>
> >> +config COMMON_CLK_MT8195_APUSYS
> >> +       tristate "Clock driver for MediaTek MT8195 apusys"
> >> +       depends on COMMON_CLK_MT8195
> >
> > Would something like
> >
> >            default COMMON_CLK_MT8195
> >
> > help with the transition?
> >
> > Otherwise we'd need to add a whole lot more stuff to arm64's defconfig,
> > and anyone running `make olddefconfig` would have many of their clock
> > drivers no longer available.
> >
> > Same applies to the MT8186 split.
> >
> > Seems like not all MediaTek SoCs apply this pattern, but at least MT7986,
> > MT8167, MT8173, MT8183 do this.
> >
> > ChenYu
>
> Right. Since MT8195 machines have been out in the wild for a bit of time now,
> I think it's worth following your advice and add `default COMMON_CLK_MT8195`
> to the new configuration options.
>
> As for MT8186, currently, there's only one board supported upstream, which
> is the "unobtainable" EVB so I would rather not do that for MT8186, unless
> you have any strong opinions on that.

I'd like something that provides a reasonable working machine if possible.
If people need to customize to shrink their kernel, they could disable the
options later on. At least for ChromeOS we'll probably start off with them
all built-in, and later on maybe experiment a bit with modules.

ChenYu