[V2,1/3] OPP: Level zero is valid
Commit Message
The level zero can be used by some OPPs to drop performance state vote
for the device. It is perfectly fine to allow the same.
_set_opp_level() considers it as an invalid value currently and returns
early.
In order to support this properly, initialize the level field with
U32_MAX, which denotes unused level field.
Reported-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 24 ++++++++++++++++++++----
drivers/opp/of.c | 8 +++++++-
include/linux/pm_opp.h | 5 ++++-
3 files changed, 31 insertions(+), 6 deletions(-)
Comments
On 30.10.2023 11:24, Viresh Kumar wrote:
> The level zero can be used by some OPPs to drop performance state vote
> for the device. It is perfectly fine to allow the same.
>
> _set_opp_level() considers it as an invalid value currently and returns
> early.
So, currently if the device is PM-enabled, but OPP requirements are lifted,
is the API currently internally stuck at the last non-zero level?
Just trying to understand if this could fix some silent issues
Konrad
On 30-10-23, 19:47, Konrad Dybcio wrote:
> On 30.10.2023 11:24, Viresh Kumar wrote:
> > The level zero can be used by some OPPs to drop performance state vote
> > for the device. It is perfectly fine to allow the same.
> >
> > _set_opp_level() considers it as an invalid value currently and returns
> > early.
> So, currently if the device is PM-enabled, but OPP requirements are lifted,
How exactly are the OPP requirements lifted ?
By calling dev_pm_opp_set_opp(dev, NULL) ?
This will work fine even without this commit.
> is the API currently internally stuck at the last non-zero level?
>
> Just trying to understand if this could fix some silent issues
Also the issue I am trying to solve here came in existence only during the 6.7
merge window and doesn't affect the genpds linked via required-opps. And this
commit will soon be merged.
On 31.10.2023 06:26, Viresh Kumar wrote:
> On 30-10-23, 19:47, Konrad Dybcio wrote:
>> On 30.10.2023 11:24, Viresh Kumar wrote:
>>> The level zero can be used by some OPPs to drop performance state vote
>>> for the device. It is perfectly fine to allow the same.
>>>
>>> _set_opp_level() considers it as an invalid value currently and returns
>>> early.
>
>> So, currently if the device is PM-enabled, but OPP requirements are lifted,
>
> How exactly are the OPP requirements lifted ?
>
> By calling dev_pm_opp_set_opp(dev, NULL) ?
>
> This will work fine even without this commit.
Ok!
>
>> is the API currently internally stuck at the last non-zero level?
>>
>> Just trying to understand if this could fix some silent issues
>
> Also the issue I am trying to solve here came in existence only during the 6.7
> merge window and doesn't affect the genpds linked via required-opps. And this
> commit will soon be merged.
Ack, thanks
Konrad
On Mon, 30 Oct 2023 at 11:24, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> The level zero can be used by some OPPs to drop performance state vote
> for the device. It is perfectly fine to allow the same.
>
> _set_opp_level() considers it as an invalid value currently and returns
> early.
>
> In order to support this properly, initialize the level field with
> U32_MAX, which denotes unused level field.
>
> Reported-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/opp/core.c | 24 ++++++++++++++++++++----
> drivers/opp/of.c | 8 +++++++-
> include/linux/pm_opp.h | 5 ++++-
> 3 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 84f345c69ea5..f2e2aa07b431 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -201,7 +201,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq_indexed);
> * @opp: opp for which level value has to be returned for
> *
> * Return: level read from device tree corresponding to the opp, else
> - * return 0.
> + * return U32_MAX.
> */
> unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp)
> {
> @@ -221,7 +221,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_level);
> * @index: index of the required opp
> *
> * Return: performance state read from device tree corresponding to the
> - * required opp, else return 0.
> + * required opp, else return U32_MAX.
> */
> unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
> unsigned int index)
> @@ -808,6 +808,14 @@ struct dev_pm_opp *dev_pm_opp_find_level_ceil(struct device *dev,
> struct dev_pm_opp *opp;
>
> opp = _find_key_ceil(dev, &temp, 0, true, _read_level, NULL);
> +
> + /* False match */
> + if (temp == OPP_LEVEL_UNSET) {
> + dev_err(dev, "%s: OPP levels aren't available\n", __func__);
> + dev_pm_opp_put(opp);
> + return ERR_PTR(-ENODEV);
> + }
> +
> *level = temp;
> return opp;
> }
> @@ -1049,12 +1057,18 @@ static int _set_opp_bw(const struct opp_table *opp_table,
> static int _set_performance_state(struct device *dev, struct device *pd_dev,
> struct dev_pm_opp *opp, int i)
> {
> - unsigned int pstate = likely(opp) ? opp->required_opps[i]->level: 0;
> + unsigned int pstate = 0;
> int ret;
>
> if (!pd_dev)
> return 0;
>
> + if (likely(opp)) {
> + pstate = opp->required_opps[i]->level;
> + if (pstate == OPP_LEVEL_UNSET)
> + return 0;
> + }
> +
> ret = dev_pm_domain_set_performance_state(pd_dev, pstate);
> if (ret) {
> dev_err(dev, "Failed to set performance state of %s: %d (%d)\n",
> @@ -1135,7 +1149,7 @@ static int _set_opp_level(struct device *dev, struct opp_table *opp_table,
> int ret = 0;
>
> if (opp) {
> - if (!opp->level)
> + if (opp->level == OPP_LEVEL_UNSET)
> return 0;
>
> level = opp->level;
> @@ -1867,6 +1881,8 @@ struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table)
>
> INIT_LIST_HEAD(&opp->node);
>
> + opp->level = OPP_LEVEL_UNSET;
> +
> return opp;
> }
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 81fa27599d58..85fad7ca0007 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -1393,8 +1393,14 @@ int of_get_required_opp_performance_state(struct device_node *np, int index)
>
> opp = _find_opp_of_np(opp_table, required_np);
> if (opp) {
> - pstate = opp->level;
> + if (opp->level == OPP_LEVEL_UNSET) {
> + pr_err("%s: OPP levels aren't available for %pOF\n",
> + __func__, np);
> + } else {
> + pstate = opp->level;
> + }
> dev_pm_opp_put(opp);
> +
> }
>
> dev_pm_opp_put_opp_table(opp_table);
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index ccd97bcef269..af53101a1383 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -92,9 +92,12 @@ struct dev_pm_opp_config {
> struct device ***virt_devs;
> };
>
> +#define OPP_LEVEL_UNSET U32_MAX
> +
> /**
> * struct dev_pm_opp_data - The data to use to initialize an OPP.
> - * @level: The performance level for the OPP.
> + * @level: The performance level for the OPP. Set level to OPP_LEVEL_UNSET if
> + * level field isn't used.
> * @freq: The clock rate in Hz for the OPP.
> * @u_volt: The voltage in uV for the OPP.
> */
> --
> 2.31.1.272.g89b43f80a514
>
@@ -201,7 +201,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq_indexed);
* @opp: opp for which level value has to be returned for
*
* Return: level read from device tree corresponding to the opp, else
- * return 0.
+ * return U32_MAX.
*/
unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp)
{
@@ -221,7 +221,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_level);
* @index: index of the required opp
*
* Return: performance state read from device tree corresponding to the
- * required opp, else return 0.
+ * required opp, else return U32_MAX.
*/
unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
unsigned int index)
@@ -808,6 +808,14 @@ struct dev_pm_opp *dev_pm_opp_find_level_ceil(struct device *dev,
struct dev_pm_opp *opp;
opp = _find_key_ceil(dev, &temp, 0, true, _read_level, NULL);
+
+ /* False match */
+ if (temp == OPP_LEVEL_UNSET) {
+ dev_err(dev, "%s: OPP levels aren't available\n", __func__);
+ dev_pm_opp_put(opp);
+ return ERR_PTR(-ENODEV);
+ }
+
*level = temp;
return opp;
}
@@ -1049,12 +1057,18 @@ static int _set_opp_bw(const struct opp_table *opp_table,
static int _set_performance_state(struct device *dev, struct device *pd_dev,
struct dev_pm_opp *opp, int i)
{
- unsigned int pstate = likely(opp) ? opp->required_opps[i]->level: 0;
+ unsigned int pstate = 0;
int ret;
if (!pd_dev)
return 0;
+ if (likely(opp)) {
+ pstate = opp->required_opps[i]->level;
+ if (pstate == OPP_LEVEL_UNSET)
+ return 0;
+ }
+
ret = dev_pm_domain_set_performance_state(pd_dev, pstate);
if (ret) {
dev_err(dev, "Failed to set performance state of %s: %d (%d)\n",
@@ -1135,7 +1149,7 @@ static int _set_opp_level(struct device *dev, struct opp_table *opp_table,
int ret = 0;
if (opp) {
- if (!opp->level)
+ if (opp->level == OPP_LEVEL_UNSET)
return 0;
level = opp->level;
@@ -1867,6 +1881,8 @@ struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table)
INIT_LIST_HEAD(&opp->node);
+ opp->level = OPP_LEVEL_UNSET;
+
return opp;
}
@@ -1393,8 +1393,14 @@ int of_get_required_opp_performance_state(struct device_node *np, int index)
opp = _find_opp_of_np(opp_table, required_np);
if (opp) {
- pstate = opp->level;
+ if (opp->level == OPP_LEVEL_UNSET) {
+ pr_err("%s: OPP levels aren't available for %pOF\n",
+ __func__, np);
+ } else {
+ pstate = opp->level;
+ }
dev_pm_opp_put(opp);
+
}
dev_pm_opp_put_opp_table(opp_table);
@@ -92,9 +92,12 @@ struct dev_pm_opp_config {
struct device ***virt_devs;
};
+#define OPP_LEVEL_UNSET U32_MAX
+
/**
* struct dev_pm_opp_data - The data to use to initialize an OPP.
- * @level: The performance level for the OPP.
+ * @level: The performance level for the OPP. Set level to OPP_LEVEL_UNSET if
+ * level field isn't used.
* @freq: The clock rate in Hz for the OPP.
* @u_volt: The voltage in uV for the OPP.
*/