[PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Mark Brown]

The panfrost driver requests a supply using regulator_get_optional()
but both the name of the supply and the usage pattern suggest that it is
being used for the main power for the device and is not at all optional
for the device for function, there is no meaningful handling for absent
supplies.  Such regulators should use the vanilla regulator_get()
interface, it will ensure that even if a supply is not described in the
system integration one will be provided in software.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 46b0b02e4289..238fb6d54df4 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -89,12 +89,9 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)
 {
 	int ret;
 
-	pfdev->regulator = devm_regulator_get_optional(pfdev->dev, "mali");
+	pfdev->regulator = devm_regulator_get(pfdev->dev, "mali");
 	if (IS_ERR(pfdev->regulator)) {
 		ret = PTR_ERR(pfdev->regulator);
-		pfdev->regulator = NULL;
-		if (ret == -ENODEV)
-			return 0;
 		dev_err(pfdev->dev, "failed to get regulator: %d\n", ret);
 		return ret;
 	}
@@ -110,8 +107,7 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)
 
 static void panfrost_regulator_fini(struct panfrost_device *pfdev)
 {
-	if (pfdev->regulator)
-		regulator_disable(pfdev->regulator);
+	regulator_disable(pfdev->regulator);
 }
 
 int panfrost_device_init(struct panfrost_device *pfdev)
-- 
2.20.1

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Rob Herring]

+Steven

On Wed, Sep 4, 2019 at 1:30 PM Mark Brown <broonie@kernel.org> wrote:
>
> The panfrost driver requests a supply using regulator_get_optional()
> but both the name of the supply and the usage pattern suggest that it is
> being used for the main power for the device and is not at all optional
> for the device for function, there is no meaningful handling for absent
> supplies.  Such regulators should use the vanilla regulator_get()
> interface, it will ensure that even if a supply is not described in the
> system integration one will be provided in software.

I guess commits e21dd290881b ("drm/panfrost: Enable devfreq to work
without regulator") and c90f30812a79 ("drm/panfrost: Add missing check
for pfdev->regulator")
in -next should be reverted or partially reverted?

Rob

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Steven Price]

On 05/09/2019 09:21, Rob Herring wrote:
> +Steven
> 
> On Wed, Sep 4, 2019 at 1:30 PM Mark Brown <broonie@kernel.org> wrote:
>>
>> The panfrost driver requests a supply using regulator_get_optional()
>> but both the name of the supply and the usage pattern suggest that it is
>> being used for the main power for the device and is not at all optional
>> for the device for function, there is no meaningful handling for absent
>> supplies.  Such regulators should use the vanilla regulator_get()
>> interface, it will ensure that even if a supply is not described in the
>> system integration one will be provided in software.
> 
> I guess commits e21dd290881b ("drm/panfrost: Enable devfreq to work
> without regulator") and c90f30812a79 ("drm/panfrost: Add missing check
> for pfdev->regulator")
> in -next should be reverted or partially reverted?

Ah, I didn't realise that regulator_get() will return a dummy regulator
if none is provided in the DT. In theory that seems like a nicer
solution to my two commits. However there's still a problem - the dummy
regulator returned from regulator_get() reports errors when
regulator_set_voltage() is called. So I get errors like this:

[  299.861165] panfrost e82c0000.mali: Cannot set voltage 1100000 uV
[  299.867294] devfreq devfreq0: dvfs failed with (-22) error

(And therefore the frequency isn't being changed)

Ideally we want a dummy regulator that will silently ignore any
regulator_set_voltage() calls.

Steve

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Mark Brown]

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

On Thu, Sep 05, 2019 at 10:37:53AM +0100, Steven Price wrote:

> Ah, I didn't realise that regulator_get() will return a dummy regulator
> if none is provided in the DT. In theory that seems like a nicer
> solution to my two commits. However there's still a problem - the dummy
> regulator returned from regulator_get() reports errors when
> regulator_set_voltage() is called. So I get errors like this:

> [  299.861165] panfrost e82c0000.mali: Cannot set voltage 1100000 uV
> [  299.867294] devfreq devfreq0: dvfs failed with (-22) error

> (And therefore the frequency isn't being changed)

> Ideally we want a dummy regulator that will silently ignore any
> regulator_set_voltage() calls.

Is that safe?  You can't rely on being able to change voltages even if
there's a physical regulator available, system constraints or the
results of sharing the regulator with other users may prevent changes.
I guess at the minute the code is assuming that if you can't vary the
regulator it's fixed at the maximum voltage and that it's safe to run at
a lower clock with a higher voltage (some devices don't like doing that).
If the device always starts up at full speed I guess that's OK.  It's
certainly in general a bad idea to do this in general, we can't tell how
important it is to the consumer that they actually get the voltage that
they asked for - for some applications like this it's just adding to the
power saving it's likely fine but for others it might break things.

If you're happy to change the frequency without the ability to vary the
voltage you can query what's supported through the API (the simplest
interface is regulator_is_supported_voltage()).  You should do the
regulator API queries at initialization time since they can be a bit
expensive, the usual pattern would be to go through your OPP table and
disable states where you can't support the voltage but you *could* also
flag states where you just don't set the voltage.  That seems especially
reasonable if no voltages in the range the device supports can be set.

I do note that the current code requires exactly specified voltages with
no variation which doesn't match the behaviour you say you're OK with
here, what you're describing sounds like the driver should be specifying
a voltage range from the hardware specified maximum down to whatever the
minimum the OPP supports rather than exactly the OPP voltage.  As things
are you might also run into voltages that can't be hit exactly (eg, in
the Exynos 5433 case in mainline a regulator that only offers steps of
2mV will error out trying to set several of the OPPs).

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

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Steven Price]

