1. Patchew
  2. linux
  3. [PATCH v5 00/27] Add support for MT8196 clock controllers
  4. View patch

[PATCH v5 19/27] clk: mediatek: Add MT8196 mdpsys clock support

Laura Nao posted 27 patches 1 month ago
There is a newer version of this series
[PATCH v5 19/27] clk: mediatek: Add MT8196 mdpsys clock support
Posted by Laura Nao 1 month ago 
Add support for the MT8196 mdpsys clock controller, which provides clock
gate control for MDP.

Signed-off-by: Laura Nao <laura.nao@collabora.com>
---
 drivers/clk/mediatek/Kconfig             |   7 +
 drivers/clk/mediatek/Makefile            |   1 +
 drivers/clk/mediatek/clk-mt8196-mdpsys.c | 186 +++++++++++++++++++++++
 3 files changed, 194 insertions(+)
 create mode 100644 drivers/clk/mediatek/clk-mt8196-mdpsys.c

diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
index 8e5cdae80748..68ac08cf8e82 100644
--- a/drivers/clk/mediatek/Kconfig
+++ b/drivers/clk/mediatek/Kconfig
@@ -1024,6 +1024,13 @@ config COMMON_CLK_MT8196_MCUSYS
 	help
 	  This driver supports MediaTek MT8196 mcusys clocks.
 
+config COMMON_CLK_MT8196_MDPSYS
+	tristate "Clock driver for MediaTek MT8196 mdpsys"
+	depends on COMMON_CLK_MT8196
+	default COMMON_CLK_MT8196
+	help
+	  This driver supports MediaTek MT8196 mdpsys clocks.
+
 config COMMON_CLK_MT8196_PEXTPSYS
 	tristate "Clock driver for MediaTek MT8196 pextpsys"
 	depends on COMMON_CLK_MT8196
diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
index 46358623c3e5..d2d8bc43e45b 100644
--- a/drivers/clk/mediatek/Makefile
+++ b/drivers/clk/mediatek/Makefile
@@ -155,6 +155,7 @@ obj-$(CONFIG_COMMON_CLK_MT8196) += clk-mt8196-apmixedsys.o clk-mt8196-topckgen.o
 				   clk-mt8196-peri_ao.o
 obj-$(CONFIG_COMMON_CLK_MT8196_IMP_IIC_WRAP) += clk-mt8196-imp_iic_wrap.o
 obj-$(CONFIG_COMMON_CLK_MT8196_MCUSYS) += clk-mt8196-mcu.o
+obj-$(CONFIG_COMMON_CLK_MT8196_MDPSYS) += clk-mt8196-mdpsys.o
 obj-$(CONFIG_COMMON_CLK_MT8196_PEXTPSYS) += clk-mt8196-pextp.o
 obj-$(CONFIG_COMMON_CLK_MT8196_UFSSYS) += clk-mt8196-ufs_ao.o
 obj-$(CONFIG_COMMON_CLK_MT8365) += clk-mt8365-apmixedsys.o clk-mt8365.o
