OPP API fixes and improvements
v4 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:
v4: - Fixed missing export of devm_pm_opp_attach_genpd(). Thanks to
Nicolas Chauvet for catching this problem.
- Removed a bit questionable locking from dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage(). We may come back to the OPP locking sometime later, it's not an essential problem right now. I moved the lock from OPP core to the PD driver for now.
- Added "Make _set_opp_custom() work without regulators" patch, which is made on top of "Prepare for ->set_opp() helper to work without regulators" patch from Viresh Kumar. The set_opp() helper now works without regulators.
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 (4):
opp: Export devm_pm_opp_attach_genpd()
opp: Add dev_pm_opp_sync_regulators()
opp: Add dev_pm_opp_set_voltage()
opp: Make _set_opp_custom() work without regulators
drivers/opp/core.c | 119 +++++++++++++++++++++++++++++++++++++---- include/linux/pm_opp.h | 12 +++++ 2 files changed, 122 insertions(+), 9 deletions(-)
Signature · 1 line
2.29.2
PATCH v4 1/4 opp: Export devm_pm_opp_attach_genpd()
The devm_pm_opp_attach_genpd() lost its export by accident when it was merged. Re-add the missing export, Tegra DRM driver now works when compiled as a loadable kernel module since it uses the exported function.
--- drivers/opp/core.c | 1 + 1 file changed, 1 insertion(+)
drivers/opp/core.c +1 -0
@@ -2297,6 +2297,7 @@ devm_pm_opp_attach_genpd(struct device *dev, const char **names,return opp_table; } +EXPORT_SYMBOL_GPL (devm_pm_opp_attach_genpd);/** * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table.
Signature · 1 line
2.29.2
PATCH v4 2/4 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 | 41 +++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 ++++++ 2 files changed, 47 insertions(+)
drivers/opp/core.c +41 -0
@@ -2659,3 +2659,44 @@ 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; + +/* Nothing to sync if voltage wasn't changed */ +if (!opp_table->enabled) + goto put_table; + +for (i =0 ; i < opp_table->regulator_count; i++) { + reg = opp_table->regulators[i]; + ret =regulator_sync_voltage (reg); +if (ret) +break ; + } +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
@@ -163,6 +163,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) {
@@ -399,6 +400,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
Re: [PATCH v4 2/4] opp: Add dev_pm_opp_sync_regulators()
21.01.2021 01:26, Dmitry Osipenko пишет:
Quoted 11 lines
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 | 41 +++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 ++++++ 2 files changed, 47 insertions(+)
Hello Viresh,
This is the last patch that is left unmerged from this series. We will need it for implementation of the PD driver for NVIDIA Tegra20/30 SoCs. Please consider applying it, thanks in advance!
Re: [PATCH v4 2/4] opp: Add dev_pm_opp_sync_regulators()
Quoted 13 lines · Dmitry Osipenko
21.01.2021 01:26, Dmitry Osipenko пишет:
Quoted 11 lines
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 | 41 +++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 ++++++ 2 files changed, 47 insertions(+)
Quoted 5 lines
Hello Viresh,
This is the last patch that is left unmerged from this series. We will need it for implementation of the PD driver for NVIDIA Tegra20/30 SoCs. Please consider applying it, thanks in advance!
I have replied to 0/4 earlier giving the reason why I haven't applied this.
Signature · 1 line
viresh
PATCH v4 3/4 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 | 51 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 +++++ 2 files changed, 57 insertions(+)
drivers/opp/core.c +51 -0
@@ -2700,3 +2700,54 @@ 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; + } + + 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 put_table; + } + + opp_table->enabled = true; + } +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
@@ -164,6 +164,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) {
@@ -405,6 +406,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
PATCH v4 4/4 opp: Make _set_opp_custom() work without regulators
In reply to Dmitry OsipenkoCheck whether OPP table has regulators in _set_opp_custom() and set up dev_pm_set_opp_data accordingly. Now _set_opp_custom() works properly, i.e. it doesn't crash if OPP table doesn't have assigned regulators.
--- drivers/opp/core.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
drivers/opp/core.c +17 -9
@@ -829,23 +829,31 @@ 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, regulator_count; + +if (opp_table->sod_supplies) + regulator_count = opp_table->regulator_count; +else + regulator_count =0 ; data = opp_table->set_opp_data; data->regulators = opp_table->regulators; - data->regulator_count = opp_table->regulator_count; + data->regulator_count = regulator_count; data->clk = opp_table->clk; data->dev = dev; data->old_opp.rate = old_freq; - size =sizeof (*old_supply) * opp_table->regulator_count; -if (!old_supply) -memset (data->old_opp.supplies,0 , size); -else -memcpy (data->old_opp.supplies, old_supply, size); - data->new_opp.rate = freq; -memcpy (data->new_opp.supplies, new_supply, size); + +if (regulator_count) { + size =sizeof (*old_supply) * opp_table->regulator_count; +if (!old_supply) +memset (data->old_opp.supplies,0 , size); +else +memcpy (data->old_opp.supplies, old_supply, size); + +memcpy (data->new_opp.supplies, new_supply, size); + }return opp_table->set_opp (data); }
Signature · 1 line
2.29.2
Re: [PATCH v4 4/4] opp: Make _set_opp_custom() work without regulators
Quoted 8 lines · Dmitry Osipenko
Check whether OPP table has regulators in
_set_opp_custom()and set updev_pm_set_opp_dataaccordingly. Now_set_opp_custom()works properly, i.e. it doesn't crash if OPP table doesn't have assigned regulators.Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
I have applied this instead:
drivers/opp/core.c +18 -11
@@ -828,24 +828,31 @@ static int _set_opp_custom(const struct opp_table *opp_table,struct dev_pm_opp_supply *old_supply, struct dev_pm_opp_supply *new_supply) { - struct dev_pm_set_opp_data *data; + struct dev_pm_set_opp_data *data = opp_table->set_opp_data;int size; - data = opp_table->set_opp_data; +/* +* We support this only if dev_pm_opp_set_regulators() was called +* earlier. +*/ +if (opp_table->sod_supplies) { + size =sizeof (*old_supply) * opp_table->regulator_count; +if (!old_supply) +memset (data->old_opp.supplies,0 , size); +else +memcpy (data->old_opp.supplies, old_supply, size); + +memcpy (data->new_opp.supplies, new_supply, size); + data->regulator_count = opp_table->regulator_count; + }else { + data->regulator_count =0 ; + } + data->regulators = opp_table->regulators; - data->regulator_count = opp_table->regulator_count; data->clk = opp_table->clk; data->dev = dev; - data->old_opp.rate = old_freq; - size =sizeof (*old_supply) * opp_table->regulator_count; -if (!old_supply) -memset (data->old_opp.supplies,0 , size); -else -memcpy (data->old_opp.supplies, old_supply, size); - data->new_opp.rate = freq; -memcpy (data->new_opp.supplies, new_supply, size);return opp_table->set_opp (data); }
Signature · 1 line
viresh
Re: [PATCH v4 4/4] opp: Make _set_opp_custom() work without regulators
22.01.2021 10:26, Viresh Kumar пишет:
Quoted 51 lines · Dmitry Osipenko
Quoted 8 lines
Check whether OPP table has regulators in
_set_opp_custom()and set updev_pm_set_opp_dataaccordingly. Now_set_opp_custom()works properly, i.e. it doesn't crash if OPP table doesn't have assigned regulators.Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)I have applied this instead:
drivers/opp/core.c +18
-5@@ -828,24 +828,31 @@ static int _set_opp_custom(const struct opp_table *opp_table,struct dev_pm_opp_supply *old_supply, struct dev_pm_opp_supply *new_supply) { - struct dev_pm_set_opp_data *data; + struct dev_pm_set_opp_data *data = opp_table->set_opp_data;int size; - data = opp_table->set_opp_data; +/* +* We support this only if dev_pm_opp_set_regulators() was called +* earlier. +*/ +if (opp_table->sod_supplies) { + size =sizeof (*old_supply) * opp_table->regulator_count; +if (!old_supply) +memset (data->old_opp.supplies,0 , size); +else +memcpy (data->old_opp.supplies, old_supply, size); + +memcpy (data->new_opp.supplies, new_supply, size); + data->regulator_count = opp_table->regulator_count; + }else { + data->regulator_count =0 ; + } + data->regulators = opp_table->regulators; - data->regulator_count = opp_table->regulator_count; data->clk = opp_table->clk; data->dev = dev; - data->old_opp.rate = old_freq; - size =sizeof (*old_supply) * opp_table->regulator_count;
drivers/opp/core.c static int _set_opp_custom(const struct opp_table *opp_table, · line 858
Quoted 10 lines
drivers/opp/core.c +0
-6-if (!old_supply) -memset (data->old_opp.supplies,0 , size); -else -memcpy (data->old_opp.supplies, old_supply, size); - data->new_opp.rate = freq; -memcpy (data->new_opp.supplies, new_supply, size);return opp_table->set_opp (data); }
I replied to the wrong email, but this also looks good :) Thanks!
Re: [PATCH v4 4/4] opp: Make _set_opp_custom() work without regulators
In reply to Dmitry Osipenko21.01.2021 01:26, Dmitry Osipenko пишет:
Quoted 45 lines
Check whether OPP table has regulators in
_set_opp_custom()and set updev_pm_set_opp_dataaccordingly. Now_set_opp_custom()works properly, i.e. it doesn't crash if OPP table doesn't have assigned regulators.Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/opp/core.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)drivers/opp/core.c +10
-9@@ -829,23 +829,31 @@ 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, regulator_count; + +if (opp_table->sod_supplies) + regulator_count = opp_table->regulator_count; +else + regulator_count =0 ; data = opp_table->set_opp_data; data->regulators = opp_table->regulators; - data->regulator_count = opp_table->regulator_count; + data->regulator_count = regulator_count; data->clk = opp_table->clk; data->dev = dev; data->old_opp.rate = old_freq; - size =sizeof (*old_supply) * opp_table->regulator_count; -if (!old_supply) -memset (data->old_opp.supplies,0 , size); -else -memcpy (data->old_opp.supplies, old_supply, size); - data->new_opp.rate = freq; -memcpy (data->new_opp.supplies, new_supply, size); + +if (regulator_count) { + size =sizeof (*old_supply) * opp_table->regulator_count;
drivers/opp/core.c static int _set_opp_custom(const struct opp_table *opp_table, · line 859
Quoted 10 lines
drivers/opp/core.c +7
-0+if (!old_supply) +memset (data->old_opp.supplies,0 , size); +else +memcpy (data->old_opp.supplies, old_supply, size); + +memcpy (data->new_opp.supplies, new_supply, size); + }return opp_table->set_opp (data); }
Looks good, thank you!
Quoted 5 lines · Dmitry Osipenko
Dmitry Osipenko (4):
opp: Exportdevm_pm_opp_attach_genpd()
opp: Adddev_pm_opp_sync_regulators()
opp: Adddev_pm_opp_set_voltage()
opp: Make_set_opp_custom()work without regulators
Patch 1 and 3 aren't required anymore, I have applied 4th already. Patch 2 can be applied only after the dependency patch for the stub definition gets merge in Linus's tree. I see that Mark has already queued that up in his for-5.11 branch, so it might be part of next -rc. I will apply 2nd patch then.
Signature · 1 line
viresh
Quoted 4 lines · Viresh Kumar
Patch 2 can be applied only after the dependency patch for the stub definition gets merge in Linus's tree. I see that Mark has already queued that up in his for-5.11 branch, so it might be part of next -rc. I will apply 2nd patch then.
The consumer.h patch landed into Linus's tree this morning and hence I have applied the last patch 2/4 from this series as well. Thanks.
Signature · 1 line
viresh