[PATCH v3 0/2] add the dvs support for rk808

[PATCH v3 0/2] add the dvs support for rk808

[Chris Zhong]

rk808 has a dvs function, there are 3 pins for dvs, dvs1 & dvs2 & dvsok.
Normally, the voltage of DCDC1/DCDC2 are controlled by BUCKn_ON_VSEL, when
we pull dvs1/dvs2 pin to active, they would be controlled by BUCKn_DVS_VSEL.
And the dvsok pin is used to indicate whether regulating has been completed.
When dvs1/dvs2 pin change, dvsok would be pull down, and it would be pull up
once the regulating is complete.

Changes in v3:
- Modify the syntax error

Changes in v2:
- increase description about dvs pins
- modify the multiline commenting

Chris Zhong (2):
  mfd: dt-bindings: add the description about dvs gpio for rk808
  regulator: rk808: add dvs support

 Documentation/devicetree/bindings/mfd/rk808.txt |  13 +-
 drivers/regulator/rk808-regulator.c             | 192 ++++++++++++++++++++++--
 2 files changed, 192 insertions(+), 13 deletions(-)

-- 
1.9.1

[PATCH v3 1/2] mfd: dt-bindings: add the description about dvs gpio for rk808

[Chris Zhong]

add the description about dvs1, dvs2, dvsok, and add the example.

Signed-off-by: Chris Zhong <zyw@rock-chips.com>
Reviewed-by: Doug Anderson <dianders@chromium.org>

---

Changes in v3:
- Modify the syntax error

Changes in v2:
- increase description about dvs pins

 Documentation/devicetree/bindings/mfd/rk808.txt | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/rk808.txt b/Documentation/devicetree/bindings/mfd/rk808.txt
index 9e6e259..9cc2941 100644
--- a/Documentation/devicetree/bindings/mfd/rk808.txt
+++ b/Documentation/devicetree/bindings/mfd/rk808.txt
@@ -24,6 +24,14 @@ Optional properties:
 - vcc10-supply: The input supply for LDO_REG6
 - vcc11-supply: The input supply for LDO_REG8
 - vcc12-supply: The input supply for SWITCH_REG2
+- dvs-gpios:  buck1/2 can be controlled by gpio dvs, this is GPIO specifiers
+  for 2 host gpio's used for dvs. The format of the gpio specifier depends in
+  the gpio controller. If DVS GPIOs aren't present, voltage changes will happen
+  very quickly with no slow ramp time.
+- dvsok-gpios: if we use dvs gpio to control buck1/2, this gpio will be pull
+  high, once RK808 completed the adjustment of voltage. If the DVSOK isn't
+  present, we'll just delay based on whatever is specified in the regulator
+  constraints.
 
 Regulators: All the regulators of RK808 to be instantiated shall be
 listed in a child node named 'regulators'. Each regulator is represented
@@ -55,7 +63,10 @@ Example:
 		interrupt-parent = <&gpio0>;
 		interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
 		pinctrl-names = "default";
-		pinctrl-0 = <&pmic_int>;
+		pinctrl-0 = <&pmic_int &dvs_ok &dvs_1 &dvs_2>;
+		dvsok-gpios = <&gpio7 9 GPIO_ACTIVE_HIGH>;
+		dvs-gpios = <&gpio7 11 GPIO_ACTIVE_HIGH>,
+			    <&gpio7 15 GPIO_ACTIVE_HIGH>;
 		reg = <0x1b>;
 		rockchip,system-power-controller;
 		wakeup-source;
-- 
1.9.1

[PATCH v3 2/2] regulator: rk808: add dvs support

[Chris Zhong]

rk808 has a dvs function, there are 3 pins for dvs, dvs1 & dvs2 & dvsok.
Normally, the voltage of DCDC1/DCDC2 are controlled by BUCKn_ON_VSEL, when
we pull dvs1/dvs2 pin to active, they would be controlled by BUCKn_DVS_VSEL.
And the dvsok pin is used to indicate whether regulating has been completed.
When dvs1/dvs2 pin change, dvsok would be pull down, and it would be pull up
once the regulating is complete.

