[RFC PATCH 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER

[RFC PATCH 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER

[Stephan Gerhold]

The OPP core manages various resources, e.g. clocks or interconnect paths.
These resources are looked up when the OPP table is allocated once
dev_pm_opp_get_opp_table() is called the first time (either directly
or indirectly through one of the many helper functions).

At this point, the resources may not be available yet, i.e. looking them
up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
is currently unable to propagate this error code since it only returns
the allocated OPP table or NULL.

This means that all consumers of the OPP core are required to make sure
that all necessary resources are available. Usually this happens by
requesting them, checking the result and releasing them immediately after.

For example, we have added "dev_pm_opp_of_find_icc_paths(dev, NULL)" to
several drivers now just to make sure the interconnect providers are
ready before the OPP table is allocated. If this call is missing,
the OPP core will only warn about this and then attempt to continue
without interconnect. This will eventually fail horribly, e.g.:

    cpu cpu0: _allocate_opp_table: Error finding interconnect paths: -517
    ... later ...
    of: _read_bw: Mismatch between opp-peak-kBps and paths (1 0)
    cpu cpu0: _opp_add_static_v2: opp key field not found
    cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22

This example happens when trying to use interconnects for a CPU OPP
table together with qcom-cpufreq-nvmem.c. qcom-cpufreq-nvmem calls
dev_pm_opp_set_supported_hw(), which ends up allocating the OPP table
early. To fix the problem with the current approach we would need to add
yet another call to dev_pm_opp_of_find_icc_paths(dev, NULL).
But actually qcom-cpufreq-nvmem.c has nothing to do with interconnects...

This commit attempts to make this more robust by allowing
dev_pm_opp_get_opp_table() to return an error pointer. Fixing all
the usages is trivial because the function is usually used indirectly
through another helper (e.g. dev_pm_opp_set_supported_hw() above).
These other helpers already return an error pointer.

The example above then works correctly because set_supported_hw() will
return -EPROBE_DEFER, and qcom-cpufreq-nvmem.c already propagates that
error. It should also be possible to remove the remaining usages of
"dev_pm_opp_of_find_icc_paths(dev, NULL)" from other drivers as well.

Note that this commit currently only handles -EPROBE_DEFER for the
clock/interconnects within _allocate_opp_table(). Other errors are just
ignored as before. Eventually those should be propagated as well.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
I wasn't sure if the changes in drivers/base/power/domain.c
should be made in a separate commit, but they need to be made together
with the other changes.

Also note that this is RFC because this is just something I got really
frustrated about. There might be situations where this won't work correctly...?
---
 drivers/base/power/domain.c | 28 ++++++++++++++++++-----
 drivers/opp/core.c          | 45 ++++++++++++++++++++++---------------
 drivers/opp/of.c            |  8 +++----
 3 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2cb5e04cf86c..c50f2de952c4 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2044,8 +2044,9 @@ int of_genpd_add_provider_simple(struct device_node *np,
 	if (genpd->set_performance_state) {
 		ret = dev_pm_opp_of_add_table(&genpd->dev);
 		if (ret) {
-			dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
-				ret);
+			if (ret != -EPROBE_DEFER)
+				dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
+					ret);
 			goto unlock;
 		}
 
@@ -2054,7 +2055,14 @@ int of_genpd_add_provider_simple(struct device_node *np,
 		 * state.
 		 */
 		genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev);
-		WARN_ON(!genpd->opp_table);
+		if (IS_ERR(genpd->opp_table)) {
+			if (PTR_ERR(genpd->opp_table) != -EPROBE_DEFER)
+				dev_err(&genpd->dev, "Failed to get OPP table: %pe\n",
+					genpd->opp_table);
+
+			dev_pm_opp_of_remove_table(&genpd->dev);
+			goto unlock;
+		}
 	}
 
 	ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
@@ -2111,8 +2119,9 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		if (genpd->set_performance_state) {
 			ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
 			if (ret) {
-				dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
-					i, ret);
+				if (ret != -EPROBE_DEFER)
+					dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
+						i, ret);
 				goto error;
 			}
 
@@ -2121,7 +2130,14 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 			 * performance state.
 			 */
 			genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i);
-			WARN_ON(!genpd->opp_table);
+			if (IS_ERR(genpd->opp_table)) {
+				if (PTR_ERR(genpd->opp_table) != -EPROBE_DEFER)
+					dev_err(&genpd->dev, "Failed to get OPP table: %pe\n",
+						genpd->opp_table);
+
+				dev_pm_opp_of_remove_table(&genpd->dev);
+				goto error;
+			}
 		}
 
 		genpd->provider = &np->fwnode;
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9d7fb45b1786..45d24de75e0e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1063,7 +1063,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 	 */
 	opp_table = kzalloc(sizeof(*opp_table), GFP_KERNEL);
 	if (!opp_table)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	mutex_init(&opp_table->lock);
 	mutex_init(&opp_table->genpd_virt_dev_lock);
