[PATCH v3 0/7] Add RSC power domain support

[PATCH v3 0/7] Add RSC power domain support

[Maulik Shah]

Changes in v3:
- Address Rob's comment on dt property value
- Address Stephen's comments on rpmh-rsc driver change
- Include sc7180 cpuidle low power mode changes from [1]
- Include hierarchical domain idle states converter change from [2]

Changes in v2:
- Add Stephen's Reviewed-By to the first three patches
- Addressed Stephen's comments on fourth patch
- Include changes to connect rpmh domain to cpuidle and genpds

Resource State Coordinator (RSC) is responsible for powering off/lowering
the requirements from CPU subsystem for the associated hardware like buses,
clocks, and regulators when all CPUs and cluster is powered down.

RSC power domain uses last-man activities provided by genpd framework based on
Ulf Hansoon's patch series[3], when the cluster of CPUs enter deepest idle
states. As a part of domain poweroff, RSC can lower resource state requirements
by flushing the cached sleep and wake state votes for resources.

[1] https://patchwork.kernel.org/patch/11218965
[2] https://patchwork.kernel.org/patch/10941671
[3] https://patchwork.kernel.org/project/linux-arm-msm/list/?series=222355

Maulik Shah (6):
  drivers: qcom: rpmh: fix macro to accept NULL argument
  drivers: qcom: rpmh: remove rpmh_flush export
  dt-bindings: soc: qcom: Add RSC power domain specifier
  drivers: qcom: rpmh-rsc: Add RSC power domain support
  arm64: dts: qcom: sc7180: Add cpuidle low power states
  arm64: dts: qcom: sc7180: Convert to the hierarchical CPU topology
    layout

Ulf Hansson (1):
  drivers: firmware: psci: Add hierarchical domain idle states converter

 .../devicetree/bindings/soc/qcom/rpmh-rsc.txt      |   9 ++
 arch/arm64/boot/dts/qcom/sc7180.dtsi               | 133 ++++++++++++++++++++
 drivers/cpuidle/cpuidle-psci-domain.c              | 137 ++++++++++++++++++---
 drivers/cpuidle/cpuidle-psci.c                     |  41 +++---
 drivers/cpuidle/cpuidle-psci.h                     |  11 ++
 drivers/soc/qcom/rpmh-internal.h                   |   3 +
 drivers/soc/qcom/rpmh-rsc.c                        |  81 ++++++++++++
 drivers/soc/qcom/rpmh.c                            |  22 ++--
 include/soc/qcom/rpmh.h                            |   5 -
 9 files changed, 389 insertions(+), 53 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 1/7] drivers: qcom: rpmh: fix macro to accept NULL argument

[Maulik Shah]

Device argument matches with dev variable declared in RPMH message.
Compiler reports error when the argument is NULL since the argument
matches the name of the property. Rename dev argument to device to
fix this.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/soc/qcom/rpmh.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 035091f..3a4579d 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -23,7 +23,7 @@
 
 #define RPMH_TIMEOUT_MS			msecs_to_jiffies(10000)
 
-#define DEFINE_RPMH_MSG_ONSTACK(dev, s, q, name)	\
+#define DEFINE_RPMH_MSG_ONSTACK(device, s, q, name)	\
 	struct rpmh_request name = {			\
 		.msg = {				\
 			.state = s,			\
@@ -33,7 +33,7 @@
 		},					\
 		.cmd = { { 0 } },			\
 		.completion = q,			\
-		.dev = dev,				\
+		.dev = device,				\
 		.needs_free = false,				\
 	}
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 2/7] drivers: qcom: rpmh: remove rpmh_flush export

[Maulik Shah]

rpmh_flush() was exported with the idea that an external entity
operation during CPU idle would know when to flush the sleep and wake
TCS. Since, this is not the case when defining a power domain for the
RSC. Remove the function export and instead allow the function to be
called internally.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/soc/qcom/rpmh-internal.h |  1 +
 drivers/soc/qcom/rpmh.c          | 18 ++++++++----------
 include/soc/qcom/rpmh.h          |  5 -----
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index a7bbbb6..6eec32b 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -110,5 +110,6 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
 int rpmh_rsc_invalidate(struct rsc_drv *drv);
 
 void rpmh_tx_done(const struct tcs_request *msg, int r);
+int rpmh_flush(struct rpmh_ctrlr *ctrlr);
 
 #endif /* __RPM_INTERNAL_H__ */
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 3a4579d..eb0ded0 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -427,11 +427,10 @@ static int is_req_valid(struct cache_req *req)
 		req->sleep_val != req->wake_val);
 }
 
-static int send_single(const struct device *dev, enum rpmh_state state,
+static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
 		       u32 addr, u32 data)
 {
-	DEFINE_RPMH_MSG_ONSTACK(dev, state, NULL, rpm_msg);
-	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+	DEFINE_RPMH_MSG_ONSTACK(NULL, state, NULL, rpm_msg);
 
 	/* Wake sets are always complete and sleep sets are not */
 	rpm_msg.msg.wait_for_compl = (state == RPMH_WAKE_ONLY_STATE);
@@ -445,7 +444,7 @@ static int send_single(const struct device *dev, enum rpmh_state state,
 /**
  * rpmh_flush: Flushes the buffered active and sleep sets to TCS
  *
- * @dev: The device making the request
+ * @ctrlr: controller making request to flush cached data
  *
  * Return: -EBUSY if the controller is busy, probably waiting on a response
  * to a RPMH request sent earlier.
@@ -454,10 +453,9 @@ static int send_single(const struct device *dev, enum rpmh_state state,
  * that is powering down the entire system. Since no other RPMH API would be
  * executing at this time, it is safe to run lockless.
  */
-int rpmh_flush(const struct device *dev)
+int rpmh_flush(struct rpmh_ctrlr *ctrlr)
 {
 	struct cache_req *p;
-	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
 	int ret;
 
 	if (!ctrlr->dirty) {
@@ -480,11 +478,12 @@ int rpmh_flush(const struct device *dev)
 				 __func__, p->addr, p->sleep_val, p->wake_val);
 			continue;
 		}
-		ret = send_single(dev, RPMH_SLEEP_STATE, p->addr, p->sleep_val);
+		ret = send_single(ctrlr, RPMH_SLEEP_STATE, p->addr,
+				  p->sleep_val);
 		if (ret)
 			return ret;
-		ret = send_single(dev, RPMH_WAKE_ONLY_STATE,
-				  p->addr, p->wake_val);
+		ret = send_single(ctrlr, RPMH_WAKE_ONLY_STATE, p->addr,
+				  p->wake_val);
 		if (ret)
 			return ret;
 	}
@@ -493,7 +492,6 @@ int rpmh_flush(const struct device *dev)
 
 	return 0;
 }
-EXPORT_SYMBOL(rpmh_flush);
 
 /**
  * rpmh_invalidate: Invalidate all sleep and active sets
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index 619e07c..f9ec353 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -20,8 +20,6 @@ int rpmh_write_async(const struct device *dev, enum rpmh_state state,
 int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
 		     const struct tcs_cmd *cmd, u32 *n);
 
-int rpmh_flush(const struct device *dev);
-
 int rpmh_invalidate(const struct device *dev);
 
 #else
@@ -40,9 +38,6 @@ static inline int rpmh_write_batch(const struct device *dev,
 				   const struct tcs_cmd *cmd, u32 *n)
 { return -ENODEV; }
 
-static inline int rpmh_flush(const struct device *dev)
-{ return -ENODEV; }
-
 static inline int rpmh_invalidate(const struct device *dev)
 { return -ENODEV; }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 3/7] dt-bindings: soc: qcom: Add RSC power domain specifier

[Maulik Shah]

In addition to transmitting resource state requests to the remote
processor, the RSC is responsible for powering off/lowering the
requirements from CPUs subsystem for the associated hardware like
buses, clocks, and regulators when all CPUs and cluster is powered down.

The power domain is configured to a low power state and when all the
CPUs are powered down, the RSC can lower resource state requirements
and power down the rails that power the CPUs.

Add PM domain specifier property for RSC controller.

Cc: devicetree@vger.kernel.org
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
index 9b86d1e..5682806 100644
--- a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
@@ -83,6 +83,14 @@ Properties:
 	Value type: <string>
 	Definition: Name for the RSC. The name would be used in trace logs.
 
+- #power-domain-cells:
+	Usage: optional
+	Value type: <u32>
+	Definition: Must be 0. Number of cells in power domain specifier.
+		    Optional for controllers that may be in 'solver' state
+		    where they can be in autonomous mode executing low power
+		    modes.
+
 Drivers that want to use the RSC to communicate with RPMH must specify their
 bindings as child nodes of the RSC controllers they wish to communicate with.
 
@@ -112,6 +120,7 @@ TCS-OFFSET: 0xD00
 				  <SLEEP_TCS   3>,
 				  <WAKE_TCS    3>,
 				  <CONTROL_TCS 1>;
+		#power-domain-cells = <0>;
 	};
 
 Example 2:
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 4/7] drivers: qcom: rpmh-rsc: Add RSC power domain support

[Maulik Shah]

Add RSC power domain support. RSC is top level power domain in
hireachical CPU LPM modes. Once the rsc domain is down flush all
cached sleep and wake votes from controller.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/soc/qcom/rpmh-internal.h |  2 +
 drivers/soc/qcom/rpmh-rsc.c      | 81 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 6eec32b..3736c14 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -8,6 +8,7 @@
 #define __RPM_INTERNAL_H__
 
 #include <linux/bitmap.h>
+#include <linux/pm_domain.h>
 #include <soc/qcom/tcs.h>
 
 #define TCS_TYPE_NR			4
@@ -102,6 +103,7 @@ struct rsc_drv {
 	DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
 	spinlock_t lock;
 	struct rpmh_ctrlr client;
+	struct generic_pm_domain rsc_pd;
 };
 
 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e278fc1..92bca4c 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -499,6 +499,32 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
 }
 
 /**
+ * rpmh_rsc_ctrlr_is_idle: Check if any of the AMCs are busy.
+ *
+ * @drv: The controller
+ *
+ * Return: false if the TCSes are engaged in handling requests, True otherwise.
+ */
+static bool rpmh_rsc_ctrlr_is_idle(struct rsc_drv *drv)
+{
+	int m;
+	struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS);
+	bool ret = true;
+	unsigned long flags;
+
+	spin_lock_irqsave(&drv->lock, flags);
+	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+		if (!tcs_is_free(drv, m)) {
+			ret = false;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&drv->lock, flags);
+
+	return ret;
+}
+
+/**
  * rpmh_rsc_write_ctrl_data: Write request to the controller
  *
  * @drv: the controller
@@ -521,6 +547,50 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
 	return tcs_ctrl_write(drv, msg);
 }
 
+static int rpmh_domain_power_off(struct generic_pm_domain *rsc_pd)
+{
+	struct rsc_drv *drv = container_of(rsc_pd, struct rsc_drv, rsc_pd);
+
+	/*
+	 * RPMh domain can not be powered off when there is pending ACK for
+	 * ACTIVE_TCS request. Exit when controller is busy.
+	 */
+	if (!rpmh_rsc_ctrlr_is_idle(drv))
+		return -EBUSY;
+
+	return rpmh_flush(&drv->client);
+}
+
+static int rpmh_probe_power_domain(struct platform_device *pdev,
+				   struct rsc_drv *drv)
+{
+	int ret;
+	struct generic_pm_domain *rsc_pd = &drv->rsc_pd;
+	struct device_node *dn = pdev->dev.of_node;
+
+	rsc_pd->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s", dn->name);
+	if (!rsc_pd->name)
+		return -ENOMEM;
+
+	rsc_pd->name = kbasename(rsc_pd->name);
+	rsc_pd->power_off = rpmh_domain_power_off;
+	rsc_pd->flags |= GENPD_FLAG_IRQ_SAFE;
+
+	ret = pm_genpd_init(rsc_pd, NULL, false);
+	if (ret)
+		return ret;
+
+	ret = of_genpd_add_provider_simple(dn, rsc_pd);
+	if (ret)
+		goto remove_pd;
+
+	return ret;
+
+remove_pd:
+	pm_genpd_remove(rsc_pd);
+	return ret;
+}
+
 static int rpmh_probe_tcs_config(struct platform_device *pdev,
 				 struct rsc_drv *drv)
 {
@@ -663,6 +733,17 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	/*
+	 * Power domain is not required for controllers that support 'solver'
+	 * mode where they can be in autonomous mode executing low power mode
+	 * to power down.
+	 */
+	if (of_property_read_bool(dn, "#power-domain-cells")) {
+		ret = rpmh_probe_power_domain(pdev, drv);
+		if (ret)
+			return ret;
+	}
+
 	/* Enable the active TCS to send requests immediately */
 	write_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, drv->tcs[ACTIVE_TCS].mask);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Maulik Shah]

