* [PATCH v3 0/4] regulator_sync_state() support @ 2020-07-16 4:20 Saravana Kannan 2020-07-16 4:20 ` [PATCH v3 1/4] driver core: Add dev_set_drv_sync_state() Saravana Kannan ` (5 more replies) 0 siblings, 6 replies; 16+ messages in thread From: Saravana Kannan @ 2020-07-16 4:20 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, Greg Kroah-Hartman Cc: Saravana Kannan, Marek Szyprowski, John Stultz, linux-kernel, kernel-team Consider the following example: - regulator-X is provided by device-X. - regulator-X is a supplier to device-A, device-B and device-C. - device-A is off/inactive from boot. - device-B and device-C are left on/active by the bootloader - regulator-X is left on boot by the bootloader at 2000 mV to supply device-B and device-C. Example boot sequence 1: 1. device-X is probed successfully. 2. device-A is probed by driver-A a. driver-A gets regulator-X b. driver-A votes on regulator-X c. driver-A initializes device-A d. driver-A votes off regulator-X e. regulator-X is turned off. 3. System crashes or device-B and device-C become unreliable because regulator-X was turned off without following the proper quiescing steps for device-B and device-C. Example boot sequence 2: 1. device-X is probed successfully. 2. device-B is probed by driver-B a. driver-B gets regulator-X b. driver-B votes on regulator-X c. driver-B lowers device-B performance point. d. driver-B lowers voltage vote to 1000 mV. e. regulator-X voltage is lowered to 1000 mV. 3. System crashes or device-C becomes unreliable because regulator-X voltage was lowered to 1000 mV when device-C still needed it at 2000 mV This patch series makes sure these examples are handled correctly and system crash or device instability is avoided and the system remains usable. More details provided in the commit texts. v2->v3: Patch 2/4 - No functional change. Simple refactor. Patch 3/4 - Was Patch 2/2 in v2. - Rewrote commit text to hopefully address all previous points. - Renamed variable/functions. Hope it's clearer. - Added more comments. - Added logging - Fixed timeout functionality. - Handle exclusive consumers properly - Handle coupled regulators properly Patch 4/4 - Prevents voltage from going too low during boot. v1->v2: Patch 1/2 - New patch Patch 2/2 - This was the only patch in v1 - Made the late_initcall_sync timeout a commandline param - If timeout is set, we also give up waiting for all consumers after the timeout expires. - Made every regulator driver add sync_state() support Saravana Kannan (4): driver core: Add dev_set_drv_sync_state() regulator: core: Add destroy_regulator() regulator: core: Add basic enable/disable support for sync_state() callbacks regulator: core: Add voltage support for sync_state() callbacks drivers/regulator/core.c | 200 ++++++++++++++++++++++++++++--- include/linux/device.h | 12 ++ include/linux/regulator/driver.h | 2 + 3 files changed, 198 insertions(+), 16 deletions(-) -- 2.28.0.rc0.105.gf9edc3c819-goog ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 1/4] driver core: Add dev_set_drv_sync_state() 2020-07-16 4:20 [PATCH v3 0/4] regulator_sync_state() support Saravana Kannan @ 2020-07-16 4:20 ` Saravana Kannan 2020-07-16 4:20 ` [PATCH v3 2/4] regulator: core: Add destroy_regulator() Saravana Kannan ` (4 subsequent siblings) 5 siblings, 0 replies; 16+ messages in thread From: Saravana Kannan @ 2020-07-16 4:20 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, Greg Kroah-Hartman Cc: Saravana Kannan, Marek Szyprowski, John Stultz, linux-kernel, kernel-team This can be used by frameworks to set the sync_state() helper functions for drivers that don't already have them set. Signed-off-by: Saravana Kannan <saravanak@google.com> --- include/linux/device.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/include/linux/device.h b/include/linux/device.h index 15460a5ac024..2f56afdd9107 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -806,6 +806,18 @@ static inline bool dev_has_sync_state(struct device *dev) return false; } +static inline int dev_set_drv_sync_state(struct device *dev, + void (*fn)(struct device *dev)) +{ + if (!dev || !dev->driver) + return 0; + if (dev->driver->sync_state && dev->driver->sync_state != fn) + return -EBUSY; + if (!dev->driver->sync_state) + dev->driver->sync_state = fn; + return 0; +} + /* * High level routines for use by the bus drivers */ -- 2.28.0.rc0.105.gf9edc3c819-goog ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 2/4] regulator: core: Add destroy_regulator() 2020-07-16 4:20 [PATCH v3 0/4] regulator_sync_state() support Saravana Kannan 2020-07-16 4:20 ` [PATCH v3 1/4] driver core: Add dev_set_drv_sync_state() Saravana Kannan @ 2020-07-16 4:20 ` Saravana Kannan 2020-07-16 4:20 ` [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks Saravana Kannan ` (3 subsequent siblings) 5 siblings, 0 replies; 16+ messages in thread From: Saravana Kannan @ 2020-07-16 4:20 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, Greg Kroah-Hartman Cc: Saravana Kannan, Marek Szyprowski, John Stultz, linux-kernel, kernel-team Part of the regulator_get() code is already factored out into create_regulator(). This patch factors out some of the regulator_put() code into destroy_regulator() so that create_regulator() has a corresponding unwind function. Subsequent patches will use this function. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/regulator/core.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 03154f5b939f..c9615d3530c7 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -105,6 +105,7 @@ static int regulator_balance_voltage(struct regulator_dev *rdev, static struct regulator *create_regulator(struct regulator_dev *rdev, struct device *dev, const char *supply_name); +static void destroy_regulator(struct regulator *regulator); static void _regulator_put(struct regulator *regulator); const char *rdev_get_name(struct regulator_dev *rdev) @@ -2034,20 +2035,9 @@ struct regulator *regulator_get_optional(struct device *dev, const char *id) } EXPORT_SYMBOL_GPL(regulator_get_optional); -/* regulator_list_mutex lock held by regulator_put() */ -static void _regulator_put(struct regulator *regulator) +static void destroy_regulator(struct regulator *regulator) { - struct regulator_dev *rdev; - - if (IS_ERR_OR_NULL(regulator)) - return; - - lockdep_assert_held_once(®ulator_list_mutex); - - /* Docs say you must disable before calling regulator_put() */ - WARN_ON(regulator->enable_count); - - rdev = regulator->rdev; + struct regulator_dev *rdev = regulator->rdev; debugfs_remove_recursive(regulator->debugfs); @@ -2068,6 +2058,24 @@ static void _regulator_put(struct regulator *regulator) kfree_const(regulator->supply_name); kfree(regulator); +} + +/* regulator_list_mutex lock held by regulator_put() */ +static void _regulator_put(struct regulator *regulator) +{ + struct regulator_dev *rdev; + + if (IS_ERR_OR_NULL(regulator)) + return; + + lockdep_assert_held_once(®ulator_list_mutex); + + /* Docs say you must disable before calling regulator_put() */ + WARN_ON(regulator->enable_count); + + rdev = regulator->rdev; + + destroy_regulator(regulator); module_put(rdev->owner); put_device(&rdev->dev); -- 2.28.0.rc0.105.gf9edc3c819-goog ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks 2020-07-16 4:20 [PATCH v3 0/4] regulator_sync_state() support Saravana Kannan 2020-07-16 4:20 ` [PATCH v3 1/4] driver core: Add dev_set_drv_sync_state() Saravana Kannan 2020-07-16 4:20 ` [PATCH v3 2/4] regulator: core: Add destroy_regulator() Saravana Kannan @ 2020-07-16 4:20 ` Saravana Kannan 2020-07-20 14:27 ` Mark Brown 2020-07-16 4:20 ` [PATCH v3 4/4] regulator: core: Add voltage " Saravana Kannan ` (2 subsequent siblings) 5 siblings, 1 reply; 16+ messages in thread From: Saravana Kannan @ 2020-07-16 4:20 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, Greg Kroah-Hartman Cc: Saravana Kannan, Marek Szyprowski, John Stultz, linux-kernel, kernel-team Consider the following example: - regulator-X is provided by device-X. - regulator-X is a supplier to device-A, device-B and device-C. - device-A is off/inactive from boot. - device-B and device-C are left on/active by the bootloader - regulator-X is left on boot by the bootloader at 2000 mV to supply device-B and device-C. Example boot sequence: 1. device-X is probed successfully. 2. device-A is probed by driver-A a. driver-A gets regulator-X b. driver-A votes on regulator-X c. driver-A initializes device-A d. driver-A votes off regulator-X e. regulator-X is turned off. 3. System crashes or device-B and device-C become unreliable because regulator-X was turned off without following the proper quiescing steps for device-B and device-C. There are Android devices that exhibit the issue in the example where regulator-X is an LDO, device-A is a camera device and device-B and device-C are UFS and USB. To avoid this, they have their own downstream changes to the regulator framework. This patch addresses the problem in the example by: 1. When a regulator is registered, a. The sync_state() callback for the regulator's device is set to regulator_sync_state(). The sync_state() callback is called when all the consumers of the regulator's device have probed successfully. b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created for the regulator and an enable vote is made. 2. When the regulator_sync_state() callback comes, all the boot-on regulators registered by that device will have the BOOT-LIMITS enable vote removed and the BOOT-LIMITS consumer freed. If an exclusive get is ever attempted on a boot-on regulator with an active BOOT-LIMITS vote, the regulator is handed off to the new consumer (and the BOOT-LIMITS freed) without affecting the regulator state. This ensures, consumers doing exclusive gets continue to work after this commit. To maintain backward compatibility with systems where some consumers of the device might never probe, a new regulator_cleanup_timeout kernel commandline argument is added and defaulted to 30 seconds. When the timeout is a non-zero value and it expires, all BOOT-LIMITS consumer votes are removed even if the sync_state() callbacks haven't been called. In systems where all the consumers are expected to probe, the regulator_cleanup_timeout can be set to 0. When that's done, the BOOT-LIMITS consumer votes for a regulator are removed only when all the consumers of the regulator's device have probed. So with this patch and regulator_cleanup_timeout=0, the example will work as follows: 1. device-X is probed successfully. a. regulator-X is registered. b. Since regulator-X is on, an enable vote is made by the BOOT-LIMITS consumer. 2. device-A is probed by driver-A a. driver-A gets regulator-X b. driver-A votes on regulator-X c. driver-A initializes device-A d. driver-A votes off regulator-X e. regulator-X is NOT turned off due to BOOT-LIMITS consumer. 3. device-B is probed by driver-B a. driver-B gets regulator-X b. driver-B votes on regulator-X c. driver-B initializes device-B. d. driver-B votes off regulator-X because device-B enters runtime idle. e. regulator-X is NOT turned off due to BOOT-LIMITS consumer. 4. device-C is probed by driver-C a. Does stuff similar to device-B and votes off regulator-X. 5. Since all consumers of device-X have probed, device-X gets sync_state() callback which is a call to regulator_sync_state(): a. BOOT-LIMITS removes enable vote for regulator-X b. regulator-X is turned off. c. BOOT-LIMITS consumer is freed. 6. The system is stable because regulator-X is only turned off after device-B and device-C get to initialize and quiesce properly. OR Same steps for 1 - 3 as above. 4. device-C is never probed because driver-C isn't available. 5. Since all consumers of device-X have NOT probed, device-X doesn't get sync_state() callback. a. BOOT-LIMITS votes continue to be enforced for regulator-X 6. device-C continues to work in the mode the boot loader left it in and the system remains usable. For example, device C is - A display backlight that doesn't have a driver - An interconnect that doesn't have an interconnect provider driver Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/regulator/core.c | 155 ++++++++++++++++++++++++++++++- include/linux/regulator/driver.h | 2 + 2 files changed, 154 insertions(+), 3 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index c9615d3530c7..f10ef6ec1af1 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1769,6 +1769,115 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev, return ERR_PTR(-ENODEV); } +/** + * regulator_add_boot_limits - Set up boot limits for a regulator device. + * @rdev: regulator device to set up boot limits for. + * + * Makes requests on the regulator device @rdev so that its operating state + * doesn't drop below the state in which the bootloader left it at before + * starting the kernel. + * + * If boot limits has already been set up, this function doesn't do anything. + * + * The boot limits can be dropped later by calling regulator_del_boot_limits(). + */ +static void regulator_add_boot_limits(struct regulator_dev *rdev) +{ + /* We already set up boot limits. */ + if (rdev->boot_limits) + return; + + /* + * If the regulator is disabled or has an error getting the status, + * assume it's not on. We won't explicitly turn it off now, that can + * happen later. + */ + if (_regulator_is_enabled(rdev) <= 0) + return; + + /* Wait for supply to get resolved */ + if (rdev->supply_name && !rdev->supply) + return; + + /* Wait for coupled regulators to be resolved */ + if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) + return; + + /* + * Get a reference to a regulator to set up boot limits to prevent it + * from going below the state in which the boot loader left it in. + */ + rdev->boot_limits = create_regulator(rdev, NULL, "BOOT-LIMITS"); + if (rdev->boot_limits == NULL) { + dev_err(&rdev->dev, "Unable to get boot limits handle\n"); + rdev->boot_limits = ERR_PTR(-EPERM); + return; + } + rdev->open_count++; + + if (regulator_enable(rdev->boot_limits)) { + dev_err(&rdev->dev, "Unable to set boot limits\n"); + destroy_regulator(rdev->boot_limits); + rdev->boot_limits = ERR_PTR(-EPERM); + } +} + +/** + * regulator_del_boot_limits - Remove boot limits for a regulator device. + * @rdev: regulator device to remove boot limits for. + * @handoff: Flag to control if the regulator should be handed off without a + * state change to the hardware. + * + * If boot limits has never been set up for this regulator, this function + * doesn't do anything and returns immediately. + * + * If boot limits have already been set up for the regulator device @rdev, this + * functions removes the boot limits so that operating state of @rdev can drop below + * the state in which the bootloader left it at before starting the kernel. + * + * In addition, if @handoff is NOT set, the function immediately updates the + * state of the regulator (potentially disabling it too). Otherwise, this + * function removes the boot limits and leaves the regulator state update to + * subsequent requests by other consumers. This is mainly useful for removing + * the boot limits when an exclusive consumer takes control of the regulator. + */ +static int regulator_del_boot_limits(struct regulator_dev *rdev, bool handoff) +{ + if (IS_ERR_OR_NULL(rdev->boot_limits)) + return 0; + + rdev_info(rdev, "removing boot limits\n"); + if (!handoff) + regulator_disable(rdev->boot_limits); + else + rdev->use_count--; + destroy_regulator(rdev->boot_limits); + /* + * Set it to an error value so that boot limits can't be set again once + * it has been removed. + */ + rdev->boot_limits = ERR_PTR(-EINVAL); + return 0; +} + +static int regulator_del_boot_limits_by_dev(struct device *dev, void *data) +{ + struct regulator_dev *rdev = dev_to_rdev(dev); + + if (dev->parent != data) + return 0; + + regulator_del_boot_limits(rdev, false); + return 0; +} + +void regulator_sync_state(struct device *dev) +{ + class_for_each_device(®ulator_class, NULL, dev, + regulator_del_boot_limits_by_dev); +} +EXPORT_SYMBOL_GPL(regulator_sync_state); + static int regulator_resolve_supply(struct regulator_dev *rdev) { struct regulator_dev *r; @@ -1827,6 +1936,10 @@ static int regulator_resolve_supply(struct regulator_dev *rdev) return ret; } + mutex_lock(®ulator_list_mutex); + regulator_add_boot_limits(rdev); + mutex_unlock(®ulator_list_mutex); + /* * In set_machine_constraints() we may have turned this regulator on * but we couldn't propagate to the supply if it hadn't been resolved @@ -1851,7 +1964,7 @@ struct regulator *_regulator_get(struct device *dev, const char *id, struct regulator_dev *rdev; struct regulator *regulator; struct device_link *link; - int ret; + int ret, open_count; if (get_type >= MAX_GET_TYPE) { dev_err(dev, "invalid type %d in %s\n", get_type, __func__); @@ -1908,7 +2021,15 @@ struct regulator *_regulator_get(struct device *dev, const char *id, return regulator; } - if (get_type == EXCLUSIVE_GET && rdev->open_count) { + /* + * Adjust for boot limits that'll be dropped if EXCLUSIVE_GET can + * succeed. + */ + open_count = rdev->open_count; + if (!IS_ERR_OR_NULL(rdev->boot_limits)) + open_count--; + + if (get_type == EXCLUSIVE_GET && open_count) { regulator = ERR_PTR(-EBUSY); put_device(&rdev->dev); return regulator; @@ -1947,6 +2068,9 @@ struct regulator *_regulator_get(struct device *dev, const char *id, rdev->open_count++; if (get_type == EXCLUSIVE_GET) { + /* Drop boot limits before exclusive consumer */ + regulator_del_boot_limits(rdev, true); + rdev->exclusive = 1; ret = _regulator_is_enabled(rdev); @@ -4896,7 +5020,9 @@ static void regulator_resolve_coupling(struct regulator_dev *rdev) regulator_unlock(c_rdev); regulator_resolve_coupling(c_rdev); + regulator_add_boot_limits(c_rdev); } + regulator_add_boot_limits(rdev); } static void regulator_remove_coupling(struct regulator_dev *rdev) @@ -5223,6 +5349,8 @@ regulator_register(const struct regulator_desc *regulator_desc, rdev_init_debugfs(rdev); + dev_set_drv_sync_state(rdev->dev.parent, regulator_sync_state); + /* try to resolve regulators coupling since a new one was registered */ mutex_lock(®ulator_list_mutex); regulator_resolve_coupling(rdev); @@ -5755,6 +5883,17 @@ static int regulator_late_cleanup(struct device *dev, void *data) return 0; } +static int regulator_boot_limits_timeout(struct device *dev, void *data) +{ + struct regulator_dev *rdev = dev_to_rdev(dev); + + regulator_del_boot_limits(rdev, false); + return 0; +} + +static unsigned int regulator_cleanup_timeout = 30000; +core_param(regulator_cleanup_timeout, regulator_cleanup_timeout, uint, 0); + static void regulator_init_complete_work_function(struct work_struct *work) { /* @@ -5767,6 +5906,16 @@ static void regulator_init_complete_work_function(struct work_struct *work) class_for_each_device(®ulator_class, NULL, NULL, regulator_register_resolve_supply); + /* + * If regulator_cleanup_timeout is set to a non-zero value, it probably + * means some of the consumers will never probe or the regulators have + * some restrictions on how long they can stay ON. So, don't wait + * forever for consumer devices to probe. + */ + if (regulator_cleanup_timeout) + class_for_each_device(®ulator_class, NULL, NULL, + regulator_boot_limits_timeout); + /* If we have a full configuration then disable any regulators * we have permission to change the status for and which are * not in use or always_on. This is effectively the default @@ -5802,7 +5951,7 @@ static int __init regulator_init_complete(void) * command line option might be useful. */ schedule_delayed_work(®ulator_init_complete_work, - msecs_to_jiffies(30000)); + msecs_to_jiffies(regulator_cleanup_timeout)); return 0; } diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h index 7eb9fea8e482..4761157a1528 100644 --- a/include/linux/regulator/driver.h +++ b/include/linux/regulator/driver.h @@ -451,6 +451,7 @@ struct regulator_dev { struct regulator *supply; /* for tree */ const char *supply_name; struct regmap *regmap; + struct regulator *boot_limits; struct delayed_work disable_work; @@ -475,6 +476,7 @@ devm_regulator_register(struct device *dev, const struct regulator_desc *regulator_desc, const struct regulator_config *config); void regulator_unregister(struct regulator_dev *rdev); +void regulator_sync_state(struct device *dev); void devm_regulator_unregister(struct device *dev, struct regulator_dev *rdev); int regulator_notifier_call_chain(struct regulator_dev *rdev, -- 2.28.0.rc0.105.gf9edc3c819-goog ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks 2020-07-16 4:20 ` [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks Saravana Kannan @ 2020-07-20 14:27 ` Mark Brown 2020-07-21 3:22 ` Saravana Kannan 0 siblings, 1 reply; 16+ messages in thread From: Mark Brown @ 2020-07-20 14:27 UTC (permalink / raw) To: Saravana Kannan Cc: Liam Girdwood, Greg Kroah-Hartman, Marek Szyprowski, John Stultz, linux-kernel, kernel-team [-- Attachment #1: Type: text/plain, Size: 2519 bytes --] On Wed, Jul 15, 2020 at 09:20:52PM -0700, Saravana Kannan wrote: > There are Android devices that exhibit the issue in the example where > regulator-X is an LDO, device-A is a camera device and device-B and > device-C are UFS and USB. To avoid this, they have their own downstream > changes to the regulator framework. Can you provide any references to these bodges? > 1. When a regulator is registered, > a. The sync_state() callback for the regulator's device is set to > regulator_sync_state(). The sync_state() callback is called when > all the consumers of the regulator's device have probed > successfully. As I indicated on my previous review this doesn't seem OK, given that a huge proportion of the regulators on most systems are part of a single PMIC this means that devices won't be able to fully control regulators for the majority of the boot process, possibly quite a long time after userspace has started in systems where not all devices have drivers. That seems unreasonably restrictive, it seems like this is shifting the problem around so we have this new very late init phase where we actually implement changes that devices asked for. Devices can work around it by using _get_exclusive() but that makes things more complex if they can also support non-exclusive use. I don't understand the motivation for doing things this way. Like I said last time it really feels like this turns the whole mechanism into a very complicated way of implementing a new initcall. > b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created > for the regulator and an enable vote is made. If something was left partially set up by the bootloader this means that drivers are no longer able to remove power from the device as part of getting it into a known good state even if they are the only consumer. Having to tune things in the bootloader isn't great for some development flows. > + /* > + * If regulator_cleanup_timeout is set to a non-zero value, it probably > + * means some of the consumers will never probe or the regulators have > + * some restrictions on how long they can stay ON. So, don't wait > + * forever for consumer devices to probe. > + */ > + if (regulator_cleanup_timeout) > + class_for_each_device(®ulator_class, NULL, NULL, > + regulator_boot_limits_timeout); It feels like this should be a negative value rather than zero, if the timeout is 0 someone might reasonably expect things to happen immediately rather than never. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks 2020-07-20 14:27 ` Mark Brown @ 2020-07-21 3:22 ` Saravana Kannan 2020-07-21 20:18 ` Mark Brown 0 siblings, 1 reply; 16+ messages in thread From: Saravana Kannan @ 2020-07-21 3:22 UTC (permalink / raw) To: Mark Brown Cc: Liam Girdwood, Greg Kroah-Hartman, Marek Szyprowski, John Stultz, LKML, Android Kernel Team On Mon, Jul 20, 2020 at 7:28 AM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jul 15, 2020 at 09:20:52PM -0700, Saravana Kannan wrote: > > > There are Android devices that exhibit the issue in the example where > > regulator-X is an LDO, device-A is a camera device and device-B and > > device-C are UFS and USB. To avoid this, they have their own downstream > > changes to the regulator framework. > > Can you provide any references to these bodges? This is the best I could dig up. The site is slow as molasses. I don't want to focus or critique any specific vendor's downstream code though. Providing these links just to prove that this is a real issue. Search for "proxy" here. You'll notice how that will also need changes to regulator header files, etc. The 4.9 kernel is the latest publicly available version AFAIK. https://source.codeaurora.org/quic/la/kernel/msm-4.9/plain/drivers/regulator/core.c?h=msm-4.9 As for the specific example of those devices, I'm not sure how much of that stuff is publicly available and I don't want to deal with finding that out. > > 1. When a regulator is registered, > > a. The sync_state() callback for the regulator's device is set to > > regulator_sync_state(). The sync_state() callback is called when > > all the consumers of the regulator's device have probed > > successfully. > > As I indicated on my previous review this doesn't seem OK, given that a > huge proportion of the regulators on most systems are part of a single > PMIC this means that devices won't be able to fully control regulators > for the majority of the boot process, possibly quite a long time after > userspace has started in systems where not all devices have drivers. By default regulator_cleanup_timeout is set to 30s. So by default, it's not going to be "possibly quite a long time". So, at worst, for the first 30 seconds voltages can't be lowered below boot level and regulators can't be turned off if they were left on by the boot loader. If a consumer is not an exclusive consumer, then none of this should break functional correctness for them and just be 30 seconds of suboptimal power. And exclusive consumers won't have this issue. I think the default behavior should be for functional correctness (example in the commit text) and then go for optimization (being able to power off regulators before 30s into boot). Even with the timeout set, this series makes it much easier for driver developers to ensure functional correctness instead of playing initcall chicken between the supplier and the N consumers. Actually on systems without all the drivers, I'd argue the correct behavior is this patch series + regulator_cleanup_timeout=-1. This patch series will prevent system instability/unusability (Eg: missing display backlight driver) at the cost of power optimization. However, to allow turning off boot on regulators in systems without all the drivers where it happens to not cause functional correctness issues, we have the timeout default to 30s. > That seems unreasonably restrictive, it seems like this is shifting the > problem around so we have this new very late init phase where we > actually implement changes that devices asked for. Devices can work > around it by using _get_exclusive() but that makes things more complex > if they can also support non-exclusive use. I agree, hacking around any issues using _get_exclusive() when they don't really need exclusive access is not a solution. > I don't understand the motivation for doing things this way. Like I > said last time it really feels like this turns the whole mechanism into > a very complicated way of implementing a new initcall. Treating this as a "LATER_initcall()" has several issues. 1. initcall levels set a max limit on the depth of device dependency. Since DT devices are added at arch initcall level, that gives us about 5 levels if you ignore the _sync ones. Adding more isn't going to scale or solve the problem because in reality, the dependencies are much deeper. 2. Also, initcall call ordering also depends on Makefile/link ordering. Which is fragile and error prone. 3. I know Greg hates playing initcall chicken. 4. "initcall levels" don't work for modules. The kernel initcall levels are done by the time modules are loaded. 5. "initcall levels" don't work with deferred probing. 6. If the answer is, "have userspace tell us when all modules are loaded" -- then we are depending on userspace for functional correctness AND for turning off regulators. Which IMHO is worse than this patch series. 7. If we somehow manage to add a "LATER_initcall" that doesn't have the issues above, it has to work for all frameworks. So, it has to come after ALL the devices in the system have probed. Not just "all devices of a supplier". So, again, it's worse than this patch series, at least for systems where all the drivers are present. > > b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created > > for the regulator and an enable vote is made. > > If something was left partially set up by the bootloader this means that > drivers are no longer able to remove power from the device as part of > getting it into a known good state even if they are the only consumer. If they really NEED to turn power off to get to a known good state, then they really need to be exclusive consumers. > Having to tune things in the bootloader isn't great for some development > flows. I'm not sure I fully understand what you'd need to tune in the bootloader. But independent of that, by default we still have the 30s timeout, so there's no boot loader tuning necessary. Just a 30s delay before being able to "optimize power" during development flow. I think this patch series actually helps a lot during the development phase (especially with regulator_cleanup_timeout=-1): 1. It will allow adding/enabling new drivers without worrying about the system crashing due to the example scenario mentioned in the commit text. 2. In my development flow, I had to do some stuff manually and then load some modules. Without this series, when some of the regulator drivers were built in, the regulators would get turned off after the 30s timeout before I could do my manual stuff. That would kill the system. Or if I start off the boot and walk away to get coffee, I'd come back to a dead device. It was super annoying to deal with this. 3. When the regulator drivers are loaded as modules (after 30s timeout), some of the boot on regulators are never turned off until one of their consumers starts making requests. For example, if a regulator that supplies some camera component is left on by the bootloader, it would never get turned off unless you open the camera app. With this series, the regulator would get turned off after the camera driver probes. To be clear, I understand the cases you are mentioning and I'm not discounting them. But compared to the 30 seconds of additional "on time", the functional correctness issues are more important. I'm not saying this series is the perfect solution, but it's certainly better than what we have now and we have the default behavior to be as least disruptive as possible to systems that work fine without this series. And if I find incremental improvements in the future, I'll send patches for that. But I'd hate to see perfect be the enemy of the good. > > + /* > > + * If regulator_cleanup_timeout is set to a non-zero value, it probably > > + * means some of the consumers will never probe or the regulators have > > + * some restrictions on how long they can stay ON. So, don't wait > > + * forever for consumer devices to probe. > > + */ > > + if (regulator_cleanup_timeout) > > + class_for_each_device(®ulator_class, NULL, NULL, > > + regulator_boot_limits_timeout); > > It feels like this should be a negative value rather than zero, if the > timeout is 0 someone might reasonably expect things to happen > immediately rather than never. Makes sense. I'll fix this. -Saravana ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks 2020-07-21 3:22 ` Saravana Kannan @ 2020-07-21 20:18 ` Mark Brown 2020-07-28 21:14 ` Saravana Kannan 0 siblings, 1 reply; 16+ messages in thread From: Mark Brown @ 2020-07-21 20:18 UTC (permalink / raw) To: Saravana Kannan Cc: Liam Girdwood, Greg Kroah-Hartman, Marek Szyprowski, John Stultz, LKML, Android Kernel Team [-- Attachment #1: Type: text/plain, Size: 10145 bytes --] On Mon, Jul 20, 2020 at 08:22:15PM -0700, Saravana Kannan wrote: > On Mon, Jul 20, 2020 at 7:28 AM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jul 15, 2020 at 09:20:52PM -0700, Saravana Kannan wrote: > > > There are Android devices that exhibit the issue in the example where > > > regulator-X is an LDO, device-A is a camera device and device-B and > > > device-C are UFS and USB. To avoid this, they have their own downstream > > > changes to the regulator framework. > > Can you provide any references to these bodges? > This is the best I could dig up. The site is slow as molasses. I don't > want to focus or critique any specific vendor's downstream code > though. Providing these links just to prove that this is a real issue. The issue is not that I can't see that there might be a problem. The issue I have is that you seem to be starting from the point of a specific solution that happens to work for your systems and then working back from that rather than reasoning forwards from the problem to come to a solution with the result that I can't explain the design you're proposing. Even with this version I have difficulty telling from the changelog that the change doesn't restrict things based on the consumers of a given regulator but rather based on the PMIC and I don't see why this is being done. > Search for "proxy" here. You'll notice how that will also need changes > to regulator header files, etc. The 4.9 kernel is the latest publicly > available version AFAIK. > https://source.codeaurora.org/quic/la/kernel/msm-4.9/plain/drivers/regulator/core.c?h=msm-4.9 Oh, the Qualcomm stuff - which looks a lot like what you've got here. > > As I indicated on my previous review this doesn't seem OK, given that a > > huge proportion of the regulators on most systems are part of a single > > PMIC this means that devices won't be able to fully control regulators > > for the majority of the boot process, possibly quite a long time after > > userspace has started in systems where not all devices have drivers. ... > I think the default behavior should be for functional correctness > (example in the commit text) and then go for optimization (being able > to power off regulators before 30s into boot). Even with the timeout > set, this series makes it much easier for driver developers to ensure > functional correctness instead of playing initcall chicken between the > supplier and the N consumers. I don't see how any of this stuff about initcall chicken or whatever is relevant here. You're replying to my concern that your change isn't just waiting for all the consumers of a given regulator, it's waiting for every other consumer of any other regulator or random other feature that happens to be supplied by the same PMIC but only that PMIC. That is not init ordering, it just seems like an arbitrary delay even once all the consumers are ready and I can't see any particular logic behind it. It's not just waiting for all the users of the individual resource to be active but it's also not waiting for the system as a whole, instead it's waiting for some effectively random grouping of devices spread over the whole system to appear. I can't articulate a reason why we'd do that, it seems to be combining the worst aspects of the two approaches. Please engage with this issue, it is crucial but you keep on sending the same thing without explaining why so I'm left guessing and I really can't come up with anything. > Actually on systems without all the drivers, I'd argue the correct > behavior is this patch series + regulator_cleanup_timeout=-1. This > patch series will prevent system instability/unusability (Eg: missing > display backlight driver) at the cost of power optimization. However, > to allow turning off boot on regulators in systems without all the > drivers where it happens to not cause functional correctness issues, > we have the timeout default to 30s. It's also preventing turning off regulators that don't have any consumers at all, not even potential ones. Those are the main target for the cleanup of idle regulators. > > I don't understand the motivation for doing things this way. Like I > > said last time it really feels like this turns the whole mechanism into > > a very complicated way of implementing a new initcall. > Treating this as a "LATER_initcall()" has several issues. > 1. initcall levels set a max limit on the depth of device dependency. > Since DT devices are added at arch initcall level, that gives us about > 5 levels if you ignore the _sync ones. Adding more isn't going to > scale or solve the problem because in reality, the dependencies are > much deeper. I don't follow what you're saying here at all, sorry. What does the depth of the dependency graph have to do with how late we action anything? A lot of what you're saying here seems to be based on jumping to abandoning deferred probe which is a bit of a leap here. > 6. If the answer is, "have userspace tell us when all modules are > loaded" -- then we are depending on userspace for functional > correctness AND for turning off regulators. Which IMHO is worse than > this patch series. > 7. If we somehow manage to add a "LATER_initcall" that doesn't have > the issues above, it has to work for all frameworks. So, it has to > come after ALL the devices in the system have probed. Not just "all > devices of a supplier". So, again, it's worse than this patch series, > at least for systems where all the drivers are present. Run the initcall when all devices in the system have a driver bound which as far as I can tell seems to be what the end result of this series is intended to be anyway, just implemented with much more complexity. That doesn't need any involvement from userspace, though I can't see why we'd want to do that rather than just work per regulator. > > > b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created > > > for the regulator and an enable vote is made. > > If something was left partially set up by the bootloader this means that > > drivers are no longer able to remove power from the device as part of > > getting it into a known good state even if they are the only consumer. > If they really NEED to turn power off to get to a known good state, > then they really need to be exclusive consumers. There's *need* to and there's can do on systems that can support the thing, the same driver will often be able to handle both situations. This also goes to a risk of regressions, it's a fairly major transformation to be doing. These issues would be massively mitigated if this were per regulator as I have repeatedly suggested. > 1. It will allow adding/enabling new drivers without worrying about > the system crashing due to the example scenario mentioned in the > commit text. Or just don't set constraints which alow things to be changed until you're ready... > 2. In my development flow, I had to do some stuff manually and then > load some modules. Without this series, when some of the regulator > drivers were built in, the regulators would get turned off after the > 30s timeout before I could do my manual stuff. That would kill the > system. Or if I start off the boot and walk away to get coffee, I'd > come back to a dead device. It was super annoying to deal with this. That's a new one, the normal thing would've been to let the driver load and then remove it to load whatever test stuff. I'm not sure that's super practical to be honest, taking advantage of it really does seem to require a vertically integrated system where the DT exactly matches what you want at runtime. > 3. When the regulator drivers are loaded as modules (after 30s > timeout), some of the boot on regulators are never turned off until > one of their consumers starts making requests. For example, if a > regulator that supplies some camera component is left on by the > bootloader, it would never get turned off unless you open the camera > app. With this series, the regulator would get turned off after the > camera driver probes. Right, that's an issue but there seem to be less invasive solutions. > To be clear, I understand the cases you are mentioning and I'm not > discounting them. But compared to the 30 seconds of additional "on > time", the functional correctness issues are more important. I'm not > saying this series is the perfect solution, but it's certainly better > than what we have now and we have the default behavior to be as least > disruptive as possible to systems that work fine without this series. > And if I find incremental improvements in the future, I'll send > patches for that. But I'd hate to see perfect be the enemy of the > good. Having the requests from drivers be acted on promptly is also a potential correctness and robustness issue. You seem very focused on the disabling of unused regulators for power reasons case but that's really not the only thing going on here, there's also just the need to control things as part of what the device is supposed to be doing. Systems aren't always built to let us do all the things we're really supposed to do with hardware so it's often advantageous to have drivers that will do these things but cope gracefully, this means that it doesn't always make sense to use exclusive gets even where you really actually want things to happen as requested if they can. Often the devices are in practice robust enough most of the time things are fine if the requests get ignored which is why systems can be built that way but strictly we're not supposed to do that so where we can we should avoid it. Forcing consumers to be exclusive means that we need to add a call that lets these consumers figure out which kind of get to use and then have the consumer have multiple code paths for that (or more realistically factor that out into the core, but then we could even more easily just not have the system wide restrictions). There's also issues around being able to reason about what the system is supposed to do and why, hence my concerns about waiting for all devices on the PMIC to appear. Like I said above this is absolutely crucial. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks 2020-07-21 20:18 ` Mark Brown @ 2020-07-28 21:14 ` Saravana Kannan 2020-08-04 21:10 ` Mark Brown 0 siblings, 1 reply; 16+ messages in thread From: Saravana Kannan @ 2020-07-28 21:14 UTC (permalink / raw) To: Mark Brown Cc: Liam Girdwood, Greg Kroah-Hartman, Marek Szyprowski, John Stultz, LKML, Android Kernel Team On Tue, Jul 21, 2020 at 1:18 PM Mark Brown <broonie@kernel.org> wrote: > Hi Mark, It *might* be easier if you jump to the bottom and read the reasoning for the current design. The stuff in between is just me trying to clarify some misunderstandings. > On Mon, Jul 20, 2020 at 08:22:15PM -0700, Saravana Kannan wrote: > > On Mon, Jul 20, 2020 at 7:28 AM Mark Brown <broonie@kernel.org> wrote: > > > On Wed, Jul 15, 2020 at 09:20:52PM -0700, Saravana Kannan wrote: > > > > > There are Android devices that exhibit the issue in the example where > > > > regulator-X is an LDO, device-A is a camera device and device-B and > > > > device-C are UFS and USB. To avoid this, they have their own downstream > > > > changes to the regulator framework. > > > > Can you provide any references to these bodges? > > > This is the best I could dig up. The site is slow as molasses. I don't > > want to focus or critique any specific vendor's downstream code > > though. Providing these links just to prove that this is a real issue. > > The issue is not that I can't see that there might be a problem. The > issue I have is that you seem to be starting from the point of a > specific solution that happens to work for your systems and then working > back from that rather than reasoning forwards from the problem to come > to a solution with the result that I can't explain the design you're > proposing. I'm sorry it comes across that way. That's certainly not my intention. Hopefully, we can walk through this together and I'll make more sense. > Even with this version I have difficulty telling from the > changelog that the change doesn't restrict things based on the consumers > of a given regulator but rather based on the PMIC and I don't see why > this is being done. Happy to copy-paste any commit text suggestions that make this clearer. > > Search for "proxy" here. You'll notice how that will also need changes > > to regulator header files, etc. The 4.9 kernel is the latest publicly > > available version AFAIK. > > https://source.codeaurora.org/quic/la/kernel/msm-4.9/plain/drivers/regulator/core.c?h=msm-4.9 > > Oh, the Qualcomm stuff - which looks a lot like what you've got here. Eh... I would disagree. The similarity stops at "both solutions use a regulator consumer to set the limits". The implementation you see there is based on QC specific DT entries, is based on late_initcall to "ensure" all consumers have probed, doesn't work for modules, etc. > > > As I indicated on my previous review this doesn't seem OK, given that a > > > huge proportion of the regulators on most systems are part of a single > > > PMIC this means that devices won't be able to fully control regulators > > > for the majority of the boot process, possibly quite a long time after > > > userspace has started in systems where not all devices have drivers. > > ... > > > I think the default behavior should be for functional correctness > > (example in the commit text) and then go for optimization (being able > > to power off regulators before 30s into boot). Even with the timeout > > set, this series makes it much easier for driver developers to ensure > > functional correctness instead of playing initcall chicken between the > > supplier and the N consumers. > > I don't see how any of this stuff about initcall chicken or whatever is > relevant here. You're replying to my concern that your change isn't > just waiting for all the consumers of a given regulator, it's waiting > for every other consumer of any other regulator or random other feature > that happens to be supplied by the same PMIC but only that PMIC. That > is not init ordering, it just seems like an arbitrary delay even once > all the consumers are ready and I can't see any particular logic behind > it. It's not just waiting for all the users of the individual resource > to be active but it's also not waiting for the system as a whole, > instead it's waiting for some effectively random grouping of devices > spread over the whole system to appear. I can't articulate a reason why > we'd do that, it seems to be combining the worst aspects of the two > approaches. > > Please engage with this issue, it is crucial but you keep on sending the > same thing without explaining why so I'm left guessing and I really > can't come up with anything. Ok, at the end of the email, I'll start from a high level and then dig into more details at each step. Hopefully that'll clarify the reasoning. [...] > > > I don't understand the motivation for doing things this way. Like I > > > said last time it really feels like this turns the whole mechanism into > > > a very complicated way of implementing a new initcall. > > > Treating this as a "LATER_initcall()" has several issues. > > 1. initcall levels set a max limit on the depth of device dependency. > > Since DT devices are added at arch initcall level, that gives us about > > 5 levels if you ignore the _sync ones. Adding more isn't going to > > scale or solve the problem because in reality, the dependencies are > > much deeper. > > I don't follow what you're saying here at all, sorry. What does the > depth of the dependency graph have to do with how late we action > anything? You keep raising the point "why not just add another initcall level" to solve the problem of "sync the suppliers after all the consumers are ready". Or you say that my patch series is just a roundabout way of adding another initcall level. I'm explaining why adding just another initcall level won't work. > A lot of what you're saying here seems to be based on jumping > to abandoning deferred probe which is a bit of a leap here. Again, definitely misunderstanding my point. I'm saying exactly the opposite. We need to take deferred probe into account and deferred probe completely invalidates any attempts to guarantee ordering based on initcall levels. I even mentioned this in point 5 in my earlier email. > > 6. If the answer is, "have userspace tell us when all modules are > > loaded" -- then we are depending on userspace for functional > > correctness AND for turning off regulators. Which IMHO is worse than > > this patch series. > > > 7. If we somehow manage to add a "LATER_initcall" that doesn't have > > the issues above, it has to work for all frameworks. So, it has to > > come after ALL the devices in the system have probed. Not just "all > > devices of a supplier". So, again, it's worse than this patch series, > > at least for systems where all the drivers are present. > > Run the initcall when all devices in the system have a driver bound And how is this going to work in systems where some devices don't have drivers? Even with one missing driver, none of the devices/drivers are going to get this initcall that you suggest. So it makes the granularity issue worse. > which as far as I can tell seems to be what the end result of this > series is intended to be anyway, just implemented with much more > complexity. No, with sync_state(), only suppliers to devices without drivers wouldn't get the callback. Rest of the suppliers will get the callback and can sync their state. > That doesn't need any involvement from userspace, though I > can't see why we'd want to do that rather than just work per regulator. > > > > > b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created > > > > for the regulator and an enable vote is made. > > > > If something was left partially set up by the bootloader this means that > > > drivers are no longer able to remove power from the device as part of > > > getting it into a known good state even if they are the only consumer. > > > If they really NEED to turn power off to get to a known good state, > > then they really need to be exclusive consumers. > > There's *need* to and there's can do on systems that can support the > thing, the same driver will often be able to handle both situations. > This also goes to a risk of regressions, it's a fairly major > transformation to be doing. These issues would be massively mitigated > if this were per regulator as I have repeatedly suggested. Even if you do per-regulator tracking, you can have one regulator shared by many consumers and you'll still hit this issue. So, going to a regulator level tracking doesn't solve this. [ ... ] > > To be clear, I understand the cases you are mentioning and I'm not > > discounting them. But compared to the 30 seconds of additional "on > > time", the functional correctness issues are more important. I'm not > > saying this series is the perfect solution, but it's certainly better > > than what we have now and we have the default behavior to be as least > > disruptive as possible to systems that work fine without this series. > > And if I find incremental improvements in the future, I'll send > > patches for that. But I'd hate to see perfect be the enemy of the > > good. > > Having the requests from drivers be acted on promptly is also a > potential correctness and robustness issue. You seem very focused on > the disabling of unused regulators for power reasons case The correctness issue I care about is not power consumption. It's about not pulling the plug on or under volting an active device before the driver can take control of it. > but that's > really not the only thing going on here, there's also just the need to > control things as part of what the device is supposed to be doing. > Systems aren't always built to let us do all the things we're really > supposed to do with hardware so it's often advantageous to have drivers > that will do these things but cope gracefully, this means that it > doesn't always make sense to use exclusive gets even where you really > actually want things to happen as requested if they can. Often the > devices are in practice robust enough most of the time things are fine > if the requests get ignored which is why systems can be built that way > but strictly we're not supposed to do that so where we can we should > avoid it. Forcing consumers to be exclusive means that we need to add a > call that lets these consumers figure out which kind of get to use and > then have the consumer have multiple code paths for that (or more > realistically factor that out into the core, but then we could even more > easily just not have the system wide restrictions). This paragraph seems to be self contradictory to me. I don't understand how you can claim the drivers handle it gracefully without exclusive_get(), but then still say they can't handle a regulator being on when they request for it to be off. They just need to handle it gracefully for the first 30s of boot with my patch series. Only when regulator_cleanup_timeout=-1 is set is there a potential for the regulator to be kept on forever. > There's also issues around being able to reason about what the system is > supposed to do and why, hence my concerns about waiting for all devices > on the PMIC to appear. Like I said above this is absolutely crucial. Ok, I'll try to explain the reasoning from the top. Firstly, the problem being solved is not specific to regulators. It's a general issue that needs to be solved for many frameworks/suppliers. We want a mechanism for a supplier driver to know when all the consumers are up and managing their resources so that the supplier driver can do a clean handoff from bootloader to the kernel. However, we don't want each framework doing very similar but slightly different things. That creates inconsistencies across frameworks, code repetition, maintenance headache, etc. In addition to that, it's also very inefficient for every framework to be trying to figure out the dependencies from firmware and tracking them separately. Even if you just try to answer "are all the consumers ready?" at a resource level (eg: at a regulator level), can't depend on get()/put() APIs provided by frameworks. Some of the reasons: 1. A consumer might do "get()" and then end up deferring further down the probe function. If that get() was the last get() you needed to decide "all consumers are ready", and you remove the boot limits, then the system could crash because the last consumer isn't actually ready. 2. Not all consumers do the "get()" in their probe functions. They might do it later for multiple reasons. 3. Not all properties in the firmware are actually used/supported by the driver. So, you can't assume all consumers referring to the resource will actually call get() on it. So, even if you track at a resource level, you need to check for the consumer readiness by looking at the device bound state. fw_devlink adds all the supplier-consumer dependencies by creating device links (currently only DT is supported). So, tracking consumer device readiness is easier. So, say you have a callback that you get for every single consumer binding successfully. What can you do there? For every consumer, you have to parse their firmware (Eg: DT node) to see what all resources they use (Eg: all the -supply properties) and increment the "consumer ref count" for those resources (Eg: regulators). And this needs to be done by every framework that manages suppliers. Ok, now you have a "number of consumers ready" refcount per resource. But to know if all the consumers of a resource are ready, you'll also need the total number of consumers per resource. How do you find that out? Whatever scheme you use to count the total number of consumers, you need to make sure you also take into account consumer devices that have not been added yet. As in device_add() hasn't been done. For example, child devices in DT that haven't been added yet because their parent hasn't probed. fw_devlink again helps avoid parsing the entire DT. It creates "proxy" device links between a supplier device and the parent device of a consumer if the consumer hasn't been added yet. So, when the "a consumer has probed" callback comes, apart from what you have to do already (tracking "number of consumers ready" at a resource level), you also need to parse all the descendant nodes (not just direct children) of that consumer's firmware node to check and potentially increment the "total consumers" at a per resource (Eg: regulator) level. Again, this will need to be done for every framework. And we still haven't dealt with the case where a consumer binds and unbinds. Say a resource (Eg: regulator) has N consumers. Say, only M (< N) consumers are ready/bound so far. Then one of the M consumers unbinds and then binds again. So, your "consumers ready" refcount per resource becomes M + 1 when only M consumers are ready. So, at a per resource level you have to now also track "already seen" devices or track device unbinding. And those "devices" themselves might vanish and come back again (Eg: DT overlays, the dwc driver that adds/removes child devices during probe, etc). I'm not even going to try and expand on how each framework would handle this -- it becomes very messy. Also, even if we do resource level tracking, it doesn't solve the problem of a consumer that expects regulator_disable() to turn off the regulator even when it's not using get_exclusive(). For example, the same regulator can have more than one consumer but the consumer that needs exclusive behavior just happens to probe first and has full control of the regulator. With boot limits applied at a regulator level, it'll still prevent the regulator from being turned off. So, long story short, tracking at a resource level: 1. Will be way more complicated than tracking at a consumer device level. 2. Will use a lot more memory. 3. Will use be a lot less efficient due to all the repeated parsing and tracking you'll have to do. 4. Doesn't really solve the "need exclusive but doesn't use get_exclusive()" consumer problem. But then why not just do a "boot done" initcall/callback instead of doing it based on supplier-consumers device level dependencies? "boot done" can be done in a couple of ways: 1. Have userspace tell that it's done loading all the modules. This is pretty much a no go as the kernel now depends on userspace for proper functionality. 2. Timer, but this is a configuration nightmare and doesn't work reliably. 3. Wait till every single device has been bound to a driver. The problem with this is that even if a single device is not bound to a driver, the entire system would be stuck in a "boot not done yet" mode. So any device left on by the boot loader would be permanently stuck on and can't go to states that are incompatible with their state at boot. So this is basically a cost (complexity) vs benefit (system vs device vs resource level removing of boot limits) problem. Device level granularity provides the best bang for the buck. Hopefully that clarifies the reasoning. Would you consider this series if the boot limit is only set when regulator_cleanup_timeout=-1 (as opposed to always)? Then the default behavior will stay identical to before this series and only when regulator_cleanup_timeout=-1 will any of this code kick in. -Saravana ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks 2020-07-28 21:14 ` Saravana Kannan @ 2020-08-04 21:10 ` Mark Brown 2020-08-25 19:58 ` Mark Brown 0 siblings, 1 reply; 16+ messages in thread From: Mark Brown @ 2020-08-04 21:10 UTC (permalink / raw) To: Saravana Kannan Cc: Liam Girdwood, Greg Kroah-Hartman, Marek Szyprowski, John Stultz, LKML, Android Kernel Team, Stephen Boyd [-- Attachment #1: Type: text/plain, Size: 16738 bytes --] On Tue, Jul 28, 2020 at 02:14:52PM -0700, Saravana Kannan wrote: > On Tue, Jul 21, 2020 at 1:18 PM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jul 20, 2020 at 08:22:15PM -0700, Saravana Kannan wrote: Copying in Stephen having done a bunch of mailing list archive trawling on the fw_devlink stuff since I'm re-raising some stuff he raised before as he happened to see that stuff before it got merged - most of that stuff is a way down in the second half of this far too long e-mail. > > Even with this version I have difficulty telling from the > > changelog that the change doesn't restrict things based on the consumers > > of a given regulator but rather based on the PMIC and I don't see why > > this is being done. > Happy to copy-paste any commit text suggestions that make this clearer. Well, like I keep saying I don't know how to explain the implementation decisions here. Simply openly stating that you do this would be a start, things like being explicit about which devices you are talking about for example - if you mean "the MFD associated with the regulator" say that, when you talk about the "regulator's device" that could easily be assumed to mean the struct device for the specific regulator. > > I don't follow what you're saying here at all, sorry. What does the > > depth of the dependency graph have to do with how late we action > > anything? > You keep raising the point "why not just add another initcall level" > to solve the problem of "sync the suppliers after all the consumers > are ready". Or you say that my patch series is just a roundabout way > of adding another initcall level. I'm explaining why adding just > another initcall level won't work. I am saying that this winds up being roughly the same thing in effect just with less clarity and more complexity so therefore consider applying the same workarounds for problems here as we do for your current code. Wait for devices to bind like your current code does, use a timer in case some of them don't bind like your current code does. > > Run the initcall when all devices in the system have a driver bound > And how is this going to work in systems where some devices don't have > drivers? Even with one missing driver, none of the devices/drivers are > going to get this initcall that you suggest. So it makes the > granularity issue worse. I don't think either of these things is a particularly great idea, it's just that doing things per system doesn't have materially different effects for regulators on most systems and I can at least explain when things happen more clearly and there'd be less random variation in behaviour in the less common cases. > > which as far as I can tell seems to be what the end result of this > > series is intended to be anyway, just implemented with much more > > complexity. > No, with sync_state(), only suppliers to devices without drivers > wouldn't get the callback. Rest of the suppliers will get the callback > and can sync their state. But with the timeout which we would need to have by default everything will get the callback anyway. > > > If they really NEED to turn power off to get to a known good state, > > > then they really need to be exclusive consumers. > > There's *need* to and there's can do on systems that can support the > > thing, the same driver will often be able to handle both situations. > > This also goes to a risk of regressions, it's a fairly major > > transformation to be doing. These issues would be massively mitigated > > if this were per regulator as I have repeatedly suggested. > Even if you do per-regulator tracking, you can have one regulator > shared by many consumers and you'll still hit this issue. So, going to > a regulator level tracking doesn't solve this. It solves it for supplies that aren't actually shared without requiring consumers to have separate code paths that figure out if they're using shared supplies or not which if nothing else is effort for existing drivers. This would be a much less surprising implementation decision in general, and much more likely to do what people want in a system. > > Having the requests from drivers be acted on promptly is also a > > potential correctness and robustness issue. You seem very focused on > > the disabling of unused regulators for power reasons case > The correctness issue I care about is not power consumption. It's > about not pulling the plug on or under volting an active device before > the driver can take control of it. Your comments suggest that you seem to think this is why devices would be controlling regulators. > > avoid it. Forcing consumers to be exclusive means that we need to add a > > call that lets these consumers figure out which kind of get to use and > > then have the consumer have multiple code paths for that (or more > > realistically factor that out into the core, but then we could even more > > easily just not have the system wide restrictions). > This paragraph seems to be self contradictory to me. I don't > understand how you can claim the drivers handle it gracefully without > exclusive_get(), but then still say they can't handle a regulator > being on when they request for it to be off. They just need to handle > it gracefully for the first 30s of boot with my patch series. Only > when regulator_cleanup_timeout=-1 is set is there a potential for the > regulator to be kept on forever. Remember that the same device and driver can be used in different systems with different configurations and integrations. While some systems might be set up so that the regulator is unshared, can be freely controlled and even rely on that happening there may be other systems that not have software controllable regulators at all and are instead set up so the regulators don't need to be controlled. Also think about all the times you see software doing things that happen to work but probably aren't robust or a good idea, and how we sometimes have to put support code in there to keep them working when they do break. Electrical engineering and general system design can be like that too. As a really simple example a hardware designer might decide they can save a GPIO by not wiring up a reset signal since the device has a dedicated power supply and software can just turn that off and on to trigger a power on reset instead. Other systems with the same device might give software control of the reset GPIO but not the power and assume that it will assert reset that way. > > There's also issues around being able to reason about what the system is > > supposed to do and why, hence my concerns about waiting for all devices > > on the PMIC to appear. Like I said above this is absolutely crucial. > Ok, I'll try to explain the reasoning from the top. > Firstly, the problem being solved is not specific to regulators. It's > a general issue that needs to be solved for many frameworks/suppliers. > We want a mechanism for a supplier driver to know when all the > consumers are up and managing their resources so that the supplier > driver can do a clean handoff from bootloader to the kernel. > However, we don't want each framework doing very similar but slightly > different things. That creates inconsistencies across frameworks, code > repetition, maintenance headache, etc. In addition to that, it's also > very inefficient for every framework to be trying to figure out the > dependencies from firmware and tracking them separately. There seem to be some very substantial assumptions in here. The biggest is that we can't share code if there's any subsystem knowledge of what's going on, this is already impacting every affected subsystem with your current implementation anyway so it's not like it's transparent to them but you're still managing to share code. It's also not 100% clear to me that we want every framework to behave in exactly the same way - resources obviously aren't completely fungible. For example while it's very common for regulators to be shared that's not the case for GPIOs and that has an impact on what drivers should be expected to handle. Consistency is obviously good but it's not the only thing to consider. To me this doesn't explain why we would want the system to behave in this way, it explains how some implementation details flow from other implementation details. At a very high level there seems to be an unstated assumption in here about existing code outside the regulator framework (mainly fw_devlink I think) being unmodifiable which really should not be the case. > Even if you just try to answer "are all the consumers ready?" at a > resource level (eg: at a regulator level), can't depend on get()/put() > APIs provided by frameworks. Some of the reasons: Why would we need to or even want to depend on or use get() and put() here? That doesn't seem at all obvious to me. We clearly have the ability to relate individual resources to their consumer devices for every subsystem that's affected here and clearly also already have the ability to make links between consumer and supplier devices prior to them probing. It shouldn't be impossible to take these two things and combine them in a way that keeps the tracking and notification code shared. In the case of the regulator API we already create a struct device and have an of_node per regulator so it should be even easier to use the existing device based stuff than with subsystems that don't have that, it should at most be some extra code to move some links around (or add new ones if that's easier). Off the top of my head a call to move any misdirected links to a given of_node to a new struct device (or add new ones leaving the old ones in place I guess) should do the trick for regulators without even modifying the sync_state() callback stuff, just the link setup. It seems like the framework is already pretty much ideally set up for this, you could even make a case that the reason there's not already per-resource links for regulators is that the existing code is not doing a good job handling the DT bindings and is misdirecting links to the wrong struct device. > So, say you have a callback that you get for every single consumer > binding successfully. What can you do there? For every consumer, you > have to parse their firmware (Eg: DT node) to see what all resources > they use (Eg: all the -supply properties) Again off the top of my head we could also do this the other way around and when making a link go and ask the subsystems if it's their link and they have a better idea about where to put it. Actually, having found the code that adds the links we don't even need to ask the subsystems if it's their link - we already know at the time we're doing the parsing which subsystem a link relates to! Perhaps we could do some of this checking/notification at the time links are connected/satisfied rather than at parse time, or perhaps when the suppliers register they could look at outgoing links. We already have a set of links and we already have the ability to figure out which resources they're talking about, we just need to join those two things up which shouldn't be an intractable problem. > and increment the "consumer > ref count" for those resources (Eg: regulators). And this needs to be > done by every framework that manages suppliers. With your current code every framework that manages resources that can be shared already needs to be modified to block actions until there's a sync_state() callback, modifying frameworks is clearly not an issue here. It even turns out that we already have some custom parsing code (which shares code!) for links for individual subsystems including regulators, unfortunately I'd not seen the regulator code until today since while it was merged back in October it's in the of code rather than the regulator code and was never copied to me (you were asked to do so at the time to by Stephen Boyd, sadly the code got merged at the version he was reviewing so there wasn't another post). Most of that is data driven but not all. I also don't see why we'd need to end up open coding reference counting, if we have code for tracking links and references let's use it, extending it where we need to. > fw_devlink again helps avoid parsing the entire DT. It creates "proxy" > device links between a supplier device and the parent device of a > consumer if the consumer hasn't been added yet. fw_devlink is something we can modify and extend, we don't have to completely discard it to do better than what's currently being done. It feels like a lot of your analysis here is predicated on not being able to change anything with that code, but obviously that reasoning gets a bit circular. At the minute it definitely seems to have some assumptions in there about the encoding of the Linux device model into DT which ought to be addressed, they seem in some fundamental way to be at the root of my concerns. > So, long story short, tracking at a resource level: > 1. Will be way more complicated than tracking at a consumer device level. > 2. Will use a lot more memory. > 3. Will use be a lot less efficient due to all the repeated parsing > and tracking you'll have to do. I don't think these things need to be the case, I think we can do much better with code reuse than you are anticipating and achieve similar performance through that reuse. > 4. Doesn't really solve the "need exclusive but doesn't use > get_exclusive()" consumer problem. It definitely does in for example the case where a regulator is intended to be actively managed and is as a result unshared in a particular system. > But then why not just do a "boot done" initcall/callback instead of > doing it based on supplier-consumers device level dependencies? > "boot done" can be done in a couple of ways: > 1. Have userspace tell that it's done loading all the modules. This is > pretty much a no go as the kernel now depends on userspace for proper > functionality. > 2. Timer, but this is a configuration nightmare and doesn't work reliably. > 3. Wait till every single device has been bound to a driver. The > problem with this is that even if a single device is not bound to a > driver, the entire system would be stuck in a "boot not done yet" > mode. So any device left on by the boot loader would be permanently > stuck on and can't go to states that are incompatible with their state > at boot. I agree that doing things per system is not great but like I keep saying at least for regulators doing things based on the MFD is at best very close to the same thing given that in most systems the vast majority of controllable supplies are provided by a single PMIC. This is why I push for handling things per supply, it's much more what someone would expect the system to do and much more likely to just do the right thing. I'm trying to understand the underlying goal of this design decision and come up with something that might actually deliver that reliably. One idea which was mentioned by Stephen was issues with hardware controlled by multiple devices (eg, a display controller and a GPU) - that might point towards doing something per system, though perhaps not since we hopefully only need the display controller in that case until userspace has come up and started drawing new things and obviously if that's needed then waiting for drivers to probe isn't particularly relevant so we'd not be gaining much. I do think we need a timeout no matter what for something like this, even with doing things per resource - users can configure them off if they prefer that but there's definitely people who are going to end up on systems with devices that don't have drivers instantiate. > Would you consider this series if the boot limit is only set when > regulator_cleanup_timeout=-1 (as opposed to always)? Then the default > behavior will stay identical to before this series and only when > regulator_cleanup_timeout=-1 will any of this code kick in. I'm having a really hard time seeing that as a good idea, merging things that work for some specific systems but don't scale has lead to ongoing problems elsewhere and I don't want to repeat that mistake. If it was a case where we had a solid, well understood design which needed some additional work to extend it to cover some cases then that sort of thing could make sense but currently the basic design is still unclear. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks 2020-08-04 21:10 ` Mark Brown @ 2020-08-25 19:58 ` Mark Brown 0 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2020-08-25 19:58 UTC (permalink / raw) To: Saravana Kannan Cc: Liam Girdwood, Greg Kroah-Hartman, Marek Szyprowski, John Stultz, LKML, Android Kernel Team, Stephen Boyd [-- Attachment #1: Type: text/plain, Size: 3100 bytes --] On Tue, Aug 04, 2020 at 10:10:49PM +0100, Mark Brown wrote: > On Tue, Jul 28, 2020 at 02:14:52PM -0700, Saravana Kannan wrote: > > So, say you have a callback that you get for every single consumer > > binding successfully. What can you do there? For every consumer, you > > have to parse their firmware (Eg: DT node) to see what all resources > > they use (Eg: all the -supply properties) > Again off the top of my head we could also do this the other way around > and when making a link go and ask the subsystems if it's their link and > they have a better idea about where to put it. Actually, having found > the code that adds the links we don't even need to ask the subsystems if > it's their link - we already know at the time we're doing the parsing > which subsystem a link relates to! Perhaps we could do some of this > checking/notification at the time links are connected/satisfied rather > than at parse time, or perhaps when the suppliers register they could > look at outgoing links. > We already have a set of links and we already have the ability to figure > out which resources they're talking about, we just need to join those > two things up which shouldn't be an intractable problem. So, having taken a look at the device_link stuff in the driver core it seems like it should be easy enough to add another parameter to device_link_add() or a variant thereof so we can get a supplier ID of some kind to the core (eg, a callback plus ID) along with the link so I don't see any issue with getting the data in there. We then need to figure out how to use that in device_links_driver_bound(), that is currently unconditionally kicking _queue_sync_state() for every supplier to go check if we need to do the sync_state() so would seem to be the logical place to schedule a per subsystem callback. It also deletes the link if it was a _SYNC_STATE link which does make things a bit more fun but we can always remember which link we're deleting and pass that on when scheduling the kick. Indeed, it occurs to me that we could be cute here and in _queue_sync_state() have it check while scanning the consumer links to see if we find a consumer with the same subsystem callback information and if we don't then that must've been the last link that just got deleted and we can call the callback. That's not quite everything, in particular at least for any subsystem where the core can be modular you'd need to have a layer of indirection for the callback (it's possibly a good idea to do that anyway), but I'm not seeing anything new with regard to locking or whatever. It looks like the work already done for basic sync_state() should be reusable unless there's some nasty gotchas I'm not seeing, that was pretty much what I was expecting to see TBH. BTW doesn't the fact that we throw away the _SYNC_STATE_ONLY links cause fun if the provider driver gets unbound then rebound? We don't reparse the DT to re-add those links. I'm also not seeing where we ever clear the state_synced flag that gets set which seems like it'd break things if a supplier gets removed and reprobed. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 4/4] regulator: core: Add voltage support for sync_state() callbacks 2020-07-16 4:20 [PATCH v3 0/4] regulator_sync_state() support Saravana Kannan ` (2 preceding siblings ...) 2020-07-16 4:20 ` [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks Saravana Kannan @ 2020-07-16 4:20 ` Saravana Kannan 2020-07-20 14:35 ` Mark Brown 2020-07-21 3:29 ` [PATCH v3 0/4] regulator_sync_state() support Saravana Kannan 2020-07-22 0:57 ` Mark Brown 5 siblings, 1 reply; 16+ messages in thread From: Saravana Kannan @ 2020-07-16 4:20 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, Greg Kroah-Hartman Cc: Saravana Kannan, Marek Szyprowski, John Stultz, linux-kernel, kernel-team Consider the following example: - regulator-X is provided by device-X. - regulator-X is a supplier to device-A, device-B and device-C. - device-A is off/inactive from boot. - device-B and device-C are left on/active by the bootloader - regulator-X is left on boot by the bootloader at 2000 mV to supply device-B and device-C. Example boot sequence: 1. device-X is probed successfully. 2. device-B is probed by driver-B a. driver-B gets regulator-X b. driver-B votes on regulator-X c. driver-B lowers device-B performance point. d. driver-B lowers voltage vote to 1000 mV. e. regulator-X voltage is lowered to 1000 mV. 3. System crashes or device-C becomes unreliable because regulator-X voltage was lowered to 1000 mV when device-C still needed it at 2000 mV The issue reported by Marek Szyprowski [1] between vdd_int and vdd_arm is very similar to example 2, except driver-B lowers the voltage of device-C due to a regulator coupling instead of a direct request from driver-B. This patch addresses the problem in the example by: 1. When a boot-on regulator is registered, a minimum voltage limit that matches the boot time voltage is placed on the regulator. 2. When the regulator_sync_state() callback comes, the minimum voltage limit is removed along with the rest of the boot limits. So with this patch and regulator_cleanup_timeout=0, the example will work as follows: 1. device-X is probed successfully. a. regulator-X is registered. b. Since regulator-X is on, a minimum voltage of 2000 mV is made by the BOOT-LIMITS consumer. c. Since regulator-X is on, an enable vote is made by the BOOT-LIMITS consumer. 2. device-B is probed by driver-B a. driver-B gets regulator-X b. driver-B votes on regulator-X c. driver-B lowers device-B performance point. d. driver-B lowers voltage vote to 1000 mV. e. regulator-X voltage is NOT lowered to 1000 mV. 3. device-C is probed by driver-C a. Does stuff similar to device-B. 4. Since all consumers of device-X have probed, device-X gets sync_state() callback which is a call to regulator_sync_state(): a. BOOT-LIMITS removes enable vote for regulator-X b. regulator-X remains on becaue device-B and device-C have their enable votes in. c. BOOT-LIMITS consumer is freed. d. regulator-X voltage drops is lowered to 1000 mV. 5. The system is stable because regulator-X voltage is NOT lowered to 1000 mV when device-C still needed it at 2000 mV. [1] - https://lore.kernel.org/lkml/20200605063724.9030-1-m.szyprowski@samsung.com/#t Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/regulator/core.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index f10ef6ec1af1..9b70295820f3 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1783,6 +1783,8 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev, */ static void regulator_add_boot_limits(struct regulator_dev *rdev) { + int boot_uV; + /* We already set up boot limits. */ if (rdev->boot_limits) return; @@ -1815,6 +1817,13 @@ static void regulator_add_boot_limits(struct regulator_dev *rdev) } rdev->open_count++; + if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) { + boot_uV = regulator_get_voltage_rdev(rdev); + if (boot_uV > 0) + regulator_set_voltage(rdev->boot_limits, boot_uV, + INT_MAX); + } + if (regulator_enable(rdev->boot_limits)) { dev_err(&rdev->dev, "Unable to set boot limits\n"); destroy_regulator(rdev->boot_limits); @@ -1847,10 +1856,12 @@ static int regulator_del_boot_limits(struct regulator_dev *rdev, bool handoff) return 0; rdev_info(rdev, "removing boot limits\n"); - if (!handoff) + if (!handoff) { regulator_disable(rdev->boot_limits); - else + regulator_set_voltage(rdev->boot_limits, 0, INT_MAX); + } else { rdev->use_count--; + } destroy_regulator(rdev->boot_limits); /* * Set it to an error value so that boot limits can't be set again once -- 2.28.0.rc0.105.gf9edc3c819-goog ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 4/4] regulator: core: Add voltage support for sync_state() callbacks 2020-07-16 4:20 ` [PATCH v3 4/4] regulator: core: Add voltage " Saravana Kannan @ 2020-07-20 14:35 ` Mark Brown 2020-07-21 3:24 ` Saravana Kannan 0 siblings, 1 reply; 16+ messages in thread From: Mark Brown @ 2020-07-20 14:35 UTC (permalink / raw) To: Saravana Kannan Cc: Liam Girdwood, Greg Kroah-Hartman, Marek Szyprowski, John Stultz, linux-kernel, kernel-team [-- Attachment #1: Type: text/plain, Size: 478 bytes --] On Wed, Jul 15, 2020 at 09:20:53PM -0700, Saravana Kannan wrote: > - if (!handoff) > + if (!handoff) { > regulator_disable(rdev->boot_limits); > - else > + regulator_set_voltage(rdev->boot_limits, 0, INT_MAX); > + } else { > rdev->use_count--; > + } > destroy_regulator(rdev->boot_limits); These sets should be completely redundant since they will go away when the regulator is destroyed, if there's an issue with that we should fix it rather than bodging around it. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 4/4] regulator: core: Add voltage support for sync_state() callbacks 2020-07-20 14:35 ` Mark Brown @ 2020-07-21 3:24 ` Saravana Kannan 2020-07-21 12:19 ` Mark Brown 0 siblings, 1 reply; 16+ messages in thread From: Saravana Kannan @ 2020-07-21 3:24 UTC (permalink / raw) To: Mark Brown Cc: Liam Girdwood, Greg Kroah-Hartman, Marek Szyprowski, John Stultz, LKML, Android Kernel Team On Mon, Jul 20, 2020 at 7:35 AM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Jul 15, 2020 at 09:20:53PM -0700, Saravana Kannan wrote: > > > - if (!handoff) > > + if (!handoff) { > > regulator_disable(rdev->boot_limits); > > - else > > + regulator_set_voltage(rdev->boot_limits, 0, INT_MAX); > > + } else { > > rdev->use_count--; > > + } > > destroy_regulator(rdev->boot_limits); > > These sets should be completely redundant since they will go away when > the regulator is destroyed, if there's an issue with that we should fix > it rather than bodging around it. Yeah, I was aware of this, but I thought it was clearer to have an explicit unwinding. Since you prefer the other way around, I can drop the set voltage. Btw, going a tangent, why is regulator_set_voltage() not dependent on a consumer's regulator enable request? If they don't care if the regulator goes off, do they really care if the voltage goes lower? What's the reason behind not tying voltage request with the enable request? -Saravana ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 4/4] regulator: core: Add voltage support for sync_state() callbacks 2020-07-21 3:24 ` Saravana Kannan @ 2020-07-21 12:19 ` Mark Brown 0 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2020-07-21 12:19 UTC (permalink / raw) To: Saravana Kannan Cc: Liam Girdwood, Greg Kroah-Hartman, Marek Szyprowski, John Stultz, LKML, Android Kernel Team [-- Attachment #1: Type: text/plain, Size: 502 bytes --] On Mon, Jul 20, 2020 at 08:24:59PM -0700, Saravana Kannan wrote: > Btw, going a tangent, why is regulator_set_voltage() not dependent on > a consumer's regulator enable request? If they don't care if the > regulator goes off, do they really care if the voltage goes lower? > What's the reason behind not tying voltage request with the enable > request? The most important thing is being able to control the conditions at power on and power off, the period while the power is off is not so important. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/4] regulator_sync_state() support 2020-07-16 4:20 [PATCH v3 0/4] regulator_sync_state() support Saravana Kannan ` (3 preceding siblings ...) 2020-07-16 4:20 ` [PATCH v3 4/4] regulator: core: Add voltage " Saravana Kannan @ 2020-07-21 3:29 ` Saravana Kannan 2020-07-22 0:57 ` Mark Brown 5 siblings, 0 replies; 16+ messages in thread From: Saravana Kannan @ 2020-07-21 3:29 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, Greg Kroah-Hartman Cc: Marek Szyprowski, John Stultz, LKML, Android Kernel Team On Wed, Jul 15, 2020 at 9:20 PM Saravana Kannan <saravanak@google.com> wrote: > > Consider the following example: > - regulator-X is provided by device-X. > - regulator-X is a supplier to device-A, device-B and device-C. > - device-A is off/inactive from boot. > - device-B and device-C are left on/active by the bootloader > - regulator-X is left on boot by the bootloader at 2000 mV to supply > device-B and device-C. > > Example boot sequence 1: > 1. device-X is probed successfully. > 2. device-A is probed by driver-A > a. driver-A gets regulator-X > b. driver-A votes on regulator-X > c. driver-A initializes device-A > d. driver-A votes off regulator-X > e. regulator-X is turned off. > 3. System crashes or device-B and device-C become unreliable because > regulator-X was turned off without following the proper quiescing > steps for device-B and device-C. > > Example boot sequence 2: > 1. device-X is probed successfully. > 2. device-B is probed by driver-B > a. driver-B gets regulator-X > b. driver-B votes on regulator-X > c. driver-B lowers device-B performance point. > d. driver-B lowers voltage vote to 1000 mV. > e. regulator-X voltage is lowered to 1000 mV. > 3. System crashes or device-C becomes unreliable because regulator-X > voltage was lowered to 1000 mV when device-C still needed it at 2000 mV Marek, I'd appreciate it if you could test this series to see if it fixes your coupled regulator issue that you reported earlier. I'm expecting it to fix it without needing any special case handling in your regulator drivers. But it'll be good to get confirmation from you. Thanks, Saravana ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/4] regulator_sync_state() support 2020-07-16 4:20 [PATCH v3 0/4] regulator_sync_state() support Saravana Kannan ` (4 preceding siblings ...) 2020-07-21 3:29 ` [PATCH v3 0/4] regulator_sync_state() support Saravana Kannan @ 2020-07-22 0:57 ` Mark Brown 5 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2020-07-22 0:57 UTC (permalink / raw) To: Liam Girdwood, Saravana Kannan, Greg Kroah-Hartman Cc: John Stultz, Marek Szyprowski, linux-kernel, kernel-team On Wed, 15 Jul 2020 21:20:49 -0700, Saravana Kannan wrote: > Consider the following example: > - regulator-X is provided by device-X. > - regulator-X is a supplier to device-A, device-B and device-C. > - device-A is off/inactive from boot. > - device-B and device-C are left on/active by the bootloader > - regulator-X is left on boot by the bootloader at 2000 mV to supply > device-B and device-C. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [1/1] regulator: core: Add destroy_regulator() commit: e1794aa43f17bf2512c10370c6be6ea24a6f29d0 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-08-25 19:58 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-16 4:20 [PATCH v3 0/4] regulator_sync_state() support Saravana Kannan 2020-07-16 4:20 ` [PATCH v3 1/4] driver core: Add dev_set_drv_sync_state() Saravana Kannan 2020-07-16 4:20 ` [PATCH v3 2/4] regulator: core: Add destroy_regulator() Saravana Kannan 2020-07-16 4:20 ` [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks Saravana Kannan 2020-07-20 14:27 ` Mark Brown 2020-07-21 3:22 ` Saravana Kannan 2020-07-21 20:18 ` Mark Brown 2020-07-28 21:14 ` Saravana Kannan 2020-08-04 21:10 ` Mark Brown 2020-08-25 19:58 ` Mark Brown 2020-07-16 4:20 ` [PATCH v3 4/4] regulator: core: Add voltage " Saravana Kannan 2020-07-20 14:35 ` Mark Brown 2020-07-21 3:24 ` Saravana Kannan 2020-07-21 12:19 ` Mark Brown 2020-07-21 3:29 ` [PATCH v3 0/4] regulator_sync_state() support Saravana Kannan 2020-07-22 0:57 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).