diff mbox series

[v10,1/8] opp: Add dev_pm_opp_get_current()

Message ID 20210831135450.26070-2-digetx@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show
Series NVIDIA Tegra power management patches for 5.16 | expand

Commit Message

Dmitry Osipenko Aug. 31, 2021, 1:54 p.m. UTC
Add dev_pm_opp_get_current() helper that returns OPP corresponding
to the current clock rate of a device.

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

Comments

Viresh Kumar Sept. 1, 2021, 4:39 a.m. UTC | #1
On 31-08-21, 16:54, Dmitry Osipenko wrote:
> Add dev_pm_opp_get_current() helper that returns OPP corresponding
> to the current clock rate of a device.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/opp/core.c     | 43 +++++++++++++++++++++++++++++++++++++++---
>  include/linux/pm_opp.h |  6 ++++++
>  2 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 04b4691a8aac..dde8a5cc948c 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -939,7 +939,7 @@ static int _set_required_opps(struct device *dev,
>  	return ret;
>  }
>  
> -static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
> +static struct dev_pm_opp *_find_current_opp(struct opp_table *opp_table)
>  {
>  	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
>  	unsigned long freq;
> @@ -949,6 +949,18 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>  		opp = _find_freq_ceil(opp_table, &freq);
>  	}
>  
> +	return opp;
> +}
> +
> +static void _find_and_set_current_opp(struct opp_table *opp_table)
> +{
> +	struct dev_pm_opp *opp;
> +
> +	if (opp_table->current_opp)
> +		return;

Why move this from caller as well ?

> +
> +	opp = _find_current_opp(opp_table);
> +
>  	/*
>  	 * Unable to find the current OPP ? Pick the first from the list since
>  	 * it is in ascending order, otherwise rest of the code will need to

If we aren't able to find current OPP based on current freq, then this
picks the first value from the list. Why shouldn't this be done in
your case as well ?

> @@ -1002,8 +1014,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>  		return _disable_opp_table(dev, opp_table);
>  
>  	/* Find the currently set OPP if we don't know already */
> -	if (unlikely(!opp_table->current_opp))
> -		_find_current_opp(dev, opp_table);
> +	_find_and_set_current_opp(opp_table);
>  
>  	old_opp = opp_table->current_opp;
>  
> @@ -2931,3 +2942,29 @@ int dev_pm_opp_sync_regulators(struct device *dev)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
> +
> +/**
> + * dev_pm_opp_get_current() - Get current OPP
> + * @dev:	device for which we do this operation
> + *
> + * Get current OPP of a device based on current clock rate or by other means.
> + *
> + * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
> + */
> +struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev)
> +{
> +	struct opp_table *opp_table;
> +	struct dev_pm_opp *opp;
> +
> +	opp_table = _find_opp_table(dev);
> +	if (IS_ERR(opp_table))
> +		return ERR_CAST(opp_table);
> +
> +	opp = _find_current_opp(opp_table);

This should not just go find the OPP based on current freq. This first
needs to check if curret_opp is set or not. If set, then just return
it, else run the _find_current_opp() function and update the
current_opp pointer as well.
Dmitry Osipenko Sept. 1, 2021, 5:43 a.m. UTC | #2
01.09.2021 07:39, Viresh Kumar пишет:
> On 31-08-21, 16:54, Dmitry Osipenko wrote:
>> Add dev_pm_opp_get_current() helper that returns OPP corresponding
>> to the current clock rate of a device.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/opp/core.c     | 43 +++++++++++++++++++++++++++++++++++++++---
>>  include/linux/pm_opp.h |  6 ++++++
>>  2 files changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 04b4691a8aac..dde8a5cc948c 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -939,7 +939,7 @@ static int _set_required_opps(struct device *dev,
>>  	return ret;
>>  }
>>  
>> -static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>> +static struct dev_pm_opp *_find_current_opp(struct opp_table *opp_table)
>>  {
>>  	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
>>  	unsigned long freq;
>> @@ -949,6 +949,18 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>>  		opp = _find_freq_ceil(opp_table, &freq);
>>  	}
>>  
>> +	return opp;
>> +}
>> +
>> +static void _find_and_set_current_opp(struct opp_table *opp_table)
>> +{
>> +	struct dev_pm_opp *opp;
>> +
>> +	if (opp_table->current_opp)
>> +		return;
> 
> Why move this from caller as well ?

To make code cleaner.

>> +
>> +	opp = _find_current_opp(opp_table);
>> +
>>  	/*
>>  	 * Unable to find the current OPP ? Pick the first from the list since
>>  	 * it is in ascending order, otherwise rest of the code will need to
> 
> If we aren't able to find current OPP based on current freq, then this
> picks the first value from the list. Why shouldn't this be done in
> your case as well ?

You will get OPP which corresponds to the lowest freq, while h/w runs on
unsupported high freq. This may end with a tragedy.

>> @@ -1002,8 +1014,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>>  		return _disable_opp_table(dev, opp_table);
>>  
>>  	/* Find the currently set OPP if we don't know already */
>> -	if (unlikely(!opp_table->current_opp))
>> -		_find_current_opp(dev, opp_table);
>> +	_find_and_set_current_opp(opp_table);
>>  
>>  	old_opp = opp_table->current_opp;
>>  
>> @@ -2931,3 +2942,29 @@ int dev_pm_opp_sync_regulators(struct device *dev)
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
>> +
>> +/**
>> + * dev_pm_opp_get_current() - Get current OPP
>> + * @dev:	device for which we do this operation
>> + *
>> + * Get current OPP of a device based on current clock rate or by other means.
>> + *
>> + * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
>> + */
>> +struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev)
>> +{
>> +	struct opp_table *opp_table;
>> +	struct dev_pm_opp *opp;
>> +
>> +	opp_table = _find_opp_table(dev);
>> +	if (IS_ERR(opp_table))
>> +		return ERR_CAST(opp_table);
>> +
>> +	opp = _find_current_opp(opp_table);
> 
> This should not just go find the OPP based on current freq. This first
> needs to check if curret_opp is set or not. If set, then just return
> it, else run the _find_current_opp() function and update the
> current_opp pointer as well.
> 

Alright, I'll change it.
Viresh Kumar Sept. 1, 2021, 6:05 a.m. UTC | #3
On 01-09-21, 08:43, Dmitry Osipenko wrote:
> You will get OPP which corresponds to the lowest freq, while h/w runs on
> unsupported high freq. This may end with a tragedy.

Yeah, because you are setting a performance state with this, it can be
a problem.
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 04b4691a8aac..dde8a5cc948c 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -939,7 +939,7 @@  static int _set_required_opps(struct device *dev,
 	return ret;
 }
 
-static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
+static struct dev_pm_opp *_find_current_opp(struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
 	unsigned long freq;
@@ -949,6 +949,18 @@  static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 		opp = _find_freq_ceil(opp_table, &freq);
 	}
 
+	return opp;
+}
+
+static void _find_and_set_current_opp(struct opp_table *opp_table)
+{
+	struct dev_pm_opp *opp;
+
+	if (opp_table->current_opp)
+		return;
+
+	opp = _find_current_opp(opp_table);
+
 	/*
 	 * Unable to find the current OPP ? Pick the first from the list since
 	 * it is in ascending order, otherwise rest of the code will need to
@@ -1002,8 +1014,7 @@  static int _set_opp(struct device *dev, struct opp_table *opp_table,
 		return _disable_opp_table(dev, opp_table);
 
 	/* Find the currently set OPP if we don't know already */
-	if (unlikely(!opp_table->current_opp))
-		_find_current_opp(dev, opp_table);
+	_find_and_set_current_opp(opp_table);
 
 	old_opp = opp_table->current_opp;
 
@@ -2931,3 +2942,29 @@  int dev_pm_opp_sync_regulators(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
+
+/**
+ * dev_pm_opp_get_current() - Get current OPP
+ * @dev:	device for which we do this operation
+ *
+ * Get current OPP of a device based on current clock rate or by other means.
+ *
+ * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
+ */
+struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev)
+{
+	struct opp_table *opp_table;
+	struct dev_pm_opp *opp;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return ERR_CAST(opp_table);
+
+	opp = _find_current_opp(opp_table);
+
+	/* Drop reference taken by _find_opp_table() */
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return opp;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_current);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 84150a22fd7c..c8091977efb8 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -168,6 +168,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);
+struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev);
 #else
 static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
 {
@@ -434,6 +435,11 @@  static inline int dev_pm_opp_sync_regulators(struct device *dev)
 	return -EOPNOTSUPP;
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_get_current(struct device *dev)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)