Question about OPP regulator for Panfrost Devfreq

Question about OPP regulator for Panfrost Devfreq

[Clément Péron]

Dear OPP Maintainers,

I'm working on adding DVFS support using the generic OPP framework to Panfrost.
I'm using the dev_pm_opp_set_regulators() to let OPP framework get and
manage the regulator.
https://github.com/clementperon/linux/commit/be310c37b82010e293b7f129ccdcb711a2abb2ce

However it seems that this function only get the regulator but never enable it.
This result that the regulator is disabled later by the
regulator_late_cleanup().

In a previous version I let the Panfrost driver to get and enable the
regulator in addition to OPP but this create a conflict in debugFS
because the regulator is "get" two times.

Quick discussion with Mark Brown point that we should try to avoid
getting two times a regulator as it can create "confusion in your code
with two different parts of the device controlling the same supply
independently."

Is my understanding correct? If yes,
Should we not add a call to regulator_enable() in the
dev_pm_opp_set_regulators() ?

My WIP branch :
https://github.com/clementperon/linux/commits/panfrost_devfreq

Thanks for your help,
Clement

Re: Question about OPP regulator for Panfrost Devfreq

[Viresh Kumar]

On 09-05-20, 21:56, Clément Péron wrote:
> Dear OPP Maintainers,
> 
> I'm working on adding DVFS support using the generic OPP framework to Panfrost.
> I'm using the dev_pm_opp_set_regulators() to let OPP framework get and
> manage the regulator.
> https://github.com/clementperon/linux/commit/be310c37b82010e293b7f129ccdcb711a2abb2ce
> 
> However it seems that this function only get the regulator but never enable it.
> This result that the regulator is disabled later by the
> regulator_late_cleanup().
> 
> In a previous version I let the Panfrost driver to get and enable the
> regulator in addition to OPP but this create a conflict in debugFS
> because the regulator is "get" two times.
> 
> Quick discussion with Mark Brown point that we should try to avoid
> getting two times a regulator as it can create "confusion in your code
> with two different parts of the device controlling the same supply
> independently."
> 
> Is my understanding correct? If yes,
> Should we not add a call to regulator_enable() in the
> dev_pm_opp_set_regulators() ?
> 
> My WIP branch :
> https://github.com/clementperon/linux/commits/panfrost_devfreq

https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/

We tried that sometime back and faced issues with it.

-- 
viresh

Re: Question about OPP regulator for Panfrost Devfreq

[Clément Péron]

Hi Viresh, Marek and Mark

On Mon, 11 May 2020 at 07:25, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 09-05-20, 21:56, Clément Péron wrote:
> > Dear OPP Maintainers,
> >
> > I'm working on adding DVFS support using the generic OPP framework to Panfrost.
> > I'm using the dev_pm_opp_set_regulators() to let OPP framework get and
> > manage the regulator.
> > https://github.com/clementperon/linux/commit/be310c37b82010e293b7f129ccdcb711a2abb2ce
> >
> > However it seems that this function only get the regulator but never enable it.
> > This result that the regulator is disabled later by the
> > regulator_late_cleanup().
> >
> > In a previous version I let the Panfrost driver to get and enable the
> > regulator in addition to OPP but this create a conflict in debugFS
> > because the regulator is "get" two times.
> >
> > Quick discussion with Mark Brown point that we should try to avoid
> > getting two times a regulator as it can create "confusion in your code
> > with two different parts of the device controlling the same supply
> > independently."
> >
> > Is my understanding correct? If yes,
> > Should we not add a call to regulator_enable() in the
> > dev_pm_opp_set_regulators() ?
> >
> > My WIP branch :
> > https://github.com/clementperon/linux/commits/panfrost_devfreq
>
> https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/
>
> We tried that sometime back and faced issues with it.

Regarding GPU it uses also the OPP framework and the regulator is not
always-on so it seems quick logic the use_count should be increased
for this regulator.

