[lkml]   [2019]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse
(+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.



 \ /
  Last update: 2019-09-06 12:01    [W:0.063 / U:1.816 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site