Re: [PATCH v2 3/3] media: iris: Add support for SM8750 (VPU v3.5)

[Krzysztof Kozlowski <krzk@kernel.org>]

On 12/08/2025 10:05, Dikshita Agarwal wrote:
>>  
>>  struct platform_clk_data {
>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_gen2.c b/drivers/media/platform/qcom/iris/iris_platform_gen2.c
>> index d3026b2bcb708c7ec31f134f628df7e57b54af4f..c7c384fce2332255ea96da69ef4dc0bc1a24771c 100644
>> --- a/drivers/media/platform/qcom/iris/iris_platform_gen2.c
>> +++ b/drivers/media/platform/qcom/iris/iris_platform_gen2.c
>> @@ -1,6 +1,7 @@
>>  // SPDX-License-Identifier: GPL-2.0-only
>>  /*
>>   * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + * Copyright (c) 2025 Linaro Ltd
> 
> 
> I don't see a need to add a copyright here.

And I see the need, I added there quite a lot of lines.

Look at your commit bb8a95aa038e099f5ec82c466e996b006e05abd7
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bb8a95aa038e099f5ec82c466e996b006e05abd7

and this hunk:
drivers/media/platform/qcom/iris/iris_resources.h

which adds 7 (!) declarations and a copyright.

If you claim you copyright 7 lines of such declarations:

+struct iris_core;
+
+int iris_enable_power_domains(struct iris_core *core, struct device
*pd_dev);
+int iris_disable_power_domains(struct iris_core *core, struct device
*pd_dev);
+int iris_unset_icc_bw(struct iris_core *core);
+int iris_set_icc_bw(struct iris_core *core, unsigned long icc_bw);
+int iris_disable_unprepare_clock(struct iris_core *core, enum
platform_clk_type clk_type);
+int iris_prepare_enable_clock(struct iris_core *core, enum
platform_clk_type clk_type);

then me adding here 68 lines of NEW CREATIVE WORK is copyrightable as well.

Anyway, you cannot reject someone's copyrights. The work is
copyrightable regardless if you see a need.

> 
>> +
>> +	iris_disable_unprepare_clock(core, IRIS_HW_FREERUN_CLK);
>> +	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
>> +}
>> +
>> +static int iris_vpu35_power_off_controller(struct iris_core *core)
>> +{
>> +	u32 clk_rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size;
>> +	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 + 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;
>> +
>> +	writel(0x0, core->reg_base + WRAPPER_IRIS_CPU_NOC_LPI_CONTROL);
>> +
>> +	writel(REQ_POWER_DOWN_PREP, core->reg_base + AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_CONTROL);
> 
> 
> Read initial status of AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_STATUS
> 
> based on value, run the retry loop.
> This loop runs till the desired LPI state is reached i.e. BIT(0) is set,
> and hardware is idle i.e. BIT(1) or BIT(2) are unset. This suggests a
> situation where the hardware might be stuck or slow to transition.
> 
> This sequence was not needed for SM8650 since it doesn't have
> AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_CONTROL/STATUS registers.
> But required for SM8750, so please add.


Sure

> 
> 
>> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_STATUS,
>> +				 val, val & (BIT(0) | BIT(1) | BIT(2)), 15, 1000);
>> +	if (ret)
>> +		goto disable_power> +
>> +	writel(0x0, core->reg_base + AON_WRAPPER_MVP_VIDEO_CTL_NOC_LPI_CONTROL);
>> +
>> +	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;
>> +
>> +disable_power:
>> +	iris_disable_unprepare_clock(core, IRIS_CTRL_CLK);
>> +	iris_disable_unprepare_clock(core, IRIS_CTRL_FREERUN_CLK);
>> +	iris_disable_unprepare_clock(core, IRIS_AXI1_CLK);
>> +
>> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);
>> +
>> +	reset_control_bulk_reset(clk_rst_tbl_size, core->resets);
>> +
>> +	return 0;
>> +}
>> +
>> +static int iris_vpu35_power_on_controller(struct iris_core *core)
>> +{
>> +	u32 rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size;
>> +	int ret;
>> +
>> +	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = reset_control_bulk_reset(rst_tbl_size, core->resets);
>> +	if (ret)
>> +		goto err_disable_power;
> 
> 
> this reset is not needed to power-on this SOC.


Hm, I will trust you on that, thanks.

Best regards,
Krzysztof