[RFC 0/2] Qualcomm RPM sleep states

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

During the review of the Qualcomm SMD RPM regulators [1], I got (offline)
feedback that my implementation did not handle "sleep states". As the problem
is shared between all families of Qualcomm platforms I use [2] (family A) to
propose a solution (as I hope to get that merged sooner).

The "sleep states" comment boils down to certain regulators (or rpm resources
in general) are used by the currently clocked/running CPU(s) and can not be
disabled while we're still running. Further more, these resources are shared
with peripherals in the system; e.g. LDO12 on PM8941 is used to clock the CPU
and WiFi/BT PLLs as well as providing power to the display in our devices. So
the suspend functionality in the regulator framework doesn't cut it.


The downstream solution to this is to expose 3 regulators per regulator
resource, each specified to control the active mode, sleep mode or both modes
respectively. Peripherals are directed to use the "both" regulator while the
CPUs are directed to the "active only" regulator.

After reviewing this solution and looking at what it's actually achieving I
here propose flagging these regulators to have "deferred disable";
* we consider the specific regulators as always-on _while running_
* hence, disable and enable affect only the sleep state
* we update both active and sleep state with all other properties

This gives us a single regulator exposed for the resource, that will be kept on
with parameters as specified by the clients if it's referenced and upon loosing
the last reference (disabling all consumers) it will be turned off when the
CPU(s) are sleeping.


As far as I can see this should give the same behaviour as we have downstream,
without the need for playing tricks with how we expose the regulators. However
there's plenty of details hidden in that code, so I hope to get some feedback
from the Qualcomm engineers on this.


The first patch in the series applies on top of [2], it should be squashed with
1 & 2 of [2] and is only included for completeness.

[1] https://lkml.org/lkml/2014/9/29/747
[2] https://lkml.org/lkml/2014/9/22/731

Bjorn Andersson (2):
  mfd: qcom-rpm: Expose sleep state resources to clients
  regulator: qcom-rpm: Implement RPM assisted disable

 Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++
 drivers/mfd/qcom_rpm.c                             |    9 +--
 drivers/regulator/qcom_rpm-regulator.c             |   67 +++++++++++++++-----
 include/linux/mfd/qcom_rpm.h                       |    5 +-
 4 files changed, 88 insertions(+), 21 deletions(-)

-- 
1.7.9.5

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

During the review of the Qualcomm SMD RPM regulators [1], I got (offline)
feedback that my implementation did not handle "sleep states". As the problem
is shared between all families of Qualcomm platforms I use [2] (family A) to
propose a solution (as I hope to get that merged sooner).

The "sleep states" comment boils down to certain regulators (or rpm resources
in general) are used by the currently clocked/running CPU(s) and can not be
disabled while we're still running. Further more, these resources are shared
with peripherals in the system; e.g. LDO12 on PM8941 is used to clock the CPU
and WiFi/BT PLLs as well as providing power to the display in our devices. So
the suspend functionality in the regulator framework doesn't cut it.


The downstream solution to this is to expose 3 regulators per regulator
resource, each specified to control the active mode, sleep mode or both modes
respectively. Peripherals are directed to use the "both" regulator while the
CPUs are directed to the "active only" regulator.

After reviewing this solution and looking at what it's actually achieving I
here propose flagging these regulators to have "deferred disable";
* we consider the specific regulators as always-on _while running_
* hence, disable and enable affect only the sleep state
* we update both active and sleep state with all other properties

This gives us a single regulator exposed for the resource, that will be kept on
with parameters as specified by the clients if it's referenced and upon loosing
the last reference (disabling all consumers) it will be turned off when the
CPU(s) are sleeping.


As far as I can see this should give the same behaviour as we have downstream,
without the need for playing tricks with how we expose the regulators. However
there's plenty of details hidden in that code, so I hope to get some feedback
from the Qualcomm engineers on this.


The first patch in the series applies on top of [2], it should be squashed with
1 & 2 of [2] and is only included for completeness.

[1] https://lkml.org/lkml/2014/9/29/747
[2] https://lkml.org/lkml/2014/9/22/731

Bjorn Andersson (2):
  mfd: qcom-rpm: Expose sleep state resources to clients
  regulator: qcom-rpm: Implement RPM assisted disable

 Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++
 drivers/mfd/qcom_rpm.c                             |    9 +--
 drivers/regulator/qcom_rpm-regulator.c             |   67 +++++++++++++++-----
 include/linux/mfd/qcom_rpm.h                       |    5 +-
 4 files changed, 88 insertions(+), 21 deletions(-)

-- 
1.7.9.5

[RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Bjorn Andersson]

Resources exposed from the RPM have an "active state" that is used during
normal operations and a "sleep state" that is used for HW assisted sleep
modes. Expose this in the api to let client drivers set the "sleep
state" as well.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/mfd/qcom_rpm.c                 |    9 +++++----
 drivers/regulator/qcom_rpm-regulator.c |    1 +
 include/linux/mfd/qcom_rpm.h           |    5 ++++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/qcom_rpm.c b/drivers/mfd/qcom_rpm.c
index 0dd7a6fe..f696328 100644
--- a/drivers/mfd/qcom_rpm.c
+++ b/drivers/mfd/qcom_rpm.c
@@ -67,7 +67,6 @@ struct qcom_rpm {
 #define RPM_ACK_SELECTOR	23
 #define RPM_SELECT_SIZE		7
 
-#define RPM_ACTIVE_STATE	BIT(0)
 #define RPM_NOTIFICATION	BIT(30)
 #define RPM_REJECTED		BIT(31)
 
@@ -332,7 +331,10 @@ static const struct of_device_id qcom_rpm_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, qcom_rpm_of_match);
 
-int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
+int qcom_rpm_write(struct qcom_rpm *rpm,
+		   int state,
+		   int resource,
+		   u32 *buf, size_t count)
 {
 	const struct qcom_rpm_resource *res;
 	const struct qcom_rpm_data *data = rpm->data;
@@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
 			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
 	}
 
-	writel_relaxed(RPM_ACTIVE_STATE,
-		       RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
+	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
 
 	reinit_completion(&rpm->ack);
 	regmap_write(rpm->ipc_regmap, rpm->ipc_offset, BIT(rpm->ipc_bit));
diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
index b55cd5b..4fc1c7e 100644
--- a/drivers/regulator/qcom_rpm-regulator.c
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -198,6 +198,7 @@ static int rpm_reg_write(struct qcom_rpm_reg *vreg,
 	vreg->val[req->word] |= value << req->shift;
 
 	return qcom_rpm_write(vreg->rpm,
+			      RPM_ACTIVE_STATE,
 			      vreg->resource,
 			      vreg->val,
 			      vreg->parts->request_len);
diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h
index a60798d..f0e70b2 100644
--- a/include/linux/mfd/qcom_rpm.h
+++ b/include/linux/mfd/qcom_rpm.h
@@ -5,6 +5,9 @@
 
 struct qcom_rpm;
 
-int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count);
+#define RPM_ACTIVE_STATE	0
+#define RPM_SLEEP_STATE		1
+
+int qcom_rpm_write(struct qcom_rpm *rpm, int state, int resource, u32 *buf, size_t count);
 
 #endif
-- 
1.7.9.5

[RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Bjorn Andersson]

Resources exposed from the RPM have an "active state" that is used during
normal operations and a "sleep state" that is used for HW assisted sleep
modes. Expose this in the api to let client drivers set the "sleep
state" as well.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/mfd/qcom_rpm.c                 |    9 +++++----
 drivers/regulator/qcom_rpm-regulator.c |    1 +
 include/linux/mfd/qcom_rpm.h           |    5 ++++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/qcom_rpm.c b/drivers/mfd/qcom_rpm.c
index 0dd7a6fe..f696328 100644
--- a/drivers/mfd/qcom_rpm.c
+++ b/drivers/mfd/qcom_rpm.c
@@ -67,7 +67,6 @@ struct qcom_rpm {
 #define RPM_ACK_SELECTOR	23
 #define RPM_SELECT_SIZE		7
 
-#define RPM_ACTIVE_STATE	BIT(0)
 #define RPM_NOTIFICATION	BIT(30)
 #define RPM_REJECTED		BIT(31)
 
@@ -332,7 +331,10 @@ static const struct of_device_id qcom_rpm_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, qcom_rpm_of_match);
 
-int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
+int qcom_rpm_write(struct qcom_rpm *rpm,
+		   int state,
+		   int resource,
+		   u32 *buf, size_t count)
 {
 	const struct qcom_rpm_resource *res;
 	const struct qcom_rpm_data *data = rpm->data;
@@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
 			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
 	}
 
-	writel_relaxed(RPM_ACTIVE_STATE,
-		       RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
+	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
 
 	reinit_completion(&rpm->ack);
 	regmap_write(rpm->ipc_regmap, rpm->ipc_offset, BIT(rpm->ipc_bit));
diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
index b55cd5b..4fc1c7e 100644
--- a/drivers/regulator/qcom_rpm-regulator.c
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -198,6 +198,7 @@ static int rpm_reg_write(struct qcom_rpm_reg *vreg,
 	vreg->val[req->word] |= value << req->shift;
 
 	return qcom_rpm_write(vreg->rpm,
+			      RPM_ACTIVE_STATE,
 			      vreg->resource,
 			      vreg->val,
 			      vreg->parts->request_len);
diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h
index a60798d..f0e70b2 100644
--- a/include/linux/mfd/qcom_rpm.h
+++ b/include/linux/mfd/qcom_rpm.h
@@ -5,6 +5,9 @@
 
 struct qcom_rpm;
 
-int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count);
+#define RPM_ACTIVE_STATE	0
+#define RPM_SLEEP_STATE		1
+
+int qcom_rpm_write(struct qcom_rpm *rpm, int state, int resource, u32 *buf, size_t count);
 
 #endif
-- 
1.7.9.5

[RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Bjorn Andersson]

Some regulators are used to power e.g. PLLs that clocks the core we're
running on, so we can't turn them off directly; but we do want them off
when the core is powered down. To handle this the Qualcomm SoC provides
a means to specify a "sleep state" for RPM resources that can be
triggered by the hardware when the cores are brought down.

Resources of this type should be considered always-on while the CPU(s)
are running and we can utilize the "sleep state" functionality to defer
disabling them until the CPU(s) go to sleep, if not used by other
peripherals at that time.

Other properties are kept in sync between the states, so that if the
CPU(s) go to sleep with a particular regulator still enabled there will
be no change.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++
 drivers/regulator/qcom_rpm-regulator.c             |   68 +++++++++++++++-----
 2 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
index 4264021..4671eef 100644
--- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
+++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
@@ -112,6 +112,13 @@ of valid subnodes that can operate on these resources.
 	Definition: select that the power supply should operate in hysteretic
 		    mode, instead of the default pwm mode
 
+- qcom,rpm-assisted-disable:
+	Usage: optional
+	Value type: <empty>
+	Definition: select that the regulator is supplying the active CPU(s)
+		    and can only be disabled with the assistans from the RPM
+		    after going to sleep
+
 Standard regulator bindings are used inside switch mode power supply subnodes.
 Check Documentation/devicetree/bindings/regulator/regulator.txt for more
 details.
@@ -160,6 +167,13 @@ details.
 						qcom,rpm-pm8921-nldo,
 						qcom,rpm-pm8921-nldo1200
 
+- qcom,rpm-assisted-disable:
+	Usage: optional
+	Value type: <empty>
+	Definition: select that the regulator is supplying the active CPU(s)
+		    and can only be disabled with the assistans from the RPM
+		    after going to sleep
+
 Standard regulator bindings are used inside switch low-dropout regulator
 subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
 more details.
@@ -190,6 +204,13 @@ more details.
 		    2740000, 2400000, 2130000, 1920000, 1750000, 1600000,
 		    1480000, 1370000, 1280000, 1200000
 
+- qcom,rpm-assisted-disable:
+	Usage: optional
+	Value type: <empty>
+	Definition: select that the regulator is supplying the active CPU(s)
+		    and can only be disabled with the assistans from the RPM
+		    after going to sleep
+
 Standard regulator bindings are used inside negative charge pump regulator
 subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
 more details.
@@ -215,6 +236,13 @@ more details.
 		    QCOM_RPM_PM8921_LVS1 - QCOM_RPM_PM8921_LVS7,
 		    QCOM_RPM_PM8921_MVS
 
+- qcom,rpm-assisted-disable:
+	Usage: optional
+	Value type: <empty>
+	Definition: select that the regulator is supplying the active CPU(s)
+		    and can only be disabled with the assistans from the RPM
+		    after going to sleep
+
 = EXAMPLE
 
 	#include <dt-bindings/mfd/qcom-rpm.h>
diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
index 4fc1c7e..2a78f59 100644
--- a/drivers/regulator/qcom_rpm-regulator.c
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -24,6 +24,7 @@
 #include <dt-bindings/mfd/qcom-rpm.h>
 
 #define MAX_REQUEST_LEN 2
+#define RPM_NUM_STATES	2
 
 struct request_member {
 	int		word;
@@ -61,13 +62,15 @@ struct qcom_rpm_reg {
 	const struct rpm_reg_parts *parts;
 
 	int resource;
-	u32 val[MAX_REQUEST_LEN];
+	u32 val[RPM_NUM_STATES][MAX_REQUEST_LEN];
 
 	int uV;
 	int is_enabled;
 
 	bool supports_force_mode_auto;
 	bool supports_force_mode_bypass;
+
+	bool rpm_assist;
 };
 
 static const struct rpm_reg_parts rpm8660_ldo_parts = {
@@ -188,19 +191,20 @@ static const struct regulator_linear_range ncp_ranges[] = {
 };
 
 static int rpm_reg_write(struct qcom_rpm_reg *vreg,
+			 int state,
 			 const struct request_member *req,
 			 const int value)
 {
 	if (WARN_ON((value << req->shift) & ~req->mask))
 		return -EINVAL;
 
-	vreg->val[req->word] &= ~req->mask;
-	vreg->val[req->word] |= value << req->shift;
+	vreg->val[state][req->word] &= ~req->mask;
+	vreg->val[state][req->word] |= value << req->shift;
 
 	return qcom_rpm_write(vreg->rpm,
-			      RPM_ACTIVE_STATE,
+			      state,
 			      vreg->resource,
-			      vreg->val,
+			      vreg->val[state],
 			      vreg->parts->request_len);
 }
 
@@ -222,8 +226,11 @@ static int rpm_reg_set_mV_sel(struct regulator_dev *rdev,
 
 	mutex_lock(&vreg->lock);
 	vreg->uV = uV;
-	if (vreg->is_enabled)
-		ret = rpm_reg_write(vreg, req, vreg->uV / 1000);
+	if (vreg->is_enabled) {
+		ret = rpm_reg_write(vreg, RPM_ACTIVE_STATE, req, vreg->uV / 1000);
+		if (!ret && vreg->rpm_assist)
+			ret = rpm_reg_write(vreg, RPM_SLEEP_STATE, req, vreg->uV / 1000);
+	}
 	mutex_unlock(&vreg->lock);
 
 	return ret;
@@ -247,8 +254,11 @@ static int rpm_reg_set_uV_sel(struct regulator_dev *rdev,
 
 	mutex_lock(&vreg->lock);
 	vreg->uV = uV;
-	if (vreg->is_enabled)
-		ret = rpm_reg_write(vreg, req, vreg->uV);
+	if (vreg->is_enabled) {
+		ret = rpm_reg_write(vreg, RPM_ACTIVE_STATE, req, vreg->uV);
+		if (!ret && vreg->rpm_assist)
+			ret = rpm_reg_write(vreg, RPM_SLEEP_STATE, req, vreg->uV);
+	}
 	mutex_unlock(&vreg->lock);
 
 	return ret;
@@ -266,13 +276,16 @@ static int rpm_reg_mV_enable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->mV;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, vreg->uV / 1000);
+	ret = rpm_reg_write(vreg, state, req, vreg->uV / 1000);
 	if (!ret)
 		vreg->is_enabled = 1;
 	mutex_unlock(&vreg->lock);
@@ -285,13 +298,16 @@ static int rpm_reg_uV_enable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->uV;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, vreg->uV);
+	ret = rpm_reg_write(vreg, state, req, vreg->uV);
 	if (!ret)
 		vreg->is_enabled = 1;
 	mutex_unlock(&vreg->lock);
@@ -304,13 +320,16 @@ static int rpm_reg_switch_enable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->enable_state;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, 1);
+	ret = rpm_reg_write(vreg, state, req, 1);
 	if (!ret)
 		vreg->is_enabled = 1;
 	mutex_unlock(&vreg->lock);
@@ -323,13 +342,16 @@ static int rpm_reg_mV_disable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->mV;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, 0);
+	ret = rpm_reg_write(vreg, state, req, 0);
 	if (!ret)
 		vreg->is_enabled = 0;
 	mutex_unlock(&vreg->lock);
@@ -342,13 +364,16 @@ static int rpm_reg_uV_disable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->uV;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, 0);
+	ret = rpm_reg_write(vreg, state, req, 0);
 	if (!ret)
 		vreg->is_enabled = 0;
 	mutex_unlock(&vreg->lock);
@@ -361,13 +386,16 @@ static int rpm_reg_switch_disable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->enable_state;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, 0);
+	ret = rpm_reg_write(vreg, state, req, 0);
 	if (!ret)
 		vreg->is_enabled = 0;
 	mutex_unlock(&vreg->lock);
@@ -590,8 +618,11 @@ static int rpm_reg_set(struct qcom_rpm_reg *vreg,
 	if (req->mask == 0 || (value << req->shift) & ~req->mask)
 		return -EINVAL;
 
-	vreg->val[req->word] &= ~req->mask;
-	vreg->val[req->word] |= value << req->shift;
+	vreg->val[RPM_ACTIVE_STATE][req->word] &= ~req->mask;
+	vreg->val[RPM_ACTIVE_STATE][req->word] |= value << req->shift;
+
+	vreg->val[RPM_SLEEP_STATE][req->word] &= ~req->mask;
+	vreg->val[RPM_SLEEP_STATE][req->word] |= value << req->shift;
 
 	return 0;
 }
@@ -690,6 +721,9 @@ static int rpm_reg_probe(struct platform_device *pdev)
 		}
 	}
 
+	key = "qcom,rpm-assisted-disable";
+	vreg->rpm_assist = of_property_read_bool(pdev->dev.of_node, key);
+
 	if (vreg->parts->freq.mask) {
 		ret = rpm_reg_of_parse_freq(&pdev->dev, vreg);
 		if (ret < 0)
-- 
1.7.9.5

[RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Bjorn Andersson]

Some regulators are used to power e.g. PLLs that clocks the core we're
running on, so we can't turn them off directly; but we do want them off
when the core is powered down. To handle this the Qualcomm SoC provides
a means to specify a "sleep state" for RPM resources that can be
triggered by the hardware when the cores are brought down.

Resources of this type should be considered always-on while the CPU(s)
are running and we can utilize the "sleep state" functionality to defer
disabling them until the CPU(s) go to sleep, if not used by other
peripherals at that time.

Other properties are kept in sync between the states, so that if the
CPU(s) go to sleep with a particular regulator still enabled there will
be no change.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++
 drivers/regulator/qcom_rpm-regulator.c             |   68 +++++++++++++++-----
 2 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
index 4264021..4671eef 100644
--- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
+++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
@@ -112,6 +112,13 @@ of valid subnodes that can operate on these resources.
 	Definition: select that the power supply should operate in hysteretic
 		    mode, instead of the default pwm mode
 
+- qcom,rpm-assisted-disable:
+	Usage: optional
+	Value type: <empty>
+	Definition: select that the regulator is supplying the active CPU(s)
+		    and can only be disabled with the assistans from the RPM
+		    after going to sleep
+
 Standard regulator bindings are used inside switch mode power supply subnodes.
 Check Documentation/devicetree/bindings/regulator/regulator.txt for more
 details.
@@ -160,6 +167,13 @@ details.
 						qcom,rpm-pm8921-nldo,
 						qcom,rpm-pm8921-nldo1200
 
+- qcom,rpm-assisted-disable:
+	Usage: optional
+	Value type: <empty>
+	Definition: select that the regulator is supplying the active CPU(s)
+		    and can only be disabled with the assistans from the RPM
+		    after going to sleep
+
 Standard regulator bindings are used inside switch low-dropout regulator
 subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
 more details.
@@ -190,6 +204,13 @@ more details.
 		    2740000, 2400000, 2130000, 1920000, 1750000, 1600000,
 		    1480000, 1370000, 1280000, 1200000
 
+- qcom,rpm-assisted-disable:
+	Usage: optional
+	Value type: <empty>
+	Definition: select that the regulator is supplying the active CPU(s)
+		    and can only be disabled with the assistans from the RPM
+		    after going to sleep
+
 Standard regulator bindings are used inside negative charge pump regulator
 subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
 more details.
@@ -215,6 +236,13 @@ more details.
 		    QCOM_RPM_PM8921_LVS1 - QCOM_RPM_PM8921_LVS7,
 		    QCOM_RPM_PM8921_MVS
 
+- qcom,rpm-assisted-disable:
+	Usage: optional
+	Value type: <empty>
+	Definition: select that the regulator is supplying the active CPU(s)
+		    and can only be disabled with the assistans from the RPM
+		    after going to sleep
+
 = EXAMPLE
 
 	#include <dt-bindings/mfd/qcom-rpm.h>
diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
index 4fc1c7e..2a78f59 100644
--- a/drivers/regulator/qcom_rpm-regulator.c
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -24,6 +24,7 @@
 #include <dt-bindings/mfd/qcom-rpm.h>
 
 #define MAX_REQUEST_LEN 2
+#define RPM_NUM_STATES	2
 
 struct request_member {
 	int		word;
@@ -61,13 +62,15 @@ struct qcom_rpm_reg {
 	const struct rpm_reg_parts *parts;
 
 	int resource;
-	u32 val[MAX_REQUEST_LEN];
+	u32 val[RPM_NUM_STATES][MAX_REQUEST_LEN];
 
 	int uV;
 	int is_enabled;
 
 	bool supports_force_mode_auto;
 	bool supports_force_mode_bypass;
+
+	bool rpm_assist;
 };
 
 static const struct rpm_reg_parts rpm8660_ldo_parts = {
@@ -188,19 +191,20 @@ static const struct regulator_linear_range ncp_ranges[] = {
 };
 
 static int rpm_reg_write(struct qcom_rpm_reg *vreg,
+			 int state,
 			 const struct request_member *req,
 			 const int value)
 {
 	if (WARN_ON((value << req->shift) & ~req->mask))
 		return -EINVAL;
 
-	vreg->val[req->word] &= ~req->mask;
-	vreg->val[req->word] |= value << req->shift;
+	vreg->val[state][req->word] &= ~req->mask;
+	vreg->val[state][req->word] |= value << req->shift;
 
 	return qcom_rpm_write(vreg->rpm,
-			      RPM_ACTIVE_STATE,
+			      state,
 			      vreg->resource,
-			      vreg->val,
+			      vreg->val[state],
 			      vreg->parts->request_len);
 }
 
@@ -222,8 +226,11 @@ static int rpm_reg_set_mV_sel(struct regulator_dev *rdev,
 
 	mutex_lock(&vreg->lock);
 	vreg->uV = uV;
-	if (vreg->is_enabled)
-		ret = rpm_reg_write(vreg, req, vreg->uV / 1000);
+	if (vreg->is_enabled) {
+		ret = rpm_reg_write(vreg, RPM_ACTIVE_STATE, req, vreg->uV / 1000);
+		if (!ret && vreg->rpm_assist)
+			ret = rpm_reg_write(vreg, RPM_SLEEP_STATE, req, vreg->uV / 1000);
+	}
 	mutex_unlock(&vreg->lock);
 
 	return ret;
@@ -247,8 +254,11 @@ static int rpm_reg_set_uV_sel(struct regulator_dev *rdev,
 
 	mutex_lock(&vreg->lock);
 	vreg->uV = uV;
-	if (vreg->is_enabled)
-		ret = rpm_reg_write(vreg, req, vreg->uV);
+	if (vreg->is_enabled) {
+		ret = rpm_reg_write(vreg, RPM_ACTIVE_STATE, req, vreg->uV);
+		if (!ret && vreg->rpm_assist)
+			ret = rpm_reg_write(vreg, RPM_SLEEP_STATE, req, vreg->uV);
+	}
 	mutex_unlock(&vreg->lock);
 
 	return ret;
@@ -266,13 +276,16 @@ static int rpm_reg_mV_enable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->mV;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, vreg->uV / 1000);
+	ret = rpm_reg_write(vreg, state, req, vreg->uV / 1000);
 	if (!ret)
 		vreg->is_enabled = 1;
 	mutex_unlock(&vreg->lock);
@@ -285,13 +298,16 @@ static int rpm_reg_uV_enable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->uV;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, vreg->uV);
+	ret = rpm_reg_write(vreg, state, req, vreg->uV);
 	if (!ret)
 		vreg->is_enabled = 1;
 	mutex_unlock(&vreg->lock);
@@ -304,13 +320,16 @@ static int rpm_reg_switch_enable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->enable_state;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, 1);
+	ret = rpm_reg_write(vreg, state, req, 1);
 	if (!ret)
 		vreg->is_enabled = 1;
 	mutex_unlock(&vreg->lock);
@@ -323,13 +342,16 @@ static int rpm_reg_mV_disable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->mV;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, 0);
+	ret = rpm_reg_write(vreg, state, req, 0);
 	if (!ret)
 		vreg->is_enabled = 0;
 	mutex_unlock(&vreg->lock);
@@ -342,13 +364,16 @@ static int rpm_reg_uV_disable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->uV;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, 0);
+	ret = rpm_reg_write(vreg, state, req, 0);
 	if (!ret)
 		vreg->is_enabled = 0;
 	mutex_unlock(&vreg->lock);
@@ -361,13 +386,16 @@ static int rpm_reg_switch_disable(struct regulator_dev *rdev)
 	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
 	const struct rpm_reg_parts *parts = vreg->parts;
 	const struct request_member *req = &parts->enable_state;
+	int state;
 	int ret;
 
 	if (req->mask == 0)
 		return -EINVAL;
 
+	state = vreg->rpm_assist ? RPM_SLEEP_STATE : RPM_ACTIVE_STATE;
+
 	mutex_lock(&vreg->lock);
-	ret = rpm_reg_write(vreg, req, 0);
+	ret = rpm_reg_write(vreg, state, req, 0);
 	if (!ret)
 		vreg->is_enabled = 0;
 	mutex_unlock(&vreg->lock);
@@ -590,8 +618,11 @@ static int rpm_reg_set(struct qcom_rpm_reg *vreg,
 	if (req->mask == 0 || (value << req->shift) & ~req->mask)
 		return -EINVAL;
 
-	vreg->val[req->word] &= ~req->mask;
-	vreg->val[req->word] |= value << req->shift;
+	vreg->val[RPM_ACTIVE_STATE][req->word] &= ~req->mask;
+	vreg->val[RPM_ACTIVE_STATE][req->word] |= value << req->shift;
+
+	vreg->val[RPM_SLEEP_STATE][req->word] &= ~req->mask;
+	vreg->val[RPM_SLEEP_STATE][req->word] |= value << req->shift;
 
 	return 0;
 }
@@ -690,6 +721,9 @@ static int rpm_reg_probe(struct platform_device *pdev)
 		}
 	}
 
+	key = "qcom,rpm-assisted-disable";
+	vreg->rpm_assist = of_property_read_bool(pdev->dev.of_node, key);
+
 	if (vreg->parts->freq.mask) {
 		ret = rpm_reg_of_parse_freq(&pdev->dev, vreg);
 		if (ret < 0)
-- 
1.7.9.5

Re: [RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Andreas Färber]

Hi,

Am 10.11.2014 um 23:52 schrieb Bjorn Andersson:
> diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> index 4264021..4671eef 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> @@ -112,6 +112,13 @@ of valid subnodes that can operate on these resources.
>  	Definition: select that the power supply should operate in hysteretic
>  		    mode, instead of the default pwm mode
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM

"assistance"

> +		    after going to sleep
> +
>  Standard regulator bindings are used inside switch mode power supply subnodes.
>  Check Documentation/devicetree/bindings/regulator/regulator.txt for more
>  details.
> @@ -160,6 +167,13 @@ details.
>  						qcom,rpm-pm8921-nldo,
>  						qcom,rpm-pm8921-nldo1200
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM

ditto

> +		    after going to sleep
> +
>  Standard regulator bindings are used inside switch low-dropout regulator
>  subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
>  more details.
> @@ -190,6 +204,13 @@ more details.
>  		    2740000, 2400000, 2130000, 1920000, 1750000, 1600000,
>  		    1480000, 1370000, 1280000, 1200000
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM

ditto

> +		    after going to sleep
> +
>  Standard regulator bindings are used inside negative charge pump regulator
>  subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
>  more details.
> @@ -215,6 +236,13 @@ more details.
>  		    QCOM_RPM_PM8921_LVS1 - QCOM_RPM_PM8921_LVS7,
>  		    QCOM_RPM_PM8921_MVS
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM

ditto

> +		    after going to sleep
> +
>  = EXAMPLE
>  
>  	#include <dt-bindings/mfd/qcom-rpm.h>
[snip]

Regards,
Andreas

-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg

[RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Andreas Färber]

Hi,

Am 10.11.2014 um 23:52 schrieb Bjorn Andersson:
> diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> index 4264021..4671eef 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> @@ -112,6 +112,13 @@ of valid subnodes that can operate on these resources.
>  	Definition: select that the power supply should operate in hysteretic
>  		    mode, instead of the default pwm mode
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM

"assistance"

> +		    after going to sleep
> +
>  Standard regulator bindings are used inside switch mode power supply subnodes.
>  Check Documentation/devicetree/bindings/regulator/regulator.txt for more
>  details.
> @@ -160,6 +167,13 @@ details.
>  						qcom,rpm-pm8921-nldo,
>  						qcom,rpm-pm8921-nldo1200
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM

ditto

> +		    after going to sleep
> +
>  Standard regulator bindings are used inside switch low-dropout regulator
>  subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
>  more details.
> @@ -190,6 +204,13 @@ more details.
>  		    2740000, 2400000, 2130000, 1920000, 1750000, 1600000,
>  		    1480000, 1370000, 1280000, 1200000
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM

ditto

> +		    after going to sleep
> +
>  Standard regulator bindings are used inside negative charge pump regulator
>  subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
>  more details.
> @@ -215,6 +236,13 @@ more details.
>  		    QCOM_RPM_PM8921_LVS1 - QCOM_RPM_PM8921_LVS7,
>  		    QCOM_RPM_PM8921_MVS
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM

ditto

> +		    after going to sleep
> +
>  = EXAMPLE
>  
>  	#include <dt-bindings/mfd/qcom-rpm.h>
[snip]

Regards,
Andreas

-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imend?rffer; HRB 21284 AG N?rnberg

Re: [RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Lee Jones]

On Mon, 10 Nov 2014, Bjorn Andersson wrote:

> Some regulators are used to power e.g. PLLs that clocks the core we're
> running on, so we can't turn them off directly; but we do want them off
> when the core is powered down. To handle this the Qualcomm SoC provides
> a means to specify a "sleep state" for RPM resources that can be
> triggered by the hardware when the cores are brought down.
> 
> Resources of this type should be considered always-on while the CPU(s)
> are running and we can utilize the "sleep state" functionality to defer
> disabling them until the CPU(s) go to sleep, if not used by other
> peripherals at that time.
> 
> Other properties are kept in sync between the states, so that if the
> CPU(s) go to sleep with a particular regulator still enabled there will
> be no change.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
>  Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++

Please place this into a seperate patch.

See: Documentation/devicetree/bindings/submitting-patches.txt

>  drivers/regulator/qcom_rpm-regulator.c             |   68 +++++++++++++++-----
>  2 files changed, 79 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> index 4264021..4671eef 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> @@ -112,6 +112,13 @@ of valid subnodes that can operate on these resources.
>  	Definition: select that the power supply should operate in hysteretic
>  		    mode, instead of the default pwm mode
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM
> +		    after going to sleep
> +

This requires a DT Ack.

>  Standard regulator bindings are used inside switch mode power supply subnodes.
>  Check Documentation/devicetree/bindings/regulator/regulator.txt for more
>  details.
> @@ -160,6 +167,13 @@ details.
>  						qcom,rpm-pm8921-nldo,
>  						qcom,rpm-pm8921-nldo1200
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM
> +		    after going to sleep
> +

Eh?  I think I'm seeing double.

>  Standard regulator bindings are used inside switch low-dropout regulator
>  subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
>  more details.
> @@ -190,6 +204,13 @@ more details.
>  		    2740000, 2400000, 2130000, 1920000, 1750000, 1600000,
>  		    1480000, 1370000, 1280000, 1200000
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM
> +		    after going to sleep
> +

Triple?

>  Standard regulator bindings are used inside negative charge pump regulator
>  subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
>  more details.
> @@ -215,6 +236,13 @@ more details.
>  		    QCOM_RPM_PM8921_LVS1 - QCOM_RPM_PM8921_LVS7,
>  		    QCOM_RPM_PM8921_MVS
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM
> +		    after going to sleep
> +

There must be a better way to document this.

>  = EXAMPLE
>  
>  	#include <dt-bindings/mfd/qcom-rpm.h>

[...]
-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Lee Jones]

