| Message ID | 20190904123032.23263-1-broonie@kernel.org |
|---|---|
| State | Accepted |
| Commit | edfa07504c5bb188b262fb2f4fc487de966f89a2 |
| Headers | show |
| Series | drm/panfrost: Fix regulator_get_optional() misuse | expand |
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. 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). 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. 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).
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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
(+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
On Fri, Sep 6, 2019 at 4:23 PM Steven Price <steven.price@arm.com> wrote: > > On 04/09/2019 13:30, Mark Brown wrote: > > The panfrost driver requests a supply using regulator_get_optional() > > but both the name of the supply and the usage pattern suggest that it is > > being used for the main power for the device and is not at all optional > > for the device for function, there is no meaningful handling for absent > > supplies. Such regulators should use the vanilla regulator_get() > > interface, it will ensure that even if a supply is not described in the > > system integration one will be provided in software. > > > > Signed-off-by: Mark Brown <broonie@kernel.org> > > Tested-by: Steven Price <steven.price@arm.com> > > Looks like my approach to this was wrong - so we should also revert the > changes I made previously. > > ----8<---- > From fe20f8abcde8444bb41a8f72fb35de943a27ec5c Mon Sep 17 00:00:00 2001 > From: Steven Price <steven.price@arm.com> > Date: Fri, 6 Sep 2019 15:20:53 +0100 > Subject: [PATCH] drm/panfrost: Revert changes to cope with NULL regulator > > Handling a NULL return from devm_regulator_get_optional() doesn't seem > like the correct way of handling this. Instead revert the changes in > favour of switching to using devm_regulator_get() which will return a > dummy regulator instead. > > Reverts commit 52282163dfa6 ("drm/panfrost: Add missing check for pfdev->regulator") > Reverts commit e21dd290881b ("drm/panfrost: Enable devfreq to work without regulator") Does a straight revert of these 2 patches not work? If it does work, can you do that and send to the list. I don't want my hand slapped again reverting things. Rob
On 09/09/2019 16:41, Rob Herring wrote: > On Fri, Sep 6, 2019 at 4:23 PM Steven Price <steven.price@arm.com> wrote: >> >> On 04/09/2019 13:30, Mark Brown wrote: >>> The panfrost driver requests a supply using regulator_get_optional() >>> but both the name of the supply and the usage pattern suggest that it is >>> being used for the main power for the device and is not at all optional >>> for the device for function, there is no meaningful handling for absent >>> supplies. Such regulators should use the vanilla regulator_get() >>> interface, it will ensure that even if a supply is not described in the >>> system integration one will be provided in software. >>> >>> Signed-off-by: Mark Brown <broonie@kernel.org> >> >> Tested-by: Steven Price <steven.price@arm.com> >> >> Looks like my approach to this was wrong - so we should also revert the >> changes I made previously. >> >> ----8<---- >> From fe20f8abcde8444bb41a8f72fb35de943a27ec5c Mon Sep 17 00:00:00 2001 >> From: Steven Price <steven.price@arm.com> >> Date: Fri, 6 Sep 2019 15:20:53 +0100 >> Subject: [PATCH] drm/panfrost: Revert changes to cope with NULL regulator >> >> Handling a NULL return from devm_regulator_get_optional() doesn't seem >> like the correct way of handling this. Instead revert the changes in >> favour of switching to using devm_regulator_get() which will return a >> dummy regulator instead. >> >> Reverts commit 52282163dfa6 ("drm/panfrost: Add missing check for pfdev->regulator") >> Reverts commit e21dd290881b ("drm/panfrost: Enable devfreq to work without regulator") > > Does a straight revert of these 2 patches not work? If it does work, > can you do that and send to the list. I don't want my hand slapped > again reverting things. I wasn't sure what was best here - 52282163dfa6 is a bug fix, so reverting that followed by e21dd290881b would (re-)introduce a regression for that one commit (i.e. not completely bisectable). Reverting in the other order would work, but seems a little odd. Squashing the reverts seemed the neatest option - but it's not my hand at risk... :) Perhaps it would be best to simply apply Mark's change followed by something like the following. That way it's not actually a revert! It also avoids (re-)adding the now redundant check in panfrost_devfreq_init(). Steve ---8<---- From fb2956acdf46ca04095ef11363c136dc94a1ab18 Mon Sep 17 00:00:00 2001 From: Steven Price <steven.price@arm.com> Date: Fri, 6 Sep 2019 15:20:53 +0100 Subject: [PATCH] drm/panfrost: Remove NULL checks for regulator devm_regulator_get() is now used to populate pfdev->regulator which ensures that this cannot be NULL (a dummy regulator will be returned if necessary). So remove the checks in panfrost_devfreq_target(). Signed-off-by: Steven Price <steven.price@arm.com> --- drivers/gpu/drm/panfrost/panfrost_devfreq.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index a1f5fa6a742a..12ff77dacc95 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -39,7 +39,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq, * If frequency scaling from low to high, adjust voltage first. * If frequency scaling from high to low, adjust frequency first. */ - if (old_clk_rate < target_rate && pfdev->regulator) { + if (old_clk_rate < target_rate) { err = regulator_set_voltage(pfdev->regulator, target_volt, target_volt); if (err) { @@ -53,14 +53,12 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq, if (err) { dev_err(dev, "Cannot set frequency %lu (%d)\n", target_rate, err); - if (pfdev->regulator) - regulator_set_voltage(pfdev->regulator, - pfdev->devfreq.cur_volt, - pfdev->devfreq.cur_volt); + regulator_set_voltage(pfdev->regulator, pfdev->devfreq.cur_volt, + pfdev->devfreq.cur_volt); return err; } - if (old_clk_rate > target_rate && pfdev->regulator) { + if (old_clk_rate > target_rate) { err = regulator_set_voltage(pfdev->regulator, target_volt, target_volt); if (err)
On Mon, Sep 9, 2019 at 11:22 AM Steven Price <steven.price@arm.com> wrote: > > On 09/09/2019 16:41, Rob Herring wrote: > > On Fri, Sep 6, 2019 at 4:23 PM Steven Price <steven.price@arm.com> wrote: > >> > >> On 04/09/2019 13:30, Mark Brown wrote: > >>> The panfrost driver requests a supply using regulator_get_optional() > >>> but both the name of the supply and the usage pattern suggest that it is > >>> being used for the main power for the device and is not at all optional > >>> for the device for function, there is no meaningful handling for absent > >>> supplies. Such regulators should use the vanilla regulator_get() > >>> interface, it will ensure that even if a supply is not described in the > >>> system integration one will be provided in software. > >>> > >>> Signed-off-by: Mark Brown <broonie@kernel.org> > >> > >> Tested-by: Steven Price <steven.price@arm.com> > >> > >> Looks like my approach to this was wrong - so we should also revert the > >> changes I made previously. > >> > >> ----8<---- > >> From fe20f8abcde8444bb41a8f72fb35de943a27ec5c Mon Sep 17 00:00:00 2001 > >> From: Steven Price <steven.price@arm.com> > >> Date: Fri, 6 Sep 2019 15:20:53 +0100 > >> Subject: [PATCH] drm/panfrost: Revert changes to cope with NULL regulator > >> > >> Handling a NULL return from devm_regulator_get_optional() doesn't seem > >> like the correct way of handling this. Instead revert the changes in > >> favour of switching to using devm_regulator_get() which will return a > >> dummy regulator instead. > >> > >> Reverts commit 52282163dfa6 ("drm/panfrost: Add missing check for pfdev->regulator") > >> Reverts commit e21dd290881b ("drm/panfrost: Enable devfreq to work without regulator") > > > > Does a straight revert of these 2 patches not work? If it does work, > > can you do that and send to the list. I don't want my hand slapped > > again reverting things. > > I wasn't sure what was best here - 52282163dfa6 is a bug fix, so > reverting that followed by e21dd290881b would (re-)introduce a > regression for that one commit (i.e. not completely bisectable). > Reverting in the other order would work, but seems a little odd. > Squashing the reverts seemed the neatest option - but it's not my hand > at risk... :) > > Perhaps it would be best to simply apply Mark's change followed by > something like the following. That way it's not actually a revert! > It also avoids (re-)adding the now redundant check in > panfrost_devfreq_init(). > > Steve > > ---8<---- > From fb2956acdf46ca04095ef11363c136dc94a1ab18 Mon Sep 17 00:00:00 2001 > From: Steven Price <steven.price@arm.com> > Date: Fri, 6 Sep 2019 15:20:53 +0100 > Subject: [PATCH] drm/panfrost: Remove NULL checks for regulator > > devm_regulator_get() is now used to populate pfdev->regulator which > ensures that this cannot be NULL (a dummy regulator will be returned if > necessary). So remove the checks in panfrost_devfreq_target(). > > Signed-off-by: Steven Price <steven.price@arm.com> > --- > drivers/gpu/drm/panfrost/panfrost_devfreq.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) Okay, I've gone this route and applied Mark's patch and this one. Rob
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c index 46b0b02e4289..238fb6d54df4 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.c +++ b/drivers/gpu/drm/panfrost/panfrost_device.c @@ -89,12 +89,9 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev) { int ret; - pfdev->regulator = devm_regulator_get_optional(pfdev->dev, "mali"); + pfdev->regulator = devm_regulator_get(pfdev->dev, "mali"); if (IS_ERR(pfdev->regulator)) { ret = PTR_ERR(pfdev->regulator); - pfdev->regulator = NULL; - if (ret == -ENODEV) - return 0; dev_err(pfdev->dev, "failed to get regulator: %d\n", ret); return ret; } @@ -110,8 +107,7 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev) static void panfrost_regulator_fini(struct panfrost_device *pfdev) { - if (pfdev->regulator) - regulator_disable(pfdev->regulator); + regulator_disable(pfdev->regulator); } int panfrost_device_init(struct panfrost_device *pfdev)
The panfrost driver requests a supply using regulator_get_optional() but both the name of the supply and the usage pattern suggest that it is being used for the main power for the device and is not at all optional for the device for function, there is no meaningful handling for absent supplies. Such regulators should use the vanilla regulator_get() interface, it will ensure that even if a supply is not described in the system integration one will be provided in software. Signed-off-by: Mark Brown <broonie@kernel.org> --- drivers/gpu/drm/panfrost/panfrost_device.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)