diff mbox

[17/17] cpufreq: dt: No need to allocate resources anymore

Message ID df2534b4ced57791ec3ee42eb34211e132aa423a.1450777582.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar Dec. 22, 2015, 10:16 a.m. UTC 
OPP layer manages it now and cpufreq-dt driver doesn't need it. But, we
still need to check for availability of resources for deferred probing.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/cpufreq/cpufreq-dt.c | 70 ++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 45 deletions(-)

-- 
2.7.0.rc1.186.g94414c4

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Viresh Kumar Jan. 12, 2016, 7:34 a.m. UTC | #1 
On 11-01-16, 18:20, Stephen Boyd wrote:
> On 12/22, Viresh Kumar wrote:

> > OPP layer manages it now and cpufreq-dt driver doesn't need it. But, we

> > still need to check for availability of resources for deferred probing.

> 

> Why? It seems cleaner to let OPP layer return an error indicating

> probe defer or failure when we try to initialize it. That way we

> aren't duplicating the same logic in two places to figure out if

> a regulator or clock is ready.


cpufreq driver's ->init() callback doesn't return the error value
properly to the probe() function, and so it was done this way in the
first place. The problem is in subsys framework. I tried to fix it but
it was rejected and we need to fix it some other way:

http://marc.info/?l=linux-kernel&m=143530948918819&w=2

> >  	policy->clk = cpu_clk;

> 

> Maybe we can have an dev_pm_opp_get_rate() API and a

> cpufreq_generic_opp_get() so we can get rid of policy->clk usage

> in this driver?


Okay, will do.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Jan. 21, 2016, 2:36 a.m. UTC | #2 
On 20-01-16, 17:18, Stephen Boyd wrote:
> Yeah I don't understand why we at least can't populate the OPP

> structures and get the clocks and regulators for all the CPU

> devices before we register the dt_cpufreq_driver structure. The

> CPU devices should exist at that point, and we can wait to do

> CPUfreq transitions until the regulators/clocks for all the CPUs

> are registered. Sure we'd need to find the OPPs that are being

> shared in the cpufreq_init callback and populate the cpu

> frequency tables, etc., but that's not a big deal.


We can do this, yes. But ->init() was really the right place to fix
that, we aren't able to do it properly because we lack a cpu processor
driver for ARM.

> Making a CPU processor driver on ARM/non-ACPI systems would be to

> solve the problem of all these random things like cpuidle-arm and

> cpufreq-dt going through the list of cpu devices and hooking up

> stuff to the cpuidle, cpufreq, and thermal frameworks. That's a

> separate but related problem to probe defering the cpufreq-dt

> driver.


Yeah, I wanted to work on that sometime :)

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Jan. 25, 2016, 10:33 a.m. UTC | #3 
On 11-01-16, 18:20, Stephen Boyd wrote:
> > @@ -241,9 +234,7 @@ static int cpufreq_init(struct cpufreq_policy *policy)

> >  	}

> >  

> >  	priv->cpu_dev = cpu_dev;

> > -	priv->cpu_reg = cpu_reg;

> >  	policy->driver_data = priv;

> > -

> >  	policy->clk = cpu_clk;

> 

> Maybe we can have an dev_pm_opp_get_rate() API and a

> cpufreq_generic_opp_get() so we can get rid of policy->clk usage

> in this driver?


I thought about this today and couldn't get to a sane solution.
If we implement dev_pm_opp_get_rate(), then it should return the
cached value of the currently programmed OPP. If the OPP isn't set
yet, it can call clk_get_rate() and return that instead. But it
doesn't make any sense to always call clk_get_rate() from
dev_pm_opp_get_rate(), as that doesn't match its name.

OTOH, cpufreq really really needs a clk_get_rate() to happen, there
can be cases where the clk is changed by some other entity, specially
during suspend/resume and there are special checks in cpufreq about
that.

We can surely add a patch later if we get answer to this, but for now,
I will keep this patch as is.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index c9b7dafd986d..c778d20f8c37 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -31,7 +31,6 @@ 
 
 struct private_data {
 	struct device *cpu_dev;
-	struct regulator *cpu_reg;
 	struct thermal_cooling_device *cdev;
 	const char *reg_name;
 };
@@ -66,8 +65,7 @@  static const char *find_supply_name(struct device *dev)
 	return NULL;
 }
 
