The get_optimum_mode() for regulator drivers is passed the input
voltage and output voltage as well as the current. This is because, in
theory, the optimum mode can depend on all three things.
It turns out that for all regulator drivers in mainline only the
current is looked at when implementing get_optimum_mode(). None of the
drivers take the input or output voltage into account. Despite the
fact that none of the drivers take the input or output voltage into
account, though, the regulator framework will error out before calling
into get_optimum_mode() if it doesn't know the input or output
voltage.
The above behavior turned out to be a probelm for some boards when we
landed commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
get_optimum_mode(), not set_load()"). Before that change we'd have no
problems running drms_uA_update() for RPMH regulators even if a
regulator's input or output voltage was unknown. After that change
drms_uA_update() started to fail. This is because typically boards
using RPMH regulators don't model the input supplies of RPMH
regulators. Input supplies for RPMH regulators nearly always come from
the output of other RPMH regulators (or always-on regulators) and RPMH
firmware is initialized with this knowledge and handles enabling (and
adjusting the voltage of) input supplies. While we could model the
parent/child relationship of the regulators in Linux, many boards
don't bother since it adds extra overhead.
Let's change the regulator core to make things work again. Now if we
fail to get the input or output voltage we'll still call into
get_optimum_mode() and we'll just pass error codes in for input_uV
and/or output_uV parameters.
Since no existing regulator drivers even look at input_uV and
output_uV we don't need to add this error handling anywhere right
now. We'll add some comments in the core so that it's obvious that (if
regulator drivers care) it's up to them to add the checks.
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]>
---
drivers/regulator/core.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5b5da14976c2..0bc4b9b0a885 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -979,10 +979,13 @@ static int drms_uA_update(struct regulator_dev *rdev)
} else {
/* get output voltage */
output_uV = regulator_get_voltage_rdev(rdev);
- if (output_uV <= 0) {
- rdev_err(rdev, "invalid output voltage found\n");
- return -EINVAL;
- }
+
+ /*
+ * Don't return an error; if regulator driver cares about
+ * output_uV then it's up to the driver to validate.
+ */
+ if (output_uV <= 0)
+ rdev_dbg(rdev, "invalid output voltage found\n");
/* get input voltage */
input_uV = 0;
@@ -990,10 +993,13 @@ static int drms_uA_update(struct regulator_dev *rdev)
input_uV = regulator_get_voltage(rdev->supply);
if (input_uV <= 0)
input_uV = rdev->constraints->input_uV;
- if (input_uV <= 0) {
- rdev_err(rdev, "invalid input voltage found\n");
- return -EINVAL;
- }
+
+ /*
+ * Don't return an error; if regulator driver cares about
+ * input_uV then it's up to the driver to validate.
+ */
+ if (input_uV <= 0)
+ rdev_dbg(rdev, "invalid input voltage found\n");
/* now get the optimum mode for our new total regulator load */
mode = rdev->desc->ops->get_optimum_mode(rdev, input_uV,
--
2.37.2.609.g9ff673ca1a-goog
Hey Doug,
On Tue, Aug 23, 2022 at 01:16:34PM -0700, Douglas Anderson wrote:
> The get_optimum_mode() for regulator drivers is passed the input
> voltage and output voltage as well as the current. This is because, in
> theory, the optimum mode can depend on all three things.
>
> It turns out that for all regulator drivers in mainline only the
> current is looked at when implementing get_optimum_mode(). None of the
> drivers take the input or output voltage into account. Despite the
> fact that none of the drivers take the input or output voltage into
> account, though, the regulator framework will error out before calling
> into get_optimum_mode() if it doesn't know the input or output
> voltage.
>
> The above behavior turned out to be a probelm for some boards when we
> landed commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> get_optimum_mode(), not set_load()"). Before that change we'd have no
> problems running drms_uA_update() for RPMH regulators even if a
> regulator's input or output voltage was unknown. After that change
> drms_uA_update() started to fail. This is because typically boards
> using RPMH regulators don't model the input supplies of RPMH
> regulators. Input supplies for RPMH regulators nearly always come from
> the output of other RPMH regulators (or always-on regulators) and RPMH
> firmware is initialized with this knowledge and handles enabling (and
> adjusting the voltage of) input supplies. While we could model the
> parent/child relationship of the regulators in Linux, many boards
> don't bother since it adds extra overhead.
>
> Let's change the regulator core to make things work again. Now if we
> fail to get the input or output voltage we'll still call into
> get_optimum_mode() and we'll just pass error codes in for input_uV
> and/or output_uV parameters.
>
> Since no existing regulator drivers even look at input_uV and
> output_uV we don't need to add this error handling anywhere right
> now. We'll add some comments in the core so that it's obvious that (if
> regulator drivers care) it's up to them to add the checks.
>
> 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]>
> ---
>
> drivers/regulator/core.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 5b5da14976c2..0bc4b9b0a885 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -979,10 +979,13 @@ static int drms_uA_update(struct regulator_dev *rdev)
> } else {
> /* get output voltage */
> output_uV = regulator_get_voltage_rdev(rdev);
> - if (output_uV <= 0) {
> - rdev_err(rdev, "invalid output voltage found\n");
> - return -EINVAL;
> - }
> +
> + /*
> + * Don't return an error; if regulator driver cares about
> + * output_uV then it's up to the driver to validate.
> + */
> + if (output_uV <= 0)
> + rdev_dbg(rdev, "invalid output voltage found\n");
>
> /* get input voltage */
> input_uV = 0;
> @@ -990,10 +993,13 @@ static int drms_uA_update(struct regulator_dev *rdev)
> input_uV = regulator_get_voltage(rdev->supply);
> if (input_uV <= 0)
> input_uV = rdev->constraints->input_uV;
> - if (input_uV <= 0) {
> - rdev_err(rdev, "invalid input voltage found\n");
> - return -EINVAL;
> - }
> +
> + /*
> + * Don't return an error; if regulator driver cares about
> + * input_uV then it's up to the driver to validate.
> + */
> + if (input_uV <= 0)
> + rdev_dbg(rdev, "invalid input voltage found\n");
>
> /* now get the optimum mode for our new total regulator load */
> mode = rdev->desc->ops->get_optimum_mode(rdev, input_uV,
> --
> 2.37.2.609.g9ff673ca1a-goog
>
I think this makes sense, but unfortunately it doesn't entirely fix my
issue introduced by efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()"),
I'm seeing this now:
[ 1.240757] vreg_l17c: mode operation not allowed
[ 1.245586] vreg_l17c: failed to get optimum mode @ 800000 uA 0 -> 2504000 uV: -EPERM
[ 1.253631] ufshcd-qcom 1d84000.ufs: ufshcd_enable_vreg: vcc enable failed, err=-1
which if I understand correctly is because my devicetree isn't setting
regulator-allowed-modes. It appears most of the qcom one's don't set
that (and a good number set regulator-allow-set-load, which I think is
necessary for the RPMH regulator to get this far), so I think others
will be in the same boat as me.
Just for clarity, I'm running with this dtb right now:
https://lore.kernel.org/all/[email protected]/
Thanks,
Andrew
Hi,
On Tue, Aug 23, 2022 at 3:14 PM Andrew Halaney <[email protected]> wrote:
>
> I think this makes sense, but unfortunately it doesn't entirely fix my
> issue introduced by efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()"),
> I'm seeing this now:
>
> [ 1.240757] vreg_l17c: mode operation not allowed
> [ 1.245586] vreg_l17c: failed to get optimum mode @ 800000 uA 0 -> 2504000 uV: -EPERM
> [ 1.253631] ufshcd-qcom 1d84000.ufs: ufshcd_enable_vreg: vcc enable failed, err=-1
>
> which if I understand correctly is because my devicetree isn't setting
> regulator-allowed-modes. It appears most of the qcom one's don't set
> that (and a good number set regulator-allow-set-load, which I think is
> necessary for the RPMH regulator to get this far), so I think others
> will be in the same boat as me.
>
> Just for clarity, I'm running with this dtb right now:
>
> https://lore.kernel.org/all/[email protected]/
Wonderful. I guess the old mechanism that RPMH was using totally
bypassed regulator_mode_constrain(). Seems crazy that nobody has
noticed that till now...
I guess maybe we could add the "initial mode" as an implicitly allowed
mode? I guess the other option is to implicitly disable
regulator-allow-set-load if no allowed modes are listed? Both of these
two will still change behavior for your board, of course. Since I
think previously we were setting modes completely ignoring
"regulator-allowed-modes" both of those two options will effectively
disable mode switching.
I guess the other option would be to say that if
"regulator-allow-set-load" is set but not "regulator-allowed-modes"
then we allow all modes? Somehow that doesn't seem like the "safest"
option, though.
...or, of course, we could say that these device trees are broken and
we're not going to care about supporting them, then we could fix the
device trees...
-Doug
On Tue, Aug 23, 2022 at 03:32:44PM -0700, Doug Anderson wrote:
> I guess maybe we could add the "initial mode" as an implicitly allowed
> mode? I guess the other option is to implicitly disable
> regulator-allow-set-load if no allowed modes are listed? Both of these
We should do the former, or equivalently just noop out of set_load() via
set_optimium_mode() if no modes are allowed.
> I guess the other option would be to say that if
> "regulator-allow-set-load" is set but not "regulator-allowed-modes"
> then we allow all modes? Somehow that doesn't seem like the "safest"
> option, though.
Yes, I'd prefer to avoid that.
Hi,
On Tue, Aug 23, 2022 at 3:49 PM Mark Brown <[email protected]> wrote:
>
> On Tue, Aug 23, 2022 at 03:32:44PM -0700, Doug Anderson wrote:
>
> > I guess maybe we could add the "initial mode" as an implicitly allowed
> > mode? I guess the other option is to implicitly disable
> > regulator-allow-set-load if no allowed modes are listed? Both of these
>
> We should do the former, or equivalently just noop out of set_load() via
> set_optimium_mode() if no modes are allowed.
OK, v2 posted where I kept patch #1 exactly the same and added a
second patch to address this case. Hopefully it works for you.
-Doug