Re: [PATCH v3 04/29] media: iris: initialize power resources

[Dikshita Agarwal]

On 8/27/2024 4:21 PM, Krzysztof Kozlowski wrote:
> On 27/08/2024 12:05, Dikshita Agarwal via B4 Relay wrote:
>> From: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
>>
>> Add support for initializing Iris "resources", which are clocks,
>> interconnects, power domains, reset clocks, and clock frequencies
>> used for iris hardware.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
>> ---
> 
> ...
> 
>> +struct iris_platform_data sm8550_data = {
>> +	.icc_tbl = sm8550_icc_table,
>> +	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
>> +	.clk_rst_tbl = sm8550_clk_reset_table,
>> +	.clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table),
>> +	.pmdomain_tbl = sm8550_pmdomain_table,
>> +	.pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table),
>> +	.opp_pd_tbl = sm8550_opp_pd_table,
>> +	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
>> +	.clk_tbl = sm8550_clk_table,
>> +	.clk_tbl_size = ARRAY_SIZE(sm8550_clk_table),
>> +};
>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
>> index 0a54fdaa1ab5..2616a31224f9 100644
>> --- a/drivers/media/platform/qcom/iris/iris_probe.c
>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
>> @@ -69,6 +69,19 @@ static int iris_probe(struct platform_device *pdev)
>>  	if (core->irq < 0)
>>  		return core->irq;
>>  
>> +	core->iris_platform_data = of_device_get_match_data(core->dev);
>> +	if (!core->iris_platform_data) {
>> +		ret = -ENODEV;
>> +		dev_err_probe(core->dev, ret, "init platform failed\n");
> 
> That's not even possible. I would suggest dropping entire if. But if yoi
> insist, then without this weird redundant code. return -EINVAL.
> 
Its possible if platform data is not initialized and this is only place we
check it, there is no further NULL check for the same.
>> +		return ret;
>> +	}
>> +
>> +	ret = iris_init_resources(core);
>> +	if (ret) {
>> +		dev_err_probe(core->dev, ret, "init resource failed\n");
>> +		return ret;
> 
> How many same errors are you printing? Not mentioning that syntax of
> dev_errp_rpboe is different...
We have these errors at multiple points to know at what point the probe
failed which is useful while debugging probe failures.
But Sure we will revisit this code and fix the syntax of dev_err_probe.
> 
> 
>> +	}
>> +
>>  	ret = v4l2_device_register(dev, &core->v4l2_dev);
>>  	if (ret)
>>  		return ret;
>> @@ -88,8 +101,14 @@ static int iris_probe(struct platform_device *pdev)
>>  }
>>  
>>  static const struct of_device_id iris_dt_match[] = {
>> -	{ .compatible = "qcom,sm8550-iris", },
>> -	{ .compatible = "qcom,sm8250-venus", },
>> +	{
>> +		.compatible = "qcom,sm8550-iris",
>> +		.data = &sm8550_data,
>> +	},
>> +	{
>> +		.compatible = "qcom,sm8250-venus",
>> +		.data = &sm8250_data,
> 
> You just added this. No, please do not add code which is immediatly
> incorrect.
It's not incorrect, in earlier patch we only added the compatible strings
and with this patch introducing the platform data and APIs to get it.
> 
>> +	},
>>  	{ },
>>  };
>>  MODULE_DEVICE_TABLE(of, iris_dt_match);
>> diff --git a/drivers/media/platform/qcom/iris/iris_resources.c b/drivers/media/platform/qcom/iris/iris_resources.c
>> new file mode 100644
>> index 000000000000..57c6f9f3449b
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/iris/iris_resources.c
>> @@ -0,0 +1,171 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/interconnect.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/pm_opp.h>
>> +#include <linux/reset.h>
>> +
>> +#include "iris_core.h"
>> +#include "iris_resources.h"
>> +
>> +static int iris_init_icc(struct iris_core *core)
>> +{
>> +	const struct icc_info *icc_tbl;
>> +	u32 ret, i = 0;
>> +
>> +	icc_tbl = core->iris_platform_data->icc_tbl;
>> +
>> +	core->icc_count = core->iris_platform_data->icc_tbl_size;
>> +	core->icc_tbl = devm_kzalloc(core->dev,
>> +				     sizeof(struct icc_bulk_data) * core->icc_count,
>> +				     GFP_KERNEL);
>> +	if (!core->icc_tbl)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < core->icc_count; i++) {
>> +		core->icc_tbl[i].name = icc_tbl[i].name;
>> +		core->icc_tbl[i].avg_bw = icc_tbl[i].bw_min_kbps;
>> +		core->icc_tbl[i].peak_bw = 0;
>> +	}
>> +
>> +	ret = devm_of_icc_bulk_get(core->dev, core->icc_count, core->icc_tbl);
>> +	if (ret)
>> +		dev_err(core->dev, "failed to get interconnect paths, NoC will stay unconfigured!\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int iris_pd_get(struct iris_core *core)
>> +{
>> +	int ret;
>> +
>> +	struct dev_pm_domain_attach_data iris_pd_data = {
>> +		.pd_names = core->iris_platform_data->pmdomain_tbl,
>> +		.num_pd_names = core->iris_platform_data->pmdomain_tbl_size,
>> +		.pd_flags = PD_FLAG_NO_DEV_LINK,
>> +	};
>> +
>> +	ret = devm_pm_domain_attach_list(core->dev, &iris_pd_data, &core->pmdomain_tbl);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int iris_opp_pd_get(struct iris_core *core)
>> +{
>> +	int ret;
>> +
>> +	struct dev_pm_domain_attach_data iris_opp_pd_data = {
>> +		.pd_names = core->iris_platform_data->opp_pd_tbl,
>> +		.num_pd_names = core->iris_platform_data->opp_pd_tbl_size,
>> +		.pd_flags = PD_FLAG_DEV_LINK_ON,
>> +	};
>> +
>> +	ret = devm_pm_domain_attach_list(core->dev, &iris_opp_pd_data, &core->opp_pmdomain_tbl);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int iris_init_power_domains(struct iris_core *core)
>> +{
>> +	const struct platform_clk_data *clk_tbl;
>> +	u32 clk_cnt, i;
>> +	int ret;
>> +
>> +	ret = iris_pd_get(core);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iris_opp_pd_get(core);
>> +	if (ret)
>> +		return ret;
>> +
>> +	clk_tbl = core->iris_platform_data->clk_tbl;
>> +	clk_cnt = core->iris_platform_data->clk_tbl_size;
>> +
>> +	for (i = 0; i < clk_cnt; i++) {
>> +		if (clk_tbl[i].clk_type == IRIS_HW_CLK) {
>> +			ret = devm_pm_opp_set_clkname(core->dev, clk_tbl[i].clk_name);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +
>> +	ret = devm_pm_opp_of_add_table(core->dev);
>> +	if (ret) {
>> +		dev_err(core->dev, "failed to add opp table\n");
>> +		return ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int iris_init_clocks(struct iris_core *core)
>> +{
>> +	int ret;
>> +
>> +	ret = devm_clk_bulk_get_all(core->dev, &core->clock_tbl);
>> +	if (ret < 0) {
>> +		dev_err(core->dev, "failed to get bulk clock\n");
> 
> Syntax is:
> return dev_err_probe(). If this is probe path. Is it?
> 
Sure will fix.
>> +		return ret;
>> +	}
>> +
>> +	core->clk_count = ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int iris_init_resets(struct iris_core *core)
>> +{
>> +	const char * const *rst_tbl;
>> +	u32 rst_tbl_size;
>> +	u32 i = 0, ret;
>> +
>> +	rst_tbl = core->iris_platform_data->clk_rst_tbl;
>> +	rst_tbl_size = core->iris_platform_data->clk_rst_tbl_size;
>> +
>> +	core->resets = devm_kzalloc(core->dev,
>> +				    sizeof(*core->resets) * rst_tbl_size,
>> +				    GFP_KERNEL);
>> +	if (rst_tbl_size && !core->resets)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < rst_tbl_size; i++)
>> +		core->resets[i].id = rst_tbl[i];
>> +
>> +	ret = devm_reset_control_bulk_get_exclusive(core->dev, rst_tbl_size, core->resets);
>> +	if (ret) {
>> +		dev_err(core->dev, "failed to get resets\n");
> 
> Syntax is:
> return dev_err_probe(). If this is probe path. Is it?
> 
Sure, will fix
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int iris_init_resources(struct iris_core *core)
>> +{
>> +	int ret;
>> +
>> +	ret = iris_init_icc(core);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iris_init_power_domains(core);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iris_init_clocks(core);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = iris_init_resets(core);
>> +
>> +	return ret;
>> +}
> 
> This should be just part of of main unit file, next to probe. It is
> unusual to see probe parts not next to probe. Sorry, that's wrong.
> 
All the APIs handling(init/enable/disable) the different resources (PM
domains, OPP, clocks, buses) are kept in this iris_resource.c file hence
this API to init all those resources is kept here to not load iris_probe.c
file.

> Best regards,
> Krzysztof
>