[PATCH v3 00/12] OPP API fixes and improvements

[PATCH v3 00/12] OPP API fixes and improvements

[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.

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/20210101165507.19486-1-tiny.windzz@gmail.com/

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(-)

-- 
2.29.2

[PATCH v3 01/12] opp: Fix adding OPP entries in a wrong order if rate is unavailable

[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 <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index dfc4208d3f87..48618ff3e99e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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);
-- 
2.29.2

Re: [PATCH v3 01/12] opp: Fix adding OPP entries in a wrong order if rate is unavailable

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> 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 <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index dfc4208d3f87..48618ff3e99e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -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);

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 ?

-- 
viresh

Re: [PATCH v3 01/12] opp: Fix adding OPP entries in a wrong order if rate is unavailable

[Dmitry Osipenko]

18.01.2021 10:44, Viresh Kumar пишет:
> On 18-01-21, 03:55, Dmitry Osipenko wrote:
>> 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 <pgwipeout@gmail.com>
>> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
>> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/opp/core.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index dfc4208d3f87..48618ff3e99e 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -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);
> 
> 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 02/12] opp: Filter out OPPs based on availability of a required-OPP

[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 <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 48618ff3e99e..7b4d07279638 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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;
 }
 
-- 
2.29.2

Re: [PATCH v3 02/12] opp: Filter out OPPs based on availability of a required-OPP

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> 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 <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 48618ff3e99e..7b4d07279638 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -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;
>  }

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.

-- 
viresh

[PATCH v3 03/12] opp: Correct debug message in _opp_add_static_v2()

[Dmitry Osipenko]

The debug message always prints rate=0 instead of a proper value, fix it.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 50df483c7dc3..63b16cdba5ea 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -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;
 	}
 
-- 
2.29.2

Re: [PATCH v3 03/12] opp: Correct debug message in _opp_add_static_v2()

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> The debug message always prints rate=0 instead of a proper value, fix it.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/of.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 50df483c7dc3..63b16cdba5ea 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -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;
>  	}
>  

Applied and added Fixes tag. Thanks.

-- 
viresh

[PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()

[Dmitry Osipenko]

Extend OPP API with dev_pm_opp_sync_regulators() function, which syncs
voltage state of regulators.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 45 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 7b4d07279638..99d18befc209 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2686,3 +2686,48 @@ void dev_pm_opp_remove_table(struct device *dev)
 	dev_pm_opp_put_opp_table(opp_table);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table);
