| Message ID | 20230511092334.3017-1-cniedermaier@dh-electronics.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | cpufreq: imx6q: Disable only available frequencies | expand |
On 11-05-23, 11:23, Christoph Niedermaier wrote: > In the example in Documentation/power/opp.rst, an availability check > is present before disabling a specific frequency. If a frequency isn't > available, the warning of a failed disabling of a non-existent > frequency is misleading. Which warning ?
On 11-05-23, 11:23, Christoph Niedermaier wrote: > @@ -254,16 +269,16 @@ static int imx6q_opp_check_speed_grading(struct device *dev) > val &= 0x3; > > if (val < OCOTP_CFG3_SPEED_996MHZ) > - if (dev_pm_opp_disable(dev, 996000000)) > + if (disable_freq_if_available(dev, 996000000)) > dev_warn(dev, "failed to disable 996MHz OPP\n"); Ahh, these warnings. What about printing the warning only when returned error != -ENODEV ? Or just marking them dev_dbg() ?
From: Viresh Kumar [mailto:viresh.kumar@linaro.org] Sent: Thursday, May 11, 2023 11:54 AM > On 11-05-23, 11:23, Christoph Niedermaier wrote: >> @@ -254,16 +269,16 @@ static int imx6q_opp_check_speed_grading(struct device *dev) >> val &= 0x3; >> >> if (val < OCOTP_CFG3_SPEED_996MHZ) >> - if (dev_pm_opp_disable(dev, 996000000)) >> + if (disable_freq_if_available(dev, 996000000)) >> dev_warn(dev, "failed to disable 996MHz OPP\n"); > > Ahh, these warnings. > > What about printing the warning only when returned error != -ENODEV ? > Or just marking them dev_dbg() ? I have kept to the documentation of opp, but if -ENODEV is possible I prefer it. My suggestion is to change each "dev_pm_opp_disable" in this way: - if (val < OCOTP_CFG3_SPEED_996MHZ) - if (dev_pm_opp_disable(dev, 996000000)) + if (val < OCOTP_CFG3_SPEED_996MHZ) { + ret_opp = dev_pm_opp_disable(dev, 996000000); + if (ret_opp < 0 && ret_opp != -ENODEV) dev_warn(dev, "failed to disable 996MHz OPP\n"); + } If that's OK, I can do a version 2 with it. Regards Christoph
On 5/11/23 11:23, Christoph Niedermaier wrote: > In the example in Documentation/power/opp.rst, an availability check > is present before disabling a specific frequency. If a frequency isn't > available, the warning of a failed disabling of a non-existent > frequency is misleading. Therefore, check the availability of the > frequency in a separate inline function before disabling it. [...] > diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c > index 48e1772e98fd..4e2d2bc47aba 100644 > --- a/drivers/cpufreq/imx6q-cpufreq.c > +++ b/drivers/cpufreq/imx6q-cpufreq.c > @@ -209,6 +209,21 @@ static struct cpufreq_driver imx6q_cpufreq_driver = { > .suspend = cpufreq_generic_suspend, > }; > > +static inline int disable_freq_if_available(struct device *dev, The inline isn't needed, esp. on static function, let the compiler figure it out. Also, "disable if available" should rather be "disable if unavailable" I think ? > + unsigned long freq) > +{ > + struct dev_pm_opp *opp; > + int ret = 0; > + > + opp = dev_pm_opp_find_freq_exact(dev, freq, true); > + if (!IS_ERR(opp)) { > + dev_pm_opp_put(opp); > + ret = dev_pm_opp_disable(dev, freq); > + } > + > + return ret; > +} > + > #define OCOTP_CFG3 0x440 > #define OCOTP_CFG3_SPEED_SHIFT 16 > #define OCOTP_CFG3_SPEED_1P2GHZ 0x3 > @@ -254,16 +269,16 @@ static int imx6q_opp_check_speed_grading(struct device *dev) > val &= 0x3; > > if (val < OCOTP_CFG3_SPEED_996MHZ) > - if (dev_pm_opp_disable(dev, 996000000)) > + if (disable_freq_if_available(dev, 996000000)) > dev_warn(dev, "failed to disable 996MHz OPP\n"); > > if (of_machine_is_compatible("fsl,imx6q") || > of_machine_is_compatible("fsl,imx6qp")) { Can we introduce a function like: void imx_disable_freq_if_unavailable(struct device *dev, u32 freq_mhz, u32 val, u32 mask) { if (val == mask) return; if (!disable_freq_if_available(dev, freq_mhz * 1000000)) return; dev_warn(dev, "failed to disable %dMHz OPP\n", mhz); } And then just call it multiple times in here, to reduce duplication ? > if (val != OCOTP_CFG3_SPEED_852MHZ) > - if (dev_pm_opp_disable(dev, 852000000)) > + if (disable_freq_if_available(dev, 852000000)) > dev_warn(dev, "failed to disable 852MHz OPP\n"); [...]
From: Marek Vasut [mailto:marex@denx.de] Sent: Thursday, May 11, 2023 9:26 PM > On 5/11/23 11:23, Christoph Niedermaier wrote: >> In the example in Documentation/power/opp.rst, an availability check >> is present before disabling a specific frequency. If a frequency isn't >> available, the warning of a failed disabling of a non-existent >> frequency is misleading. Therefore, check the availability of the >> frequency in a separate inline function before disabling it. > > [...] > >> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c >> index 48e1772e98fd..4e2d2bc47aba 100644 >> --- a/drivers/cpufreq/imx6q-cpufreq.c >> +++ b/drivers/cpufreq/imx6q-cpufreq.c >> @@ -209,6 +209,21 @@ static struct cpufreq_driver imx6q_cpufreq_driver = { >> .suspend = cpufreq_generic_suspend, >> }; >> >> +static inline int disable_freq_if_available(struct device *dev, > > The inline isn't needed, esp. on static function, let the compiler > figure it out. > > Also, "disable if available" should rather be "disable if unavailable" I > think ? Here I mean the OPP and I can only disable an available frequency. > >> + unsigned long freq) >> +{ >> + struct dev_pm_opp *opp; >> + int ret = 0; >> + >> + opp = dev_pm_opp_find_freq_exact(dev, freq, true); >> + if (!IS_ERR(opp)) { >> + dev_pm_opp_put(opp); >> + ret = dev_pm_opp_disable(dev, freq); >> + } >> + >> + return ret; >> +} >> + >> #define OCOTP_CFG3 0x440 >> #define OCOTP_CFG3_SPEED_SHIFT 16 >> #define OCOTP_CFG3_SPEED_1P2GHZ 0x3 >> @@ -254,16 +269,16 @@ static int imx6q_opp_check_speed_grading(struct device *dev) >> val &= 0x3; >> >> if (val < OCOTP_CFG3_SPEED_996MHZ) >> - if (dev_pm_opp_disable(dev, 996000000)) >> + if (disable_freq_if_available(dev, 996000000)) >> dev_warn(dev, "failed to disable 996MHz OPP\n"); >> >> if (of_machine_is_compatible("fsl,imx6q") || >> of_machine_is_compatible("fsl,imx6qp")) { > > Can we introduce a function like: > > void imx_disable_freq_if_unavailable(struct device *dev, u32 freq_mhz, > u32 val, u32 mask) > { > if (val == mask) > return; > if (!disable_freq_if_available(dev, freq_mhz * 1000000)) > return; > dev_warn(dev, "failed to disable %dMHz OPP\n", mhz); > } > > And then just call it multiple times in here, to reduce duplication ? > >> if (val != OCOTP_CFG3_SPEED_852MHZ) >> - if (dev_pm_opp_disable(dev, 852000000)) >> + if (disable_freq_if_available(dev, 852000000)) >> dev_warn(dev, "failed to disable 852MHz OPP\n"); > Yes, using a function to reduce duplications would be good. Regards Christoph
On 11-05-23, 14:38, Christoph Niedermaier wrote: > - if (val < OCOTP_CFG3_SPEED_996MHZ) > - if (dev_pm_opp_disable(dev, 996000000)) > + if (val < OCOTP_CFG3_SPEED_996MHZ) { > + ret_opp = dev_pm_opp_disable(dev, 996000000); > + if (ret_opp < 0 && ret_opp != -ENODEV) > dev_warn(dev, "failed to disable 996MHz OPP\n"); > + } > > If that's OK, I can do a version 2 with it. Looks okay to me.
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c index 48e1772e98fd..4e2d2bc47aba 100644 --- a/drivers/cpufreq/imx6q-cpufreq.c +++ b/drivers/cpufreq/imx6q-cpufreq.c @@ -209,6 +209,21 @@ static struct cpufreq_driver imx6q_cpufreq_driver = { .suspend = cpufreq_generic_suspend, }; +static inline int disable_freq_if_available(struct device *dev, + unsigned long freq) +{ + struct dev_pm_opp *opp; + int ret = 0; + + opp = dev_pm_opp_find_freq_exact(dev, freq, true); + if (!IS_ERR(opp)) { + dev_pm_opp_put(opp); + ret = dev_pm_opp_disable(dev, freq); + } + + return ret; +} + #define OCOTP_CFG3 0x440 #define OCOTP_CFG3_SPEED_SHIFT 16 #define OCOTP_CFG3_SPEED_1P2GHZ 0x3 @@ -254,16 +269,16 @@ static int imx6q_opp_check_speed_grading(struct device *dev) val &= 0x3; if (val < OCOTP_CFG3_SPEED_996MHZ) - if (dev_pm_opp_disable(dev, 996000000)) + if (disable_freq_if_available(dev, 996000000)) dev_warn(dev, "failed to disable 996MHz OPP\n"); if (of_machine_is_compatible("fsl,imx6q") || of_machine_is_compatible("fsl,imx6qp")) { if (val != OCOTP_CFG3_SPEED_852MHZ) - if (dev_pm_opp_disable(dev, 852000000)) + if (disable_freq_if_available(dev, 852000000)) dev_warn(dev, "failed to disable 852MHz OPP\n"); if (val != OCOTP_CFG3_SPEED_1P2GHZ) - if (dev_pm_opp_disable(dev, 1200000000)) + if (disable_freq_if_available(dev, 1200000000)) dev_warn(dev, "failed to disable 1.2GHz OPP\n"); } @@ -318,17 +333,17 @@ static int imx6ul_opp_check_speed_grading(struct device *dev) if (of_machine_is_compatible("fsl,imx6ul")) { if (val != OCOTP_CFG3_6UL_SPEED_696MHZ) - if (dev_pm_opp_disable(dev, 696000000)) + if (disable_freq_if_available(dev, 696000000)) dev_warn(dev, "failed to disable 696MHz OPP\n"); } if (of_machine_is_compatible("fsl,imx6ull")) { if (val != OCOTP_CFG3_6ULL_SPEED_792MHZ) - if (dev_pm_opp_disable(dev, 792000000)) + if (disable_freq_if_available(dev, 792000000)) dev_warn(dev, "failed to disable 792MHz OPP\n"); if (val != OCOTP_CFG3_6ULL_SPEED_900MHZ) - if (dev_pm_opp_disable(dev, 900000000)) + if (disable_freq_if_available(dev, 900000000)) dev_warn(dev, "failed to disable 900MHz OPP\n"); }
In the example in Documentation/power/opp.rst, an availability check is present before disabling a specific frequency. If a frequency isn't available, the warning of a failed disabling of a non-existent frequency is misleading. Therefore, check the availability of the frequency in a separate inline function before disabling it. Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com> --- Cc: Viresh Kumar <viresh.kumar@linaro.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Marek Vasut <marex@denx.de> Cc: Fabio Estevam <festevam@denx.de> Cc: NXP Linux Team <linux-imx@nxp.com> Cc: linux-pm@vger.kernel.org (open list:CPU FREQUENCY SCALING FRAMEWORK) Cc: linux-kernel@vger.kernel.org (open list) To: linux-arm-kernel@lists.infradead.org --- drivers/cpufreq/imx6q-cpufreq.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)