linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
@ 2014-12-10 20:58 Doug Anderson
  2014-12-10 20:58 ` [PATCH 2/4] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Doug Anderson @ 2014-12-10 20:58 UTC (permalink / raw)
  To: Ulf Hansson, Mark Brown, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon
  Cc: Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	linux-rockchip, linux-arm-kernel, Doug Anderson, lgirdwood,
	linux-kernel

There are some cases where you'd like to set a voltage for a regulator
and a range of voltages are OK, but you'd really like to get as close
as you can to a specific voltage if at all possible.

The VQMMC (IO voltage) regulator for SD cards is the inspiration for
needing this function.  A few things about SD cards to justify its
need:

1. In old systems (before UHS), the VMMC and VQMMC were provided by
the same regulator.  That means that SD cards were all tested with
VMMC and VQMMC being exactly the same.  With UHS we need to have two
regulators providing VMMC and VQMMC.  For maximum compatibility, we'd
like to keep those two regulators providing the exact same voltage
when we're using "3.3V signaling".  Note: VMMC is managed with very
specific rules in the MMC core and we tend to pick the _highest_
supported voltage in range.

2. On UHS systems we'll eventually negotiate the IO voltage (VQMMC)
down to 1.8V.  Note that we don't provide a 1.8V reference voltage to
the SD card so it comes up with 1.8V on its own based on VMMC (perhaps
using an on-card LDO).  While the SD card spec says that VQMMC can be
1.7V to 1.95V, it seems best to try to get the closest voltage.
Trying to achieve 1.8V means that the card and controller drive the IO
lines (which are push-pull) with as close to the same voltage as
possible.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/regulator/core.c           | 62 ++++++++++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h |  9 ++++++
 2 files changed, 71 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e225711..ce91b20 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2664,6 +2664,68 @@ out2:
 EXPORT_SYMBOL_GPL(regulator_set_voltage);
 
 /**
+ * regulator_set_closest_voltage - set voltage trying to reach a certain one.
+ * @regulator: regulator source
+ * @min_uV: Minimum required voltage in uV
+ * @ideal_uV: Try to get as close to this voltage as possible
+ * @max_uV: Maximum acceptable voltage in uV
+ *
+ * This function is useful in cases where we want to try to get as close to
+ * a target voltage as possible but we would accept other voltages as fallback.
+ */
+int regulator_set_closest_voltage(struct regulator *regulator, int min_uV,
+				  int ideal_uV, int max_uV)
+{
+	int best_uV, delta, best_delta;
+	int i, voltages, ret;
+	struct regulator_dev *rdev = regulator->rdev;
+
+	if (ideal_uV < min_uV || ideal_uV > max_uV)
+		return -EINVAL;
+
+	/* Handle continuous ranges */
+	if (rdev->desc->continuous_voltage_range) {
+		min_uV = max(min_uV, rdev->constraints->min_uV);
+		max_uV = min(max_uV, rdev->constraints->max_uV);
+
+		if (min_uV > max_uV)
+			return -EINVAL;
+
+		best_uV = min(max(ideal_uV, min_uV), max_uV);
+		return regulator_set_voltage(regulator, best_uV, best_uV);
+	}
+
+	ret = regulator_count_voltages(regulator);
+	if (ret < 0)
+		return ret;
+	voltages = ret;
+
+	best_uV = 0;
+	best_delta = INT_MAX;
+	for (i = 0; i < voltages; i++) {
+		ret = regulator_list_voltage(regulator, i);
+
+		if (ret < min_uV || ret > max_uV)
+			continue;
+
+		delta = ideal_uV - ret;
+		delta = abs(delta);
+
+		if (delta < best_delta) {
+			best_delta = delta;
+			best_uV = ret;
+		}
+	}
+
+	/* If no voltages, let regulator_set_voltage() decide if we're OK */
+	if (best_uV == 0)
+		return regulator_set_voltage(regulator, min_uV, max_uV);
+
+	return regulator_set_voltage(regulator, best_uV, best_uV);
+}
+EXPORT_SYMBOL_GPL(regulator_set_closest_voltage);
+
+/**
  * regulator_set_voltage_time - get raise/fall time
  * @regulator: regulator source
  * @old_uV: starting voltage in microvolts
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index d17e1ff..71c9ec2 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -228,6 +228,8 @@ int regulator_is_supported_voltage(struct regulator *regulator,
 				   int min_uV, int max_uV);
 unsigned int regulator_get_linear_step(struct regulator *regulator);
 int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV);
+int regulator_set_closest_voltage(struct regulator *regulator, int min_uV,
+				  int ideal_uV, int max_uV);
 int regulator_set_voltage_time(struct regulator *regulator,
 			       int old_uV, int new_uV);
 int regulator_get_voltage(struct regulator *regulator);
@@ -440,6 +442,13 @@ static inline int regulator_set_voltage(struct regulator *regulator,
 	return 0;
 }
 
+static inline int regulator_set_closest_voltage(struct regulator *regulator,
+						int min_uV, int ideal_uV,
+						int max_uV)
+{
+	return 0;
+}
+
 static inline int regulator_set_voltage_time(struct regulator *regulator,
 					     int old_uV, int new_uV)
 {
-- 
2.2.0.rc0.207.ga3a616c


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

* [PATCH 2/4] mmc: core: Add mmc_regulator_set_vqmmc()
  2014-12-10 20:58 [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage Doug Anderson
@ 2014-12-10 20:58 ` Doug Anderson
  2014-12-10 20:58 ` [PATCH 3/4] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch Doug Anderson
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 15+ messages in thread
From: Doug Anderson @ 2014-12-10 20:58 UTC (permalink / raw)
  To: Ulf Hansson, Mark Brown, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon
  Cc: Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	linux-rockchip, linux-arm-kernel, Doug Anderson, chris,
	johan.rudholm, tim.kryger, adrian.hunter, andrew_gabbasov,
	linux-mmc, linux-kernel

This adds logic to the MMC core to set VQMMC.  This is expected to be
called by MMC drivers like dw_mmc as part of (or instead of) their
start_signal_voltage_switch() callback.

A few notes:

* When setting the signal voltage to 3.3V we do our best to make VQMMC
  and VMMC match.  It's been reported that this makes some old cards
  happy since they were tested back in the day before UHS when VQMMC
  and VMMC were provided by the same regulator.  A nice side effect of
  this is that we don't end up on the hairy edge of VQMMC (2.7V),
  which some EEs claim is a little too close to the minimum for
  comfort.

* When setting the signal voltage to 1.8V or 1.2V we aim for that
  specific voltage instead of picking the lowest one in the range.

* We very purposely don't print errors in mmc_regulator_set_vqmmc().
  There are cases where the MMC core will try several different
  voltages and we don't want to pollute the logs.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/mmc/core/core.c  | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h |  7 +++++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 9584bff..d0da480 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1333,6 +1333,59 @@ int mmc_regulator_set_ocr(struct mmc_host *mmc,
 }
 EXPORT_SYMBOL_GPL(mmc_regulator_set_ocr);
 
+static int mmc_regulator_set_voltage_if_supported(struct regulator *regulator,
+						  int min_uV, int ideal_uV,
+						  int max_uV)
+{
+	/*
+	 * Check if supported first to avoid errors since we may try several
+	 * signal levels during power up and don't want to show errors.
+	 */
+	if (!regulator_is_supported_voltage(regulator, min_uV, max_uV))
+		return -EINVAL;
+
+	return regulator_set_closest_voltage(regulator, min_uV, ideal_uV,
+					     max_uV);
+}
+
+/**
+ * mmc_regulator_set_vqmmc - Set VQMMC as per the ios
+ *
+ * For 3.3V signaling, we try to match vqmmc to vmmc as closely as possible.
+ * That will match the behavior of old boards where vqmmc and vmmc were supplied
+ * by the same supply.
+ *
+ * For 1.2V and 1.8V signaling we'll try to get as close as possible to the
+ * requested voltage.  This is definitely a good idea for UHS where there's a
+ * separate regulator on the card that's trying to make 1.8V and it's best if
+ * we match.
+ *
+ * This function is expected to be used by a controller's
+ * start_signal_voltage_switch() function.
+ */
+int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	/* If no vqmmc supply then we can't change the voltage */
+	if (IS_ERR(mmc->supply.vqmmc))
+		return -EINVAL;
+
+	switch (ios->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_120:
+		return mmc_regulator_set_voltage_if_supported(
+			mmc->supply.vqmmc, 1100000, 1200000, 1300000);
+	case MMC_SIGNAL_VOLTAGE_180:
+		return mmc_regulator_set_voltage_if_supported(
+			mmc->supply.vqmmc, 1700000, 1800000, 1950000);
+	case MMC_SIGNAL_VOLTAGE_330:
+		return mmc_regulator_set_voltage_if_supported(
+			mmc->supply.vqmmc, 2700000,
+			regulator_get_voltage(mmc->supply.vmmc), 3600000);
+	default:
+		return -EINVAL;
+	}
+}
+EXPORT_SYMBOL_GPL(mmc_regulator_set_vqmmc);
+
 #endif /* CONFIG_REGULATOR */
 
 int mmc_regulator_get_supply(struct mmc_host *mmc)
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 9f32270..524d6fc 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -416,6 +416,7 @@ int mmc_regulator_get_ocrmask(struct regulator *supply);
 int mmc_regulator_set_ocr(struct mmc_host *mmc,
 			struct regulator *supply,
 			unsigned short vdd_bit);
+int mmc_regulator_set_vqmmc(struct mmc_host *mmc, struct mmc_ios *ios);
 #else
 static inline int mmc_regulator_get_ocrmask(struct regulator *supply)
 {
@@ -428,6 +429,12 @@ static inline int mmc_regulator_set_ocr(struct mmc_host *mmc,
 {
 	return 0;
 }
+
+static inline int mmc_regulator_set_vqmmc(struct mmc_host *mmc,
+					  struct mmc_ios *ios)
+{
+	return -EINVAL;
+}
 #endif
 
 int mmc_regulator_get_supply(struct mmc_host *mmc);
-- 
2.2.0.rc0.207.ga3a616c


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

* [PATCH 3/4] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch
  2014-12-10 20:58 [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage Doug Anderson
  2014-12-10 20:58 ` [PATCH 2/4] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
