[PATCH v4 0/4] OPP API fixes and improvements

[PATCH v4 0/4] 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:

v4: - Fixed missing export of devm_pm_opp_attach_genpd(). Thanks to
      Nicolas Chauvet for catching this problem.

    - Removed a bit questionable locking from dev_pm_opp_sync_regulators()
      and dev_pm_opp_set_voltage(). We may come back to the OPP locking
      sometime later, it's not an essential problem right now. I moved
      the lock from OPP core to the PD driver for now.

    - Added "Make _set_opp_custom() work without regulators" patch,
      which is made on top of "Prepare for ->set_opp() helper to work without
      regulators" patch from Viresh Kumar. The set_opp() helper now works
      without regulators.

v3: - Reordered patches by importance.

    - Added locking to dev_pm_opp_set_voltage().

    - Reworked "Fix adding OPP entries in a wrong order if rate is unavailable"
      patch, like it was suggested by Viresh Kumar.

    - Reworked "Support set_opp() customization without requiring to use
      regulators" patch, like it was suggested by Viresh Kumar.

      The opp_table->set_opp_data is now allocated by dev_pm_opp_register_set_opp_helper().

      The set_opp_data is refcounted now and can be allocated by any other
      OPP functions if this will become needed in the future for other OPP API
      changes.

Dmitry Osipenko (4):
  opp: Export devm_pm_opp_attach_genpd()
  opp: Add dev_pm_opp_sync_regulators()
  opp: Add dev_pm_opp_set_voltage()
  opp: Make _set_opp_custom() work without regulators

 drivers/opp/core.c     | 119 +++++++++++++++++++++++++++++++++++++----
 include/linux/pm_opp.h |  12 +++++
 2 files changed, 122 insertions(+), 9 deletions(-)

-- 
2.29.2

[PATCH v4 1/4] opp: Export devm_pm_opp_attach_genpd()

[Dmitry Osipenko]

The devm_pm_opp_attach_genpd() lost its export by accident when it was
merged. Re-add the missing export, Tegra DRM driver now works when
compiled as a loadable kernel module since it uses the exported function.

Fixes: 22300b8fd92a ("opp: Add devm_pm_opp_attach_genpd")
Reported-by: Nicolas Chauvet <kwizart@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index f80b6f1ca108..6049b17f99d6 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2297,6 +2297,7 @@ devm_pm_opp_attach_genpd(struct device *dev, const char **names,
 
 	return opp_table;
 }
