Re: [PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state
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.
Re: [PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state
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.
Re: [PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state
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?
Re: [PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state
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?
Re: [PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state
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(_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(_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", )) {
+ 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;
+}
Re: [PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state
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(_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(_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", )) {
+ 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;
+}
Re: [PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state
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(_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(_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", )) {
> + 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;
> +}
Re: [PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state
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(_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(_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", )) {
> + 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;
> +}
[PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state
Add support for the .set_performace_state() and .opp_to_performance_state()
callbacks in the rpmpd driver.
Signed-off-by: Rajendra Nayak
Signed-off-by: Viresh Kumar
Reviewed-by: Ulf Hansson
---
drivers/soc/qcom/rpmpd.c | 46
1 file changed, 46 insertions(+)
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
#include
#include
+#include
#include
#include
@@ -28,6 +29,8 @@
#define KEY_ENABLE 0x6e657773 /* swen */
#define KEY_FLOOR_CORNER 0x636676 /* vfc */
+#define MAX_RPMPD_STATE6
+
#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(_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(_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", )) {
+ 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([i]->pd, NULL, true);
data->domains[i] = [i]->pd;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
[PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state
Add support for the .set_performace_state() and .opp_to_performance_state()
callbacks in the rpmpd driver.
Signed-off-by: Rajendra Nayak
Signed-off-by: Viresh Kumar
Reviewed-by: Ulf Hansson
---
drivers/soc/qcom/rpmpd.c | 46
1 file changed, 46 insertions(+)
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
#include
#include
+#include
#include
#include
@@ -28,6 +29,8 @@
#define KEY_ENABLE 0x6e657773 /* swen */
#define KEY_FLOOR_CORNER 0x636676 /* vfc */
+#define MAX_RPMPD_STATE6
+
#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(_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(_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", )) {
+ 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([i]->pd, NULL, true);
data->domains[i] = [i]->pd;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