From: Ulf Hansson <ulf.hansson@linaro.org>

If the hierarchical CPU topology is used, but the OS initiated mode isn't
supported, we need to rely solely on the regular cpuidle framework to
manage the idle state selection, rather than using genpd and its
governor.

For this reason, introduce a new PSCI DT helper function,
psci_dt_pm_domains_parse_states(), which parses and converts the
hierarchically described domain idle states from DT, into regular flattened
cpuidle states. The converted states are added to the existing cpuidle
driver's array of idle states, which make them available for cpuidle.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
[applied to new path, resolved conflicts]
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/cpuidle/cpuidle-psci-domain.c | 137 +++++++++++++++++++++++++++++-----
 drivers/cpuidle/cpuidle-psci.c        |  41 +++++-----
 drivers/cpuidle/cpuidle-psci.h        |  11 +++
 3 files changed, 153 insertions(+), 36 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index 423f03b..3c417f7 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -26,13 +26,17 @@ struct psci_pd_provider {
 };
 
 static LIST_HEAD(psci_pd_providers);
-static bool osi_mode_enabled __initdata;
+static bool osi_mode_enabled;
 
 static int psci_pd_power_off(struct generic_pm_domain *pd)
 {
 	struct genpd_power_state *state = &pd->states[pd->state_idx];
 	u32 *pd_state;
 
+	/* If we have failed to enable OSI mode, then abort power off. */
+	if ((psci_has_osi_support()) && !osi_mode_enabled)
+		return -EBUSY;
+
 	if (!state->data)
 		return 0;
 
@@ -101,6 +105,105 @@ static void psci_pd_free_states(struct genpd_power_state *states,
 	kfree(states);
 }
 
+static void psci_pd_convert_states(struct cpuidle_state *idle_state,
+			u32 *psci_state, struct genpd_power_state *state)
+{
+	u32 *state_data = state->data;
+	u64 target_residency_us = state->residency_ns;
+	u64 exit_latency_us = state->power_on_latency_ns +
+			state->power_off_latency_ns;
+
+	*psci_state = *state_data;
+	do_div(target_residency_us, 1000);
+	idle_state->target_residency = target_residency_us;
+	do_div(exit_latency_us, 1000);
+	idle_state->exit_latency = exit_latency_us;
+	idle_state->enter = &psci_enter_domain_idle_state;
+	idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
+
+	strncpy(idle_state->name, to_of_node(state->fwnode)->name,
+		CPUIDLE_NAME_LEN - 1);
+	strncpy(idle_state->desc, to_of_node(state->fwnode)->name,
+		CPUIDLE_NAME_LEN - 1);
+}
+
+static bool psci_pd_is_provider(struct device_node *np)
+{
+	struct psci_pd_provider *pd_prov, *it;
+
+	list_for_each_entry_safe(pd_prov, it, &psci_pd_providers, link) {
+		if (pd_prov->node == np)
+			return true;
+	}
+
+	return false;
+}
+
+int __init psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
+			struct device_node *cpu_node, u32 *psci_states)
+{
+	struct genpd_power_state *pd_states;
+	struct of_phandle_args args;
+	int ret, pd_state_count, i, state_idx, psci_idx;
+	u32 cpu_psci_state = psci_states[drv->state_count - 1];
+	struct device_node *np = of_node_get(cpu_node);
+
+	/* Walk the CPU topology to find compatible domain idle states. */
+	while (np) {
+		ret = of_parse_phandle_with_args(np, "power-domains",
+					"#power-domain-cells", 0, &args);
+		of_node_put(np);
+		if (ret)
+			return 0;
+
+		np = args.np;
+
+		/* Verify that the node represents a psci pd provider. */
+		if (!psci_pd_is_provider(np)) {
+			of_node_put(np);
+			return 0;
+		}
+
+		/* Parse for compatible domain idle states. */
+		ret = psci_pd_parse_states(np, &pd_states, &pd_state_count);
+		if (ret) {
+			of_node_put(np);
+			return ret;
+		}
+
+		i = 0;
+		while (i < pd_state_count) {
+
+			state_idx = drv->state_count;
+			if (state_idx >= CPUIDLE_STATE_MAX) {
+				pr_warn("exceeding max cpuidle states\n");
+				of_node_put(np);
+				return 0;
+			}
+
+			psci_idx = state_idx + i;
+			psci_pd_convert_states(&drv->states[state_idx + i],
+					&psci_states[psci_idx], &pd_states[i]);
+
+			/*
+			 * In the hierarchical CPU topology the master PM domain
+			 * idle state's DT property, "arm,psci-suspend-param",
+			 * don't contain the bits for the idle state of the CPU,
+			 * let's add those here.
+			 */
+			psci_states[psci_idx] |= cpu_psci_state;
+			pr_debug("psci-power-state %#x index %d\n",
+				psci_states[psci_idx], psci_idx);
+
+			drv->state_count++;
+			i++;
+		}
+		psci_pd_free_states(pd_states, pd_state_count);
+	}
+
+	return 0;
+}
+
 static int __init psci_pd_init(struct device_node *np)
 {
 	struct generic_pm_domain *pd;
@@ -125,11 +228,14 @@ static int __init psci_pd_init(struct device_node *np)
 	 * Parse the domain idle states and let genpd manage the state selection
 	 * for those being compatible with "domain-idle-state".
 	 */
-	ret = psci_pd_parse_states(np, &states, &state_count);
-	if (ret)
-		goto free_name;
 
-	pd->free_states = psci_pd_free_states;
+	if (psci_has_osi_support()) {
+		ret = psci_pd_parse_states(np, &states, &state_count);
+		if (ret)
+			goto free_name;
+		pd->free_states = psci_pd_free_states;
+	}
+
 	pd->name = kbasename(pd->name);
 	pd->power_off = psci_pd_power_off;
 	pd->states = states;
@@ -236,10 +342,6 @@ static int __init psci_idle_init_domains(void)
 	if (!np)
 		return -ENODEV;
 
-	/* Currently limit the hierarchical topology to be used in OSI mode. */
-	if (!psci_has_osi_support())
-		goto out;
-
 	/*
 	 * Parse child nodes for the "#power-domain-cells" property and
 	 * initialize a genpd/genpd-of-provider pair when it's found.
@@ -265,14 +367,16 @@ static int __init psci_idle_init_domains(void)
 		goto remove_pd;
 
 	/* Try to enable OSI mode. */
-	ret = psci_set_osi_mode();
-	if (ret) {
-		pr_warn("failed to enable OSI mode: %d\n", ret);
-		psci_pd_remove_topology(np);
-		goto remove_pd;
+	if (psci_has_osi_support()) {
+		ret = psci_set_osi_mode();
+		if (ret) {
+			pr_warn("failed to enable OSI mode: %d\n", ret);
+			psci_pd_remove_topology(np);
+			goto remove_pd;
+		} else
+			osi_mode_enabled = true;
 	}
 
-	osi_mode_enabled = true;
 	of_node_put(np);
 	pr_info("Initialized CPU PM domain topology\n");
 	return pd_count;
@@ -293,9 +397,6 @@ struct device __init *psci_dt_attach_cpu(int cpu)
 {
 	struct device *dev;
 
-	if (!osi_mode_enabled)
-		return NULL;
-
 	dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
 	if (IS_ERR_OR_NULL(dev))
 		return dev;
diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index edd7a54..3fa2aee 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -49,7 +49,7 @@ static inline int psci_enter_state(int idx, u32 state)
 	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
 }
 
-static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
+int psci_enter_domain_idle_state(struct cpuidle_device *dev,
 					struct cpuidle_driver *drv, int idx)
 {
 	struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
@@ -193,24 +193,29 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 		goto free_mem;
 	}
 
-	/* Currently limit the hierarchical topology to be used in OSI mode. */
-	if (psci_has_osi_support()) {
-		data->dev = psci_dt_attach_cpu(cpu);
-		if (IS_ERR(data->dev)) {
-			ret = PTR_ERR(data->dev);
+	if (!psci_has_osi_support()) {
+		ret = psci_dt_pm_domains_parse_states(drv, cpu_node,
+					      psci_states);
+		if (ret)
 			goto free_mem;
-		}
-
-		/*
-		 * Using the deepest state for the CPU to trigger a potential
-		 * selection of a shared state for the domain, assumes the
-		 * domain states are all deeper states.
-		 */
-		if (data->dev) {
-			drv->states[state_count - 1].enter =
-				psci_enter_domain_idle_state;
-			psci_cpuidle_use_cpuhp = true;
-		}
+	}
+
+	data->dev = psci_dt_attach_cpu(cpu);
+	if (IS_ERR(data->dev)) {
+		ret = PTR_ERR(data->dev);
+		goto free_mem;
+	}
+
+	/*
+	 * Using the deepest state for the CPU to trigger a potential
+	 * selection of a shared state for the domain, assumes the
+	 * domain states are all deeper states.
+	 */
+
+	if (data->dev) {
+		drv->states[state_count - 1].enter =
+			psci_enter_domain_idle_state;
+		psci_cpuidle_use_cpuhp = true;
 	}
 
 	/* Idle states parsed correctly, store them in the per-cpu struct. */
diff --git a/drivers/cpuidle/cpuidle-psci.h b/drivers/cpuidle/cpuidle-psci.h
index 7299a04..18c93d7 100644
--- a/drivers/cpuidle/cpuidle-psci.h
+++ b/drivers/cpuidle/cpuidle-psci.h
@@ -3,15 +3,26 @@
 #ifndef __CPUIDLE_PSCI_H
 #define __CPUIDLE_PSCI_H
 
+#include <linux/cpuidle.h>
+
 struct device_node;
 
 void psci_set_domain_state(u32 state);
 int __init psci_dt_parse_state_node(struct device_node *np, u32 *state);
+int psci_enter_domain_idle_state(struct cpuidle_device *dev,
+			  struct cpuidle_driver *drv, int idx);
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 struct device __init *psci_dt_attach_cpu(int cpu);
+int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
+				    struct device_node *cpu_node,
+				    u32 *psci_states);
 #else
 static inline struct device __init *psci_dt_attach_cpu(int cpu) { return NULL; }
+static inline int psci_dt_pm_domains_parse_states(
+					struct cpuidle_driver *drv,
+					struct device_node *cpu_node,
+					u32 *psci_states) { return 0; }
 #endif
 
 #endif /* __CPUIDLE_PSCI_H */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 6/7] arm64: dts: qcom: sc7180: Add cpuidle low power states

[Maulik Shah]

Add device bindings for cpuidle states for cpu devices.

Cc: devicetree@vger.kernel.org
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 78 ++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 8011c5f..0aa0ced 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -86,6 +86,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_0>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -103,6 +106,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x100>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_100>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -117,6 +123,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x200>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_200>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -131,6 +140,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x300>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_300>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -145,6 +157,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x400>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_400>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -159,6 +174,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x500>;
 			enable-method = "psci";
+			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
+					   &LITTLE_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_500>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -173,6 +191,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x600>;
 			enable-method = "psci";
+			cpu-idle-states = <&BIG_CPU_SLEEP_0
+					   &BIG_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_600>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
@@ -187,6 +208,9 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x700>;
 			enable-method = "psci";
+			cpu-idle-states = <&BIG_CPU_SLEEP_0
+					   &BIG_CPU_SLEEP_1
+					   &CLUSTER_SLEEP_0>;
 			next-level-cache = <&L2_700>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
@@ -195,6 +219,60 @@
 				next-level-cache = <&L3_0>;
 			};
 		};
+
+		idle-states {
+			entry-method = "psci";
+
+			LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "little-power-down";
+				arm,psci-suspend-param = <0x40000003>;
+				entry-latency-us = <549>;
+				exit-latency-us = <901>;
+				min-residency-us = <1774>;
+				local-timer-stop;
+			};
+
+			LITTLE_CPU_SLEEP_1: cpu-sleep-0-1 {
+				compatible = "arm,idle-state";
+				idle-state-name = "little-rail-power-down";
+				arm,psci-suspend-param = <0x40000004>;
+				entry-latency-us = <702>;
+				exit-latency-us = <915>;
+				min-residency-us = <4001>;
+				local-timer-stop;
+			};
+
+			BIG_CPU_SLEEP_0: cpu-sleep-1-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "big-power-down";
+				arm,psci-suspend-param = <0x40000003>;
+				entry-latency-us = <523>;
+				exit-latency-us = <1244>;
+				min-residency-us = <2207>;
+				local-timer-stop;
+			};
+
+			BIG_CPU_SLEEP_1: cpu-sleep-1-1 {
+				compatible = "arm,idle-state";
+				idle-state-name = "big-rail-power-down";
+				arm,psci-suspend-param = <0x40000004>;
+				entry-latency-us = <526>;
+				exit-latency-us = <1854>;
+				min-residency-us = <5555>;
+				local-timer-stop;
+			};
+
+			CLUSTER_SLEEP_0: cluster-sleep-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "cluster-power-down";
+				arm,psci-suspend-param = <0x40003444>;
+				entry-latency-us = <3263>;
+				exit-latency-us = <6562>;
+				min-residency-us = <9926>;
+				local-timer-stop;
+			};
+		};
 	};
 
 	memory@80000000 {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 7/7] arm64: dts: qcom: sc7180: Convert to the hierarchical CPU topology layout