@@ -1074,8 +1074,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 
 	opp_dev = _add_opp_dev(dev, opp_table);
 	if (!opp_dev) {
-		kfree(opp_table);
-		return NULL;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	_of_init_opp_table(opp_table, dev, index);
@@ -1087,13 +1087,18 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 		if (ret != -EPROBE_DEFER)
 			dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__,
 				ret);
+		else
+			goto err;
 	}
 
 	/* Find interconnect path(s) for the device */
 	ret = dev_pm_opp_of_find_icc_paths(dev, opp_table);
-	if (ret)
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			goto err;
 		dev_warn(dev, "%s: Error finding interconnect paths: %d\n",
 			 __func__, ret);
+	}
 
 	BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
 	INIT_LIST_HEAD(&opp_table->opp_list);
@@ -1102,6 +1107,10 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 	/* Secure the device table modification */
 	list_add(&opp_table->node, &opp_tables);
 	return opp_table;
+
+err:
+	kfree(opp_table);
+	return ERR_PTR(ret);
 }
 
 void _get_opp_table_kref(struct opp_table *opp_table)
@@ -1568,8 +1577,8 @@ struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev,
 	struct opp_table *opp_table;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(opp_table))
+		return opp_table;
 
 	/* Make sure there are no concurrent readers while updating opp_table */
 	WARN_ON(!list_empty(&opp_table->opp_list));
@@ -1627,8 +1636,8 @@ struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
 	struct opp_table *opp_table;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(opp_table))
+		return opp_table;
 
 	/* Make sure there are no concurrent readers while updating opp_table */
 	WARN_ON(!list_empty(&opp_table->opp_list));
@@ -1720,8 +1729,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
 	int ret, i;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(opp_table))
+		return opp_table;
 
 	/* This should be called before OPPs are initialized */
 	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
@@ -1830,8 +1839,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
 	int ret;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(opp_table))
+		return opp_table;
 
 	/* This should be called before OPPs are initialized */
 	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
@@ -1898,8 +1907,8 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
 		return ERR_PTR(-EINVAL);
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (!IS_ERR(opp_table))
+		return opp_table;
 
 	/* This should be called before OPPs are initialized */
 	if (WARN_ON(!list_empty(&opp_table->opp_list))) {
@@ -1979,8 +1988,8 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
 	const char **name = names;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(opp_table))
+		return opp_table;
 
 	/*
 	 * If the genpd's OPP table isn't already initialized, parsing of the
@@ -2150,8 +2159,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 	int ret;
 
 	opp_table = dev_pm_opp_get_opp_table(dev);
-	if (!opp_table)
-		return -ENOMEM;
+	if (IS_ERR(opp_table))
+		return PTR_ERR(opp_table);
 
 	/* Fix regulator count for dynamic OPPs */
 	opp_table->regulator_count = 1;
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 0430290670ab..d8b623cc015a 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -947,8 +947,8 @@ int dev_pm_opp_of_add_table(struct device *dev)
 	int ret;
 
 	opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0);