diff --git a/drivers/clk/mediatek/clk-mt8196-mdpsys.c b/drivers/clk/mediatek/clk-mt8196-mdpsys.c
new file mode 100644
index 000000000000..a46b1627f1f3
--- /dev/null
+++ b/drivers/clk/mediatek/clk-mt8196-mdpsys.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2025 MediaTek Inc.
+ *                    Guangjie Song <guangjie.song@mediatek.com>
+ * Copyright (c) 2025 Collabora Ltd.
+ *                    Laura Nao <laura.nao@collabora.com>
+ */
+#include <dt-bindings/clock/mediatek,mt8196-clock.h>
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include "clk-gate.h"
+#include "clk-mtk.h"
+
+static const struct mtk_gate_regs mdp0_cg_regs = {
+	.set_ofs = 0x104,
+	.clr_ofs = 0x108,
+	.sta_ofs = 0x100,
+};
+
+static const struct mtk_gate_regs mdp1_cg_regs = {
+	.set_ofs = 0x114,
+	.clr_ofs = 0x118,
+	.sta_ofs = 0x110,
+};
+
+static const struct mtk_gate_regs mdp2_cg_regs = {
+	.set_ofs = 0x124,
+	.clr_ofs = 0x128,
+	.sta_ofs = 0x120,
+};
+
+#define GATE_MDP0(_id, _name, _parent, _shift) {	\
+		.id = _id,				\
+		.name = _name,				\
+		.parent_name = _parent,			\
+		.regs = &mdp0_cg_regs,			\
+		.shift = _shift,			\
+		.flags = CLK_OPS_PARENT_ENABLE,		\
+		.ops = &mtk_clk_gate_ops_setclr,	\
+	}
+
+#define GATE_MDP1(_id, _name, _parent, _shift) {	\
+		.id = _id,				\
+		.name = _name,				\
+		.parent_name = _parent,			\
+		.regs = &mdp1_cg_regs,			\
+		.shift = _shift,			\
+		.ops = &mtk_clk_gate_ops_setclr,	\
+	}
+
+#define GATE_MDP2(_id, _name, _parent, _shift) {	\
+		.id = _id,				\
+		.name = _name,				\
+		.parent_name = _parent,			\
+		.regs = &mdp2_cg_regs,			\
+		.shift = _shift,			\
+		.flags = CLK_OPS_PARENT_ENABLE,		\
+		.ops = &mtk_clk_gate_ops_setclr,	\
+	}
+
+static const struct mtk_gate mdp1_clks[] = {
+	/* MDP1-0 */
+	GATE_MDP0(CLK_MDP1_MDP_MUTEX0, "mdp1_mdp_mutex0", "mdp", 0),
+	GATE_MDP0(CLK_MDP1_SMI0, "mdp1_smi0", "mdp", 1),
+	GATE_MDP0(CLK_MDP1_APB_BUS, "mdp1_apb_bus", "mdp", 2),
+	GATE_MDP0(CLK_MDP1_MDP_RDMA0, "mdp1_mdp_rdma0", "mdp", 3),
+	GATE_MDP0(CLK_MDP1_MDP_RDMA1, "mdp1_mdp_rdma1", "mdp", 4),
+	GATE_MDP0(CLK_MDP1_MDP_RDMA2, "mdp1_mdp_rdma2", "mdp", 5),
+	GATE_MDP0(CLK_MDP1_MDP_BIRSZ0, "mdp1_mdp_birsz0", "mdp", 6),
+	GATE_MDP0(CLK_MDP1_MDP_HDR0, "mdp1_mdp_hdr0", "mdp", 7),
+	GATE_MDP0(CLK_MDP1_MDP_AAL0, "mdp1_mdp_aal0", "mdp", 8),
+	GATE_MDP0(CLK_MDP1_MDP_RSZ0, "mdp1_mdp_rsz0", "mdp", 9),
+	GATE_MDP0(CLK_MDP1_MDP_RSZ2, "mdp1_mdp_rsz2", "mdp", 10),
+	GATE_MDP0(CLK_MDP1_MDP_TDSHP0, "mdp1_mdp_tdshp0", "mdp", 11),
+	GATE_MDP0(CLK_MDP1_MDP_COLOR0, "mdp1_mdp_color0", "mdp", 12),
+	GATE_MDP0(CLK_MDP1_MDP_WROT0, "mdp1_mdp_wrot0", "mdp", 13),
+	GATE_MDP0(CLK_MDP1_MDP_WROT1, "mdp1_mdp_wrot1", "mdp", 14),
+	GATE_MDP0(CLK_MDP1_MDP_WROT2, "mdp1_mdp_wrot2", "mdp", 15),
+	GATE_MDP0(CLK_MDP1_MDP_FAKE_ENG0, "mdp1_mdp_fake_eng0", "mdp", 16),
+	GATE_MDP0(CLK_MDP1_APB_DB, "mdp1_apb_db", "mdp", 17),
+	GATE_MDP0(CLK_MDP1_MDP_DLI_ASYNC0, "mdp1_mdp_dli_async0", "mdp", 18),
+	GATE_MDP0(CLK_MDP1_MDP_DLI_ASYNC1, "mdp1_mdp_dli_async1", "mdp", 19),
+	GATE_MDP0(CLK_MDP1_MDP_DLO_ASYNC0, "mdp1_mdp_dlo_async0", "mdp", 20),
+	GATE_MDP0(CLK_MDP1_MDP_DLO_ASYNC1, "mdp1_mdp_dlo_async1", "mdp", 21),
+	GATE_MDP0(CLK_MDP1_MDP_DLI_ASYNC2, "mdp1_mdp_dli_async2", "mdp", 22),
+	GATE_MDP0(CLK_MDP1_MDP_DLO_ASYNC2, "mdp1_mdp_dlo_async2", "mdp", 23),
+	GATE_MDP0(CLK_MDP1_MDP_DLO_ASYNC3, "mdp1_mdp_dlo_async3", "mdp", 24),
+	GATE_MDP0(CLK_MDP1_IMG_DL_ASYNC0, "mdp1_img_dl_async0", "mdp", 25),
+	GATE_MDP0(CLK_MDP1_MDP_RROT0, "mdp1_mdp_rrot0", "mdp", 26),
+	GATE_MDP0(CLK_MDP1_MDP_MERGE0, "mdp1_mdp_merge0", "mdp", 27),
+	GATE_MDP0(CLK_MDP1_MDP_C3D0, "mdp1_mdp_c3d0", "mdp", 28),
+	GATE_MDP0(CLK_MDP1_MDP_FG0, "mdp1_mdp_fg0", "mdp", 29),
+	GATE_MDP0(CLK_MDP1_MDP_CLA2, "mdp1_mdp_cla2", "mdp", 30),
+	GATE_MDP0(CLK_MDP1_MDP_DLO_ASYNC4, "mdp1_mdp_dlo_async4", "mdp", 31),
+	/* MDP1-1 */
+	GATE_MDP1(CLK_MDP1_VPP_RSZ0, "mdp1_vpp_rsz0", "mdp", 0),
+	GATE_MDP1(CLK_MDP1_VPP_RSZ1, "mdp1_vpp_rsz1", "mdp", 1),
+	GATE_MDP1(CLK_MDP1_MDP_DLO_ASYNC5, "mdp1_mdp_dlo_async5", "mdp", 2),
+	GATE_MDP1(CLK_MDP1_IMG0, "mdp1_img0", "mdp", 3),
+	GATE_MDP1(CLK_MDP1_F26M, "mdp1_f26m", "clk26m", 27),
+	/* MDP1-2 */
+	GATE_MDP2(CLK_MDP1_IMG_DL_RELAY0, "mdp1_img_dl_relay0", "mdp", 0),
+	GATE_MDP2(CLK_MDP1_IMG_DL_RELAY1, "mdp1_img_dl_relay1", "mdp", 8),
+};
+
+static const struct mtk_clk_desc mdp1_mcd = {
+	.clks = mdp1_clks,
+	.num_clks = ARRAY_SIZE(mdp1_clks),
+	.need_runtime_pm = true,
+};
+
+
+static const struct mtk_gate mdp_clks[] = {
+	/* MDP0 */
+	GATE_MDP0(CLK_MDP_MDP_MUTEX0, "mdp_mdp_mutex0", "mdp", 0),
+	GATE_MDP0(CLK_MDP_SMI0, "mdp_smi0", "mdp", 1),
+	GATE_MDP0(CLK_MDP_APB_BUS, "mdp_apb_bus", "mdp", 2),
+	GATE_MDP0(CLK_MDP_MDP_RDMA0, "mdp_mdp_rdma0", "mdp", 3),
+	GATE_MDP0(CLK_MDP_MDP_RDMA1, "mdp_mdp_rdma1", "mdp", 4),
+	GATE_MDP0(CLK_MDP_MDP_RDMA2, "mdp_mdp_rdma2", "mdp", 5),
+	GATE_MDP0(CLK_MDP_MDP_BIRSZ0, "mdp_mdp_birsz0", "mdp", 6),
+	GATE_MDP0(CLK_MDP_MDP_HDR0, "mdp_mdp_hdr0", "mdp", 7),
+	GATE_MDP0(CLK_MDP_MDP_AAL0, "mdp_mdp_aal0", "mdp", 8),
+	GATE_MDP0(CLK_MDP_MDP_RSZ0, "mdp_mdp_rsz0", "mdp", 9),
+	GATE_MDP0(CLK_MDP_MDP_RSZ2, "mdp_mdp_rsz2", "mdp", 10),
+	GATE_MDP0(CLK_MDP_MDP_TDSHP0, "mdp_mdp_tdshp0", "mdp", 11),
+	GATE_MDP0(CLK_MDP_MDP_COLOR0, "mdp_mdp_color0", "mdp", 12),
+	GATE_MDP0(CLK_MDP_MDP_WROT0, "mdp_mdp_wrot0", "mdp", 13),
+	GATE_MDP0(CLK_MDP_MDP_WROT1, "mdp_mdp_wrot1", "mdp", 14),
+	GATE_MDP0(CLK_MDP_MDP_WROT2, "mdp_mdp_wrot2", "mdp", 15),
+	GATE_MDP0(CLK_MDP_MDP_FAKE_ENG0, "mdp_mdp_fake_eng0", "mdp", 16),
+	GATE_MDP0(CLK_MDP_APB_DB, "mdp_apb_db", "mdp", 17),
+	GATE_MDP0(CLK_MDP_MDP_DLI_ASYNC0, "mdp_mdp_dli_async0", "mdp", 18),
+	GATE_MDP0(CLK_MDP_MDP_DLI_ASYNC1, "mdp_mdp_dli_async1", "mdp", 19),
+	GATE_MDP0(CLK_MDP_MDP_DLO_ASYNC0, "mdp_mdp_dlo_async0", "mdp", 20),
+	GATE_MDP0(CLK_MDP_MDP_DLO_ASYNC1, "mdp_mdp_dlo_async1", "mdp", 21),
+	GATE_MDP0(CLK_MDP_MDP_DLI_ASYNC2, "mdp_mdp_dli_async2", "mdp", 22),
+	GATE_MDP0(CLK_MDP_MDP_DLO_ASYNC2, "mdp_mdp_dlo_async2", "mdp", 23),
+	GATE_MDP0(CLK_MDP_MDP_DLO_ASYNC3, "mdp_mdp_dlo_async3", "mdp", 24),
+	GATE_MDP0(CLK_MDP_IMG_DL_ASYNC0, "mdp_img_dl_async0", "mdp", 25),
+	GATE_MDP0(CLK_MDP_MDP_RROT0, "mdp_mdp_rrot0", "mdp", 26),
+	GATE_MDP0(CLK_MDP_MDP_MERGE0, "mdp_mdp_merge0", "mdp", 27),
+	GATE_MDP0(CLK_MDP_MDP_C3D0, "mdp_mdp_c3d0", "mdp", 28),
+	GATE_MDP0(CLK_MDP_MDP_FG0, "mdp_mdp_fg0", "mdp", 29),
+	GATE_MDP0(CLK_MDP_MDP_CLA2, "mdp_mdp_cla2", "mdp", 30),
+	GATE_MDP0(CLK_MDP_MDP_DLO_ASYNC4, "mdp_mdp_dlo_async4", "mdp", 31),
+	/* MDP1 */
+	GATE_MDP1(CLK_MDP_VPP_RSZ0, "mdp_vpp_rsz0", "mdp", 0),
+	GATE_MDP1(CLK_MDP_VPP_RSZ1, "mdp_vpp_rsz1", "mdp", 1),
+	GATE_MDP1(CLK_MDP_MDP_DLO_ASYNC5, "mdp_mdp_dlo_async5", "mdp", 2),
+	GATE_MDP1(CLK_MDP_IMG0, "mdp_img0", "mdp", 3),
+	GATE_MDP1(CLK_MDP_F26M, "mdp_f26m", "clk26m", 27),
+	/* MDP2 */
+	GATE_MDP2(CLK_MDP_IMG_DL_RELAY0, "mdp_img_dl_relay0", "mdp", 0),
+	GATE_MDP2(CLK_MDP_IMG_DL_RELAY1, "mdp_img_dl_relay1", "mdp", 8),
+};
+
+static const struct mtk_clk_desc mdp_mcd = {
+	.clks = mdp_clks,
+	.num_clks = ARRAY_SIZE(mdp_clks),
+	.need_runtime_pm = true,
+};
+
+static const struct of_device_id of_match_clk_mt8196_mdpsys[] = {
+	{ .compatible = "mediatek,mt8196-mdpsys1", .data = &mdp1_mcd },
+	{ .compatible = "mediatek,mt8196-mdpsys0", .data = &mdp_mcd },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_mdpsys);
+
+static struct platform_driver clk_mt8196_mdpsys_drv = {
+	.probe = mtk_clk_simple_probe,
+	.remove = mtk_clk_simple_remove,
+	.driver = {
+		.name = "clk-mt8196-mdpsys",
+		.of_match_table = of_match_clk_mt8196_mdpsys,
+	},
+};
+module_platform_driver(clk_mt8196_mdpsys_drv);
+
+MODULE_DESCRIPTION("MediaTek MT8196 Multimedia Data Path clocks driver");
+MODULE_LICENSE("GPL");
-- 
2.39.5
Re: [PATCH v5 19/27] clk: mediatek: Add MT8196 mdpsys clock support
Posted by AngeloGioacchino Del Regno 4 weeks ago 
Il 29/08/25 11:19, Laura Nao ha scritto:
> Add support for the MT8196 mdpsys clock controller, which provides clock
> gate control for MDP.
> 
> Signed-off-by: Laura Nao <laura.nao@collabora.com>
> ---
>   drivers/clk/mediatek/Kconfig             |   7 +
>   drivers/clk/mediatek/Makefile            |   1 +
>   drivers/clk/mediatek/clk-mt8196-mdpsys.c | 186 +++++++++++++++++++++++
>   3 files changed, 194 insertions(+)
>   create mode 100644 drivers/clk/mediatek/clk-mt8196-mdpsys.c
> 
> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
> index 8e5cdae80748..68ac08cf8e82 100644
> --- a/drivers/clk/mediatek/Kconfig
> +++ b/drivers/clk/mediatek/Kconfig
> @@ -1024,6 +1024,13 @@ config COMMON_CLK_MT8196_MCUSYS
>   	help
>   	  This driver supports MediaTek MT8196 mcusys clocks.
>   
> +config COMMON_CLK_MT8196_MDPSYS
> +	tristate "Clock driver for MediaTek MT8196 mdpsys"
> +	depends on COMMON_CLK_MT8196
> +	default COMMON_CLK_MT8196
> +	help
> +	  This driver supports MediaTek MT8196 mdpsys clocks.
> +
>   config COMMON_CLK_MT8196_PEXTPSYS
>   	tristate "Clock driver for MediaTek MT8196 pextpsys"
>   	depends on COMMON_CLK_MT8196
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index 46358623c3e5..d2d8bc43e45b 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -155,6 +155,7 @@ obj-$(CONFIG_COMMON_CLK_MT8196) += clk-mt8196-apmixedsys.o clk-mt8196-topckgen.o
>   				   clk-mt8196-peri_ao.o
>   obj-$(CONFIG_COMMON_CLK_MT8196_IMP_IIC_WRAP) += clk-mt8196-imp_iic_wrap.o
>   obj-$(CONFIG_COMMON_CLK_MT8196_MCUSYS) += clk-mt8196-mcu.o
> +obj-$(CONFIG_COMMON_CLK_MT8196_MDPSYS) += clk-mt8196-mdpsys.o
>   obj-$(CONFIG_COMMON_CLK_MT8196_PEXTPSYS) += clk-mt8196-pextp.o
>   obj-$(CONFIG_COMMON_CLK_MT8196_UFSSYS) += clk-mt8196-ufs_ao.o
>   obj-$(CONFIG_COMMON_CLK_MT8365) += clk-mt8365-apmixedsys.o clk-mt8365.o
> diff --git a/drivers/clk/mediatek/clk-mt8196-mdpsys.c b/drivers/clk/mediatek/clk-mt8196-mdpsys.c
> new file mode 100644
> index 000000000000..a46b1627f1f3
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8196-mdpsys.c
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 MediaTek Inc.
> + *                    Guangjie Song <guangjie.song@mediatek.com>
> + * Copyright (c) 2025 Collabora Ltd.
> + *                    Laura Nao <laura.nao@collabora.com>
> + */
> +#include <dt-bindings/clock/mediatek,mt8196-clock.h>
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +#include "clk-gate.h"
> +#include "clk-mtk.h"
> +
> +static const struct mtk_gate_regs mdp0_cg_regs = {
> +	.set_ofs = 0x104,
> +	.clr_ofs = 0x108,
> +	.sta_ofs = 0x100,
> +};
> +
> +static const struct mtk_gate_regs mdp1_cg_regs = {
> +	.set_ofs = 0x114,
> +	.clr_ofs = 0x118,
> +	.sta_ofs = 0x110,
> +};
> +
> +static const struct mtk_gate_regs mdp2_cg_regs = {
> +	.set_ofs = 0x124,
> +	.clr_ofs = 0x128,
> +	.sta_ofs = 0x120,
> +};
> +
> +#define GATE_MDP0(_id, _name, _parent, _shift) {	\
> +		.id = _id,				\
> +		.name = _name,				\
> +		.parent_name = _parent,			\
> +		.regs = &mdp0_cg_regs,			\
> +		.shift = _shift,			\
> +		.flags = CLK_OPS_PARENT_ENABLE,		\