+
+/**
+ * dev_pm_opp_sync_regulators() - Sync state of voltage regulators
+ * @dev:	device for which we do this operation
+ *
+ * Sync voltage state of the OPP table regulators.
+ *
+ * Return: 0 on success or a negative error value.
+ */
+int dev_pm_opp_sync_regulators(struct device *dev)
+{
+	struct opp_table *opp_table;
+	struct regulator *reg;
+	int i, ret = 0;
+
+	/* Device may not have OPP table */
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return 0;
+
+	/* Regulator may not be required for the device */
+	if (!opp_table->regulators)
+		goto put_table;
+
+	mutex_lock(&opp_table->lock);
+
+	/* Nothing to sync if voltage wasn't changed */
+	if (!opp_table->enabled)
+		goto unlock;
+
+	for (i = 0; i < opp_table->regulator_count; i++) {
+		reg = opp_table->regulators[i];
+		ret = regulator_sync_voltage(reg);
+		if (ret)
+			break;
+	}
+unlock:
+	mutex_unlock(&opp_table->lock);
+put_table:
+	/* Drop reference taken by _find_opp_table() */
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index c24bd34339d7..1c3a09cc8dcd 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cp
 int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
 void dev_pm_opp_remove_table(struct device *dev);
 void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
+int dev_pm_opp_sync_regulators(struct device *dev);
 #else
 static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
 {
@@ -398,6 +399,11 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask
 {
 }
 
+static inline int dev_pm_opp_sync_regulators(struct device *dev)
+{
+	return -ENOTSUPP;
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
2.29.2

Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2919 bytes --]

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
Reported-by: kernel test robot <lkp@intel.com>

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_voltage
   cc1: 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

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 45916 bytes --]

Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> Extend OPP API with dev_pm_opp_sync_regulators() function, which syncs
> voltage state of regulators.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c     | 45 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  6 ++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 7b4d07279638..99d18befc209 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2686,3 +2686,48 @@ void dev_pm_opp_remove_table(struct device *dev)
>  	dev_pm_opp_put_opp_table(opp_table);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table);
> +
> +/**
> + * dev_pm_opp_sync_regulators() - Sync state of voltage regulators
> + * @dev:	device for which we do this operation
> + *
> + * Sync voltage state of the OPP table regulators.
> + *
> + * Return: 0 on success or a negative error value.
> + */
> +int dev_pm_opp_sync_regulators(struct device *dev)
> +{
> +	struct opp_table *opp_table;
> +	struct regulator *reg;
> +	int i, ret = 0;
> +
> +	/* Device may not have OPP table */
> +	opp_table = _find_opp_table(dev);
> +	if (IS_ERR(opp_table))
> +		return 0;
> +
> +	/* Regulator may not be required for the device */
> +	if (!opp_table->regulators)
> +		goto put_table;
> +
> +	mutex_lock(&opp_table->lock);

What exactly do you need this lock for ?

> +
> +	/* Nothing to sync if voltage wasn't changed */
> +	if (!opp_table->enabled)
> +		goto unlock;
> +
> +	for (i = 0; i < opp_table->regulator_count; i++) {
> +		reg = opp_table->regulators[i];
> +		ret = regulator_sync_voltage(reg);
> +		if (ret)
> +			break;
> +	}
> +unlock:
> +	mutex_unlock(&opp_table->lock);
> +put_table:
> +	/* Drop reference taken by _find_opp_table() */
> +	dev_pm_opp_put_opp_table(opp_table);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index c24bd34339d7..1c3a09cc8dcd 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cp
>  int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
>  void dev_pm_opp_remove_table(struct device *dev);
>  void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
> +int dev_pm_opp_sync_regulators(struct device *dev);
>  #else
>  static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
>  {
> @@ -398,6 +399,11 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask
>  {
>  }
>  
> +static inline int dev_pm_opp_sync_regulators(struct device *dev)
> +{
> +	return -ENOTSUPP;
> +}
> +
>  #endif		/* CONFIG_PM_OPP */
>  
>  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
> -- 
> 2.29.2

-- 
viresh

Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()

[Dmitry Osipenko]

18.01.2021 11:20, Viresh Kumar пишет:
>> +int dev_pm_opp_sync_regulators(struct device *dev)
>> +{
>> +	struct opp_table *opp_table;
>> +	struct regulator *reg;
>> +	int i, ret = 0;
>> +
>> +	/* Device may not have OPP table */
>> +	opp_table = _find_opp_table(dev);
>> +	if (IS_ERR(opp_table))
>> +		return 0;
>> +
>> +	/* Regulator may not be required for the device */
>> +	if (!opp_table->regulators)
>> +		goto put_table;
>> +
>> +	mutex_lock(&opp_table->lock);
> What exactly do you need this lock for ?

It is needed for protecting simultaneous invocations of
dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage().

The sync_regulators() should be invoked only after completion of the
set_voltage() in a case of Tegra power domain driver since potentially
both could be running in parallel. For example device driver may be
changing performance state in a work thread, while PM domain state is
syncing.

But maybe it will be better to move the protection to the PM driver
since we're focused on sync_regulators() and set_voltage() here, but
there are other OPP API functions which use regulators. I'll need to
take a closer look at it.

Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()

[Viresh Kumar]

On 18-01-21, 21:35, Dmitry Osipenko wrote:
> 18.01.2021 11:20, Viresh Kumar пишет:
> >> +int dev_pm_opp_sync_regulators(struct device *dev)
> >> +{
> >> +	struct opp_table *opp_table;
> >> +	struct regulator *reg;
> >> +	int i, ret = 0;
> >> +
> >> +	/* Device may not have OPP table */
> >> +	opp_table = _find_opp_table(dev);
> >> +	if (IS_ERR(opp_table))
> >> +		return 0;
> >> +
> >> +	/* Regulator may not be required for the device */
> >> +	if (!opp_table->regulators)
> >> +		goto put_table;
> >> +
> >> +	mutex_lock(&opp_table->lock);
> > What exactly do you need this lock for ?
> 
> It is needed for protecting simultaneous invocations of
> dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage().
> 
> The sync_regulators() should be invoked only after completion of the
> set_voltage() in a case of Tegra power domain driver since potentially
> both could be running in parallel. For example device driver may be
> changing performance state in a work thread, while PM domain state is
> syncing.

I think just checking the 'enabled' flag should be enough here (you may need a
lock for it though, but the lock should cover only the area it is supposed to
cover and nothing else.

> 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.

-- 
viresh

Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()

[Dmitry Osipenko]

19.01.2021 07:58, Viresh Kumar пишет:
> On 18-01-21, 21:35, Dmitry Osipenko wrote:
>> 18.01.2021 11:20, Viresh Kumar пишет:
>>>> +int dev_pm_opp_sync_regulators(struct device *dev)
>>>> +{
>>>> +	struct opp_table *opp_table;
>>>> +	struct regulator *reg;
>>>> +	int i, ret = 0;
>>>> +
>>>> +	/* Device may not have OPP table */
>>>> +	opp_table = _find_opp_table(dev);
>>>> +	if (IS_ERR(opp_table))
>>>> +		return 0;
>>>> +
>>>> +	/* Regulator may not be required for the device */
>>>> +	if (!opp_table->regulators)
>>>> +		goto put_table;
>>>> +
>>>> +	mutex_lock(&opp_table->lock);
>>> What exactly do you need this lock for ?
>>
>> It is needed for protecting simultaneous invocations of
>> dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage().
>>
>> The sync_regulators() should be invoked only after completion of the
>> set_voltage() in a case of Tegra power domain driver since potentially
>> both could be running in parallel. For example device driver may be
>> changing performance state in a work thread, while PM domain state is
>> syncing.
> 
> I think just checking the 'enabled' flag should be enough here (you may need a
> lock for it though, but the lock should cover only the area it is supposed to
> cover and nothing else.

I'll remove the locks from these OPP patches and move them to the PD
driver. It should be the best option right now since OPP API isn't
entirely thread-safe, making it thread-safe should be a separate topic.

Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> Extend OPP API with dev_pm_opp_sync_regulators() function, which syncs
> voltage state of regulators.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c     | 45 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  6 ++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 7b4d07279638..99d18befc209 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2686,3 +2686,48 @@ void dev_pm_opp_remove_table(struct device *dev)
>  	dev_pm_opp_put_opp_table(opp_table);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table);
> +
> +/**
> + * dev_pm_opp_sync_regulators() - Sync state of voltage regulators
> + * @dev:	device for which we do this operation
> + *
> + * Sync voltage state of the OPP table regulators.
> + *
> + * Return: 0 on success or a negative error value.
> + */
> +int dev_pm_opp_sync_regulators(struct device *dev)
> +{
> +	struct opp_table *opp_table;
> +	struct regulator *reg;
> +	int i, ret = 0;
> +
> +	/* Device may not have OPP table */
> +	opp_table = _find_opp_table(dev);
> +	if (IS_ERR(opp_table))
> +		return 0;
> +
> +	/* Regulator may not be required for the device */
> +	if (!opp_table->regulators)
> +		goto put_table;
> +
> +	mutex_lock(&opp_table->lock);
> +
> +	/* Nothing to sync if voltage wasn't changed */
> +	if (!opp_table->enabled)
> +		goto unlock;
> +
> +	for (i = 0; i < opp_table->regulator_count; i++) {
> +		reg = opp_table->regulators[i];
> +		ret = regulator_sync_voltage(reg);
> +		if (ret)
> +			break;
> +	}
> +unlock:
> +	mutex_unlock(&opp_table->lock);
> +put_table:
> +	/* Drop reference taken by _find_opp_table() */
> +	dev_pm_opp_put_opp_table(opp_table);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index c24bd34339d7..1c3a09cc8dcd 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cp
>  int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
>  void dev_pm_opp_remove_table(struct device *dev);
>  void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
> +int dev_pm_opp_sync_regulators(struct device *dev);
>  #else
>  static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
>  {
> @@ -398,6 +399,11 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask
>  {
>  }
>  
> +static inline int dev_pm_opp_sync_regulators(struct device *dev)
> +{
> +	return -ENOTSUPP;
> +}
> +
>  #endif		/* CONFIG_PM_OPP */
>  
>  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)

Applied. Thanks.

I had to apply it manually, please make sure it looks okay.

-- 
viresh

Re: [PATCH v3 04/12] opp: Add dev_pm_opp_sync_regulators()

[Viresh Kumar]

On 18-01-21, 16:30, Viresh Kumar wrote:
> On 18-01-21, 03:55, Dmitry Osipenko wrote:
> > Extend OPP API with dev_pm_opp_sync_regulators() function, which syncs
> > voltage state of regulators.
> > 
> > Tested-by: Peter Geis <pgwipeout@gmail.com>
> > Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> > Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > ---
> >  drivers/opp/core.c     | 45 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pm_opp.h |  6 ++++++
> >  2 files changed, 51 insertions(+)
> > 
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 7b4d07279638..99d18befc209 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -2686,3 +2686,48 @@ void dev_pm_opp_remove_table(struct device *dev)
> >  	dev_pm_opp_put_opp_table(opp_table);
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table);
> > +
> > +/**
> > + * dev_pm_opp_sync_regulators() - Sync state of voltage regulators
> > + * @dev:	device for which we do this operation
> > + *
> > + * Sync voltage state of the OPP table regulators.
> > + *
> > + * Return: 0 on success or a negative error value.
> > + */
> > +int dev_pm_opp_sync_regulators(struct device *dev)
> > +{
> > +	struct opp_table *opp_table;
> > +	struct regulator *reg;
> > +	int i, ret = 0;
> > +
> > +	/* Device may not have OPP table */
> > +	opp_table = _find_opp_table(dev);
> > +	if (IS_ERR(opp_table))
> > +		return 0;
> > +
> > +	/* Regulator may not be required for the device */
> > +	if (!opp_table->regulators)
> > +		goto put_table;
> > +
> > +	mutex_lock(&opp_table->lock);
> > +
> > +	/* Nothing to sync if voltage wasn't changed */
> > +	if (!opp_table->enabled)
> > +		goto unlock;
> > +
> > +	for (i = 0; i < opp_table->regulator_count; i++) {
> > +		reg = opp_table->regulators[i];
> > +		ret = regulator_sync_voltage(reg);
> > +		if (ret)
> > +			break;
> > +	}
> > +unlock:
> > +	mutex_unlock(&opp_table->lock);
> > +put_table:
> > +	/* Drop reference taken by _find_opp_table() */
> > +	dev_pm_opp_put_opp_table(opp_table);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
> > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> > index c24bd34339d7..1c3a09cc8dcd 100644
> > --- a/include/linux/pm_opp.h
> > +++ b/include/linux/pm_opp.h
> > @@ -162,6 +162,7 @@ int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cp
> >  int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
> >  void dev_pm_opp_remove_table(struct device *dev);
> >  void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
> > +int dev_pm_opp_sync_regulators(struct device *dev);
> >  #else
> >  static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
> >  {
> > @@ -398,6 +399,11 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask
> >  {
> >  }
> >  
> > +static inline int dev_pm_opp_sync_regulators(struct device *dev)
> > +{
> > +	return -ENOTSUPP;
> > +}
> > +
> >  #endif		/* CONFIG_PM_OPP */
> >  
> >  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
> 
> Applied. Thanks.
> 
> I had to apply it manually, please make sure it looks okay.

Sorry about this, I wanted to reply to
"opp: Add devm_pm_opp_register_set_opp_helper" and replied to this one
accidentally.

-- 
viresh

[PATCH v3 05/12] opp: Add dev_pm_opp_set_voltage()

[Dmitry Osipenko]

Add 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.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  6 +++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 99d18befc209..341484d58e6c 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 1c3a09cc8dcd..f344be844bde 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -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)
-- 
2.29.2

Re: [PATCH v3 05/12] opp: Add dev_pm_opp_set_voltage()

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 99d18befc209..341484d58e6c 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -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)

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 ?

-- 
viresh

Re: [PATCH v3 05/12] opp: Add dev_pm_opp_set_voltage()

[Dmitry Osipenko]

18.01.2021 12:52, Viresh Kumar пишет:
> On 18-01-21, 03:55, Dmitry Osipenko wrote:
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 99d18befc209..341484d58e6c 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -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)
> 
> 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 ?
> 

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()

[Dmitry Osipenko]

18.01.2021 22:14, Dmitry Osipenko пишет:
> 18.01.2021 12:52, Viresh Kumar пишет:
>> On 18-01-21, 03:55, Dmitry Osipenko wrote:
>>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>>> index 99d18befc209..341484d58e6c 100644
>>> --- a/drivers/opp/core.c
>>> +++ b/drivers/opp/core.c
>>> @@ -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)
>>
>> 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 ?
>>
> 
> 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.
> 

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()

[Viresh Kumar]

On 21-01-21, 00:57, Dmitry Osipenko wrote:
> 18.01.2021 22:14, Dmitry Osipenko пишет:
> > 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.

Yes.
 
> 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!

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.

-- 
viresh

[PATCH v3 06/12] opp: Add dev_pm_opp_find_level_ceil()

[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 <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 49 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  8 +++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 341484d58e6c..df0969002555 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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)
 {
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index f344be844bde..b7dc993487c7 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -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)
 {
-- 
2.29.2

Re: [PATCH v3 06/12] opp: Add dev_pm_opp_find_level_ceil()

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> 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 <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c     | 49 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  8 +++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 341484d58e6c..df0969002555 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -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)
>  {
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index f344be844bde..b7dc993487c7 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -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)
>  {

Applied. Thanks.

-- 
viresh

[PATCH v3 07/12] opp: Add dev_pm_opp_get_required_pstate()

[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 <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 22 ++++++++++++++++++++++
 include/linux/pm_opp.h | 10 ++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index df0969002555..fde2ec00ab0e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index b7dc993487c7..e072148ae0e1 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -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;
-- 
2.29.2

Re: [PATCH v3 07/12] opp: Add dev_pm_opp_get_required_pstate()

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> Add dev_pm_opp_get_required_pstate() which allows OPP users to retrieve
> required performance state of a given OPP.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c     | 22 ++++++++++++++++++++++
>  include/linux/pm_opp.h | 10 ++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index df0969002555..fde2ec00ab0e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -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
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index b7dc993487c7..e072148ae0e1 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -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;

Applied. Thanks.

-- 
viresh

[PATCH v3 08/12] opp: Add devm_pm_opp_register_set_opp_helper

[Dmitry Osipenko]

Add resource-managed version of dev_pm_opp_register_set_opp_helper().

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 34 ++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  8 ++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index fde2ec00ab0e..8e0d2193fd5f 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index e072148ae0e1..6de5853aaada 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -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)
-- 
2.29.2

Re: [PATCH v3 08/12] opp: Add devm_pm_opp_register_set_opp_helper

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> Add resource-managed version of dev_pm_opp_register_set_opp_helper().
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c     | 34 ++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  8 ++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index fde2ec00ab0e..8e0d2193fd5f 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -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);
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index e072148ae0e1..6de5853aaada 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -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)

Applied. Thanks.

I had to apply it manually, please make sure it works fine.

-- 
viresh

[PATCH v3 09/12] opp: Add devm_pm_opp_attach_genpd

[Dmitry Osipenko]

Add resource-managed version of dev_pm_opp_attach_genpd().

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 35 +++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  8 ++++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 8e0d2193fd5f..49419ab9fbb4 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 6de5853aaada..eefd0b15890c 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -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)
-- 
2.29.2

Re: [PATCH v3 09/12] opp: Add devm_pm_opp_attach_genpd

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> Add resource-managed version of dev_pm_opp_attach_genpd().
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c     | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  8 ++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 8e0d2193fd5f..49419ab9fbb4 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -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);
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 6de5853aaada..eefd0b15890c 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -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)