[Maulik Shah]

To enable the OS to manage last-man standing activities for a CPU, while
an idle state for a group of CPUs is selected, let's convert the sc7180
platform into using the hierarchical CPU topology layout.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 105 ++++++++++++++++++++++++++---------
 1 file changed, 80 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 0aa0ced..c366d10 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -86,9 +86,8 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD0>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_0>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -106,9 +105,8 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x100>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD1>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_100>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -123,9 +121,8 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x200>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD2>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_200>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -140,9 +137,8 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x300>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD3>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_300>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -157,9 +153,8 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x400>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD4>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_400>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -174,9 +169,8 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x500>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD5>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_500>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 0>;
@@ -191,9 +185,8 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x600>;
 			enable-method = "psci";
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD6>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_600>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
@@ -208,9 +201,8 @@
 			compatible = "arm,armv8";
 			reg = <0x0 0x700>;
 			enable-method = "psci";
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD7>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_700>;
 			#cooling-cells = <2>;
 			qcom,freq-domain = <&cpufreq_hw 1>;
@@ -264,7 +256,7 @@
 			};
 
 			CLUSTER_SLEEP_0: cluster-sleep-0 {
-				compatible = "arm,idle-state";
+				compatible = "domain-idle-state";
 				idle-state-name = "cluster-power-down";
 				arm,psci-suspend-param = <0x40003444>;
 				entry-latency-us = <3263>;
@@ -375,6 +367,68 @@
 	psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
+
+		CPU_PD0: cpu-pd0 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>,
+					     <&LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD1: cpu-pd1 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>,
+					     <&LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD2: cpu-pd2 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>,
+					     <&LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD3: cpu-pd3 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>,
+					     <&LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD4: cpu-pd4 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>,
+					     <&LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD5: cpu-pd5 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0>,
+					     <&LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD6: cpu-pd6 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0>,
+					     <&BIG_CPU_SLEEP_1>;
+		};
+
+		CPU_PD7: cpu-pd7 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0>,
+					     <&BIG_CPU_SLEEP_1>;
+		};
+
+		CLUSTER_PD: cluster-pd {
+			#power-domain-cells = <0>;
+			power-domains = <&apps_rsc>;
+			domain-idle-states = <&CLUSTER_SLEEP_0>;
+		};
 	};
 
 	soc: soc {
@@ -1495,6 +1549,7 @@
 					  <SLEEP_TCS   3>,
 					  <WAKE_TCS    3>,
 					  <CONTROL_TCS 1>;
+			#power-domain-cells = <0>;
 
 			rpmhcc: clock-controller {
 				compatible = "qcom,sc7180-rpmh-clk";
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Sudeep Holla]

On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> If the hierarchical CPU topology is used, but the OS initiated mode isn't
> supported, we need to rely solely on the regular cpuidle framework to
> manage the idle state selection, rather than using genpd and its
> governor.
>
> For this reason, introduce a new PSCI DT helper function,
> psci_dt_pm_domains_parse_states(), which parses and converts the
> hierarchically described domain idle states from DT, into regular flattened
> cpuidle states. The converted states are added to the existing cpuidle
> driver's array of idle states, which make them available for cpuidle.
>

And what's the main motivation for this if OSI is not supported in the
firmware ?

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> [applied to new path, resolved conflicts]
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  drivers/cpuidle/cpuidle-psci-domain.c | 137 +++++++++++++++++++++++++++++-----
>  drivers/cpuidle/cpuidle-psci.c        |  41 +++++-----
>  drivers/cpuidle/cpuidle-psci.h        |  11 +++
>  3 files changed, 153 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> index 423f03b..3c417f7 100644
> --- a/drivers/cpuidle/cpuidle-psci-domain.c
> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> @@ -26,13 +26,17 @@ struct psci_pd_provider {
>  };
>
>  static LIST_HEAD(psci_pd_providers);
> -static bool osi_mode_enabled __initdata;
> +static bool osi_mode_enabled;
>
>  static int psci_pd_power_off(struct generic_pm_domain *pd)
>  {
>  	struct genpd_power_state *state = &pd->states[pd->state_idx];
>  	u32 *pd_state;
>
> +	/* If we have failed to enable OSI mode, then abort power off. */
> +	if ((psci_has_osi_support()) && !osi_mode_enabled)
> +		return -EBUSY;
> +

Why is this needed ? IIUC we don't create genpd domains if OSI is not
enabled.

>  	if (!state->data)
>  		return 0;
>
> @@ -101,6 +105,105 @@ static void psci_pd_free_states(struct genpd_power_state *states,
>  	kfree(states);
>  }
>
> +static void psci_pd_convert_states(struct cpuidle_state *idle_state,
> +			u32 *psci_state, struct genpd_power_state *state)
> +{
> +	u32 *state_data = state->data;
> +	u64 target_residency_us = state->residency_ns;
> +	u64 exit_latency_us = state->power_on_latency_ns +
> +			state->power_off_latency_ns;
> +
> +	*psci_state = *state_data;
> +	do_div(target_residency_us, 1000);
> +	idle_state->target_residency = target_residency_us;
> +	do_div(exit_latency_us, 1000);
> +	idle_state->exit_latency = exit_latency_us;
> +	idle_state->enter = &psci_enter_domain_idle_state;
> +	idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> +
> +	strncpy(idle_state->name, to_of_node(state->fwnode)->name,
> +		CPUIDLE_NAME_LEN - 1);
> +	strncpy(idle_state->desc, to_of_node(state->fwnode)->name,
> +		CPUIDLE_NAME_LEN - 1);
> +}
> +
> +static bool psci_pd_is_provider(struct device_node *np)
> +{
> +	struct psci_pd_provider *pd_prov, *it;
> +
> +	list_for_each_entry_safe(pd_prov, it, &psci_pd_providers, link) {
> +		if (pd_prov->node == np)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +int __init psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> +			struct device_node *cpu_node, u32 *psci_states)
> +{
> +	struct genpd_power_state *pd_states;
> +	struct of_phandle_args args;
> +	int ret, pd_state_count, i, state_idx, psci_idx;
> +	u32 cpu_psci_state = psci_states[drv->state_count - 1];
> +	struct device_node *np = of_node_get(cpu_node);
> +
> +	/* Walk the CPU topology to find compatible domain idle states. */
> +	while (np) {
> +		ret = of_parse_phandle_with_args(np, "power-domains",
> +					"#power-domain-cells", 0, &args);
> +		of_node_put(np);
> +		if (ret)
> +			return 0;
> +
> +		np = args.np;
> +
> +		/* Verify that the node represents a psci pd provider. */
> +		if (!psci_pd_is_provider(np)) {
> +			of_node_put(np);
> +			return 0;
> +		}
> +
> +		/* Parse for compatible domain idle states. */
> +		ret = psci_pd_parse_states(np, &pd_states, &pd_state_count);
> +		if (ret) {
> +			of_node_put(np);
> +			return ret;
> +		}
> +
> +		i = 0;
> +		while (i < pd_state_count) {
> +
> +			state_idx = drv->state_count;
> +			if (state_idx >= CPUIDLE_STATE_MAX) {
> +				pr_warn("exceeding max cpuidle states\n");
> +				of_node_put(np);
> +				return 0;
> +			}
> +
> +			psci_idx = state_idx + i;
> +			psci_pd_convert_states(&drv->states[state_idx + i],
> +					&psci_states[psci_idx], &pd_states[i]);
> +
> +			/*
> +			 * In the hierarchical CPU topology the master PM domain
> +			 * idle state's DT property, "arm,psci-suspend-param",
> +			 * don't contain the bits for the idle state of the CPU,
> +			 * let's add those here.
> +			 */
> +			psci_states[psci_idx] |= cpu_psci_state;

No we can't do that. Refer previous discussions around that.

> +			pr_debug("psci-power-state %#x index %d\n",
> +				psci_states[psci_idx], psci_idx);
> +
> +			drv->state_count++;
> +			i++;
> +		}
> +		psci_pd_free_states(pd_states, pd_state_count);
> +	}
> +
> +	return 0;
> +}
> +
>  static int __init psci_pd_init(struct device_node *np)
>  {
>  	struct generic_pm_domain *pd;
> @@ -125,11 +228,14 @@ static int __init psci_pd_init(struct device_node *np)
>  	 * Parse the domain idle states and let genpd manage the state selection
>  	 * for those being compatible with "domain-idle-state".
>  	 */
> -	ret = psci_pd_parse_states(np, &states, &state_count);
> -	if (ret)
> -		goto free_name;
>
> -	pd->free_states = psci_pd_free_states;
> +	if (psci_has_osi_support()) {
> +		ret = psci_pd_parse_states(np, &states, &state_count);
> +		if (ret)
> +			goto free_name;
> +		pd->free_states = psci_pd_free_states;
> +	}
> +
>  	pd->name = kbasename(pd->name);
>  	pd->power_off = psci_pd_power_off;
>  	pd->states = states;
> @@ -236,10 +342,6 @@ static int __init psci_idle_init_domains(void)
>  	if (!np)
>  		return -ENODEV;
>
> -	/* Currently limit the hierarchical topology to be used in OSI mode. */
> -	if (!psci_has_osi_support())
> -		goto out;
> -
>  	/*
>  	 * Parse child nodes for the "#power-domain-cells" property and
>  	 * initialize a genpd/genpd-of-provider pair when it's found.
> @@ -265,14 +367,16 @@ static int __init psci_idle_init_domains(void)
>  		goto remove_pd;
>
>  	/* Try to enable OSI mode. */
> -	ret = psci_set_osi_mode();
> -	if (ret) {
> -		pr_warn("failed to enable OSI mode: %d\n", ret);
> -		psci_pd_remove_topology(np);
> -		goto remove_pd;
> +	if (psci_has_osi_support()) {
> +		ret = psci_set_osi_mode();
> +		if (ret) {
> +			pr_warn("failed to enable OSI mode: %d\n", ret);
> +			psci_pd_remove_topology(np);
> +			goto remove_pd;
> +		} else
> +			osi_mode_enabled = true;
>  	}
>
> -	osi_mode_enabled = true;
>  	of_node_put(np);
>  	pr_info("Initialized CPU PM domain topology\n");
>  	return pd_count;
> @@ -293,9 +397,6 @@ struct device __init *psci_dt_attach_cpu(int cpu)
>  {
>  	struct device *dev;
>
> -	if (!osi_mode_enabled)
> -		return NULL;
> -
>  	dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
>  	if (IS_ERR_OR_NULL(dev))
>  		return dev;
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index edd7a54..3fa2aee 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -49,7 +49,7 @@ static inline int psci_enter_state(int idx, u32 state)
>  	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
>  }
>
> -static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> +int psci_enter_domain_idle_state(struct cpuidle_device *dev,
>  					struct cpuidle_driver *drv, int idx)
>  {
>  	struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> @@ -193,24 +193,29 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
>  		goto free_mem;
>  	}
>
> -	/* Currently limit the hierarchical topology to be used in OSI mode. */
> -	if (psci_has_osi_support()) {
> -		data->dev = psci_dt_attach_cpu(cpu);
> -		if (IS_ERR(data->dev)) {
> -			ret = PTR_ERR(data->dev);
> +	if (!psci_has_osi_support()) {
> +		ret = psci_dt_pm_domains_parse_states(drv, cpu_node,
> +					      psci_states);
> +		if (ret)
>  			goto free_mem;
> -		}
> -
> -		/*
> -		 * Using the deepest state for the CPU to trigger a potential
> -		 * selection of a shared state for the domain, assumes the
> -		 * domain states are all deeper states.
> -		 */
> -		if (data->dev) {
> -			drv->states[state_count - 1].enter =
> -				psci_enter_domain_idle_state;
> -			psci_cpuidle_use_cpuhp = true;
> -		}
> +	}
> +
> +	data->dev = psci_dt_attach_cpu(cpu);
> +	if (IS_ERR(data->dev)) {
> +		ret = PTR_ERR(data->dev);
> +		goto free_mem;
> +	}
> +
> +	/*
> +	 * Using the deepest state for the CPU to trigger a potential
> +	 * selection of a shared state for the domain, assumes the
> +	 * domain states are all deeper states.
> +	 */
> +
> +	if (data->dev) {
> +		drv->states[state_count - 1].enter =
> +			psci_enter_domain_idle_state;
> +		psci_cpuidle_use_cpuhp = true;
>  	}
>
>  	/* Idle states parsed correctly, store them in the per-cpu struct. */

--
Regards,
Sudeep

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Maulik Shah]

On 2/3/2020 10:38 PM, Sudeep Holla wrote:
> On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> If the hierarchical CPU topology is used, but the OS initiated mode isn't
>> supported, we need to rely solely on the regular cpuidle framework to
>> manage the idle state selection, rather than using genpd and its
>> governor.
>>
>> For this reason, introduce a new PSCI DT helper function,
>> psci_dt_pm_domains_parse_states(), which parses and converts the
>> hierarchically described domain idle states from DT, into regular flattened
>> cpuidle states. The converted states are added to the existing cpuidle
>> driver's array of idle states, which make them available for cpuidle.
>>
> And what's the main motivation for this if OSI is not supported in the
> firmware ?

Hi Sudeep,

Main motivation is to do last-man activities before the CPU cluster can 
enter a deep idle state.

>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> [applied to new path, resolved conflicts]
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>>   drivers/cpuidle/cpuidle-psci-domain.c | 137 +++++++++++++++++++++++++++++-----
>>   drivers/cpuidle/cpuidle-psci.c        |  41 +++++-----
>>   drivers/cpuidle/cpuidle-psci.h        |  11 +++
>>   3 files changed, 153 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
>> index 423f03b..3c417f7 100644
>> --- a/drivers/cpuidle/cpuidle-psci-domain.c
>> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
>> @@ -26,13 +26,17 @@ struct psci_pd_provider {
>>   };
>>
>>   static LIST_HEAD(psci_pd_providers);
>> -static bool osi_mode_enabled __initdata;
>> +static bool osi_mode_enabled;
>>
>>   static int psci_pd_power_off(struct generic_pm_domain *pd)
>>   {
>>   	struct genpd_power_state *state = &pd->states[pd->state_idx];
>>   	u32 *pd_state;
>>
>> +	/* If we have failed to enable OSI mode, then abort power off. */
>> +	if ((psci_has_osi_support()) && !osi_mode_enabled)
>> +		return -EBUSY;
>> +
> Why is this needed ? IIUC we don't create genpd domains if OSI is not
> enabled.

we do create genpd domains, for cpu domains, we just abort power off 
here since idle states are converted into regular flattened mode.

however genpd poweroff will be used by parent domain (rsc in this case) 
which is kept in hireachy in DTSI with cluster domain to do last man 
activities.

>>   	if (!state->data)
>>   		return 0;
>>
>> @@ -101,6 +105,105 @@ static void psci_pd_free_states(struct genpd_power_state *states,
>>   	kfree(states);
>>   }
>>
>> +static void psci_pd_convert_states(struct cpuidle_state *idle_state,
>> +			u32 *psci_state, struct genpd_power_state *state)
>> +{
>> +	u32 *state_data = state->data;
>> +	u64 target_residency_us = state->residency_ns;
>> +	u64 exit_latency_us = state->power_on_latency_ns +
>> +			state->power_off_latency_ns;
>> +
>> +	*psci_state = *state_data;
>> +	do_div(target_residency_us, 1000);
>> +	idle_state->target_residency = target_residency_us;
>> +	do_div(exit_latency_us, 1000);
>> +	idle_state->exit_latency = exit_latency_us;
>> +	idle_state->enter = &psci_enter_domain_idle_state;
>> +	idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
>> +
>> +	strncpy(idle_state->name, to_of_node(state->fwnode)->name,
>> +		CPUIDLE_NAME_LEN - 1);
>> +	strncpy(idle_state->desc, to_of_node(state->fwnode)->name,
>> +		CPUIDLE_NAME_LEN - 1);
>> +}
>> +
>> +static bool psci_pd_is_provider(struct device_node *np)
>> +{
>> +	struct psci_pd_provider *pd_prov, *it;
>> +
>> +	list_for_each_entry_safe(pd_prov, it, &psci_pd_providers, link) {
>> +		if (pd_prov->node == np)
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +int __init psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
>> +			struct device_node *cpu_node, u32 *psci_states)
>> +{
>> +	struct genpd_power_state *pd_states;
>> +	struct of_phandle_args args;
>> +	int ret, pd_state_count, i, state_idx, psci_idx;
>> +	u32 cpu_psci_state = psci_states[drv->state_count - 1];
>> +	struct device_node *np = of_node_get(cpu_node);
>> +
>> +	/* Walk the CPU topology to find compatible domain idle states. */
>> +	while (np) {
>> +		ret = of_parse_phandle_with_args(np, "power-domains",
>> +					"#power-domain-cells", 0, &args);
>> +		of_node_put(np);
>> +		if (ret)
>> +			return 0;
>> +
>> +		np = args.np;
>> +
>> +		/* Verify that the node represents a psci pd provider. */
>> +		if (!psci_pd_is_provider(np)) {
>> +			of_node_put(np);
>> +			return 0;
>> +		}
>> +
>> +		/* Parse for compatible domain idle states. */
>> +		ret = psci_pd_parse_states(np, &pd_states, &pd_state_count);
>> +		if (ret) {
>> +			of_node_put(np);
>> +			return ret;
>> +		}
>> +
>> +		i = 0;
>> +		while (i < pd_state_count) {
>> +
>> +			state_idx = drv->state_count;
>> +			if (state_idx >= CPUIDLE_STATE_MAX) {
>> +				pr_warn("exceeding max cpuidle states\n");
>> +				of_node_put(np);
>> +				return 0;
>> +			}
>> +
>> +			psci_idx = state_idx + i;
>> +			psci_pd_convert_states(&drv->states[state_idx + i],
>> +					&psci_states[psci_idx], &pd_states[i]);
>> +
>> +			/*
>> +			 * In the hierarchical CPU topology the master PM domain
>> +			 * idle state's DT property, "arm,psci-suspend-param",
>> +			 * don't contain the bits for the idle state of the CPU,
>> +			 * let's add those here.
>> +			 */
>> +			psci_states[psci_idx] |= cpu_psci_state;
> No we can't do that. Refer previous discussions around that.

Thanks for pointing this.

i will remove this in next version, we already have cpu idle state bits 
present in cluster modes.

>
>> +			pr_debug("psci-power-state %#x index %d\n",
>> +				psci_states[psci_idx], psci_idx);
>> +
>> +			drv->state_count++;
>> +			i++;
>> +		}
>> +		psci_pd_free_states(pd_states, pd_state_count);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int __init psci_pd_init(struct device_node *np)
>>   {
>>   	struct generic_pm_domain *pd;
>> @@ -125,11 +228,14 @@ static int __init psci_pd_init(struct device_node *np)
>>   	 * Parse the domain idle states and let genpd manage the state selection
>>   	 * for those being compatible with "domain-idle-state".
>>   	 */
>> -	ret = psci_pd_parse_states(np, &states, &state_count);
>> -	if (ret)
>> -		goto free_name;
>>
>> -	pd->free_states = psci_pd_free_states;
>> +	if (psci_has_osi_support()) {
>> +		ret = psci_pd_parse_states(np, &states, &state_count);
>> +		if (ret)
>> +			goto free_name;
>> +		pd->free_states = psci_pd_free_states;
>> +	}
>> +
>>   	pd->name = kbasename(pd->name);
>>   	pd->power_off = psci_pd_power_off;
>>   	pd->states = states;
>> @@ -236,10 +342,6 @@ static int __init psci_idle_init_domains(void)
>>   	if (!np)
>>   		return -ENODEV;
>>
>> -	/* Currently limit the hierarchical topology to be used in OSI mode. */
>> -	if (!psci_has_osi_support())
>> -		goto out;
>> -
>>   	/*
>>   	 * Parse child nodes for the "#power-domain-cells" property and
>>   	 * initialize a genpd/genpd-of-provider pair when it's found.
>> @@ -265,14 +367,16 @@ static int __init psci_idle_init_domains(void)
>>   		goto remove_pd;
>>
>>   	/* Try to enable OSI mode. */
>> -	ret = psci_set_osi_mode();
>> -	if (ret) {
>> -		pr_warn("failed to enable OSI mode: %d\n", ret);
>> -		psci_pd_remove_topology(np);
>> -		goto remove_pd;
>> +	if (psci_has_osi_support()) {
>> +		ret = psci_set_osi_mode();
>> +		if (ret) {
>> +			pr_warn("failed to enable OSI mode: %d\n", ret);
>> +			psci_pd_remove_topology(np);
>> +			goto remove_pd;
>> +		} else
>> +			osi_mode_enabled = true;
>>   	}
>>
>> -	osi_mode_enabled = true;
>>   	of_node_put(np);
>>   	pr_info("Initialized CPU PM domain topology\n");
>>   	return pd_count;
>> @@ -293,9 +397,6 @@ struct device __init *psci_dt_attach_cpu(int cpu)
>>   {
>>   	struct device *dev;
>>
>> -	if (!osi_mode_enabled)
>> -		return NULL;
>> -
>>   	dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
>>   	if (IS_ERR_OR_NULL(dev))
>>   		return dev;
>> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
>> index edd7a54..3fa2aee 100644
>> --- a/drivers/cpuidle/cpuidle-psci.c
>> +++ b/drivers/cpuidle/cpuidle-psci.c
>> @@ -49,7 +49,7 @@ static inline int psci_enter_state(int idx, u32 state)
>>   	return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
>>   }
>>
>> -static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
>> +int psci_enter_domain_idle_state(struct cpuidle_device *dev,
>>   					struct cpuidle_driver *drv, int idx)
>>   {
>>   	struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
>> @@ -193,24 +193,29 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
>>   		goto free_mem;
>>   	}
>>
>> -	/* Currently limit the hierarchical topology to be used in OSI mode. */
>> -	if (psci_has_osi_support()) {
>> -		data->dev = psci_dt_attach_cpu(cpu);
>> -		if (IS_ERR(data->dev)) {
>> -			ret = PTR_ERR(data->dev);
>> +	if (!psci_has_osi_support()) {
>> +		ret = psci_dt_pm_domains_parse_states(drv, cpu_node,
>> +					      psci_states);
>> +		if (ret)
>>   			goto free_mem;
>> -		}
>> -
>> -		/*
>> -		 * Using the deepest state for the CPU to trigger a potential
>> -		 * selection of a shared state for the domain, assumes the
>> -		 * domain states are all deeper states.
>> -		 */
>> -		if (data->dev) {
>> -			drv->states[state_count - 1].enter =
>> -				psci_enter_domain_idle_state;
>> -			psci_cpuidle_use_cpuhp = true;
>> -		}
>> +	}
>> +
>> +	data->dev = psci_dt_attach_cpu(cpu);
>> +	if (IS_ERR(data->dev)) {
>> +		ret = PTR_ERR(data->dev);
>> +		goto free_mem;
>> +	}
>> +
>> +	/*
>> +	 * Using the deepest state for the CPU to trigger a potential
>> +	 * selection of a shared state for the domain, assumes the
>> +	 * domain states are all deeper states.
>> +	 */
>> +
>> +	if (data->dev) {
>> +		drv->states[state_count - 1].enter =
>> +			psci_enter_domain_idle_state;
>> +		psci_cpuidle_use_cpuhp = true;
>>   	}
>>
>>   	/* Idle states parsed correctly, store them in the per-cpu struct. */
> --
> Regards,
> Sudeep

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Sudeep Holla]

On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote:
>
> On 2/3/2020 10:38 PM, Sudeep Holla wrote:
> > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
> > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > >
> > > If the hierarchical CPU topology is used, but the OS initiated mode isn't
> > > supported, we need to rely solely on the regular cpuidle framework to
> > > manage the idle state selection, rather than using genpd and its
> > > governor.
> > >
> > > For this reason, introduce a new PSCI DT helper function,
> > > psci_dt_pm_domains_parse_states(), which parses and converts the
> > > hierarchically described domain idle states from DT, into regular flattened
> > > cpuidle states. The converted states are added to the existing cpuidle
> > > driver's array of idle states, which make them available for cpuidle.
> > >
> > And what's the main motivation for this if OSI is not supported in the
> > firmware ?
>
> Hi Sudeep,
>
> Main motivation is to do last-man activities before the CPU cluster can
> enter a deep idle state.
>

Details on those last-man activities will help the discussion. Basically
I am wondering what they are and why they need to done in OSPM ?

> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > [applied to new path, resolved conflicts]
> > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> > > ---
> > >   drivers/cpuidle/cpuidle-psci-domain.c | 137 +++++++++++++++++++++++++++++-----
> > >   drivers/cpuidle/cpuidle-psci.c        |  41 +++++-----
> > >   drivers/cpuidle/cpuidle-psci.h        |  11 +++
> > >   3 files changed, 153 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > > index 423f03b..3c417f7 100644
> > > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > > @@ -26,13 +26,17 @@ struct psci_pd_provider {
> > >   };
> > >
> > >   static LIST_HEAD(psci_pd_providers);
> > > -static bool osi_mode_enabled __initdata;
> > > +static bool osi_mode_enabled;
> > >
> > >   static int psci_pd_power_off(struct generic_pm_domain *pd)
> > >   {
> > >   	struct genpd_power_state *state = &pd->states[pd->state_idx];
> > >   	u32 *pd_state;
> > >
> > > +	/* If we have failed to enable OSI mode, then abort power off. */
> > > +	if ((psci_has_osi_support()) && !osi_mode_enabled)
> > > +		return -EBUSY;
> > > +
> > Why is this needed ? IIUC we don't create genpd domains if OSI is not
> > enabled.
>
> we do create genpd domains, for cpu domains, we just abort power off here
> since idle states are converted into regular flattened mode.
>

OK, IIRC the OSI patches from Ulf didn't add the genpd or rather removed
them in case of any failure to enable OSI. Has that been changed ? If so,
why ?

> however genpd poweroff will be used by parent domain (rsc in this case)
> which is kept in hireachy in DTSI with cluster domain to do last man
> activities.
>

I am bit confused here. Either we do OSI or PC and what you are describing
sounds like a mix-n-match to me and I am totally against it.

--
Regards,
Sudeep

Re: [PATCH v3 6/7] arm64: dts: qcom: sc7180: Add cpuidle low power states

[Matthias Kaehlcke]

On Mon, Feb 03, 2020 at 07:05:39PM +0530, Maulik Shah wrote:
> Add device bindings for cpuidle states for cpu devices.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 78 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 8011c5f..0aa0ced 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -86,6 +86,9 @@
>  			compatible = "arm,armv8";
>  			reg = <0x0 0x0>;
>  			enable-method = "psci";
> +			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
> +					   &LITTLE_CPU_SLEEP_1
> +					   &CLUSTER_SLEEP_0>;

These entries are deleted again by the next patch in this series ('arm64:
dts: qcom: sc7180: Convert to the hierarchical CPU topology layout').
What is the point in adding them in the first place?

Re: [PATCH v3 6/7] arm64: dts: qcom: sc7180: Add cpuidle low power states

[Maulik Shah]

On 2/5/2020 4:45 AM, Matthias Kaehlcke wrote:
> On Mon, Feb 03, 2020 at 07:05:39PM +0530, Maulik Shah wrote:
>> Add device bindings for cpuidle states for cpu devices.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>>   arch/arm64/boot/dts/qcom/sc7180.dtsi | 78 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 78 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index 8011c5f..0aa0ced 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -86,6 +86,9 @@
>>   			compatible = "arm,armv8";
>>   			reg = <0x0 0x0>;
>>   			enable-method = "psci";
>> +			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
>> +					   &LITTLE_CPU_SLEEP_1
>> +					   &CLUSTER_SLEEP_0>;
> These entries are deleted again by the next patch in this series ('arm64:
> dts: qcom: sc7180: Convert to the hierarchical CPU topology layout').
> What is the point in adding them in the first place?

Clubbed 6th and 7th patch in v4 series.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Maulik Shah]

On 2/4/2020 8:51 PM, Sudeep Holla wrote:
> On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote:
>> On 2/3/2020 10:38 PM, Sudeep Holla wrote:
>>> On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>> If the hierarchical CPU topology is used, but the OS initiated mode isn't
>>>> supported, we need to rely solely on the regular cpuidle framework to
>>>> manage the idle state selection, rather than using genpd and its
>>>> governor.
>>>>
>>>> For this reason, introduce a new PSCI DT helper function,
>>>> psci_dt_pm_domains_parse_states(), which parses and converts the
>>>> hierarchically described domain idle states from DT, into regular flattened
>>>> cpuidle states. The converted states are added to the existing cpuidle
>>>> driver's array of idle states, which make them available for cpuidle.
>>>>
>>> And what's the main motivation for this if OSI is not supported in the
>>> firmware ?
>> Hi Sudeep,
>>
>> Main motivation is to do last-man activities before the CPU cluster can
>> enter a deep idle state.
>>
> Details on those last-man activities will help the discussion. Basically
> I am wondering what they are and why they need to done in OSPM ?

Hi Sudeep,

there are cases like,

Last cpu going to deepest idle mode need to lower various resoruce 
requirements (for eg DDR freq).

This is done by calling rpmh_flush which send SLEEP values for various 
shared resources.

>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> [applied to new path, resolved conflicts]
>>>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>>>> ---
>>>>    drivers/cpuidle/cpuidle-psci-domain.c | 137 +++++++++++++++++++++++++++++-----
>>>>    drivers/cpuidle/cpuidle-psci.c        |  41 +++++-----
>>>>    drivers/cpuidle/cpuidle-psci.h        |  11 +++
>>>>    3 files changed, 153 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
>>>> index 423f03b..3c417f7 100644
>>>> --- a/drivers/cpuidle/cpuidle-psci-domain.c
>>>> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
>>>> @@ -26,13 +26,17 @@ struct psci_pd_provider {
>>>>    };
>>>>
>>>>    static LIST_HEAD(psci_pd_providers);
>>>> -static bool osi_mode_enabled __initdata;
>>>> +static bool osi_mode_enabled;
>>>>
>>>>    static int psci_pd_power_off(struct generic_pm_domain *pd)
>>>>    {
>>>>    	struct genpd_power_state *state = &pd->states[pd->state_idx];
>>>>    	u32 *pd_state;
>>>>
>>>> +	/* If we have failed to enable OSI mode, then abort power off. */
>>>> +	if ((psci_has_osi_support()) && !osi_mode_enabled)
>>>> +		return -EBUSY;
>>>> +
>>> Why is this needed ? IIUC we don't create genpd domains if OSI is not
>>> enabled.
>> we do create genpd domains, for cpu domains, we just abort power off here
>> since idle states are converted into regular flattened mode.
>>
> OK, IIRC the OSI patches from Ulf didn't add the genpd or rather removed
> them in case of any failure to enable OSI. Has that been changed ? If so,
> why ?
>
>> however genpd poweroff will be used by parent domain (rsc in this case)
>> which is kept in hireachy in DTSI with cluster domain to do last man
>> activities.
>>
> I am bit confused here. Either we do OSI or PC and what you are describing
> sounds like a mix-n-match to me and I am totally against it.

we still do PC based on sc7180. there is no OSI.

can you please check v4 series, i have cleaned this change by remove 
converter part.

Thanks,

Maulik

>
> --
> Regards,
> Sudeep

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Sudeep Holla]

On Wed, Feb 05, 2020 at 05:53:00PM +0530, Maulik Shah wrote:
>
> On 2/4/2020 8:51 PM, Sudeep Holla wrote:
> > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote:
> > > On 2/3/2020 10:38 PM, Sudeep Holla wrote:
> > > > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
> > > > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > > >
> > > > > If the hierarchical CPU topology is used, but the OS initiated mode isn't
> > > > > supported, we need to rely solely on the regular cpuidle framework to
> > > > > manage the idle state selection, rather than using genpd and its
> > > > > governor.
> > > > >
> > > > > For this reason, introduce a new PSCI DT helper function,
> > > > > psci_dt_pm_domains_parse_states(), which parses and converts the
> > > > > hierarchically described domain idle states from DT, into regular flattened
> > > > > cpuidle states. The converted states are added to the existing cpuidle
> > > > > driver's array of idle states, which make them available for cpuidle.
> > > > >
> > > > And what's the main motivation for this if OSI is not supported in the
> > > > firmware ?
> > > Hi Sudeep,
> > >
> > > Main motivation is to do last-man activities before the CPU cluster can
> > > enter a deep idle state.
> > >
> > Details on those last-man activities will help the discussion. Basically
> > I am wondering what they are and why they need to done in OSPM ?
>
> Hi Sudeep,
>
> there are cases like,
>
> Last cpu going to deepest idle mode need to lower various resoruce
> requirements (for eg DDR freq).
>

In PC mode, only PSCI implementation knows the last man and there shouldn't
be any notion of it in OS. If you need it, you may need OSI. You are still
mixing up the things. NACK for any such approach, sorry.

--
Regards,
Sudeep

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Ulf Hansson]

On Wed, 5 Feb 2020 at 15:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Feb 05, 2020 at 05:53:00PM +0530, Maulik Shah wrote:
> >
> > On 2/4/2020 8:51 PM, Sudeep Holla wrote:
> > > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote:
> > > > On 2/3/2020 10:38 PM, Sudeep Holla wrote:
> > > > > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
> > > > > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > >
> > > > > > If the hierarchical CPU topology is used, but the OS initiated mode isn't
> > > > > > supported, we need to rely solely on the regular cpuidle framework to
> > > > > > manage the idle state selection, rather than using genpd and its
> > > > > > governor.
> > > > > >
> > > > > > For this reason, introduce a new PSCI DT helper function,
> > > > > > psci_dt_pm_domains_parse_states(), which parses and converts the
> > > > > > hierarchically described domain idle states from DT, into regular flattened
> > > > > > cpuidle states. The converted states are added to the existing cpuidle
> > > > > > driver's array of idle states, which make them available for cpuidle.
> > > > > >
> > > > > And what's the main motivation for this if OSI is not supported in the
> > > > > firmware ?
> > > > Hi Sudeep,
> > > >
> > > > Main motivation is to do last-man activities before the CPU cluster can
> > > > enter a deep idle state.
> > > >
> > > Details on those last-man activities will help the discussion. Basically
> > > I am wondering what they are and why they need to done in OSPM ?
> >
> > Hi Sudeep,
> >
> > there are cases like,
> >
> > Last cpu going to deepest idle mode need to lower various resoruce
> > requirements (for eg DDR freq).
> >
>
> In PC mode, only PSCI implementation knows the last man and there shouldn't
> be any notion of it in OS. If you need it, you may need OSI. You are still
> mixing up the things. NACK for any such approach, sorry.

Sudeep, I don't quite agree with your NACK to this. At least not yet. :-)

I do agree that the best suited solution seems to be OSI, as to
support this kind of SoC requirements.

However, if for some reason the PC mode is being used, we could still
allow Linux to control "last-man activities" as it knows what each CPU
has voted for when going idle. Yes, the PSCI FW decides in the end,
but that doesn't really matter. Or is there another technical reason
to why you object?

As a matter of fact, if we allow support for PC mode with
"last-man-activities", it would allow us to make a fair
performance/energy comparison between the two PSCI CPU suspend modes,
for the same SoC. I would be thrilled about looking into doing such
tests, I bet you are as well!?

Kind regards
Uffe

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Sudeep Holla]

On Wed, Feb 05, 2020 at 04:55:17PM +0100, Ulf Hansson wrote:
> On Wed, 5 Feb 2020 at 15:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, Feb 05, 2020 at 05:53:00PM +0530, Maulik Shah wrote:
> > >
> > > On 2/4/2020 8:51 PM, Sudeep Holla wrote:
> > > > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote:
> > > > > On 2/3/2020 10:38 PM, Sudeep Holla wrote:
> > > > > > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
> > > > > > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > >
> > > > > > > If the hierarchical CPU topology is used, but the OS initiated mode isn't
> > > > > > > supported, we need to rely solely on the regular cpuidle framework to
> > > > > > > manage the idle state selection, rather than using genpd and its
> > > > > > > governor.
> > > > > > >
> > > > > > > For this reason, introduce a new PSCI DT helper function,
> > > > > > > psci_dt_pm_domains_parse_states(), which parses and converts the
> > > > > > > hierarchically described domain idle states from DT, into regular flattened
> > > > > > > cpuidle states. The converted states are added to the existing cpuidle
> > > > > > > driver's array of idle states, which make them available for cpuidle.
> > > > > > >
> > > > > > And what's the main motivation for this if OSI is not supported in the
> > > > > > firmware ?
> > > > > Hi Sudeep,
> > > > >
> > > > > Main motivation is to do last-man activities before the CPU cluster can
> > > > > enter a deep idle state.
> > > > >
> > > > Details on those last-man activities will help the discussion. Basically
> > > > I am wondering what they are and why they need to done in OSPM ?
> > >
> > > Hi Sudeep,
> > >
> > > there are cases like,
> > >
> > > Last cpu going to deepest idle mode need to lower various resoruce
> > > requirements (for eg DDR freq).
> > >
> >
> > In PC mode, only PSCI implementation knows the last man and there shouldn't
> > be any notion of it in OS. If you need it, you may need OSI. You are still
> > mixing up the things. NACK for any such approach, sorry.
>
> Sudeep, I don't quite agree with your NACK to this. At least not yet. :-)
>

OK, I am not surprised :-)

> I do agree that the best suited solution seems to be OSI, as to
> support this kind of SoC requirements.
>

That's the main point. We need to draw some line as what we want to do
with PC and OSI mode. If we plan to take up all last man responsibility
in the kernel, what's the point in not supporting OSI in the firmware
then ? I can't buy it yet.

> However, if for some reason the PC mode is being used, we could still
> allow Linux to control "last-man activities" as it knows what each CPU
> has voted for when going idle. Yes, the PSCI FW decides in the end,
> but that doesn't really matter. Or is there another technical reason
> to why you object?
>

Precisely, FW decides and let it. Just because we can do in the kernel
doesn't mean we must do it. It's clear in the spec and doing it in the
kernel will be sub-optimal if PSCI f/w aborted entering the deeper
state that required some action in the first place.

> As a matter of fact, if we allow support for PC mode with
> "last-man-activities", it would allow us to make a fair
> performance/energy comparison between the two PSCI CPU suspend modes,
> for the same SoC. I would be thrilled about looking into doing such
> tests, I bet you are as well!?
>

I was, but not anymore, especially if we want such changes in the kernel
to do so.

Just use OSI as that was the point of adding all these after years of
discussion claiming it's more optimal compared to PC. Now telling that
you need more changes to compare it with PC just doesn't make any sense
at all to me.

--
Regards,
Sudeep

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Ulf Hansson]

On Wed, 5 Feb 2020 at 17:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Feb 05, 2020 at 04:55:17PM +0100, Ulf Hansson wrote:
> > On Wed, 5 Feb 2020 at 15:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Wed, Feb 05, 2020 at 05:53:00PM +0530, Maulik Shah wrote:
> > > >
> > > > On 2/4/2020 8:51 PM, Sudeep Holla wrote:
> > > > > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote:
> > > > > > On 2/3/2020 10:38 PM, Sudeep Holla wrote:
> > > > > > > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
> > > > > > > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > >
> > > > > > > > If the hierarchical CPU topology is used, but the OS initiated mode isn't
> > > > > > > > supported, we need to rely solely on the regular cpuidle framework to
> > > > > > > > manage the idle state selection, rather than using genpd and its
> > > > > > > > governor.
> > > > > > > >
> > > > > > > > For this reason, introduce a new PSCI DT helper function,
> > > > > > > > psci_dt_pm_domains_parse_states(), which parses and converts the
> > > > > > > > hierarchically described domain idle states from DT, into regular flattened
> > > > > > > > cpuidle states. The converted states are added to the existing cpuidle
> > > > > > > > driver's array of idle states, which make them available for cpuidle.
> > > > > > > >
> > > > > > > And what's the main motivation for this if OSI is not supported in the
> > > > > > > firmware ?
> > > > > > Hi Sudeep,
> > > > > >
> > > > > > Main motivation is to do last-man activities before the CPU cluster can
> > > > > > enter a deep idle state.
> > > > > >
> > > > > Details on those last-man activities will help the discussion. Basically
> > > > > I am wondering what they are and why they need to done in OSPM ?
> > > >
> > > > Hi Sudeep,
> > > >
> > > > there are cases like,
> > > >
> > > > Last cpu going to deepest idle mode need to lower various resoruce
> > > > requirements (for eg DDR freq).
> > > >
> > >
> > > In PC mode, only PSCI implementation knows the last man and there shouldn't
> > > be any notion of it in OS. If you need it, you may need OSI. You are still
> > > mixing up the things. NACK for any such approach, sorry.
> >
> > Sudeep, I don't quite agree with your NACK to this. At least not yet. :-)
> >
>
> OK, I am not surprised :-)

Apologize for troubling you again. :-)

>
> > I do agree that the best suited solution seems to be OSI, as to
> > support this kind of SoC requirements.
> >
>
> That's the main point. We need to draw some line as what we want to do
> with PC and OSI mode. If we plan to take up all last man responsibility
> in the kernel, what's the point in not supporting OSI in the firmware
> then ? I can't buy it yet.
>
> > However, if for some reason the PC mode is being used, we could still
> > allow Linux to control "last-man activities" as it knows what each CPU
> > has voted for when going idle. Yes, the PSCI FW decides in the end,
> > but that doesn't really matter. Or is there another technical reason
> > to why you object?
> >
>
> Precisely, FW decides and let it. Just because we can do in the kernel
> doesn't mean we must do it. It's clear in the spec and doing it in the
> kernel will be sub-optimal if PSCI f/w aborted entering the deeper
> state that required some action in the first place.

Yes, it may be suboptimal for PC-mode.

On the other hand, we already fire CPU PM notifiers while exit/enter
idle states (except for WFI). Those may also be suboptimal for kind of
the similar reasons.

Maybe it's not the best argument, but it sounds like allowing us to
control cluster power on/off notifications for last-man activities,
would just conform to the similar behaviour we already have. No?

>
> > As a matter of fact, if we allow support for PC mode with
> > "last-man-activities", it would allow us to make a fair
> > performance/energy comparison between the two PSCI CPU suspend modes,
> > for the same SoC. I would be thrilled about looking into doing such
> > tests, I bet you are as well!?
> >
>
> I was, but not anymore, especially if we want such changes in the kernel
> to do so.
>
> Just use OSI as that was the point of adding all these after years of
> discussion claiming it's more optimal compared to PC. Now telling that
> you need more changes to compare it with PC just doesn't make any sense
> at all to me.

Fair enough.

I was just pondering over if there are other reasons to why we may want this.

One other thing that could be problematic to support, is when are
other resources, I/O controllers for example, sharing the same power
rail as a cluster. When such controller is in use, idle states of the
cluster must be prevented. Without using genpd to model the CPU
topology, it may be difficult to deal with this.

Of course, using PC mode when trying to deal with this
platform/board-requirement would also be suboptimal. In other words,
your argument about when using OSI vs PC mode, still stands.

Kind regards
Uffe

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Lina Iyer]

On Thu, Feb 06 2020 at 01:46 -0700, Ulf Hansson wrote:
>On Wed, 5 Feb 2020 at 17:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>> On Wed, Feb 05, 2020 at 04:55:17PM +0100, Ulf Hansson wrote:
>> > On Wed, 5 Feb 2020 at 15:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> > >
>> > > On Wed, Feb 05, 2020 at 05:53:00PM +0530, Maulik Shah wrote:
>> > > >
>> > > > On 2/4/2020 8:51 PM, Sudeep Holla wrote:
>> > > > > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote:
>> > > > > > On 2/3/2020 10:38 PM, Sudeep Holla wrote:
>> > > > > > > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
>> > > > > > > > From: Ulf Hansson <ulf.hansson@linaro.org>
>> > > > > > > >
>> I was, but not anymore, especially if we want such changes in the kernel
>> to do so.
>>
>> Just use OSI as that was the point of adding all these after years of
>> discussion claiming it's more optimal compared to PC. Now telling that
>> you need more changes to compare it with PC just doesn't make any sense
>> at all to me.
>
>Fair enough.
>
>I was just pondering over if there are other reasons to why we may want this.
>
>One other thing that could be problematic to support, is when are
>other resources, I/O controllers for example, sharing the same power
>rail as a cluster. When such controller is in use, idle states of the
>cluster must be prevented. Without using genpd to model the CPU
>topology, it may be difficult to deal with this.
>
>Of course, using PC mode when trying to deal with this
>platform/board-requirement would also be suboptimal. In other words,
>your argument about when using OSI vs PC mode, still stands.
>
I understand the arguments for using PC vs OSI and agree with it. But
what in PSCI is against Linux knowing when the last core is powering
down when the PSCI is configured to do only Platform Cordinated.
There should not be any objection to drivers knowing when all the cores
are powered down, be it reference counting CPU PM notifications or using
a cleaner approach like this where GendPD framwork does everything
cleanly and gives a nice callback. ARM architecture allows for different
aspects of CPU access be handled at different levels. I see this as an
extension of that approach.

-- Lina

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Bjorn Andersson]

On Wed 05 Feb 06:06 PST 2020, Sudeep Holla wrote:

> On Wed, Feb 05, 2020 at 05:53:00PM +0530, Maulik Shah wrote:
> >
> > On 2/4/2020 8:51 PM, Sudeep Holla wrote:
> > > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote:
> > > > On 2/3/2020 10:38 PM, Sudeep Holla wrote:
> > > > > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
> > > > > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > >
> > > > > > If the hierarchical CPU topology is used, but the OS initiated mode isn't
> > > > > > supported, we need to rely solely on the regular cpuidle framework to
> > > > > > manage the idle state selection, rather than using genpd and its
> > > > > > governor.
> > > > > >
> > > > > > For this reason, introduce a new PSCI DT helper function,
> > > > > > psci_dt_pm_domains_parse_states(), which parses and converts the
> > > > > > hierarchically described domain idle states from DT, into regular flattened
> > > > > > cpuidle states. The converted states are added to the existing cpuidle
> > > > > > driver's array of idle states, which make them available for cpuidle.
> > > > > >
> > > > > And what's the main motivation for this if OSI is not supported in the
> > > > > firmware ?
> > > > Hi Sudeep,
> > > >
> > > > Main motivation is to do last-man activities before the CPU cluster can
> > > > enter a deep idle state.
> > > >
> > > Details on those last-man activities will help the discussion. Basically
> > > I am wondering what they are and why they need to done in OSPM ?
> >
> > Hi Sudeep,
> >
> > there are cases like,
> >
> > Last cpu going to deepest idle mode need to lower various resoruce
> > requirements (for eg DDR freq).
> >
> 
> In PC mode, only PSCI implementation knows the last man and there shouldn't
> be any notion of it in OS. If you need it, you may need OSI. You are still
> mixing up the things. NACK for any such approach, sorry.
> 

Forgive me if I'm misunderstanding PSCI's role here, but doesn't it deal
with the power management of the "processor subsystem" in the SoC?


In the Qualcomm platforms most resources (voltage rails, clocks, etc)
are controlled through a power controller that provides controls for a
state when the CPU subsystem is running and when it's asleep. This
allows non-CPU-related device to control if resources that are shared
with the CPU subsystem should be kept on when the last CPU/cluster goes
down.

An example of this would be the display controller voting to keep a
voltage rail on after the CPU subsystem collapses, because the display
is still on.

But as long as the CPU subsystem is running it will keep these resources
available and there's no need to change these votes (e.g. if the display
is turned on and off while the CPU is active the sleep-requests cancels
out), so they are simply cached/batched up in the RPMh driver and what
Maulik's series is attempting to do is to flush the cached values when
Linux believes that the firmware might decide to enter a lower power
state.

Regards,
Bjorn

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Sudeep Holla]

On Thu, Feb 06, 2020 at 01:45:14PM -0700, Lina Iyer wrote:
> On Thu, Feb 06 2020 at 01:46 -0700, Ulf Hansson wrote:
> > On Wed, 5 Feb 2020 at 17:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > 
> > > On Wed, Feb 05, 2020 at 04:55:17PM +0100, Ulf Hansson wrote:
> > > > On Wed, 5 Feb 2020 at 15:06, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > On Wed, Feb 05, 2020 at 05:53:00PM +0530, Maulik Shah wrote:
> > > > > >
> > > > > > On 2/4/2020 8:51 PM, Sudeep Holla wrote:
> > > > > > > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote:
> > > > > > > > On 2/3/2020 10:38 PM, Sudeep Holla wrote:
> > > > > > > > > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
> > > > > > > > > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > > > > >
> > > I was, but not anymore, especially if we want such changes in the kernel
> > > to do so.
> > > 
> > > Just use OSI as that was the point of adding all these after years of
> > > discussion claiming it's more optimal compared to PC. Now telling that
> > > you need more changes to compare it with PC just doesn't make any sense
> > > at all to me.
> > 
> > Fair enough.
> > 
> > I was just pondering over if there are other reasons to why we may want this.
> > 
> > One other thing that could be problematic to support, is when are
> > other resources, I/O controllers for example, sharing the same power
> > rail as a cluster. When such controller is in use, idle states of the
> > cluster must be prevented. Without using genpd to model the CPU
> > topology, it may be difficult to deal with this.
> > 
> > Of course, using PC mode when trying to deal with this
> > platform/board-requirement would also be suboptimal. In other words,
> > your argument about when using OSI vs PC mode, still stands.
> > 
> I understand the arguments for using PC vs OSI and agree with it. But
> what in PSCI is against Linux knowing when the last core is powering
> down when the PSCI is configured to do only Platform Cordinated.

Nothing :D. But knowing the evolution and reasons for adding OSI in the
PSCI specification and having argued about benefits of OSI over PC for
years and finally when we have it in mainline, this argument of using
PC for exact reasons why OSI evolved is something I can't understand
and I am confused.

> There should not be any objection to drivers knowing when all the cores
> are powered down, be it reference counting CPU PM notifications or using
> a cleaner approach like this where GendPD framwork does everything
> cleanly and gives a nice callback. ARM architecture allows for different
> aspects of CPU access be handled at different levels. I see this as an
> extension of that approach.
>

One thing that was repeatedly pointed out during OSI patch review was no
extra overhead for PC mode where firmware can make decisions. So, just
use OSI now and let us be done with this discussion of OSI vs PC. If PC
is what you think you need for future, we can revert all OSI changes and
start discussing again :-)

--
Regards,
Sudeep

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Sudeep Holla]

On Thu, Feb 06, 2020 at 01:11:33PM -0800, Bjorn Andersson wrote:
> On Wed 05 Feb 06:06 PST 2020, Sudeep Holla wrote:
>
> > On Wed, Feb 05, 2020 at 05:53:00PM +0530, Maulik Shah wrote:
> > >
> > > On 2/4/2020 8:51 PM, Sudeep Holla wrote:
> > > > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote:
> > > > > On 2/3/2020 10:38 PM, Sudeep Holla wrote:
> > > > > > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
> > > > > > > From: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > >
> > > > > > > If the hierarchical CPU topology is used, but the OS initiated mode isn't
> > > > > > > supported, we need to rely solely on the regular cpuidle framework to
> > > > > > > manage the idle state selection, rather than using genpd and its
> > > > > > > governor.
> > > > > > >
> > > > > > > For this reason, introduce a new PSCI DT helper function,
> > > > > > > psci_dt_pm_domains_parse_states(), which parses and converts the
> > > > > > > hierarchically described domain idle states from DT, into regular flattened
> > > > > > > cpuidle states. The converted states are added to the existing cpuidle
> > > > > > > driver's array of idle states, which make them available for cpuidle.
> > > > > > >
> > > > > > And what's the main motivation for this if OSI is not supported in the
> > > > > > firmware ?
> > > > > Hi Sudeep,
> > > > >
> > > > > Main motivation is to do last-man activities before the CPU cluster can
> > > > > enter a deep idle state.
> > > > >
> > > > Details on those last-man activities will help the discussion. Basically
> > > > I am wondering what they are and why they need to done in OSPM ?
> > >
> > > Hi Sudeep,
> > >
> > > there are cases like,
> > >
> > > Last cpu going to deepest idle mode need to lower various resoruce
> > > requirements (for eg DDR freq).
> > >
> >
> > In PC mode, only PSCI implementation knows the last man and there shouldn't
> > be any notion of it in OS. If you need it, you may need OSI. You are still
> > mixing up the things. NACK for any such approach, sorry.
> >
>
> Forgive me if I'm misunderstanding PSCI's role here, but doesn't it deal
> with the power management of the "processor subsystem" in the SoC?
>

Yes.

> In the Qualcomm platforms most resources (voltage rails, clocks, etc)
> are controlled through a power controller that provides controls for a
> state when the CPU subsystem is running and when it's asleep. This
> allows non-CPU-related device to control if resources that are shared
> with the CPU subsystem should be kept on when the last CPU/cluster goes
> down.
>

I understand that.

> An example of this would be the display controller voting to keep a
> voltage rail on after the CPU subsystem collapses, because the display
> is still on.
>

OK

> But as long as the CPU subsystem is running it will keep these resources
> available and there's no need to change these votes (e.g. if the display
> is turned on and off while the CPU is active the sleep-requests cancels
> out), so they are simply cached/batched up in the RPMh driver and what
> Maulik's series is attempting to do is to flush the cached values when
> Linux believes that the firmware might decide to enter a lower power
> state.
>