Why would MDP0 and MDP2 be different, as in why would MDP1 be so special to not
need CLK_OPS_PARENT_ENABLE while the others do?

Either they all do, or they all don't.

I guess they all don't, but I'm not sure how you tested that at all, since the
only way to test this is downstream (and upstream will very likely be different
from that).

Even though I think they don't need that - please add back CLK_OPS_PARENT_ENABLE
to GATE_MDP1 to be safe, as in (all) MediaTek SoCs the multimedia subsystem is
kinda separate from the rest.

Once MT8196 MDP support is upstreamed, we will be able to run a number of tests
to evaluate whether this flag is really needed or not.

After all, if it turns out we can remove it, it's going to be a 3 lines patch,
not a big deal.

> +		.ops = &mtk_clk_gate_ops_setclr,	\
> +	}
> +
> +#define GATE_MDP1(_id, _name, _parent, _shift) {	\
> +		.id = _id,				\
> +		.name = _name,				\
> +		.parent_name = _parent,			\
> +		.regs = &mdp1_cg_regs,			\
> +		.shift = _shift,			\
> +		.ops = &mtk_clk_gate_ops_setclr,	\
> +	}
> +
> +#define GATE_MDP2(_id, _name, _parent, _shift) {	\
> +		.id = _id,				\
> +		.name = _name,				\
> +		.parent_name = _parent,			\
> +		.regs = &mdp2_cg_regs,			\
> +		.shift = _shift,			\
> +		.flags = CLK_OPS_PARENT_ENABLE,		\
> +		.ops = &mtk_clk_gate_ops_setclr,	\
> +	}
> +

