Re: [PATCH v2 4/4] clk-816x: add sys_ck clock as sys_clkin_ck alias

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: ti: dm816x: adding sys_ck alias triggers invalid OMAP3 VC/VP init
--

commit 1a28e92700d48db3c2d33b03820789ccb46fb7f1
Author: Ettore Chimenti <ettore.chimenti@linaro.org>

clk-816x: add sys_ck clock as sys_clkin_ck alias

This commit adds a clock alias mapping sys_ck to sys_clkin_ck in
dm816x_clks[] so that the OMAP voltage management driver can find sys_ck on
DM816x systems.

> diff --git a/drivers/clk/ti/clk-816x.c b/drivers/clk/ti/clk-816x.c
> index 3b8e483aec928..fe6f66fc5a05d 100644
> --- a/drivers/clk/ti/clk-816x.c
> +++ b/drivers/clk/ti/clk-816x.c
> @@ -57,6 +57,7 @@ static struct ti_dt_clk dm816x_clks[] = {
>  	DT_CLK(NULL, "timer_sys_ck", "sys_clkin_ck"),
>  	DT_CLK(NULL, "timer_32k_ck", "sysclk18_ck"),
>  	DT_CLK(NULL, "timer_ext_ck", "tclkin_ck"),
> +	DT_CLK(NULL, "sys_ck", "sys_clkin_ck"),

[Severity: Medium]
Does adding the sys_ck alias in dm816x_clks[] cause omap_voltage_late_init()
to run OMAP34xx/36xx voltage controller and voltage processor initialization
on DM816x where that hardware and PMIC data are absent?

On DM816x boot, ti816x_init_early() (as well as ti814x_init_early() on TI814x)
calls omap3xxx_voltagedomains_init() in
arch/arm/mach-omap2/voltagedomains3xxx_data.c:

	if (soc_is_am35xx())
		voltdms = voltagedomains_am35xx;
	else
		voltdms = voltagedomains_omap3;

Because only soc_is_am35xx() is checked, TI81xx falls back to
voltagedomains_omap3, which registers omap3_voltdm_mpu and omap3_voltdm_core
with .scalable = true, .vc, and .vp populated.

Later, omap2_common_pm_late_init() calls omap3_twl_init(), which returns
-ENODEV because cpu_is_omap34xx() is false on DM816x, leaving voltdm->pmic as
NULL. It then calls omap_voltage_late_init() in
arch/arm/mach-omap2/voltage.c:

	list_for_each_entry(voltdm, &voltdm_list, node) {
		struct clk *sys_ck;

		if (!voltdm->scalable)
			continue;

		sys_ck = clk_get(NULL, voltdm->sys_clk.name);
		if (IS_ERR(sys_ck)) {
			pr_warn("%s: Could not get sys clk.\n", __func__);
			return -EINVAL;
		}
		voltdm->sys_clk.rate = clk_get_rate(sys_ck);
		WARN_ON(!voltdm->sys_clk.rate);
		clk_put(sys_ck);

		if (voltdm->vc) {
			voltdm->scale = omap_vc_bypass_scale;
			omap_vc_init_channel(voltdm);
		}

		if (voltdm->vp) {
			voltdm->scale = omap_vp_forceupdate_scale;
			omap_vp_init(voltdm);
		}
	}

When clk_get(NULL, "sys_ck") succeeds, omap_voltage_late_init() proceeds to
call omap_vc_init_channel() in arch/arm/mach-omap2/vc.c and omap_vp_init() in
arch/arm/mach-omap2/vp.c for both mpu_iva and core:

	if (!voltdm->pmic || !voltdm->pmic->uv_to_vsel) {
		pr_err("%s: No PMIC info for vdd_%s\n", __func__, voltdm->name);
		return;
	}

This trades the single pr_warn() in omap_voltage_late_init() for four pr_err()
messages on boot and leaves voltdm->scale set to omap_vp_forceupdate_scale on
uninitialized VP instances.

Should TI81xx instead use non-scalable voltage domains in
omap3xxx_voltagedomains_init(), similar to how commit ddf536d0d7e1 ("ARM:
OMAP: AM3517/3505: fix crash on boot due to incorrect voltagedomain data")
handled AM35xx with voltagedomains_am35xx?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-patch-ektor5-dm8168-clk-v2-0-dc6bf0fabae4@linaro.org?part=4