[PATCH v2 2/2] clk: mediatek: add MT8188 clock driver

[David Lechner]

On 12/11/25 2:39 AM, Mattijs Korpershoek wrote:
> Hi Julien,
> 
> Thank you for the patch.
> 
> On Tue, Dec 09, 2025 at 11:22, Julien Stephan <jstephan at baylibre.com> wrote:
> 
>> From: Julien Masson <jmasson at baylibre.com>
>>
>> The following clocks have been added for MT8188 SoC:
>> apmixedsys, topckgen, infracfg, pericfg and imp_iic_wrap
>>
>> These clocks driver are based on the ones present in the kernel:
>> drivers/clk/mediatek/clk-mt8188-*
>>
>> Signed-off-by: Julien Masson <jmasson at baylibre.com>
>> Signed-off-by: Julien Stephan <jstephan at baylibre.com>
>> ---
>>  drivers/clk/mediatek/Makefile     |    1 +
>>  drivers/clk/mediatek/clk-mt8188.c | 1840 +++++++++++++++++++++++++++++++++++++
>>  2 files changed, 1841 insertions(+)
>>
>> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
>> index 12893687b68fc6c136a06e19305b1dd0c8a8101a..68b3d6e9610d8e7f4c4c625f52e525174e92787a 100644
>> --- a/drivers/clk/mediatek/Makefile
>> +++ b/drivers/clk/mediatek/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_TARGET_MT7981) += clk-mt7981.o
>>  obj-$(CONFIG_TARGET_MT7988) += clk-mt7988.o
>>  obj-$(CONFIG_TARGET_MT7987) += clk-mt7987.o
>>  obj-$(CONFIG_TARGET_MT8183) += clk-mt8183.o
>> +obj-$(CONFIG_TARGET_MT8188) += clk-mt8188.o
>>  obj-$(CONFIG_TARGET_MT8365) += clk-mt8365.o
>>  obj-$(CONFIG_TARGET_MT8512) += clk-mt8512.o
>>  obj-$(CONFIG_TARGET_MT8516) += clk-mt8516.o
>> diff --git a/drivers/clk/mediatek/clk-mt8188.c b/drivers/clk/mediatek/clk-mt8188.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..55dfadddfe3cf743602533de30275bc93d4f15a5
>> --- /dev/null
>> +++ b/drivers/clk/mediatek/clk-mt8188.c
>> @@ -0,0 +1,1840 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * MediaTek clock driver for MT8188 SoC
>> + *
>> + * Copyright (C) 2025 BayLibre, SAS
>> + * Copyright (c) 2025 MediaTek Inc.
>> + * Authors: Julien Masson <jmasson at baylibre.com>
>> + *          Garmin Chang <garmin.chang at mediatek.com>
>> + */
>> +
>> +#include <clk-uclass.h>
>> +#include <dm/device_compat.h>
>> +#include <dm.h>
>> +#include <asm/io.h>
>> +#include <dt-bindings/clock/mediatek,mt8188-clk.h>
>> +
>> +#include "clk-mtk.h"
>> +
>> +#define MT8188_PLL_FMAX		(3800UL * MHZ)
>> +#define MT8188_PLL_FMIN		(1500UL * MHZ)
>> +
>> +/* Missing topckgen clocks definition in dt-bindings */
>> +#define CLK_TOP_ADSPPLL		206
>> +#define CLK_TOP_CLK13M		207
>> +#define CLK_TOP_CLK26M		208
>> +#define CLK_TOP_CLK32K		209
>> +#define CLK_TOP_IMGPLL		210
>> +#define CLK_TOP_MSDCPLL		211
>> +#define CLK_TOP_ULPOSC1_CK1	212
>> +#define CLK_TOP_ULPOSC_CK1	213
> 
> Why are these clock definitions missing from the dt-bindings?
> Were they just forgotten, or is there another reason?

I was looking at mt8365 clocks yesterday and noticed a similar pattern.
So I am interested in getting feedback on this too. And I can give at
least a partial answer.

Root clocks
-----------

The three CLK_TOP_CLKXXX clocks are in the devicetree as "fixed-clock"
with labels "clkXXx".

These should go in a separate group named "PAD" since they aren't
part of the TOPCKGEN group. And it would make sense to make the
macros CLK_PAD_CLKXXX.

Unless we should be reading these from devicetree somehow instead?

PLL clocks
----------

This is just a guess, but I suspect in Linux, since CLK_TOP_ADSPPL is
just a 1:1 divider clock of CLK_APMIXED_ADSPPLL, they took the shortcut
of leaving out CLK_TOP_ADSPPL from the clock tree and set the parent
of CLK_TOP_ADSPPLL_Dx to CLK_APMIXED_ADSPPLL instead of CLK_TOP_ADSPPLL.

The actual full picture should be like this:

	77, /* CLK_TOP_ADSPPLL */

	...

	FACTOR0(CLK_TOP_ADSPPL, CLK_APMIXED_ADSPPLL, 1, 1),
	FACTOR0(CLK_TOP_ADSPPLL_D2, CLK_TOP_ADSPPL, 1, 2),
	FACTOR0(CLK_TOP_ADSPPLL_D4, CLK_TOP_ADSPPL, 1, 4),
	FACTOR0(CLK_TOP_ADSPPLL_D8, CLK_TOP_ADSPPL, 1, 8),

Instead of:
	
	-1, /* CLK_TOP_ADSPPLL */

	...

	FACTOR0(CLK_TOP_ADSPPLL_D2, CLK_APMIXED_ADSPPLL, 1, 2),
	FACTOR0(CLK_TOP_ADSPPLL_D4, CLK_APMIXED_ADSPPLL, 1, 4),
	FACTOR0(CLK_TOP_ADSPPLL_D8, CLK_APMIXED_ADSPPLL, 1, 8),

So we could either follow Linux and use CLK_APMIXED_ADSPPLL everywhere
instead of adding CLK_TOP_ADSPPL. Or we could be more correct and add
the missing clocks.

The other PLL clocks seem to follow a similar pattern. So whatever we
do for ADSPPLL would apply to the others.

CK1 clocks
----------

I can't see why these would be different from the same name without the
_CK1 suffix. There is already CLK_TOP_ULPOSC and CLK_TOP_ULPOSC1 and they
seem like they should be the same clocks.

Perhaps we could not add these?

> 
> Could we (long term) add these definitions to the dt-bindings by
> contributing them to the kernel?

I guess it depends on if taking these PLL shortcuts was intentional or
not if upstream would want to add them or not. But clearly the PAD clocks
are fine they way they are upstream.

> 
> Note: I'm not requesting to change this patch, I'm just curious as of
> why we need to add these definitions here.
> 
> Note that I also don't see these CLKs in the linux driver so why are
> they needed for U-Boot ?

I have a feeling someone will be asking me the same question soon. :-)

> (I searched for CLK_TOP_CLK13M in Linux master commit e7c375b18160 ("Merge tag 'vfs-6.18-rc7.fixes' of gitolite.kernel.org:pub/scm/linux/kernel/git/vfs/vfs"))
>