On 05/09/2019 13:40, Mark Brown wrote:
> On Thu, Sep 05, 2019 at 10:37:53AM +0100, Steven Price wrote:
> 
>> Ah, I didn't realise that regulator_get() will return a dummy regulator
>> if none is provided in the DT. In theory that seems like a nicer
>> solution to my two commits. However there's still a problem - the dummy
>> regulator returned from regulator_get() reports errors when
>> regulator_set_voltage() is called. So I get errors like this:
> 
>> [  299.861165] panfrost e82c0000.mali: Cannot set voltage 1100000 uV
>> [  299.867294] devfreq devfreq0: dvfs failed with (-22) error
> 
>> (And therefore the frequency isn't being changed)
> 
>> Ideally we want a dummy regulator that will silently ignore any
>> regulator_set_voltage() calls.
> 
> Is that safe?  You can't rely on being able to change voltages even if
> there's a physical regulator available, system constraints or the
> results of sharing the regulator with other users may prevent changes.

Perhaps I didn't express myself clearly. I mean that in the case of the
Hikey960 it would be convenient to have a "dummy regulator" that simply
accepted any change because ultimately Linux doesn't have direct control
of the voltages but simply requests a particular operating frequency via
the mailbox.

> I guess at the minute the code is assuming that if you can't vary the
> regulator it's fixed at the maximum voltage and that it's safe to run at
> a lower clock with a higher voltage (some devices don't like doing that).

No - at the moment if the regulator reports an error then the function
bails out and doesn't change the frequency.

> If the device always starts up at full speed I guess that's OK.  It's
> certainly in general a bad idea to do this in general, we can't tell how
> important it is to the consumer that they actually get the voltage that
> they asked for - for some applications like this it's just adding to the
> power saving it's likely fine but for others it might break things.

Agreed.

> If you're happy to change the frequency without the ability to vary the
> voltage you can query what's supported through the API (the simplest
> interface is regulator_is_supported_voltage()).  You should do the
> regulator API queries at initialization time since they can be a bit
> expensive, the usual pattern would be to go through your OPP table and
> disable states where you can't support the voltage but you *could* also
> flag states where you just don't set the voltage.  That seems especially
> reasonable if no voltages in the range the device supports can be set.
> 
> I do note that the current code requires exactly specified voltages with
> no variation which doesn't match the behaviour you say you're OK with
> here, what you're describing sounds like the driver should be specifying
> a voltage range from the hardware specified maximum down to whatever the
> minimum the OPP supports rather than exactly the OPP voltage.  As things
> are you might also run into voltages that can't be hit exactly (eg, in
> the Exynos 5433 case in mainline a regulator that only offers steps of
> 2mV will error out trying to set several of the OPPs).

Perhaps there's a better way of doing devfreq? Panfrost itself doesn't
really care must about this - we just need to be able to scaling up/down
the operating point depending on load.

On many platforms to set the frequency it's necessary to do the dance to
set an appropriate voltage before/afterwards, but on the Hikey960
because this is handled via a mailbox we don't actually have a regulator
to set the voltage on. My commit[1] supports this by simply not listing
the regulator in the DT and assuming that nothing is needed when
switching frequency. I'm happy for some other way of handling this if
there's a better method.

At the moment your change from devm_regulator_get_optional() to
devm_regulator_get() is a regression on this platform because it means
there is now a dummy regulator which will always fail the
regulator_set_voltage() calls preventing frequency changes. And I can't
see anything I can do in the DT to fix that.

Steve

[1] e21dd290881b drm/panfrost: Enable devfreq to work without regulator
(plus bug fix in 52282163dfa6)

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Neil Armstrong]