On Mon, 10 Nov 2014, Bjorn Andersson wrote:

> Some regulators are used to power e.g. PLLs that clocks the core we're
> running on, so we can't turn them off directly; but we do want them off
> when the core is powered down. To handle this the Qualcomm SoC provides
> a means to specify a "sleep state" for RPM resources that can be
> triggered by the hardware when the cores are brought down.
> 
> Resources of this type should be considered always-on while the CPU(s)
> are running and we can utilize the "sleep state" functionality to defer
> disabling them until the CPU(s) go to sleep, if not used by other
> peripherals at that time.
> 
> Other properties are kept in sync between the states, so that if the
> CPU(s) go to sleep with a particular regulator still enabled there will
> be no change.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
>  Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++

Please place this into a seperate patch.

See: Documentation/devicetree/bindings/submitting-patches.txt

>  drivers/regulator/qcom_rpm-regulator.c             |   68 +++++++++++++++-----
>  2 files changed, 79 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> index 4264021..4671eef 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> @@ -112,6 +112,13 @@ of valid subnodes that can operate on these resources.
>  	Definition: select that the power supply should operate in hysteretic
>  		    mode, instead of the default pwm mode
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM
> +		    after going to sleep
> +

This requires a DT Ack.

>  Standard regulator bindings are used inside switch mode power supply subnodes.
>  Check Documentation/devicetree/bindings/regulator/regulator.txt for more
>  details.
> @@ -160,6 +167,13 @@ details.
>  						qcom,rpm-pm8921-nldo,
>  						qcom,rpm-pm8921-nldo1200
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM
> +		    after going to sleep
> +

Eh?  I think I'm seeing double.

>  Standard regulator bindings are used inside switch low-dropout regulator
>  subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
>  more details.
> @@ -190,6 +204,13 @@ more details.
>  		    2740000, 2400000, 2130000, 1920000, 1750000, 1600000,
>  		    1480000, 1370000, 1280000, 1200000
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM
> +		    after going to sleep
> +

Triple?

>  Standard regulator bindings are used inside negative charge pump regulator
>  subnodes.  Check Documentation/devicetree/bindings/regulator/regulator.txt for
>  more details.
> @@ -215,6 +236,13 @@ more details.
>  		    QCOM_RPM_PM8921_LVS1 - QCOM_RPM_PM8921_LVS7,
>  		    QCOM_RPM_PM8921_MVS
>  
> +- qcom,rpm-assisted-disable:
> +	Usage: optional
> +	Value type: <empty>
> +	Definition: select that the regulator is supplying the active CPU(s)
> +		    and can only be disabled with the assistans from the RPM
> +		    after going to sleep
> +

There must be a better way to document this.

>  = EXAMPLE
>  
>  	#include <dt-bindings/mfd/qcom-rpm.h>

[...]
-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Lee Jones]

On Mon, 10 Nov 2014, Bjorn Andersson wrote:

> Resources exposed from the RPM have an "active state" that is used during
> normal operations and a "sleep state" that is used for HW assisted sleep
> modes. Expose this in the api to let client drivers set the "sleep
> state" as well.

I assume you have users lined up which will request a sleeping state?

> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
>  drivers/mfd/qcom_rpm.c                 |    9 +++++----
>  drivers/regulator/qcom_rpm-regulator.c |    1 +
>  include/linux/mfd/qcom_rpm.h           |    5 ++++-
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mfd/qcom_rpm.c b/drivers/mfd/qcom_rpm.c
> index 0dd7a6fe..f696328 100644
> --- a/drivers/mfd/qcom_rpm.c
> +++ b/drivers/mfd/qcom_rpm.c
> @@ -67,7 +67,6 @@ struct qcom_rpm {
>  #define RPM_ACK_SELECTOR	23
>  #define RPM_SELECT_SIZE		7
>  
> -#define RPM_ACTIVE_STATE	BIT(0)
>  #define RPM_NOTIFICATION	BIT(30)
>  #define RPM_REJECTED		BIT(31)
>  
> @@ -332,7 +331,10 @@ static const struct of_device_id qcom_rpm_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, qcom_rpm_of_match);
>  
> -int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
> +int qcom_rpm_write(struct qcom_rpm *rpm,
> +		   int state,
> +		   int resource,
> +		   u32 *buf, size_t count)
>  {
>  	const struct qcom_rpm_resource *res;
>  	const struct qcom_rpm_data *data = rpm->data;
> @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
>  			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
>  	}
>  
> -	writel_relaxed(RPM_ACTIVE_STATE,
> -		       RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));

How are the state bits organised?

>  	reinit_completion(&rpm->ack);
>  	regmap_write(rpm->ipc_regmap, rpm->ipc_offset, BIT(rpm->ipc_bit));
> diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
> index b55cd5b..4fc1c7e 100644
> --- a/drivers/regulator/qcom_rpm-regulator.c
> +++ b/drivers/regulator/qcom_rpm-regulator.c
> @@ -198,6 +198,7 @@ static int rpm_reg_write(struct qcom_rpm_reg *vreg,
>  	vreg->val[req->word] |= value << req->shift;
>  
>  	return qcom_rpm_write(vreg->rpm,
> +			      RPM_ACTIVE_STATE,
>  			      vreg->resource,
>  			      vreg->val,
>  			      vreg->parts->request_len);
> diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h
> index a60798d..f0e70b2 100644
> --- a/include/linux/mfd/qcom_rpm.h
> +++ b/include/linux/mfd/qcom_rpm.h
> @@ -5,6 +5,9 @@
>  
>  struct qcom_rpm;
>  
> -int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count);
> +#define RPM_ACTIVE_STATE	0
> +#define RPM_SLEEP_STATE		1
> +
> +int qcom_rpm_write(struct qcom_rpm *rpm, int state, int resource, u32 *buf, size_t count);
>  
>  #endif

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Lee Jones]

On Mon, 10 Nov 2014, Bjorn Andersson wrote:

> Resources exposed from the RPM have an "active state" that is used during
> normal operations and a "sleep state" that is used for HW assisted sleep
> modes. Expose this in the api to let client drivers set the "sleep
> state" as well.

I assume you have users lined up which will request a sleeping state?

> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
>  drivers/mfd/qcom_rpm.c                 |    9 +++++----
>  drivers/regulator/qcom_rpm-regulator.c |    1 +
>  include/linux/mfd/qcom_rpm.h           |    5 ++++-
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mfd/qcom_rpm.c b/drivers/mfd/qcom_rpm.c
> index 0dd7a6fe..f696328 100644
> --- a/drivers/mfd/qcom_rpm.c
> +++ b/drivers/mfd/qcom_rpm.c
> @@ -67,7 +67,6 @@ struct qcom_rpm {
>  #define RPM_ACK_SELECTOR	23
>  #define RPM_SELECT_SIZE		7
>  
> -#define RPM_ACTIVE_STATE	BIT(0)
>  #define RPM_NOTIFICATION	BIT(30)
>  #define RPM_REJECTED		BIT(31)
>  
> @@ -332,7 +331,10 @@ static const struct of_device_id qcom_rpm_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, qcom_rpm_of_match);
>  
> -int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
> +int qcom_rpm_write(struct qcom_rpm *rpm,
> +		   int state,
> +		   int resource,
> +		   u32 *buf, size_t count)
>  {
>  	const struct qcom_rpm_resource *res;
>  	const struct qcom_rpm_data *data = rpm->data;
> @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
>  			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
>  	}
>  
> -	writel_relaxed(RPM_ACTIVE_STATE,
> -		       RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));

How are the state bits organised?

>  	reinit_completion(&rpm->ack);
>  	regmap_write(rpm->ipc_regmap, rpm->ipc_offset, BIT(rpm->ipc_bit));
> diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
> index b55cd5b..4fc1c7e 100644
> --- a/drivers/regulator/qcom_rpm-regulator.c
> +++ b/drivers/regulator/qcom_rpm-regulator.c
> @@ -198,6 +198,7 @@ static int rpm_reg_write(struct qcom_rpm_reg *vreg,
>  	vreg->val[req->word] |= value << req->shift;
>  
>  	return qcom_rpm_write(vreg->rpm,
> +			      RPM_ACTIVE_STATE,
>  			      vreg->resource,
>  			      vreg->val,
>  			      vreg->parts->request_len);
> diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h
> index a60798d..f0e70b2 100644
> --- a/include/linux/mfd/qcom_rpm.h
> +++ b/include/linux/mfd/qcom_rpm.h
> @@ -5,6 +5,9 @@
>  
>  struct qcom_rpm;
>  
> -int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count);
> +#define RPM_ACTIVE_STATE	0
> +#define RPM_SLEEP_STATE		1
> +
> +int qcom_rpm_write(struct qcom_rpm *rpm, int state, int resource, u32 *buf, size_t count);
>  
>  #endif

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Javier Martinez Canillas]

Hello Bjorn,

On Mon, Nov 10, 2014 at 11:52 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> Some regulators are used to power e.g. PLLs that clocks the core we're
> running on, so we can't turn them off directly; but we do want them off
> when the core is powered down. To handle this the Qualcomm SoC provides
> a means to specify a "sleep state" for RPM resources that can be
> triggered by the hardware when the cores are brought down.
>

The regulator core already has support to control the state of
regulators when the system enters into a sleep state. Now the generic
regulator DT binding has also been extended to support defining if a
regulator should be "regulator-{on,off}-in-suspend". Please take a
look at the topic/suspend branch [0] in the regulator tree that has
the latest binding.

If that is not enough for your hardware and use case, I've been
working on adding initial and suspend mode for regulators. The latest
series is [1] and the needed bits for the max77802 regulator driver is
[2].

It's always better to use existing functionality if possible, instead
of adding custom per driver DT bindings. So it would be great if you
can take a look to the mentioned patches.

>  Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++
>  drivers/regulator/qcom_rpm-regulator.c             |   68 +++++++++++++++-----

Against which tree this patch has been developed? afaict
Documentation/devicetree/bindings/mfd/qcom-rpm.txt does not exist even
in the latest for-next [3] branch of the mfd tree.

>
>         #include <dt-bindings/mfd/qcom-rpm.h>

This file doesn't exit either and the regulator driver depends on
MFD_QCOM_RPM which also is not present in mainline.

By looking at the mail archives I see that you posted both the
regulator and mfd driver in the same series [4] but only the regulator
driver was picked by Mark so I guess Lee has to pick the mfd driver in
order to allow the regulator driver to be built.

Best regards,
Javier

[0]: https://git.kernel.org/cgit/linux/kernel/git/broonie/regulator.git/log/?h=topic/suspend
[1]: https://lkml.org/lkml/2014/11/10/325
[2]: https://lkml.org/lkml/2014/11/11/403
[3]: https://git.kernel.org/cgit/linux/kernel/git/lee/mfd.git/log/?h=for-mfd-next
[4]: https://lkml.org/lkml/2014/9/22/731

[RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Javier Martinez Canillas]

Hello Bjorn,

On Mon, Nov 10, 2014 at 11:52 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> Some regulators are used to power e.g. PLLs that clocks the core we're
> running on, so we can't turn them off directly; but we do want them off
> when the core is powered down. To handle this the Qualcomm SoC provides
> a means to specify a "sleep state" for RPM resources that can be
> triggered by the hardware when the cores are brought down.
>

The regulator core already has support to control the state of
regulators when the system enters into a sleep state. Now the generic
regulator DT binding has also been extended to support defining if a
regulator should be "regulator-{on,off}-in-suspend". Please take a
look at the topic/suspend branch [0] in the regulator tree that has
the latest binding.

If that is not enough for your hardware and use case, I've been
working on adding initial and suspend mode for regulators. The latest
series is [1] and the needed bits for the max77802 regulator driver is
[2].

It's always better to use existing functionality if possible, instead
of adding custom per driver DT bindings. So it would be great if you
can take a look to the mentioned patches.

>  Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++
>  drivers/regulator/qcom_rpm-regulator.c             |   68 +++++++++++++++-----

Against which tree this patch has been developed? afaict
Documentation/devicetree/bindings/mfd/qcom-rpm.txt does not exist even
in the latest for-next [3] branch of the mfd tree.

>
>         #include <dt-bindings/mfd/qcom-rpm.h>

This file doesn't exit either and the regulator driver depends on
MFD_QCOM_RPM which also is not present in mainline.

By looking at the mail archives I see that you posted both the
regulator and mfd driver in the same series [4] but only the regulator
driver was picked by Mark so I guess Lee has to pick the mfd driver in
order to allow the regulator driver to be built.

Best regards,
Javier

[0]: https://git.kernel.org/cgit/linux/kernel/git/broonie/regulator.git/log/?h=topic/suspend
[1]: https://lkml.org/lkml/2014/11/10/325
[2]: https://lkml.org/lkml/2014/11/11/403
[3]: https://git.kernel.org/cgit/linux/kernel/git/lee/mfd.git/log/?h=for-mfd-next
[4]: https://lkml.org/lkml/2014/9/22/731

Re: [RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Bjorn Andersson]

On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:

> On Mon, 10 Nov 2014, Bjorn Andersson wrote:
> 
> > Resources exposed from the RPM have an "active state" that is used during
> > normal operations and a "sleep state" that is used for HW assisted sleep
> > modes. Expose this in the api to let client drivers set the "sleep
> > state" as well.
> 
> I assume you have users lined up which will request a sleeping state?
> 

All users of this interface (regulators, clocks and bus scaling) will have to
be able to specify this.

[..]
> > @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
> >  			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
> >  	}
> >  
> > -	writel_relaxed(RPM_ACTIVE_STATE,
> > -		       RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> 
> How are the state bits organised?
> 

BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
some sanity checking here if you would like to.

[..]
> > diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h
> > index a60798d..f0e70b2 100644
> > --- a/include/linux/mfd/qcom_rpm.h
> > +++ b/include/linux/mfd/qcom_rpm.h
> > +#define RPM_ACTIVE_STATE	0
> > +#define RPM_SLEEP_STATE		1

Regards,
Bjorn

[RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Bjorn Andersson]

On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:

> On Mon, 10 Nov 2014, Bjorn Andersson wrote:
> 
> > Resources exposed from the RPM have an "active state" that is used during
> > normal operations and a "sleep state" that is used for HW assisted sleep
> > modes. Expose this in the api to let client drivers set the "sleep
> > state" as well.
> 
> I assume you have users lined up which will request a sleeping state?
> 

All users of this interface (regulators, clocks and bus scaling) will have to
be able to specify this.

[..]
> > @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
> >  			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
> >  	}
> >  
> > -	writel_relaxed(RPM_ACTIVE_STATE,
> > -		       RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> 
> How are the state bits organised?
> 

BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
some sanity checking here if you would like to.

[..]
> > diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h
> > index a60798d..f0e70b2 100644
> > --- a/include/linux/mfd/qcom_rpm.h
> > +++ b/include/linux/mfd/qcom_rpm.h
> > +#define RPM_ACTIVE_STATE	0
> > +#define RPM_SLEEP_STATE		1

Regards,
Bjorn

Re: [RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Bjorn Andersson]

On Tue 11 Nov 01:11 PST 2014, Andreas F?rber wrote:

> Hi,
> 
> Am 10.11.2014 um 23:52 schrieb Bjorn Andersson:
> > diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > index 4264021..4671eef 100644
> > --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > @@ -112,6 +112,13 @@ of valid subnodes that can operate on these resources.
> >  	Definition: select that the power supply should operate in hysteretic
> >  		    mode, instead of the default pwm mode
> >  
> > +- qcom,rpm-assisted-disable:
> > +	Usage: optional
> > +	Value type: <empty>
> > +	Definition: select that the regulator is supplying the active CPU(s)
> > +		    and can only be disabled with the assistans from the RPM
> 
> "assistance"
> 

Thanks

Regards,
Bjorn

[RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Bjorn Andersson]

On Tue 11 Nov 01:11 PST 2014, Andreas F?rber wrote:

> Hi,
> 
> Am 10.11.2014 um 23:52 schrieb Bjorn Andersson:
> > diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > index 4264021..4671eef 100644
> > --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > @@ -112,6 +112,13 @@ of valid subnodes that can operate on these resources.
> >  	Definition: select that the power supply should operate in hysteretic
> >  		    mode, instead of the default pwm mode
> >  
> > +- qcom,rpm-assisted-disable:
> > +	Usage: optional
> > +	Value type: <empty>
> > +	Definition: select that the regulator is supplying the active CPU(s)
> > +		    and can only be disabled with the assistans from the RPM
> 
> "assistance"
> 

Thanks

Regards,
Bjorn

Re: [RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Bjorn Andersson]

On Tue 11 Nov 03:59 PST 2014, Lee Jones wrote:

> On Mon, 10 Nov 2014, Bjorn Andersson wrote:
> 
> > Some regulators are used to power e.g. PLLs that clocks the core we're
> > running on, so we can't turn them off directly; but we do want them off
> > when the core is powered down. To handle this the Qualcomm SoC provides
> > a means to specify a "sleep state" for RPM resources that can be
> > triggered by the hardware when the cores are brought down.
> > 
> > Resources of this type should be considered always-on while the CPU(s)
> > are running and we can utilize the "sleep state" functionality to defer
> > disabling them until the CPU(s) go to sleep, if not used by other
> > peripherals at that time.
> > 
> > Other properties are kept in sync between the states, so that if the
> > CPU(s) go to sleep with a particular regulator still enabled there will
> > be no change.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++
> 
> Please place this into a seperate patch.
> 
> See: Documentation/devicetree/bindings/submitting-patches.txt
> 

Yes, I'm aware. As you probably remember we still haven't gotten an ack on the
original file, so given the outcome of the discussion of the design of this I
plan to fold it into the original patch and send out a new version.

> >  drivers/regulator/qcom_rpm-regulator.c             |   68 +++++++++++++++-----
> >  2 files changed, 79 insertions(+), 17 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > index 4264021..4671eef 100644
> > --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > @@ -112,6 +112,13 @@ of valid subnodes that can operate on these resources.
> >  	Definition: select that the power supply should operate in hysteretic
> >  		    mode, instead of the default pwm mode
> >  
> > +- qcom,rpm-assisted-disable:
> > +	Usage: optional
> > +	Value type: <empty>
> > +	Definition: select that the regulator is supplying the active CPU(s)
> > +		    and can only be disabled with the assistans from the RPM
> > +		    after going to sleep
> > +
> 
> This requires a DT Ack.
> 

The entire file still requires a dt ack, but the "sleep state" will change the
binding so let's see where this RFC takes us.

I would have preferred if someone gave me the "sleep state" feedback once we
had this out for review, but it didn't reach the right people at Qualcomm at
the time.

> >  Standard regulator bindings are used inside switch mode power supply subnodes.
> >  Check Documentation/devicetree/bindings/regulator/regulator.txt for more
> >  details.
> > @@ -160,6 +167,13 @@ details.
> >  						qcom,rpm-pm8921-nldo,
> >  						qcom,rpm-pm8921-nldo1200
> >  
> > +- qcom,rpm-assisted-disable:
> > +	Usage: optional
> > +	Value type: <empty>
> > +	Definition: select that the regulator is supplying the active CPU(s)
> > +		    and can only be disabled with the assistans from the RPM
> > +		    after going to sleep
> > +
> 
> Eh?  I think I'm seeing double.
> 

You're right, it would make sense to move this to a "common subnode properties"
section.

Regards,
Bjorn

[RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Bjorn Andersson]

On Tue 11 Nov 03:59 PST 2014, Lee Jones wrote:

> On Mon, 10 Nov 2014, Bjorn Andersson wrote:
> 
> > Some regulators are used to power e.g. PLLs that clocks the core we're
> > running on, so we can't turn them off directly; but we do want them off
> > when the core is powered down. To handle this the Qualcomm SoC provides
> > a means to specify a "sleep state" for RPM resources that can be
> > triggered by the hardware when the cores are brought down.
> > 
> > Resources of this type should be considered always-on while the CPU(s)
> > are running and we can utilize the "sleep state" functionality to defer
> > disabling them until the CPU(s) go to sleep, if not used by other
> > peripherals at that time.
> > 
> > Other properties are kept in sync between the states, so that if the
> > CPU(s) go to sleep with a particular regulator still enabled there will
> > be no change.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++
> 
> Please place this into a seperate patch.
> 
> See: Documentation/devicetree/bindings/submitting-patches.txt
> 

Yes, I'm aware. As you probably remember we still haven't gotten an ack on the
original file, so given the outcome of the discussion of the design of this I
plan to fold it into the original patch and send out a new version.

> >  drivers/regulator/qcom_rpm-regulator.c             |   68 +++++++++++++++-----
> >  2 files changed, 79 insertions(+), 17 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > index 4264021..4671eef 100644
> > --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
> > @@ -112,6 +112,13 @@ of valid subnodes that can operate on these resources.
> >  	Definition: select that the power supply should operate in hysteretic
> >  		    mode, instead of the default pwm mode
> >  
> > +- qcom,rpm-assisted-disable:
> > +	Usage: optional
> > +	Value type: <empty>
> > +	Definition: select that the regulator is supplying the active CPU(s)
> > +		    and can only be disabled with the assistans from the RPM
> > +		    after going to sleep
> > +
> 
> This requires a DT Ack.
> 

The entire file still requires a dt ack, but the "sleep state" will change the
binding so let's see where this RFC takes us.

I would have preferred if someone gave me the "sleep state" feedback once we
had this out for review, but it didn't reach the right people at Qualcomm at
the time.

> >  Standard regulator bindings are used inside switch mode power supply subnodes.
> >  Check Documentation/devicetree/bindings/regulator/regulator.txt for more
> >  details.
> > @@ -160,6 +167,13 @@ details.
> >  						qcom,rpm-pm8921-nldo,
> >  						qcom,rpm-pm8921-nldo1200
> >  
> > +- qcom,rpm-assisted-disable:
> > +	Usage: optional
> > +	Value type: <empty>
> > +	Definition: select that the regulator is supplying the active CPU(s)
> > +		    and can only be disabled with the assistans from the RPM
> > +		    after going to sleep
> > +
> 
> Eh?  I think I'm seeing double.
> 

You're right, it would make sense to move this to a "common subnode properties"
section.

Regards,
Bjorn

Re: [RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Bjorn Andersson]

On Tue 11 Nov 06:21 PST 2014, Javier Martinez Canillas wrote:

> Hello Bjorn,
> 

Hi Javier,

> On Mon, Nov 10, 2014 at 11:52 PM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
> > Some regulators are used to power e.g. PLLs that clocks the core we're
> > running on, so we can't turn them off directly; but we do want them off
> > when the core is powered down. To handle this the Qualcomm SoC provides
> > a means to specify a "sleep state" for RPM resources that can be
> > triggered by the hardware when the cores are brought down.
> >
> 
> The regulator core already has support to control the state of
> regulators when the system enters into a sleep state. Now the generic
> regulator DT binding has also been extended to support defining if a
> regulator should be "regulator-{on,off}-in-suspend". Please take a
> look at the topic/suspend branch [0] in the regulator tree that has
> the latest binding.
> 

I was planning to utilize the suspend states functionality in the core and was
looking at implementing [0] myself, so glad that's coming in place.

However, as a practical example we have LDO12 on the MSM8974 SoC that is used
for powering CPU PLLs, WiFi/BT PLLs, display, camera sensor and hdmi.

In a phone it's most reasonable to expect the WiFi core keeping a vote for the
regulators to be enabled during a suspend, but if you're in airplane mode (or
WiFi is turned off) you want to save the power - so it's not possible to
configure this statically.

Further more, the CPU vote is not tied to suspend state but rather cpuidle
state. It's not unreasonable to think of a state where we're clocking out
pixels to the display in Android with the CPU turned off and hence the CPU PLL
vote lifted.

> If that is not enough for your hardware and use case, I've been
> working on adding initial and suspend mode for regulators. The latest
> series is [1] and the needed bits for the max77802 regulator driver is
> [2].
> 

Looks good.

> It's always better to use existing functionality if possible, instead
> of adding custom per driver DT bindings. So it would be great if you
> can take a look to the mentioned patches.
> 

I totally agree, but due to the dynamic nature described above I have a hard
time figuring out how to make it fit; hence this RFC.

> >  Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++
> >  drivers/regulator/qcom_rpm-regulator.c             |   68 +++++++++++++++-----
> 
> Against which tree this patch has been developed? afaict
> Documentation/devicetree/bindings/mfd/qcom-rpm.txt does not exist even
> in the latest for-next [3] branch of the mfd tree.
> 

v3.18-rc1 + https://lkml.org/lkml/2014/9/22/731

As I tried to explain in the cover letter, the DT bindings that I change here
are not acked and this RFC is an attempt to come to a conclusion in how to
design the sleep state part - which will change the bindings.

> >
> >         #include <dt-bindings/mfd/qcom-rpm.h>
> 
> This file doesn't exit either and the regulator driver depends on
> MFD_QCOM_RPM which also is not present in mainline.
> 
> By looking at the mail archives I see that you posted both the
> regulator and mfd driver in the same series [4] but only the regulator
> driver was picked by Mark so I guess Lee has to pick the mfd driver in
> order to allow the regulator driver to be built.
> 

Correct, I hope that after sorting the sleep state part out we can get some
acks on things.

Thanks for your review!

Regards,
Bjorn

[RFC 2/2] regulator: qcom-rpm: Implement RPM assisted disable

[Bjorn Andersson]

On Tue 11 Nov 06:21 PST 2014, Javier Martinez Canillas wrote:

> Hello Bjorn,
> 

Hi Javier,

> On Mon, Nov 10, 2014 at 11:52 PM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
> > Some regulators are used to power e.g. PLLs that clocks the core we're
> > running on, so we can't turn them off directly; but we do want them off
> > when the core is powered down. To handle this the Qualcomm SoC provides
> > a means to specify a "sleep state" for RPM resources that can be
> > triggered by the hardware when the cores are brought down.
> >
> 
> The regulator core already has support to control the state of
> regulators when the system enters into a sleep state. Now the generic
> regulator DT binding has also been extended to support defining if a
> regulator should be "regulator-{on,off}-in-suspend". Please take a
> look at the topic/suspend branch [0] in the regulator tree that has
> the latest binding.
> 

I was planning to utilize the suspend states functionality in the core and was
looking at implementing [0] myself, so glad that's coming in place.

However, as a practical example we have LDO12 on the MSM8974 SoC that is used
for powering CPU PLLs, WiFi/BT PLLs, display, camera sensor and hdmi.

In a phone it's most reasonable to expect the WiFi core keeping a vote for the
regulators to be enabled during a suspend, but if you're in airplane mode (or
WiFi is turned off) you want to save the power - so it's not possible to
configure this statically.

Further more, the CPU vote is not tied to suspend state but rather cpuidle
state. It's not unreasonable to think of a state where we're clocking out
pixels to the display in Android with the CPU turned off and hence the CPU PLL
vote lifted.

> If that is not enough for your hardware and use case, I've been
> working on adding initial and suspend mode for regulators. The latest
> series is [1] and the needed bits for the max77802 regulator driver is
> [2].
> 

Looks good.

> It's always better to use existing functionality if possible, instead
> of adding custom per driver DT bindings. So it would be great if you
> can take a look to the mentioned patches.
> 

I totally agree, but due to the dynamic nature described above I have a hard
time figuring out how to make it fit; hence this RFC.

> >  Documentation/devicetree/bindings/mfd/qcom-rpm.txt |   28 ++++++++
> >  drivers/regulator/qcom_rpm-regulator.c             |   68 +++++++++++++++-----
> 
> Against which tree this patch has been developed? afaict
> Documentation/devicetree/bindings/mfd/qcom-rpm.txt does not exist even
> in the latest for-next [3] branch of the mfd tree.
> 

v3.18-rc1 + https://lkml.org/lkml/2014/9/22/731

As I tried to explain in the cover letter, the DT bindings that I change here
are not acked and this RFC is an attempt to come to a conclusion in how to
design the sleep state part - which will change the bindings.

> >
> >         #include <dt-bindings/mfd/qcom-rpm.h>
> 
> This file doesn't exit either and the regulator driver depends on
> MFD_QCOM_RPM which also is not present in mainline.
> 
> By looking at the mail archives I see that you posted both the
> regulator and mfd driver in the same series [4] but only the regulator
> driver was picked by Mark so I guess Lee has to pick the mfd driver in
> order to allow the regulator driver to be built.
> 

Correct, I hope that after sorting the sleep state part out we can get some
acks on things.

Thanks for your review!

Regards,
Bjorn

Re: [RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Lee Jones]

On Tue, 11 Nov 2014, Bjorn Andersson wrote:

> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:
> 
> > On Mon, 10 Nov 2014, Bjorn Andersson wrote:
> > 
> > > Resources exposed from the RPM have an "active state" that is used during
> > > normal operations and a "sleep state" that is used for HW assisted sleep
> > > modes. Expose this in the api to let client drivers set the "sleep
> > > state" as well.
> > 
> > I assume you have users lined up which will request a sleeping state?
> > 
> 
> All users of this interface (regulators, clocks and bus scaling) will have to
> be able to specify this.
> 
> [..]
> > > @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
> > >  			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
> > >  	}
> > >  
> > > -	writel_relaxed(RPM_ACTIVE_STATE,
> > > -		       RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> > > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> > 
> > How are the state bits organised?
> > 
> 
> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
> some sanity checking here if you would like to.

I'm just double checking that you know what that means.

BIT(0) == b01
BIT(1) == b10

It seems strange to represent a single boolean state over 2 bits.

Also, what happens if b11 or b00 occurs?

> [..]
> > > diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h
> > > index a60798d..f0e70b2 100644
> > > --- a/include/linux/mfd/qcom_rpm.h
> > > +++ b/include/linux/mfd/qcom_rpm.h
> > > +#define RPM_ACTIVE_STATE	0
> > > +#define RPM_SLEEP_STATE		1
> 
> Regards,
> Bjorn

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Lee Jones]

On Tue, 11 Nov 2014, Bjorn Andersson wrote:

> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:
> 
> > On Mon, 10 Nov 2014, Bjorn Andersson wrote:
> > 
> > > Resources exposed from the RPM have an "active state" that is used during
> > > normal operations and a "sleep state" that is used for HW assisted sleep
> > > modes. Expose this in the api to let client drivers set the "sleep
> > > state" as well.
> > 
> > I assume you have users lined up which will request a sleeping state?
> > 
> 
> All users of this interface (regulators, clocks and bus scaling) will have to
> be able to specify this.
> 
> [..]
> > > @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
> > >  			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
> > >  	}
> > >  
> > > -	writel_relaxed(RPM_ACTIVE_STATE,
> > > -		       RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> > > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> > 
> > How are the state bits organised?
> > 
> 
> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
> some sanity checking here if you would like to.

I'm just double checking that you know what that means.

BIT(0) == b01
BIT(1) == b10

It seems strange to represent a single boolean state over 2 bits.

Also, what happens if b11 or b00 occurs?

> [..]
> > > diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h
> > > index a60798d..f0e70b2 100644
> > > --- a/include/linux/mfd/qcom_rpm.h
> > > +++ b/include/linux/mfd/qcom_rpm.h
> > > +#define RPM_ACTIVE_STATE	0
> > > +#define RPM_SLEEP_STATE		1
> 
> Regards,
> Bjorn

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Lina Iyer]

