* [PATCH 0/2] opp: core: add regulators enable and disable
@ 2020-05-15 7:57 ` Viresh Kumar
2020-05-15 7:57 ` [PATCH 1/2] opp: Reorder the code for !target_freq case Viresh Kumar
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Viresh Kumar @ 2020-05-15 7:57 UTC (permalink / raw)
To: k.konieczny, peron.clem, m.szyprowski, Nishanth Menon,
Stephen Boyd, Viresh Kumar
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki, broonie,
linux-kernel
Hi,
This series reintroduces the usage of regulator_enable/disable() to the
OPP core after the previous attempt was reverted [1] shortly after getting
applied. This time the regulator is enabled only after it is configured
by the OPP core.
Marek, Kamil and Clément: Can you guys please test this out and report
if this doesn't work as expected ?
--
viresh
[1] https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/
Kamil Konieczny (1):
opp: core: add regulators enable and disable
Viresh Kumar (1):
opp: Reorder the code for !target_freq case
drivers/opp/core.c | 39 ++++++++++++++++++++++++++++++++++-----
drivers/opp/opp.h | 2 ++
2 files changed, 36 insertions(+), 5 deletions(-)
--
2.25.0.rc1.19.g042ed3e048af
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] opp: Reorder the code for !target_freq case
2020-05-15 7:57 ` [PATCH 0/2] opp: core: add regulators enable and disable Viresh Kumar
@ 2020-05-15 7:57 ` Viresh Kumar
2020-05-15 14:34 ` kbuild test robot
2020-05-15 7:57 ` [PATCH 2/2] opp: core: add regulators enable and disable Viresh Kumar
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Viresh Kumar @ 2020-05-15 7:57 UTC (permalink / raw)
To: k.konieczny, peron.clem, m.szyprowski, Viresh Kumar,
Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki, broonie,
linux-kernel
Reorder the code a bit to make it more readable. Add additional comment
as well.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e4f01e7771a2..dda8164fad56 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -817,15 +817,20 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
}
if (unlikely(!target_freq)) {
- if (opp_table->required_opp_tables) {
- ret = _set_required_opps(dev, opp_table, NULL);
- } else if (!_get_opp_count(opp_table)) {
+ /*
+ * Some drivers need to support cases where some platforms may
+ * have OPP table for the device, while others don't and
+ * opp_set_rate() just needs to behave like clk_set_rate().
+ */
+ if (!_get_opp_count(opp_table))
return 0;
- } else {
+
+ if (!opp_table->required_opp_tables) {
dev_err(dev, "target frequency can't be 0\n");
ret = -EINVAL;
}
+ ret = _set_required_opps(dev, opp_table, NULL);
goto put_opp_table;
}
--
2.25.0.rc1.19.g042ed3e048af
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] opp: core: add regulators enable and disable
2020-05-15 7:57 ` [PATCH 0/2] opp: core: add regulators enable and disable Viresh Kumar
2020-05-15 7:57 ` [PATCH 1/2] opp: Reorder the code for !target_freq case Viresh Kumar
@ 2020-05-15 7:57 ` Viresh Kumar
2020-05-15 9:16 ` [PATCH 0/2] " Marek Szyprowski
2020-05-15 12:00 ` Clément Péron
3 siblings, 0 replies; 12+ messages in thread
From: Viresh Kumar @ 2020-05-15 7:57 UTC (permalink / raw)
To: k.konieczny, peron.clem, m.szyprowski, Viresh Kumar,
Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki, broonie,
linux-kernel
From: Kamil Konieczny <k.konieczny@samsung.com>
Add enable regulators to dev_pm_opp_set_regulators() and disable
regulators to dev_pm_opp_put_regulators(). Even if bootloader
leaves regulators enabled, they should be enabled in kernel in
order to increase the reference count.
Signed-off-by: Kamil Konieczny <k.konieczny@samsung.com>
[ Viresh: Enable the regulator only after it is programmed and add a
flag to track its status. ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 28 ++++++++++++++++++++++++++--
drivers/opp/opp.h | 2 ++
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index dda8164fad56..da6533d5526f 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -664,7 +664,7 @@ static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
return ret;
}
-static int _generic_set_opp_regulator(const struct opp_table *opp_table,
+static int _generic_set_opp_regulator(struct opp_table *opp_table,
struct device *dev,
unsigned long old_freq,
unsigned long freq,
@@ -699,6 +699,18 @@ static int _generic_set_opp_regulator(const struct opp_table *opp_table,
goto restore_freq;
}
+ /*
+ * Enable the regulator after setting its voltages, otherwise it breaks
+ * some boot-enabled regulators.
+ */
+ if (unlikely(!opp_table->regulator_enabled)) {
+ ret = regulator_enable(reg);
+ if (ret < 0)
+ dev_warn(dev, "Failed to enable regulator: %d", ret);
+ else
+ opp_table->regulator_enabled = true;
+ }
+
return 0;
restore_freq:
@@ -825,11 +837,16 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
if (!_get_opp_count(opp_table))
return 0;
- if (!opp_table->required_opp_tables) {
+ if (!opp_table->required_opp_tables && !opp_table->regulators) {
dev_err(dev, "target frequency can't be 0\n");
ret = -EINVAL;
}
+ if (opp_table->regulator_enabled) {
+ regulator_disable(opp_table->regulators[0]);
+ opp_table->regulator_enabled = false;
+ }
+
ret = _set_required_opps(dev, opp_table, NULL);
goto put_opp_table;
}
@@ -1675,6 +1692,13 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
/* Make sure there are no concurrent readers while updating opp_table */
WARN_ON(!list_empty(&opp_table->opp_list));
+ if (opp_table->regulator_enabled) {
+ for (i = opp_table->regulator_count - 1; i >= 0; i--)
+ regulator_disable(opp_table->regulators[i]);
+
+ opp_table->regulator_enabled = false;
+ }
+
for (i = opp_table->regulator_count - 1; i >= 0; i--)
regulator_put(opp_table->regulators[i]);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index d14e27102730..b4b2713d84f1 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -144,6 +144,7 @@ enum opp_table_access {
* @clk: Device's clock handle
* @regulators: Supply regulators
* @regulator_count: Number of power supply regulators. Its value can be -1
+ * @regulator_enabled: Set to true if regulators were previously enabled.
* (uninitialized), 0 (no opp-microvolt property) or > 0 (has opp-microvolt
* property).
* @genpd_performance_state: Device's power domain support performance state.
@@ -189,6 +190,7 @@ struct opp_table {
struct clk *clk;
struct regulator **regulators;
int regulator_count;
+ bool regulator_enabled;
bool genpd_performance_state;
bool is_genpd;
--
2.25.0.rc1.19.g042ed3e048af
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] opp: core: add regulators enable and disable
2020-05-15 7:57 ` [PATCH 0/2] opp: core: add regulators enable and disable Viresh Kumar
2020-05-15 7:57 ` [PATCH 1/2] opp: Reorder the code for !target_freq case Viresh Kumar
2020-05-15 7:57 ` [PATCH 2/2] opp: core: add regulators enable and disable Viresh Kumar
@ 2020-05-15 9:16 ` Marek Szyprowski
2020-05-15 12:00 ` Clément Péron
3 siblings, 0 replies; 12+ messages in thread
From: Marek Szyprowski @ 2020-05-15 9:16 UTC (permalink / raw)
To: Viresh Kumar, k.konieczny, peron.clem, Nishanth Menon,
Stephen Boyd, Viresh Kumar
Cc: linux-pm, Vincent Guittot, Rafael Wysocki, broonie, linux-kernel
Hi Viresh,
On 15.05.2020 09:57, Viresh Kumar wrote:
> This series reintroduces the usage of regulator_enable/disable() to the
> OPP core after the previous attempt was reverted [1] shortly after getting
> applied. This time the regulator is enabled only after it is configured
> by the OPP core.
>
> Marek, Kamil and Clément: Can you guys please test this out and report
> if this doesn't work as expected ?
Works fine for my test cases, especially Samsung Chromebook Peach-Pit/Pi
still boots fine. Feel free to add to the both patches:
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Thanks!
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] opp: core: add regulators enable and disable
2020-05-15 7:57 ` [PATCH 0/2] opp: core: add regulators enable and disable Viresh Kumar
` (2 preceding siblings ...)
2020-05-15 9:16 ` [PATCH 0/2] " Marek Szyprowski
@ 2020-05-15 12:00 ` Clément Péron
2020-05-21 12:23 ` Clément Péron
3 siblings, 1 reply; 12+ messages in thread
From: Clément Péron @ 2020-05-15 12:00 UTC (permalink / raw)
To: Viresh Kumar
Cc: k.konieczny, Marek Szyprowski, Nishanth Menon, Stephen Boyd,
Viresh Kumar, open list:ALLWINNER CPUFREQ DRIVER,
Vincent Guittot, Rafael Wysocki, Mark Brown, linux-kernel
Hi Viresh,
On Fri, 15 May 2020 at 09:57, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Hi,
>
> This series reintroduces the usage of regulator_enable/disable() to the
> OPP core after the previous attempt was reverted [1] shortly after getting
> applied. This time the regulator is enabled only after it is configured
> by the OPP core.
>
> Marek, Kamil and Clément: Can you guys please test this out and report
> if this doesn't work as expected ?
I have reviewed the patch and it seems fine for my use case.
Unfortunately I can't test it until next week.
Acked-by: Clément Péron <peron.clem@gmail.com>
Regards,
Clement
>
> --
> viresh
>
> [1] https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/
>
> Kamil Konieczny (1):
> opp: core: add regulators enable and disable
>
> Viresh Kumar (1):
> opp: Reorder the code for !target_freq case
>
> drivers/opp/core.c | 39 ++++++++++++++++++++++++++++++++++-----
> drivers/opp/opp.h | 2 ++
> 2 files changed, 36 insertions(+), 5 deletions(-)
>
> --
> 2.25.0.rc1.19.g042ed3e048af
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] opp: Reorder the code for !target_freq case
2020-05-15 7:57 ` [PATCH 1/2] opp: Reorder the code for !target_freq case Viresh Kumar
@ 2020-05-15 14:34 ` kbuild test robot
2020-05-18 7:06 ` Viresh Kumar
0 siblings, 1 reply; 12+ messages in thread
From: kbuild test robot @ 2020-05-15 14:34 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 6109 bytes --]
Hi Viresh,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Viresh-Kumar/opp-core-add-regulators-enable-and-disable/20200515-155824
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1ae7efb388540adc1653a51a3bc3b2c9cef5ec1a
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
cppcheck warnings: (new ones prefixed by >>)
>> drivers/opp/core.c:833:7: warning: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment]
ret = _set_required_opps(dev, opp_table, NULL);
^
drivers/opp/core.c:830:8: note: ret is assigned
ret = -EINVAL;
^
drivers/opp/core.c:833:7: note: ret is overwritten
ret = _set_required_opps(dev, opp_table, NULL);
^
vim +/ret +833 drivers/opp/core.c
793
794 /**
795 * dev_pm_opp_set_rate() - Configure new OPP based on frequency
796 * @dev: device for which we do this operation
797 * @target_freq: frequency to achieve
798 *
799 * This configures the power-supplies to the levels specified by the OPP
800 * corresponding to the target_freq, and programs the clock to a value <=
801 * target_freq, as rounded by clk_round_rate(). Device wanting to run@fmax
802 * provided by the opp, should have already rounded to the target OPP's
803 * frequency.
804 */
805 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
806 {
807 struct opp_table *opp_table;
808 unsigned long freq, old_freq, temp_freq;
809 struct dev_pm_opp *old_opp, *opp;
810 struct clk *clk;
811 int ret;
812
813 opp_table = _find_opp_table(dev);
814 if (IS_ERR(opp_table)) {
815 dev_err(dev, "%s: device opp doesn't exist\n", __func__);
816 return PTR_ERR(opp_table);
817 }
818
819 if (unlikely(!target_freq)) {
820 /*
821 * Some drivers need to support cases where some platforms may
822 * have OPP table for the device, while others don't and
823 * opp_set_rate() just needs to behave like clk_set_rate().
824 */
825 if (!_get_opp_count(opp_table))
826 return 0;
827
828 if (!opp_table->required_opp_tables) {
829 dev_err(dev, "target frequency can't be 0\n");
830 ret = -EINVAL;
831 }
832
> 833 ret = _set_required_opps(dev, opp_table, NULL);
834 goto put_opp_table;
835 }
836
837 clk = opp_table->clk;
838 if (IS_ERR(clk)) {
839 dev_err(dev, "%s: No clock available for the device\n",
840 __func__);
841 ret = PTR_ERR(clk);
842 goto put_opp_table;
843 }
844
845 freq = clk_round_rate(clk, target_freq);
846 if ((long)freq <= 0)
847 freq = target_freq;
848
849 old_freq = clk_get_rate(clk);
850
851 /* Return early if nothing to do */
852 if (old_freq == freq) {
853 dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",
854 __func__, freq);
855 ret = 0;
856 goto put_opp_table;
857 }
858
859 /*
860 * For IO devices which require an OPP on some platforms/SoCs
861 * while just needing to scale the clock on some others
862 * we look for empty OPP tables with just a clock handle and
863 * scale only the clk. This makes dev_pm_opp_set_rate()
864 * equivalent to a clk_set_rate()
865 */
866 if (!_get_opp_count(opp_table)) {
867 ret = _generic_set_opp_clk_only(dev, clk, freq);
868 goto put_opp_table;
869 }
870
871 temp_freq = old_freq;
872 old_opp = _find_freq_ceil(opp_table, &temp_freq);
873 if (IS_ERR(old_opp)) {
874 dev_err(dev, "%s: failed to find current OPP for freq %lu (%ld)\n",
875 __func__, old_freq, PTR_ERR(old_opp));
876 }
877
878 temp_freq = freq;
879 opp = _find_freq_ceil(opp_table, &temp_freq);
880 if (IS_ERR(opp)) {
881 ret = PTR_ERR(opp);
882 dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
883 __func__, freq, ret);
884 goto put_old_opp;
885 }
886
887 dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
888 old_freq, freq);
889
890 /* Scaling up? Configure required OPPs before frequency */
891 if (freq >= old_freq) {
892 ret = _set_required_opps(dev, opp_table, opp);
893 if (ret)
894 goto put_opp;
895 }
896
897 if (opp_table->set_opp) {
898 ret = _set_opp_custom(opp_table, dev, old_freq, freq,
899 IS_ERR(old_opp) ? NULL : old_opp->supplies,
900 opp->supplies);
901 } else if (opp_table->regulators) {
902 ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
903 IS_ERR(old_opp) ? NULL : old_opp->supplies,
904 opp->supplies);
905 } else {
906 /* Only frequency scaling */
907 ret = _generic_set_opp_clk_only(dev, clk, freq);
908 }
909
910 /* Scaling down? Configure required OPPs after frequency */
911 if (!ret && freq < old_freq) {
912 ret = _set_required_opps(dev, opp_table, opp);
913 if (ret)
914 dev_err(dev, "Failed to set required opps: %d\n", ret);
915 }
916
917 put_opp:
918 dev_pm_opp_put(opp);
919 put_old_opp:
920 if (!IS_ERR(old_opp))
921 dev_pm_opp_put(old_opp);
922 put_opp_table:
923 dev_pm_opp_put_opp_table(opp_table);
924 return ret;
925 }
926 EXPORT_SYMBOL_GPL(dev_pm_opp_set_rate);
927
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] opp: Reorder the code for !target_freq case
2020-05-15 14:34 ` kbuild test robot
@ 2020-05-18 7:06 ` Viresh Kumar
0 siblings, 0 replies; 12+ messages in thread
From: Viresh Kumar @ 2020-05-18 7:06 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]
On 15-05-20, 22:34, kbuild test robot wrote:
> Hi Viresh,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url: https://github.com/0day-ci/linux/commits/Viresh-Kumar/opp-core-add-regulators-enable-and-disable/20200515-155824
> base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1ae7efb388540adc1653a51a3bc3b2c9cef5ec1a
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>
>
>
> cppcheck warnings: (new ones prefixed by >>)
>
> >> drivers/opp/core.c:833:7: warning: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment]
> ret = _set_required_opps(dev, opp_table, NULL);
> ^
> drivers/opp/core.c:830:8: note: ret is assigned
> ret = -EINVAL;
> ^
> drivers/opp/core.c:833:7: note: ret is overwritten
> ret = _set_required_opps(dev, opp_table, NULL);
> ^
>
> vim +/ret +833 drivers/opp/core.c
Fixed, thanks.
--
viresh
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] opp: core: add regulators enable and disable
2020-05-15 12:00 ` Clément Péron
@ 2020-05-21 12:23 ` Clément Péron
2022-09-03 20:35 ` Clément Péron
0 siblings, 1 reply; 12+ messages in thread
From: Clément Péron @ 2020-05-21 12:23 UTC (permalink / raw)
To: Viresh Kumar
Cc: k.konieczny, Marek Szyprowski, Nishanth Menon, Stephen Boyd,
Viresh Kumar, open list:ALLWINNER CPUFREQ DRIVER,
Vincent Guittot, Rafael Wysocki, Mark Brown, linux-kernel
Hi,
On Fri, 15 May 2020 at 14:00, Clément Péron <peron.clem@gmail.com> wrote:
>
> Hi Viresh,
>
> On Fri, 15 May 2020 at 09:57, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Hi,
> >
> > This series reintroduces the usage of regulator_enable/disable() to the
> > OPP core after the previous attempt was reverted [1] shortly after getting
> > applied. This time the regulator is enabled only after it is configured
> > by the OPP core.
> >
> > Marek, Kamil and Clément: Can you guys please test this out and report
> > if this doesn't work as expected ?
>
> I have reviewed the patch and it seems fine for my use case.
> Unfortunately I can't test it until next week.
Ok, before the patch the regulator was released by regulator_late_cleanup() :
[ 33.756849] vdd-gpu: disabling
Now it works fine and the vdd-gpu is no more disabled.
Tested-by: Clément Péron <peron.clem@gmail.com>
Regards,
Clement
>
> Acked-by: Clément Péron <peron.clem@gmail.com>
>
> Regards,
> Clement
>
>
> >
> > --
> > viresh
> >
> > [1] https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/
> >
> > Kamil Konieczny (1):
> > opp: core: add regulators enable and disable
> >
> > Viresh Kumar (1):
> > opp: Reorder the code for !target_freq case
> >
> > drivers/opp/core.c | 39 ++++++++++++++++++++++++++++++++++-----
> > drivers/opp/opp.h | 2 ++
> > 2 files changed, 36 insertions(+), 5 deletions(-)
> >
> > --
> > 2.25.0.rc1.19.g042ed3e048af
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] opp: core: add regulators enable and disable
2020-05-21 12:23 ` Clément Péron
@ 2022-09-03 20:35 ` Clément Péron
2022-09-05 4:35 ` Viresh Kumar
0 siblings, 1 reply; 12+ messages in thread
From: Clément Péron @ 2022-09-03 20:35 UTC (permalink / raw)
To: Viresh Kumar
Cc: k.konieczny, Marek Szyprowski, Nishanth Menon, Stephen Boyd,
Viresh Kumar, open list:ALLWINNER CPUFREQ DRIVER,
Vincent Guittot, Rafael Wysocki, Mark Brown, linux-kernel
Hi Viresh,
On Thu, 21 May 2020 at 14:23, Clément Péron <peron.clem@gmail.com> wrote:
>
> Hi,
>
> On Fri, 15 May 2020 at 14:00, Clément Péron <peron.clem@gmail.com> wrote:
> >
> > Hi Viresh,
> >
> > On Fri, 15 May 2020 at 09:57, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > Hi,
> > >
> > > This series reintroduces the usage of regulator_enable/disable() to the
> > > OPP core after the previous attempt was reverted [1] shortly after getting
> > > applied. This time the regulator is enabled only after it is configured
> > > by the OPP core.
> > >
> > > Marek, Kamil and Clément: Can you guys please test this out and report
> > > if this doesn't work as expected ?
> >
> > I have reviewed the patch and it seems fine for my use case.
> > Unfortunately I can't test it until next week.
>
> Ok, before the patch the regulator was released by regulator_late_cleanup() :
> [ 33.756849] vdd-gpu: disabling
>
> Now it works fine and the vdd-gpu is no more disabled.
Today, I compiled my kernel without any program requiring GPU
computing at boot. This makes the dev_pm_opp_set_rate() to never be
called and so the regulator is not enabled before the regulator
framework switches off all the regulators that haven't been enabled.
Unfortunately switching off the GPU regulator makes my board hang..
I'm not sure what is the best approach to fix this.
Is it required that the dev_pm_opp_set_rate() must be called one time
at the GPU driver init?
Panfost already calls devfreq_recommended_opp() and dev_pm_opp_put()
but that doesn't trigger dev_pm_opp_set_rate().
Thanks for your help,
BR,
Clement
>
> Tested-by: Clément Péron <peron.clem@gmail.com>
>
> Regards,
> Clement
>
> >
> > Acked-by: Clément Péron <peron.clem@gmail.com>
> >
> > Regards,
> > Clement
> >
> >
> > >
> > > --
> > > viresh
> > >
> > > [1] https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/
> > >
> > > Kamil Konieczny (1):
> > > opp: core: add regulators enable and disable
> > >
> > > Viresh Kumar (1):
> > > opp: Reorder the code for !target_freq case
> > >
> > > drivers/opp/core.c | 39 ++++++++++++++++++++++++++++++++++-----
> > > drivers/opp/opp.h | 2 ++
> > > 2 files changed, 36 insertions(+), 5 deletions(-)
> > >
> > > --
> > > 2.25.0.rc1.19.g042ed3e048af
> > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] opp: core: add regulators enable and disable
2022-09-03 20:35 ` Clément Péron
@ 2022-09-05 4:35 ` Viresh Kumar
2022-09-05 8:28 ` Clément Péron
0 siblings, 1 reply; 12+ messages in thread
From: Viresh Kumar @ 2022-09-05 4:35 UTC (permalink / raw)
To: Clément Péron
Cc: k.konieczny, Marek Szyprowski, Nishanth Menon, Stephen Boyd,
Viresh Kumar, open list:ALLWINNER CPUFREQ DRIVER,
Vincent Guittot, Rafael Wysocki, Mark Brown, linux-kernel
On 03-09-22, 22:35, Clément Péron wrote:
> Today, I compiled my kernel without any program requiring GPU
> computing at boot. This makes the dev_pm_opp_set_rate() to never be
> called and so the regulator is not enabled before the regulator
> framework switches off all the regulators that haven't been enabled.
>
> Unfortunately switching off the GPU regulator makes my board hang..
Why does the board hang? I mean the kernel should boot fine with the
GPU disabled, isn't it ? Or is the regulator shared with some other
critical resource, or something else.
> I'm not sure what is the best approach to fix this.
>
> Is it required that the dev_pm_opp_set_rate() must be called one time
> at the GPU driver init?
Right now, Yes. And it looks like the right approach as well.
> Panfost already calls devfreq_recommended_opp() and dev_pm_opp_put()
> but that doesn't trigger dev_pm_opp_set_rate().
Can you also point to your code ? Which file are you working on ?
--
viresh
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] opp: core: add regulators enable and disable
2022-09-05 4:35 ` Viresh Kumar
@ 2022-09-05 8:28 ` Clément Péron
2022-09-05 10:24 ` Viresh Kumar
0 siblings, 1 reply; 12+ messages in thread
From: Clément Péron @ 2022-09-05 8:28 UTC (permalink / raw)
To: Viresh Kumar
Cc: k.konieczny, Marek Szyprowski, Nishanth Menon, Stephen Boyd,
Viresh Kumar, open list:ALLWINNER CPUFREQ DRIVER,
Vincent Guittot, Rafael Wysocki, Mark Brown, linux-kernel
Hi Viresh,
On Mon, 5 Sept 2022 at 06:35, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 03-09-22, 22:35, Clément Péron wrote:
> > Today, I compiled my kernel without any program requiring GPU
> > computing at boot. This makes the dev_pm_opp_set_rate() to never be
> > called and so the regulator is not enabled before the regulator
> > framework switches off all the regulators that haven't been enabled.
> >
> > Unfortunately switching off the GPU regulator makes my board hang..
>
> Why does the board hang? I mean the kernel should boot fine with the
> GPU disabled, isn't it ? Or is the regulator shared with some other
> critical resource, or something else.
The regulator is dedicated to the GPU and the board could certainly
run without GPU, the issue is that the driver (here panfrost) may do
some regular access to GPU memory (I suppose).
>
> > I'm not sure what is the best approach to fix this.
> >
> > Is it required that the dev_pm_opp_set_rate() must be called one time
> > at the GPU driver init?
>
> Right now, Yes. And it looks like the right approach as well.
>
> > Panfost already calls devfreq_recommended_opp() and dev_pm_opp_put()
> > but that doesn't trigger dev_pm_opp_set_rate().
>
> Can you also point to your code ? Which file are you working on ?
The code I'm pointing is panfrost_devfreq_init() in
drivers/gpu/drm/panfrost/panfrost_devfreq.c
Regards,
Clement
>
> --
> viresh
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] opp: core: add regulators enable and disable
2022-09-05 8:28 ` Clément Péron
@ 2022-09-05 10:24 ` Viresh Kumar
0 siblings, 0 replies; 12+ messages in thread
From: Viresh Kumar @ 2022-09-05 10:24 UTC (permalink / raw)
To: Clément Péron
Cc: k.konieczny, Marek Szyprowski, Nishanth Menon, Stephen Boyd,
Viresh Kumar, open list:ALLWINNER CPUFREQ DRIVER,
Vincent Guittot, Rafael Wysocki, Mark Brown, linux-kernel
On 05-09-22, 10:28, Clément Péron wrote:
> Hi Viresh,
>
> On Mon, 5 Sept 2022 at 06:35, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 03-09-22, 22:35, Clément Péron wrote:
> > > Today, I compiled my kernel without any program requiring GPU
> > > computing at boot.
I thought you disabled most of GPU stuff here and so thought panfrost
devfreq will be gone too :)
> > > This makes the dev_pm_opp_set_rate() to never be
> > > called and so the regulator is not enabled before the regulator
> > > framework switches off all the regulators that haven't been enabled.
> > >
> > > Unfortunately switching off the GPU regulator makes my board hang..
> >
> > Why does the board hang? I mean the kernel should boot fine with the
> > GPU disabled, isn't it ? Or is the regulator shared with some other
> > critical resource, or something else.
>
> The regulator is dedicated to the GPU and the board could certainly
> run without GPU, the issue is that the driver (here panfrost) may do
> some regular access to GPU memory (I suppose).
So we do need GPU to be functional at this point ? i.e. both
clk/regulators should be enabled.
> The code I'm pointing is panfrost_devfreq_init() in
> drivers/gpu/drm/panfrost/panfrost_devfreq.c
I think it would be better to call dev_pm_opp_set_opp() after calling
devfreq_recommended_opp() in this driver, since you already have the
OPP known to you.
--
viresh
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-09-05 10:25 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20200515075745eucas1p2f14c7fcec7c3d190704ddc6f608b6ce9@eucas1p2.samsung.com>
2020-05-15 7:57 ` [PATCH 0/2] opp: core: add regulators enable and disable Viresh Kumar
2020-05-15 7:57 ` [PATCH 1/2] opp: Reorder the code for !target_freq case Viresh Kumar
2020-05-15 14:34 ` kbuild test robot
2020-05-18 7:06 ` Viresh Kumar
2020-05-15 7:57 ` [PATCH 2/2] opp: core: add regulators enable and disable Viresh Kumar
2020-05-15 9:16 ` [PATCH 0/2] " Marek Szyprowski
2020-05-15 12:00 ` Clément Péron
2020-05-21 12:23 ` Clément Péron
2022-09-03 20:35 ` Clément Péron
2022-09-05 4:35 ` Viresh Kumar
2022-09-05 8:28 ` Clément Péron
2022-09-05 10:24 ` Viresh Kumar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.