Re: [PATCH v2 2/7] media: platform: qcom/iris: split iris_vpu_power_off_controller in multiple steps

[Dikshita Agarwal <quic_dikshita@quicinc.com>]

On 3/6/2025 12:35 AM, Neil Armstrong wrote:
> In order to support vpu33, the iris_vpu_power_off_controller needs to be
> reused and extended, but the AON_WRAPPER_MVP_NOC_LPI_CONTROL cannot be
> set from the power_off_controller sequence like vpu2 and vpu3 so
> split the power_off_controller into 3 steps:
> - iris_vpu_power_off_controller_begin
> - iris_vpu_power_off_controller_end
> - iris_vpu_power_off_controller_disable
> 
NAK.
I don't think splitting the API into these small functions is beneficial in
this case, The extracted parts are too trivial to justify separate
functions, and this actually makes the flow harder to follow rather than
improving re-usability or clarity.
If there is no clear or significant re-use case, I'd prefer keeping the
logic consolidated in single API to maintain readability.

Thanks,
Dikshita
> And use them in a common iris_vpu_power_off_controller() for
> vpu2 and vpu3 based platforms.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/media/platform/qcom/iris/iris_vpu_common.c | 46 ++++++++++++++++------
>  1 file changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index fe9896d66848cdcd8c67bd45bbf3b6ce4a01ab10..d6ce92f3c7544e44dccca26bf6a4c95a720f9229 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -211,33 +211,29 @@ int iris_vpu_prepare_pc(struct iris_core *core)
>  	return -EAGAIN;
>  }
>  
> -static int iris_vpu_power_off_controller(struct iris_core *core)
> +static void iris_vpu_power_off_controller_begin(struct iris_core *core)
>  {
> -	u32 val = 0;
> -	int ret;
> -
>  	writel(MSK_SIGNAL_FROM_TENSILICA | MSK_CORE_POWER_ON, core->reg_base + CPU_CS_X2RPMH);
> +}
>  
> -	writel(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,
> -				 val, val & BIT(0), 200, 2000);
> -	if (ret)
> -		goto disable_power;
> +static int iris_vpu_power_off_controller_end(struct iris_core *core)
> +{
> +	u32 val = 0;
> +	int ret;
>  
>  	writel(REQ_POWER_DOWN_PREP, core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_CONTROL);
>  
>  	ret = readl_poll_timeout(core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_STATUS,
>  				 val, val & BIT(0), 200, 2000);
>  	if (ret)
> -		goto disable_power;
> +		return ret;
>  
>  	writel(0x0, core->reg_base + WRAPPER_DEBUG_BRIDGE_LPI_CONTROL);
>  
>  	ret = readl_poll_timeout(core->reg_base + WRAPPER_DEBUG_BRIDGE_LPI_STATUS,
>  				 val, val == 0, 200, 2000);
>  	if (ret)
> -		goto disable_power;
> +		return ret;
>  
>  	writel(CTL_AXI_CLK_HALT | CTL_CLK_HALT,
>  	       core->reg_base + WRAPPER_TZ_CTL_AXI_CLOCK_CONFIG);
> @@ -245,10 +241,34 @@ static int iris_vpu_power_off_controller(struct iris_core *core)
>  	writel(0x0, core->reg_base + WRAPPER_TZ_QNS4PDXFIFO_RESET);
>  	writel(0x0, core->reg_base + WRAPPER_TZ_CTL_AXI_CLOCK_CONFIG);
>  
> -disable_power:
> +	return 0;
> +}
> +
> +static void iris_vpu_power_off_controller_disable(struct iris_core *core)
> +{
>  	iris_disable_unprepare_clock(core, IRIS_CTRL_CLK);
>  	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
>  	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);
> +}
> +
> +static int iris_vpu_power_off_controller(struct iris_core *core)
> +{
> +	u32 val = 0;
> +	int ret;
> +
> +	iris_vpu_power_off_controller_begin(core);
> +
> +	writel(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,
> +				 val, val & BIT(0), 200, 2000);
> +	if (ret)
> +		goto disable_power;
> +
> +	iris_vpu_power_off_controller_end(core);
> +
> +disable_power:
> +	iris_vpu_power_off_controller_disable(core);
>  
>  	return 0;
>  }
>