| Message ID | 20210118005524.27787-5-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | OPP API fixes and improvements | expand |
On 18-01-21, 03:55, Dmitry Osipenko wrote: > Extend OPP API with dev_pm_opp_sync_regulators() function, which syncs > voltage state of regulators. > > Tested-by: Peter Geis <pgwipeout@gmail.com> > Tested-by: Nicolas Chauvet <kwizart@gmail.com> > Tested-by: Matt Merhar <mattmerhar@protonmail.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/opp/core.c | 45 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/pm_opp.h | 6 ++++++ > 2 files changed, 51 insertions(+) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 7b4d07279638..99d18befc209 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -2686,3 +2686,48 @@ void dev_pm_opp_remove_table(struct device *dev) > dev_pm_opp_put_opp_table(opp_table); > } > EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table); > + > +/** > + * dev_pm_opp_sync_regulators() - Sync state of voltage regulators > + * @dev: device for which we do this operation > + * > + * Sync voltage state of the OPP table regulators. > + * > + * Return: 0 on success or a negative error value. > + */ > +int dev_pm_opp_sync_regulators(struct device *dev) > +{ > + struct opp_table *opp_table; > + struct regulator *reg; > + int i, ret = 0; > + > + /* Device may not have OPP table */ > + opp_table = _find_opp_table(dev); > + if (IS_ERR(opp_table)) > + return 0; > + > + /* Regulator may not be required for the device */ > + if (!opp_table->regulators) > + goto put_table; > + > + mutex_lock(&opp_table->lock); What exactly do you need this lock for ? > + > + /* Nothing to sync if voltage wasn't changed */ > + if (!opp_table->enabled) > + goto unlock; > + > + for (i = 0; i < opp_table->regulator_count; i++) { > + reg = opp_table->regulators[i]; > + ret = regulator_sync_voltage(reg); > + if (ret) > + break; > + } > +unlock: > + mutex_unlock(&opp_table->lock); > +put_table: > + /* Drop reference taken by _find_opp_table() */ > + dev_pm_opp_put_opp_table(opp_table); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators); > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index c24bd34339d7..1c3a09cc8dcd 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cp > int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask); > void dev_pm_opp_remove_table(struct device *dev); > void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask); > +int dev_pm_opp_sync_regulators(struct device *dev); > #else > static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev) > { > @@ -398,6 +399,11 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask > { > } > > +static inline int dev_pm_opp_sync_regulators(struct device *dev) > +{ > + return -ENOTSUPP; > +} > + > #endif /* CONFIG_PM_OPP */ > > #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) > -- > 2.29.2
On 18-01-21, 03:55, Dmitry Osipenko wrote: > Extend OPP API with dev_pm_opp_sync_regulators() function, which syncs > voltage state of regulators. > > Tested-by: Peter Geis <pgwipeout@gmail.com> > Tested-by: Nicolas Chauvet <kwizart@gmail.com> > Tested-by: Matt Merhar <mattmerhar@protonmail.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/opp/core.c | 45 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/pm_opp.h | 6 ++++++ > 2 files changed, 51 insertions(+) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 7b4d07279638..99d18befc209 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -2686,3 +2686,48 @@ void dev_pm_opp_remove_table(struct device *dev) > dev_pm_opp_put_opp_table(opp_table); > } > EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table); > + > +/** > + * dev_pm_opp_sync_regulators() - Sync state of voltage regulators > + * @dev: device for which we do this operation > + * > + * Sync voltage state of the OPP table regulators. > + * > + * Return: 0 on success or a negative error value. > + */ > +int dev_pm_opp_sync_regulators(struct device *dev) > +{ > + struct opp_table *opp_table; > + struct regulator *reg; > + int i, ret = 0; > + > + /* Device may not have OPP table */ > + opp_table = _find_opp_table(dev); > + if (IS_ERR(opp_table)) > + return 0; > + > + /* Regulator may not be required for the device */ > + if (!opp_table->regulators) > + goto put_table; > + > + mutex_lock(&opp_table->lock); > + > + /* Nothing to sync if voltage wasn't changed */ > + if (!opp_table->enabled) > + goto unlock; > + > + for (i = 0; i < opp_table->regulator_count; i++) { > + reg = opp_table->regulators[i]; > + ret = regulator_sync_voltage(reg); > + if (ret) > + break; > + } > +unlock: > + mutex_unlock(&opp_table->lock); > +put_table: > + /* Drop reference taken by _find_opp_table() */ > + dev_pm_opp_put_opp_table(opp_table); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators); > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index c24bd34339d7..1c3a09cc8dcd 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cp > int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask); > void dev_pm_opp_remove_table(struct device *dev); > void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask); > +int dev_pm_opp_sync_regulators(struct device *dev); > #else > static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev) > { > @@ -398,6 +399,11 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask > { > } > > +static inline int dev_pm_opp_sync_regulators(struct device *dev) > +{ > + return -ENOTSUPP; > +} > + > #endif /* CONFIG_PM_OPP */ > > #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) Applied. Thanks. I had to apply it manually, please make sure it looks okay.
On 18-01-21, 16:30, Viresh Kumar wrote: > On 18-01-21, 03:55, Dmitry Osipenko wrote: > > Extend OPP API with dev_pm_opp_sync_regulators() function, which syncs > > voltage state of regulators. > > > > Tested-by: Peter Geis <pgwipeout@gmail.com> > > Tested-by: Nicolas Chauvet <kwizart@gmail.com> > > Tested-by: Matt Merhar <mattmerhar@protonmail.com> > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > --- > > drivers/opp/core.c | 45 ++++++++++++++++++++++++++++++++++++++++++ > > include/linux/pm_opp.h | 6 ++++++ > > 2 files changed, 51 insertions(+) > > > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > > index 7b4d07279638..99d18befc209 100644 > > --- a/drivers/opp/core.c > > +++ b/drivers/opp/core.c > > @@ -2686,3 +2686,48 @@ void dev_pm_opp_remove_table(struct device *dev) > > dev_pm_opp_put_opp_table(opp_table); > > } > > EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table); > > + > > +/** > > + * dev_pm_opp_sync_regulators() - Sync state of voltage regulators > > + * @dev: device for which we do this operation > > + * > > + * Sync voltage state of the OPP table regulators. > > + * > > + * Return: 0 on success or a negative error value. > > + */ > > +int dev_pm_opp_sync_regulators(struct device *dev) > > +{ > > + struct opp_table *opp_table; > > + struct regulator *reg; > > + int i, ret = 0; > > + > > + /* Device may not have OPP table */ > > + opp_table = _find_opp_table(dev); > > + if (IS_ERR(opp_table)) > > + return 0; > > + > > + /* Regulator may not be required for the device */ > > + if (!opp_table->regulators) > > + goto put_table; > > + > > + mutex_lock(&opp_table->lock); > > + > > + /* Nothing to sync if voltage wasn't changed */ > > + if (!opp_table->enabled) > > + goto unlock; > > + > > + for (i = 0; i < opp_table->regulator_count; i++) { > > + reg = opp_table->regulators[i]; > > + ret = regulator_sync_voltage(reg); > > + if (ret) > > + break; > > + } > > +unlock: > > + mutex_unlock(&opp_table->lock); > > +put_table: > > + /* Drop reference taken by _find_opp_table() */ > > + dev_pm_opp_put_opp_table(opp_table); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators); > > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > > index c24bd34339d7..1c3a09cc8dcd 100644 > > --- a/include/linux/pm_opp.h > > +++ b/include/linux/pm_opp.h > > @@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cp > > int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask); > > void dev_pm_opp_remove_table(struct device *dev); > > void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask); > > +int dev_pm_opp_sync_regulators(struct device *dev); > > #else > > static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev) > > { > > @@ -398,6 +399,11 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask > > { > > } > > > > +static inline int dev_pm_opp_sync_regulators(struct device *dev) > > +{ > > + return -ENOTSUPP; > > +} > > + > > #endif /* CONFIG_PM_OPP */ > > > > #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) > > Applied. Thanks. > > I had to apply it manually, please make sure it looks okay. Sorry about this, I wanted to reply to "opp: Add devm_pm_opp_register_set_opp_helper" and replied to this one accidentally.
18.01.2021 11:20, Viresh Kumar пишет: >> +int dev_pm_opp_sync_regulators(struct device *dev) >> +{ >> + struct opp_table *opp_table; >> + struct regulator *reg; >> + int i, ret = 0; >> + >> + /* Device may not have OPP table */ >> + opp_table = _find_opp_table(dev); >> + if (IS_ERR(opp_table)) >> + return 0; >> + >> + /* Regulator may not be required for the device */ >> + if (!opp_table->regulators) >> + goto put_table; >> + >> + mutex_lock(&opp_table->lock); > What exactly do you need this lock for ? It is needed for protecting simultaneous invocations of dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage(). The sync_regulators() should be invoked only after completion of the set_voltage() in a case of Tegra power domain driver since potentially both could be running in parallel. For example device driver may be changing performance state in a work thread, while PM domain state is syncing. But maybe it will be better to move the protection to the PM driver since we're focused on sync_regulators() and set_voltage() here, but there are other OPP API functions which use regulators. I'll need to take a closer look at it.
On 18-01-21, 21:35, Dmitry Osipenko wrote: > 18.01.2021 11:20, Viresh Kumar пишет: > >> +int dev_pm_opp_sync_regulators(struct device *dev) > >> +{ > >> + struct opp_table *opp_table; > >> + struct regulator *reg; > >> + int i, ret = 0; > >> + > >> + /* Device may not have OPP table */ > >> + opp_table = _find_opp_table(dev); > >> + if (IS_ERR(opp_table)) > >> + return 0; > >> + > >> + /* Regulator may not be required for the device */ > >> + if (!opp_table->regulators) > >> + goto put_table; > >> + > >> + mutex_lock(&opp_table->lock); > > What exactly do you need this lock for ? > > It is needed for protecting simultaneous invocations of > dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage(). > > The sync_regulators() should be invoked only after completion of the > set_voltage() in a case of Tegra power domain driver since potentially > both could be running in parallel. For example device driver may be > changing performance state in a work thread, while PM domain state is > syncing. I think just checking the 'enabled' flag should be enough here (you may need a lock for it though, but the lock should cover only the area it is supposed to cover and nothing else. > But maybe it will be better to move the protection to the PM driver > since we're focused on sync_regulators() and set_voltage() here, but > there are other OPP API functions which use regulators. I'll need to > take a closer look at it.
19.01.2021 07:58, Viresh Kumar пишет: > On 18-01-21, 21:35, Dmitry Osipenko wrote: >> 18.01.2021 11:20, Viresh Kumar пишет: >>>> +int dev_pm_opp_sync_regulators(struct device *dev) >>>> +{ >>>> + struct opp_table *opp_table; >>>> + struct regulator *reg; >>>> + int i, ret = 0; >>>> + >>>> + /* Device may not have OPP table */ >>>> + opp_table = _find_opp_table(dev); >>>> + if (IS_ERR(opp_table)) >>>> + return 0; >>>> + >>>> + /* Regulator may not be required for the device */ >>>> + if (!opp_table->regulators) >>>> + goto put_table; >>>> + >>>> + mutex_lock(&opp_table->lock); >>> What exactly do you need this lock for ? >> >> It is needed for protecting simultaneous invocations of >> dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage(). >> >> The sync_regulators() should be invoked only after completion of the >> set_voltage() in a case of Tegra power domain driver since potentially >> both could be running in parallel. For example device driver may be >> changing performance state in a work thread, while PM domain state is >> syncing. > > I think just checking the 'enabled' flag should be enough here (you may need a > lock for it though, but the lock should cover only the area it is supposed to > cover and nothing else. I'll remove the locks from these OPP patches and move them to the PD driver. It should be the best option right now since OPP API isn't entirely thread-safe, making it thread-safe should be a separate topic.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 7b4d07279638..99d18befc209 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -2686,3 +2686,48 @@ void dev_pm_opp_remove_table(struct device *dev) dev_pm_opp_put_opp_table(opp_table); } EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table); + +/** + * dev_pm_opp_sync_regulators() - Sync state of voltage regulators + * @dev: device for which we do this operation + * + * Sync voltage state of the OPP table regulators. + * + * Return: 0 on success or a negative error value. + */ +int dev_pm_opp_sync_regulators(struct device *dev) +{ + struct opp_table *opp_table; + struct regulator *reg; + int i, ret = 0; + + /* Device may not have OPP table */ + opp_table = _find_opp_table(dev); + if (IS_ERR(opp_table)) + return 0; + + /* Regulator may not be required for the device */ + if (!opp_table->regulators) + goto put_table; + + mutex_lock(&opp_table->lock); + + /* Nothing to sync if voltage wasn't changed */ + if (!opp_table->enabled) + goto unlock; + + for (i = 0; i < opp_table->regulator_count; i++) { + reg = opp_table->regulators[i]; + ret = regulator_sync_voltage(reg); + if (ret) + break; + } +unlock: + mutex_unlock(&opp_table->lock); +put_table: + /* Drop reference taken by _find_opp_table() */ + dev_pm_opp_put_opp_table(opp_table); + + return ret; +} +EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators); diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index c24bd34339d7..1c3a09cc8dcd 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cp int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask); void dev_pm_opp_remove_table(struct device *dev); void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask); +int dev_pm_opp_sync_regulators(struct device *dev); #else static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev) { @@ -398,6 +399,11 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask { } +static inline int dev_pm_opp_sync_regulators(struct device *dev) +{ + return -ENOTSUPP; +} + #endif /* CONFIG_PM_OPP */ #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)