[PATCH 4/7] regulator: core: Only count load for enabled consumers
In general when the consumer of a regulator requests that the
regulator be disabled it no longer will be drawing much load from the
regulator--it should just be the leakage current and that should be
very close to 0.
Up to this point the regulator framework has continued to count a
consumer's load request for disabled regulators. This has led to code
patterns that look like this:
enable_my_thing():
regular_set_load(reg, load_uA)
regulator_enable(reg)
disable_my_thing():
regulator_disable(reg)
regulator_set_load(reg, 0)
Sometimgs disable_my_thing() sets a nominal (<= 100 uA) load instead
of setting a 0 uA load. I will make the assertion that nearly all (if
not all) places where we set a nominal load of 100 uA or less we end
up with a result that is the same as if we had set a load of 0 uA.
Specifically:
- The whole point of setting the load is to help set the operating
mode of the regulator. Higher loads may need less efficient
operating modes.
- The only time this matters at all is if there is another consumer of
the regulator that wants the regulator on. If there are no other
consumers of the regulator then the regulator will turn off and we
don't care about the operating mode.
- If there's another consumer that actually wants the regulator on
then presumably it is requesting a load that makes our nominal
<= 100 uA load insignificant.
A quick survey of the existing callers to regulator_set_load() to see
how everyone uses it:
--
Places that set a load of 0 on every disable:
drivers/bluetooth/hci_qca.c
drivers/net/wireless/ath/ath10k/snoc.c
drivers/remoteproc/qcom_q6v5_mss.c
drivers/scsi/ufs/ufshcd.c
Places that set a nominal disable load (<= 100 uA):
drivers/gpu/drm/msm/dsi/dsi_host.c:
git grep -A10 'struct msm_dsi_config' | grep -A10 regs | grep '{.*}'
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c:
git grep -A10 'struct msm_dsi_phy_cfg' | grep -A10 regs | grep '{.*}'
drivers/gpu/drm/msm/edp/edp_ctrl.c
Places that appear to continue to request load when disabled; I
believe this is not intentional and is a bug. AKA: either the
regulator will be left in a higher power operating mode sometimes when
it doesn't need to or it is understood that there are no other
consumers of these rails and thus the regulator_set_load() shouldn't
have been needed at all (we should just force the regulator to high
power all the time in that case):
drivers/phy/qualcomm/phy-qcom-ufs.c
drivers/phy/qualcomm/phy-qcom-usb-hs.c
drivers/remoteproc/qcom_q6v5_pas.c
drivers/remoteproc/qcom_wcnss.c (*)
drivers/remoteproc/qcom_wcnss_iris.c
(*) Quick glace makes me feel like this driver is buggy since
wcnss_start() enables regulators but wcnss_stop() doesn't disable
them.
--
Given the above argument, I propose changing the regulator core to
only consider loads from consumers that are currently enabled. If
later we find a use case where we really need to count a non-zero load
when a consumer requests that a regulator be disabled we could just
count load like this in the system_load for the regulator. If somehow
that's not possible I suppose we could add consider adding a new API
regulator_set_disabled_load().
In order to accomplish the above, we need to start tracking the enable
count per regulator consumer (it used to be tracked only globally
across all consumers of a given regulator). This means:
- We can (and will) spit errors out for code that used to be invalid
but was never caught before. Specifically if someone leaves a
regulator enabled and calls regulator_put() we'll yell. We'll also
yell if a single consumer calls more disables than enables.
- We now need to deal with "always-on" regulators slightly
differently. It's assumed that an always-on regulator could still
transition between power modes, so if all of the active consumers of
an always-on regulator don't need the regulator on we should still
remove their requested load from the total. The core used to have
some optimizations where it didn't bother keeping track of the
enable counts for always-on regulators because they were, well,
always on. While we could keep the optimization still for some
cases, it's cleaner to just remove it. A later patch will attempt
to get some efficiency back by not propogating enables up
unnecessarily.
Signed-off-by: Douglas Anderson
---
Please give this patch lots of extra review. It seems to work for me,
but I haven't done massive amounts of stress testing on it and I don't
touch every use case of the regulator core. Thanks!
drivers/regulator/core.c | 190 +++
drivers/regulator/internal.h | 2 +
include/linux/regulator/driver.h | 1 -
3 files changed, 142 insertions(+), 51 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 63a8af1e2256..c5da6079e1cb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -99,7 +99,7 @@ struct
[PATCH 4/7] regulator: core: Only count load for enabled consumers
In general when the consumer of a regulator requests that the
regulator be disabled it no longer will be drawing much load from the
regulator--it should just be the leakage current and that should be
very close to 0.
Up to this point the regulator framework has continued to count a
consumer's load request for disabled regulators. This has led to code
patterns that look like this:
enable_my_thing():
regular_set_load(reg, load_uA)
regulator_enable(reg)
disable_my_thing():
regulator_disable(reg)
regulator_set_load(reg, 0)
Sometimgs disable_my_thing() sets a nominal (<= 100 uA) load instead
of setting a 0 uA load. I will make the assertion that nearly all (if
not all) places where we set a nominal load of 100 uA or less we end
up with a result that is the same as if we had set a load of 0 uA.
Specifically:
- The whole point of setting the load is to help set the operating
mode of the regulator. Higher loads may need less efficient
operating modes.
- The only time this matters at all is if there is another consumer of
the regulator that wants the regulator on. If there are no other
consumers of the regulator then the regulator will turn off and we
don't care about the operating mode.
- If there's another consumer that actually wants the regulator on
then presumably it is requesting a load that makes our nominal
<= 100 uA load insignificant.
A quick survey of the existing callers to regulator_set_load() to see
how everyone uses it:
--
Places that set a load of 0 on every disable:
drivers/bluetooth/hci_qca.c
drivers/net/wireless/ath/ath10k/snoc.c
drivers/remoteproc/qcom_q6v5_mss.c
drivers/scsi/ufs/ufshcd.c
Places that set a nominal disable load (<= 100 uA):
drivers/gpu/drm/msm/dsi/dsi_host.c:
git grep -A10 'struct msm_dsi_config' | grep -A10 regs | grep '{.*}'
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c:
git grep -A10 'struct msm_dsi_phy_cfg' | grep -A10 regs | grep '{.*}'
drivers/gpu/drm/msm/edp/edp_ctrl.c
Places that appear to continue to request load when disabled; I
believe this is not intentional and is a bug. AKA: either the
regulator will be left in a higher power operating mode sometimes when
it doesn't need to or it is understood that there are no other
consumers of these rails and thus the regulator_set_load() shouldn't
have been needed at all (we should just force the regulator to high
power all the time in that case):
drivers/phy/qualcomm/phy-qcom-ufs.c
drivers/phy/qualcomm/phy-qcom-usb-hs.c
drivers/remoteproc/qcom_q6v5_pas.c
drivers/remoteproc/qcom_wcnss.c (*)
drivers/remoteproc/qcom_wcnss_iris.c
(*) Quick glace makes me feel like this driver is buggy since
wcnss_start() enables regulators but wcnss_stop() doesn't disable
them.
--
Given the above argument, I propose changing the regulator core to
only consider loads from consumers that are currently enabled. If
later we find a use case where we really need to count a non-zero load
when a consumer requests that a regulator be disabled we could just
count load like this in the system_load for the regulator. If somehow
that's not possible I suppose we could add consider adding a new API
regulator_set_disabled_load().
In order to accomplish the above, we need to start tracking the enable
count per regulator consumer (it used to be tracked only globally
across all consumers of a given regulator). This means:
- We can (and will) spit errors out for code that used to be invalid
but was never caught before. Specifically if someone leaves a
regulator enabled and calls regulator_put() we'll yell. We'll also
yell if a single consumer calls more disables than enables.
- We now need to deal with "always-on" regulators slightly
differently. It's assumed that an always-on regulator could still
transition between power modes, so if all of the active consumers of
an always-on regulator don't need the regulator on we should still
remove their requested load from the total. The core used to have
some optimizations where it didn't bother keeping track of the
enable counts for always-on regulators because they were, well,
always on. While we could keep the optimization still for some
cases, it's cleaner to just remove it. A later patch will attempt
to get some efficiency back by not propogating enables up
unnecessarily.
Signed-off-by: Douglas Anderson
---
Please give this patch lots of extra review. It seems to work for me,
but I haven't done massive amounts of stress testing on it and I don't
touch every use case of the regulator core. Thanks!
drivers/regulator/core.c | 190 +++
drivers/regulator/internal.h | 2 +
include/linux/regulator/driver.h | 1 -
3 files changed, 142 insertions(+), 51 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 63a8af1e2256..c5da6079e1cb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -99,7 +99,7 @@ struct