Signed-off-by: Chris Zhong <zyw@rock-chips.com>
Reviewed-by: Doug Anderson <dianders@chromium.org>

---

Changes in v3: None
Changes in v2:
- modify the multiline commenting

 drivers/regulator/rk808-regulator.c | 192 +++++++++++++++++++++++++++++++++---
 1 file changed, 180 insertions(+), 12 deletions(-)

diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c
index 33042eb..f31e4fc 100644
--- a/drivers/regulator/rk808-regulator.c
+++ b/drivers/regulator/rk808-regulator.c
@@ -16,10 +16,13 @@
  * more details.
  */
 
-#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
 #include <linux/i2c.h>
-#include <linux/mfd/rk808.h>
+#include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/mfd/rk808.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/of_regulator.h>
 
@@ -36,12 +39,23 @@
 #define RK808_RAMP_RATE_6MV_PER_US	(2 << RK808_RAMP_RATE_OFFSET)
 #define RK808_RAMP_RATE_10MV_PER_US	(3 << RK808_RAMP_RATE_OFFSET)
 
+#define RK808_DVS2_POL		BIT(2)
+#define RK808_DVS1_POL		BIT(1)
+
 /* Offset from XXX_ON_VSEL to XXX_SLP_VSEL */
 #define RK808_SLP_REG_OFFSET 1
 
+/* Offset from XXX_ON_VSEL to XXX_DVS_VSEL */
+#define RK808_DVS_REG_OFFSET 2
+
 /* Offset from XXX_EN_REG to SLEEP_SET_OFF_XXX */
 #define RK808_SLP_SET_OFF_REG_OFFSET 2
 
