Re: [PATCH v5 10/14] media: iris: Add power sequence for Glymur

[Dmitry Baryshkov]

On Mon, May 11, 2026 at 09:42:01PM +0530, Vishnu Reddy wrote:
> 
> On 5/9/2026 2:24 AM, Dmitry Baryshkov wrote:
> > On Sat, May 09, 2026 at 12:29:59AM +0530, Vishnu Reddy wrote:
> >> Glymur has a secondary video codec core (vcodec1), equivalent to the
> >> primary core (vcodec0), but with independent power domains, clocks,
> >> and reset lines. Reuse the existing code wherever possible and add
> >> power sequence for vcodec1.
> >>
> >> Reviewed-by: Vikash Garodia <vikash.garodia@xxxxxxxxxxxxxxxx>
> >> Signed-off-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
> >> ---
> >>  .../platform/qcom/iris/iris_platform_common.h      |   4 +
> >>  drivers/media/platform/qcom/iris/iris_vpu3x.c      | 141 ++++++++++++++++++++-
> >>  drivers/media/platform/qcom/iris/iris_vpu_common.h |   1 +
> >>  .../platform/qcom/iris/iris_vpu_register_defines.h |  10 ++
> >>  4 files changed, 154 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
> >> index 7d59e6364e9d..8995136ad29e 100644
> >> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
> >> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
> >> @@ -61,6 +61,9 @@ enum platform_clk_type {
> >>  	IRIS_VPP0_HW_CLK,
> >>  	IRIS_VPP1_HW_CLK,
> >>  	IRIS_APV_HW_CLK,
> >> +	IRIS_AXI_VCODEC1_CLK,
> >> +	IRIS_VCODEC1_CLK,
> >> +	IRIS_VCODEC1_FREERUN_CLK,
> > I think I have asked the same question while reviewing some other code.
> > When seeing such enums my expectation would be that the set of clocks is
> > more or less generic, while the platform specifics should be
> > encapsulated in platform-specific code. Instead these lists keep on
> > growing to accomodate platform details.
> >
> > Can we stop that tradition? Adding a peculiarity of the platform should
> > not require touching of the generic code.
> >
> >>  };
> >>  
> >>  struct platform_clk_data {
> >> @@ -210,6 +213,7 @@ enum platform_pm_domain_type {
> >>  	IRIS_VPP0_HW_POWER_DOMAIN,
> >>  	IRIS_VPP1_HW_POWER_DOMAIN,
> >>  	IRIS_APV_HW_POWER_DOMAIN,
> >> +	IRIS_VCODEC1_POWER_DOMAIN,
> >>  };
> >>  
> >>  struct platform_pd_data {
> >> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> >> index 13fbb21c2182..ff90c375e805 100644
> >> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> >> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> >> @@ -22,9 +22,19 @@ static bool iris_vpu3x_hw_power_collapsed(struct iris_core *core)
> >>  	u32 value, pwr_status;
> >>  
> >>  	value = readl(core->reg_base + WRAPPER_CORE_POWER_STATUS);
> >> -	pwr_status = value & BIT(1);
> >> +	pwr_status = value & VCODEC0_POWER_STATUS;
> >>  
> >> -	return pwr_status ? false : true;
> >> +	return !pwr_status;
> >> +}
> >> +
> >> +static bool iris_vpu36_hw1_power_collapsed(struct iris_core *core)
> >> +{
> >> +	u32 value, pwr_status;
> >> +
> >> +	value = readl(core->reg_base + WRAPPER_CORE_POWER_STATUS);
> >> +	pwr_status = value & VCODEC1_POWER_STATUS;
> >> +
> >> +	return !pwr_status;
> > Add core as an argument to the function instead of c&p'ing it.
> 
> Ack, will pass the vcodecx_power_status bit to this function.

u32 core, please.

> 
> >>  }
> >>  
> >>  static void iris_vpu3_power_off_hardware(struct iris_core *core)
> >> @@ -254,6 +264,124 @@ static void iris_vpu35_power_off_hw(struct iris_core *core)
> >>  	iris_disable_unprepare_clock(core, IRIS_AXI_VCODEC_CLK);
> >>  }
> >>  
> >> +static int iris_vpu36_power_on_hw1(struct iris_core *core)
> > Hmmm... And if 3.7 gets 4 cores, will we have 4 copies of the function?
> 
> As of now, none of the near upcoming targets introduce a significantly higher
> number of cores. If that changes in the future, we can revisit and optimize it
> then.

Okay....

-- 
With best wishes
Dmitry