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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

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?

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

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

> +
> +/* apmixedsys */
> +#define PLL(_id, _reg, _pwr_reg, _en_mask, _flags, _pcwbits, _pd_reg,	\
> +	    _pd_shift, _pcw_reg, _pcw_shift) {				\
> +		.id = _id,						\
> +		.reg = _reg,						\
> +		.pwr_reg = _pwr_reg,					\
> +		.en_mask = _en_mask,					\
> +		.rst_bar_mask = BIT(23),				\
> +		.fmin = MT8188_PLL_FMIN,				\
> +		.fmax = MT8188_PLL_FMAX,				\
> +		.flags = _flags,					\
> +		.pcwbits = _pcwbits,					\
> +		.pcwibits = 8,						\
> +		.pd_reg = _pd_reg,					\
> +		.pd_shift = _pd_shift,					\
> +		.pcw_reg = _pcw_reg,					\
> +		.pcw_shift = _pcw_shift,				\
> +	}
> +