I understand all these. What I am arguing is that in PC mode, PSCI
firmware is the one who needs to vote and not OSPM because it is
responsible for pulling the plugs off the CPU/Cluster. So lets us not
bring that to OSPM. OSI was invented to do all such crazy things in OSPM,
please feel free to play with that ;-)

--
Regards,
Sudeep

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Ulf Hansson]

[...]

> > I understand the arguments for using PC vs OSI and agree with it. But
> > what in PSCI is against Linux knowing when the last core is powering
> > down when the PSCI is configured to do only Platform Cordinated.
>
> Nothing :D. But knowing the evolution and reasons for adding OSI in the
> PSCI specification and having argued about benefits of OSI over PC for
> years and finally when we have it in mainline, this argument of using
> PC for exact reasons why OSI evolved is something I can't understand
> and I am confused.
>
> > There should not be any objection to drivers knowing when all the cores
> > are powered down, be it reference counting CPU PM notifications or using
> > a cleaner approach like this where GendPD framwork does everything
> > cleanly and gives a nice callback. ARM architecture allows for different
> > aspects of CPU access be handled at different levels. I see this as an
> > extension of that approach.
> >
>
> One thing that was repeatedly pointed out during OSI patch review was no
> extra overhead for PC mode where firmware can make decisions. So, just
> use OSI now and let us be done with this discussion of OSI vs PC. If PC
> is what you think you need for future, we can revert all OSI changes and
> start discussing again :-)