-	if (!opp_table)
-		return -ENOMEM;
+	if (IS_ERR(opp_table))
+		return PTR_ERR(opp_table);
 
 	/*
 	 * OPPs have two version of bindings now. Also try the old (v1)
@@ -1002,8 +1002,8 @@ int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
 	}
 
 	opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
-	if (!opp_table)
-		return -ENOMEM;
+	if (IS_ERR(opp_table))
+		return PTR_ERR(opp_table);
 
 	ret = _of_add_opp_table_v2(dev, opp_table);
 	if (ret)
-- 
2.27.0

[RFC PATCH 2/2] cpufreq: dt: Refactor initialization to handle probe deferral properly

[Stephan Gerhold]

cpufreq-dt is currently unable to handle -EPROBE_DEFER properly
because the error code is not propagated for the cpufreq_driver->init()
callback. Instead, it attempts to avoid the situation by temporarily
requesting all resources within resources_available() and releasing them
again immediately after. This has several disadvantages:

  - Whenever we add something like interconnect handling to the OPP core
    we need to patch cpufreq-dt to request these resources early.

  - resources_available() is only run for CPU0, but other clusters may
    eventually depend on other resources that are not available yet.
    (See FIXME comment removed by this commit...)

  - All resources need to be looked up several times.

Now that the OPP core can propagate -EPROBE_DEFER during initialization,
it would be nice to avoid all that trouble and just propagate its error
code when necessary.

This commit refactors the cpufreq-dt driver to initialize private_data
before registering the cpufreq driver. We do this by iterating over
all possible CPUs and ensure that all resources are initialized:

  1. dev_pm_opp_get_opp_table() ensures the OPP table is allocated
     and initialized with clock and interconnects.

  2. dev_pm_opp_set_regulators() requests the regulators and assigns
     them to the OPP table.

  3. We call dev_pm_opp_of_get_sharing_cpus() early so that we only
     initialize the OPP table once for each shared policy.

With these changes, we actually end up saving a few lines of code,
the resources are no longer looked up multiple times and everything
should be much more robust.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
An alternative (and possibly more clean) solution would be to handle
-EPROBE_DEFER for the cpufreq_driver->init() callback. However, this
would require patching subsys_interface_register() to handle the
return values of the subsys_interface->add_dev() callback (and
rollback appropriately). The subsys functionality is used in a lot
of places so I did not really feel comfortable changing that...
---
 drivers/cpufreq/cpufreq-dt.c | 275 +++++++++++++++++------------------
 1 file changed, 133 insertions(+), 142 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 944d7b45afe9..42cdde325dff 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -24,18 +24,35 @@
 #include "cpufreq-dt.h"
 
 struct private_data {
-	struct opp_table *opp_table;
+	cpumask_var_t cpus;
+	struct private_data *next;
+
 	struct device *cpu_dev;
-	const char *reg_name;
+	struct opp_table *opp_table;
+	struct opp_table *reg_opp_table;
 	bool have_static_opps;
 };
 
+static struct private_data *cpu_data;
+
 static struct freq_attr *cpufreq_dt_attr[] = {
 	&cpufreq_freq_attr_scaling_available_freqs,
 	NULL,   /* Extra space for boost-attr if required */
 	NULL,
 };
 
+static struct private_data *cpufreq_dt_find_data(int cpu)
+{
+	struct private_data *priv = cpu_data;
+
+	while (priv) {
+		if (cpumask_test_cpu(cpu, priv->cpus))
+			break;
+		priv = priv->next;
+	}
+	return priv;
+}
+
 static int set_target(struct cpufreq_policy *policy, unsigned int index)
 {
 	struct private_data *priv = policy->driver_data;
@@ -90,83 +107,23 @@ static const char *find_supply_name(struct device *dev)
 	return name;
 }
 
