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

[Mark Brown <broonie@kernel.org>]

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