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