Just to make it clear, I fully agree with you in regards to overhead
for PC-mode. This is especially critical for ARM SoCs with lots of
cores, I assume.

However, the overhead you refer to, is *only* going to be present in
case when the DTS has the hierarchical CPU topology description with
"power-domains". Because, that is *optional* to use, I am expecting
only those SoC/platforms that needs to manage last-man activities to
use this layout, the others will remain unaffected.

That said, does that address your concern?

Kind regards
Uffe

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Lorenzo Pieralisi]

On Fri, Feb 07, 2020 at 01:32:28PM +0100, Ulf Hansson wrote:
> [...]
> 
> > > I understand the arguments for using PC vs OSI and agree with it. But
> > > what in PSCI is against Linux knowing when the last core is powering
> > > down when the PSCI is configured to do only Platform Cordinated.
> >
> > Nothing :D. But knowing the evolution and reasons for adding OSI in the
> > PSCI specification and having argued about benefits of OSI over PC for
> > years and finally when we have it in mainline, this argument of using
> > PC for exact reasons why OSI evolved is something I can't understand
> > and I am confused.
> >
> > > There should not be any objection to drivers knowing when all the cores
> > > are powered down, be it reference counting CPU PM notifications or using
> > > a cleaner approach like this where GendPD framwork does everything
> > > cleanly and gives a nice callback. ARM architecture allows for different
> > > aspects of CPU access be handled at different levels. I see this as an
> > > extension of that approach.
> > >
> >
> > One thing that was repeatedly pointed out during OSI patch review was no
> > extra overhead for PC mode where firmware can make decisions. So, just
> > use OSI now and let us be done with this discussion of OSI vs PC. If PC
> > is what you think you need for future, we can revert all OSI changes and
> > start discussing again :-)
> 
> Just to make it clear, I fully agree with you in regards to overhead
> for PC-mode. This is especially critical for ARM SoCs with lots of
> cores, I assume.
> 
> However, the overhead you refer to, is *only* going to be present in
> case when the DTS has the hierarchical CPU topology description with
> "power-domains". Because, that is *optional* to use, I am expecting
> only those SoC/platforms that needs to manage last-man activities to
> use this layout, the others will remain unaffected.

