| Message ID | 20240902224815.78220-3-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | OPP/pmdomain: Assign the correct required-dev | expand |
On 04-09-24, 14:57, Ulf Hansson wrote: > > Yeah, I missed that, it doesn't happen via DT but by platform code. I > > do see problems where situation would be a bit ambiguous. Your example > > with a minor change to your code: > > > > opp_table_devA: opp-table-devA { > > compatible = "operating-points-v2"; > > > > opp-devA-50 { > > opp-hz = /bits/ 64 <2500>; > > required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order) > > }; > > .... > > > > devA { > > compatible = "foo,bar"; > > power-domains = <&pd_perf0>, <&pd_perf1>; //both > > pd_perf0 and pd_perf1 has OPP tables. > > power-domain-names = "perf0", "perf1"; > > operating-points-v2 = <&opp_table_devA>; > > }; > > > > Here, I don't think there is a way for us to know which genpd does > > opp_pd_50 belongs to and to which one opp_pd_51 does. > > > > We solve this by sending clock_names and regulator_names in OPP > > config structure. That gives the ordering in which required_opps are > > present. The same needs to be done for genpd, and then genpd core > > would be able to attach the right genpd with right required opp. > > No, we don't need this for gend as $subject patch is addressing this > problem too. Let me elaborate. > > The OPP core holds the information about the devA's required-opps and > to what OPP table each required-opps belongs to > (opp_table->required_opp_tables[n]). > > The genpd core holds the information about the allocated virtual > devices that it creates when it attached devA to its power-domains. > The virtual device(s) gets a genpd attached to it and that genpd also > has an OPP table associated with it (genpd->opp_table). > > By asking the OPP core to walk through the array of allocated > required-opps for devA and to match it against a *one* of the virtual > devices' genpd->opp_table, we can figure out at what index we should > assign the virtual device to in the opp_table->required_devs[index]. How do we differentiate between two cases where the required-opps can be defined as either of these: required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order) OR required-opps = <&opp_pd_51, &opp_pd_50>; //corresponds to pd_perf0 and pd_perf1 I thought this can't be fixed without some platform code telling how the DT is really configured, i.e. order of the power domains in the required-opps.
On Fri, 6 Sept 2024 at 08:14, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 04-09-24, 14:57, Ulf Hansson wrote: > > > Yeah, I missed that, it doesn't happen via DT but by platform code. I > > > do see problems where situation would be a bit ambiguous. Your example > > > with a minor change to your code: > > > > > > opp_table_devA: opp-table-devA { > > > compatible = "operating-points-v2"; > > > > > > opp-devA-50 { > > > opp-hz = /bits/ 64 <2500>; > > > required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order) > > > }; > > > .... > > > > > > devA { > > > compatible = "foo,bar"; > > > power-domains = <&pd_perf0>, <&pd_perf1>; //both > > > pd_perf0 and pd_perf1 has OPP tables. > > > power-domain-names = "perf0", "perf1"; > > > operating-points-v2 = <&opp_table_devA>; > > > }; > > > > > > Here, I don't think there is a way for us to know which genpd does > > > opp_pd_50 belongs to and to which one opp_pd_51 does. > > > > > > We solve this by sending clock_names and regulator_names in OPP > > > config structure. That gives the ordering in which required_opps are > > > present. The same needs to be done for genpd, and then genpd core > > > would be able to attach the right genpd with right required opp. > > > > No, we don't need this for gend as $subject patch is addressing this > > problem too. Let me elaborate. > > > > The OPP core holds the information about the devA's required-opps and > > to what OPP table each required-opps belongs to > > (opp_table->required_opp_tables[n]). > > > > The genpd core holds the information about the allocated virtual > > devices that it creates when it attached devA to its power-domains. > > The virtual device(s) gets a genpd attached to it and that genpd also > > has an OPP table associated with it (genpd->opp_table). > > > > By asking the OPP core to walk through the array of allocated > > required-opps for devA and to match it against a *one* of the virtual > > devices' genpd->opp_table, we can figure out at what index we should > > assign the virtual device to in the opp_table->required_devs[index]. > > How do we differentiate between two cases where the required-opps can > be defined as either of these: > > required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order) > > OR > > required-opps = <&opp_pd_51, &opp_pd_50>; //corresponds to pd_perf0 and pd_perf1 > > I thought this can't be fixed without some platform code telling how > the DT is really configured, i.e. order of the power domains in the > required-opps. I don't think we need platform code for this. When registering a genpd provider, an OPP table gets assigned to it. When hooking up a device to one of its genpd providers, that virtual device then also gets a handle to its genpd's OPP table. Each of the phandles in the required-opps points to another OPP table, which OPP table should be associated with a specific genpd. In other words, the information is there, we should not need anything additional in DT. Kind regards Uffe
FYI, I am on holidays now :) On Fri, 6 Sept 2024 at 14:19, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > How do we differentiate between two cases where the required-opps can > > be defined as either of these: > > > > required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order) > > > > OR > > > > required-opps = <&opp_pd_51, &opp_pd_50>; //corresponds to pd_perf0 and pd_perf1 > > > > I thought this can't be fixed without some platform code telling how > > the DT is really configured, i.e. order of the power domains in the > > required-opps. > > I don't think we need platform code for this. > > When registering a genpd provider, an OPP table gets assigned to it. So we will create a real OPP table in code, which will point to the common OPP table in DT. Fine. > When hooking up a device to one of its genpd providers, that virtual > device then also gets a handle to its genpd's OPP table. Right. If there are two genpds required for a device from the same genpd provider, the picture isn't very clear at this point. i.e. which required OPP belongs to which genpd, as both have same table in DT. > Each of the phandles in the required-opps points to another OPP table, > which OPP table should be associated with a specific genpd. Yes, but a simple order reversal in DT (which I sent in my last email), will not be picked by code at all. i.e. DT doesn't give the order in which required OPPs are present. > In other words, the information is there, we should not need anything > additional in DT.
On Wed, 11 Sept 2024 at 16:15, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Wed, 11 Sept 2024 at 08:03, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > FYI, I am on holidays now :) > > Oh, nice! Enjoy! > > > > > On Fri, 6 Sept 2024 at 14:19, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > How do we differentiate between two cases where the required-opps can > > > > be defined as either of these: > > > > > > > > required-opps = <&opp_pd_50, &opp_pd_51>; //corresponds to pd_perf1 and pd_perf0 (in reverse order) > > > > > > > > OR > > > > > > > > required-opps = <&opp_pd_51, &opp_pd_50>; //corresponds to pd_perf0 and pd_perf1 > > > > > > > > I thought this can't be fixed without some platform code telling how > > > > the DT is really configured, i.e. order of the power domains in the > > > > required-opps. > > > > > > I don't think we need platform code for this. > > > > > > When registering a genpd provider, an OPP table gets assigned to it. > > > > So we will create a real OPP table in code, which will point to the common > > OPP table in DT. Fine. > > > > > When hooking up a device to one of its genpd providers, that virtual > > > device then also gets a handle to its genpd's OPP table. > > > > Right. > > > > If there are two genpds required for a device from the same genpd provider, the > > picture isn't very clear at this point. i.e. which required OPP > > belongs to which genpd, > > as both have same table in DT. > > I agree that it's not very clear. > > But to me, this seems like an orthogonal problem that really should > not be managed by platform specific code in consumer drivers. > Moreover, unless I am mistaken, I believe this isn't really a problem > for the currently supported use cases we have for required-opps. Or is > it? Answering my own question. After some further investigation, I am afraid that your concern was correct. One sm8250, venus is using three power-domains,"venus", "vcodec0", "mx", but there is only one phandle in the required-opp. "venus" and "vcodec0" correspond to the "videocc" power-domain, which has a parent-domain that is the "rpmhpd". "mx" corresponds to the "rpmhpd". The rpmhpd power-domain has one shared OPP table used for all the power-domains it provides. :-( Because we also need to look for a matching OPP table for the required-opp by walking the power-domain parents (needed on Tegra), we simply can't tell what power-domain the required-opp belongs to. > > That said, we already have two methods that helps us to deal with this issue: > > 1) > For a genpd OF provider that provides multiple genpds, the genpd/OPP > core tries to assign an OPP table for each genpd, based on the > power-domain index. In other words, if corresponding OPP-tables are > specified in the operating-points-v2 list, those would get assigned > accordingly. > > 2) > The genpd OF provider can control on a per genpd basis, whether there > should be an OPP table assigned to it. This is managed by assigning > the ->set_performance_state() callback for the genpd or leaving it > unassigned. Typically this works well, when there is one OPP-table > specified in the operating-points-v2 list for the provider - and only > one of the genpds that should use it. > > If it turns out that we need something more flexible, I think we need > to look at extending the OPP/power-domain DT bindings. We would > probably need a "by-names" DT property, allowing us to specify the > mapping between the OPP-tables and the power-domains. > > > > > > Each of the phandles in the required-opps points to another OPP table, > > > which OPP table should be associated with a specific genpd. > > > > Yes, but a simple order reversal in DT (which I sent in my last > > email), will not be picked > > by code at all. i.e. DT doesn't give the order in which required OPPs > > are present. > > Assuming genpd OF providers are following 1) or 2), I don't think this > should be an issue. It looks like I was wrong. This whole problem boils down to that we are allowing the OPP table to be shared for a genpd OF provider for multiple power-domains. We could consider adding some new DT property to point out at what power-domain the required-opps belongs to, but it doesn't really matter as we need to keep supporting the current DTS. Oh well, back to the drawing table to re-work this again. It looks like we need to make it possible for consumer drivers to provide additional information to dev_pm_domain_attach_list(), allowing it to understand how the required-devs should be assigned. Unless you have some better ideas? [...] Kind regards Uffe
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
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@quicinc.com> Fixes: f0d2dcc9b087 ("OPP/pmdomain: Set the required_dev for a required OPP during genpd attach") Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/opp/core.c | 15 +++++++----- drivers/pmdomain/core.c | 52 +++++++++++++++++++++++------------------ include/linux/pm_opp.h | 7 ++++-- 3 files changed, 43 insertions(+), 31 deletions(-)