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

[David Lechner]

On 12/11/25 11:55 AM, David Lechner wrote:
> 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?

It took me all day, but I learned some more. So some of what I wrote
below is wrong.

> 
> 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?

I see now that this driver is using mt8188_id_offs_map to correct
these issues, so the suggestion to rename to "PAD" is wrong. Don't
do that.

(Conceptually it made sense, but it doesn't match how the drivers
are implemented for other mediatek clocks already.)

> 
> 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?
> 

Since these values are devicetree ABI, we unfortunately can't really
fix them upstream. The problem is that the order matters, so we can't
insert the missing values in the correct position and change all of
the other numbers. We only get away with adding additional numbers
here because we have mt8188_id_offs_map to workaround the problem.

Most of these "missing" ones are only parent clocks of other clocks
so wouldn't be used in the devicetree anyway. (Maybe they were skipped
intentionally for that reason).


> 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"))
>>