Re: [PATCH v9 08/14] media: iris: Add power sequence for glymur

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] If the NOC LPI handshake times out during readl_poll_timeout, the PREP bit is left asserted in the AON register, permanently hanging the NOC on the next boot.
--

--- Patch [8]: [PATCH v9 08/14] media: iris: Add power sequence for glymur ---
commit d6ece601b7cdb53f3a84fa94a79400eb75a5853d
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Add power sequence for glymur

This commit introduces power sequencing for the Glymur platform, which has
two video codec cores. It ensures that clock and power rail voting is
coordinated since both cores share the same power rails and clock source.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c

[ ... ]

> +static void iris_vpu36_power_off_vcodec(struct iris_core *core, u32 core_id)
> +{

[ ... ]

> +	do {
> +		writel(power_down_prep[core_id], core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +		usleep_range(15, 20);
> +		value = readl(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS);
> +
> +		handshake_done = value & lpi_status_done[core_id];
> +		handshake_busy = value & (lpi_status_deny[core_id] | lpi_status_active[core_id]);
> +
> +		if (handshake_done || !handshake_busy)
> +			break;
> +
> +		writel(0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +		usleep_range(15, 20);
> +	} while (++count < 1000);
> +
> +	if (!handshake_done && handshake_busy)
> +		goto disable_power;
> +
> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS, value,
> +				 value & lpi_status_done[core_id], 200, 2000);
> +	if (ret)
> +		goto disable_power;

[Severity: High]
If this readl_poll_timeout() fails and returns an error, the code jumps
directly to disable_power, skipping the zeroing of the control register below.
Since the AON_WRAPPER_MVP_NOC_LPI_CONTROL register is in the Always-On (AON)
domain, won't this leave the power_down_prep bit asserted across power cycles
and permanently hang the NOC on the next boot?

Should writel(0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL) be called
before jumping to disable_power on timeout?

> +
> +	writel(0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +
> +	writel(bridge_sw_reset[core_id] | bridge_hw_reset[core_id],
> +	       core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +	writel(bridge_hw_reset[core_id], core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +	writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +
> +disable_power:
> +	iris_genpd_set_hwmode(&core->vcodec[core_id], false);
> +	iris_disable_power_domain_and_clocks(core, &core->vcodec[core_id]);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-glymur-v9-0-8cf2cbe12a07@oss.qualcomm.com?part=8