..snip..

> +
> +static const struct mtk_clk_desc mdp_mcd = {
> +	.clks = mdp_clks,
> +	.num_clks = ARRAY_SIZE(mdp_clks),
> +	.need_runtime_pm = true,
> +};
> +
> +static const struct of_device_id of_match_clk_mt8196_mdpsys[] = {
> +	{ .compatible = "mediatek,mt8196-mdpsys1", .data = &mdp1_mcd },
> +	{ .compatible = "mediatek,mt8196-mdpsys0", .data = &mdp_mcd },

0 comes before 1, swap those entries please.

After applying the proposed fixes

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_mdpsys);
> +
> +static struct platform_driver clk_mt8196_mdpsys_drv = {
> +	.probe = mtk_clk_simple_probe,
> +	.remove = mtk_clk_simple_remove,
> +	.driver = {
> +		.name = "clk-mt8196-mdpsys",
> +		.of_match_table = of_match_clk_mt8196_mdpsys,
> +	},
> +};
> +module_platform_driver(clk_mt8196_mdpsys_drv);
> +
> +MODULE_DESCRIPTION("MediaTek MT8196 Multimedia Data Path clocks driver");
> +MODULE_LICENSE("GPL");
Re: [PATCH v5 19/27] clk: mediatek: Add MT8196 mdpsys clock support
Posted by Chen-Yu Tsai 4 weeks ago 
On Fri, Sep 5, 2025 at 4:39 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 29/08/25 11:19, Laura Nao ha scritto:
> > Add support for the MT8196 mdpsys clock controller, which provides clock
> > gate control for MDP.
> >
> > Signed-off-by: Laura Nao <laura.nao@collabora.com>
> > ---
> >   drivers/clk/mediatek/Kconfig             |   7 +
> >   drivers/clk/mediatek/Makefile            |   1 +
> >   drivers/clk/mediatek/clk-mt8196-mdpsys.c | 186 +++++++++++++++++++++++
> >   3 files changed, 194 insertions(+)
> >   create mode 100644 drivers/clk/mediatek/clk-mt8196-mdpsys.c
> >
> > diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
> > index 8e5cdae80748..68ac08cf8e82 100644
> > --- a/drivers/clk/mediatek/Kconfig
> > +++ b/drivers/clk/mediatek/Kconfig
> > @@ -1024,6 +1024,13 @@ config COMMON_CLK_MT8196_MCUSYS
> >       help
> >         This driver supports MediaTek MT8196 mcusys clocks.
> >
> > +config COMMON_CLK_MT8196_MDPSYS
> > +     tristate "Clock driver for MediaTek MT8196 mdpsys"
> > +     depends on COMMON_CLK_MT8196
> > +     default COMMON_CLK_MT8196
> > +     help
> > +       This driver supports MediaTek MT8196 mdpsys clocks.
> > +
> >   config COMMON_CLK_MT8196_PEXTPSYS
> >       tristate "Clock driver for MediaTek MT8196 pextpsys"
> >       depends on COMMON_CLK_MT8196
> > diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> > index 46358623c3e5..d2d8bc43e45b 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -155,6 +155,7 @@ obj-$(CONFIG_COMMON_CLK_MT8196) += clk-mt8196-apmixedsys.o clk-mt8196-topckgen.o
> >                                  clk-mt8196-peri_ao.o
> >   obj-$(CONFIG_COMMON_CLK_MT8196_IMP_IIC_WRAP) += clk-mt8196-imp_iic_wrap.o
> >   obj-$(CONFIG_COMMON_CLK_MT8196_MCUSYS) += clk-mt8196-mcu.o
> > +obj-$(CONFIG_COMMON_CLK_MT8196_MDPSYS) += clk-mt8196-mdpsys.o
> >   obj-$(CONFIG_COMMON_CLK_MT8196_PEXTPSYS) += clk-mt8196-pextp.o
> >   obj-$(CONFIG_COMMON_CLK_MT8196_UFSSYS) += clk-mt8196-ufs_ao.o
> >   obj-$(CONFIG_COMMON_CLK_MT8365) += clk-mt8365-apmixedsys.o clk-mt8365.o
> > diff --git a/drivers/clk/mediatek/clk-mt8196-mdpsys.c b/drivers/clk/mediatek/clk-mt8196-mdpsys.c
> > new file mode 100644
> > index 000000000000..a46b1627f1f3
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-mt8196-mdpsys.c
> > @@ -0,0 +1,186 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2025 MediaTek Inc.
> > + *                    Guangjie Song <guangjie.song@mediatek.com>
> > + * Copyright (c) 2025 Collabora Ltd.
> > + *                    Laura Nao <laura.nao@collabora.com>
> > + */
> > +#include <dt-bindings/clock/mediatek,mt8196-clock.h>
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "clk-gate.h"
> > +#include "clk-mtk.h"
> > +
> > +static const struct mtk_gate_regs mdp0_cg_regs = {
> > +     .set_ofs = 0x104,
> > +     .clr_ofs = 0x108,
> > +     .sta_ofs = 0x100,
> > +};
> > +
> > +static const struct mtk_gate_regs mdp1_cg_regs = {
> > +     .set_ofs = 0x114,
> > +     .clr_ofs = 0x118,
> > +     .sta_ofs = 0x110,
> > +};
> > +
> > +static const struct mtk_gate_regs mdp2_cg_regs = {
> > +     .set_ofs = 0x124,
> > +     .clr_ofs = 0x128,
> > +     .sta_ofs = 0x120,
> > +};
> > +
> > +#define GATE_MDP0(_id, _name, _parent, _shift) {     \
> > +             .id = _id,                              \
> > +             .name = _name,                          \
> > +             .parent_name = _parent,                 \
> > +             .regs = &mdp0_cg_regs,                  \
> > +             .shift = _shift,                        \
> > +             .flags = CLK_OPS_PARENT_ENABLE,         \
>
> Why would MDP0 and MDP2 be different, as in why would MDP1 be so special to not
> need CLK_OPS_PARENT_ENABLE while the others do?
>
> Either they all do, or they all don't.
>
> I guess they all don't, but I'm not sure how you tested that at all, since the
> only way to test this is downstream (and upstream will very likely be different
> from that).
>
> Even though I think they don't need that - please add back CLK_OPS_PARENT_ENABLE
> to GATE_MDP1 to be safe, as in (all) MediaTek SoCs the multimedia subsystem is
> kinda separate from the rest.