-static int allocate_resources(int cpu, struct device **cdev,
-			      struct regulator **creg, struct clk **cclk)
+static int resources_available(void)
 {
 	struct device *cpu_dev;
 	struct regulator *cpu_reg;
@@ -75,17 +73,14 @@  static int allocate_resources(int cpu, struct device **cdev,
 	int ret = 0;
 	char *reg_cpu0 = "cpu0", *reg_cpu = "cpu", *reg;
 
-	cpu_dev = get_cpu_device(cpu);
+	cpu_dev = get_cpu_device(0);
 	if (!cpu_dev) {
-		pr_err("failed to get cpu%d device\n", cpu);
+		pr_err("failed to get cpu0 device\n");
 		return -ENODEV;
 	}
 
 	/* Try "cpu0" for older DTs */
-	if (!cpu)
-		reg = reg_cpu0;
-	else
-		reg = reg_cpu;
+	reg = reg_cpu0;
 
 try_again:
 	cpu_reg = regulator_get_optional(cpu_dev, reg);
@@ -95,8 +90,8 @@  static int allocate_resources(int cpu, struct device **cdev,
 		 * not yet registered, we should try defering probe.
 		 */
 		if (PTR_ERR(cpu_reg) == -EPROBE_DEFER) {
-			dev_dbg(cpu_dev, "cpu%d regulator not ready, retry\n",
-				cpu);
+			dev_dbg(cpu_dev, "cpu%s regulator not ready, retry\n",
+				reg);
 			return -EPROBE_DEFER;
 		}
 
@@ -105,17 +100,12 @@  static int allocate_resources(int cpu, struct device **cdev,
 			reg = reg_cpu;
 			goto try_again;
 		}
-
-		dev_dbg(cpu_dev, "no regulator for cpu%d: %ld\n",
-			cpu, PTR_ERR(cpu_reg));
+	} else {
+		regulator_put(cpu_reg);
 	}
 
 	cpu_clk = clk_get(cpu_dev, NULL);
 	if (IS_ERR(cpu_clk)) {
-		/* put regulator */
-		if (!IS_ERR(cpu_reg))
-			regulator_put(cpu_reg);
-
 		ret = PTR_ERR(cpu_clk);
 
 		/*
@@ -123,14 +113,11 @@  static int allocate_resources(int cpu, struct device **cdev,
 		 * registered, we should try defering probe.
 		 */
 		if (ret == -EPROBE_DEFER)
-			dev_dbg(cpu_dev, "cpu%d clock not ready, retry\n", cpu);
+			dev_dbg(cpu_dev, "cpu0 clock not ready, retry\n");
 		else
-			dev_err(cpu_dev, "failed to get cpu%d clock: %d\n", cpu,
-				ret);
+			dev_err(cpu_dev, "failed to get cpu0 clock: %d\n", ret);
 	} else {
-		*cdev = cpu_dev;
-		*creg = cpu_reg;
-		*cclk = cpu_clk;
+		clk_put(cpu_clk);
 	}
 
 	return ret;
@@ -141,7 +128,6 @@  static int cpufreq_init(struct cpufreq_policy *policy)
 	struct cpufreq_frequency_table *freq_table;
 	struct private_data *priv;
 	struct device *cpu_dev;
-	struct regulator *cpu_reg;
 	struct clk *cpu_clk;
 	struct dev_pm_opp *suspend_opp;
 	unsigned int transition_latency;
@@ -149,9 +135,16 @@  static int cpufreq_init(struct cpufreq_policy *policy)
 	const char *name = NULL;
 	int ret;
 
-	ret = allocate_resources(policy->cpu, &cpu_dev, &cpu_reg, &cpu_clk);
-	if (ret) {
-		pr_err("%s: Failed to allocate resources: %d\n", __func__, ret);
+	cpu_dev = get_cpu_device(policy->cpu);
+	if (!cpu_dev) {
+		pr_err("failed to get cpu%d device\n", policy->cpu);
+		return -ENODEV;
+	}
+
+	cpu_clk = clk_get(cpu_dev, NULL);
+	if (IS_ERR(cpu_clk)) {
+		ret = PTR_ERR(cpu_clk);
+		dev_err(cpu_dev, "%s: failed to get clk: %d\n", __func__, ret);
 		return ret;
 	}
 
@@ -165,7 +158,7 @@  static int cpufreq_init(struct cpufreq_policy *policy)
 		if (ret == -ENOENT)
 			opp_v1 = true;
 		else
-			goto out_put_reg_clk;
+			goto out_put_clk;
 	}
 
 	/*
@@ -241,9 +234,7 @@  static int cpufreq_init(struct cpufreq_policy *policy)
 	}
 
 	priv->cpu_dev = cpu_dev;
-	priv->cpu_reg = cpu_reg;
 	policy->driver_data = priv;
-
 	policy->clk = cpu_clk;
 
 	rcu_read_lock();
@@ -284,10 +275,8 @@  static int cpufreq_init(struct cpufreq_policy *policy)
 	dev_pm_opp_of_cpumask_remove_table(policy->cpus);
 	if (opp_v1)
 		dev_pm_opp_put_regulator(cpu_dev);
-out_put_reg_clk:
+out_put_clk:
 	clk_put(cpu_clk);
-	if (!IS_ERR(cpu_reg))
-		regulator_put(cpu_reg);
 
 	return ret;
 }
@@ -303,8 +292,6 @@  static int cpufreq_exit(struct cpufreq_policy *policy)
 		dev_pm_opp_put_regulator(priv->cpu_dev);
 
 	clk_put(policy->clk);
-	if (!IS_ERR(priv->cpu_reg))
-		regulator_put(priv->cpu_reg);
 	kfree(priv);
 
 	return 0;
@@ -357,9 +344,6 @@  static struct cpufreq_driver dt_cpufreq_driver = {
 
 static int dt_cpufreq_probe(struct platform_device *pdev)
 {
-	struct device *cpu_dev;
-	struct regulator *cpu_reg;
-	struct clk *cpu_clk;
 	int ret;
 
 	/*
@@ -369,19 +353,15 @@  static int dt_cpufreq_probe(struct platform_device *pdev)
 	 *
 	 * FIXME: Is checking this only for CPU0 sufficient ?
 	 */
-	ret = allocate_resources(0, &cpu_dev, &cpu_reg, &cpu_clk);
+	ret = resources_available();
 	if (ret)
 		return ret;
 
-	clk_put(cpu_clk);
-	if (!IS_ERR(cpu_reg))
-		regulator_put(cpu_reg);
-
 	dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);
 
 	ret = cpufreq_register_driver(&dt_cpufreq_driver);
 	if (ret)
-		dev_err(cpu_dev, "failed register driver: %d\n", ret);
+		dev_err(&pdev->dev, "failed register driver: %d\n", ret);
 
 	return ret;
 }