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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi David,

On Thu, Dec 11, 2025 at 22:22, David Lechner <dlechner@baylibre.com> wrote:

> 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@baylibre.com> wrote:
>>>
>>>> From: Julien Masson <jmasson@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@baylibre.com>
>>>> Signed-off-by: Julien Stephan <jstephan@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@baylibre.com>
>>>> + *          Garmin Chang <garmin.chang@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.

Thank you for investigating. This is very helpful.

I'd say that we should update the above comment to include something
like you wrote below:

/*
 * Missing topckgen clocks definition in dt-bindings
 * Note: we can't add these to the upstream bindings without
 * breaking the ABI so add them here instead.
 */

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

Ack.

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

That is a little unfortunate but I understand.

Adding a more detailed comment next to the additional clock definitions
would be enough then. (In my opinion)


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