On Fri, Sep 5, 2025 at 4:39 PM AngeloGioacchino Del Regno
[off-list ref] wrote:

Il 29/08/25 11:19, Laura Nao ha scritto:

quoted

Add support for the MT8196 mdpsys clock controller, which provides clock
gate control for MDP.

Signed-off-by: Laura Nao <redacted>
---
  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

quoted

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

quoted

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

quoted

+     { /* 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");