+EXPORT_SYMBOL_GPL(devm_pm_opp_attach_genpd);
 
 /**
  * dev_pm_opp_xlate_performance_state() - Find required OPP's pstate for src_table.
-- 
2.29.2

[PATCH v4 2/4] 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     | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 6049b17f99d6..43e62e0e3d5b 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2659,3 +2659,44 @@ void dev_pm_opp_remove_table(struct device *dev)
 	dev_pm_opp_put_opp_table(opp_table);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_remove_table);
+
+/**
+ * dev_pm_opp_sync_regulators() - Sync state of voltage regulators
+ * @dev:	device for which we do this operation
+ *
+ * Sync voltage state of the OPP table regulators.
+ *
+ * Return: 0 on success or a negative error value.
+ */
+int dev_pm_opp_sync_regulators(struct device *dev)
+{
+	struct opp_table *opp_table;
+	struct regulator *reg;
+	int i, ret = 0;
+
+	/* Device may not have OPP table */
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return 0;
+
+	/* Regulator may not be required for the device */
+	if (!opp_table->regulators)
+		goto put_table;
+
+	/* Nothing to sync if voltage wasn't changed */
+	if (!opp_table->enabled)
+		goto put_table;
+
+	for (i = 0; i < opp_table->regulator_count; i++) {
+		reg = opp_table->regulators[i];
+		ret = regulator_sync_voltage(reg);
+		if (ret)
+			break;
+	}
+put_table:
+	/* Drop reference taken by _find_opp_table() */
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index f8e9a8e3eb59..80cdb9af8268 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -163,6 +163,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)
 {
@@ -399,6 +400,11 @@ static inline void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask
 {
 }
 
+static inline int dev_pm_opp_sync_regulators(struct device *dev)
+{
+	return -ENOTSUPP;
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
2.29.2

[PATCH v4 3/4] 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     | 51 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  6 +++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 43e62e0e3d5b..08d205a55c07 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2700,3 +2700,54 @@ int dev_pm_opp_sync_regulators(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
+
+/**
+ * dev_pm_opp_set_voltage() - Change voltage of regulators
+ * @dev:	device for which we do this operation
+ * @opp:	opp based on which the voltages are to be configured
+ *
+ * Change voltage of the OPP table regulators.
+ *
+ * Return: 0 on success or a negative error value.
+ */
+int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp)
+{
+	struct opp_table *opp_table;
+	struct regulator *reg;
+	int ret = 0;
+
+	/* Device may not have OPP table */
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return 0;
+
+	/* Regulator may not be required for the device */
+	if (!opp_table->regulators)
+		goto put_table;
+
+	/* This function only supports single regulator per device */
+	if (WARN_ON(opp_table->regulator_count > 1)) {
+		dev_err(dev, "multiple regulators are not supported\n");
+		ret = -EINVAL;
+		goto put_table;
+	}
+
+	reg = opp_table->regulators[0];
+	ret = _set_opp_voltage(dev, reg, opp->supplies);
+
+	if (!opp_table->enabled) {
+		ret = regulator_enable(reg);
+		if (ret < 0) {
+			dev_warn(dev, "Failed to enable regulator: %d", ret);
+			goto put_table;
+		}
+
+		opp_table->enabled = true;
+	}
+put_table:
+	/* Drop reference taken by _find_opp_table() */
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_voltage);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 80cdb9af8268..d8f412dfc03a 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -164,6 +164,7 @@ int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
 void dev_pm_opp_remove_table(struct device *dev);
 void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
 int dev_pm_opp_sync_regulators(struct device *dev);
+int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp);
 #else
 static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
 {
@@ -405,6 +406,11 @@ static inline int dev_pm_opp_sync_regulators(struct device *dev)
 	return -ENOTSUPP;
 }
 
+static inline int dev_pm_opp_set_voltage(struct device *dev, struct dev_pm_opp *opp)
+{
+	return -ENOTSUPP;
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
2.29.2

[PATCH v4 4/4] opp: Make _set_opp_custom() work without regulators

[Dmitry Osipenko]

Check whether OPP table has regulators in _set_opp_custom() and set up
dev_pm_set_opp_data accordingly. Now _set_opp_custom() works properly,
i.e. it doesn't crash if OPP table doesn't have assigned regulators.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/opp/core.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 08d205a55c07..c38ac1cf62ac 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -829,23 +829,31 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 			   struct dev_pm_opp_supply *new_supply)
 {
 	struct dev_pm_set_opp_data *data;
-	int size;
+	int size, regulator_count;
+
+	if (opp_table->sod_supplies)
+		regulator_count = opp_table->regulator_count;
+	else
+		regulator_count = 0;
 
 	data = opp_table->set_opp_data;
 	data->regulators = opp_table->regulators;
-	data->regulator_count = opp_table->regulator_count;
+	data->regulator_count = regulator_count;
 	data->clk = opp_table->clk;
 	data->dev = dev;
 
 	data->old_opp.rate = old_freq;
-	size = sizeof(*old_supply) * opp_table->regulator_count;
-	if (!old_supply)
-		memset(data->old_opp.supplies, 0, size);
-	else
-		memcpy(data->old_opp.supplies, old_supply, size);
-
 	data->new_opp.rate = freq;
-	memcpy(data->new_opp.supplies, new_supply, size);
+
+	if (regulator_count) {
+		size = sizeof(*old_supply) * opp_table->regulator_count;
+		if (!old_supply)
+			memset(data->old_opp.supplies, 0, size);
+		else
+			memcpy(data->old_opp.supplies, old_supply, size);
+
+		memcpy(data->new_opp.supplies, new_supply, size);
+	}
 
 	return opp_table->set_opp(data);
 }
-- 
2.29.2

Re: [PATCH v4 4/4] opp: Make _set_opp_custom() work without regulators

[Viresh Kumar]

On 21-01-21, 01:26, Dmitry Osipenko wrote:
> Check whether OPP table has regulators in _set_opp_custom() and set up
> dev_pm_set_opp_data accordingly. Now _set_opp_custom() works properly,
> i.e. it doesn't crash if OPP table doesn't have assigned regulators.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)