On 05/09/2019 15:02, Steven Price wrote:
> On 05/09/2019 13:40, Mark Brown wrote:
>> On Thu, Sep 05, 2019 at 10:37:53AM +0100, Steven Price wrote:
>>
>>> Ah, I didn't realise that regulator_get() will return a dummy regulator
>>> if none is provided in the DT. In theory that seems like a nicer
>>> solution to my two commits. However there's still a problem - the dummy
>>> regulator returned from regulator_get() reports errors when
>>> regulator_set_voltage() is called. So I get errors like this:
>>
>>> [  299.861165] panfrost e82c0000.mali: Cannot set voltage 1100000 uV
>>> [  299.867294] devfreq devfreq0: dvfs failed with (-22) error
>>
>>> (And therefore the frequency isn't being changed)
>>
>>> Ideally we want a dummy regulator that will silently ignore any
>>> regulator_set_voltage() calls.
>>
>> Is that safe?  You can't rely on being able to change voltages even if
>> there's a physical regulator available, system constraints or the
>> results of sharing the regulator with other users may prevent changes.
> 
> Perhaps I didn't express myself clearly. I mean that in the case of the
> Hikey960 it would be convenient to have a "dummy regulator" that simply
> accepted any change because ultimately Linux doesn't have direct control
> of the voltages but simply requests a particular operating frequency via
> the mailbox.
> 
>> I guess at the minute the code is assuming that if you can't vary the
>> regulator it's fixed at the maximum voltage and that it's safe to run at
>> a lower clock with a higher voltage (some devices don't like doing that).
> 
> No - at the moment if the regulator reports an error then the function
> bails out and doesn't change the frequency.
> 
>> If the device always starts up at full speed I guess that's OK.  It's
>> certainly in general a bad idea to do this in general, we can't tell how
>> important it is to the consumer that they actually get the voltage that
>> they asked for - for some applications like this it's just adding to the
>> power saving it's likely fine but for others it might break things.
> 
> Agreed.
> 
>> If you're happy to change the frequency without the ability to vary the
>> voltage you can query what's supported through the API (the simplest
>> interface is regulator_is_supported_voltage()).  You should do the
>> regulator API queries at initialization time since they can be a bit
>> expensive, the usual pattern would be to go through your OPP table and
>> disable states where you can't support the voltage but you *could* also
>> flag states where you just don't set the voltage.  That seems especially
>> reasonable if no voltages in the range the device supports can be set.
>>
>> I do note that the current code requires exactly specified voltages with
>> no variation which doesn't match the behaviour you say you're OK with
>> here, what you're describing sounds like the driver should be specifying
>> a voltage range from the hardware specified maximum down to whatever the
>> minimum the OPP supports rather than exactly the OPP voltage.  As things
>> are you might also run into voltages that can't be hit exactly (eg, in
>> the Exynos 5433 case in mainline a regulator that only offers steps of
>> 2mV will error out trying to set several of the OPPs).

In case we need a fixed voltage, simply add a new fixed-regulator in the board/soc DT,
use this regulator for panfrost and use the same fixed voltage in the OPP table.

It relies on a correct DT, but isn't is the goal of mainline linux ?

Neil

> 
> Perhaps there's a better way of doing devfreq? Panfrost itself doesn't
> really care must about this - we just need to be able to scaling up/down
> the operating point depending on load.
> 
> On many platforms to set the frequency it's necessary to do the dance to
> set an appropriate voltage before/afterwards, but on the Hikey960
> because this is handled via a mailbox we don't actually have a regulator
> to set the voltage on. My commit[1] supports this by simply not listing
> the regulator in the DT and assuming that nothing is needed when
> switching frequency. I'm happy for some other way of handling this if
> there's a better method.
> 
> At the moment your change from devm_regulator_get_optional() to
> devm_regulator_get() is a regression on this platform because it means
> there is now a dummy regulator which will always fail the
> regulator_set_voltage() calls preventing frequency changes. And I can't
> see anything I can do in the DT to fix that.
> 
> Steve
> 
> [1] e21dd290881b drm/panfrost: Enable devfreq to work without regulator
> (plus bug fix in 52282163dfa6)
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Mark Brown]

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

On Thu, Sep 05, 2019 at 02:02:38PM +0100, Steven Price wrote:
> On 05/09/2019 13:40, Mark Brown wrote:

> > Is that safe?  You can't rely on being able to change voltages even if
> > there's a physical regulator available, system constraints or the
> > results of sharing the regulator with other users may prevent changes.

> Perhaps I didn't express myself clearly. I mean that in the case of the
> Hikey960 it would be convenient to have a "dummy regulator" that simply
> accepted any change because ultimately Linux doesn't have direct control
> of the voltages but simply requests a particular operating frequency via
> the mailbox.

There's more platforms than just HiKey supported here though, I'm pretty
sure some of them don't have the regulator under firmware control (and
HiKey doesn't seem to have this device enabled upstream at all?).

