Because the uA_load propagation change takes out a lock on a regulator and
then the regulators that it supplies, it will cause deadlock with the current
regulator_enable implementation. regulator_disable can also deadlock with
regulator_enable, but it requires two threads and precise timing to observe.
Therefore, regulator_enable must be fixed before current propagation can be
used.
David Collins (2):
regulator: Remove possible deadlock from regulator_enable
regulator: Propagate uA_load requirements up supply chain
drivers/regulator/core.c | 147 +++++++++++++++++++++++++++++++-------
include/linux/regulator/driver.h | 5 ++
2 files changed, 125 insertions(+), 27 deletions(-)
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Assume a regulator supply chain in which S1 supplies L2 (S1 --> L2).
It is possible to achieve deadlock using two threads if one thread
calls regulator_enable on a regulator L2 while the other thread calls
regulator_disable on S1. This happens because _regulator_enable(L2)
holds a lock for L2 and then takes out a lock on S1.
_regulator_disable(S1) on the other hand, is called with a lock taken
for S1 and then it calls _notifier_call_chain which attempts to take a
lock for L2. Because these locks are taken in opposite orders, deadlock
will result.
Change regulator_enable and _regulator_enable so that only one lock is
held at a time to avoid this deadlock scenario.
Signed-off-by: David Collins <[email protected]>
---
drivers/regulator/core.c | 78 +++++++++++++++++++++++++++++++---------------
1 files changed, 53 insertions(+), 25 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9fa2095..425beba 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1279,38 +1279,54 @@ static int _regulator_can_change_status(struct regulator_dev *rdev)
return 0;
}
-/* locks held by regulator_enable() */
+/* Locks are *not* held by regulator_enable(). */
static int _regulator_enable(struct regulator_dev *rdev)
{
- int ret, delay;
+ struct regulator_dev *supply_rdev = NULL;
+ int ret = 0, delay;
+ mutex_lock(&rdev->mutex);
if (rdev->use_count == 0) {
+ /*
+ * Incrementing here removes a race condition for simultaneous
+ * regulator_enable calls with use_count == 0.
+ */
+ rdev->use_count++;
+ mutex_unlock(&rdev->mutex);
+
/* do we need to enable the supply regulator first */
if (rdev->supply) {
- mutex_lock(&rdev->supply->mutex);
ret = _regulator_enable(rdev->supply);
- mutex_unlock(&rdev->supply->mutex);
if (ret < 0) {
rdev_err(rdev, "failed to enable: %d\n", ret);
+ mutex_lock(&rdev->mutex);
+ rdev->use_count--;
+ mutex_unlock(&rdev->mutex);
return ret;
}
+ supply_rdev = rdev->supply;
}
- }
- /* check voltage and requested load before enabling */
- if (rdev->constraints &&
- (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
- drms_uA_update(rdev);
+ mutex_lock(&rdev->mutex);
+ rdev->use_count--;
+
+ /* check voltage and requested load before enabling */
+ if (rdev->constraints &&
+ (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
+ drms_uA_update(rdev);
- if (rdev->use_count == 0) {
- /* The regulator may on if it's not switchable or left on */
+ /* The regulator may be on if it's not switchable or left on */
ret = _regulator_is_enabled(rdev);
if (ret == -EINVAL || ret == 0) {
- if (!_regulator_can_change_status(rdev))
- return -EPERM;
+ if (!_regulator_can_change_status(rdev)) {
+ ret = -EPERM;
+ goto out;
+ }
- if (!rdev->desc->ops->enable)
- return -EINVAL;
+ if (!rdev->desc->ops->enable) {
+ ret = -EINVAL;
+ goto out;
+ }
/* Query before enabling in case configuration
* dependant. */
@@ -1330,7 +1346,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
* regulators can ramp together. */
ret = rdev->desc->ops->enable(rdev);
if (ret < 0)
- return ret;
+ goto out;
trace_regulator_enable_delay(rdev_get_name(rdev));
@@ -1345,14 +1361,32 @@ static int _regulator_enable(struct regulator_dev *rdev)
} else if (ret < 0) {
rdev_err(rdev, "is_enabled() failed: %d\n", ret);
- return ret;
+ goto out;
}
/* Fallthrough on positive return values - already enabled */
+ } else {
+ /* Check voltage and requested load. */
+ if (rdev->constraints &&
+ (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
+ drms_uA_update(rdev);
}
rdev->use_count++;
+out:
+ mutex_unlock(&rdev->mutex);
- return 0;
+ if (ret < 0) {
+ /* Enable has failed, disable supplies if necessary. */
+ while (supply_rdev != NULL) {
+ rdev = supply_rdev;
+
+ mutex_lock(&rdev->mutex);
+ _regulator_disable(rdev, &supply_rdev);
+ mutex_unlock(&rdev->mutex);
+ }
+ }
+
+ return ret;
}
/**
@@ -1368,13 +1402,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
*/
int regulator_enable(struct regulator *regulator)
{
- struct regulator_dev *rdev = regulator->rdev;
- int ret = 0;
-
- mutex_lock(&rdev->mutex);
- ret = _regulator_enable(rdev);
- mutex_unlock(&rdev->mutex);
- return ret;
+ return _regulator_enable(regulator->rdev);
}
EXPORT_SYMBOL_GPL(regulator_enable);
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
regulator_set_optimum_mode currently only determines the load
on the specified regulator. Physically however, this current
must be provided by regulators further up the supply chain.
Add code to handle uA_load propagation up through the regulator
supply chain.
Signed-off-by: David Collins <[email protected]>
---
drivers/regulator/core.c | 69 ++++++++++++++++++++++++++++++++++++-
include/linux/regulator/driver.h | 5 +++
2 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 425beba..2591fae 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -422,10 +422,20 @@ static ssize_t regulator_total_uA_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct regulator_dev *rdev = dev_get_drvdata(dev);
+ struct regulator_dev *consumer_rdev;
struct regulator *regulator;
int uA = 0;
+ /* Calculate total load of consumer regulator devices. */
+ list_for_each_entry(consumer_rdev, &rdev->supply_list, slist)
+ if (consumer_rdev->desc->ops->get_current_required) {
+ mutex_lock(&consumer_rdev->mutex);
+ uA += consumer_rdev->uA_load;
+ mutex_unlock(&consumer_rdev->mutex);
+ }
+
mutex_lock(&rdev->mutex);
+ /* Calculate total load of consumer devices. */
list_for_each_entry(regulator, &rdev->consumer_list, list)
uA += regulator->uA_load;
mutex_unlock(&rdev->mutex);
@@ -574,10 +584,14 @@ static struct class regulator_class = {
.dev_attrs = regulator_dev_attrs,
};
-/* Calculate the new optimum regulator operating mode based on the new total
- * consumer load. All locks held by caller */
+/*
+ * Calculate the new optimum regulator operating mode based on the new total
+ * consumer load. Lock for rdev is held by caller. Locks will be taken for
+ * consumer regulators of rdev.
+ */
static void drms_uA_update(struct regulator_dev *rdev)
{
+ struct regulator_dev *consumer_rdev;
struct regulator *sibling;
int current_uA = 0, output_uV, input_uV, err;
unsigned int mode;
@@ -607,6 +621,14 @@ static void drms_uA_update(struct regulator_dev *rdev)
list_for_each_entry(sibling, &rdev->consumer_list, list)
current_uA += sibling->uA_load;
+ /* calculate total load of consumer regulator devices before locking */
+ list_for_each_entry(consumer_rdev, &rdev->supply_list, slist)
+ if (consumer_rdev->desc->ops->get_current_required) {
+ mutex_lock(&consumer_rdev->mutex);
+ current_uA += consumer_rdev->uA_load;
+ mutex_unlock(&consumer_rdev->mutex);
+ }
+
/* now get the optimum mode for our new total regulator load */
mode = rdev->desc->ops->get_optimum_mode(rdev, input_uV,
output_uV, current_uA);
@@ -615,6 +637,14 @@ static void drms_uA_update(struct regulator_dev *rdev)
err = regulator_check_mode(rdev, mode);
if (err == 0)
rdev->desc->ops->set_mode(rdev, mode);
+
+ if (rdev->desc->ops->get_current_required) {
+ err = rdev->desc->ops->get_current_required(rdev, input_uV,
+ output_uV, current_uA);
+ if (err < 0)
+ return;
+ rdev->uA_load = err;
+ }
}
static int suspend_set_state(struct regulator_dev *rdev,
@@ -2033,10 +2063,20 @@ EXPORT_SYMBOL_GPL(regulator_get_mode);
int regulator_set_optimum_mode(struct regulator *regulator, int uA_load)
{
struct regulator_dev *rdev = regulator->rdev;
+ struct regulator_dev *consumer_rdev;
struct regulator *consumer;
int ret, output_uV, input_uV, total_uA_load = 0;
unsigned int mode;
+ /* calculate total load of consumer regulator devices before locking */
+ list_for_each_entry(consumer_rdev, &rdev->supply_list, slist) {
+ if (consumer_rdev->desc->ops->get_current_required) {
+ mutex_lock(&consumer_rdev->mutex);
+ total_uA_load += consumer_rdev->uA_load;
+ mutex_unlock(&consumer_rdev->mutex);
+ }
+ }
+
mutex_lock(&rdev->mutex);
regulator->uA_load = uA_load;
@@ -2086,9 +2126,34 @@ int regulator_set_optimum_mode(struct regulator *regulator, int uA_load)
rdev_err(rdev, "failed to set optimum mode %x\n", mode);
goto out;
}
+
+ if (rdev->desc->ops->get_current_required) {
+ ret = rdev->desc->ops->get_current_required(rdev, input_uV,
+ output_uV, total_uA_load);
+ if (ret < 0) {
+ rdev_err(rdev, "failed to get required load @ %d uA "
+ "%d -> %d uV, rc=%d\n", total_uA_load,
+ input_uV, output_uV, ret);
+ goto out;
+ }
+
+ rdev->uA_load = ret;
+ }
+
ret = mode;
+
out:
mutex_unlock(&rdev->mutex);
+
+ /* Update load for our supplies */
+ while (rdev->desc->ops->get_current_required && rdev->supply) {
+ rdev = rdev->supply;
+
+ mutex_lock(&rdev->mutex);
+ drms_uA_update(rdev);
+ mutex_unlock(&rdev->mutex);
+ }
+
return ret;
}
EXPORT_SYMBOL_GPL(regulator_set_optimum_mode);
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index b8ed16a..521b1b6 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -117,6 +117,10 @@ struct regulator_ops {
unsigned int (*get_optimum_mode) (struct regulator_dev *, int input_uV,
int output_uV, int load_uA);
+ /* get supply current required for load */
+ int (*get_current_required) (struct regulator_dev *, int input_uV,
+ int output_uV, int load_uA);
+
/* the operations below are for configuration of regulator state when
* its parent PMIC enters a global STANDBY/HIBERNATE state */
@@ -178,6 +182,7 @@ struct regulator_dev {
int exclusive;
u32 use_count;
u32 open_count;
+ int uA_load;
/* lists we belong to */
struct list_head list; /* list of all regulators */
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On Mon, Mar 28, 2011 at 08:34:42AM -0700, David Collins wrote:
> regulator_set_optimum_mode currently only determines the load
> on the specified regulator. Physically however, this current
> must be provided by regulators further up the supply chain.
> Add code to handle uA_load propagation up through the regulator
> supply chain.
We can't do this - current doesn't map 1:1 through a regulator, the
power consumption will map through but obviously there's a voltage
change involved and the regulators will not be 100% efficient so there
will also be some overhead from the chipld regulator. The child
regulator needs to do the mapping in a regulator specific fashion.
On Mon, Mar 28, 2011 at 08:34:41AM -0700, David Collins wrote:
Review curtailed by me having to dash off but one comment...
> -/* locks held by regulator_enable() */
> +/* Locks are *not* held by regulator_enable(). */
> static int _regulator_enable(struct regulator_dev *rdev)
> {
> - int ret, delay;
> + struct regulator_dev *supply_rdev = NULL;
> + int ret = 0, delay;
>
> + mutex_lock(&rdev->mutex);
This is going to be terribly confusing - the _ versions of the functions
all by convention rely on their callers taking the mutex, allowing them
to be safely used from internal APIs.
On Mon, Mar 28, 2011 at 07:02:55PM +0100, Mark Brown wrote:
> On Mon, Mar 28, 2011 at 08:34:42AM -0700, David Collins wrote:
> > regulator_set_optimum_mode currently only determines the load
> > on the specified regulator. Physically however, this current
> > must be provided by regulators further up the supply chain.
> > Add code to handle uA_load propagation up through the regulator
> > supply chain.
>
> We can't do this - current doesn't map 1:1 through a regulator, the
> power consumption will map through but obviously there's a voltage
> change involved and the regulators will not be 100% efficient so there
> will also be some overhead from the chipld regulator. The child
> regulator needs to do the mapping in a regulator specific fashion.
That's not true. Firstly, *all* regulators are not 100% efficient. They
lose *power* in the form of heat. So power into the regulator will always
be more than power out.
For linear regulators, the current flowing into a regulator is the sum
of the output current and the regulators operating current (which may
itself depend on the output current.) The input voltage will always be
greater than the output voltage. Therefore, from P=IV, power in will
always be greater than power out. So, if you have a 5V regulator
connected to a 10V supply, supplying 1A to a load, then:
Power out = 5V * 1A = 5W
Power in = 10V * (1A + operating current) = >10W
Power lost = >10W - 5W = >5W which will be in the form of heat.
For switching regulators, you have power lost again in the form of heat,
generated from the current required to run the regulators electronics
and the need to charge and discharge capacitances. Although these are
much more efficient than linear regulators, they don't get you to 100%
efficiency.
Note that also because P=IV, if the current doesn't map 1:1 through a
regulator, the power certainly won't either.
On 03/28/2011 11:02 AM, Mark Brown wrote:
> On Mon, Mar 28, 2011 at 08:34:42AM -0700, David Collins wrote:
>> regulator_set_optimum_mode currently only determines the load
>> on the specified regulator. Physically however, this current
>> must be provided by regulators further up the supply chain.
>> Add code to handle uA_load propagation up through the regulator
>> supply chain.
>
> We can't do this - current doesn't map 1:1 through a regulator, the
> power consumption will map through but obviously there's a voltage
> change involved and the regulators will not be 100% efficient so there
> will also be some overhead from the chipld regulator. The child
> regulator needs to do the mapping in a regulator specific fashion.
I am aware that input current of a regulator will not simply be the sum of
the output current loads required by consumers. I have accounted for this
by adding a new callback function in struct regulator_ops (from the patch):
+ /* get supply current required for load */
+ int (*get_current_required) (struct regulator_dev *, int input_uV,
+ int output_uV, int load_uA);
The intention is that all hardware specific characteristics of a given
regulator can captured with this callback function. The two simple cases
would be a linear regulator:
current_required = load_uA / efficiency + ground_current;
and a switching regulator:
current_required = load_uA * output_uV / (input_uV * efficiency)
I have set up the usage of this callback in the core such that if it is
not defined for regulators in a system, then the overall system operation
is identical to how it would be without the current propagation patch.
Can you look through the patch to see if the new API is reasonable?
Thanks,
David
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On 03/28/2011 11:11 AM, Mark Brown wrote:
>> -/* locks held by regulator_enable() */
>> +/* Locks are *not* held by regulator_enable(). */
>> static int _regulator_enable(struct regulator_dev *rdev)
>> {
>> - int ret, delay;
>> + struct regulator_dev *supply_rdev = NULL;
>> + int ret = 0, delay;
>>
>> + mutex_lock(&rdev->mutex);
>
> This is going to be terribly confusing - the _ versions of the functions
> all by convention rely on their callers taking the mutex, allowing them
> to be safely used from internal APIs.
_regulator_enable is only being called within regulator_enable and
_regulator_enable. Would it remove the confusion to rename
_regulator_enable to something different?
-David
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Because the uA_load propagation change takes out a lock on a regulator and
then the regulators that it supplies, it will cause deadlock with the current
regulator_enable implementation. regulator_disable can also deadlock with
regulator_enable, but it requires two threads and precise timing to observe.
Therefore, regulator_enable must be fixed before current propagation can be
used.
Changes from v1:
- Renamed _regulator_enable to regulator_dev_enable.
- Made change logs more verbose.
David Collins (2):
regulator: Remove possible deadlock from regulator_enable
regulator: Propagate uA_load requirements up supply chain
drivers/regulator/core.c | 151 ++++++++++++++++++++++++++++++-------
include/linux/regulator/driver.h | 5 +
2 files changed, 127 insertions(+), 29 deletions(-)
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Assume a regulator supply chain in which S1 supplies L2 (S1 --> L2).
It is possible to achieve deadlock using two threads if one thread
calls regulator_enable on a regulator L2 while the other thread calls
regulator_disable on S1. This happens because _regulator_enable(L2)
holds a lock for L2 and then takes out a lock on S1.
_regulator_disable(S1) on the other hand, is called with a lock taken
for S1 and then it calls _notifier_call_chain which attempts to take a
lock for L2. Because these locks are taken in opposite orders, deadlock
will result.
Change regulator_enable and _regulator_enable so that only one lock is
held at a time to avoid this deadlock scenario. Rename
_regulator_enable to regulator_dev_enable to avoid confusion with other
_* functions which assume that all locks are already taken.
Signed-off-by: David Collins <[email protected]>
---
drivers/regulator/core.c | 82 +++++++++++++++++++++++++++++++---------------
1 files changed, 55 insertions(+), 27 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9fa2095..05904be 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1279,38 +1279,54 @@ static int _regulator_can_change_status(struct regulator_dev *rdev)
return 0;
}
-/* locks held by regulator_enable() */
-static int _regulator_enable(struct regulator_dev *rdev)
+/* Locks are *not* held by regulator_enable(). */
+static int regulator_dev_enable(struct regulator_dev *rdev)
{
- int ret, delay;
+ struct regulator_dev *supply_rdev = NULL;
+ int ret = 0, delay;
+ mutex_lock(&rdev->mutex);
if (rdev->use_count == 0) {
+ /*
+ * Incrementing here removes a race condition for simultaneous
+ * regulator_enable calls with use_count == 0.
+ */
+ rdev->use_count++;
+ mutex_unlock(&rdev->mutex);
+
/* do we need to enable the supply regulator first */
if (rdev->supply) {
- mutex_lock(&rdev->supply->mutex);
- ret = _regulator_enable(rdev->supply);
- mutex_unlock(&rdev->supply->mutex);
+ ret = regulator_dev_enable(rdev->supply);
if (ret < 0) {
rdev_err(rdev, "failed to enable: %d\n", ret);
+ mutex_lock(&rdev->mutex);
+ rdev->use_count--;
+ mutex_unlock(&rdev->mutex);
return ret;
}
+ supply_rdev = rdev->supply;
}
- }
- /* check voltage and requested load before enabling */
- if (rdev->constraints &&
- (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
- drms_uA_update(rdev);
+ mutex_lock(&rdev->mutex);
+ rdev->use_count--;
+
+ /* check voltage and requested load before enabling */
+ if (rdev->constraints &&
+ (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
+ drms_uA_update(rdev);
- if (rdev->use_count == 0) {
- /* The regulator may on if it's not switchable or left on */
+ /* The regulator may be on if it's not switchable or left on */
ret = _regulator_is_enabled(rdev);
if (ret == -EINVAL || ret == 0) {
- if (!_regulator_can_change_status(rdev))
- return -EPERM;
+ if (!_regulator_can_change_status(rdev)) {
+ ret = -EPERM;
+ goto out;
+ }
- if (!rdev->desc->ops->enable)
- return -EINVAL;
+ if (!rdev->desc->ops->enable) {
+ ret = -EINVAL;
+ goto out;
+ }
/* Query before enabling in case configuration
* dependant. */
@@ -1330,7 +1346,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
* regulators can ramp together. */
ret = rdev->desc->ops->enable(rdev);
if (ret < 0)
- return ret;
+ goto out;
trace_regulator_enable_delay(rdev_get_name(rdev));
@@ -1345,14 +1361,32 @@ static int _regulator_enable(struct regulator_dev *rdev)
} else if (ret < 0) {
rdev_err(rdev, "is_enabled() failed: %d\n", ret);
- return ret;
+ goto out;
}
/* Fallthrough on positive return values - already enabled */
+ } else {
+ /* Check voltage and requested load. */
+ if (rdev->constraints &&
+ (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
+ drms_uA_update(rdev);
}
rdev->use_count++;
+out:
+ mutex_unlock(&rdev->mutex);
- return 0;
+ if (ret < 0) {
+ /* Enable has failed, disable supplies if necessary. */
+ while (supply_rdev != NULL) {
+ rdev = supply_rdev;
+
+ mutex_lock(&rdev->mutex);
+ _regulator_disable(rdev, &supply_rdev);
+ mutex_unlock(&rdev->mutex);
+ }
+ }
+
+ return ret;
}
/**
@@ -1368,13 +1402,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
*/
int regulator_enable(struct regulator *regulator)
{
- struct regulator_dev *rdev = regulator->rdev;
- int ret = 0;
-
- mutex_lock(&rdev->mutex);
- ret = _regulator_enable(rdev);
- mutex_unlock(&rdev->mutex);
- return ret;
+ return regulator_dev_enable(regulator->rdev);
}
EXPORT_SYMBOL_GPL(regulator_enable);
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
regulator_set_optimum_mode currently only determines the load
on the specified regulator. Physically however, this current
must be provided by regulators further up the supply chain.
Add code to handle uA_load propagation up through the regulator
supply chain in both regulator_set_optimum_mode and drms_uA_update.
Add a new regulator operation callback function named
get_current_required to struct regulator_ops to assist in this
propagation. get_current_required will return the input current
required for a given regulator based on input voltage, output
voltage, and output current. It should be able to capture all
hardware specific current characteristics of a regulator.
The input current required for a typical linear and switching
regulator would be simple to describe in this callback.
Usage of the get_current_required callback is conditional such
that if it is not specified for a given regulator, then that
regulator will not attempt to propagate its required current
to its supply regulator. This means that there is no change
to system operation if get_current_required is unspecified.
Extend struct regulator_dev slightly by adding a new member:
uA_load. This will default to 0 and only be filled with the
return value of get_current_required (if specified). It
effectively caches the load required by a regulator so that it is
not necessary to walk through the tree of supplied regulators
every time that regulator_set_optimum_mode or drms_uA_update is
called.
Modify regulator_total_uA_show so that it also sums in the loads
of supplied regulators.
Signed-off-by: David Collins <[email protected]>
---
drivers/regulator/core.c | 69 ++++++++++++++++++++++++++++++++++++-
include/linux/regulator/driver.h | 5 +++
2 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 05904be..dc2bbcf 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -422,10 +422,20 @@ static ssize_t regulator_total_uA_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct regulator_dev *rdev = dev_get_drvdata(dev);
+ struct regulator_dev *consumer_rdev;
struct regulator *regulator;
int uA = 0;
+ /* Calculate total load of consumer regulator devices. */
+ list_for_each_entry(consumer_rdev, &rdev->supply_list, slist)
+ if (consumer_rdev->desc->ops->get_current_required) {
+ mutex_lock(&consumer_rdev->mutex);
+ uA += consumer_rdev->uA_load;
+ mutex_unlock(&consumer_rdev->mutex);
+ }
+
mutex_lock(&rdev->mutex);
+ /* Calculate total load of consumer devices. */
list_for_each_entry(regulator, &rdev->consumer_list, list)
uA += regulator->uA_load;
mutex_unlock(&rdev->mutex);
@@ -574,10 +584,14 @@ static struct class regulator_class = {
.dev_attrs = regulator_dev_attrs,
};
-/* Calculate the new optimum regulator operating mode based on the new total
- * consumer load. All locks held by caller */
+/*
+ * Calculate the new optimum regulator operating mode based on the new total
+ * consumer load. Lock for rdev is held by caller. Locks will be taken for
+ * consumer regulators of rdev.
+ */
static void drms_uA_update(struct regulator_dev *rdev)
{
+ struct regulator_dev *consumer_rdev;
struct regulator *sibling;
int current_uA = 0, output_uV, input_uV, err;
unsigned int mode;
@@ -607,6 +621,14 @@ static void drms_uA_update(struct regulator_dev *rdev)
list_for_each_entry(sibling, &rdev->consumer_list, list)
current_uA += sibling->uA_load;
+ /* calculate total load of consumer regulator devices before locking */
+ list_for_each_entry(consumer_rdev, &rdev->supply_list, slist)
+ if (consumer_rdev->desc->ops->get_current_required) {
+ mutex_lock(&consumer_rdev->mutex);
+ current_uA += consumer_rdev->uA_load;
+ mutex_unlock(&consumer_rdev->mutex);
+ }
+
/* now get the optimum mode for our new total regulator load */
mode = rdev->desc->ops->get_optimum_mode(rdev, input_uV,
output_uV, current_uA);
@@ -615,6 +637,14 @@ static void drms_uA_update(struct regulator_dev *rdev)
err = regulator_check_mode(rdev, mode);
if (err == 0)
rdev->desc->ops->set_mode(rdev, mode);
+
+ if (rdev->desc->ops->get_current_required) {
+ err = rdev->desc->ops->get_current_required(rdev, input_uV,
+ output_uV, current_uA);
+ if (err < 0)
+ return;
+ rdev->uA_load = err;
+ }
}
static int suspend_set_state(struct regulator_dev *rdev,
@@ -2033,10 +2063,20 @@ EXPORT_SYMBOL_GPL(regulator_get_mode);
int regulator_set_optimum_mode(struct regulator *regulator, int uA_load)
{
struct regulator_dev *rdev = regulator->rdev;
+ struct regulator_dev *consumer_rdev;
struct regulator *consumer;
int ret, output_uV, input_uV, total_uA_load = 0;
unsigned int mode;
+ /* calculate total load of consumer regulator devices before locking */
+ list_for_each_entry(consumer_rdev, &rdev->supply_list, slist) {
+ if (consumer_rdev->desc->ops->get_current_required) {
+ mutex_lock(&consumer_rdev->mutex);
+ total_uA_load += consumer_rdev->uA_load;
+ mutex_unlock(&consumer_rdev->mutex);
+ }
+ }
+
mutex_lock(&rdev->mutex);
regulator->uA_load = uA_load;
@@ -2086,9 +2126,34 @@ int regulator_set_optimum_mode(struct regulator *regulator, int uA_load)
rdev_err(rdev, "failed to set optimum mode %x\n", mode);
goto out;
}
+
+ if (rdev->desc->ops->get_current_required) {
+ ret = rdev->desc->ops->get_current_required(rdev, input_uV,
+ output_uV, total_uA_load);
+ if (ret < 0) {
+ rdev_err(rdev, "failed to get required load @ %d uA "
+ "%d -> %d uV, rc=%d\n", total_uA_load,
+ input_uV, output_uV, ret);
+ goto out;
+ }
+
+ rdev->uA_load = ret;
+ }
+
ret = mode;
+
out:
mutex_unlock(&rdev->mutex);
+
+ /* Update load for our supplies */
+ while (rdev->desc->ops->get_current_required && rdev->supply) {
+ rdev = rdev->supply;
+
+ mutex_lock(&rdev->mutex);
+ drms_uA_update(rdev);
+ mutex_unlock(&rdev->mutex);
+ }
+
return ret;
}
EXPORT_SYMBOL_GPL(regulator_set_optimum_mode);
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index b8ed16a..521b1b6 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -117,6 +117,10 @@ struct regulator_ops {
unsigned int (*get_optimum_mode) (struct regulator_dev *, int input_uV,
int output_uV, int load_uA);
+ /* get supply current required for load */
+ int (*get_current_required) (struct regulator_dev *, int input_uV,
+ int output_uV, int load_uA);
+
/* the operations below are for configuration of regulator state when
* its parent PMIC enters a global STANDBY/HIBERNATE state */
@@ -178,6 +182,7 @@ struct regulator_dev {
int exclusive;
u32 use_count;
u32 open_count;
+ int uA_load;
/* lists we belong to */
struct list_head list; /* list of all regulators */
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On Mon, Mar 28, 2011 at 07:14:15PM +0100, Russell King - ARM Linux wrote:
> That's not true. Firstly, *all* regulators are not 100% efficient. They
> lose *power* in the form of heat. So power into the regulator will always
> be more than power out.
That's pretty much what I said, yes.
On Tue, Mar 29, 2011 at 08:53:11AM +0100, Mark Brown wrote:
> On Mon, Mar 28, 2011 at 07:14:15PM +0100, Russell King - ARM Linux wrote:
>
> > That's not true. Firstly, *all* regulators are not 100% efficient. They
> > lose *power* in the form of heat. So power into the regulator will always
> > be more than power out.
>
> That's pretty much what I said, yes.
No, you said the power will map through but the current will not. I'm
saying that the power will *not* map through either.
On Tue, Mar 29, 2011 at 09:28:48AM +0100, Russell King - ARM Linux wrote:
> On Tue, Mar 29, 2011 at 08:53:11AM +0100, Mark Brown wrote:
> > That's pretty much what I said, yes.
> No, you said the power will map through but the current will not. I'm
> saying that the power will *not* map through either.
I said that even with a perfectly efficient regulator the current won't
map through, and that physical regulators make things even less direct.
On Mon, Mar 28, 2011 at 04:53:52PM -0700, David Collins wrote:
> regulator_set_optimum_mode currently only determines the load
> on the specified regulator. Physically however, this current
> must be provided by regulators further up the supply chain.
This isn't actually true - it's very common for the system to be
designed such that all the regulators are supplied from a single
unregulated system power rail, especially with modern PMICs.
> Add code to handle uA_load propagation up through the regulator
> supply chain in both regulator_set_optimum_mode and drms_uA_update.
So, my main concern here is essentially the same thing I said with
regard to your original posting about the locking - this all seems
like it's the wrong approach and if we just treated supplies the same as
other consumers life would get a lot simpler. This is adding a lot more
special cases for supplies which feels like it's making the code more
complicated. Supplied regulators are essentially just consumers and
this is adding more code that diverges the two and makes it hard to
follow what's going on - ideally when dealing with the parent regulator
you shouldn't have to worry about child regulators at all, they should
just look like all the other consumers.
> Add a new regulator operation callback function named
> get_current_required to struct regulator_ops to assist in this
> propagation. get_current_required will return the input current
> required for a given regulator based on input voltage, output
> voltage, and output current. It should be able to capture all
> hardware specific current characteristics of a regulator.
> The input current required for a typical linear and switching
> regulator would be simple to describe in this callback.
The other issue here is that I'm concerned about the direction this
appears to be heading given that it seems like in order for this to do
something useful we'd need to start supplying current drain information
from consumers. We're not doing that at the minute and I'm not terribly
optimistic that we'd ever get enough useful information to make it
generally worthwhile.
Besides, the interesting stuff tends to be around response to sudden
changes in load and that's basically all been pushed down into hardware
as there's no way for software to propagate the information fast enough.
For software you tend to have to assume the worst case load but that
will usually be a massive overestimate, and of course transients will be
absorbed by the passives to a certain extent which complicates matters
further.
Perhaps if you could explain your use case for this things might become
clearer?
On 03/29/2011 01:44 AM, Mark Brown wrote:
>> Add code to handle uA_load propagation up through the regulator
>> supply chain in both regulator_set_optimum_mode and drms_uA_update.
>
> So, my main concern here is essentially the same thing I said with
> regard to your original posting about the locking - this all seems
> like it's the wrong approach and if we just treated supplies the same as
> other consumers life would get a lot simpler. This is adding a lot more
> special cases for supplies which feels like it's making the code more
> complicated. Supplied regulators are essentially just consumers and
> this is adding more code that diverges the two and makes it hard to
> follow what's going on - ideally when dealing with the parent regulator
> you shouldn't have to worry about child regulators at all, they should
> just look like all the other consumers.
I agree that it would be beneficial to change regulator_dev.supply from
type struct regulator_dev * to type struct regulator *. However, I think
that going that route will be a major undertaking with a lot of details to
work out and more room for deadlocking scenarios to get introduced. Who
would be responsible for making such a change?
>> Add a new regulator operation callback function named
>> get_current_required to struct regulator_ops to assist in this
>> propagation. get_current_required will return the input current
>> required for a given regulator based on input voltage, output
>> voltage, and output current. It should be able to capture all
>> hardware specific current characteristics of a regulator.
>> The input current required for a typical linear and switching
>> regulator would be simple to describe in this callback.
>
> The other issue here is that I'm concerned about the direction this
> appears to be heading given that it seems like in order for this to do
> something useful we'd need to start supplying current drain information
> from consumers. We're not doing that at the minute and I'm not terribly
> optimistic that we'd ever get enough useful information to make it
> generally worthwhile.
The regulator framework does have a mechanism for consumers to specify
their current drain information: regulator_set_optimum_mode. Systems that
do not wish to take advantage of the current propagation API are under no
obligation to use it.
> Besides, the interesting stuff tends to be around response to sudden
> changes in load and that's basically all been pushed down into hardware
> as there's no way for software to propagate the information fast enough.
> For software you tend to have to assume the worst case load but that
> will usually be a massive overestimate, and of course transients will be
> absorbed by the passives to a certain extent which complicates matters
> further.
>
> Perhaps if you could explain your use case for this things might become
> clearer?
The system that I am working with utilizes a PMIC containing 8 switching
regulators (SMPS) and 26 LDO regulators. Many of the LDO's are
subregulated from the SMPS's. The system state at the beginning of boot
will have all non-essential regulators disabled and in low power mode (LPM
signified with REGULATOR_MODE_STANDBY). Consumer drivers will then be
responsible for setting up the regulators that their peripheral devices
require. Most of the LDO regulators have a LPM threshold of 10mA and high
power mode (HPM signified with REGULATOR_MODE_FAST) threshold of 50-1200mA
depending upon type. Thus, consumers will need to call
regulator_set_optimum_mode with their peak load current in order to change
their supply regulator from LPM to HPM. It is also worth noting that many
of the regulators are shared between different consumers so it is
important that regulator_set_optimum_mode is used instead of
regulator_set_mode to ensure that all consumer needs are met.
The regulator framework will already handle enabling the regulators
further up the supply chain. In this case, enabling an LDO will also
enable its parent SMPS. However, changing the mode of a regulator based
on current draw will not affect the mode of its supply regulator at the
moment. This is a problem in the system I am working on because the SMPS
regulators will also be in LPM by default. The max LPM output of the SMPS
regulators is 100mA and the max HPM output is 1500 or 2000mA (depending
upon type). Consumer drivers will not be aware of the regulator supply
chain; nor should they need to be. Therefore, it is critical for system
functionality that changing the load of a child regulator will also change
the load of its parent regulator.
One could solve this problem by using HPM at all times, but there is a
design requirement that system use the smallest amount of power possible.
Do you have thoughts about a different solution?
Thanks,
David
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On Tue, Mar 29, 2011 at 09:08:15AM -0700, David Collins wrote:
> I agree that it would be beneficial to change regulator_dev.supply from
> type struct regulator_dev * to type struct regulator *. However, I think
> that going that route will be a major undertaking with a lot of details to
Hrm, it doesn't look too bad - as far as I can see it should just be
fairly direct refactorings of each of get, put, enable and disable?
> work out and more room for deadlocking scenarios to get introduced. Who
> would be responsible for making such a change?
I don't know, but I don't think we should make their life more difficult
by adding additional complexity that then needs to be unpicked. I might
look into this (it's been bugging me for a while), no idea when but if
people are actively working with supplies that obviously helps.
> > The other issue here is that I'm concerned about the direction this
> > appears to be heading given that it seems like in order for this to do
> > something useful we'd need to start supplying current drain information
> > from consumers. We're not doing that at the minute and I'm not terribly
> > optimistic that we'd ever get enough useful information to make it
> > generally worthwhile.
> The regulator framework does have a mechanism for consumers to specify
> their current drain information: regulator_set_optimum_mode. Systems that
> do not wish to take advantage of the current propagation API are under no
> obligation to use it.
It's true that there's some code for this, but it's also true that there
are no actual users of the code and only a very few drivers (mostly
written by me) that implement the required bits on the driver side.
This API has been there since the regulator API was merged and has still
not been picked up by any users. To a large extent I think this is
because it is difficult to deploy effectively.
I'd also not say that there's no impact on systems not using it - any
consumer that needs to participate in the current management will need
to have support for telling the core about what it's using. The actual
numbers there are going to be dependant on a range of per-system
factors, including supply voltages, workloads, hardware configuration,
different software compatible silicon and the ability of the supplies to
cope with brief load transients. Providing that information is not a
trivial requirement, it's a very wide ranging one with an impact on any
device that uses power.
> The system that I am working with utilizes a PMIC containing 8 switching
> regulators (SMPS) and 26 LDO regulators. Many of the LDO's are
> subregulated from the SMPS's. The system state at the beginning of boot
> will have all non-essential regulators disabled and in low power mode (LPM
> signified with REGULATOR_MODE_STANDBY). Consumer drivers will then be
> responsible for setting up the regulators that their peripheral devices
> require. Most of the LDO regulators have a LPM threshold of 10mA and high
This is very different to the normal usage model for the regulator API -
you'd not expect every single consumer driver on the board to have to be
involved in specifying the load they're drawing as actual current
numbers and you'll also need to take into account things that draw
current without having a driver. The idea is that there should be very
little impact on most devices that aren't actively managing their
supplies by doing things like changing voltages at runtime, most
regulator configuration should be done for them by the board.
> power mode (HPM signified with REGULATOR_MODE_FAST) threshold of 50-1200mA
You should be using _NORMAL for this mode, that just sounds like a
regular LDO. _FAST is intended for use on older DCDCs which have modes
where they can respond particularly quickly to dramatic changes in load
at the expense of some overhead on lower loads (modern regulators are
able to just cope with this and don't need any software control here,
though they often still provide it just in case some workload requires
it). _NORMAL is just the vanilla mode which you should use if you've
not got any particular requirements.
> depending upon type. Thus, consumers will need to call
> regulator_set_optimum_mode with their peak load current in order to change
> their supply regulator from LPM to HPM. It is also worth noting that many
This is the thing I was saying about all the consumer drivers needing to
get involved.
> One could solve this problem by using HPM at all times, but there is a
> design requirement that system use the smallest amount of power possible.
> Do you have thoughts about a different solution?
In principle if we could get the information about the consumption of
the consumers reliably and accurately supplied this would work well but
that first step is the issue. It doesn't seem terribly realistic, and
if you've got some consumers in play which aren't taking part the wheels
will start to come off the system. I think we'll run into lots of
implementation issues if we try to do things this way and that it'll be
very fragile, especially with consumer devices that are used on many
different platforms.
I expect that if you look at the majority of consumer drivers you've got
they'll have at most one or two loads documented, probably a normal
operation mode and an idle mode. I think this is the sort of
information that we could realistically get from drivers and we can then
do the mapping into actual numbers in the machine driver, adding more
data to the supply mapping. This degrades much better than having all
consumers specify actual numbers at all times as it means you can get
basic operation with no involvement whatsoever from most of the drivers,
and we could also hook in with disabling the regulators so that we go
into the idle mode when the consumer disables.
Does the above map onto what your system is doing? If we can come up
with a higher level external API such as this which abstracts away the
numbers from consumers then the problem becomes a lot more tractable and
reasonable to deploy system wide.
On Mon, Mar 28, 2011 at 04:53:52PM -0700, David Collins wrote:
Some more specific comments on the code:
> + if (rdev->desc->ops->get_current_required) {
> + err = rdev->desc->ops->get_current_required(rdev, input_uV,
> + output_uV, current_uA);
> + if (err < 0)
> + return;
> + rdev->uA_load = err;
> + }
I think we need something to handle misssing data here if there is a
supply connected, otherwise we'll end up failing silently. Off the top
of my head we could either just complain loudly if there's no operation
and there should be one or make a pessimistic assumption about
efficiency by default.
> + /* get supply current required for load */
> + int (*get_current_required) (struct regulator_dev *, int input_uV,
> + int output_uV, int load_uA);
> +
I like the interface for the driver but possibly call it something like
get_supply_current() or something to make it a little clearer which
current is being requested? Also needs to have kerneldoc added for the
new function - ideally the documentation would say this is for sustained
rather than peak load.
On Wed, Mar 30, 2011 at 06:20:10AM +0900, Mark Brown wrote:
> On Tue, Mar 29, 2011 at 09:08:15AM -0700, David Collins wrote:
> > I agree that it would be beneficial to change regulator_dev.supply from
> > type struct regulator_dev * to type struct regulator *. However, I think
> > that going that route will be a major undertaking with a lot of details to
> Hrm, it doesn't look too bad - as far as I can see it should just be
> fairly direct refactorings of each of get, put, enable and disable?
I had a look at this, it all looks very straightforward apart from get
where we need to either do a dance to set up a supply mapping or
restructure to expose the core get operation internally without map
lookups (the latter I think) and that doesn't seem terribly invasive.
I may actually try coding it up next time I'm sitting in front of an
appropriate test system.