Does declaring the regulator as 'regulator-boot-on' could fix the issue?

Regards,
Clement


>
> --
> viresh

Re: Question about OPP regulator for Panfrost Devfreq

[Viresh Kumar]

On 12-05-20, 22:51, Clément Péron wrote:
> Hi Viresh, Marek and Mark
> 
> On Mon, 11 May 2020 at 07:25, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 09-05-20, 21:56, Clément Péron wrote:
> > > Dear OPP Maintainers,
> > >
> > > I'm working on adding DVFS support using the generic OPP framework to Panfrost.
> > > I'm using the dev_pm_opp_set_regulators() to let OPP framework get and
> > > manage the regulator.
> > > https://github.com/clementperon/linux/commit/be310c37b82010e293b7f129ccdcb711a2abb2ce
> > >
> > > However it seems that this function only get the regulator but never enable it.
> > > This result that the regulator is disabled later by the
> > > regulator_late_cleanup().
> > >
> > > In a previous version I let the Panfrost driver to get and enable the
> > > regulator in addition to OPP but this create a conflict in debugFS
> > > because the regulator is "get" two times.
> > >
> > > Quick discussion with Mark Brown point that we should try to avoid
> > > getting two times a regulator as it can create "confusion in your code
> > > with two different parts of the device controlling the same supply
> > > independently."
> > >
> > > Is my understanding correct? If yes,
> > > Should we not add a call to regulator_enable() in the
> > > dev_pm_opp_set_regulators() ?
> > >
> > > My WIP branch :
> > > https://github.com/clementperon/linux/commits/panfrost_devfreq
> >
> > https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/
> >
> > We tried that sometime back and faced issues with it.
> 
> Regarding GPU it uses also the OPP framework and the regulator is not
> always-on so it seems quick logic the use_count should be increased
> for this regulator.
> 
> Does declaring the regulator as 'regulator-boot-on' could fix the issue?

The bootloader would still be required to power it on first and it
would be wastage of power as GPU isn't getting used then..

What about doing runtime_pm_get/put() from the driver which should
enable/disable all power related stuff for the device ?

-- 
viresh

Re: Question about OPP regulator for Panfrost Devfreq

[Clément Péron]

Hi Viresh,

On Wed, 13 May 2020 at 07:46, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 12-05-20, 22:51, Clément Péron wrote:
> > Hi Viresh, Marek and Mark
> >
> > On Mon, 11 May 2020 at 07:25, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 09-05-20, 21:56, Clément Péron wrote:
> > > > Dear OPP Maintainers,
> > > >
> > > > I'm working on adding DVFS support using the generic OPP framework to Panfrost.
> > > > I'm using the dev_pm_opp_set_regulators() to let OPP framework get and
> > > > manage the regulator.
> > > > https://github.com/clementperon/linux/commit/be310c37b82010e293b7f129ccdcb711a2abb2ce
> > > >
> > > > However it seems that this function only get the regulator but never enable it.
> > > > This result that the regulator is disabled later by the
> > > > regulator_late_cleanup().
> > > >
> > > > In a previous version I let the Panfrost driver to get and enable the
> > > > regulator in addition to OPP but this create a conflict in debugFS
> > > > because the regulator is "get" two times.
> > > >
> > > > Quick discussion with Mark Brown point that we should try to avoid
> > > > getting two times a regulator as it can create "confusion in your code
> > > > with two different parts of the device controlling the same supply
> > > > independently."
> > > >
> > > > Is my understanding correct? If yes,
> > > > Should we not add a call to regulator_enable() in the
> > > > dev_pm_opp_set_regulators() ?
> > > >
> > > > My WIP branch :
> > > > https://github.com/clementperon/linux/commits/panfrost_devfreq
> > >
> > > https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/
> > >
> > > We tried that sometime back and faced issues with it.
> >
> > Regarding GPU it uses also the OPP framework and the regulator is not
> > always-on so it seems quick logic the use_count should be increased
> > for this regulator.
> >
> > Does declaring the regulator as 'regulator-boot-on' could fix the issue?
>
> The bootloader would still be required to power it on first and it
> would be wastage of power as GPU isn't getting used then..