On Wed, Nov 12 2014 at 02:52 -0700, Lee Jones wrote:
>On Tue, 11 Nov 2014, Bjorn Andersson wrote:
>
>> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:
>>
>> > On Mon, 10 Nov 2014, Bjorn Andersson wrote:
>> >

> > > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
>> >
>> > How are the state bits organised?
>> >
>>
>> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
>> some sanity checking here if you would like to.
>
>I'm just double checking that you know what that means.
>
>BIT(0) == b01
>BIT(1) == b10
>
>It seems strange to represent a single boolean state over 2 bits.
>
>Also, what happens if b11 or b00 occurs?
>
Lee is correct, it should be 0 for Active and 1 for Sleep set. 


Lina

[RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Lina Iyer]

On Wed, Nov 12 2014 at 02:52 -0700, Lee Jones wrote:
>On Tue, 11 Nov 2014, Bjorn Andersson wrote:
>
>> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:
>>
>> > On Mon, 10 Nov 2014, Bjorn Andersson wrote:
>> >

> > > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
>> >
>> > How are the state bits organised?
>> >
>>
>> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
>> some sanity checking here if you would like to.
>
>I'm just double checking that you know what that means.
>
>BIT(0) == b01
>BIT(1) == b10
>
>It seems strange to represent a single boolean state over 2 bits.
>
>Also, what happens if b11 or b00 occurs?
>
Lee is correct, it should be 0 for Active and 1 for Sleep set. 


Lina

Re: [RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Bjorn Andersson]

On Wed 12 Nov 06:45 PST 2014, Lina Iyer wrote:

> On Wed, Nov 12 2014 at 02:52 -0700, Lee Jones wrote:
> >On Tue, 11 Nov 2014, Bjorn Andersson wrote:
> >
> >> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:
> >>
> >> > On Mon, 10 Nov 2014, Bjorn Andersson wrote:
> >> >
> 
> > > > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> >> >
> >> > How are the state bits organised?
> >> >
> >>
> >> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
> >> some sanity checking here if you would like to.
> >
> >I'm just double checking that you know what that means.
> >
> >BIT(0) == b01
> >BIT(1) == b10
> >
> >It seems strange to represent a single boolean state over 2 bits.
> >
> >Also, what happens if b11 or b00 occurs?
> >
> Lee is correct, it should be 0 for Active and 1 for Sleep set. 
> 

In the caf msm-3.4 tree the regulator core will call e.g. vreg_set_voltage()
that will call vreg_set(), that will call msm_rpm_set(MSM_RPM_CTX_SET_0,...).
That comes from the following:

enum {
	MSM_RPM_CTX_SET_0,
	MSM_RPM_CTX_SET_SLEEP,
	MSM_RPM_CTX_SET_COUNT,

	MSM_RPM_CTX_NOTIFICATION = 30,
	MSM_RPM_CTX_REJECTED = 31,
};

So there's your 0 and 1.

msm_rpm_set() calls msm_rpm_set_common() that calls msm_rpm_set_exclusive()
that contains these two statements:

        uint32_t ctx_mask = msm_rpm_get_ctx_mask(ctx);
	...
	msm_rpm_write(MSM_RPM_PAGE_CTRL, target_ctrl(MSM_RPM_CTRL_REQ_CTX_0), ctx_mask);

And we have:

static inline uint32_t msm_rpm_get_ctx_mask(unsigned int ctx)
{
	return 1UL << ctx;
}

So, as far as I can see it should be BIT(state) here. But there's a lot of code
and a lot of indirections here and I've been tricked by it before, so please
let me know if I got something wrong on the way.

Regards,
Bjorn

[RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Bjorn Andersson]

On Wed 12 Nov 06:45 PST 2014, Lina Iyer wrote:

> On Wed, Nov 12 2014 at 02:52 -0700, Lee Jones wrote:
> >On Tue, 11 Nov 2014, Bjorn Andersson wrote:
> >
> >> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:
> >>
> >> > On Mon, 10 Nov 2014, Bjorn Andersson wrote:
> >> >
> 
> > > > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> >> >
> >> > How are the state bits organised?
> >> >
> >>
> >> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
> >> some sanity checking here if you would like to.
> >
> >I'm just double checking that you know what that means.
> >
> >BIT(0) == b01
> >BIT(1) == b10
> >
> >It seems strange to represent a single boolean state over 2 bits.
> >
> >Also, what happens if b11 or b00 occurs?
> >
> Lee is correct, it should be 0 for Active and 1 for Sleep set. 
> 

In the caf msm-3.4 tree the regulator core will call e.g. vreg_set_voltage()
that will call vreg_set(), that will call msm_rpm_set(MSM_RPM_CTX_SET_0,...).
That comes from the following:

enum {
	MSM_RPM_CTX_SET_0,
	MSM_RPM_CTX_SET_SLEEP,
	MSM_RPM_CTX_SET_COUNT,

	MSM_RPM_CTX_NOTIFICATION = 30,
	MSM_RPM_CTX_REJECTED = 31,
};

So there's your 0 and 1.

msm_rpm_set() calls msm_rpm_set_common() that calls msm_rpm_set_exclusive()
that contains these two statements:

        uint32_t ctx_mask = msm_rpm_get_ctx_mask(ctx);
	...
	msm_rpm_write(MSM_RPM_PAGE_CTRL, target_ctrl(MSM_RPM_CTRL_REQ_CTX_0), ctx_mask);

And we have:

static inline uint32_t msm_rpm_get_ctx_mask(unsigned int ctx)
{
	return 1UL << ctx;
}

So, as far as I can see it should be BIT(state) here. But there's a lot of code
and a lot of indirections here and I've been tricked by it before, so please
let me know if I got something wrong on the way.

Regards,
Bjorn

Re: [RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Bjorn Andersson]

On Wed 12 Nov 01:52 PST 2014, Lee Jones wrote:

> On Tue, 11 Nov 2014, Bjorn Andersson wrote:
> 
> > On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:
> > 
> > > On Mon, 10 Nov 2014, Bjorn Andersson wrote:
> > > 
> > > > Resources exposed from the RPM have an "active state" that is used during
> > > > normal operations and a "sleep state" that is used for HW assisted sleep
> > > > modes. Expose this in the api to let client drivers set the "sleep
> > > > state" as well.
> > > 
> > > I assume you have users lined up which will request a sleeping state?
> > > 
> > 
> > All users of this interface (regulators, clocks and bus scaling) will have to
> > be able to specify this.
> > 
> > [..]
> > > > @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
> > > >  			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
> > > >  	}
> > > >  
> > > > -	writel_relaxed(RPM_ACTIVE_STATE,
> > > > -		       RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> > > > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> > > 
> > > How are the state bits organised?
> > > 
> > 
> > BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
> > some sanity checking here if you would like to.
> 
> I'm just double checking that you know what that means.
> 
> BIT(0) == b01
> BIT(1) == b10
> 
> It seems strange to represent a single boolean state over 2 bits.
> 

I'm aware of this and find most parts of this solution to be strange.

> Also, what happens if b11 or b00 occurs?
> 

Had to pull out the firmware for the other side; it boils down to a count of
trailing zeros, with 0 giving 0, that is then turned back into and enum where
ACTIVE is 0 and SLEEP is 1. This is then sanity checked to be less than 2.

So the possible values and results would be:
 00 => ACTIVE (seems invalid but "accidentally" accepted)
 01 => ACTIVE
 10 => SLEEP
 11 => ACTIVE

In one of the later versions of the RPM (not applicable for any of these
platforms) 2 seems to be a valid choice as well; but there it's passed as an
u32 and not a bitmask.

Regards,
Bjorn

[RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Bjorn Andersson]

On Wed 12 Nov 01:52 PST 2014, Lee Jones wrote:

> On Tue, 11 Nov 2014, Bjorn Andersson wrote:
> 
> > On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:
> > 
> > > On Mon, 10 Nov 2014, Bjorn Andersson wrote:
> > > 
> > > > Resources exposed from the RPM have an "active state" that is used during
> > > > normal operations and a "sleep state" that is used for HW assisted sleep
> > > > modes. Expose this in the api to let client drivers set the "sleep
> > > > state" as well.
> > > 
> > > I assume you have users lined up which will request a sleeping state?
> > > 
> > 
> > All users of this interface (regulators, clocks and bus scaling) will have to
> > be able to specify this.
> > 
> > [..]
> > > > @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count)
> > > >  			       RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i));
> > > >  	}
> > > >  
> > > > -	writel_relaxed(RPM_ACTIVE_STATE,
> > > > -		       RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> > > > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
> > > 
> > > How are the state bits organised?
> > > 
> > 
> > BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
> > some sanity checking here if you would like to.
> 
> I'm just double checking that you know what that means.
> 
> BIT(0) == b01
> BIT(1) == b10
> 
> It seems strange to represent a single boolean state over 2 bits.
> 

I'm aware of this and find most parts of this solution to be strange.

> Also, what happens if b11 or b00 occurs?
> 

Had to pull out the firmware for the other side; it boils down to a count of
trailing zeros, with 0 giving 0, that is then turned back into and enum where
ACTIVE is 0 and SLEEP is 1. This is then sanity checked to be less than 2.

So the possible values and results would be:
 00 => ACTIVE (seems invalid but "accidentally" accepted)
 01 => ACTIVE
 10 => SLEEP
 11 => ACTIVE

In one of the later versions of the RPM (not applicable for any of these
platforms) 2 seems to be a valid choice as well; but there it's passed as an
u32 and not a bitmask.

Regards,
Bjorn

Re: [RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Lina Iyer]

On Wed, Nov 12 2014 at 12:23 -0700, Bjorn Andersson wrote:
>On Wed 12 Nov 06:45 PST 2014, Lina Iyer wrote:
>
>> On Wed, Nov 12 2014 at 02:52 -0700, Lee Jones wrote:
>> >On Tue, 11 Nov 2014, Bjorn Andersson wrote:
>> >
>> >> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:
>> >>
>> >> > On Mon, 10 Nov 2014, Bjorn Andersson wrote:
>> >> >
>>
>> > > > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
>> >> >
>> >> > How are the state bits organised?
>> >> >
>> >>
>> >> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
>> >> some sanity checking here if you would like to.
>> >
>> >I'm just double checking that you know what that means.
>> >
>> >BIT(0) == b01
>> >BIT(1) == b10
>> >
>> >It seems strange to represent a single boolean state over 2 bits.
>> >
>> >Also, what happens if b11 or b00 occurs?
>> >
>> Lee is correct, it should be 0 for Active and 1 for Sleep set.
>>
>
>In the caf msm-3.4 tree the regulator core will call e.g. vreg_set_voltage()
>that will call vreg_set(), that will call msm_rpm_set(MSM_RPM_CTX_SET_0,...).
>That comes from the following:
>
>enum {
>	MSM_RPM_CTX_SET_0,
>	MSM_RPM_CTX_SET_SLEEP,
>	MSM_RPM_CTX_SET_COUNT,
>
>	MSM_RPM_CTX_NOTIFICATION = 30,
>	MSM_RPM_CTX_REJECTED = 31,
>};
>
>So there's your 0 and 1.
>
>msm_rpm_set() calls msm_rpm_set_common() that calls msm_rpm_set_exclusive()
>that contains these two statements:
>
>        uint32_t ctx_mask = msm_rpm_get_ctx_mask(ctx);
>	...
>	msm_rpm_write(MSM_RPM_PAGE_CTRL, target_ctrl(MSM_RPM_CTRL_REQ_CTX_0), ctx_mask);
>
>And we have:
>
>static inline uint32_t msm_rpm_get_ctx_mask(unsigned int ctx)
>{
>	return 1UL << ctx;
>}
>
>So, as far as I can see it should be BIT(state) here. But there's a lot of code
>and a lot of indirections here and I've been tricked by it before, so please
>let me know if I got something wrong on the way.
Sorry, I retract my earlier objection. This is how it is, strangely.

>
>Regards,
>Bjorn

[RFC 1/2] mfd: qcom-rpm: Expose sleep state resources to clients

[Lina Iyer]

On Wed, Nov 12 2014 at 12:23 -0700, Bjorn Andersson wrote:
>On Wed 12 Nov 06:45 PST 2014, Lina Iyer wrote:
>
>> On Wed, Nov 12 2014 at 02:52 -0700, Lee Jones wrote:
>> >On Tue, 11 Nov 2014, Bjorn Andersson wrote:
>> >
>> >> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote:
>> >>
>> >> > On Mon, 10 Nov 2014, Bjorn Andersson wrote:
>> >> >
>>
>> > > > +	writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT));
>> >> >
>> >> > How are the state bits organised?
>> >> >
>> >>
>> >> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add
>> >> some sanity checking here if you would like to.
>> >
>> >I'm just double checking that you know what that means.
>> >
>> >BIT(0) == b01
>> >BIT(1) == b10
>> >
>> >It seems strange to represent a single boolean state over 2 bits.
>> >
>> >Also, what happens if b11 or b00 occurs?
>> >
>> Lee is correct, it should be 0 for Active and 1 for Sleep set.
>>
>
>In the caf msm-3.4 tree the regulator core will call e.g. vreg_set_voltage()
>that will call vreg_set(), that will call msm_rpm_set(MSM_RPM_CTX_SET_0,...).
>That comes from the following:
>
>enum {
>	MSM_RPM_CTX_SET_0,
>	MSM_RPM_CTX_SET_SLEEP,
>	MSM_RPM_CTX_SET_COUNT,
>
>	MSM_RPM_CTX_NOTIFICATION = 30,
>	MSM_RPM_CTX_REJECTED = 31,
>};
>
>So there's your 0 and 1.
>
>msm_rpm_set() calls msm_rpm_set_common() that calls msm_rpm_set_exclusive()
>that contains these two statements:
>
>        uint32_t ctx_mask = msm_rpm_get_ctx_mask(ctx);
>	...
>	msm_rpm_write(MSM_RPM_PAGE_CTRL, target_ctrl(MSM_RPM_CTRL_REQ_CTX_0), ctx_mask);
>
>And we have:
>
>static inline uint32_t msm_rpm_get_ctx_mask(unsigned int ctx)
>{
>	return 1UL << ctx;
>}
>
>So, as far as I can see it should be BIT(state) here. But there's a lot of code
>and a lot of indirections here and I've been tricked by it before, so please
>let me know if I got something wrong on the way.
Sorry, I retract my earlier objection. This is how it is, strangely.

>
>Regards,
>Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

Hi Bjorn,

On 11/10/2014 02:52 PM, Bjorn Andersson wrote:
> During the review of the Qualcomm SMD RPM regulators [1], I got (offline)
> feedback that my implementation did not handle "sleep states". As the problem
> is shared between all families of Qualcomm platforms I use [2] (family A) to
> propose a solution (as I hope to get that merged sooner).
>
> The "sleep states" comment boils down to certain regulators (or rpm resources
> in general) are used by the currently clocked/running CPU(s) and can not be
> disabled while we're still running. 

This is the first flaw in the argument. There are certain regulators
that are used to power the clocks for CPUs that can be disabled while
we're running as long as the CPUs aren't using these clocks. I'll
explain further down below.

> Further more, these resources are shared
> with peripherals in the system; e.g. LDO12 on PM8941 is used to clock the CPU
> and WiFi/BT PLLs as well as providing power to the display in our devices. So
> the suspend functionality in the regulator framework doesn't cut it.

Furthermore, we can trigger a transition to the sleep set during idle
paths so suspend definitely won't cut it.

>
>
> The downstream solution to this is to expose 3 regulators per regulator
> resource, each specified to control the active mode, sleep mode or both modes
> respectively. Peripherals are directed to use the "both" regulator while the
> CPUs are directed to the "active only" regulator.
>
> After reviewing this solution and looking at what it's actually achieving I
> here propose flagging these regulators to have "deferred disable";
> * we consider the specific regulators as always-on _while running_
> * hence, disable and enable affect only the sleep state
> * we update both active and sleep state with all other properties
>
> This gives us a single regulator exposed for the resource, that will be kept on
> with parameters as specified by the clients if it's referenced and upon loosing
> the last reference (disabling all consumers) it will be turned off when the
> CPU(s) are sleeping.
>
>
> As far as I can see this should give the same behaviour as we have downstream,
> without the need for playing tricks with how we expose the regulators. However
> there's plenty of details hidden in that code, so I hope to get some feedback
> from the Qualcomm engineers on this.

First some background. The Krait CPUs are clocked by dedicated per-cpu
PLLs and a global PLL. The dedicated per-cpu PLLs are supplied by a
regulator (actually a handful of them, but let's just assume one for the
sake of argument). When we go to suspend or idle we will turn off these
dedicated per-cpu PLLs and switch the CPUs to source off the global PLL.
The global PLL is also used as the lowest rate that the CPUs can run at.
We'd like to not communicate with the RPM unnecessarily during
idle/suspend to turn off these resources when we go idle/suspend because
it takes more time and thus more power.

So the CPU really wants to only be voting for the HFPLL regulator
supplies in the active set. This way, if we're not using the HFPLLs
(i.e. the CPUs are all running off the global PLL), then we can disable
the regulator in the active set (RPM code only caches sleep set so we
won't be doing unnecessary flushes in the idle/suspend path). The clock
driver will make sure to turn off the HFPLLs before we go into a sleep
state that would trigger a switch from active to sleep set (commonly
referred to as RPM notification). It very well could be that there are
other consumers of the same regulator, but that doesn't matter to the
CPU clock driver because it only cares about the active set. Now you may
ask why can't the CPU clock driver disable the regulator when the HFPLL
is disabled? We don't do that in this case because a) it causes more RPM
communication overhead and b) we will be in atomic context when the
HFPLL is disabled during idle/suspend and the regulator APIs are
sleeping calls. In the non-idle/suspend path we will disable the regulator.

Also the active/sleep sets are about more than just on/off state. We may
have a situation where the active set voltage (or some other attribute
like current, mode, etc.) is different than the sleep set voltage. For
example, the CPU is supplied by a digital logic regulator that is shared
with other digital logic in the SoC (GPU, wifi, etc.). The CPU may
require some high voltage, but the GPU only needs some lower voltage.
The suspend/idle code relies on the fact that the GPU is voting on the
active+sleep regulator while the CPU is voting on the active only
regulator so that the RPM can automatically switch between high voltage
and low voltage when the CPU notifies the RPM that it's gone idle (or
the CPU wakes up).

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

Hi Bjorn,

On 11/10/2014 02:52 PM, Bjorn Andersson wrote:
> During the review of the Qualcomm SMD RPM regulators [1], I got (offline)
> feedback that my implementation did not handle "sleep states". As the problem
> is shared between all families of Qualcomm platforms I use [2] (family A) to
> propose a solution (as I hope to get that merged sooner).
>
> The "sleep states" comment boils down to certain regulators (or rpm resources
> in general) are used by the currently clocked/running CPU(s) and can not be
> disabled while we're still running. 

This is the first flaw in the argument. There are certain regulators
that are used to power the clocks for CPUs that can be disabled while
we're running as long as the CPUs aren't using these clocks. I'll
explain further down below.

> Further more, these resources are shared
> with peripherals in the system; e.g. LDO12 on PM8941 is used to clock the CPU
> and WiFi/BT PLLs as well as providing power to the display in our devices. So
> the suspend functionality in the regulator framework doesn't cut it.

Furthermore, we can trigger a transition to the sleep set during idle
paths so suspend definitely won't cut it.

>
>
> The downstream solution to this is to expose 3 regulators per regulator
> resource, each specified to control the active mode, sleep mode or both modes
> respectively. Peripherals are directed to use the "both" regulator while the
> CPUs are directed to the "active only" regulator.
>
> After reviewing this solution and looking at what it's actually achieving I
> here propose flagging these regulators to have "deferred disable";
> * we consider the specific regulators as always-on _while running_
> * hence, disable and enable affect only the sleep state
> * we update both active and sleep state with all other properties
>
> This gives us a single regulator exposed for the resource, that will be kept on
> with parameters as specified by the clients if it's referenced and upon loosing
> the last reference (disabling all consumers) it will be turned off when the
> CPU(s) are sleeping.
>
>
> As far as I can see this should give the same behaviour as we have downstream,
> without the need for playing tricks with how we expose the regulators. However
> there's plenty of details hidden in that code, so I hope to get some feedback
> from the Qualcomm engineers on this.

First some background. The Krait CPUs are clocked by dedicated per-cpu
PLLs and a global PLL. The dedicated per-cpu PLLs are supplied by a
regulator (actually a handful of them, but let's just assume one for the
sake of argument). When we go to suspend or idle we will turn off these
dedicated per-cpu PLLs and switch the CPUs to source off the global PLL.
The global PLL is also used as the lowest rate that the CPUs can run at.
We'd like to not communicate with the RPM unnecessarily during
idle/suspend to turn off these resources when we go idle/suspend because
it takes more time and thus more power.

So the CPU really wants to only be voting for the HFPLL regulator
supplies in the active set. This way, if we're not using the HFPLLs
(i.e. the CPUs are all running off the global PLL), then we can disable
the regulator in the active set (RPM code only caches sleep set so we
won't be doing unnecessary flushes in the idle/suspend path). The clock
driver will make sure to turn off the HFPLLs before we go into a sleep
state that would trigger a switch from active to sleep set (commonly
referred to as RPM notification). It very well could be that there are
other consumers of the same regulator, but that doesn't matter to the
CPU clock driver because it only cares about the active set. Now you may
ask why can't the CPU clock driver disable the regulator when the HFPLL
is disabled? We don't do that in this case because a) it causes more RPM
communication overhead and b) we will be in atomic context when the
HFPLL is disabled during idle/suspend and the regulator APIs are
sleeping calls. In the non-idle/suspend path we will disable the regulator.

Also the active/sleep sets are about more than just on/off state. We may
have a situation where the active set voltage (or some other attribute
like current, mode, etc.) is different than the sleep set voltage. For
example, the CPU is supplied by a digital logic regulator that is shared
with other digital logic in the SoC (GPU, wifi, etc.). The CPU may
require some high voltage, but the GPU only needs some lower voltage.
The suspend/idle code relies on the fact that the GPU is voting on the
active+sleep regulator while the CPU is voting on the active only
regulator so that the RPM can automatically switch between high voltage
and low voltage when the CPU notifies the RPM that it's gone idle (or
the CPU wakes up).

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3136 bytes --]

On Fri, Nov 21, 2014 at 03:10:44PM -0800, Stephen Boyd wrote:
> On 11/10/2014 02:52 PM, Bjorn Andersson wrote:

I don't seem to have been CCed on earlier messages here, or perhaps the
lack of obvious relevance (like mention of regulators) caused me not to
read the mails so I'm missing a *lot* of context.

> > Further more, these resources are shared
> > with peripherals in the system; e.g. LDO12 on PM8941 is used to clock the CPU
> > and WiFi/BT PLLs as well as providing power to the display in our devices. So
> > the suspend functionality in the regulator framework doesn't cut it.

> Furthermore, we can trigger a transition to the sleep set during idle
> paths so suspend definitely won't cut it.

Honestly I'm not really sure what support the above is talking about...
the regulator API has support for setting the configuration the
regulator has when we are in suspend but it doesn't have any real
support for entering or leaving such a mode which appears to be what is
being discussed.

> So the CPU really wants to only be voting for the HFPLL regulator
> supplies in the active set. This way, if we're not using the HFPLLs
> (i.e. the CPUs are all running off the global PLL), then we can disable
> the regulator in the active set (RPM code only caches sleep set so we
> won't be doing unnecessary flushes in the idle/suspend path). The clock
> driver will make sure to turn off the HFPLLs before we go into a sleep
> state that would trigger a switch from active to sleep set (commonly
> referred to as RPM notification). It very well could be that there are
> other consumers of the same regulator, but that doesn't matter to the
> CPU clock driver because it only cares about the active set. Now you may
> ask why can't the CPU clock driver disable the regulator when the HFPLL
> is disabled? We don't do that in this case because a) it causes more RPM
> communication overhead and b) we will be in atomic context when the
> HFPLL is disabled during idle/suspend and the regulator APIs are
> sleeping calls. In the non-idle/suspend path we will disable the regulator.

I'm afraid the above is making very little sense to me.  What is
"voting" and how is it different to "enabling", "notification" or
"flushing"?

> Also the active/sleep sets are about more than just on/off state. We may
> have a situation where the active set voltage (or some other attribute
> like current, mode, etc.) is different than the sleep set voltage. For

In a regulator API context if we are talking about suspend it's expected
that all properties may vary in suspend.

> example, the CPU is supplied by a digital logic regulator that is shared
> with other digital logic in the SoC (GPU, wifi, etc.). The CPU may
> require some high voltage, but the GPU only needs some lower voltage.
> The suspend/idle code relies on the fact that the GPU is voting on the
> active+sleep regulator while the CPU is voting on the active only
> regulator so that the RPM can automatically switch between high voltage
> and low voltage when the CPU notifies the RPM that it's gone idle (or
> the CPU wakes up).

Again I'm not following this at all, sorry.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Fri, Nov 21, 2014 at 03:10:44PM -0800, Stephen Boyd wrote:
> On 11/10/2014 02:52 PM, Bjorn Andersson wrote:

I don't seem to have been CCed on earlier messages here, or perhaps the
lack of obvious relevance (like mention of regulators) caused me not to
read the mails so I'm missing a *lot* of context.

> > Further more, these resources are shared
> > with peripherals in the system; e.g. LDO12 on PM8941 is used to clock the CPU
> > and WiFi/BT PLLs as well as providing power to the display in our devices. So
> > the suspend functionality in the regulator framework doesn't cut it.

> Furthermore, we can trigger a transition to the sleep set during idle
> paths so suspend definitely won't cut it.

Honestly I'm not really sure what support the above is talking about...
the regulator API has support for setting the configuration the
regulator has when we are in suspend but it doesn't have any real
support for entering or leaving such a mode which appears to be what is
being discussed.

> So the CPU really wants to only be voting for the HFPLL regulator
> supplies in the active set. This way, if we're not using the HFPLLs
> (i.e. the CPUs are all running off the global PLL), then we can disable
> the regulator in the active set (RPM code only caches sleep set so we
> won't be doing unnecessary flushes in the idle/suspend path). The clock
> driver will make sure to turn off the HFPLLs before we go into a sleep
> state that would trigger a switch from active to sleep set (commonly
> referred to as RPM notification). It very well could be that there are
> other consumers of the same regulator, but that doesn't matter to the
> CPU clock driver because it only cares about the active set. Now you may
> ask why can't the CPU clock driver disable the regulator when the HFPLL
> is disabled? We don't do that in this case because a) it causes more RPM
> communication overhead and b) we will be in atomic context when the
> HFPLL is disabled during idle/suspend and the regulator APIs are
> sleeping calls. In the non-idle/suspend path we will disable the regulator.

I'm afraid the above is making very little sense to me.  What is
"voting" and how is it different to "enabling", "notification" or
"flushing"?

> Also the active/sleep sets are about more than just on/off state. We may
> have a situation where the active set voltage (or some other attribute
> like current, mode, etc.) is different than the sleep set voltage. For

In a regulator API context if we are talking about suspend it's expected
that all properties may vary in suspend.

> example, the CPU is supplied by a digital logic regulator that is shared
> with other digital logic in the SoC (GPU, wifi, etc.). The CPU may
> require some high voltage, but the GPU only needs some lower voltage.
> The suspend/idle code relies on the fact that the GPU is voting on the
> active+sleep regulator while the CPU is voting on the active only
> regulator so that the RPM can automatically switch between high voltage
> and low voltage when the CPU notifies the RPM that it's gone idle (or
> the CPU wakes up).

Again I'm not following this at all, sorry.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141121/baed5af0/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/21/2014 03:27 PM, Mark Brown wrote:
> On Fri, Nov 21, 2014 at 03:10:44PM -0800, Stephen Boyd wrote:
>> On 11/10/2014 02:52 PM, Bjorn Andersson wrote:
> I don't seem to have been CCed on earlier messages here, or perhaps the
> lack of obvious relevance (like mention of regulators) caused me not to
> read the mails so I'm missing a *lot* of context.

This mail has been lingering in my inbox for weeks just haven't gotten
around to replying.

This sleep set/active set stuff has to do with more than just
regulators. It applies to any resource that the RPM provides, but
regulators are a primary use case so you're on Cc and it would be great
if you fully understood the regulator aspect here.

>
>
>> So the CPU really wants to only be voting for the HFPLL regulator
>> supplies in the active set. This way, if we're not using the HFPLLs
>> (i.e. the CPUs are all running off the global PLL), then we can disable
>> the regulator in the active set (RPM code only caches sleep set so we
>> won't be doing unnecessary flushes in the idle/suspend path). The clock
>> driver will make sure to turn off the HFPLLs before we go into a sleep
>> state that would trigger a switch from active to sleep set (commonly
>> referred to as RPM notification). It very well could be that there are
>> other consumers of the same regulator, but that doesn't matter to the
>> CPU clock driver because it only cares about the active set. Now you may
>> ask why can't the CPU clock driver disable the regulator when the HFPLL
>> is disabled? We don't do that in this case because a) it causes more RPM
>> communication overhead and b) we will be in atomic context when the
>> HFPLL is disabled during idle/suspend and the regulator APIs are
>> sleeping calls. In the non-idle/suspend path we will disable the regulator.
> I'm afraid the above is making very little sense to me.  What is
> "voting" and how is it different to "enabling", "notification" or
> "flushing"?

The regulators are shared between multiple "masters" in the SoC. So the
CPUs that are running linux only "vote" on attributes of regulators and
then another processor that isn't running linux (called RPM) aggregates
the request from Linux along with other "masters" like wifi, modem, etc.
and then changes something for that regulator like voltage,
enable/disable, etc. We also have some hardware next to our CPUs that
notifies the RPM when we transition into or out of idle/suspend (it's
called an SPM). This is the notification part. Flushing has to do with
batching up multiple RPM sleep set requests and sending them before we
enter idle/suspend.

>
>> Also the active/sleep sets are about more than just on/off state. We may
>> have a situation where the active set voltage (or some other attribute
>> like current, mode, etc.) is different than the sleep set voltage. For
> In a regulator API context if we are talking about suspend it's expected
> that all properties may vary in suspend.
>
>> example, the CPU is supplied by a digital logic regulator that is shared
>> with other digital logic in the SoC (GPU, wifi, etc.). The CPU may
>> require some high voltage, but the GPU only needs some lower voltage.
>> The suspend/idle code relies on the fact that the GPU is voting on the
>> active+sleep regulator while the CPU is voting on the active only
>> regulator so that the RPM can automatically switch between high voltage
>> and low voltage when the CPU notifies the RPM that it's gone idle (or
>> the CPU wakes up).
> Again I'm not following this at all, sorry.

Hopefully it's clear now. If not I can clarify further.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/21/2014 03:27 PM, Mark Brown wrote:
> On Fri, Nov 21, 2014 at 03:10:44PM -0800, Stephen Boyd wrote:
>> On 11/10/2014 02:52 PM, Bjorn Andersson wrote:
> I don't seem to have been CCed on earlier messages here, or perhaps the
> lack of obvious relevance (like mention of regulators) caused me not to
> read the mails so I'm missing a *lot* of context.

This mail has been lingering in my inbox for weeks just haven't gotten
around to replying.

This sleep set/active set stuff has to do with more than just
regulators. It applies to any resource that the RPM provides, but
regulators are a primary use case so you're on Cc and it would be great
if you fully understood the regulator aspect here.

