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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4a37f32e-2f30-468b-98f4-b68bcf8e85f1@collabora.com>
Date: Fri, 5 Sep 2025 10:20:31 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Laura Nao <laura.nao@...labora.com>, mturquette@...libre.com,
 sboyd@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 matthias.bgg@...il.com, p.zabel@...gutronix.de, richardcochran@...il.com
Cc: guangjie.song@...iatek.com, wenst@...omium.org,
 linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org,
 kernel@...labora.com
Subject: Re: [PATCH v5 13/27] clk: mediatek: Add MT8196 vlpckgen clock support

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@...labora.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@...iatek.com>
> + * Copyright (c) 2025 Collabora Ltd.
> + *                    Laura Nao <laura.nao@...labora.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@...labora.com>