[PATCH v2 0/7] General regulator and pmic uclass improvements
This patchset derives from discussion of TPS65913 bringup and aims to cycle all regulator management inside uclass removing need of any board calls for regulators.
My hw setup is Tegra 3 LG Optimus Vu P895 (PMIC is MAX77663) with native spl u-boot build.
power: regulator: expand basic reference counter onto all uclass Commit is basically expansion of 4fcba5d ("regulator: implement basic reference counter") onto all regulators. - my testing on hw has shown no issues so far with both pmic regulators and fixed regulators. Counting works as expected, dm test is set in test of regulator_set_enable_if_allowed.
power: regulator-uclass: perform regulator setup inside uclass All self-sufficient regulators are set to probe after bind to perform initial setup later in post-probe with pdata from device tree. - self-sufficient regulator is one that is not dependent on any other device (this category does not include pmic regulators for example). I have tested fixed regulators and got correct probing and setup according to device tree. DM test is set by checking regulators data without pre-configuring them manually
power: pmic-uclass: probe every child on pmic_post_probe All non self-sufficient regulators must be probed after main device is probed (in this case it is pmic_post_probe). In all other aspects pmic regulators behave same. - tested with MAX77663 ldo and sd regulators, no errors or inconsistencies were tracked, regulator props (boot-on, always-on etc) and consumer calls work as expected. DM test is set by checking regulators data without pre- configuring them manually just after pmic probe.
power: regulator-uclass: remove all deprecated API use This is where everything gets tricky. All board dedicated API of regulators has to be removed. System presented above should cover all regulators setup but non the less this should be disscussed with maintainers and tested on affected boards. This commit removes and cleans most of those API traces.
--- Changes from v1: - adapted description of regulator_set_enable - remove uc_pdata->enable_count from post_probe - added tests from counter and regulators post_probe ---
Svyatoslav Ryhel (7): power: regulator: expand basic reference counter onto all uclass test: dm: regulator: test counter in set_enable_if_allowed test power: regulator-uclass: perform regulator setup inside uclass test: dm: regulator: provide test of auto setup power: pmic-uclass: probe every child on pmic_post_probe test: dm: pmic: provide test of child autosetup power: regulator-uclass: remove all deprecated API use
arch/arm/mach-rockchip/board.c | 8 - arch/arm/mach-rockchip/rk3399/rk3399.c | 10 - board/Marvell/octeontx2_cn913x/board.c | 5 - .../amlogic/odroid-go-ultra/odroid-go-ultra.c | 2 - board/dhelectronics/dh_stm32mp1/board.c | 2 - board/engicam/stm32mp1/stm32mp1.c | 3 - board/google/veyron/veyron.c | 6 - board/samsung/common/exynos5-dt.c | 4 - board/samsung/odroid/odroid.c | 10 - board/st/stm32mp1/stm32mp1.c | 9 - drivers/power/pmic/pmic-uclass.c | 10 + drivers/power/regulator/regulator-uclass.c | 253 +++++++----------- drivers/power/regulator/regulator_common.c | 22 -- drivers/power/regulator/regulator_common.h | 21 -- drivers/video/bridge/ps862x.c | 12 +- drivers/video/rockchip/rk_vop.c | 6 +- include/power/regulator.h | 121 +-------- include/power/sandbox_pmic.h | 2 +- test/dm/pmic.c | 34 +++ test/dm/regulator.c | 148 +++------- 20 files changed, 204 insertions(+), 484 deletions(-)
Commit is based on 4fcba5d ("regulator: implement basic reference counter") but expands the idea to all regulators instead of just fixed/gpio regulators.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com --- drivers/power/regulator/regulator-uclass.c | 41 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.c | 22 ------------ drivers/power/regulator/regulator_common.h | 21 ----------- include/power/regulator.h | 2 ++ 4 files changed, 43 insertions(+), 43 deletions(-)
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index 3a6ba69f6d..fc7a4631b4 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -159,6 +159,25 @@ int regulator_get_enable(struct udevice *dev) return ops->get_enable(dev); }
+/* + * 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 + * @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_set_enable(struct udevice *dev, bool enable) { const struct dm_regulator_ops *ops = dev_get_driver_ops(dev); @@ -172,6 +191,23 @@ int regulator_set_enable(struct udevice *dev, bool enable) if (!enable && uc_pdata->always_on) return -EACCES;
+ /* If previously enabled, increase count */ + if (enable && uc_pdata->enable_count > 0) { + uc_pdata->enable_count++; + return -EALREADY; + } + + if (!enable) { + if (uc_pdata->enable_count > 1) { + /* If enabled multiple times, decrease count */ + uc_pdata->enable_count--; + return -EBUSY; + } else if (!uc_pdata->enable_count) { + /* If already disabled, do nothing */ + return -EALREADY; + } + } + if (uc_pdata->ramp_delay) old_enable = regulator_get_enable(dev);
@@ -187,6 +223,11 @@ int regulator_set_enable(struct udevice *dev, bool enable) } }
+ if (enable) + uc_pdata->enable_count++; + else + uc_pdata->enable_count--; + return ret; }
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index e26f5ebec3..d88bc6f6de 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -72,23 +72,6 @@ int regulator_common_set_enable(const struct udevice *dev, return 0; }
- /* If previously enabled, increase count */ - if (enable && plat->enable_count > 0) { - plat->enable_count++; - return -EALREADY; - } - - if (!enable) { - if (plat->enable_count > 1) { - /* If enabled multiple times, decrease count */ - plat->enable_count--; - return -EBUSY; - } else if (!plat->enable_count) { - /* If already disabled, do nothing */ - return -EALREADY; - } - } - ret = dm_gpio_set_value(&plat->gpio, enable); if (ret) { pr_err("Can't set regulator : %s gpio to: %d\n", dev->name, @@ -103,10 +86,5 @@ int regulator_common_set_enable(const struct udevice *dev, if (!enable && plat->off_on_delay_us) udelay(plat->off_on_delay_us);
- if (enable) - plat->enable_count++; - else - plat->enable_count--; - return 0; } diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index d4962899d8..15f1fa4c93 100644 --- a/drivers/power/regulator/regulator_common.h +++ b/drivers/power/regulator/regulator_common.h @@ -13,7 +13,6 @@ 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, @@ -21,26 +20,6 @@ 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 *plat); -/* - * 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 - * @plat: 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 *plat, bool enable);
diff --git a/include/power/regulator.h b/include/power/regulator.h index ff1bfc2435..727776a8cf 100644 --- a/include/power/regulator.h +++ b/include/power/regulator.h @@ -158,6 +158,7 @@ enum regulator_flag { * @name** - fdt regulator name - should be taken from the device tree * ctrl_reg: - Control register offset used to enable/disable regulator * volt_reg: - register offset for writing voltage vsel values + * enable_count - counter of enable calls for this regulator * * Note: * * - set automatically on device probe by the uclass's '.pre_probe' method. @@ -184,6 +185,7 @@ struct dm_regulator_uclass_plat { u8 volt_reg; bool suspend_on; u32 suspend_uV; + u32 enable_count; };
/* Regulator device operations */
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Commit is based on 4fcba5d ("regulator: implement basic reference counter") but expands the idea to all regulators instead of just fixed/gpio regulators.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 41 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.c | 22 ------------ drivers/power/regulator/regulator_common.h | 21 ----------- include/power/regulator.h | 2 ++ 4 files changed, 43 insertions(+), 43 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
nit below
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index 3a6ba69f6d..fc7a4631b4 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -159,6 +159,25 @@ int regulator_get_enable(struct udevice *dev) return ops->get_enable(dev); }
+/*
- 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
- @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_set_enable(struct udevice *dev, bool enable) { const struct dm_regulator_ops *ops = dev_get_driver_ops(dev); @@ -172,6 +191,23 @@ int regulator_set_enable(struct udevice *dev, bool enable) if (!enable && uc_pdata->always_on) return -EACCES;
/* If previously enabled, increase count */if (enable && uc_pdata->enable_count > 0) {uc_pdata->enable_count++;return -EALREADY;}if (!enable) {if (uc_pdata->enable_count > 1) {/* If enabled multiple times, decrease count */uc_pdata->enable_count--;return -EBUSY;} else if (!uc_pdata->enable_count) {/* If already disabled, do nothing */return -EALREADY;}}if (uc_pdata->ramp_delay) old_enable = regulator_get_enable(dev);@@ -187,6 +223,11 @@ int regulator_set_enable(struct udevice *dev, bool enable) } }
if (enable)uc_pdata->enable_count++;elseuc_pdata->enable_count--;return ret;}
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index e26f5ebec3..d88bc6f6de 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -72,23 +72,6 @@ int regulator_common_set_enable(const struct udevice *dev, return 0; }
/* If previously enabled, increase count */if (enable && plat->enable_count > 0) {plat->enable_count++;return -EALREADY;}if (!enable) {if (plat->enable_count > 1) {/* If enabled multiple times, decrease count */plat->enable_count--;return -EBUSY;} else if (!plat->enable_count) {/* If already disabled, do nothing */return -EALREADY;}}ret = dm_gpio_set_value(&plat->gpio, enable); if (ret) { pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,@@ -103,10 +86,5 @@ int regulator_common_set_enable(const struct udevice *dev, if (!enable && plat->off_on_delay_us) udelay(plat->off_on_delay_us);
if (enable)plat->enable_count++;elseplat->enable_count--;return 0;} diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index d4962899d8..15f1fa4c93 100644 --- a/drivers/power/regulator/regulator_common.h +++ b/drivers/power/regulator/regulator_common.h @@ -13,7 +13,6 @@ 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, @@ -21,26 +20,6 @@ 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 *plat); -/*
- 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
- @plat: 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 *plat, bool enable);
diff --git a/include/power/regulator.h b/include/power/regulator.h index ff1bfc2435..727776a8cf 100644 --- a/include/power/regulator.h +++ b/include/power/regulator.h @@ -158,6 +158,7 @@ enum regulator_flag {
- @name** - fdt regulator name - should be taken from the device tree
- ctrl_reg: - Control register offset used to enable/disable regulator
- volt_reg: - register offset for writing voltage vsel values
- enable_count - counter of enable calls for this regulator
- Note:
- set automatically on device probe by the uclass's '.pre_probe' method.
@@ -184,6 +185,7 @@ struct dm_regulator_uclass_plat { u8 volt_reg; bool suspend_on; u32 suspend_uV;
u32 enable_count;
Please add a comment for this
};
/* Regulator device operations */
2.39.2
Regards, Simon
чт, 20 лип. 2023 р. о 22:43 Simon Glass sjg@chromium.org пише:
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Commit is based on 4fcba5d ("regulator: implement basic reference counter") but expands the idea to all regulators instead of just fixed/gpio regulators.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 41 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.c | 22 ------------ drivers/power/regulator/regulator_common.h | 21 ----------- include/power/regulator.h | 2 ++ 4 files changed, 43 insertions(+), 43 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
nit below
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index 3a6ba69f6d..fc7a4631b4 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -159,6 +159,25 @@ int regulator_get_enable(struct udevice *dev) return ops->get_enable(dev); }
+/*
- 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
- @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_set_enable(struct udevice *dev, bool enable) { const struct dm_regulator_ops *ops = dev_get_driver_ops(dev); @@ -172,6 +191,23 @@ int regulator_set_enable(struct udevice *dev, bool enable) if (!enable && uc_pdata->always_on) return -EACCES;
/* If previously enabled, increase count */if (enable && uc_pdata->enable_count > 0) {uc_pdata->enable_count++;return -EALREADY;}if (!enable) {if (uc_pdata->enable_count > 1) {/* If enabled multiple times, decrease count */uc_pdata->enable_count--;return -EBUSY;} else if (!uc_pdata->enable_count) {/* If already disabled, do nothing */return -EALREADY;}}if (uc_pdata->ramp_delay) old_enable = regulator_get_enable(dev);@@ -187,6 +223,11 @@ int regulator_set_enable(struct udevice *dev, bool enable) } }
if (enable)uc_pdata->enable_count++;elseuc_pdata->enable_count--;return ret;}
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index e26f5ebec3..d88bc6f6de 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -72,23 +72,6 @@ int regulator_common_set_enable(const struct udevice *dev, return 0; }
/* If previously enabled, increase count */if (enable && plat->enable_count > 0) {plat->enable_count++;return -EALREADY;}if (!enable) {if (plat->enable_count > 1) {/* If enabled multiple times, decrease count */plat->enable_count--;return -EBUSY;} else if (!plat->enable_count) {/* If already disabled, do nothing */return -EALREADY;}}ret = dm_gpio_set_value(&plat->gpio, enable); if (ret) { pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,@@ -103,10 +86,5 @@ int regulator_common_set_enable(const struct udevice *dev, if (!enable && plat->off_on_delay_us) udelay(plat->off_on_delay_us);
if (enable)plat->enable_count++;elseplat->enable_count--;return 0;} diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index d4962899d8..15f1fa4c93 100644 --- a/drivers/power/regulator/regulator_common.h +++ b/drivers/power/regulator/regulator_common.h @@ -13,7 +13,6 @@ 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, @@ -21,26 +20,6 @@ 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 *plat); -/*
- 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
- @plat: 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 *plat, bool enable);
diff --git a/include/power/regulator.h b/include/power/regulator.h index ff1bfc2435..727776a8cf 100644 --- a/include/power/regulator.h +++ b/include/power/regulator.h @@ -158,6 +158,7 @@ enum regulator_flag {
- @name** - fdt regulator name - should be taken from the device tree
- ctrl_reg: - Control register offset used to enable/disable regulator
- volt_reg: - register offset for writing voltage vsel values
- enable_count - counter of enable calls for this regulator
- Note:
- set automatically on device probe by the uclass's '.pre_probe' method.
@@ -184,6 +185,7 @@ struct dm_regulator_uclass_plat { u8 volt_reg; bool suspend_on; u32 suspend_uV;
u32 enable_count;Please add a comment for this
Description included in struct description here: + * enable_count - counter of enable calls for this regulator
};
/* Regulator device operations */
2.39.2
Regards, Simon
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 23:23, Svyatoslav Ryhel clamor95@gmail.com wrote:
чт, 20 лип. 2023 р. о 22:43 Simon Glass sjg@chromium.org пише:
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Commit is based on 4fcba5d ("regulator: implement basic reference counter") but expands the idea to all regulators instead of just fixed/gpio regulators.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 41 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.c | 22 ------------ drivers/power/regulator/regulator_common.h | 21 ----------- include/power/regulator.h | 2 ++ 4 files changed, 43 insertions(+), 43 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
nit below
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index 3a6ba69f6d..fc7a4631b4 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -159,6 +159,25 @@ int regulator_get_enable(struct udevice *dev) return ops->get_enable(dev); }
+/*
- 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
- @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_set_enable(struct udevice *dev, bool enable) { const struct dm_regulator_ops *ops = dev_get_driver_ops(dev); @@ -172,6 +191,23 @@ int regulator_set_enable(struct udevice *dev, bool enable) if (!enable && uc_pdata->always_on) return -EACCES;
/* If previously enabled, increase count */if (enable && uc_pdata->enable_count > 0) {uc_pdata->enable_count++;return -EALREADY;}if (!enable) {if (uc_pdata->enable_count > 1) {/* If enabled multiple times, decrease count */uc_pdata->enable_count--;return -EBUSY;} else if (!uc_pdata->enable_count) {/* If already disabled, do nothing */return -EALREADY;}}if (uc_pdata->ramp_delay) old_enable = regulator_get_enable(dev);@@ -187,6 +223,11 @@ int regulator_set_enable(struct udevice *dev, bool enable) } }
if (enable)uc_pdata->enable_count++;elseuc_pdata->enable_count--;return ret;}
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index e26f5ebec3..d88bc6f6de 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -72,23 +72,6 @@ int regulator_common_set_enable(const struct udevice *dev, return 0; }
/* If previously enabled, increase count */if (enable && plat->enable_count > 0) {plat->enable_count++;return -EALREADY;}if (!enable) {if (plat->enable_count > 1) {/* If enabled multiple times, decrease count */plat->enable_count--;return -EBUSY;} else if (!plat->enable_count) {/* If already disabled, do nothing */return -EALREADY;}}ret = dm_gpio_set_value(&plat->gpio, enable); if (ret) { pr_err("Can't set regulator : %s gpio to: %d\n", dev->name,@@ -103,10 +86,5 @@ int regulator_common_set_enable(const struct udevice *dev, if (!enable && plat->off_on_delay_us) udelay(plat->off_on_delay_us);
if (enable)plat->enable_count++;elseplat->enable_count--;return 0;} diff --git a/drivers/power/regulator/regulator_common.h b/drivers/power/regulator/regulator_common.h index d4962899d8..15f1fa4c93 100644 --- a/drivers/power/regulator/regulator_common.h +++ b/drivers/power/regulator/regulator_common.h @@ -13,7 +13,6 @@ 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, @@ -21,26 +20,6 @@ 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 *plat); -/*
- 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
- @plat: 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 *plat, bool enable);
diff --git a/include/power/regulator.h b/include/power/regulator.h index ff1bfc2435..727776a8cf 100644 --- a/include/power/regulator.h +++ b/include/power/regulator.h @@ -158,6 +158,7 @@ enum regulator_flag {
- @name** - fdt regulator name - should be taken from the device tree
- ctrl_reg: - Control register offset used to enable/disable regulator
- volt_reg: - register offset for writing voltage vsel values
- enable_count - counter of enable calls for this regulator
- Note:
- set automatically on device probe by the uclass's '.pre_probe' method.
@@ -184,6 +185,7 @@ struct dm_regulator_uclass_plat { u8 volt_reg; bool suspend_on; u32 suspend_uV;
u32 enable_count;Please add a comment for this
Description included in struct description here:
- enable_count - counter of enable calls for this regulator
OK thanks, I wonder why it says it is part of 'enum regulator_flag' ?
Regards, Simon
Emulate use of the regulator by a few consumers with balanced on/off calls.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com --- test/dm/regulator.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/test/dm/regulator.c b/test/dm/regulator.c index 86f4862d9d..9b503e4c59 100644 --- a/test/dm/regulator.c +++ b/test/dm/regulator.c @@ -194,6 +194,25 @@ int dm_test_power_regulator_set_enable_if_allowed(struct unit_test_state *uts) ut_assertok(regulator_set_enable_if_allowed(dev, val_set)); ut_asserteq(regulator_get_enable(dev), !val_set);
+ /* Set the Enable of LDO1 - default is disabled */ + platname = regulator_names[LDO1][PLATNAME]; + ut_assertok(regulator_autoset_by_name(platname, &dev_autoset)); + ut_assertok(regulator_get_by_platname(platname, &dev)); + + /* Get the Enable state of LDO1 and compare it with the requested one */ + ut_assertok(regulator_set_enable_if_allowed(dev, true)); + ut_asserteq(regulator_get_enable(dev), true); + + /* Emulate second consumer */ + ut_assertok(regulator_set_enable_if_allowed(dev, true)); + ut_asserteq(regulator_get_enable(dev), true); + + /* Emulate one of counsumers disable call */ + ut_assertok(regulator_set_enable_if_allowed(dev, false)); + + /* Regulator should still be on since counter indicates one consumer active */ + ut_asserteq(regulator_get_enable(dev), true); + return 0; } DM_TEST(dm_test_power_regulator_set_enable_if_allowed, UT_TESTF_SCAN_FDT);
Regulators initial setup was previously dependent on board call. To move from this behaviour were introduced two steps. First is that all individual regulators will be probed just after binding which ensures that regulator pdata is filled and second is setting up regulator in post probe which enseres that correct regulator state according to pdata is reached.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com --- drivers/power/regulator/regulator-uclass.c | 212 ++++++--------------- include/power/regulator.h | 119 ------------ 2 files changed, 62 insertions(+), 269 deletions(-)
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index fc7a4631b4..32f4e64ca4 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -327,112 +327,6 @@ int device_get_supply_regulator(struct udevice *dev, const char *supply_name, supply_name, devp); }
-int regulator_autoset(struct udevice *dev) -{ - struct dm_regulator_uclass_plat *uc_pdata; - int ret = 0; - - uc_pdata = dev_get_uclass_plat(dev); - - ret = regulator_set_suspend_enable(dev, uc_pdata->suspend_on); - if (ret == -ENOSYS) - ret = 0; - - if (!ret && uc_pdata->suspend_on) { - ret = regulator_set_suspend_value(dev, uc_pdata->suspend_uV); - if (ret == -ENOSYS) - ret = 0; - - if (ret) - return ret; - } - - if (!uc_pdata->always_on && !uc_pdata->boot_on) - return -EMEDIUMTYPE; - - if (uc_pdata->type == REGULATOR_TYPE_FIXED) - return regulator_set_enable(dev, true); - - if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV) - ret = regulator_set_value(dev, uc_pdata->min_uV); - if (uc_pdata->init_uV > 0) - ret = regulator_set_value(dev, uc_pdata->init_uV); - if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA)) - ret = regulator_set_current(dev, uc_pdata->min_uA); - - if (!ret) - ret = regulator_set_enable(dev, true); - - return ret; -} - -int regulator_unset(struct udevice *dev) -{ - struct dm_regulator_uclass_plat *uc_pdata; - - uc_pdata = dev_get_uclass_plat(dev); - if (uc_pdata && uc_pdata->force_off) - return regulator_set_enable(dev, false); - - return -EMEDIUMTYPE; -} - -static void regulator_show(struct udevice *dev, int ret) -{ - struct dm_regulator_uclass_plat *uc_pdata; - - uc_pdata = dev_get_uclass_plat(dev); - - printf("%s@%s: ", dev->name, uc_pdata->name); - if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV) - printf("set %d uV", uc_pdata->min_uV); - if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA) - printf("; set %d uA", uc_pdata->min_uA); - printf("; enabling"); - if (ret) - printf(" (ret: %d)", ret); - printf("\n"); -} - -int regulator_autoset_by_name(const char *platname, struct udevice **devp) -{ - struct udevice *dev; - int ret; - - ret = regulator_get_by_platname(platname, &dev); - if (devp) - *devp = dev; - if (ret) { - debug("Can get the regulator: %s (err=%d)\n", platname, ret); - return ret; - } - - return regulator_autoset(dev); -} - -int regulator_list_autoset(const char *list_platname[], - struct udevice *list_devp[], - bool verbose) -{ - struct udevice *dev; - int error = 0, i = 0, ret; - - while (list_platname[i]) { - ret = regulator_autoset_by_name(list_platname[i], &dev); - if (ret != -EMEDIUMTYPE && verbose) - regulator_show(dev, ret); - if (ret & !error) - error = ret; - - if (list_devp) - list_devp[i] = dev; - - i++; - } - - return error; -} - static bool regulator_name_is_unique(struct udevice *check_dev, const char *check_name) { @@ -463,6 +357,7 @@ static int regulator_post_bind(struct udevice *dev) { struct dm_regulator_uclass_plat *uc_pdata; const char *property = "regulator-name"; + int ret;
uc_pdata = dev_get_uclass_plat(dev);
@@ -476,13 +371,28 @@ static int regulator_post_bind(struct udevice *dev) return -EINVAL; }
- if (regulator_name_is_unique(dev, uc_pdata->name)) - return 0; + ret = regulator_name_is_unique(dev, uc_pdata->name); + if (!ret) { + debug("'%s' of dev: '%s', has nonunique value: '%s\n", + property, dev->name, uc_pdata->name); + return -EINVAL; + }
- debug("'%s' of dev: '%s', has nonunique value: '%s\n", - property, dev->name, uc_pdata->name); + switch (device_get_uclass_id(dev->parent)) { + /* In case class can have regulator child add it here */ + case UCLASS_PMIC: + break; + + default: + /* + * Trigger probe() to configure default + * regulators state during startup. + */ + dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND); + break; + }
- return -EINVAL; + return 0; }
static int regulator_pre_probe(struct udevice *dev) @@ -536,55 +446,56 @@ static int regulator_pre_probe(struct udevice *dev) return 0; }
-int regulators_enable_boot_on(bool verbose) +static int regulator_post_probe(struct udevice *dev) { - struct udevice *dev; - struct uclass *uc; + struct dm_regulator_uclass_plat *uc_pdata = + dev_get_uclass_plat(dev); int ret;
- ret = uclass_get(UCLASS_REGULATOR, &uc); - if (ret) - return ret; - for (uclass_first_device(UCLASS_REGULATOR, &dev); - dev; - uclass_next_device(&dev)) { - ret = regulator_autoset(dev); - if (ret == -EMEDIUMTYPE) { - ret = 0; - continue; - } - if (verbose) - regulator_show(dev, ret); - if (ret == -ENOSYS) - ret = 0; - } + /* + * Disable is performed in case regulator was somehow + * active, for example it is active by PMIC design etc. + */ + uc_pdata->enable_count = 1; + regulator_set_enable(dev, false);
- return ret; -} + /* + * Always-on regulator can not be disabled so zero out + * enable_count in case regulator has this property. + */ + uc_pdata->enable_count = 0;
-int regulators_enable_boot_off(bool verbose) -{ - struct udevice *dev; - struct uclass *uc; - int ret; + if (uc_pdata->force_off) + return 0;
- ret = uclass_get(UCLASS_REGULATOR, &uc); - if (ret) - return ret; - for (uclass_first_device(UCLASS_REGULATOR, &dev); - dev; - uclass_next_device(&dev)) { - ret = regulator_unset(dev); - if (ret == -EMEDIUMTYPE) { - ret = 0; - continue; - } - if (verbose) - regulator_show(dev, ret); + ret = regulator_set_suspend_enable(dev, uc_pdata->suspend_on); + if (ret == -ENOSYS) + ret = 0; + + if (!ret && uc_pdata->suspend_on) { + ret = regulator_set_suspend_value(dev, uc_pdata->suspend_uV); if (ret == -ENOSYS) ret = 0; + + if (ret) + return ret; + } + + if (uc_pdata->type != REGULATOR_TYPE_FIXED) { + if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV) + ret = regulator_set_value(dev, uc_pdata->min_uV); + if (uc_pdata->init_uV > 0) + ret = regulator_set_value(dev, uc_pdata->init_uV); + if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA)) + ret = regulator_set_current(dev, uc_pdata->min_uA); }
+ if (!uc_pdata->always_on && !uc_pdata->boot_on) + return 0; + + if (!ret) + ret = regulator_set_enable(dev, true); + return ret; }
@@ -593,5 +504,6 @@ UCLASS_DRIVER(regulator) = { .name = "regulator", .post_bind = regulator_post_bind, .pre_probe = regulator_pre_probe, + .post_probe = regulator_post_probe, .per_device_plat_auto = sizeof(struct dm_regulator_uclass_plat), }; diff --git a/include/power/regulator.h b/include/power/regulator.h index 727776a8cf..e58e2dee16 100644 --- a/include/power/regulator.h +++ b/include/power/regulator.h @@ -413,104 +413,6 @@ int regulator_get_mode(struct udevice *dev); */ int regulator_set_mode(struct udevice *dev, int mode_id);
-/** - * regulators_enable_boot_on() - enable regulators needed for boot - * - * This enables all regulators which are marked to be on at boot time. This - * only works for regulators which don't have a range for voltage/current, - * since in that case it is not possible to know which value to use. - * - * This effectively calls regulator_autoset() for every regulator. - */ -int regulators_enable_boot_on(bool verbose); - -/** - * regulators_enable_boot_off() - disable regulators needed for boot - * - * This disables all regulators which are marked to be off at boot time. - * - * This effectively calls regulator_unset() for every regulator. - */ -int regulators_enable_boot_off(bool verbose); - -/** - * regulator_autoset: setup the voltage/current on a regulator - * - * The setup depends on constraints found in device's uclass's platform data - * (struct dm_regulator_uclass_plat): - * - * - Enable - will set - if any of: 'always_on' or 'boot_on' is set to true, - * or if both are unset, then the function returns - * - Voltage value - will set - if '.min_uV' and '.max_uV' values are equal - * - Current limit - will set - if '.min_uA' and '.max_uA' values are equal - * - * The function returns on the first-encountered error. - * - * @platname - expected string for dm_regulator_uclass_plat .name field - * @devp - returned pointer to the regulator device - if non-NULL passed - * @return: 0 on success or negative value of errno. - */ -int regulator_autoset(struct udevice *dev); - -/** - * regulator_unset: turn off a regulator - * - * The setup depends on constraints found in device's uclass's platform data - * (struct dm_regulator_uclass_platdata): - * - * - Disable - will set - if 'force_off' is set to true, - * - * The function returns on the first-encountered error. - */ -int regulator_unset(struct udevice *dev); - -/** - * regulator_autoset_by_name: setup the regulator given by its uclass's - * platform data name field. The setup depends on constraints found in device's - * uclass's platform data (struct dm_regulator_uclass_plat): - * - Enable - will set - if any of: 'always_on' or 'boot_on' is set to true, - * or if both are unset, then the function returns - * - Voltage value - will set - if '.min_uV' and '.max_uV' values are equal - * - Current limit - will set - if '.min_uA' and '.max_uA' values are equal - * - * The function returns on first encountered error. - * - * @platname - expected string for dm_regulator_uclass_plat .name field - * @devp - returned pointer to the regulator device - if non-NULL passed - * @return: 0 on success or negative value of errno. - * - * The returned 'regulator' device can be used with: - * - regulator_get/set_* - */ -int regulator_autoset_by_name(const char *platname, struct udevice **devp); - -/** - * regulator_list_autoset: setup the regulators given by list of their uclass's - * platform data name field. The setup depends on constraints found in device's - * uclass's platform data. The function loops with calls to: - * regulator_autoset_by_name() for each name from the list. - * - * @list_platname - an array of expected strings for .name field of each - * regulator's uclass plat - * @list_devp - an array of returned pointers to the successfully setup - * regulator devices if non-NULL passed - * @verbose - (true/false) print each regulator setup info, or be quiet - * Return: 0 on successfully setup of all list entries, otherwise first error. - * - * The returned 'regulator' devices can be used with: - * - regulator_get/set_* - * - * Note: The list must ends with NULL entry, like in the "platname" list below: - * char *my_regulators[] = { - * "VCC_3.3V", - * "VCC_1.8V", - * NULL, - * }; - */ -int regulator_list_autoset(const char *list_platname[], - struct udevice *list_devp[], - bool verbose); - /** * regulator_get_by_devname: returns the pointer to the pmic regulator device. * Search by name, found in regulator device's name. @@ -628,27 +530,6 @@ static inline int regulator_set_mode(struct udevice *dev, int mode_id) return -ENOSYS; }
-static inline int regulators_enable_boot_on(bool verbose) -{ - return -ENOSYS; -} - -static inline int regulator_autoset(struct udevice *dev) -{ - return -ENOSYS; -} - -static inline int regulator_autoset_by_name(const char *platname, struct udevice **devp) -{ - return -ENOSYS; -} - -static inline int regulator_list_autoset(const char *list_platname[], struct udevice *list_devp[], - bool verbose) -{ - return -ENOSYS; -} - static inline int regulator_get_by_devname(const char *devname, struct udevice **devp) { return -ENOSYS;
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Regulators initial setup was previously dependent on board call. To move from this behaviour were introduced two steps. First is that all individual regulators will be probed just after binding
We must not probe devices automatically when bound. The i2c interface may not be available, etc. Do a probe afterwards.
Perhaps I am misunderstanding this, iwc please reword this commit message.
which ensures that regulator pdata is filled and second is setting up regulator in post probe which enseres that correct regulator state according to pdata is reached.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 212 ++++++--------------- include/power/regulator.h | 119 ------------ 2 files changed, 62 insertions(+), 269 deletions(-)
Regards, SImon
чт, 20 лип. 2023 р. о 22:43 Simon Glass sjg@chromium.org пише:
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Regulators initial setup was previously dependent on board call. To move from this behaviour were introduced two steps. First is that all individual regulators will be probed just after binding
We must not probe devices automatically when bound. The i2c interface may not be available, etc. Do a probe afterwards.
Perhaps I am misunderstanding this, iwc please reword this commit message.
After bound. If the regulator is a self-sufficient i2c device then it will be bound after i2c is available by code design so i2c interface should be available at that moment. At least led and gpio uclasses perform this for initial setup of devices.
Platform regulators (aka fixed/gpio regulators) work perfectly fine. I have no i2c regulators to test deeply.
As for now only one uclass is not compatible with this system - PMIC which has strong dependency between regulator and main mfd. This is why probing after bind for pmic regulators is disabled and pmic regulators are probed on pmic core post_probe.
which ensures that regulator pdata is filled and second is setting up regulator in post probe which enseres that correct regulator state according to pdata is reached.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 212 ++++++--------------- include/power/regulator.h | 119 ------------ 2 files changed, 62 insertions(+), 269 deletions(-)
Regards, SImon
Hi,
On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
чт, 20 лип. 2023 р. о 22:43 Simon Glass sjg@chromium.org пише:
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Regulators initial setup was previously dependent on board call. To move from this behaviour were introduced two steps. First is that all individual regulators will be probed just after binding
We must not probe devices automatically when bound. The i2c interface may not be available, etc. Do a probe afterwards.
Perhaps I am misunderstanding this, iwc please reword this commit message.
After bound. If the regulator is a self-sufficient i2c device then it will be bound after i2c is available by code design so i2c interface should be available at that moment. At least led and gpio uclasses perform this for initial setup of devices.
Platform regulators (aka fixed/gpio regulators) work perfectly fine. I have no i2c regulators to test deeply.
As for now only one uclass is not compatible with this system - PMIC which has strong dependency between regulator and main mfd. This is why probing after bind for pmic regulators is disabled and pmic regulators are probed on pmic core post_probe.
This sounds more like a possible bug of some dependency not being probed in correct order or not leaving the device in a ready state after probe.
If pmic regulators are bind in pmic bind with the pmic as parent, then at regulator probe the driver core ensure that pmic has probed before regulator use.
This works perfect fine with the RK8xx I2C PMIC and its regulators. Wich a call to device_get_supply_regulator(dev, "test-supply", ®) probe happens in i2c, pmic and regulator order.
which ensures that regulator pdata is filled and second is setting up regulator in post probe which enseres that correct regulator state according to pdata is reached.
I think that the approach in this patch is trying to change too much all at once.
Have made some adjustments that I think should help with transition. - Added a flag to protect regulator_autoset from being called more then once for each regulator, in a separate patch. - Changed to only probe-after-bind on regulators marked as always-on/boot-on.
And it works something like this:
static int regulator_post_bind(struct udevice *dev) { [...]
if (uc_pdata->always_on || uc_pdata->boot_on) dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND); }
static int regulator_post_probe(struct udevice *dev) { ret = regulator_autoset(dev); }
With that any always-on/boot-on regulator would automatically probe and autoset after bind, remaining would only probe and autoset if they are used.
This approach should also prevent having to change existing code that may call autoset, and cleanup can be done in follow-up patches/series.
Probe-after-bind and call to autoset could also be protected with a new Kconfig to help with transition.
See following for a working example using a new driver that depends on that regulators have been autoset prior to regulator_get_value. https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
Or the two commits separate:
power: regulator: Only run autoset once for each regulator https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7...
power: regulator: Perform regulator setup inside uclass https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54...
Regards, Jonas
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 212 ++++++--------------- include/power/regulator.h | 119 ------------ 2 files changed, 62 insertions(+), 269 deletions(-)
Regards, SImon
5 серпня 2023 р. 15:49:32 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
Hi,
On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
чт, 20 лип. 2023 р. о 22:43 Simon Glass sjg@chromium.org пише:
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Regulators initial setup was previously dependent on board call. To move from this behaviour were introduced two steps. First is that all individual regulators will be probed just after binding
We must not probe devices automatically when bound. The i2c interface may not be available, etc. Do a probe afterwards.
Perhaps I am misunderstanding this, iwc please reword this commit message.
After bound. If the regulator is a self-sufficient i2c device then it will be bound after i2c is available by code design so i2c interface should be available at that moment. At least led and gpio uclasses perform this for initial setup of devices.
Platform regulators (aka fixed/gpio regulators) work perfectly fine. I have no i2c regulators to test deeply.
As for now only one uclass is not compatible with this system - PMIC which has strong dependency between regulator and main mfd. This is why probing after bind for pmic regulators is disabled and pmic regulators are probed on pmic core post_probe.
This sounds more like a possible bug of some dependency not being probed in correct order or not leaving the device in a ready state after probe.
If pmic regulators are bind in pmic bind with the pmic as parent, then at regulator probe the driver core ensure that pmic has probed before regulator use.
This works perfect fine with the RK8xx I2C PMIC and its regulators. Wich a call to device_get_supply_regulator(dev, "test-supply", ®) probe happens in i2c, pmic and regulator order.
I will check and re-test drivers I have ASAP.
which ensures that regulator pdata is filled and second is setting up regulator in post probe which enseres that correct regulator state according to pdata is reached.
I think that the approach in this patch is trying to change too much all at once.
Have made some adjustments that I think should help with transition.
- Added a flag to protect regulator_autoset from being called more then
once for each regulator, in a separate patch.
It is not a good decision in the long run and you should keep in mind that there is nothing more constant than a temporary solution.
- Changed to only probe-after-bind on regulators marked as
always-on/boot-on.
And it works something like this:
static int regulator_post_bind(struct udevice *dev) { [...]
if (uc_pdata->always_on || uc_pdata->boot_on) dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND); }
static int regulator_post_probe(struct udevice *dev) { ret = regulator_autoset(dev); }
With that any always-on/boot-on regulator would automatically probe and autoset after bind, remaining would only probe and autoset if they are used.
uc_pdata is filled in pre_probe, while you are runing check in post_bind.
This approach should also prevent having to change existing code that may call autoset, and cleanup can be done in follow-up patches/series.
Probe-after-bind and call to autoset could also be protected with a new Kconfig to help with transition.
See following for a working example using a new driver that depends on that regulators have been autoset prior to regulator_get_value. https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
Or the two commits separate:
power: regulator: Only run autoset once for each regulator https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7...
power: regulator: Perform regulator setup inside uclass https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54...
Regards, Jonas
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 212 ++++++--------------- include/power/regulator.h | 119 ------------ 2 files changed, 62 insertions(+), 269 deletions(-)
Regards, SImon
On 2023-08-05 21:58, Svyatoslav Ryhel wrote:
5 серпня 2023 р. 15:49:32 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
Hi,
On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
чт, 20 лип. 2023 р. о 22:43 Simon Glass sjg@chromium.org пише:
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Regulators initial setup was previously dependent on board call. To move from this behaviour were introduced two steps. First is that all individual regulators will be probed just after binding
We must not probe devices automatically when bound. The i2c interface may not be available, etc. Do a probe afterwards.
Perhaps I am misunderstanding this, iwc please reword this commit message.
After bound. If the regulator is a self-sufficient i2c device then it will be bound after i2c is available by code design so i2c interface should be available at that moment. At least led and gpio uclasses perform this for initial setup of devices.
Platform regulators (aka fixed/gpio regulators) work perfectly fine. I have no i2c regulators to test deeply.
As for now only one uclass is not compatible with this system - PMIC which has strong dependency between regulator and main mfd. This is why probing after bind for pmic regulators is disabled and pmic regulators are probed on pmic core post_probe.
This sounds more like a possible bug of some dependency not being probed in correct order or not leaving the device in a ready state after probe.
If pmic regulators are bind in pmic bind with the pmic as parent, then at regulator probe the driver core ensure that pmic has probed before regulator use.
This works perfect fine with the RK8xx I2C PMIC and its regulators. Wich a call to device_get_supply_regulator(dev, "test-supply", ®) probe happens in i2c, pmic and regulator order.
I will check and re-test drivers I have ASAP.
which ensures that regulator pdata is filled and second is setting up regulator in post probe which enseres that correct regulator state according to pdata is reached.
I think that the approach in this patch is trying to change too much all at once.
Have made some adjustments that I think should help with transition.
- Added a flag to protect regulator_autoset from being called more then
once for each regulator, in a separate patch.
It is not a good decision in the long run and you should keep in mind that there is nothing more constant than a temporary solution.
Nor do I propose this to be a long-term solution, only that it is more reviewable to see changes in non-breaking smaller steps. In current state this patch breaks building U-Boot and requires the last patch to fix build again.
Anyway, I will be posting a similar patch for autoset as linked below as part of a series to add a Rockchip IO-domain driver shortly. In current state autoset cannot safely be called multiple times, and such patch should not have an impact on the direction of this series.
- Changed to only probe-after-bind on regulators marked as
always-on/boot-on.
And it works something like this:
static int regulator_post_bind(struct udevice *dev) { [...]
if (uc_pdata->always_on || uc_pdata->boot_on) dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND); }
static int regulator_post_probe(struct udevice *dev) { ret = regulator_autoset(dev); }
With that any always-on/boot-on regulator would automatically probe and autoset after bind, remaining would only probe and autoset if they are used.
uc_pdata is filled in pre_probe, while you are runing check in post_bind.
Please look closer at the code and not the snippet above, they are filled in post_bind or the code would not have worked :-)
Regards, Jonas
This approach should also prevent having to change existing code that may call autoset, and cleanup can be done in follow-up patches/series.
Probe-after-bind and call to autoset could also be protected with a new Kconfig to help with transition.
See following for a working example using a new driver that depends on that regulators have been autoset prior to regulator_get_value. https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
Or the two commits separate:
power: regulator: Only run autoset once for each regulator https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7...
power: regulator: Perform regulator setup inside uclass https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54...
Regards, Jonas
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 212 ++++++--------------- include/power/regulator.h | 119 ------------ 2 files changed, 62 insertions(+), 269 deletions(-)
Regards, SImon
5 серпня 2023 р. 23:46:27 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-08-05 21:58, Svyatoslav Ryhel wrote:
5 серпня 2023 р. 15:49:32 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
Hi,
On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
чт, 20 лип. 2023 р. о 22:43 Simon Glass sjg@chromium.org пише:
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Regulators initial setup was previously dependent on board call. To move from this behaviour were introduced two steps. First is that all individual regulators will be probed just after binding
We must not probe devices automatically when bound. The i2c interface may not be available, etc. Do a probe afterwards.
Perhaps I am misunderstanding this, iwc please reword this commit message.
After bound. If the regulator is a self-sufficient i2c device then it will be bound after i2c is available by code design so i2c interface should be available at that moment. At least led and gpio uclasses perform this for initial setup of devices.
Platform regulators (aka fixed/gpio regulators) work perfectly fine. I have no i2c regulators to test deeply.
As for now only one uclass is not compatible with this system - PMIC which has strong dependency between regulator and main mfd. This is why probing after bind for pmic regulators is disabled and pmic regulators are probed on pmic core post_probe.
This sounds more like a possible bug of some dependency not being probed in correct order or not leaving the device in a ready state after probe.
If pmic regulators are bind in pmic bind with the pmic as parent, then at regulator probe the driver core ensure that pmic has probed before regulator use.
This works perfect fine with the RK8xx I2C PMIC and its regulators. Wich a call to device_get_supply_regulator(dev, "test-supply", ®) probe happens in i2c, pmic and regulator order.
I will check and re-test drivers I have ASAP.
which ensures that regulator pdata is filled and second is setting up regulator in post probe which enseres that correct regulator state according to pdata is reached.
I think that the approach in this patch is trying to change too much all at once.
Have made some adjustments that I think should help with transition.
- Added a flag to protect regulator_autoset from being called more then
once for each regulator, in a separate patch.
It is not a good decision in the long run and you should keep in mind that there is nothing more constant than a temporary solution.
Nor do I propose this to be a long-term solution, only that it is more reviewable to see changes in non-breaking smaller steps. In current state this patch breaks building U-Boot and requires the last patch to fix build again.
Anyway, I will be posting a similar patch for autoset as linked below as part of a series to add a Rockchip IO-domain driver shortly. In current state autoset cannot safely be called multiple times, and such patch should not have an impact on the direction of this series.
- Changed to only probe-after-bind on regulators marked as
always-on/boot-on.
And it works something like this:
static int regulator_post_bind(struct udevice *dev) { [...]
if (uc_pdata->always_on || uc_pdata->boot_on) dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND); }
static int regulator_post_probe(struct udevice *dev) { ret = regulator_autoset(dev); }
With that any always-on/boot-on regulator would automatically probe and autoset after bind, remaining would only probe and autoset if they are used.
uc_pdata is filled in pre_probe, while you are runing check in post_bind.
Please look closer at the code and not the snippet above, they are filled in post_bind or the code would not have worked :-)
So you have moved that, alright. Well, I am fine with less invasive patch as long as my devices continue to work as intended. I will test your suggestions and reload patchset. Thanks.
Best regards, Svyatoslav R.
Regards, Jonas
This approach should also prevent having to change existing code that may call autoset, and cleanup can be done in follow-up patches/series.
Probe-after-bind and call to autoset could also be protected with a new Kconfig to help with transition.
See following for a working example using a new driver that depends on that regulators have been autoset prior to regulator_get_value. https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
Or the two commits separate:
power: regulator: Only run autoset once for each regulator https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7...
power: regulator: Perform regulator setup inside uclass https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54...
Regards, Jonas
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 212 ++++++--------------- include/power/regulator.h | 119 ------------ 2 files changed, 62 insertions(+), 269 deletions(-)
Regards, SImon
сб, 5 серп. 2023 р. о 15:49 Jonas Karlman jonas@kwiboo.se пише:
Hi,
On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
чт, 20 лип. 2023 р. о 22:43 Simon Glass sjg@chromium.org пише:
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Regulators initial setup was previously dependent on board call. To move from this behaviour were introduced two steps. First is that all individual regulators will be probed just after binding
We must not probe devices automatically when bound. The i2c interface may not be available, etc. Do a probe afterwards.
Perhaps I am misunderstanding this, iwc please reword this commit message.
After bound. If the regulator is a self-sufficient i2c device then it will be bound after i2c is available by code design so i2c interface should be available at that moment. At least led and gpio uclasses perform this for initial setup of devices.
Platform regulators (aka fixed/gpio regulators) work perfectly fine. I have no i2c regulators to test deeply.
As for now only one uclass is not compatible with this system - PMIC which has strong dependency between regulator and main mfd. This is why probing after bind for pmic regulators is disabled and pmic regulators are probed on pmic core post_probe.
This sounds more like a possible bug of some dependency not being probed in correct order or not leaving the device in a ready state after probe.
If pmic regulators are bind in pmic bind with the pmic as parent, then at regulator probe the driver core ensure that pmic has probed before regulator use.
I have found why this occurs. With this patchset always/boot-on regulators are probed immediately after bind, since pmic regulators are bound on pmics bind it causes a situation when regulator is probed before pmic and pmics ops are not available. Moving pmic children bind to pmics probe fixed this issue.
This works perfect fine with the RK8xx I2C PMIC and its regulators. Wich a call to device_get_supply_regulator(dev, "test-supply", ®) probe happens in i2c, pmic and regulator order.
which ensures that regulator pdata is filled and second is setting up regulator in post probe which enseres that correct regulator state according to pdata is reached.
I think that the approach in this patch is trying to change too much all at once.
Have made some adjustments that I think should help with transition.
- Added a flag to protect regulator_autoset from being called more then once for each regulator, in a separate patch.
- Changed to only probe-after-bind on regulators marked as always-on/boot-on.
And it works something like this:
static int regulator_post_bind(struct udevice *dev) { [...]
if (uc_pdata->always_on || uc_pdata->boot_on) dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);}
static int regulator_post_probe(struct udevice *dev) { ret = regulator_autoset(dev); }
With that any always-on/boot-on regulator would automatically probe and autoset after bind, remaining would only probe and autoset if they are used.
This approach should also prevent having to change existing code that may call autoset, and cleanup can be done in follow-up patches/series.
Probe-after-bind and call to autoset could also be protected with a new Kconfig to help with transition.
See following for a working example using a new driver that depends on that regulators have been autoset prior to regulator_get_value. https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
Or the two commits separate:
power: regulator: Only run autoset once for each regulator https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7...
power: regulator: Perform regulator setup inside uclass https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54...
Regards, Jonas
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 212 ++++++--------------- include/power/regulator.h | 119 ------------ 2 files changed, 62 insertions(+), 269 deletions(-)
Regards, SImon
нд, 6 серп. 2023 р. о 19:21 Svyatoslav Ryhel clamor95@gmail.com пише:
сб, 5 серп. 2023 р. о 15:49 Jonas Karlman jonas@kwiboo.se пише:
Hi,
On 2023-07-21 07:34, Svyatoslav Ryhel wrote:
чт, 20 лип. 2023 р. о 22:43 Simon Glass sjg@chromium.org пише:
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Regulators initial setup was previously dependent on board call. To move from this behaviour were introduced two steps. First is that all individual regulators will be probed just after binding
We must not probe devices automatically when bound. The i2c interface may not be available, etc. Do a probe afterwards.
Perhaps I am misunderstanding this, iwc please reword this commit message.
After bound. If the regulator is a self-sufficient i2c device then it will be bound after i2c is available by code design so i2c interface should be available at that moment. At least led and gpio uclasses perform this for initial setup of devices.
Platform regulators (aka fixed/gpio regulators) work perfectly fine. I have no i2c regulators to test deeply.
As for now only one uclass is not compatible with this system - PMIC which has strong dependency between regulator and main mfd. This is why probing after bind for pmic regulators is disabled and pmic regulators are probed on pmic core post_probe.
This sounds more like a possible bug of some dependency not being probed in correct order or not leaving the device in a ready state after probe.
If pmic regulators are bind in pmic bind with the pmic as parent, then at regulator probe the driver core ensure that pmic has probed before regulator use.
I have found why this occurs. With this patchset always/boot-on regulators are probed immediately after bind, since pmic regulators are bound on pmics bind it causes a situation when regulator is probed before pmic and pmics ops are not available. Moving pmic children bind to pmics probe fixed this issue.
I have made a mailing list reduction. This issue goes deeper, both pmic and its regulator children work as expected with no issues there. I get error from i2c driver directly that it fails on xfer. This function
https://github.com/clamor-s/u-boot/blob/master/drivers/i2c/tegra_i2c.c#L477
returns me EREMOTEIO. At the same time if the regulator is autoset on pmics post_probe I do not have this error. Maybe you have any suggestions on how to handle this situation?
Best regards, Svyatoslav R.
This works perfect fine with the RK8xx I2C PMIC and its regulators. Wich a call to device_get_supply_regulator(dev, "test-supply", ®) probe happens in i2c, pmic and regulator order.
which ensures that regulator pdata is filled and second is setting up regulator in post probe which enseres that correct regulator state according to pdata is reached.
I think that the approach in this patch is trying to change too much all at once.
Have made some adjustments that I think should help with transition.
- Added a flag to protect regulator_autoset from being called more then once for each regulator, in a separate patch.
- Changed to only probe-after-bind on regulators marked as always-on/boot-on.
And it works something like this:
static int regulator_post_bind(struct udevice *dev) { [...]
if (uc_pdata->always_on || uc_pdata->boot_on) dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);}
static int regulator_post_probe(struct udevice *dev) { ret = regulator_autoset(dev); }
With that any always-on/boot-on regulator would automatically probe and autoset after bind, remaining would only probe and autoset if they are used.
This approach should also prevent having to change existing code that may call autoset, and cleanup can be done in follow-up patches/series.
Probe-after-bind and call to autoset could also be protected with a new Kconfig to help with transition.
See following for a working example using a new driver that depends on that regulators have been autoset prior to regulator_get_value. https://github.com/Kwiboo/u-boot-rockchip/compare/master...rk3568-io-domain
Or the two commits separate:
power: regulator: Only run autoset once for each regulator https://github.com/Kwiboo/u-boot-rockchip/commit/05db4dbcb8f908b417ed5cae8a7...
power: regulator: Perform regulator setup inside uclass https://github.com/Kwiboo/u-boot-rockchip/commit/489bd5d2c1a2a33824eac4f2d54...
Regards, Jonas
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/regulator/regulator-uclass.c | 212 ++++++--------------- include/power/regulator.h | 119 ------------ 2 files changed, 62 insertions(+), 269 deletions(-)
Regards, SImon
Replace deprecated API test with test of uclass wide autosetup.
LDO2 voltage according to dts should be 3.3v not 3.0v
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com --- include/power/sandbox_pmic.h | 2 +- test/dm/regulator.c | 131 +++++++---------------------------- 2 files changed, 27 insertions(+), 106 deletions(-)
diff --git a/include/power/sandbox_pmic.h b/include/power/sandbox_pmic.h index 1dbd15b523..50fc5881f8 100644 --- a/include/power/sandbox_pmic.h +++ b/include/power/sandbox_pmic.h @@ -137,7 +137,7 @@ enum { #define SANDBOX_LDO1_AUTOSET_EXPECTED_UA 100000 #define SANDBOX_LDO1_AUTOSET_EXPECTED_ENABLE true
-#define SANDBOX_LDO2_AUTOSET_EXPECTED_UV 3000000 +#define SANDBOX_LDO2_AUTOSET_EXPECTED_UV 3300000 #define SANDBOX_LDO2_AUTOSET_EXPECTED_UA -ENOSYS #define SANDBOX_LDO2_AUTOSET_EXPECTED_ENABLE false
diff --git a/test/dm/regulator.c b/test/dm/regulator.c index 9b503e4c59..eb96ddcdac 100644 --- a/test/dm/regulator.c +++ b/test/dm/regulator.c @@ -182,12 +182,11 @@ static int dm_test_power_regulator_set_enable_if_allowed(struct unit_test_state *uts) { const char *platname; - struct udevice *dev, *dev_autoset; + struct udevice *dev; bool val_set = false;
/* Get BUCK1 - always on regulator */ platname = regulator_names[BUCK1][PLATNAME]; - ut_assertok(regulator_autoset_by_name(platname, &dev_autoset)); ut_assertok(regulator_get_by_platname(platname, &dev));
/* Try disabling always-on regulator */ @@ -196,7 +195,6 @@ int dm_test_power_regulator_set_enable_if_allowed(struct unit_test_state *uts)
/* Set the Enable of LDO1 - default is disabled */ platname = regulator_names[LDO1][PLATNAME]; - ut_assertok(regulator_autoset_by_name(platname, &dev_autoset)); ut_assertok(regulator_get_by_platname(platname, &dev));
/* Get the Enable state of LDO1 and compare it with the requested one */ @@ -293,131 +291,54 @@ static int dm_test_power_regulator_set_get_suspend_enable(struct unit_test_state } DM_TEST(dm_test_power_regulator_set_get_suspend_enable, UT_TESTF_SCAN_FDT);
-/* Test regulator autoset method */ -static int dm_test_power_regulator_autoset(struct unit_test_state *uts) +/* Test regulator setup inside uclass driver */ +static int dm_test_power_regulator_set(struct unit_test_state *uts) { const char *platname; - struct udevice *dev, *dev_autoset; + struct udevice *dev;
/* - * Test the BUCK1 with fdt properties - * - min-microvolt = max-microvolt = 1200000 - * - min-microamp = max-microamp = 200000 - * - always-on = set - * - boot-on = not set - * Expected output state: uV=1200000; uA=200000; output enabled + * LDO1 with fdt properties: + * - min-microvolt = max-microvolt = 1800000 + * - min-microamp = max-microamp = 100000 + * - always-on = not set + * - boot-on = set + * Expected output state: uV=1800000; uA=100000; output enabled */ - platname = regulator_names[BUCK1][PLATNAME]; - ut_assertok(regulator_autoset_by_name(platname, &dev_autoset));
/* Check, that the returned device is proper */ + platname = regulator_names[LDO1][PLATNAME]; ut_assertok(regulator_get_by_platname(platname, &dev)); - ut_asserteq_ptr(dev, dev_autoset);
/* Check the setup after autoset */ ut_asserteq(regulator_get_value(dev), - SANDBOX_BUCK1_AUTOSET_EXPECTED_UV); + SANDBOX_LDO1_AUTOSET_EXPECTED_UV); ut_asserteq(regulator_get_current(dev), - SANDBOX_BUCK1_AUTOSET_EXPECTED_UA); + SANDBOX_LDO1_AUTOSET_EXPECTED_UA); ut_asserteq(regulator_get_enable(dev), - SANDBOX_BUCK1_AUTOSET_EXPECTED_ENABLE); - - return 0; -} -DM_TEST(dm_test_power_regulator_autoset, UT_TESTF_SCAN_FDT); - -/* - * Struct setting: to keep the expected output settings. - * @voltage: Voltage value [uV] - * @current: Current value [uA] - * @enable: output enable state: true/false - */ -struct setting { - int voltage; - int current; - bool enable; -}; - -/* - * platname_list: an array of regulator platform names. - * For testing regulator_list_autoset() for outputs: - * - LDO1 - * - LDO2 - */ -static const char *platname_list[] = { - SANDBOX_LDO1_PLATNAME, - SANDBOX_LDO2_PLATNAME, - NULL, -}; - -/* - * expected_setting_list: an array of regulator output setting, expected after - * call of the regulator_list_autoset() for the "platname_list" array. - * For testing results of regulator_list_autoset() for outputs: - * - LDO1 - * - LDO2 - * The settings are defined in: include/power/sandbox_pmic.h - */ -static const struct setting expected_setting_list[] = { - [0] = { /* LDO1 */ - .voltage = SANDBOX_LDO1_AUTOSET_EXPECTED_UV, - .current = SANDBOX_LDO1_AUTOSET_EXPECTED_UA, - .enable = SANDBOX_LDO1_AUTOSET_EXPECTED_ENABLE, - }, - [1] = { /* LDO2 */ - .voltage = SANDBOX_LDO2_AUTOSET_EXPECTED_UV, - .current = SANDBOX_LDO2_AUTOSET_EXPECTED_UA, - .enable = SANDBOX_LDO2_AUTOSET_EXPECTED_ENABLE, - }, -}; - -static int list_count = ARRAY_SIZE(expected_setting_list); - -/* Test regulator list autoset method */ -static int dm_test_power_regulator_autoset_list(struct unit_test_state *uts) -{ - struct udevice *dev_list[2], *dev; - int i; + SANDBOX_LDO1_AUTOSET_EXPECTED_ENABLE);
/* - * Test the settings of the regulator list: - * LDO1 with fdt properties: - * - min-microvolt = max-microvolt = 1800000 - * - min-microamp = max-microamp = 100000 - * - always-on = not set - * - boot-on = set - * Expected output state: uV=1800000; uA=100000; output enabled - * * LDO2 with fdt properties: * - min-microvolt = max-microvolt = 3300000 * - always-on = not set * - boot-on = not set - * Expected output state: uV=300000(default); output disabled(default) + * Expected output state: uV=330000; output disabled * The expected settings are defined in: include/power/sandbox_pmic.h. */ - ut_assertok(regulator_list_autoset(platname_list, dev_list, false));
- for (i = 0; i < list_count; i++) { - /* Check, that the returned device is non-NULL */ - ut_assert(dev_list[i]); - - /* Check, that the returned device is proper */ - ut_assertok(regulator_get_by_platname(platname_list[i], &dev)); - ut_asserteq_ptr(dev_list[i], dev); - - /* Check, that regulator output Voltage value is as expected */ - ut_asserteq(regulator_get_value(dev_list[i]), - expected_setting_list[i].voltage); - - /* Check, that regulator output Current value is as expected */ - ut_asserteq(regulator_get_current(dev_list[i]), - expected_setting_list[i].current); + /* Check, that the returned device is proper */ + platname = regulator_names[LDO2][PLATNAME]; + ut_assertok(regulator_get_by_platname(platname, &dev));
- /* Check, that regulator output Enable state is as expected */ - ut_asserteq(regulator_get_enable(dev_list[i]), - expected_setting_list[i].enable); - } + /* Check the setup after autoset */ + ut_asserteq(regulator_get_value(dev), + SANDBOX_LDO2_AUTOSET_EXPECTED_UV); + ut_asserteq(regulator_get_current(dev), + SANDBOX_LDO2_AUTOSET_EXPECTED_UA); + ut_asserteq(regulator_get_enable(dev), + SANDBOX_LDO2_AUTOSET_EXPECTED_ENABLE);
return 0; } -DM_TEST(dm_test_power_regulator_autoset_list, UT_TESTF_SCAN_FDT); +DM_TEST(dm_test_power_regulator_set, UT_TESTF_SCAN_FDT);
Main goal is to probe all regulator childrens for their proper setup but if pmic has non regulator children they should not suffer from this either.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com --- drivers/power/pmic/pmic-uclass.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 0e2f5e1f41..8ca717bd5e 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -16,6 +16,7 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <power/pmic.h> +#include <power/regulator.h> #include <linux/ctype.h>
#if CONFIG_IS_ENABLED(PMIC_CHILDREN) @@ -198,9 +199,18 @@ static int pmic_pre_probe(struct udevice *dev) return 0; }
+static int pmic_post_probe(struct udevice *dev) +{ + struct udevice *child; + + device_foreach_child_probe(child, dev); + return 0; +} + UCLASS_DRIVER(pmic) = { .id = UCLASS_PMIC, .name = "pmic", .pre_probe = pmic_pre_probe, + .post_probe = pmic_post_probe, .per_device_auto = sizeof(struct uc_pmic_priv), };
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Main goal is to probe all regulator childrens for their proper setup but if pmic has non regulator children they should not suffer from this either.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/pmic/pmic-uclass.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 0e2f5e1f41..8ca717bd5e 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -16,6 +16,7 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <power/pmic.h> +#include <power/regulator.h> #include <linux/ctype.h>
I'm not sure about this.
The idea is that power is handling automatically, e.g. a device is probed and so its power is enabled. If you do everything at the start, doesn't that violate the 'lazy' init side of U-Boot?
#if CONFIG_IS_ENABLED(PMIC_CHILDREN) @@ -198,9 +199,18 @@ static int pmic_pre_probe(struct udevice *dev) return 0; }
+static int pmic_post_probe(struct udevice *dev) +{
struct udevice *child;device_foreach_child_probe(child, dev);return 0;+}
UCLASS_DRIVER(pmic) = { .id = UCLASS_PMIC, .name = "pmic", .pre_probe = pmic_pre_probe,
.post_probe = pmic_post_probe, .per_device_auto = sizeof(struct uc_pmic_priv),};
2.39.2
Regards, Simon
чт, 20 лип. 2023 р. о 22:43 Simon Glass sjg@chromium.org пише:
Hi Svyatoslav,
On Thu, 20 Jul 2023 at 06:38, Svyatoslav Ryhel clamor95@gmail.com wrote:
Main goal is to probe all regulator childrens for their proper setup but if pmic has non regulator children they should not suffer from this either.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/pmic/pmic-uclass.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 0e2f5e1f41..8ca717bd5e 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -16,6 +16,7 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <power/pmic.h> +#include <power/regulator.h> #include <linux/ctype.h>
I'm not sure about this.
The idea is that power is handling automatically, e.g. a device is probed and so its power is enabled. If you do everything at the start,
Why do you think probe == power on? As for now in u-boot pmic childrens are mostly regulators with few minor exceptions. Unlike any other devices regulators have special properties like boot-on or always-on and are required by other devices to operate correctly.
Before this patch special properties were either neglected or their establishment was performed with a board call, both ways are bad solutions. With this patch all pmic regulators are probed and set according to their dts properties (yes, if the regulator is not in use it will be turned off so no power consumption issue there) which is desired behavior.
doesn't that violate the 'lazy' init side of U-Boot?
That 'lazy' init resulted in more issues for me (half working devices, broken usb) then if it was handled actively. I am not against this approach, but pmic and regulators is not the device which should embrace it.
#if CONFIG_IS_ENABLED(PMIC_CHILDREN) @@ -198,9 +199,18 @@ static int pmic_pre_probe(struct udevice *dev) return 0; }
+static int pmic_post_probe(struct udevice *dev) +{
struct udevice *child;device_foreach_child_probe(child, dev);return 0;+}
UCLASS_DRIVER(pmic) = { .id = UCLASS_PMIC, .name = "pmic", .pre_probe = pmic_pre_probe,
.post_probe = pmic_post_probe, .per_device_auto = sizeof(struct uc_pmic_priv),};
2.39.2
Regards, Simon
Test if regulator uclass children of PMIC are auto probed and set after PMIC probe.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com --- test/dm/pmic.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/test/dm/pmic.c b/test/dm/pmic.c index ce671202fb..d0a3c24a54 100644 --- a/test/dm/pmic.c +++ b/test/dm/pmic.c @@ -18,6 +18,7 @@ #include <dm/uclass-internal.h> #include <dm/util.h> #include <power/pmic.h> +#include <power/regulator.h> #include <power/sandbox_pmic.h> #include <test/test.h> #include <test/ut.h> @@ -129,3 +130,36 @@ static int dm_test_power_pmic_mc34708_rw_val(struct unit_test_state *uts) }
DM_TEST(dm_test_power_pmic_mc34708_rw_val, UT_TESTF_SCAN_FDT); + +static int dm_test_power_pmic_child_probe(struct unit_test_state *uts) +{ + const char *name = "sandbox_pmic"; + const char *devname = "ldo1"; + struct udevice *dev; + + ut_assertok(pmic_get(name, &dev)); + + /* + * LDO1 with fdt properties: + * - min-microvolt = max-microvolt = 1800000 + * - min-microamp = max-microamp = 100000 + * - always-on = not set + * - boot-on = set + * Expected output state: uV=1800000; uA=100000; output enabled + */ + + /* Check, that the returned device is proper */ + ut_assertok(regulator_get_by_devname(devname, &dev)); + + /* Check the setup after autoset */ + ut_asserteq(regulator_get_value(dev), + SANDBOX_LDO1_AUTOSET_EXPECTED_UV); + ut_asserteq(regulator_get_current(dev), + SANDBOX_LDO1_AUTOSET_EXPECTED_UA); + ut_asserteq(regulator_get_enable(dev), + SANDBOX_LDO1_AUTOSET_EXPECTED_ENABLE); + + return 0; +} + +DM_TEST(dm_test_power_pmic_child_probe, UT_TESTF_SCAN_FDT);
THIS REQUIRES PRECISE TESTING!
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com --- arch/arm/mach-rockchip/board.c | 8 -------- arch/arm/mach-rockchip/rk3399/rk3399.c | 10 ---------- board/Marvell/octeontx2_cn913x/board.c | 5 ----- board/amlogic/odroid-go-ultra/odroid-go-ultra.c | 2 -- board/dhelectronics/dh_stm32mp1/board.c | 2 -- board/engicam/stm32mp1/stm32mp1.c | 3 --- board/google/veyron/veyron.c | 6 ------ board/samsung/common/exynos5-dt.c | 4 ---- board/samsung/odroid/odroid.c | 10 ---------- board/st/stm32mp1/stm32mp1.c | 9 --------- drivers/video/bridge/ps862x.c | 12 ++++++++---- drivers/video/rockchip/rk_vop.c | 6 ++---- 12 files changed, 10 insertions(+), 67 deletions(-)
diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index 8d7b39ba15..3eaea4b04a 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -189,14 +189,6 @@ int board_late_init(void)
int board_init(void) { - int ret; - -#ifdef CONFIG_DM_REGULATOR - ret = regulators_enable_boot_on(false); - if (ret) - debug("%s: Cannot enable boot on regulator\n", __func__); -#endif - return 0; }
diff --git a/arch/arm/mach-rockchip/rk3399/rk3399.c b/arch/arm/mach-rockchip/rk3399/rk3399.c index a7cc91a952..cbd2ea047d 100644 --- a/arch/arm/mach-rockchip/rk3399/rk3399.c +++ b/arch/arm/mach-rockchip/rk3399/rk3399.c @@ -280,15 +280,5 @@ void spl_board_init(void) if (cru->glb_rst_st != 0) rk3399_force_power_on_reset(); } - - if (IS_ENABLED(CONFIG_SPL_DM_REGULATOR)) { - /* - * Turning the eMMC and SPI back on (if disabled via the Qseven - * BIOS_ENABLE) signal is done through a always-on regulator). - */ - if (regulators_enable_boot_on(false)) - debug("%s: Cannot enable boot on regulator\n", - __func__); - } } #endif diff --git a/board/Marvell/octeontx2_cn913x/board.c b/board/Marvell/octeontx2_cn913x/board.c index 3d20cfb2fa..3ffe15d42b 100644 --- a/board/Marvell/octeontx2_cn913x/board.c +++ b/board/Marvell/octeontx2_cn913x/board.c @@ -23,11 +23,6 @@ int board_early_init_f(void)
int board_early_init_r(void) { - if (CONFIG_IS_ENABLED(DM_REGULATOR)) { - /* Check if any existing regulator should be turned down */ - regulators_enable_boot_off(false); - } - return 0; }
diff --git a/board/amlogic/odroid-go-ultra/odroid-go-ultra.c b/board/amlogic/odroid-go-ultra/odroid-go-ultra.c index bbd23e20fc..fa6105a071 100644 --- a/board/amlogic/odroid-go-ultra/odroid-go-ultra.c +++ b/board/amlogic/odroid-go-ultra/odroid-go-ultra.c @@ -16,7 +16,5 @@ int mmc_get_env_dev(void)
int board_init(void) { - regulators_enable_boot_on(_DEBUG); - return 0; } diff --git a/board/dhelectronics/dh_stm32mp1/board.c b/board/dhelectronics/dh_stm32mp1/board.c index f9cfabe242..a8ad5a1620 100644 --- a/board/dhelectronics/dh_stm32mp1/board.c +++ b/board/dhelectronics/dh_stm32mp1/board.c @@ -615,8 +615,6 @@ static void board_init_regulator_av96(void) static void board_init_regulator(void) { board_init_regulator_av96(); - - regulators_enable_boot_on(_DEBUG); } #else static inline int board_get_regulator_buck3_nvm_uv_av96(int *uv) diff --git a/board/engicam/stm32mp1/stm32mp1.c b/board/engicam/stm32mp1/stm32mp1.c index 5223e9bae8..c98bbdc71b 100644 --- a/board/engicam/stm32mp1/stm32mp1.c +++ b/board/engicam/stm32mp1/stm32mp1.c @@ -38,9 +38,6 @@ int checkboard(void) /* board dependent setup after realloc */ int board_init(void) { - if (IS_ENABLED(CONFIG_DM_REGULATOR)) - regulators_enable_boot_on(_DEBUG); - return 0; }
diff --git a/board/google/veyron/veyron.c b/board/google/veyron/veyron.c index 32dbcdc4d1..527e9d4b0e 100644 --- a/board/google/veyron/veyron.c +++ b/board/google/veyron/veyron.c @@ -62,12 +62,6 @@ static int veyron_init(void) if (ret) return ret;
- ret = regulators_enable_boot_on(false); - if (ret) { - debug("%s: Cannot enable boot on regulators\n", __func__); - return ret; - } - return 0; } #endif diff --git a/board/samsung/common/exynos5-dt.c b/board/samsung/common/exynos5-dt.c index cde77d79a0..45d34d838d 100644 --- a/board/samsung/common/exynos5-dt.c +++ b/board/samsung/common/exynos5-dt.c @@ -92,10 +92,6 @@ int exynos_power_init(void) if (ret == -ENODEV) return 0;
- ret = regulators_enable_boot_on(false); - if (ret) - return ret; - ret = exynos_set_regulator("vdd_mif", 1100000); if (ret) return ret; diff --git a/board/samsung/odroid/odroid.c b/board/samsung/odroid/odroid.c index 39a60e4ad2..28086ee92e 100644 --- a/board/samsung/odroid/odroid.c +++ b/board/samsung/odroid/odroid.c @@ -431,16 +431,6 @@ int exynos_init(void)
int exynos_power_init(void) { - const char *mmc_regulators[] = { - "VDDQ_EMMC_1.8V", - "VDDQ_EMMC_2.8V", - "TFLASH_2.8V", - NULL, - }; - - if (regulator_list_autoset(mmc_regulators, NULL, true)) - pr_err("Unable to init all mmc regulators\n"); - return 0; }
diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c index 3205a31c6d..7e425edefd 100644 --- a/board/st/stm32mp1/stm32mp1.c +++ b/board/st/stm32mp1/stm32mp1.c @@ -602,13 +602,6 @@ static int board_stm32mp15x_dk2_init(void) goto error; }
- /* power-up audio IC */ - regulator_autoset_by_name("v1v8_audio", NULL); - - /* power-up HDMI IC */ - regulator_autoset_by_name("v1v2_hdmi", NULL); - regulator_autoset_by_name("v3v3_hdmi", NULL); - error: return ret; } @@ -665,8 +658,6 @@ int board_init(void) if (board_is_stm32mp15x_dk2()) board_stm32mp15x_dk2_init();
- regulators_enable_boot_on(_DEBUG); - /* * sysconf initialisation done only when U-Boot is running in secure * done in TF-A for TFABOOT. diff --git a/drivers/video/bridge/ps862x.c b/drivers/video/bridge/ps862x.c index d1d22a6e23..52a343bde2 100644 --- a/drivers/video/bridge/ps862x.c +++ b/drivers/video/bridge/ps862x.c @@ -77,13 +77,17 @@ static int ps8622_attach(struct udevice *dev) /* set the LDO providing the 1.2V rail to the Parade bridge */ ret = uclass_get_device_by_phandle(UCLASS_REGULATOR, dev, "power-supply", ®); - if (!ret) { - ret = regulator_autoset(reg); - } else if (ret != -ENOENT) { - debug("%s: Failed to enable power: ret=%d\n", __func__, ret); + if (ret) { + debug("%s: Failed to get power: ret=%d\n", __func__, ret); return ret; }
+ if (reg) { + ret = regulator_set_enable(reg, true); + if (ret) + return ret; + } + ret = video_bridge_set_active(dev, true); if (ret) return ret; diff --git a/drivers/video/rockchip/rk_vop.c b/drivers/video/rockchip/rk_vop.c index c514e2a0e4..e81eed5ffa 100644 --- a/drivers/video/rockchip/rk_vop.c +++ b/drivers/video/rockchip/rk_vop.c @@ -397,7 +397,7 @@ static int rk_display_init(struct udevice *dev, ulong fbbase, ofnode ep_node) void rk_vop_probe_regulators(struct udevice *dev, const char * const *names, int cnt) { - int i, ret; + int i; const char *name; struct udevice *reg;
@@ -405,9 +405,7 @@ void rk_vop_probe_regulators(struct udevice *dev, name = names[i]; debug("%s: probing regulator '%s'\n", dev->name, name);
- ret = regulator_autoset_by_name(name, ®); - if (!ret) - ret = regulator_set_enable(reg, true); + regulator_set_enable(reg, true); } }
participants (3)
-
Jonas Karlman -
Simon Glass -
Svyatoslav Ryhel