> > I guess at the minute the code is assuming that if you can't vary the
> > regulator it's fixed at the maximum voltage and that it's safe to run at
> > a lower clock with a higher voltage (some devices don't like doing that).

> No - at the moment if the regulator reports an error then the function
> bails out and doesn't change the frequency.

I'm talking about the case where you didn't get a regulator at all where
it won't even try to set anything (ie, current behaviour).

> > I do note that the current code requires exactly specified voltages with
> > no variation which doesn't match the behaviour you say you're OK with
> > here, what you're describing sounds like the driver should be specifying
> > a voltage range from the hardware specified maximum down to whatever the
> > minimum the OPP supports rather than exactly the OPP voltage.  As things
> > are you might also run into voltages that can't be hit exactly (eg, in
> > the Exynos 5433 case in mainline a regulator that only offers steps of
> > 2mV will error out trying to set several of the OPPs).

> Perhaps there's a better way of doing devfreq? Panfrost itself doesn't
> really care must about this - we just need to be able to scaling up/down
> the operating point depending on load.

The idiomatic thing for this sort of usage would be to set the voltage
to a range between the minimum voltage the OPP can support and the
maximum the hardware can support.  That's basically saying "try to set
the voltage to the lowest thing between this minimum and maximum" which
seems to be about what you're asking for here.

> On many platforms to set the frequency it's necessary to do the dance to
> set an appropriate voltage before/afterwards, but on the Hikey960
> because this is handled via a mailbox we don't actually have a regulator
> to set the voltage on. My commit[1] supports this by simply not listing
> the regulator in the DT and assuming that nothing is needed when
> switching frequency. I'm happy for some other way of handling this if
> there's a better method.

> At the moment your change from devm_regulator_get_optional() to
> devm_regulator_get() is a regression on this platform because it means
> there is now a dummy regulator which will always fail the
> regulator_set_voltage() calls preventing frequency changes. And I can't
> see anything I can do in the DT to fix that.

Like I say that system doesn't have any enablement at all for thse
devices upstream that I can see, the only thing with any OPPs is the
Exynos 5433 which does have a regulator.

The simplest thing to do what you're trying to do inside the driver is
the approach I suggested in my previous mail with checking to see what
voltages are actually supported on the system and do something with
that information, I'd recommend eliminating individual OPPs if some are
supported or just never doing any regulator configuration if none can be
set.

However you're probably better off hiding all this stuff with the
generic OPP code rather than open coding it - this already has much
better handling for this, it supports voltage ranges rather than single
voltages and optional regulators already.  I'm not 100% clear why this
is open coded TBH but I might be missing something, if there's some
restriction preventing the generic code being used it seems like those
sohuld be fixed.

In the short term I'd also strongly suggest adding documentation to the
code so it's clear that there's some intentionality to this, at the
minute it does not appear at all intentional.

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

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Steven Price]