>
>
>> So the CPU really wants to only be voting for the HFPLL regulator
>> supplies in the active set. This way, if we're not using the HFPLLs
>> (i.e. the CPUs are all running off the global PLL), then we can disable
>> the regulator in the active set (RPM code only caches sleep set so we
>> won't be doing unnecessary flushes in the idle/suspend path). The clock
>> driver will make sure to turn off the HFPLLs before we go into a sleep
>> state that would trigger a switch from active to sleep set (commonly
>> referred to as RPM notification). It very well could be that there are
>> other consumers of the same regulator, but that doesn't matter to the
>> CPU clock driver because it only cares about the active set. Now you may
>> ask why can't the CPU clock driver disable the regulator when the HFPLL
>> is disabled? We don't do that in this case because a) it causes more RPM
>> communication overhead and b) we will be in atomic context when the
>> HFPLL is disabled during idle/suspend and the regulator APIs are
>> sleeping calls. In the non-idle/suspend path we will disable the regulator.
> I'm afraid the above is making very little sense to me.  What is
> "voting" and how is it different to "enabling", "notification" or
> "flushing"?

The regulators are shared between multiple "masters" in the SoC. So the
CPUs that are running linux only "vote" on attributes of regulators and
then another processor that isn't running linux (called RPM) aggregates
the request from Linux along with other "masters" like wifi, modem, etc.
and then changes something for that regulator like voltage,
enable/disable, etc. We also have some hardware next to our CPUs that
notifies the RPM when we transition into or out of idle/suspend (it's
called an SPM). This is the notification part. Flushing has to do with
batching up multiple RPM sleep set requests and sending them before we
enter idle/suspend.

>
>> Also the active/sleep sets are about more than just on/off state. We may
>> have a situation where the active set voltage (or some other attribute
>> like current, mode, etc.) is different than the sleep set voltage. For
> In a regulator API context if we are talking about suspend it's expected
> that all properties may vary in suspend.
>
>> example, the CPU is supplied by a digital logic regulator that is shared
>> with other digital logic in the SoC (GPU, wifi, etc.). The CPU may
>> require some high voltage, but the GPU only needs some lower voltage.
>> The suspend/idle code relies on the fact that the GPU is voting on the
>> active+sleep regulator while the CPU is voting on the active only
>> regulator so that the RPM can automatically switch between high voltage
>> and low voltage when the CPU notifies the RPM that it's gone idle (or
>> the CPU wakes up).
> Again I'm not following this at all, sorry.

Hopefully it's clear now. If not I can clarify further.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]

On Fri, Nov 21, 2014 at 03:43:54PM -0800, Stephen Boyd wrote:

> This sleep set/active set stuff has to do with more than just
> regulators. It applies to any resource that the RPM provides, but
> regulators are a primary use case so you're on Cc and it would be great
> if you fully understood the regulator aspect here.

Step back: what is the RPM?  Some kind of power management coprocessor
from the sounds of it?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Fri, Nov 21, 2014 at 03:43:54PM -0800, Stephen Boyd wrote:

> This sleep set/active set stuff has to do with more than just
> regulators. It applies to any resource that the RPM provides, but
> regulators are a primary use case so you're on Cc and it would be great
> if you fully understood the regulator aspect here.

Step back: what is the RPM?  Some kind of power management coprocessor
from the sounds of it?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141121/52c6136a/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/21/2014 03:54 PM, Mark Brown wrote:
> On Fri, Nov 21, 2014 at 03:43:54PM -0800, Stephen Boyd wrote:
>
>> This sleep set/active set stuff has to do with more than just
>> regulators. It applies to any resource that the RPM provides, but
>> regulators are a primary use case so you're on Cc and it would be great
>> if you fully understood the regulator aspect here.
> Step back: what is the RPM?  Some kind of power management coprocessor
> from the sounds of it?

Yes.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/21/2014 03:54 PM, Mark Brown wrote:
> On Fri, Nov 21, 2014 at 03:43:54PM -0800, Stephen Boyd wrote:
>
>> This sleep set/active set stuff has to do with more than just
>> regulators. It applies to any resource that the RPM provides, but
>> regulators are a primary use case so you're on Cc and it would be great
>> if you fully understood the regulator aspect here.
> Step back: what is the RPM?  Some kind of power management coprocessor
> from the sounds of it?

Yes.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Fri 21 Nov 15:54 PST 2014, Mark Brown wrote:

> On Fri, Nov 21, 2014 at 03:43:54PM -0800, Stephen Boyd wrote:
> 
> > This sleep set/active set stuff has to do with more than just
> > regulators. It applies to any resource that the RPM provides, but
> > regulators are a primary use case so you're on Cc and it would be great
> > if you fully understood the regulator aspect here.
> 
> Step back: what is the RPM?  Some kind of power management coprocessor
> from the sounds of it?

Correct, it's a coprocessor that does the actual poking of the PMIC.

To each processor in the SoC it exposes a set of resources, e.g. regulators.
Each resource is represented by two set of properties, the active and the sleep
state sets. The selection between the two state sets are basically controlled
by the CPU being in WFI (wait-for-interrupt) or not.

The qcom_rpm-regulator.c (and rfc for qcom_smd-regulator.c) that we have in the
tree only pokes the active state and we end up leaving certain regulators as
always-on; due to the fact that we're clocked off PLLs powered by them.

To further complicate matters these regulators are also used to power
peripherals, so enable is not 1:1 with WFI, but rather depends on other
consumers.

My proposal was to flag these regulators so that we write the active/sleep
states in a way that they will be turned off on WFI, iff the regulator isn't
enabled. But as Stephen pointed out, this might not be enough.

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Fri 21 Nov 15:54 PST 2014, Mark Brown wrote:

> On Fri, Nov 21, 2014 at 03:43:54PM -0800, Stephen Boyd wrote:
> 
> > This sleep set/active set stuff has to do with more than just
> > regulators. It applies to any resource that the RPM provides, but
> > regulators are a primary use case so you're on Cc and it would be great
> > if you fully understood the regulator aspect here.
> 
> Step back: what is the RPM?  Some kind of power management coprocessor
> from the sounds of it?

Correct, it's a coprocessor that does the actual poking of the PMIC.

To each processor in the SoC it exposes a set of resources, e.g. regulators.
Each resource is represented by two set of properties, the active and the sleep
state sets. The selection between the two state sets are basically controlled
by the CPU being in WFI (wait-for-interrupt) or not.

The qcom_rpm-regulator.c (and rfc for qcom_smd-regulator.c) that we have in the
tree only pokes the active state and we end up leaving certain regulators as
always-on; due to the fact that we're clocked off PLLs powered by them.

To further complicate matters these regulators are also used to power
peripherals, so enable is not 1:1 with WFI, but rather depends on other
consumers.

My proposal was to flag these regulators so that we write the active/sleep
states in a way that they will be turned off on WFI, iff the regulator isn't
enabled. But as Stephen pointed out, this might not be enough.

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Fri, Nov 21, 2014 at 3:10 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> Hi Bjorn,
>
Hi Stephen,

Thanks for taking the time to look at my proposal.

[..]

>
> So the CPU really wants to only be voting for the HFPLL regulator
> supplies in the active set. This way, if we're not using the HFPLLs
> (i.e. the CPUs are all running off the global PLL), then we can disable
> the regulator in the active set (RPM code only caches sleep set so we
> won't be doing unnecessary flushes in the idle/suspend path). The clock

In the codeaurora tree the flushing is handled by a call from the Qcom PM
driver straight into the regulator/resource driver that have a private list of
all regulators that it goes through and pushes out requests (while in atomic
context).

I'm not sure how to model this in a sane way...

> driver will make sure to turn off the HFPLLs before we go into a sleep
> state that would trigger a switch from active to sleep set (commonly
> referred to as RPM notification). It very well could be that there are
> other consumers of the same regulator, but that doesn't matter to the
> CPU clock driver because it only cares about the active set. Now you may

Okay, so CPUFREQ or a system suspend can explicitly call regulator_disable() on
our shared regulator and in CPUIDLE we will just depend on the sleep state?

> ask why can't the CPU clock driver disable the regulator when the HFPLL
> is disabled? We don't do that in this case because a) it causes more RPM
> communication overhead and b) we will be in atomic context when the
> HFPLL is disabled during idle/suspend and the regulator APIs are
> sleeping calls. In the non-idle/suspend path we will disable the regulator.
>
> Also the active/sleep sets are about more than just on/off state. We may
> have a situation where the active set voltage (or some other attribute
> like current, mode, etc.) is different than the sleep set voltage. For

You're using the word "may" here and the code clearly would let you do this,
the part I've been struggeling with is to backtrack the consumers to figure out
if this is actually done.

> example, the CPU is supplied by a digital logic regulator that is shared
> with other digital logic in the SoC (GPU, wifi, etc.). The CPU may
> require some high voltage, but the GPU only needs some lower voltage.
> The suspend/idle code relies on the fact that the GPU is voting on the
> active+sleep regulator while the CPU is voting on the active only
> regulator so that the RPM can automatically switch between high voltage
> and low voltage when the CPU notifies the RPM that it's gone idle (or
> the CPU wakes up).

Okay, so when we're hitting CPUIDLE we need to lower the voltage on the
regulator, should I assume that this then must be raised before the CPU starts
executing again?


So what it boils down to is, like the codeaurora code shows, our regulator
driver must be able to set properties on active only or both states depending
on which regulator consumer makes the request.

For the reference; the codeaurora code models this as "resources" with 2 set of
properties, multiple regulators are then tied to the same resource, poking the
individual sets based on properties of the regulator (active or both). So in
essense we get multiple regulator instances per regulator hardware.


The problem with this model is that it's circumventing the fact that the
regulator framework doesn't support our use cases - and I don't like this.
I think we need to sit down and discuss this more in detail, most likely with
Mark.


This concludes that we cannot merge my currently proposed solution, because we
will have to redesign the DT bindings.

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Fri, Nov 21, 2014 at 3:10 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> Hi Bjorn,
>
Hi Stephen,

Thanks for taking the time to look at my proposal.

[..]

>
> So the CPU really wants to only be voting for the HFPLL regulator
> supplies in the active set. This way, if we're not using the HFPLLs
> (i.e. the CPUs are all running off the global PLL), then we can disable
> the regulator in the active set (RPM code only caches sleep set so we
> won't be doing unnecessary flushes in the idle/suspend path). The clock

In the codeaurora tree the flushing is handled by a call from the Qcom PM
driver straight into the regulator/resource driver that have a private list of
all regulators that it goes through and pushes out requests (while in atomic
context).

I'm not sure how to model this in a sane way...

> driver will make sure to turn off the HFPLLs before we go into a sleep
> state that would trigger a switch from active to sleep set (commonly
> referred to as RPM notification). It very well could be that there are
> other consumers of the same regulator, but that doesn't matter to the
> CPU clock driver because it only cares about the active set. Now you may

Okay, so CPUFREQ or a system suspend can explicitly call regulator_disable() on
our shared regulator and in CPUIDLE we will just depend on the sleep state?

> ask why can't the CPU clock driver disable the regulator when the HFPLL
> is disabled? We don't do that in this case because a) it causes more RPM
> communication overhead and b) we will be in atomic context when the
> HFPLL is disabled during idle/suspend and the regulator APIs are
> sleeping calls. In the non-idle/suspend path we will disable the regulator.
>
> Also the active/sleep sets are about more than just on/off state. We may
> have a situation where the active set voltage (or some other attribute
> like current, mode, etc.) is different than the sleep set voltage. For

You're using the word "may" here and the code clearly would let you do this,
the part I've been struggeling with is to backtrack the consumers to figure out
if this is actually done.

> example, the CPU is supplied by a digital logic regulator that is shared
> with other digital logic in the SoC (GPU, wifi, etc.). The CPU may
> require some high voltage, but the GPU only needs some lower voltage.
> The suspend/idle code relies on the fact that the GPU is voting on the
> active+sleep regulator while the CPU is voting on the active only
> regulator so that the RPM can automatically switch between high voltage
> and low voltage when the CPU notifies the RPM that it's gone idle (or
> the CPU wakes up).

Okay, so when we're hitting CPUIDLE we need to lower the voltage on the
regulator, should I assume that this then must be raised before the CPU starts
executing again?


So what it boils down to is, like the codeaurora code shows, our regulator
driver must be able to set properties on active only or both states depending
on which regulator consumer makes the request.

For the reference; the codeaurora code models this as "resources" with 2 set of
properties, multiple regulators are then tied to the same resource, poking the
individual sets based on properties of the regulator (active or both). So in
essense we get multiple regulator instances per regulator hardware.


The problem with this model is that it's circumventing the fact that the
regulator framework doesn't support our use cases - and I don't like this.
I think we need to sit down and discuss this more in detail, most likely with
Mark.


This concludes that we cannot merge my currently proposed solution, because we
will have to redesign the DT bindings.

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

On Fri, Nov 21, 2014 at 03:43:54PM -0800, Stephen Boyd wrote:
> On 11/21/2014 03:27 PM, Mark Brown wrote:

> > I'm afraid the above is making very little sense to me.  What is
> > "voting" and how is it different to "enabling", "notification" or
> > "flushing"?

> The regulators are shared between multiple "masters" in the SoC. So the
> CPUs that are running linux only "vote" on attributes of regulators and
> then another processor that isn't running linux (called RPM) aggregates
> the request from Linux along with other "masters" like wifi, modem, etc.
> and then changes something for that regulator like voltage,
> enable/disable, etc. We also have some hardware next to our CPUs that
> notifies the RPM when we transition into or out of idle/suspend (it's
> called an SPM). This is the notification part. Flushing has to do with
> batching up multiple RPM sleep set requests and sending them before we
> enter idle/suspend.

...

> Hopefully it's clear now. If not I can clarify further.

A bit, but I'm afraid I'm still at a loss as to what the problems and
debate are here.  Why is this more complex than the Linux part of the
system just saying what it wants at any given time?  It sounds like that
has some sort of performance issue?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Fri, Nov 21, 2014 at 03:43:54PM -0800, Stephen Boyd wrote:
> On 11/21/2014 03:27 PM, Mark Brown wrote:

> > I'm afraid the above is making very little sense to me.  What is
> > "voting" and how is it different to "enabling", "notification" or
> > "flushing"?

> The regulators are shared between multiple "masters" in the SoC. So the
> CPUs that are running linux only "vote" on attributes of regulators and
> then another processor that isn't running linux (called RPM) aggregates
> the request from Linux along with other "masters" like wifi, modem, etc.
> and then changes something for that regulator like voltage,
> enable/disable, etc. We also have some hardware next to our CPUs that
> notifies the RPM when we transition into or out of idle/suspend (it's
> called an SPM). This is the notification part. Flushing has to do with
> batching up multiple RPM sleep set requests and sending them before we
> enter idle/suspend.

...

> Hopefully it's clear now. If not I can clarify further.

A bit, but I'm afraid I'm still at a loss as to what the problems and
debate are here.  Why is this more complex than the Linux part of the
system just saying what it wants at any given time?  It sounds like that
has some sort of performance issue?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141124/0f9234ad/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/24/2014 10:16 AM, Mark Brown wrote:
> On Fri, Nov 21, 2014 at 03:43:54PM -0800, Stephen Boyd wrote:
>> On 11/21/2014 03:27 PM, Mark Brown wrote:
>>> I'm afraid the above is making very little sense to me.  What is
>>> "voting" and how is it different to "enabling", "notification" or
>>> "flushing"?
>> The regulators are shared between multiple "masters" in the SoC. So the
>> CPUs that are running linux only "vote" on attributes of regulators and
>> then another processor that isn't running linux (called RPM) aggregates
>> the request from Linux along with other "masters" like wifi, modem, etc.
>> and then changes something for that regulator like voltage,
>> enable/disable, etc. We also have some hardware next to our CPUs that
>> notifies the RPM when we transition into or out of idle/suspend (it's
>> called an SPM). This is the notification part. Flushing has to do with
>> batching up multiple RPM sleep set requests and sending them before we
>> enter idle/suspend.
> ...
>
>> Hopefully it's clear now. If not I can clarify further.
> A bit, but I'm afraid I'm still at a loss as to what the problems and
> debate are here.  Why is this more complex than the Linux part of the
> system just saying what it wants at any given time?  It sounds like that
> has some sort of performance issue?

Yes, communicating with the RPM is not that fast. The sleep set could be
updated hundreds of times before we go idle and actually switch from the
active set to sleep set. One optimization is to buffer the sleep set
requests and send them right before we switch to the sleep set. This
saves on the messaging overhead for sleep sets that don't matter when
we're active by moving the overhead to the idle/suspend path. The hope
is to consolidate multiple sleep set requests into one request.

I think the problem that Bjorn is trying to come up with a solution for
is how to represent the sleep set and active set in the kernel clock and
regulator frameworks. You can think of each RPM resource as a regulator
supply. Each one of those resources has an active and sleep set. In the
downstream vendor kernels we make two regulators for an RPM resource.
One regulator for the active set and one regulator for the active +
sleep set. We call these 'active only' and 'active + sleep' regulators
respectively. The RPM regulator driver aggregates the active set for
both the regulators via a max() operation and sends that as a request to
the RPM. The sleep set is the same as the active set for the 'active +
sleep' regulator, so we just send whatever the value is that was sent
down via the regulator APIs on the 'active + sleep' regulator. The only
driver that really cares about the active only regulators is the CPU
clock driver. Otherwise drivers are using the active + sleep regulators
because their devices don't stop running when the CPU goes to idle/suspend.

Maybe another solution would be to push the problem into the regulator
core and educate it about the two different sets. RPM resources would
map one-to-one with a regulator and the sleep set and active sets would
be selectable via the regulator_get() API or some other consumer mapping
method. This would allow consumers to request whatever set they care
about and consolidate the aggregation logic that's duplicated at the
consumer level and the driver level into the core.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/24/2014 10:16 AM, Mark Brown wrote:
> On Fri, Nov 21, 2014 at 03:43:54PM -0800, Stephen Boyd wrote:
>> On 11/21/2014 03:27 PM, Mark Brown wrote:
>>> I'm afraid the above is making very little sense to me.  What is
>>> "voting" and how is it different to "enabling", "notification" or
>>> "flushing"?
>> The regulators are shared between multiple "masters" in the SoC. So the
>> CPUs that are running linux only "vote" on attributes of regulators and
>> then another processor that isn't running linux (called RPM) aggregates
>> the request from Linux along with other "masters" like wifi, modem, etc.
>> and then changes something for that regulator like voltage,
>> enable/disable, etc. We also have some hardware next to our CPUs that
>> notifies the RPM when we transition into or out of idle/suspend (it's
>> called an SPM). This is the notification part. Flushing has to do with
>> batching up multiple RPM sleep set requests and sending them before we
>> enter idle/suspend.
> ...
>
>> Hopefully it's clear now. If not I can clarify further.
> A bit, but I'm afraid I'm still at a loss as to what the problems and
> debate are here.  Why is this more complex than the Linux part of the
> system just saying what it wants at any given time?  It sounds like that
> has some sort of performance issue?

Yes, communicating with the RPM is not that fast. The sleep set could be
updated hundreds of times before we go idle and actually switch from the
active set to sleep set. One optimization is to buffer the sleep set
requests and send them right before we switch to the sleep set. This
saves on the messaging overhead for sleep sets that don't matter when
we're active by moving the overhead to the idle/suspend path. The hope
is to consolidate multiple sleep set requests into one request.

I think the problem that Bjorn is trying to come up with a solution for
is how to represent the sleep set and active set in the kernel clock and
regulator frameworks. You can think of each RPM resource as a regulator
supply. Each one of those resources has an active and sleep set. In the
downstream vendor kernels we make two regulators for an RPM resource.
One regulator for the active set and one regulator for the active +
sleep set. We call these 'active only' and 'active + sleep' regulators
respectively. The RPM regulator driver aggregates the active set for
both the regulators via a max() operation and sends that as a request to
the RPM. The sleep set is the same as the active set for the 'active +
sleep' regulator, so we just send whatever the value is that was sent
down via the regulator APIs on the 'active + sleep' regulator. The only
driver that really cares about the active only regulators is the CPU
clock driver. Otherwise drivers are using the active + sleep regulators
because their devices don't stop running when the CPU goes to idle/suspend.

Maybe another solution would be to push the problem into the regulator
core and educate it about the two different sets. RPM resources would
map one-to-one with a regulator and the sleep set and active sets would
be selectable via the regulator_get() API or some other consumer mapping
method. This would allow consumers to request whatever set they care
about and consolidate the aggregation logic that's duplicated at the
consumer level and the driver level into the core.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/24/2014 09:02 AM, Bjorn Andersson wrote:
> On Fri, Nov 21, 2014 at 3:10 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> Hi Bjorn,
>>
> Hi Stephen,
>
> Thanks for taking the time to look at my proposal.
>
> [..]
>
>> So the CPU really wants to only be voting for the HFPLL regulator
>> supplies in the active set. This way, if we're not using the HFPLLs
>> (i.e. the CPUs are all running off the global PLL), then we can disable
>> the regulator in the active set (RPM code only caches sleep set so we
>> won't be doing unnecessary flushes in the idle/suspend path). The clock
> In the codeaurora tree the flushing is handled by a call from the Qcom PM
> driver straight into the regulator/resource driver that have a private list of
> all regulators that it goes through and pushes out requests (while in atomic
> context).
>
> I'm not sure how to model this in a sane way...
>
>> driver will make sure to turn off the HFPLLs before we go into a sleep
>> state that would trigger a switch from active to sleep set (commonly
>> referred to as RPM notification). It very well could be that there are
>> other consumers of the same regulator, but that doesn't matter to the
>> CPU clock driver because it only cares about the active set. Now you may
> Okay, so CPUFREQ or a system suspend can explicitly call regulator_disable() on
> our shared regulator and in CPUIDLE we will just depend on the sleep state?

Yes.

>> ask why can't the CPU clock driver disable the regulator when the HFPLL
>> is disabled? We don't do that in this case because a) it causes more RPM
>> communication overhead and b) we will be in atomic context when the
>> HFPLL is disabled during idle/suspend and the regulator APIs are
>> sleeping calls. In the non-idle/suspend path we will disable the regulator.
>>
>> Also the active/sleep sets are about more than just on/off state. We may
>> have a situation where the active set voltage (or some other attribute
>> like current, mode, etc.) is different than the sleep set voltage. For
> You're using the word "may" here and the code clearly would let you do this,
> the part I've been struggeling with is to backtrack the consumers to figure out
> if this is actually done.

s/may//

We have this case for the example I gave, digital logic. On msm8960 it's
S3, where the CPU clock driver requests some active only voltage to
power the CPU itself whereas the peripheral clock driver requests an
active and sleep set voltage for some peripheral like uart or i2c that
will be active even when the CPU is idle or suspended.

>
>> example, the CPU is supplied by a digital logic regulator that is shared
>> with other digital logic in the SoC (GPU, wifi, etc.). The CPU may
>> require some high voltage, but the GPU only needs some lower voltage.
>> The suspend/idle code relies on the fact that the GPU is voting on the
>> active+sleep regulator while the CPU is voting on the active only
>> regulator so that the RPM can automatically switch between high voltage
>> and low voltage when the CPU notifies the RPM that it's gone idle (or
>> the CPU wakes up).
> Okay, so when we're hitting CPUIDLE we need to lower the voltage on the
> regulator, should I assume that this then must be raised before the CPU starts
> executing again?

Yes the RPM will raise the voltage when it transitions us from sleep set
to active set.

>
>
> So what it boils down to is, like the codeaurora code shows, our regulator
> driver must be able to set properties on active only or both states depending
> on which regulator consumer makes the request.
>
> For the reference; the codeaurora code models this as "resources" with 2 set of
> properties, multiple regulators are then tied to the same resource, poking the
> individual sets based on properties of the regulator (active or both). So in
> essense we get multiple regulator instances per regulator hardware.
>
>
> The problem with this model is that it's circumventing the fact that the
> regulator framework doesn't support our use cases - and I don't like this.
> I think we need to sit down and discuss this more in detail, most likely with
> Mark.

What exactly are we circumventing? I can only guess that we're talking
about the aggregation logic?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/24/2014 09:02 AM, Bjorn Andersson wrote:
> On Fri, Nov 21, 2014 at 3:10 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> Hi Bjorn,
>>
> Hi Stephen,
>
> Thanks for taking the time to look at my proposal.
>
> [..]
>
>> So the CPU really wants to only be voting for the HFPLL regulator
>> supplies in the active set. This way, if we're not using the HFPLLs
>> (i.e. the CPUs are all running off the global PLL), then we can disable
>> the regulator in the active set (RPM code only caches sleep set so we
>> won't be doing unnecessary flushes in the idle/suspend path). The clock
> In the codeaurora tree the flushing is handled by a call from the Qcom PM
> driver straight into the regulator/resource driver that have a private list of
> all regulators that it goes through and pushes out requests (while in atomic
> context).
>
> I'm not sure how to model this in a sane way...
>
>> driver will make sure to turn off the HFPLLs before we go into a sleep
>> state that would trigger a switch from active to sleep set (commonly
>> referred to as RPM notification). It very well could be that there are
>> other consumers of the same regulator, but that doesn't matter to the
>> CPU clock driver because it only cares about the active set. Now you may
> Okay, so CPUFREQ or a system suspend can explicitly call regulator_disable() on
> our shared regulator and in CPUIDLE we will just depend on the sleep state?

Yes.

>> ask why can't the CPU clock driver disable the regulator when the HFPLL
>> is disabled? We don't do that in this case because a) it causes more RPM
>> communication overhead and b) we will be in atomic context when the
>> HFPLL is disabled during idle/suspend and the regulator APIs are
>> sleeping calls. In the non-idle/suspend path we will disable the regulator.
>>
>> Also the active/sleep sets are about more than just on/off state. We may
>> have a situation where the active set voltage (or some other attribute
>> like current, mode, etc.) is different than the sleep set voltage. For
> You're using the word "may" here and the code clearly would let you do this,
> the part I've been struggeling with is to backtrack the consumers to figure out
> if this is actually done.

s/may//

We have this case for the example I gave, digital logic. On msm8960 it's
S3, where the CPU clock driver requests some active only voltage to
power the CPU itself whereas the peripheral clock driver requests an
active and sleep set voltage for some peripheral like uart or i2c that
will be active even when the CPU is idle or suspended.

>
>> example, the CPU is supplied by a digital logic regulator that is shared
>> with other digital logic in the SoC (GPU, wifi, etc.). The CPU may
>> require some high voltage, but the GPU only needs some lower voltage.
>> The suspend/idle code relies on the fact that the GPU is voting on the
>> active+sleep regulator while the CPU is voting on the active only
>> regulator so that the RPM can automatically switch between high voltage
>> and low voltage when the CPU notifies the RPM that it's gone idle (or
>> the CPU wakes up).
> Okay, so when we're hitting CPUIDLE we need to lower the voltage on the
> regulator, should I assume that this then must be raised before the CPU starts
> executing again?

Yes the RPM will raise the voltage when it transitions us from sleep set
to active set.

>
>
> So what it boils down to is, like the codeaurora code shows, our regulator
> driver must be able to set properties on active only or both states depending
> on which regulator consumer makes the request.
>
> For the reference; the codeaurora code models this as "resources" with 2 set of
> properties, multiple regulators are then tied to the same resource, poking the
> individual sets based on properties of the regulator (active or both). So in
> essense we get multiple regulator instances per regulator hardware.
>
>
> The problem with this model is that it's circumventing the fact that the
> regulator framework doesn't support our use cases - and I don't like this.
> I think we need to sit down and discuss this more in detail, most likely with
> Mark.

What exactly are we circumventing? I can only guess that we're talking
about the aggregation logic?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Mon 24 Nov 13:19 PST 2014, Stephen Boyd wrote:

[..]
> 
> What exactly are we circumventing? I can only guess that we're talking
> about the aggregation logic?
> 

We're circumventing the fact that the regulator core doesn't have knowledge
about our multiple presented views of the same resource.

As the downstream driver shows, if we just implement the right pieces it should
work, i.e. give us the correct end result, but it will not be future proof nor
pretty. That's why I think we need to discuss how to solve it either in the
regulator driver or in the framework.

And based on your feedback, it looks like we would have to do something about
this in the framework.

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Mon 24 Nov 13:19 PST 2014, Stephen Boyd wrote:

[..]
> 
> What exactly are we circumventing? I can only guess that we're talking
> about the aggregation logic?
> 

We're circumventing the fact that the regulator core doesn't have knowledge
about our multiple presented views of the same resource.

As the downstream driver shows, if we just implement the right pieces it should
work, i.e. give us the correct end result, but it will not be future proof nor
pretty. That's why I think we need to discuss how to solve it either in the
regulator driver or in the framework.

And based on your feedback, it looks like we would have to do something about
this in the framework.

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/24/2014 01:59 PM, Bjorn Andersson wrote:
> On Mon 24 Nov 13:19 PST 2014, Stephen Boyd wrote:
>
> [..]
>> What exactly are we circumventing? I can only guess that we're talking
>> about the aggregation logic?
>>
> We're circumventing the fact that the regulator core doesn't have knowledge
> about our multiple presented views of the same resource.

Sorry I don't follow here. Why would the regulator framework care about
the different views of a resource? Each regulator we make for the
different views will reflect the request made by Linux for a particular
set of that RPM resource. So all consumers who are using the S3 active +
sleep set regulator will aggregate into one request. Likewise, all the
consumers for the S3 active set regulator will aggregate into another
request. The only thing that isn't visible is the aggregation between
the active only and active + sleep regulators, but that can be
determined by doing a max() of the regulators. Even then, if we consider
that there are other masters also requesting voltages or enable/disable
for these resources we quickly discover there are other things the
regulator core doesn't have knowledge about, like what the actual
voltage is or if the regulator is really off vs. Linux requesting for it
to be off.

>
> As the downstream driver shows, if we just implement the right pieces it should
> work, i.e. give us the correct end result, but it will not be future proof nor
> pretty. That's why I think we need to discuss how to solve it either in the
> regulator driver or in the framework.
>
> And based on your feedback, it looks like we would have to do something about
> this in the framework.

I don't see any problems with making multiple regulators for one RPM
resource that represent the active set or active + sleep set. Everything
could be handled in the RPM regulator driver by looking for the other
regulators that are acting on the same RPM resource and aggregating.
Maybe you can elaborate on what you think isn't future proof nor pretty
about this design?

As a thought experiment, what if there really was two physical
independent controllable regulators and a pin from the CPU to the PMIC
toggled a mux to select between the two. Such a pin would only be
asserted when the CPU turned off. Would you still want to expose this as
one regulator to the kernel given that only one supply goes to the CPU
at any given time?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/24/2014 01:59 PM, Bjorn Andersson wrote:
> On Mon 24 Nov 13:19 PST 2014, Stephen Boyd wrote:
>
> [..]
>> What exactly are we circumventing? I can only guess that we're talking
>> about the aggregation logic?
>>
> We're circumventing the fact that the regulator core doesn't have knowledge
> about our multiple presented views of the same resource.

Sorry I don't follow here. Why would the regulator framework care about
the different views of a resource? Each regulator we make for the
different views will reflect the request made by Linux for a particular
set of that RPM resource. So all consumers who are using the S3 active +
sleep set regulator will aggregate into one request. Likewise, all the
consumers for the S3 active set regulator will aggregate into another
request. The only thing that isn't visible is the aggregation between
the active only and active + sleep regulators, but that can be
determined by doing a max() of the regulators. Even then, if we consider
that there are other masters also requesting voltages or enable/disable
for these resources we quickly discover there are other things the
regulator core doesn't have knowledge about, like what the actual
voltage is or if the regulator is really off vs. Linux requesting for it
to be off.

>
> As the downstream driver shows, if we just implement the right pieces it should
> work, i.e. give us the correct end result, but it will not be future proof nor
> pretty. That's why I think we need to discuss how to solve it either in the
> regulator driver or in the framework.
>
> And based on your feedback, it looks like we would have to do something about
> this in the framework.

I don't see any problems with making multiple regulators for one RPM
resource that represent the active set or active + sleep set. Everything
could be handled in the RPM regulator driver by looking for the other
regulators that are acting on the same RPM resource and aggregating.
Maybe you can elaborate on what you think isn't future proof nor pretty
about this design?

