[PATCH v4 1/2] regulator: implement basic reference counter
Some devices share a regulator supply, when the first one will request regulator disable, the second device will have it's supply cut off before graciously shutting down. Hence there will be timeouts and other failed operations. Implement a reference counter mechanism similar with what is done in Linux, to keep track of enable and disable requests, and only disable the regulator when the last of the consumers has requested shutdown.
Signed-off-by: Eugen Hristev eugen.hristev@collabora.com Reviewed-by: Simon Glass sjg@chromium.org --- Changes in v4: - add documentation for error codes Changes in v3: - add error return codes Changes in v2: - add info in header regarding the function
drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.h | 21 +++++++++++++++++++++ 2 files changed, 43 insertions(+)
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 93d8196b381e..484a4fc31ef7 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -73,6 +73,23 @@ int regulator_common_set_enable(const struct udevice *dev, return 0; }
+ /* If previously enabled, increase count */ + if (enable && dev_pdata->enable_count > 0) { + dev_pdata->enable_count++; + return -EALREADY; + } + + if (!enable) { + if (dev_pdata->enable_count > 1) { + /* If enabled multiple times, decrease count */ + dev_pdata->enable_count--; + return -EBUSY; + } else if (!dev_pdata->enable_count) { + /* If already disabled, do nothing */ + return -EALREADY; + } + } + ret = dm_gpio_set_value(&dev_pdata->gpio, enable); if (ret) { pr_err("Can't set regulator : %s gpio to: %d\n", dev->name, @@ -87,5 +104,10 @@ int regulator_common_set_enable(const struct udevice *dev, if (!enable && dev_pdata->off_on_delay_us) udelay(dev_pdata->off_on_delay_us);
+ if (enable) + dev_pdata->enable_count++; + else + dev_pdata->enable_count--; + return 0; } diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index c10492f01675..0faab447d099 100644 --- a/drivers/power/regulator/regulator_common.h +++ b/drivers/power/regulator/regulator_common.h @@ -13,6 +13,7 @@ struct regulator_common_plat { struct gpio_desc gpio; /* GPIO for regulator enable control */ unsigned int startup_delay_us; unsigned int off_on_delay_us; + unsigned int enable_count; };
int regulator_common_of_to_plat(struct udevice *dev, @@ -20,6 +21,26 @@ int regulator_common_of_to_plat(struct udevice *dev, char *enable_gpio_name); int regulator_common_get_enable(const struct udevice *dev, struct regulator_common_plat *dev_pdata); +/* + * Enable or Disable a regulator + * + * This is a reentrant function and subsequent calls that enable will + * increase an internal counter, and disable calls will decrease the counter. + * The actual resource will be enabled when the counter gets to 1 coming from 0, + * and disabled when it reaches 0 coming from 1. + * + * @dev: regulator device + * @dev_pdata: Platform data + * @enable: bool indicating whether to enable or disable the regulator + * @return: + * 0 on Success + * -EBUSY if the regulator cannot be disabled because it's requested by + * another device + * -EALREADY if the regulator has already been enabled or has already been + * disabled + * -EACCES if there is no possibility to enable/disable the regulator + * -ve on different error situation + */ int regulator_common_set_enable(const struct udevice *dev, struct regulator_common_plat *dev_pdata, bool enable);
Simplify the subsystem by renaming `dev_pdata` to just `plat`. No functional change, just trivial renaming.
Suggested-by: Simon Glass sjg@chromium.org Signed-off-by: Eugen Hristev eugen.hristev@collabora.com Reviewed-by: Simon Glass sjg@chromium.org --- Changes in v3,v4: - none Changes in v2: - this is a new patch
drivers/power/regulator/fan53555.c | 10 ++-- drivers/power/regulator/fixed.c | 16 +++---- drivers/power/regulator/gpio-regulator.c | 46 +++++++++---------- drivers/power/regulator/regulator_common.c | 53 +++++++++++----------- drivers/power/regulator/regulator_common.h | 8 ++-- 5 files changed, 66 insertions(+), 67 deletions(-)
diff --git a/drivers/power/regulator/fan53555.c b/drivers/power/regulator/fan53555.c index 5681206bbafe..815f96beef61 100644 --- a/drivers/power/regulator/fan53555.c +++ b/drivers/power/regulator/fan53555.c @@ -101,7 +101,7 @@ struct fan53555_priv {
static int fan53555_regulator_of_to_plat(struct udevice *dev) { - struct fan53555_plat *dev_pdata = dev_get_plat(dev); + struct fan53555_plat *plat = dev_get_plat(dev); struct dm_regulator_uclass_plat *uc_pdata = dev_get_uclass_plat(dev); u32 sleep_vsel; @@ -118,12 +118,12 @@ static int fan53555_regulator_of_to_plat(struct udevice *dev) */ switch (sleep_vsel) { case FAN53555_VSEL0: - dev_pdata->sleep_reg = FAN53555_VSEL0; - dev_pdata->vol_reg = FAN53555_VSEL1; + plat->sleep_reg = FAN53555_VSEL0; + plat->vol_reg = FAN53555_VSEL1; break; case FAN53555_VSEL1: - dev_pdata->sleep_reg = FAN53555_VSEL1; - dev_pdata->vol_reg = FAN53555_VSEL0; + plat->sleep_reg = FAN53555_VSEL1; + plat->vol_reg = FAN53555_VSEL0; break; default: pr_err("%s: invalid vsel id %d\n", dev->name, sleep_vsel); diff --git a/drivers/power/regulator/fixed.c b/drivers/power/regulator/fixed.c index 90004d1601a9..ad3b4b98d667 100644 --- a/drivers/power/regulator/fixed.c +++ b/drivers/power/regulator/fixed.c @@ -24,16 +24,16 @@ struct fixed_clock_regulator_plat { static int fixed_regulator_of_to_plat(struct udevice *dev) { struct dm_regulator_uclass_plat *uc_pdata; - struct regulator_common_plat *dev_pdata; + struct regulator_common_plat *plat;
- dev_pdata = dev_get_plat(dev); + plat = dev_get_plat(dev); uc_pdata = dev_get_uclass_plat(dev); if (!uc_pdata) return -ENXIO;
uc_pdata->type = REGULATOR_TYPE_FIXED;
- return regulator_common_of_to_plat(dev, dev_pdata, "gpio"); + return regulator_common_of_to_plat(dev, plat, "gpio"); }
static int fixed_regulator_get_value(struct udevice *dev) @@ -88,7 +88,7 @@ static int fixed_clock_regulator_get_enable(struct udevice *dev) static int fixed_clock_regulator_set_enable(struct udevice *dev, bool enable) { struct fixed_clock_regulator_plat *priv = dev_get_priv(dev); - struct regulator_common_plat *dev_pdata = dev_get_plat(dev); + struct regulator_common_plat *plat = dev_get_plat(dev); int ret = 0;
if (enable) { @@ -101,11 +101,11 @@ static int fixed_clock_regulator_set_enable(struct udevice *dev, bool enable) if (ret) return ret;
- if (enable && dev_pdata->startup_delay_us) - udelay(dev_pdata->startup_delay_us); + if (enable && plat->startup_delay_us) + udelay(plat->startup_delay_us);
- if (!enable && dev_pdata->off_on_delay_us) - udelay(dev_pdata->off_on_delay_us); + if (!enable && plat->off_on_delay_us) + udelay(plat->off_on_delay_us);
return ret; } diff --git a/drivers/power/regulator/gpio-regulator.c b/drivers/power/regulator/gpio-regulator.c index 9c0a68aa5af4..ded7be059bb0 100644 --- a/drivers/power/regulator/gpio-regulator.c +++ b/drivers/power/regulator/gpio-regulator.c @@ -27,12 +27,12 @@ struct gpio_regulator_plat { static int gpio_regulator_of_to_plat(struct udevice *dev) { struct dm_regulator_uclass_plat *uc_pdata; - struct gpio_regulator_plat *dev_pdata; + struct gpio_regulator_plat *plat; struct gpio_desc *gpio; int ret, count, i, j; u32 states_array[GPIO_REGULATOR_MAX_STATES * 2];
- dev_pdata = dev_get_plat(dev); + plat = dev_get_plat(dev); uc_pdata = dev_get_uclass_plat(dev); if (!uc_pdata) return -ENXIO; @@ -47,7 +47,7 @@ static int gpio_regulator_of_to_plat(struct udevice *dev) * per gpio-regulator. As of now no instance with multiple * gpios is presnt */ - gpio = &dev_pdata->gpio; + gpio = &plat->gpio; ret = gpio_request_by_name(dev, "gpios", 0, gpio, GPIOD_IS_OUT); if (ret) debug("regulator gpio - not found! Error: %d", ret); @@ -68,21 +68,21 @@ static int gpio_regulator_of_to_plat(struct udevice *dev) return ret;
for (i = 0, j = 0; i < count; i += 2) { - dev_pdata->voltages[j] = states_array[i]; - dev_pdata->states[j] = states_array[i + 1]; + plat->voltages[j] = states_array[i]; + plat->states[j] = states_array[i + 1]; j++; }
- return regulator_common_of_to_plat(dev, &dev_pdata->common, "enable-gpios"); + return regulator_common_of_to_plat(dev, &plat->common, "enable-gpios"); }
static int gpio_regulator_get_value(struct udevice *dev) { struct dm_regulator_uclass_plat *uc_pdata; - struct gpio_regulator_plat *dev_pdata = dev_get_plat(dev); + struct gpio_regulator_plat *plat = dev_get_plat(dev); int enable;
- if (!dev_pdata->gpio.dev) + if (!plat->gpio.dev) return -ENOSYS;
uc_pdata = dev_get_uclass_plat(dev); @@ -91,30 +91,30 @@ static int gpio_regulator_get_value(struct udevice *dev) return -EINVAL; }
- enable = dm_gpio_get_value(&dev_pdata->gpio); - if (enable == dev_pdata->states[0]) - return dev_pdata->voltages[0]; + enable = dm_gpio_get_value(&plat->gpio); + if (enable == plat->states[0]) + return plat->voltages[0]; else - return dev_pdata->voltages[1]; + return plat->voltages[1]; }
static int gpio_regulator_set_value(struct udevice *dev, int uV) { - struct gpio_regulator_plat *dev_pdata = dev_get_plat(dev); + struct gpio_regulator_plat *plat = dev_get_plat(dev); int ret; bool enable;
- if (!dev_pdata->gpio.dev) + if (!plat->gpio.dev) return -ENOSYS;
- if (uV == dev_pdata->voltages[0]) - enable = dev_pdata->states[0]; - else if (uV == dev_pdata->voltages[1]) - enable = dev_pdata->states[1]; + if (uV == plat->voltages[0]) + enable = plat->states[0]; + else if (uV == plat->voltages[1]) + enable = plat->states[1]; else return -EINVAL;
- ret = dm_gpio_set_value(&dev_pdata->gpio, enable); + ret = dm_gpio_set_value(&plat->gpio, enable); if (ret) { pr_err("Can't set regulator : %s gpio to: %d\n", dev->name, enable); @@ -126,14 +126,14 @@ static int gpio_regulator_set_value(struct udevice *dev, int uV)
static int gpio_regulator_get_enable(struct udevice *dev) { - struct gpio_regulator_plat *dev_pdata = dev_get_plat(dev); - return regulator_common_get_enable(dev, &dev_pdata->common); + struct gpio_regulator_plat *plat = dev_get_plat(dev); + return regulator_common_get_enable(dev, &plat->common); }
static int gpio_regulator_set_enable(struct udevice *dev, bool enable) { - struct gpio_regulator_plat *dev_pdata = dev_get_plat(dev); - return regulator_common_set_enable(dev, &dev_pdata->common, enable); + struct gpio_regulator_plat *plat = dev_get_plat(dev); + return regulator_common_set_enable(dev, &plat->common, enable); }
static const struct dm_regulator_ops gpio_regulator_ops = { diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 484a4fc31ef7..e26f5ebec347 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -13,7 +13,7 @@ #include "regulator_common.h"
int regulator_common_of_to_plat(struct udevice *dev, - struct regulator_common_plat *dev_pdata, + struct regulator_common_plat *plat, const char *enable_gpio_name) { struct gpio_desc *gpio; @@ -26,7 +26,7 @@ int regulator_common_of_to_plat(struct udevice *dev, flags |= GPIOD_IS_OUT_ACTIVE;
/* Get optional enable GPIO desc */ - gpio = &dev_pdata->gpio; + gpio = &plat->gpio; ret = gpio_request_by_name(dev, enable_gpio_name, 0, gpio, flags); if (ret) { debug("Regulator '%s' optional enable GPIO - not found! Error: %d\n", @@ -36,12 +36,11 @@ int regulator_common_of_to_plat(struct udevice *dev, }
/* Get optional ramp up delay */ - dev_pdata->startup_delay_us = dev_read_u32_default(dev, - "startup-delay-us", 0); - dev_pdata->off_on_delay_us = - dev_read_u32_default(dev, "off-on-delay-us", 0); - if (!dev_pdata->off_on_delay_us) { - dev_pdata->off_on_delay_us = + plat->startup_delay_us = dev_read_u32_default(dev, + "startup-delay-us", 0); + plat->off_on_delay_us = dev_read_u32_default(dev, "off-on-delay-us", 0); + if (!plat->off_on_delay_us) { + plat->off_on_delay_us = dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0); }
@@ -49,65 +48,65 @@ int regulator_common_of_to_plat(struct udevice *dev, }
int regulator_common_get_enable(const struct udevice *dev, - struct regulator_common_plat *dev_pdata) + struct regulator_common_plat *plat) { /* Enable GPIO is optional */ - if (!dev_pdata->gpio.dev) + if (!plat->gpio.dev) return true;
- return dm_gpio_get_value(&dev_pdata->gpio); + return dm_gpio_get_value(&plat->gpio); }
int regulator_common_set_enable(const struct udevice *dev, - struct regulator_common_plat *dev_pdata, bool enable) + struct regulator_common_plat *plat, bool enable) { int ret;
debug("%s: dev='%s', enable=%d, delay=%d, has_gpio=%d\n", __func__, - dev->name, enable, dev_pdata->startup_delay_us, - dm_gpio_is_valid(&dev_pdata->gpio)); + dev->name, enable, plat->startup_delay_us, + dm_gpio_is_valid(&plat->gpio)); /* Enable GPIO is optional */ - if (!dm_gpio_is_valid(&dev_pdata->gpio)) { + if (!dm_gpio_is_valid(&plat->gpio)) { if (!enable) return -ENOSYS; return 0; }
/* If previously enabled, increase count */ - if (enable && dev_pdata->enable_count > 0) { - dev_pdata->enable_count++; + if (enable && plat->enable_count > 0) { + plat->enable_count++; return -EALREADY; }
if (!enable) { - if (dev_pdata->enable_count > 1) { + if (plat->enable_count > 1) { /* If enabled multiple times, decrease count */ - dev_pdata->enable_count--; + plat->enable_count--; return -EBUSY; - } else if (!dev_pdata->enable_count) { + } else if (!plat->enable_count) { /* If already disabled, do nothing */ return -EALREADY; } }
- ret = dm_gpio_set_value(&dev_pdata->gpio, enable); + ret = dm_gpio_set_value(&plat->gpio, enable); if (ret) { pr_err("Can't set regulator : %s gpio to: %d\n", dev->name, enable); return ret; }
- if (enable && dev_pdata->startup_delay_us) - udelay(dev_pdata->startup_delay_us); + if (enable && plat->startup_delay_us) + udelay(plat->startup_delay_us); debug("%s: done\n", __func__);
- if (!enable && dev_pdata->off_on_delay_us) - udelay(dev_pdata->off_on_delay_us); + if (!enable && plat->off_on_delay_us) + udelay(plat->off_on_delay_us);
if (enable) - dev_pdata->enable_count++; + plat->enable_count++; else - dev_pdata->enable_count--; + plat->enable_count--;
return 0; } diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index 0faab447d099..d4962899d830 100644 --- a/drivers/power/regulator/regulator_common.h +++ b/drivers/power/regulator/regulator_common.h @@ -17,10 +17,10 @@ struct regulator_common_plat { };
int regulator_common_of_to_plat(struct udevice *dev, - struct regulator_common_plat *dev_pdata, const + struct regulator_common_plat *plat, const char *enable_gpio_name); int regulator_common_get_enable(const struct udevice *dev, - struct regulator_common_plat *dev_pdata); + struct regulator_common_plat *plat); /* * Enable or Disable a regulator * @@ -30,7 +30,7 @@ int regulator_common_get_enable(const struct udevice *dev, * and disabled when it reaches 0 coming from 1. * * @dev: regulator device - * @dev_pdata: Platform data + * @plat: Platform data * @enable: bool indicating whether to enable or disable the regulator * @return: * 0 on Success @@ -42,6 +42,6 @@ int regulator_common_get_enable(const struct udevice *dev, * -ve on different error situation */ int regulator_common_set_enable(const struct udevice *dev, - struct regulator_common_plat *dev_pdata, bool enable); + struct regulator_common_plat *plat, bool enable);
#endif /* _REGULATOR_COMMON_H */
Hi Eugen,
On 2023-03-31 11:15, Eugen Hristev wrote:
Some devices share a regulator supply, when the first one will request regulator disable, the second device will have it's supply cut off before graciously shutting down. Hence there will be timeouts and other failed operations. Implement a reference counter mechanism similar with what is done in Linux, to keep track of enable and disable requests, and only disable the regulator when the last of the consumers has requested shutdown.
Signed-off-by: Eugen Hristev eugen.hristev@collabora.com Reviewed-by: Simon Glass sjg@chromium.org
Changes in v4:
- add documentation for error codes
Changes in v3:
- add error return codes
Changes in v2:
- add info in header regarding the function
drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.h | 21 +++++++++++++++++++++ 2 files changed, 43 insertions(+)
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 93d8196b381e..484a4fc31ef7 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -73,6 +73,23 @@ int regulator_common_set_enable(const struct udevice *dev, return 0; }
- /* If previously enabled, increase count */
- if (enable && dev_pdata->enable_count > 0) {
dev_pdata->enable_count++;return -EALREADY;- }
- if (!enable) {
if (dev_pdata->enable_count > 1) {/* If enabled multiple times, decrease count */dev_pdata->enable_count--;return -EBUSY;} else if (!dev_pdata->enable_count) {/* If already disabled, do nothing */return -EALREADY;}- }
- ret = dm_gpio_set_value(&dev_pdata->gpio, enable); if (ret) { pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
@@ -87,5 +104,10 @@ int regulator_common_set_enable(const struct udevice *dev, if (!enable && dev_pdata->off_on_delay_us) udelay(dev_pdata->off_on_delay_us);
- if (enable)
dev_pdata->enable_count++;- else
dev_pdata->enable_count--;- return 0;
} diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index c10492f01675..0faab447d099 100644 --- a/drivers/power/regulator/regulator_common.h +++ b/drivers/power/regulator/regulator_common.h @@ -13,6 +13,7 @@ struct regulator_common_plat { struct gpio_desc gpio; /* GPIO for regulator enable control */ unsigned int startup_delay_us; unsigned int off_on_delay_us;
- unsigned int enable_count;
};
int regulator_common_of_to_plat(struct udevice *dev, @@ -20,6 +21,26 @@ int regulator_common_of_to_plat(struct udevice *dev, char *enable_gpio_name); int regulator_common_get_enable(const struct udevice *dev, struct regulator_common_plat *dev_pdata); +/*
- Enable or Disable a regulator
- This is a reentrant function and subsequent calls that enable will
- increase an internal counter, and disable calls will decrease the counter.
- The actual resource will be enabled when the counter gets to 1 coming from 0,
- and disabled when it reaches 0 coming from 1.
- @dev: regulator device
- @dev_pdata: Platform data
- @enable: bool indicating whether to enable or disable the regulator
- @return:
- 0 on Success
- -EBUSY if the regulator cannot be disabled because it's requested by
another device
- -EALREADY if the regulator has already been enabled or has already been
disabled
With this new return value added this has potential to break IO voltage switching for mmc UHS support. Main pattern for IO voltage switch seem to be: disable regulator, set voltage, enable regulator.
regulator_set_enable_if_allowed should probably be updated to return success on -EALREADY, but that also has the potential to hide regulators enabled at boot not being disabled unless initial enable_count reflects the initial regulator state.
Guessing initial enable_count will match for regulator-boot-on regulators in Rockchip and few other platforms/boards because they the call regulators_enable_boot_on, does not look like all platform will have initial enable_count match regulator initial state.
Regards, Jonas
- -EACCES if there is no possibility to enable/disable the regulator
- -ve on different error situation
- */
int regulator_common_set_enable(const struct udevice *dev, struct regulator_common_plat *dev_pdata, bool enable);
On 4/2/23 13:45, Jonas Karlman wrote:
Hi Eugen,
On 2023-03-31 11:15, Eugen Hristev wrote:
Some devices share a regulator supply, when the first one will request regulator disable, the second device will have it's supply cut off before graciously shutting down. Hence there will be timeouts and other failed operations. Implement a reference counter mechanism similar with what is done in Linux, to keep track of enable and disable requests, and only disable the regulator when the last of the consumers has requested shutdown.
Signed-off-by: Eugen Hristev eugen.hristev@collabora.com Reviewed-by: Simon Glass sjg@chromium.org
Changes in v4:
- add documentation for error codes
Changes in v3:
- add error return codes
Changes in v2:
- add info in header regarding the function
drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.h | 21 +++++++++++++++++++++ 2 files changed, 43 insertions(+)
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 93d8196b381e..484a4fc31ef7 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -73,6 +73,23 @@ int regulator_common_set_enable(const struct udevice *dev, return 0; }
- /* If previously enabled, increase count */
- if (enable && dev_pdata->enable_count > 0) {
dev_pdata->enable_count++;return -EALREADY;- }
- if (!enable) {
if (dev_pdata->enable_count > 1) {/* If enabled multiple times, decrease count */dev_pdata->enable_count--;return -EBUSY;} else if (!dev_pdata->enable_count) {/* If already disabled, do nothing */return -EALREADY;}- }
- ret = dm_gpio_set_value(&dev_pdata->gpio, enable); if (ret) { pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
@@ -87,5 +104,10 @@ int regulator_common_set_enable(const struct udevice *dev, if (!enable && dev_pdata->off_on_delay_us) udelay(dev_pdata->off_on_delay_us);
- if (enable)
dev_pdata->enable_count++;- else
dev_pdata->enable_count--;- return 0; }
diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index c10492f01675..0faab447d099 100644 --- a/drivers/power/regulator/regulator_common.h +++ b/drivers/power/regulator/regulator_common.h @@ -13,6 +13,7 @@ struct regulator_common_plat { struct gpio_desc gpio; /* GPIO for regulator enable control */ unsigned int startup_delay_us; unsigned int off_on_delay_us;
unsigned int enable_count; };
int regulator_common_of_to_plat(struct udevice *dev,
@@ -20,6 +21,26 @@ int regulator_common_of_to_plat(struct udevice *dev, char *enable_gpio_name); int regulator_common_get_enable(const struct udevice *dev, struct regulator_common_plat *dev_pdata); +/*
- Enable or Disable a regulator
- This is a reentrant function and subsequent calls that enable will
- increase an internal counter, and disable calls will decrease the counter.
- The actual resource will be enabled when the counter gets to 1 coming from 0,
- and disabled when it reaches 0 coming from 1.
- @dev: regulator device
- @dev_pdata: Platform data
- @enable: bool indicating whether to enable or disable the regulator
- @return:
- 0 on Success
- -EBUSY if the regulator cannot be disabled because it's requested by
another device
- -EALREADY if the regulator has already been enabled or has already been
disabledWith this new return value added this has potential to break IO voltage switching for mmc UHS support. Main pattern for IO voltage switch seem to be: disable regulator, set voltage, enable regulator.
How can the breakage happen in this case ?
regulator_set_enable_if_allowed should probably be updated to return success on -EALREADY, but that also has the potential to hide regulators enabled at boot not being disabled unless initial enable_count reflects the initial regulator state.
If the regulators enabled at boot are fixed/gpio regulators (which have the enable count), then at boot , the autoset should call the enable_common, thus taking into account the enable_count.
So I think that if I updated regulator_set_enable_if_allowed to return success on -EALREADY it should be fine. Do you have any other scenario in mind ?
Guessing initial enable_count will match for regulator-boot-on regulators in Rockchip and few other platforms/boards because they the call regulators_enable_boot_on, does not look like all platform will have initial enable_count match regulator initial state.
If the other platforms do not call regulator enable, then they will have the enable_count==0, which is expected right ? You refer to specific platforms where the regulator is already enabled from hardware but no code is called to enable, and by looking at enable_count it would appear disabled ? There are such cases ? And those regulators are not initialized at all ?
Regards, Jonas
- -EACCES if there is no possibility to enable/disable the regulator
- -ve on different error situation
- */ int regulator_common_set_enable(const struct udevice *dev, struct regulator_common_plat *dev_pdata, bool enable);
Hi Eugen, On 2023-04-03 11:59, Eugen Hristev wrote:
On 4/2/23 13:45, Jonas Karlman wrote:
Hi Eugen,
On 2023-03-31 11:15, Eugen Hristev wrote:
Some devices share a regulator supply, when the first one will request regulator disable, the second device will have it's supply cut off before graciously shutting down. Hence there will be timeouts and other failed operations. Implement a reference counter mechanism similar with what is done in Linux, to keep track of enable and disable requests, and only disable the regulator when the last of the consumers has requested shutdown.
Signed-off-by: Eugen Hristev eugen.hristev@collabora.com Reviewed-by: Simon Glass sjg@chromium.org
Changes in v4:
- add documentation for error codes
Changes in v3:
- add error return codes
Changes in v2:
- add info in header regarding the function
drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.h | 21 +++++++++++++++++++++ 2 files changed, 43 insertions(+)
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 93d8196b381e..484a4fc31ef7 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -73,6 +73,23 @@ int regulator_common_set_enable(const struct udevice *dev, return 0; }
- /* If previously enabled, increase count */
- if (enable && dev_pdata->enable_count > 0) {
dev_pdata->enable_count++;return -EALREADY;- }
- if (!enable) {
if (dev_pdata->enable_count > 1) {/* If enabled multiple times, decrease count */dev_pdata->enable_count--;return -EBUSY;} else if (!dev_pdata->enable_count) {/* If already disabled, do nothing */return -EALREADY;}- }
- ret = dm_gpio_set_value(&dev_pdata->gpio, enable); if (ret) { pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,
@@ -87,5 +104,10 @@ int regulator_common_set_enable(const struct udevice *dev, if (!enable && dev_pdata->off_on_delay_us) udelay(dev_pdata->off_on_delay_us);
- if (enable)
dev_pdata->enable_count++;- else
dev_pdata->enable_count--;- return 0; }
diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index c10492f01675..0faab447d099 100644 --- a/drivers/power/regulator/regulator_common.h +++ b/drivers/power/regulator/regulator_common.h @@ -13,6 +13,7 @@ struct regulator_common_plat { struct gpio_desc gpio; /* GPIO for regulator enable control */ unsigned int startup_delay_us; unsigned int off_on_delay_us;
unsigned int enable_count; };
int regulator_common_of_to_plat(struct udevice *dev,
@@ -20,6 +21,26 @@ int regulator_common_of_to_plat(struct udevice *dev, char *enable_gpio_name); int regulator_common_get_enable(const struct udevice *dev, struct regulator_common_plat *dev_pdata); +/*
- Enable or Disable a regulator
- This is a reentrant function and subsequent calls that enable will
- increase an internal counter, and disable calls will decrease the counter.
- The actual resource will be enabled when the counter gets to 1 coming from 0,
- and disabled when it reaches 0 coming from 1.
- @dev: regulator device
- @dev_pdata: Platform data
- @enable: bool indicating whether to enable or disable the regulator
- @return:
- 0 on Success
- -EBUSY if the regulator cannot be disabled because it's requested by
another device
- -EALREADY if the regulator has already been enabled or has already been
disabledWith this new return value added this has potential to break IO voltage switching for mmc UHS support. Main pattern for IO voltage switch seem to be: disable regulator, set voltage, enable regulator.
How can the breakage happen in this case ?
This was more of a theoretical issue then anything I have observed.
regulator_set_enable_if_allowed should probably be updated to return success on -EALREADY, but that also has the potential to hide regulators enabled at boot not being disabled unless initial enable_count reflects the initial regulator state.
If the regulators enabled at boot are fixed/gpio regulators (which have the enable count), then at boot , the autoset should call the enable_common, thus taking into account the enable_count.
So I think that if I updated regulator_set_enable_if_allowed to return success on -EALREADY it should be fine. Do you have any other scenario in mind ?
I did more runtime test of this with USB on RK3568 and [1], -EBUSY should possibly also return success.
Testing a "usb start" / "usb stop" cycle report the following -EBUSY error with a shared regulator:
=> usb stop stopping USB.. Host not halted after 16000 microseconds. rockchip_usb2phy_port host-port: PHY: Failed to disable regulator vcc5v0-usb-host-regulator: -16. Host not halted after 16000 microseconds. rockchip_usb2phy_port otg-port: PHY: Failed to disable regulator vcc5v0-usb-host-regulator: -16.
Unsure if this should be reported as error or not.
Guessing initial enable_count will match for regulator-boot-on regulators in Rockchip and few other platforms/boards because they the call regulators_enable_boot_on, does not look like all platform will have initial enable_count match regulator initial state.
If the other platforms do not call regulator enable, then they will have the enable_count==0, which is expected right ? You refer to specific platforms where the regulator is already enabled from hardware but no code is called to enable, and by looking at enable_count it would appear disabled ? There are such cases ? And those regulators are not initialized at all ?
This was more of a theoretical issue then anything I have observed.
There could be a need in the future to initialize the enable_count based on the state of e.g. the gpio at regulator probe time or at first call to set enable/disable if this becomes an issue.
I did notice that there have been some calls to regulator enable/disable missing that result in unbalanced enable_count. E.g. in dw_mmc and rk pcie.
This looks and works good, as long as set enable/disable calls are balanced so,
Tested-by: Jonas Karlman jonas@kwiboo.se Reviewed-by: Jonas Karlman jonas@kwiboo.se
[1] https://github.com/Kwiboo/u-boot-rockchip/commits/rk3568-usb-v1
Regards, Jonas
Regards, Jonas
- -EACCES if there is no possibility to enable/disable the regulator
- -ve on different error situation
- */ int regulator_common_set_enable(const struct udevice *dev, struct regulator_common_plat *dev_pdata, bool enable);
participants (2)
-
Eugen Hristev -
Jonas Karlman