+struct rk808_regulator_data {
+	struct gpio_desc *dvsok_gpio;
+	struct gpio_desc *dvs_gpio[2];
+};
+
 static const int rk808_buck_config_regs[] = {
 	RK808_BUCK1_CONFIG_REG,
 	RK808_BUCK2_CONFIG_REG,
@@ -70,6 +84,97 @@ static const struct regulator_linear_range rk808_ldo6_voltage_ranges[] = {
 	REGULATOR_LINEAR_RANGE(800000, 0, 17, 100000),
 };
 
+int rk808_buck1_2_get_voltage_sel_regmap(struct regulator_dev *rdev)
+{
+	struct rk808_regulator_data *pdata = rdev_get_drvdata(rdev);
+	int id = rdev->desc->id - RK808_ID_DCDC1;
+	struct gpio_desc *gpio = pdata->dvs_gpio[id];
+	unsigned int val;
+	int ret;
+
+	if (IS_ERR(gpio) || gpiod_get_value(gpio) == 0)
+		return regulator_get_voltage_sel_regmap(rdev);
+
+	ret = regmap_read(rdev->regmap,
+			  rdev->desc->vsel_reg + RK808_DVS_REG_OFFSET,
+			  &val);
+	if (ret != 0)
+		return ret;
+
+	val &= rdev->desc->vsel_mask;
+	val >>= ffs(rdev->desc->vsel_mask) - 1;
+
+	return val;
+}
+
+int rk808_buck1_2_set_voltage_sel(struct regulator_dev *rdev, unsigned sel)
+{
+	struct rk808_regulator_data *pdata = rdev_get_drvdata(rdev);
+	int id = rdev->desc->id - RK808_ID_DCDC1;
+	struct gpio_desc *gpio = pdata->dvs_gpio[id];
+	unsigned int reg = rdev->desc->vsel_reg;
+	unsigned long timeout;
+	unsigned old_sel;
+	int ret, gpio_level;
+
+	if (IS_ERR(gpio))
+		return regulator_set_voltage_sel_regmap(rdev, sel);
+
+	gpio_level = gpiod_get_value(gpio);
+	if (gpio_level == 0) {
+		reg += RK808_DVS_REG_OFFSET;
+		ret = regmap_read(rdev->regmap, rdev->desc->vsel_reg, &old_sel);
+	} else {
+		ret = regmap_read(rdev->regmap,
+				  reg + RK808_DVS_REG_OFFSET,
+				  &old_sel);
+	}
+
+	if (ret != 0)
+		return ret;
+
+	sel <<= ffs(rdev->desc->vsel_mask) - 1;
+	sel |= old_sel & ~rdev->desc->vsel_mask;
+
+	ret = regmap_write(rdev->regmap, reg, sel);
+	if (ret)
+		return ret;
+
+	gpiod_set_value(gpio, !gpio_level);
+
+	/*
+	 * dvsok pin would be pull down when dvs1/2 pin changed, and
+	 * it would be pull up once the voltage regulate complete.
+	 * No need to wait dvsok signal when voltage falling.
+	 */
+	if (old_sel < sel && !IS_ERR(pdata->dvsok_gpio)) {
+		timeout = jiffies + msecs_to_jiffies(1);
+		while (!time_after(jiffies, timeout)) {
+			udelay(1);
+			if (gpiod_get_value(pdata->dvsok_gpio))
+				return 0;
+		}
+		gpiod_set_value(gpio, gpio_level);
+		pr_err("%s wait dvsok timeout\n", rdev->desc->name);
+		ret = -EBUSY;
+	}
+
+	return ret;
+}
+
+int rk808_buck1_2_set_voltage_time_sel(struct regulator_dev *rdev,
+				       unsigned int old_selector,
+				       unsigned int new_selector)
+{
+	struct rk808_regulator_data *pdata = rdev_get_drvdata(rdev);
+
+	/* if there is a dvsok pin, we don't need wait extra time here */
+	if (!IS_ERR(pdata->dvsok_gpio))
+		return 0;
+
+	return regulator_set_voltage_time_sel(rdev, old_selector, new_selector);
+}
+
 static int rk808_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
 {
 	unsigned int ramp_value = RK808_RAMP_RATE_10MV_PER_US;
@@ -137,8 +242,9 @@ static int rk808_set_suspend_disable(struct regulator_dev *rdev)
 static struct regulator_ops rk808_buck1_2_ops = {
 	.list_voltage		= regulator_list_voltage_linear_range,
 	.map_voltage		= regulator_map_voltage_linear_range,
-	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
-	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.get_voltage_sel	= rk808_buck1_2_get_voltage_sel_regmap,
+	.set_voltage_sel	= rk808_buck1_2_set_voltage_sel,
+	.set_voltage_time_sel	= rk808_buck1_2_set_voltage_time_sel,
 	.enable			= regulator_enable_regmap,
 	.disable		= regulator_disable_regmap,
 	.is_enabled		= regulator_is_enabled_regmap,
@@ -372,25 +478,86 @@ static struct of_regulator_match rk808_reg_matches[] = {
 	[RK808_ID_SWITCH2]	= { .name = "SWITCH_REG2" },
 };
 
+int rk808_regulator_dt_parse_pdata(struct device *dev,
+				   struct device *client_dev,
+				   struct regmap *map,
+				   struct rk808_regulator_data *pdata)
+{
+	struct device_node *np;
+	int tmp, ret, i;
+
+	np = of_get_child_by_name(client_dev->of_node, "regulators");
+	if (!np)
+		return -ENXIO;
+
+	ret = of_regulator_match(dev, np, rk808_reg_matches,
+				 RK808_NUM_REGULATORS);
+	if (ret < 0)
+		goto dt_parse_end;
+
+	for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
+		pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i);
+		if (IS_ERR(pdata->dvs_gpio[i])) {
+			dev_warn(dev, "there is no dvs%d gpio\n", i);
+			continue;
+		}
+
+		gpiod_direction_output(pdata->dvs_gpio[i], 0);
+
+		tmp = i ? RK808_DVS2_POL : RK808_DVS1_POL;
+		ret = regmap_update_bits(map, RK808_IO_POL_REG, tmp,
+				gpiod_is_active_low(pdata->dvs_gpio[i]) ?
+				0 : tmp);
+	}
+
+	pdata->dvsok_gpio = gpiod_get_index(client_dev, "dvsok", 0);
+	if (IS_ERR(pdata->dvsok_gpio)) {
+		dev_warn(dev, "there is no dvs ok gpio\n");
+		goto dt_parse_end;
+	}
+
+	gpiod_direction_input(pdata->dvsok_gpio);
+
+dt_parse_end:
+	of_node_put(np);
+	return ret;
+}
+
+static int rk808_regulator_remove(struct platform_device *pdev)
+{
+	struct rk808_regulator_data *pdata = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
+		if (!IS_ERR(pdata->dvs_gpio[i]))
+			gpiod_put(pdata->dvs_gpio[i]);
+	}
+
+	if (!IS_ERR(pdata->dvsok_gpio))
+			gpiod_put(pdata->dvsok_gpio);
+	return 0;
+}
+
 static int rk808_regulator_probe(struct platform_device *pdev)
 {
 	struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
 	struct i2c_client *client = rk808->i2c;
-	struct device_node *reg_np;
 	struct regulator_config config = {};
 	struct regulator_dev *rk808_rdev;
+	struct rk808_regulator_data *pdata;
 	int ret, i;
 
-	reg_np = of_get_child_by_name(client->dev.of_node, "regulators");
-	if (!reg_np)
-		return -ENXIO;
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
 
-	ret = of_regulator_match(&pdev->dev, reg_np, rk808_reg_matches,
-				 RK808_NUM_REGULATORS);
-	of_node_put(reg_np);
+	ret = rk808_regulator_dt_parse_pdata(&pdev->dev, &client->dev,
+					     rk808->regmap, pdata);
 	if (ret < 0)
 		return ret;
 
+	platform_set_drvdata(pdev, pdata);
+
 	/* Instantiate the regulators */
 	for (i = 0; i < RK808_NUM_REGULATORS; i++) {
 		if (!rk808_reg_matches[i].init_data ||
@@ -398,7 +565,7 @@ static int rk808_regulator_probe(struct platform_device *pdev)
 			continue;
 
 		config.dev = &client->dev;
-		config.driver_data = rk808;
+		config.driver_data = pdata;
 		config.regmap = rk808->regmap;
 		config.of_node = rk808_reg_matches[i].of_node;
 		config.init_data = rk808_reg_matches[i].init_data;
@@ -417,6 +584,7 @@ static int rk808_regulator_probe(struct platform_device *pdev)
 
 static struct platform_driver rk808_regulator_driver = {
 	.probe = rk808_regulator_probe,
+	.remove = rk808_regulator_remove,
 	.driver = {
 		.name = "rk808-regulator",
 		.owner = THIS_MODULE,
-- 
1.9.1

Re: [PATCH v3 2/2] regulator: rk808: add dvs support

[Mark Brown]

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

On Mon, Dec 15, 2014 at 11:07:58AM +0800, Chris Zhong wrote:

> +	sel <<= ffs(rdev->desc->vsel_mask) - 1;
> +	sel |= old_sel & ~rdev->desc->vsel_mask;
> +
> +	ret = regmap_write(rdev->regmap, reg, sel);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value(gpio, !gpio_level);

So, this seems a bit odd.  What we appear to be doing here is
alternating between the two different voltage setting registers which is
all well and good but makes me wonder why we're bothering - it's a bit
more work than just sticking with one.  We do get...

> +	/*
> +	 * dvsok pin would be pull down when dvs1/2 pin changed, and
> +	 * it would be pull up once the voltage regulate complete.
> +	 * No need to wait dvsok signal when voltage falling.
> +	 */

...this but unless the voltage typically ramps much faster than spec
it's never clear to me that we're actually winning by polling the pin
instead of just dead reckoning the time, it's more work for the CPU to
poll the GPIO than to sleep after all.

One thing we can do with hardware like this is to program in a voltage
we're likely to want to switch to quickly and then use the GPIO to get
there.  That can be a bit hard to arrange with the regulator API as it
currently stands since we don't exactly have an interface for it.  

We can just check to see what the two values are current set to before
switching and skip the register write if it's the same (making things
faster since we're typically avoiding an I2C or SPI transaction by doing
that) but that's a bit meh.  We can also try to do things like keep the
top voltage from the voltage ranges we're being given programmed which
for DVS typically ends up doing a reasonable job since governors often
like to jump straight to top speed when things get busy so that's one of
the common cases where we most want to change voltages as quickly as
possible.

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

Re: [PATCH v3 2/2] regulator: rk808: add dvs support

[Chris Zhong]

On 12/30/2014 01:25 AM, Mark Brown wrote:
> On Mon, Dec 15, 2014 at 11:07:58AM +0800, Chris Zhong wrote:
>
>> +	sel <<= ffs(rdev->desc->vsel_mask) - 1;
>> +	sel |= old_sel & ~rdev->desc->vsel_mask;
>> +
>> +	ret = regmap_write(rdev->regmap, reg, sel);
>> +	if (ret)
>> +		return ret;
>> +
>> +	gpiod_set_value(gpio, !gpio_level);
> So, this seems a bit odd.  What we appear to be doing here is
> alternating between the two different voltage setting registers which is
> all well and good but makes me wonder why we're bothering - it's a bit
> more work than just sticking with one.  We do get...
you mean check the old_selector and selector? I think
_regulator_do_set_voltage have done it.
>
>> +	/*
>> +	 * dvsok pin would be pull down when dvs1/2 pin changed, and
>> +	 * it would be pull up once the voltage regulate complete.
>> +	 * No need to wait dvsok signal when voltage falling.
>> +	 */
> ...this but unless the voltage typically ramps much faster than spec
> it's never clear to me that we're actually winning by polling the pin
> instead of just dead reckoning the time, it's more work for the CPU to
> poll the GPIO than to sleep after all.
Actually, it's slower than spec, so I think getting this dvsok pin state
is safer than delay.
>
> One thing we can do with hardware like this is to program in a voltage
> we're likely to want to switch to quickly and then use the GPIO to get
> there.  That can be a bit hard to arrange with the regulator API as it
> currently stands since we don't exactly have an interface for it.
>
> We can just check to see what the two values are current set to before
> switching and skip the register write if it's the same (making things
> faster since we're typically avoiding an I2C or SPI transaction by doing
> that) but that's a bit meh.  We can also try to do things like keep the
> top voltage from the voltage ranges we're being given programmed which
> for DVS typically ends up doing a reasonable job since governors often
> like to jump straight to top speed when things get busy so that's one of
> the common cases where we most want to change voltages as quickly as
> possible.

Re: [PATCH v3 2/2] regulator: rk808: add dvs support

[Mark Brown]

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

On Tue, Dec 30, 2014 at 11:07:14AM +0800, Chris Zhong wrote:
> On 12/30/2014 01:25 AM, Mark Brown wrote:

> >So, this seems a bit odd.  What we appear to be doing here is
> >alternating between the two different voltage setting registers which is
> >all well and good but makes me wonder why we're bothering - it's a bit
> >more work than just sticking with one.  We do get...

> you mean check the old_selector and selector? I think
> _regulator_do_set_voltage have done it.

No, I mean that we may as well just always write to the same register
and save a bunch of code.

> >...this but unless the voltage typically ramps much faster than spec
> >it's never clear to me that we're actually winning by polling the pin
> >instead of just dead reckoning the time, it's more work for the CPU to
> >poll the GPIO than to sleep after all.

> Actually, it's slower than spec, so I think getting this dvsok pin state
> is safer than delay.

Well, that suggests that the spec is wrong which ought to be fixed
anyway oughtn't it?  Or are you saying that the delay is inconsistent as
well as slower than advertised?

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

Re: [PATCH v3 2/2] regulator: rk808: add dvs support

[Chris Zhong]

On 12/31/2014 01:04 AM, Mark Brown wrote:
> On Tue, Dec 30, 2014 at 11:07:14AM +0800, Chris Zhong wrote:
>> On 12/30/2014 01:25 AM, Mark Brown wrote:
>>> So, this seems a bit odd.  What we appear to be doing here is
>>> alternating between the two different voltage setting registers which is
>>> all well and good but makes me wonder why we're bothering - it's a bit
>>> more work than just sticking with one.  We do get...
>> you mean check the old_selector and selector? I think
>> _regulator_do_set_voltage have done it.
> No, I mean that we may as well just always write to the same register
> and save a bunch of code.
No, when we pull down DVSn pin, the voltage value is from 
RK808_BUCK1_ON_VSEL_REG,
and when we pull up DVSn pin, the voltage value if from 
RK808_BUCK1_ON_VSEL_REG+2.
We want to this dvs function for a better voltage wave, avoid overshoot, 
if someone do not
need this function, they could remove the setting of DVSn pin in dts 
file, and at that time,
rk808_regulator will use a same register for setting voltage.


>>> ...this but unless the voltage typically ramps much faster than spec
>>> it's never clear to me that we're actually winning by polling the pin
>>> instead of just dead reckoning the time, it's more work for the CPU to
>>> poll the GPIO than to sleep after all.
>> Actually, it's slower than spec, so I think getting this dvsok pin state
>> is safer than delay.
> Well, that suggests that the spec is wrong which ought to be fixed
> anyway oughtn't it?  Or are you saying that the delay is inconsistent as
> well as slower than advertised?
the spec said 2/4/6/10 mv/us, but the ramp will change depending on the 
load.
So I think the dvsok pin is more accurate, since it It will soon change, 
once the
regulator is completed. the delay is a fixed time. it is faster than 
dvsok pin.

Re: [PATCH v3 2/2] regulator: rk808: add dvs support

[Doug Anderson]

Mark,

On Mon, Dec 29, 2014 at 9:25 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Dec 15, 2014 at 11:07:58AM +0800, Chris Zhong wrote:
>
>> +     sel <<= ffs(rdev->desc->vsel_mask) - 1;
>> +     sel |= old_sel & ~rdev->desc->vsel_mask;
>> +
>> +     ret = regmap_write(rdev->regmap, reg, sel);
>> +     if (ret)
>> +             return ret;
>> +
>> +     gpiod_set_value(gpio, !gpio_level);
>
> So, this seems a bit odd.  What we appear to be doing here is
> alternating between the two different voltage setting registers which is
> all well and good but makes me wonder why we're bothering - it's a bit
> more work than just sticking with one.  We do get...

It's odd, but the rk808 is odd.

Specifically if you don't use the "DVFS" pins like this then the
hardware ignores the ramp rate that's programmed into it.  When I
submitted (8af2522 regulator: rk808: Add function for ramp delay for
buck1/buck2) upstream I verified my change by looking at the register
values that were being set (ran i2cget in userspace).  I didn't
measure the waveforms myself.  Later, Chris found that my change was
not doing anything at all because the ramp delay has no effect if you
are setting the voltage using an i2c write--it only has an effect if
you're setting the voltage using DVFS pins.

If you write the voltage directly using an i2c write the rk808 will
ramp as fast as it possibly can, which will probably lead to an
overshoot.  You could fix this by changing the voltage more slowly in
software, but it seems better to use the DVFS pin if we have it...


>> +     /*
>> +      * dvsok pin would be pull down when dvs1/2 pin changed, and
>> +      * it would be pull up once the voltage regulate complete.
>> +      * No need to wait dvsok signal when voltage falling.
>> +      */
>
> ...this but unless the voltage typically ramps much faster than spec
> it's never clear to me that we're actually winning by polling the pin
> instead of just dead reckoning the time, it's more work for the CPU to
> poll the GPIO than to sleep after all.

Possibly even fewer CPU cycles: we could configure this GPIO as an
interrupt.  Then we can look for the rising edge in hardware.  ...you
might need to make sure that the pin actually falls and goes back up,
though.  If you changed between two voltages that were pretty close to
begin with I wonder if the edge could be missed.  I guess you could
make it high-level sensitive and put the 1us delay before unmasking
it...


> One thing we can do with hardware like this is to program in a voltage
> we're likely to want to switch to quickly and then use the GPIO to get
> there.  That can be a bit hard to arrange with the regulator API as it
> currently stands since we don't exactly have an interface for it.
>
> We can just check to see what the two values are current set to before
> switching and skip the register write if it's the same (making things
> faster since we're typically avoiding an I2C or SPI transaction by doing
> that) but that's a bit meh.  We can also try to do things like keep the
> top voltage from the voltage ranges we're being given programmed which
> for DVS typically ends up doing a reasonable job since governors often
> like to jump straight to top speed when things get busy so that's one of
> the common cases where we most want to change voltages as quickly as
> possible.

You're right that there could be some cool optimizations here, but
since we really need to do a switch from A-to-B on every voltage
change to get the ramp speed right then I don't think we can do them.

Note that the MFD driver sets things up using regcache, so we
shouldn't ever do a needless i2c transaction.

-Doug

Re: [PATCH v3 2/2] regulator: rk808: add dvs support

[Mark Brown]

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

On Fri, Jan 02, 2015 at 10:41:13AM -0800, Doug Anderson wrote:
> On Mon, Dec 29, 2014 at 9:25 AM, Mark Brown <broonie@kernel.org> wrote:

> > So, this seems a bit odd.  What we appear to be doing here is
> > alternating between the two different voltage setting registers which is
> > all well and good but makes me wonder why we're bothering - it's a bit
> > more work than just sticking with one.  We do get...

> It's odd, but the rk808 is odd.

> Specifically if you don't use the "DVFS" pins like this then the
> hardware ignores the ramp rate that's programmed into it.  When I
> submitted (8af2522 regulator: rk808: Add function for ramp delay for
> buck1/buck2) upstream I verified my change by looking at the register
> values that were being set (ran i2cget in userspace).  I didn't
> measure the waveforms myself.  Later, Chris found that my change was
> not doing anything at all because the ramp delay has no effect if you
> are setting the voltage using an i2c write--it only has an effect if
> you're setting the voltage using DVFS pins.

> If you write the voltage directly using an i2c write the rk808 will
> ramp as fast as it possibly can, which will probably lead to an
> overshoot.  You could fix this by changing the voltage more slowly in
> software, but it seems better to use the DVFS pin if we have it...

OK, so this is all stuff that should go in at *least* the commit log and
most likely comments in the driver since right now it just looks like
it's adding complexity.  It does also sound like the regulation here is
a bit interesting, normally the ramp time control is more about limiting
inrush currents than suppressing transients.

Currrently we'd end up with code that just looks redundant.

> > ...this but unless the voltage typically ramps much faster than spec
> > it's never clear to me that we're actually winning by polling the pin
> > instead of just dead reckoning the time, it's more work for the CPU to
> > poll the GPIO than to sleep after all.

> Possibly even fewer CPU cycles: we could configure this GPIO as an
> interrupt.  Then we can look for the rising edge in hardware.  ...you
> might need to make sure that the pin actually falls and goes back up,
> though.  If you changed between two voltages that were pretty close to
> begin with I wonder if the edge could be missed.  I guess you could
> make it high-level sensitive and put the 1us delay before unmasking
> it...

Yes, if you make it interrupt driven it might be less work though then
you have to worry about interrupt and scheduling latencies.

> > We can just check to see what the two values are current set to before
> > switching and skip the register write if it's the same (making things
> > faster since we're typically avoiding an I2C or SPI transaction by doing
> > that) but that's a bit meh.  We can also try to do things like keep the
> > top voltage from the voltage ranges we're being given programmed which
> > for DVS typically ends up doing a reasonable job since governors often
> > like to jump straight to top speed when things get busy so that's one of
> > the common cases where we most want to change voltages as quickly as
> > possible.

> You're right that there could be some cool optimizations here, but
> since we really need to do a switch from A-to-B on every voltage
> change to get the ramp speed right then I don't think we can do them.

Like I say this needs to be obvious in the code, the combination of
calling this "DVS" (which is normally some sort of performance
optimisation thing for fast voltage changes) and lack of any information
on what it's supposed to achieve mean that the patch as it stands has
no clear goal or benefit.  Someone has to know not to come along and try
to do such optimizations later on if nothing else.

> Note that the MFD driver sets things up using regcache, so we
> shouldn't ever do a needless i2c transaction.

That's not the point really, the point is about doing a better job of
predicting what voltages we might want to transition to.

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