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

[Vishnu Reddy]

On 9/25/2025 2:48 PM, Konrad Dybcio wrote:
> On 9/25/25 1:14 AM, 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 <quic_bvisredd@xxxxxxxxxxx>
> > Signed-off-by: Vishnu Reddy <quic_bvisredd@xxxxxxxxxxx>
> > Signed-off-by: Vikash Garodia <vikash.garodia@xxxxxxxxxxxxxxxx>
> > ---
>
> [...]
>
> > +#include <linux/iopoll.h>
> > +#include <linux/reset.h>
> > +#include "iris_instance.h"
> > +#include "iris_vpu_common.h"
> > +#include "iris_vpu_register_defines.h"
> > +
> > +#define WRAPPER_EFUSE_MONITOR			(WRAPPER_BASE_OFFS + 0x08)
> > +#define AON_WRAPPER_MVP_NOC_RESET_SYNCRST	(AON_MVP_NOC_RESET + 0x08)
> > +#define CPU_CS_APV_BRIDGE_SYNC_RESET		(CPU_BASE_OFFS + 0x174)
> > +#define DISABLE_VIDEO_APV_BIT			BIT(27)
> > +#define DISABLE_VIDEO_VPP1_BIT			BIT(28)
> > +#define DISABLE_VIDEO_VPP0_BIT			BIT(29)
> > +#define CORE_CLK_HALT				BIT(0)
> > +#define APV_CLK_HALT				BIT(1)
> > +#define CORE_PWR_ON				BIT(1)
> > +
> > +static int iris_vpu4x_genpd_set_hwmode(struct iris_core *core, bool hw_mode)
> > +{
> > +	u32 value = readl(core->reg_base + WRAPPER_EFUSE_MONITOR);
>
> I think this could use some explanations.
>
> I'll go ahead and assume that the eFuse tells us that parts of the
> IP are disables (hopefully not all three at once.. we shouldn't
> advertise the v4l2 device then, probably)
>
> You read back the fuse register a lot, even though I presume it's not
> supposed to change at runtime. How about we add:
>
> bool vpp0_fused_off
> bool vpp1_fused_off
> bool apv_fused_off
>
> instead?

 Hi Konrad, Thanks for your review and suggestion.

 The poweroff and poweron ops will be called in each test. There is no
 ops available that called onetime only to cache these values.
 And, to create any variable, Need to add as static global in this file
 because these are specific to this platform and I feel it's not
 recommended code to add into any common structures as a member.

 Do you have any suggestion from your side how this can be do it in a
 simple way?

> [...]
>
> > +	if (!(value & DISABLE_VIDEO_VPP0_BIT)) {
> > +		ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs
> > +						[IRIS_VPP0_HW_POWER_DOMAIN]);
>
> Maybe the iris_en/disable_foo functions could get a wrapper like:
>
> int iris_enable_power_domains_if(core, pd_devs[IRIS_VPP0_HW_POWER_DOMAIN],
> 				 !foo->vpp0_fused_off)
>
> I'm not super sure about it, but that's something to consider
>
> [...]
>
> > +	readl_poll_timeout(core->reg_base + VCODEC_SS_IDLE_STATUSN, value,
> > +			   value & 0x7103, 2000, 20000);
>
> That's a nice magic number.. but what does it mean?

 ACK, Will add macro definitions for these numbers.

> [...]
>
> > +	writel(0x070103, core->reg_base + AON_WRAPPER_MVP_NOC_RESET_REQ);
> > +	readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_RESET_ACK,
> > +			   value, value == 0x070103, 200, 2000);
>
> That's a slightly different magic number, but it's oddly similar to
> the one above

 ACK.

 Thanks and regards,
 Vishnu Reddy