(+CC Rob - I'm not sure why he was dropped)

On 05/09/2019 17:34, Mark Brown wrote:
> On Thu, Sep 05, 2019 at 02:02:38PM +0100, Steven Price wrote:
>> On 05/09/2019 13:40, Mark Brown wrote:
> 
>>> Is that safe?  You can't rely on being able to change voltages even if
>>> there's a physical regulator available, system constraints or the
>>> results of sharing the regulator with other users may prevent changes.
> 
>> Perhaps I didn't express myself clearly. I mean that in the case of the
>> Hikey960 it would be convenient to have a "dummy regulator" that simply
>> accepted any change because ultimately Linux doesn't have direct control
>> of the voltages but simply requests a particular operating frequency via
>> the mailbox.
> 
> There's more platforms than just HiKey supported here though, I'm pretty
> sure some of them don't have the regulator under firmware control (and
> HiKey doesn't seem to have this device enabled upstream at all?).

Yes there are platforms that have a regulator under Linux's control. On
those devm_regulator_get(_optional) will of course return that regulator
and the panfrost driver will use regulator_set_voltage() to set the
voltage as appropriate.

You are also correct that HiKey does not (yet) have this enabled
upstream - hence my questions about whether there is a better way of
representing this in device tree than just omitting the regulator.

>>> I guess at the minute the code is assuming that if you can't vary the
>>> regulator it's fixed at the maximum voltage and that it's safe to run at
>>> a lower clock with a higher voltage (some devices don't like doing that).
> 
>> No - at the moment if the regulator reports an error then the function
>> bails out and doesn't change the frequency.
> 
> I'm talking about the case where you didn't get a regulator at all where
> it won't even try to set anything (ie, current behaviour).

Ok, the current code in drm-misc will indeed not try to set anything if
there's no regulator.

>>> I do note that the current code requires exactly specified voltages with
>>> no variation which doesn't match the behaviour you say you're OK with
>>> here, what you're describing sounds like the driver should be specifying
>>> a voltage range from the hardware specified maximum down to whatever the
>>> minimum the OPP supports rather than exactly the OPP voltage.  As things
>>> are you might also run into voltages that can't be hit exactly (eg, in
>>> the Exynos 5433 case in mainline a regulator that only offers steps of
>>> 2mV will error out trying to set several of the OPPs).
> 
>> Perhaps there's a better way of doing devfreq? Panfrost itself doesn't
>> really care must about this - we just need to be able to scaling up/down
>> the operating point depending on load.
> 
> The idiomatic thing for this sort of usage would be to set the voltage
> to a range between the minimum voltage the OPP can support and the
> maximum the hardware can support.  That's basically saying "try to set
> the voltage to the lowest thing between this minimum and maximum" which
> seems to be about what you're asking for here.

It's not my present concern - but it may be worth changing the calls to
regulator_set_voltage to specify a range as you suggest.

>> On many platforms to set the frequency it's necessary to do the dance to
>> set an appropriate voltage before/afterwards, but on the Hikey960
>> because this is handled via a mailbox we don't actually have a regulator
>> to set the voltage on. My commit[1] supports this by simply not listing
>> the regulator in the DT and assuming that nothing is needed when
>> switching frequency. I'm happy for some other way of handling this if
>> there's a better method.
> 
>> At the moment your change from devm_regulator_get_optional() to
>> devm_regulator_get() is a regression on this platform because it means
>> there is now a dummy regulator which will always fail the
>> regulator_set_voltage() calls preventing frequency changes. And I can't
>> see anything I can do in the DT to fix that.
> 
> Like I say that system doesn't have any enablement at all for thse
> devices upstream that I can see, the only thing with any OPPs is the
> Exynos 5433 which does have a regulator.
> 
> The simplest thing to do what you're trying to do inside the driver is
> the approach I suggested in my previous mail with checking to see what
> voltages are actually supported on the system and do something with
> that information, I'd recommend eliminating individual OPPs if some are
> supported or just never doing any regulator configuration if none can be
> set.

The problem on the Hikey960 is that the voltage control is not done by
Linux. At the moment I have a DT with a set of operating-points:

	operating-points = <
		/* <frequency> <voltage>*/
		178000  650000
		400000	700000
		533000	800000
		807000	900000
		960000	1000000
		1037000 1100000
	>;

But while Linux can set the frequency (via the mailbox interface) the
voltages are not set by Linux but are implicit by choosing a frequency.
At the moment my DT has a clock but no regulator and with the code in
drm-next this works.

Your change swapping devm_regulator_get_optional() to
devm_regulator_get() breaks this because that will return a dummy
regulator which will reject any regulator_set_voltage() calls.

I don't currently see how I can write a DT configuration for the
Hikey960 which would work with the devm_regulator_get() call.

> However you're probably better off hiding all this stuff with the
> generic OPP code rather than open coding it - this already has much
> better handling for this, it supports voltage ranges rather than single
> voltages and optional regulators already.  I'm not 100% clear why this
> is open coded TBH but I might be missing something, if there's some
> restriction preventing the generic code being used it seems like those
> sohuld be fixed.

To be honest I've no idea how to use the generic OPP code to do this. I
suspect the original open coding was cargo-culted from another driver:
the comments in the function look like they were lifted from
drivers/devfreq/rk3399_dmc.c. Any help tidying this up would be appreciated.

> In the short term I'd also strongly suggest adding documentation to the
> code so it's clear that there's some intentionality to this, at the
> minute it does not appear at all intentional.

Good point - although if it's possible to switch to generic OPP code
that would be even better.

Thanks,

Steve

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Mark Brown]

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

On Fri, Sep 06, 2019 at 11:00:53AM +0100, Steven Price wrote:
> On 05/09/2019 17:34, Mark Brown wrote:

> > that information, I'd recommend eliminating individual OPPs if some are
> > supported or just never doing any regulator configuration if none can be
> > set.

> The problem on the Hikey960 is that the voltage control is not done by
> Linux. At the moment I have a DT with a set of operating-points:

Like I said above:

| > > that information, I'd recommend eliminating individual OPPs if some are
| > > supported or just never doing any regulator configuration if none can be
| > > set.

> But while Linux can set the frequency (via the mailbox interface) the
> voltages are not set by Linux but are implicit by choosing a frequency.
> At the moment my DT has a clock but no regulator and with the code in
> drm-next this works.

If you're just not getting any voltages for the OPPs then your
code shouldn't be trying to set voltages in the first place,
regulator or not.

> Your change swapping devm_regulator_get_optional() to
> devm_regulator_get() breaks this because that will return a dummy
> regulator which will reject any regulator_set_voltage() calls.

OTOH the current code won't work on a system which does specify a
regulator but doesn't have voltages configured in the OPP table
or where the regulator constraints for the board haven't been
configured to allow the voltage to be varied (perhaps the voltage
bit hasn't been worked out, perhaps the voltages were just added
to the OPPs in the SoC DT and the board constraints weren't
updated to allow voltage variation).

