Re: [PATCH v3 6/6] media: iris: Introduce vpu ops for vpu4 with necessary hooks

[Bryan O'Donoghue <bod@kernel.org>]

On 07/11/2025 09:49, Vikash Garodia wrote:
> Add power sequence for vpu4 by reusing from previous generation wherever
> possible. Hook up vpu4 op with vpu4 specific implemtation or resue from
> earlier generation wherever feasible, like clock calculation in this
> case.
> 
> Co-developed-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
> Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
> Signed-off-by: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
> ---

> +static void iris_vpu4x_power_off_hardware(struct iris_core *core)
> +{
> +	u32 efuse_value = readl(core->reg_base + WRAPPER_EFUSE_MONITOR);
> +	bool handshake_done, handshake_busy;
> +	u32 value, count = 0;
> +
> +	iris_vpu4x_genpd_set_hwmode(core, false, efuse_value);
> +
> +	if (!(efuse_value & DISABLE_VIDEO_APV_BIT))
> +		iris_vpu4x_power_off_apv(core);
> +
> +	value = readl(core->reg_base + WRAPPER_CORE_POWER_STATUS);
> +
> +	if (!(value & CORE_PWR_ON))
> +		goto disable_clocks_and_power;
> +
> +	value = readl(core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
> +
> +	if (value & CORE_CLK_HALT)
> +		writel(0x0, core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
> +
> +	readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN, value,
> +			   value & VPU_IDLE_BITS, 2000, 20000);
> +
> +	do {
> +		writel(REQ_POWER_DOWN_PREP, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +		usleep_range(10, 20);
> +		value = readl(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS);
> +
> +		handshake_done = value & NOC_LPI_STATUS_DONE;
> +		handshake_busy = value & (NOC_LPI_STATUS_DENY | NOC_LPI_STATUS_ACTIVE);
> +
> +		if (handshake_done || !handshake_busy)
> +			break;
> +
> +		writel(0x0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +		usleep_range(10, 20);
> +
> +	} while (++count < 1000);
> +
> +	if (!handshake_done && handshake_busy)
> +		dev_err(core->dev, "LPI handshake timeout\n");
> +
> +	writel(MVP_NOC_RESET_REQ_MASK, core->reg_base + AON_WRAPPER_MVP_NOC_RESET_REQ);
> +	readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_RESET_ACK,
> +			   value, value == MVP_NOC_RESET_REQ_MASK, 200, 2000);

I realise it replicates what we already have upstream but...

if (!handshake_done && etc) is true then how does it make sense to 
continue on with the routine at all ?

We would expect the poll_timeout to timeout .. ?

If the documentation states loop 1000 times trying this then does it 
also say continue to try to power things off if that 1000 retries fails ?

I realise its a nit-pick but the logic seems a bit fluffy here. Could 
you tidy it up ?

Also now that I look at it - vpu2 and vpu3 both trap the timeout and 
jump to some kind of cleanup routine.

goto disable_power;

Why is that logic not being followed here ?

> +	writel(0x0, core->reg_base + AON_WRAPPER_MVP_NOC_RESET_SYNCRST);
> +	writel(0x0, core->reg_base + AON_WRAPPER_MVP_NOC_RESET_REQ);
> +	readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_RESET_ACK,
> +			   value, value == 0x0, 200, 2000);

Feels like there is little point in having a poll timeout if we throw 
away a timeout result code..

Not sure why you're changing up the logic from previous versions ?

---
bod