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

[Mark Brown]

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: signature.asc
Description: PGP signature