OPP API fixes and improvements
v3 cover OPP API fixes and improvements
Hi,
This series fixes problems and adds features to OPP API that are required for implementation of a power domain driver for NVIDIA Tegra SoCs.
It is a continuation of [1], where Viresh Kumar asked to factor OPP patches into a separate series. I factored out the patches into this series, addressed the previous review comments and re-based patches on top of [2], which replaced some of my patches that added resource-managed helpers.
[1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=221130
[2] https://lore.kernel.org/linux-pm/[email protected]/
Changelog:
v3: - Reordered patches by importance.
- Added locking to dev_pm_opp_set_voltage().
- Reworked "Fix adding OPP entries in a wrong order if rate is unavailable" patch, like it was suggested by Viresh Kumar.
- Reworked "Support set_opp() customization without requiring to use regulators" patch, like it was suggested by Viresh Kumar.
The opp_table->set_opp_data is now allocated by dev_pm_opp_register_set_opp_helper().
The set_opp_data is refcounted now and can be allocated by any other OPP functions if this will become needed in the future for other OPP API changes.
Dmitry Osipenko (12):
opp: Fix adding OPP entries in a wrong order if rate is unavailable
opp: Filter out OPPs based on availability of a required-OPP
opp: Correct debug message in _opp_add_static_v2()
opp: Add dev_pm_opp_sync_regulators()
opp: Add dev_pm_opp_set_voltage()
opp: Add dev_pm_opp_find_level_ceil()
opp: Add dev_pm_opp_get_required_pstate()
opp: Add devm_pm_opp_register_set_opp_helper
opp: Add devm_pm_opp_attach_genpd
opp: Support set_opp() customization without requiring to use
regulators
opp: Handle missing OPP table in dev_pm_opp_xlate_performance_state()
opp: Print OPP level in debug message of _opp_add_static_v2()
drivers/opp/core.c | 309 +++++++++++++++++++++++++++++++++++++++-- drivers/opp/of.c | 9 +- include/linux/pm_opp.h | 49 +++++++ 3 files changed, 349 insertions(+), 18 deletions(-)
Signature · 1 line
2.29.2
PATCH v3 1/12 opp: Fix adding OPP entries in a wrong order if rate is unavailable
Fix adding OPP entries in a wrong (opposite) order if OPP rate is unavailable. The OPP comparison was erroneously skipped, thus OPPs were left unsorted.
--- drivers/opp/core.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/opp/core.c +4 -6
@@ -1527,12 +1527,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,mutex_lock (&opp_table->lock); head = &opp_table->opp_list; -if (likely (!rate_not_available)) { - ret =_opp_is_duplicate (dev, new_opp, opp_table, &head); -if (ret) { -mutex_unlock (&opp_table->lock); -return ret; - } + ret =_opp_is_duplicate (dev, new_opp, opp_table, &head); +if (ret) { +mutex_unlock (&opp_table->lock); +return ret; }list_add (&new_opp->node, head);
Signature · 1 line
2.29.2
Viresh Kumar
AppliedRe: [PATCH v3 01/12] opp: Fix adding OPP entries in a wrong order if rate is unavailable
Quoted 23 lines · Dmitry Osipenko
Fix adding OPP entries in a wrong (opposite) order if OPP rate is unavailable. The OPP comparison was erroneously skipped, thus OPPs were left unsorted.
Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)drivers/opp/core.c +0
-3@@ -1527,12 +1527,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,mutex_lock (&opp_table->lock); head = &opp_table->opp_list; -if (likely (!rate_not_available)) { - ret =_opp_is_duplicate (dev, new_opp, opp_table, &head); -if (ret) {
drivers/opp/core.c int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, · line 1536
Quoted 10 lines
drivers/opp/core.c +4
-3-mutex_unlock (&opp_table->lock); -return ret; - } + ret =_opp_is_duplicate (dev, new_opp, opp_table, &head); +if (ret) { +mutex_unlock (&opp_table->lock); +return ret; }list_add (&new_opp->node, head);
Applied. Thanks.
I am not sending it for 5.11-rc as there shouldn't be any users which are impacted because of this right now, right ?
Signature · 1 line
viresh
Re: [PATCH v3 01/12] opp: Fix adding OPP entries in a wrong order if rate is unavailable
18.01.2021 10:44, Viresh Kumar пишет:
Quoted 33 lines · Dmitry Osipenko
Quoted 32 lines
Fix adding OPP entries in a wrong (opposite) order if OPP rate is unavailable. The OPP comparison was erroneously skipped, thus OPPs were left unsorted.
Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)drivers/opp/core.c +4
-6@@ -1527,12 +1527,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,mutex_lock (&opp_table->lock); head = &opp_table->opp_list; -if (likely (!rate_not_available)) { - ret =_opp_is_duplicate (dev, new_opp, opp_table, &head); -if (ret) { -mutex_unlock (&opp_table->lock); -return ret; - } + ret =_opp_is_duplicate (dev, new_opp, opp_table, &head); +if (ret) { +mutex_unlock (&opp_table->lock); +return ret; }
Quoted 7 lines
drivers/opp/core.c int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, · line 1536
Quoted 1 line
drivers/opp/core.c +0
-0list_add (&new_opp->node, head);Applied. Thanks.
I am not sending it for 5.11-rc as there shouldn't be any users which are impacted because of this right now, right ?
right
PATCH v3 2/12 opp: Filter out OPPs based on availability of a required-OPP
In reply to Dmitry OsipenkoA required OPP may not be available, and thus, all OPPs which are using this required OPP should be unavailable too.
--- drivers/opp/core.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
drivers/opp/core.c +11 -0
@@ -1522,6 +1522,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available) {struct list_head *head; +unsigned int i;int ret;mutex_lock (&opp_table->lock);
@@ -1547,6 +1548,16 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, __func__, new_opp->rate); } +for (i =0 ; i < opp_table->required_opp_count; i++) { +if (new_opp->required_opps[i]->available) +continue ; + + new_opp->available = false; +dev_warn (dev,"%s: OPP not supported by required OPP %pOF (%lu)\n" , + __func__, new_opp->required_opps[i]->np, new_opp->rate); +break ; + } +return 0 ; }
Signature · 1 line
2.29.2
Viresh Kumar
AppliedRe: [PATCH v3 02/12] opp: Filter out OPPs based on availability of a required-OPP
Quoted 29 lines · Dmitry Osipenko
A required OPP may not be available, and thus, all OPPs which are using this required OPP should be unavailable too.
Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 11 +++++++++++ 1 file changed, 11 insertions(+)drivers/opp/core.c +3
-0@@ -1522,6 +1522,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available) {struct list_head *head; +unsigned int i;int ret;mutex_lock (&opp_table->lock);@@ -1547,6 +1548,16 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, __func__, new_opp->rate); } +for (i =0 ; i < opp_table->required_opp_count; i++) { +if (new_opp->required_opps[i]->available)
drivers/opp/core.c int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, · line 1562
Quoted 10 lines
drivers/opp/core.c +8
-0+continue ; + + new_opp->available = false; +dev_warn (dev,"%s: OPP not supported by required OPP %pOF (%lu)\n" , + __func__, new_opp->required_opps[i]->np, new_opp->rate); +break ; + } +return 0 ; }
Applied. Thanks.
Though I am concerned about who will enable this back again if the required-opp comes back. And I am not sure if we should even care about that.
Signature · 1 line
viresh
PATCH v3 3/12 opp: Correct debug message in _opp_add_static_v2()
In reply to Dmitry OsipenkoThe debug message always prints rate=0 instead of a proper value, fix it.
--- drivers/opp/of.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/opp/of.c +2 -2
@@ -755,7 +755,6 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table, struct device *dev, struct device_node *np) { struct dev_pm_opp *new_opp; -u64 rate =0 ;u32 val;int ret;bool rate_not_available = false;
@@ -772,7 +771,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,/* Check if the OPP supports hardware's hierarchy of versions or not */ if (!_opp_is_supported (dev, opp_table, np)) { -dev_dbg (dev,"OPP not supported by hardware: %llu\n" , rate); +dev_dbg (dev,"OPP not supported by hardware: %lu\n" , + new_opp->rate); goto free_opp; }
Signature · 1 line
2.29.2
Re: [PATCH v3 03/12] opp: Correct debug message in _opp_add_static_v2()
Quoted 23 lines · Dmitry Osipenko
The debug message always prints rate=0 instead of a proper value, fix it.
Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/of.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/opp/of.c +0
-1@@ -755,7 +755,6 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table, struct device *dev, struct device_node *np) { struct dev_pm_opp *new_opp; -u64 rate =0 ;u32 val;int ret;bool rate_not_available = false;
drivers/opp/of.c static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table, · line 778
Quoted 9 lines
drivers/opp/of.c +2
-1@@ -772,7 +771,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,/* Check if the OPP supports hardware's hierarchy of versions or not */ if (!_opp_is_supported (dev, opp_table, np)) { -dev_dbg (dev,"OPP not supported by hardware: %llu\n" , rate); +dev_dbg (dev,"OPP not supported by hardware: %lu\n" , + new_opp->rate); goto free_opp; }
Applied and added Fixes tag. Thanks.
Signature · 1 line
viresh
PATCH v3 4/12 opp: Add dev_pm_opp_sync_regulators()
In reply to Dmitry OsipenkoExtend OPP API with dev_pm_opp_sync_regulators() function, which syncs voltage state of regulators.
--- drivers/opp/core.c | 45 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 ++++++ 2 files changed, 51 insertions(+)
drivers/opp/core.c +45 -0
@@ -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);
include/linux/pm_opp.h +6 -0
@@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpint 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 )
Signature · 1 line
2.29.2
kernel test robot
build botRe: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()
I love your patch! Yet something to improve:
Hi Dmitry,
I love your patch! Yet something to improve:
[auto build test ERROR on pm/linux-next]
[also build test ERROR on linux/master linus/master v5.11-rc4 next-20210115] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Dmitry-Osipenko/OPP-API-fixes-and-improvements/20210118-090646 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next config: x86_64-rhel (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/6bab64a141641bb0f56524763ef836d21bdfd73d git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Dmitry-Osipenko/OPP-API-fixes-and-improvements/20210118-090646 git checkout 6bab64a141641bb0f56524763ef836d21bdfd73d # save the attached .config to linux build tree make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
drivers/opp/core.c: In function 'dev_pm_opp_sync_regulators':
>> drivers/opp/core.c:2548:9: error: implicit declaration of function 'regulator_sync_voltage'; did you mean 'regulator_set_voltage'? [-Werror=implicit-function-declaration]
2548 | ret = regulator_sync_voltage(reg);
| ^~~~~~~~~~~~~~~~~~~~~~
| regulator_set_voltagecc1: some warnings being treated as errors
vim +2548 drivers/opp/core.c
2516
2517 /**
2518 * dev_pm_opp_sync_regulators() - Sync state of voltage regulators
2519 * @dev: device for which we do this operation
2520 *
2521 * Sync voltage state of the OPP table regulators.
2522 *
2523 * Return: 0 on success or a negative error value.
2524 */
2525 int dev_pm_opp_sync_regulators(struct device *dev)
2526 {
2527 struct opp_table *opp_table;
2528 struct regulator *reg;
2529 int i, ret = 0;
2530
2531 /* Device may not have OPP table */
2532 opp_table = _find_opp_table(dev);
2533 if (IS_ERR(opp_table))
2534 return 0;
2535
2536 /* Regulator may not be required for the device */
2537 if (!opp_table->regulators)
2538 goto put_table;
2539
2540 mutex_lock(&opp_table->lock);
2541
2542 /* Nothing to sync if voltage wasn't changed */
2543 if (!opp_table->enabled)
2544 goto unlock;
2545
2546 for (i = 0; i < opp_table->regulator_count; i++) {
2547 reg = opp_table->regulators[i];
> 2548 ret = regulator_sync_voltage(reg);
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()
In reply to Dmitry OsipenkoQuoted 35 lines · Dmitry Osipenko
Extend OPP API with
dev_pm_opp_sync_regulators()function, which syncs voltage state of regulators.Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 45 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 ++++++ 2 files changed, 51 insertions(+)drivers/opp/core.c +15
-0@@ -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 ; +
drivers/opp/core.c void dev_pm_opp_remove_table(struct device *dev) · line 2713
Quoted 10 lines
drivers/opp/core.c +10
-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 ?
Quoted 46 lines
drivers/opp/core.c +20
-0+ +/* 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);include/linux/pm_opp.h +6
-0@@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpint 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 )Signature · 1 line
2.29.2
Signature · 1 line
viresh
Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()
18.01.2021 11:20, Viresh Kumar пишет:
Quoted 9 lines
Quoted 9 lines
drivers/opp/core.c +9
-0+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))
Quoted 8 lines
drivers/opp/core.c void dev_pm_opp_remove_table(struct device *dev) · line 2713
Quoted 7 lines
drivers/opp/core.c +7
-0+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.
Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()
Quoted 19 lines · Dmitry Osipenko
18.01.2021 11:20, Viresh Kumar пишет:
Quoted 9 lines
Quoted 9 lines
drivers/opp/core.c +9
-0+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))Quoted 8 lines
drivers/opp/core.c void dev_pm_opp_remove_table(struct device *dev) · line 2713
Quoted 7 lines
drivers/opp/core.c +7
-0+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 ?
Quoted 8 lines
It is needed for protecting simultaneous invocations of
dev_pm_opp_sync_regulators()anddev_pm_opp_set_voltage().The
sync_regulators()should be invoked only after completion of theset_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.
Quoted 4 lines
But maybe it will be better to move the protection to the PM driver since we're focused on
sync_regulators()andset_voltage()here, but there are other OPP API functions which use regulators. I'll need to take a closer look at it.
Signature · 1 line
viresh
Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()
19.01.2021 07:58, Viresh Kumar пишет:
Quoted 29 lines · Dmitry Osipenko
Quoted 19 lines
18.01.2021 11:20, Viresh Kumar пишет:
Quoted 9 lines
Quoted 9 lines
drivers/opp/core.c +9
-0+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))Quoted 8 lines
drivers/opp/core.c void dev_pm_opp_remove_table(struct device *dev) · line 2713
Quoted 7 lines
drivers/opp/core.c +7
-0+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 ?
Quoted 8 lines
It is needed for protecting simultaneous invocations of
dev_pm_opp_sync_regulators()anddev_pm_opp_set_voltage().The
sync_regulators()should be invoked only after completion of theset_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.
Quoted 3 lines
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.
Viresh Kumar
AppliedRe: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()
In reply to Dmitry OsipenkoQuoted 79 lines · Dmitry Osipenko
Extend OPP API with
dev_pm_opp_sync_regulators()function, which syncs voltage state of regulators.Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 45 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 ++++++ 2 files changed, 51 insertions(+)drivers/opp/core.c +45
-0@@ -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);include/linux/pm_opp.h +1
-0@@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpint 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 {
include/linux/pm_opp.h static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask · line 409
Quoted 10 lines
include/linux/pm_opp.h +5
-0} +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.
Signature · 1 line
viresh
Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()
Quoted 89 lines · Viresh Kumar
Quoted 88 lines · Dmitry Osipenko
Extend OPP API with
dev_pm_opp_sync_regulators()function, which syncs voltage state of regulators.Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 45 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 ++++++ 2 files changed, 51 insertions(+)drivers/opp/core.c +45
-0@@ -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);include/linux/pm_opp.h +6
-0@@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpint 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 */
Quoted 5 lines
include/linux/pm_opp.h static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask · line 409
Quoted 1 line
include/linux/pm_opp.h +0
-0#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.
Signature · 1 line
viresh
PATCH v3 5/12 opp: Add dev_pm_opp_set_voltage()
In reply to Dmitry OsipenkoAdd dev_pm_opp_set_voltage() which allows OPP table users to set voltage in accordance to a given OPP. In particular this is needed for driving voltage of a generic power domain which uses OPPs and doesn't have a clock.
--- drivers/opp/core.c | 55 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 +++++ 2 files changed, 61 insertions(+)
drivers/opp/core.c +55 -0
@@ -2731,3 +2731,58 @@ int dev_pm_opp_sync_regulators(struct device *dev)return ret; }EXPORT_SYMBOL_GPL (dev_pm_opp_sync_regulators); + +/** +* dev_pm_opp_set_voltage() - Change voltage of regulators +* @dev: device for which we do this operation +* @opp: opp based on which the voltages are to be configured +* +* Change voltage of the OPP table regulators. +* +* Return: 0 on success or a negative error value. +*/ +int dev_pm_opp_set_voltage (struct device *dev,struct dev_pm_opp *opp) +{ +struct opp_table *opp_table; +struct regulator *reg; +int 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; + +/* This function only supports single regulator per device */ +if (WARN_ON (opp_table->regulator_count >1 )) { +dev_err (dev,"multiple regulators are not supported\n" ); + ret = -EINVAL ; + goto put_table; + } + +mutex_lock (&opp_table->lock); + + reg = opp_table->regulators[0 ]; + ret =_set_opp_voltage (dev, reg, opp->supplies); + +if (!opp_table->enabled) { + ret =regulator_enable (reg); +if (ret <0 ) { +dev_warn (dev,"Failed to enable regulator: %d" , ret); + goto unlock; + } + + opp_table->enabled = true; + } +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_set_voltage);
include/linux/pm_opp.h +6 -0
@@ -163,6 +163,7 @@ 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); +int dev_pm_opp_set_voltage (struct device *dev,struct dev_pm_opp *opp);#else static inline struct opp_table *dev_pm_opp_get_opp_table (struct device *dev) {
@@ -404,6 +405,11 @@ static inline int dev_pm_opp_sync_regulators(struct device *dev) return -ENOTSUPP; } +static inline int dev_pm_opp_set_voltage (struct device *dev,struct dev_pm_opp *opp) +{ +return -ENOTSUPP ; +} +#endif /* CONFIG_PM_OPP */ #if defined(CONFIG_PM_OPP ) && defined(CONFIG_OF )
Signature · 1 line
2.29.2
Re: [PATCH v3 05/12] opp: Add dev_pm_opp_set_voltage()
Quoted 9 lines · Dmitry Osipenko
drivers/opp/core.c +1
-0@@ -2731,3 +2731,58 @@ int dev_pm_opp_sync_regulators(struct device *dev)return ret; }EXPORT_SYMBOL_GPL (dev_pm_opp_sync_regulators); +
drivers/opp/core.c int dev_pm_opp_sync_regulators(struct device *dev) · line 2744
Quoted 10 lines
drivers/opp/core.c +10
-0+/** +* dev_pm_opp_set_voltage() - Change voltage of regulators +* @dev: device for which we do this operation +* @opp: opp based on which the voltages are to be configured +* +* Change voltage of the OPP table regulators. +* +* Return: 0 on success or a negative error value. +*/ +int dev_pm_opp_set_voltage (struct device *dev,struct dev_pm_opp *opp)
I think we should do better than this, will require some work from your part though (or I can do it if you want).
Basically what you wanted to do here is set the OPP for a device and this means do whatever is required for setting the OPP. It is normally frequency, which is not your case, but it is other things as well. Like setting multiple regulators, bandwidth, required-opps, etc.
I feel the right way of doing this would be to do this:
Factor out dev_pm_opp_set_opp() from dev_pm_opp_set_rate() and make the later call the former. And then we can just call dev_pm_opp_set_opp() from your usecase. This will make sure we have a single code path for all the set-opp stuff. What do you think ?
Signature · 1 line
viresh
Re: [PATCH v3 05/12] opp: Add dev_pm_opp_set_voltage()
18.01.2021 12:52, Viresh Kumar пишет:
Quoted 29 lines · Dmitry Osipenko
Quoted 9 lines
drivers/opp/core.c +1
-0@@ -2731,3 +2731,58 @@ int dev_pm_opp_sync_regulators(struct device *dev)return ret; }EXPORT_SYMBOL_GPL (dev_pm_opp_sync_regulators); +drivers/opp/core.c int dev_pm_opp_sync_regulators(struct device *dev) · line 2744
Quoted 10 lines
drivers/opp/core.c +10
-0+/** +* dev_pm_opp_set_voltage() - Change voltage of regulators +* @dev: device for which we do this operation +* @opp: opp based on which the voltages are to be configured +* +* Change voltage of the OPP table regulators. +* +* Return: 0 on success or a negative error value. +*/ +int dev_pm_opp_set_voltage (struct device *dev,struct dev_pm_opp *opp)I think we should do better than this, will require some work from your part though (or I can do it if you want).
Basically what you wanted to do here is set the OPP for a device and this means do whatever is required for setting the OPP. It is normally frequency, which is not your case, but it is other things as well. Like setting multiple regulators, bandwidth, required-opps, etc.
Quoted 7 lines
I feel the right way of doing this would be to do this:
Factor out
dev_pm_opp_set_opp()fromdev_pm_opp_set_rate()and make the later call the former. And then we can just calldev_pm_opp_set_opp()from your usecase. This will make sure we have a single code path for all the set-opp stuff. What do you think ?
Sounds like it could be a lot of code moving and some extra complexity will be added to the code. If nobody will ever need the universal dev_pm_opp_set_opp(), then it could become a wasted effort. I'd choose the easiest path, i.e. to defer the dev_pm_opp_set_opp() implementation until somebody will really need it.
But if it looks to you that it won't be a too much effort, then I'll appreciate if you could type the patch.
Re: [PATCH v3 05/12] opp: Add dev_pm_opp_set_voltage()
18.01.2021 22:14, Dmitry Osipenko пишет:
Quoted 38 lines
18.01.2021 12:52, Viresh Kumar пишет:
Quoted 29 lines · Dmitry Osipenko
Quoted 9 lines
drivers/opp/core.c +1
-0@@ -2731,3 +2731,58 @@ int dev_pm_opp_sync_regulators(struct device *dev)return ret; }EXPORT_SYMBOL_GPL (dev_pm_opp_sync_regulators); +drivers/opp/core.c int dev_pm_opp_sync_regulators(struct device *dev) · line 2744
Quoted 10 lines
drivers/opp/core.c +10
-0+/** +* dev_pm_opp_set_voltage() - Change voltage of regulators +* @dev: device for which we do this operation +* @opp: opp based on which the voltages are to be configured +* +* Change voltage of the OPP table regulators. +* +* Return: 0 on success or a negative error value. +*/ +int dev_pm_opp_set_voltage (struct device *dev,struct dev_pm_opp *opp)I think we should do better than this, will require some work from your part though (or I can do it if you want).
Basically what you wanted to do here is set the OPP for a device and this means do whatever is required for setting the OPP. It is normally frequency, which is not your case, but it is other things as well. Like setting multiple regulators, bandwidth, required-opps, etc.
Quoted 7 lines
I feel the right way of doing this would be to do this:
Factor out
dev_pm_opp_set_opp()fromdev_pm_opp_set_rate()and make the later call the former. And then we can just calldev_pm_opp_set_opp()from your usecase. This will make sure we have a single code path for all the set-opp stuff. What do you think ?
Quoted 9 lines
Sounds like it could be a lot of code moving and some extra complexity will be added to the code. If nobody will ever need the universal
dev_pm_opp_set_opp(), then it could become a wasted effort. I'd choose the easiest path, i.e. to defer thedev_pm_opp_set_opp()implementation until somebody will really need it.But if it looks to you that it won't be a too much effort, then I'll appreciate if you could type the patch.
Let's start with dev_pm_opp_set_voltage() for now. It shouldn't be a problem at all to upgrade it to dev_pm_opp_set_opp() later on.
I'll make a v4 with the dev_pm_opp_set_voltage(), please let me know if you have objections or more suggestions!
Re: [PATCH v3 05/12] opp: Add dev_pm_opp_set_voltage()
Quoted 9 lines · Dmitry Osipenko
18.01.2021 22:14, Dmitry Osipenko пишет:
Quoted 8 lines
Sounds like it could be a lot of code moving and some extra complexity will be added to the code. If nobody will ever need the universal
dev_pm_opp_set_opp(), then it could become a wasted effort. I'd choose the easiest path, i.e. to defer thedev_pm_opp_set_opp()implementation until somebody will really need it.But if it looks to you that it won't be a too much effort, then I'll appreciate if you could type the patch.
Yes.
Quoted 5 lines
Let's start with
dev_pm_opp_set_voltage()for now. It shouldn't be a problem at all to upgrade it todev_pm_opp_set_opp()later on.I'll make a v4 with the
dev_pm_opp_set_voltage(), please let me know if you have objections or more suggestions!
Sorry about this, I have been working on this stuff for last 2 days. I didn't reply earlier as I thought I would be able to finish this earlier. Once you see the patches you will see it was a significant change :)
I have cc'd you to the relevant patches now. Please see if they work fine for you or not.
Signature · 1 line
viresh
PATCH v3 6/12 opp: Add dev_pm_opp_find_level_ceil()
In reply to Dmitry OsipenkoAdd a ceil version of the dev_pm_opp_find_level(). It's handy to have if levels don't start from 0 in OPP table and zero usually means a minimal level.
--- drivers/opp/core.c | 49 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 8 +++++++ 2 files changed, 57 insertions(+)
drivers/opp/core.c +49 -0
@@ -449,6 +449,55 @@ struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev, }EXPORT_SYMBOL_GPL (dev_pm_opp_find_level_exact); +/** +* dev_pm_opp_find_level_ceil() - search for an rounded up level +* @dev: device for which we do this operation +* @level: level to search for +* +* Return: Searches for rounded up match in the opp table and returns pointer +* to the matching opp if found, else returns ERR_PTR in case of error and +* should be handled using IS_ERR. Error return values can be: +* EINVAL: for bad pointer +* ERANGE: no match found for search +* ENODEV: if device not found in list of registered devices +* +* The callers are required to call dev_pm_opp_put() for the returned OPP after +* use. +*/ +struct dev_pm_opp *dev_pm_opp_find_level_ceil (struct device *dev, +unsigned int *level) +{ +struct opp_table *opp_table; +struct dev_pm_opp *temp_opp, *opp =ERR_PTR (-ERANGE ); + + opp_table =_find_opp_table (dev); +if (IS_ERR (opp_table)) { +int r =PTR_ERR (opp_table); + +dev_err (dev,"%s: OPP table not found (%d)\n" , __func__, r); +return ERR_PTR (r); + } + +mutex_lock (&opp_table->lock); + +list_for_each_entry (temp_opp, &opp_table->opp_list, node) { +if (temp_opp->available && temp_opp->level >= *level) { + opp = temp_opp; + *level = opp->level; + +/* Increment the reference count of OPP */ +dev_pm_opp_get (opp); +break ; + } + } + +mutex_unlock (&opp_table->lock); +dev_pm_opp_put_opp_table (opp_table); + +return opp; +} +EXPORT_SYMBOL_GPL (dev_pm_opp_find_level_ceil); + static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,unsigned long *freq) {
include/linux/pm_opp.h +8 -0
@@ -111,6 +111,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev, bool available);struct dev_pm_opp *dev_pm_opp_find_level_exact (struct device *dev,unsigned int level); +struct dev_pm_opp *dev_pm_opp_find_level_ceil (struct device *dev, +unsigned int *level);struct dev_pm_opp *dev_pm_opp_find_freq_floor (struct device *dev,unsigned long *freq);
@@ -234,6 +236,12 @@ static inline struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev,return ERR_PTR (-ENOTSUPP ); } +static inline struct dev_pm_opp *dev_pm_opp_find_level_ceil (struct device *dev, +unsigned int *level) +{ +return ERR_PTR (-ENOTSUPP ); +} +static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor (struct device *dev,unsigned long *freq) {
Signature · 1 line
2.29.2
Viresh Kumar
AppliedRe: [PATCH v3 06/12] opp: Add dev_pm_opp_find_level_ceil()
Quoted 89 lines · Dmitry Osipenko
Add a ceil version of the
dev_pm_opp_find_level(). It's handy to have if levels don't start from 0 in OPP table and zero usually means a minimal level.Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 49 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 8 +++++++ 2 files changed, 57 insertions(+)drivers/opp/core.c +49
-0@@ -449,6 +449,55 @@ struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev, }EXPORT_SYMBOL_GPL (dev_pm_opp_find_level_exact); +/** +* dev_pm_opp_find_level_ceil() - search for an rounded up level +* @dev: device for which we do this operation +* @level: level to search for +* +* Return: Searches for rounded up match in the opp table and returns pointer +* to the matching opp if found, else returns ERR_PTR in case of error and +* should be handled using IS_ERR. Error return values can be: +* EINVAL: for bad pointer +* ERANGE: no match found for search +* ENODEV: if device not found in list of registered devices +* +* The callers are required to call dev_pm_opp_put() for the returned OPP after +* use. +*/ +struct dev_pm_opp *dev_pm_opp_find_level_ceil (struct device *dev, +unsigned int *level) +{ +struct opp_table *opp_table; +struct dev_pm_opp *temp_opp, *opp =ERR_PTR (-ERANGE ); + + opp_table =_find_opp_table (dev); +if (IS_ERR (opp_table)) { +int r =PTR_ERR (opp_table); + +dev_err (dev,"%s: OPP table not found (%d)\n" , __func__, r); +return ERR_PTR (r); + } + +mutex_lock (&opp_table->lock); + +list_for_each_entry (temp_opp, &opp_table->opp_list, node) { +if (temp_opp->available && temp_opp->level >= *level) { + opp = temp_opp; + *level = opp->level; + +/* Increment the reference count of OPP */ +dev_pm_opp_get (opp); +break ; + } + } + +mutex_unlock (&opp_table->lock); +dev_pm_opp_put_opp_table (opp_table); + +return opp; +} +EXPORT_SYMBOL_GPL (dev_pm_opp_find_level_ceil); + static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,unsigned long *freq) {include/linux/pm_opp.h +2
-0@@ -111,6 +111,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev, bool available);struct dev_pm_opp *dev_pm_opp_find_level_exact (struct device *dev,unsigned int level); +struct dev_pm_opp *dev_pm_opp_find_level_ceil (struct device *dev, +unsigned int *level);struct dev_pm_opp *dev_pm_opp_find_freq_floor (struct device *dev,unsigned long *freq);@@ -234,6 +236,12 @@ static inline struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev,return ERR_PTR (-ENOTSUPP ); }
include/linux/pm_opp.h static inline struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev, · line 247
Quoted 10 lines
include/linux/pm_opp.h +6
-0+static inline struct dev_pm_opp *dev_pm_opp_find_level_ceil (struct device *dev, +unsigned int *level) +{ +return ERR_PTR (-ENOTSUPP ); +} +static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor (struct device *dev,unsigned long *freq) {
Applied. Thanks.
Signature · 1 line
viresh
PATCH v3 7/12 opp: Add dev_pm_opp_get_required_pstate()
In reply to Dmitry OsipenkoAdd dev_pm_opp_get_required_pstate() which allows OPP users to retrieve required performance state of a given OPP.
--- drivers/opp/core.c | 22 ++++++++++++++++++++++ include/linux/pm_opp.h | 10 ++++++++++ 2 files changed, 32 insertions(+)
drivers/opp/core.c +22 -0
@@ -145,6 +145,28 @@ unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp) }EXPORT_SYMBOL_GPL (dev_pm_opp_get_level); +/** +* dev_pm_opp_get_required_pstate() - Gets the required performance state +* corresponding to an available opp +* @opp: opp for which performance state has to be returned for +* @index: index of the required opp +* +* Return: performance state read from device tree corresponding to the +* required opp, else return 0. +*/ +unsigned int dev_pm_opp_get_required_pstate (struct dev_pm_opp *opp, +unsigned int index) +{ +if (IS_ERR_OR_NULL (opp) || !opp->available || + index >= opp->opp_table->required_opp_count) { +pr_err ("%s: Invalid parameters\n" , __func__); +return 0 ; + } + +return opp->required_opps[index]->pstate; +} +EXPORT_SYMBOL_GPL (dev_pm_opp_get_required_pstate); +/** * dev_pm_opp_is_turbo() - Returns if opp is turbo OPP or not * @opp: opp for which turbo mode is being verified
include/linux/pm_opp.h +10 -0
@@ -98,6 +98,9 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);unsigned int dev_pm_opp_get_level (struct dev_pm_opp *opp); +unsigned int dev_pm_opp_get_required_pstate (struct dev_pm_opp *opp, +unsigned int index); +bool dev_pm_opp_is_turbo (struct dev_pm_opp *opp);int dev_pm_opp_get_opp_count (struct device *dev);
@@ -194,6 +197,13 @@ static inline unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp)return 0 ; } +static inline +unsigned int dev_pm_opp_get_required_pstate (struct dev_pm_opp *opp, +unsigned int index) +{ +return 0 ; +} +static inline bool dev_pm_opp_is_turbo (struct dev_pm_opp *opp) {return false;
Signature · 1 line
2.29.2
Viresh Kumar
AppliedRe: [PATCH v3 07/12] opp: Add dev_pm_opp_get_required_pstate()
Quoted 63 lines · Dmitry Osipenko
Add
dev_pm_opp_get_required_pstate()which allows OPP users to retrieve required performance state of a given OPP.Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 22 ++++++++++++++++++++++ include/linux/pm_opp.h | 10 ++++++++++ 2 files changed, 32 insertions(+)drivers/opp/core.c +22
-0@@ -145,6 +145,28 @@ unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp) }EXPORT_SYMBOL_GPL (dev_pm_opp_get_level); +/** +* dev_pm_opp_get_required_pstate() - Gets the required performance state +* corresponding to an available opp +* @opp: opp for which performance state has to be returned for +* @index: index of the required opp +* +* Return: performance state read from device tree corresponding to the +* required opp, else return 0. +*/ +unsigned int dev_pm_opp_get_required_pstate (struct dev_pm_opp *opp, +unsigned int index) +{ +if (IS_ERR_OR_NULL (opp) || !opp->available || + index >= opp->opp_table->required_opp_count) { +pr_err ("%s: Invalid parameters\n" , __func__); +return 0 ; + } + +return opp->required_opps[index]->pstate; +} +EXPORT_SYMBOL_GPL (dev_pm_opp_get_required_pstate); +/** * dev_pm_opp_is_turbo() - Returns if opp is turbo OPP or not * @opp: opp for which turbo mode is being verified include/linux/pm_opp.h +3
-0@@ -98,6 +98,9 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);unsigned int dev_pm_opp_get_level (struct dev_pm_opp *opp); +unsigned int dev_pm_opp_get_required_pstate (struct dev_pm_opp *opp, +unsigned int index); +bool dev_pm_opp_is_turbo (struct dev_pm_opp *opp);int dev_pm_opp_get_opp_count (struct device *dev);@@ -194,6 +197,13 @@ static inline unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp)return 0 ; }
include/linux/pm_opp.h static inline unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp) · line 209
Quoted 10 lines
include/linux/pm_opp.h +7
-0+static inline +unsigned int dev_pm_opp_get_required_pstate (struct dev_pm_opp *opp, +unsigned int index) +{ +return 0 ; +} +static inline bool dev_pm_opp_is_turbo (struct dev_pm_opp *opp) {return false;
Applied. Thanks.
Signature · 1 line
viresh
PATCH v3 8/12 opp: Add devm_pm_opp_register_set_opp_helper
In reply to Dmitry OsipenkoAdd resource-managed version of dev_pm_opp_register_set_opp_helper().
--- drivers/opp/core.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 8 ++++++++ 2 files changed, 42 insertions(+)
drivers/opp/core.c +34 -0
@@ -2857,3 +2857,37 @@ int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp)return ret; }EXPORT_SYMBOL_GPL (dev_pm_opp_set_voltage); + +static void devm_pm_opp_unregister_set_opp_helper (void *data) +{ +dev_pm_opp_unregister_set_opp_helper (data); +} + +/** +* devm_pm_opp_register_set_opp_helper() - Register custom set OPP helper +* @dev: Device for which the helper is getting registered. +* @set_opp: Custom set OPP helper. +* +* This is a resource-managed version of dev_pm_opp_register_set_opp_helper(). +* +* Return: pointer to 'struct opp_table' on success and errorno otherwise. +*/ +struct opp_table * +devm_pm_opp_register_set_opp_helper (struct device *dev, +int (*set_opp)(struct dev_pm_set_opp_data *data)) +{ +struct opp_table *opp_table; +int err; + + opp_table =dev_pm_opp_register_set_opp_helper (dev, set_opp); +if (IS_ERR (opp_table)) +return opp_table; + + err =devm_add_action_or_reset (dev, devm_pm_opp_unregister_set_opp_helper, + opp_table); +if (err) +return ERR_PTR (err); + +return opp_table; +} +EXPORT_SYMBOL_GPL (devm_pm_opp_register_set_opp_helper);
include/linux/pm_opp.h +8 -0
@@ -169,6 +169,7 @@ 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);int dev_pm_opp_set_voltage (struct device *dev,struct dev_pm_opp *opp); +struct opp_table *devm_pm_opp_register_set_opp_helper (struct device *dev,int (*set_opp)(struct dev_pm_set_opp_data *data));#else static inline struct opp_table *dev_pm_opp_get_opp_table (struct device *dev) {
@@ -428,6 +429,13 @@ static inline int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp * return -ENOTSUPP; } +static inline struct opp_table * +devm_pm_opp_register_set_opp_helper (struct device *dev, +int (*set_opp)(struct dev_pm_set_opp_data *data)) +{ +return ERR_PTR (-ENOTSUPP ); +} +#endif /* CONFIG_PM_OPP */ #if defined(CONFIG_PM_OPP ) && defined(CONFIG_OF )
Signature · 1 line
2.29.2
Viresh Kumar
AppliedRe: [PATCH v3 08/12] opp: Add devm_pm_opp_register_set_opp_helper
Quoted 69 lines · Dmitry Osipenko
Add resource-managed version of
dev_pm_opp_register_set_opp_helper().Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 8 ++++++++ 2 files changed, 42 insertions(+)drivers/opp/core.c +34
-0@@ -2857,3 +2857,37 @@ int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp)return ret; }EXPORT_SYMBOL_GPL (dev_pm_opp_set_voltage); + +static void devm_pm_opp_unregister_set_opp_helper (void *data) +{ +dev_pm_opp_unregister_set_opp_helper (data); +} + +/** +* devm_pm_opp_register_set_opp_helper() - Register custom set OPP helper +* @dev: Device for which the helper is getting registered. +* @set_opp: Custom set OPP helper. +* +* This is a resource-managed version of dev_pm_opp_register_set_opp_helper(). +* +* Return: pointer to 'struct opp_table' on success and errorno otherwise. +*/ +struct opp_table * +devm_pm_opp_register_set_opp_helper (struct device *dev, +int (*set_opp)(struct dev_pm_set_opp_data *data)) +{ +struct opp_table *opp_table; +int err; + + opp_table =dev_pm_opp_register_set_opp_helper (dev, set_opp); +if (IS_ERR (opp_table)) +return opp_table; + + err =devm_add_action_or_reset (dev, devm_pm_opp_unregister_set_opp_helper, + opp_table); +if (err) +return ERR_PTR (err); + +return opp_table; +} +EXPORT_SYMBOL_GPL (devm_pm_opp_register_set_opp_helper);include/linux/pm_opp.h +1
-0@@ -169,6 +169,7 @@ 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);int dev_pm_opp_set_voltage (struct device *dev,struct dev_pm_opp *opp); +struct opp_table *devm_pm_opp_register_set_opp_helper (struct device *dev,int (*set_opp)(struct dev_pm_set_opp_data *data));#else static inline struct opp_table *dev_pm_opp_get_opp_table (struct device *dev) {@@ -428,6 +429,13 @@ static inline int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *return -ENOTSUPP ; }
include/linux/pm_opp.h static inline int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp * · line 441
Quoted 10 lines
include/linux/pm_opp.h +7
-0+static inline struct opp_table * +devm_pm_opp_register_set_opp_helper (struct device *dev, +int (*set_opp)(struct dev_pm_set_opp_data *data)) +{ +return ERR_PTR (-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 works fine.
Signature · 1 line
viresh
PATCH v3 9/12 opp: Add devm_pm_opp_attach_genpd
In reply to Dmitry OsipenkoAdd resource-managed version of dev_pm_opp_attach_genpd().
--- drivers/opp/core.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 8 ++++++++ 2 files changed, 43 insertions(+)
drivers/opp/core.c +35 -0
@@ -2891,3 +2891,38 @@ devm_pm_opp_register_set_opp_helper(struct device *dev,return opp_table; }EXPORT_SYMBOL_GPL (devm_pm_opp_register_set_opp_helper); + +static void devm_pm_opp_detach_genpd (void *data) +{ +dev_pm_opp_detach_genpd (data); +} + +/** +* devm_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual device pointer +* @dev: Consumer device for which the genpd is getting attached. +* @names: Null terminated array of pointers containing names of genpd to attach. +* @virt_devs: Pointer to return the array of virtual devices. +* +* This is a resource-managed version of dev_pm_opp_attach_genpd(). +* +* Return: pointer to 'struct opp_table' on success and errorno otherwise. +*/ +struct opp_table * +devm_pm_opp_attach_genpd (struct device *dev,const char **names, +struct device ***virt_devs) +{ +struct opp_table *opp_table; +int err; + + opp_table =dev_pm_opp_attach_genpd (dev, names, virt_devs); +if (IS_ERR (opp_table)) +return opp_table; + + err =devm_add_action_or_reset (dev, devm_pm_opp_detach_genpd, + opp_table); +if (err) +return ERR_PTR (err); + +return opp_table; +} +EXPORT_SYMBOL_GPL (devm_pm_opp_attach_genpd);
include/linux/pm_opp.h +8 -0
@@ -170,6 +170,7 @@ void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);int dev_pm_opp_sync_regulators (struct device *dev);int dev_pm_opp_set_voltage (struct device *dev,struct dev_pm_opp *opp);struct opp_table *devm_pm_opp_register_set_opp_helper (struct device *dev,int (*set_opp)(struct dev_pm_set_opp_data *data)); +struct opp_table *devm_pm_opp_attach_genpd (struct device *dev,const char **names,struct device ***virt_devs);#else static inline struct opp_table *dev_pm_opp_get_opp_table (struct device *dev) {
@@ -436,6 +437,13 @@ devm_pm_opp_register_set_opp_helper(struct device *dev, return ERR_PTR(-ENOTSUPP); } +static inline struct opp_table * +devm_pm_opp_attach_genpd (struct device *dev,const char **names, +struct device ***virt_devs) +{ +return ERR_PTR (-ENOTSUPP ); +} +#endif /* CONFIG_PM_OPP */ #if defined(CONFIG_PM_OPP ) && defined(CONFIG_OF )
Signature · 1 line
2.29.2
Re: [PATCH v3 09/12] opp: Add devm_pm_opp_attach_genpd
Quoted 67 lines · Dmitry Osipenko
Add resource-managed version of
dev_pm_opp_attach_genpd().Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 8 ++++++++ 2 files changed, 43 insertions(+)drivers/opp/core.c +35
-0@@ -2891,3 +2891,38 @@ devm_pm_opp_register_set_opp_helper(struct device *dev,return opp_table; }EXPORT_SYMBOL_GPL (devm_pm_opp_register_set_opp_helper); + +static void devm_pm_opp_detach_genpd (void *data) +{ +dev_pm_opp_detach_genpd (data); +} + +/** +* devm_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual device pointer +* @dev: Consumer device for which the genpd is getting attached. +* @names: Null terminated array of pointers containing names of genpd to attach. +* @virt_devs: Pointer to return the array of virtual devices. +* +* This is a resource-managed version of dev_pm_opp_attach_genpd(). +* +* Return: pointer to 'struct opp_table' on success and errorno otherwise. +*/ +struct opp_table * +devm_pm_opp_attach_genpd (struct device *dev,const char **names, +struct device ***virt_devs) +{ +struct opp_table *opp_table; +int err; + + opp_table =dev_pm_opp_attach_genpd (dev, names, virt_devs); +if (IS_ERR (opp_table)) +return opp_table; + + err =devm_add_action_or_reset (dev, devm_pm_opp_detach_genpd, + opp_table); +if (err) +return ERR_PTR (err); + +return opp_table; +} +EXPORT_SYMBOL_GPL (devm_pm_opp_attach_genpd);include/linux/pm_opp.h +1
-0@@ -170,6 +170,7 @@ void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);int dev_pm_opp_sync_regulators (struct device *dev);int dev_pm_opp_set_voltage (struct device *dev,struct dev_pm_opp *opp);struct opp_table *devm_pm_opp_register_set_opp_helper (struct device *dev,int (*set_opp)(struct dev_pm_set_opp_data *data)); +struct opp_table *devm_pm_opp_attach_genpd (struct device *dev,const char **names,struct device ***virt_devs);#else static inline struct opp_table *dev_pm_opp_get_opp_table (struct device *dev) {@@ -436,6 +437,13 @@ devm_pm_opp_register_set_opp_helper(struct device *dev,return ERR_PTR (-ENOTSUPP ); }
include/linux/pm_opp.h devm_pm_opp_register_set_opp_helper(struct device *dev, · line 449
Quoted 10 lines
include/linux/pm_opp.h +7
-0+static inline struct opp_table * +devm_pm_opp_attach_genpd (struct device *dev,const char **names, +struct device ***virt_devs) +{ +return ERR_PTR (-ENOTSUPP ); +} +#endif /* CONFIG_PM_OPP */ #if defined(CONFIG_PM_OPP ) && defined(CONFIG_OF )
Manually applied. Thanks.
Signature · 1 line
viresh
PATCH v3 10/12 opp: Support set_opp() customization without requiring to use regulators
In reply to Dmitry OsipenkoSupport set_opp() customization without requiring to use regulators. This is needed by drivers which want to use dev_pm_opp_set_rate() for changing rates of a multiple clocks and don't need to touch regulator.
One example is NVIDIA Tegra30/114 SoCs which have two sibling 3D hardware units which should be use to the same clock rate, meanwhile voltage scaling is done using a power domain. In this case OPP table doesn't have a regulator, causing a NULL dereference in _set_opp_custom().
--- drivers/opp/core.c | 46 +++++++++++++++++++++++++++++++++++------- include/linux/pm_opp.h | 3 +++ 2 files changed, 42 insertions(+), 7 deletions(-)
drivers/opp/core.c +39 -7
@@ -829,16 +829,21 @@ static int _set_opp_custom(const struct opp_table *opp_table, struct dev_pm_opp_supply *new_supply) {struct dev_pm_set_opp_data *data; -int size; +int size, count; + +if (opp_table->regulators) + count = opp_table->regulator_count; +else + count =0 ; data = opp_table->set_opp_data; data->regulators = opp_table->regulators; - data->regulator_count = opp_table->regulator_count; + data->regulator_count = count; data->clk = opp_table->clk; data->dev = dev; data->old_opp.rate = old_freq; - size =sizeof (*old_supply) * opp_table->regulator_count; + size =sizeof (*old_supply) * count;if (!old_supply)memset (data->old_opp.supplies,0 , size);else
@@ -1860,8 +1865,14 @@ static int _allocate_set_opp_data(struct opp_table *opp_table)struct dev_pm_set_opp_data *data;int len, count = opp_table->regulator_count; -if (WARN_ON (!opp_table->regulators)) -return -EINVAL ; +/* just bump the refcount if already allocated */ +if (opp_table->set_opp_data) { +kref_get (&opp_table->set_opp_data->kref); +return 0 ; + } + +/* allocate maximum number of regulators, for simplicity */ + count =max (count,1 );/* space for set_opp_data */ len =sizeof (*data);
@@ -1873,6 +1884,7 @@ static int _allocate_set_opp_data(struct opp_table *opp_table)if (!data)return -ENOMEM ; +kref_init (&data->kref); data->old_opp.supplies = (void *)(data +1 ); data->new_opp.supplies = data->old_opp.supplies + count;
@@ -1881,10 +1893,17 @@ static int _allocate_set_opp_data(struct opp_table *opp_table)return 0 ; } +static void _release_set_opp_data (struct kref *kref) +{ +kfree (container_of(kref ,struct dev_pm_set_opp_data ,kref )); +} +static void _free_set_opp_data (struct opp_table *opp_table) { -kfree (opp_table->set_opp_data); - opp_table->set_opp_data =NULL ; +struct dev_pm_set_opp_data *data = opp_table->set_opp_data; + +if (kref_put (&data->kref, _release_set_opp_data)) + opp_table->set_opp_data =NULL ; }/**
@@ -2182,6 +2201,7 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data)) {struct opp_table *opp_table; +int ret;if (!set_opp)return ERR_PTR (-EINVAL );
@@ -2196,11 +2216,21 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,return ERR_PTR (-EBUSY ); } +/* Allocate block to pass to set_opp() routines */ + ret =_allocate_set_opp_data (opp_table); +if (ret) + goto err; +/* Another CPU that shares the OPP table has set the helper ? */ if (!opp_table->set_opp) opp_table->set_opp = set_opp;return opp_table; + +err: +dev_pm_opp_put_opp_table (opp_table); + +return ERR_PTR (ret); }EXPORT_SYMBOL_GPL (dev_pm_opp_register_set_opp_helper);
@@ -2219,6 +2249,8 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)/* Make sure there are no concurrent readers while updating opp_table */ WARN_ON (!list_empty (&opp_table->opp_list)); +_free_set_opp_data (opp_table); + opp_table->set_opp =NULL ;dev_pm_opp_put_opp_table (opp_table); }
include/linux/pm_opp.h +3 -0
@@ -13,6 +13,7 @@#include <linux/energy_model.h> #include <linux/err.h> +#include <linux/kref.h> #include <linux/notifier.h> struct clk ;
@@ -74,6 +75,7 @@ struct dev_pm_opp_info {* @regulator_count: Number of regulators * @clk: Pointer to clk * @dev: Pointer to the struct device +* @kref: Reference counter * * This structure contains all information required for setting an OPP. */
@@ -85,6 +87,7 @@ struct dev_pm_set_opp_data {unsigned int regulator_count;struct clk *clk;struct device *dev; +struct kref kref; };#if defined(CONFIG_PM_OPP )
Signature · 1 line
2.29.2
Re: [PATCH v3 10/12] opp: Support set_opp() customization without requiring to use regulators
Quoted 21 lines · Dmitry Osipenko
include/linux/pm_opp.h +2
-0@@ -13,6 +13,7 @@#include <linux/energy_model.h> #include <linux/err.h> +#include <linux/kref.h> #include <linux/notifier.h> struct clk ;@@ -74,6 +75,7 @@ struct dev_pm_opp_info {* @regulator_count: Number of regulators * @clk: Pointer to clk * @dev: Pointer to the struct device +* @kref: Reference counter * * This structure contains all information required for setting an OPP. */
include/linux/pm_opp.h struct dev_pm_set_opp_data { · line 91
Quoted 5 lines
include/linux/pm_opp.h +1
-0@@ -85,6 +87,7 @@ struct dev_pm_set_opp_data {unsigned int regulator_count;struct clk *clk;struct device *dev; +struct kref kref; };
Instead of kref thing, allocate the memory for supplies from dev_pm_opp_set_regulators() and store it in new entries in opp-table and for rest of the data from dev_pm_opp_register_set_opp_helper(), to which you can copy supplies pointers then.
Signature · 1 line
viresh
Re: [PATCH v3 10/12] opp: Support set_opp() customization without requiring to use regulators
18.01.2021 14:44, Viresh Kumar пишет:
Quoted 24 lines · Dmitry Osipenko
Quoted 23 lines
include/linux/pm_opp.h +2
-0@@ -13,6 +13,7 @@#include <linux/energy_model.h> #include <linux/err.h> +#include <linux/kref.h> #include <linux/notifier.h> struct clk ;@@ -74,6 +75,7 @@ struct dev_pm_opp_info {* @regulator_count: Number of regulators * @clk: Pointer to clk * @dev: Pointer to the struct device +* @kref: Reference counter * * This structure contains all information required for setting an OPP. */ @@ -85,6 +87,7 @@ struct dev_pm_set_opp_data {unsigned int regulator_count;struct clk *clk;
Quoted 9 lines
include/linux/pm_opp.h struct dev_pm_set_opp_data { · line 91
Quoted 3 lines
include/linux/pm_opp.h +1
-0struct device *dev; +struct kref kref; };Instead of kref thing, allocate the memory for supplies from
dev_pm_opp_set_regulators()and store it in new entries in opp-table and for rest of the data fromdev_pm_opp_register_set_opp_helper(), to which you can copy supplies pointers then.
Could you please show a code sample?
PATCH 1/1 opp: Prepare for ->set_opp() helper to work without regulators
Until now the ->set_opp() helper (i.e. special implementation for setting the OPPs for platforms) was implemented only to take care of multiple regulators case, but going forward we would need that for other use cases as well.
This patch prepares for that by allocating the regulator specific part from dev_pm_opp_set_regulators() and the opp helper part from dev_pm_opp_register_set_opp_helper().
---
Dmitry,
I haven't tested this patch, can you please help with that ?
drivers/opp/core.c | 81 ++++++++++++++++++++++++---------------------- drivers/opp/opp.h | 2 ++ 2 files changed, 45 insertions(+), 38 deletions(-)
drivers/opp/core.c +43 -38
@@ -1817,38 +1817,6 @@ void dev_pm_opp_put_prop_name(struct opp_table *opp_table) }EXPORT_SYMBOL_GPL (dev_pm_opp_put_prop_name); -static int _allocate_set_opp_data (struct opp_table *opp_table) -{ -struct dev_pm_set_opp_data *data; -int len, count = opp_table->regulator_count; - -if (WARN_ON (!opp_table->regulators)) -return -EINVAL ; - -/* space for set_opp_data */ - len =sizeof (*data); - -/* space for old_opp.supplies and new_opp.supplies */ - len +=2 *sizeof (struct dev_pm_opp_supply ) * count; - - data =kzalloc (len,GFP_KERNEL ); -if (!data) -return -ENOMEM ; - - data->old_opp.supplies = (void *)(data +1 ); - data->new_opp.supplies = data->old_opp.supplies + count; - - opp_table->set_opp_data = data; - -return 0 ; -} - -static void _free_set_opp_data (struct opp_table *opp_table) -{ -kfree (opp_table->set_opp_data); - opp_table->set_opp_data =NULL ; -} -/** * dev_pm_opp_set_regulators() - Set regulator names for the device * @dev: Device for which regulator name is being set.
@@ -1865,6 +1833,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count) { +struct dev_pm_opp_supply *supplies; +struct dev_pm_set_opp_data *data;struct opp_table *opp_table;struct regulator *reg;int ret, i;
@@ -1906,10 +1876,20 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, opp_table->regulator_count = count; -/* Allocate block only once to pass to set_opp() routines */ - ret =_allocate_set_opp_data (opp_table); -if (ret) + supplies =kmalloc_array (count *2 ,sizeof (*supplies),GFP_KERNEL ); +if (!supplies) { + ret = -ENOMEM ; goto free_regulators; + } + +mutex_lock (&opp_table->lock); + opp_table->sod_supplies = supplies; +if (opp_table->set_opp_data) { + data = opp_table->set_opp_data; + data->old_opp.supplies = supplies; + data->new_opp.supplies = supplies + count; + } +mutex_unlock (&opp_table->lock);return opp_table;
@@ -1952,9 +1932,16 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)for (i = opp_table->regulator_count -1 ; i >=0 ; i--)regulator_put (opp_table->regulators[i]); -_free_set_opp_data (opp_table); +mutex_lock (&opp_table->lock); +if (opp_table->sod_supplies) { + opp_table->set_opp_data->old_opp.supplies =NULL ; + opp_table->set_opp_data->new_opp.supplies =NULL ; + } +mutex_unlock (&opp_table->lock); +kfree (opp_table->sod_supplies);kfree (opp_table->regulators); + opp_table->sod_supplies =NULL ; opp_table->regulators =NULL ; opp_table->regulator_count =-1 ;
@@ -2046,6 +2033,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_clkname);struct opp_table *dev_pm_opp_register_set_opp_helper (struct device *dev,int (*set_opp)(struct dev_pm_set_opp_data *data)) { +struct dev_pm_set_opp_data *data;struct opp_table *opp_table;if (!set_opp)
@@ -2062,8 +2050,23 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, }/* Another CPU that shares the OPP table has set the helper ? */ -if (!opp_table->set_opp) - opp_table->set_opp = set_opp; +if (opp_table->set_opp) +return opp_table; + + data =kzalloc (sizeof (*data),GFP_KERNEL ); +if (!data) +return ERR_PTR (-ENOMEM ); + +mutex_lock (&opp_table->lock); + opp_table->set_opp_data = data; +if (opp_table->sod_supplies) { + data->old_opp.supplies = opp_table->sod_supplies; + data->new_opp.supplies = opp_table->sod_supplies + + opp_table->regulator_count; + } +mutex_unlock (&opp_table->lock); + + opp_table->set_opp = set_opp;return opp_table; }
@@ -2085,6 +2088,8 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)WARN_ON (!list_empty (&opp_table->opp_list)); opp_table->set_opp =NULL ; +kfree (opp_table->set_opp_data); + opp_table->set_opp_data =NULL ;dev_pm_opp_put_opp_table (opp_table); }EXPORT_SYMBOL_GPL (dev_pm_opp_unregister_set_opp_helper);
drivers/opp/opp.h +2 -0
@@ -155,6 +155,7 @@ enum opp_table_access {* @genpd_performance_state: Device's power domain support performance state. * @is_genpd: Marks if the OPP table belongs to a genpd. * @set_opp: Platform specific set_opp callback +* @sod_supplies: Set opp data supplies * @set_opp_data: Data to be passed to set_opp callback * @dentry: debugfs dentry pointer of the real device directory (not links). * @dentry_name: Name of the real dentry.
@@ -202,6 +203,7 @@ struct opp_table {bool is_genpd;int (*set_opp)(struct dev_pm_set_opp_data *data); +struct dev_pm_opp_supply *sod_supplies;struct dev_pm_set_opp_data *set_opp_data;#ifdef CONFIG_DEBUG_FS
Signature · 1 line
2.25.0.rc1.19.g042ed3e048af
Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators
19.01.2021 09:35, Viresh Kumar пишет:
drivers/opp/core.c struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, · line 2067
Quoted 8 lines
drivers/opp/core.c +8
-0+mutex_lock (&opp_table->lock); + opp_table->set_opp_data = data; +if (opp_table->sod_supplies) { + data->old_opp.supplies = opp_table->sod_supplies; + data->new_opp.supplies = opp_table->sod_supplies + + opp_table->regulator_count; + } +mutex_unlock (&opp_table->lock);
Why do we need all these locks in this patch?
The OPP API isn't thread-safe, these locks won't make the API thread-safe. At least both sod_supplies and set_opp() pointers should be set and unset under the lock.
If you're about to make OPP thread-safe, then I suggest to do it separately from this change. Otherwise this patch looks good to me.
Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators
Quoted 11 lines · Dmitry Osipenko
19.01.2021 09:35, Viresh Kumar пишет:
drivers/opp/core.c struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, · line 2067
Quoted 8 lines
drivers/opp/core.c +8
-0+mutex_lock (&opp_table->lock); + opp_table->set_opp_data = data; +if (opp_table->sod_supplies) { + data->old_opp.supplies = opp_table->sod_supplies; + data->new_opp.supplies = opp_table->sod_supplies + + opp_table->regulator_count; + } +mutex_unlock (&opp_table->lock);Why do we need all these locks in this patch?
In case dev_pm_opp_set_regulators() and dev_pm_opp_register_set_opp_helper() get called at the same time. Which can actually happen, though is a corner case.
Quoted 2 lines
The OPP API isn't thread-safe, these locks won't make the API thread-safe.
I am not sure what you mean by that, can you please explain ?
Quoted 2 lines
At least both
sod_suppliesandset_opp()pointers should be set and unset under the lock.
The ->set_opp pointer isn't getting used for a comparison and so putting that inside a lock won't get us anything. We are only using set_opp_data and sod_supplies for comparison at both the places and so they need to be updated within the lock.
Signature · 1 line
viresh
Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators
20.01.2021 10:39, Viresh Kumar пишет:
Quoted 22 lines · Dmitry Osipenko
Quoted 11 lines
19.01.2021 09:35, Viresh Kumar пишет:
drivers/opp/core.c struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, · line 2067
Quoted 8 lines
drivers/opp/core.c +8
-0+mutex_lock (&opp_table->lock); + opp_table->set_opp_data = data; +if (opp_table->sod_supplies) { + data->old_opp.supplies = opp_table->sod_supplies; + data->new_opp.supplies = opp_table->sod_supplies + + opp_table->regulator_count; + } +mutex_unlock (&opp_table->lock);Why do we need all these locks in this patch?
In case
dev_pm_opp_set_regulators()anddev_pm_opp_register_set_opp_helper()get called at the same time. Which can actually happen, though is a corner case.Quoted 2 lines
The OPP API isn't thread-safe, these locks won't make the API thread-safe.
I am not sure what you mean by that, can you please explain ?
Quoted 8 lines
Quoted 2 lines
At least both
sod_suppliesandset_opp()pointers should be set and unset under the lock.The ->set_opp pointer isn't getting used for a comparison and so putting that inside a lock won't get us anything. We are only using
set_opp_dataandsod_suppliesfor comparison at both the places and so they need to be updated within the lock.
If OPP API was meant to be thread-safe, then the dev_pm_opp_unregister_set_opp_helper() should unset the opp_table->set_opp_data under the lock since it races with dev_pm_opp_set_regulators().
Secondly, functions like dev_pm_opp_set_rate() don't have any locks at all.
It should be better not to add "random" locks into the code because it only creates an illusion for an oblivious API user that OPP API cares about thread safety, IMO.
Making OPP API thread-safe will take some effort and a careful review of every lock will be needed.
Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators
Quoted 4 lines · Dmitry Osipenko
If OPP API was meant to be thread-safe, then the
dev_pm_opp_unregister_set_opp_helper()should unset theopp_table->set_opp_dataunder the lock since it races withdev_pm_opp_set_regulators().
Right, I will fix that.
Quoted 1 line
Secondly, functions like
dev_pm_opp_set_rate()don't have any locks at all.
It was on purpose. It is expected that this routine specially will only have a single caller and calls will be in sequence. This gets called a lot and we wanted to make it as much efficient as possible.
Quoted 6 lines
It should be better not to add "random" locks into the code because it only creates an illusion for an oblivious API user that OPP API cares about thread safety, IMO.
Making OPP API thread-safe will take some effort and a careful review of every lock will be needed.
I agree, we have kept some part out of the lock intentionally, but every other thing which can happen in parallel is well protected. There maybe bugs, which I am not aware of.
Another reason you see less locks is because of the way I have used the kref thing here. That allows us to take locks for very small section of code and not big routines.
Signature · 1 line
viresh
Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators
In reply to Viresh Kumar19.01.2021 09:35, Viresh Kumar пишет:
Quoted 10 lines
Until now the ->set_opp() helper (i.e. special implementation for setting the OPPs for platforms) was implemented only to take care of multiple regulators case, but going forward we would need that for other use cases as well.
This patch prepares for that by allocating the regulator specific part from
dev_pm_opp_set_regulators()and the opp helper part fromdev_pm_opp_register_set_opp_helper().Signed-off-by Viresh Kumar <[email protected]>
Quoted 8 lines
---
Dmitry,I haven't tested this patch, can you please help with that ?
drivers/opp/core.c | 81 ++++++++++++++++++++++++---------------------- drivers/opp/opp.h | 2 ++ 2 files changed, 45 insertions(+), 38 deletions(-)
Works good, thank you. It also almost looks good to me, please see my other reply regarding the locks.
Now the set_opp() needs to be taught about the sod_supplies in order to actually make it to work without the regulators, I'll make a patch on top of this change.
Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators
In reply to Viresh KumarQuoted 10 lines · Viresh Kumar
Until now the ->set_opp() helper (i.e. special implementation for setting the OPPs for platforms) was implemented only to take care of multiple regulators case, but going forward we would need that for other use cases as well.
This patch prepares for that by allocating the regulator specific part from
dev_pm_opp_set_regulators()and the opp helper part fromdev_pm_opp_register_set_opp_helper().Signed-off-by Viresh Kumar <[email protected]>
Quoted 8 lines
---
Dmitry,I haven't tested this patch, can you please help with that ?
drivers/opp/core.c | 81 ++++++++++++++++++++++++---------------------- drivers/opp/opp.h | 2 ++ 2 files changed, 45 insertions(+), 38 deletions(-)
Pushed with this diff.
drivers/opp/core.c +3 -5
@@ -1939,7 +1939,6 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, unsigned int count) {struct dev_pm_opp_supply *supplies; -struct dev_pm_set_opp_data *data;struct opp_table *opp_table;struct regulator *reg;int ret, i;
@@ -1990,9 +1989,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,mutex_lock (&opp_table->lock); opp_table->sod_supplies = supplies;if (opp_table->set_opp_data) { - data = opp_table->set_opp_data; - data->old_opp.supplies = supplies; - data->new_opp.supplies = supplies + count; + opp_table->set_opp_data->old_opp.supplies = supplies; + opp_table->set_opp_data->new_opp.supplies = supplies + count; }mutex_unlock (&opp_table->lock);
@@ -2038,7 +2036,7 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)regulator_put (opp_table->regulators[i]);mutex_lock (&opp_table->lock); -if (opp_table->sod_supplies) { +if (opp_table->set_opp_data) { opp_table->set_opp_data->old_opp.supplies =NULL ; opp_table->set_opp_data->new_opp.supplies =NULL ; }
Signature · 1 line
viresh
Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators
Quoted 47 lines · Viresh Kumar
Quoted 10 lines · Viresh Kumar
Until now the ->set_opp() helper (i.e. special implementation for setting the OPPs for platforms) was implemented only to take care of multiple regulators case, but going forward we would need that for other use cases as well.
This patch prepares for that by allocating the regulator specific part from
dev_pm_opp_set_regulators()and the opp helper part fromdev_pm_opp_register_set_opp_helper().Signed-off-by Viresh Kumar <[email protected]>Quoted 8 lines
---
Dmitry,I haven't tested this patch, can you please help with that ?
drivers/opp/core.c | 81 ++++++++++++++++++++++++---------------------- drivers/opp/opp.h | 2 ++ 2 files changed, 45 insertions(+), 38 deletions(-)Pushed with this diff.
drivers/opp/core.c +2
-4@@ -1939,7 +1939,6 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, unsigned int count) {struct dev_pm_opp_supply *supplies; -struct dev_pm_set_opp_data *data;struct opp_table *opp_table;struct regulator *reg;int ret, i;@@ -1990,9 +1989,8 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,mutex_lock (&opp_table->lock); opp_table->sod_supplies = supplies;if (opp_table->set_opp_data) { - data = opp_table->set_opp_data; - data->old_opp.supplies = supplies; - data->new_opp.supplies = supplies + count; + opp_table->set_opp_data->old_opp.supplies = supplies; + opp_table->set_opp_data->new_opp.supplies = supplies + count; }mutex_unlock (&opp_table->lock);
drivers/opp/core.c void dev_pm_opp_put_regulators(struct opp_table *opp_table) · line 2042
Quoted 8 lines
drivers/opp/core.c +1
-1@@ -2038,7 +2036,7 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)regulator_put (opp_table->regulators[i]);mutex_lock (&opp_table->lock); -if (opp_table->sod_supplies) { +if (opp_table->set_opp_data) { opp_table->set_opp_data->old_opp.supplies =NULL ; opp_table->set_opp_data->new_opp.supplies =NULL ; }
And some more as pointed out by Dmitry (I will resend it again properly).
t a/drivers/opp/core.c b/drivers/opp/core.c
index d8ca15d96ce9..747bdc4338f3 100644
drivers/opp/core.c +4 -0
@@ -2118,8 +2118,12 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)WARN_ON (!list_empty (&opp_table->opp_list)); opp_table->set_opp =NULL ; + +mutex_lock (&opp_table->lock);kfree (opp_table->set_opp_data); opp_table->set_opp_data =NULL ; +mutex_unlock (&opp_table->lock); +dev_pm_opp_put_opp_table (opp_table); }EXPORT_SYMBOL_GPL (dev_pm_opp_unregister_set_opp_helper);
Signature · 1 line
viresh
Re: [PATCH v3 10/12] opp: Support set_opp() customization without requiring to use regulators
In reply to Dmitry OsipenkoQuoted 35 lines · Dmitry Osipenko
18.01.2021 14:44, Viresh Kumar пишет:
Quoted 24 lines · Dmitry Osipenko
Quoted 23 lines
include/linux/pm_opp.h +2
-0@@ -13,6 +13,7 @@#include <linux/energy_model.h> #include <linux/err.h> +#include <linux/kref.h> #include <linux/notifier.h> struct clk ;@@ -74,6 +75,7 @@ struct dev_pm_opp_info {* @regulator_count: Number of regulators * @clk: Pointer to clk * @dev: Pointer to the struct device +* @kref: Reference counter * * This structure contains all information required for setting an OPP. */ @@ -85,6 +87,7 @@ struct dev_pm_set_opp_data {unsigned int regulator_count;struct clk *clk;Quoted 9 lines
include/linux/pm_opp.h struct dev_pm_set_opp_data { · line 91
Quoted 3 lines
include/linux/pm_opp.h +1
-0struct device *dev; +struct kref kref; };Instead of kref thing, allocate the memory for supplies from
dev_pm_opp_set_regulators()and store it in new entries in opp-table and for rest of the data fromdev_pm_opp_register_set_opp_helper(), to which you can copy supplies pointers then.
Quoted 1 line
Could you please show a code sample?
Sent a patch to you.
Signature · 1 line
viresh
PATCH v2 1/1 opp: Prepare for ->set_opp() helper to work without regulators
In reply to Dmitry OsipenkoUntil now the ->set_opp() helper (i.e. special implementation for setting the OPPs for platforms) was implemented only to take care of multiple regulators case, but going forward we would need that for other use cases as well.
This patch prepares for that by allocating the regulator specific part from dev_pm_opp_set_regulators() and the opp helper part from dev_pm_opp_register_set_opp_helper().
---
V2:
- Fixed a bug where we accessed the wrong pointer
- Add locks in dev_pm_opp_unregister_set_opp_helper()
drivers/opp/core.c | 83 +++++++++++++++++++++++++--------------------- drivers/opp/opp.h | 2 ++ 2 files changed, 47 insertions(+), 38 deletions(-)
drivers/opp/core.c +45 -38
@@ -1817,38 +1817,6 @@ void dev_pm_opp_put_prop_name(struct opp_table *opp_table) }EXPORT_SYMBOL_GPL (dev_pm_opp_put_prop_name); -static int _allocate_set_opp_data (struct opp_table *opp_table) -{ -struct dev_pm_set_opp_data *data; -int len, count = opp_table->regulator_count; - -if (WARN_ON (!opp_table->regulators)) -return -EINVAL ; - -/* space for set_opp_data */ - len =sizeof (*data); - -/* space for old_opp.supplies and new_opp.supplies */ - len +=2 *sizeof (struct dev_pm_opp_supply ) * count; - - data =kzalloc (len,GFP_KERNEL ); -if (!data) -return -ENOMEM ; - - data->old_opp.supplies = (void *)(data +1 ); - data->new_opp.supplies = data->old_opp.supplies + count; - - opp_table->set_opp_data = data; - -return 0 ; -} - -static void _free_set_opp_data (struct opp_table *opp_table) -{ -kfree (opp_table->set_opp_data); - opp_table->set_opp_data =NULL ; -} -/** * dev_pm_opp_set_regulators() - Set regulator names for the device * @dev: Device for which regulator name is being set.
@@ -1865,6 +1833,7 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count) { +struct dev_pm_opp_supply *supplies;struct opp_table *opp_table;struct regulator *reg;int ret, i;
@@ -1906,10 +1875,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev, opp_table->regulator_count = count; -/* Allocate block only once to pass to set_opp() routines */ - ret =_allocate_set_opp_data (opp_table); -if (ret) + supplies =kmalloc_array (count *2 ,sizeof (*supplies),GFP_KERNEL ); +if (!supplies) { + ret = -ENOMEM ; goto free_regulators; + } + +mutex_lock (&opp_table->lock); + opp_table->sod_supplies = supplies; +if (opp_table->set_opp_data) { + opp_table->set_opp_data->old_opp.supplies = supplies; + opp_table->set_opp_data->new_opp.supplies = supplies + count; + } +mutex_unlock (&opp_table->lock);return opp_table;
@@ -1952,9 +1930,16 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)for (i = opp_table->regulator_count -1 ; i >=0 ; i--)regulator_put (opp_table->regulators[i]); -_free_set_opp_data (opp_table); +mutex_lock (&opp_table->lock); +if (opp_table->set_opp_data) { + opp_table->set_opp_data->old_opp.supplies =NULL ; + opp_table->set_opp_data->new_opp.supplies =NULL ; + } +mutex_unlock (&opp_table->lock); +kfree (opp_table->sod_supplies);kfree (opp_table->regulators); + opp_table->sod_supplies =NULL ; opp_table->regulators =NULL ; opp_table->regulator_count =-1 ;
@@ -2046,6 +2031,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_clkname);struct opp_table *dev_pm_opp_register_set_opp_helper (struct device *dev,int (*set_opp)(struct dev_pm_set_opp_data *data)) { +struct dev_pm_set_opp_data *data;struct opp_table *opp_table;if (!set_opp)
@@ -2062,8 +2048,23 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, }/* Another CPU that shares the OPP table has set the helper ? */ -if (!opp_table->set_opp) - opp_table->set_opp = set_opp; +if (opp_table->set_opp) +return opp_table; + + data =kzalloc (sizeof (*data),GFP_KERNEL ); +if (!data) +return ERR_PTR (-ENOMEM ); + +mutex_lock (&opp_table->lock); + opp_table->set_opp_data = data; +if (opp_table->sod_supplies) { + data->old_opp.supplies = opp_table->sod_supplies; + data->new_opp.supplies = opp_table->sod_supplies + + opp_table->regulator_count; + } +mutex_unlock (&opp_table->lock); + + opp_table->set_opp = set_opp;return opp_table; }
@@ -2085,6 +2086,12 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)WARN_ON (!list_empty (&opp_table->opp_list)); opp_table->set_opp =NULL ; + +mutex_lock (&opp_table->lock); +kfree (opp_table->set_opp_data); + opp_table->set_opp_data =NULL ; +mutex_unlock (&opp_table->lock); +dev_pm_opp_put_opp_table (opp_table); }EXPORT_SYMBOL_GPL (dev_pm_opp_unregister_set_opp_helper);
drivers/opp/opp.h +2 -0
@@ -155,6 +155,7 @@ enum opp_table_access {* @genpd_performance_state: Device's power domain support performance state. * @is_genpd: Marks if the OPP table belongs to a genpd. * @set_opp: Platform specific set_opp callback +* @sod_supplies: Set opp data supplies * @set_opp_data: Data to be passed to set_opp callback * @dentry: debugfs dentry pointer of the real device directory (not links). * @dentry_name: Name of the real dentry.
@@ -202,6 +203,7 @@ struct opp_table {bool is_genpd;int (*set_opp)(struct dev_pm_set_opp_data *data); +struct dev_pm_opp_supply *sod_supplies;struct dev_pm_set_opp_data *set_opp_data;#ifdef CONFIG_DEBUG_FS
Signature · 1 line
2.25.0.rc1.19.g042ed3e048af
Re: [PATCH V2] opp: Prepare for ->set_opp() helper to work without regulators
21.01.2021 14:30, Viresh Kumar пишет:
Quoted 7 lines
drivers/opp/core.c +2
-1@@ -1952,9 +1930,16 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)for (i = opp_table->regulator_count -1 ; i >=0 ; i--)regulator_put (opp_table->regulators[i]); -_free_set_opp_data (opp_table); +mutex_lock (&opp_table->lock); +if (opp_table->set_opp_data) {
drivers/opp/core.c void dev_pm_opp_put_regulators(struct opp_table *opp_table) · line 1944
Quoted 10 lines
drivers/opp/core.c +6
-0+ opp_table->set_opp_data->old_opp.supplies =NULL ; + opp_table->set_opp_data->new_opp.supplies =NULL ; + } +mutex_unlock (&opp_table->lock); +kfree (opp_table->sod_supplies);kfree (opp_table->regulators); + opp_table->sod_supplies =NULL ; opp_table->regulators =NULL ; opp_table->regulator_count =-1 ;
The sod_supplies should be unset under the lock.
Re: [PATCH V2] opp: Prepare for ->set_opp() helper to work without regulators
Quoted 13 lines · Dmitry Osipenko
21.01.2021 14:30, Viresh Kumar пишет:
Quoted 12 lines
drivers/opp/core.c +6
-1@@ -1952,9 +1930,16 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)for (i = opp_table->regulator_count -1 ; i >=0 ; i--)regulator_put (opp_table->regulators[i]); -_free_set_opp_data (opp_table); +mutex_lock (&opp_table->lock); +if (opp_table->set_opp_data) { + opp_table->set_opp_data->old_opp.supplies =NULL ; + opp_table->set_opp_data->new_opp.supplies =NULL ; + } +mutex_unlock (&opp_table->lock);
Quoted 7 lines
drivers/opp/core.c void dev_pm_opp_put_regulators(struct opp_table *opp_table) · line 1944
Quoted 5 lines
drivers/opp/core.c +2
-0+kfree (opp_table->sod_supplies);kfree (opp_table->regulators); + opp_table->sod_supplies =NULL ; opp_table->regulators =NULL ; opp_table->regulator_count =-1 ;The
sod_suppliesshould be unset under the lock.
Fixed, thanks.
Signature · 1 line
viresh
PATCH v3 11/12 opp: Handle missing OPP table in dev_pm_opp_xlate_performance_state()
In reply to Dmitry OsipenkoNVIDIA Tegra SoCs have a power domains topology such that child domains only clamp a power rail, while parent domain controls shared performance state of the multiple child domains. In this case child's domain doesn't need to have OPP table. Hence we want to allow children power domains to pass performance state to the parent domain if child's domain doesn't have OPP table.
The dev_pm_opp_xlate_performance_state() gets src_table=NULL if a child power domain doesn't have OPP table and in this case we should pass the performance state to the parent domain.
--- drivers/opp/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/opp/core.c +1 -1
@@ -2419,7 +2419,7 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,* and so none of them have the "required-opps" property set. Return the * pstate of the src_table as it is in such cases. */ -if (!src_table->required_opp_count) +if (!src_table || !src_table->required_opp_count)return pstate;for (i =0 ; i < src_table->required_opp_count; i++) {
Signature · 1 line
2.29.2
Viresh Kumar
AppliedRe: [PATCH v3 11/12] opp: Handle missing OPP table in dev_pm_opp_xlate_performance_state()
Quoted 24 lines · Dmitry Osipenko
NVIDIA Tegra SoCs have a power domains topology such that child domains only clamp a power rail, while parent domain controls shared performance state of the multiple child domains. In this case child's domain doesn't need to have OPP table. Hence we want to allow children power domains to pass performance state to the parent domain if child's domain doesn't have OPP table.
The
dev_pm_opp_xlate_performance_state()gets src_table=NULL if a child power domain doesn't have OPP table and in this case we should pass the performance state to the parent domain.Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/opp/core.c +0
-0
drivers/opp/core.c int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, · line 2425
Quoted 8 lines
drivers/opp/core.c +1
-1@@ -2419,7 +2419,7 @@ int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,* and so none of them have the "required-opps" property set. Return the * pstate of the src_table as it is in such cases. */ -if (!src_table->required_opp_count) +if (!src_table || !src_table->required_opp_count)return pstate;for (i =0 ; i < src_table->required_opp_count; i++) {
Applied. Thanks.
Signature · 1 line
viresh
PATCH v3 12/12 opp: Print OPP level in debug message of _opp_add_static_v2()
In reply to Dmitry OsipenkoPrint OPP level in debug message of _opp_add_static_v2(). This helps to chase GENPD bugs.
--- drivers/opp/of.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/opp/of.c +3 -2
@@ -822,10 +822,11 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,if (new_opp->clock_latency_ns > opp_table->clock_latency_ns_max) opp_table->clock_latency_ns_max = new_opp->clock_latency_ns; -pr_debug ("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n" , +pr_debug ("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu level:%u\n" , __func__, new_opp->turbo, new_opp->rate, new_opp->supplies[0 ].u_volt, new_opp->supplies[0 ].u_volt_min, - new_opp->supplies[0 ].u_volt_max, new_opp->clock_latency_ns); + new_opp->supplies[0 ].u_volt_max, new_opp->clock_latency_ns, + new_opp->level);/* * Notify the changes in the availability of the operable
Signature · 1 line
2.29.2
Viresh Kumar
AppliedRe: [PATCH v3 12/12] opp: Print OPP level in debug message of _opp_add_static_v2()
Quoted 19 lines · Dmitry Osipenko
Print OPP level in debug message of
_opp_add_static_v2(). This helps to chase GENPD bugs.Tested-by Peter Geis <[email protected]>Tested-by Nicolas Chauvet <[email protected]>Tested-by Matt Merhar <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/of.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/opp/of.c +0
-0@@ -822,10 +822,11 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,if (new_opp->clock_latency_ns > opp_table->clock_latency_ns_max) opp_table->clock_latency_ns_max = new_opp->clock_latency_ns;
drivers/opp/of.c static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table, · line 832
Quoted 10 lines
drivers/opp/of.c +3
-2-pr_debug ("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n" , +pr_debug ("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu level:%u\n" , __func__, new_opp->turbo, new_opp->rate, new_opp->supplies[0 ].u_volt, new_opp->supplies[0 ].u_volt_min, - new_opp->supplies[0 ].u_volt_max, new_opp->clock_latency_ns); + new_opp->supplies[0 ].u_volt_max, new_opp->clock_latency_ns, + new_opp->level);/* * Notify the changes in the availability of the operable
Applied. Thanks.
Signature · 1 line
viresh
Viresh Kumar
AppliedQuoted 5 lines · Dmitry Osipenko
Hi,
This series fixes problems and adds features to OPP API that are required for implementation of a power domain driver for NVIDIA Tegra SoCs.
Quoted 8 lines
It is a continuation of [1], where Viresh Kumar asked to factor OPP patches into a separate series. I factored out the patches into this series, addressed the previous review comments and re-based patches on top of [2], which replaced some of my patches that added resource-managed helpers.
[1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=221130
[2] https://lore.kernel.org/linux-pm/[email protected]/
Hi Dmitry,
I have applied 9 out of 12 patches already. Thanks.
Signature · 1 line
viresh
18.01.2021 14:46, Viresh Kumar пишет:
Quoted 12 lines · Dmitry Osipenko
Quoted 11 lines
Hi,
This series fixes problems and adds features to OPP API that are required for implementation of a power domain driver for NVIDIA Tegra SoCs.
It is a continuation of [1], where Viresh Kumar asked to factor OPP patches into a separate series. I factored out the patches into this series, addressed the previous review comments and re-based patches on top of [2], which replaced some of my patches that added resource-managed helpers.
Quoted 7 lines
Quoted 2 lines
[1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=221130
[2] https://lore.kernel.org/linux-pm/[email protected]/Hi Dmitry,
I have applied 9 out of 12 patches already. Thanks.
Thanks, I checked that everything is applied properly using today's linux-next.
19.01.2021 20:35, Dmitry Osipenko пишет:
Quoted 16 lines
18.01.2021 14:46, Viresh Kumar пишет:
Quoted 15 lines · Dmitry Osipenko
Quoted 13 lines
Hi,
This series fixes problems and adds features to OPP API that are required for implementation of a power domain driver for NVIDIA Tegra SoCs.
It is a continuation of [1], where Viresh Kumar asked to factor OPP patches into a separate series. I factored out the patches into this series, addressed the previous review comments and re-based patches on top of [2], which replaced some of my patches that added resource-managed helpers.
[1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=221130
[2] https://lore.kernel.org/linux-pm/[email protected]/
Quoted 8 lines
Quoted 4 lines
Hi Dmitry,
I have applied 9 out of 12 patches already. Thanks.
Thanks, I checked that everything is applied properly using today's linux-next.
Turned out that one minor issue was actually introduced, the devm_pm_opp_attach_genpd() lost the export. I'll make a patch to fix this.
Quoted 22 lines · Dmitry Osipenko
19.01.2021 20:35, Dmitry Osipenko пишет:
Quoted 21 lines
18.01.2021 14:46, Viresh Kumar пишет:
Quoted 19 lines · Dmitry Osipenko
Quoted 5 lines
Hi,
This series fixes problems and adds features to OPP API that are required for implementation of a power domain driver for NVIDIA Tegra SoCs.
Quoted 8 lines
It is a continuation of [1], where Viresh Kumar asked to factor OPP patches into a separate series. I factored out the patches into this series, addressed the previous review comments and re-based patches on top of [2], which replaced some of my patches that added resource-managed helpers.
[1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=221130
[2] https://lore.kernel.org/linux-pm/[email protected]/Hi Dmitry,
I have applied 9 out of 12 patches already. Thanks.
Quoted 6 lines
Quoted 3 lines
Thanks, I checked that everything is applied properly using today's linux-next.
Turned out that one minor issue was actually introduced, the
devm_pm_opp_attach_genpd()lost the export. I'll make a patch to fix this.
I have fixed the original patch for that.
Signature · 1 line
viresh
21.01.2021 10:51, Viresh Kumar пишет:
Quoted 23 lines · Dmitry Osipenko
Quoted 22 lines
19.01.2021 20:35, Dmitry Osipenko пишет:
Quoted 21 lines
18.01.2021 14:46, Viresh Kumar пишет:
Quoted 19 lines · Dmitry Osipenko
Quoted 5 lines
Hi,
This series fixes problems and adds features to OPP API that are required for implementation of a power domain driver for NVIDIA Tegra SoCs.
Quoted 8 lines
It is a continuation of [1], where Viresh Kumar asked to factor OPP patches into a separate series. I factored out the patches into this series, addressed the previous review comments and re-based patches on top of [2], which replaced some of my patches that added resource-managed helpers.
[1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=221130
[2] https://lore.kernel.org/linux-pm/[email protected]/Hi Dmitry,
I have applied 9 out of 12 patches already. Thanks.
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Thanks, I checked that everything is applied properly using today's linux-next.
Turned out that one minor issue was actually introduced, the
devm_pm_opp_attach_genpd()lost the export. I'll make a patch to fix this.I have fixed the original patch for that.
Okay, thank you.