Manually applied. Thanks.

-- 
viresh

[PATCH v3 10/12] opp: Support set_opp() customization without requiring to use regulators

[Dmitry Osipenko]

Support 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().

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c     | 46 +++++++++++++++++++++++++++++++++++-------
 include/linux/pm_opp.h |  3 +++
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 49419ab9fbb4..7726c4c40b53 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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);
 }
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index eefd0b15890c..c98fd2add563 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -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)
-- 
2.29.2

Re: [PATCH v3 10/12] opp: Support set_opp() customization without requiring to use regulators

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index eefd0b15890c..c98fd2add563 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -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;
>  };

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.

-- 
viresh

Re: [PATCH v3 10/12] opp: Support set_opp() customization without requiring to use regulators

[Dmitry Osipenko]

18.01.2021 14:44, Viresh Kumar пишет:
> On 18-01-21, 03:55, Dmitry Osipenko wrote:
>> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>> index eefd0b15890c..c98fd2add563 100644
>> --- a/include/linux/pm_opp.h
>> +++ b/include/linux/pm_opp.h
>> @@ -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;
>>  };
> 
> 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.
> 

Could you please show a code sample?

[PATCH] opp: Prepare for ->set_opp() helper to work without regulators

[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 from
dev_pm_opp_register_set_opp_helper().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
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(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index b1a2296f3065..f80b6f1ca108 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 4ced7ffa8158..4408cfcb0f31 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -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
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators

[Dmitry Osipenko]

19.01.2021 09:35, Viresh Kumar пишет:
> +	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

[Viresh Kumar]

On 19-01-21, 20:16, Dmitry Osipenko wrote:
> 19.01.2021 09:35, Viresh Kumar пишет:
> > +	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.

> 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 ?

> At least both sod_supplies and set_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.

-- 
viresh

Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators

[Dmitry Osipenko]

20.01.2021 10:39, Viresh Kumar пишет:
> On 19-01-21, 20:16, Dmitry Osipenko wrote:
>> 19.01.2021 09:35, Viresh Kumar пишет:
>>> +	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.
> 
>> 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 ?
> 
>> At least both sod_supplies and set_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.
> 
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

[Viresh Kumar]

On 20-01-21, 17:50, Dmitry Osipenko wrote:
> 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().

Right, I will fix that.

> 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.

> 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.

-- 
viresh

Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators

[Dmitry Osipenko]

19.01.2021 09:35, 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 from
> dev_pm_opp_register_set_opp_helper().
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> 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.

Tested-by: Dmitry Osipenko <digetx@gmail.com>

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

[Viresh Kumar]

On 19-01-21, 12:05, Viresh Kumar wrote:
> 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().
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> 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.

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 1ed673334565..1dc5ca3f6d26 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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;
        }

-- 
viresh

Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators

[Viresh Kumar]

On 20-01-21, 13:38, Viresh Kumar wrote:
> On 19-01-21, 12:05, Viresh Kumar wrote:
> > 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().
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > 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.
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 1ed673334565..1dc5ca3f6d26 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -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;
>         }

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
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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);

