Patchwork [v5,4/8] soc: qcom: rpmpd: Add support for get/set performance state

login
register
mail settings
Submitter Rajendra Nayak
Date Dec. 4, 2018, 5:21 a.m.
Message ID <20181204052119.806-5-rnayak@codeaurora.org>
Download mbox | patch
Permalink /patch/671569/
State New
Headers show

Comments

Rajendra Nayak - Dec. 4, 2018, 5:21 a.m.
Add support for the .set_performace_state() and .opp_to_performance_state()
callbacks in the rpmpd driver.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/soc/qcom/rpmpd.c | 46 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
Stephen Boyd - Dec. 4, 2018, 11:14 p.m.
Quoting Rajendra Nayak (2018-12-03 21:21:15)
> @@ -221,6 +224,47 @@ static int rpmpd_power_off(struct generic_pm_domain *domain)
>         return ret;
>  }
>  
> +static int rpmpd_set_performance(struct generic_pm_domain *domain,
> +                                unsigned int state)
> +{
> +       int ret = 0;
> +       struct rpmpd *pd = domain_to_rpmpd(domain);
> +
> +       mutex_lock(&rpmpd_lock);
> +
> +       if (state > MAX_RPMPD_STATE)
> +               goto out;

Does this need to be under the mutex lock? Doesn't look like it.

> +
> +       pd->corner = state;
> +
> +       if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER))

Please drop useless parenthesis.

> +               goto out;
> +
> +       ret = rpmpd_aggregate_corner(pd);
> +
> +out:
> +       mutex_unlock(&rpmpd_lock);
> +
> +       return ret;
> +}
> +
> +static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd,
> +                                         struct dev_pm_opp *opp)
> +{
> +       struct device_node *np;
> +       unsigned int corner = 0;
> +
> +       np = dev_pm_opp_get_of_node(opp);
> +       if (of_property_read_u32(np, "qcom,level", &corner)) {
> +               pr_err("%s: missing 'qcom,level' property\n", __func__);

We leak np reference here, assuming dev_pm_opp_get_of_node() returns an
of_node_get() pointer to the caller.

> +               return 0;
> +       }
> +
> +       of_node_put(np);

This same code exists twice. Perhaps a helper needs to exist for
qcom_rpm_get_performance() to pull the number out of the DT.

> +
> +       return corner;
> +}
Rajendra Nayak - Dec. 5, 2018, 7:03 a.m.
On 12/5/2018 4:44 AM, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2018-12-03 21:21:15)
>> @@ -221,6 +224,47 @@ static int rpmpd_power_off(struct generic_pm_domain *domain)
>>          return ret;
>>   }
>>   
>> +static int rpmpd_set_performance(struct generic_pm_domain *domain,
>> +                                unsigned int state)
>> +{
>> +       int ret = 0;
>> +       struct rpmpd *pd = domain_to_rpmpd(domain);
>> +
>> +       mutex_lock(&rpmpd_lock);
>> +
>> +       if (state > MAX_RPMPD_STATE)
>> +               goto out;
> 
> Does this need to be under the mutex lock? Doesn't look like it.

Nope, will move it out.

> 
>> +
>> +       pd->corner = state;
>> +
>> +       if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER))
> 
> Please drop useless parenthesis.

sure

> 
>> +               goto out;
>> +
>> +       ret = rpmpd_aggregate_corner(pd);
>> +
>> +out:
>> +       mutex_unlock(&rpmpd_lock);
>> +
>> +       return ret;
>> +}
>> +
>> +static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd,
>> +                                         struct dev_pm_opp *opp)
>> +{
>> +       struct device_node *np;
>> +       unsigned int corner = 0;
>> +
>> +       np = dev_pm_opp_get_of_node(opp);
>> +       if (of_property_read_u32(np, "qcom,level", &corner)) {
>> +               pr_err("%s: missing 'qcom,level' property\n", __func__);
> 
> We leak np reference here, assuming dev_pm_opp_get_of_node() returns an
> of_node_get() pointer to the caller.

good catch, will fix.

> 
>> +               return 0;
>> +       }
>> +
>> +       of_node_put(np);
> 
> This same code exists twice. Perhaps a helper needs to exist for
> qcom_rpm_get_performance() to pull the number out of the DT.

Sure I can make both drivers use a common helper instead of duplicating it.

> 
>> +
>> +       return corner;
>> +}
Rajendra Nayak - Dec. 5, 2018, 10:11 a.m.
On 12/5/2018 12:33 PM, Rajendra Nayak wrote:
>>
>>> +               return 0;
>>> +       }
>>> +
>>> +       of_node_put(np);
>>
>> This same code exists twice. Perhaps a helper needs to exist for
>> qcom_rpm_get_performance() to pull the number out of the DT.
> 
> Sure I can make both drivers use a common helper instead of duplicating it.

