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
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