> I don't currently see how I can write a DT configuration for the
> Hikey960 which would work with the devm_regulator_get() call.

Like I've been saying you can discover if you can configure
individual voltages and use that information to either suppress
the OPPs concerned or just skip setting voltages for them, my
suggestion is to suppress OPPs only if you can set some of them.
At the very least if you don't have a voltage at all on a given
OPP you should skip the set.

> > However you're probably better off hiding all this stuff with the
> > generic OPP code rather than open coding it - this already has much
> > better handling for this, it supports voltage ranges rather than single
> > voltages and optional regulators already.  I'm not 100% clear why this
> > is open coded TBH but I might be missing something, if there's some
> > restriction preventing the generic code being used it seems like those
> > sohuld be fixed.

> To be honest I've no idea how to use the generic OPP code to do this. I
> suspect the original open coding was cargo-culted from another driver:
> the comments in the function look like they were lifted from
> drivers/devfreq/rk3399_dmc.c. Any help tidying this up would be appreciated.

Yes, there's a lot of cargo culting of bad regulator API usage in
the DRM subsystem for some reason, I keep having to do these
periodic sweeps and there's always stuff in there.  I think a lot
of it comes from BSP code that just gets dropped in without
review and then cut'n'pasted but I've not figured out why DRM is
so badly affected.

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

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Steven Price]

On 06/09/2019 11:55, Mark Brown wrote:
[...]
>>> However you're probably better off hiding all this stuff with the
>>> generic OPP code rather than open coding it - this already has much
>>> better handling for this, it supports voltage ranges rather than single
>>> voltages and optional regulators already.  I'm not 100% clear why this
>>> is open coded TBH but I might be missing something, if there's some
>>> restriction preventing the generic code being used it seems like those
>>> sohuld be fixed.
> 
>> To be honest I've no idea how to use the generic OPP code to do this. I
>> suspect the original open coding was cargo-culted from another driver:
>> the comments in the function look like they were lifted from
>> drivers/devfreq/rk3399_dmc.c. Any help tidying this up would be appreciated.
> 
> Yes, there's a lot of cargo culting of bad regulator API usage in
> the DRM subsystem for some reason, I keep having to do these
> periodic sweeps and there's always stuff in there.  I think a lot
> of it comes from BSP code that just gets dropped in without
> review and then cut'n'pasted but I've not figured out why DRM is
> so badly affected.

I've been working on tidying up the devfreq usage in Panfrost. From what
I can see your patch is correct and I just needed to work out how to fix
my DT.

Steve

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Steven Price]

On 04/09/2019 13:30, Mark Brown wrote:
> The panfrost driver requests a supply using regulator_get_optional()
> but both the name of the supply and the usage pattern suggest that it is
> being used for the main power for the device and is not at all optional
> for the device for function, there is no meaningful handling for absent
> supplies.  Such regulators should use the vanilla regulator_get()
> interface, it will ensure that even if a supply is not described in the
> system integration one will be provided in software.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Tested-by: Steven Price <steven.price@arm.com>

Looks like my approach to this was wrong - so we should also revert the
changes I made previously.

----8<----
From fe20f8abcde8444bb41a8f72fb35de943a27ec5c Mon Sep 17 00:00:00 2001
From: Steven Price <steven.price@arm.com>
Date: Fri, 6 Sep 2019 15:20:53 +0100
Subject: [PATCH] drm/panfrost: Revert changes to cope with NULL regulator

Handling a NULL return from devm_regulator_get_optional() doesn't seem
like the correct way of handling this. Instead revert the changes in
favour of switching to using devm_regulator_get() which will return a
dummy regulator instead.

Reverts commit 52282163dfa6 ("drm/panfrost: Add missing check for pfdev->regulator")
Reverts commit e21dd290881b ("drm/panfrost: Enable devfreq to work without regulator")

Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index a1f5fa6a742a..076983071e58 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -39,7 +39,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 	 * If frequency scaling from low to high, adjust voltage first.
 	 * If frequency scaling from high to low, adjust frequency first.
 	 */
