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

[Vikash Garodia]

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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Vikash Garodia <vikash.garodia@xxxxxxxxxxxxxxxx>
> > ---
>
> > +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