-static int resources_available(void)
-{
-	struct device *cpu_dev;
-	struct regulator *cpu_reg;
-	struct clk *cpu_clk;
-	int ret = 0;
-	const char *name;
-
-	cpu_dev = get_cpu_device(0);
-	if (!cpu_dev) {
-		pr_err("failed to get cpu0 device\n");
-		return -ENODEV;
-	}
-
-	cpu_clk = clk_get(cpu_dev, NULL);
-	ret = PTR_ERR_OR_ZERO(cpu_clk);
-	if (ret) {
-		/*
-		 * If cpu's clk node is present, but clock is not yet
-		 * registered, we should try defering probe.
-		 */
-		if (ret == -EPROBE_DEFER)
-			dev_dbg(cpu_dev, "clock not ready, retry\n");
-		else
-			dev_err(cpu_dev, "failed to get clock: %d\n", ret);
-
-		return ret;
-	}
-
-	clk_put(cpu_clk);
-
-	ret = dev_pm_opp_of_find_icc_paths(cpu_dev, NULL);
-	if (ret)
-		return ret;
-
-	name = find_supply_name(cpu_dev);
-	/* Platform doesn't require regulator */
-	if (!name)
-		return 0;
-
-	cpu_reg = regulator_get_optional(cpu_dev, name);
-	ret = PTR_ERR_OR_ZERO(cpu_reg);
-	if (ret) {
-		/*
-		 * If cpu's regulator supply node is present, but regulator is
-		 * not yet registered, we should try defering probe.
-		 */
-		if (ret == -EPROBE_DEFER)
-			dev_dbg(cpu_dev, "cpu0 regulator not ready, retry\n");
-		else
-			dev_dbg(cpu_dev, "no regulator for cpu0: %d\n", ret);
-
-		return ret;
-	}
-
-	regulator_put(cpu_reg);
-	return 0;
-}
-
 static int cpufreq_init(struct cpufreq_policy *policy)
 {
+	struct private_data *priv = cpufreq_dt_find_data(policy->cpu);
 	struct cpufreq_frequency_table *freq_table;
-	struct opp_table *opp_table = NULL;
-	struct private_data *priv;
 	struct device *cpu_dev;
 	struct clk *cpu_clk;
 	unsigned int transition_latency;
-	bool fallback = false;
-	const char *name;
 	int ret;
 
-	cpu_dev = get_cpu_device(policy->cpu);
-	if (!cpu_dev) {
-		pr_err("failed to get cpu%d device\n", policy->cpu);
+	if (!priv) {
+		pr_err("failed to find data for cpu%d\n", policy->cpu);
 		return -ENODEV;
 	}
 
+	cpu_dev = priv->cpu_dev;
+	cpumask_copy(policy->cpus, priv->cpus);
+
 	cpu_clk = clk_get(cpu_dev, NULL);
 	if (IS_ERR(cpu_clk)) {
 		ret = PTR_ERR(cpu_clk);
@@ -174,45 +131,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		return ret;
 	}
 
-	/* Get OPP-sharing information from "operating-points-v2" bindings */
-	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, policy->cpus);
-	if (ret) {
-		if (ret != -ENOENT)
-			goto out_put_clk;
-
-		/*
-		 * operating-points-v2 not supported, fallback to old method of
-		 * finding shared-OPPs for backward compatibility if the
-		 * platform hasn't set sharing CPUs.
-		 */
-		if (dev_pm_opp_get_sharing_cpus(cpu_dev, policy->cpus))
-			fallback = true;
-	}
-
-	/*
-	 * OPP layer will be taking care of regulators now, but it needs to know
-	 * the name of the regulator first.
-	 */
-	name = find_supply_name(cpu_dev);
-	if (name) {
-		opp_table = dev_pm_opp_set_regulators(cpu_dev, &name, 1);
-		if (IS_ERR(opp_table)) {
-			ret = PTR_ERR(opp_table);
-			dev_err(cpu_dev, "Failed to set regulator for cpu%d: %d\n",
-				policy->cpu, ret);
-			goto out_put_clk;
-		}
-	}
-
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		ret = -ENOMEM;
-		goto out_put_regulator;
-	}
-
-	priv->reg_name = name;
-	priv->opp_table = opp_table;
-
 	/*
 	 * Initialize OPP tables for all policy->cpus. They will be shared by
 	 * all CPUs which have marked their CPUs shared with OPP bindings.
@@ -237,26 +155,12 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		goto out_free_opp;
 	}
 
-	if (fallback) {
-		cpumask_setall(policy->cpus);
-
-		/*
-		 * OPP tables are initialized only for policy->cpu, do it for
-		 * others as well.
-		 */
-		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
-		if (ret)
-			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
-				__func__, ret);
-	}
-
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
 		dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
 		goto out_free_opp;
 	}
 
-	priv->cpu_dev = cpu_dev;
 	policy->driver_data = priv;
 	policy->clk = cpu_clk;
 	policy->freq_table = freq_table;
@@ -288,11 +192,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 out_free_opp:
 	if (priv->have_static_opps)
 		dev_pm_opp_of_cpumask_remove_table(policy->cpus);
-	kfree(priv);
-out_put_regulator:
-	if (name)
-		dev_pm_opp_put_regulators(opp_table);
-out_put_clk:
 	clk_put(cpu_clk);
 
 	return ret;
@@ -320,12 +219,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	if (priv->have_static_opps)
 		dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
-	if (priv->reg_name)
-		dev_pm_opp_put_regulators(priv->opp_table);
-
 	clk_put(policy->clk);
-	kfree(priv);
-
 	return 0;
 }
 
@@ -344,21 +238,112 @@ static struct cpufreq_driver dt_cpufreq_driver = {
 	.suspend = cpufreq_generic_suspend,
 };
 