As a thought experiment, what if there really was two physical
independent controllable regulators and a pin from the CPU to the PMIC
toggled a mux to select between the two. Such a pin would only be
asserted when the CPU turned off. Would you still want to expose this as
one regulator to the kernel given that only one supply goes to the CPU
at any given time?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4159 bytes --]

On Mon, Nov 24, 2014 at 01:19:47PM -0800, Stephen Boyd wrote:
> On 11/24/2014 10:16 AM, Mark Brown wrote:

> > A bit, but I'm afraid I'm still at a loss as to what the problems and
> > debate are here.  Why is this more complex than the Linux part of the
> > system just saying what it wants at any given time?  It sounds like that
> > has some sort of performance issue?

I'm still quite confused here...

> Yes, communicating with the RPM is not that fast. The sleep set could be
> updated hundreds of times before we go idle and actually switch from the
> active set to sleep set. One optimization is to buffer the sleep set
> requests and send them right before we switch to the sleep set. This
> saves on the messaging overhead for sleep sets that don't matter when
> we're active by moving the overhead to the idle/suspend path. The hope
> is to consolidate multiple sleep set requests into one request.

Right, that's one of the obvious ones.  Though I am a bit mystified as
to why you're needing to change these settings so often that it's a
problem - most hardware can do this but generally the configuration when
idle is fairly constant so it's irrelevant for performance how often we
set these things, a lot of the time it's going to boil down to I2C
writes so we're talking really infrequent here.  What's different about
these systems that means that this stuff needs constant tweaking?

> I think the problem that Bjorn is trying to come up with a solution for
> is how to represent the sleep set and active set in the kernel clock and
> regulator frameworks. You can think of each RPM resource as a regulator
> supply. Each one of those resources has an active and sleep set. In the
> downstream vendor kernels we make two regulators for an RPM resource.
> One regulator for the active set and one regulator for the active +
> sleep set. We call these 'active only' and 'active + sleep' regulators

That seems like it's a big abstraction problem.

> respectively. The RPM regulator driver aggregates the active set for
> both the regulators via a max() operation and sends that as a request to
> the RPM. The sleep set is the same as the active set for the 'active +
> sleep' regulator, so we just send whatever the value is that was sent
> down via the regulator APIs on the 'active + sleep' regulator. The only
> driver that really cares about the active only regulators is the CPU
> clock driver. Otherwise drivers are using the active + sleep regulators
> because their devices don't stop running when the CPU goes to idle/suspend.

Hang on a minute.  What you're saying seems to be that this isn't really
about suspend but actually about the normal operating configuration?
That makes it a bit more understandable why one would change the
settings a lot, I think what I'm hearing here is that the runtime state
changes a lot for some reason and the system needs the suspend state to
track this?

I can't help but think that this all sounds like the RPM isn't mapping
very well onto practical systems and needs revisiting in future
versions...  for example with what I'm parsing out of the above an
active+sleep set command or otherwise having the two modes tied together
for some regulators would make the whole problem go away.

> Maybe another solution would be to push the problem into the regulator
> core and educate it about the two different sets. RPM resources would
> map one-to-one with a regulator and the sleep set and active sets would
> be selectable via the regulator_get() API or some other consumer mapping
> method. This would allow consumers to request whatever set they care
> about and consolidate the aggregation logic that's duplicated at the
> consumer level and the driver level into the core.

I think any duplication that's going on sounds like a consequence of
the way this is currently implemented.  I think based on what I *think*
you're saying the RPM driver probably ought to be hiding this and adding
a property which makes the active and sleep sets track each other with
normal suspend mode control otherwise.  That could potentially be done
in the core, though the tracking would be substantial surgery.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Mon, Nov 24, 2014 at 01:19:47PM -0800, Stephen Boyd wrote:
> On 11/24/2014 10:16 AM, Mark Brown wrote:

> > A bit, but I'm afraid I'm still at a loss as to what the problems and
> > debate are here.  Why is this more complex than the Linux part of the
> > system just saying what it wants at any given time?  It sounds like that
> > has some sort of performance issue?

I'm still quite confused here...

> Yes, communicating with the RPM is not that fast. The sleep set could be
> updated hundreds of times before we go idle and actually switch from the
> active set to sleep set. One optimization is to buffer the sleep set
> requests and send them right before we switch to the sleep set. This
> saves on the messaging overhead for sleep sets that don't matter when
> we're active by moving the overhead to the idle/suspend path. The hope
> is to consolidate multiple sleep set requests into one request.

Right, that's one of the obvious ones.  Though I am a bit mystified as
to why you're needing to change these settings so often that it's a
problem - most hardware can do this but generally the configuration when
idle is fairly constant so it's irrelevant for performance how often we
set these things, a lot of the time it's going to boil down to I2C
writes so we're talking really infrequent here.  What's different about
these systems that means that this stuff needs constant tweaking?

> I think the problem that Bjorn is trying to come up with a solution for
> is how to represent the sleep set and active set in the kernel clock and
> regulator frameworks. You can think of each RPM resource as a regulator
> supply. Each one of those resources has an active and sleep set. In the
> downstream vendor kernels we make two regulators for an RPM resource.
> One regulator for the active set and one regulator for the active +
> sleep set. We call these 'active only' and 'active + sleep' regulators

That seems like it's a big abstraction problem.

> respectively. The RPM regulator driver aggregates the active set for
> both the regulators via a max() operation and sends that as a request to
> the RPM. The sleep set is the same as the active set for the 'active +
> sleep' regulator, so we just send whatever the value is that was sent
> down via the regulator APIs on the 'active + sleep' regulator. The only
> driver that really cares about the active only regulators is the CPU
> clock driver. Otherwise drivers are using the active + sleep regulators
> because their devices don't stop running when the CPU goes to idle/suspend.

Hang on a minute.  What you're saying seems to be that this isn't really
about suspend but actually about the normal operating configuration?
That makes it a bit more understandable why one would change the
settings a lot, I think what I'm hearing here is that the runtime state
changes a lot for some reason and the system needs the suspend state to
track this?

I can't help but think that this all sounds like the RPM isn't mapping
very well onto practical systems and needs revisiting in future
versions...  for example with what I'm parsing out of the above an
active+sleep set command or otherwise having the two modes tied together
for some regulators would make the whole problem go away.

> Maybe another solution would be to push the problem into the regulator
> core and educate it about the two different sets. RPM resources would
> map one-to-one with a regulator and the sleep set and active sets would
> be selectable via the regulator_get() API or some other consumer mapping
> method. This would allow consumers to request whatever set they care
> about and consolidate the aggregation logic that's duplicated at the
> consumer level and the driver level into the core.

I think any duplication that's going on sounds like a consequence of
the way this is currently implemented.  I think based on what I *think*
you're saying the RPM driver probably ought to be hiding this and adding
a property which makes the active and sleep sets track each other with
normal suspend mode control otherwise.  That could potentially be done
in the core, though the tracking would be substantial surgery.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141125/1821dd83/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/25/2014 12:44 PM, Mark Brown wrote:
> On Mon, Nov 24, 2014 at 01:19:47PM -0800, Stephen Boyd wrote:
>> On 11/24/2014 10:16 AM, Mark Brown wrote:
>>> A bit, but I'm afraid I'm still at a loss as to what the problems and
>>> debate are here.  Why is this more complex than the Linux part of the
>>> system just saying what it wants at any given time?  It sounds like that
>>> has some sort of performance issue?
> I'm still quite confused here...

Hm... hopefully it's not getting worse.

>
>> Yes, communicating with the RPM is not that fast. The sleep set could be
>> updated hundreds of times before we go idle and actually switch from the
>> active set to sleep set. One optimization is to buffer the sleep set
>> requests and send them right before we switch to the sleep set. This
>> saves on the messaging overhead for sleep sets that don't matter when
>> we're active by moving the overhead to the idle/suspend path. The hope
>> is to consolidate multiple sleep set requests into one request.
> Right, that's one of the obvious ones.  Though I am a bit mystified as
> to why you're needing to change these settings so often that it's a
> problem - most hardware can do this but generally the configuration when
> idle is fairly constant so it's irrelevant for performance how often we
> set these things, a lot of the time it's going to boil down to I2C
> writes so we're talking really infrequent here.  What's different about
> these systems that means that this stuff needs constant tweaking?

Yes I don't think we're constantly changing the regulators because it 
does boil down to I2C writes or something similar, but we could be 
modifying other resources that the RPM controls more often. For example, 
the RPM controls some clock frequencies that eventually end up being 
some mmio writes. We change these frequencies quite often in response to 
CPU frequency scaling or other DCVS algorithms.

>> I think the problem that Bjorn is trying to come up with a solution for
>> is how to represent the sleep set and active set in the kernel clock and
>> regulator frameworks. You can think of each RPM resource as a regulator
>> supply. Each one of those resources has an active and sleep set. In the
>> downstream vendor kernels we make two regulators for an RPM resource.
>> One regulator for the active set and one regulator for the active +
>> sleep set. We call these 'active only' and 'active + sleep' regulators
> That seems like it's a big abstraction problem.
>
>> respectively. The RPM regulator driver aggregates the active set for
>> both the regulators via a max() operation and sends that as a request to
>> the RPM. The sleep set is the same as the active set for the 'active +
>> sleep' regulator, so we just send whatever the value is that was sent
>> down via the regulator APIs on the 'active + sleep' regulator. The only
>> driver that really cares about the active only regulators is the CPU
>> clock driver. Otherwise drivers are using the active + sleep regulators
>> because their devices don't stop running when the CPU goes to idle/suspend.
> Hang on a minute.  What you're saying seems to be that this isn't really
> about suspend but actually about the normal operating configuration?
> That makes it a bit more understandable why one would change the
> settings a lot, I think what I'm hearing here is that the runtime state
> changes a lot for some reason and the system needs the suspend state to
> track this?

It also matters for idle. Both suspend and idle can cause an RPM 
notification and switch to the sleep set.

>
> I can't help but think that this all sounds like the RPM isn't mapping
> very well onto practical systems and needs revisiting in future
> versions...  for example with what I'm parsing out of the above an
> active+sleep set command or otherwise having the two modes tied together
> for some regulators would make the whole problem go away.

We create the 'active only' regulators for consumers that actually need 
them. From the set of regulators on a board only a couple need this 
treatment. I don't see how tying the two states together via an 
active+sleep set command would make this problem go away for the cases I 
already described before, i.e. CPU wants some voltage and other IO 
devices want another voltage and the CPU doesn't care what the voltage 
is when the CPU is in idle or suspend. Having a combination active + 
sleep set command would be nice. The RPM already sort of supports this 
by allowing you to only modify the active set. If you never modify the 
sleep set, then the RPM just applies whatever is in the active set to 
the sleep set. We can probably go through and figure out what resources 
could get away with only using the active set so we can cut down on 
sleep set requests that are always the same between active and sleep set.

>> Maybe another solution would be to push the problem into the regulator
>> core and educate it about the two different sets. RPM resources would
>> map one-to-one with a regulator and the sleep set and active sets would
>> be selectable via the regulator_get() API or some other consumer mapping
>> method. This would allow consumers to request whatever set they care
>> about and consolidate the aggregation logic that's duplicated at the
>> consumer level and the driver level into the core.
> I think any duplication that's going on sounds like a consequence of
> the way this is currently implemented.  I think based on what I *think*
> you're saying the RPM driver probably ought to be hiding this and adding
> a property which makes the active and sleep sets track each other with
> normal suspend mode control otherwise.  That could potentially be done
> in the core, though the tracking would be substantial surgery.

Sorry I don't follow this part. It's about more than suspend, we also 
care about idle. I agree that pushing the concept of active vs. sleep 
into the framework is substantial.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/25/2014 12:44 PM, Mark Brown wrote:
> On Mon, Nov 24, 2014 at 01:19:47PM -0800, Stephen Boyd wrote:
>> On 11/24/2014 10:16 AM, Mark Brown wrote:
>>> A bit, but I'm afraid I'm still at a loss as to what the problems and
>>> debate are here.  Why is this more complex than the Linux part of the
>>> system just saying what it wants at any given time?  It sounds like that
>>> has some sort of performance issue?
> I'm still quite confused here...

Hm... hopefully it's not getting worse.

>
>> Yes, communicating with the RPM is not that fast. The sleep set could be
>> updated hundreds of times before we go idle and actually switch from the
>> active set to sleep set. One optimization is to buffer the sleep set
>> requests and send them right before we switch to the sleep set. This
>> saves on the messaging overhead for sleep sets that don't matter when
>> we're active by moving the overhead to the idle/suspend path. The hope
>> is to consolidate multiple sleep set requests into one request.
> Right, that's one of the obvious ones.  Though I am a bit mystified as
> to why you're needing to change these settings so often that it's a
> problem - most hardware can do this but generally the configuration when
> idle is fairly constant so it's irrelevant for performance how often we
> set these things, a lot of the time it's going to boil down to I2C
> writes so we're talking really infrequent here.  What's different about
> these systems that means that this stuff needs constant tweaking?

Yes I don't think we're constantly changing the regulators because it 
does boil down to I2C writes or something similar, but we could be 
modifying other resources that the RPM controls more often. For example, 
the RPM controls some clock frequencies that eventually end up being 
some mmio writes. We change these frequencies quite often in response to 
CPU frequency scaling or other DCVS algorithms.

>> I think the problem that Bjorn is trying to come up with a solution for
>> is how to represent the sleep set and active set in the kernel clock and
>> regulator frameworks. You can think of each RPM resource as a regulator
>> supply. Each one of those resources has an active and sleep set. In the
>> downstream vendor kernels we make two regulators for an RPM resource.
>> One regulator for the active set and one regulator for the active +
>> sleep set. We call these 'active only' and 'active + sleep' regulators
> That seems like it's a big abstraction problem.
>
>> respectively. The RPM regulator driver aggregates the active set for
>> both the regulators via a max() operation and sends that as a request to
>> the RPM. The sleep set is the same as the active set for the 'active +
>> sleep' regulator, so we just send whatever the value is that was sent
>> down via the regulator APIs on the 'active + sleep' regulator. The only
>> driver that really cares about the active only regulators is the CPU
>> clock driver. Otherwise drivers are using the active + sleep regulators
>> because their devices don't stop running when the CPU goes to idle/suspend.
> Hang on a minute.  What you're saying seems to be that this isn't really
> about suspend but actually about the normal operating configuration?
> That makes it a bit more understandable why one would change the
> settings a lot, I think what I'm hearing here is that the runtime state
> changes a lot for some reason and the system needs the suspend state to
> track this?

It also matters for idle. Both suspend and idle can cause an RPM 
notification and switch to the sleep set.

>
> I can't help but think that this all sounds like the RPM isn't mapping
> very well onto practical systems and needs revisiting in future
> versions...  for example with what I'm parsing out of the above an
> active+sleep set command or otherwise having the two modes tied together
> for some regulators would make the whole problem go away.

We create the 'active only' regulators for consumers that actually need 
them. From the set of regulators on a board only a couple need this 
treatment. I don't see how tying the two states together via an 
active+sleep set command would make this problem go away for the cases I 
already described before, i.e. CPU wants some voltage and other IO 
devices want another voltage and the CPU doesn't care what the voltage 
is when the CPU is in idle or suspend. Having a combination active + 
sleep set command would be nice. The RPM already sort of supports this 
by allowing you to only modify the active set. If you never modify the 
sleep set, then the RPM just applies whatever is in the active set to 
the sleep set. We can probably go through and figure out what resources 
could get away with only using the active set so we can cut down on 
sleep set requests that are always the same between active and sleep set.

>> Maybe another solution would be to push the problem into the regulator
>> core and educate it about the two different sets. RPM resources would
>> map one-to-one with a regulator and the sleep set and active sets would
>> be selectable via the regulator_get() API or some other consumer mapping
>> method. This would allow consumers to request whatever set they care
>> about and consolidate the aggregation logic that's duplicated at the
>> consumer level and the driver level into the core.
> I think any duplication that's going on sounds like a consequence of
> the way this is currently implemented.  I think based on what I *think*
> you're saying the RPM driver probably ought to be hiding this and adding
> a property which makes the active and sleep sets track each other with
> normal suspend mode control otherwise.  That could potentially be done
> in the core, though the tracking would be substantial surgery.

Sorry I don't follow this part. It's about more than suspend, we also 
care about idle. I agree that pushing the concept of active vs. sleep 
into the framework is substantial.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3097 bytes --]

On Tue, Nov 25, 2014 at 05:02:52PM -0800, Stephen Boyd wrote:
> On 11/25/2014 12:44 PM, Mark Brown wrote:

> >I'm still quite confused here...

> Hm... hopefully it's not getting worse.

It is a bit, the story keeps changing a lot.

> >I can't help but think that this all sounds like the RPM isn't mapping
> >very well onto practical systems and needs revisiting in future
> >versions...  for example with what I'm parsing out of the above an
> >active+sleep set command or otherwise having the two modes tied together
> >for some regulators would make the whole problem go away.

> We create the 'active only' regulators for consumers that actually need
> them. From the set of regulators on a board only a couple need this

That's what I don't think is making any sense.  Creating two regulators
for the same thing seems like bad news, at best it's going to make reuse
harder.

> treatment. I don't see how tying the two states together via an active+sleep
> set command would make this problem go away for the cases I already
> described before, i.e. CPU wants some voltage and other IO devices want
> another voltage and the CPU doesn't care what the voltage is when the CPU is

Not paying attention to requests from a disabled consumer seems like
something we should be able to do in general.  

> in idle or suspend. Having a combination active + sleep set command would be
> nice. The RPM already sort of supports this by allowing you to only modify
> the active set. If you never modify the sleep set, then the RPM just applies
> whatever is in the active set to the sleep set. We can probably go through
> and figure out what resources could get away with only using the active set
> so we can cut down on sleep set requests that are always the same between
> active and sleep set.

This version is starting to sound a lot like the consumers need to be
able to do an idle to sleep transition and change their settings when
they enter their sleep mode.  It seems a lot like what many devices do
now when they enter and exit runtime PM but with deferred application
depending on other users (for regulators I'm assuming the active set
would actually be a subset of the sleep set of valid voltages).

> >I think any duplication that's going on sounds like a consequence of
> >the way this is currently implemented.  I think based on what I *think*
> >you're saying the RPM driver probably ought to be hiding this and adding
> >a property which makes the active and sleep sets track each other with
> >normal suspend mode control otherwise.  That could potentially be done
> >in the core, though the tracking would be substantial surgery.

> Sorry I don't follow this part. It's about more than suspend, we also care
> about idle. I agree that pushing the concept of active vs. sleep into the
> framework is substantial.

That's not what it seemed like people had been talking about up until
this most recent e-mail, you'd been talking about suspend mode.  The
starting point was mapping onto the suspend support the regulator has.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Tue, Nov 25, 2014 at 05:02:52PM -0800, Stephen Boyd wrote:
> On 11/25/2014 12:44 PM, Mark Brown wrote:

> >I'm still quite confused here...

> Hm... hopefully it's not getting worse.

It is a bit, the story keeps changing a lot.

> >I can't help but think that this all sounds like the RPM isn't mapping
> >very well onto practical systems and needs revisiting in future
> >versions...  for example with what I'm parsing out of the above an
> >active+sleep set command or otherwise having the two modes tied together
> >for some regulators would make the whole problem go away.

> We create the 'active only' regulators for consumers that actually need
> them. From the set of regulators on a board only a couple need this

That's what I don't think is making any sense.  Creating two regulators
for the same thing seems like bad news, at best it's going to make reuse
harder.

> treatment. I don't see how tying the two states together via an active+sleep
> set command would make this problem go away for the cases I already
> described before, i.e. CPU wants some voltage and other IO devices want
> another voltage and the CPU doesn't care what the voltage is when the CPU is

Not paying attention to requests from a disabled consumer seems like
something we should be able to do in general.  

> in idle or suspend. Having a combination active + sleep set command would be
> nice. The RPM already sort of supports this by allowing you to only modify
> the active set. If you never modify the sleep set, then the RPM just applies
> whatever is in the active set to the sleep set. We can probably go through
> and figure out what resources could get away with only using the active set
> so we can cut down on sleep set requests that are always the same between
> active and sleep set.

This version is starting to sound a lot like the consumers need to be
able to do an idle to sleep transition and change their settings when
they enter their sleep mode.  It seems a lot like what many devices do
now when they enter and exit runtime PM but with deferred application
depending on other users (for regulators I'm assuming the active set
would actually be a subset of the sleep set of valid voltages).

> >I think any duplication that's going on sounds like a consequence of
> >the way this is currently implemented.  I think based on what I *think*
> >you're saying the RPM driver probably ought to be hiding this and adding
> >a property which makes the active and sleep sets track each other with
> >normal suspend mode control otherwise.  That could potentially be done
> >in the core, though the tracking would be substantial surgery.

> Sorry I don't follow this part. It's about more than suspend, we also care
> about idle. I agree that pushing the concept of active vs. sleep into the
> framework is substantial.

That's not what it seemed like people had been talking about up until
this most recent e-mail, you'd been talking about suspend mode.  The
starting point was mapping onto the suspend support the regulator has.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141126/99fea7c6/attachment-0001.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Mon 24 Nov 16:02 PST 2014, Stephen Boyd wrote:

> On 11/24/2014 01:59 PM, Bjorn Andersson wrote:
> > On Mon 24 Nov 13:19 PST 2014, Stephen Boyd wrote:
> >
> > [..]
> >> What exactly are we circumventing? I can only guess that we're talking
> >> about the aggregation logic?
> >>
> > We're circumventing the fact that the regulator core doesn't have knowledge
> > about our multiple presented views of the same resource.
> 
> Sorry I don't follow here. Why would the regulator framework care about
> the different views of a resource? Each regulator we make for the
> different views will reflect the request made by Linux for a particular
> set of that RPM resource. So all consumers who are using the S3 active +
> sleep set regulator will aggregate into one request. Likewise, all the
> consumers for the S3 active set regulator will aggregate into another
> request. The only thing that isn't visible is the aggregation between
> the active only and active + sleep regulators, but that can be
> determined by doing a max() of the regulators. Even then, if we consider

My concern was the outcome of this snippet:

regulator_disable(ldo1_active);
regulator_enable(ldo1_both);
regulator_is_enable(ldo1_active);

But after reviewing it again, if one treat it in any other way then 'both' and
'active' being separate on a regulator driver level the end result will not be
sane.

So you're right, if we're to expose X number of regulators per resource they
have to be separate devices.

> that there are other masters also requesting voltages or enable/disable
> for these resources we quickly discover there are other things the
> regulator core doesn't have knowledge about, like what the actual
> voltage is or if the regulator is really off vs. Linux requesting for it
> to be off.
> 

As you say, we're quite a bit down the rabbit hole already by not even being
able to query the hardware for its current state.

> >
> > As the downstream driver shows, if we just implement the right pieces it should
> > work, i.e. give us the correct end result, but it will not be future proof nor
> > pretty. That's why I think we need to discuss how to solve it either in the
> > regulator driver or in the framework.
> >
> > And based on your feedback, it looks like we would have to do something about
> > this in the framework.
> 
> I don't see any problems with making multiple regulators for one RPM
> resource that represent the active set or active + sleep set. Everything
> could be handled in the RPM regulator driver by looking for the other
> regulators that are acting on the same RPM resource and aggregating.
> Maybe you can elaborate on what you think isn't future proof nor pretty
> about this design?
> 

If the regulators are considered completely separate, then the regulator
framework would not notice. I was, incorrectly, assuming that some state was
shared between them.

The "not pretty" part comes from the regulator driver (or a common parent
entity) needing to know what other regulators share a resource and thereby
aggregating the requests. An aggregation that does look a lot like the one
already done in the regulator core.

> As a thought experiment, what if there really was two physical
> independent controllable regulators and a pin from the CPU to the PMIC
> toggled a mux to select between the two. Such a pin would only be
> asserted when the CPU turned off. Would you still want to expose this as
> one regulator to the kernel given that only one supply goes to the CPU
> at any given time?
> 

I think we would have to expose this as two different regulators to the non-cpu
consumers, as that looks like the only way we can affect the "both state".

A possible way around that would be to have the individual regulators exposed
and then provide a regulator with both specified as supply. The regulator core
would aggregate "both" and individual consumer requests to the individual
regulators - without the need of the regulator devices needing to know about
each other.


Something like:
s3a: pm8921_s3_active {
	compatible = "pm8921-smps";
	...
};

s3s: pm8921_s3_sleep {
	compatible = "pm8921-smps";
	sleep; /* <- make requests affect the sleep state only */
};

pm8921_s3 {
	compatible = "dual-supply-regulator";

	active-supply = <&s3a>;
	sleep-supply = <&s3s>;
};

But maybe that's just too crazy...

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Mon 24 Nov 16:02 PST 2014, Stephen Boyd wrote:

> On 11/24/2014 01:59 PM, Bjorn Andersson wrote:
> > On Mon 24 Nov 13:19 PST 2014, Stephen Boyd wrote:
> >
> > [..]
> >> What exactly are we circumventing? I can only guess that we're talking
> >> about the aggregation logic?
> >>
> > We're circumventing the fact that the regulator core doesn't have knowledge
> > about our multiple presented views of the same resource.
> 
> Sorry I don't follow here. Why would the regulator framework care about
> the different views of a resource? Each regulator we make for the
> different views will reflect the request made by Linux for a particular
> set of that RPM resource. So all consumers who are using the S3 active +
> sleep set regulator will aggregate into one request. Likewise, all the
> consumers for the S3 active set regulator will aggregate into another
> request. The only thing that isn't visible is the aggregation between
> the active only and active + sleep regulators, but that can be
> determined by doing a max() of the regulators. Even then, if we consider

My concern was the outcome of this snippet:

regulator_disable(ldo1_active);
regulator_enable(ldo1_both);
regulator_is_enable(ldo1_active);

But after reviewing it again, if one treat it in any other way then 'both' and
'active' being separate on a regulator driver level the end result will not be
sane.

So you're right, if we're to expose X number of regulators per resource they
have to be separate devices.

> that there are other masters also requesting voltages or enable/disable
> for these resources we quickly discover there are other things the
> regulator core doesn't have knowledge about, like what the actual
> voltage is or if the regulator is really off vs. Linux requesting for it
> to be off.
> 

As you say, we're quite a bit down the rabbit hole already by not even being
able to query the hardware for its current state.

> >
> > As the downstream driver shows, if we just implement the right pieces it should
> > work, i.e. give us the correct end result, but it will not be future proof nor
> > pretty. That's why I think we need to discuss how to solve it either in the
> > regulator driver or in the framework.
> >
> > And based on your feedback, it looks like we would have to do something about
> > this in the framework.
> 
> I don't see any problems with making multiple regulators for one RPM
> resource that represent the active set or active + sleep set. Everything
> could be handled in the RPM regulator driver by looking for the other
> regulators that are acting on the same RPM resource and aggregating.
> Maybe you can elaborate on what you think isn't future proof nor pretty
> about this design?
> 

If the regulators are considered completely separate, then the regulator
framework would not notice. I was, incorrectly, assuming that some state was
shared between them.

The "not pretty" part comes from the regulator driver (or a common parent
entity) needing to know what other regulators share a resource and thereby
aggregating the requests. An aggregation that does look a lot like the one
already done in the regulator core.

> As a thought experiment, what if there really was two physical
> independent controllable regulators and a pin from the CPU to the PMIC
> toggled a mux to select between the two. Such a pin would only be
> asserted when the CPU turned off. Would you still want to expose this as
> one regulator to the kernel given that only one supply goes to the CPU
> at any given time?
> 

I think we would have to expose this as two different regulators to the non-cpu
consumers, as that looks like the only way we can affect the "both state".

A possible way around that would be to have the individual regulators exposed
and then provide a regulator with both specified as supply. The regulator core
would aggregate "both" and individual consumer requests to the individual
regulators - without the need of the regulator devices needing to know about
each other.


Something like:
s3a: pm8921_s3_active {
	compatible = "pm8921-smps";
	...
};

s3s: pm8921_s3_sleep {
	compatible = "pm8921-smps";
	sleep; /* <- make requests affect the sleep state only */
};

pm8921_s3 {
	compatible = "dual-supply-regulator";

	active-supply = <&s3a>;
	sleep-supply = <&s3s>;
};

But maybe that's just too crazy...

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Tue 25 Nov 12:44 PST 2014, Mark Brown wrote:

> On Mon, Nov 24, 2014 at 01:19:47PM -0800, Stephen Boyd wrote:
> > On 11/24/2014 10:16 AM, Mark Brown wrote:

[..]

> > respectively. The RPM regulator driver aggregates the active set for
> > both the regulators via a max() operation and sends that as a request to
> > the RPM. The sleep set is the same as the active set for the 'active +
> > sleep' regulator, so we just send whatever the value is that was sent
> > down via the regulator APIs on the 'active + sleep' regulator. The only
> > driver that really cares about the active only regulators is the CPU
> > clock driver. Otherwise drivers are using the active + sleep regulators
> > because their devices don't stop running when the CPU goes to idle/suspend.
> 
> Hang on a minute.  What you're saying seems to be that this isn't really
> about suspend but actually about the normal operating configuration?
> That makes it a bit more understandable why one would change the
> settings a lot, I think what I'm hearing here is that the runtime state
> changes a lot for some reason and the system needs the suspend state to
> track this?
> 

I think it's important that we clarify that we have 2 (somewhat) diffrent
problem on the table here:

Problem 1:
Regulators are shared between CPU PLLs and other peripherals and when we take
the cpu to idle we need a way to let go of the CPU PLL "vote" on the regulator.

In the case of the CPU being the only consumer I think we've come down to the
problems being:
* we're in atomic context
* we don't want to waste the time of making sending out the request
* we have hardware support for this and we want to use it(?)

In the case of us having other active consumers (e.g. GPU, display or some
peripherals) the voltage range or operating mode specified by the cpufreq
driver might be suboptimal when we remove the cpu.

I assume we don't want to handle this explicitly due to the same reasons as
above(?)


Problem 2:
As we're implementing a solution for problem 1 we end up with a set of writes
to the sleep state that are superfluous. The codeaurora solution for this is to
buffer any writes to the sleep state and right before putting the cpu into idle
state there's a direct call going, that flushes the buffered writes.

This is "only" and optimization and I think the tricky parts are how to
actually trigger the flush from the cpuidle driver and how to handle the smd
communication in a sane way.

> I can't help but think that this all sounds like the RPM isn't mapping
> very well onto practical systems and needs revisiting in future
> versions...  for example with what I'm parsing out of the above an
> active+sleep set command or otherwise having the two modes tied together
> for some regulators would make the whole problem go away.
> 

As Stephen answered for regulators that only have the "both state" only one
write is needed.

And if I read the docs correctly there are 4 regulators that needs this special
handling and 29 that works just as you normally would expect.

> > Maybe another solution would be to push the problem into the regulator
> > core and educate it about the two different sets. RPM resources would
> > map one-to-one with a regulator and the sleep set and active sets would
> > be selectable via the regulator_get() API or some other consumer mapping
> > method. This would allow consumers to request whatever set they care
> > about and consolidate the aggregation logic that's duplicated at the
> > consumer level and the driver level into the core.
> 
> I think any duplication that's going on sounds like a consequence of
> the way this is currently implemented.  I think based on what I *think*
> you're saying the RPM driver probably ought to be hiding this and adding
> a property which makes the active and sleep sets track each other with
> normal suspend mode control otherwise.  That could potentially be done
> in the core, though the tracking would be substantial surgery.

I think we should start off by hiding this in the driver, if others come
forward needing something similar we can look at how to integrate parts of it
into the core.

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Tue 25 Nov 12:44 PST 2014, Mark Brown wrote:

> On Mon, Nov 24, 2014 at 01:19:47PM -0800, Stephen Boyd wrote:
> > On 11/24/2014 10:16 AM, Mark Brown wrote:

[..]

