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

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

[Sibi Sankar]

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;
+	}
 
 	np = of_node_get(dev->of_node);
 	if (!np)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Viresh Kumar]

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:

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);
+	}
+
+	/* 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;

-- 
viresh

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

[Sibi Sankar]

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.

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

[Viresh Kumar]

On 29-05-20, 19:47, Sibi Sankar wrote:
> opp_np needs to be subjected
> to NULL check as well.

No, it isn't. It should already be valid and is set by the OPP core.
Actually we don't need to do of_node_get(opp_table->np) and just use
np, I did that to not have a special case while putting the resource.

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

Thanks.

-- 
viresh

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

[Sibi Sankar]

On 2020-06-01 09:37, Viresh Kumar wrote:
> On 29-05-20, 19:47, Sibi Sankar wrote:
>> opp_np needs to be subjected
>> to NULL check as well.
> 
> No, it isn't. It should already be valid and is set by the OPP core.
> Actually we don't need to do of_node_get(opp_table->np) and just use
> np, I did that to not have a special case while putting the resource.
> 

I should have phrased it differently.
opp_np needs to be checked to deal
with cases where devices don't have
"operating-points-v2" associated with
it.

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index a5d87ca0ab571..06976d14e6ccb 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -344,14 +344,14 @@ static int _bandwidth_supported(struct device 
*dev, struct opp_table *opp_table)

                 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);
         }

+       /* Lets not fail in case we are parsing opp-v1 bindings */
+       if (!opp_np)
+               return 0;
+

sdhci_msm 7c4000.sdhci: OPP table empty
sdhci_msm 7c4000.sdhci: _allocate_opp_table: Error finding interconnect 
paths: -22

I see the following errors without
the check.


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

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

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

[Viresh Kumar]

On 01-06-20, 12:09, Sibi Sankar wrote:
> On 2020-06-01 09:37, Viresh Kumar wrote:
> > On 29-05-20, 19:47, Sibi Sankar wrote:
> > > opp_np needs to be subjected
> > > to NULL check as well.
> > 
> > No, it isn't. It should already be valid and is set by the OPP core.
> > Actually we don't need to do of_node_get(opp_table->np) and just use
> > np, I did that to not have a special case while putting the resource.
> > 
> 
> I should have phrased it differently.
> opp_np needs to be checked to deal
> with cases where devices don't have
> "operating-points-v2" associated with
> it.
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index a5d87ca0ab571..06976d14e6ccb 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -344,14 +344,14 @@ static int _bandwidth_supported(struct device *dev,
> struct opp_table *opp_table)
> 
>                 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);
>         }
> 
> +       /* Lets not fail in case we are parsing opp-v1 bindings */
> +       if (!opp_np)
> +               return 0;
> +
> 
> sdhci_msm 7c4000.sdhci: OPP table empty
> sdhci_msm 7c4000.sdhci: _allocate_opp_table: Error finding interconnect
> paths: -22
> 
> I see the following errors without
> the check.

My reply unfortunately only considered the case where this routine was
called from within the opp table. Are you testing it for the case
where you are adding OPPs dynamically from the code ?

-- 
viresh

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

[Sibi Sankar]

On 2020-06-01 12:43, Viresh Kumar wrote:
> On 01-06-20, 12:09, Sibi Sankar wrote:
>> On 2020-06-01 09:37, Viresh Kumar wrote:
>> > On 29-05-20, 19:47, Sibi Sankar wrote:
>> > > opp_np needs to be subjected
>> > > to NULL check as well.
>> >
>> > No, it isn't. It should already be valid and is set by the OPP core.
>> > Actually we don't need to do of_node_get(opp_table->np) and just use
>> > np, I did that to not have a special case while putting the resource.
>> >
>> 
>> I should have phrased it differently.
>> opp_np needs to be checked to deal
>> with cases where devices don't have
>> "operating-points-v2" associated with
>> it.
>> 
>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>> index a5d87ca0ab571..06976d14e6ccb 100644
>> --- a/drivers/opp/of.c
>> +++ b/drivers/opp/of.c
>> @@ -344,14 +344,14 @@ static int _bandwidth_supported(struct device 
>> *dev,
>> struct opp_table *opp_table)
>> 
>>                 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);
>>         }
>> 
>> +       /* Lets not fail in case we are parsing opp-v1 bindings */
>> +       if (!opp_np)
>> +               return 0;
>> +
>> 
>> sdhci_msm 7c4000.sdhci: OPP table empty
>> sdhci_msm 7c4000.sdhci: _allocate_opp_table: Error finding 
>> interconnect
>> paths: -22
>> 
>> I see the following errors without
>> the check.
> 
> My reply unfortunately only considered the case where this routine was
> called from within the opp table. Are you testing it for the case
> where you are adding OPPs dynamically from the code ?

Yeah dev_pm_opp_add/dev_pm_opp_set_clkname
or pretty much any api doing a
dev_pm_opp_get_opp_table without
a opp_table node associated with
it will run into this issue.

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

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

[Viresh Kumar]

On 01-06-20, 15:30, Sibi Sankar wrote:
> Yeah dev_pm_opp_add/dev_pm_opp_set_clkname
> or pretty much any api doing a
> dev_pm_opp_get_opp_table without
> a opp_table node associated with
> it will run into this issue.

Not sure if what you wrote now is correct, the problem shouldn't
happen from within dev_pm_opp_set_clkname() but only when we try to do
bw thing.

Anyway, I have pushed the change already.

-- 
viresh

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

[Sibi Sankar]

On 2020-06-01 15:45, Viresh Kumar wrote:
> On 01-06-20, 15:30, Sibi Sankar wrote:
>> Yeah dev_pm_opp_add/dev_pm_opp_set_clkname
>> or pretty much any api doing a
>> dev_pm_opp_get_opp_table without
>> a opp_table node associated with
>> it will run into this issue.
> 
> Not sure if what you wrote now is correct, the problem shouldn't
> happen from within dev_pm_opp_set_clkname() but only when we try to do
> bw thing.
> 
> Anyway, I have pushed the change already.

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