On 05/09/2019 15:02, Steven Price wrote: > On 05/09/2019 13:40, Mark Brown wrote: >> 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. > > 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. > >> 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. > >> 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. > > Agreed. > >> 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). In case we need a fixed voltage, simply add a new fixed-regulator in the board/soc DT, use this regulator for panfrost and use the same fixed voltage in the OPP table. It relies on a correct DT, but isn't is the goal of mainline linux ? Neil > > 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. > > 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. > > Steve > > [1] e21dd290881b drm/panfrost: Enable devfreq to work without regulator > (plus bug fix in 52282163dfa6) > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
