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

[Vikash Garodia <vikash.garodia@oss.qualcomm.com>]

On 12/7/2025 4:34 AM, Bryan O'Donoghue wrote:
> 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 ?

Yes, LPI (low power..) is an optional "good to have" routine before 
resetting the NOC and powering down.

> 
> We would expect the poll_timeout to timeout .. ?

during good times, it would not, and when it times out, it is always 
good to force reset the NOC before the GDSCs/clocks are turned off.

> 
> 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 ?

Thats correct.

> 
> 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 ?
> 

Given the NOC reset/ack is recommended, even if LPI times out, we can 
move to disable_power i.e power off clocks/GDSCs, once NOC reset times out.

Regards,
Vikash