Re: [PATCH v1 2/2] clk: mediatek: Add MT6789 clock controllers

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

Il 16/07/25 00:22, Arseniy Velikanov ha scritto:
> Add support for MT6789 clock controllers, which includes apmixed, afe,
> camsys, imgsys, infracfg, mcusys, mdpsys, mfgcfg, mmsys, topckgen,
> vdec, venc.
> 
> Signed-off-by: Arseniy Velikanov <me@adomerle.pw>

Hi Arseniy,
Thanks for all this work!

The submission looks generally fine, and I'm happy that you've ported those drivers
to the common probe, however, there are a few (relatively small) things to fixup.

Please check below.

> ---
>   drivers/clk/mediatek/Kconfig                  |  68 ++
>   drivers/clk/mediatek/Makefile                 |  11 +
>   drivers/clk/mediatek/clk-mt6789-apmixed.c     | 147 +++
>   drivers/clk/mediatek/clk-mt6789-audiosys.c    | 100 +++
>   drivers/clk/mediatek/clk-mt6789-cam.c         | 131 +++
>   drivers/clk/mediatek/clk-mt6789-img.c         | 100 +++
>   .../clk/mediatek/clk-mt6789-imp_iic_wrap.c    |  90 ++
>   drivers/clk/mediatek/clk-mt6789-infra_ao.c    | 228 +++++
>   drivers/clk/mediatek/clk-mt6789-mcu.c         |  68 ++
>   drivers/clk/mediatek/clk-mt6789-mdp.c         |  81 ++
>   drivers/clk/mediatek/clk-mt6789-mfgcfg.c      |  61 ++
>   drivers/clk/mediatek/clk-mt6789-mm.c          |  85 ++
>   drivers/clk/mediatek/clk-mt6789-topckgen.c    | 846 ++++++++++++++++++
>   drivers/clk/mediatek/clk-mt6789-vdec.c        | 119 +++
>   drivers/clk/mediatek/clk-mt6789-venc.c        |  65 ++
>   15 files changed, 2200 insertions(+)
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-apmixed.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-audiosys.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-cam.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-img.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-imp_iic_wrap.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-infra_ao.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-mcu.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-mdp.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-mfgcfg.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-mm.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-topckgen.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-vdec.c
>   create mode 100644 drivers/clk/mediatek/clk-mt6789-venc.c
> 

..snip..

> diff --git a/drivers/clk/mediatek/clk-mt6789-topckgen.c b/drivers/clk/mediatek/clk-mt6789-topckgen.c
> new file mode 100644
> index 000000000000..bd986e861eb4
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt6789-topckgen.c
> @@ -0,0 +1,846 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Copyright (c) 2025 Arseniy Velikanov <me@adomerle.pw>
> + */

