From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Brown Subject: Re: [PATCH] drm/panfrost: Fix regulator_get_optional() misuse Date: Thu, 5 Sep 2019 13:40:14 +0100 Message-ID: <20190905124014.GA4053@sirena.co.uk> References: <20190904123032.23263-1-broonie@kernel.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1796057387==" Return-path: Received: from heliosphere.sirena.org.uk (heliosphere.sirena.org.uk [IPv6:2a01:7e01::f03c:91ff:fed4:a3b6]) by gabe.freedesktop.org (Postfix) with ESMTPS id 351D089BB0 for ; Thu, 5 Sep 2019 12:40:19 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Steven Price Cc: David Airlie , Tomeu Vizoso , dri-devel , "linux-kernel@vger.kernel.org" List-Id: dri-devel@lists.freedesktop.org --===============1796057387== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="tThc/1wpZn/ma/RB" Content-Disposition: inline --tThc/1wpZn/ma/RB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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). --tThc/1wpZn/ma/RB Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAl1xAisACgkQJNaLcl1U h9Dktwf/cU2P46lDZKOxus22wJ39FsrKnSMmarYUxJuVwMOvHiUCedJgRxcjMSJG 8qxD91/c5mLJn2adGdZx/3Wrf0RbY/EEjpk/Ru3sbFl/LEow2OwsAWLG9U/RBmJV GfPwPTRgCOcKIJIEnrCkJV/XjaJhVdAQ6akUaOQtE8N5/+UAgi1qnmDkwndsLPcJ SPvfK7oVN4/xtwxmwomTqz92oCIlcfwubjEl07jZB/DZARaN6LjhGFj18aHSiZqM HV/6gNWaxeVMUpQktnuKzW5fGhAPmutN/+67cx0S9pUXIOeHENz8pE/H9RGeLKo0 oiXGbcDp/ied1I8DHJF8j5PiQ0qUdw== =aLW0 -----END PGP SIGNATURE----- --tThc/1wpZn/ma/RB-- --===============1796057387== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs --===============1796057387==--