-	if (old_clk_rate < target_rate && pfdev->regulator) {
+	if (old_clk_rate < target_rate) {
 		err = regulator_set_voltage(pfdev->regulator, target_volt,
 					    target_volt);
 		if (err) {
@@ -53,14 +53,12 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 	if (err) {
 		dev_err(dev, "Cannot set frequency %lu (%d)\n", target_rate,
 			err);
-		if (pfdev->regulator)
-			regulator_set_voltage(pfdev->regulator,
-					      pfdev->devfreq.cur_volt,
-					      pfdev->devfreq.cur_volt);
+		regulator_set_voltage(pfdev->regulator, pfdev->devfreq.cur_volt,
+				      pfdev->devfreq.cur_volt);
 		return err;
 	}
 
-	if (old_clk_rate > target_rate && pfdev->regulator) {
+	if (old_clk_rate > target_rate) {
 		err = regulator_set_voltage(pfdev->regulator, target_volt,
 					    target_volt);
 		if (err)
@@ -138,6 +136,9 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	int ret;
 	struct dev_pm_opp *opp;
 
+	if (!pfdev->regulator)
+		return 0;
+
 	ret = dev_pm_opp_of_add_table(&pfdev->pdev->dev);
 	if (ret == -ENODEV) /* Optional, continue without devfreq */
 		return 0;
-- 
2.20.1

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Rob Herring]

On Fri, Sep 6, 2019 at 4:23 PM Steven Price <steven.price@arm.com> wrote:
>
> On 04/09/2019 13:30, Mark Brown wrote:
> > The panfrost driver requests a supply using regulator_get_optional()
> > but both the name of the supply and the usage pattern suggest that it is
> > being used for the main power for the device and is not at all optional
> > for the device for function, there is no meaningful handling for absent
> > supplies.  Such regulators should use the vanilla regulator_get()
> > interface, it will ensure that even if a supply is not described in the
> > system integration one will be provided in software.
> >
> > Signed-off-by: Mark Brown <broonie@kernel.org>
>
> Tested-by: Steven Price <steven.price@arm.com>
>
> Looks like my approach to this was wrong - so we should also revert the
> changes I made previously.
>
> ----8<----
> From fe20f8abcde8444bb41a8f72fb35de943a27ec5c Mon Sep 17 00:00:00 2001
> From: Steven Price <steven.price@arm.com>
> Date: Fri, 6 Sep 2019 15:20:53 +0100
> Subject: [PATCH] drm/panfrost: Revert changes to cope with NULL regulator
>
> Handling a NULL return from devm_regulator_get_optional() doesn't seem
> like the correct way of handling this. Instead revert the changes in
> favour of switching to using devm_regulator_get() which will return a
> dummy regulator instead.
>
> Reverts commit 52282163dfa6 ("drm/panfrost: Add missing check for pfdev->regulator")
> Reverts commit e21dd290881b ("drm/panfrost: Enable devfreq to work without regulator")

Does a straight revert of these 2 patches not work? If it does work,
can you do that and send to the list. I don't want my hand slapped
again reverting things.

Rob

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Steven Price]

On 09/09/2019 16:41, Rob Herring wrote:
> On Fri, Sep 6, 2019 at 4:23 PM Steven Price <steven.price@arm.com> wrote:
>>
>> On 04/09/2019 13:30, Mark Brown wrote:
>>> The panfrost driver requests a supply using regulator_get_optional()
>>> but both the name of the supply and the usage pattern suggest that it is
>>> being used for the main power for the device and is not at all optional
>>> for the device for function, there is no meaningful handling for absent
>>> supplies.  Such regulators should use the vanilla regulator_get()
>>> interface, it will ensure that even if a supply is not described in the
>>> system integration one will be provided in software.
>>>
>>> Signed-off-by: Mark Brown <broonie@kernel.org>
>>
>> Tested-by: Steven Price <steven.price@arm.com>
>>
>> Looks like my approach to this was wrong - so we should also revert the
>> changes I made previously.
>>
>> ----8<----
>> From fe20f8abcde8444bb41a8f72fb35de943a27ec5c Mon Sep 17 00:00:00 2001
>> From: Steven Price <steven.price@arm.com>
>> Date: Fri, 6 Sep 2019 15:20:53 +0100
>> Subject: [PATCH] drm/panfrost: Revert changes to cope with NULL regulator
>>
>> Handling a NULL return from devm_regulator_get_optional() doesn't seem
>> like the correct way of handling this. Instead revert the changes in
>> favour of switching to using devm_regulator_get() which will return a
>> dummy regulator instead.
>>
>> Reverts commit 52282163dfa6 ("drm/panfrost: Add missing check for pfdev->regulator")
>> Reverts commit e21dd290881b ("drm/panfrost: Enable devfreq to work without regulator")
> 
> Does a straight revert of these 2 patches not work? If it does work,
> can you do that and send to the list. I don't want my hand slapped
> again reverting things.

I wasn't sure what was best here - 52282163dfa6 is a bug fix, so
reverting that followed by e21dd290881b would (re-)introduce a
regression for that one commit (i.e. not completely bisectable).
Reverting in the other order would work, but seems a little odd.
Squashing the reverts seemed the neatest option - but it's not my hand
at risk... :)

Perhaps it would be best to simply apply Mark's change followed by
something like the following. That way it's not actually a revert!
It also avoids (re-)adding the now redundant check in
panfrost_devfreq_init().

Steve

---8<----
From fb2956acdf46ca04095ef11363c136dc94a1ab18 Mon Sep 17 00:00:00 2001
From: Steven Price <steven.price@arm.com>
Date: Fri, 6 Sep 2019 15:20:53 +0100
Subject: [PATCH] drm/panfrost: Remove NULL checks for regulator

devm_regulator_get() is now used to populate pfdev->regulator which
ensures that this cannot be NULL (a dummy regulator will be returned if
necessary). So remove the checks in panfrost_devfreq_target().

Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index a1f5fa6a742a..12ff77dacc95 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -39,7 +39,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 	 * If frequency scaling from low to high, adjust voltage first.
 	 * If frequency scaling from high to low, adjust frequency first.
 	 */
-	if (old_clk_rate < target_rate && pfdev->regulator) {
+	if (old_clk_rate < target_rate) {
 		err = regulator_set_voltage(pfdev->regulator, target_volt,
 					    target_volt);
 		if (err) {
@@ -53,14 +53,12 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 	if (err) {
 		dev_err(dev, "Cannot set frequency %lu (%d)\n", target_rate,
 			err);
-		if (pfdev->regulator)
-			regulator_set_voltage(pfdev->regulator,
-					      pfdev->devfreq.cur_volt,
-					      pfdev->devfreq.cur_volt);
+		regulator_set_voltage(pfdev->regulator, pfdev->devfreq.cur_volt,
+				      pfdev->devfreq.cur_volt);
 		return err;
 	}
 
-	if (old_clk_rate > target_rate && pfdev->regulator) {
+	if (old_clk_rate > target_rate) {
 		err = regulator_set_voltage(pfdev->regulator, target_volt,
 					    target_volt);
 		if (err)
-- 
2.20.1

Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse

[Rob Herring]

On Mon, Sep 9, 2019 at 11:22 AM Steven Price <steven.price@arm.com> wrote:
>
> On 09/09/2019 16:41, Rob Herring wrote:
> > On Fri, Sep 6, 2019 at 4:23 PM Steven Price <steven.price@arm.com> wrote:
> >>
> >> On 04/09/2019 13:30, Mark Brown wrote:
> >>> The panfrost driver requests a supply using regulator_get_optional()
> >>> but both the name of the supply and the usage pattern suggest that it is
> >>> being used for the main power for the device and is not at all optional
> >>> for the device for function, there is no meaningful handling for absent
> >>> supplies.  Such regulators should use the vanilla regulator_get()
> >>> interface, it will ensure that even if a supply is not described in the
> >>> system integration one will be provided in software.
> >>>
> >>> Signed-off-by: Mark Brown <broonie@kernel.org>
> >>
> >> Tested-by: Steven Price <steven.price@arm.com>
> >>
> >> Looks like my approach to this was wrong - so we should also revert the
> >> changes I made previously.
> >>
> >> ----8<----
> >> From fe20f8abcde8444bb41a8f72fb35de943a27ec5c Mon Sep 17 00:00:00 2001
> >> From: Steven Price <steven.price@arm.com>
> >> Date: Fri, 6 Sep 2019 15:20:53 +0100
> >> Subject: [PATCH] drm/panfrost: Revert changes to cope with NULL regulator
> >>
> >> Handling a NULL return from devm_regulator_get_optional() doesn't seem
> >> like the correct way of handling this. Instead revert the changes in
> >> favour of switching to using devm_regulator_get() which will return a
> >> dummy regulator instead.
> >>
> >> Reverts commit 52282163dfa6 ("drm/panfrost: Add missing check for pfdev->regulator")
> >> Reverts commit e21dd290881b ("drm/panfrost: Enable devfreq to work without regulator")
> >
> > Does a straight revert of these 2 patches not work? If it does work,
> > can you do that and send to the list. I don't want my hand slapped
> > again reverting things.
>
> I wasn't sure what was best here - 52282163dfa6 is a bug fix, so
> reverting that followed by e21dd290881b would (re-)introduce a
> regression for that one commit (i.e. not completely bisectable).
> Reverting in the other order would work, but seems a little odd.
> Squashing the reverts seemed the neatest option - but it's not my hand
> at risk... :)
>
> Perhaps it would be best to simply apply Mark's change followed by
> something like the following. That way it's not actually a revert!
> It also avoids (re-)adding the now redundant check in
> panfrost_devfreq_init().
>
> Steve
>
> ---8<----
> From fb2956acdf46ca04095ef11363c136dc94a1ab18 Mon Sep 17 00:00:00 2001
> From: Steven Price <steven.price@arm.com>
> Date: Fri, 6 Sep 2019 15:20:53 +0100
> Subject: [PATCH] drm/panfrost: Remove NULL checks for regulator
>
> devm_regulator_get() is now used to populate pfdev->regulator which
> ensures that this cannot be NULL (a dummy regulator will be returned if
> necessary). So remove the checks in panfrost_devfreq_target().
>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

Okay, I've gone this route and applied Mark's patch and this one.

Rob