..snip..


 > +static const struct mtk_fixed_factor top_divs[] = {
 > +	FACTOR(CLK_TOP_MFGPLL, "mfgpll_ck", "mfgpll", 1, 1),

..snip..

 > +	FACTOR(CLK_TOP_F26M, "f26m_ck", "clk26m", 1, 1),
 > +	FACTOR(CLK_TOP_AXI, "axi_ck", "axi_sel", 1, 1),
 > +	FACTOR(CLK_TOP_DISP, "disp_ck", "disp_sel", 1, 1),

Please, remove all instances of FACTOR(.... 1, 1) and use the right parent directly
for the other clocks.

The factor(1,1) means clock_rate * 1 / 1 == clock_rate.

I'm not sure why MediaTek likes to declare these - they are doing that in newer
SoCs as well, but that's not for upstream: it does not make any sense to have those
clocks in the driver, as those are effectively just name wrappers and nothing else.

The only thing those do is increasing the footprint for, well, no good reason...!

There's a pattern that you want to check: all of the "_ck" clocks are suspect! :-)

 > +	FACTOR(CLK_TOP_MDP, "mdp_ck", "mdp_sel", 1, 1),
 > +	FACTOR(CLK_TOP_IMG1, "img1_ck", "img1_sel", 1, 1),
 > +	FACTOR(CLK_TOP_IPE, "ipe_ck", "ipe_sel", 1, 1),
 > +	FACTOR(CLK_TOP_CAM, "cam_ck", "cam_sel", 1, 1),
 > +	FACTOR(CLK_TOP_MFG_REF, "mfg_ref_ck", "mfg_ref_sel", 1, 1),
 > +	FACTOR(CLK_TOP_MFG_PLL, "mfg_pll_ck", "mfg_pll_sel", 1, 1),

..snip..

> +
> +static const char * const dvfsrc_parents[] = {
> +	"tck_26m_mx9_ck",
> +	"osc_d10"
> +};
> +
> +static const char * const aes_msdcfde_parents[] = {
> +	"tck_26m_mx9_ck",
> +	"mainpll_d4_d2",
> +	"mainpll_d6",
> +	"mainpll_d4_d4",
> +	"univpll_d4_d2",
> +	"univpll_d6"
> +};
> +
> +static const char * const mcupm_parents[] = {
> +	"tck_26m_mx9_ck",
> +	"mainpll_d6_d4",
> +	"mainpll_d6_d2"
> +};
> +
> +static const char * const dsi_occ_parents[] = {
> +	"tck_26m_mx9_ck",
> +	"mainpll_d6_d2",
> +	"univpll_d5_d2",
> +	"univpll_d4_d2"
> +};
> +

Look at those parents, you're redefining the very same identical array 10 times
in a row!

Please, avoid this kind of duplication: delete all the duplicated arrays and
just have one called, for example, "apll_i2s_mck_parents", then assign that
to all of the relevant clocks.

Same goes for camtg and others :-)

> +static const char * const apll_i2s0_mck_parents[] = {
> +	"aud_1_sel",
> +	"aud_2_sel"
> +};
> +
> +static const char * const apll_i2s1_mck_parents[] = {
> +	"aud_1_sel",
> +	"aud_2_sel"
> +};
> +
> +static const char * const apll_i2s2_mck_parents[] = {
> +	"aud_1_sel",
> +	"aud_2_sel"
> +};
> +
> +static const char * const apll_i2s3_mck_parents[] = {
> +	"aud_1_sel",
> +	"aud_2_sel"
> +};
> +
> +static const char * const apll_i2s4_mck_parents[] = {
> +	"aud_1_sel",
> +	"aud_2_sel"
> +};
> +
> +static const char * const apll_i2s5_mck_parents[] = {
> +	"aud_1_sel",
> +	"aud_2_sel"
> +};
> +
> +static const char * const apll_i2s6_mck_parents[] = {
> +	"aud_1_sel",
> +	"aud_2_sel"
> +};
> +
> +static const char * const apll_i2s7_mck_parents[] = {
> +	"aud_1_sel",
> +	"aud_2_sel"
> +};
> +
> +static const char * const apll_i2s8_mck_parents[] = {
> +	"aud_1_sel",
> +	"aud_2_sel"
> +};
> +
> +static const char * const apll_i2s9_mck_parents[] = {
> +	"aud_1_sel",
> +	"aud_2_sel"
> +};

..snip..

> +module_platform_driver(clk_mt6789_topckgen_drv);
> +
> +MODULE_DESCRIPTION("MediaTek MT6789 top clock generators driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/mediatek/clk-mt6789-vdec.c b/drivers/clk/mediatek/clk-mt6789-vdec.c
> new file mode 100644
> index 000000000000..81d6e720b6cd
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt6789-vdec.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Copyright (c) 2025 Arseniy Velikanov <me@adomerle.pw>
> + */
> +

..snip..

> +
> +static const struct mtk_clk_desc vde2_desc = {
> +	.clks = vde2_clks,
> +	.num_clks = ARRAY_SIZE(vde2_clks),
> +};
> +
> +static const struct of_device_id of_match_clk_mt6789_vdec[] = {

Can you please compress all of those?

static const struct of_device_id of_match_clk_mt6789_vdec[] ={
	{ .compatible = "mediatek,mt6789-vdecsys", .data = &vde2_desc },
	{ /* sentinel */ }
};


Note that the same comments do apply to other clock drivers that you're introducing
but I'm only writing once - please apply the comments in the others and wherever
they fit.

Cheers!
Angelo