-static int dt_cpufreq_probe(struct platform_device *pdev)
+static int dt_cpufreq_init_cpu(struct device *dev, int cpu)
 {
-	struct cpufreq_dt_platform_data *data = dev_get_platdata(&pdev->dev);
+	struct private_data *priv;
+	struct device *cpu_dev;
+	const char *reg_name;
 	int ret;
 
+	/* Check if this CPU is already covered by some other policy */
+	if (cpufreq_dt_find_data(cpu))
+		return 0;
+
+	cpu_dev = get_cpu_device(cpu);
+	if (!cpu_dev)
+		return -EPROBE_DEFER;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	if (!alloc_cpumask_var(&priv->cpus, GFP_KERNEL))
+		return -ENOMEM;
+
+	priv->cpu_dev = cpu_dev;
+	priv->next = cpu_data;
+	cpu_data = priv;
+
+	/* Try to get OPP table early to ensure resources are available */
+	priv->opp_table = dev_pm_opp_get_opp_table(cpu_dev);
+	if (IS_ERR(priv->opp_table)) {
+		ret = PTR_ERR(priv->opp_table);
+		if (ret != -EPROBE_DEFER)
+			dev_err(cpu_dev, "failed to get OPP table: %d\n", ret);
+		return ret;
+	}
+
 	/*
-	 * All per-cluster (CPUs sharing clock/voltages) initialization is done
-	 * from ->init(). In probe(), we just need to make sure that clk and
-	 * regulators are available. Else defer probe and retry.
-	 *
-	 * FIXME: Is checking this only for CPU0 sufficient ?
+	 * OPP layer will be taking care of regulators now, but it needs to know
+	 * the name of the regulator first.
 	 */
-	ret = resources_available();
-	if (ret)
-		return ret;
+	reg_name = find_supply_name(cpu_dev);
+	if (reg_name) {
+		priv->reg_opp_table = dev_pm_opp_set_regulators(cpu_dev,
+								&reg_name, 1);
+		if (IS_ERR(priv->reg_opp_table)) {
+			ret = PTR_ERR(priv->reg_opp_table);
+			if (ret != -EPROBE_DEFER)
+				dev_err(cpu_dev, "failed to set regulators: %d\n",
+					ret);
+			return ret;
+		}
+	}
+
+	/* Get OPP-sharing information from "operating-points-v2" bindings */
+	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, priv->cpus);
+	if (ret) {
+		if (ret != -ENOENT)
+			return ret;
+
+		/*
+		 * operating-points-v2 not supported, fallback to old method of
+		 * finding shared-OPPs for backward compatibility if the
+		 * platform hasn't set sharing CPUs.
+		 */
+		if (dev_pm_opp_get_sharing_cpus(cpu_dev, priv->cpus)) {
+			/* Fallback to all CPUs */
+			cpumask_setall(priv->cpus);
+
+			/*
+			 * OPP tables are initialized only for cpu, do it for
+			 * others as well.
+			 */
+			ret = dev_pm_opp_set_sharing_cpus(cpu_dev, priv->cpus);
+			if (ret)
+				dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
+					__func__, ret);
+		}
+	}
+
+	return 0;
+}
+
+static void dt_cpufreq_release(void)
+{
+	struct private_data *priv = cpu_data;
+
+	while (priv) {
+		if (!IS_ERR_OR_NULL(priv->reg_opp_table))
+			dev_pm_opp_put_regulators(priv->reg_opp_table);
+		if (!IS_ERR_OR_NULL(priv->opp_table))
+			dev_pm_opp_put_opp_table(priv->opp_table);
+		free_cpumask_var(priv->cpus);
+		priv = priv->next;
+	}
+	cpu_data = NULL;
+}
+
+static int dt_cpufreq_probe(struct platform_device *pdev)
+{
+	struct cpufreq_dt_platform_data *data = dev_get_platdata(&pdev->dev);
+	int ret, cpu;
+
+	/* Request resources early so we can return in case of -EPROBE_DEFER */
+	for_each_possible_cpu(cpu) {
+		ret = dt_cpufreq_init_cpu(&pdev->dev, cpu);
+		if (ret)
+			goto err;
+	};
 
 	if (data) {
 		if (data->have_governor_per_policy)
@@ -374,15 +359,21 @@ static int dt_cpufreq_probe(struct platform_device *pdev)
 	}
 
 	ret = cpufreq_register_driver(&dt_cpufreq_driver);
-	if (ret)
+	if (ret) {
 		dev_err(&pdev->dev, "failed register driver: %d\n", ret);
+		goto err;
+	}
 
+	return 0;
+err:
+	dt_cpufreq_release();
 	return ret;
 }
 
 static int dt_cpufreq_remove(struct platform_device *pdev)
 {
 	cpufreq_unregister_driver(&dt_cpufreq_driver);
+	dt_cpufreq_release();
 	return 0;
 }
 
-- 
2.27.0