In PC mode not only there is no need but it is wrong to manage
any last-man activity in the kernel. I wonder why we are still
talking about this to be honest.

Code to handle PSCI platform coordinated mode has been/is in
the kernel today and that's all is needed according to the PSCI
specifications.

Thanks,
Lorenzo

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Ulf Hansson]

On Fri, 7 Feb 2020 at 15:48, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Fri, Feb 07, 2020 at 01:32:28PM +0100, Ulf Hansson wrote:
> > [...]
> >
> > > > I understand the arguments for using PC vs OSI and agree with it. But
> > > > what in PSCI is against Linux knowing when the last core is powering
> > > > down when the PSCI is configured to do only Platform Cordinated.
> > >
> > > Nothing :D. But knowing the evolution and reasons for adding OSI in the
> > > PSCI specification and having argued about benefits of OSI over PC for
> > > years and finally when we have it in mainline, this argument of using
> > > PC for exact reasons why OSI evolved is something I can't understand
> > > and I am confused.
> > >
> > > > There should not be any objection to drivers knowing when all the cores
> > > > are powered down, be it reference counting CPU PM notifications or using
> > > > a cleaner approach like this where GendPD framwork does everything
> > > > cleanly and gives a nice callback. ARM architecture allows for different
> > > > aspects of CPU access be handled at different levels. I see this as an
> > > > extension of that approach.
> > > >
> > >
> > > One thing that was repeatedly pointed out during OSI patch review was no
> > > extra overhead for PC mode where firmware can make decisions. So, just
> > > use OSI now and let us be done with this discussion of OSI vs PC. If PC
> > > is what you think you need for future, we can revert all OSI changes and
> > > start discussing again :-)
> >
> > Just to make it clear, I fully agree with you in regards to overhead
> > for PC-mode. This is especially critical for ARM SoCs with lots of
> > cores, I assume.
> >
> > However, the overhead you refer to, is *only* going to be present in
> > case when the DTS has the hierarchical CPU topology description with
> > "power-domains". Because, that is *optional* to use, I am expecting
> > only those SoC/platforms that needs to manage last-man activities to
> > use this layout, the others will remain unaffected.
>
> In PC mode not only there is no need but it is wrong to manage
> any last-man activity in the kernel. I wonder why we are still
> talking about this to be honest.