Sorry,  my message was not clear, I was trying to see if there is a
way to fix exynos issue other than removing the regulator_enable().
I'm wondering if calling regulator_enable() on an reg which is
'regulator-boot-on' declared will trigger this issue.

>
> What about doing runtime_pm_get/put() from the driver which should
> enable/disable all power related stuff for the device ?

Not sure what you meant there but the regulator isn't get by the
driver itself, so the driver runtime_pm will call devfreq/opp to
properly set the regulator voltage.

I'm really not an expert on this but for what I understand I think
that removing the regulator_enable in the OPP framework to fix some
side-effects is not the proper fix.
I wonder if we can get where the voltage is changed and why? Maybe
there is a proper fix like setting the required OPP voltage before
enabling the regulator or declaring the regulator as
'regulator-boot-on'.

Regards,
Clement


>
> --
> viresh

Re: Question about OPP regulator for Panfrost Devfreq

[Viresh Kumar]

On 13-05-20, 10:18, Clément Péron wrote:
> Sorry,  my message was not clear, I was trying to see if there is a
> way to fix exynos issue other than removing the regulator_enable().
> I'm wondering if calling regulator_enable() on an reg which is
> 'regulator-boot-on' declared will trigger this issue.

Ahh, that's actually the real problem. Enabling the regulator which is
already enabled by the boot loader has some side effects explained in
the below thread and so the patch was quickly reverted after being
applied.

https://lkml.org/lkml/2019/10/8/459

> > What about doing runtime_pm_get/put() from the driver which should
> > enable/disable all power related stuff for the device ?

Actually runtime_pm_get() may not affect the regulator, but just the
pm domain.

> I'm really not an expert on this but for what I understand I think
> that removing the regulator_enable in the OPP framework to fix some
> side-effects is not the proper fix.

It was removed quickly after we added it. And everyone knew that it
isn't the best fix possible :(

> I wonder if we can get where the voltage is changed and why? Maybe
> there is a proper fix like setting the required OPP voltage before
> enabling the regulator or declaring the regulator as
> 'regulator-boot-on'.

I am not sure how to fix that problem, but ...

@Mark: Regarding enabling/disabling regulators from the OPP core, what
about skipping that only for the regulators that are enabled at boot,
i.e. to avoid the issue we faced earlier, (by exporting a helper from
regulator core for that first) and then applying this patch again: 

https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/

-- 
viresh

Re: Question about OPP regulator for Panfrost Devfreq

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

On Wed, May 13, 2020 at 02:49:22PM +0530, Viresh Kumar wrote:

> @Mark: Regarding enabling/disabling regulators from the OPP core, what

My name is spelt Mark.

> about skipping that only for the regulators that are enabled at boot,
> i.e. to avoid the issue we faced earlier, (by exporting a helper from
> regulator core for that first) and then applying this patch again: 

As ever if you have requirements for the voltage of a regulator you
should use regulator_set_voltage() to tell the core about it.  The core
cannot be expected to infer these requirements without being told by the
users.  If you need the voltage to be a particular level when you enable
the regulator you should set that voltage.  Why can't the code do that
instead of trying to add these complex and fragile bodges?  Randomly
skipping applying configurations some of the time is not going to make
anything more robust or easier to understand.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Question about OPP regulator for Panfrost Devfreq

[Viresh Kumar]

On 13-05-20, 11:18, Mark Brown wrote:
> On Wed, May 13, 2020 at 02:49:22PM +0530, Viresh Kumar wrote:
> 
> > @Mark: Regarding enabling/disabling regulators from the OPP core, what
> 
> My name is spelt Mark.

Hmm. But isn't that what I wrote ? I used @ as that's what people use
to address questions to someone. :)

