opp: core: add regulators enable and disable

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 <[email protected]>

cppcheck warnings: (new ones prefixed by >>)

Quoted 1 line

Quoted 1 line

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

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 ?

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/[email protected]/

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

Hi Viresh,

Quoted 8 lines · Viresh Kumar

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.

Quoted 2 lines

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 <[email protected]>

Regards,
Clement

Quoted 19 lines

--
viresh

[1] https://lore.kernel.org/lkml/[email protected]/

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

Hi,

Quoted 15 lines · Clément Péron

Hi Viresh,

Quoted 10 lines · Viresh Kumar

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 ?

Quoted 2 lines

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 <[email protected]>

Regards,
Clement

Quoted 26 lines

Acked-by Clément Péron <[email protected]>

Regards,
Clement

Quoted 19 lines

--
viresh

[1] https://lore.kernel.org/lkml/[email protected]/

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

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().

Quoted 5 lines · Clément Péron

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.

Quoted 1 line

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.

Quoted 5 lines

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.

Quoted 2 lines

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

Hi Viresh,

Quoted 4 lines · Viresh Kumar

Quoted 2 lines · Clément Péron

Today, I compiled my kernel without any program requiring GPU computing at boot.

Quoted 6 lines

Quoted 5 lines

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..

Quoted 3 lines

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).

The code I'm pointing is panfrost_devfreq_init() in drivers/gpu/drm/panfrost/panfrost_devfreq.c