> > respectively. The RPM regulator driver aggregates the active set for
> > both the regulators via a max() operation and sends that as a request to
> > the RPM. The sleep set is the same as the active set for the 'active +
> > sleep' regulator, so we just send whatever the value is that was sent
> > down via the regulator APIs on the 'active + sleep' regulator. The only
> > driver that really cares about the active only regulators is the CPU
> > clock driver. Otherwise drivers are using the active + sleep regulators
> > because their devices don't stop running when the CPU goes to idle/suspend.
> 
> Hang on a minute.  What you're saying seems to be that this isn't really
> about suspend but actually about the normal operating configuration?
> That makes it a bit more understandable why one would change the
> settings a lot, I think what I'm hearing here is that the runtime state
> changes a lot for some reason and the system needs the suspend state to
> track this?
> 

I think it's important that we clarify that we have 2 (somewhat) diffrent
problem on the table here:

Problem 1:
Regulators are shared between CPU PLLs and other peripherals and when we take
the cpu to idle we need a way to let go of the CPU PLL "vote" on the regulator.

In the case of the CPU being the only consumer I think we've come down to the
problems being:
* we're in atomic context
* we don't want to waste the time of making sending out the request
* we have hardware support for this and we want to use it(?)

In the case of us having other active consumers (e.g. GPU, display or some
peripherals) the voltage range or operating mode specified by the cpufreq
driver might be suboptimal when we remove the cpu.

I assume we don't want to handle this explicitly due to the same reasons as
above(?)


Problem 2:
As we're implementing a solution for problem 1 we end up with a set of writes
to the sleep state that are superfluous. The codeaurora solution for this is to
buffer any writes to the sleep state and right before putting the cpu into idle
state there's a direct call going, that flushes the buffered writes.

This is "only" and optimization and I think the tricky parts are how to
actually trigger the flush from the cpuidle driver and how to handle the smd
communication in a sane way.

> I can't help but think that this all sounds like the RPM isn't mapping
> very well onto practical systems and needs revisiting in future
> versions...  for example with what I'm parsing out of the above an
> active+sleep set command or otherwise having the two modes tied together
> for some regulators would make the whole problem go away.
> 

As Stephen answered for regulators that only have the "both state" only one
write is needed.

And if I read the docs correctly there are 4 regulators that needs this special
handling and 29 that works just as you normally would expect.

> > Maybe another solution would be to push the problem into the regulator
> > core and educate it about the two different sets. RPM resources would
> > map one-to-one with a regulator and the sleep set and active sets would
> > be selectable via the regulator_get() API or some other consumer mapping
> > method. This would allow consumers to request whatever set they care
> > about and consolidate the aggregation logic that's duplicated at the
> > consumer level and the driver level into the core.
> 
> I think any duplication that's going on sounds like a consequence of
> the way this is currently implemented.  I think based on what I *think*
> you're saying the RPM driver probably ought to be hiding this and adding
> a property which makes the active and sleep sets track each other with
> normal suspend mode control otherwise.  That could potentially be done
> in the core, though the tracking would be substantial surgery.

I think we should start off by hiding this in the driver, if others come
forward needing something similar we can look at how to integrate parts of it
into the core.

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/26/2014 05:40 AM, Mark Brown wrote:
> On Tue, Nov 25, 2014 at 05:02:52PM -0800, Stephen Boyd wrote:
>> On 11/25/2014 12:44 PM, Mark Brown wrote:
>>
>>
>>> I can't help but think that this all sounds like the RPM isn't mapping
>>> very well onto practical systems and needs revisiting in future
>>> versions...  for example with what I'm parsing out of the above an
>>> active+sleep set command or otherwise having the two modes tied together
>>> for some regulators would make the whole problem go away.
>> We create the 'active only' regulators for consumers that actually need
>> them. From the set of regulators on a board only a couple need this
> That's what I don't think is making any sense.  Creating two regulators
> for the same thing seems like bad news, at best it's going to make reuse
> harder.

What reuse is harder?

>
>> treatment. I don't see how tying the two states together via an active+sleep
>> set command would make this problem go away for the cases I already
>> described before, i.e. CPU wants some voltage and other IO devices want
>> another voltage and the CPU doesn't care what the voltage is when the CPU is
> Not paying attention to requests from a disabled consumer seems like
> something we should be able to do in general.

Sounds good.

>
>> in idle or suspend. Having a combination active + sleep set command would be
>> nice. The RPM already sort of supports this by allowing you to only modify
>> the active set. If you never modify the sleep set, then the RPM just applies
>> whatever is in the active set to the sleep set. We can probably go through
>> and figure out what resources could get away with only using the active set
>> so we can cut down on sleep set requests that are always the same between
>> active and sleep set.
> This version is starting to sound a lot like the consumers need to be
> able to do an idle to sleep transition and change their settings when
> they enter their sleep mode.  It seems a lot like what many devices do
> now when they enter and exit runtime PM but with deferred application
> depending on other users (for regulators I'm assuming the active set
> would actually be a subset of the sleep set of valid voltages).

Yes, one key point is that we want the transition to be handled by the 
RPM co-processor. The reason being that usually we want to change 
settings after the last CPU transitions into their sleep mode. There's 
no way to do this without relying on some external co-processor to go 
and change the settings for us.

>
>>> I think any duplication that's going on sounds like a consequence of
>>> the way this is currently implemented.  I think based on what I *think*
>>> you're saying the RPM driver probably ought to be hiding this and adding
>>> a property which makes the active and sleep sets track each other with
>>> normal suspend mode control otherwise.  That could potentially be done
>>> in the core, though the tracking would be substantial surgery.
>> Sorry I don't follow this part. It's about more than suspend, we also care
>> about idle. I agree that pushing the concept of active vs. sleep into the
>> framework is substantial.
> That's not what it seemed like people had been talking about up until
> this most recent e-mail, you'd been talking about suspend mode.  The
> starting point was mapping onto the suspend support the regulator has.

Hm? I've been saying "idle and suspend" each time. Perhaps the initial 
discussion about regulator suspend support threw things off.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/26/2014 05:40 AM, Mark Brown wrote:
> On Tue, Nov 25, 2014 at 05:02:52PM -0800, Stephen Boyd wrote:
>> On 11/25/2014 12:44 PM, Mark Brown wrote:
>>
>>
>>> I can't help but think that this all sounds like the RPM isn't mapping
>>> very well onto practical systems and needs revisiting in future
>>> versions...  for example with what I'm parsing out of the above an
>>> active+sleep set command or otherwise having the two modes tied together
>>> for some regulators would make the whole problem go away.
>> We create the 'active only' regulators for consumers that actually need
>> them. From the set of regulators on a board only a couple need this
> That's what I don't think is making any sense.  Creating two regulators
> for the same thing seems like bad news, at best it's going to make reuse
> harder.

What reuse is harder?

>
>> treatment. I don't see how tying the two states together via an active+sleep
>> set command would make this problem go away for the cases I already
>> described before, i.e. CPU wants some voltage and other IO devices want
>> another voltage and the CPU doesn't care what the voltage is when the CPU is
> Not paying attention to requests from a disabled consumer seems like
> something we should be able to do in general.

Sounds good.

>
>> in idle or suspend. Having a combination active + sleep set command would be
>> nice. The RPM already sort of supports this by allowing you to only modify
>> the active set. If you never modify the sleep set, then the RPM just applies
>> whatever is in the active set to the sleep set. We can probably go through
>> and figure out what resources could get away with only using the active set
>> so we can cut down on sleep set requests that are always the same between
>> active and sleep set.
> This version is starting to sound a lot like the consumers need to be
> able to do an idle to sleep transition and change their settings when
> they enter their sleep mode.  It seems a lot like what many devices do
> now when they enter and exit runtime PM but with deferred application
> depending on other users (for regulators I'm assuming the active set
> would actually be a subset of the sleep set of valid voltages).

Yes, one key point is that we want the transition to be handled by the 
RPM co-processor. The reason being that usually we want to change 
settings after the last CPU transitions into their sleep mode. There's 
no way to do this without relying on some external co-processor to go 
and change the settings for us.

>
>>> I think any duplication that's going on sounds like a consequence of
>>> the way this is currently implemented.  I think based on what I *think*
>>> you're saying the RPM driver probably ought to be hiding this and adding
>>> a property which makes the active and sleep sets track each other with
>>> normal suspend mode control otherwise.  That could potentially be done
>>> in the core, though the tracking would be substantial surgery.
>> Sorry I don't follow this part. It's about more than suspend, we also care
>> about idle. I agree that pushing the concept of active vs. sleep into the
>> framework is substantial.
> That's not what it seemed like people had been talking about up until
> this most recent e-mail, you'd been talking about suspend mode.  The
> starting point was mapping onto the suspend support the regulator has.

Hm? I've been saying "idle and suspend" each time. Perhaps the initial 
discussion about regulator suspend support threw things off.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2004 bytes --]

On Wed, Nov 26, 2014 at 05:51:32PM -0800, Stephen Boyd wrote:
> On 11/26/2014 05:40 AM, Mark Brown wrote:

> >That's what I don't think is making any sense.  Creating two regulators
> >for the same thing seems like bad news, at best it's going to make reuse
> >harder.

> What reuse is harder?

The consumer needs to understand the fact that a given supply might be
represented by multiple different regulators that it sees.  This means
the system integration is not being abstracted away from the consumer.

> >This version is starting to sound a lot like the consumers need to be
> >able to do an idle to sleep transition and change their settings when
> >they enter their sleep mode.  It seems a lot like what many devices do
> >now when they enter and exit runtime PM but with deferred application
> >depending on other users (for regulators I'm assuming the active set
> >would actually be a subset of the sleep set of valid voltages).

> Yes, one key point is that we want the transition to be handled by the RPM
> co-processor. The reason being that usually we want to change settings after
> the last CPU transitions into their sleep mode. There's no way to do this
> without relying on some external co-processor to go and change the settings
> for us.

That all seems like something that can be handled in the RPM driver, it
just needs something to kick it to do the actual writes and let the
consumers proceed as normal.

> >That's not what it seemed like people had been talking about up until
> >this most recent e-mail, you'd been talking about suspend mode.  The
> >starting point was mapping onto the suspend support the regulator has.

> Hm? I've been saying "idle and suspend" each time. Perhaps the initial
> discussion about regulator suspend support threw things off.

I was being asked about use of regulator suspend support, I naively
thought that this might mean the discussion was something about suspend!
It's also not terribly clear to me what the involvement of idle is here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Wed, Nov 26, 2014 at 05:51:32PM -0800, Stephen Boyd wrote:
> On 11/26/2014 05:40 AM, Mark Brown wrote:

> >That's what I don't think is making any sense.  Creating two regulators
> >for the same thing seems like bad news, at best it's going to make reuse
> >harder.

> What reuse is harder?

The consumer needs to understand the fact that a given supply might be
represented by multiple different regulators that it sees.  This means
the system integration is not being abstracted away from the consumer.

> >This version is starting to sound a lot like the consumers need to be
> >able to do an idle to sleep transition and change their settings when
> >they enter their sleep mode.  It seems a lot like what many devices do
> >now when they enter and exit runtime PM but with deferred application
> >depending on other users (for regulators I'm assuming the active set
> >would actually be a subset of the sleep set of valid voltages).

> Yes, one key point is that we want the transition to be handled by the RPM
> co-processor. The reason being that usually we want to change settings after
> the last CPU transitions into their sleep mode. There's no way to do this
> without relying on some external co-processor to go and change the settings
> for us.

That all seems like something that can be handled in the RPM driver, it
just needs something to kick it to do the actual writes and let the
consumers proceed as normal.

> >That's not what it seemed like people had been talking about up until
> >this most recent e-mail, you'd been talking about suspend mode.  The
> >starting point was mapping onto the suspend support the regulator has.

> Hm? I've been saying "idle and suspend" each time. Perhaps the initial
> discussion about regulator suspend support threw things off.

I was being asked about use of regulator suspend support, I naively
thought that this might mean the discussion was something about suspend!
It's also not terribly clear to me what the involvement of idle is here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/a56329be/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

On Wed, Nov 26, 2014 at 03:34:47PM -0800, Bjorn Andersson wrote:

> Problem 1:
> Regulators are shared between CPU PLLs and other peripherals and when we take
> the cpu to idle we need a way to let go of the CPU PLL "vote" on the regulator.

> In the case of the CPU being the only consumer I think we've come down to the
> problems being:
> * we're in atomic context

That shouldn't be an issue, we can do something like what we've got for
regmap and allow regulators to replace their mutexes with spinlocks
(users and drivers will need to be careful they don't do expensive
things, plus we wamy want to look at optimizing the core, but that's
true anyway).

> * we don't want to waste the time of making sending out the request
> * we have hardware support for this and we want to use it(?)

I'm still hazy on what that actually means.  I thought the CPU case was
one of the big ones where you *did* want to act on the configuration
changes?

> In the case of us having other active consumers (e.g. GPU, display or some
> peripherals) the voltage range or operating mode specified by the cpufreq
> driver might be suboptimal when we remove the cpu.

I don't understand.  Why is the other consumer asking for something it
doesn't need itself?

> Problem 2:
> As we're implementing a solution for problem 1 we end up with a set of writes
> to the sleep state that are superfluous. The codeaurora solution for this is to
> buffer any writes to the sleep state and right before putting the cpu into idle
> state there's a direct call going, that flushes the buffered writes.

> This is "only" and optimization and I think the tricky parts are how to
> actually trigger the flush from the cpuidle driver and how to handle the smd
> communication in a sane way.

That seems like a very obvious thing to do.

> > I think any duplication that's going on sounds like a consequence of
> > the way this is currently implemented.  I think based on what I *think*
> > you're saying the RPM driver probably ought to be hiding this and adding
> > a property which makes the active and sleep sets track each other with
> > normal suspend mode control otherwise.  That could potentially be done
> > in the core, though the tracking would be substantial surgery.

> I think we should start off by hiding this in the driver, if others come
> forward needing something similar we can look at how to integrate parts of it
> into the core.

If you're hiding it in the driver make sure it's *hidden* in the driver
- exporting duplicate regulators doesn't sound like hiding to me.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Wed, Nov 26, 2014 at 03:34:47PM -0800, Bjorn Andersson wrote:

> Problem 1:
> Regulators are shared between CPU PLLs and other peripherals and when we take
> the cpu to idle we need a way to let go of the CPU PLL "vote" on the regulator.

> In the case of the CPU being the only consumer I think we've come down to the
> problems being:
> * we're in atomic context

That shouldn't be an issue, we can do something like what we've got for
regmap and allow regulators to replace their mutexes with spinlocks
(users and drivers will need to be careful they don't do expensive
things, plus we wamy want to look at optimizing the core, but that's
true anyway).

> * we don't want to waste the time of making sending out the request
> * we have hardware support for this and we want to use it(?)

I'm still hazy on what that actually means.  I thought the CPU case was
one of the big ones where you *did* want to act on the configuration
changes?

> In the case of us having other active consumers (e.g. GPU, display or some
> peripherals) the voltage range or operating mode specified by the cpufreq
> driver might be suboptimal when we remove the cpu.

I don't understand.  Why is the other consumer asking for something it
doesn't need itself?

> Problem 2:
> As we're implementing a solution for problem 1 we end up with a set of writes
> to the sleep state that are superfluous. The codeaurora solution for this is to
> buffer any writes to the sleep state and right before putting the cpu into idle
> state there's a direct call going, that flushes the buffered writes.

> This is "only" and optimization and I think the tricky parts are how to
> actually trigger the flush from the cpuidle driver and how to handle the smd
> communication in a sane way.

That seems like a very obvious thing to do.

> > I think any duplication that's going on sounds like a consequence of
> > the way this is currently implemented.  I think based on what I *think*
> > you're saying the RPM driver probably ought to be hiding this and adding
> > a property which makes the active and sleep sets track each other with
> > normal suspend mode control otherwise.  That could potentially be done
> > in the core, though the tracking would be substantial surgery.

> I think we should start off by hiding this in the driver, if others come
> forward needing something similar we can look at how to integrate parts of it
> into the core.

If you're hiding it in the driver make sure it's *hidden* in the driver
- exporting duplicate regulators doesn't sound like hiding to me.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141127/afd860fd/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Thu, Nov 27, 2014 at 11:02 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Nov 26, 2014 at 03:34:47PM -0800, Bjorn Andersson wrote:
>
>> Problem 1:
>> Regulators are shared between CPU PLLs and other peripherals and when we take
>> the cpu to idle we need a way to let go of the CPU PLL "vote" on the regulator.
>
>> In the case of the CPU being the only consumer I think we've come down to the
>> problems being:
>> * we're in atomic context
>
> That shouldn't be an issue, we can do something like what we've got for
> regmap and allow regulators to replace their mutexes with spinlocks
> (users and drivers will need to be careful they don't do expensive
> things, plus we wamy want to look at optimizing the core, but that's
> true anyway).
>

If I understand Stephen correctly, this is basically what would happen
when we go to suspend (take the CPUs off the individual PLLs and
releasing the regulators), but for going in and out of _idle_ this is
too costly and we want to utilise the hardware assist for this.

>> * we don't want to waste the time of making sending out the request
>> * we have hardware support for this and we want to use it(?)
>
> I'm still hazy on what that actually means.  I thought the CPU case was
> one of the big ones where you *did* want to act on the configuration
> changes?
>

By utilising the hardware support for switching between active and
sleep state (and having the cpu only affecting the active state) we
can do this asynchronously, while being in wfi.

>> In the case of us having other active consumers (e.g. GPU, display or some
>> peripherals) the voltage range or operating mode specified by the cpufreq
>> driver might be suboptimal when we remove the cpu.
>
> I don't understand.  Why is the other consumer asking for something it
> doesn't need itself?
>

I don't have the actual numbers, but we have a situation that looks like this:
gpu driver does regulator_set_voltage(.5V, 1V);
cpufreq driver does regulator_set_voltage(1V,1V);

The regulator framework will make the regulator tick at 1V

Upon going to sleep the cpuidle driver will, through the hardware
support, make the regulator tick at .5V. Before returning the cpu from
idle the hardware will make the regulator tick at 1V again.

[..]

>> > I think any duplication that's going on sounds like a consequence of
>> > the way this is currently implemented.  I think based on what I *think*
>> > you're saying the RPM driver probably ought to be hiding this and adding
>> > a property which makes the active and sleep sets track each other with
>> > normal suspend mode control otherwise.  That could potentially be done
>> > in the core, though the tracking would be substantial surgery.
>
>> I think we should start off by hiding this in the driver, if others come
>> forward needing something similar we can look at how to integrate parts of it
>> into the core.
>
> If you're hiding it in the driver make sure it's *hidden* in the driver
> - exporting duplicate regulators doesn't sound like hiding to me.
>

This is the problem at hand, we have not found any way to actually
hide this in the driver as the two modes depend on the consumer and
not the aggregated values we get out of the regulator framework.

Or rather, my proposal would do that, but that will not be able to
handle the case when there's different voltages in the two states. It
would only take care of the enable/disable case.


The exposure of multiple regulators moves the problem to the
devicetree, making sure to map the consumers to the right
state/regulator. But it should only be the cpu (in its various forms)
that ever consume the active only regulator.

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Thu, Nov 27, 2014 at 11:02 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Nov 26, 2014 at 03:34:47PM -0800, Bjorn Andersson wrote:
>
>> Problem 1:
>> Regulators are shared between CPU PLLs and other peripherals and when we take
>> the cpu to idle we need a way to let go of the CPU PLL "vote" on the regulator.
>
>> In the case of the CPU being the only consumer I think we've come down to the
>> problems being:
>> * we're in atomic context
>
> That shouldn't be an issue, we can do something like what we've got for
> regmap and allow regulators to replace their mutexes with spinlocks
> (users and drivers will need to be careful they don't do expensive
> things, plus we wamy want to look at optimizing the core, but that's
> true anyway).
>

If I understand Stephen correctly, this is basically what would happen
when we go to suspend (take the CPUs off the individual PLLs and
releasing the regulators), but for going in and out of _idle_ this is
too costly and we want to utilise the hardware assist for this.

>> * we don't want to waste the time of making sending out the request
>> * we have hardware support for this and we want to use it(?)
>
> I'm still hazy on what that actually means.  I thought the CPU case was
> one of the big ones where you *did* want to act on the configuration
> changes?
>

By utilising the hardware support for switching between active and
sleep state (and having the cpu only affecting the active state) we
can do this asynchronously, while being in wfi.

>> In the case of us having other active consumers (e.g. GPU, display or some
>> peripherals) the voltage range or operating mode specified by the cpufreq
>> driver might be suboptimal when we remove the cpu.
>
> I don't understand.  Why is the other consumer asking for something it
> doesn't need itself?
>

I don't have the actual numbers, but we have a situation that looks like this:
gpu driver does regulator_set_voltage(.5V, 1V);
cpufreq driver does regulator_set_voltage(1V,1V);

The regulator framework will make the regulator tick at 1V

Upon going to sleep the cpuidle driver will, through the hardware
support, make the regulator tick at .5V. Before returning the cpu from
idle the hardware will make the regulator tick at 1V again.

[..]

>> > I think any duplication that's going on sounds like a consequence of
>> > the way this is currently implemented.  I think based on what I *think*
>> > you're saying the RPM driver probably ought to be hiding this and adding
>> > a property which makes the active and sleep sets track each other with
>> > normal suspend mode control otherwise.  That could potentially be done
>> > in the core, though the tracking would be substantial surgery.
>
>> I think we should start off by hiding this in the driver, if others come
>> forward needing something similar we can look at how to integrate parts of it
>> into the core.
>
> If you're hiding it in the driver make sure it's *hidden* in the driver
> - exporting duplicate regulators doesn't sound like hiding to me.
>

This is the problem at hand, we have not found any way to actually
hide this in the driver as the two modes depend on the consumer and
not the aggregated values we get out of the regulator framework.

Or rather, my proposal would do that, but that will not be able to
handle the case when there's different voltages in the two states. It
would only take care of the enable/disable case.


The exposure of multiple regulators moves the problem to the
devicetree, making sure to map the consumers to the right
state/regulator. But it should only be the cpu (in its various forms)
that ever consume the active only regulator.

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2905 bytes --]

On Thu, Nov 27, 2014 at 11:42:41AM -0800, Bjorn Andersson wrote:
> On Thu, Nov 27, 2014 at 11:02 AM, Mark Brown <broonie@kernel.org> wrote:

> >> * we don't want to waste the time of making sending out the request
> >> * we have hardware support for this and we want to use it(?)

> > I'm still hazy on what that actually means.  I thought the CPU case was
> > one of the big ones where you *did* want to act on the configuration
> > changes?

> By utilising the hardware support for switching between active and
> sleep state (and having the cpu only affecting the active state) we
> can do this asynchronously, while being in wfi.

Hrm, right.  This is starting to make sense.  So what we're saying here
is roughly that you want to use a separate suspend-like mode (the extra
set of registers for the low power mode mode) but are entering it
without actually suspending the whole system?

> >> In the case of us having other active consumers (e.g. GPU, display or some
> >> peripherals) the voltage range or operating mode specified by the cpufreq
> >> driver might be suboptimal when we remove the cpu.

> > I don't understand.  Why is the other consumer asking for something it
> > doesn't need itself?

> I don't have the actual numbers, but we have a situation that looks like this:
> gpu driver does regulator_set_voltage(.5V, 1V);
> cpufreq driver does regulator_set_voltage(1V,1V);

> The regulator framework will make the regulator tick at 1V

> Upon going to sleep the cpuidle driver will, through the hardware
> support, make the regulator tick at .5V. Before returning the cpu from
> idle the hardware will make the regulator tick at 1V again.

So the regulator framework should be fine if the CPU regulator
constraint was disabled while the CPU consumer was disabled (modulo the
performance issue)?

> > If you're hiding it in the driver make sure it's *hidden* in the driver
> > - exporting duplicate regulators doesn't sound like hiding to me.

> This is the problem at hand, we have not found any way to actually
> hide this in the driver as the two modes depend on the consumer and
> not the aggregated values we get out of the regulator framework.

> Or rather, my proposal would do that, but that will not be able to
> handle the case when there's different voltages in the two states. It
> would only take care of the enable/disable case.

So, I think something along the lines of suspend mode ought to be able
to handle this.  What we need is a way of accelerating the entry and
exit, right?

> The exposure of multiple regulators moves the problem to the
> devicetree, making sure to map the consumers to the right
> state/regulator. But it should only be the cpu (in its various forms)
> that ever consume the active only regulator.

He said hopefully...  :)

I think I now have a reasonable picture of what's going on but wanted to
confirm that what I'm saying above makes sense.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Thu, Nov 27, 2014 at 11:42:41AM -0800, Bjorn Andersson wrote:
> On Thu, Nov 27, 2014 at 11:02 AM, Mark Brown <broonie@kernel.org> wrote:

> >> * we don't want to waste the time of making sending out the request
> >> * we have hardware support for this and we want to use it(?)

> > I'm still hazy on what that actually means.  I thought the CPU case was
> > one of the big ones where you *did* want to act on the configuration
> > changes?

> By utilising the hardware support for switching between active and
> sleep state (and having the cpu only affecting the active state) we
> can do this asynchronously, while being in wfi.

Hrm, right.  This is starting to make sense.  So what we're saying here
is roughly that you want to use a separate suspend-like mode (the extra
set of registers for the low power mode mode) but are entering it
without actually suspending the whole system?

> >> In the case of us having other active consumers (e.g. GPU, display or some
> >> peripherals) the voltage range or operating mode specified by the cpufreq
> >> driver might be suboptimal when we remove the cpu.

> > I don't understand.  Why is the other consumer asking for something it
> > doesn't need itself?

> I don't have the actual numbers, but we have a situation that looks like this:
> gpu driver does regulator_set_voltage(.5V, 1V);
> cpufreq driver does regulator_set_voltage(1V,1V);

> The regulator framework will make the regulator tick at 1V

> Upon going to sleep the cpuidle driver will, through the hardware
> support, make the regulator tick at .5V. Before returning the cpu from
> idle the hardware will make the regulator tick at 1V again.

So the regulator framework should be fine if the CPU regulator
constraint was disabled while the CPU consumer was disabled (modulo the
performance issue)?

> > If you're hiding it in the driver make sure it's *hidden* in the driver
> > - exporting duplicate regulators doesn't sound like hiding to me.

> This is the problem at hand, we have not found any way to actually
> hide this in the driver as the two modes depend on the consumer and
> not the aggregated values we get out of the regulator framework.

> Or rather, my proposal would do that, but that will not be able to
> handle the case when there's different voltages in the two states. It
> would only take care of the enable/disable case.

So, I think something along the lines of suspend mode ought to be able
to handle this.  What we need is a way of accelerating the entry and
exit, right?

> The exposure of multiple regulators moves the problem to the
> devicetree, making sure to map the consumers to the right
> state/regulator. But it should only be the cpu (in its various forms)
> that ever consume the active only regulator.

He said hopefully...  :)

I think I now have a reasonable picture of what's going on but wanted to
confirm that what I'm saying above makes sense.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141128/8704411b/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/28/2014 12:16 PM, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 11:42:41AM -0800, Bjorn Andersson wrote:
>> On Thu, Nov 27, 2014 at 11:02 AM, Mark Brown <broonie@kernel.org> wrote:
>>>> * we don't want to waste the time of making sending out the request
>>>> * we have hardware support for this and we want to use it(?)
>>> I'm still hazy on what that actually means.  I thought the CPU case was
>>> one of the big ones where you *did* want to act on the configuration
>>> changes?
>> By utilising the hardware support for switching between active and
>> sleep state (and having the cpu only affecting the active state) we
>> can do this asynchronously, while being in wfi.
> Hrm, right.  This is starting to make sense.  So what we're saying here
> is roughly that you want to use a separate suspend-like mode (the extra
> set of registers for the low power mode mode) but are entering it
> without actually suspending the whole system?

For the idle path, yes. We also want to do the same sort of thing on the
system suspend path.

>
>>>> In the case of us having other active consumers (e.g. GPU, display or some
>>>> peripherals) the voltage range or operating mode specified by the cpufreq
>>>> driver might be suboptimal when we remove the cpu.
>>> I don't understand.  Why is the other consumer asking for something it
>>> doesn't need itself?
>> I don't have the actual numbers, but we have a situation that looks like this:
>> gpu driver does regulator_set_voltage(.5V, 1V);
>> cpufreq driver does regulator_set_voltage(1V,1V);
>> The regulator framework will make the regulator tick at 1V
>> Upon going to sleep the cpuidle driver will, through the hardware
>> support, make the regulator tick at .5V. Before returning the cpu from
>> idle the hardware will make the regulator tick at 1V again.
> So the regulator framework should be fine if the CPU regulator
> constraint was disabled while the CPU consumer was disabled (modulo the
> performance issue)?

The CPU regulator consumer won't be disabled through any software
mechanism. It will stay enabled in software and when we execute wfi the
hardware will drop that consumers request because it was only requesting
in the active set.

>
>>> If you're hiding it in the driver make sure it's *hidden* in the driver
>>> - exporting duplicate regulators doesn't sound like hiding to me.
>> This is the problem at hand, we have not found any way to actually
>> hide this in the driver as the two modes depend on the consumer and
>> not the aggregated values we get out of the regulator framework.
>> Or rather, my proposal would do that, but that will not be able to
>> handle the case when there's different voltages in the two states. It
>> would only take care of the enable/disable case.
> So, I think something along the lines of suspend mode ought to be able
> to handle this.  What we need is a way of accelerating the entry and
> exit, right?
>
>> The exposure of multiple regulators moves the problem to the
>> devicetree, making sure to map the consumers to the right
>> state/regulator. But it should only be the cpu (in its various forms)
>> that ever consume the active only regulator.
> He said hopefully...  :)
>
> I think I now have a reasonable picture of what's going on but wanted to
> confirm that what I'm saying above makes sense.

That's good. Is there any conclusion here? I'm still thinking that
having multiple regulators for the same physical supply is the right
thing to do.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC 0/2] Qualcomm RPM sleep states

[Stephen Boyd]

On 11/28/2014 12:16 PM, Mark Brown wrote:
> On Thu, Nov 27, 2014 at 11:42:41AM -0800, Bjorn Andersson wrote:
>> On Thu, Nov 27, 2014 at 11:02 AM, Mark Brown <broonie@kernel.org> wrote:
>>>> * we don't want to waste the time of making sending out the request
>>>> * we have hardware support for this and we want to use it(?)
>>> I'm still hazy on what that actually means.  I thought the CPU case was
>>> one of the big ones where you *did* want to act on the configuration
>>> changes?
>> By utilising the hardware support for switching between active and
>> sleep state (and having the cpu only affecting the active state) we
>> can do this asynchronously, while being in wfi.
> Hrm, right.  This is starting to make sense.  So what we're saying here
> is roughly that you want to use a separate suspend-like mode (the extra
> set of registers for the low power mode mode) but are entering it
> without actually suspending the whole system?

For the idle path, yes. We also want to do the same sort of thing on the
system suspend path.

>
>>>> In the case of us having other active consumers (e.g. GPU, display or some
>>>> peripherals) the voltage range or operating mode specified by the cpufreq
>>>> driver might be suboptimal when we remove the cpu.
>>> I don't understand.  Why is the other consumer asking for something it
>>> doesn't need itself?
>> I don't have the actual numbers, but we have a situation that looks like this:
>> gpu driver does regulator_set_voltage(.5V, 1V);
>> cpufreq driver does regulator_set_voltage(1V,1V);
>> The regulator framework will make the regulator tick at 1V
>> Upon going to sleep the cpuidle driver will, through the hardware
>> support, make the regulator tick at .5V. Before returning the cpu from
>> idle the hardware will make the regulator tick at 1V again.
> So the regulator framework should be fine if the CPU regulator
> constraint was disabled while the CPU consumer was disabled (modulo the
> performance issue)?

The CPU regulator consumer won't be disabled through any software
mechanism. It will stay enabled in software and when we execute wfi the
hardware will drop that consumers request because it was only requesting
in the active set.