@ 2014-12-10 20:58 ` Doug Anderson
  2014-12-10 20:58 ` [PATCH 4/4] ARK: dts: Specify VMMC and VQMMC on rk3288-evb Doug Anderson
  2014-12-10 23:53 ` [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage Mark Brown
  3 siblings, 0 replies; 15+ messages in thread
From: Doug Anderson @ 2014-12-10 20:58 UTC (permalink / raw)
  To: Ulf Hansson, Mark Brown, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon
  Cc: Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	linux-rockchip, linux-arm-kernel, Doug Anderson, chris,
	linux-mmc, linux-kernel

We've introduced a new helper in the MMC core:
mmc_regulator_set_vqmmc().  Let's use this in dw_mmc.  Using this new
helper has some advantages:

1. We get the mmc_regulator_set_vqmmc() behavior of trying to match
   VQMMC and VMMC when the signal voltage is 3.3V.  This ensures max
   compatibility.

2. We get rid of a few more warnings when probing unsupported
   voltages.

3. We get rid of some non-dw_mmc specific code in dw_mmc.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/mmc/host/dw_mmc.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 67c0451..4ae800c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1160,8 +1160,6 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
 	struct dw_mci *host = slot->host;
 	u32 uhs;
 	u32 v18 = SDMMC_UHS_18V << slot->id;
-	int min_uv, max_uv;
-	int ret;
 
 	/*
 	 * Program the voltage.  Note that some instances of dw_mmc may use
@@ -1170,24 +1168,11 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios)
 	 */
 	uhs = mci_readl(host, UHS_REG);
 	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
-		min_uv = 2700000;
-		max_uv = 3600000;
 		uhs &= ~v18;
 	} else {
-		min_uv = 1700000;
-		max_uv = 1950000;
 		uhs |= v18;
 	}
-	if (!IS_ERR(mmc->supply.vqmmc)) {
-		ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv);
-
-		if (ret) {
-			dev_dbg(&mmc->class_dev,
-					 "Regulator set error %d: %d - %d\n",
-					 ret, min_uv, max_uv);
-			return ret;
-		}
-	}
+	mmc_regulator_set_vqmmc(mmc, ios);
 	mci_writel(host, UHS_REG, uhs);
 
 	return 0;
-- 
2.2.0.rc0.207.ga3a616c


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

* [PATCH 4/4] ARK: dts: Specify VMMC and VQMMC on rk3288-evb
  2014-12-10 20:58 [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage Doug Anderson
  2014-12-10 20:58 ` [PATCH 2/4] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
  2014-12-10 20:58 ` [PATCH 3/4] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch Doug Anderson
@ 2014-12-10 20:58 ` Doug Anderson
  2014-12-10 23:53 ` [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage Mark Brown
  3 siblings, 0 replies; 15+ messages in thread
From: Doug Anderson @ 2014-12-10 20:58 UTC (permalink / raw)
  To: Ulf Hansson, Mark Brown, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon
  Cc: Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	linux-rockchip, linux-arm-kernel, Doug Anderson, robh+dt,
	pawel.moll, mark.rutland, ijc+devicetree, galak, linux,
	devicetree, linux-kernel

Specifying these rails should eventually let us do UHS.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 arch/arm/boot/dts/rk3288-evb.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi b/arch/arm/boot/dts/rk3288-evb.dtsi
index 6194d67..1956a23 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -114,6 +114,8 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
 	status = "okay";
+	vmmc-supply = <&vcc_io>;
+	vqmmc-supply = <&vccio_sd>;
 };
 
 &i2c0 {
-- 
2.2.0.rc0.207.ga3a616c


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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-10 20:58 [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage Doug Anderson
                   ` (2 preceding siblings ...)
  2014-12-10 20:58 ` [PATCH 4/4] ARK: dts: Specify VMMC and VQMMC on rk3288-evb Doug Anderson
@ 2014-12-10 23:53 ` Mark Brown
  2014-12-11  1:08   ` Alexandru Stan
  3 siblings, 1 reply; 15+ messages in thread
From: Mark Brown @ 2014-12-10 23:53 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Ulf Hansson, Heiko Stuebner, Jaehoon Chung, Seungwon Jeon,
	Alexandru Stan, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	linux-rockchip, linux-arm-kernel, lgirdwood, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 407 bytes --]

On Wed, Dec 10, 2014 at 12:58:02PM -0800, Doug Anderson wrote:
> There are some cases where you'd like to set a voltage for a regulator
> and a range of voltages are OK, but you'd really like to get as close
> as you can to a specific voltage if at all possible.

This looks like regulator_set_voltage_tol(), why not use that?  The spec
probably even has a number for the tolerance, or you can make one up.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-10 23:53 ` [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage Mark Brown
@ 2014-12-11  1:08   ` Alexandru Stan
  2014-12-11 12:31     ` Mark Brown
  0 siblings, 1 reply; 15+ messages in thread
From: Alexandru Stan @ 2014-12-11  1:08 UTC (permalink / raw)
  To: Mark Brown
  Cc: Doug Anderson, Ulf Hansson, Heiko Stuebner, Jaehoon Chung,
	Seungwon Jeon, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, lgirdwood, linux-kernel

The spec is from 1.7V to 1.95V, with 1.8V being ideal. It's not that
symmetric. But let's say that 0.1V is fine for tolerance(so 1.7-1.9V)

regulator_set_voltage_tol looks interesting, but i still think it's
not the appropriate thing to use in this case.

Imagine a board has a 1V-1.79 V regulator, we tell it to
regulator_set_voltage_tol(1800000,100000). It will try the 1.8V-1.9V
range, when it sees that it can't it'll fallback to 1.7V - 1.9V, and
it will just be lazy and pick the lowest of the range again:
1.7V(causing voltage drop issues because we're exactly at the minimum
of the spec). The correct voltage would be 1.79V

It's unfortunate that regulator_set_voltage was designed to always
pick the lowest voltage in that range. I understand that it's a power
efficiency thing, but it's not ideal in cases like this.

Alexandru Stan (amstan)


On Wed, Dec 10, 2014 at 3:53 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Dec 10, 2014 at 12:58:02PM -0800, Doug Anderson wrote:
>> There are some cases where you'd like to set a voltage for a regulator
>> and a range of voltages are OK, but you'd really like to get as close
>> as you can to a specific voltage if at all possible.
>
> This looks like regulator_set_voltage_tol(), why not use that?  The spec
> probably even has a number for the tolerance, or you can make one up.

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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-11  1:08   ` Alexandru Stan
@ 2014-12-11 12:31     ` Mark Brown
  2014-12-11 16:09       ` Doug Anderson
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Brown @ 2014-12-11 12:31 UTC (permalink / raw)
  To: Alexandru Stan
  Cc: Doug Anderson, Ulf Hansson, Heiko Stuebner, Jaehoon Chung,
	Seungwon Jeon, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, lgirdwood, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

On Wed, Dec 10, 2014 at 05:08:37PM -0800, Alexandru Stan wrote:
> The spec is from 1.7V to 1.95V, with 1.8V being ideal. It's not that
> symmetric. But let's say that 0.1V is fine for tolerance(so 1.7-1.9V)

Don't top post so people reading your message have some context to give
them some idea what you are talking about.

> Imagine a board has a 1V-1.79 V regulator, we tell it to
> regulator_set_voltage_tol(1800000,100000). It will try the 1.8V-1.9V
> range, when it sees that it can't it'll fallback to 1.7V - 1.9V, and
> it will just be lazy and pick the lowest of the range again:
> 1.7V(causing voltage drop issues because we're exactly at the minimum
> of the spec). The correct voltage would be 1.79V

This is just an implementation detail, everything you're saying here
applies equally to any user specifying by tolerance rather than range.
If the reason for doing this is to fix that problem then a new API isn't
the way to go about it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-11 12:31     ` Mark Brown
@ 2014-12-11 16:09       ` Doug Anderson
  2014-12-11 17:09         ` Mark Brown
  0 siblings, 1 reply; 15+ messages in thread
From: Doug Anderson @ 2014-12-11 16:09 UTC (permalink / raw)
  To: Mark Brown
  Cc: Alexandru Stan, Ulf Hansson, Heiko Stuebner, Jaehoon Chung,
	Seungwon Jeon, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Liam Girdwood, linux-kernel

Mark,

On Thu, Dec 11, 2014 at 4:31 AM, Mark Brown <broonie@kernel.org> wrote:
> This is just an implementation detail, everything you're saying here
> applies equally to any user specifying by tolerance rather than range.
> If the reason for doing this is to fix that problem then a new API isn't
> the way to go about it.

OK, I'll give a shot at taking my code and using it as a new
implementation for regulator_set_voltage_tol().  In the SD card code
I'll pick some reasonable tolerances--they won't be exactly what the
spec says, but they ought to be good enough.  If Ulf comes back and
yells at me then we can revisit adding a new API.

Thanks!

-Doug

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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-11 16:09       ` Doug Anderson
@ 2014-12-11 17:09         ` Mark Brown
  2014-12-11 19:55           ` Doug Anderson
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Brown @ 2014-12-11 17:09 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Alexandru Stan, Ulf Hansson, Heiko Stuebner, Jaehoon Chung,
	Seungwon Jeon, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Liam Girdwood, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

On Thu, Dec 11, 2014 at 08:09:13AM -0800, Doug Anderson wrote:
> On Thu, Dec 11, 2014 at 4:31 AM, Mark Brown <broonie@kernel.org> wrote:

> > This is just an implementation detail, everything you're saying here
> > applies equally to any user specifying by tolerance rather than range.
> > If the reason for doing this is to fix that problem then a new API isn't
> > the way to go about it.

> OK, I'll give a shot at taking my code and using it as a new
> implementation for regulator_set_voltage_tol().  In the SD card code
> I'll pick some reasonable tolerances--they won't be exactly what the
> spec says, but they ought to be good enough.  If Ulf comes back and
> yells at me then we can revisit adding a new API.

OK, thanks.  Even if a new interface does get added the implementation
needs to be shared with that for setting by tolerance, they're doing the
same thing.

Please also bear in mind the need to handle shared supplies in your
implementation.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-11 17:09         ` Mark Brown
@ 2014-12-11 19:55           ` Doug Anderson
  2014-12-12  0:24             ` Mark Brown
  0 siblings, 1 reply; 15+ messages in thread
From: Doug Anderson @ 2014-12-11 19:55 UTC (permalink / raw)
  To: Mark Brown
  Cc: Alexandru Stan, Ulf Hansson, Heiko Stuebner, Jaehoon Chung,
	Seungwon Jeon, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Liam Girdwood, linux-kernel

Mark,

On Thu, Dec 11, 2014 at 9:09 AM, Mark Brown <broonie@kernel.org> wrote:
>> OK, I'll give a shot at taking my code and using it as a new
>> implementation for regulator_set_voltage_tol().  In the SD card code
>> I'll pick some reasonable tolerances--they won't be exactly what the
>> spec says, but they ought to be good enough.  If Ulf comes back and
>> yells at me then we can revisit adding a new API.
>
> OK, thanks.  Even if a new interface does get added the implementation
> needs to be shared with that for setting by tolerance, they're doing the
> same thing.

Yup, exactly.  I'll admit wasn't originally aware of the tolerance API
and my first thought after your email was to reimplement it atop my
patch.  ...but just having tolerance right now also seems sane.


> Please also bear in mind the need to handle shared supplies in your
> implementation.

I'm being dense, can you give more details?  Do you want me to grab
the mutex or do something smarter like track the voltage / tolerance
requested by multiple clients and resolve them, or ...?

I didn't grab any mutex because I thought all of the attributes I was
looking at were unchanging.  If they're not then
regulator_is_supported_voltage() probably has a bug since I'm reading
roughly the same attributes that it is.  ...and in fact the flow of my
code is also amost the same as regulator_is_supported_voltage()...

-Doug

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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-11 19:55           ` Doug Anderson
@ 2014-12-12  0:24             ` Mark Brown
  2014-12-12  3:31               ` Doug Anderson
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Brown @ 2014-12-12  0:24 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Alexandru Stan, Ulf Hansson, Heiko Stuebner, Jaehoon Chung,
	Seungwon Jeon, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Liam Girdwood, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

On Thu, Dec 11, 2014 at 11:55:55AM -0800, Doug Anderson wrote:
> On Thu, Dec 11, 2014 at 9:09 AM, Mark Brown <broonie@kernel.org> wrote:

> > Please also bear in mind the need to handle shared supplies in your
> > implementation.

> I'm being dense, can you give more details?  Do you want me to grab
> the mutex or do something smarter like track the voltage / tolerance
> requested by multiple clients and resolve them, or ...?

I mean the latter - what happens if more than one consumer is trying to
use the regulator?  This is IIRC why _set_voltage_tol() uses the cheap
and nasty implementation it does.  There's also the potential
performance considerations for the DVS type applications now I think
about it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-12  0:24             ` Mark Brown
@ 2014-12-12  3:31               ` Doug Anderson
  2014-12-12 12:59                 ` Mark Brown
  0 siblings, 1 reply; 15+ messages in thread
From: Doug Anderson @ 2014-12-12  3:31 UTC (permalink / raw)
  To: Mark Brown
  Cc: Alexandru Stan, Ulf Hansson, Heiko Stuebner, Jaehoon Chung,
	Seungwon Jeon, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Liam Girdwood, linux-kernel

Mark,

On Thu, Dec 11, 2014 at 4:24 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Dec 11, 2014 at 11:55:55AM -0800, Doug Anderson wrote:
>> On Thu, Dec 11, 2014 at 9:09 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > Please also bear in mind the need to handle shared supplies in your
>> > implementation.
>
>> I'm being dense, can you give more details?  Do you want me to grab
>> the mutex or do something smarter like track the voltage / tolerance
>> requested by multiple clients and resolve them, or ...?
>
> I mean the latter - what happens if more than one consumer is trying to
> use the regulator?  This is IIRC why _set_voltage_tol() uses the cheap
> and nasty implementation it does.

Ah, I see.  I don't think I've ever encountered a case where there
were two consumers for a regulator that actually requested voltages...

...but isn't regulator_set_voltage_tol() broken there?  If you have
two clients, A and B and a regulator that can go 1.0V to 5.0V in .05V
increments:

A requests 1.8V +/- .1V.  We get 1.8V
B requests 1.7V +/- .05V.

The above could be achievable with a voltage of 1.75V but it won't
work with the current regulator_set_voltage_tol() I think.


> There's also the potential
> performance considerations for the DVS type applications now I think
> about it.

Iterating through voltages is really that slow?  If so, perhaps we
could add some caching to keep track of what voltage we actually got
last time...  I could also add an optimization to try the exact
requested voltage right away...

-Doug

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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-12  3:31               ` Doug Anderson
@ 2014-12-12 12:59                 ` Mark Brown
  2014-12-15 22:11                   ` Doug Anderson
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Brown @ 2014-12-12 12:59 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Alexandru Stan, Ulf Hansson, Heiko Stuebner, Jaehoon Chung,
	Seungwon Jeon, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Liam Girdwood, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

On Thu, Dec 11, 2014 at 07:31:43PM -0800, Doug Anderson wrote:
> On Thu, Dec 11, 2014 at 4:24 PM, Mark Brown <broonie@kernel.org> wrote:

> > I mean the latter - what happens if more than one consumer is trying to
> > use the regulator?  This is IIRC why _set_voltage_tol() uses the cheap
> > and nasty implementation it does.

> Ah, I see.  I don't think I've ever encountered a case where there
> were two consumers for a regulator that actually requested voltages...

> ...but isn't regulator_set_voltage_tol() broken there?  If you have
> two clients, A and B and a regulator that can go 1.0V to 5.0V in .05V
> increments:

> A requests 1.8V +/- .1V.  We get 1.8V
> B requests 1.7V +/- .05V.

> The above could be achievable with a voltage of 1.75V but it won't
> work with the current regulator_set_voltage_tol() I think.

Yeah, it's not perfect but it'll work most of the time.

> > There's also the potential
> > performance considerations for the DVS type applications now I think
> > about it.

> Iterating through voltages is really that slow?  If so, perhaps we
> could add some caching to keep track of what voltage we actually got
> last time...  I could also add an optimization to try the exact
> requested voltage right away...

Applications like DVS get pretty performance sensitive and for a
regulator with high resolution if you're trying to hit a voltage at the
top of the range you could be iterating over a hundred or more values.
Perhaps doing something based on the various factorings out of the
voltage mapping would do the trick, add a new op for getting to the
closest voltage?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-12 12:59                 ` Mark Brown
@ 2014-12-15 22:11                   ` Doug Anderson
  2014-12-16 13:13                     ` Mark Brown
  0 siblings, 1 reply; 15+ messages in thread
From: Doug Anderson @ 2014-12-15 22:11 UTC (permalink / raw)
  To: Mark Brown
  Cc: Alexandru Stan, Ulf Hansson, Heiko Stuebner, Jaehoon Chung,
	Seungwon Jeon, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Liam Girdwood, linux-kernel

Mark,

On Fri, Dec 12, 2014 at 4:59 AM, Mark Brown <broonie@kernel.org> wrote:
>> > There's also the potential
>> > performance considerations for the DVS type applications now I think
>> > about it.
>
>> Iterating through voltages is really that slow?  If so, perhaps we
>> could add some caching to keep track of what voltage we actually got
>> last time...  I could also add an optimization to try the exact
>> requested voltage right away...
>
> Applications like DVS get pretty performance sensitive and for a
> regulator with high resolution if you're trying to hit a voltage at the
> top of the range you could be iterating over a hundred or more values.

OK, you've convinced me.  :)


> Perhaps doing something based on the various factorings out of the
> voltage mapping would do the trick, add a new op for getting to the
> closest voltage?

I'd really rather add a new op because I think it would mean that all
the old regulators that don't implement the op would be slow all of a
sudden.  :(


I looked at trying to refactor everything, but I think the answer is
that I should drop my patch and consider the existing
regulator_set_voltage_tol() experience good enough.  While I could go
through and try to make regulator_set_voltage_tol() better:

1. It fixes no issues that I know of.  On all boards that I've worked
with it is possible to make VMMC and VQMMC exactly equal.

2. As you said, regulator_set_voltage_tol() is on the critical path
for CPUfreq and the existing code is heavily optimized to work fast on
a large number of different types of regulators.  Trying to replicate
that without huge code duplication and without any bugs would be
difficult.


I'll spin my MMC patch to use regulator_set_voltage_tol().  If Ulf
responds and says that he'd like to more accurately specify the
min/max voltage then I'll post up a new function using a scheme like
regulator_set_voltage_tol() but asymmetric.


As always, thanks for your great feedback!

-Doug

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

* Re: [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage
  2014-12-15 22:11                   ` Doug Anderson
@ 2014-12-16 13:13                     ` Mark Brown
  0 siblings, 0 replies; 15+ messages in thread
From: Mark Brown @ 2014-12-16 13:13 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Alexandru Stan, Ulf Hansson, Heiko Stuebner, Jaehoon Chung,
	Seungwon Jeon, Alim Akhtar, Sonny Rao, Andrew Bresticker,
	open list:ARM/Rockchip SoC...,
	linux-arm-kernel, Liam Girdwood, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]

On Mon, Dec 15, 2014 at 02:11:02PM -0800, Doug Anderson wrote:
> On Fri, Dec 12, 2014 at 4:59 AM, Mark Brown <broonie@kernel.org> wrote:

> > Perhaps doing something based on the various factorings out of the
> > voltage mapping would do the trick, add a new op for getting to the
> > closest voltage?

> I'd really rather add a new op because I think it would mean that all
> the old regulators that don't implement the op would be slow all of a
> sudden.  :(

Assuming there's a not missing there...  I don't think it's quite that
bad, most regulators do use one of the standard mapping functions and
for those that are likely to matter for performance there's usually a
calculation rather than a lookup table.  That said...

> I looked at trying to refactor everything, but I think the answer is
> that I should drop my patch and consider the existing
> regulator_set_voltage_tol() experience good enough.  While I could go
> through and try to make regulator_set_voltage_tol() better:

...I do agree that the current situation is probably adequate though,
the optimisation would be nice but it's mainly going to benefit corner
cases.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

end of thread, other threads:[~2014-12-16 13:13 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-10 20:58 [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage Doug Anderson
2014-12-10 20:58 ` [PATCH 2/4] mmc: core: Add mmc_regulator_set_vqmmc() Doug Anderson
2014-12-10 20:58 ` [PATCH 3/4] mmc: dw_mmc: Use mmc_regulator_set_vqmmc in start_signal_voltage_switch Doug Anderson
2014-12-10 20:58 ` [PATCH 4/4] ARK: dts: Specify VMMC and VQMMC on rk3288-evb Doug Anderson
2014-12-10 23:53 ` [PATCH 1/4] regulator: core: Support trying to get close to a certain voltage Mark Brown
2014-12-11  1:08   ` Alexandru Stan
2014-12-11 12:31     ` Mark Brown
2014-12-11 16:09       ` Doug Anderson
2014-12-11 17:09         ` Mark Brown
2014-12-11 19:55           ` Doug Anderson
2014-12-12  0:24             ` Mark Brown
2014-12-12  3:31               ` Doug Anderson
2014-12-12 12:59                 ` Mark Brown
2014-12-15 22:11                   ` Doug Anderson
2014-12-16 13:13                     ` 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).