| Message ID | 20240619140849.368580-4-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | OPP/pmdomain: Assign required_devs for required OPPs through genpd | expand |
On 19-06-24, 16:08, Ulf Hansson wrote: > @@ -2494,36 +2495,68 @@ static int _opp_set_required_devs(struct opp_table *opp_table, > return -EINVAL; > } > > - /* Another device that shares the OPP table has set the required devs ? */ > - if (opp_table->required_devs[0]) > - return 0; > + /* Genpd core takes care of propagation to parent genpd */ > + if (opp_table->is_genpd) { A genpd can have non-genpd devices in the required OPPs and so this isn't sufficient. What we were ignoring earlier was genpd having another genpd as required opp. > + dev_err(dev, "%s: Operation not supported for genpds\n", __func__); > + return -EOPNOTSUPP; > + } > > for (i = 0; i < opp_table->required_opp_count; i++) { > - /* Genpd core takes care of propagation to parent genpd */ > - if (required_devs[i] && opp_table->is_genpd && > - opp_table->required_opp_tables[i]->is_genpd) { > - dev_err(dev, "%s: Operation not supported for genpds\n", __func__); > - return -EOPNOTSUPP; > - } > + struct opp_table *table = opp_table->required_opp_tables[i]; > + > + /* > + * The OPP table should be available at this point. If not, it's > + * not the one we are looking for. > + */ > + if (IS_ERR(table)) > + continue; > + > + /* Move to the next available index. */ > + if (opp_table->required_devs[i]) > + continue; > > - opp_table->required_devs[i] = required_devs[i]; > + /* > + * 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) { > + We don't keep such empty lines in OPP core generally at this place. > + /* Cross check the OPP tables and fix it if needed. */ Copy the bigger comment from_opp_attach_genpd() here too. It helps understanding why required_opp_tables entry is getting replaced. > + if (required_opp_table != table) { > + dev_pm_opp_put_opp_table(table); > + _get_opp_table_kref(required_opp_table); > + opp_table->required_opp_tables[i] = required_opp_table; > + } > + > + opp_table->required_devs[i] = required_dev; > + > + /* > + * Add the required_dev as a user of the OPP table, so > + * we can call dev_pm_opp_set_opp() on it directly. > + */ > + if (!_add_opp_dev(required_dev, required_opp_table)) { > + dev_err(dev, "Failed to add the device to the required OPP table\n"); > + return -ENOMEM; > + } > + > + return i; > + } > }
On Wed, 26 Jun 2024 at 08:33, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 19-06-24, 16:08, Ulf Hansson wrote: > > @@ -2494,36 +2495,68 @@ static int _opp_set_required_devs(struct opp_table *opp_table, > > return -EINVAL; > > } > > > > - /* Another device that shares the OPP table has set the required devs ? */ > > - if (opp_table->required_devs[0]) > > - return 0; > > + /* Genpd core takes care of propagation to parent genpd */ > > + if (opp_table->is_genpd) { > > A genpd can have non-genpd devices in the required OPPs and so this > isn't sufficient. What we were ignoring earlier was genpd having > another genpd as required opp. Unless I am mistaken, I don't think that is a scenario we should care about here. _opp_set_required_dev() is being called for a device that is about to be attached to its corresponding genpd. Yes, in some cases, we attach a genpd provider's device to its genpd-parent, but that is not to control the required-opps. > > > + dev_err(dev, "%s: Operation not supported for genpds\n", __func__); > > + return -EOPNOTSUPP; > > + } > > > > for (i = 0; i < opp_table->required_opp_count; i++) { > > - /* Genpd core takes care of propagation to parent genpd */ > > - if (required_devs[i] && opp_table->is_genpd && > > - opp_table->required_opp_tables[i]->is_genpd) { > > - dev_err(dev, "%s: Operation not supported for genpds\n", __func__); > > - return -EOPNOTSUPP; > > - } > > + struct opp_table *table = opp_table->required_opp_tables[i]; > > + > > + /* > > + * The OPP table should be available at this point. If not, it's > > + * not the one we are looking for. > > + */ > > + if (IS_ERR(table)) > > + continue; > > + > > + /* Move to the next available index. */ > > + if (opp_table->required_devs[i]) > > + continue; > > > > - opp_table->required_devs[i] = required_devs[i]; > > + /* > > + * 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) { > > + > > We don't keep such empty lines in OPP core generally at this place. Yep, let me drop it! > > > + /* Cross check the OPP tables and fix it if needed. */ > > Copy the bigger comment from_opp_attach_genpd() here too. It helps > understanding why required_opp_tables entry is getting replaced. Right, makes sense! > > > + if (required_opp_table != table) { > > + dev_pm_opp_put_opp_table(table); > > + _get_opp_table_kref(required_opp_table); > > + opp_table->required_opp_tables[i] = required_opp_table; > > + } > > + > > + opp_table->required_devs[i] = required_dev; > > + > > + /* > > + * Add the required_dev as a user of the OPP table, so > > + * we can call dev_pm_opp_set_opp() on it directly. > > + */ > > + if (!_add_opp_dev(required_dev, required_opp_table)) { > > + dev_err(dev, "Failed to add the device to the required OPP table\n"); > > + return -ENOMEM; > > + } > > + > > + return i; > > + } > > } > > -- > viresh Kind regards Uffe
On 11-07-24, 12:19, Ulf Hansson wrote: > Unless I am mistaken, I don't think that is a scenario we should care > about here. > > _opp_set_required_dev() is being called for a device that is about to > be attached to its corresponding genpd. > > Yes, in some cases, we attach a genpd provider's device to its > genpd-parent, but that is not to control the required-opps. I understand and I am okay with what you are suggesting, we can fix it later on if required anyway. But just to give my reasoning behind that is that we want to avoid a very specific case here and allow everything else. The special case being genpd propagation, so I would normally not do a blanket ban but just that case. But as I said, its okay :)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 02ba963d11ff..bc1ed1d3d60d 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -2483,9 +2483,10 @@ static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev, } -static int _opp_set_required_devs(struct opp_table *opp_table, - struct device *dev, - struct device **required_devs) +static int _opp_set_required_dev(struct opp_table *opp_table, + struct device *dev, + struct device *required_dev, + struct opp_table *required_opp_table) { int i; @@ -2494,36 +2495,68 @@ static int _opp_set_required_devs(struct opp_table *opp_table, return -EINVAL; } - /* Another device that shares the OPP table has set the required devs ? */ - if (opp_table->required_devs[0]) - return 0; + /* Genpd core takes care of propagation to parent genpd */ + if (opp_table->is_genpd) { + dev_err(dev, "%s: Operation not supported for genpds\n", __func__); + return -EOPNOTSUPP; + } for (i = 0; i < opp_table->required_opp_count; i++) { - /* Genpd core takes care of propagation to parent genpd */ - if (required_devs[i] && opp_table->is_genpd && - opp_table->required_opp_tables[i]->is_genpd) { - dev_err(dev, "%s: Operation not supported for genpds\n", __func__); - return -EOPNOTSUPP; - } + struct opp_table *table = opp_table->required_opp_tables[i]; + + /* + * The OPP table should be available at this point. If not, it's + * not the one we are looking for. + */ + if (IS_ERR(table)) + continue; + + /* Move to the next available index. */ + if (opp_table->required_devs[i]) + continue; - opp_table->required_devs[i] = required_devs[i]; + /* + * 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) { + + /* Cross check the OPP tables and fix it if needed. */ + if (required_opp_table != table) { + dev_pm_opp_put_opp_table(table); + _get_opp_table_kref(required_opp_table); + opp_table->required_opp_tables[i] = required_opp_table; + } + + opp_table->required_devs[i] = required_dev; + + /* + * Add the required_dev as a user of the OPP table, so + * we can call dev_pm_opp_set_opp() on it directly. + */ + if (!_add_opp_dev(required_dev, required_opp_table)) { + dev_err(dev, "Failed to add the device to the required OPP table\n"); + return -ENOMEM; + } + + return i; + } } - return 0; + dev_err(dev, "Missing OPP table, unable to set the required dev\n"); + return -ENODEV; } -static void _opp_put_required_devs(struct opp_table *opp_table) +static void _opp_put_required_dev(struct opp_table *opp_table, + unsigned int index) { - int i; - - for (i = 0; i < opp_table->required_opp_count; i++) - opp_table->required_devs[i] = NULL; + opp_table->required_devs[index] = NULL; } static void _opp_clear_config(struct opp_config_data *data) { - if (data->flags & OPP_CONFIG_REQUIRED_DEVS) - _opp_put_required_devs(data->opp_table); + if (data->flags & OPP_CONFIG_REQUIRED_DEV) + _opp_put_required_dev(data->opp_table, data->index); else if (data->flags & OPP_CONFIG_GENPD) _opp_detach_genpd(data->opp_table); @@ -2640,7 +2673,7 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config) /* Attach genpds */ if (config->genpd_names) { - if (config->required_devs) + if (config->required_dev) goto err; ret = _opp_attach_genpd(opp_table, dev, config->genpd_names, @@ -2649,13 +2682,15 @@ int dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config) goto err; data->flags |= OPP_CONFIG_GENPD; - } else if (config->required_devs) { - ret = _opp_set_required_devs(opp_table, dev, - config->required_devs); - if (ret) + } else if (config->required_dev && config->required_opp_table) { + ret = _opp_set_required_dev(opp_table, dev, + config->required_dev, + config->required_opp_table); + if (ret < 0) goto err; - data->flags |= OPP_CONFIG_REQUIRED_DEVS; + data->index = ret; + data->flags |= OPP_CONFIG_REQUIRED_DEV; } ret = xa_alloc(&opp_configs, &id, data, XA_LIMIT(1, INT_MAX), diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h index cff1fabd1ae3..5b5a4bd89c9e 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -35,12 +35,13 @@ extern struct list_head opp_tables; #define OPP_CONFIG_PROP_NAME BIT(3) #define OPP_CONFIG_SUPPORTED_HW BIT(4) #define OPP_CONFIG_GENPD BIT(5) -#define OPP_CONFIG_REQUIRED_DEVS BIT(6) +#define OPP_CONFIG_REQUIRED_DEV BIT(6) /** * struct opp_config_data - data for set config operations * @opp_table: OPP table * @flags: OPP config flags + * @index: The position in the array of required_devs * * This structure stores the OPP config information for each OPP table * configuration by the callers. @@ -48,6 +49,7 @@ extern struct list_head opp_tables; struct opp_config_data { struct opp_table *opp_table; unsigned int flags; + unsigned int index; }; /** diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index dd7c8441af42..2b6599f6037d 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -63,10 +63,11 @@ 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. * @genpd_names: Null terminated array of pointers containing names of genpd to - * attach. Mutually exclusive with required_devs. + * attach. Mutually exclusive with required_dev. * @virt_devs: Pointer to return the array of genpd virtual devices. Mutually - * exclusive with required_devs. - * @required_devs: Required OPP devices. Mutually exclusive with genpd_names/virt_devs. + * exclusive with required_dev. + * @required_dev: Required OPP device. Mutually exclusive with genpd_names/virt_devs. + * @required_opp_table: The corresponding required OPP table for @required_dev. * * This structure contains platform specific OPP configurations for the device. */ @@ -81,7 +82,8 @@ struct dev_pm_opp_config { const char * const *regulator_names; const char * const *genpd_names; struct device ***virt_devs; - struct device **required_devs; + struct device *required_dev; + struct opp_table *required_opp_table; }; #define OPP_LEVEL_UNSET U32_MAX
At this point there are no consumer drivers that makes use of _set_required_devs(), hence it should be straightforward to rework the code to enable it to better integrate with the genpd attach procedure. During genpd attach, one device is being attached to its PM domain. Therefore, let's also update the _set_required_devs() to work with this behaviour and instead trust callers to fill out one required_dev per call. Moving forward and as shown from a subsequent change, genpd becomes the first user of the reworked _set_required_dev(). Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/opp/core.c | 89 +++++++++++++++++++++++++++++------------- drivers/opp/opp.h | 4 +- include/linux/pm_opp.h | 10 +++-- 3 files changed, 71 insertions(+), 32 deletions(-)