| Message ID | 1418612879-16094-3-git-send-email-zyw@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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?
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.
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
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.
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,