-- 
viresh

Re: [PATCH v3 10/12] opp: Support set_opp() customization without requiring to use regulators

[Viresh Kumar]

On 18-01-21, 21:48, Dmitry Osipenko wrote:
> 18.01.2021 14:44, Viresh Kumar пишет:
> > On 18-01-21, 03:55, Dmitry Osipenko wrote:
> >> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> >> index eefd0b15890c..c98fd2add563 100644
> >> --- a/include/linux/pm_opp.h
> >> +++ b/include/linux/pm_opp.h
> >> @@ -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;
> >>  };
> > 
> > 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.
> > 
> 
> Could you please show a code sample?

Sent a patch to you.

-- 
viresh

[PATCH V2] opp: Prepare for ->set_opp() helper to work without regulators

[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 from
dev_pm_opp_register_set_opp_helper().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
---
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(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 846390c9434a..dff939ed5118 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 4ced7ffa8158..4408cfcb0f31 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -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
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH V2] opp: Prepare for ->set_opp() helper to work without regulators

[Dmitry Osipenko]

21.01.2021 14:30, Viresh Kumar пишет:
> @@ -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;

The sod_supplies should be unset under the lock.

Re: [PATCH V2] opp: Prepare for ->set_opp() helper to work without regulators

[Viresh Kumar]

On 22-01-21, 00:02, Dmitry Osipenko wrote:
> 21.01.2021 14:30, Viresh Kumar пишет:
> > @@ -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;
> 
> The sod_supplies should be unset under the lock.

Fixed, thanks.

-- 
viresh

[PATCH v3 11/12] opp: Handle missing OPP table in dev_pm_opp_xlate_performance_state()

[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 <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 7726c4c40b53..ca8c6acc29f4 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -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++) {
-- 
2.29.2

Re: [PATCH v3 11/12] opp: Handle missing OPP table in dev_pm_opp_xlate_performance_state()

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> 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 <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 7726c4c40b53..ca8c6acc29f4 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -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.

-- 
viresh

[PATCH v3 12/12] opp: Print OPP level in debug message of _opp_add_static_v2()

[Dmitry Osipenko]

Print OPP level in debug message of _opp_add_static_v2(). This helps to
chase GENPD bugs.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Tested-by: Matt Merhar <mattmerhar@protonmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/of.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 63b16cdba5ea..758730d070da 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -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
-- 
2.29.2

Re: [PATCH v3 12/12] opp: Print OPP level in debug message of _opp_add_static_v2()

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> Print OPP level in debug message of _opp_add_static_v2(). This helps to
> chase GENPD bugs.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Tested-by: Matt Merhar <mattmerhar@protonmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/of.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 63b16cdba5ea..758730d070da 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -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

Applied. Thanks.

-- 
viresh

Re: [PATCH v3 00/12] OPP API fixes and improvements

[Viresh Kumar]

On 18-01-21, 03:55, Dmitry Osipenko wrote:
> 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/20210101165507.19486-1-tiny.windzz@gmail.com/

Hi Dmitry,

I have applied 9 out of 12 patches already. Thanks.

-- 
viresh

Re: [PATCH v3 00/12] OPP API fixes and improvements

[Dmitry Osipenko]

18.01.2021 14:46, Viresh Kumar пишет:
> On 18-01-21, 03:55, Dmitry Osipenko wrote:
>> 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/20210101165507.19486-1-tiny.windzz@gmail.com/
> 
> 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.

Re: [PATCH v3 00/12] OPP API fixes and improvements

[Dmitry Osipenko]

19.01.2021 20:35, Dmitry Osipenko пишет:
> 18.01.2021 14:46, Viresh Kumar пишет:
>> On 18-01-21, 03:55, Dmitry Osipenko wrote:
>>> 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/20210101165507.19486-1-tiny.windzz@gmail.com/
>>
>> 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.

Re: [PATCH v3 00/12] OPP API fixes and improvements

[Viresh Kumar]

On 20-01-21, 18:41, Dmitry Osipenko wrote:
> 19.01.2021 20:35, Dmitry Osipenko пишет:
> > 18.01.2021 14:46, Viresh Kumar пишет:
> >> On 18-01-21, 03:55, Dmitry Osipenko wrote:
> >>> 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/20210101165507.19486-1-tiny.windzz@gmail.com/
> >>
> >> 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.

I have fixed the original patch for that.

-- 
viresh

Re: [PATCH v3 00/12] OPP API fixes and improvements

[Dmitry Osipenko]

21.01.2021 10:51, Viresh Kumar пишет:
> On 20-01-21, 18:41, Dmitry Osipenko wrote:
>> 19.01.2021 20:35, Dmitry Osipenko пишет:
>>> 18.01.2021 14:46, Viresh Kumar пишет:
>>>> On 18-01-21, 03:55, Dmitry Osipenko wrote:
>>>>> 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/20210101165507.19486-1-tiny.windzz@gmail.com/
>>>>
>>>> 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.
> 
> I have fixed the original patch for that.
> 

Okay, thank you.