Re: [PATCH v5 16/25] clk: mediatek: Add MT8189 apmixedsys clock support
From: Louis-Alexis Eyraud
Date: Fri Sep 04 2026 - 06:33:02 EST
Hi Brian,
On Mon, 2026-08-03 at 11:12 -0400, Brian Masney wrote:
> On Sat, Aug 01, 2026 at 01:21:02PM +0200, Louis-Alexis Eyraud wrote:
> > Add support for the MT8189 apmixedsys clock controller, which
> > provides
> > PLLs generated from SoC 26m.
> >
> > Co-developed-by: Irving-CH Lin <irving-ch.lin@xxxxxxxxxxxx>
> > Signed-off-by: Irving-CH Lin <irving-ch.lin@xxxxxxxxxxxx>
> > Co-developed-by: AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > Signed-off-by: AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > Signed-off-by: Louis-Alexis Eyraud
> > <louisalexis.eyraud@xxxxxxxxxxxxx>
> > ---
> > drivers/clk/mediatek/Kconfig | 13 ++
> > drivers/clk/mediatek/Makefile | 1 +
> > drivers/clk/mediatek/clk-mt8189-apmixedsys.c | 203
> > +++++++++++++++++++++++++++
> > 3 files changed, 217 insertions(+)
> >
> > diff --git a/drivers/clk/mediatek/Kconfig
> > b/drivers/clk/mediatek/Kconfig
> > index 2c09fd729bab..f67dfb6cd019 100644
> > --- a/drivers/clk/mediatek/Kconfig
> > +++ b/drivers/clk/mediatek/Kconfig
> > @@ -815,6 +815,19 @@ config COMMON_CLK_MT8188_WPESYS
> > help
> > This driver supports MediaTek MT8188 Warp Engine clocks.
> >
> > +config COMMON_CLK_MT8189
> > + tristate "Clock driver for MediaTek MT8189"
> > + depends on ARM64 || COMPILE_TEST
> > + select COMMON_CLK_MEDIATEK
> > + select COMMON_CLK_MEDIATEK_FHCTL
> > + default ARCH_MEDIATEK
> > + help
> > + Enable this option to support the clock management for
> > MediaTek MT8189 SoC. This
> > + includes handling of all primary clock functions and
> > features specific to the MT8189
> > + platform. Enabling this driver ensures that the system's
> > clock functionality aligns
> > + with the MediaTek MT8189 hardware capabilities,
> > providing efficient management of
> > + clock speeds and power consumption.
> > +
> > config COMMON_CLK_MT8192
> > tristate "Clock driver for MediaTek MT8192"
> > depends on ARM64 || COMPILE_TEST
> > diff --git a/drivers/clk/mediatek/Makefile
> > b/drivers/clk/mediatek/Makefile
> > index d8736a060dbd..66577ccb9b93 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -123,6 +123,7 @@ obj-$(CONFIG_COMMON_CLK_MT8188_VDOSYS) += clk-
> > mt8188-vdo0.o clk-mt8188-vdo1.o
> > obj-$(CONFIG_COMMON_CLK_MT8188_VENCSYS) += clk-mt8188-venc.o
> > obj-$(CONFIG_COMMON_CLK_MT8188_VPPSYS) += clk-mt8188-vpp0.o clk-
> > mt8188-vpp1.o
> > obj-$(CONFIG_COMMON_CLK_MT8188_WPESYS) += clk-mt8188-wpe.o
> > +obj-$(CONFIG_COMMON_CLK_MT8189) += clk-mt8189-apmixedsys.o
> > obj-$(CONFIG_COMMON_CLK_MT8192) += clk-mt8192-apmixedsys.o clk-
> > mt8192.o
> > obj-$(CONFIG_COMMON_CLK_MT8192_AUDSYS) += clk-mt8192-aud.o
> > obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS) += clk-mt8192-cam.o
> > diff --git a/drivers/clk/mediatek/clk-mt8189-apmixedsys.c
> > b/drivers/clk/mediatek/clk-mt8189-apmixedsys.c
> > new file mode 100644
> > index 000000000000..a50a8979b849
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mt8189-apmixedsys.c
> > @@ -0,0 +1,203 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2025-2026 MediaTek Inc.
> > + * Qiqi Wang <qiqi.wang@xxxxxxxxxxxx>
> > + * Irving-CH Lin <irving-ch.lin@xxxxxxxxxxxx>
> > + * Copyright (C) 2026 Collabora Ltd.
> > + * AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > + * Louis-Alexis Eyraud
> > <louisalexis.eyraud@xxxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "clk-fhctl.h"
> > +#include "clk-mtk.h"
> > +#include "clk-pll.h"
> > +
> > +#include <dt-bindings/clock/mediatek,mt8189-clk.h>
> > +
> > +#define MT8189_PLL_FMAX (3800UL * MHZ)
> > +#define MT8189_PLL_FMIN (1500UL * MHZ)
> > +#define MT8189_PLLEN_OFS 0x70
> > +#define MT8189_PLLEN_SET_OFS 0x74
> > +#define MT8189_PLLEN_CLR_OFS 0x78
> > +#define MT8189_RSTBAR_OFS 0x80
> > +#define MT8189_RSTBAR_SET_OFS 0x84
> > +#define MT8189_RSTBAR_CLR_OFS 0x88
> > +#define MT8189_INTEGER_BITS 8
> > +
> > +#define PLL_SETCLR(_id, _name, _reg, _pwr_reg,
> > _en_setclr_bit, \
> > + _rstb_setclr_bit, _flags, _pd_reg, \
> > + _pd_shift, _tuner_reg, _tuner_en_reg, \
> > + _tuner_en_bit, _pcw_reg, _pcw_shift, \
> > + _pcwbits) { \
> > + .id = _id, \
> > + .name = _name, \
> > + .en_reg = MT8189_PLLEN_OFS, \
> > + .en_set_reg = MT8189_PLLEN_SET_OFS, \
> > + .en_clr_reg = MT8189_PLLEN_CLR_OFS, \
> > + .reg = _reg, \
> > + .pwr_reg = _pwr_reg, \
> > + .pll_en_bit = _en_setclr_bit, \
> > + .rst_bar_reg = MT8189_RSTBAR_OFS, \
> > + .rst_bar_set_reg = MT8189_RSTBAR_SET_OFS, \
> > + .rst_bar_clr_reg = MT8189_RSTBAR_CLR_OFS, \
> > + .rst_bar_mask = BIT(_rstb_setclr_bit), \
> > + .flags = _flags, \
> > + .fmax = MT8189_PLL_FMAX, \
> > + .fmin = MT8189_PLL_FMIN, \
> > + .pd_reg = _pd_reg, \
> > + .pd_shift = _pd_shift, \
> > + .tuner_reg = _tuner_reg, \
> > + .tuner_en_reg = _tuner_en_reg, \
> > + .tuner_en_bit = _tuner_en_bit, \
> > + .pcw_reg = _pcw_reg, \
> > + .pcw_shift = _pcw_shift, \
> > + .pcwbits = _pcwbits, \
> > + .pcwibits = MT8189_INTEGER_BITS, \
> > + .ops = &mtk_pll_setclr_ops, \
> > + }
> > +
> > +static const struct mtk_pll_data apmixed_plls[] = {
> > + PLL_SETCLR(CLK_APMIXED_ARMPLL_LL, "armpll-ll", 0x204,
> > 0x210, 18,
> > + 0, PLL_AO, 0x208, 24, 0, 0, 0, 0x208, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_ARMPLL_BL, "armpll-bl", 0x214,
> > 0x220, 17,
> > + 0, PLL_AO, 0x218, 24, 0, 0, 0, 0x218, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_CCIPLL, "ccipll", 0x224, 0x230, 16,
> > + 0, PLL_AO, 0x228, 24, 0, 0, 0, 0x228, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_MAINPLL, "mainpll", 0x304, 0x310,
> > 15,
> > + 2, HAVE_RST_BAR | PLL_AO,
> > + 0x308, 24, 0, 0, 0, 0x308, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_UNIVPLL, "univpll", 0x314, 0x320,
> > 14,
> > + 1, HAVE_RST_BAR, 0x318, 24, 0, 0, 0, 0x318, 0,
> > 22),
> > + PLL_SETCLR(CLK_APMIXED_MMPLL, "mmpll", 0x324, 0x330, 13,
> > + 0, HAVE_RST_BAR, 0x328, 24, 0, 0, 0, 0x328, 0,
> > 22),
> > + PLL_SETCLR(CLK_APMIXED_MFGPLL, "mfgpll", 0x504, 0x510, 7,
> > + 0, 0, 0x508, 24, 0, 0, 0, 0x508, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_APLL1, "apll1", 0x404, 0x414, 11,
> > + 0, 0, 0x408, 24, 0x040, 0x00c, 0, 0x40c, 0,
> > 32),
> > + PLL_SETCLR(CLK_APMIXED_APLL2, "apll2", 0x418, 0x428, 10,
> > + 0, 0, 0x41c, 24, 0x044, 0x00c, 1, 0x420, 0,
> > 32),
> > + PLL_SETCLR(CLK_APMIXED_EMIPLL, "emipll", 0x334, 0x340, 12,
> > + 0, PLL_AO, 0x338, 24, 0, 0, 0, 0x338, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_APUPLL2, "apupll2", 0x614, 0x620,
> > 2,
> > + 0, 0, 0x618, 24, 0, 0, 0, 0x618, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_APUPLL, "apupll", 0x604, 0x610, 3,
> > + 0, 0, 0x608, 24, 0, 0, 0, 0x608, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_TVDPLL1, "tvdpll1", 0x42c, 0x438,
> > 9,
> > + 0, 0, 0x430, 24, 0, 0, 0, 0x430, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_TVDPLL2, "tvdpll2", 0x43c, 0x448,
> > 8,
> > + 0, 0, 0x440, 24, 0, 0, 0, 0x440, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_ETHPLL, "ethpll", 0x514, 0x520, 6,
> > + 0, 0, 0x518, 24, 0, 0, 0, 0x518, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_MSDCPLL, "msdcpll", 0x524, 0x530,
> > 5,
> > + 0, 0, 0x528, 24, 0, 0, 0, 0x528, 0, 22),
> > + PLL_SETCLR(CLK_APMIXED_UFSPLL, "ufspll", 0x534, 0x540, 4,
> > + 0, 0, 0x538, 24, 0, 0, 0, 0x538, 0, 22),
> > +};
> > +
> > +#define FH(_pllid, _fhid, _offset) { \
> > + .data = { \
> > + .pll_id = _pllid, \
> > + .fh_id =
> > _fhid, \
> > + .fh_ver = FHCTL_PLLFH_V2, \
> > + .fhx_offset = _offset, \
> > + .dds_mask = GENMASK(21, 0), \
> > + .slope0_value = 0x6003c97, \
> > + .slope1_value = 0x6003c97, \
> > + .sfstrx_en = BIT(2), \
> > + .frddsx_en = BIT(1), \
> > + .fhctlx_en = BIT(0), \
> > + .tgl_org = BIT(31), \
> > + .dvfs_tri = BIT(31), \
> > + .pcwchg = BIT(31), \
> > + .dt_val = 0x0, \
> > + .df_val = 0x9, \
> > + .updnlmt_shft = 16, \
> > + .msk_frddsx_dys = GENMASK(23, 20), \
> > + .msk_frddsx_dts = GENMASK(19, 16), \
> > + .ops = &mtk_pllfh_setclr_ops, \
> > + }, \
> > + }
> > +
> > +static struct mtk_pllfh_data pllfhs[] = {
> > + FH(CLK_APMIXED_ARMPLL_LL, 0, 0x003C),
> > + FH(CLK_APMIXED_ARMPLL_BL, 1, 0x0050),
> > + FH(CLK_APMIXED_CCIPLL, 2, 0x0064),
> > + FH(CLK_APMIXED_MAINPLL, 3, 0x0078),
> > + FH(CLK_APMIXED_MMPLL, 4, 0x008C),
> > + FH(CLK_APMIXED_MFGPLL, 5, 0x00A0),
> > + FH(CLK_APMIXED_EMIPLL, 6, 0x00B4),
> > + FH(CLK_APMIXED_TVDPLL1, 7, 0x00C8),
> > + FH(CLK_APMIXED_TVDPLL2, 8, 0x00DC),
> > + FH(CLK_APMIXED_MSDCPLL, 9, 0x00F0),
> > + FH(CLK_APMIXED_UFSPLL, 10, 0x0104),
> > + FH(CLK_APMIXED_APUPLL, 11, 0x0118),
> > + FH(CLK_APMIXED_APUPLL2, 12, 0x012c),
> > +};
> > +
> > +static const struct of_device_id of_match_clk_mt8189_apmixed[] = {
> > + { .compatible = "mediatek,mt8189-apmixedsys" },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, of_match_clk_mt8189_apmixed);
> > +
> > +static int clk_mt8189_apmixed_probe(struct platform_device *pdev)
> > +{
> > + const u8 *fhctl_node = "mediatek,mt8189-fhctl";
> > + struct device_node *node = pdev->dev.of_node;
> > + struct clk_hw_onecell_data *clk_data;
> > + int r;
> > +
> > + clk_data = mtk_alloc_clk_data(ARRAY_SIZE(apmixed_plls));
> > + if (!clk_data)
> > + return -ENOMEM;
> > +
> > + fhctl_parse_dt(fhctl_node, pllfhs, ARRAY_SIZE(pllfhs));
>
> So I know this is how the existing drivers are written. This calls
> of_iomap(), and the iounmap is called by mtk_clk_unregister_pllfhs().
> I usually like to see things in pairs to make it easier to review
> where there's some register, and an unregister with the same name.
>
> Could the fhctl_parse_dt() call be moved into
> mtk_clk_register_pllfhs() ?
>
Thanks for the v5 patches review first.
It is indeed a good idea to move this function in
mtk_clk_register_pllfhs. The needed modifications are rather small
(only 5 calls in the existing mediatek clk drivers) and would make this
function private to clk-pllfh.c as well. It is also complementary to
the other cleanup series, that is reworking the probe functions of
apmixedsys drivers with pllfh support ([1]).
I'll add a new patch in v6 along the other fixes, and adapt this one.
[1]:
https://lore.kernel.org/linux-mediatek/20260803141659.559129-13-akkun11.open@xxxxxxxxx/
> Additionally fhctl_parse_dt() sets some state variables that are
> never
> cleared in the unregister:
>
> pllfh->state.fh_enable = 1;
> pllfh->state.ssc_rate = ssc_rate;
> pllfh->state.base = base;
>
> Just a quick glance though I think this is fine.
>
They are indeed never cleared.
Since they are stored in a static variable, it would be best to
clean them anyway, for instance in mtk_clk_cleanup_pllfhs function,
that used in both unregister and register (in error path) functions.
I'll also add a new patch in for that.
Regards,
Louis-Alexis
> Brian
>