diff mbox series

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

Message ID 20210118005524.27787-5-digetx@gmail.com (mailing list archive)
State New
Delegated to: viresh kumar
Headers show
Series OPP API fixes and improvements | expand

Commit Message

Dmitry Osipenko Jan. 18, 2021, 12:55 a.m. UTC
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(+)

Comments

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

Applied. Thanks.

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

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

Patch

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)