Re: [PATCH v7 07/12] media: iris: Add power sequence for glymur

[Vishnu Reddy <busanna.reddy@oss.qualcomm.com>]

On 6/8/2026 11:09 AM, Vishnu Reddy wrote:
> On 6/8/2026 3:17 AM, Dmitry Baryshkov wrote:
>> On Wed, Jun 03, 2026 at 07:48:45PM +0530, Vishnu Reddy wrote:
>>> Glymur platform has two video codec cores: vcodec0 and vcodec1.
>>>
>>> Both cores share a common clock source (video_cc_mvs0_clk_src) and the
>>> same power rails. The clock dividers between the source and the branch
>>> clocks are fixed. So when both cores are running, the source clock always
>>> runs at the highest frequency requested by either core.
>>>
>>> Since both cores share the same power rails, the power corner cannot be
>>> voted independently. Scaling one core's power corner up or down would
>>> directly affect the other, leading to under or over-voting.
>>>
>>> For these reasons, both cores should voted the clock and power rail must
>>> be based on the workload of both cores.
>>>
>>> Reviewed-by: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
>>> Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
>>> ---
>>>  drivers/media/platform/qcom/iris/iris_vpu3x.c      | 111 ++++++++++++++++++++-
>>>  drivers/media/platform/qcom/iris/iris_vpu_common.h |   1 +
>>>  .../platform/qcom/iris/iris_vpu_register_defines.h |  10 ++
>>>  3 files changed, 117 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>>> index 7cda94601555..f07eaf4b3be2 100644
>>> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
>>> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
>>> @@ -17,14 +17,14 @@
>>>  #define NOC_HALT				BIT(0)
>>>  #define AON_WRAPPER_SPARE			(AON_BASE_OFFS + 0x28)
>>>  
>>> -static bool iris_vpu3x_hw_power_collapsed(struct iris_core *core)
>>> +static bool iris_vpu3x_hw_power_collapsed(struct iris_core *core, u32 pwr_status_bit)
>>>  {
>>>  	u32 value, pwr_status;
>>>  
>>>  	value = readl(core->reg_base + WRAPPER_CORE_POWER_STATUS);
>>> -	pwr_status = value & BIT(1);
>>> +	pwr_status = value & pwr_status_bit;
>>>  
>>> -	return pwr_status ? false : true;
>>> +	return !pwr_status;
>>>  }
>>>  
>>>  static void iris_vpu3_power_off_hardware(struct iris_core *core)
>>> @@ -32,7 +32,7 @@ static void iris_vpu3_power_off_hardware(struct iris_core *core)
>>>  	u32 reg_val = 0, value, i;
>>>  	int ret;
>>>  
>>> -	if (iris_vpu3x_hw_power_collapsed(core))
>>> +	if (iris_vpu3x_hw_power_collapsed(core, VCODEC0_POWER_STATUS))
>>>  		goto disable_power;
>>>  
>>>  	dev_err(core->dev, "video hw is power on\n");
>>> @@ -78,7 +78,7 @@ static void iris_vpu33_power_off_hardware(struct iris_core *core)
>>>  	u32 count = 0;
>>>  	int ret;
>>>  
>>> -	if (iris_vpu3x_hw_power_collapsed(core))
>>> +	if (iris_vpu3x_hw_power_collapsed(core, VCODEC0_POWER_STATUS))
>>>  		goto disable_power;
>>>  
>>>  	dev_err(core->dev, "video hw is power on\n");
>>> @@ -221,6 +221,98 @@ static void iris_vpu35_power_off_hw(struct iris_core *core)
>>>  	iris_vpu33_power_off_hardware(core);
>>>  }
>>>  
>>> +static void iris_vpu36_power_off_vcodec(struct iris_core *core, u32 core_id)
>>> +{
>>> +	u32 value, i;
>>> +	int ret;
>>> +
>>> +	if (iris_vpu3x_hw_power_collapsed(core,
>>> +					  core_id ? VCODEC1_POWER_STATUS : VCODEC0_POWER_STATUS))
>>> +		goto disable_power;
>>> +
>>> +	value = readl(core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
>>> +	if (value)
>>> +		writel(CORE_CLK_RUN, core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
>>> +
>>> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
>>> +		ret = readl_poll_timeout(core->reg_base + (core_id ? VCODEC1_SS_IDLE_STATUSN :
>>> +					 VCODEC_SS_IDLE_STATUSN) + 4 * i, value,
>>> +					 value & DMA_NOC_IDLE, 2000, 20000);
>> Unreadable. Extract register addresses to separate variables.
> Ack
>
>>> +		if (ret)
>>> +			goto disable_power;
>>> +	}
>>> +
>>> +	writel(core_id ? REQ_VCODEC1_POWER_DOWN_PREP : REQ_POWER_DOWN_PREP,
>>> +	       core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
>>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS, value,
>>> +				 value & (core_id ? NOC_LPI_VCODEC1_STATUS_DONE :
>>> +				 NOC_LPI_STATUS_DONE), 2000, 20000);
>>> +	if (ret)
>>> +		goto disable_power;
>>> +
>>> +	writel(0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
>>> +
>>> +	writel((core_id ? VCODEC1_BRIDGE_SW_RESET | VCODEC1_BRIDGE_HW_RESET_DISABLE :
>>> +	       CORE_BRIDGE_SW_RESET | CORE_BRIDGE_HW_RESET_DISABLE),
>>> +	       core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>> +	writel(core_id ? VCODEC1_BRIDGE_HW_RESET_DISABLE : CORE_BRIDGE_HW_RESET_DISABLE,
>>> +	       core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>> +	writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>>> +
>>> +disable_power:
>>> +	dev_pm_genpd_set_hwmode(core->vcodec[core_id].dev, false);
>>> +	iris_disable_power_domain_and_clocks(core, &core->vcodec[core_id]);
>>> +}
>>> +
>>> +static void iris_vpu36_power_off_hw(struct iris_core *core)
>>> +{
>>> +	u32 num_cores = max(core->iris_platform_data->num_cores, 1);
>> Can we trust the patch authors that VPU3.6 hardware data will have at
>> least 1 core?
> On Glymur (VPU3.6), num_cores is 2. The max() guard is unnecessary.
> Will access num_cores directly.
>
>>> +	int i;
>>> +
>>> +	for (i = 0; i < num_cores; i++)
>>> +		iris_vpu36_power_off_vcodec(core, i);
>>> +}
>>> +
>>>  #define WRAPPER_DEBUG_BRIDGE_LPI_STATUS		(WRAPPER_BASE_OFFS + 0x58)
>>>  #define WRAPPER_IRIS_CPU_NOC_LPI_CONTROL	(WRAPPER_BASE_OFFS + 0x5C)
>> Lowercase hex.
> Ack

Just now I found that this change is not part of this series.
Can I take this into separate post?

>>>  #define REQ_POWER_DOWN_PREP			BIT(0)
>>> +#define REQ_VCODEC1_POWER_DOWN_PREP		BIT(1)
>>>