I have applied this instead:

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 32d653774adc..805fc2602808 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -828,24 +828,31 @@ static int _set_opp_custom(const struct opp_table *opp_table,
                           struct dev_pm_opp_supply *old_supply,
                           struct dev_pm_opp_supply *new_supply)
 {
-       struct dev_pm_set_opp_data *data;
+       struct dev_pm_set_opp_data *data = opp_table->set_opp_data;
        int size;
 
-       data = opp_table->set_opp_data;
+       /*
+        * We support this only if dev_pm_opp_set_regulators() was called
+        * earlier.
+        */
+       if (opp_table->sod_supplies) {
+               size = sizeof(*old_supply) * opp_table->regulator_count;
+               if (!old_supply)
+                       memset(data->old_opp.supplies, 0, size);
+               else
+                       memcpy(data->old_opp.supplies, old_supply, size);
+
+               memcpy(data->new_opp.supplies, new_supply, size);
+               data->regulator_count = opp_table->regulator_count;
+       } else {
+               data->regulator_count = 0;
+       }
+
        data->regulators = opp_table->regulators;
-       data->regulator_count = opp_table->regulator_count;
        data->clk = opp_table->clk;
        data->dev = dev;
-
        data->old_opp.rate = old_freq;
-       size = sizeof(*old_supply) * opp_table->regulator_count;
-       if (!old_supply)
-               memset(data->old_opp.supplies, 0, size);
-       else
-               memcpy(data->old_opp.supplies, old_supply, size);
-
        data->new_opp.rate = freq;
-       memcpy(data->new_opp.supplies, new_supply, size);
 
        return opp_table->set_opp(data);
 }


-- 
viresh

Re: [PATCH v4 0/4] OPP API fixes and improvements

[Viresh Kumar]

On 21-01-21, 01:26, Dmitry Osipenko wrote:
> Dmitry Osipenko (4):
>   opp: Export devm_pm_opp_attach_genpd()
>   opp: Add dev_pm_opp_sync_regulators()
>   opp: Add dev_pm_opp_set_voltage()
>   opp: Make _set_opp_custom() work without regulators

Patch 1 and 3 aren't required anymore, I have applied 4th already.
Patch 2 can be applied only after the dependency patch for the stub
definition gets merge in Linus's tree. I see that Mark has already
queued that up in his for-5.11 branch, so it might be part of next
-rc. I will apply 2nd patch then.

-- 
viresh

Re: [PATCH v4 4/4] opp: Make _set_opp_custom() work without regulators

[Dmitry Osipenko]

21.01.2021 01:26, Dmitry Osipenko пишет:
> Check whether OPP table has regulators in _set_opp_custom() and set up
> dev_pm_set_opp_data accordingly. Now _set_opp_custom() works properly,
> i.e. it doesn't crash if OPP table doesn't have assigned regulators.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 08d205a55c07..c38ac1cf62ac 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -829,23 +829,31 @@ static int _set_opp_custom(const struct opp_table *opp_table,
>  			   struct dev_pm_opp_supply *new_supply)
>  {
>  	struct dev_pm_set_opp_data *data;
> -	int size;
> +	int size, regulator_count;
> +
> +	if (opp_table->sod_supplies)
> +		regulator_count = opp_table->regulator_count;
> +	else
> +		regulator_count = 0;
>  
>  	data = opp_table->set_opp_data;
>  	data->regulators = opp_table->regulators;
> -	data->regulator_count = opp_table->regulator_count;
> +	data->regulator_count = regulator_count;
>  	data->clk = opp_table->clk;
>  	data->dev = dev;
>  
>  	data->old_opp.rate = old_freq;
> -	size = sizeof(*old_supply) * opp_table->regulator_count;
> -	if (!old_supply)
> -		memset(data->old_opp.supplies, 0, size);
> -	else
> -		memcpy(data->old_opp.supplies, old_supply, size);
> -
>  	data->new_opp.rate = freq;
> -	memcpy(data->new_opp.supplies, new_supply, size);
> +
> +	if (regulator_count) {
> +		size = sizeof(*old_supply) * opp_table->regulator_count;
> +		if (!old_supply)
> +			memset(data->old_opp.supplies, 0, size);
> +		else
> +			memcpy(data->old_opp.supplies, old_supply, size);
> +
> +		memcpy(data->new_opp.supplies, new_supply, size);
> +	}
>  
>  	return opp_table->set_opp(data);
>  }
> 

