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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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