I guess the discussion is here because there is a use case to consider now.

For sure, we agree on what is the best solution. But this is rather
about what can we do to improve the current situation, if we should do
anything.

>
> Code to handle PSCI platform coordinated mode has been/is in
> the kernel today and that's all is needed according to the PSCI
> specifications.

PSCI specifies CPU power management, not SoC power management. If
these things were completely decoupled, I would agree with you, but
that's not the case. Maybe SCMI, etc, helps with this in future.

Anyway, my fear is that not many ARM vendors implements OSI support,
but still they have "last-man-activities" to deal with. This is not
only QCOM.

I guess an option would be to add OSI support to the public ARM
Trusted Firmware, then we could more easily point to that - rather
than trying to mitigate the problem on the kernel side.

Kind regards
Uffe

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Sudeep Holla]

On Fri, Feb 07, 2020 at 01:32:28PM +0100, Ulf Hansson wrote:
> [...]
>
> > > I understand the arguments for using PC vs OSI and agree with it. But
> > > what in PSCI is against Linux knowing when the last core is powering
> > > down when the PSCI is configured to do only Platform Cordinated.
> >
> > Nothing :D. But knowing the evolution and reasons for adding OSI in the
> > PSCI specification and having argued about benefits of OSI over PC for
> > years and finally when we have it in mainline, this argument of using
> > PC for exact reasons why OSI evolved is something I can't understand
> > and I am confused.
> >
> > > There should not be any objection to drivers knowing when all the cores
> > > are powered down, be it reference counting CPU PM notifications or using
> > > a cleaner approach like this where GendPD framwork does everything
> > > cleanly and gives a nice callback. ARM architecture allows for different
> > > aspects of CPU access be handled at different levels. I see this as an
> > > extension of that approach.
> > >
> >
> > One thing that was repeatedly pointed out during OSI patch review was no
> > extra overhead for PC mode where firmware can make decisions. So, just
> > use OSI now and let us be done with this discussion of OSI vs PC. If PC
> > is what you think you need for future, we can revert all OSI changes and
> > start discussing again :-)
>
> Just to make it clear, I fully agree with you in regards to overhead
> for PC-mode. This is especially critical for ARM SoCs with lots of
> cores, I assume.
>
> However, the overhead you refer to, is *only* going to be present in
> case when the DTS has the hierarchical CPU topology description with
> "power-domains". Because, that is *optional* to use, I am expecting
> only those SoC/platforms that needs to manage last-man activities to
> use this layout, the others will remain unaffected.
>
> That said, does that address your concern?
>

I have already expressed my view and concerns in response to Lina and
Bjorn's emails.

--
Regards,
Sudeep

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Sudeep Holla]

