| Message ID | 20190415145505.18397-4-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | NVIDIA Tegra devfreq improvements and Tegra20/30 support | expand |
Hi, On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote: > The clk_set_min_rate() could fail and in this case clk_set_rate() sets > rate to 0, which may drop EMC rate to minimum and make machine very > difficult to use. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/devfreq/tegra-devfreq.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c > index 0c0909fba545..69581c9082d4 100644 > --- a/drivers/devfreq/tegra-devfreq.c > +++ b/drivers/devfreq/tegra-devfreq.c > @@ -487,6 +487,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, > struct tegra_devfreq *tegra = dev_get_drvdata(dev); > struct dev_pm_opp *opp; > unsigned long rate; > + int err; > > opp = devfreq_recommended_opp(dev, freq, flags); > if (IS_ERR(opp)) { > @@ -496,8 +497,13 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, > rate = dev_pm_opp_get_freq(opp); > dev_pm_opp_put(opp); > > - clk_set_min_rate(tegra->emc_clock, rate); > - clk_set_rate(tegra->emc_clock, 0); > + err = clk_set_min_rate(tegra->emc_clock, rate); > + if (err) > + return err; > + > + err = clk_set_rate(tegra->emc_clock, 0); > + if (err) Why don't you restore the previous minimum clock for tegra->emc_clock when failed to execute 'clk_set_rate(tegra->emc_clock, 0);'? I think that if you control the restoring the previous minimum clock, it is more nice to handle the error handling. > + return err; > > return 0; > } >
16.04.2019 4:52, Chanwoo Choi пишет: > Hi, > > On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote: >> The clk_set_min_rate() could fail and in this case clk_set_rate() sets >> rate to 0, which may drop EMC rate to minimum and make machine very >> difficult to use. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/devfreq/tegra-devfreq.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c >> index 0c0909fba545..69581c9082d4 100644 >> --- a/drivers/devfreq/tegra-devfreq.c >> +++ b/drivers/devfreq/tegra-devfreq.c >> @@ -487,6 +487,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, >> struct tegra_devfreq *tegra = dev_get_drvdata(dev); >> struct dev_pm_opp *opp; >> unsigned long rate; >> + int err; >> >> opp = devfreq_recommended_opp(dev, freq, flags); >> if (IS_ERR(opp)) { >> @@ -496,8 +497,13 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, >> rate = dev_pm_opp_get_freq(opp); >> dev_pm_opp_put(opp); >> >> - clk_set_min_rate(tegra->emc_clock, rate); >> - clk_set_rate(tegra->emc_clock, 0); >> + err = clk_set_min_rate(tegra->emc_clock, rate); >> + if (err) >> + return err; >> + >> + err = clk_set_rate(tegra->emc_clock, 0); >> + if (err) > > Why don't you restore the previous minimum clock for tegra->emc_clock > when failed to execute 'clk_set_rate(tegra->emc_clock, 0);'? > > I think that if you control the restoring the previous minimum clock, > it is more nice to handle the error handling. I now see that devfreq has the previous_freq field and indeed it is possible to restore the min_rate value using it. Thanks for the suggestion.
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c index 0c0909fba545..69581c9082d4 100644 --- a/drivers/devfreq/tegra-devfreq.c +++ b/drivers/devfreq/tegra-devfreq.c @@ -487,6 +487,7 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, struct tegra_devfreq *tegra = dev_get_drvdata(dev); struct dev_pm_opp *opp; unsigned long rate; + int err; opp = devfreq_recommended_opp(dev, freq, flags); if (IS_ERR(opp)) { @@ -496,8 +497,13 @@ static int tegra_devfreq_target(struct device *dev, unsigned long *freq, rate = dev_pm_opp_get_freq(opp); dev_pm_opp_put(opp); - clk_set_min_rate(tegra->emc_clock, rate); - clk_set_rate(tegra->emc_clock, 0); + err = clk_set_min_rate(tegra->emc_clock, rate); + if (err) + return err; + + err = clk_set_rate(tegra->emc_clock, 0); + if (err) + return err; return 0; }
The clk_set_min_rate() could fail and in this case clk_set_rate() sets rate to 0, which may drop EMC rate to minimum and make machine very difficult to use. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/devfreq/tegra-devfreq.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)