LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] regulator: core: Allow for regulators that can't be read at bootup
@ 2018-05-15 22:07 Douglas Anderson
  2018-05-16 22:47 ` David Collins
  2018-05-17 16:41 ` Applied "regulator: core: Allow for regulators that can't be read at bootup" to the regulator tree Mark Brown
  0 siblings, 2 replies; 3+ messages in thread
From: Douglas Anderson @ 2018-05-15 22:07 UTC (permalink / raw)
  To: broonie; +Cc: mka, collinsd, Douglas Anderson, Liam Girdwood, linux-kernel

Regulators attached via RPMh on Qualcomm sdm845 apparently are
write-only.  Specifically you can send a request for a certain voltage
but you can't read back to see what voltage you've requested.  What
this means is that at bootup we have absolutely no idea what voltage
we could be at.

As discussed in the patches to try to support the RPMh regulators [1],
the fact that regulators are write-only means that its driver's
get_voltage_sel() should return an error code if it's called before
any calls to set_voltage_sel().  This causes problems in
machine_constraints_voltage() when trying to apply the constraints.

A proposed fix was to come up with an error code that could be
returned by get_voltage_sel() which would cause the regulator
framework to simply try setting the voltage with the current
constraints.

In this patch I propose the error code -ENOTRECOVERABLE.  In errno.h
this error is described as "State not recoverable".  Though the error
code was originally intended "for robust mutexes", the description of
the error code seems to apply here because we can't read the state of
the regulator.  Also note that the only existing user of this error
code in the regulator framework is tps65090-regulator.c which returns
this error code from the enable() call (not get_voltage() or
get_voltage_sel()), so there should be no existing regulators that
might accidentally get the new behavior.  (Side note is that tps65090
seems to interpret this error code to mean an error that you can't
recover from rather than some data that can't be recovered).

[1] https://patchwork.kernel.org/patch/10340897/

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
If this patch is accepted then we'll need to update the latest RPMh
regulator patch to return it.  The current patch returns -EINVAL which
means that the regulator always fails.

 drivers/regulator/core.c         | 12 ++++++++++++
 include/linux/regulator/driver.h |  7 +++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d4803460a557..fe314ff56772 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -886,6 +886,18 @@ static int machine_constraints_voltage(struct regulator_dev *rdev,
 	    rdev->constraints->min_uV && rdev->constraints->max_uV) {
 		int target_min, target_max;
 		int current_uV = _regulator_get_voltage(rdev);
+
+		if (current_uV == -ENOTRECOVERABLE) {
+			/* This regulator can't be read and must be initted */
+			rdev_info(rdev, "Setting %d-%duV\n",
+				  rdev->constraints->min_uV,
+				  rdev->constraints->max_uV);
+			_regulator_do_set_voltage(rdev,
+						  rdev->constraints->min_uV,
+						  rdev->constraints->max_uV);
+			current_uV = _regulator_get_voltage(rdev);
+		}
+
 		if (current_uV < 0) {
 			rdev_err(rdev,
 				 "failed to get the current voltage(%d)\n",
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 4fc96cb8e5d7..14e512ad6d4f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -81,9 +81,12 @@ struct regulator_linear_range {
  * @set_voltage_sel: Set the voltage for the regulator using the specified
  *                   selector.
  * @map_voltage: Convert a voltage into a selector
- * @get_voltage: Return the currently configured voltage for the regulator.
+ * @get_voltage: Return the currently configured voltage for the regulator;
+ *                   return -ENOTRECOVERABLE if regulator can't be read at
+ *                   bootup and hasn't been set yet.
  * @get_voltage_sel: Return the currently configured voltage selector for the
- *                   regulator.
+ *                   regulator; return -ENOTRECOVERABLE if regulator can't
+ *                   be read at bootup and hasn't been set yet.
  * @list_voltage: Return one of the supported voltages, in microvolts; zero
  *	if the selector indicates a voltage that is unusable on this system;
  *	or negative errno.  Selectors range from zero to one less than
-- 
2.17.0.441.gb46fe60e1d-goog

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] regulator: core: Allow for regulators that can't be read at bootup
  2018-05-15 22:07 [PATCH] regulator: core: Allow for regulators that can't be read at bootup Douglas Anderson
@ 2018-05-16 22:47 ` David Collins
  2018-05-17 16:41 ` Applied "regulator: core: Allow for regulators that can't be read at bootup" to the regulator tree Mark Brown
  1 sibling, 0 replies; 3+ messages in thread
From: David Collins @ 2018-05-16 22:47 UTC (permalink / raw)
  To: Douglas Anderson, broonie; +Cc: mka, Liam Girdwood, linux-kernel, linux-arm-msm

On 05/15/2018 03:07 PM, Douglas Anderson wrote:
> As discussed in the patches to try to support the RPMh regulators [1],
> the fact that regulators are write-only means that its driver's
> get_voltage_sel() should return an error code if it's called before
> any calls to set_voltage_sel().  This causes problems in
> machine_constraints_voltage() when trying to apply the constraints.

This problem specifically arises in the latest RPMh regulator patch [1] if
regulator-min-microvolt and regulator-max-microvolt are specified for a
VRM managed RPMh regulator device but the qcom,regulator-initial-microvolt
property is not provided.  Therefore, listing
qcom,regulator-initial-microvolt is a way to avoid regulator registration
issues.


> In this patch I propose the error code -ENOTRECOVERABLE.  In errno.h
> this error is described as "State not recoverable".

I'm fine with this constant.  Hopefully Mark is as well.


> If this patch is accepted then we'll need to update the latest RPMh
> regulator patch to return it.  The current patch returns -EINVAL which
> means that the regulator always fails.

I'll make this modification if needed.

Take care,
David

[1] https://lkml.org/lkml/2018/5/11/703

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Applied "regulator: core: Allow for regulators that can't be read at bootup" to the regulator tree
  2018-05-15 22:07 [PATCH] regulator: core: Allow for regulators that can't be read at bootup Douglas Anderson
  2018-05-16 22:47 ` David Collins
@ 2018-05-17 16:41 ` Mark Brown
  1 sibling, 0 replies; 3+ messages in thread
From: Mark Brown @ 2018-05-17 16:41 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Mark Brown, broonie, mka, collinsd, Liam Girdwood, linux-kernel,
	linux-kernel

The patch

   regulator: core: Allow for regulators that can't be read at bootup

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 84b3a7c9c6befe5ab4d49070fe7bcab2da22637e Mon Sep 17 00:00:00 2001
From: Douglas Anderson <dianders@chromium.org>
Date: Tue, 15 May 2018 15:07:17 -0700
Subject: [PATCH] regulator: core: Allow for regulators that can't be read at
 bootup

Regulators attached via RPMh on Qualcomm sdm845 apparently are
write-only.  Specifically you can send a request for a certain voltage
but you can't read back to see what voltage you've requested.  What
this means is that at bootup we have absolutely no idea what voltage
we could be at.

As discussed in the patches to try to support the RPMh regulators [1],
the fact that regulators are write-only means that its driver's
get_voltage_sel() should return an error code if it's called before
any calls to set_voltage_sel().  This causes problems in
machine_constraints_voltage() when trying to apply the constraints.

A proposed fix was to come up with an error code that could be
returned by get_voltage_sel() which would cause the regulator
framework to simply try setting the voltage with the current
constraints.

In this patch I propose the error code -ENOTRECOVERABLE.  In errno.h
this error is described as "State not recoverable".  Though the error
code was originally intended "for robust mutexes", the description of
the error code seems to apply here because we can't read the state of
the regulator.  Also note that the only existing user of this error
code in the regulator framework is tps65090-regulator.c which returns
this error code from the enable() call (not get_voltage() or
get_voltage_sel()), so there should be no existing regulators that
might accidentally get the new behavior.  (Side note is that tps65090
seems to interpret this error code to mean an error that you can't
recover from rather than some data that can't be recovered).

[1] https://patchwork.kernel.org/patch/10340897/

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c         | 12 ++++++++++++
 include/linux/regulator/driver.h |  7 +++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d4803460a557..fe314ff56772 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -886,6 +886,18 @@ static int machine_constraints_voltage(struct regulator_dev *rdev,
 	    rdev->constraints->min_uV && rdev->constraints->max_uV) {
 		int target_min, target_max;
 		int current_uV = _regulator_get_voltage(rdev);
+
+		if (current_uV == -ENOTRECOVERABLE) {
+			/* This regulator can't be read and must be initted */
+			rdev_info(rdev, "Setting %d-%duV\n",
+				  rdev->constraints->min_uV,
+				  rdev->constraints->max_uV);
+			_regulator_do_set_voltage(rdev,
+						  rdev->constraints->min_uV,
+						  rdev->constraints->max_uV);
+			current_uV = _regulator_get_voltage(rdev);
+		}
+
 		if (current_uV < 0) {
 			rdev_err(rdev,
 				 "failed to get the current voltage(%d)\n",
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 4fc96cb8e5d7..14e512ad6d4f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -81,9 +81,12 @@ struct regulator_linear_range {
  * @set_voltage_sel: Set the voltage for the regulator using the specified
  *                   selector.
  * @map_voltage: Convert a voltage into a selector
- * @get_voltage: Return the currently configured voltage for the regulator.
+ * @get_voltage: Return the currently configured voltage for the regulator;
+ *                   return -ENOTRECOVERABLE if regulator can't be read at
+ *                   bootup and hasn't been set yet.
  * @get_voltage_sel: Return the currently configured voltage selector for the
- *                   regulator.
+ *                   regulator; return -ENOTRECOVERABLE if regulator can't
+ *                   be read at bootup and hasn't been set yet.
  * @list_voltage: Return one of the supported voltages, in microvolts; zero
  *	if the selector indicates a voltage that is unusable on this system;
  *	or negative errno.  Selectors range from zero to one less than
-- 
2.17.0

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-05-17 16:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-15 22:07 [PATCH] regulator: core: Allow for regulators that can't be read at bootup Douglas Anderson
2018-05-16 22:47 ` David Collins
2018-05-17 16:41 ` Applied "regulator: core: Allow for regulators that can't be read at bootup" to the regulator tree Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).