| Message ID | 20201217180638.22748-15-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs | expand |
On 17-12-20, 21:06, Dmitry Osipenko wrote: > A required OPP may not be available, and thus, all OPPs which are using > this required OPP should be unavailable too. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/opp/core.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) Please send a separate patchset for fixes, as these can also go to 5.11 itself. > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index d9feb7639598..3d02fe33630b 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, > struct opp_table *opp_table, bool rate_not_available) > { > struct list_head *head; > - int ret; > + int i, ret; > > mutex_lock(&opp_table->lock); > head = &opp_table->opp_list; > @@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, > __func__, new_opp->rate); > } > > + for (i = 0; i < opp_table->required_opp_count && new_opp->available; i++) { > + if (new_opp->required_opps[i]->available) > + continue; > + > + new_opp->available = false; > + dev_warn(dev, "%s: OPP not supported by required OPP %pOF (%lu)\n", > + __func__, new_opp->required_opps[i]->np, new_opp->rate); Why not just break from here ? > + } > + > return 0; > } > > -- > 2.29.2
22.12.2020 11:59, Viresh Kumar пишет: > On 17-12-20, 21:06, Dmitry Osipenko wrote: >> A required OPP may not be available, and thus, all OPPs which are using >> this required OPP should be unavailable too. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/opp/core.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) > > Please send a separate patchset for fixes, as these can also go to 5.11 itself. Alright, although I don't think that this patch fixes any problems for existing OPP users. >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >> index d9feb7639598..3d02fe33630b 100644 >> --- a/drivers/opp/core.c >> +++ b/drivers/opp/core.c >> @@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, >> struct opp_table *opp_table, bool rate_not_available) >> { >> struct list_head *head; >> - int ret; >> + int i, ret; >> >> mutex_lock(&opp_table->lock); >> head = &opp_table->opp_list; >> @@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, >> __func__, new_opp->rate); >> } >> >> + for (i = 0; i < opp_table->required_opp_count && new_opp->available; i++) { >> + if (new_opp->required_opps[i]->available) >> + continue; >> + >> + new_opp->available = false; >> + dev_warn(dev, "%s: OPP not supported by required OPP %pOF (%lu)\n", >> + __func__, new_opp->required_opps[i]->np, new_opp->rate); > > Why not just break from here ? The new_opp could be already marked as unavailable by a previous voltage check, hence this loop should be skipped entirely in that case.
On 22-12-20, 22:17, Dmitry Osipenko wrote: > 22.12.2020 11:59, Viresh Kumar пишет: > > On 17-12-20, 21:06, Dmitry Osipenko wrote: > >> A required OPP may not be available, and thus, all OPPs which are using > >> this required OPP should be unavailable too. > >> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/opp/core.c | 11 ++++++++++- > >> 1 file changed, 10 insertions(+), 1 deletion(-) > > > > Please send a separate patchset for fixes, as these can also go to 5.11 itself. > > Alright, although I don't think that this patch fixes any problems for > existing OPP users. Because nobody is using this feature, but otherwise this is a fix for me. > >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c > >> index d9feb7639598..3d02fe33630b 100644 > >> --- a/drivers/opp/core.c > >> +++ b/drivers/opp/core.c > >> @@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, > >> struct opp_table *opp_table, bool rate_not_available) > >> { > >> struct list_head *head; > >> - int ret; > >> + int i, ret; > >> > >> mutex_lock(&opp_table->lock); > >> head = &opp_table->opp_list; > >> @@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, > >> __func__, new_opp->rate); > >> } > >> > >> + for (i = 0; i < opp_table->required_opp_count && new_opp->available; i++) { > >> + if (new_opp->required_opps[i]->available) > >> + continue; > >> + > >> + new_opp->available = false; > >> + dev_warn(dev, "%s: OPP not supported by required OPP %pOF (%lu)\n", > >> + __func__, new_opp->required_opps[i]->np, new_opp->rate); > > > > Why not just break from here ? > > The new_opp could be already marked as unavailable by a previous voltage > check, hence this loop should be skipped entirely in that case. Then add a separate check for that before the loop as we don't need that check on every iteration here.
23.12.2020 07:22, Viresh Kumar пишет: > On 22-12-20, 22:17, Dmitry Osipenko wrote: >> 22.12.2020 11:59, Viresh Kumar пишет: >>> On 17-12-20, 21:06, Dmitry Osipenko wrote: >>>> A required OPP may not be available, and thus, all OPPs which are using >>>> this required OPP should be unavailable too. >>>> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> drivers/opp/core.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> Please send a separate patchset for fixes, as these can also go to 5.11 itself. >> >> Alright, although I don't think that this patch fixes any problems for >> existing OPP users. > > Because nobody is using this feature, but otherwise this is a fix for me. > >>>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >>>> index d9feb7639598..3d02fe33630b 100644 >>>> --- a/drivers/opp/core.c >>>> +++ b/drivers/opp/core.c >>>> @@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, >>>> struct opp_table *opp_table, bool rate_not_available) >>>> { >>>> struct list_head *head; >>>> - int ret; >>>> + int i, ret; >>>> >>>> mutex_lock(&opp_table->lock); >>>> head = &opp_table->opp_list; >>>> @@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, >>>> __func__, new_opp->rate); >>>> } >>>> >>>> + for (i = 0; i < opp_table->required_opp_count && new_opp->available; i++) { >>>> + if (new_opp->required_opps[i]->available) >>>> + continue; >>>> + >>>> + new_opp->available = false; >>>> + dev_warn(dev, "%s: OPP not supported by required OPP %pOF (%lu)\n", >>>> + __func__, new_opp->required_opps[i]->np, new_opp->rate); >>> >>> Why not just break from here ? >> >> The new_opp could be already marked as unavailable by a previous voltage >> check, hence this loop should be skipped entirely in that case. > > Then add a separate check for that before the loop as we don't need that check > on every iteration here. > Perhaps the break will be a better option in this case, since it won't hurt at all to print the additional message even if OPP was already disabled by another check. I'll update it in next revision, thanks.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index d9feb7639598..3d02fe33630b 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available) { struct list_head *head; - int ret; + int i, ret; mutex_lock(&opp_table->lock); head = &opp_table->opp_list; @@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, __func__, new_opp->rate); } + for (i = 0; i < opp_table->required_opp_count && new_opp->available; i++) { + if (new_opp->required_opps[i]->available) + continue; + + new_opp->available = false; + dev_warn(dev, "%s: OPP not supported by required OPP %pOF (%lu)\n", + __func__, new_opp->required_opps[i]->np, new_opp->rate); + } + return 0; }
A required OPP may not be available, and thus, all OPPs which are using this required OPP should be unavailable too. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/opp/core.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)