On Fri, Feb 07, 2020 at 04:52:52PM +0100, Ulf Hansson wrote:
> On Fri, 7 Feb 2020 at 15:48, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Fri, Feb 07, 2020 at 01:32:28PM +0100, Ulf Hansson wrote:
> > > [...]
> > >
> > > > > I understand the arguments for using PC vs OSI and agree with it. But
> > > > > what in PSCI is against Linux knowing when the last core is powering
> > > > > down when the PSCI is configured to do only Platform Cordinated.
> > > >
> > > > Nothing :D. But knowing the evolution and reasons for adding OSI in the
> > > > PSCI specification and having argued about benefits of OSI over PC for
> > > > years and finally when we have it in mainline, this argument of using
> > > > PC for exact reasons why OSI evolved is something I can't understand
> > > > and I am confused.
> > > >
> > > > > There should not be any objection to drivers knowing when all the cores
> > > > > are powered down, be it reference counting CPU PM notifications or using
> > > > > a cleaner approach like this where GendPD framwork does everything
> > > > > cleanly and gives a nice callback. ARM architecture allows for different
> > > > > aspects of CPU access be handled at different levels. I see this as an
> > > > > extension of that approach.
> > > > >
> > > >
> > > > One thing that was repeatedly pointed out during OSI patch review was no
> > > > extra overhead for PC mode where firmware can make decisions. So, just
> > > > use OSI now and let us be done with this discussion of OSI vs PC. If PC
> > > > is what you think you need for future, we can revert all OSI changes and
> > > > start discussing again :-)
> > >
> > > Just to make it clear, I fully agree with you in regards to overhead
> > > for PC-mode. This is especially critical for ARM SoCs with lots of
> > > cores, I assume.
> > >
> > > However, the overhead you refer to, is *only* going to be present in
> > > case when the DTS has the hierarchical CPU topology description with
> > > "power-domains". Because, that is *optional* to use, I am expecting
> > > only those SoC/platforms that needs to manage last-man activities to
> > > use this layout, the others will remain unaffected.
> >
> > In PC mode not only there is no need but it is wrong to manage
> > any last-man activity in the kernel. I wonder why we are still
> > talking about this to be honest.
>
> I guess the discussion is here because there is a use case to consider now.
>

If this is what Bjorn presented in his email, I have responded to that.
If it's any different, please let us know the complete details.

> For sure, we agree on what is the best solution. But this is rather
> about what can we do to improve the current situation, if we should do
> anything.
>

Sure, and I haven't found a reason to do that in OSPM yet(as part of the
discussion in this thread)

> >
> > Code to handle PSCI platform coordinated mode has been/is in
> > the kernel today and that's all is needed according to the PSCI
> > specifications.
>
> PSCI specifies CPU power management, not SoC power management. If
> these things were completely decoupled, I would agree with you, but
> that's not the case. Maybe SCMI, etc, helps with this in future.
>

Why does that not work even if they are not decoupled. The IO/device
that share with CPU votes from OSPM and the CPU/Cluster from PSCI in
PC mode. There is no argument there, but why it needs to be done in OSPM
is the objection here.

> Anyway, my fear is that not many ARM vendors implements OSI support,
> but still they have "last-man-activities" to deal with. This is not
> only QCOM.
>

I am interested to hear from them. And the same question to same too as
above.

> I guess an option would be to add OSI support to the public ARM
> Trusted Firmware, then we could more easily point to that - rather
> than trying to mitigate the problem on the kernel side.
>

I would say go for it. But don't mix responsibility of OSPM in PC vs OSI.
We have discussed this for years and I hope this discussion ends ASAP.
I don't see any point in dragging this any further.

--
Regards,
Sudeep

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Ulf Hansson]

On Fri, 7 Feb 2020 at 17:15, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Feb 07, 2020 at 04:52:52PM +0100, Ulf Hansson wrote:
> > On Fri, 7 Feb 2020 at 15:48, Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > >
> > > On Fri, Feb 07, 2020 at 01:32:28PM +0100, Ulf Hansson wrote:
> > > > [...]
> > > >
> > > > > > I understand the arguments for using PC vs OSI and agree with it. But
> > > > > > what in PSCI is against Linux knowing when the last core is powering
> > > > > > down when the PSCI is configured to do only Platform Cordinated.
> > > > >
> > > > > Nothing :D. But knowing the evolution and reasons for adding OSI in the
> > > > > PSCI specification and having argued about benefits of OSI over PC for
> > > > > years and finally when we have it in mainline, this argument of using
> > > > > PC for exact reasons why OSI evolved is something I can't understand
> > > > > and I am confused.
> > > > >
> > > > > > There should not be any objection to drivers knowing when all the cores
> > > > > > are powered down, be it reference counting CPU PM notifications or using
> > > > > > a cleaner approach like this where GendPD framwork does everything
> > > > > > cleanly and gives a nice callback. ARM architecture allows for different
> > > > > > aspects of CPU access be handled at different levels. I see this as an
> > > > > > extension of that approach.
> > > > > >
> > > > >
> > > > > One thing that was repeatedly pointed out during OSI patch review was no
> > > > > extra overhead for PC mode where firmware can make decisions. So, just
> > > > > use OSI now and let us be done with this discussion of OSI vs PC. If PC
> > > > > is what you think you need for future, we can revert all OSI changes and
> > > > > start discussing again :-)
> > > >
> > > > Just to make it clear, I fully agree with you in regards to overhead
> > > > for PC-mode. This is especially critical for ARM SoCs with lots of
> > > > cores, I assume.
> > > >
> > > > However, the overhead you refer to, is *only* going to be present in
> > > > case when the DTS has the hierarchical CPU topology description with
> > > > "power-domains". Because, that is *optional* to use, I am expecting
> > > > only those SoC/platforms that needs to manage last-man activities to
> > > > use this layout, the others will remain unaffected.
> > >
> > > In PC mode not only there is no need but it is wrong to manage
> > > any last-man activity in the kernel. I wonder why we are still
> > > talking about this to be honest.
> >
> > I guess the discussion is here because there is a use case to consider now.
> >
>
> If this is what Bjorn presented in his email, I have responded to that.
> If it's any different, please let us know the complete details.
>
> > For sure, we agree on what is the best solution. But this is rather
> > about what can we do to improve the current situation, if we should do
> > anything.
> >
>
> Sure, and I haven't found a reason to do that in OSPM yet(as part of the
> discussion in this thread)
>
> > >
> > > Code to handle PSCI platform coordinated mode has been/is in
> > > the kernel today and that's all is needed according to the PSCI
> > > specifications.
> >
> > PSCI specifies CPU power management, not SoC power management. If
> > these things were completely decoupled, I would agree with you, but
> > that's not the case. Maybe SCMI, etc, helps with this in future.
> >
>
> Why does that not work even if they are not decoupled. The IO/device
> that share with CPU votes from OSPM and the CPU/Cluster from PSCI in
> PC mode. There is no argument there, but why it needs to be done in OSPM
> is the objection here.

That implies the votes from I/O devices needs to reach the FW
immediately when the vote is done. No caching or other optimizations
can be done at OSPM.

In principle, the FW needs to have an always up to date view of the
votes, etc. That sounds highly inefficient, both from energy and
latency point of view, at least in my opinion.

>
> > Anyway, my fear is that not many ARM vendors implements OSI support,
> > but still they have "last-man-activities" to deal with. This is not
> > only QCOM.
> >
>
> I am interested to hear from them. And the same question to same too as
> above.

I have been talking to some of them. But, yes, we need to hear more from them.

>
> > I guess an option would be to add OSI support to the public ARM
> > Trusted Firmware, then we could more easily point to that - rather
> > than trying to mitigate the problem on the kernel side.
> >
>
> I would say go for it. But don't mix responsibility of OSPM in PC vs OSI.
> We have discussed this for years and I hope this discussion ends ASAP.
> I don't see any point in dragging this any further.

Okay.

KInd regards
Uffe

Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter

[Sudeep Holla]

On Sat, Feb 08, 2020 at 11:25:18AM +0100, Ulf Hansson wrote:
> On Fri, 7 Feb 2020 at 17:15, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Fri, Feb 07, 2020 at 04:52:52PM +0100, Ulf Hansson wrote:
> > > On Fri, 7 Feb 2020 at 15:48, Lorenzo Pieralisi
> > > <lorenzo.pieralisi@arm.com> wrote:
> > > >
> > > > On Fri, Feb 07, 2020 at 01:32:28PM +0100, Ulf Hansson wrote:
> > > > > [...]
> > > > >
> > > > > > > I understand the arguments for using PC vs OSI and agree with it. But
> > > > > > > what in PSCI is against Linux knowing when the last core is powering
> > > > > > > down when the PSCI is configured to do only Platform Cordinated.
> > > > > >
> > > > > > Nothing :D. But knowing the evolution and reasons for adding OSI in the
> > > > > > PSCI specification and having argued about benefits of OSI over PC for
> > > > > > years and finally when we have it in mainline, this argument of using
> > > > > > PC for exact reasons why OSI evolved is something I can't understand
> > > > > > and I am confused.
> > > > > >
> > > > > > > There should not be any objection to drivers knowing when all the cores
> > > > > > > are powered down, be it reference counting CPU PM notifications or using
> > > > > > > a cleaner approach like this where GendPD framwork does everything
> > > > > > > cleanly and gives a nice callback. ARM architecture allows for different
> > > > > > > aspects of CPU access be handled at different levels. I see this as an
> > > > > > > extension of that approach.
> > > > > > >
> > > > > >
> > > > > > One thing that was repeatedly pointed out during OSI patch review was no
> > > > > > extra overhead for PC mode where firmware can make decisions. So, just
> > > > > > use OSI now and let us be done with this discussion of OSI vs PC. If PC
> > > > > > is what you think you need for future, we can revert all OSI changes and
> > > > > > start discussing again :-)
> > > > >
> > > > > Just to make it clear, I fully agree with you in regards to overhead
> > > > > for PC-mode. This is especially critical for ARM SoCs with lots of
> > > > > cores, I assume.
> > > > >
> > > > > However, the overhead you refer to, is *only* going to be present in
> > > > > case when the DTS has the hierarchical CPU topology description with
> > > > > "power-domains". Because, that is *optional* to use, I am expecting
> > > > > only those SoC/platforms that needs to manage last-man activities to
> > > > > use this layout, the others will remain unaffected.
> > > >
> > > > In PC mode not only there is no need but it is wrong to manage
> > > > any last-man activity in the kernel. I wonder why we are still
> > > > talking about this to be honest.
> > >
> > > I guess the discussion is here because there is a use case to consider now.
> > >
> >
> > If this is what Bjorn presented in his email, I have responded to that.
> > If it's any different, please let us know the complete details.
> >
> > > For sure, we agree on what is the best solution. But this is rather
> > > about what can we do to improve the current situation, if we should do
> > > anything.
> > >
> >
> > Sure, and I haven't found a reason to do that in OSPM yet(as part of the
> > discussion in this thread)
> >
> > > >
> > > > Code to handle PSCI platform coordinated mode has been/is in
> > > > the kernel today and that's all is needed according to the PSCI
> > > > specifications.
> > >
> > > PSCI specifies CPU power management, not SoC power management. If
> > > these things were completely decoupled, I would agree with you, but
> > > that's not the case. Maybe SCMI, etc, helps with this in future.
> > >
> >
> > Why does that not work even if they are not decoupled. The IO/device
> > that share with CPU votes from OSPM and the CPU/Cluster from PSCI in
> > PC mode. There is no argument there, but why it needs to be done in OSPM
> > is the objection here.
>
> That implies the votes from I/O devices needs to reach the FW
> immediately when the vote is done. No caching or other optimizations
> can be done at OSPM.
>
> In principle, the FW needs to have an always up to date view of the
> votes, etc. That sounds highly inefficient, both from energy and
> latency point of view, at least in my opinion.
>

Sorry but I need to re-iterate, use OSI if you need all those fancy
caching and other optimizations.

> >
> > > Anyway, my fear is that not many ARM vendors implements OSI support,
> > > but still they have "last-man-activities" to deal with. This is not
> > > only QCOM.
> > >
> >
> > I am interested to hear from them. And the same question to same too as
> > above.
>
> I have been talking to some of them. But, yes, we need to hear more from them.
>
> >
> > > I guess an option would be to add OSI support to the public ARM
> > > Trusted Firmware, then we could more easily point to that - rather
> > > than trying to mitigate the problem on the kernel side.
> > >
> >
> > I would say go for it. But don't mix responsibility of OSPM in PC vs OSI.
> > We have discussed this for years and I hope this discussion ends ASAP.
> > I don't see any point in dragging this any further.
>
> Okay.
>

I keep saying that but still responding to the discussions. I must stop ;-)

--
Regards,
Sudeep