Looks good, thank you!

Re: [PATCH v4 4/4] opp: Make _set_opp_custom() work without regulators

[Dmitry Osipenko]

22.01.2021 10:26, Viresh Kumar пишет:
> On 21-01-21, 01:26, Dmitry Osipenko wrote:
>> Check whether OPP table has regulators in _set_opp_custom() and set up
>> dev_pm_set_opp_data accordingly. Now _set_opp_custom() works properly,
>> i.e. it doesn't crash if OPP table doesn't have assigned regulators.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/opp/core.c | 26 +++++++++++++++++---------
>>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> I have applied this instead:
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 32d653774adc..805fc2602808 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -828,24 +828,31 @@ static int _set_opp_custom(const struct opp_table *opp_table,
>                            struct dev_pm_opp_supply *old_supply,
>                            struct dev_pm_opp_supply *new_supply)
>  {
> -       struct dev_pm_set_opp_data *data;
> +       struct dev_pm_set_opp_data *data = opp_table->set_opp_data;
>         int size;
>  
> -       data = opp_table->set_opp_data;
> +       /*
> +        * We support this only if dev_pm_opp_set_regulators() was called
> +        * earlier.
> +        */
> +       if (opp_table->sod_supplies) {
> +               size = sizeof(*old_supply) * opp_table->regulator_count;
> +               if (!old_supply)
> +                       memset(data->old_opp.supplies, 0, size);
> +               else
> +                       memcpy(data->old_opp.supplies, old_supply, size);
> +
> +               memcpy(data->new_opp.supplies, new_supply, size);
> +               data->regulator_count = opp_table->regulator_count;
> +       } else {
> +               data->regulator_count = 0;
> +       }
> +
>         data->regulators = opp_table->regulators;
> -       data->regulator_count = opp_table->regulator_count;
>         data->clk = opp_table->clk;
>         data->dev = dev;
> -
>         data->old_opp.rate = old_freq;
> -       size = sizeof(*old_supply) * opp_table->regulator_count;
> -       if (!old_supply)
> -               memset(data->old_opp.supplies, 0, size);
> -       else
> -               memcpy(data->old_opp.supplies, old_supply, size);
> -
>         data->new_opp.rate = freq;
> -       memcpy(data->new_opp.supplies, new_supply, size);
>  
>         return opp_table->set_opp(data);
>  }
> 
> 

I replied to the wrong email, but this also looks good :) Thanks!

Re: [PATCH v4 2/4] opp: Add dev_pm_opp_sync_regulators()

[Dmitry Osipenko]

21.01.2021 01:26, 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     | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  6 ++++++
>  2 files changed, 47 insertions(+)

Hello Viresh,

This is the last patch that is left unmerged from this series. We will
need it for implementation of the PD driver for NVIDIA Tegra20/30 SoCs.
Please consider applying it, thanks in advance!

Re: [PATCH v4 2/4] opp: Add dev_pm_opp_sync_regulators()

[Viresh Kumar]

On 26-01-21, 01:30, Dmitry Osipenko wrote:
> 21.01.2021 01:26, 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     | 41 +++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pm_opp.h |  6 ++++++
> >  2 files changed, 47 insertions(+)
> 
> Hello Viresh,
> 
> This is the last patch that is left unmerged from this series. We will
> need it for implementation of the PD driver for NVIDIA Tegra20/30 SoCs.
> Please consider applying it, thanks in advance!

I have replied to 0/4 earlier giving the reason why I haven't applied this.

-- 
viresh

Re: [PATCH v4 0/4] OPP API fixes and improvements

[Viresh Kumar]

On 22-01-21, 13:04, Viresh Kumar wrote:
> Patch 2 can be applied only after the dependency patch for the stub
> definition gets merge in Linus's tree. I see that Mark has already
> queued that up in his for-5.11 branch, so it might be part of next
> -rc. I will apply 2nd patch then.

The consumer.h patch landed into Linus's tree this morning and hence I have
applied the last patch 2/4 from this series as well. Thanks.

-- 
viresh