[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20240902224815.78220-3-ulf.hansson@linaro.org>
Date: Tue, 3 Sep 2024 00:48:15 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Viresh Kumar <vireshk@...nel.org>,
Nishanth Menon <nm@...com>,
Stephen Boyd <sboyd@...nel.org>,
Dikshita Agarwal <quic_dikshita@...cinc.com>
Cc: Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <quic_kdybcio@...cinc.com>,
Nikunj Kela <nkela@...cinc.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Thierry Reding <thierry.reding@...il.com>,
Mikko Perttunen <mperttunen@...dia.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Stephan Gerhold <stephan@...hold.net>,
Ilia Lin <ilia.lin@...nel.org>,
Stanimir Varbanov <stanimir.k.varbanov@...il.com>,
Vikash Garodia <quic_vgarodia@...cinc.com>,
Ulf Hansson <ulf.hansson@...aro.org>,
linux-pm@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: [PATCH 2/2] OPP/pmdomain: Fix the assignment of the required-devs
The recent attempt to make genpd first lookup an OPP table for a device
that has been attached to it and then let the OPP core validate whether the
OPP table is correct, fails for some configurations.
More precisely, if a device has multiple power-domains, which all has an
OPP table associated, doesn't necessarily mean that the device's OPP table
needs multiple phandles specified in the required-opps. Instead it's
perfectly possible to use a single phandle in the required-opps, which is
typically where the current code fails to assign the correct required-dev.
To fix this problem, let's instead start by letting the OPP core find the
device node for the required OPP table and then let genpd search for a
corresponding OPP table, allowing us the find the correct required-dev to
assign for it.
Reported-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
Fixes: f0d2dcc9b087 ("OPP/pmdomain: Set the required_dev for a required OPP during genpd attach")
Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
---
drivers/opp/core.c | 15 +++++++-----
drivers/pmdomain/core.c | 52 +++++++++++++++++++++++------------------
include/linux/pm_opp.h | 7 ++++--
3 files changed, 43 insertions(+), 31 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 66cac7a1d9db..538612886446 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2363,7 +2363,7 @@ static void _opp_put_config_regulators_helper(struct opp_table *opp_table)
static int _opp_set_required_dev(struct opp_table *opp_table,
struct device *dev,
struct device *required_dev,
- struct opp_table *required_opp_table)
+ config_required_dev_t config_required_dev)
{
int i;
@@ -2380,6 +2380,7 @@ static int _opp_set_required_dev(struct opp_table *opp_table,
for (i = 0; i < opp_table->required_opp_count; i++) {
struct opp_table *table = opp_table->required_opp_tables[i];
+ struct opp_table *required_opp_table;
/*
* The OPP table should be available at this point. If not, it's
@@ -2396,7 +2397,9 @@ static int _opp_set_required_dev(struct opp_table *opp_table,
* We need to compare the nodes for the OPP tables, rather than
* the OPP tables themselves, as we may have separate instances.
*/
- if (required_opp_table->np == table->np) {
+ required_opp_table = config_required_dev(required_dev,
+ table->np);
+ if (required_opp_table) {
/*
* The required_opp_tables parsing is not perfect, as
* the OPP core does the parsing solely based on the DT
@@ -2422,8 +2425,8 @@ static int _opp_set_required_dev(struct opp_table *opp_table,
}
}
- dev_err(dev, "Missing OPP table, unable to set the required dev\n");
- return -ENODEV;
+ /* No matching OPP table found for the required_dev. */
+ return -ERANGE;
}
static void _opp_put_required_dev(struct opp_table *opp_table,
@@ -2547,10 +2550,10 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
data->flags |= OPP_CONFIG_REGULATOR;
}
- if (config->required_dev && config->required_opp_table) {
+ if (config->required_dev && config->config_required_dev) {
ret = _opp_set_required_dev(opp_table, dev,
config->required_dev,
- config->required_opp_table);
+ config->config_required_dev);
if (ret < 0)
goto err;
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index edef1a520110..0ff0b513b2a1 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2874,20 +2874,21 @@ static void genpd_dev_pm_sync(struct device *dev)
genpd_queue_power_off_work(pd);
}
-static struct opp_table *genpd_find_opp_table(struct generic_pm_domain *genpd,
- unsigned int depth)
+static struct opp_table *_genpd_find_opp_table(struct generic_pm_domain *genpd,
+ struct device_node *np,
+ unsigned int depth)
{
- struct opp_table *opp_table;
+ struct opp_table *opp_table = genpd->opp_table;
struct gpd_link *link;
- if (genpd->opp_table)
- return genpd->opp_table;
+ if (opp_table && (dev_pm_opp_table_to_of_node(opp_table) == np))
+ return opp_table;
list_for_each_entry(link, &genpd->child_links, child_node) {
struct generic_pm_domain *parent = link->parent;
genpd_lock_nested(parent, depth + 1);
- opp_table = genpd_find_opp_table(parent, depth + 1);
+ opp_table = _genpd_find_opp_table(parent, np, depth + 1);
genpd_unlock(parent);
if (opp_table)
@@ -2897,12 +2898,27 @@ static struct opp_table *genpd_find_opp_table(struct generic_pm_domain *genpd,
return NULL;
}
-static int genpd_set_required_opp_dev(struct device *dev,
- struct device *base_dev)
+static struct opp_table *genpd_find_opp_table(struct device *dev,
+ struct device_node *np)
{
struct generic_pm_domain *genpd = dev_to_genpd(dev);
struct opp_table *opp_table;
- int ret = 0;
+
+ genpd_lock(genpd);
+ opp_table = _genpd_find_opp_table(genpd, np, 0);
+ genpd_unlock(genpd);
+
+ return opp_table;
+}
+
+static int genpd_set_required_opp_dev(struct device *dev,
+ struct device *base_dev)
+{
+ struct dev_pm_opp_config config = {
+ .required_dev = dev,
+ .config_required_dev = genpd_find_opp_table,
+ };
+ int ret;
/* Limit support to non-providers for now. */
if (of_property_present(base_dev->of_node, "#power-domain-cells"))
@@ -2911,20 +2927,10 @@ static int genpd_set_required_opp_dev(struct device *dev,
if (!dev_pm_opp_of_has_required_opp(base_dev))
return 0;
- genpd_lock(genpd);
- opp_table = genpd_find_opp_table(genpd, 0);
- genpd_unlock(genpd);
-
- if (opp_table) {
- struct dev_pm_opp_config config = {
- .required_dev = dev,
- .required_opp_table = opp_table,
- };
-
- ret = devm_pm_opp_set_config(base_dev, &config);
- if (ret < 0)
- dev_err(dev, "failed to set opp config %d\n", ret);
- }
+ ret = devm_pm_opp_set_config(base_dev, &config);
+ ret = ret == -ERANGE ? 0 : ret;
+ if (ret < 0)
+ dev_err(dev, "failed to set opp config %d\n", ret);
return ret;
}
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 7894e631cded..0ada4a5057c8 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -53,6 +53,9 @@ typedef int (*config_regulators_t)(struct device *dev,
typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table,
struct dev_pm_opp *opp, void *data, bool scaling_down);
+typedef struct opp_table *(*config_required_dev_t)(struct device *dev,
+ struct device_node *opp_table_np);
+
/**
* struct dev_pm_opp_config - Device OPP configuration values
* @clk_names: Clk names, NULL terminated array.
@@ -63,7 +66,7 @@ typedef int (*config_clks_t)(struct device *dev, struct opp_table *opp_table,
* @supported_hw_count: Number of elements in the array.
* @regulator_names: Array of pointers to the names of the regulator, NULL terminated.
* @required_dev: Required OPP device.
- * @required_opp_table: The corresponding required OPP table for @required_dev.
+ * @config_required_dev: Custom helper to find the required OPP table for $required_dev.
*
* This structure contains platform specific OPP configurations for the device.
*/
@@ -77,7 +80,7 @@ struct dev_pm_opp_config {
unsigned int supported_hw_count;
const char * const *regulator_names;
struct device *required_dev;
- struct opp_table *required_opp_table;
+ config_required_dev_t config_required_dev;
};
#define OPP_LEVEL_UNSET U32_MAX
--
2.34.1
Powered by blists - more mailing lists