Re: [PATCH v5 13/27] clk: mediatek: Add MT8196 vlpckgen clock support

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

Il 29/08/25 11:18, Laura Nao ha scritto:
> Add support for the MT8196 vlpckgen clock controller, which provides
> muxes and dividers for clock selection in other IP blocks.
> 
> Signed-off-by: Laura Nao <laura.nao@collabora.com>
> ---
>   drivers/clk/mediatek/Makefile              |   2 +-
>   drivers/clk/mediatek/clk-mt8196-vlpckgen.c | 729 +++++++++++++++++++++
>   2 files changed, 730 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/clk/mediatek/clk-mt8196-vlpckgen.c
> 
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index c415453e02fd..031e7ac38804 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -151,7 +151,7 @@ obj-$(CONFIG_COMMON_CLK_MT8195_VENCSYS) += clk-mt8195-venc.o
>   obj-$(CONFIG_COMMON_CLK_MT8195_VPPSYS) += clk-mt8195-vpp0.o clk-mt8195-vpp1.o
>   obj-$(CONFIG_COMMON_CLK_MT8195_WPESYS) += clk-mt8195-wpe.o
>   obj-$(CONFIG_COMMON_CLK_MT8196) += clk-mt8196-apmixedsys.o clk-mt8196-topckgen.o \
> -				   clk-mt8196-topckgen2.o
> +				   clk-mt8196-topckgen2.o clk-mt8196-vlpckgen.o
>   obj-$(CONFIG_COMMON_CLK_MT8365) += clk-mt8365-apmixedsys.o clk-mt8365.o
>   obj-$(CONFIG_COMMON_CLK_MT8365_APU) += clk-mt8365-apu.o
>   obj-$(CONFIG_COMMON_CLK_MT8365_CAM) += clk-mt8365-cam.o
> diff --git a/drivers/clk/mediatek/clk-mt8196-vlpckgen.c b/drivers/clk/mediatek/clk-mt8196-vlpckgen.c
> new file mode 100644
> index 000000000000..c38d1e80a5ba
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8196-vlpckgen.c
> @@ -0,0 +1,729 @@
> +// 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>
> + */

..snip..

> +
> +static const char * const vlp_noc_vlp_parents[] = {
> +	"clk26m",
> +	"osc_d20",
> +	"mainpll_d9"
> +};
> +
> +static const char * const vlp_audio_h_parents[] = {
> +	"clk26m",

Please remove clk26m from this list (and from engen1/2/intbus).
We shall either use vlp_clk26m or clk26m directly - and I suspect that the VLP
specific 26m one is there for a reason.

What I'm not sure of is whether vlp_clk26m is really parented to clk26m or if it
is an additional internal crystal, but let's be cautious for now and keep the
parenting.

> +	"vlp_clk26m",
> +	"vlp_apll1",
> +	"vlp_apll2"
> +};
> +
> +static const char * const vlp_aud_engen1_parents[] = {
> +	"clk26m",
> +	"vlp_clk26m",
> +	"apll1_d8",
> +	"apll1_d4"
> +};
> +
> +static const char * const vlp_aud_engen2_parents[] = {
> +	"clk26m",
> +	"vlp_clk26m",
> +	"apll2_d8",
> +	"apll2_d4"
> +};
> +
> +static const char * const vlp_aud_intbus_parents[] = {
> +	"clk26m",
> +	"vlp_clk26m",
> +	"mainpll_d7_d4",
> +	"mainpll_d4_d4"
> +};
> +
> +static const u8 vlp_aud_parent_index[] = { 1, 2, 3 };

... of course, this index list doesn't match the parents list, so this will
break all of the clocks above because you're selecting wrong index in HW and
ignoring the last entry as well, producing a clock rate mismatch.

Once the clk26m is dropped from the names list, though, this index list becomes
valid.

After fixing:

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