Re: [RFC PATCH 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER

[Ulf Hansson]

On Mon, 27 Jul 2020 at 11:31, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> The OPP core manages various resources, e.g. clocks or interconnect paths.
> These resources are looked up when the OPP table is allocated once
> dev_pm_opp_get_opp_table() is called the first time (either directly
> or indirectly through one of the many helper functions).
>
> At this point, the resources may not be available yet, i.e. looking them
> up will result in -EPROBE_DEFER. Unfortunately, dev_pm_opp_get_opp_table()
> is currently unable to propagate this error code since it only returns
> the allocated OPP table or NULL.
>
> This means that all consumers of the OPP core are required to make sure
> that all necessary resources are available. Usually this happens by
> requesting them, checking the result and releasing them immediately after.
>
> For example, we have added "dev_pm_opp_of_find_icc_paths(dev, NULL)" to
> several drivers now just to make sure the interconnect providers are
> ready before the OPP table is allocated. If this call is missing,
> the OPP core will only warn about this and then attempt to continue
> without interconnect. This will eventually fail horribly, e.g.:
>
>     cpu cpu0: _allocate_opp_table: Error finding interconnect paths: -517
>     ... later ...
>     of: _read_bw: Mismatch between opp-peak-kBps and paths (1 0)
>     cpu cpu0: _opp_add_static_v2: opp key field not found
>     cpu cpu0: _of_add_opp_table_v2: Failed to add OPP, -22
>
> This example happens when trying to use interconnects for a CPU OPP
> table together with qcom-cpufreq-nvmem.c. qcom-cpufreq-nvmem calls
> dev_pm_opp_set_supported_hw(), which ends up allocating the OPP table
> early. To fix the problem with the current approach we would need to add
> yet another call to dev_pm_opp_of_find_icc_paths(dev, NULL).
> But actually qcom-cpufreq-nvmem.c has nothing to do with interconnects...
>
> This commit attempts to make this more robust by allowing
> dev_pm_opp_get_opp_table() to return an error pointer. Fixing all
> the usages is trivial because the function is usually used indirectly
> through another helper (e.g. dev_pm_opp_set_supported_hw() above).
> These other helpers already return an error pointer.
>
> The example above then works correctly because set_supported_hw() will
> return -EPROBE_DEFER, and qcom-cpufreq-nvmem.c already propagates that
> error. It should also be possible to remove the remaining usages of
> "dev_pm_opp_of_find_icc_paths(dev, NULL)" from other drivers as well.
>
> Note that this commit currently only handles -EPROBE_DEFER for the
> clock/interconnects within _allocate_opp_table(). Other errors are just
> ignored as before. Eventually those should be propagated as well.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> I wasn't sure if the changes in drivers/base/power/domain.c
> should be made in a separate commit, but they need to be made together
> with the other changes.

I would suggest to move the changes in drivers/base/power/domain.c
into a separate patch, still part of the series, but let it preceed
$subject patch.

>
> Also note that this is RFC because this is just something I got really
> frustrated about. There might be situations where this won't work correctly...?
> ---
>  drivers/base/power/domain.c | 28 ++++++++++++++++++-----

The above changes look good to me.

For the below, at a quick look I have no objections, but I am
deferring to Viresh to have a closer look.

Kind regards
Uffe

>  drivers/opp/core.c          | 45 ++++++++++++++++++++++---------------
>  drivers/opp/of.c            |  8 +++----
>  3 files changed, 53 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2cb5e04cf86c..c50f2de952c4 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2044,8 +2044,9 @@ int of_genpd_add_provider_simple(struct device_node *np,
>         if (genpd->set_performance_state) {
>                 ret = dev_pm_opp_of_add_table(&genpd->dev);
>                 if (ret) {
> -                       dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> -                               ret);
> +                       if (ret != -EPROBE_DEFER)
> +                               dev_err(&genpd->dev, "Failed to add OPP table: %d\n",
> +                                       ret);
>                         goto unlock;
>                 }
>
> @@ -2054,7 +2055,14 @@ int of_genpd_add_provider_simple(struct device_node *np,
>                  * state.
>                  */
>                 genpd->opp_table = dev_pm_opp_get_opp_table(&genpd->dev);
> -               WARN_ON(!genpd->opp_table);
> +               if (IS_ERR(genpd->opp_table)) {
> +                       if (PTR_ERR(genpd->opp_table) != -EPROBE_DEFER)
> +                               dev_err(&genpd->dev, "Failed to get OPP table: %pe\n",
> +                                       genpd->opp_table);
> +
> +                       dev_pm_opp_of_remove_table(&genpd->dev);
> +                       goto unlock;
> +               }
>         }
>
>         ret = genpd_add_provider(np, genpd_xlate_simple, genpd);
> @@ -2111,8 +2119,9 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                 if (genpd->set_performance_state) {
>                         ret = dev_pm_opp_of_add_table_indexed(&genpd->dev, i);
>                         if (ret) {
> -                               dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> -                                       i, ret);
> +                               if (ret != -EPROBE_DEFER)
> +                                       dev_err(&genpd->dev, "Failed to add OPP table for index %d: %d\n",
> +                                               i, ret);
>                                 goto error;
>                         }
>
> @@ -2121,7 +2130,14 @@ int of_genpd_add_provider_onecell(struct device_node *np,
>                          * performance state.
>                          */
>                         genpd->opp_table = dev_pm_opp_get_opp_table_indexed(&genpd->dev, i);
> -                       WARN_ON(!genpd->opp_table);
> +                       if (IS_ERR(genpd->opp_table)) {
> +                               if (PTR_ERR(genpd->opp_table) != -EPROBE_DEFER)
> +                                       dev_err(&genpd->dev, "Failed to get OPP table: %pe\n",
> +                                               genpd->opp_table);
> +
> +                               dev_pm_opp_of_remove_table(&genpd->dev);
> +                               goto error;
> +                       }
>                 }
>
>                 genpd->provider = &np->fwnode;
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 9d7fb45b1786..45d24de75e0e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1063,7 +1063,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>          */
>         opp_table = kzalloc(sizeof(*opp_table), GFP_KERNEL);
>         if (!opp_table)
> -               return NULL;
> +               return ERR_PTR(-ENOMEM);
>
>         mutex_init(&opp_table->lock);
>         mutex_init(&opp_table->genpd_virt_dev_lock);
> @@ -1074,8 +1074,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>
>         opp_dev = _add_opp_dev(dev, opp_table);
>         if (!opp_dev) {
> -               kfree(opp_table);
> -               return NULL;
> +               ret = -ENOMEM;
> +               goto err;
>         }
>
>         _of_init_opp_table(opp_table, dev, index);
> @@ -1087,13 +1087,18 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>                 if (ret != -EPROBE_DEFER)
>                         dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__,
>                                 ret);
> +               else
> +                       goto err;
>         }
>
>         /* Find interconnect path(s) for the device */
>         ret = dev_pm_opp_of_find_icc_paths(dev, opp_table);
> -       if (ret)
> +       if (ret) {
> +               if (ret == -EPROBE_DEFER)
> +                       goto err;
>                 dev_warn(dev, "%s: Error finding interconnect paths: %d\n",
>                          __func__, ret);
> +       }
>
>         BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
>         INIT_LIST_HEAD(&opp_table->opp_list);
> @@ -1102,6 +1107,10 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
>         /* Secure the device table modification */
>         list_add(&opp_table->node, &opp_tables);
>         return opp_table;
> +
> +err:
> +       kfree(opp_table);
> +       return ERR_PTR(ret);
>  }
>
>  void _get_opp_table_kref(struct opp_table *opp_table)
> @@ -1568,8 +1577,8 @@ struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev,
>         struct opp_table *opp_table;
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (IS_ERR(opp_table))
> +               return opp_table;
>
>         /* Make sure there are no concurrent readers while updating opp_table */
>         WARN_ON(!list_empty(&opp_table->opp_list));
> @@ -1627,8 +1636,8 @@ struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
>         struct opp_table *opp_table;
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (IS_ERR(opp_table))
> +               return opp_table;
>
>         /* Make sure there are no concurrent readers while updating opp_table */
>         WARN_ON(!list_empty(&opp_table->opp_list));
> @@ -1720,8 +1729,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>         int ret, i;
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (IS_ERR(opp_table))
> +               return opp_table;
>
>         /* This should be called before OPPs are initialized */
>         if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1830,8 +1839,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
>         int ret;
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (IS_ERR(opp_table))
> +               return opp_table;
>
>         /* This should be called before OPPs are initialized */
>         if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1898,8 +1907,8 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
>                 return ERR_PTR(-EINVAL);
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (!IS_ERR(opp_table))
> +               return opp_table;
>
>         /* This should be called before OPPs are initialized */
>         if (WARN_ON(!list_empty(&opp_table->opp_list))) {
> @@ -1979,8 +1988,8 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
>         const char **name = names;
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return ERR_PTR(-ENOMEM);
> +       if (IS_ERR(opp_table))
> +               return opp_table;
>
>         /*
>          * If the genpd's OPP table isn't already initialized, parsing of the
> @@ -2150,8 +2159,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>         int ret;
>
>         opp_table = dev_pm_opp_get_opp_table(dev);
> -       if (!opp_table)
> -               return -ENOMEM;
> +       if (IS_ERR(opp_table))
> +               return PTR_ERR(opp_table);
>
>         /* Fix regulator count for dynamic OPPs */
>         opp_table->regulator_count = 1;
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 0430290670ab..d8b623cc015a 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -947,8 +947,8 @@ int dev_pm_opp_of_add_table(struct device *dev)
>         int ret;
>
>         opp_table = dev_pm_opp_get_opp_table_indexed(dev, 0);
> -       if (!opp_table)
> -               return -ENOMEM;
> +       if (IS_ERR(opp_table))
> +               return PTR_ERR(opp_table);
>
>         /*
>          * OPPs have two version of bindings now. Also try the old (v1)
> @@ -1002,8 +1002,8 @@ int dev_pm_opp_of_add_table_indexed(struct device *dev, int index)
>         }
>
>         opp_table = dev_pm_opp_get_opp_table_indexed(dev, index);
> -       if (!opp_table)
> -               return -ENOMEM;
> +       if (IS_ERR(opp_table))
> +               return PTR_ERR(opp_table);
>
>         ret = _of_add_opp_table_v2(dev, opp_table);
>         if (ret)
> --
> 2.27.0
>

Re: [RFC PATCH 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER

[Stephan Gerhold]

On Wed, Aug 12, 2020 at 11:10:38AM +0200, Ulf Hansson wrote:
> > I wasn't sure if the changes in drivers/base/power/domain.c
> > should be made in a separate commit, but they need to be made together
> > with the other changes.
> 
> I would suggest to move the changes in drivers/base/power/domain.c
> into a separate patch, still part of the series, but let it preceed
> $subject patch.
> 

OK, will do that in v2 - thank you!

I have another small build fix reported by the kernel test robot,
but will wait with sending that out until Viresh had a chance to give
some feedback on the basic idea. :)

Thanks!
Stephan

Re: [RFC PATCH 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER

[Viresh Kumar]

On 12-08-20, 12:53, Stephan Gerhold wrote:
> On Wed, Aug 12, 2020 at 11:10:38AM +0200, Ulf Hansson wrote:
> > > I wasn't sure if the changes in drivers/base/power/domain.c
> > > should be made in a separate commit, but they need to be made together
> > > with the other changes.
> > 
> > I would suggest to move the changes in drivers/base/power/domain.c
> > into a separate patch, still part of the series, but let it preceed
> > $subject patch.
> > 
> 
> OK, will do that in v2 - thank you!
> 
> I have another small build fix reported by the kernel test robot,
> but will wait with sending that out until Viresh had a chance to give
> some feedback on the basic idea. :)

What was the issue that was reported ? I may end up applying V1 only
with some of my changes.

-- 
viresh

Re: [RFC PATCH 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER

[Stephan Gerhold]

Hi Viresh,

On Wed, Aug 12, 2020 at 08:31:22PM +0530, Viresh Kumar wrote:
> On 12-08-20, 12:53, Stephan Gerhold wrote:
> > I have another small build fix reported by the kernel test robot,
> > but will wait with sending that out until Viresh had a chance to give
> > some feedback on the basic idea. :)
> 
> What was the issue that was reported ? I may end up applying V1 only
> with some of my changes.
> 

It was this build error on SH (for the second patch):

>> drivers/cpufreq/cpufreq-dt.c:36:29: error: conflicting types for 'cpu_data'
      36 | static struct private_data *cpu_data;
         |                             ^~~~~~~~
   In file included from arch/sh/include/asm/thread_info.h:27,
                    from include/linux/thread_info.h:38,
                    from include/asm-generic/current.h:5,
                    from ./arch/sh/include/generated/asm/current.h:1,
                    from include/linux/mutex.h:14,
                    from include/linux/notifier.h:14,
                    from include/linux/clk.h:14,
                    from drivers/cpufreq/cpufreq-dt.c:11:
   arch/sh/include/asm/processor.h:90:26: note: previous declaration of 'cpu_data' was here
      90 | extern struct sh_cpuinfo cpu_data[];
         |                          ^~~~~~~~

I fixed it by renaming "cpu_data" to "cpufreq_dt_data". (I could not
think of a better name so feel free to use something else...)

Thanks!
Stephan

Re: [RFC PATCH 1/2] opp: Allow dev_pm_opp_get_opp_table() to return -EPROBE_DEFER

[Viresh Kumar]

On 12-08-20, 11:10, Ulf Hansson wrote:
> On Mon, 27 Jul 2020 at 11:31, Stephan Gerhold <stephan@gerhold.net> wrote:
> > I wasn't sure if the changes in drivers/base/power/domain.c
> > should be made in a separate commit, but they need to be made together
> > with the other changes.
> 
> I would suggest to move the changes in drivers/base/power/domain.c
> into a separate patch, still part of the series, but let it preceed
> $subject patch.

That can't be done as the return type of a routine was changing and that needs
to be sorted out in the same patch.

-- 
viresh