Re: [PATCH v4 1/6] media: iris: wait for vpu NoC to enter low power during power off

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: iris: unhandled timeout on VPU NoC LPI status poll
- [Low] media: iris: magic number used instead of NOC_LPI_STATUS_DONE
--

commit ad8593fbbb8fd303ff77853725986ef7bd6d05a9
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

    media: iris: wait for vpu NoC to enter low power during power off
    
    The recommended hardware sequence requires software to wait for vpu NoC
    to enter in low power mode and then deassert the low power idle (LPI)
    control. Add the missing LPI status poll and move the LPI control
    deassertion after the hardware acks LPI mode. This avoids any pending
    transactions on vpu NoC before shutting it down.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu4x.c b/drivers/media/platform/qcom/iris/iris_vpu4x.c
> index 02e100a4045fc..32a161c8d534a 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu4x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu4x.c
> @@ -316,6 +316,10 @@ static void iris_vpu4x_power_off_hardware(struct iris_core *core)
>  	if (!handshake_done && handshake_busy)
>  		dev_err(core->dev, "LPI handshake timeout\n");
>  
> +	readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
> +			   value, value & BIT(0), 200, 2000);

[Severity: Low]
This isn't a bug, but could NOC_LPI_STATUS_DONE be used here instead of the
magic number BIT(0) to remain consistent with the rest of the function?

[Severity: Medium]
Is it intentional to discard the return value of readl_poll_timeout() here?

If the poll times out, the code silently proceeds to deassert LPI control and
reset the NoC, which could lead to hardware stalls from pending transactions.

Additionally, if the preceding do-while loop times out, LPI_CONTROL is left
at 0x0. This appears to guarantee the new poll will unconditionally fail and
wait the full 2000 microseconds. 

Could this return value be assigned to 'ret' and handled similarly to the
other polling loops in this function?

> +	writel(0x0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +
>  	writel(MVP_NOC_RESET_REQ_MASK, core->reg_base + AON_WRAPPER_MVP_NOC_RESET_REQ);
>  	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_RESET_ACK,
>  				 value, value & MVP_NOC_RESET_REQ_MASK, 200, 2000);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-kaanapali-iris-v4-0-642f9ac5e699@oss.qualcomm.com?part=1