[PATCH 1/6] PM / OPP: reuse of_parse_phandle()

[PATCH 1/6] PM / OPP: reuse of_parse_phandle()

[Viresh Kumar]

We already have a better API to get the opp descriptor block's node from
cpu-node. Lets reuse that instead of creating our own routines for the
same stuff. That cleans the code a lot.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 70 +++++++++++++-----------------------------------
 1 file changed, 18 insertions(+), 52 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 204c6c945168..1daaa1a418a2 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1250,69 +1250,33 @@ void of_cpumask_free_opp_table(cpumask_var_t cpumask)
 }
 EXPORT_SYMBOL_GPL(of_cpumask_free_opp_table);
 
-/* Returns opp descriptor node from its phandle. Caller must do of_node_put() */
-static struct device_node *
-_of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop)
-{
-	struct device_node *opp_np;
-
-	opp_np = of_find_node_by_phandle(be32_to_cpup(prop->value));
-	if (!opp_np) {
-		dev_err(dev, "%s: Prop: %s contains invalid opp desc phandle\n",
-			__func__, prop->name);
-		return ERR_PTR(-EINVAL);
-	}
-
-	return opp_np;
-}
-
-/* Returns opp descriptor node for a device. Caller must do of_node_put() */
+/* Returns opp descriptor node for a device, caller must do of_node_put() */
 static struct device_node *_of_get_opp_desc_node(struct device *dev)
 {
-	const struct property *prop;
-
-	prop = of_find_property(dev->of_node, "operating-points-v2", NULL);
-	if (!prop)
-		return ERR_PTR(-ENODEV);
-	if (!prop->value)
-		return ERR_PTR(-ENODATA);
-
 	/*
 	 * TODO: Support for multiple OPP tables.
 	 *
 	 * There should be only ONE phandle present in "operating-points-v2"
 	 * property.
 	 */
-	if (prop->length != sizeof(__be32)) {
-		dev_err(dev, "%s: Invalid opp desc phandle\n", __func__);
-		return ERR_PTR(-EINVAL);
-	}
 
-	return _of_get_opp_desc_node_from_prop(dev, prop);
+	return of_parse_phandle(dev->of_node, "operating-points-v2", 0);
 }
 
 /* Initializes OPP tables based on new bindings */
 static int _of_init_opp_table_v2(struct device *dev,
-				 const struct property *prop)
+				 struct device_node *opp_np)
 {
-	struct device_node *opp_np, *np;
+	struct device_node *np;
 	struct device_opp *dev_opp;
 	int ret = 0, count = 0;
 
-	if (!prop->value)
-		return -ENODATA;
-
-	/* Get opp node */
-	opp_np = _of_get_opp_desc_node_from_prop(dev, prop);
-	if (IS_ERR(opp_np))
-		return PTR_ERR(opp_np);
-
 	dev_opp = _managed_opp(opp_np);
 	if (dev_opp) {
 		/* OPPs are already managed */
 		if (!_add_list_dev(dev, dev_opp))
 			ret = -ENOMEM;
-		goto put_opp_np;
+		goto out;
 	}
 
 	/* We have opp-list node now, iterate over it and add OPPs */
@@ -1329,13 +1293,13 @@ static int _of_init_opp_table_v2(struct device *dev,
 
 	/* There should be one of more OPP defined */
 	if (WARN_ON(!count))
-		goto put_opp_np;
+		goto out;
 
 	if (!ret) {
 		if (!dev_opp) {
 			dev_opp = _find_device_opp(dev);
 			if (WARN_ON(!dev_opp))
-				goto put_opp_np;
+				goto out;
 		}
 
 		dev_opp->np = opp_np;
@@ -1345,9 +1309,7 @@ static int _of_init_opp_table_v2(struct device *dev,
 		of_free_opp_table(dev);
 	}
 
-put_opp_np:
-	of_node_put(opp_np);
-
+out:
 	return ret;
 }
 
@@ -1413,14 +1375,15 @@ static int _of_init_opp_table_v1(struct device *dev)
  */
 int of_init_opp_table(struct device *dev)
 {
-	const struct property *prop;
+	struct device_node *opp_np;
+	int ret;
 
 	/*
 	 * OPPs have two version of bindings now. The older one is deprecated,
 	 * try for the new binding first.
 	 */
-	prop = of_find_property(dev->of_node, "operating-points-v2", NULL);
-	if (!prop) {
+	opp_np = _of_get_opp_desc_node(dev);
+	if (!opp_np) {
 		/*
 		 * Try old-deprecated bindings for backward compatibility with
 		 * older dtbs.
@@ -1428,7 +1391,10 @@ int of_init_opp_table(struct device *dev)
 		return _of_init_opp_table_v1(dev);
 	}
 
-	return _of_init_opp_table_v2(dev, prop);
+	ret = _of_init_opp_table_v2(dev, opp_np);
+	of_node_put(opp_np);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 
@@ -1517,7 +1483,7 @@ int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
 
 	/* Get OPP descriptor node */
 	np = _of_get_opp_desc_node(cpu_dev);
-	if (IS_ERR(np)) {
+	if (!np) {
 		dev_dbg(cpu_dev, "%s: Couldn't find opp node: %ld\n", __func__,
 			PTR_ERR(np));
 		return -ENOENT;
@@ -1541,7 +1507,7 @@ int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
 
 		/* Get OPP descriptor node */
 		tmp_np = _of_get_opp_desc_node(tcpu_dev);
-		if (IS_ERR(tmp_np)) {
+		if (!tmp_np) {
 			dev_err(tcpu_dev, "%s: Couldn't find opp node: %ld\n",
 				__func__, PTR_ERR(tmp_np));
 			ret = PTR_ERR(tmp_np);
-- 
2.4.0

Re: [PATCH 1/6] PM / OPP: reuse of_parse_phandle()

[Stephen Boyd]

On 08/10, Viresh Kumar wrote:
> We already have a better API to get the opp descriptor block's node from
> cpu-node. Lets reuse that instead of creating our own routines for the
> same stuff. That cleans the code a lot.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 204c6c945168..1daaa1a418a2 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
>  	/*
>  	 * TODO: Support for multiple OPP tables.
>  	 *
>  	 * There should be only ONE phandle present in "operating-points-v2"
>  	 * property.
>  	 */
> -	if (prop->length != sizeof(__be32)) {
> -		dev_err(dev, "%s: Invalid opp desc phandle\n", __func__);
> -		return ERR_PTR(-EINVAL);
> -	}

But we lost this check? Perhaps we can use
of_count_phandle_with_args() to make suer we only have one
phandle?

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

Re: [PATCH 1/6] PM / OPP: reuse of_parse_phandle()

[Viresh Kumar]

On 10-08-15, 23:02, Stephen Boyd wrote:
> > -	if (prop->length != sizeof(__be32)) {
> > -		dev_err(dev, "%s: Invalid opp desc phandle\n", __func__);
> > -		return ERR_PTR(-EINVAL);
> > -	}
> 
> But we lost this check? Perhaps we can use
> of_count_phandle_with_args() to make suer we only have one
> phandle?

I thought about it earlier and it looked like we don't need to care
about this. Even if the user passes multiple strings here, its his
problem. We will just pick the first entry and parse it.

And that's true until the point we support multiple table entries,
ofcourse :)

Now that you are still awake, let me update the other patches as well
where u commented :)

-- 
viresh

Re: [PATCH 1/6] PM / OPP: reuse of_parse_phandle()

[Stephen Boyd]

On 08/11, Viresh Kumar wrote:
> On 10-08-15, 23:02, Stephen Boyd wrote:
> > > -	if (prop->length != sizeof(__be32)) {
> > > -		dev_err(dev, "%s: Invalid opp desc phandle\n", __func__);
> > > -		return ERR_PTR(-EINVAL);
> > > -	}
> > 
> > But we lost this check? Perhaps we can use
> > of_count_phandle_with_args() to make suer we only have one
> > phandle?
> 
> I thought about it earlier and it looked like we don't need to care
> about this. Even if the user passes multiple strings here, its his
> problem. We will just pick the first entry and parse it.
> 
> And that's true until the point we support multiple table entries,
> ofcourse :)

Ok. That's worth a mention in the commit text please.

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

Re: [PATCH 1/6] PM / OPP: reuse of_parse_phandle()

[Viresh Kumar]

On 10-08-15, 23:23, Stephen Boyd wrote:
> Ok. That's worth a mention in the commit text please.

-----------------------8<------------------------

Message-Id: <8c4503fe1c1c545d5f7ac68351d81d0238532b54.1439275133.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Thu, 30 Jul 2015 16:58:19 +0530
Subject: [PATCH] PM / OPP: reuse of_parse_phandle()

We already have a better API to get the opp descriptor block's node from
cpu-node. Lets reuse that instead of creating our own routines for the
same stuff. That cleans the code a lot.

This also kills a check we had earlier (as we are using the generic API
now). Earlier we used to check if the operating-points-v2 property
contained multiple phandles instead of a single phandle.

Killing this check isn't an issue because, we only parse the first entry
with of_parse_phandle(). So, if a user passes multiple phandles, its
really his problem :)

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 70 +++++++++++++-----------------------------------
 1 file changed, 18 insertions(+), 52 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 204c6c945168..1daaa1a418a2 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1250,69 +1250,33 @@ void of_cpumask_free_opp_table(cpumask_var_t cpumask)
 }
 EXPORT_SYMBOL_GPL(of_cpumask_free_opp_table);
 
-/* Returns opp descriptor node from its phandle. Caller must do of_node_put() */
-static struct device_node *
-_of_get_opp_desc_node_from_prop(struct device *dev, const struct property *prop)
-{
-	struct device_node *opp_np;
-
-	opp_np = of_find_node_by_phandle(be32_to_cpup(prop->value));
-	if (!opp_np) {
-		dev_err(dev, "%s: Prop: %s contains invalid opp desc phandle\n",
-			__func__, prop->name);
-		return ERR_PTR(-EINVAL);
-	}
-
-	return opp_np;
-}
-
-/* Returns opp descriptor node for a device. Caller must do of_node_put() */
+/* Returns opp descriptor node for a device, caller must do of_node_put() */
 static struct device_node *_of_get_opp_desc_node(struct device *dev)
 {
-	const struct property *prop;
-
-	prop = of_find_property(dev->of_node, "operating-points-v2", NULL);
-	if (!prop)
-		return ERR_PTR(-ENODEV);
-	if (!prop->value)
-		return ERR_PTR(-ENODATA);
-
 	/*
 	 * TODO: Support for multiple OPP tables.
 	 *
 	 * There should be only ONE phandle present in "operating-points-v2"
 	 * property.
 	 */
-	if (prop->length != sizeof(__be32)) {
-		dev_err(dev, "%s: Invalid opp desc phandle\n", __func__);
-		return ERR_PTR(-EINVAL);
-	}
 
-	return _of_get_opp_desc_node_from_prop(dev, prop);
+	return of_parse_phandle(dev->of_node, "operating-points-v2", 0);
 }
 
 /* Initializes OPP tables based on new bindings */
 static int _of_init_opp_table_v2(struct device *dev,
-				 const struct property *prop)
+				 struct device_node *opp_np)
 {
-	struct device_node *opp_np, *np;
+	struct device_node *np;
 	struct device_opp *dev_opp;
 	int ret = 0, count = 0;
 
-	if (!prop->value)
-		return -ENODATA;
-
-	/* Get opp node */
-	opp_np = _of_get_opp_desc_node_from_prop(dev, prop);
-	if (IS_ERR(opp_np))
-		return PTR_ERR(opp_np);
-
 	dev_opp = _managed_opp(opp_np);
 	if (dev_opp) {
 		/* OPPs are already managed */
 		if (!_add_list_dev(dev, dev_opp))
 			ret = -ENOMEM;
-		goto put_opp_np;
+		goto out;
 	}
 
 	/* We have opp-list node now, iterate over it and add OPPs */
@@ -1329,13 +1293,13 @@ static int _of_init_opp_table_v2(struct device *dev,
 
 	/* There should be one of more OPP defined */
 	if (WARN_ON(!count))
-		goto put_opp_np;
+		goto out;
 
 	if (!ret) {
 		if (!dev_opp) {
 			dev_opp = _find_device_opp(dev);
 			if (WARN_ON(!dev_opp))
-				goto put_opp_np;
+				goto out;
 		}
 
 		dev_opp->np = opp_np;
@@ -1345,9 +1309,7 @@ static int _of_init_opp_table_v2(struct device *dev,
 		of_free_opp_table(dev);
 	}
 
-put_opp_np:
-	of_node_put(opp_np);
-
+out:
 	return ret;
 }
 
@@ -1413,14 +1375,15 @@ static int _of_init_opp_table_v1(struct device *dev)
  */
 int of_init_opp_table(struct device *dev)
 {
-	const struct property *prop;
+	struct device_node *opp_np;
+	int ret;
 
 	/*
 	 * OPPs have two version of bindings now. The older one is deprecated,
 	 * try for the new binding first.
 	 */
-	prop = of_find_property(dev->of_node, "operating-points-v2", NULL);
-	if (!prop) {
+	opp_np = _of_get_opp_desc_node(dev);
+	if (!opp_np) {
 		/*
 		 * Try old-deprecated bindings for backward compatibility with
 		 * older dtbs.
@@ -1428,7 +1391,10 @@ int of_init_opp_table(struct device *dev)
 		return _of_init_opp_table_v1(dev);
 	}
 
-	return _of_init_opp_table_v2(dev, prop);
+	ret = _of_init_opp_table_v2(dev, opp_np);
+	of_node_put(opp_np);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 
@@ -1517,7 +1483,7 @@ int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
 
 	/* Get OPP descriptor node */
 	np = _of_get_opp_desc_node(cpu_dev);
-	if (IS_ERR(np)) {
+	if (!np) {
 		dev_dbg(cpu_dev, "%s: Couldn't find opp node: %ld\n", __func__,
 			PTR_ERR(np));
 		return -ENOENT;
@@ -1541,7 +1507,7 @@ int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
 
 		/* Get OPP descriptor node */
 		tmp_np = _of_get_opp_desc_node(tcpu_dev);
-		if (IS_ERR(tmp_np)) {
+		if (!tmp_np) {
 			dev_err(tcpu_dev, "%s: Couldn't find opp node: %ld\n",
 				__func__, PTR_ERR(tmp_np));
 			ret = PTR_ERR(tmp_np);

[PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

[Viresh Kumar]

'dev_opp' will always be NULL in _of_init_opp_table_v2() after creating
OPPs for a device. There is no point comparing it against NULL there.

Restructure code a bit to make it more efficient.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 1daaa1a418a2..c9747fb192b1 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1295,20 +1295,19 @@ static int _of_init_opp_table_v2(struct device *dev,
 	if (WARN_ON(!count))
 		goto out;
 
-	if (!ret) {
-		if (!dev_opp) {
-			dev_opp = _find_device_opp(dev);
-			if (WARN_ON(!dev_opp))
-				goto out;
-		}
-
-		dev_opp->np = opp_np;
-		dev_opp->shared_opp = of_property_read_bool(opp_np,
-							    "opp-shared");
-	} else {
+	if (ret) {
 		of_free_opp_table(dev);
+		goto out;
 	}
 
+	dev_opp = _find_device_opp(dev);
+	if (WARN_ON(!dev_opp))
+		goto out;
+
+	dev_opp->np = opp_np;
+	dev_opp->shared_opp = of_property_read_bool(opp_np,
+			"opp-shared");
+
 out:
 	return ret;
 }
-- 
2.4.0

Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

[Stephen Boyd]

On 08/10, Viresh Kumar wrote:
> 'dev_opp' will always be NULL in _of_init_opp_table_v2() after creating
> OPPs for a device. There is no point comparing it against NULL there.
> 
> Restructure code a bit to make it more efficient.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Curious if these are a response to the static checker mails? If
so it would be good to add a reported-by tag.

> ---
>  drivers/base/power/opp.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 1daaa1a418a2..c9747fb192b1 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -1295,20 +1295,19 @@ static int _of_init_opp_table_v2(struct device *dev,
>  	if (WARN_ON(!count))
>  		goto out;
>  
> -	if (!ret) {
> -		if (!dev_opp) {
> -			dev_opp = _find_device_opp(dev);
> -			if (WARN_ON(!dev_opp))
> -				goto out;
> -		}
> -
> -		dev_opp->np = opp_np;
> -		dev_opp->shared_opp = of_property_read_bool(opp_np,
> -							    "opp-shared");
> -	} else {
> +	if (ret) {
>  		of_free_opp_table(dev);
> +		goto out;
>  	}
>  
> +	dev_opp = _find_device_opp(dev);
> +	if (WARN_ON(!dev_opp))
> +		goto out;

Doesn't ret = 0 in this case? Why not drop the goto and just
return some error code. Same for the goto out up above.

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

Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

[Viresh Kumar]

On 10-08-15, 12:23, Stephen Boyd wrote:
> On 08/10, Viresh Kumar wrote:
> > 'dev_opp' will always be NULL in _of_init_opp_table_v2() after creating
> > OPPs for a device. There is no point comparing it against NULL there.
> > 
> > Restructure code a bit to make it more efficient.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Curious if these are a response to the static checker mails? If
> so it would be good to add a reported-by tag.

No it wasn't, I had these waiting in my queue even before Rafael
applied the earlier ones. I was waiting for them to get in before
sending more stuff :)

> > ---
> >  drivers/base/power/opp.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> > index 1daaa1a418a2..c9747fb192b1 100644
> > --- a/drivers/base/power/opp.c
> > +++ b/drivers/base/power/opp.c
> > @@ -1295,20 +1295,19 @@ static int _of_init_opp_table_v2(struct device *dev,
> >  	if (WARN_ON(!count))
> >  		goto out;
> >  
> > -	if (!ret) {
> > -		if (!dev_opp) {
> > -			dev_opp = _find_device_opp(dev);
> > -			if (WARN_ON(!dev_opp))
> > -				goto out;
> > -		}
> > -
> > -		dev_opp->np = opp_np;
> > -		dev_opp->shared_opp = of_property_read_bool(opp_np,
> > -							    "opp-shared");
> > -	} else {
> > +	if (ret) {
> >  		of_free_opp_table(dev);
> > +		goto out;
> >  	}
> >  
> > +	dev_opp = _find_device_opp(dev);
> > +	if (WARN_ON(!dev_opp))
> > +		goto out;
> 
> Doesn't ret = 0 in this case?

Because ret is already 0, juse see the above if (ret) check.

> Why not drop the goto and just
> return some error code. Same for the goto out up above.

Actually yes, because we don't do anything special in goto now. But it
required more (unrelated) code changes, plus I didn't wanted to break
the 'return from single place' rule for this function, in case we
really need to free some resource or undo some work from the goto
place.

But if you suggest/insist, then I will do it in a separate patch.

-- 
viresh

Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

[Stephen Boyd]

On 08/10/2015 05:23 PM, Viresh Kumar wrote:
> On 10-08-15, 12:23, Stephen Boyd wrote:
>>
>>> ---
>>>   drivers/base/power/opp.c | 21 ++++++++++-----------
>>>   1 file changed, 10 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>>> index 1daaa1a418a2..c9747fb192b1 100644
>>> --- a/drivers/base/power/opp.c
>>> +++ b/drivers/base/power/opp.c
>>> @@ -1295,20 +1295,19 @@ static int _of_init_opp_table_v2(struct device *dev,
>>>   	if (WARN_ON(!count))
>>>   		goto out;
>>>   
>>> -	if (!ret) {
>>> -		if (!dev_opp) {
>>> -			dev_opp = _find_device_opp(dev);
>>> -			if (WARN_ON(!dev_opp))
>>> -				goto out;
>>> -		}
>>> -
>>> -		dev_opp->np = opp_np;
>>> -		dev_opp->shared_opp = of_property_read_bool(opp_np,
>>> -							    "opp-shared");
>>> -	} else {
>>> +	if (ret) {
>>>   		of_free_opp_table(dev);
>>> +		goto out;
>>>   	}
>>>   
>>> +	dev_opp = _find_device_opp(dev);
>>> +	if (WARN_ON(!dev_opp))
>>> +		goto out;
>> Doesn't ret = 0 in this case?
> Because ret is already 0, juse see the above if (ret) check.

So ret is 0. I thought it was an error path, but I guess this is a 
warning path and we return 0 still?

>
>> Why not drop the goto and just
>> return some error code. Same for the goto out up above.
> Actually yes, because we don't do anything special in goto now. But it
> required more (unrelated) code changes, plus I didn't wanted to break
> the 'return from single place' rule for this function, in case we
> really need to free some resource or undo some work from the goto
> place.
>
> But if you suggest/insist, then I will do it in a separate patch.

I am not insisting anything. But another patch to get rid of the goto 
sounds fine.

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

Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

[Viresh Kumar]

On 10-08-15, 17:31, Stephen Boyd wrote:
> So ret is 0. I thought it was an error path, but I guess this is a
> warning path and we return 0 still?

Urg ..

-------------------------8<-------------------------

Message-Id: <d0e8a9cd6b0fb38fa946cb6274f258d7aa66c00e.1439259818.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 4 Aug 2015 11:57:36 +0530
Subject: [PATCH] PM / OPP: restructure _of_init_opp_table_v2()

'dev_opp' will always be NULL in _of_init_opp_table_v2() after creating
OPPs for a device. There is no point comparing it against NULL there.

Restructure code a bit to make it more efficient.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 1daaa1a418a2..f42e098b71fe 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1295,20 +1295,21 @@ static int _of_init_opp_table_v2(struct device *dev,
 	if (WARN_ON(!count))
 		goto out;
 
-	if (!ret) {
-		if (!dev_opp) {
-			dev_opp = _find_device_opp(dev);
-			if (WARN_ON(!dev_opp))
-				goto out;
-		}
-
-		dev_opp->np = opp_np;
-		dev_opp->shared_opp = of_property_read_bool(opp_np,
-							    "opp-shared");
-	} else {
+	if (ret) {
 		of_free_opp_table(dev);
+		goto out;
 	}
 
+	dev_opp = _find_device_opp(dev);
+	if (WARN_ON(!dev_opp)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	dev_opp->np = opp_np;
+	dev_opp->shared_opp = of_property_read_bool(opp_np,
+			"opp-shared");
+
 out:
 	return ret;
 }

Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

[Stephen Boyd]

On 08/11, Viresh Kumar wrote:
> On 10-08-15, 17:31, Stephen Boyd wrote:
> > So ret is 0. I thought it was an error path, but I guess this is a
> > warning path and we return 0 still?
> 
> Urg ..

Oh I see this is an existing problem... Same problem goes for the
count check. It may be better to fix those two cases first and
then do this cleanup. But I don't mind either way.

> 
> -------------------------8<-------------------------
> 
> Message-Id: <d0e8a9cd6b0fb38fa946cb6274f258d7aa66c00e.1439259818.git.viresh.kumar@linaro.org>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Tue, 4 Aug 2015 11:57:36 +0530
> Subject: [PATCH] PM / OPP: restructure _of_init_opp_table_v2()
> 
> 'dev_opp' will always be NULL in _of_init_opp_table_v2() after creating
> OPPs for a device. There is no point comparing it against NULL there.
> 
> Restructure code a bit to make it more efficient.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

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

Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

[Viresh Kumar]

On 10-08-15, 23:08, Stephen Boyd wrote:
> On 08/11, Viresh Kumar wrote:
> > On 10-08-15, 17:31, Stephen Boyd wrote:
> > > So ret is 0. I thought it was an error path, but I guess this is a
> > > warning path and we return 0 still?
> > 
> > Urg ..
> 
> Oh I see this is an existing problem... Same problem goes for the
> count check. It may be better to fix those two cases first and
> then do this cleanup. But I don't mind either way.

-------------------------8<-------------------------
Message-Id: <b2d2d842165ec1748a56ecc03b85bd89321a644b.1439276231.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 11 Aug 2015 12:22:07 +0530
Subject: [PATCH] PM / OPP: Return proper errors on failure

We are returning 0 by mistake, fix it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 1daaa1a418a2..be3356a3a452 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1292,14 +1292,18 @@ static int _of_init_opp_table_v2(struct device *dev,
 	}
 
 	/* There should be one of more OPP defined */
-	if (WARN_ON(!count))
+	if (WARN_ON(!count)) {
+		ret = -ENOENT;
 		goto out;
+	}
 
 	if (!ret) {
 		if (!dev_opp) {
 			dev_opp = _find_device_opp(dev);
-			if (WARN_ON(!dev_opp))
+			if (WARN_ON(!dev_opp)) {
+				ret = -ENOENT;
 				goto out;
+			}
 		}
 
 		dev_opp->np = opp_np;

Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

[Viresh Kumar]

On 11-08-15, 12:27, Viresh Kumar wrote:
> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> index 1daaa1a418a2..be3356a3a452 100644
> --- a/drivers/base/power/opp.c
> +++ b/drivers/base/power/opp.c
> @@ -1292,14 +1292,18 @@ static int _of_init_opp_table_v2(struct device *dev,
>  	}
>  
>  	/* There should be one of more OPP defined */
> -	if (WARN_ON(!count))
> +	if (WARN_ON(!count)) {
> +		ret = -ENOENT;
>  		goto out;
> +	}
>  
>  	if (!ret) {
>  		if (!dev_opp) {
>  			dev_opp = _find_device_opp(dev);
> -			if (WARN_ON(!dev_opp))
> +			if (WARN_ON(!dev_opp)) {

Dan also reported that !dev_opp isn't enough as we need to use IS_ERR
here.

We have got enough updates here, let me resend the series again to get
more reviews on proper patches.

-- 
viresh

Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

[Viresh Kumar]

On 10-08-15, 17:31, Stephen Boyd wrote:
> I am not insisting anything. But another patch to get rid of the
> goto sounds fine.

The separate patch to kill goto.

---------------------8<-------------------------

Message-Id: <9f9b9ea6ea3685d5f71c132e7efb18917253961f.1439260355.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 11 Aug 2015 08:01:01 +0530
Subject: [PATCH] PM / OPP: Remove unnecessary goto statements

The code simply returns after jumping to the out label and doesn't free
any resources, etc.

Kill the unnecessary goto statements.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index f42e098b71fe..220e8d77e2c4 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1269,14 +1269,14 @@ static int _of_init_opp_table_v2(struct device *dev,
 {
 	struct device_node *np;
 	struct device_opp *dev_opp;
-	int ret = 0, count = 0;
+	int ret, count = 0;
 
 	dev_opp = _managed_opp(opp_np);
 	if (dev_opp) {
 		/* OPPs are already managed */
 		if (!_add_list_dev(dev, dev_opp))
-			ret = -ENOMEM;
-		goto out;
+			return -ENOMEM;
+		return 0;
 	}
 
 	/* We have opp-list node now, iterate over it and add OPPs */
@@ -1287,31 +1287,24 @@ static int _of_init_opp_table_v2(struct device *dev,
 		if (ret) {
 			dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
 				ret);
-			break;
+			of_free_opp_table(dev);
+			return ret;
 		}
 	}
 
 	/* There should be one of more OPP defined */
 	if (WARN_ON(!count))
-		goto out;
-
-	if (ret) {
-		of_free_opp_table(dev);
-		goto out;
-	}
+		return -ENOENT;
 
 	dev_opp = _find_device_opp(dev);
-	if (WARN_ON(!dev_opp)) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (WARN_ON(!dev_opp))
+		return -EINVAL;
 
 	dev_opp->np = opp_np;
 	dev_opp->shared_opp = of_property_read_bool(opp_np,
 			"opp-shared");
 
-out:
-	return ret;
+	return 0;
 }
 
 /* Initializes OPP tables based on old-deprecated bindings */

Re: [PATCH 2/6] PM / OPP: restructure _of_init_opp_table_v2()

[Stephen Boyd]

On 08/11, Viresh Kumar wrote:
> On 10-08-15, 17:31, Stephen Boyd wrote:
> > I am not insisting anything. But another patch to get rid of the
> > goto sounds fine.
> 
> The separate patch to kill goto.
> 
> ---------------------8<-------------------------
> 
> Message-Id: <9f9b9ea6ea3685d5f71c132e7efb18917253961f.1439260355.git.viresh.kumar@linaro.org>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Tue, 11 Aug 2015 08:01:01 +0530
> Subject: [PATCH] PM / OPP: Remove unnecessary goto statements
> 
> The code simply returns after jumping to the out label and doesn't free
> any resources, etc.
> 
> Kill the unnecessary goto statements.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>


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

[PATCH 3/6] PM / OPP: Prefix exported opp routines with dev_pm_opp_

[Viresh Kumar]

That's the naming convention followed in most of opp core, but few
recent additions didn't follow this, fix them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c     | 18 +++++++++---------
 drivers/cpufreq/cpufreq-dt.c | 10 +++++-----
 include/linux/pm_opp.h       | 16 ++++++++--------
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index c9747fb192b1..677457409245 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -1230,7 +1230,7 @@ void of_free_opp_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(of_free_opp_table);
 
-void of_cpumask_free_opp_table(cpumask_var_t cpumask)
+void dev_pm_opp_cpumask_free_table(cpumask_var_t cpumask)
 {
 	struct device *cpu_dev;
 	int cpu;
@@ -1248,7 +1248,7 @@ void of_cpumask_free_opp_table(cpumask_var_t cpumask)
 		of_free_opp_table(cpu_dev);
 	}
 }
-EXPORT_SYMBOL_GPL(of_cpumask_free_opp_table);
+EXPORT_SYMBOL_GPL(dev_pm_opp_cpumask_free_table);
 
 /* Returns opp descriptor node for a device, caller must do of_node_put() */
 static struct device_node *_of_get_opp_desc_node(struct device *dev)
@@ -1397,7 +1397,7 @@ int of_init_opp_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 
-int of_cpumask_init_opp_table(cpumask_var_t cpumask)
+int dev_pm_opp_cpumask_init_opp(cpumask_var_t cpumask)
 {
 	struct device *cpu_dev;
 	int cpu, ret = 0;
@@ -1418,17 +1418,17 @@ int of_cpumask_init_opp_table(cpumask_var_t cpumask)
 			       __func__, cpu, ret);
 
 			/* Free all other OPPs */
-			of_cpumask_free_opp_table(cpumask);
+			dev_pm_opp_cpumask_free_table(cpumask);
 			break;
 		}
 	}
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(of_cpumask_init_opp_table);
+EXPORT_SYMBOL_GPL(dev_pm_opp_cpumask_init_opp);
 
 /* Required only for V1 bindings, as v2 can manage it from DT itself */
-int set_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
+int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
 {
 	struct device_list_opp *list_dev;
 	struct device_opp *dev_opp;
@@ -1466,7 +1466,7 @@ int set_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(set_cpus_sharing_opps);
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus);
 
 /*
  * Works only for OPP v2 bindings.
@@ -1474,7 +1474,7 @@ EXPORT_SYMBOL_GPL(set_cpus_sharing_opps);
  * cpumask should be already set to mask of cpu_dev->id.
  * Returns -ENOENT if operating-points-v2 bindings aren't supported.
  */
-int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
+int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
 {
 	struct device_node *np, *tmp_np;
 	struct device *tcpu_dev;
@@ -1524,5 +1524,5 @@ int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
 	of_node_put(np);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(of_get_cpus_sharing_opps);
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_sharing_cpus);
 #endif
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index c3583cdfadbd..025e76efdec3 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -215,7 +215,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	/* Get OPP-sharing information from "operating-points-v2" bindings */
-	ret = of_get_cpus_sharing_opps(cpu_dev, policy->cpus);
+	ret = dev_pm_opp_get_sharing_cpus(cpu_dev, policy->cpus);
 	if (ret) {
 		/*
 		 * operating-points-v2 not supported, fallback to old method of
@@ -237,7 +237,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	 *
 	 * OPPs might be populated at runtime, don't check for error here
 	 */
-	of_cpumask_init_opp_table(policy->cpus);
+	dev_pm_opp_cpumask_init_opp(policy->cpus);
 
 	if (need_update) {
 		struct cpufreq_dt_platform_data *pd = cpufreq_get_driver_data();
@@ -249,7 +249,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		 * OPP tables are initialized only for policy->cpu, do it for
 		 * others as well.
 		 */
-		set_cpus_sharing_opps(cpu_dev, policy->cpus);
+		dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
 
 		of_property_read_u32(np, "clock-latency", &transition_latency);
 	} else {
@@ -356,7 +356,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 out_free_priv:
 	kfree(priv);
 out_free_opp:
-	of_cpumask_free_opp_table(policy->cpus);
+	dev_pm_opp_cpumask_free_table(policy->cpus);
 out_node_put:
 	of_node_put(np);
 out_put_reg_clk:
@@ -373,7 +373,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 
 	cpufreq_cooling_unregister(priv->cdev);
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
-	of_cpumask_free_opp_table(policy->related_cpus);
+	dev_pm_opp_cpumask_free_table(policy->related_cpus);
 	clk_put(policy->clk);
 	if (!IS_ERR(priv->cpu_reg))
 		regulator_put(priv->cpu_reg);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index cab7ba55bedb..076e0309f206 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -128,10 +128,10 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
 int of_init_opp_table(struct device *dev);
 void of_free_opp_table(struct device *dev);
-int of_cpumask_init_opp_table(cpumask_var_t cpumask);
-void of_cpumask_free_opp_table(cpumask_var_t cpumask);
-int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask);
-int set_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask);
+int dev_pm_opp_cpumask_init_opp(cpumask_var_t cpumask);
+void dev_pm_opp_cpumask_free_table(cpumask_var_t cpumask);
+int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask);
+int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask);
 #else
 static inline int of_init_opp_table(struct device *dev)
 {
@@ -142,21 +142,21 @@ static inline void of_free_opp_table(struct device *dev)
 {
 }
 
-static inline int of_cpumask_init_opp_table(cpumask_var_t cpumask)
+static inline int dev_pm_opp_cpumask_init_opp(cpumask_var_t cpumask)
 {
 	return -ENOSYS;
 }
 
-static inline void of_cpumask_free_opp_table(cpumask_var_t cpumask)
+static inline void dev_pm_opp_cpumask_free_table(cpumask_var_t cpumask)
 {
 }
 
-static inline int of_get_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
+static inline int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
 {
 	return -ENOSYS;
 }
 
-static inline int set_cpus_sharing_opps(struct device *cpu_dev, cpumask_var_t cpumask)
+static inline int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
 {
 	return -ENOSYS;
 }
-- 
2.4.0

Re: [PATCH 3/6] PM / OPP: Prefix exported opp routines with dev_pm_opp_

[Stephen Boyd]

On 08/10, Viresh Kumar wrote:
> That's the naming convention followed in most of opp core, but few
> recent additions didn't follow this, fix them.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

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

[PATCH 4/6] PM / OPP: Move opp core to its own directory

[Viresh Kumar]

OPP code is expanding and is already present in multiple directories
(cpufreq and power). Lets move it to its own directory, to manage it
better.

This also moves/renames the cpufreq_opp file to cpu.c, as it will
contain helpers for cpu device. Its not just about cpufreq, other
frameworks can use OPPs as well.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/Makefile                             | 2 +-
 drivers/base/power/opp/Makefile                         | 2 ++
 drivers/base/power/{opp.c => opp/core.c}                | 0
 drivers/{cpufreq/cpufreq_opp.c => base/power/opp/cpu.c} | 4 +++-
 drivers/cpufreq/Makefile                                | 1 -
 5 files changed, 6 insertions(+), 3 deletions(-)
 create mode 100644 drivers/base/power/opp/Makefile
 rename drivers/base/power/{opp.c => opp/core.c} (100%)
 rename drivers/{cpufreq/cpufreq_opp.c => base/power/opp/cpu.c} (97%)

diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index f94a6ccfe787..7ff2726dab6c 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -1,8 +1,8 @@
 obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
 obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
-obj-$(CONFIG_PM_OPP)	+= opp.o
 obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
 obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
+obj-$(CONFIG_PM_OPP)	+= opp/
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
diff --git a/drivers/base/power/opp/Makefile b/drivers/base/power/opp/Makefile
new file mode 100644
index 000000000000..33c1e18c41a4
--- /dev/null
+++ b/drivers/base/power/opp/Makefile
@@ -0,0 +1,2 @@
+ccflags-$(CONFIG_DEBUG_DRIVER)	:= -DDEBUG
+obj-y				+= core.o cpu.o
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp/core.c
similarity index 100%
rename from drivers/base/power/opp.c
rename to drivers/base/power/opp/core.c
diff --git a/drivers/cpufreq/cpufreq_opp.c b/drivers/base/power/opp/cpu.c
similarity index 97%
rename from drivers/cpufreq/cpufreq_opp.c
rename to drivers/base/power/opp/cpu.c
index 0f5e6d5f6da0..0dd033016e9d 100644
--- a/drivers/cpufreq/cpufreq_opp.c
+++ b/drivers/base/power/opp/cpu.c
@@ -1,5 +1,5 @@
 /*
- * Generic OPP helper interface for CPUFreq drivers
+ * Generic OPP helper interface for CPU device
  *
  * Copyright (C) 2009-2014 Texas Instruments Incorporated.
  *	Nishanth Menon
@@ -20,6 +20,7 @@
 #include <linux/rcupdate.h>
 #include <linux/slab.h>
 
+#ifdef CONFIG_CPU_FREQ
 /**
  * dev_pm_opp_init_cpufreq_table() - create a cpufreq table for a device
  * @dev:	device for which we do this operation
@@ -112,3 +113,4 @@ void dev_pm_opp_free_cpufreq_table(struct device *dev,
 	*table = NULL;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_free_cpufreq_table);
+#endif	/* CONFIG_CPU_FREQ */
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 2169bf792db7..2ba5b7c0bed1 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -1,6 +1,5 @@
 # CPUfreq core
 obj-$(CONFIG_CPU_FREQ)			+= cpufreq.o freq_table.o
-obj-$(CONFIG_PM_OPP)			+= cpufreq_opp.o
 
 # CPUfreq stats
 obj-$(CONFIG_CPU_FREQ_STAT)             += cpufreq_stats.o
-- 
2.4.0

Re: [PATCH 4/6] PM / OPP: Move opp core to its own directory

[Stephen Boyd]

On 08/10, Viresh Kumar wrote:
> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index f94a6ccfe787..7ff2726dab6c 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -1,8 +1,8 @@
>  obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
>  obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
>  obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
> -obj-$(CONFIG_PM_OPP)	+= opp.o
>  obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
>  obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
> +obj-$(CONFIG_PM_OPP)	+= opp/
>  
Any reason this moved from the previous location?

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

Re: [PATCH 4/6] PM / OPP: Move opp core to its own directory

[Viresh Kumar]

On 10-08-15, 12:51, Stephen Boyd wrote:
> On 08/10, Viresh Kumar wrote:
> > diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> > index f94a6ccfe787..7ff2726dab6c 100644
> > --- a/drivers/base/power/Makefile
> > +++ b/drivers/base/power/Makefile
> > @@ -1,8 +1,8 @@
> >  obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
> >  obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
> >  obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
> > -obj-$(CONFIG_PM_OPP)	+= opp.o
> >  obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
> >  obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
> > +obj-$(CONFIG_PM_OPP)	+= opp/
> >  
> Any reason this moved from the previous location?

So that all the direct file compilation lines stay at the top and the
directory thing at the bottom. Just that order, nothing else.

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

-- 
viresh

[PATCH 5/6] PM / OPP: Move cpu specific code to opp/cpu.c

[Viresh Kumar]

Move cpu device specific code out of generic opp library, and add it to
cpu.c.

Along with that, create a core-internal opp.h header, which will be used
to share structures and function prototypes within opp core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 276 +-----------------------------------------
 drivers/base/power/opp/cpu.c  | 160 +++++++++++++++++++++++-
 drivers/base/power/opp/opp.h  | 143 ++++++++++++++++++++++
 3 files changed, 304 insertions(+), 275 deletions(-)
 create mode 100644 drivers/base/power/opp/opp.h

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 677457409245..b1f72b4cad24 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -11,131 +11,14 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/cpu.h>
-#include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/device.h>
-#include <linux/list.h>
-#include <linux/rculist.h>
-#include <linux/rcupdate.h>
-#include <linux/pm_opp.h>
 #include <linux/of.h>
 #include <linux/export.h>
 
-/*
- * Internal data structure organization with the OPP layer library is as
- * follows:
- * dev_opp_list (root)
- *	|- device 1 (represents voltage domain 1)
- *	|	|- opp 1 (availability, freq, voltage)
- *	|	|- opp 2 ..
- *	...	...
- *	|	`- opp n ..
- *	|- device 2 (represents the next voltage domain)
- *	...
- *	`- device m (represents mth voltage domain)
- * device 1, 2.. are represented by dev_opp structure while each opp
- * is represented by the opp structure.
- */
-
-/**
- * struct dev_pm_opp - Generic OPP description structure
- * @node:	opp list node. The nodes are maintained throughout the lifetime
- *		of boot. It is expected only an optimal set of OPPs are
- *		added to the library by the SoC framework.
- *		RCU usage: opp list is traversed with RCU locks. node
- *		modification is possible realtime, hence the modifications
- *		are protected by the dev_opp_list_lock for integrity.
- *		IMPORTANT: the opp nodes should be maintained in increasing
- *		order.
- * @dynamic:	not-created from static DT entries.
- * @available:	true/false - marks if this OPP as available or not
- * @turbo:	true if turbo (boost) OPP
- * @rate:	Frequency in hertz
- * @u_volt:	Target voltage in microvolts corresponding to this OPP
- * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
- * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
- * @u_amp:	Maximum current drawn by the device in microamperes
- * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
- *		frequency from any other OPP's frequency.
- * @dev_opp:	points back to the device_opp struct this opp belongs to
- * @rcu_head:	RCU callback head used for deferred freeing
- * @np:		OPP's device node.
- *
- * This structure stores the OPP information for a given device.
- */
-struct dev_pm_opp {
-	struct list_head node;
-
-	bool available;
-	bool dynamic;
-	bool turbo;
-	unsigned long rate;
-
-	unsigned long u_volt;
-	unsigned long u_volt_min;
-	unsigned long u_volt_max;
-	unsigned long u_amp;
-	unsigned long clock_latency_ns;
-
-	struct device_opp *dev_opp;
-	struct rcu_head rcu_head;
-
-	struct device_node *np;
-};
-
-/**
- * struct device_list_opp - devices managed by 'struct device_opp'
- * @node:	list node
- * @dev:	device to which the struct object belongs
- * @rcu_head:	RCU callback head used for deferred freeing
- *
- * This is an internal data structure maintaining the list of devices that are
- * managed by 'struct device_opp'.
- */
-struct device_list_opp {
-	struct list_head node;
-	const struct device *dev;
-	struct rcu_head rcu_head;
-};
-
-/**
- * struct device_opp - Device opp structure
- * @node:	list node - contains the devices with OPPs that
- *		have been registered. Nodes once added are not modified in this
- *		list.
- *		RCU usage: nodes are not modified in the list of device_opp,
- *		however addition is possible and is secured by dev_opp_list_lock
- * @srcu_head:	notifier head to notify the OPP availability changes.
- * @rcu_head:	RCU callback head used for deferred freeing
- * @dev_list:	list of devices that share these OPPs
- * @opp_list:	list of opps
- * @np:		struct device_node pointer for opp's DT node.
- * @shared_opp: OPP is shared between multiple devices.
- *
- * This is an internal data structure maintaining the link to opps attached to
- * a device. This structure is not meant to be shared to users as it is
- * meant for book keeping and private to OPP library.
- *
- * Because the opp structures can be used from both rcu and srcu readers, we
- * need to wait for the grace period of both of them before freeing any
- * resources. And so we have used kfree_rcu() from within call_srcu() handlers.
- */
-struct device_opp {
-	struct list_head node;
-
-	struct srcu_notifier_head srcu_head;
-	struct rcu_head rcu_head;
-	struct list_head dev_list;
-	struct list_head opp_list;
-
-	struct device_node *np;
-	unsigned long clock_latency_ns_max;
-	bool shared_opp;
-	struct dev_pm_opp *suspend_opp;
-};
+#include "opp.h"
 
 /*
  * The root of the list of all devices. All device_opp structures branch off
@@ -200,7 +83,7 @@ static struct device_opp *_managed_opp(const struct device_node *np)
  * is a RCU protected pointer. This means that device_opp is valid as long
  * as we are under RCU lock.
  */
-static struct device_opp *_find_device_opp(struct device *dev)
+struct device_opp *_find_device_opp(struct device *dev)
 {
 	struct device_opp *dev_opp;
 
@@ -551,8 +434,8 @@ static void _remove_list_dev(struct device_list_opp *list_dev,
 		  _kfree_list_dev_rcu);
 }
 
-static struct device_list_opp *_add_list_dev(const struct device *dev,
-					     struct device_opp *dev_opp)
+struct device_list_opp *_add_list_dev(const struct device *dev,
+				      struct device_opp *dev_opp)
 {
 	struct device_list_opp *list_dev;
 
@@ -1230,28 +1113,8 @@ void of_free_opp_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(of_free_opp_table);
 
-void dev_pm_opp_cpumask_free_table(cpumask_var_t cpumask)
-{
-	struct device *cpu_dev;
-	int cpu;
-
-	WARN_ON(cpumask_empty(cpumask));
-
-	for_each_cpu(cpu, cpumask) {
-		cpu_dev = get_cpu_device(cpu);
-		if (!cpu_dev) {
-			pr_err("%s: failed to get cpu%d device\n", __func__,
-			       cpu);
-			continue;
-		}
-
-		of_free_opp_table(cpu_dev);
-	}
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_cpumask_free_table);
-
 /* Returns opp descriptor node for a device, caller must do of_node_put() */
-static struct device_node *_of_get_opp_desc_node(struct device *dev)
+struct device_node *_of_get_opp_desc_node(struct device *dev)
 {
 	/*
 	 * TODO: Support for multiple OPP tables.
@@ -1396,133 +1259,4 @@ int of_init_opp_table(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
-
-int dev_pm_opp_cpumask_init_opp(cpumask_var_t cpumask)
-{
-	struct device *cpu_dev;
-	int cpu, ret = 0;
-
-	WARN_ON(cpumask_empty(cpumask));
-
-	for_each_cpu(cpu, cpumask) {
-		cpu_dev = get_cpu_device(cpu);
-		if (!cpu_dev) {
-			pr_err("%s: failed to get cpu%d device\n", __func__,
-			       cpu);
-			continue;
-		}
-
-		ret = of_init_opp_table(cpu_dev);
-		if (ret) {
-			pr_err("%s: couldn't find opp table for cpu:%d, %d\n",
-			       __func__, cpu, ret);
-
-			/* Free all other OPPs */
-			dev_pm_opp_cpumask_free_table(cpumask);
-			break;
-		}
-	}
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_cpumask_init_opp);
-
-/* Required only for V1 bindings, as v2 can manage it from DT itself */
-int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
-{
-	struct device_list_opp *list_dev;
-	struct device_opp *dev_opp;
-	struct device *dev;
-	int cpu, ret = 0;
-
-	rcu_read_lock();
-
-	dev_opp = _find_device_opp(cpu_dev);
-	if (IS_ERR(dev_opp)) {
-		ret = -EINVAL;
-		goto out_rcu_read_unlock;
-	}
-
-	for_each_cpu(cpu, cpumask) {
-		if (cpu == cpu_dev->id)
-			continue;
-
-		dev = get_cpu_device(cpu);
-		if (!dev) {
-			dev_err(cpu_dev, "%s: failed to get cpu%d device\n",
-				__func__, cpu);
-			continue;
-		}
-
-		list_dev = _add_list_dev(dev, dev_opp);
-		if (!list_dev) {
-			dev_err(dev, "%s: failed to add list-dev for cpu%d device\n",
-				__func__, cpu);
-			continue;
-		}
-	}
-out_rcu_read_unlock:
-	rcu_read_unlock();
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus);
-
-/*
- * Works only for OPP v2 bindings.
- *
- * cpumask should be already set to mask of cpu_dev->id.
- * Returns -ENOENT if operating-points-v2 bindings aren't supported.
- */
-int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
-{
-	struct device_node *np, *tmp_np;
-	struct device *tcpu_dev;
-	int cpu, ret = 0;
-
-	/* Get OPP descriptor node */
-	np = _of_get_opp_desc_node(cpu_dev);
-	if (!np) {
-		dev_dbg(cpu_dev, "%s: Couldn't find opp node: %ld\n", __func__,
-			PTR_ERR(np));
-		return -ENOENT;
-	}
-
-	/* OPPs are shared ? */
-	if (!of_property_read_bool(np, "opp-shared"))
-		goto put_cpu_node;
-
-	for_each_possible_cpu(cpu) {
-		if (cpu == cpu_dev->id)
-			continue;
-
-		tcpu_dev = get_cpu_device(cpu);
-		if (!tcpu_dev) {
-			dev_err(cpu_dev, "%s: failed to get cpu%d device\n",
-				__func__, cpu);
-			ret = -ENODEV;
-			goto put_cpu_node;
-		}
-
-		/* Get OPP descriptor node */
-		tmp_np = _of_get_opp_desc_node(tcpu_dev);
-		if (!tmp_np) {
-			dev_err(tcpu_dev, "%s: Couldn't find opp node: %ld\n",
-				__func__, PTR_ERR(tmp_np));
-			ret = PTR_ERR(tmp_np);
-			goto put_cpu_node;
-		}
-
-		/* CPUs are sharing opp node */
-		if (np == tmp_np)
-			cpumask_set_cpu(cpu, cpumask);
-
-		of_node_put(tmp_np);
-	}
-
-put_cpu_node:
-	of_node_put(np);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_get_sharing_cpus);
 #endif
diff --git a/drivers/base/power/opp/cpu.c b/drivers/base/power/opp/cpu.c
index 0dd033016e9d..7a56e98a7ba6 100644
--- a/drivers/base/power/opp/cpu.c
+++ b/drivers/base/power/opp/cpu.c
@@ -10,17 +10,18 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#include <linux/cpu.h>
 #include <linux/cpufreq.h>
-#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/export.h>
-#include <linux/kernel.h>
-#include <linux/pm_opp.h>
-#include <linux/rcupdate.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 
+#include "opp.h"
+
 #ifdef CONFIG_CPU_FREQ
+
 /**
  * dev_pm_opp_init_cpufreq_table() - create a cpufreq table for a device
  * @dev:	device for which we do this operation
@@ -114,3 +115,154 @@ void dev_pm_opp_free_cpufreq_table(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_free_cpufreq_table);
 #endif	/* CONFIG_CPU_FREQ */
+
+/* Required only for V1 bindings, as v2 can manage it from DT itself */
+int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
+{
+	struct device_list_opp *list_dev;
+	struct device_opp *dev_opp;
+	struct device *dev;
+	int cpu, ret = 0;
+
+	rcu_read_lock();
+
+	dev_opp = _find_device_opp(cpu_dev);
+	if (IS_ERR(dev_opp)) {
+		ret = -EINVAL;
+		goto out_rcu_read_unlock;
+	}
+
+	for_each_cpu(cpu, cpumask) {
+		if (cpu == cpu_dev->id)
+			continue;
+
+		dev = get_cpu_device(cpu);
+		if (!dev) {
+			dev_err(cpu_dev, "%s: failed to get cpu%d device\n",
+				__func__, cpu);
+			continue;
+		}
+
+		list_dev = _add_list_dev(dev, dev_opp);
+		if (!list_dev) {
+			dev_err(dev, "%s: failed to add list-dev for cpu%d device\n",
+				__func__, cpu);
+			continue;
+		}
+	}
+out_rcu_read_unlock:
+	rcu_read_unlock();
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_sharing_cpus);
+
+#ifdef CONFIG_OF
+void dev_pm_opp_cpumask_free_table(cpumask_var_t cpumask)
+{
+	struct device *cpu_dev;
+	int cpu;
+
+	WARN_ON(cpumask_empty(cpumask));
+
+	for_each_cpu(cpu, cpumask) {
+		cpu_dev = get_cpu_device(cpu);
+		if (!cpu_dev) {
+			pr_err("%s: failed to get cpu%d device\n", __func__,
+			       cpu);
+			continue;
+		}
+
+		of_free_opp_table(cpu_dev);
+	}
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_cpumask_free_table);
+
+int dev_pm_opp_cpumask_init_opp(cpumask_var_t cpumask)
+{
+	struct device *cpu_dev;
+	int cpu, ret = 0;
+
+	WARN_ON(cpumask_empty(cpumask));
+
+	for_each_cpu(cpu, cpumask) {
+		cpu_dev = get_cpu_device(cpu);
+		if (!cpu_dev) {
+			pr_err("%s: failed to get cpu%d device\n", __func__,
+			       cpu);
+			continue;
+		}
+
+		ret = of_init_opp_table(cpu_dev);
+		if (ret) {
+			pr_err("%s: couldn't find opp table for cpu:%d, %d\n",
+			       __func__, cpu, ret);
+
+			/* Free all other OPPs */
+			dev_pm_opp_cpumask_free_table(cpumask);
+			break;
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_cpumask_init_opp);
+
+/*
+ * Works only for OPP v2 bindings.
+ *
+ * cpumask should be already set to mask of cpu_dev->id.
+ * Returns -ENOENT if operating-points-v2 bindings aren't supported.
+ */
+int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, cpumask_var_t cpumask)
+{
+	struct device_node *np, *tmp_np;
+	struct device *tcpu_dev;
+	int cpu, ret = 0;
+
+	/* Get OPP descriptor node */
+	np = _of_get_opp_desc_node(cpu_dev);
+	if (!np) {
+		dev_dbg(cpu_dev, "%s: Couldn't find opp node: %ld\n", __func__,
+			PTR_ERR(np));
+		return -ENOENT;
+	}
+
+	/* OPPs are shared ? */
+	if (!of_property_read_bool(np, "opp-shared"))
+		goto put_cpu_node;
+
+	for_each_possible_cpu(cpu) {
+		if (cpu == cpu_dev->id)
+			continue;
+
+		tcpu_dev = get_cpu_device(cpu);
+		if (!tcpu_dev) {
+			dev_err(cpu_dev, "%s: failed to get cpu%d device\n",
+				__func__, cpu);
+			ret = -ENODEV;
+			goto put_cpu_node;
+		}
+
+		/* Get OPP descriptor node */
+		tmp_np = _of_get_opp_desc_node(tcpu_dev);
+		if (!tmp_np) {
+			dev_err(tcpu_dev, "%s: Couldn't find opp node: %ld\n",
+				__func__, PTR_ERR(tmp_np));
+			ret = PTR_ERR(tmp_np);
+			goto put_cpu_node;
+		}
+
+		/* CPUs are sharing opp node */
+		if (np == tmp_np)
+			cpumask_set_cpu(cpu, cpumask);
+
+		of_node_put(tmp_np);
+	}
+
+put_cpu_node:
+	of_node_put(np);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_sharing_cpus);
+#endif
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
new file mode 100644
index 000000000000..dcb38f78dae4
--- /dev/null
+++ b/drivers/base/power/opp/opp.h
@@ -0,0 +1,143 @@
+/*
+ * Generic OPP Interface
+ *
+ * Copyright (C) 2009-2010 Texas Instruments Incorporated.
+ *	Nishanth Menon
+ *	Romit Dasgupta
+ *	Kevin Hilman
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __DRIVER_OPP_H__
+#define __DRIVER_OPP_H__
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/pm_opp.h>
+#include <linux/rculist.h>
+#include <linux/rcupdate.h>
+
+/*
+ * Internal data structure organization with the OPP layer library is as
+ * follows:
+ * dev_opp_list (root)
+ *	|- device 1 (represents voltage domain 1)
+ *	|	|- opp 1 (availability, freq, voltage)
+ *	|	|- opp 2 ..
+ *	...	...
+ *	|	`- opp n ..
+ *	|- device 2 (represents the next voltage domain)
+ *	...
+ *	`- device m (represents mth voltage domain)
+ * device 1, 2.. are represented by dev_opp structure while each opp
+ * is represented by the opp structure.
+ */
+
+/**
+ * struct dev_pm_opp - Generic OPP description structure
+ * @node:	opp list node. The nodes are maintained throughout the lifetime
+ *		of boot. It is expected only an optimal set of OPPs are
+ *		added to the library by the SoC framework.
+ *		RCU usage: opp list is traversed with RCU locks. node
+ *		modification is possible realtime, hence the modifications
+ *		are protected by the dev_opp_list_lock for integrity.
+ *		IMPORTANT: the opp nodes should be maintained in increasing
+ *		order.
+ * @dynamic:	not-created from static DT entries.
+ * @available:	true/false - marks if this OPP as available or not
+ * @turbo:	true if turbo (boost) OPP
+ * @rate:	Frequency in hertz
+ * @u_volt:	Target voltage in microvolts corresponding to this OPP
+ * @u_volt_min:	Minimum voltage in microvolts corresponding to this OPP
+ * @u_volt_max:	Maximum voltage in microvolts corresponding to this OPP
+ * @u_amp:	Maximum current drawn by the device in microamperes
+ * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
+ *		frequency from any other OPP's frequency.
+ * @dev_opp:	points back to the device_opp struct this opp belongs to
+ * @rcu_head:	RCU callback head used for deferred freeing
+ * @np:		OPP's device node.
+ *
+ * This structure stores the OPP information for a given device.
+ */
+struct dev_pm_opp {
+	struct list_head node;
+
+	bool available;
+	bool dynamic;
+	bool turbo;
+	unsigned long rate;
+
+	unsigned long u_volt;
+	unsigned long u_volt_min;
+	unsigned long u_volt_max;
+	unsigned long u_amp;
+	unsigned long clock_latency_ns;
+
+	struct device_opp *dev_opp;
+	struct rcu_head rcu_head;
+
+	struct device_node *np;
+};
+
+/**
+ * struct device_list_opp - devices managed by 'struct device_opp'
+ * @node:	list node
+ * @dev:	device to which the struct object belongs
+ * @rcu_head:	RCU callback head used for deferred freeing
+ *
+ * This is an internal data structure maintaining the list of devices that are
+ * managed by 'struct device_opp'.
+ */
+struct device_list_opp {
+	struct list_head node;
+	const struct device *dev;
+	struct rcu_head rcu_head;
+};
+
+/**
+ * struct device_opp - Device opp structure
+ * @node:	list node - contains the devices with OPPs that
+ *		have been registered. Nodes once added are not modified in this
+ *		list.
+ *		RCU usage: nodes are not modified in the list of device_opp,
+ *		however addition is possible and is secured by dev_opp_list_lock
+ * @srcu_head:	notifier head to notify the OPP availability changes.
+ * @rcu_head:	RCU callback head used for deferred freeing
+ * @dev_list:	list of devices that share these OPPs
+ * @opp_list:	list of opps
+ * @np:		struct device_node pointer for opp's DT node.
+ * @shared_opp: OPP is shared between multiple devices.
+ *
+ * This is an internal data structure maintaining the link to opps attached to
+ * a device. This structure is not meant to be shared to users as it is
+ * meant for book keeping and private to OPP library.
+ *
+ * Because the opp structures can be used from both rcu and srcu readers, we
+ * need to wait for the grace period of both of them before freeing any
+ * resources. And so we have used kfree_rcu() from within call_srcu() handlers.
+ */
+struct device_opp {
+	struct list_head node;
+
+	struct srcu_notifier_head srcu_head;
+	struct rcu_head rcu_head;
+	struct list_head dev_list;
+	struct list_head opp_list;
+
+	struct device_node *np;
+	unsigned long clock_latency_ns_max;
+	bool shared_opp;
+	struct dev_pm_opp *suspend_opp;
+};
+
+/* Routines internal to opp core */
+struct device_opp *_find_device_opp(struct device *dev);
+struct device_list_opp *_add_list_dev(const struct device *dev,
+				      struct device_opp *dev_opp);
+struct device_node *_of_get_opp_desc_node(struct device *dev);
+
+#endif		/* __DRIVER_OPP_H__ */
-- 
2.4.0

Re: [PATCH 5/6] PM / OPP: Move cpu specific code to opp/cpu.c

[Stephen Boyd]

On 08/10, Viresh Kumar wrote:
> Move cpu device specific code out of generic opp library, and add it to
> cpu.c.
> 
> Along with that, create a core-internal opp.h header, which will be used
> to share structures and function prototypes within opp core.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

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

[PATCH 6/6] PM / OPP: Add debugfs support

[Viresh Kumar]

This patch adds debugfs support to OPP layer to export OPPs and their
properties for all the devices.

This creates a top level directory: /sys/kernel/debug/opp and then
device specific directories (based on device names) inside it. For
example: 'cpu0', 'cpu1', etc..

If multiple devices share the OPP table, then the real directory is
created only for the first device. For all others, links are created to
the real directory.

Inside the device specific directory, a separate directory is created
for each OPP. And within that files per opp property.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/Makefile  |   1 +
 drivers/base/power/opp/core.c    |  15 ++++
 drivers/base/power/opp/debugfs.c | 165 +++++++++++++++++++++++++++++++++++++++
 drivers/base/power/opp/opp.h     |  36 +++++++++
 4 files changed, 217 insertions(+)
 create mode 100644 drivers/base/power/opp/debugfs.c

diff --git a/drivers/base/power/opp/Makefile b/drivers/base/power/opp/Makefile
index 33c1e18c41a4..19837ef04d8e 100644
--- a/drivers/base/power/opp/Makefile
+++ b/drivers/base/power/opp/Makefile
@@ -1,2 +1,3 @@
 ccflags-$(CONFIG_DEBUG_DRIVER)	:= -DDEBUG
 obj-y				+= core.o cpu.o
+obj-$(CONFIG_DEBUG_FS)		+= debugfs.o
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index b1f72b4cad24..84b084f60d60 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -429,6 +429,7 @@ static void _kfree_list_dev_rcu(struct rcu_head *head)
 static void _remove_list_dev(struct device_list_opp *list_dev,
 			     struct device_opp *dev_opp)
 {
+	opp_debug_unregister(list_dev, dev_opp);
 	list_del(&list_dev->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &list_dev->rcu_head,
 		  _kfree_list_dev_rcu);
@@ -438,6 +439,7 @@ struct device_list_opp *_add_list_dev(const struct device *dev,
 				      struct device_opp *dev_opp)
 {
 	struct device_list_opp *list_dev;
+	int ret;
 
 	list_dev = kzalloc(sizeof(*list_dev), GFP_KERNEL);
 	if (!list_dev)
@@ -447,6 +449,12 @@ struct device_list_opp *_add_list_dev(const struct device *dev,
 	list_dev->dev = dev;
 	list_add_rcu(&list_dev->node, &dev_opp->dev_list);
 
+	/* Create debugfs entries for the dev_opp */
+	ret = opp_debug_register(list_dev, dev_opp);
+	if (ret)
+		dev_err(dev, "%s: Failed to register opp debugfs (%d)\n",
+			__func__, ret);
+
 	return list_dev;
 }
 
@@ -562,6 +570,7 @@ static void _opp_remove(struct device_opp *dev_opp,
 	 */
 	if (notify)
 		srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
+	opp_debug_remove_one(opp);
 	list_del_rcu(&opp->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
 
@@ -639,6 +648,7 @@ static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 {
 	struct dev_pm_opp *opp;
 	struct list_head *head = &dev_opp->opp_list;
+	int ret;
 
 	/*
 	 * Insert new OPP in order of increasing frequency and discard if
@@ -669,6 +679,11 @@ static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	new_opp->dev_opp = dev_opp;
 	list_add_rcu(&new_opp->node, head);
 
+	ret = opp_debug_create_one(new_opp, dev_opp->dentry);
+	if (ret)
+		dev_err(dev, "%s: Failed to register opp to debugfs (%d)\n",
+			__func__, ret);
+
 	return 0;
 }
 
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
new file mode 100644
index 000000000000..d4e18eac8278
--- /dev/null
+++ b/drivers/base/power/opp/debugfs.c
@@ -0,0 +1,165 @@
+/*
+ * Generic OPP debugfs interface
+ *
+ * Copyright (C) 2015-2016 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/debugfs.h>
+#include <linux/err.h>
+
+#include "opp.h"
+
+static struct dentry *rootdir;
+
+void opp_debug_remove_one(struct dev_pm_opp *opp)
+{
+	debugfs_remove_recursive(opp->dentry);
+}
+
+int opp_debug_create_one(struct dev_pm_opp *opp, struct dentry *pdentry)
+{
+	struct dentry *d;
+	char name[15];
+
+	/* Rate is unique to each OPP, use it to give opp-name */
+	sprintf(name, "opp:%lu", opp->rate);
+
+	/* Create per-opp directory */
+	d = debugfs_create_dir(name, pdentry);
+	if (!d)
+		return -ENOMEM;
+
+	if (!debugfs_create_bool("available", S_IRUGO, d,
+				 (u32 *)&opp->available))
+		return -ENOMEM;
+
+	if (!debugfs_create_bool("dynamic", S_IRUGO, d, (u32 *)&opp->dynamic))
+		return -ENOMEM;
+
+	if (!debugfs_create_bool("turbo", S_IRUGO, d, (u32 *)&opp->turbo))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("rate_hz", S_IRUGO, d, (u32 *)&opp->rate))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("u_volt_target", S_IRUGO, d,
+				(u32 *)&opp->u_volt))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("u_volt_min", S_IRUGO, d,
+				(u32 *)&opp->u_volt_min))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("u_volt_max", S_IRUGO, d,
+				(u32 *)&opp->u_volt_max))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("u_amp", S_IRUGO, d, (u32 *)&opp->u_amp))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("clock_latency_ns", S_IRUGO, d,
+				(u32 *)&opp->clock_latency_ns))
+		return -ENOMEM;
+
+	opp->dentry = d;
+	return 0;
+}
+
+static int device_opp_debug_create_dir(struct device_list_opp *list_dev,
+				       struct device_opp *dev_opp)
+{
+	const struct device *dev = list_dev->dev;
+	struct dentry *d = NULL;
+
+	/* Create device specific directory */
+	d = debugfs_create_dir(dev_name(dev), rootdir);
+	if (!d) {
+		dev_err(dev, "%s: Failed to create debugfs dir\n", __func__);
+		return -ENOMEM;
+	}
+
+	list_dev->dentry = d;
+	dev_opp->dentry = d;
+	dev_opp->debugfs_dev = dev;
+
+	return 0;
+}
+
+static int device_opp_debug_create_link(struct device_list_opp *list_dev,
+					struct device_opp *dev_opp)
+{
+	const struct device *dev = list_dev->dev;
+	struct dentry *d = NULL;
+
+	/* Create device specific directory link */
+	d = debugfs_create_symlink(dev_name(dev), rootdir,
+				   dev_name(dev_opp->debugfs_dev));
+	if (!d) {
+		dev_err(dev, "%s: Failed to create link\n", __func__);
+		return -ENOMEM;
+	}
+
+	list_dev->dentry = d;
+
+	return 0;
+}
+
+/**
+ * opp_debug_register - add a device opp node to the debugfs 'opp' directory
+ * @list_dev: list-dev pointer for device
+ * @dev_opp: the device-opp being added
+ *
+ * Dynamically adds device specific directory in debugfs 'opp' directory. If the
+ * device-opp is shared with other devices, then links will be created for all
+ * devices except the first.
+ *
+ * Return: 0 on success, otherwise negative error.
+ */
+int opp_debug_register(struct device_list_opp *list_dev,
+		       struct device_opp *dev_opp)
+{
+	if (!rootdir) {
+		pr_debug("%s: Uninitialized rootdir\n", __func__);
+		return -EINVAL;
+	}
+
+	if (dev_opp->dentry)
+		return device_opp_debug_create_link(list_dev, dev_opp);
+
+	return device_opp_debug_create_dir(list_dev, dev_opp);
+}
+
+/**
+ * opp_debug_unregister - remove a device opp node from debugfs opp directory
+ * @list_dev: list-dev pointer for device
+ * @dev_opp: the device-opp being removed
+ *
+ * Dynamically removes device specific directory from debugfs 'opp' directory.
+ */
+void opp_debug_unregister(struct device_list_opp *list_dev,
+			  struct device_opp *dev_opp)
+{
+	debugfs_remove_recursive(list_dev->dentry);
+	if (list_dev->dentry == dev_opp->dentry)
+		dev_opp->dentry = NULL;
+	list_dev->dentry = NULL;
+}
+
+static int __init opp_debug_init(void)
+{
+	/* Create /sys/kernel/debug/opp directory */
+	rootdir = debugfs_create_dir("opp", NULL);
+	if (!rootdir) {
+		pr_err("%s: Failed to create root directory\n", __func__);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+core_initcall(opp_debug_init);
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index dcb38f78dae4..8eb6a9a098a1 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -60,6 +60,7 @@
  * @dev_opp:	points back to the device_opp struct this opp belongs to
  * @rcu_head:	RCU callback head used for deferred freeing
  * @np:		OPP's device node.
+ * @dentry:	debugfs dentry pointer (per opp)
  *
  * This structure stores the OPP information for a given device.
  */
@@ -81,6 +82,9 @@ struct dev_pm_opp {
 	struct rcu_head rcu_head;
 
 	struct device_node *np;
+
+	/* debugfs */
+	struct dentry *dentry;
 };
 
 /**
@@ -88,6 +92,7 @@ struct dev_pm_opp {
  * @node:	list node
  * @dev:	device to which the struct object belongs
  * @rcu_head:	RCU callback head used for deferred freeing
+ * @dentry:	debugfs dentry pointer (per device)
  *
  * This is an internal data structure maintaining the list of devices that are
  * managed by 'struct device_opp'.
@@ -96,6 +101,9 @@ struct device_list_opp {
 	struct list_head node;
 	const struct device *dev;
 	struct rcu_head rcu_head;
+
+	/* debugfs */
+	struct dentry *dentry;
 };
 
 /**
@@ -111,6 +119,8 @@ struct device_list_opp {
  * @opp_list:	list of opps
  * @np:		struct device_node pointer for opp's DT node.
  * @shared_opp: OPP is shared between multiple devices.
+ * @dentry:	debugfs dentry pointer of the real device directory (not links).
+ * @debugfs_dev: Pointer to device for which the real directory was created.
  *
  * This is an internal data structure maintaining the link to opps attached to
  * a device. This structure is not meant to be shared to users as it is
@@ -132,6 +142,10 @@ struct device_opp {
 	unsigned long clock_latency_ns_max;
 	bool shared_opp;
 	struct dev_pm_opp *suspend_opp;
+
+	/* debugfs */
+	struct dentry *dentry;
+	const struct device *debugfs_dev;
 };
 
 /* Routines internal to opp core */
@@ -140,4 +154,26 @@ struct device_list_opp *_add_list_dev(const struct device *dev,
 				      struct device_opp *dev_opp);
 struct device_node *_of_get_opp_desc_node(struct device *dev);
 
+#ifdef CONFIG_DEBUG_FS
+void opp_debug_remove_one(struct dev_pm_opp *opp);
+int opp_debug_create_one(struct dev_pm_opp *opp, struct dentry *pdentry);
+int opp_debug_register(struct device_list_opp *list_dev,
+		       struct device_opp *dev_opp);
+void opp_debug_unregister(struct device_list_opp *list_dev,
+			  struct device_opp *dev_opp);
+#else
+static inline void opp_debug_remove_one(struct dev_pm_opp *opp) {}
+
+static inline int opp_debug_create_one(struct dev_pm_opp *opp,
+				       struct dentry *pdentry)
+{ return 0; }
+static inline int opp_debug_register(struct device_list_opp *list_dev,
+				     struct device_opp *dev_opp)
+{ return 0; }
+
+static inline void opp_debug_unregister(struct device_list_opp *list_dev,
+					struct device_opp *dev_opp)
+{ }
+#endif		/* DEBUG_FS */
+
 #endif		/* __DRIVER_OPP_H__ */
-- 
2.4.0

Re: [PATCH 6/6] PM / OPP: Add debugfs support

[Stephen Boyd]

On 08/10, Viresh Kumar wrote:
> diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
> new file mode 100644
> index 000000000000..d4e18eac8278
> --- /dev/null
> +++ b/drivers/base/power/opp/debugfs.c
> @@ -0,0 +1,165 @@
> +/*
> + * Generic OPP debugfs interface
> + *
> + * Copyright (C) 2015-2016 Viresh Kumar <viresh.kumar@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/debugfs.h>
> +#include <linux/err.h>
> +
> +#include "opp.h"
> +
> +static struct dentry *rootdir;
> +
> +void opp_debug_remove_one(struct dev_pm_opp *opp)
> +{
> +	debugfs_remove_recursive(opp->dentry);
> +}
> +
> +int opp_debug_create_one(struct dev_pm_opp *opp, struct dentry *pdentry)

Maybe this could take dev_opp instead of dentry.

> +{
> +	struct dentry *d;
[..]
> +	return 0;
> +}
> +
> +static int device_opp_debug_create_dir(struct device_list_opp *list_dev,
> +				       struct device_opp *dev_opp)
> +{
> +	const struct device *dev = list_dev->dev;
> +	struct dentry *d = NULL;

Why assign this to NULL?

> +
> +	/* Create device specific directory */
> +	d = debugfs_create_dir(dev_name(dev), rootdir);
> +	if (!d) {
> +		dev_err(dev, "%s: Failed to create debugfs dir\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	list_dev->dentry = d;
> +	dev_opp->dentry = d;
> +	dev_opp->debugfs_dev = dev;
> +
> +	return 0;
> +}
> +
> +static int device_opp_debug_create_link(struct device_list_opp *list_dev,
> +					struct device_opp *dev_opp)
> +{
> +	const struct device *dev = list_dev->dev;
> +	struct dentry *d = NULL;

Why assign this to NULL?

> +
> +	/* Create device specific directory link */
> +	d = debugfs_create_symlink(dev_name(dev), rootdir,

The dev_name thing is going to fail. Eventually we'll get into a
situation where two devices with the same name on different
busses have OPPs. When we add them to debugfs their names are
going to conflict. The regulator core suffered this problem, see
commit a9eaa8130707 (regulator: Ensure unique regulator debugfs
directory names, 2014-10-17) for one solution.

> +				   dev_name(dev_opp->debugfs_dev));
> +	if (!d) {
> +		dev_err(dev, "%s: Failed to create link\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	list_dev->dentry = d;
> +
> +	return 0;
> +}
[..]
> +
> +/**
> + * opp_debug_unregister - remove a device opp node from debugfs opp directory
> + * @list_dev: list-dev pointer for device
> + * @dev_opp: the device-opp being removed
> + *
> + * Dynamically removes device specific directory from debugfs 'opp' directory.
> + */
> +void opp_debug_unregister(struct device_list_opp *list_dev,
> +			  struct device_opp *dev_opp)
> +{
> +	debugfs_remove_recursive(list_dev->dentry);

Can this break the symlink that other devices are linking to?

> +	if (list_dev->dentry == dev_opp->dentry)
> +		dev_opp->dentry = NULL;
> +	list_dev->dentry = NULL;

> +}
> +
> diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
> index dcb38f78dae4..8eb6a9a098a1 100644
> --- a/drivers/base/power/opp/opp.h
> +++ b/drivers/base/power/opp/opp.h
> @@ -60,6 +60,7 @@
>   * @dev_opp:	points back to the device_opp struct this opp belongs to
>   * @rcu_head:	RCU callback head used for deferred freeing
>   * @np:		OPP's device node.
> + * @dentry:	debugfs dentry pointer (per opp)
>   *
>   * This structure stores the OPP information for a given device.
>   */
> @@ -81,6 +82,9 @@ struct dev_pm_opp {
>  	struct rcu_head rcu_head;
>  
>  	struct device_node *np;
> +
> +	/* debugfs */
> +	struct dentry *dentry;

Should this be behind a CONFIG_DEBUG_FS ifdef?

>  };
>  
>  /**
> @@ -88,6 +92,7 @@ struct dev_pm_opp {
>   * @node:	list node
>   * @dev:	device to which the struct object belongs
>   * @rcu_head:	RCU callback head used for deferred freeing
> + * @dentry:	debugfs dentry pointer (per device)
>   *
>   * This is an internal data structure maintaining the list of devices that are
>   * managed by 'struct device_opp'.
> @@ -96,6 +101,9 @@ struct device_list_opp {
>  	struct list_head node;
>  	const struct device *dev;
>  	struct rcu_head rcu_head;
> +
> +	/* debugfs */
> +	struct dentry *dentry;

Should this be behind a CONFIG_DEBUG_FS ifdef?

>  };
>  
>  /**
> @@ -111,6 +119,8 @@ struct device_list_opp {
>   * @opp_list:	list of opps
>   * @np:		struct device_node pointer for opp's DT node.
>   * @shared_opp: OPP is shared between multiple devices.
> + * @dentry:	debugfs dentry pointer of the real device directory (not links).
> + * @debugfs_dev: Pointer to device for which the real directory was created.
>   *
>   * This is an internal data structure maintaining the link to opps attached to
>   * a device. This structure is not meant to be shared to users as it is
> @@ -132,6 +142,10 @@ struct device_opp {
>  	unsigned long clock_latency_ns_max;
>  	bool shared_opp;
>  	struct dev_pm_opp *suspend_opp;
> +
> +	/* debugfs */
> +	struct dentry *dentry;
> +	const struct device *debugfs_dev;

Should this be behind a CONFIG_DEBUG_FS guard?


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

Re: [PATCH 6/6] PM / OPP: Add debugfs support

[Viresh Kumar]

On 10-08-15, 12:50, Stephen Boyd wrote:
> > +	/* Create device specific directory link */
> > +	d = debugfs_create_symlink(dev_name(dev), rootdir,
> 
> The dev_name thing is going to fail. Eventually we'll get into a
> situation where two devices with the same name on different
> busses have OPPs. When we add them to debugfs their names are
> going to conflict. The regulator core suffered this problem, see
> commit a9eaa8130707 (regulator: Ensure unique regulator debugfs
> directory names, 2014-10-17) for one solution.

The regulator core had a slightly different problem probably as it was
assigning similar names to devices of same type. And we may not hit
it.

But still, I have prefixed it with parent name to make it look
slightly stronger :)

Diff with earlier patch (only compile tested for now, don't have
access to setup currently):

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index b3af1825266a..871502dec381 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -679,7 +679,7 @@ static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	new_opp->dev_opp = dev_opp;
 	list_add_rcu(&new_opp->node, head);
 
-	ret = opp_debug_create_one(new_opp, dev_opp->dentry);
+	ret = opp_debug_create_one(new_opp, dev_opp);
 	if (ret)
 		dev_err(dev, "%s: Failed to register opp to debugfs (%d)\n",
 			__func__, ret);
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
index d4e18eac8278..6c048bfbf954 100644
--- a/drivers/base/power/opp/debugfs.c
+++ b/drivers/base/power/opp/debugfs.c
@@ -17,13 +17,24 @@
 
 static struct dentry *rootdir;
 
+static void opp_set_dev_name(const struct device *dev, char *name)
+{
+
+	if (dev->parent)
+		snprintf(name, NAME_MAX, "%s-%s", dev_name(dev->parent),
+			 dev_name(dev));
+	else
+		snprintf(name, NAME_MAX, "%s", dev_name(dev));
+}
+
 void opp_debug_remove_one(struct dev_pm_opp *opp)
 {
 	debugfs_remove_recursive(opp->dentry);
 }
 
-int opp_debug_create_one(struct dev_pm_opp *opp, struct dentry *pdentry)
+int opp_debug_create_one(struct dev_pm_opp *opp, struct device_opp *dev_opp)
 {
+	struct dentry *pdentry = dev_opp->dentry;
 	struct dentry *d;
 	char name[15];
 
@@ -75,10 +86,12 @@ static int device_opp_debug_create_dir(struct device_list_opp *list_dev,
 				       struct device_opp *dev_opp)
 {
 	const struct device *dev = list_dev->dev;
-	struct dentry *d = NULL;
+	struct dentry *d;
+
+	opp_set_dev_name(dev, dev_opp->dentry_name);
 
 	/* Create device specific directory */
-	d = debugfs_create_dir(dev_name(dev), rootdir);
+	d = debugfs_create_dir(dev_opp->dentry_name, rootdir);
 	if (!d) {
 		dev_err(dev, "%s: Failed to create debugfs dir\n", __func__);
 		return -ENOMEM;
@@ -86,7 +99,6 @@ static int device_opp_debug_create_dir(struct device_list_opp *list_dev,
 
 	list_dev->dentry = d;
 	dev_opp->dentry = d;
-	dev_opp->debugfs_dev = dev;
 
 	return 0;
 }
@@ -95,11 +107,13 @@ static int device_opp_debug_create_link(struct device_list_opp *list_dev,
 					struct device_opp *dev_opp)
 {
 	const struct device *dev = list_dev->dev;
-	struct dentry *d = NULL;
+	char name[NAME_MAX];
+	struct dentry *d;
+
+	opp_set_dev_name(list_dev->dev, name);
 
 	/* Create device specific directory link */
-	d = debugfs_create_symlink(dev_name(dev), rootdir,
-				   dev_name(dev_opp->debugfs_dev));
+	d = debugfs_create_symlink(name, rootdir, dev_opp->dentry_name);
 	if (!d) {
 		dev_err(dev, "%s: Failed to create link\n", __func__);
 		return -ENOMEM;
@@ -135,6 +149,36 @@ int opp_debug_register(struct device_list_opp *list_dev,
 	return device_opp_debug_create_dir(list_dev, dev_opp);
 }
 
+static void opp_migrate_dentry(struct device_list_opp *list_dev,
+			       struct device_opp *dev_opp)
+{
+	struct device_list_opp *new_dev;
+	const struct device *dev;
+	struct dentry *dentry;
+
+	/* Look for next list-dev */
+	list_for_each_entry(new_dev, &dev_opp->dev_list, node)
+		if (new_dev != list_dev)
+			break;
+
+	/* new_dev is guaranteed to be valid here */
+	dev = new_dev->dev;
+	debugfs_remove_recursive(new_dev->dentry);
+
+	opp_set_dev_name(dev, dev_opp->dentry_name);
+
+	dentry = debugfs_rename(rootdir, list_dev->dentry, rootdir,
+				dev_opp->dentry_name);
+	if (!dentry) {
+		dev_err(dev, "%s: Failed to rename link from: %s to %s\n",
+			__func__, dev_name(list_dev->dev), dev_name(dev));
+		return;
+	}
+
+	new_dev->dentry = dentry;
+	dev_opp->dentry = dentry;
+}
+
 /**
  * opp_debug_unregister - remove a device opp node from debugfs opp directory
  * @list_dev: list-dev pointer for device
@@ -145,9 +189,18 @@ int opp_debug_register(struct device_list_opp *list_dev,
 void opp_debug_unregister(struct device_list_opp *list_dev,
 			  struct device_opp *dev_opp)
 {
-	debugfs_remove_recursive(list_dev->dentry);
-	if (list_dev->dentry == dev_opp->dentry)
+	if (list_dev->dentry == dev_opp->dentry) {
+		/* Move the real dentry object under another device */
+		if (!list_is_singular(&dev_opp->dev_list)) {
+			opp_migrate_dentry(list_dev, dev_opp);
+			goto out;
+		}
 		dev_opp->dentry = NULL;
+	}
+
+	debugfs_remove_recursive(list_dev->dentry);
+
+out:
 	list_dev->dentry = NULL;
 }
 
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 8eb6a9a098a1..0e4c27fb0c4a 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
+#include <linux/limits.h>
 #include <linux/pm_opp.h>
 #include <linux/rculist.h>
 #include <linux/rcupdate.h>
@@ -83,8 +84,10 @@ struct dev_pm_opp {
 
 	struct device_node *np;
 
+#ifdef CONFIG_DEBUG_FS
 	/* debugfs */
 	struct dentry *dentry;
+#endif
 };
 
 /**
@@ -102,8 +105,10 @@ struct device_list_opp {
 	const struct device *dev;
 	struct rcu_head rcu_head;
 
+#ifdef CONFIG_DEBUG_FS
 	/* debugfs */
 	struct dentry *dentry;
+#endif
 };
 
 /**
@@ -120,7 +125,7 @@ struct device_list_opp {
  * @np:		struct device_node pointer for opp's DT node.
  * @shared_opp: OPP is shared between multiple devices.
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
- * @debugfs_dev: Pointer to device for which the real directory was created.
+ * @dentry_name: Name of the real dentry.
  *
  * This is an internal data structure maintaining the link to opps attached to
  * a device. This structure is not meant to be shared to users as it is
@@ -143,9 +148,11 @@ struct device_opp {
 	bool shared_opp;
 	struct dev_pm_opp *suspend_opp;
 
+#ifdef CONFIG_DEBUG_FS
 	/* debugfs */
 	struct dentry *dentry;
-	const struct device *debugfs_dev;
+	char dentry_name[NAME_MAX];
+#endif
 };
 
 /* Routines internal to opp core */
@@ -156,7 +163,7 @@ struct device_node *_of_get_opp_desc_node(struct device *dev);
 
 #ifdef CONFIG_DEBUG_FS
 void opp_debug_remove_one(struct dev_pm_opp *opp);
-int opp_debug_create_one(struct dev_pm_opp *opp, struct dentry *pdentry);
+int opp_debug_create_one(struct dev_pm_opp *opp, struct device_opp *dev_opp);
 int opp_debug_register(struct device_list_opp *list_dev,
 		       struct device_opp *dev_opp);
 void opp_debug_unregister(struct device_list_opp *list_dev,
@@ -165,7 +172,7 @@ void opp_debug_unregister(struct device_list_opp *list_dev,
 static inline void opp_debug_remove_one(struct dev_pm_opp *opp) {}
 
 static inline int opp_debug_create_one(struct dev_pm_opp *opp,
-				       struct dentry *pdentry)
+				       struct device_opp *dev_opp)
 { return 0; }
 static inline int opp_debug_register(struct device_list_opp *list_dev,
 				     struct device_opp *dev_opp)

Re: [PATCH 6/6] PM / OPP: Add debugfs support

[Viresh Kumar]

On 10-08-15, 12:50, Stephen Boyd wrote:
> The dev_name thing is going to fail. Eventually we'll get into a
> situation where two devices with the same name on different
> busses have OPPs. When we add them to debugfs their names are
> going to conflict. The regulator core suffered this problem, see
> commit a9eaa8130707 (regulator: Ensure unique regulator debugfs
> directory names, 2014-10-17) for one solution.

And here is the complete patch:

----------------------------8<----------------------------
Message-Id: <2f5ba56544df2838f61490765b365ab159558dae.1439274919.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Tue, 4 Aug 2015 11:57:36 +0530
Subject: [PATCH] PM / OPP: Add debugfs support

This patch adds debugfs support to OPP layer to export OPPs and their
properties for all the devices.

This creates a top level directory: /sys/kernel/debug/opp and then
device specific directories (based on device names) inside it. For
example: 'cpu-cpu0', 'cpu-cpu1', etc..

If multiple devices share the OPP table, then the real directory is
created only for the first device. For all others, links are created to
the real directory.

Inside the device specific directory, a separate directory is created
for each OPP. And within that files per opp property.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/Makefile  |   1 +
 drivers/base/power/opp/core.c    |  15 +++
 drivers/base/power/opp/debugfs.c | 218 +++++++++++++++++++++++++++++++++++++++
 drivers/base/power/opp/opp.h     |  43 ++++++++
 4 files changed, 277 insertions(+)
 create mode 100644 drivers/base/power/opp/debugfs.c

diff --git a/drivers/base/power/opp/Makefile b/drivers/base/power/opp/Makefile
index 33c1e18c41a4..19837ef04d8e 100644
--- a/drivers/base/power/opp/Makefile
+++ b/drivers/base/power/opp/Makefile
@@ -1,2 +1,3 @@
 ccflags-$(CONFIG_DEBUG_DRIVER)	:= -DDEBUG
 obj-y				+= core.o cpu.o
+obj-$(CONFIG_DEBUG_FS)		+= debugfs.o
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 5db5d2fd781f..871502dec381 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -429,6 +429,7 @@ static void _kfree_list_dev_rcu(struct rcu_head *head)
 static void _remove_list_dev(struct device_list_opp *list_dev,
 			     struct device_opp *dev_opp)
 {
+	opp_debug_unregister(list_dev, dev_opp);
 	list_del(&list_dev->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &list_dev->rcu_head,
 		  _kfree_list_dev_rcu);
@@ -438,6 +439,7 @@ struct device_list_opp *_add_list_dev(const struct device *dev,
 				      struct device_opp *dev_opp)
 {
 	struct device_list_opp *list_dev;
+	int ret;
 
 	list_dev = kzalloc(sizeof(*list_dev), GFP_KERNEL);
 	if (!list_dev)
@@ -447,6 +449,12 @@ struct device_list_opp *_add_list_dev(const struct device *dev,
 	list_dev->dev = dev;
 	list_add_rcu(&list_dev->node, &dev_opp->dev_list);
 
+	/* Create debugfs entries for the dev_opp */
+	ret = opp_debug_register(list_dev, dev_opp);
+	if (ret)
+		dev_err(dev, "%s: Failed to register opp debugfs (%d)\n",
+			__func__, ret);
+
 	return list_dev;
 }
 
@@ -562,6 +570,7 @@ static void _opp_remove(struct device_opp *dev_opp,
 	 */
 	if (notify)
 		srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
+	opp_debug_remove_one(opp);
 	list_del_rcu(&opp->node);
 	call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
 
@@ -639,6 +648,7 @@ static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 {
 	struct dev_pm_opp *opp;
 	struct list_head *head = &dev_opp->opp_list;
+	int ret;
 
 	/*
 	 * Insert new OPP in order of increasing frequency and discard if
@@ -669,6 +679,11 @@ static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	new_opp->dev_opp = dev_opp;
 	list_add_rcu(&new_opp->node, head);
 
+	ret = opp_debug_create_one(new_opp, dev_opp);
+	if (ret)
+		dev_err(dev, "%s: Failed to register opp to debugfs (%d)\n",
+			__func__, ret);
+
 	return 0;
 }
 
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
new file mode 100644
index 000000000000..6c048bfbf954
--- /dev/null
+++ b/drivers/base/power/opp/debugfs.c
@@ -0,0 +1,218 @@
+/*
+ * Generic OPP debugfs interface
+ *
+ * Copyright (C) 2015-2016 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/debugfs.h>
+#include <linux/err.h>
+
+#include "opp.h"
+
+static struct dentry *rootdir;
+
+static void opp_set_dev_name(const struct device *dev, char *name)
+{
+
+	if (dev->parent)
+		snprintf(name, NAME_MAX, "%s-%s", dev_name(dev->parent),
+			 dev_name(dev));
+	else
+		snprintf(name, NAME_MAX, "%s", dev_name(dev));
+}
+
+void opp_debug_remove_one(struct dev_pm_opp *opp)
+{
+	debugfs_remove_recursive(opp->dentry);
+}
+
+int opp_debug_create_one(struct dev_pm_opp *opp, struct device_opp *dev_opp)
+{
+	struct dentry *pdentry = dev_opp->dentry;
+	struct dentry *d;
+	char name[15];
+
+	/* Rate is unique to each OPP, use it to give opp-name */
+	sprintf(name, "opp:%lu", opp->rate);
+
+	/* Create per-opp directory */
+	d = debugfs_create_dir(name, pdentry);
+	if (!d)
+		return -ENOMEM;
+
+	if (!debugfs_create_bool("available", S_IRUGO, d,
+				 (u32 *)&opp->available))
+		return -ENOMEM;
+
+	if (!debugfs_create_bool("dynamic", S_IRUGO, d, (u32 *)&opp->dynamic))
+		return -ENOMEM;
+
+	if (!debugfs_create_bool("turbo", S_IRUGO, d, (u32 *)&opp->turbo))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("rate_hz", S_IRUGO, d, (u32 *)&opp->rate))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("u_volt_target", S_IRUGO, d,
+				(u32 *)&opp->u_volt))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("u_volt_min", S_IRUGO, d,
+				(u32 *)&opp->u_volt_min))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("u_volt_max", S_IRUGO, d,
+				(u32 *)&opp->u_volt_max))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("u_amp", S_IRUGO, d, (u32 *)&opp->u_amp))
+		return -ENOMEM;
+
+	if (!debugfs_create_u32("clock_latency_ns", S_IRUGO, d,
+				(u32 *)&opp->clock_latency_ns))
+		return -ENOMEM;
+
+	opp->dentry = d;
+	return 0;
+}
+
+static int device_opp_debug_create_dir(struct device_list_opp *list_dev,
+				       struct device_opp *dev_opp)
+{
+	const struct device *dev = list_dev->dev;
+	struct dentry *d;
+
+	opp_set_dev_name(dev, dev_opp->dentry_name);
+
+	/* Create device specific directory */
+	d = debugfs_create_dir(dev_opp->dentry_name, rootdir);
+	if (!d) {
+		dev_err(dev, "%s: Failed to create debugfs dir\n", __func__);
+		return -ENOMEM;
+	}
+
+	list_dev->dentry = d;
+	dev_opp->dentry = d;
+
+	return 0;
+}
+
+static int device_opp_debug_create_link(struct device_list_opp *list_dev,
+					struct device_opp *dev_opp)
+{
+	const struct device *dev = list_dev->dev;
+	char name[NAME_MAX];
+	struct dentry *d;
+
+	opp_set_dev_name(list_dev->dev, name);
+
+	/* Create device specific directory link */
+	d = debugfs_create_symlink(name, rootdir, dev_opp->dentry_name);
+	if (!d) {
+		dev_err(dev, "%s: Failed to create link\n", __func__);
+		return -ENOMEM;
+	}
+
+	list_dev->dentry = d;
+
+	return 0;
+}
+
+/**
+ * opp_debug_register - add a device opp node to the debugfs 'opp' directory
+ * @list_dev: list-dev pointer for device
+ * @dev_opp: the device-opp being added
+ *
+ * Dynamically adds device specific directory in debugfs 'opp' directory. If the
+ * device-opp is shared with other devices, then links will be created for all
+ * devices except the first.
+ *
+ * Return: 0 on success, otherwise negative error.
+ */
+int opp_debug_register(struct device_list_opp *list_dev,
+		       struct device_opp *dev_opp)
+{
+	if (!rootdir) {
+		pr_debug("%s: Uninitialized rootdir\n", __func__);
+		return -EINVAL;
+	}
+
+	if (dev_opp->dentry)
+		return device_opp_debug_create_link(list_dev, dev_opp);
+
+	return device_opp_debug_create_dir(list_dev, dev_opp);
+}
+
+static void opp_migrate_dentry(struct device_list_opp *list_dev,
+			       struct device_opp *dev_opp)
+{
+	struct device_list_opp *new_dev;
+	const struct device *dev;
+	struct dentry *dentry;
+
+	/* Look for next list-dev */
+	list_for_each_entry(new_dev, &dev_opp->dev_list, node)
+		if (new_dev != list_dev)
+			break;
+
+	/* new_dev is guaranteed to be valid here */
+	dev = new_dev->dev;
+	debugfs_remove_recursive(new_dev->dentry);
+
+	opp_set_dev_name(dev, dev_opp->dentry_name);
+
+	dentry = debugfs_rename(rootdir, list_dev->dentry, rootdir,
+				dev_opp->dentry_name);
+	if (!dentry) {
+		dev_err(dev, "%s: Failed to rename link from: %s to %s\n",
+			__func__, dev_name(list_dev->dev), dev_name(dev));
+		return;
+	}
+
+	new_dev->dentry = dentry;
+	dev_opp->dentry = dentry;
+}
+
+/**
+ * opp_debug_unregister - remove a device opp node from debugfs opp directory
+ * @list_dev: list-dev pointer for device
+ * @dev_opp: the device-opp being removed
+ *
+ * Dynamically removes device specific directory from debugfs 'opp' directory.
+ */
+void opp_debug_unregister(struct device_list_opp *list_dev,
+			  struct device_opp *dev_opp)
+{
+	if (list_dev->dentry == dev_opp->dentry) {
+		/* Move the real dentry object under another device */
+		if (!list_is_singular(&dev_opp->dev_list)) {
+			opp_migrate_dentry(list_dev, dev_opp);
+			goto out;
+		}
+		dev_opp->dentry = NULL;
+	}
+
+	debugfs_remove_recursive(list_dev->dentry);
+
+out:
+	list_dev->dentry = NULL;
+}
+
+static int __init opp_debug_init(void)
+{
+	/* Create /sys/kernel/debug/opp directory */
+	rootdir = debugfs_create_dir("opp", NULL);
+	if (!rootdir) {
+		pr_err("%s: Failed to create root directory\n", __func__);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+core_initcall(opp_debug_init);
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index dcb38f78dae4..0e4c27fb0c4a 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
+#include <linux/limits.h>
 #include <linux/pm_opp.h>
 #include <linux/rculist.h>
 #include <linux/rcupdate.h>
@@ -60,6 +61,7 @@
  * @dev_opp:	points back to the device_opp struct this opp belongs to
  * @rcu_head:	RCU callback head used for deferred freeing
  * @np:		OPP's device node.
+ * @dentry:	debugfs dentry pointer (per opp)
  *
  * This structure stores the OPP information for a given device.
  */
@@ -81,6 +83,11 @@ struct dev_pm_opp {
 	struct rcu_head rcu_head;
 
 	struct device_node *np;
+
+#ifdef CONFIG_DEBUG_FS
+	/* debugfs */
+	struct dentry *dentry;
+#endif
 };
 
 /**
@@ -88,6 +95,7 @@ struct dev_pm_opp {
  * @node:	list node
  * @dev:	device to which the struct object belongs
  * @rcu_head:	RCU callback head used for deferred freeing
+ * @dentry:	debugfs dentry pointer (per device)
  *
  * This is an internal data structure maintaining the list of devices that are
  * managed by 'struct device_opp'.
@@ -96,6 +104,11 @@ struct device_list_opp {
 	struct list_head node;
 	const struct device *dev;
 	struct rcu_head rcu_head;
+
+#ifdef CONFIG_DEBUG_FS
+	/* debugfs */
+	struct dentry *dentry;
+#endif
 };
 
 /**
@@ -111,6 +124,8 @@ struct device_list_opp {
  * @opp_list:	list of opps
  * @np:		struct device_node pointer for opp's DT node.
  * @shared_opp: OPP is shared between multiple devices.
+ * @dentry:	debugfs dentry pointer of the real device directory (not links).
+ * @dentry_name: Name of the real dentry.
  *
  * This is an internal data structure maintaining the link to opps attached to
  * a device. This structure is not meant to be shared to users as it is
@@ -132,6 +147,12 @@ struct device_opp {
 	unsigned long clock_latency_ns_max;
 	bool shared_opp;
 	struct dev_pm_opp *suspend_opp;
+
+#ifdef CONFIG_DEBUG_FS
+	/* debugfs */
+	struct dentry *dentry;
+	char dentry_name[NAME_MAX];
+#endif
 };
 
 /* Routines internal to opp core */
@@ -140,4 +161,26 @@ struct device_list_opp *_add_list_dev(const struct device *dev,
 				      struct device_opp *dev_opp);
 struct device_node *_of_get_opp_desc_node(struct device *dev);
 
+#ifdef CONFIG_DEBUG_FS
+void opp_debug_remove_one(struct dev_pm_opp *opp);
+int opp_debug_create_one(struct dev_pm_opp *opp, struct device_opp *dev_opp);
+int opp_debug_register(struct device_list_opp *list_dev,
+		       struct device_opp *dev_opp);
+void opp_debug_unregister(struct device_list_opp *list_dev,
+			  struct device_opp *dev_opp);
+#else
+static inline void opp_debug_remove_one(struct dev_pm_opp *opp) {}
+
+static inline int opp_debug_create_one(struct dev_pm_opp *opp,
+				       struct device_opp *dev_opp)
+{ return 0; }
+static inline int opp_debug_register(struct device_list_opp *list_dev,
+				     struct device_opp *dev_opp)
+{ return 0; }
+
+static inline void opp_debug_unregister(struct device_list_opp *list_dev,
+					struct device_opp *dev_opp)
+{ }
+#endif		/* DEBUG_FS */
+
 #endif		/* __DRIVER_OPP_H__ */