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