That kind of doesn't fly since the parent of mdp_f26m is clk26m, not the
mdp clock. So either this block doesn't need a clock for register access
or this clock is going to be broken.

This is why I raised the question about the validity of the flag in the
first place.

> +       GATE_MDP1(CLK_MDP_F26M, "mdp_f26m", "clk26m", 27),

> Once MT8196 MDP support is upstreamed, we will be able to run a number of tests
> to evaluate whether this flag is really needed or not.
>
> After all, if it turns out we can remove it, it's going to be a 3 lines patch,
> not a big deal.

That also works. Though IMO it makes the error harder to notice.

ChenYu

> > +             .ops = &mtk_clk_gate_ops_setclr,        \
> > +     }
> > +
> > +#define GATE_MDP1(_id, _name, _parent, _shift) {     \
> > +             .id = _id,                              \
> > +             .name = _name,                          \
> > +             .parent_name = _parent,                 \
> > +             .regs = &mdp1_cg_regs,                  \
> > +             .shift = _shift,                        \
> > +             .ops = &mtk_clk_gate_ops_setclr,        \
> > +     }
> > +
> > +#define GATE_MDP2(_id, _name, _parent, _shift) {     \
> > +             .id = _id,                              \
> > +             .name = _name,                          \
> > +             .parent_name = _parent,                 \
> > +             .regs = &mdp2_cg_regs,                  \
> > +             .shift = _shift,                        \
> > +             .flags = CLK_OPS_PARENT_ENABLE,         \
> > +             .ops = &mtk_clk_gate_ops_setclr,        \
> > +     }
> > +
>
> ..snip..
>
> > +
> > +static const struct mtk_clk_desc mdp_mcd = {
> > +     .clks = mdp_clks,
> > +     .num_clks = ARRAY_SIZE(mdp_clks),
> > +     .need_runtime_pm = true,
> > +};
> > +
> > +static const struct of_device_id of_match_clk_mt8196_mdpsys[] = {
> > +     { .compatible = "mediatek,mt8196-mdpsys1", .data = &mdp1_mcd },
> > +     { .compatible = "mediatek,mt8196-mdpsys0", .data = &mdp_mcd },
>
> 0 comes before 1, swap those entries please.
>
> After applying the proposed fixes
>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, of_match_clk_mt8196_mdpsys);
> > +
> > +static struct platform_driver clk_mt8196_mdpsys_drv = {
> > +     .probe = mtk_clk_simple_probe,
> > +     .remove = mtk_clk_simple_remove,
> > +     .driver = {
> > +             .name = "clk-mt8196-mdpsys",
> > +             .of_match_table = of_match_clk_mt8196_mdpsys,
> > +     },
> > +};
> > +module_platform_driver(clk_mt8196_mdpsys_drv);
> > +
> > +MODULE_DESCRIPTION("MediaTek MT8196 Multimedia Data Path clocks driver");
> > +MODULE_LICENSE("GPL");
>
Re: [PATCH v5 19/27] clk: mediatek: Add MT8196 mdpsys clock support
Posted by AngeloGioacchino Del Regno 2 weeks, 4 days ago 
Il 05/09/25 10:53, Chen-Yu Tsai ha scritto:
> On Fri, Sep 5, 2025 at 4:39 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 29/08/25 11:19, Laura Nao ha scritto:
>>> Add support for the MT8196 mdpsys clock controller, which provides clock
>>> gate control for MDP.
>>>
>>> Signed-off-by: Laura Nao <laura.nao@collabora.com>
>>> ---
>>>    drivers/clk/mediatek/Kconfig             |   7 +
>>>    drivers/clk/mediatek/Makefile            |   1 +
>>>    drivers/clk/mediatek/clk-mt8196-mdpsys.c | 186 +++++++++++++++++++++++
>>>    3 files changed, 194 insertions(+)
>>>    create mode 100644 drivers/clk/mediatek/clk-mt8196-mdpsys.c
>>>
>>> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
>>> index 8e5cdae80748..68ac08cf8e82 100644
>>> --- a/drivers/clk/mediatek/Kconfig
>>> +++ b/drivers/clk/mediatek/Kconfig
>>> @@ -1024,6 +1024,13 @@ config COMMON_CLK_MT8196_MCUSYS
>>>        help
>>>          This driver supports MediaTek MT8196 mcusys clocks.
>>>
>>> +config COMMON_CLK_MT8196_MDPSYS
>>> +     tristate "Clock driver for MediaTek MT8196 mdpsys"
>>> +     depends on COMMON_CLK_MT8196
>>> +     default COMMON_CLK_MT8196
>>> +     help
>>> +       This driver supports MediaTek MT8196 mdpsys clocks.
>>> +
>>>    config COMMON_CLK_MT8196_PEXTPSYS
>>>        tristate "Clock driver for MediaTek MT8196 pextpsys"
>>>        depends on COMMON_CLK_MT8196
>>> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
>>> index 46358623c3e5..d2d8bc43e45b 100644
>>> --- a/drivers/clk/mediatek/Makefile
>>> +++ b/drivers/clk/mediatek/Makefile
>>> @@ -155,6 +155,7 @@ obj-$(CONFIG_COMMON_CLK_MT8196) += clk-mt8196-apmixedsys.o clk-mt8196-topckgen.o
>>>                                   clk-mt8196-peri_ao.o
>>>    obj-$(CONFIG_COMMON_CLK_MT8196_IMP_IIC_WRAP) += clk-mt8196-imp_iic_wrap.o
>>>    obj-$(CONFIG_COMMON_CLK_MT8196_MCUSYS) += clk-mt8196-mcu.o
>>> +obj-$(CONFIG_COMMON_CLK_MT8196_MDPSYS) += clk-mt8196-mdpsys.o
>>>    obj-$(CONFIG_COMMON_CLK_MT8196_PEXTPSYS) += clk-mt8196-pextp.o
>>>    obj-$(CONFIG_COMMON_CLK_MT8196_UFSSYS) += clk-mt8196-ufs_ao.o
>>>    obj-$(CONFIG_COMMON_CLK_MT8365) += clk-mt8365-apmixedsys.o clk-mt8365.o
>>> diff --git a/drivers/clk/mediatek/clk-mt8196-mdpsys.c b/drivers/clk/mediatek/clk-mt8196-mdpsys.c
>>> new file mode 100644
>>> index 000000000000..a46b1627f1f3
>>> --- /dev/null
>>> +++ b/drivers/clk/mediatek/clk-mt8196-mdpsys.c
>>> @@ -0,0 +1,186 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2025 MediaTek Inc.
>>> + *                    Guangjie Song <guangjie.song@mediatek.com>
>>> + * Copyright (c) 2025 Collabora Ltd.
>>> + *                    Laura Nao <laura.nao@collabora.com>
>>> + */
>>> +#include <dt-bindings/clock/mediatek,mt8196-clock.h>
>>> +
>>> +#include <linux/clk-provider.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#include "clk-gate.h"
>>> +#include "clk-mtk.h"
>>> +
>>> +static const struct mtk_gate_regs mdp0_cg_regs = {
>>> +     .set_ofs = 0x104,
>>> +     .clr_ofs = 0x108,
>>> +     .sta_ofs = 0x100,
>>> +};
>>> +
>>> +static const struct mtk_gate_regs mdp1_cg_regs = {
>>> +     .set_ofs = 0x114,
>>> +     .clr_ofs = 0x118,
>>> +     .sta_ofs = 0x110,
>>> +};
>>> +
>>> +static const struct mtk_gate_regs mdp2_cg_regs = {
>>> +     .set_ofs = 0x124,
>>> +     .clr_ofs = 0x128,
>>> +     .sta_ofs = 0x120,
>>> +};
>>> +
>>> +#define GATE_MDP0(_id, _name, _parent, _shift) {     \
>>> +             .id = _id,                              \
>>> +             .name = _name,                          \
>>> +             .parent_name = _parent,                 \
>>> +             .regs = &mdp0_cg_regs,                  \
>>> +             .shift = _shift,                        \
>>> +             .flags = CLK_OPS_PARENT_ENABLE,         \
>>
>> Why would MDP0 and MDP2 be different, as in why would MDP1 be so special to not
>> need CLK_OPS_PARENT_ENABLE while the others do?
>>
>> Either they all do, or they all don't.
>>
>> I guess they all don't, but I'm not sure how you tested that at all, since the
>> only way to test this is downstream (and upstream will very likely be different
>> from that).
>>
>> Even though I think they don't need that - please add back CLK_OPS_PARENT_ENABLE
>> to GATE_MDP1 to be safe, as in (all) MediaTek SoCs the multimedia subsystem is
>> kinda separate from the rest.
> 
> That kind of doesn't fly since the parent of mdp_f26m is clk26m, not the
> mdp clock. So either this block doesn't need a clock for register access
> or this clock is going to be broken.
> 
> This is why I raised the question about the validity of the flag in the
> first place.
> 
>> +       GATE_MDP1(CLK_MDP_F26M, "mdp_f26m", "clk26m", 27),
> 
>> Once MT8196 MDP support is upstreamed, we will be able to run a number of tests
>> to evaluate whether this flag is really needed or not.
>>
>> After all, if it turns out we can remove it, it's going to be a 3 lines patch,
>> not a big deal.
> 
> That also works. Though IMO it makes the error harder to notice.
> 

Okay, I understand your point.

Let's keep CLK_OPS_PARENT_ENABLE out of MDP1.

Cheers,
Angelo
Re: [PATCH v5 19/27] clk: mediatek: Add MT8196 mdpsys clock support
Posted by Chen-Yu Tsai 4 weeks ago 
On Fri, Aug 29, 2025 at 5:21 PM Laura Nao <laura.nao@collabora.com> wrote:
>
> Add support for the MT8196 mdpsys clock controller, which provides clock
> gate control for MDP.
>
> Signed-off-by: Laura Nao <laura.nao@collabora.com>

IMO removing CLK_OPS_PARENT_ENABLE is the right thing to do.

However if the hardware ends up does having a requirement that _some_
clock be enabled before touching the registers, then I think the
MTK clock library needs to be refactored, so that a register access
clock can be tied to the regmap. That might also require some work
on the syscon API.

Whether the hardware needs such a clock or not, we would need some input
from MediaTek. There's nothing in the datasheet on this.

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> # CLK_OPS_PARENT_ENABLE removal

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.