>
>>> If you're hiding it in the driver make sure it's *hidden* in the driver
>>> - exporting duplicate regulators doesn't sound like hiding to me.
>> This is the problem at hand, we have not found any way to actually
>> hide this in the driver as the two modes depend on the consumer and
>> not the aggregated values we get out of the regulator framework.
>> Or rather, my proposal would do that, but that will not be able to
>> handle the case when there's different voltages in the two states. It
>> would only take care of the enable/disable case.
> So, I think something along the lines of suspend mode ought to be able
> to handle this.  What we need is a way of accelerating the entry and
> exit, right?
>
>> The exposure of multiple regulators moves the problem to the
>> devicetree, making sure to map the consumers to the right
>> state/regulator. But it should only be the cpu (in its various forms)
>> that ever consume the active only regulator.
> He said hopefully...  :)
>
> I think I now have a reasonable picture of what's going on but wanted to
> confirm that what I'm saying above makes sense.

That's good. Is there any conclusion here? I'm still thinking that
having multiple regulators for the same physical supply is the right
thing to do.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Thu 04 Dec 13:15 PST 2014, Stephen Boyd wrote:

> On 11/28/2014 12:16 PM, Mark Brown wrote:
> > On Thu, Nov 27, 2014 at 11:42:41AM -0800, Bjorn Andersson wrote:

[..]

> >> The exposure of multiple regulators moves the problem to the
> >> devicetree, making sure to map the consumers to the right
> >> state/regulator. But it should only be the cpu (in its various forms)
> >> that ever consume the active only regulator.
> > He said hopefully...  :)

:)

> >
> > I think I now have a reasonable picture of what's going on but wanted to
> > confirm that what I'm saying above makes sense.
> 
> That's good. Is there any conclusion here? I'm still thinking that
> having multiple regulators for the same physical supply is the right
> thing to do.
> 

As I said, the only other idea I've come up with will not cut it for the
regulators that need more than enable/disable support when going to sleep.

Even if we came up with some model of exposing something similar to the suspend
state, we would still need to expose it in a way that we can specify which
(active/sleep/both) state in the consumer. Hence in practice we end up with
exposing multiple regulators in one way or another.


I can't help thinking that this would be a problem with the static suspend
settings as well; i.e. what is the static suspend state for a regulator that
powers a WiFi chip? For me the answer would often be "it's enabled iff the WiFi
consumer asks for it" - but maybe it's not supposed to be used for "dynamic"
regulators.


Nontheless, we have reached conclusions regarding my RFC. So I'll move on and
finish up a multi-regulator implementation and we can continue the discussion
based on that.

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Thu 04 Dec 13:15 PST 2014, Stephen Boyd wrote:

> On 11/28/2014 12:16 PM, Mark Brown wrote:
> > On Thu, Nov 27, 2014 at 11:42:41AM -0800, Bjorn Andersson wrote:

[..]

> >> The exposure of multiple regulators moves the problem to the
> >> devicetree, making sure to map the consumers to the right
> >> state/regulator. But it should only be the cpu (in its various forms)
> >> that ever consume the active only regulator.
> > He said hopefully...  :)

:)

> >
> > I think I now have a reasonable picture of what's going on but wanted to
> > confirm that what I'm saying above makes sense.
> 
> That's good. Is there any conclusion here? I'm still thinking that
> having multiple regulators for the same physical supply is the right
> thing to do.
> 

As I said, the only other idea I've come up with will not cut it for the
regulators that need more than enable/disable support when going to sleep.

Even if we came up with some model of exposing something similar to the suspend
state, we would still need to expose it in a way that we can specify which
(active/sleep/both) state in the consumer. Hence in practice we end up with
exposing multiple regulators in one way or another.


I can't help thinking that this would be a problem with the static suspend
settings as well; i.e. what is the static suspend state for a regulator that
powers a WiFi chip? For me the answer would often be "it's enabled iff the WiFi
consumer asks for it" - but maybe it's not supposed to be used for "dynamic"
regulators.


Nontheless, we have reached conclusions regarding my RFC. So I'll move on and
finish up a multi-regulator implementation and we can continue the discussion
based on that.

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]

On Mon, Dec 08, 2014 at 10:06:36AM -0800, Bjorn Andersson wrote:
> On Thu 04 Dec 13:15 PST 2014, Stephen Boyd wrote:
> > On 11/28/2014 12:16 PM, Mark Brown wrote:

> > > I think I now have a reasonable picture of what's going on but wanted to
> > > confirm that what I'm saying above makes sense.

> > That's good. Is there any conclusion here? I'm still thinking that
> > having multiple regulators for the same physical supply is the right
> > thing to do.

I'm still not finding it easy to find this tasteful.

> As I said, the only other idea I've come up with will not cut it for the
> regulators that need more than enable/disable support when going to sleep.

> Even if we came up with some model of exposing something similar to the suspend
> state, we would still need to expose it in a way that we can specify which
> (active/sleep/both) state in the consumer. Hence in practice we end up with
> exposing multiple regulators in one way or another.

Now I'm confused again.  I thought entry and exit was all done
separately so it was just about saying what should happen if the device
were to idle?

> I can't help thinking that this would be a problem with the static suspend
> settings as well; i.e. what is the static suspend state for a regulator that
> powers a WiFi chip? For me the answer would often be "it's enabled iff the WiFi
> consumer asks for it" - but maybe it's not supposed to be used for "dynamic"
> regulators.

That's what should happen, we just don't currently really support doing
this configuration dynamically (practically speaking I suspect anyone
who cares just doesn't have a suspend mode for affected regulators at
the minute).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Mon, Dec 08, 2014 at 10:06:36AM -0800, Bjorn Andersson wrote:
> On Thu 04 Dec 13:15 PST 2014, Stephen Boyd wrote:
> > On 11/28/2014 12:16 PM, Mark Brown wrote:

> > > I think I now have a reasonable picture of what's going on but wanted to
> > > confirm that what I'm saying above makes sense.

> > That's good. Is there any conclusion here? I'm still thinking that
> > having multiple regulators for the same physical supply is the right
> > thing to do.

I'm still not finding it easy to find this tasteful.

> As I said, the only other idea I've come up with will not cut it for the
> regulators that need more than enable/disable support when going to sleep.

> Even if we came up with some model of exposing something similar to the suspend
> state, we would still need to expose it in a way that we can specify which
> (active/sleep/both) state in the consumer. Hence in practice we end up with
> exposing multiple regulators in one way or another.

Now I'm confused again.  I thought entry and exit was all done
separately so it was just about saying what should happen if the device
were to idle?

> I can't help thinking that this would be a problem with the static suspend
> settings as well; i.e. what is the static suspend state for a regulator that
> powers a WiFi chip? For me the answer would often be "it's enabled iff the WiFi
> consumer asks for it" - but maybe it's not supposed to be used for "dynamic"
> regulators.

That's what should happen, we just don't currently really support doing
this configuration dynamically (practically speaking I suspect anyone
who cares just doesn't have a suspend mode for affected regulators at
the minute).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141208/29eb47f8/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Mon 08 Dec 11:39 PST 2014, Mark Brown wrote:

> On Mon, Dec 08, 2014 at 10:06:36AM -0800, Bjorn Andersson wrote:
> > On Thu 04 Dec 13:15 PST 2014, Stephen Boyd wrote:
> > > On 11/28/2014 12:16 PM, Mark Brown wrote:
> 
> > > > I think I now have a reasonable picture of what's going on but wanted to
> > > > confirm that what I'm saying above makes sense.
> 
> > > That's good. Is there any conclusion here? I'm still thinking that
> > > having multiple regulators for the same physical supply is the right
> > > thing to do.
> 
> I'm still not finding it easy to find this tasteful.
> 
> > As I said, the only other idea I've come up with will not cut it for the
> > regulators that need more than enable/disable support when going to sleep.
> 
> > Even if we came up with some model of exposing something similar to the suspend
> > state, we would still need to expose it in a way that we can specify which
> > (active/sleep/both) state in the consumer. Hence in practice we end up with
> > exposing multiple regulators in one way or another.
> 
> Now I'm confused again.  I thought entry and exit was all done
> separately so it was just about saying what should happen if the device
> were to idle?
> 

But how do you actually expose that control to the consumers?

> > I can't help thinking that this would be a problem with the static suspend
> > settings as well; i.e. what is the static suspend state for a regulator that
> > powers a WiFi chip? For me the answer would often be "it's enabled iff the WiFi
> > consumer asks for it" - but maybe it's not supposed to be used for "dynamic"
> > regulators.
> 
> That's what should happen, we just don't currently really support doing
> this configuration dynamically (practically speaking I suspect anyone
> who cares just doesn't have a suspend mode for affected regulators at
> the minute).

What would such configuration look like?

In the suspend case you could introduce an api for setting parameters of the
suspend state, but that means that the individual drivers needs to be written
with awareness of the system state. Our proposal is to use the same api, but
with a different regulator*, but how should that acquired (and expressed in dt).


In our case, as most things change the both state (or active only if no
both/sleep), the standard regulator api would have to affect the both state.
Would we then have a separate api to modify the active state?

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Mon 08 Dec 11:39 PST 2014, Mark Brown wrote:

> On Mon, Dec 08, 2014 at 10:06:36AM -0800, Bjorn Andersson wrote:
> > On Thu 04 Dec 13:15 PST 2014, Stephen Boyd wrote:
> > > On 11/28/2014 12:16 PM, Mark Brown wrote:
> 
> > > > I think I now have a reasonable picture of what's going on but wanted to
> > > > confirm that what I'm saying above makes sense.
> 
> > > That's good. Is there any conclusion here? I'm still thinking that
> > > having multiple regulators for the same physical supply is the right
> > > thing to do.
> 
> I'm still not finding it easy to find this tasteful.
> 
> > As I said, the only other idea I've come up with will not cut it for the
> > regulators that need more than enable/disable support when going to sleep.
> 
> > Even if we came up with some model of exposing something similar to the suspend
> > state, we would still need to expose it in a way that we can specify which
> > (active/sleep/both) state in the consumer. Hence in practice we end up with
> > exposing multiple regulators in one way or another.
> 
> Now I'm confused again.  I thought entry and exit was all done
> separately so it was just about saying what should happen if the device
> were to idle?
> 

But how do you actually expose that control to the consumers?

> > I can't help thinking that this would be a problem with the static suspend
> > settings as well; i.e. what is the static suspend state for a regulator that
> > powers a WiFi chip? For me the answer would often be "it's enabled iff the WiFi
> > consumer asks for it" - but maybe it's not supposed to be used for "dynamic"
> > regulators.
> 
> That's what should happen, we just don't currently really support doing
> this configuration dynamically (practically speaking I suspect anyone
> who cares just doesn't have a suspend mode for affected regulators at
> the minute).

What would such configuration look like?

In the suspend case you could introduce an api for setting parameters of the
suspend state, but that means that the individual drivers needs to be written
with awareness of the system state. Our proposal is to use the same api, but
with a different regulator*, but how should that acquired (and expressed in dt).


In our case, as most things change the both state (or active only if no
both/sleep), the standard regulator api would have to affect the both state.
Would we then have a separate api to modify the active state?

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

On Mon, Dec 08, 2014 at 12:55:03PM -0800, Bjorn Andersson wrote:
> On Mon 08 Dec 11:39 PST 2014, Mark Brown wrote:

> > Now I'm confused again.  I thought entry and exit was all done
> > separately so it was just about saying what should happen if the device
> > were to idle?

> But how do you actually expose that control to the consumers?

