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@xxxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> [ Viresh: Support the case of !opp_table and massaged changelog ]
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxx>
Reviewed-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx>

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