which would mean I will need to create a new file just to define the
common helper. Does that seem like an overkill?
Stephen Boyd - Dec. 5, 2018, 8:46 p.m.
Quoting Rajendra Nayak (2018-12-05 02:11:22)
> 
> On 12/5/2018 12:33 PM, Rajendra Nayak wrote:
> >>
> >>> +               return 0;
> >>> +       }
> >>> +
> >>> +       of_node_put(np);
> >>
> >> This same code exists twice. Perhaps a helper needs to exist for
> >> qcom_rpm_get_performance() to pull the number out of the DT.
> > 
> > Sure I can make both drivers use a common helper instead of duplicating it.
> 
> which would mean I will need to create a new file just to define the
> common helper. Does that seem like an overkill?

Maybe put it in the genpd code and let it take a const char *name
argument that picks out the property that drivers want to look at? That
way other OPP properties can be picked out with a simple call to the
function but it's generic enough to be used in other places.

Patch

diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
index a0b9f122d793..eb1cfa6a03d6 100644
--- a/drivers/soc/qcom/rpmpd.c
+++ b/drivers/soc/qcom/rpmpd.c
@@ -12,6 +12,7 @@ 
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/soc/qcom/smd-rpm.h>
 
 #include <dt-bindings/mfd/qcom-rpm.h>
@@ -28,6 +29,8 @@ 
 #define KEY_ENABLE		0x6e657773 /* swen */
 #define KEY_FLOOR_CORNER	0x636676   /* vfc */
 
+#define MAX_RPMPD_STATE		6
+
 #define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id)		\
 	static struct rpmpd _platform##_##_active;			\
 	static struct rpmpd _platform##_##_name = {			\
@@ -221,6 +224,47 @@  static int rpmpd_power_off(struct generic_pm_domain *domain)
 	return ret;
 }
 
+static int rpmpd_set_performance(struct generic_pm_domain *domain,
+				 unsigned int state)
+{
+	int ret = 0;
+	struct rpmpd *pd = domain_to_rpmpd(domain);
+
+	mutex_lock(&rpmpd_lock);
+
+	if (state > MAX_RPMPD_STATE)
+		goto out;
+
+	pd->corner = state;
+
+	if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER))
+		goto out;
+
+	ret = rpmpd_aggregate_corner(pd);
+
+out:
+	mutex_unlock(&rpmpd_lock);
+
+	return ret;
+}
+
+static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd,
+					  struct dev_pm_opp *opp)
+{
+	struct device_node *np;
+	unsigned int corner = 0;
+
+	np = dev_pm_opp_get_of_node(opp);
+	if (of_property_read_u32(np, "qcom,level", &corner)) {
+		pr_err("%s: missing 'qcom,level' property\n", __func__);
+		return 0;
+	}
+
+	of_node_put(np);
+
+	return corner;
+}
+
 static int rpmpd_probe(struct platform_device *pdev)
 {
 	int i;
@@ -261,6 +305,8 @@  static int rpmpd_probe(struct platform_device *pdev)
 		rpmpds[i]->rpm = rpm;
 		rpmpds[i]->pd.power_off = rpmpd_power_off;
 		rpmpds[i]->pd.power_on = rpmpd_power_on;
+		rpmpds[i]->pd.set_performance_state = rpmpd_set_performance;
+		rpmpds[i]->pd.opp_to_performance_state = rpmpd_get_performance;
 		pm_genpd_init(&rpmpds[i]->pd, NULL, true);
 
 		data->domains[i] = &rpmpds[i]->pd;