(+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