[PATCH v5 1/3] 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 v5: - none 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,v5: - 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 */
On 4/19/23 15:45, Eugen Hristev wrote:
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,v5:
- 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;
dev_pdata->sleep_reg = FAN53555_VSEL1;dev_pdata->vol_reg = FAN53555_VSEL0;
plat->sleep_reg = FAN53555_VSEL1;plat->vol_reg = FAN53555_VSEL0;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];
- 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];
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];
- 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));
- 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);
- 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++;
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 */
Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com
Thanks Patrice
The regulator core can return different codes which are not considered a real error for this function. Return success in such cases.
Signed-off-by: Eugen Hristev eugen.hristev@collabora.com --- Changes in v5: - this is a new patch
drivers/power/regulator/regulator-uclass.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index d608f7c23657..3a6ba69f6d5f 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -197,6 +197,12 @@ int regulator_set_enable_if_allowed(struct udevice *dev, bool enable) ret = regulator_set_enable(dev, enable); if (ret == -ENOSYS || ret == -EACCES) return 0; + /* if we want to disable but it's in use by someone else */ + if (!enable && ret == -EBUSY) + return 0; + /* if it's already enabled/disabled */ + if (ret == -EALREADY) + return 0;
return ret; }
On Thu, 20 Apr 2023 at 01:45, Eugen Hristev eugen.hristev@collabora.com wrote:
The regulator core can return different codes which are not considered a real error for this function. Return success in such cases.
Signed-off-by: Eugen Hristev eugen.hristev@collabora.com
Changes in v5:
- this is a new patch
drivers/power/regulator/regulator-uclass.c | 6 ++++++ 1 file changed, 6 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
On 4/19/23 15:45, Eugen Hristev wrote:
The regulator core can return different codes which are not considered a real error for this function. Return success in such cases.
Signed-off-by: Eugen Hristev eugen.hristev@collabora.com
Changes in v5:
- this is a new patch
drivers/power/regulator/regulator-uclass.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index d608f7c23657..3a6ba69f6d5f 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -197,6 +197,12 @@ int regulator_set_enable_if_allowed(struct udevice *dev, bool enable) ret = regulator_set_enable(dev, enable); if (ret == -ENOSYS || ret == -EACCES) return 0;
/* if we want to disable but it's in use by someone else */
if (!enable && ret == -EBUSY)
return 0;/* if it's already enabled/disabled */
if (ret == -EALREADY)
return 0;return ret;
}
Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com
Thanks Patrice
On 4/19/23 15:45, 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 v5:
- none
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);
Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com
Patrice
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev eugen.hristev@collabora.com 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 v5:
- none
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;}}
Eugen,
Thank you for working on this series!
I wonder if instead of returning a failure you should print an error here but return 0 in order to not break unbalanced regulator calls (like what is done in clk_disable()). I see that you have another patch in this series which handles htis for regulator_set_enable_if_allowed() but that doesn't cover drivers that call regulator_common_set_enable() directly such as drivers/power/regulator/fixed.c and drivers/power/regulator/gpio-regulator.c.
I know there is an unbalanced call currently on imx8mm that this patch causes a failure where it previously did not: u-boot=> usb start && usb tree starting USB... Bus usb@32e40000: Bus usb@32e50000: Error enabling VBUS supply (ret=-114) probe failed, error -114
Best Regards,
Tim
On 4/28/23 02:39, Tim Harvey wrote:
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev eugen.hristev@collabora.com 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 v5:
- none
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;}}Eugen,
Thank you for working on this series!
Hi Tim,
Thank you for testing and having a look on my patches.
I wonder if instead of returning a failure you should print an error here but return 0 in order to not break unbalanced regulator calls
Initially I did that, but Simon forced me to return error as you see now.
(like what is done in clk_disable()). I see that you have another patch in this series which handles htis for regulator_set_enable_if_allowed() but that doesn't cover drivers that call regulator_common_set_enable() directly such as drivers/power/regulator/fixed.c and drivers/power/regulator/gpio-regulator.c.
I believe Jonas (in CC) fixed those with a patch, but at the moment I am lost in providing you a link to it
I know there is an unbalanced call currently on imx8mm that this patch causes a failure where it previously did not: u-boot=> usb start && usb tree starting USB... Bus usb@32e40000: Bus usb@32e50000: Error enabling VBUS supply (ret=-114) probe failed, error -114
That's a good catch. I expected such things would happen if I would return error in those cases, so it might be that this is not the only case. If you are able to test that board, do you wish me to send you a patch that changes the code to using regulator_set_enable_if_allowed() ?
Best Regards,
Tim
On Fri, Apr 28, 2023 at 12:39 AM Eugen Hristev eugen.hristev@collabora.com wrote:
On 4/28/23 02:39, Tim Harvey wrote:
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev eugen.hristev@collabora.com 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 v5:
- none
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;}}Eugen,
Thank you for working on this series!
Hi Tim,
Thank you for testing and having a look on my patches.
I wonder if instead of returning a failure you should print an error here but return 0 in order to not break unbalanced regulator calls
Initially I did that, but Simon forced me to return error as you see now.
Ok, I see that discussion here: https://patchwork.ozlabs.org/project/uboot/patch/20230328130127.20279-1-euge...
(like what is done in clk_disable()). I see that you have another patch in this series which handles htis for regulator_set_enable_if_allowed() but that doesn't cover drivers that call regulator_common_set_enable() directly such as drivers/power/regulator/fixed.c and drivers/power/regulator/gpio-regulator.c.
I believe Jonas (in CC) fixed those with a patch, but at the moment I am lost in providing you a link to it
Are you thinking of "pci: pcie_dw_rockchip: Use regulator_set_enable_if_allowed" https://patchwork.ozlabs.org/project/uboot/patch/20230422181943.889436-3-jon... ?
I added some debug prints to regulator_set_enable and see: starting USB... Bus usb@32e40000: regulator_set_enable regulator-usb-otg1 enable regulator_set_enable regulator-usb-otg1 enable ret=0 Bus usb@32e50000: regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=0 regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=-114 Error enabling VBUS supply (ret=-114) regulator_set_enable regulator-usb-otg2 disable regulator_set_enable regulator-usb-otg2 disable ret=-16 probe failed, error -114
So clearly something is trying to enable regulator-usb-otg2 when it's already enabled but I don't think that should be considered an error and cause a failure.
Simon, is this what you were intending with your feedback?
I know there is an unbalanced call currently on imx8mm that this patch causes a failure where it previously did not: u-boot=> usb start && usb tree starting USB... Bus usb@32e40000: Bus usb@32e50000: Error enabling VBUS supply (ret=-114) probe failed, error -114
That's a good catch. I expected such things would happen if I would return error in those cases, so it might be that this is not the only case. If you are able to test that board, do you wish me to send you a patch that changes the code to using regulator_set_enable_if_allowed() ?
Yes, I can test. Are you thinking changing the calls to regulator_common_set_enable (used in drivers/power/regulator/fixed{gpio-regulator,regulator_common} to a wrapper calling regulator_common_set_enable_if_allowed instead? I think that would just be the same thing as removing the error as no callers of that function would be left.
Your series solves a necessary issue where a regulator_disable call would disable a regulator that was still being used by another consumer but I don't think it should break calls to regulator_enable just because another consumer is also using it.
Best Regards,
Tim
Hi Tim, On 2023-04-28 18:36, Tim Harvey wrote:
On Fri, Apr 28, 2023 at 12:39 AM Eugen Hristev eugen.hristev@collabora.com wrote:
On 4/28/23 02:39, Tim Harvey wrote:
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev eugen.hristev@collabora.com 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 v5:
- none
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;}}Eugen,
Thank you for working on this series!
Hi Tim,
Thank you for testing and having a look on my patches.
I wonder if instead of returning a failure you should print an error here but return 0 in order to not break unbalanced regulator calls
Initially I did that, but Simon forced me to return error as you see now.
Ok, I see that discussion here: https://patchwork.ozlabs.org/project/uboot/patch/20230328130127.20279-1-euge...
(like what is done in clk_disable()). I see that you have another patch in this series which handles htis for regulator_set_enable_if_allowed() but that doesn't cover drivers that call regulator_common_set_enable() directly such as drivers/power/regulator/fixed.c and drivers/power/regulator/gpio-regulator.c.
I believe Jonas (in CC) fixed those with a patch, but at the moment I am lost in providing you a link to it
Are you thinking of "pci: pcie_dw_rockchip: Use regulator_set_enable_if_allowed" https://patchwork.ozlabs.org/project/uboot/patch/20230422181943.889436-3-jon... ?
I added some debug prints to regulator_set_enable and see: starting USB... Bus usb@32e40000: regulator_set_enable regulator-usb-otg1 enable regulator_set_enable regulator-usb-otg1 enable ret=0 Bus usb@32e50000: regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=0 regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=-114 Error enabling VBUS supply (ret=-114) regulator_set_enable regulator-usb-otg2 disable regulator_set_enable regulator-usb-otg2 disable ret=-16 probe failed, error -114
So clearly something is trying to enable regulator-usb-otg2 when it's already enabled but I don't think that should be considered an error and cause a failure.
Simon, is this what you were intending with your feedback?
I know there is an unbalanced call currently on imx8mm that this patch causes a failure where it previously did not: u-boot=> usb start && usb tree starting USB... Bus usb@32e40000: Bus usb@32e50000: Error enabling VBUS supply (ret=-114) probe failed, error -114
That's a good catch. I expected such things would happen if I would return error in those cases, so it might be that this is not the only case. If you are able to test that board, do you wish me to send you a patch that changes the code to using regulator_set_enable_if_allowed() ?
Yes, I can test. Are you thinking changing the calls to regulator_common_set_enable (used in drivers/power/regulator/fixed{gpio-regulator,regulator_common} to a wrapper calling regulator_common_set_enable_if_allowed instead? I think that would just be the same thing as removing the error as no callers of that function would be left.
This is nothing that should be changed in the fixed/gpio/common regulator code. Such change should happen in the drivers that call the regulator_set_enable function. In your case the usb driver that tries to enable/disable the regulator.
Your series solves a necessary issue where a regulator_disable call would disable a regulator that was still being used by another consumer but I don't think it should break calls to regulator_enable just because another consumer is also using it.
With this patch at least fixed/gpio regulators have a basic reference counter and because of this the regulator_set_enable may now return a more detailed errno, "0 on success or -errno val if fails".
The call to regulator_set_enable can be replaced with a call to regulator_set_enable_if_allowed when the caller requires less strict errors, "0 on success or if enabling is not supported or -errno val if fails."
Another option could be to also relax the errors returned by regulator_set_enable, and instead print an error message for the two new errno.
Regards, Jonas
Best Regards,
Tim
On 4/30/23 21:21, Jonas Karlman wrote:
Hi Tim, On 2023-04-28 18:36, Tim Harvey wrote:
On Fri, Apr 28, 2023 at 12:39 AM Eugen Hristev eugen.hristev@collabora.com wrote:
On 4/28/23 02:39, Tim Harvey wrote:
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev eugen.hristev@collabora.com 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 v5:
- none
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;}}Eugen,
Thank you for working on this series!
Hi Tim,
Thank you for testing and having a look on my patches.
I wonder if instead of returning a failure you should print an error here but return 0 in order to not break unbalanced regulator calls
Initially I did that, but Simon forced me to return error as you see now.
Ok, I see that discussion here: https://patchwork.ozlabs.org/project/uboot/patch/20230328130127.20279-1-euge...
(like what is done in clk_disable()). I see that you have another patch in this series which handles htis for regulator_set_enable_if_allowed() but that doesn't cover drivers that call regulator_common_set_enable() directly such as drivers/power/regulator/fixed.c and drivers/power/regulator/gpio-regulator.c.
I believe Jonas (in CC) fixed those with a patch, but at the moment I am lost in providing you a link to it
Are you thinking of "pci: pcie_dw_rockchip: Use regulator_set_enable_if_allowed" https://patchwork.ozlabs.org/project/uboot/patch/20230422181943.889436-3-jon... ?
I added some debug prints to regulator_set_enable and see: starting USB... Bus usb@32e40000: regulator_set_enable regulator-usb-otg1 enable regulator_set_enable regulator-usb-otg1 enable ret=0 Bus usb@32e50000: regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=0 regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=-114 Error enabling VBUS supply (ret=-114) regulator_set_enable regulator-usb-otg2 disable regulator_set_enable regulator-usb-otg2 disable ret=-16 probe failed, error -114
So clearly something is trying to enable regulator-usb-otg2 when it's already enabled but I don't think that should be considered an error and cause a failure.
Simon, is this what you were intending with your feedback?
I know there is an unbalanced call currently on imx8mm that this patch causes a failure where it previously did not: u-boot=> usb start && usb tree starting USB... Bus usb@32e40000: Bus usb@32e50000: Error enabling VBUS supply (ret=-114) probe failed, error -114
That's a good catch. I expected such things would happen if I would return error in those cases, so it might be that this is not the only case. If you are able to test that board, do you wish me to send you a patch that changes the code to using regulator_set_enable_if_allowed() ?
Yes, I can test. Are you thinking changing the calls to regulator_common_set_enable (used in drivers/power/regulator/fixed{gpio-regulator,regulator_common} to a wrapper calling regulator_common_set_enable_if_allowed instead? I think that would just be the same thing as removing the error as no callers of that function would be left.
This is nothing that should be changed in the fixed/gpio/common regulator code. Such change should happen in the drivers that call the regulator_set_enable function. In your case the usb driver that tries to enable/disable the regulator.
Your series solves a necessary issue where a regulator_disable call would disable a regulator that was still being used by another consumer but I don't think it should break calls to regulator_enable just because another consumer is also using it.
With this patch at least fixed/gpio regulators have a basic reference counter and because of this the regulator_set_enable may now return a more detailed errno, "0 on success or -errno val if fails".
The call to regulator_set_enable can be replaced with a call to regulator_set_enable_if_allowed when the caller requires less strict errors, "0 on success or if enabling is not supported or -errno val if fails."
I think that the best option is to replace calls to regulator_set_enable with regulator_set_enable_if_allowed.
Tim, what is the driver that is now broken ? I will try to create a patch and send it your way so you can test ?
Another option could be to also relax the errors returned by regulator_set_enable, and instead print an error message for the two new errno.
Regards, Jonas
Best Regards,
Tim
On Mon, May 1, 2023 at 1:12 AM Eugen Hristev eugen.hristev@collabora.com wrote:
On 4/30/23 21:21, Jonas Karlman wrote:
Hi Tim, On 2023-04-28 18:36, Tim Harvey wrote:
On Fri, Apr 28, 2023 at 12:39 AM Eugen Hristev eugen.hristev@collabora.com wrote:
On 4/28/23 02:39, Tim Harvey wrote:
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev eugen.hristev@collabora.com 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 v5:
- none
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;}}Eugen,
Thank you for working on this series!
Hi Tim,
Thank you for testing and having a look on my patches.
I wonder if instead of returning a failure you should print an error here but return 0 in order to not break unbalanced regulator calls
Initially I did that, but Simon forced me to return error as you see now.
Ok, I see that discussion here: https://patchwork.ozlabs.org/project/uboot/patch/20230328130127.20279-1-euge...
(like what is done in clk_disable()). I see that you have another patch in this series which handles htis for regulator_set_enable_if_allowed() but that doesn't cover drivers that call regulator_common_set_enable() directly such as drivers/power/regulator/fixed.c and drivers/power/regulator/gpio-regulator.c.
I believe Jonas (in CC) fixed those with a patch, but at the moment I am lost in providing you a link to it
Are you thinking of "pci: pcie_dw_rockchip: Use regulator_set_enable_if_allowed" https://patchwork.ozlabs.org/project/uboot/patch/20230422181943.889436-3-jon... ?
I added some debug prints to regulator_set_enable and see: starting USB... Bus usb@32e40000: regulator_set_enable regulator-usb-otg1 enable regulator_set_enable regulator-usb-otg1 enable ret=0 Bus usb@32e50000: regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=0 regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=-114 Error enabling VBUS supply (ret=-114) regulator_set_enable regulator-usb-otg2 disable regulator_set_enable regulator-usb-otg2 disable ret=-16 probe failed, error -114
So clearly something is trying to enable regulator-usb-otg2 when it's already enabled but I don't think that should be considered an error and cause a failure.
Simon, is this what you were intending with your feedback?
I know there is an unbalanced call currently on imx8mm that this patch causes a failure where it previously did not: u-boot=> usb start && usb tree starting USB... Bus usb@32e40000: Bus usb@32e50000: Error enabling VBUS supply (ret=-114) probe failed, error -114
That's a good catch. I expected such things would happen if I would return error in those cases, so it might be that this is not the only case. If you are able to test that board, do you wish me to send you a patch that changes the code to using regulator_set_enable_if_allowed() ?
Yes, I can test. Are you thinking changing the calls to regulator_common_set_enable (used in drivers/power/regulator/fixed{gpio-regulator,regulator_common} to a wrapper calling regulator_common_set_enable_if_allowed instead? I think that would just be the same thing as removing the error as no callers of that function would be left.
This is nothing that should be changed in the fixed/gpio/common regulator code. Such change should happen in the drivers that call the regulator_set_enable function. In your case the usb driver that tries to enable/disable the regulator.
Your series solves a necessary issue where a regulator_disable call would disable a regulator that was still being used by another consumer but I don't think it should break calls to regulator_enable just because another consumer is also using it.
With this patch at least fixed/gpio regulators have a basic reference counter and because of this the regulator_set_enable may now return a more detailed errno, "0 on success or -errno val if fails".
The call to regulator_set_enable can be replaced with a call to regulator_set_enable_if_allowed when the caller requires less strict errors, "0 on success or if enabling is not supported or -errno val if fails."
I think that the best option is to replace calls to regulator_set_enable with regulator_set_enable_if_allowed.
Tim, what is the driver that is now broken ? I will try to create a patch and send it your way so you can test ?
The breakage I saw was from drivers/usb/host/ehci-mx6.c with several calls to enable the vbus_supply. I assume there are many other drivers that would have the same problem as there are just over 100 calls to regulator_set_enable throughout U-Boot. I'm not sure I understand what cases there are where a call to enable a regulator should fail just because another consumer still needs it.
Best Regards,
Tim
Another option could be to also relax the errors returned by regulator_set_enable, and instead print an error message for the two new errno.
Regards, Jonas
Best Regards,
Tim
Hi Eugen,
On Mon, 1 May 2023 at 02:12, Eugen Hristev eugen.hristev@collabora.com wrote:
On 4/30/23 21:21, Jonas Karlman wrote:
Hi Tim, On 2023-04-28 18:36, Tim Harvey wrote:
On Fri, Apr 28, 2023 at 12:39 AM Eugen Hristev eugen.hristev@collabora.com wrote:
On 4/28/23 02:39, Tim Harvey wrote:
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev eugen.hristev@collabora.com 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 v5:
- none
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;}}Eugen,
Thank you for working on this series!
Hi Tim,
Thank you for testing and having a look on my patches.
I wonder if instead of returning a failure you should print an error here but return 0 in order to not break unbalanced regulator calls
Initially I did that, but Simon forced me to return error as you see now.
Ok, I see that discussion here: https://patchwork.ozlabs.org/project/uboot/patch/20230328130127.20279-1-euge...
(like what is done in clk_disable()). I see that you have another patch in this series which handles htis for regulator_set_enable_if_allowed() but that doesn't cover drivers that call regulator_common_set_enable() directly such as drivers/power/regulator/fixed.c and drivers/power/regulator/gpio-regulator.c.
I believe Jonas (in CC) fixed those with a patch, but at the moment I am lost in providing you a link to it
Are you thinking of "pci: pcie_dw_rockchip: Use regulator_set_enable_if_allowed" https://patchwork.ozlabs.org/project/uboot/patch/20230422181943.889436-3-jon... ?
I added some debug prints to regulator_set_enable and see: starting USB... Bus usb@32e40000: regulator_set_enable regulator-usb-otg1 enable regulator_set_enable regulator-usb-otg1 enable ret=0 Bus usb@32e50000: regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=0 regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=-114 Error enabling VBUS supply (ret=-114) regulator_set_enable regulator-usb-otg2 disable regulator_set_enable regulator-usb-otg2 disable ret=-16 probe failed, error -114
So clearly something is trying to enable regulator-usb-otg2 when it's already enabled but I don't think that should be considered an error and cause a failure.
Simon, is this what you were intending with your feedback?
I know there is an unbalanced call currently on imx8mm that this patch causes a failure where it previously did not: u-boot=> usb start && usb tree starting USB... Bus usb@32e40000: Bus usb@32e50000: Error enabling VBUS supply (ret=-114) probe failed, error -114
That's a good catch. I expected such things would happen if I would return error in those cases, so it might be that this is not the only case. If you are able to test that board, do you wish me to send you a patch that changes the code to using regulator_set_enable_if_allowed() ?
Yes, I can test. Are you thinking changing the calls to regulator_common_set_enable (used in drivers/power/regulator/fixed{gpio-regulator,regulator_common} to a wrapper calling regulator_common_set_enable_if_allowed instead? I think that would just be the same thing as removing the error as no callers of that function would be left.
This is nothing that should be changed in the fixed/gpio/common regulator code. Such change should happen in the drivers that call the regulator_set_enable function. In your case the usb driver that tries to enable/disable the regulator.
Your series solves a necessary issue where a regulator_disable call would disable a regulator that was still being used by another consumer but I don't think it should break calls to regulator_enable just because another consumer is also using it.
With this patch at least fixed/gpio regulators have a basic reference counter and because of this the regulator_set_enable may now return a more detailed errno, "0 on success or -errno val if fails".
The call to regulator_set_enable can be replaced with a call to regulator_set_enable_if_allowed when the caller requires less strict errors, "0 on success or if enabling is not supported or -errno val if fails."
I think that the best option is to replace calls to regulator_set_enable with regulator_set_enable_if_allowed.
That sounds good. Or even regulator_set_enable_maybe() since it is shorter and the question is not whether it is allowed, but whether we want to for other reasons (i.e other device's desires). I can imagine having quite a few 'maybe' functions about the place eventually.
Another option would be to make it more explicit, like regulator_inc_set_enable()/ dec_set_enable(), but that seems a bit wordy and confusing.
Tim, what is the driver that is now broken ? I will try to create a patch and send it your way so you can test ?
Another option could be to also relax the errors returned by regulator_set_enable, and instead print an error message for the two new errno.
Regards, Jonas
Best Regards,
Tim
Regards, Simon
Hi Eugen,
On Fri, 28 Apr 2023 at 01:39, Eugen Hristev eugen.hristev@collabora.com wrote:
On 4/28/23 02:39, Tim Harvey wrote:
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev eugen.hristev@collabora.com 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 v5:
- none
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;}}Eugen,
Thank you for working on this series!
Hi Tim,
Thank you for testing and having a look on my patches.
I wonder if instead of returning a failure you should print an error here but return 0 in order to not break unbalanced regulator calls
Initially I did that, but Simon forced me to return error as you see now.
If you want to have a special function that consumes errors, you can add one, e.g. that wraps the one that does report errors. My objection is when something goes wrong and it is silently ignored, since it tends to make problems build up, people add work-arounds, etc.
(like what is done in clk_disable()). I see that you have another patch in this series which handles htis for regulator_set_enable_if_allowed() but that doesn't cover drivers that call regulator_common_set_enable() directly such as drivers/power/regulator/fixed.c and drivers/power/regulator/gpio-regulator.c.
I believe Jonas (in CC) fixed those with a patch, but at the moment I am lost in providing you a link to it
I know there is an unbalanced call currently on imx8mm that this patch causes a failure where it previously did not: u-boot=> usb start && usb tree starting USB... Bus usb@32e40000: Bus usb@32e50000: Error enabling VBUS supply (ret=-114) probe failed, error -114
That's a good catch. I expected such things would happen if I would return error in those cases, so it might be that this is not the only case. If you are able to test that board, do you wish me to send you a patch that changes the code to using regulator_set_enable_if_allowed() ?
Regards, Simon
participants (5)
-
Eugen Hristev -
Jonas Karlman -
Patrice CHOTARD -
Simon Glass -
Tim Harvey