Re: [PATCH] OPP: Check for bandwidth values before creating icc paths

[Sibi Sankar <sibis@codeaurora.org>]

On 2020-05-29 10:50, Viresh Kumar wrote:
> On 28-05-20, 00:54, Sibi Sankar wrote:
>> Prevent the core from creating and voting on icc paths when the
>> opp-table does not have the bandwidth values populated. Currently
>> this check is performed on the first OPP node.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/opp/of.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>> 
>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>> index 61fce1284f012..95cf6f1312765 100644
>> --- a/drivers/opp/of.c
>> +++ b/drivers/opp/of.c
>> @@ -338,6 +338,21 @@ int dev_pm_opp_of_find_icc_paths(struct device 
>> *dev,
>>  	struct device_node *np;
>>  	int ret = 0, i, count, num_paths;
>>  	struct icc_path **paths;
>> +	struct property *prop;
>> +
>> +	/* Check for bandwidth values on the first OPP node */
>> +	if (opp_table && opp_table->np) {
>> +		np = of_get_next_available_child(opp_table->np, NULL);
>> +		if (!np) {
>> +			dev_err(dev, "Empty OPP table\n");
>> +			return 0;
>> +		}
>> +
>> +		prop = of_find_property(np, "opp-peak-kBps", NULL);
>> +		of_node_put(np);
>> +		if (!prop || !prop->length)
>> +			return 0;
>> +	}
> 
> This doesn't support the call made from cpufreq-dt driver. Pushed
> this, please give this a try:

Viresh,
Thanks for the patch!

> 
> From: Sibi Sankar <sibis@codeaurora.org>
> Date: Thu, 28 May 2020 00:54:18 +0530
> Subject: [PATCH] opp: Don't parse icc paths unnecessarily
> 
> The DT node of the device may contain interconnect paths while the OPP
> table doesn't have the bandwidth values. There is no need to parse the
> paths in such cases.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> [ Viresh: Support the case of !opp_table and massaged changelog ]
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/of.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 61fce1284f01..8c1bf01f0e50 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -332,13 +332,56 @@ static int _of_opp_alloc_required_opps(struct
> opp_table *opp_table,
>  	return ret;
>  }
> 
> +static int _bandwidth_supported(struct device *dev, struct opp_table
> *opp_table)
> +{
> +	struct device_node *np, *opp_np;
> +	struct property *prop;
> +
> +	if (!opp_table) {
> +		np = of_node_get(dev->of_node);
> +		if (!np)
> +			return -ENODEV;
> +
> +		opp_np = _opp_of_get_opp_desc_node(np, 0);
> +		of_node_put(np);
> +
> +		/* Lets not fail in case we are parsing opp-v1 bindings */
> +		if (!opp_np)
> +			return 0;
> +	} else {
> +		opp_np = of_node_get(opp_table->np);

opp_np needs to be subjected
to NULL check as well. Lets
move "if (!opp_np)" to outside
the if/else. With the above
change in place:

Tested-by: Sibi Sankar <sibis@codeaurora.org>
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>

> +	}
> +
> +	/* Checking only first OPP is sufficient */
> +	np = of_get_next_available_child(opp_np, NULL);
> +	if (!np) {
> +		dev_err(dev, "OPP table empty\n");
> +		return -EINVAL;
> +	}
> +	of_node_put(opp_np);
> +
> +	prop = of_find_property(np, "opp-peak-kBps", NULL);
> +	of_node_put(np);
> +
> +	if (!prop || !prop->length)
> +		return 0;
> +
> +	return 1;
> +}
> +
>  int dev_pm_opp_of_find_icc_paths(struct device *dev,
>  				 struct opp_table *opp_table)
>  {
>  	struct device_node *np;
> -	int ret = 0, i, count, num_paths;
> +	int ret, i, count, num_paths;
>  	struct icc_path **paths;
> 
> +	ret = _bandwidth_supported(dev, opp_table);
> +	if (ret <= 0)
> +		return ret;
> +
> +	ret = 0;
> +
>  	np = of_node_get(dev->of_node);
>  	if (!np)
>  		return 0;

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.