> > about skipping that only for the regulators that are enabled at boot,
> > i.e. to avoid the issue we faced earlier, (by exporting a helper from
> > regulator core for that first) and then applying this patch again: 
> 
> As ever if you have requirements for the voltage of a regulator you
> should use regulator_set_voltage() to tell the core about it.  The core
> cannot be expected to infer these requirements without being told by the
> users.  If you need the voltage to be a particular level when you enable
> the regulator you should set that voltage.  Why can't the code do that
> instead of trying to add these complex and fragile bodges?  Randomly
> skipping applying configurations some of the time is not going to make
> anything more robust or easier to understand.

Right.

Clément is facing a problem where his regulator isn't getting enabled
and he was asking why the OPP core can't call regulator_enable(), when
it handles everything else around them. Can you suggest to him on what
he should be doing here ?

Thanks.

-- 
viresh

Re: Question about OPP regulator for Panfrost Devfreq

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

On Wed, May 13, 2020 at 04:10:15PM +0530, Viresh Kumar wrote:
> On 13-05-20, 11:18, Mark Brown wrote:
> > On Wed, May 13, 2020 at 02:49:22PM +0530, Viresh Kumar wrote:

> > > @Mark: Regarding enabling/disabling regulators from the OPP core, what

> > My name is spelt Mark.

> Hmm. But isn't that what I wrote ? I used @ as that's what people use
> to address questions to someone. :)

No, it isn't what people use to address a question to someone.  Some
social media uses it to prefix usernames but email is not one of those
systems.  If anything it makes it *less* likely that people will see
things as people mostly read by pattern matching the shapes of words
(especially when scanning) rather than spelling out individual letters.

> > As ever if you have requirements for the voltage of a regulator you
> > should use regulator_set_voltage() to tell the core about it.  The core
> > cannot be expected to infer these requirements without being told by the
> > users.  If you need the voltage to be a particular level when you enable
> > the regulator you should set that voltage.  Why can't the code do that
> > instead of trying to add these complex and fragile bodges?  Randomly
> > skipping applying configurations some of the time is not going to make
> > anything more robust or easier to understand.

> Right.

> Clément is facing a problem where his regulator isn't getting enabled
> and he was asking why the OPP core can't call regulator_enable(), when
> it handles everything else around them. Can you suggest to him on what
> he should be doing here ?

The OPP code can and should be calling regulator_enable().  If the OPP
code needs some particular voltage configuration it can and should be
calling regulator_set_voltage() to tell the core what it needs.  What
I'm saying is that if when the OPP code enables the regulator it needs
a particular voltage configuration it should tell the core about that
prior to enabling.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Question about OPP regulator for Panfrost Devfreq

[Viresh Kumar]

On 13-05-20, 12:00, Mark Brown wrote:
> The OPP code can and should be calling regulator_enable().  If the OPP
> code needs some particular voltage configuration it can and should be
> calling regulator_set_voltage() to tell the core what it needs.  What
> I'm saying is that if when the OPP code enables the regulator it needs
> a particular voltage configuration it should tell the core about that
> prior to enabling.

I see. Thanks for that.

Clement: I will prepare a patch for you to test then.

-- 
viresh

Re: Question about OPP regulator for Panfrost Devfreq

[Clément Péron]

Hi Viresh,

On Wed, 13 May 2020 at 13:03, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 13-05-20, 12:00, Mark Brown wrote:
> > The OPP code can and should be calling regulator_enable().  If the OPP
> > code needs some particular voltage configuration it can and should be
> > calling regulator_set_voltage() to tell the core what it needs.  What
> > I'm saying is that if when the OPP code enables the regulator it needs
> > a particular voltage configuration it should tell the core about that
> > prior to enabling.
>
> I see. Thanks for that.
>
> Clement: I will prepare a patch for you to test then.

Thanks for taking care of that, I'm sure it will works fine for me,
but maybe it should be double tested by Marek Szyprowski on the exynos
bus.

Thanks
Clement

>
> --
> viresh