Apparently the device trees of some boards have the property
"regulator-allow-set-load" for some of their regulators but then they
don't specify anything for "regulator-allowed-modes". That's not
really legit, but...
...before commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
get_optimum_mode(), not set_load()") they used to get away with it, at
least on boards using RPMH regulators. That's because when a regulator
driver implements set_load() then the core doesn't look at
"regulator-allowed-modes" when trying to automatically adjust things
in response to the regulator's load. The core doesn't know what mode
we'll end up in, so how could it validate it?
Said another way: before commit efb0cb50c427 ("regulator: qcom-rpmh:
Implement get_optimum_mode(), not set_load()") some boards _were_
having the regulator mode adjusted despite listing no allowed
modes. After commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
get_optimum_mode(), not set_load()") these same boards were now
getting an error returned when trying to use their regulators, since
simply enabling a regulator tries to update its load and that was
failing.
We don't really want to go back to the behavior from before commit
efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not
set_load()"). Boards shouldn't have been changing modes if no allowed
modes were listed. However, the behavior after commit efb0cb50c427
("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
isn't the best because now boards can't even turn their regulators on.
Let's choose to detect this case and return "no error" from
drms_uA_update(). The net-result will be _different_ behavior than we
had before commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
get_optimum_mode(), not set_load()"), but this new behavior seems more
correct. If a board truly needed the mode switched then its device
tree should be updated to list the allowed modes.
Reported-by: Andrew Halaney <[email protected]>
Fixes: efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
Signed-off-by: Douglas Anderson <[email protected]>
---
Changes in v2:
- Added ("Don't err if allow-set-load but no allowed-modes").
drivers/regulator/core.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0bc4b9b0a885..1d030831aeae 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -977,6 +977,18 @@ static int drms_uA_update(struct regulator_dev *rdev)
rdev_err(rdev, "failed to set load %d: %pe\n",
current_uA, ERR_PTR(err));
} else {
+ /*
+ * Unfortunately in some cases the constraints->valid_ops has
+ * REGULATOR_CHANGE_DRMS but there are no valid modes listed.
+ * That's not really legit but we won't consider it a fatal
+ * error here. We'll treat it as if REGULATOR_CHANGE_DRMS
+ * wasn't set.
+ */
+ if (!rdev->constraints->valid_modes_mask) {
+ rdev_dbg(rdev, "Can change modes; but no valid mode\n");
+ return 0;
+ }
+
/* get output voltage */
output_uV = regulator_get_voltage_rdev(rdev);
--
2.37.2.672.g94769d06f0-goog
On Wed, Aug 24, 2022 at 02:22:57PM -0700, Douglas Anderson wrote:
> Apparently the device trees of some boards have the property
> "regulator-allow-set-load" for some of their regulators but then they
> don't specify anything for "regulator-allowed-modes". That's not
> really legit, but...
>
> ...before commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> get_optimum_mode(), not set_load()") they used to get away with it, at
> least on boards using RPMH regulators. That's because when a regulator
> driver implements set_load() then the core doesn't look at
> "regulator-allowed-modes" when trying to automatically adjust things
> in response to the regulator's load. The core doesn't know what mode
> we'll end up in, so how could it validate it?
>
> Said another way: before commit efb0cb50c427 ("regulator: qcom-rpmh:
> Implement get_optimum_mode(), not set_load()") some boards _were_
> having the regulator mode adjusted despite listing no allowed
> modes. After commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> get_optimum_mode(), not set_load()") these same boards were now
> getting an error returned when trying to use their regulators, since
> simply enabling a regulator tries to update its load and that was
> failing.
>
> We don't really want to go back to the behavior from before commit
> efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not
> set_load()"). Boards shouldn't have been changing modes if no allowed
> modes were listed. However, the behavior after commit efb0cb50c427
> ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
> isn't the best because now boards can't even turn their regulators on.
>
> Let's choose to detect this case and return "no error" from
> drms_uA_update(). The net-result will be _different_ behavior than we
> had before commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> get_optimum_mode(), not set_load()"), but this new behavior seems more
> correct. If a board truly needed the mode switched then its device
> tree should be updated to list the allowed modes.
>
> Reported-by: Andrew Halaney <[email protected]>
> Fixes: efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
> Signed-off-by: Douglas Anderson <[email protected]>
Tested-by: Andrew Halaney <[email protected]>
As you made clear in the commit message, a good number of boards will
have a change in behavior since efb0cb50c427 ("regulator: qcom-rpmh: Implement
get_optimum_mode(), not set_load()") and associated fixes. I agree that
these devices are not properly described. Is there any sort of heads up we
should give? Just looking at the Qualcomm devicetrees for aarch64, I see all
of these are affected:
apq8016-sbc.dts
apq8096-db820c.dts
apq8096-ifc6640.dts
msm8916-alcatel-idol347.dts
msm8916-asus-z00l.dts
msm8916-huawei-g7.dts
msm8916-longcheer-l8150.dts
msm8916-longcheer-l8910.dts
msm8916-samsung-a2015-common.dtsi
msm8916-samsung-j5.dts
msm8916-samsung-serranove.dts
msm8916-wingtech-wt88047.dts
msm8992-lg-bullhead.dtsi
msm8992-xiaomi-libra.dts
msm8994-msft-lumia-octagon.dtsi
msm8994-sony-xperia-kitakami.dtsi
msm8996-sony-xperia-tone.dtsi
msm8996-xiaomi-common.dtsi
msm8998-clamshell.dtsi
msm8998-fxtec-pro1.dts
msm8998-mtp.dts
msm8998-oneplus-common.dtsi
msm8998-sony-xperia-yoshino.dtsi
sa8155p-adp.dts
sa8xxxp-auto-adp.dtsi
sc8280xp-crd.dts
sc8280xp-lenovo-thinkpad-x13s.dts
sda660-inforce-ifc6560.dts
sdm630-sony-xperia-nile.dtsi
sdm660-xiaomi-lavender.dts
sm8150-sony-xperia-kumano.dtsi
sm8250-sony-xperia-edo.dtsi
sm8350-hdk.dts
Thanks,
Andrew
Hi,
On Thu, Aug 25, 2022 at 8:14 AM Andrew Halaney <[email protected]> wrote:
>
> On Wed, Aug 24, 2022 at 02:22:57PM -0700, Douglas Anderson wrote:
> > Apparently the device trees of some boards have the property
> > "regulator-allow-set-load" for some of their regulators but then they
> > don't specify anything for "regulator-allowed-modes". That's not
> > really legit, but...
> >
> > ...before commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> > get_optimum_mode(), not set_load()") they used to get away with it, at
> > least on boards using RPMH regulators. That's because when a regulator
> > driver implements set_load() then the core doesn't look at
> > "regulator-allowed-modes" when trying to automatically adjust things
> > in response to the regulator's load. The core doesn't know what mode
> > we'll end up in, so how could it validate it?
> >
> > Said another way: before commit efb0cb50c427 ("regulator: qcom-rpmh:
> > Implement get_optimum_mode(), not set_load()") some boards _were_
> > having the regulator mode adjusted despite listing no allowed
> > modes. After commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> > get_optimum_mode(), not set_load()") these same boards were now
> > getting an error returned when trying to use their regulators, since
> > simply enabling a regulator tries to update its load and that was
> > failing.
> >
> > We don't really want to go back to the behavior from before commit
> > efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not
> > set_load()"). Boards shouldn't have been changing modes if no allowed
> > modes were listed. However, the behavior after commit efb0cb50c427
> > ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
> > isn't the best because now boards can't even turn their regulators on.
> >
> > Let's choose to detect this case and return "no error" from
> > drms_uA_update(). The net-result will be _different_ behavior than we
> > had before commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> > get_optimum_mode(), not set_load()"), but this new behavior seems more
> > correct. If a board truly needed the mode switched then its device
> > tree should be updated to list the allowed modes.
> >
> > Reported-by: Andrew Halaney <[email protected]>
> > Fixes: efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
> > Signed-off-by: Douglas Anderson <[email protected]>
>
> Tested-by: Andrew Halaney <[email protected]>
>
> As you made clear in the commit message, a good number of boards will
> have a change in behavior since efb0cb50c427 ("regulator: qcom-rpmh: Implement
> get_optimum_mode(), not set_load()") and associated fixes. I agree that
> these devices are not properly described. Is there any sort of heads up we
> should give? Just looking at the Qualcomm devicetrees for aarch64, I see all
> of these are affected:
>
> apq8016-sbc.dts
> apq8096-db820c.dts
> apq8096-ifc6640.dts
> msm8916-alcatel-idol347.dts
> msm8916-asus-z00l.dts
> msm8916-huawei-g7.dts
> msm8916-longcheer-l8150.dts
> msm8916-longcheer-l8910.dts
> msm8916-samsung-a2015-common.dtsi
> msm8916-samsung-j5.dts
> msm8916-samsung-serranove.dts
> msm8916-wingtech-wt88047.dts
> msm8992-lg-bullhead.dtsi
> msm8992-xiaomi-libra.dts
> msm8994-msft-lumia-octagon.dtsi
> msm8994-sony-xperia-kitakami.dtsi
> msm8996-sony-xperia-tone.dtsi
> msm8996-xiaomi-common.dtsi
> msm8998-clamshell.dtsi
> msm8998-fxtec-pro1.dts
> msm8998-mtp.dts
> msm8998-oneplus-common.dtsi
> msm8998-sony-xperia-yoshino.dtsi
> sa8155p-adp.dts
> sa8xxxp-auto-adp.dtsi
> sc8280xp-crd.dts
> sc8280xp-lenovo-thinkpad-x13s.dts
> sda660-inforce-ifc6560.dts
> sdm630-sony-xperia-nile.dtsi
> sdm660-xiaomi-lavender.dts
> sm8150-sony-xperia-kumano.dtsi
> sm8250-sony-xperia-edo.dtsi
> sm8350-hdk.dts
True, it would be a good idea to send out fixes. OK, so let's see. We
can probably get fairly close to seeing who is affected with these
greps:
rpmh_users=$(git grep -l -i rpmh -- arch/arm*/boot/dts/qcom)
set_modes=$(grep -l regulator-allow-set-load ${rpmh_users})
but_no_allowed_modes=$(grep -l -v regulator-allowed-modes ${set_modes})
That actually gives a (much)shorter list than yours. Why?
Ah. Your list includes not just RPMH users but also RPM users. RPM
users _won't_ be affected. In RPM regulators we don't actually track
the modes in the kernel--we actually pass the load directly to the
remote processor and it handles translating that into loads. RPM
regulators don't even have a way to directly set the mode.
...so we only need to fix a small number (7) boards.
Posting up patches now. OK, the cover letter should show up shortly at:
https://lore.kernel.org/r/[email protected]
-Doug
On Thu, Aug 25, 2022 at 09:43:45AM -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, Aug 25, 2022 at 8:14 AM Andrew Halaney <[email protected]> wrote:
> >
> > On Wed, Aug 24, 2022 at 02:22:57PM -0700, Douglas Anderson wrote:
> > > Apparently the device trees of some boards have the property
> > > "regulator-allow-set-load" for some of their regulators but then they
> > > don't specify anything for "regulator-allowed-modes". That's not
> > > really legit, but...
> > >
> > > ...before commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> > > get_optimum_mode(), not set_load()") they used to get away with it, at
> > > least on boards using RPMH regulators. That's because when a regulator
> > > driver implements set_load() then the core doesn't look at
> > > "regulator-allowed-modes" when trying to automatically adjust things
> > > in response to the regulator's load. The core doesn't know what mode
> > > we'll end up in, so how could it validate it?
> > >
> > > Said another way: before commit efb0cb50c427 ("regulator: qcom-rpmh:
> > > Implement get_optimum_mode(), not set_load()") some boards _were_
> > > having the regulator mode adjusted despite listing no allowed
> > > modes. After commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> > > get_optimum_mode(), not set_load()") these same boards were now
> > > getting an error returned when trying to use their regulators, since
> > > simply enabling a regulator tries to update its load and that was
> > > failing.
> > >
> > > We don't really want to go back to the behavior from before commit
> > > efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not
> > > set_load()"). Boards shouldn't have been changing modes if no allowed
> > > modes were listed. However, the behavior after commit efb0cb50c427
> > > ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
> > > isn't the best because now boards can't even turn their regulators on.
> > >
> > > Let's choose to detect this case and return "no error" from
> > > drms_uA_update(). The net-result will be _different_ behavior than we
> > > had before commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> > > get_optimum_mode(), not set_load()"), but this new behavior seems more
> > > correct. If a board truly needed the mode switched then its device
> > > tree should be updated to list the allowed modes.
> > >
> > > Reported-by: Andrew Halaney <[email protected]>
> > > Fixes: efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
> > > Signed-off-by: Douglas Anderson <[email protected]>
> >
> > Tested-by: Andrew Halaney <[email protected]>
> >
> > As you made clear in the commit message, a good number of boards will
> > have a change in behavior since efb0cb50c427 ("regulator: qcom-rpmh: Implement
> > get_optimum_mode(), not set_load()") and associated fixes. I agree that
> > these devices are not properly described. Is there any sort of heads up we
> > should give? Just looking at the Qualcomm devicetrees for aarch64, I see all
> > of these are affected:
> >
> > apq8016-sbc.dts
> > apq8096-db820c.dts
> > apq8096-ifc6640.dts
> > msm8916-alcatel-idol347.dts
> > msm8916-asus-z00l.dts
> > msm8916-huawei-g7.dts
> > msm8916-longcheer-l8150.dts
> > msm8916-longcheer-l8910.dts
> > msm8916-samsung-a2015-common.dtsi
> > msm8916-samsung-j5.dts
> > msm8916-samsung-serranove.dts
> > msm8916-wingtech-wt88047.dts
> > msm8992-lg-bullhead.dtsi
> > msm8992-xiaomi-libra.dts
> > msm8994-msft-lumia-octagon.dtsi
> > msm8994-sony-xperia-kitakami.dtsi
> > msm8996-sony-xperia-tone.dtsi
> > msm8996-xiaomi-common.dtsi
> > msm8998-clamshell.dtsi
> > msm8998-fxtec-pro1.dts
> > msm8998-mtp.dts
> > msm8998-oneplus-common.dtsi
> > msm8998-sony-xperia-yoshino.dtsi
> > sa8155p-adp.dts
> > sa8xxxp-auto-adp.dtsi
> > sc8280xp-crd.dts
> > sc8280xp-lenovo-thinkpad-x13s.dts
> > sda660-inforce-ifc6560.dts
> > sdm630-sony-xperia-nile.dtsi
> > sdm660-xiaomi-lavender.dts
> > sm8150-sony-xperia-kumano.dtsi
> > sm8250-sony-xperia-edo.dtsi
> > sm8350-hdk.dts
>
> True, it would be a good idea to send out fixes. OK, so let's see. We
> can probably get fairly close to seeing who is affected with these
> greps:
>
> rpmh_users=$(git grep -l -i rpmh -- arch/arm*/boot/dts/qcom)
> set_modes=$(grep -l regulator-allow-set-load ${rpmh_users})
> but_no_allowed_modes=$(grep -l -v regulator-allowed-modes ${set_modes})
>
> That actually gives a (much)shorter list than yours. Why?
>
> Ah. Your list includes not just RPMH users but also RPM users. RPM
> users _won't_ be affected. In RPM regulators we don't actually track
> the modes in the kernel--we actually pass the load directly to the
> remote processor and it handles translating that into loads. RPM
> regulators don't even have a way to directly set the mode.
>
> ...so we only need to fix a small number (7) boards.
>
> Posting up patches now. OK, the cover letter should show up shortly at:
>
> https://lore.kernel.org/r/[email protected]
>
> -Doug
>
Thanks for the grep-fu, I didn't even think about RPM vs RPMH,
I will try and review those today!
- Andrew