I think having an idle state (or possibly just using the suspend state,
I'm still foggy if the two states are actually different) seems to make
sense?

> > That's what should happen, we just don't currently really support doing
> > this configuration dynamically (practically speaking I suspect anyone
> > who cares just doesn't have a suspend mode for affected regulators at
> > the minute).

> What would such configuration look like?

> In the suspend case you could introduce an api for setting parameters of the
> suspend state, but that means that the individual drivers needs to be written
> with awareness of the system state. Our proposal is to use the same api, but
> with a different regulator*, but how should that acquired (and expressed in dt).

We already have an API for the suspend state, we just don't enable its
use in DT yet other than static configuration.  I'm not sure I see the
connection to the system state in the consumer API here - I thought this
was pretty much just setting the state when the device is turned off
which seems device specific?

> In our case, as most things change the both state (or active only if no
> both/sleep), the standard regulator api would have to affect the both state.
> Would we then have a separate api to modify the active state?

Why would we need to introduce a new API for the active state?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Mon, Dec 08, 2014 at 12:55:03PM -0800, Bjorn Andersson wrote:
> On Mon 08 Dec 11:39 PST 2014, Mark Brown wrote:

> > Now I'm confused again.  I thought entry and exit was all done
> > separately so it was just about saying what should happen if the device
> > were to idle?

> But how do you actually expose that control to the consumers?

I think having an idle state (or possibly just using the suspend state,
I'm still foggy if the two states are actually different) seems to make
sense?

> > That's what should happen, we just don't currently really support doing
> > this configuration dynamically (practically speaking I suspect anyone
> > who cares just doesn't have a suspend mode for affected regulators at
> > the minute).

> What would such configuration look like?

> In the suspend case you could introduce an api for setting parameters of the
> suspend state, but that means that the individual drivers needs to be written
> with awareness of the system state. Our proposal is to use the same api, but
> with a different regulator*, but how should that acquired (and expressed in dt).

We already have an API for the suspend state, we just don't enable its
use in DT yet other than static configuration.  I'm not sure I see the
connection to the system state in the consumer API here - I thought this
was pretty much just setting the state when the device is turned off
which seems device specific?

> In our case, as most things change the both state (or active only if no
> both/sleep), the standard regulator api would have to affect the both state.
> Would we then have a separate api to modify the active state?

Why would we need to introduce a new API for the active state?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141209/4bc5766f/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Tue 09 Dec 10:16 PST 2014, Mark Brown wrote:

> On Mon, Dec 08, 2014 at 12:55:03PM -0800, Bjorn Andersson wrote:
> > On Mon 08 Dec 11:39 PST 2014, Mark Brown wrote:
> 
> > > Now I'm confused again.  I thought entry and exit was all done
> > > separately so it was just about saying what should happen if the device
> > > were to idle?
> 
> > But how do you actually expose that control to the consumers?
> 
> I think having an idle state (or possibly just using the suspend state,
> I'm still foggy if the two states are actually different) seems to make
> sense?
> 

In 8974 we have LDO12 that have the following consumers:
 csi
 dsi
 hdmi
 edp
 modem pll
 pronto pll (wifi/bt)
 krait-clock

Each of these drivers will have a regulator object, upon which they can make
requests on.

The drivers for csi to pronto does not care about the state of the cpu - we
want to output pixels even if the cpu is idle - so their requests (on their
regulator *) must be aggregated towards the active state as well as the idle
state.

The krait-clock should not affect the idle state.


So the idle state enable property should be aggregated from csi to pronto
enable properties. I.e. if they are all off, then the idle state should be off,
otherwise it's on.


The switch-mode regulators also needs to aggregate voltage and mode - from a
subset of the regulator objects/consumers.

> > > That's what should happen, we just don't currently really support doing
> > > this configuration dynamically (practically speaking I suspect anyone
> > > who cares just doesn't have a suspend mode for affected regulators at
> > > the minute).
> 
> > What would such configuration look like?
> 
> > In the suspend case you could introduce an api for setting parameters of the
> > suspend state, but that means that the individual drivers needs to be written
> > with awareness of the system state. Our proposal is to use the same api, but
> > with a different regulator*, but how should that acquired (and expressed in dt).
> 
> We already have an API for the suspend state, we just don't enable its
> use in DT yet other than static configuration.  I'm not sure I see the
> connection to the system state in the consumer API here - I thought this
> was pretty much just setting the state when the device is turned off
> which seems device specific?
> 

The suspend state (with the additional exposure in DT) allows us to specify a
static configuration of the suspend state of a regulator. Here we see a need
for the properties of that state to be aggregated from a subset of the
regulator objects.

> > In our case, as most things change the both state (or active only if no
> > both/sleep), the standard regulator api would have to affect the both state.
> > Would we then have a separate api to modify the active state?
> 
> Why would we need to introduce a new API for the active state?

It's just that I, possibly incorrectly, considered that to be the outlier and
the one that would be the easiest to model separately.

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Tue 09 Dec 10:16 PST 2014, Mark Brown wrote:

> On Mon, Dec 08, 2014 at 12:55:03PM -0800, Bjorn Andersson wrote:
> > On Mon 08 Dec 11:39 PST 2014, Mark Brown wrote:
> 
> > > Now I'm confused again.  I thought entry and exit was all done
> > > separately so it was just about saying what should happen if the device
> > > were to idle?
> 
> > But how do you actually expose that control to the consumers?
> 
> I think having an idle state (or possibly just using the suspend state,
> I'm still foggy if the two states are actually different) seems to make
> sense?
> 

In 8974 we have LDO12 that have the following consumers:
 csi
 dsi
 hdmi
 edp
 modem pll
 pronto pll (wifi/bt)
 krait-clock

Each of these drivers will have a regulator object, upon which they can make
requests on.

The drivers for csi to pronto does not care about the state of the cpu - we
want to output pixels even if the cpu is idle - so their requests (on their
regulator *) must be aggregated towards the active state as well as the idle
state.

The krait-clock should not affect the idle state.


So the idle state enable property should be aggregated from csi to pronto
enable properties. I.e. if they are all off, then the idle state should be off,
otherwise it's on.


The switch-mode regulators also needs to aggregate voltage and mode - from a
subset of the regulator objects/consumers.

> > > That's what should happen, we just don't currently really support doing
> > > this configuration dynamically (practically speaking I suspect anyone
> > > who cares just doesn't have a suspend mode for affected regulators at
> > > the minute).
> 
> > What would such configuration look like?
> 
> > In the suspend case you could introduce an api for setting parameters of the
> > suspend state, but that means that the individual drivers needs to be written
> > with awareness of the system state. Our proposal is to use the same api, but
> > with a different regulator*, but how should that acquired (and expressed in dt).
> 
> We already have an API for the suspend state, we just don't enable its
> use in DT yet other than static configuration.  I'm not sure I see the
> connection to the system state in the consumer API here - I thought this
> was pretty much just setting the state when the device is turned off
> which seems device specific?
> 

The suspend state (with the additional exposure in DT) allows us to specify a
static configuration of the suspend state of a regulator. Here we see a need
for the properties of that state to be aggregated from a subset of the
regulator objects.

> > In our case, as most things change the both state (or active only if no
> > both/sleep), the standard regulator api would have to affect the both state.
> > Would we then have a separate api to modify the active state?
> 
> Why would we need to introduce a new API for the active state?

It's just that I, possibly incorrectly, considered that to be the outlier and
the one that would be the easiest to model separately.

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4092 bytes --]

On Tue, Dec 09, 2014 at 11:25:18AM -0800, Bjorn Andersson wrote:
> On Tue 09 Dec 10:16 PST 2014, Mark Brown wrote:

> > I think having an idle state (or possibly just using the suspend state,
> > I'm still foggy if the two states are actually different) seems to make
> > sense?

> In 8974 we have LDO12 that have the following consumers:
>  csi
>  dsi
>  hdmi
>  edp
>  modem pll
>  pronto pll (wifi/bt)
>  krait-clock

> Each of these drivers will have a regulator object, upon which they can make
> requests on.

> The drivers for csi to pronto does not care about the state of the cpu - we
> want to output pixels even if the cpu is idle - so their requests (on their
> regulator *) must be aggregated towards the active state as well as the idle
> state.

Are they actually twiddling anything except enable, and I'm not sure
that this answers my question about suspend state either?

> The krait-clock should not affect the idle state.

> So the idle state enable property should be aggregated from csi to pronto
> enable properties. I.e. if they are all off, then the idle state should be off,
> otherwise it's on.

OK, so this is pointing back to the idea that we just want a way to
knock out this one consumer from the constraints when it's going idle
(you mention later on that other regulators need parameters other than
the enable changing).  Now, I seem to recall that the reason that this
is a problem is that you want to use this state changing stuff to
accelerate that transition.  This seems like something other regulators
can do which we don't really take advantage of yet except in that some
regualtors remember previous settings for voltage and can switch to them
quickly.

What this is *now* sounding like is that we need a way to say that this
one consumer is magic and we want to essentially tell the hardware about
settings with and without that consumer then the hardware will magically
switch between the two when the consumer is enabled and disabled.  Does
that sound about right?

> The switch-mode regulators also needs to aggregate voltage and mode - from a
> subset of the regulator objects/consumers.

We don't have any support at all for aggregating mode except via current
estimation which is unused and I'd be surprised if that works
usefully...

> > We already have an API for the suspend state, we just don't enable its
> > use in DT yet other than static configuration.  I'm not sure I see the
> > connection to the system state in the consumer API here - I thought this
> > was pretty much just setting the state when the device is turned off
> > which seems device specific?

> The suspend state (with the additional exposure in DT) allows us to specify a
> static configuration of the suspend state of a regulator. Here we see a need
> for the properties of that state to be aggregated from a subset of the
> regulator objects.

We already have a runtime API for specifying suspend state.  It doesn't
currently aggregate but that's a simple matter of programming.

> > > In our case, as most things change the both state (or active only if no
> > > both/sleep), the standard regulator api would have to affect the both state.
> > > Would we then have a separate api to modify the active state?

> > Why would we need to introduce a new API for the active state?

> It's just that I, possibly incorrectly, considered that to be the outlier and
> the one that would be the easiest to model separately.

I'm sorry, I'm just not getting this at all.

Can I please ask that people stop giving partial pictures of what's
going on?  There's a few reasons why this is all so hard to follow - one
of them is that the story is coming out in dribs and drabs so every time
I feel I understand what everyone is talking about everything shifts.

I really do fear that the bodge you're using at the minute with multiple
regulators has poor abstraction and is hence too fragile - it seems like
there's too much knowledge of the system spread around different drivers
and it's all vulnerable to changes in system integration which could
potentially be made per board.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Tue, Dec 09, 2014 at 11:25:18AM -0800, Bjorn Andersson wrote:
> On Tue 09 Dec 10:16 PST 2014, Mark Brown wrote:

> > I think having an idle state (or possibly just using the suspend state,
> > I'm still foggy if the two states are actually different) seems to make
> > sense?

> In 8974 we have LDO12 that have the following consumers:
>  csi
>  dsi
>  hdmi
>  edp
>  modem pll
>  pronto pll (wifi/bt)
>  krait-clock

> Each of these drivers will have a regulator object, upon which they can make
> requests on.

> The drivers for csi to pronto does not care about the state of the cpu - we
> want to output pixels even if the cpu is idle - so their requests (on their
> regulator *) must be aggregated towards the active state as well as the idle
> state.

Are they actually twiddling anything except enable, and I'm not sure
that this answers my question about suspend state either?

> The krait-clock should not affect the idle state.

> So the idle state enable property should be aggregated from csi to pronto
> enable properties. I.e. if they are all off, then the idle state should be off,
> otherwise it's on.

OK, so this is pointing back to the idea that we just want a way to
knock out this one consumer from the constraints when it's going idle
(you mention later on that other regulators need parameters other than
the enable changing).  Now, I seem to recall that the reason that this
is a problem is that you want to use this state changing stuff to
accelerate that transition.  This seems like something other regulators
can do which we don't really take advantage of yet except in that some
regualtors remember previous settings for voltage and can switch to them
quickly.

What this is *now* sounding like is that we need a way to say that this
one consumer is magic and we want to essentially tell the hardware about
settings with and without that consumer then the hardware will magically
switch between the two when the consumer is enabled and disabled.  Does
that sound about right?

> The switch-mode regulators also needs to aggregate voltage and mode - from a
> subset of the regulator objects/consumers.

We don't have any support at all for aggregating mode except via current
estimation which is unused and I'd be surprised if that works
usefully...

> > We already have an API for the suspend state, we just don't enable its
> > use in DT yet other than static configuration.  I'm not sure I see the
> > connection to the system state in the consumer API here - I thought this
> > was pretty much just setting the state when the device is turned off
> > which seems device specific?

> The suspend state (with the additional exposure in DT) allows us to specify a
> static configuration of the suspend state of a regulator. Here we see a need
> for the properties of that state to be aggregated from a subset of the
> regulator objects.

We already have a runtime API for specifying suspend state.  It doesn't
currently aggregate but that's a simple matter of programming.

> > > In our case, as most things change the both state (or active only if no
> > > both/sleep), the standard regulator api would have to affect the both state.
> > > Would we then have a separate api to modify the active state?

> > Why would we need to introduce a new API for the active state?

> It's just that I, possibly incorrectly, considered that to be the outlier and
> the one that would be the easiest to model separately.

I'm sorry, I'm just not getting this at all.

Can I please ask that people stop giving partial pictures of what's
going on?  There's a few reasons why this is all so hard to follow - one
of them is that the story is coming out in dribs and drabs so every time
I feel I understand what everyone is talking about everything shifts.

I really do fear that the bodge you're using at the minute with multiple
regulators has poor abstraction and is hence too fragile - it seems like
there's too much knowledge of the system spread around different drivers
and it's all vulnerable to changes in system integration which could
potentially be made per board.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141209/498255d9/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Tue 09 Dec 12:28 PST 2014, Mark Brown wrote:

> On Tue, Dec 09, 2014 at 11:25:18AM -0800, Bjorn Andersson wrote:
> > On Tue 09 Dec 10:16 PST 2014, Mark Brown wrote:
> 
> > > I think having an idle state (or possibly just using the suspend state,
> > > I'm still foggy if the two states are actually different) seems to make
> > > sense?
> 
> > In 8974 we have LDO12 that have the following consumers:
> >  csi
> >  dsi
> >  hdmi
> >  edp
> >  modem pll
> >  pronto pll (wifi/bt)
> >  krait-clock
> 
> > Each of these drivers will have a regulator object, upon which they can make
> > requests on.
> 
> > The drivers for csi to pronto does not care about the state of the cpu - we
> > want to output pixels even if the cpu is idle - so their requests (on their
> > regulator *) must be aggregated towards the active state as well as the idle
> > state.
> 
> Are they actually twiddling anything except enable, and I'm not sure
> that this answers my question about suspend state either?
> 

Yes, for the switch-mode regulators there's different enable state, voltage and
mode.

> > The krait-clock should not affect the idle state.
> 
> > So the idle state enable property should be aggregated from csi to pronto
> > enable properties. I.e. if they are all off, then the idle state should be off,
> > otherwise it's on.
> 
> OK, so this is pointing back to the idea that we just want a way to
> knock out this one consumer from the constraints when it's going idle
> (you mention later on that other regulators need parameters other than
> the enable changing).  Now, I seem to recall that the reason that this
> is a problem is that you want to use this state changing stuff to
> accelerate that transition.  This seems like something other regulators
> can do which we don't really take advantage of yet except in that some
> regualtors remember previous settings for voltage and can switch to them
> quickly.
> 

Correct, the problem here is twofold in my eyes;

At the point then the consumer decides that it does not need the regulator
it's in atomic context. So the regulator core and driver would need to handle
this. Further more the regulator driver uses a packet-based protocol over a
shared memory channel, so this entire path would have to be invoked in atomic
context.

We have to put the consumer into a state where we can "release" the regulator,
then we need to tell the RPM to update the state of the regulator and we have
to wait for the ack to come back before we can actually go idle.

> What this is *now* sounding like is that we need a way to say that this
> one consumer is magic and we want to essentially tell the hardware about
> settings with and without that consumer then the hardware will magically
> switch between the two when the consumer is enabled and disabled.  Does
> that sound about right?
> 

I think it sounds spot on.

> > The switch-mode regulators also needs to aggregate voltage and mode - from a
> > subset of the regulator objects/consumers.
> 
> We don't have any support at all for aggregating mode except via current
> estimation which is unused and I'd be surprised if that works
> usefully...
> 

Let's treat that as a separate topic.

> > > We already have an API for the suspend state, we just don't enable its
> > > use in DT yet other than static configuration.  I'm not sure I see the
> > > connection to the system state in the consumer API here - I thought this
> > > was pretty much just setting the state when the device is turned off
> > > which seems device specific?
> 
> > The suspend state (with the additional exposure in DT) allows us to specify a
> > static configuration of the suspend state of a regulator. Here we see a need
> > for the properties of that state to be aggregated from a subset of the
> > regulator objects.
> 
> We already have a runtime API for specifying suspend state.  It doesn't
> currently aggregate but that's a simple matter of programming.
> 

Do you mean alter properties of a state or select between the predefined
states? I've found some apis related to the latter, but not the first.

> > > > In our case, as most things change the both state (or active only if no
> > > > both/sleep), the standard regulator api would have to affect the both state.
> > > > Would we then have a separate api to modify the active state?
> 
> > > Why would we need to introduce a new API for the active state?
> 
> > It's just that I, possibly incorrectly, considered that to be the outlier and
> > the one that would be the easiest to model separately.
> 
> I'm sorry, I'm just not getting this at all.
> 

Because for the overall system the active state is the outlier here. But it
probably doesn't matter for our conclusions.

> Can I please ask that people stop giving partial pictures of what's
> going on?  There's a few reasons why this is all so hard to follow - one
> of them is that the story is coming out in dribs and drabs so every time
> I feel I understand what everyone is talking about everything shifts.
> 

Sorry, I've tried to describe the hardware setup but have probably not made a
clear enough distinction towards the downstream implementation and solution
suggestions.

> I really do fear that the bodge you're using at the minute with multiple
> regulators has poor abstraction and is hence too fragile - it seems like
> there's too much knowledge of the system spread around different drivers
> and it's all vulnerable to changes in system integration which could
> potentially be made per board.

Just to make sure we're on the same page here, the way this is expressed in
downstream is like the following:

ldo12 {
	l12: l12 {
		set-both-states;

		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <1800000>;
	};

	l12_active: l12-active {
		set-only-active-state;

		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <1800000>;
	};
};

dsi {
	vddio-supply = <&l12>;
};

clock-krait {
	hfpll-analog-supply = <&l12_active>;
};

All the consumers uses the standard regulator accessor functions to operate on
their respective regulator *. Further more it's only the clock driver for the
core (krait) that references the active only regulators.

So the knowledge spread out would be contained to the dt bindings things
together.

I can however not argue against you on the fact that the way this is exposed is
suboptimal. The question is if we can figure a better way to express it - note
that we do not want e.g. the dsi driver to know anything about the fact that
ldo12 is special.



What I don't like with that solution is that we above have two different rdev
and that we need to aggregate state between the various rdevs in the ldo12
grouping. (so that active only and both actually affect the active state).

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Tue 09 Dec 12:28 PST 2014, Mark Brown wrote:

> On Tue, Dec 09, 2014 at 11:25:18AM -0800, Bjorn Andersson wrote:
> > On Tue 09 Dec 10:16 PST 2014, Mark Brown wrote:
> 
> > > I think having an idle state (or possibly just using the suspend state,
> > > I'm still foggy if the two states are actually different) seems to make
> > > sense?
> 
> > In 8974 we have LDO12 that have the following consumers:
> >  csi
> >  dsi
> >  hdmi
> >  edp
> >  modem pll
> >  pronto pll (wifi/bt)
> >  krait-clock
> 
> > Each of these drivers will have a regulator object, upon which they can make
> > requests on.
> 
> > The drivers for csi to pronto does not care about the state of the cpu - we
> > want to output pixels even if the cpu is idle - so their requests (on their
> > regulator *) must be aggregated towards the active state as well as the idle
> > state.
> 
> Are they actually twiddling anything except enable, and I'm not sure
> that this answers my question about suspend state either?
> 

Yes, for the switch-mode regulators there's different enable state, voltage and
mode.

> > The krait-clock should not affect the idle state.
> 
> > So the idle state enable property should be aggregated from csi to pronto
> > enable properties. I.e. if they are all off, then the idle state should be off,
> > otherwise it's on.
> 
> OK, so this is pointing back to the idea that we just want a way to
> knock out this one consumer from the constraints when it's going idle
> (you mention later on that other regulators need parameters other than
> the enable changing).  Now, I seem to recall that the reason that this
> is a problem is that you want to use this state changing stuff to
> accelerate that transition.  This seems like something other regulators
> can do which we don't really take advantage of yet except in that some
> regualtors remember previous settings for voltage and can switch to them
> quickly.
> 

Correct, the problem here is twofold in my eyes;

At the point then the consumer decides that it does not need the regulator
it's in atomic context. So the regulator core and driver would need to handle
this. Further more the regulator driver uses a packet-based protocol over a
shared memory channel, so this entire path would have to be invoked in atomic
context.

We have to put the consumer into a state where we can "release" the regulator,
then we need to tell the RPM to update the state of the regulator and we have
to wait for the ack to come back before we can actually go idle.

> What this is *now* sounding like is that we need a way to say that this
> one consumer is magic and we want to essentially tell the hardware about
> settings with and without that consumer then the hardware will magically
> switch between the two when the consumer is enabled and disabled.  Does
> that sound about right?
> 

I think it sounds spot on.

> > The switch-mode regulators also needs to aggregate voltage and mode - from a
> > subset of the regulator objects/consumers.
> 
> We don't have any support at all for aggregating mode except via current
> estimation which is unused and I'd be surprised if that works
> usefully...
> 

Let's treat that as a separate topic.

> > > We already have an API for the suspend state, we just don't enable its
> > > use in DT yet other than static configuration.  I'm not sure I see the
> > > connection to the system state in the consumer API here - I thought this
> > > was pretty much just setting the state when the device is turned off
> > > which seems device specific?
> 
> > The suspend state (with the additional exposure in DT) allows us to specify a
> > static configuration of the suspend state of a regulator. Here we see a need
> > for the properties of that state to be aggregated from a subset of the
> > regulator objects.
> 
> We already have a runtime API for specifying suspend state.  It doesn't
> currently aggregate but that's a simple matter of programming.
> 

Do you mean alter properties of a state or select between the predefined
states? I've found some apis related to the latter, but not the first.

> > > > In our case, as most things change the both state (or active only if no
> > > > both/sleep), the standard regulator api would have to affect the both state.
> > > > Would we then have a separate api to modify the active state?
> 
> > > Why would we need to introduce a new API for the active state?
> 
> > It's just that I, possibly incorrectly, considered that to be the outlier and
> > the one that would be the easiest to model separately.
> 
> I'm sorry, I'm just not getting this at all.
> 

Because for the overall system the active state is the outlier here. But it
probably doesn't matter for our conclusions.

> Can I please ask that people stop giving partial pictures of what's
> going on?  There's a few reasons why this is all so hard to follow - one
> of them is that the story is coming out in dribs and drabs so every time
> I feel I understand what everyone is talking about everything shifts.
> 

Sorry, I've tried to describe the hardware setup but have probably not made a
clear enough distinction towards the downstream implementation and solution
suggestions.

> I really do fear that the bodge you're using at the minute with multiple
> regulators has poor abstraction and is hence too fragile - it seems like
> there's too much knowledge of the system spread around different drivers
> and it's all vulnerable to changes in system integration which could
> potentially be made per board.

Just to make sure we're on the same page here, the way this is expressed in
downstream is like the following:

ldo12 {
	l12: l12 {
		set-both-states;

		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <1800000>;
	};

	l12_active: l12-active {
		set-only-active-state;

		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <1800000>;
	};
};

dsi {
	vddio-supply = <&l12>;
};

clock-krait {
	hfpll-analog-supply = <&l12_active>;
};

All the consumers uses the standard regulator accessor functions to operate on
their respective regulator *. Further more it's only the clock driver for the
core (krait) that references the active only regulators.

So the knowledge spread out would be contained to the dt bindings things
together.

I can however not argue against you on the fact that the way this is exposed is
suboptimal. The question is if we can figure a better way to express it - note
that we do not want e.g. the dsi driver to know anything about the fact that
ldo12 is special.



What I don't like with that solution is that we above have two different rdev
and that we need to aggregate state between the various rdevs in the ldo12
grouping. (so that active only and both actually affect the active state).

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3737 bytes --]

On Thu, Dec 11, 2014 at 02:36:54PM -0800, Bjorn Andersson wrote:

> At the point then the consumer decides that it does not need the regulator
> it's in atomic context. So the regulator core and driver would need to handle
> this. Further more the regulator driver uses a packet-based protocol over a
> shared memory channel, so this entire path would have to be invoked in atomic
> context.

> We have to put the consumer into a state where we can "release" the regulator,
> then we need to tell the RPM to update the state of the regulator and we have
> to wait for the ack to come back before we can actually go idle.

None of which sounds like such a big deal, a bigger problem seemed to be
trying to suppress intermediate updates of the device state that were
being done though that can comfortably be done in the driver I think.

> > What this is *now* sounding like is that we need a way to say that this
> > one consumer is magic and we want to essentially tell the hardware about
> > settings with and without that consumer then the hardware will magically
> > switch between the two when the consumer is enabled and disabled.  Does
> > that sound about right?

> I think it sounds spot on.


> > We already have a runtime API for specifying suspend state.  It doesn't
> > currently aggregate but that's a simple matter of programming.

> Do you mean alter properties of a state or select between the predefined
> states? I've found some apis related to the latter, but not the first.

Hrm, indeed.  Well, duplicating the existing APIs seems like a good way
forwards - the logic is all the same, it's just that we don't want to
apply the changes to the state used at runtime.

> > > > > both/sleep), the standard regulator api would have to affect the both state.
> > > > > Would we then have a separate api to modify the active state?

> > > > Why would we need to introduce a new API for the active state?

> > > It's just that I, possibly incorrectly, considered that to be the outlier and
> > > the one that would be the easiest to model separately.

> > I'm sorry, I'm just not getting this at all.

> Because for the overall system the active state is the outlier here. But it
> probably doesn't matter for our conclusions.

I'd argue that for the purpose of running Linux it's the common state...

> > I really do fear that the bodge you're using at the minute with multiple
> > regulators has poor abstraction and is hence too fragile - it seems like
> > there's too much knowledge of the system spread around different drivers
> > and it's all vulnerable to changes in system integration which could
> > potentially be made per board.

> Just to make sure we're on the same page here, the way this is expressed in
> downstream is like the following:
> 
> ldo12 {
> 	l12: l12 {

> 	l12_active: l12-active {

> All the consumers uses the standard regulator accessor functions to operate on
> their respective regulator *. Further more it's only the clock driver for the
> core (krait) that references the active only regulators.

> So the knowledge spread out would be contained to the dt bindings things
> together.

It's not just the DT binding, it's also the regulator driver that needs
to do the aggregation that's being discussed and is going to assume a
particular arrangement of clients.

> What I don't like with that solution is that we above have two different rdev
> and that we need to aggregate state between the various rdevs in the ldo12
> grouping. (so that active only and both actually affect the active state).

Yes, that's a big part of the problem - there's things going on that the
core should know about but doesn't.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Thu, Dec 11, 2014 at 02:36:54PM -0800, Bjorn Andersson wrote:

> At the point then the consumer decides that it does not need the regulator
> it's in atomic context. So the regulator core and driver would need to handle
> this. Further more the regulator driver uses a packet-based protocol over a
> shared memory channel, so this entire path would have to be invoked in atomic
> context.

> We have to put the consumer into a state where we can "release" the regulator,
> then we need to tell the RPM to update the state of the regulator and we have
> to wait for the ack to come back before we can actually go idle.

None of which sounds like such a big deal, a bigger problem seemed to be
trying to suppress intermediate updates of the device state that were
being done though that can comfortably be done in the driver I think.

> > What this is *now* sounding like is that we need a way to say that this
> > one consumer is magic and we want to essentially tell the hardware about
> > settings with and without that consumer then the hardware will magically
> > switch between the two when the consumer is enabled and disabled.  Does
> > that sound about right?

> I think it sounds spot on.


> > We already have a runtime API for specifying suspend state.  It doesn't
> > currently aggregate but that's a simple matter of programming.

> Do you mean alter properties of a state or select between the predefined
> states? I've found some apis related to the latter, but not the first.

Hrm, indeed.  Well, duplicating the existing APIs seems like a good way
forwards - the logic is all the same, it's just that we don't want to
apply the changes to the state used at runtime.

> > > > > both/sleep), the standard regulator api would have to affect the both state.
> > > > > Would we then have a separate api to modify the active state?

> > > > Why would we need to introduce a new API for the active state?

> > > It's just that I, possibly incorrectly, considered that to be the outlier and
> > > the one that would be the easiest to model separately.

> > I'm sorry, I'm just not getting this at all.

> Because for the overall system the active state is the outlier here. But it
> probably doesn't matter for our conclusions.

I'd argue that for the purpose of running Linux it's the common state...

> > I really do fear that the bodge you're using at the minute with multiple
> > regulators has poor abstraction and is hence too fragile - it seems like
> > there's too much knowledge of the system spread around different drivers
> > and it's all vulnerable to changes in system integration which could
> > potentially be made per board.

> Just to make sure we're on the same page here, the way this is expressed in
> downstream is like the following:
> 
> ldo12 {
> 	l12: l12 {

> 	l12_active: l12-active {

> All the consumers uses the standard regulator accessor functions to operate on
> their respective regulator *. Further more it's only the clock driver for the
> core (krait) that references the active only regulators.

> So the knowledge spread out would be contained to the dt bindings things
> together.

It's not just the DT binding, it's also the regulator driver that needs
to do the aggregation that's being discussed and is going to assume a
particular arrangement of clients.

> What I don't like with that solution is that we above have two different rdev
> and that we need to aggregate state between the various rdevs in the ldo12
> grouping. (so that active only and both actually affect the active state).

Yes, that's a big part of the problem - there's things going on that the
core should know about but doesn't.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141215/23b963f3/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Mon, Dec 15, 2014 at 10:04 AM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Dec 11, 2014 at 02:36:54PM -0800, Bjorn Andersson wrote:
>
[..]
>> We have to put the consumer into a state where we can "release" the regulator,
>> then we need to tell the RPM to update the state of the regulator and we have
>> to wait for the ack to come back before we can actually go idle.
>
> None of which sounds like such a big deal, a bigger problem seemed to be
> trying to suppress intermediate updates of the device state that were
> being done though that can comfortably be done in the driver I think.
>

As I said previously, suppressing unnecessary updates is an
optimization - although an important one. The key question is how we
model the multiple states.

[..]

>> > We already have a runtime API for specifying suspend state.  It doesn't
>> > currently aggregate but that's a simple matter of programming.
>
>> Do you mean alter properties of a state or select between the predefined
>> states? I've found some apis related to the latter, but not the first.
>
> Hrm, indeed.  Well, duplicating the existing APIs seems like a good way
> forwards - the logic is all the same, it's just that we don't want to
> apply the changes to the state used at runtime.
>

Are you suggesting that we introduce a shadow of the current API for
the purpose of affecting a certain state? Can you elaborate on how
that would look like and work?

>> > > > > both/sleep), the standard regulator api would have to affect the both state.
>> > > > > Would we then have a separate api to modify the active state?
>
>> > > > Why would we need to introduce a new API for the active state?
>
>> > > It's just that I, possibly incorrectly, considered that to be the outlier and
>> > > the one that would be the easiest to model separately.
>
>> > I'm sorry, I'm just not getting this at all.
>
>> Because for the overall system the active state is the outlier here. But it
>> probably doesn't matter for our conclusions.
>
> I'd argue that for the purpose of running Linux it's the common state...
>

Does "running Linux" indicate that there's instructions flowing
through the CPU pipeline or that the hardware overall is up and
running?

>> > I really do fear that the bodge you're using at the minute with multiple
>> > regulators has poor abstraction and is hence too fragile - it seems like
>> > there's too much knowledge of the system spread around different drivers
>> > and it's all vulnerable to changes in system integration which could
>> > potentially be made per board.
>
>> Just to make sure we're on the same page here, the way this is expressed in
>> downstream is like the following:
>>
>> ldo12 {
>>       l12: l12 {
>
>>       l12_active: l12-active {
>
>> All the consumers uses the standard regulator accessor functions to operate on
>> their respective regulator *. Further more it's only the clock driver for the
>> core (krait) that references the active only regulators.
>
>> So the knowledge spread out would be contained to the dt bindings things
>> together.
>
> It's not just the DT binding, it's also the regulator driver that needs
> to do the aggregation that's being discussed and is going to assume a
> particular arrangement of clients.
>

The downstream implementation sports 3 rdevs per regulator and their
requests are aggregated into the two states. So the regulator
implementation are not aware of the individual clients, it just
aggregates the 3 rdev states and programs the hardware accordingly.

>> What I don't like with that solution is that we above have two different rdev
>> and that we need to aggregate state between the various rdevs in the ldo12
>> grouping. (so that active only and both actually affect the active state).
>
> Yes, that's a big part of the problem - there's things going on that the
> core should know about but doesn't.

The way that the aggregation of these properties works there's no
problems doing it in stages. But it comes with code duplication and
the need of the various rdevs to share common state.

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Mon, Dec 15, 2014 at 10:04 AM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Dec 11, 2014 at 02:36:54PM -0800, Bjorn Andersson wrote:
>
[..]
>> We have to put the consumer into a state where we can "release" the regulator,
>> then we need to tell the RPM to update the state of the regulator and we have
>> to wait for the ack to come back before we can actually go idle.
>
> None of which sounds like such a big deal, a bigger problem seemed to be
> trying to suppress intermediate updates of the device state that were
> being done though that can comfortably be done in the driver I think.
>

As I said previously, suppressing unnecessary updates is an
optimization - although an important one. The key question is how we
model the multiple states.

[..]

>> > We already have a runtime API for specifying suspend state.  It doesn't
>> > currently aggregate but that's a simple matter of programming.
>
>> Do you mean alter properties of a state or select between the predefined
>> states? I've found some apis related to the latter, but not the first.
>
> Hrm, indeed.  Well, duplicating the existing APIs seems like a good way
> forwards - the logic is all the same, it's just that we don't want to
> apply the changes to the state used at runtime.
>

Are you suggesting that we introduce a shadow of the current API for
the purpose of affecting a certain state? Can you elaborate on how
that would look like and work?

>> > > > > both/sleep), the standard regulator api would have to affect the both state.
>> > > > > Would we then have a separate api to modify the active state?
>
>> > > > Why would we need to introduce a new API for the active state?
>
>> > > It's just that I, possibly incorrectly, considered that to be the outlier and
>> > > the one that would be the easiest to model separately.
>
>> > I'm sorry, I'm just not getting this at all.
>
>> Because for the overall system the active state is the outlier here. But it
>> probably doesn't matter for our conclusions.
>
> I'd argue that for the purpose of running Linux it's the common state...
>

Does "running Linux" indicate that there's instructions flowing
through the CPU pipeline or that the hardware overall is up and
running?

>> > I really do fear that the bodge you're using at the minute with multiple
>> > regulators has poor abstraction and is hence too fragile - it seems like
>> > there's too much knowledge of the system spread around different drivers
>> > and it's all vulnerable to changes in system integration which could
>> > potentially be made per board.
>
>> Just to make sure we're on the same page here, the way this is expressed in
>> downstream is like the following:
>>
>> ldo12 {
>>       l12: l12 {
>
>>       l12_active: l12-active {
>
>> All the consumers uses the standard regulator accessor functions to operate on
>> their respective regulator *. Further more it's only the clock driver for the
>> core (krait) that references the active only regulators.
>
>> So the knowledge spread out would be contained to the dt bindings things
>> together.
>
> It's not just the DT binding, it's also the regulator driver that needs
> to do the aggregation that's being discussed and is going to assume a
> particular arrangement of clients.
>

The downstream implementation sports 3 rdevs per regulator and their
requests are aggregated into the two states. So the regulator
implementation are not aware of the individual clients, it just
aggregates the 3 rdev states and programs the hardware accordingly.

>> What I don't like with that solution is that we above have two different rdev
>> and that we need to aggregate state between the various rdevs in the ldo12
>> grouping. (so that active only and both actually affect the active state).
>
> Yes, that's a big part of the problem - there's things going on that the
> core should know about but doesn't.

The way that the aggregation of these properties works there's no
problems doing it in stages. But it comes with code duplication and
the need of the various rdevs to share common state.

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

On Mon, Dec 15, 2014 at 10:05:54PM -0800, Bjorn Andersson wrote:
> On Mon, Dec 15, 2014 at 10:04 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Dec 11, 2014 at 02:36:54PM -0800, Bjorn Andersson wrote:

> >> > We already have a runtime API for specifying suspend state.  It doesn't
> >> > currently aggregate but that's a simple matter of programming.

> >> Do you mean alter properties of a state or select between the predefined
> >> states? I've found some apis related to the latter, but not the first.

> > Hrm, indeed.  Well, duplicating the existing APIs seems like a good way
> > forwards - the logic is all the same, it's just that we don't want to
> > apply the changes to the state used at runtime.

> Are you suggesting that we introduce a shadow of the current API for
> the purpose of affecting a certain state? Can you elaborate on how
> that would look like and work?

Like the current APIs but specifying a state?

> >> Because for the overall system the active state is the outlier here. But it
> >> probably doesn't matter for our conclusions.

> > I'd argue that for the purpose of running Linux it's the common state...

> Does "running Linux" indicate that there's instructions flowing
> through the CPU pipeline or that the hardware overall is up and
> running?

I think that for most practical purposes with Android (which is the
interesting thing here) those are very much the same?

> > It's not just the DT binding, it's also the regulator driver that needs
> > to do the aggregation that's being discussed and is going to assume a
> > particular arrangement of clients.

> The downstream implementation sports 3 rdevs per regulator and their
> requests are aggregated into the two states. So the regulator
> implementation are not aware of the individual clients, it just
> aggregates the 3 rdev states and programs the hardware accordingly.

To me doing that aggregation requires some knowledge.

> >> What I don't like with that solution is that we above have two different rdev
> >> and that we need to aggregate state between the various rdevs in the ldo12
> >> grouping. (so that active only and both actually affect the active state).

> > Yes, that's a big part of the problem - there's things going on that the
> > core should know about but doesn't.

> The way that the aggregation of these properties works there's no
> problems doing it in stages. But it comes with code duplication and
> the need of the various rdevs to share common state.

To me both the sharing of state and the code duplication are problems.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Mon, Dec 15, 2014 at 10:05:54PM -0800, Bjorn Andersson wrote:
> On Mon, Dec 15, 2014 at 10:04 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Dec 11, 2014 at 02:36:54PM -0800, Bjorn Andersson wrote:

> >> > We already have a runtime API for specifying suspend state.  It doesn't
> >> > currently aggregate but that's a simple matter of programming.

> >> Do you mean alter properties of a state or select between the predefined
> >> states? I've found some apis related to the latter, but not the first.

> > Hrm, indeed.  Well, duplicating the existing APIs seems like a good way
> > forwards - the logic is all the same, it's just that we don't want to
> > apply the changes to the state used at runtime.

> Are you suggesting that we introduce a shadow of the current API for
> the purpose of affecting a certain state? Can you elaborate on how
> that would look like and work?

Like the current APIs but specifying a state?

> >> Because for the overall system the active state is the outlier here. But it
> >> probably doesn't matter for our conclusions.

> > I'd argue that for the purpose of running Linux it's the common state...

> Does "running Linux" indicate that there's instructions flowing
> through the CPU pipeline or that the hardware overall is up and
> running?

I think that for most practical purposes with Android (which is the
interesting thing here) those are very much the same?

> > It's not just the DT binding, it's also the regulator driver that needs
> > to do the aggregation that's being discussed and is going to assume a
> > particular arrangement of clients.

> The downstream implementation sports 3 rdevs per regulator and their
> requests are aggregated into the two states. So the regulator
> implementation are not aware of the individual clients, it just
> aggregates the 3 rdev states and programs the hardware accordingly.

To me doing that aggregation requires some knowledge.

> >> What I don't like with that solution is that we above have two different rdev
> >> and that we need to aggregate state between the various rdevs in the ldo12
> >> grouping. (so that active only and both actually affect the active state).

> > Yes, that's a big part of the problem - there's things going on that the
> > core should know about but doesn't.

> The way that the aggregation of these properties works there's no
> problems doing it in stages. But it comes with code duplication and
> the need of the various rdevs to share common state.

To me both the sharing of state and the code duplication are problems.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141226/4b243377/attachment.sig>

Re: [RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Fri 26 Dec 09:09 PST 2014, Mark Brown wrote:

> On Mon, Dec 15, 2014 at 10:05:54PM -0800, Bjorn Andersson wrote:
> > On Mon, Dec 15, 2014 at 10:04 AM, Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Dec 11, 2014 at 02:36:54PM -0800, Bjorn Andersson wrote:
> 
> > >> > We already have a runtime API for specifying suspend state.  It doesn't
> > >> > currently aggregate but that's a simple matter of programming.
> 
> > >> Do you mean alter properties of a state or select between the predefined
> > >> states? I've found some apis related to the latter, but not the first.
> 
> > > Hrm, indeed.  Well, duplicating the existing APIs seems like a good way
> > > forwards - the logic is all the same, it's just that we don't want to
> > > apply the changes to the state used at runtime.
> 
> > Are you suggesting that we introduce a shadow of the current API for
> > the purpose of affecting a certain state? Can you elaborate on how
> > that would look like and work?
> 
> Like the current APIs but specifying a state?
> 

So then we have two choices:

1) We make the standard API affect both our states and use the special api to
affect only the active state. Which doesn't seem completely unreasonable to
implement as we only have one special consumer and the aggregation in the
driver is trivial.

2) We make the "run queue empty" the special state and in e.g. the display
driver (DSI) we have to pair every regulator api call with a call to the
special version. So suddenly we have riddled drivers with "knowledge" about how
cpuidle works on these platforms (but not all).

> > >> Because for the overall system the active state is the outlier here. But it
> > >> probably doesn't matter for our conclusions.
> 
> > > I'd argue that for the purpose of running Linux it's the common state...
> 
> > Does "running Linux" indicate that there's instructions flowing
> > through the CPU pipeline or that the hardware overall is up and
> > running?
> 
> I think that for most practical purposes with Android (which is the
> interesting thing here) those are very much the same?
> 

While Android is not the single usecase that we have to consider it's an
excellent example of the opposite.

Take e.g. a Nexus 5, hit the power button to "wake it up" sitting in
lockscreen. You will now have pm8941_ldo12 requested enabled by the display
core (DSI) and the CPU PLLs. When the run queue empties out the display will be
kept on but the CPU no longer need ldo12. (And it might sit there for the next
15 seconds - or whatever timeout you have)


So I don't know which one is statistically more common, but both are very
common to be in.

> > > It's not just the DT binding, it's also the regulator driver that needs
> > > to do the aggregation that's being discussed and is going to assume a
> > > particular arrangement of clients.
> 
> > The downstream implementation sports 3 rdevs per regulator and their
> > requests are aggregated into the two states. So the regulator
> > implementation are not aware of the individual clients, it just
> > aggregates the 3 rdev states and programs the hardware accordingly.
> 
> To me doing that aggregation requires some knowledge.
> 

Yeah, but all the aggregations that we do can be done stepwise, so taking the
various regulator_dev* requests and aggregating them one step futhrer (with the
added knowledge about our states) is fine.

> > >> What I don't like with that solution is that we above have two different rdev
> > >> and that we need to aggregate state between the various rdevs in the ldo12
> > >> grouping. (so that active only and both actually affect the active state).
> 
> > > Yes, that's a big part of the problem - there's things going on that the
> > > core should know about but doesn't.
> 
> > The way that the aggregation of these properties works there's no
> > problems doing it in stages. But it comes with code duplication and
> > the need of the various rdevs to share common state.
> 
> To me both the sharing of state and the code duplication are problems.

I agree, but it's enough time and power cost for us to have to have a solution.

If we implement 1) above then we would need to share the system knowledge with
1 driver (the CPU clock driver) and the actual aggregation in the regulator_dev
driver would be extreemly trivial and more importantly specific.

But it's a step away from the current idea of what those states are meant to
define.


I'll make some prototyping and see how it looks.

Regards,
Bjorn

[RFC 0/2] Qualcomm RPM sleep states

[Bjorn Andersson]

On Fri 26 Dec 09:09 PST 2014, Mark Brown wrote:

> On Mon, Dec 15, 2014 at 10:05:54PM -0800, Bjorn Andersson wrote:
> > On Mon, Dec 15, 2014 at 10:04 AM, Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Dec 11, 2014 at 02:36:54PM -0800, Bjorn Andersson wrote:
> 
> > >> > We already have a runtime API for specifying suspend state.  It doesn't
> > >> > currently aggregate but that's a simple matter of programming.
> 
> > >> Do you mean alter properties of a state or select between the predefined
> > >> states? I've found some apis related to the latter, but not the first.
> 
> > > Hrm, indeed.  Well, duplicating the existing APIs seems like a good way
> > > forwards - the logic is all the same, it's just that we don't want to
> > > apply the changes to the state used at runtime.
> 
> > Are you suggesting that we introduce a shadow of the current API for
> > the purpose of affecting a certain state? Can you elaborate on how
> > that would look like and work?
> 
> Like the current APIs but specifying a state?
> 

So then we have two choices:

1) We make the standard API affect both our states and use the special api to
affect only the active state. Which doesn't seem completely unreasonable to
implement as we only have one special consumer and the aggregation in the
driver is trivial.

2) We make the "run queue empty" the special state and in e.g. the display
driver (DSI) we have to pair every regulator api call with a call to the
special version. So suddenly we have riddled drivers with "knowledge" about how
cpuidle works on these platforms (but not all).

> > >> Because for the overall system the active state is the outlier here. But it
> > >> probably doesn't matter for our conclusions.
> 
> > > I'd argue that for the purpose of running Linux it's the common state...
> 
> > Does "running Linux" indicate that there's instructions flowing
> > through the CPU pipeline or that the hardware overall is up and
> > running?
> 
> I think that for most practical purposes with Android (which is the
> interesting thing here) those are very much the same?
> 

While Android is not the single usecase that we have to consider it's an
excellent example of the opposite.

Take e.g. a Nexus 5, hit the power button to "wake it up" sitting in
lockscreen. You will now have pm8941_ldo12 requested enabled by the display
core (DSI) and the CPU PLLs. When the run queue empties out the display will be
kept on but the CPU no longer need ldo12. (And it might sit there for the next
15 seconds - or whatever timeout you have)


So I don't know which one is statistically more common, but both are very
common to be in.

> > > It's not just the DT binding, it's also the regulator driver that needs
> > > to do the aggregation that's being discussed and is going to assume a
> > > particular arrangement of clients.
> 
> > The downstream implementation sports 3 rdevs per regulator and their
> > requests are aggregated into the two states. So the regulator
> > implementation are not aware of the individual clients, it just
> > aggregates the 3 rdev states and programs the hardware accordingly.
> 
> To me doing that aggregation requires some knowledge.
> 

Yeah, but all the aggregations that we do can be done stepwise, so taking the
various regulator_dev* requests and aggregating them one step futhrer (with the
added knowledge about our states) is fine.

> > >> What I don't like with that solution is that we above have two different rdev
> > >> and that we need to aggregate state between the various rdevs in the ldo12
> > >> grouping. (so that active only and both actually affect the active state).
> 
> > > Yes, that's a big part of the problem - there's things going on that the
> > > core should know about but doesn't.
> 
> > The way that the aggregation of these properties works there's no
> > problems doing it in stages. But it comes with code duplication and
> > the need of the various rdevs to share common state.
> 
> To me both the sharing of state and the code duplication are problems.

I agree, but it's enough time and power cost for us to have to have a solution.

If we implement 1) above then we would need to share the system knowledge with
1 driver (the CPU clock driver) and the actual aggregation in the regulator_dev
driver would be extreemly trivial and more importantly specific.

But it's a step away from the current idea of what those states are meant to
define.


I'll make some prototyping and see how it looks.

Regards,
Bjorn

Re: [RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2807 bytes --]

On Mon, Dec 29, 2014 at 01:54:55PM -0800, Bjorn Andersson wrote:

Could you please fix your mailer to wrap sufficiently far before 80
characters to allow for some quoting?

> On Fri 26 Dec 09:09 PST 2014, Mark Brown wrote:
> > On Mon, Dec 15, 2014 at 10:05:54PM -0800, Bjorn Andersson wrote:

> > > Are you suggesting that we introduce a shadow of the current API for
> > > the purpose of affecting a certain state? Can you elaborate on how
> > > that would look like and work?

> > Like the current APIs but specifying a state?

> So then we have two choices:

> 1) We make the standard API affect both our states and use the special api to
> affect only the active state. Which doesn't seem completely unreasonable to
> implement as we only have one special consumer and the aggregation in the
> driver is trivial.

> 2) We make the "run queue empty" the special state and in e.g. the display
> driver (DSI) we have to pair every regulator api call with a call to the
> special version. So suddenly we have riddled drivers with "knowledge" about how
> cpuidle works on these platforms (but not all).

I think there's more options than that but whatever...

> > > Does "running Linux" indicate that there's instructions flowing
> > > through the CPU pipeline or that the hardware overall is up and
> > > running?

> > I think that for most practical purposes with Android (which is the
> > interesting thing here) those are very much the same?

> While Android is not the single usecase that we have to consider it's an
> excellent example of the opposite.

> Take e.g. a Nexus 5, hit the power button to "wake it up" sitting in
> lockscreen. You will now have pm8941_ldo12 requested enabled by the display
> core (DSI) and the CPU PLLs. When the run queue empties out the display will be
> kept on but the CPU no longer need ldo12. (And it might sit there for the next
> 15 seconds - or whatever timeout you have)

> So I don't know which one is statistically more common, but both are very
> common to be in.

So, a couple of things here.  One is that unless the system leakages are
truly spectacular I'd expect nobody is going to notice a practical
impact from leaving the CPU PLL LDO on while the display is on given
that in an idle display state the display will tend to dominate the
system power consumption, making it all a bit academic.  If you can run
the CPU without the PLL for low power but long term tasks that's more
interesting but a different problem.  The other is that you're thinking
of the common state as being the one that is running most whereas I'm
thinking of it as being the one that we have to work with the most - I'd
expect that we're going to be making more changes to the state while
we're actively running than we will be adjusting the state we're going
to go into on idle.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 0/2] Qualcomm RPM sleep states

[Mark Brown]

On Mon, Dec 29, 2014 at 01:54:55PM -0800, Bjorn Andersson wrote:

Could you please fix your mailer to wrap sufficiently far before 80
characters to allow for some quoting?

> On Fri 26 Dec 09:09 PST 2014, Mark Brown wrote:
> > On Mon, Dec 15, 2014 at 10:05:54PM -0800, Bjorn Andersson wrote:

> > > Are you suggesting that we introduce a shadow of the current API for
> > > the purpose of affecting a certain state? Can you elaborate on how
> > > that would look like and work?

> > Like the current APIs but specifying a state?

> So then we have two choices:

> 1) We make the standard API affect both our states and use the special api to
> affect only the active state. Which doesn't seem completely unreasonable to
> implement as we only have one special consumer and the aggregation in the
> driver is trivial.

> 2) We make the "run queue empty" the special state and in e.g. the display
> driver (DSI) we have to pair every regulator api call with a call to the
> special version. So suddenly we have riddled drivers with "knowledge" about how
> cpuidle works on these platforms (but not all).

I think there's more options than that but whatever...

> > > Does "running Linux" indicate that there's instructions flowing
> > > through the CPU pipeline or that the hardware overall is up and
> > > running?

> > I think that for most practical purposes with Android (which is the
> > interesting thing here) those are very much the same?

> While Android is not the single usecase that we have to consider it's an
> excellent example of the opposite.

> Take e.g. a Nexus 5, hit the power button to "wake it up" sitting in
> lockscreen. You will now have pm8941_ldo12 requested enabled by the display
> core (DSI) and the CPU PLLs. When the run queue empties out the display will be
> kept on but the CPU no longer need ldo12. (And it might sit there for the next
> 15 seconds - or whatever timeout you have)

> So I don't know which one is statistically more common, but both are very
> common to be in.

So, a couple of things here.  One is that unless the system leakages are
truly spectacular I'd expect nobody is going to notice a practical
impact from leaving the CPU PLL LDO on while the display is on given
that in an idle display state the display will tend to dominate the
system power consumption, making it all a bit academic.  If you can run
the CPU without the PLL for low power but long term tasks that's more
interesting but a different problem.  The other is that you're thinking
of the common state as being the one that is running most whereas I'm
thinking of it as being the one that we have to work with the most - I'd
expect that we're going to be making more changes to the state while
we're actively running than we will be adjusting the state we're going
to go into on idle.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141230/224da6da/attachment.sig>