[PATCH RFC v1 0/8] drivers: qcom: Add cpu power domain for SDM845

[PATCH RFC v1 0/8] drivers: qcom: Add cpu power domain for SDM845

[Raju P.L.S.S.S.N]

Changes in v2:
 - Create a cpu pm domain for CPUs based on Ulf Hansson's patches[5][6].
   The reference count to determine the last CPU in the system to enter
   low power mode would be taken care by genpd. The domain power off
   callback is used to perform the last-man activities.

Please note: the first three patches in this series  are taken from Ulf
Hansoon's series[8] and included here for context and completeness of
present series. They would be merged as part of their series[8].

Hi,

This is an attempt at a solution to perform activities necessary while
entering deeper low power modes supported by firmware for QCOM SoCs which
have hardened IP (Resource Power Manager Hardened - RPMH) for shared
resource management.

The shared resources that are no longer used, when processor enters deep
idle states, can be turned off or put to lower state via sleep requests.
Clients vote for lower resource state when application processor is asleep.
These votes need to be flushed only during entry to low power modes.

In addition to this, an always-on power domain wake-up timer present in PDC
(Power Domain Controller)  needs to be programmed so that RSC is up and
running by the time CPU has to wake-up. 

The kernel does not notify that the CPU powering down is the last CPU.
Therefore, in this version, we are using genpd framework based on Ulf
Hansoon's approach for reference counting to determine the last CPU in
the system to enter idle and use that to perform the last-man activities.
It would be optimal to do this than to flush whenever a core enters idle.
The current approach can be revisited in future if OS-initiated support
becomes available that enables certain actions to be taken when last core
enters deepest low power mode.

Please review these patches. Your inputs would be greatly appreciated.

Thanks,
Raju

---
Dependencies:

The current series depends on patches[1][2][3][4], which add RPMH
communication support, to send shared resource request votes by clients.
The patches[1][2][3][4] also provide functions that are expected to be
called by domain manager during low power mode entry. Apart from RPMH
related patches, the current series depends on attaching CPU devices to
genpd framework[5][6] & knowing the next wake up timer of a CPU[7] for
programing the always-on timer present in PDC


[1]. https://patchwork.kernel.org/patch/10589385/
[2]. https://patchwork.kernel.org/patch/10575721/
[3]. https://patchwork.kernel.org/project/linux-arm-msm/list/?series=26079
[4]. https://patchwork.kernel.org/project/linux-arm-msm/list/?series=28381
[5]. https://patchwork.kernel.org/patch/10478167/
[6]. https://patchwork.kernel.org/patch/10478153/
[7]. https://patchwork.kernel.org/patch/10478021/
[8]. https://lkml.org/lkml/2018/6/20/807

Lina Iyer (1):
  timer: Export next wakeup time of a CPU

Raju P.L.S.S.S.N (5):
  drivers: qcom: cpu_pd: add cpu power domain support using genpd
  dt-bindings: introduce cpu power domain bindings for Qualcomm SoCs
  drivers: qcom: cpu_pd: program next wakeup to PDC timer
  drivers: qcom: cpu_pd: Handle cpu hotplug in the domain
  arm64: dtsi: sdm845: Add cpu power domain support

Ulf Hansson (2):
  PM / Domains: Add helper functions to attach/detach CPUs to/from genpd
  kernel/cpu_pm: Manage runtime PM in the idle path for CPUs

 .../bindings/soc/qcom/cpu_power_domain.txt         |  39 ++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi               |  13 ++
 drivers/base/power/domain.c                        |  70 ++++++++
 drivers/soc/qcom/Kconfig                           |   9 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/cpu_pd.c                          | 200 +++++++++++++++++++++
 include/linux/cpuhotplug.h                         |   1 +
 include/linux/pm_domain.h                          |   9 +
 include/linux/tick.h                               |   8 +
 kernel/cpu_pm.c                                    |  11 ++
 kernel/time/tick-sched.c                           |  10 ++
 11 files changed, 371 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt
 create mode 100644 drivers/soc/qcom/cpu_pd.c

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

[PATCH RFC v1 1/8] PM / Domains: Add helper functions to attach/detach CPUs to/from genpd

[Raju P.L.S.S.S.N]

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

Introduce two new genpd helper functions, of_genpd_attach|detach_cpu(),
which takes the CPU-number as an in-parameter.

To attach a CPU to a genpd, of_genpd_attach_cpu() starts by fetching the
struct device belonging to the CPU. Then it calls genpd_dev_pm_attach(),
which via DT tries to hook up the CPU device to its corresponding PM
domain. If it succeeds, of_genpd_attach_cpu() continues to prepare/enable
runtime PM of the device.

To detach a CPU from its PM domain, of_genpd_attach_cpu() reverse the
operations made from of_genpd_attach_cpu(). However, first it checks that
the CPU device has a valid PM domain pointer assigned, as to make sure it
belongs to genpd.

Cc: Lina Iyer <ilina@codeaurora.org>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
[rplsssn@codeaurora.org: Fix compilation issue]
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
(am from https://patchwork.kernel.org/patch/10478167/)
---
 drivers/base/power/domain.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  9 ++++++
 2 files changed, 79 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4b57141..82973e3 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -6,6 +6,7 @@
  * This file is released under the GPLv2.
  */
 
+#include <linux/cpu.h>
 #include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
@@ -2398,6 +2399,75 @@ struct device *genpd_dev_pm_attach_by_name(struct device *dev, char *name)
 	return genpd_dev_pm_attach_by_id(dev, index);
 }
 
+/*
+ * of_genpd_attach_cpu() - Attach a CPU to its PM domain
+ * @cpu: The CPU to be attached.
+ *
+ * Parses the OF node of the CPU's device, to find a PM domain specifier. If
+ * such is found, attaches the CPU's device to the retrieved pm_domain ops and
+ * enables runtime PM for it. This to allow the CPU to be power managed through
+ * its PM domain.
+ *
+ * Returns zero when successfully attached the CPU's device to its PM domain,
+ * else a negative error code.
+ */
+int of_genpd_attach_cpu(int cpu)
+{
+	struct device *dev = get_cpu_device(cpu);
+	int ret;
+
+	if (!dev) {
+		pr_warn("genpd: no dev for cpu%d\n", cpu);
+		return -ENODEV;
+	}
+
+	ret = genpd_dev_pm_attach(dev);
+	if (ret != 1) {
+		dev_warn(dev, "genpd: attach cpu failed %d\n", ret);
+		return ret < 0 ? ret : -ENODEV;
+	}
+
+	pm_runtime_irq_safe(dev);
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	dev_info(dev, "genpd: attached cpu\n");
+	return 0;
+}
+EXPORT_SYMBOL(of_genpd_attach_cpu);
+
+/**
+ * of_genpd_detach_cpu() - Detach a CPU from its PM domain
+ * @cpu: The CPU to be detached.
+ *
+ * Detach the CPU's device from its corresponding PM domain. If detaching is
+ * completed successfully, disable runtime PM and restore the runtime PM usage
+ * count for the CPU's device.
+ */
+void of_genpd_detach_cpu(int cpu)
+{
+	struct device *dev = get_cpu_device(cpu);
+
+	if (!dev) {
+		pr_warn("genpd: no dev for cpu%d\n", cpu);
+		return;
+	}
+
+	/* Check that the device is attached to a genpd. */
+	if (!(dev->pm_domain && dev->pm_domain->detach == genpd_dev_pm_detach))
+		return;
+
+	genpd_dev_pm_detach(dev, true);
+
+	pm_runtime_disable(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_reinit(dev);
+
+	dev_info(dev, "genpd: detached cpu\n");
+}
+EXPORT_SYMBOL(of_genpd_detach_cpu);
+
 static const struct of_device_id idle_state_match[] = {
 	{ .compatible = "domain-idle-state", },
 	{ }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 776c546..48850af 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -241,6 +241,8 @@ struct device *genpd_dev_pm_attach_by_id(struct device *dev,
 					 unsigned int index);
 struct device *genpd_dev_pm_attach_by_name(struct device *dev,
 					   char *name);
+int of_genpd_attach_cpu(int cpu);
+void of_genpd_detach_cpu(int cpu);
 #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
 static inline int of_genpd_add_provider_simple(struct device_node *np,
 					struct generic_pm_domain *genpd)
@@ -298,6 +300,13 @@ static inline struct device *genpd_dev_pm_attach_by_name(struct device *dev,
 	return NULL;
 }
 
+static inline int of_genpd_attach_cpu(int cpu)
+{
+	return -ENODEV;
+}
+
+static inline void of_genpd_detach_cpu(int cpu) {}
+
 static inline
 struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
 {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

[PATCH RFC v1 2/8] kernel/cpu_pm: Manage runtime PM in the idle path for CPUs

[Raju P.L.S.S.S.N]

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

To allow CPUs being power managed by PM domains, let's deploy support for
runtime PM for the CPU's corresponding struct device.

More precisely, at the point when the CPU is about to enter an idle state,
decrease the runtime PM usage count for its corresponding struct device,
via calling pm_runtime_put_sync_suspend(). Then, at the point when the CPU
resumes from idle, let's increase the runtime PM usage count, via calling
pm_runtime_get_sync().

Cc: Lina Iyer <ilina@codeaurora.org>
Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
(am from https://patchwork.kernel.org/patch/10478153/)
---
 kernel/cpu_pm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
index 67b02e1..492d4a8 100644
--- a/kernel/cpu_pm.c
+++ b/kernel/cpu_pm.c
@@ -16,9 +16,11 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/cpu.h>
 #include <linux/cpu_pm.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
+#include <linux/pm_runtime.h>
 #include <linux/spinlock.h>
 #include <linux/syscore_ops.h>
 
@@ -91,6 +93,7 @@ int cpu_pm_enter(void)
 {
 	int nr_calls;
 	int ret = 0;
+	struct device *dev = get_cpu_device(smp_processor_id());
 
 	ret = cpu_pm_notify(CPU_PM_ENTER, -1, &nr_calls);
 	if (ret)
@@ -100,6 +103,9 @@ int cpu_pm_enter(void)
 		 */
 		cpu_pm_notify(CPU_PM_ENTER_FAILED, nr_calls - 1, NULL);
 
+	if (!ret && dev && dev->pm_domain)
+		pm_runtime_put_sync_suspend(dev);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cpu_pm_enter);
@@ -118,6 +124,11 @@ int cpu_pm_enter(void)
  */
 int cpu_pm_exit(void)
 {
+	struct device *dev = get_cpu_device(smp_processor_id());
+
+	if (dev && dev->pm_domain)
+		pm_runtime_get_sync(dev);
+
 	return cpu_pm_notify(CPU_PM_EXIT, -1, NULL);
 }
 EXPORT_SYMBOL_GPL(cpu_pm_exit);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH RFC v1 2/8] kernel/cpu_pm: Manage runtime PM in the idle path for CPUs

[Rafael J. Wysocki]

On Wednesday, October 10, 2018 11:20:49 PM CEST Raju P.L.S.S.S.N wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
> 
> To allow CPUs being power managed by PM domains, let's deploy support for
> runtime PM for the CPU's corresponding struct device.
> 
> More precisely, at the point when the CPU is about to enter an idle state,
> decrease the runtime PM usage count for its corresponding struct device,
> via calling pm_runtime_put_sync_suspend(). Then, at the point when the CPU
> resumes from idle, let's increase the runtime PM usage count, via calling
> pm_runtime_get_sync().
> 
> Cc: Lina Iyer <ilina@codeaurora.org>
> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> (am from https://patchwork.kernel.org/patch/10478153/)
> ---
>  kernel/cpu_pm.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
> index 67b02e1..492d4a8 100644
> --- a/kernel/cpu_pm.c
> +++ b/kernel/cpu_pm.c
> @@ -16,9 +16,11 @@
>   */
>  
>  #include <linux/kernel.h>
> +#include <linux/cpu.h>
>  #include <linux/cpu_pm.h>
>  #include <linux/module.h>
>  #include <linux/notifier.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/spinlock.h>
>  #include <linux/syscore_ops.h>
>  
> @@ -91,6 +93,7 @@ int cpu_pm_enter(void)
>  {
>  	int nr_calls;
>  	int ret = 0;
> +	struct device *dev = get_cpu_device(smp_processor_id());
>  
>  	ret = cpu_pm_notify(CPU_PM_ENTER, -1, &nr_calls);
>  	if (ret)
> @@ -100,6 +103,9 @@ int cpu_pm_enter(void)
>  		 */
>  		cpu_pm_notify(CPU_PM_ENTER_FAILED, nr_calls - 1, NULL);
>  
> +	if (!ret && dev && dev->pm_domain)
> +		pm_runtime_put_sync_suspend(dev);

This may cause a power domain to go off, but if it goes off, then the idle
governor has already selected idle states for all of the CPUs in that domain.

Is there any way to ensure that turning the domain off (and later on) will
no cause the target residency and exit latency expectations for those idle
states to be exceeded?

> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(cpu_pm_enter);
> @@ -118,6 +124,11 @@ int cpu_pm_enter(void)
>   */
>  int cpu_pm_exit(void)
>  {
> +	struct device *dev = get_cpu_device(smp_processor_id());
> +
> +	if (dev && dev->pm_domain)
> +		pm_runtime_get_sync(dev);
> +
>  	return cpu_pm_notify(CPU_PM_EXIT, -1, NULL);
>  }
>  EXPORT_SYMBOL_GPL(cpu_pm_exit);
> 

Re: [PATCH RFC v1 2/8] kernel/cpu_pm: Manage runtime PM in the idle path for CPUs

[Lina Iyer]

On Thu, Oct 11 2018 at 14:56 -0600, Rafael J. Wysocki wrote:
>On Wednesday, October 10, 2018 11:20:49 PM CEST Raju P.L.S.S.S.N wrote:
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> To allow CPUs being power managed by PM domains, let's deploy support for
>> runtime PM for the CPU's corresponding struct device.
>>
>> More precisely, at the point when the CPU is about to enter an idle state,
>> decrease the runtime PM usage count for its corresponding struct device,
>> via calling pm_runtime_put_sync_suspend(). Then, at the point when the CPU
>> resumes from idle, let's increase the runtime PM usage count, via calling
>> pm_runtime_get_sync().
>>
>> Cc: Lina Iyer <ilina@codeaurora.org>
>> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
>> (am from https://patchwork.kernel.org/patch/10478153/)
>> ---
>>  kernel/cpu_pm.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
>> index 67b02e1..492d4a8 100644
>> --- a/kernel/cpu_pm.c
>> +++ b/kernel/cpu_pm.c
>> @@ -16,9 +16,11 @@
>>   */
>>
>>  #include <linux/kernel.h>
>> +#include <linux/cpu.h>
>>  #include <linux/cpu_pm.h>
>>  #include <linux/module.h>
>>  #include <linux/notifier.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/syscore_ops.h>
>>
>> @@ -91,6 +93,7 @@ int cpu_pm_enter(void)
>>  {
>>  	int nr_calls;
>>  	int ret = 0;
>> +	struct device *dev = get_cpu_device(smp_processor_id());
>>
>>  	ret = cpu_pm_notify(CPU_PM_ENTER, -1, &nr_calls);
>>  	if (ret)
>> @@ -100,6 +103,9 @@ int cpu_pm_enter(void)
>>  		 */
>>  		cpu_pm_notify(CPU_PM_ENTER_FAILED, nr_calls - 1, NULL);
>>
>> +	if (!ret && dev && dev->pm_domain)
>> +		pm_runtime_put_sync_suspend(dev);
>
>This may cause a power domain to go off, but if it goes off, then the idle
>governor has already selected idle states for all of the CPUs in that domain.
>
>Is there any way to ensure that turning the domain off (and later on) will
>no cause the target residency and exit latency expectations for those idle
>states to be exceeded?
>
Good point.

The cluster states should account for that additional latency. Just the
CPU's power down states need not care about that.

But, it would be nice if the PM domain governor could be cognizant of
the idle state chosen for each CPU, that way we dont configure the
domain to be powered off when the CPUs have just chosen to power down
(not chosen a cluster state). I think that is a whole different topic to
discuss.

-- Lina

>> +
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(cpu_pm_enter);
>> @@ -118,6 +124,11 @@ int cpu_pm_enter(void)
>>   */
>>  int cpu_pm_exit(void)
>>  {
>> +	struct device *dev = get_cpu_device(smp_processor_id());
>> +
>> +	if (dev && dev->pm_domain)
>> +		pm_runtime_get_sync(dev);
>> +
>>  	return cpu_pm_notify(CPU_PM_EXIT, -1, NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(cpu_pm_exit);
>>
>
>

Re: [PATCH RFC v1 2/8] kernel/cpu_pm: Manage runtime PM in the idle path for CPUs

[Rafael J. Wysocki]

On Fri, Oct 12, 2018 at 12:08 AM Lina Iyer <ilina@codeaurora.org> wrote:
>
> On Thu, Oct 11 2018 at 14:56 -0600, Rafael J. Wysocki wrote:
> >On Wednesday, October 10, 2018 11:20:49 PM CEST Raju P.L.S.S.S.N wrote:
> >> From: Ulf Hansson <ulf.hansson@linaro.org>
> >>
> >> To allow CPUs being power managed by PM domains, let's deploy support for
> >> runtime PM for the CPU's corresponding struct device.
> >>
> >> More precisely, at the point when the CPU is about to enter an idle state,
> >> decrease the runtime PM usage count for its corresponding struct device,
> >> via calling pm_runtime_put_sync_suspend(). Then, at the point when the CPU
> >> resumes from idle, let's increase the runtime PM usage count, via calling
> >> pm_runtime_get_sync().
> >>
> >> Cc: Lina Iyer <ilina@codeaurora.org>
> >> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> >> (am from https://patchwork.kernel.org/patch/10478153/)
> >> ---
> >>  kernel/cpu_pm.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
> >> index 67b02e1..492d4a8 100644
> >> --- a/kernel/cpu_pm.c
> >> +++ b/kernel/cpu_pm.c
> >> @@ -16,9 +16,11 @@
> >>   */
> >>
> >>  #include <linux/kernel.h>
> >> +#include <linux/cpu.h>
> >>  #include <linux/cpu_pm.h>
> >>  #include <linux/module.h>
> >>  #include <linux/notifier.h>
> >> +#include <linux/pm_runtime.h>
> >>  #include <linux/spinlock.h>
> >>  #include <linux/syscore_ops.h>
> >>
> >> @@ -91,6 +93,7 @@ int cpu_pm_enter(void)
> >>  {
> >>      int nr_calls;
> >>      int ret = 0;
> >> +    struct device *dev = get_cpu_device(smp_processor_id());
> >>
> >>      ret = cpu_pm_notify(CPU_PM_ENTER, -1, &nr_calls);
> >>      if (ret)
> >> @@ -100,6 +103,9 @@ int cpu_pm_enter(void)
> >>               */
> >>              cpu_pm_notify(CPU_PM_ENTER_FAILED, nr_calls - 1, NULL);
> >>
> >> +    if (!ret && dev && dev->pm_domain)
> >> +            pm_runtime_put_sync_suspend(dev);
> >
> >This may cause a power domain to go off, but if it goes off, then the idle
> >governor has already selected idle states for all of the CPUs in that domain.
> >
> >Is there any way to ensure that turning the domain off (and later on) will
> >no cause the target residency and exit latency expectations for those idle
> >states to be exceeded?
> >
> Good point.
>
> The cluster states should account for that additional latency.

But even then, you need to be sure that the idle governor selected
"cluster" states for all of the CPUs in the cluster.  It might select
WFI for one of them for reasons unrelated to the distance to the next
timer (so to speak), for example.

> Just the CPU's power down states need not care about that.

The meaning of this sentence isn't particularly clear to me. :-)

> But, it would be nice if the PM domain governor could be cognizant of
> the idle state chosen for each CPU, that way we dont configure the
> domain to be powered off when the CPUs have just chosen to power down
> (not chosen a cluster state). I think that is a whole different topic to
> discuss.

This needs to be sorted out before the approach becomes viable, though.

Basically, the domain governor needs to track what the idle governor
did for all of the CPUs in the domain and only let the domain go off
if the latency matches all of the states selected by the idle
governor.  Otherwise the idle governor's assumptions would be violated
and it would become essentially useless overhead.

Thanks,
Rafael

Re: [PATCH RFC v1 2/8] kernel/cpu_pm: Manage runtime PM in the idle path for CPUs

[Ulf Hansson]

On 12 October 2018 at 09:43, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Oct 12, 2018 at 12:08 AM Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> On Thu, Oct 11 2018 at 14:56 -0600, Rafael J. Wysocki wrote:
>> >On Wednesday, October 10, 2018 11:20:49 PM CEST Raju P.L.S.S.S.N wrote:
>> >> From: Ulf Hansson <ulf.hansson@linaro.org>
>> >>
>> >> To allow CPUs being power managed by PM domains, let's deploy support for
>> >> runtime PM for the CPU's corresponding struct device.
>> >>
>> >> More precisely, at the point when the CPU is about to enter an idle state,
>> >> decrease the runtime PM usage count for its corresponding struct device,
>> >> via calling pm_runtime_put_sync_suspend(). Then, at the point when the CPU
>> >> resumes from idle, let's increase the runtime PM usage count, via calling
>> >> pm_runtime_get_sync().
>> >>
>> >> Cc: Lina Iyer <ilina@codeaurora.org>
>> >> Co-developed-by: Lina Iyer <lina.iyer@linaro.org>
>> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
>> >> (am from https://patchwork.kernel.org/patch/10478153/)
>> >> ---
>> >>  kernel/cpu_pm.c | 11 +++++++++++
>> >>  1 file changed, 11 insertions(+)
>> >>
>> >> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
>> >> index 67b02e1..492d4a8 100644
>> >> --- a/kernel/cpu_pm.c
>> >> +++ b/kernel/cpu_pm.c
>> >> @@ -16,9 +16,11 @@
>> >>   */
>> >>
>> >>  #include <linux/kernel.h>
>> >> +#include <linux/cpu.h>
>> >>  #include <linux/cpu_pm.h>
>> >>  #include <linux/module.h>
>> >>  #include <linux/notifier.h>
>> >> +#include <linux/pm_runtime.h>
>> >>  #include <linux/spinlock.h>
>> >>  #include <linux/syscore_ops.h>
>> >>
>> >> @@ -91,6 +93,7 @@ int cpu_pm_enter(void)
>> >>  {
>> >>      int nr_calls;
>> >>      int ret = 0;
>> >> +    struct device *dev = get_cpu_device(smp_processor_id());
>> >>
>> >>      ret = cpu_pm_notify(CPU_PM_ENTER, -1, &nr_calls);
>> >>      if (ret)
>> >> @@ -100,6 +103,9 @@ int cpu_pm_enter(void)
>> >>               */
>> >>              cpu_pm_notify(CPU_PM_ENTER_FAILED, nr_calls - 1, NULL);
>> >>
>> >> +    if (!ret && dev && dev->pm_domain)
>> >> +            pm_runtime_put_sync_suspend(dev);
>> >
>> >This may cause a power domain to go off, but if it goes off, then the idle
>> >governor has already selected idle states for all of the CPUs in that domain.
>> >
>> >Is there any way to ensure that turning the domain off (and later on) will
>> >no cause the target residency and exit latency expectations for those idle
>> >states to be exceeded?
>> >
>> Good point.
>>
>> The cluster states should account for that additional latency.
>
> But even then, you need to be sure that the idle governor selected
> "cluster" states for all of the CPUs in the cluster.  It might select
> WFI for one of them for reasons unrelated to the distance to the next
> timer (so to speak), for example.

The approach here is that cpu_pm_enter isn't called for WFI, hence
there is no pm_runtime_get|put() done for a CPU going to WFI. In other
words, the cluster will never be powered off/on if there is any CPU in
the cluster in WFI.

>
>> Just the CPU's power down states need not care about that.
>
> The meaning of this sentence isn't particularly clear to me. :-)
>
>> But, it would be nice if the PM domain governor could be cognizant of
>> the idle state chosen for each CPU, that way we dont configure the
>> domain to be powered off when the CPUs have just chosen to power down
>> (not chosen a cluster state). I think that is a whole different topic to
>> discuss.
>
> This needs to be sorted out before the approach becomes viable, though.
>
> Basically, the domain governor needs to track what the idle governor
> did for all of the CPUs in the domain and only let the domain go off
> if the latency matches all of the states selected by the idle
> governor.  Otherwise the idle governor's assumptions would be violated
> and it would become essentially useless overhead.

That is correct!

The reason why it works in the current approach, is because there is
only one additional CPU idle state besides WFI. Hence
pm_runtime_get|put() is correctly called, as it's done only when the
cpuidle-governor has picked the deepest idle state for the CPU.

To solve this in the long run (where CPUs have > 1 idle state besides
WFI), I think there are two options.

1)
We either have to select the idle state of the CPU (and not only the
cluster) as part of the genpd and the genpd governors. This makes
genpd in charge and can thus decide internally what idle state that
should be selected, even hierarchically. Then it becomes a matter of
how to share code and interact between cpuidle/cpuidle-governors and
genpd.

2)
Leave genpd and the genpd governor to deal with cluster idle states,
but not the CPU idle states, as is the suggested approach. Then, to
solve the problem you pointed out, we need to provide the cpuidle
driver with information about which of the available idle state(s) it
should call pm_runtime_get|put() for, as to allow cluster idle
management through genpd.

I am currently looking at option 2), as I think it requires less
changes and I guess it's better to move slowly forward.

Does it make sense?

Kind regards
Uffe

Re: [PATCH RFC v1 2/8] kernel/cpu_pm: Manage runtime PM in the idle path for CPUs

[Lina Iyer]

On Fri, Oct 12 2018 at 01:43 -0600, Rafael J. Wysocki wrote:
>On Fri, Oct 12, 2018 at 12:08 AM Lina Iyer <ilina@codeaurora.org> wrote:
>> On Thu, Oct 11 2018 at 14:56 -0600, Rafael J. Wysocki wrote:
>> >On Wednesday, October 10, 2018 11:20:49 PM CEST Raju P.L.S.S.S.N wrote:
>> >> From: Ulf Hansson <ulf.hansson@linaro.org>

>> The cluster states should account for that additional latency.
>
>But even then, you need to be sure that the idle governor selected
>"cluster" states for all of the CPUs in the cluster.  It might select
>WFI for one of them for reasons unrelated to the distance to the next
>timer (so to speak), for example.
>
Well, if cpuidle chooses WFI, cpu_pm_enter() will not be called. So for
that case we are okay with this approach.

>> Just the CPU's power down states need not care about that.
>
>The meaning of this sentence isn't particularly clear to me. :-)
>
What I meant to say is that if cpuidle chooses a CPU only power down
state, then, atleast in ARM architecture, we would not choose to power
down the cluster in the firmware. To power down the cluster in the
firmware, all CPUs need to choose a cluster state, which would account
the additional latency of powering off and on the domain.

How I ever thought that I could convey this point in that line is beyond
me now. Sorry!

>> But, it would be nice if the PM domain governor could be cognizant of
>> the idle state chosen for each CPU, that way we dont configure the
>> domain to be powered off when the CPUs have just chosen to power down
>> (not chosen a cluster state). I think that is a whole different topic to
>> discuss.
>
>This needs to be sorted out before the approach becomes viable, though.
>
We embarked on that discussion a few years ago, but realized that there
is a lot more complexity involved in specifying that especially with DT.
I believe ACPI has a way to specify this. But DT and driver code
currently don't have a nice way to propagate this requirement to the
domain governor. So we shelved it for the future.

>Basically, the domain governor needs to track what the idle governor
>did for all of the CPUs in the domain and only let the domain go off
>if the latency matches all of the states selected by the idle
>governor.  Otherwise the idle governor's assumptions would be violated
>and it would become essentially useless overhead.
>
Well, we kinda do that in the CPU PM domain governor. By looking at the
next wakeup and the latency/QoS requirement of each CPU in the domain,
we determine if the domain can be powered off. But, if we were to do
this by correlating domain idle states to that of the required CPU idle
state, then a lot needs to plumbed in to the cpuidle and driver model.
The current approach is rather simple while meeting most of the
requirement.

Thanks,
Lina

[PATCH RFC v1 3/8] timer: Export next wakeup time of a CPU

[Raju P.L.S.S.S.N]

From: Lina Iyer <lina.iyer@linaro.org>

Knowing the sleep duration of CPUs, is known to be needed while selecting
the most energy efficient idle state for a CPU or a group of CPUs.

However, to be able to compute the sleep duration, we need to know at what
time the next expected wakeup is for the CPU. Therefore, let's export this
information via a new function, tick_nohz_get_next_wakeup(). Following
changes make use of it.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Lina Iyer <ilina@codeaurora.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Co-developed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
(am from https://patchwork.kernel.org/patch/10478021/)
---
 include/linux/tick.h     |  8 ++++++++
 kernel/time/tick-sched.c | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 55388ab..e48f6b2 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -125,6 +125,7 @@ enum tick_dep_bits {
 extern ktime_t tick_nohz_get_sleep_length(ktime_t *delta_next);
 extern unsigned long tick_nohz_get_idle_calls(void);
 extern unsigned long tick_nohz_get_idle_calls_cpu(int cpu);
+extern ktime_t tick_nohz_get_next_wakeup(int cpu);
 extern u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time);
 extern u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time);
 
@@ -151,6 +152,13 @@ static inline ktime_t tick_nohz_get_sleep_length(ktime_t *delta_next)
 	*delta_next = TICK_NSEC;
 	return *delta_next;
 }
+
+static inline ktime_t tick_nohz_get_next_wakeup(int cpu)
+{
+	/* Next wake up is the tick period, assume it starts now */
+	return ktime_add(ktime_get(), TICK_NSEC);
+}
+
 static inline u64 get_cpu_idle_time_us(int cpu, u64 *unused) { return -1; }
 static inline u64 get_cpu_iowait_time_us(int cpu, u64 *unused) { return -1; }
 
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 5b33e2f..bca95f9 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1089,6 +1089,16 @@ unsigned long tick_nohz_get_idle_calls(void)
 	return ts->idle_calls;
 }
 
+/**
+ * tick_nohz_get_next_wakeup - return the next wake up of the CPU
+ */
+ktime_t tick_nohz_get_next_wakeup(int cpu)
+{
+	struct clock_event_device *dev = per_cpu(tick_cpu_device.evtdev, cpu);
+
+	return dev->next_event;
+}
+
 static void tick_nohz_account_idle_ticks(struct tick_sched *ts)
 {
 #ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH RFC v1 3/8] timer: Export next wakeup time of a CPU

[Thomas Gleixner]

Raju,

On Thu, 11 Oct 2018, Raju P.L.S.S.S.N wrote:
>  
> +/**
> + * tick_nohz_get_next_wakeup - return the next wake up of the CPU

Lacks documentation of @cpu. Please include kernel docs into your test
builds.

> + */
> +ktime_t tick_nohz_get_next_wakeup(int cpu)
> +{
> +	struct clock_event_device *dev = per_cpu(tick_cpu_device.evtdev, cpu);
> +
> +	return dev->next_event;
> +}

Thanks,

	tglx

Re: [PATCH RFC v1 3/8] timer: Export next wakeup time of a CPU

[Ulf Hansson]

On 29 October 2018 at 23:36, Thomas Gleixner <tglx@linutronix.de> wrote:
> Raju,
>
> On Thu, 11 Oct 2018, Raju P.L.S.S.S.N wrote:
>>
>> +/**
>> + * tick_nohz_get_next_wakeup - return the next wake up of the CPU
>
> Lacks documentation of @cpu. Please include kernel docs into your test
> builds.

Good point!

>
>> + */
>> +ktime_t tick_nohz_get_next_wakeup(int cpu)
>> +{
>> +     struct clock_event_device *dev = per_cpu(tick_cpu_device.evtdev, cpu);
>> +
>> +     return dev->next_event;
>> +}

Thanks for your review!

Kind regards
Uffe

[PATCH RFC v1 4/8] drivers: qcom: cpu_pd: add cpu power domain support using genpd

[Raju P.L.S.S.S.N]

RPMH based targets require that the sleep and wake state request votes
be sent during system low power mode entry. The votes help reduce the
power consumption when the AP is not using them. The votes sent by the
clients are cached in RPMH controller and needs to be flushed when the
last cpu enters low power mode. So add cpu power domain using Linux
generic power domain infrastructure to perform necessary tasks as part
of domain power down.

Suggested-by: Lina Iyer <ilina@codeaurora.org>
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
---
 drivers/soc/qcom/Kconfig  |   9 ++++
 drivers/soc/qcom/Makefile |   1 +
 drivers/soc/qcom/cpu_pd.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 drivers/soc/qcom/cpu_pd.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index ba79b60..91e8b3b 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -95,6 +95,7 @@ config QCOM_RMTFS_MEM
 config QCOM_RPMH
 	bool "Qualcomm RPM-Hardened (RPMH) Communication"
 	depends on ARCH_QCOM && ARM64 && OF || COMPILE_TEST
+	select QCOM_CPU_PD
 	help
 	  Support for communication with the hardened-RPM blocks in
 	  Qualcomm Technologies Inc (QTI) SoCs. RPMH communication uses an
@@ -102,6 +103,14 @@ config QCOM_RPMH
 	  of hardware components aggregate requests for these resources and
 	  help apply the aggregated state on the resource.
 
+config QCOM_CPU_PD
+    bool "Qualcomm cpu power domain driver"
+    depends on QCOM_RPMH && PM_GENERIC_DOMAINS || COMPILE_TEST
+    help
+	  Support for QCOM platform cpu power management to perform tasks
+	  necessary while application processor votes for deeper modes so that
+	  the firmware can enter SoC level low power modes to save power.
+
 config QCOM_SMEM
 	tristate "Qualcomm Shared Memory Manager (SMEM)"
 	depends on ARCH_QCOM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index f25b54c..57a1b0e 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
 obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
 qcom_rpmh-y			+= rpmh-rsc.o
 qcom_rpmh-y			+= rpmh.o
+obj-$(CONFIG_QCOM_CPU_PD) += cpu_pd.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
 obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
diff --git a/drivers/soc/qcom/cpu_pd.c b/drivers/soc/qcom/cpu_pd.c
new file mode 100644
index 0000000..565c510
--- /dev/null
+++ b/drivers/soc/qcom/cpu_pd.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/of_platform.h>
+#include <linux/pm_domain.h>
+#include <linux/slab.h>
+
+#include <soc/qcom/rpmh.h>
+
+static struct device *cpu_pd_dev;
+
+static int cpu_pd_power_off(struct generic_pm_domain *domain)
+{
+	if (rpmh_ctrlr_idle(cpu_pd_dev)) {
+		/* Flush the sleep/wake sets */
+		rpmh_flush(cpu_pd_dev);
+	} else {
+		pr_debug("rpmh controller is busy\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int cpu_pm_domain_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct generic_pm_domain *cpu_pd;
+	int ret = -EINVAL, cpu;
+
+	if (!np) {
+		dev_err(dev, "device tree node not found\n");
+		return -ENODEV;
+	}
+
+	if (!of_find_property(np, "#power-domain-cells", NULL)) {
+		pr_err("power-domain-cells not found\n");
+		return -ENODEV;
+	}
+
+	cpu_pd_dev = &pdev->dev;
+	if (IS_ERR_OR_NULL(cpu_pd_dev))
+		return PTR_ERR(cpu_pd_dev);
+
+	cpu_pd = devm_kzalloc(dev, sizeof(*cpu_pd), GFP_KERNEL);
+	if (!cpu_pd)
+		return -ENOMEM;
+
+	cpu_pd->name = kasprintf(GFP_KERNEL, "%s", np->name);
+	if (!cpu_pd->name)
+		goto free_cpu_pd;
+	cpu_pd->name = kbasename(cpu_pd->name);
+	cpu_pd->power_off = cpu_pd_power_off;
+	cpu_pd->flags |= GENPD_FLAG_IRQ_SAFE;
+
+	ret = pm_genpd_init(cpu_pd, NULL, false);
+	if (ret)
+		goto free_name;
+
+	ret = of_genpd_add_provider_simple(np, cpu_pd);
+	if (ret)
+		goto remove_pd;
+
+	pr_info("init PM domain %s\n", cpu_pd->name);
+
+	for_each_present_cpu(cpu) {
+		ret = of_genpd_attach_cpu(cpu);
+		if (ret)
+			goto detach_cpu;
+	}
+	return 0;
+
+detach_cpu:
+	of_genpd_detach_cpu(cpu);
+
+remove_pd:
+	pm_genpd_remove(cpu_pd);
+
+free_name:
+	kfree(cpu_pd->name);
+
+free_cpu_pd:
+	kfree(cpu_pd);
+	cpu_pd_dev = NULL;
+	pr_err("failed to init PM domain ret=%d %pOF\n", ret, np);
+	return ret;
+}
+
+static const struct of_device_id cpu_pd_drv_match[] = {
+	{ .compatible = "qcom,cpu-pm-domain", },
+	{ }
+};
+
+static struct platform_driver cpu_pm_domain_driver = {
+	.probe = cpu_pm_domain_probe,
+	.driver	= {
+		.name = "cpu_pm_domain",
+		.of_match_table = cpu_pd_drv_match,
+	},
+};
+builtin_platform_driver(cpu_pm_domain_driver);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH RFC v1 4/8] drivers: qcom: cpu_pd: add cpu power domain support using genpd

[Sudeep Holla]

On Thu, Oct 11, 2018 at 02:50:51AM +0530, Raju P.L.S.S.S.N wrote:
> RPMH based targets require that the sleep and wake state request votes
> be sent during system low power mode entry. The votes help reduce the
> power consumption when the AP is not using them. The votes sent by the
> clients are cached in RPMH controller and needs to be flushed when the
> last cpu enters low power mode. So add cpu power domain using Linux
> generic power domain infrastructure to perform necessary tasks as part
> of domain power down.
>

You seem to have either randomly chosen just 3 patches from Lina/Ulf's
CPU genpd series or this series doesn't entirely depend on it ?

If latter, how does this work with PSCI CPU_SUSPEND operations ?

And why this can be part of PSCI firmware implementation. Only PSCI
firmware needs if RPMH votes need to be flushed or not. So, honestly
I don't see the need for this in Linux.

--
Regards,
Sudeep

Re: [PATCH RFC v1 4/8] drivers: qcom: cpu_pd: add cpu power domain support using genpd

[Ulf Hansson]

On 11 October 2018 at 13:13, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Thu, Oct 11, 2018 at 02:50:51AM +0530, Raju P.L.S.S.S.N wrote:
>> RPMH based targets require that the sleep and wake state request votes
>> be sent during system low power mode entry. The votes help reduce the
>> power consumption when the AP is not using them. The votes sent by the
>> clients are cached in RPMH controller and needs to be flushed when the
>> last cpu enters low power mode. So add cpu power domain using Linux
>> generic power domain infrastructure to perform necessary tasks as part
>> of domain power down.
>>
>
> You seem to have either randomly chosen just 3 patches from Lina/Ulf's
> CPU genpd series or this series doesn't entirely depend on it ?

Yep, it not easy to follow. But I do understand what you are trying to do here.

>
> If latter, how does this work with PSCI CPU_SUSPEND operations ?
>
> And why this can be part of PSCI firmware implementation. Only PSCI
> firmware needs if RPMH votes need to be flushed or not. So, honestly
> I don't see the need for this in Linux.

I do think there is clear need for this in Linux. More precisely,
since the PSCI firmware have knowledge solely about CPUs (and clusters
of CPUs), but not about other shared resources/devices present on the
SoC.

What Raju is trying to do here, is to manage those resources which
needs special treatment, before and after the CPU (likely cluster) is
going idle and returns from idle.

One question here though, what particular idle state is relevant for
the QCOM SoC to take last-man-actions for? I assume it's only cluster
idle states, and not about cpu idle states, no? Raju, can you please
clarify?

Historically, the typical solution have been to use the
cpu_cluster_pm_enter|exit() notifiers. Those could potentially be
replaced by instead building a hierarchical topology, using
master/subdomain of genpd/"power-domains", along the lines of what
Raju is doing. However, I am not sure if that is the correct approach,
at least we need to make sure it models the HW in DT correctly.

Kind regards
Uffe

Re: [PATCH RFC v1 4/8] drivers: qcom: cpu_pd: add cpu power domain support using genpd

[Sudeep Holla]

On Thu, Oct 11, 2018 at 05:27:59PM +0200, Ulf Hansson wrote:
> On 11 October 2018 at 13:13, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > On Thu, Oct 11, 2018 at 02:50:51AM +0530, Raju P.L.S.S.S.N wrote:
> >> RPMH based targets require that the sleep and wake state request votes
> >> be sent during system low power mode entry. The votes help reduce the
> >> power consumption when the AP is not using them. The votes sent by the
> >> clients are cached in RPMH controller and needs to be flushed when the
> >> last cpu enters low power mode. So add cpu power domain using Linux
> >> generic power domain infrastructure to perform necessary tasks as part
> >> of domain power down.
> >>
> >
> > You seem to have either randomly chosen just 3 patches from Lina/Ulf's
> > CPU genpd series or this series doesn't entirely depend on it ?
> 
> Yep, it not easy to follow. But I do understand what you are trying to do here.
> 
> >
> > If latter, how does this work with PSCI CPU_SUSPEND operations ?
> >
> > And why this can be part of PSCI firmware implementation. Only PSCI
> > firmware needs if RPMH votes need to be flushed or not. So, honestly
> > I don't see the need for this in Linux.
> 
> I do think there is clear need for this in Linux. More precisely,
> since the PSCI firmware have knowledge solely about CPUs (and clusters
> of CPUs), but not about other shared resources/devices present on the
> SoC.
>

I disagree. Even with OSI, you indicate the cluster power off though
PSCI CPU_SUSPEND call. If for any async wakeup reasons, firmware decides
not to enter cluster OFF, then it may skip flushing RPMH vote. So doing
it in PSCI is more correct and elegant to avoid such corner cases.

> What Raju is trying to do here, is to manage those resources which
> needs special treatment, before and after the CPU (likely cluster) is
> going idle and returns from idle.
>

OK I get that, but why is Linux better than PSCI. I have my reasoning
above.

> One question here though, what particular idle state is relevant for
> the QCOM SoC to take last-man-actions for? I assume it's only cluster
> idle states, and not about cpu idle states, no? Raju, can you please
> clarify?
>

I assume so. I did see some comment or commit message to indicate the
same.

> Historically, the typical solution have been to use the
> cpu_cluster_pm_enter|exit() notifiers. Those could potentially be
> replaced by instead building a hierarchical topology, using
> master/subdomain of genpd/"power-domains", along the lines of what
> Raju is doing. However, I am not sure if that is the correct approach,
> at least we need to make sure it models the HW in DT correctly.
>

Indeed. I am not sure how to represent both PSCI and this power domains ?
Though I believe the latter is not required at all.

--
Regards,
Sudeep

Re: [PATCH RFC v1 4/8] drivers: qcom: cpu_pd: add cpu power domain support using genpd

[Ulf Hansson]

On 11 October 2018 at 17:59, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Thu, Oct 11, 2018 at 05:27:59PM +0200, Ulf Hansson wrote:
>> On 11 October 2018 at 13:13, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> > On Thu, Oct 11, 2018 at 02:50:51AM +0530, Raju P.L.S.S.S.N wrote:
>> >> RPMH based targets require that the sleep and wake state request votes
>> >> be sent during system low power mode entry. The votes help reduce the
>> >> power consumption when the AP is not using them. The votes sent by the
>> >> clients are cached in RPMH controller and needs to be flushed when the
>> >> last cpu enters low power mode. So add cpu power domain using Linux
>> >> generic power domain infrastructure to perform necessary tasks as part
>> >> of domain power down.
>> >>
>> >
>> > You seem to have either randomly chosen just 3 patches from Lina/Ulf's
>> > CPU genpd series or this series doesn't entirely depend on it ?
>>
>> Yep, it not easy to follow. But I do understand what you are trying to do here.
>>
>> >
>> > If latter, how does this work with PSCI CPU_SUSPEND operations ?
>> >
>> > And why this can be part of PSCI firmware implementation. Only PSCI
>> > firmware needs if RPMH votes need to be flushed or not. So, honestly
>> > I don't see the need for this in Linux.
>>
>> I do think there is clear need for this in Linux. More precisely,
>> since the PSCI firmware have knowledge solely about CPUs (and clusters
>> of CPUs), but not about other shared resources/devices present on the
>> SoC.
>>
>
> I disagree. Even with OSI, you indicate the cluster power off though
> PSCI CPU_SUSPEND call. If for any async wakeup reasons, firmware decides
> not to enter cluster OFF, then it may skip flushing RPMH vote. So doing
> it in PSCI is more correct and elegant to avoid such corner cases.
>
>> What Raju is trying to do here, is to manage those resources which
>> needs special treatment, before and after the CPU (likely cluster) is
>> going idle and returns from idle.
>>
>
> OK I get that, but why is Linux better than PSCI. I have my reasoning
> above.

I assume Lina, in the other thread from Raju, have provided you the
details why PSCI is not the place to do this and why Linux need to be
involved. In any case, let's continue that discussion in that thread
rather than here.

>
>> One question here though, what particular idle state is relevant for
>> the QCOM SoC to take last-man-actions for? I assume it's only cluster
>> idle states, and not about cpu idle states, no? Raju, can you please
>> clarify?
>>
>
> I assume so. I did see some comment or commit message to indicate the
> same.
>
>> Historically, the typical solution have been to use the
>> cpu_cluster_pm_enter|exit() notifiers. Those could potentially be
>> replaced by instead building a hierarchical topology, using
>> master/subdomain of genpd/"power-domains", along the lines of what
>> Raju is doing. However, I am not sure if that is the correct approach,
>> at least we need to make sure it models the HW in DT correctly.
>>
>
> Indeed. I am not sure how to represent both PSCI and this power domains ?
> Though I believe the latter is not required at all.

After my re-work of the PSCI power domain series, I believe the
power-domain that Raju is adding, should then be modeled as the master
power-domain of the PSCI *cluster* power domain. But, as stated, I am
not sure if it's the correct way to model the HW/topology. Maybe it
is.

The other option is to explore the cpu_cluster_pm_enter|exit(), which
today is the only viable solution in the kernel. In principle we then
need to call cpu_cluster_pm_enter() from the PSCI's cluster PM domain
genpd ->power_off() callback, and cpu_cluster_pm_exit() from the
->power_on() callback.

Or maybe we simply need something entirely new, like genpd PM
domain-on/off notifiers, which may scale better. It has even been
suggested on the mailing list, long time ago.

Kind regards
Uffe

Re: [PATCH RFC v1 4/8] drivers: qcom: cpu_pd: add cpu power domain support using genpd

[Sudeep Holla]

On Thu, Oct 11, 2018 at 02:50:51AM +0530, Raju P.L.S.S.S.N wrote:
> RPMH based targets require that the sleep and wake state request votes
> be sent during system low power mode entry. The votes help reduce the
> power consumption when the AP is not using them. The votes sent by the
> clients are cached in RPMH controller and needs to be flushed when the
> last cpu enters low power mode. So add cpu power domain using Linux
> generic power domain infrastructure to perform necessary tasks as part
> of domain power down.
>
> Suggested-by: Lina Iyer <ilina@codeaurora.org>
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> ---
>  drivers/soc/qcom/Kconfig  |   9 ++++
>  drivers/soc/qcom/Makefile |   1 +
>  drivers/soc/qcom/cpu_pd.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 114 insertions(+)
>  create mode 100644 drivers/soc/qcom/cpu_pd.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index ba79b60..91e8b3b 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -95,6 +95,7 @@ config QCOM_RMTFS_MEM
>  config QCOM_RPMH
>  	bool "Qualcomm RPM-Hardened (RPMH) Communication"
>  	depends on ARCH_QCOM && ARM64 && OF || COMPILE_TEST
> +	select QCOM_CPU_PD
>  	help
>  	  Support for communication with the hardened-RPM blocks in
>  	  Qualcomm Technologies Inc (QTI) SoCs. RPMH communication uses an
> @@ -102,6 +103,14 @@ config QCOM_RPMH
>  	  of hardware components aggregate requests for these resources and
>  	  help apply the aggregated state on the resource.
>
> +config QCOM_CPU_PD
> +    bool "Qualcomm cpu power domain driver"
> +    depends on QCOM_RPMH && PM_GENERIC_DOMAINS || COMPILE_TEST
> +    help
> +	  Support for QCOM platform cpu power management to perform tasks
> +	  necessary while application processor votes for deeper modes so that
> +	  the firmware can enter SoC level low power modes to save power.
> +
>  config QCOM_SMEM
>  	tristate "Qualcomm Shared Memory Manager (SMEM)"
>  	depends on ARCH_QCOM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index f25b54c..57a1b0e 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
>  obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
>  qcom_rpmh-y			+= rpmh-rsc.o
>  qcom_rpmh-y			+= rpmh.o
> +obj-$(CONFIG_QCOM_CPU_PD) += cpu_pd.o
>  obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
>  obj-$(CONFIG_QCOM_SMEM) +=	smem.o
>  obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> diff --git a/drivers/soc/qcom/cpu_pd.c b/drivers/soc/qcom/cpu_pd.c
> new file mode 100644
> index 0000000..565c510
> --- /dev/null
> +++ b/drivers/soc/qcom/cpu_pd.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/of_platform.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +
> +#include <soc/qcom/rpmh.h>
> +
> +static struct device *cpu_pd_dev;
> +

This doesn't scale if you have 2 instances.

> +static int cpu_pd_power_off(struct generic_pm_domain *domain)
> +{
> +	if (rpmh_ctrlr_idle(cpu_pd_dev)) {

How is this expected to compile ? I couldn't find any instance of this.

> +		/* Flush the sleep/wake sets */
> +		rpmh_flush(cpu_pd_dev);

So it's just flushing the pending requests on the controller. The function
implementation carries a note that it's assumed to be called only from
system PM and we may call it in cpu idle path here. Is that fine ?
If so, may be the comment needs to be dropped.

Also, where exactly this voting for CPU is happening in this path ?

> +	} else {
> +		pr_debug("rpmh controller is busy\n");
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cpu_pm_domain_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct generic_pm_domain *cpu_pd;
> +	int ret = -EINVAL, cpu;
> +
> +	if (!np) {
> +		dev_err(dev, "device tree node not found\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!of_find_property(np, "#power-domain-cells", NULL)) {
> +		pr_err("power-domain-cells not found\n");
> +		return -ENODEV;
> +	}
> +
> +	cpu_pd_dev = &pdev->dev;
> +	if (IS_ERR_OR_NULL(cpu_pd_dev))

Isn't this too late to check ? You would have crashed on dev->of_node.
So sounds pretty useless

> +		return PTR_ERR(cpu_pd_dev);
> +
> +	cpu_pd = devm_kzalloc(dev, sizeof(*cpu_pd), GFP_KERNEL);
> +	if (!cpu_pd)
> +		return -ENOMEM;
> +
> +	cpu_pd->name = kasprintf(GFP_KERNEL, "%s", np->name);
> +	if (!cpu_pd->name)
> +		goto free_cpu_pd;
> +	cpu_pd->name = kbasename(cpu_pd->name);
> +	cpu_pd->power_off = cpu_pd_power_off;

If some kind of voting is done in off, why is there nothing to take care
of that in pd_power_on  if it's per EL(linux/hyp/secure).

--
Regards,
Sudeep

Re: [PATCH RFC v1 4/8] drivers: qcom: cpu_pd: add cpu power domain support using genpd

[Raju P L S S S N]

On 10/12/2018 8:03 PM, Sudeep Holla wrote:
> On Thu, Oct 11, 2018 at 02:50:51AM +0530, Raju P.L.S.S.S.N wrote:
>> RPMH based targets require that the sleep and wake state request votes
>> be sent during system low power mode entry. The votes help reduce the
>> power consumption when the AP is not using them. The votes sent by the
>> clients are cached in RPMH controller and needs to be flushed when the
>> last cpu enters low power mode. So add cpu power domain using Linux
>> generic power domain infrastructure to perform necessary tasks as part
>> of domain power down.
>>
>> Suggested-by: Lina Iyer <ilina@codeaurora.org>
>> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
>> ---
>>   drivers/soc/qcom/Kconfig  |   9 ++++
>>   drivers/soc/qcom/Makefile |   1 +
>>   drivers/soc/qcom/cpu_pd.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 114 insertions(+)
>>   create mode 100644 drivers/soc/qcom/cpu_pd.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index ba79b60..91e8b3b 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -95,6 +95,7 @@ config QCOM_RMTFS_MEM
>>   config QCOM_RPMH
>>   	bool "Qualcomm RPM-Hardened (RPMH) Communication"
>>   	depends on ARCH_QCOM && ARM64 && OF || COMPILE_TEST
>> +	select QCOM_CPU_PD
>>   	help
>>   	  Support for communication with the hardened-RPM blocks in
>>   	  Qualcomm Technologies Inc (QTI) SoCs. RPMH communication uses an
>> @@ -102,6 +103,14 @@ config QCOM_RPMH
>>   	  of hardware components aggregate requests for these resources and
>>   	  help apply the aggregated state on the resource.
>>
>> +config QCOM_CPU_PD
>> +    bool "Qualcomm cpu power domain driver"
>> +    depends on QCOM_RPMH && PM_GENERIC_DOMAINS || COMPILE_TEST
>> +    help
>> +	  Support for QCOM platform cpu power management to perform tasks
>> +	  necessary while application processor votes for deeper modes so that
>> +	  the firmware can enter SoC level low power modes to save power.
>> +
>>   config QCOM_SMEM
>>   	tristate "Qualcomm Shared Memory Manager (SMEM)"
>>   	depends on ARCH_QCOM
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index f25b54c..57a1b0e 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
>>   obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
>>   qcom_rpmh-y			+= rpmh-rsc.o
>>   qcom_rpmh-y			+= rpmh.o
>> +obj-$(CONFIG_QCOM_CPU_PD) += cpu_pd.o
>>   obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
>>   obj-$(CONFIG_QCOM_SMEM) +=	smem.o
>>   obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
>> diff --git a/drivers/soc/qcom/cpu_pd.c b/drivers/soc/qcom/cpu_pd.c
>> new file mode 100644
>> index 0000000..565c510
>> --- /dev/null
>> +++ b/drivers/soc/qcom/cpu_pd.c
>> @@ -0,0 +1,104 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/of_platform.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/slab.h>
>> +
>> +#include <soc/qcom/rpmh.h>
>> +
>> +static struct device *cpu_pd_dev;
>> +
> 
> This doesn't scale if you have 2 instances.

There would be one instance of this driver for this platform.
This platform has single cluster & single power domain which includes 
all the cpus. Even if there are more than one cluster (lets say, later 
on) the top level grouping of all the clusters will be considered as a 
domain. In this case, if hierarchical topological representation is 
needed, the driver probe needs to be modified. The naming might have led 
to the confusion. Should I change it to something like top_level_pd_dev 
? Another way is to define the compatible flag as SoC specific like 
"qcom,cpu-pm-domain-sdm845" but then there will be multiple SoCs based 
on single cluster. For each of them, compatible flag needs to be added.




> 
>> +static int cpu_pd_power_off(struct generic_pm_domain *domain)
>> +{
>> +	if (rpmh_ctrlr_idle(cpu_pd_dev)) {
> 
> How is this expected to compile ? I couldn't find any instance of this.
> 
>> +		/* Flush the sleep/wake sets */
>> +		rpmh_flush(cpu_pd_dev);
> 
> So it's just flushing the pending requests on the controller. The function
> implementation carries a note that it's assumed to be called only from
> system PM and we may call it in cpu idle path here. Is that fine ?
> If so, may be the comment needs to be dropped.

Sure. we will change the comment.
When RPMh driver was being developed, it named the top level power 
domain which includes all the cpus as system PM.


> 
> Also, where exactly this voting for CPU is happening in this path ?
> 

This SoC uses PC mode and we are not voting for CPU's idle state here.. 
We are just doing power domain activities that are needed when all the 
cpus are down.



>> +	} else {
>> +		pr_debug("rpmh controller is busy\n");
>> +		return -EBUSY;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cpu_pm_domain_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct generic_pm_domain *cpu_pd;
>> +	int ret = -EINVAL, cpu;
>> +
>> +	if (!np) {
>> +		dev_err(dev, "device tree node not found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!of_find_property(np, "#power-domain-cells", NULL)) {
>> +		pr_err("power-domain-cells not found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	cpu_pd_dev = &pdev->dev;
>> +	if (IS_ERR_OR_NULL(cpu_pd_dev))
> 
> Isn't this too late to check ? You would have crashed on dev->of_node.
> So sounds pretty useless

Agree. I will change this.


> 
>> +		return PTR_ERR(cpu_pd_dev);
>> +
>> +	cpu_pd = devm_kzalloc(dev, sizeof(*cpu_pd), GFP_KERNEL);
>> +	if (!cpu_pd)
>> +		return -ENOMEM;
>> +
>> +	cpu_pd->name = kasprintf(GFP_KERNEL, "%s", np->name);
>> +	if (!cpu_pd->name)
>> +		goto free_cpu_pd;
>> +	cpu_pd->name = kbasename(cpu_pd->name);
>> +	cpu_pd->power_off = cpu_pd_power_off;
> 
> If some kind of voting is done in off, why is there nothing to take care
> of that in pd_power_on  if it's per EL(linux/hyp/secure).
Both sleep state votes (which would be applied while powering down)and 
wake state votes (which would be applied while powering on) are applied 
in hardware. Software is expected to write both these types of votes to 
RPM while powering down only. As hardware takes care of applying the 
votes, no need for any pd_power_on operations.


Thanks a lot for your time & review Sudeep. Happy weekend.

- Raju.



> 
> --
> Regards,
> Sudeep
> 

[PATCH RFC v1 5/8] dt-bindings: introduce cpu power domain bindings for Qualcomm SoCs

[Raju P.L.S.S.S.N]

Add device binding documentation for Qualcomm Technology Inc's cpu
domain driver. The driver is used for managing system sleep activities
that are required when application processor is going to deepest low
power mode.

Cc: devicetree@vger.kernel.org
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
---
 .../bindings/soc/qcom/cpu_power_domain.txt         | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt

diff --git a/Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt b/Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt
new file mode 100644
index 0000000..1c8fe69
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt
@@ -0,0 +1,39 @@
+Qualcomm Technologies cpu power domain
+-----------------------------------------
+
+CPU power domain handles the tasks that need to be performed during
+application processor deeper low power mode entry for QCOM SoCs which
+have hardened IP blocks combinedly called as RPMH (Resource Power Manager
+Hardened) for shared resource management. Flushing the buffered requests
+to TCS (Triggered Command Set) in RSC (Resource State Coordinator) and
+programming the wakeup timer in PDC (Power Domain Controller) for timer
+based wakeup are handled as part of domain power down.
+
+The bindings for cpu power domain is specified in the RSC section in
+devicetree.
+
+Properties:
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be "qcom,cpu-pm-domain".
+
+- #power-domain-cells: number of cells in power domain specifier;
+    must be 0.
+
+Node of a device using power domains must have a power-domains property
+defined with a phandle to respective power domain.
+
+Example:
+
+	apps_rsc: rsc@179c0000 {
+		[...]
+		cpu_pd: power-domain-controller {
+			compatible = "qcom,cpu-pm-domain";
+			#power-domain-cells = <0>;
+		};
+	};
+
+
+See Documentation/devicetree/bindings/power/power_domain.txt for description
+of consumer-side bindings.
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH RFC v1 5/8] dt-bindings: introduce cpu power domain bindings for Qualcomm SoCs

[Sudeep Holla]

On Thu, Oct 11, 2018 at 02:50:52AM +0530, Raju P.L.S.S.S.N wrote:
> Add device binding documentation for Qualcomm Technology Inc's cpu
> domain driver. The driver is used for managing system sleep activities
> that are required when application processor is going to deepest low
> power mode.
>

So either we are not using PSCI or the binding is not so clear on
how this co-exist with PSCI power domains. Could you provide details ?

> Cc: devicetree@vger.kernel.org
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> ---
>  .../bindings/soc/qcom/cpu_power_domain.txt         | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt b/Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt
> new file mode 100644
> index 0000000..1c8fe69
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt
> @@ -0,0 +1,39 @@
> +Qualcomm Technologies cpu power domain
> +-----------------------------------------
> +
> +CPU power domain handles the tasks that need to be performed during
> +application processor deeper low power mode entry for QCOM SoCs which
> +have hardened IP blocks combinedly called as RPMH (Resource Power Manager
> +Hardened) for shared resource management. Flushing the buffered requests
> +to TCS (Triggered Command Set) in RSC (Resource State Coordinator) and
> +programming the wakeup timer in PDC (Power Domain Controller) for timer
> +based wakeup are handled as part of domain power down.
> +

And which is this not hidden as part of PSCI CPU_SUSPEND ?

> +The bindings for cpu power domain is specified in the RSC section in
> +devicetree.
> +
> +Properties:
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be "qcom,cpu-pm-domain".
> +

NACK until details on how this can co-exist with PSCI is provided.

--
Regards,
Sudeep

Re: [PATCH RFC v1 5/8] dt-bindings: introduce cpu power domain bindings for Qualcomm SoCs

[Raju P L S S S N]

On 10/11/2018 4:38 PM, Sudeep Holla wrote:
> On Thu, Oct 11, 2018 at 02:50:52AM +0530, Raju P.L.S.S.S.N wrote:
>> Add device binding documentation for Qualcomm Technology Inc's cpu
>> domain driver. The driver is used for managing system sleep activities
>> that are required when application processor is going to deepest low
>> power mode.
>>
> 
> So either we are not using PSCI or the binding is not so clear on
> how this co-exist with PSCI power domains. Could you provide details ?
> 
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
>> ---
>>   .../bindings/soc/qcom/cpu_power_domain.txt         | 39 ++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt b/Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt
>> new file mode 100644
>> index 0000000..1c8fe69
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/cpu_power_domain.txt
>> @@ -0,0 +1,39 @@
>> +Qualcomm Technologies cpu power domain
>> +-----------------------------------------
>> +
>> +CPU power domain handles the tasks that need to be performed during
>> +application processor deeper low power mode entry for QCOM SoCs which
>> +have hardened IP blocks combinedly called as RPMH (Resource Power Manager
>> +Hardened) for shared resource management. Flushing the buffered requests
>> +to TCS (Triggered Command Set) in RSC (Resource State Coordinator) and
>> +programming the wakeup timer in PDC (Power Domain Controller) for timer
>> +based wakeup are handled as part of domain power down.
>> +
> 
> And which is this not hidden as part of PSCI CPU_SUSPEND ?
> 
>> +The bindings for cpu power domain is specified in the RSC section in
>> +devicetree.
>> +
>> +Properties:
>> +- compatible:
>> +	Usage: required
>> +	Value type: <string>
>> +	Definition: must be "qcom,cpu-pm-domain".
>> +
> 
> NACK until details on how this can co-exist with PSCI is provided.

Platform coordinated mode is used on this SoC. So they both can 
co-exist. I hope with the discussion on other thread, the details are clear.

> 
> --
> Regards,
> Sudeep
> 

[PATCH RFC v1 6/8] drivers: qcom: cpu_pd: program next wakeup to PDC timer

[Raju P.L.S.S.S.N]

In addition to sleep and wake request votes that need to be sent to
remote processor as part of low power mode entry, the next wake-up timer
value needs to be programmed to PDC (Power Domain Controller) which has
its own timer and is in an always on power domain. A specific control
register is provided in RSC address space for this purpose. PDC wakes-up
the RSC and sets up the resources back in active state before the
processor is woken up by a timer interrupt.

Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
---
 drivers/soc/qcom/Kconfig  |  2 +-
 drivers/soc/qcom/cpu_pd.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 91e8b3b..9abaeab 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -105,7 +105,7 @@ config QCOM_RPMH
 
 config QCOM_CPU_PD
     bool "Qualcomm cpu power domain driver"
-    depends on QCOM_RPMH && PM_GENERIC_DOMAINS || COMPILE_TEST
+    depends on QCOM_RPMH && PM_GENERIC_DOMAINS && PM_SLEEP || COMPILE_TEST
     help
 	  Support for QCOM platform cpu power management to perform tasks
 	  necessary while application processor votes for deeper modes so that
diff --git a/drivers/soc/qcom/cpu_pd.c b/drivers/soc/qcom/cpu_pd.c
index 565c510..242eced 100644
--- a/drivers/soc/qcom/cpu_pd.c
+++ b/drivers/soc/qcom/cpu_pd.c
@@ -3,19 +3,80 @@
  * Copyright (c) 2018, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/ktime.h>
 #include <linux/of_platform.h>
 #include <linux/pm_domain.h>
 #include <linux/slab.h>
+#include <linux/tick.h>
 
 #include <soc/qcom/rpmh.h>
 
+#define ARCH_TIMER_HZ (19200000)
+#define PDC_TIME_VALID_SHIFT	31
+#define PDC_TIME_UPPER_MASK	0xFFFFFF
 static struct device *cpu_pd_dev;
+static bool suspend;
+
+static uint64_t us_to_ticks(uint64_t time_us)
+{
+	uint64_t sec, nsec, time_cycles;
+
+	sec = time_us;
+	do_div(sec, USEC_PER_SEC);
+	nsec = time_us - sec * USEC_PER_SEC;
+
+	if (nsec > 0) {
+		nsec = nsec * NSEC_PER_USEC;
+		do_div(nsec, NSEC_PER_SEC);
+	}
+
+	sec += nsec;
+
+	time_cycles = (u64)sec * ARCH_TIMER_HZ;
+
+	return time_cycles;
+}
+
+static int setup_pdc_wakeup_timer(struct device *dev)
+{
+	int cpu;
+	struct tcs_cmd cmd[2] = { { 0 } };
+	ktime_t next_wakeup, cpu_wakeup;
+	uint64_t wakeup_cycles = ~0ULL;
+
+	if (!suspend) {
+		/*
+		 * Find the next wakeup for any of the online CPUs
+		 */
+		next_wakeup = ktime_set(KTIME_SEC_MAX, 0);
+		for_each_online_cpu(cpu) {
+			cpu_wakeup = tick_nohz_get_next_wakeup(cpu);
+			if (ktime_before(cpu_wakeup, next_wakeup))
+				next_wakeup = cpu_wakeup;
+		}
+		wakeup_cycles = us_to_ticks(ktime_to_us(next_wakeup));
+	}
+
+	cmd[0].data =  (wakeup_cycles >> 32) & PDC_TIME_UPPER_MASK;
+	cmd[0].data |= 1 << PDC_TIME_VALID_SHIFT;
+	cmd[1].data = (wakeup_cycles & 0xFFFFFFFF);
+
+	return rpmh_write_pdc_data(dev, cmd, ARRAY_SIZE(cmd));
+}
 
 static int cpu_pd_power_off(struct generic_pm_domain *domain)
 {
 	if (rpmh_ctrlr_idle(cpu_pd_dev)) {
 		/* Flush the sleep/wake sets */
 		rpmh_flush(cpu_pd_dev);
+		/*
+		 * The next wakeup value is converted to ticks
+		 * and copied to the Power Domain Controller
+		 * that has its own timer, which is in an
+		 * always-on power domain. The programming is
+		 * done through a separate register on the RSC
+		 */
+		setup_pdc_wakeup_timer(cpu_pd_dev);
 	} else {
 		pr_debug("rpmh controller is busy\n");
 		return -EBUSY;
@@ -89,6 +150,23 @@ static int cpu_pm_domain_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int cpu_pd_suspend(struct device *dev)
+{
+	suspend = true;
+	return 0;
+}
+
+static int cpu_pd_resume(struct device *dev)
+{
+	suspend = false;
+	return 0;
+}
+
+static const struct dev_pm_ops cpu_pd_dev_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(cpu_pd_suspend, cpu_pd_resume)
+};
+
+
 static const struct of_device_id cpu_pd_drv_match[] = {
 	{ .compatible = "qcom,cpu-pm-domain", },
 	{ }
@@ -99,6 +177,7 @@ static int cpu_pm_domain_probe(struct platform_device *pdev)
 	.driver	= {
 		.name = "cpu_pm_domain",
 		.of_match_table = cpu_pd_drv_match,
+		.pm = &cpu_pd_dev_pm_ops,
 	},
 };
 builtin_platform_driver(cpu_pm_domain_driver);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

[PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Raju P.L.S.S.S.N]

Use cpu hotplug callback mechanism to attach/dettach the cpu in
the cpu power domain. During cpu hotplug callback registration,
the starting callback is invoked on all online cpus. So there is
no need to attach from device probe.

Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
---
 drivers/soc/qcom/cpu_pd.c  | 33 +++++++++++++++++++++++++--------
 include/linux/cpuhotplug.h |  1 +
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/qcom/cpu_pd.c b/drivers/soc/qcom/cpu_pd.c
index 242eced..bf2d95d 100644
--- a/drivers/soc/qcom/cpu_pd.c
+++ b/drivers/soc/qcom/cpu_pd.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2018, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/cpuhotplug.h>
 #include <linux/ktime.h>
 #include <linux/of_platform.h>
 #include <linux/pm_domain.h>
@@ -85,12 +86,26 @@ static int cpu_pd_power_off(struct generic_pm_domain *domain)
 	return 0;
 }
 
+static int cpu_pd_starting(unsigned int cpu)
+{
+	if (!suspend && of_genpd_attach_cpu(cpu))
+		pr_err("%s: genpd_attach_cpu fail\n", __func__);
+	return 0;
+}
+
+static int cpu_pd_dying(unsigned int cpu)
+{
+	if (!suspend)
+		of_genpd_detach_cpu(cpu);
+	return 0;
+}
+
 static int cpu_pm_domain_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct generic_pm_domain *cpu_pd;
-	int ret = -EINVAL, cpu;
+	int ret = -EINVAL;
 
 	if (!np) {
 		dev_err(dev, "device tree node not found\n");
@@ -127,15 +142,17 @@ static int cpu_pm_domain_probe(struct platform_device *pdev)
 
 	pr_info("init PM domain %s\n", cpu_pd->name);
 
-	for_each_present_cpu(cpu) {
-		ret = of_genpd_attach_cpu(cpu);
-		if (ret)
-			goto detach_cpu;
-	}
+	/* Install hotplug callbacks */
+	ret = cpuhp_setup_state(CPUHP_AP_QCOM_SYS_PM_DOMAIN_STARTING,
+				"AP_QCOM_SYS_PM_DOMAIN_STARTING",
+				cpu_pd_starting, cpu_pd_dying);
+	if (ret)
+		goto remove_hotplug;
+
 	return 0;
 
-detach_cpu:
-	of_genpd_detach_cpu(cpu);
+remove_hotplug:
+	cpuhp_remove_state(CPUHP_AP_QCOM_SYS_PM_DOMAIN_STARTING);
 
 remove_pd:
 	pm_genpd_remove(cpu_pd);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index caf40ad..4bfe8e2 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -139,6 +139,7 @@ enum cpuhp_state {
 	CPUHP_AP_X86_TBOOT_DYING,
 	CPUHP_AP_ARM_CACHE_B15_RAC_DYING,
 	CPUHP_AP_ONLINE,
+	CPUHP_AP_QCOM_SYS_PM_DOMAIN_STARTING,
 	CPUHP_TEARDOWN_CPU,
 	CPUHP_AP_ONLINE_IDLE,
 	CPUHP_AP_SMPBOOT_THREADS,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Sudeep Holla]

On Thu, Oct 11, 2018 at 02:50:54AM +0530, Raju P.L.S.S.S.N wrote:
> Use cpu hotplug callback mechanism to attach/dettach the cpu in
> the cpu power domain. During cpu hotplug callback registration,
> the starting callback is invoked on all online cpus. So there is
> no need to attach from device probe.
>

To be more explicit, NACK to this series(patches 4-8 to be more specific)
unless you provide details on:

1. Why this is not in PSCI implementation ?
2. If PSCI is used on this platform, how does it work/co-exist ?
3. Is this a shortcut approached taken to bypass the CPU genpd attempts
   from Lina/Ulf ?

--
Regards,
Sudeep

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Lina Iyer]

Sudeep,

This is idea is based on GenPD/PM Domains, but solves for the CPU domain
activities that need to be done from Linux when the CPU domain could be
powered off. In that, it shares the same ideas from the series posted by
Ulf. But this has no bearing on PSCI. The 845 SoC that Raju is working
on, uses Platform-coordinated and so is largely deficient in meeting the
power benefits achievable on this SoC, from Linux.

If you have looked at the patches, you probably know by now, there are
two things this patch does when the CPUs are powered off -
- Flush RPMH requests to the controller (shared resource state requests
  after the CPU is powered off and resume to the current state before
  the CPU wakes up).
- Setup the wakeup time for the CPUs in the hardware registers such that
  the hardware blocks are awake before a CPU is powered back on. This is
  like a back-off time for handling timer wakeups faster.

The CPU PD does not power off the domain from Linux. That is done from
PSCI firmware (ATF). These patches are doing the part that Linux has do,
when powering off the CPUs, to achieve a low standby power consumption.

On Thu, Oct 11 2018 at 05:20 -0600, Sudeep Holla wrote:
>On Thu, Oct 11, 2018 at 02:50:54AM +0530, Raju P.L.S.S.S.N wrote:
>> Use cpu hotplug callback mechanism to attach/dettach the cpu in
>> the cpu power domain. During cpu hotplug callback registration,
>> the starting callback is invoked on all online cpus. So there is
>> no need to attach from device probe.
>>
>
>To be more explicit, NACK to this series(patches 4-8 to be more specific)
>unless you provide details on:
>
>1. Why this is not in PSCI implementation ?
This is a linux activity and there is no provision in ATF or QC firmware
to do this activity. The task of physically powering down the domain,
still is a firmware decision and is done through PSCI platform
coordinated in the firmware.

>2. If PSCI is used on this platform, how does it work/co-exist ?
Yes PSCI is used in this platform. ATF is the firmware and that supports
only Platform Coordinated. This SoC uses cpuidle and the regular PSCI
methods to power off the domain from the firmware. However, Linux has
responsibilities that it needs to complete before the power down can be
beneficial.

>3. Is this a shortcut approached taken to bypass the CPU genpd attempts
>   from Lina/Ulf ?
>
This is an incorrect interpretation and an unwarranted accusation. There
is no need to bypass CPU genpd attempts. That series stands on its own
merits. (Thank you, Ulf). Raju has mentioned in the cover letter
explicitly, that Ulf's patches posted here are for completeness of the
concept, as that series is a dependency. But it will merged as part of
Ulf's efforts.

Isn't sharing ideas a key aspect of working with the community? This
series just goes to say that the idea of CPU PM domains are useful,
whether PSCI uses it or not. If you still need clarifications, Raju and
I will be happy to set up a meeting and go over the idea.

Thanks,
Lina

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Sudeep Holla]

On Thu, Oct 11, 2018 at 10:00:53AM -0600, Lina Iyer wrote:
> Sudeep,
>
> This is idea is based on GenPD/PM Domains, but solves for the CPU domain
> activities that need to be done from Linux when the CPU domain could be
> powered off. In that, it shares the same ideas from the series posted by
> Ulf. But this has no bearing on PSCI. The 845 SoC that Raju is working
> on, uses Platform-coordinated and so is largely deficient in meeting the
> power benefits achievable on this SoC, from Linux.
>

Interesting to know we have some QCOM part with PC mode.

> If you have looked at the patches, you probably know by now, there are
> two things this patch does when the CPUs are powered off -
> - Flush RPMH requests to the controller (shared resource state requests
>  after the CPU is powered off and resume to the current state before
>  the CPU wakes up).
> - Setup the wakeup time for the CPUs in the hardware registers such that
>  the hardware blocks are awake before a CPU is powered back on. This is
>  like a back-off time for handling timer wakeups faster.
>

Yes I understand these.

> The CPU PD does not power off the domain from Linux. That is done from
> PSCI firmware (ATF). These patches are doing the part that Linux has do,
> when powering off the CPUs, to achieve a low standby power consumption.
>

I don't understand why Linux *has do* this part as PSCI manages CPU PM.

> On Thu, Oct 11 2018 at 05:20 -0600, Sudeep Holla wrote:
> > On Thu, Oct 11, 2018 at 02:50:54AM +0530, Raju P.L.S.S.S.N wrote:
> > > Use cpu hotplug callback mechanism to attach/dettach the cpu in
> > > the cpu power domain. During cpu hotplug callback registration,
> > > the starting callback is invoked on all online cpus. So there is
> > > no need to attach from device probe.
> > >
> >
> > To be more explicit, NACK to this series(patches 4-8 to be more specific)
> > unless you provide details on:
> >
> > 1. Why this is not in PSCI implementation ?
> This is a linux activity and there is no provision in ATF or QC firmware
> to do this activity. The task of physically powering down the domain,
> still is a firmware decision and is done through PSCI platform
> coordinated in the firmware.
>

Yes that was my understanding. So the addon question here is: if PSCI
decides to abort entering the idle state, the Linux doing the RPMH
request is of no use which can be prevented if done once PSCI f/w is
about to enter the state. I know it may be corner case, but we have
whole OSI mode based on such corner cases.

> > 2. If PSCI is used on this platform, how does it work/co-exist ?
> Yes PSCI is used in this platform. ATF is the firmware and that supports
> only Platform Coordinated. This SoC uses cpuidle and the regular PSCI
> methods to power off the domain from the firmware. However, Linux has
> responsibilities that it needs to complete before the power down can be
> beneficial.
>

I understand the need to inform RPMH. So I take only reason to do that
in Linux is TF-A doesn't have any support to talk to RPMH ?

> > 3. Is this a shortcut approached taken to bypass the CPU genpd attempts
> >   from Lina/Ulf ?
> >
> This is an incorrect interpretation and an unwarranted accusation. There
> is no need to bypass CPU genpd attempts. That series stands on its own
> merits. (Thank you, Ulf). Raju has mentioned in the cover letter
> explicitly, that Ulf's patches posted here are for completeness of the
> concept, as that series is a dependency. But it will merged as part of
> Ulf's efforts.
>

Sorry if it sounded like accusation. I didn't mean to. I was checking
if this was alternative solution. I do understand the need to deal with
product pressures.

Now that we have some platform with PC, it's good to compare PC vs OSI
which we always lacked. Thanks for letting us know this platform is PC
based.

> Isn't sharing ideas a key aspect of working with the community? This
> series just goes to say that the idea of CPU PM domains are useful,
> whether PSCI uses it or not. If you still need clarifications, Raju and
> I will be happy to set up a meeting and go over the idea.
>
Ah OK, so this platform will have flattened cpu-idle-states list ? That's
absolutely fine. But what if we want to represent hierarchical PSCI based
PM domains and this power domain for some platform. That's the main
concern I raised. For me, the power-domains in DT introduced in this
is just to deal with RPMH though the actual work is done by PSCI.
That just doesn't look that good for me.

--
Regards,
Sudeep

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Lina Iyer]

On Thu, Oct 11 2018 at 10:19 -0600, Sudeep Holla wrote:
>On Thu, Oct 11, 2018 at 10:00:53AM -0600, Lina Iyer wrote:
>> Sudeep,
>>
>> The CPU PD does not power off the domain from Linux. That is done from
>> PSCI firmware (ATF). These patches are doing the part that Linux has do,
>> when powering off the CPUs, to achieve a low standby power consumption.
>>
>
>I don't understand why Linux *has do* this part as PSCI manages CPU PM.
>
If we don't do this, then we leave a lot of power saving on the table,
when the CPU powered off. Why should the DDR and the shared busses and
clocks be idling at high power, when not needed ? PSCI has no clue to
what resource requests was made my Linux and its Linux's responsibility
to relinquish them when not needed. Therefore has to done from Linux.

>> On Thu, Oct 11 2018 at 05:20 -0600, Sudeep Holla wrote:
>> > On Thu, Oct 11, 2018 at 02:50:54AM +0530, Raju P.L.S.S.S.N wrote:
>> > > Use cpu hotplug callback mechanism to attach/dettach the cpu in
>> > > the cpu power domain. During cpu hotplug callback registration,
>> > > the starting callback is invoked on all online cpus. So there is
>> > > no need to attach from device probe.
>> > >
>> >
>> > To be more explicit, NACK to this series(patches 4-8 to be more specific)
>> > unless you provide details on:
>> >
>> > 1. Why this is not in PSCI implementation ?
>> This is a linux activity and there is no provision in ATF or QC firmware
>> to do this activity. The task of physically powering down the domain,
>> still is a firmware decision and is done through PSCI platform
>> coordinated in the firmware.
>>
>
>Yes that was my understanding. So the addon question here is: if PSCI
>decides to abort entering the idle state, the Linux doing the RPMH
>request is of no use which can be prevented if done once PSCI f/w is
>about to enter the state. I know it may be corner case, but we have
>whole OSI mode based on such corner cases.
>
Yes, it is a possibility and worth the chance taken. On an SoC, there
are other processors that may vote against powering down the shared
resources even when Linux has shutdown and it is a very likely
possibility. Ex., when you are on a phone call, the CPU subsystem could
be powered off and the flush of RPMH requests is a 'waste', but it is a
chance we need to take. The alternate is a synchronization nightmare.

Even with all the unnecessary flushing it is totally worth it. OSI helps
alleviates this a bit because it embodies the same CPU PD concepts at
its core. Imagine if you didn't have CPU PM domain, the every CPU would
be flushing RPMH request, whenever they power down, because you never know
when all CPUs are going to be powered down at the same time. That is the
biggest benefit of OSI over PC mode in PSCI.

>> > 2. If PSCI is used on this platform, how does it work/co-exist ?
>> Yes PSCI is used in this platform. ATF is the firmware and that supports
>> only Platform Coordinated. This SoC uses cpuidle and the regular PSCI
>> methods to power off the domain from the firmware. However, Linux has
>> responsibilities that it needs to complete before the power down can be
>> beneficial.
>>
>
>I understand the need to inform RPMH. So I take only reason to do that
>in Linux is TF-A doesn't have any support to talk to RPMH ?
>
It may or may not, depending on which firmware you talk to. But consider
this, if the firmware votes for lowering resource state, it is doing it
for its exception level and not for Linux. So Linux has to take care of
its own.

>Now that we have some platform with PC, it's good to compare PC vs OSI
>which we always lacked. Thanks for letting us know this platform is PC
>based.
>
Can I take that you are okay with the OSI idea and this one, then :)
Yes, we are close to having a platform have both, possibly.

>> Isn't sharing ideas a key aspect of working with the community? This
>> series just goes to say that the idea of CPU PM domains are useful,
>> whether PSCI uses it or not. If you still need clarifications, Raju and
>> I will be happy to set up a meeting and go over the idea.
>>
>Ah OK, so this platform will have flattened cpu-idle-states list ? That's
>absolutely fine. But what if we want to represent hierarchical PSCI based
>PM domains and this power domain for some platform. That's the main
>concern I raised. For me, the power-domains in DT introduced in this
>is just to deal with RPMH though the actual work is done by PSCI.
>That just doesn't look that good for me.
>
What problem do you see with that? It would help if you could clarify
your concern.

Thanks,
Lina

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Sudeep Holla]

On Thu, Oct 11, 2018 at 10:58:22AM -0600, Lina Iyer wrote:
> On Thu, Oct 11 2018 at 10:19 -0600, Sudeep Holla wrote:
> > On Thu, Oct 11, 2018 at 10:00:53AM -0600, Lina Iyer wrote:
> > > Sudeep,
> > >
> > > The CPU PD does not power off the domain from Linux. That is done from
> > > PSCI firmware (ATF). These patches are doing the part that Linux has do,
> > > when powering off the CPUs, to achieve a low standby power consumption.
> > >
> >
> > I don't understand why Linux *has do* this part as PSCI manages CPU PM.
> >
> If we don't do this, then we leave a lot of power saving on the table,
> when the CPU powered off. Why should the DDR and the shared busses and
> clocks be idling at high power, when not needed ? PSCI has no clue to
> what resource requests was made my Linux and its Linux's responsibility
> to relinquish them when not needed. Therefore has to done from Linux.
>

Is DDR managed by Linux ? I assumed it was handled by higher exception
levels. Can you give examples of resources used by CPU in this context.
When CPU can be powered on or woken up without Linux intervention, the
same holds true for CPU power down or sleep states. I still see no reason
other than the firmware has no support to talk to RPMH.

> > > On Thu, Oct 11 2018 at 05:20 -0600, Sudeep Holla wrote:
> > > > On Thu, Oct 11, 2018 at 02:50:54AM +0530, Raju P.L.S.S.S.N wrote:
> > > > > Use cpu hotplug callback mechanism to attach/dettach the cpu in
> > > > > the cpu power domain. During cpu hotplug callback registration,
> > > > > the starting callback is invoked on all online cpus. So there is
> > > > > no need to attach from device probe.
> > > > >
> > > >
> > > > To be more explicit, NACK to this series(patches 4-8 to be more specific)
> > > > unless you provide details on:
> > > >
> > > > 1. Why this is not in PSCI implementation ?
> > > This is a linux activity and there is no provision in ATF or QC firmware
> > > to do this activity. The task of physically powering down the domain,
> > > still is a firmware decision and is done through PSCI platform
> > > coordinated in the firmware.
> > >
> >
> > Yes that was my understanding. So the addon question here is: if PSCI
> > decides to abort entering the idle state, the Linux doing the RPMH
> > request is of no use which can be prevented if done once PSCI f/w is
> > about to enter the state. I know it may be corner case, but we have
> > whole OSI mode based on such corner cases.
> >
> Yes, it is a possibility and worth the chance taken. On an SoC, there
> are other processors that may vote against powering down the shared
> resources even when Linux has shutdown and it is a very likely
> possibility. Ex., when you are on a phone call, the CPU subsystem could
> be powered off and the flush of RPMH requests is a 'waste', but it is a
> chance we need to take. The alternate is a synchronization nightmare.
>

I am not against voting here. I am saying need not be done in Linux. The
last piece of software running before powering down is EL3 and it should
so the voting. I can understand for devices controlled in/by Linux. EL3
firmware controls the CPU PM, so that needs to vote and by that it's
assumed nothing is running in lower EL in that path.

> Even with all the unnecessary flushing it is totally worth it. OSI helps
> alleviates this a bit because it embodies the same CPU PD concepts at
> its core. Imagine if you didn't have CPU PM domain, the every CPU would
> be flushing RPMH request, whenever they power down, because you never know
> when all CPUs are going to be powered down at the same time. That is the
> biggest benefit of OSI over PC mode in PSCI.
>

I am not saying every CPU needs to do that, last CPU can do that in PSCI.

> > > > 2. If PSCI is used on this platform, how does it work/co-exist ?
> > > Yes PSCI is used in this platform. ATF is the firmware and that supports
> > > only Platform Coordinated. This SoC uses cpuidle and the regular PSCI
> > > methods to power off the domain from the firmware. However, Linux has
> > > responsibilities that it needs to complete before the power down can be
> > > beneficial.
> > >
> >
> > I understand the need to inform RPMH. So I take only reason to do that
> > in Linux is TF-A doesn't have any support to talk to RPMH ?
> >
> It may or may not, depending on which firmware you talk to. But consider
> this, if the firmware votes for lowering resource state, it is doing it
> for its exception level and not for Linux. So Linux has to take care of
> its own.
>

Oh interesting, wasn't aware RPMH really needs to care about exception
level. For me, we know CPU is powering down, so it needs to release all
the resource. RPMH needs to know that and any exception level can let
RPMH know that. Sorry may be I don't have enough knowledge on SDM SoC.

But IMO Linux shouldn't contribute to any votes around CPU PM as EL3/PSCI
is the one managing it and not Linux. If CPU can be powered up without
Linux voting for anything, why is it required for going down. I simply
fail to understand there and get completely lost :(

> > Now that we have some platform with PC, it's good to compare PC vs OSI
> > which we always lacked. Thanks for letting us know this platform is PC
> > based.
> >
> Can I take that you are okay with the OSI idea and this one, then :)
> Yes, we are close to having a platform have both, possibly.
>

Comparison numbers please :)

> > > Isn't sharing ideas a key aspect of working with the community? This
> > > series just goes to say that the idea of CPU PM domains are useful,
> > > whether PSCI uses it or not. If you still need clarifications, Raju and
> > > I will be happy to set up a meeting and go over the idea.
> > >
> > Ah OK, so this platform will have flattened cpu-idle-states list ? That's
> > absolutely fine. But what if we want to represent hierarchical PSCI based
> > PM domains and this power domain for some platform. That's the main
> > concern I raised. For me, the power-domains in DT introduced in this
> > is just to deal with RPMH though the actual work is done by PSCI.
> > That just doesn't look that good for me.
> >
> What problem do you see with that? It would help if you could clarify
> your concern.
>
Having to adapt DT to the firmware though the feature is fully discoverable
is not at all good IMO. So the DT in this series *should work* with OSI
mode if the firmware has the support for it, it's as simple as that.

--
Regards,
Sudeep

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Lina Iyer]

On Thu, Oct 11 2018 at 11:37 -0600, Sudeep Holla wrote:
>On Thu, Oct 11, 2018 at 10:58:22AM -0600, Lina Iyer wrote:
>> On Thu, Oct 11 2018 at 10:19 -0600, Sudeep Holla wrote:
>> > On Thu, Oct 11, 2018 at 10:00:53AM -0600, Lina Iyer wrote:
>> > > Sudeep,
>> > >
>> > > The CPU PD does not power off the domain from Linux. That is done from
>> > > PSCI firmware (ATF). These patches are doing the part that Linux has do,
>> > > when powering off the CPUs, to achieve a low standby power consumption.
>> > >
>> >
>> > I don't understand why Linux *has do* this part as PSCI manages CPU PM.
>> >
>> If we don't do this, then we leave a lot of power saving on the table,
>> when the CPU powered off. Why should the DDR and the shared busses and
>> clocks be idling at high power, when not needed ? PSCI has no clue to
>> what resource requests was made my Linux and its Linux's responsibility
>> to relinquish them when not needed. Therefore has to done from Linux.
>>
>
>Is DDR managed by Linux ? I assumed it was handled by higher exception
>levels. Can you give examples of resources used by CPU in this context.
>When CPU can be powered on or woken up without Linux intervention, the
>same holds true for CPU power down or sleep states. I still see no reason
>other than the firmware has no support to talk to RPMH.
>
DDR, shared clocks, regulators etc. Imagine you are running something on
the screen and CPUs enter low power mode, while the CPUs were active,
there was a need for bunch of display resources, and things the app may
have requested resources, while the CPU powered down the requests may
not be needed the full extent as when the CPU was running, so they can
voted down to a lower state of in some cases turn off the resources
completely. What the driver voted for is dependent on the runtime state
and the usecase currently active. The 'sleep' state value is also
determined by the driver/framework.

>> Even with all the unnecessary flushing it is totally worth it. OSI helps
>> alleviates this a bit because it embodies the same CPU PD concepts at
>> its core. Imagine if you didn't have CPU PM domain, the every CPU would
>> be flushing RPMH request, whenever they power down, because you never know
>> when all CPUs are going to be powered down at the same time. That is the
>> biggest benefit of OSI over PC mode in PSCI.
>>
>
>I am not saying every CPU needs to do that, last CPU can do that in PSCI.
>
Yes, the last CPU is what we are getting to with this series.. Now this
can't be done from PSCI. I will explain below.

>Oh interesting, wasn't aware RPMH really needs to care about exception
>level. For me, we know CPU is powering down, so it needs to release all
>the resource. RPMH needs to know that and any exception level can let
>RPMH know that. Sorry may be I don't have enough knowledge on SDM SoC.
>
Some resources are secure resources used in secure environments. They
cannot be requested from non-secure. Hence secure levels are voters of
their own accord.

Now, since we are considering linux and secure (infact linux,hyp,secure)
as separate voters they have to each request their votes and release
their votes separately. PSCI cannot release a request made from Linux.
This is how the SoC is designed. All exception levels will abide by
that.

>> Yes, we are close to having a platform have both, possibly.
>>
>
>Comparison numbers please :)
>
We are far from it, for that, atleast now. But we will get there.

>Having to adapt DT to the firmware though the feature is fully discoverable
>is not at all good IMO. So the DT in this series *should work* with OSI
>mode if the firmware has the support for it, it's as simple as that.
>
The firmware is ATF and does not support OSI.

Thanks,
Lina

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Sudeep Holla]

On Thu, Oct 11, 2018 at 03:06:09PM -0600, Lina Iyer wrote:
> On Thu, Oct 11 2018 at 11:37 -0600, Sudeep Holla wrote:
[...]

> >
> > Is DDR managed by Linux ? I assumed it was handled by higher exception
> > levels. Can you give examples of resources used by CPU in this context.
> > When CPU can be powered on or woken up without Linux intervention, the
> > same holds true for CPU power down or sleep states. I still see no reason
> > other than the firmware has no support to talk to RPMH.
> >
> DDR, shared clocks, regulators etc. Imagine you are running something on
> the screen and CPUs enter low power mode, while the CPUs were active,
> there was a need for bunch of display resources, and things the app may
> have requested resources, while the CPU powered down the requests may
> not be needed the full extent as when the CPU was running, so they can
> voted down to a lower state of in some cases turn off the resources
> completely. What the driver voted for is dependent on the runtime state
> and the usecase currently active. The 'sleep' state value is also
> determined by the driver/framework.
>

Why does CPU going down says that another (screen - supposedly shared)
resource needs to be relinquished ? Shouldn't display decide that on it's
own ? I have no idea why screen/display is brought into this discussion.
CPU can just say: hey I am going down and I don't need my resource.
How can it say: hey I am going down and display or screen also doesn't
need the resource. On a multi-cluster, how will the last CPU on one know
that it needs to act on behalf of the shared resource instead of another
cluster.

I think we are mixing the system sleep states with CPU idle here.
If it's system sleeps states, the we need to deal it in some system ops
when it's the last CPU in the system and not the cluster/power domain.

[...]

> > Oh interesting, wasn't aware RPMH really needs to care about exception
> > level. For me, we know CPU is powering down, so it needs to release all
> > the resource. RPMH needs to know that and any exception level can let
> > RPMH know that. Sorry may be I don't have enough knowledge on SDM SoC.
> >
> Some resources are secure resources used in secure environments. They
> cannot be requested from non-secure. Hence secure levels are voters of
> their own accord.
>

I still don't think RPMH can more than refcounting and all I am saying
is for CPU's EL3 can manage that refcount on behalf of all ELx

> Now, since we are considering linux and secure (infact linux,hyp,secure)
> as separate voters they have to each request their votes and release
> their votes separately. PSCI cannot release a request made from Linux.

Why should Linux make that request at the first instance as CPU is
already up and running.

> This is how the SoC is designed. All exception levels will abide by
> that.
>
> > > Yes, we are close to having a platform have both, possibly.
> > >
> >
> > Comparison numbers please :)
> >
> We are far from it, for that, atleast now. But we will get there.
>

Hopefully one day. We are waiting for that day for few years now :)

> > Having to adapt DT to the firmware though the feature is fully discoverable
> > is not at all good IMO. So the DT in this series *should work* with OSI
> > mode if the firmware has the support for it, it's as simple as that.
> >
> The firmware is ATF and does not support OSI.
>

OK, to keep it simple: If a platform with PC mode only replaces the firmware
with one that has OSI mode, we *shouldn't need* to change DT to suite it.
I think I asked Ulf to add something similar in DT bindings.

--
Regards,
Sudeep

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Ulf Hansson]

On 12 October 2018 at 17:04, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Thu, Oct 11, 2018 at 03:06:09PM -0600, Lina Iyer wrote:
>> On Thu, Oct 11 2018 at 11:37 -0600, Sudeep Holla wrote:
> [...]
>
>> >
>> > Is DDR managed by Linux ? I assumed it was handled by higher exception
>> > levels. Can you give examples of resources used by CPU in this context.
>> > When CPU can be powered on or woken up without Linux intervention, the
>> > same holds true for CPU power down or sleep states. I still see no reason
>> > other than the firmware has no support to talk to RPMH.
>> >
>> DDR, shared clocks, regulators etc. Imagine you are running something on
>> the screen and CPUs enter low power mode, while the CPUs were active,
>> there was a need for bunch of display resources, and things the app may
>> have requested resources, while the CPU powered down the requests may
>> not be needed the full extent as when the CPU was running, so they can
>> voted down to a lower state of in some cases turn off the resources
>> completely. What the driver voted for is dependent on the runtime state
>> and the usecase currently active. The 'sleep' state value is also
>> determined by the driver/framework.
>>
>
> Why does CPU going down says that another (screen - supposedly shared)
> resource needs to be relinquished ? Shouldn't display decide that on it's
> own ? I have no idea why screen/display is brought into this discussion.
> CPU can just say: hey I am going down and I don't need my resource.
> How can it say: hey I am going down and display or screen also doesn't
> need the resource. On a multi-cluster, how will the last CPU on one know
> that it needs to act on behalf of the shared resource instead of another
> cluster.

Apologize for sidetracking the discussion, just want to fold in a few comments.

This is becoming a complicated story. May I suggest we pick the GIC as
an example instead?

Let's assume the simple case, we have one cluster and when the cluster
becomes powered off, the GIC needs to be re-configured and wakeups
must be routed through some "always on" external logic.

The PSCI spec mentions nothing about how to manage this and not the
rest of the SoC topology for that matter. Hence if the GIC is managed
by Linux - then Linux also needs to take actions before cluster power
down and after cluster power up. So, if PSCI FW can't deal with GIC,
how would manage it?

>
> I think we are mixing the system sleep states with CPU idle here.
> If it's system sleeps states, the we need to deal it in some system ops
> when it's the last CPU in the system and not the cluster/power domain.

What is really a system sleep state? One could consider it just being
another idles state, having heaver residency targets and greater
enter/exit latency values, couldn't you?

In the end, there is no reason to keep things powered on, unless they
are being in used (or soon to be used), that is main point.

We are also working on S2I at Linaro. We strive towards being able to
show the same power numbers as for S2R, but then we need to get these
cluster-idle things right.

[...]

Have a nice weekend!

Kind regards
Uffe

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Lina Iyer]

On Fri, Oct 12 2018 at 09:46 -0600, Ulf Hansson wrote:
>On 12 October 2018 at 17:04, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On Thu, Oct 11, 2018 at 03:06:09PM -0600, Lina Iyer wrote:
>>> On Thu, Oct 11 2018 at 11:37 -0600, Sudeep Holla wrote:
>> [...]
>>
>>> >
>>> > Is DDR managed by Linux ? I assumed it was handled by higher exception
>>> > levels. Can you give examples of resources used by CPU in this context.
>>> > When CPU can be powered on or woken up without Linux intervention, the
>>> > same holds true for CPU power down or sleep states. I still see no reason
>>> > other than the firmware has no support to talk to RPMH.
>>> >
>>> DDR, shared clocks, regulators etc. Imagine you are running something on
>>> the screen and CPUs enter low power mode, while the CPUs were active,
>>> there was a need for bunch of display resources, and things the app may
>>> have requested resources, while the CPU powered down the requests may
>>> not be needed the full extent as when the CPU was running, so they can
>>> voted down to a lower state of in some cases turn off the resources
>>> completely. What the driver voted for is dependent on the runtime state
>>> and the usecase currently active. The 'sleep' state value is also
>>> determined by the driver/framework.
>>>
>>
>> Why does CPU going down says that another (screen - supposedly shared)
>> resource needs to be relinquished ? Shouldn't display decide that on it's
>> own ? I have no idea why screen/display is brought into this discussion.
>> CPU can just say: hey I am going down and I don't need my resource.
>> How can it say: hey I am going down and display or screen also doesn't
>> need the resource. On a multi-cluster, how will the last CPU on one know
>> that it needs to act on behalf of the shared resource instead of another
>> cluster.
>
>Apologize for sidetracking the discussion, just want to fold in a few comments.
>
No, this is perfect to warp the whole thing around. Thanks Ulf.

>This is becoming a complicated story. May I suggest we pick the GIC as
>an example instead?
>
>Let's assume the simple case, we have one cluster and when the cluster
>becomes powered off, the GIC needs to be re-configured and wakeups
>must be routed through some "always on" external logic.
>
>The PSCI spec mentions nothing about how to manage this and not the
>rest of the SoC topology for that matter. Hence if the GIC is managed
>by Linux - then Linux also needs to take actions before cluster power
>down and after cluster power up. So, if PSCI FW can't deal with GIC,
>how would manage it?
>
>>
>> I think we are mixing the system sleep states with CPU idle here.
>> If it's system sleeps states, the we need to deal it in some system ops
>> when it's the last CPU in the system and not the cluster/power domain.
>
>What is really a system sleep state? One could consider it just being
>another idles state, having heaver residency targets and greater
>enter/exit latency values, couldn't you?
>
While I explained the driver's side of the story, for the CPUs, system
sleep is a deeper low power mode that ties in with OSI.

Thanks,
Lina

>In the end, there is no reason to keep things powered on, unless they
>are being in used (or soon to be used), that is main point.
>
>We are also working on S2I at Linaro. We strive towards being able to
>show the same power numbers as for S2R, but then we need to get these
>cluster-idle things right.
>
>[...]
>
>Have a nice weekend!
>
>Kind regards
>Uffe

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Sudeep Holla]

On Fri, Oct 12, 2018 at 05:46:13PM +0200, Ulf Hansson wrote:
[...]

>
> Apologize for sidetracking the discussion, just want to fold in a few comments.
>
No need to apologize, you are just contributing to get better
understanding of the system.

> This is becoming a complicated story. May I suggest we pick the GIC as
> an example instead?
>

Sure.

> Let's assume the simple case, we have one cluster and when the cluster
> becomes powered off, the GIC needs to be re-configured and wakeups
> must be routed through some "always on" external logic.
>

OK, but is the cluster powered off with any wakeup configured(idling)
or with selected wakeups(system suspend/idle)

> The PSCI spec mentions nothing about how to manage this and not the
> rest of the SoC topology for that matter. Hence if the GIC is managed
> by Linux - then Linux also needs to take actions before cluster power
> down and after cluster power up. So, if PSCI FW can't deal with GIC,
> how would manage it?
>

To add to the complications, some of the configurations in GIC can be
done only in higher exception level. So we expect PSCI to power down
the GIC if possible and move the wakeup source accordingly based on the
platform. So PSCI F/W unable to deal with GIC is simply impossible.
It configures Group0/1 interrupts and generally Linux deals with Group 1.
E.g. First 8 SGI are put in Group 0 which is secure interrupts.

So by default, GIC driver in Linux sets MASK_ON_SUSPEND which ensures
only wake-up sources are kept enabled before entering suspend. If the
GIC is being powered down, secure side has to do it's book-keeping if
any and transfer the wakeups to any external always on wakeup controller.

> >
> > I think we are mixing the system sleep states with CPU idle here.
> > If it's system sleeps states, the we need to deal it in some system ops
> > when it's the last CPU in the system and not the cluster/power domain.
>
> What is really a system sleep state? One could consider it just being
> another idles state, having heaver residency targets and greater
> enter/exit latency values, couldn't you?
>

Yes, but theses are user triggered states where the system resources are
informed before entering it. By that I am referring system_suspend_ops.

> In the end, there is no reason to keep things powered on, unless they
> are being in used (or soon to be used), that is main point.
>

I assume by "they", you refer the GIC for example.

> We are also working on S2I at Linaro. We strive towards being able to
> show the same power numbers as for S2R, but then we need to get these
> cluster-idle things right.
>

I don't think anything prevents it. You may need to check how to execute
cpu_pm_suspend which in case S2R gets executed as syscore_suspend_ops.

We can't assume any idle states will take the GIC down and do that
always. At the same, representing this is DT is equal challenging as we
can't assume single idle state power domains. So the platform specific
firmware need to handle this transparently for OSPM.

> [...]
>
> Have a nice weekend!
>

You too have a nice one.

--
Regards,
Sudeep

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Lina Iyer]

On Fri, Oct 12 2018 at 09:04 -0600, Sudeep Holla wrote:
>On Thu, Oct 11, 2018 at 03:06:09PM -0600, Lina Iyer wrote:
>> On Thu, Oct 11 2018 at 11:37 -0600, Sudeep Holla wrote:
>[...]
>
>> >
>> > Is DDR managed by Linux ? I assumed it was handled by higher exception
>> > levels. Can you give examples of resources used by CPU in this context.
>> > When CPU can be powered on or woken up without Linux intervention, the
>> > same holds true for CPU power down or sleep states. I still see no reason
>> > other than the firmware has no support to talk to RPMH.
>> >
>> DDR, shared clocks, regulators etc. Imagine you are running something on
>> the screen and CPUs enter low power mode, while the CPUs were active,
>> there was a need for bunch of display resources, and things the app may
>> have requested resources, while the CPU powered down the requests may
>> not be needed the full extent as when the CPU was running, so they can
>> voted down to a lower state of in some cases turn off the resources
>> completely. What the driver voted for is dependent on the runtime state
>> and the usecase currently active. The 'sleep' state value is also
>> determined by the driver/framework.
>>
>
>Why does CPU going down says that another (screen - supposedly shared)
>resource needs to be relinquished ? Shouldn't display decide that on it's
>own ? I have no idea why screen/display is brought into this discussion.

>CPU can just say: hey I am going down and I don't need my resource.
>How can it say: hey I am going down and display or screen also doesn't
>need the resource. On a multi-cluster, how will the last CPU on one know
>that it needs to act on behalf of the shared resource instead of another
>cluster.
>
Fair questions. Now how would the driver know that the CPUs have powered
down, to say, if you are not active, then you can put these resources in
low power state?
Well they don't, because sending out CPU power down notifications for
all CPUs and the cluster are expensive and can lead to lot of latency.
Instead, the drivers let the RPMH driver know that if and when the CPUs
power down, then you could request these resources to be in that low
power state. The CPU PD power off callbacks trigger the RPMH driver to
flush and request a low power state on behalf of all the drivers.

Drivers let know what their active state request for the resource is as
well as their CPU powered down state request is, in advance. The
'active' request is made immediately, while the 'sleep' request is
staged in. When the CPUs are to be powered off, this request is written
into a hardware registers. The CPU PM domain controller, after powering
down, will make these state requests in hardware thereby lowering the
standby power. The resource state is brought back into the 'active'
value before powering on the first CPU.

>I think we are mixing the system sleep states with CPU idle here.
>If it's system sleeps states, the we need to deal it in some system ops
>when it's the last CPU in the system and not the cluster/power domain.
>
I think the confusion for you is system sleep vs suspend. System sleep
here (probably more of a QC terminology), refers to powering down the
entire SoC for very small durations, while not actually suspended. The
drivers are unaware that this is happening. No hotplug happens and the
interrupts are not migrated during system sleep. When all the CPUs go
into cpuidle, the system sleep state is activated and the resource
requirements are lowered. The resources are brought back to their
previous active values before we exit cpuidle on any CPU. The drivers
have no idea that this happened. We have been doing this on QCOM SoCs
for a decade, so this is not something new for this SoC. Every QCOM SoC
has been doing this, albeit differently because of their architecture.
The newer ones do most of these transitions in hardware as opposed to an
remote CPU. But this is the first time, we are upstreaming this :)

Suspend is an altogether another idle state where drivers are notified
and relinquish their resources before the CPU powers down. Similar
things happen there as well, but at a much deeper level. Resources may
be turned off completely instead of just lowering to a low power state.

For example, suspend happens when the screen times out on a phone.
System sleep happens few hundred times when you are actively reading
something on the phone.

>> > Having to adapt DT to the firmware though the feature is fully discoverable
>> > is not at all good IMO. So the DT in this series *should work* with OSI
>> > mode if the firmware has the support for it, it's as simple as that.
>> >
>> The firmware is ATF and does not support OSI.
>>
>
>OK, to keep it simple: If a platform with PC mode only replaces the firmware
>with one that has OSI mode, we *shouldn't need* to change DT to suite it.
>I think I asked Ulf to add something similar in DT bindings.
>
Fair point and that is what this RFC intends to bring. That PM domains
are useful not just for PSCI, but also for Linux PM drivers such as this
one. We will discuss more how we can fold in platform specific
activities along with PSCI OSI state determination when the
domain->power_off is called. I have some ideas on that. Was hoping to
get to that after the inital idea is conveyed.

Thanks for your time.

Lina

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Sudeep Holla]

On Fri, Oct 12, 2018 at 10:04:27AM -0600, Lina Iyer wrote:
> On Fri, Oct 12 2018 at 09:04 -0600, Sudeep Holla wrote:

[...]

> > Why does CPU going down says that another (screen - supposedly shared)
> > resource needs to be relinquished ? Shouldn't display decide that on it's
> > own ? I have no idea why screen/display is brought into this discussion.
>
> > CPU can just say: hey I am going down and I don't need my resource.
> > How can it say: hey I am going down and display or screen also doesn't
> > need the resource. On a multi-cluster, how will the last CPU on one know
> > that it needs to act on behalf of the shared resource instead of another
> > cluster.
> >
> Fair questions. Now how would the driver know that the CPUs have powered
> down, to say, if you are not active, then you can put these resources in
> low power state?
> Well they don't, because sending out CPU power down notifications for
> all CPUs and the cluster are expensive and can lead to lot of latency.
> Instead, the drivers let the RPMH driver know that if and when the CPUs
> power down, then you could request these resources to be in that low
> power state. The CPU PD power off callbacks trigger the RPMH driver to
> flush and request a low power state on behalf of all the drivers.
>
> Drivers let know what their active state request for the resource is as
> well as their CPU powered down state request is, in advance. The
> 'active' request is made immediately, while the 'sleep' request is
> staged in. When the CPUs are to be powered off, this request is written
> into a hardware registers. The CPU PM domain controller, after powering
> down, will make these state requests in hardware thereby lowering the
> standby power. The resource state is brought back into the 'active'
> value before powering on the first CPU.
>

My understanding was in sync with most of the above except the staging
part in advance. So thanks for the detailed explanation.

Yes all these are fine but with multiple power-domains/cluster, it's
hard to determine the first CPU. You may be able to identify it within
the power domain but not system wide. So this doesn't scale with large
systems(e.g. 4 - 8 clusters with 16 CPUs).

> > I think we are mixing the system sleep states with CPU idle here.
> > If it's system sleeps states, the we need to deal it in some system ops
> > when it's the last CPU in the system and not the cluster/power domain.
> >
> I think the confusion for you is system sleep vs suspend. System sleep
> here (probably more of a QC terminology), refers to powering down the
> entire SoC for very small durations, while not actually suspended. The
> drivers are unaware that this is happening. No hotplug happens and the
> interrupts are not migrated during system sleep. When all the CPUs go
> into cpuidle, the system sleep state is activated and the resource
> requirements are lowered. The resources are brought back to their
> previous active values before we exit cpuidle on any CPU. The drivers
> have no idea that this happened. We have been doing this on QCOM SoCs
> for a decade, so this is not something new for this SoC. Every QCOM SoC
> has been doing this, albeit differently because of their architecture.
> The newer ones do most of these transitions in hardware as opposed to an
> remote CPU. But this is the first time, we are upstreaming this :)
>

Indeed, I know mobile platforms do such optimisations and I agree it may
save power. As I mentioned above it doesn't scale well with large systems
and also even with single power domains having multiple idle states where
only one state can do this system level idle but not all. As I mentioned
in the other email to Ulf, it's had to generalise this even with DT.
So it's better to have this dealt transparently in the firmware.

> Suspend is an altogether another idle state where drivers are notified
> and relinquish their resources before the CPU powers down. Similar
> things happen there as well, but at a much deeper level. Resources may
> be turned off completely instead of just lowering to a low power state.
>

Yes I understand the difference.

> For example, suspend happens when the screen times out on a phone.
> System sleep happens few hundred times when you are actively reading
> something on the phone.
>

Sure

> > > > Having to adapt DT to the firmware though the feature is fully discoverable
> > > > is not at all good IMO. So the DT in this series *should work* with OSI
> > > > mode if the firmware has the support for it, it's as simple as that.
> > > >
> > > The firmware is ATF and does not support OSI.
> > >
> >
> > OK, to keep it simple: If a platform with PC mode only replaces the firmware
> > with one that has OSI mode, we *shouldn't need* to change DT to suite it.
> > I think I asked Ulf to add something similar in DT bindings.
> >
> Fair point and that is what this RFC intends to bring. That PM domains
> are useful not just for PSCI, but also for Linux PM drivers such as this
> one. We will discuss more how we can fold in platform specific
> activities along with PSCI OSI state determination when the
> domain->power_off is called. I have some ideas on that. Was hoping to
> get to that after the inital idea is conveyed.
>

Got it. This is not a new discussion, I am sure this has been discussed
several times in the past. We have so much platform dependent code that
coming up with generic solution with DT is challenging. I have mentioned
just few of those. I am sure the list is much bigger. Hence the suggestion
is always to got with firmware based solution which is bested suited for
upstream and proven to work(e.g. on x86).

Have a nice weekend!

--
Regards,
Sudeep

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Lina Iyer]

On Fri, Oct 12 2018 at 11:01 -0600, Sudeep Holla wrote:
>On Fri, Oct 12, 2018 at 10:04:27AM -0600, Lina Iyer wrote:
>> On Fri, Oct 12 2018 at 09:04 -0600, Sudeep Holla wrote:
>
>[...]
>
>Yes all these are fine but with multiple power-domains/cluster, it's
>hard to determine the first CPU. You may be able to identify it within
>the power domain but not system wide. So this doesn't scale with large
>systems(e.g. 4 - 8 clusters with 16 CPUs).
>
We would probably not worry too much about power savings in a msec
scale, if we have that big a system. The driver is a platform specific
driver, primarily intended for a mobile class CPU and usage. In fact, we
haven't done this for QC's server class CPUs.

>> > I think we are mixing the system sleep states with CPU idle here.
>> > If it's system sleeps states, the we need to deal it in some system ops
>> > when it's the last CPU in the system and not the cluster/power domain.
>> >
>> I think the confusion for you is system sleep vs suspend. System sleep
>> here (probably more of a QC terminology), refers to powering down the
>> entire SoC for very small durations, while not actually suspended. The
>> drivers are unaware that this is happening. No hotplug happens and the
>> interrupts are not migrated during system sleep. When all the CPUs go
>> into cpuidle, the system sleep state is activated and the resource
>> requirements are lowered. The resources are brought back to their
>> previous active values before we exit cpuidle on any CPU. The drivers
>> have no idea that this happened. We have been doing this on QCOM SoCs
>> for a decade, so this is not something new for this SoC. Every QCOM SoC
>> has been doing this, albeit differently because of their architecture.
>> The newer ones do most of these transitions in hardware as opposed to an
>> remote CPU. But this is the first time, we are upstreaming this :)
>>
>
>Indeed, I know mobile platforms do such optimisations and I agree it may
>save power. As I mentioned above it doesn't scale well with large systems
>and also even with single power domains having multiple idle states where
>only one state can do this system level idle but not all. As I mentioned
>in the other email to Ulf, it's had to generalise this even with DT.
>So it's better to have this dealt transparently in the firmware.
>
Good, then we are on agreement here.
But this is how this platform is. It cannot be done in firmware and what
we doing here is a Linux platform driver that cleans up nicely without
having to piggy back on an external dependency.

Thanks,
Lina

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Sudeep Holla]

On Fri, Oct 12, 2018 at 11:19:10AM -0600, Lina Iyer wrote:
> On Fri, Oct 12 2018 at 11:01 -0600, Sudeep Holla wrote:
> > On Fri, Oct 12, 2018 at 10:04:27AM -0600, Lina Iyer wrote:
> > > On Fri, Oct 12 2018 at 09:04 -0600, Sudeep Holla wrote:
> >
> > [...]
> >
> > Yes all these are fine but with multiple power-domains/cluster, it's
> > hard to determine the first CPU. You may be able to identify it within
> > the power domain but not system wide. So this doesn't scale with large
> > systems(e.g. 4 - 8 clusters with 16 CPUs).
> >
> We would probably not worry too much about power savings in a msec
> scale, if we have that big a system. The driver is a platform specific
> driver, primarily intended for a mobile class CPU and usage. In fact, we
> haven't done this for QC's server class CPUs.
>

OK, along as there's no attempt to make it generic and keep it platform
specific, I am not that bothered.

> > > > I think we are mixing the system sleep states with CPU idle here.
> > > > If it's system sleeps states, the we need to deal it in some system ops
> > > > when it's the last CPU in the system and not the cluster/power domain.
> > > >
> > > I think the confusion for you is system sleep vs suspend. System sleep
> > > here (probably more of a QC terminology), refers to powering down the
> > > entire SoC for very small durations, while not actually suspended. The
> > > drivers are unaware that this is happening. No hotplug happens and the
> > > interrupts are not migrated during system sleep. When all the CPUs go
> > > into cpuidle, the system sleep state is activated and the resource
> > > requirements are lowered. The resources are brought back to their
> > > previous active values before we exit cpuidle on any CPU. The drivers
> > > have no idea that this happened. We have been doing this on QCOM SoCs
> > > for a decade, so this is not something new for this SoC. Every QCOM SoC
> > > has been doing this, albeit differently because of their architecture.
> > > The newer ones do most of these transitions in hardware as opposed to an
> > > remote CPU. But this is the first time, we are upstreaming this :)
> > >
> >
> > Indeed, I know mobile platforms do such optimisations and I agree it may
> > save power. As I mentioned above it doesn't scale well with large systems
> > and also even with single power domains having multiple idle states where
> > only one state can do this system level idle but not all. As I mentioned
> > in the other email to Ulf, it's had to generalise this even with DT.
> > So it's better to have this dealt transparently in the firmware.
> >
> Good, then we are on agreement here.

No worries.

> But this is how this platform is. It cannot be done in firmware and what
> we doing here is a Linux platform driver that cleans up nicely without
> having to piggy back on an external dependency.
>
Yes Qcom always says it can't be done in firmware. Even PSCI was adopted 
after couple of years of pushback.

--
Regards,
Sudeep

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Lina Iyer]

On Fri, Oct 12 2018 at 11:25 -0600, Sudeep Holla wrote:
>On Fri, Oct 12, 2018 at 11:19:10AM -0600, Lina Iyer wrote:
>> On Fri, Oct 12 2018 at 11:01 -0600, Sudeep Holla wrote:
>> > On Fri, Oct 12, 2018 at 10:04:27AM -0600, Lina Iyer wrote:
>> > > On Fri, Oct 12 2018 at 09:04 -0600, Sudeep Holla wrote:
>> >
>> > [...]
>> >
>> > Yes all these are fine but with multiple power-domains/cluster, it's
>> > hard to determine the first CPU. You may be able to identify it within
>> > the power domain but not system wide. So this doesn't scale with large
>> > systems(e.g. 4 - 8 clusters with 16 CPUs).
>> >
>> We would probably not worry too much about power savings in a msec
>> scale, if we have that big a system. The driver is a platform specific
>> driver, primarily intended for a mobile class CPU and usage. In fact, we
>> haven't done this for QC's server class CPUs.
>>
>
>OK, along as there's no attempt to make it generic and keep it platform
>specific, I am not that bothered.
>
>> > > > I think we are mixing the system sleep states with CPU idle here.
>> > > > If it's system sleeps states, the we need to deal it in some system ops
>> > > > when it's the last CPU in the system and not the cluster/power domain.
>> > > >
>> > > I think the confusion for you is system sleep vs suspend. System sleep
>> > > here (probably more of a QC terminology), refers to powering down the
>> > > entire SoC for very small durations, while not actually suspended. The
>> > > drivers are unaware that this is happening. No hotplug happens and the
>> > > interrupts are not migrated during system sleep. When all the CPUs go
>> > > into cpuidle, the system sleep state is activated and the resource
>> > > requirements are lowered. The resources are brought back to their
>> > > previous active values before we exit cpuidle on any CPU. The drivers
>> > > have no idea that this happened. We have been doing this on QCOM SoCs
>> > > for a decade, so this is not something new for this SoC. Every QCOM SoC
>> > > has been doing this, albeit differently because of their architecture.
>> > > The newer ones do most of these transitions in hardware as opposed to an
>> > > remote CPU. But this is the first time, we are upstreaming this :)
>> > >
>> >
>> > Indeed, I know mobile platforms do such optimisations and I agree it may
>> > save power. As I mentioned above it doesn't scale well with large systems
>> > and also even with single power domains having multiple idle states where
>> > only one state can do this system level idle but not all. As I mentioned
>> > in the other email to Ulf, it's had to generalise this even with DT.
>> > So it's better to have this dealt transparently in the firmware.
>> >
>> Good, then we are on agreement here.
>
It was brought to my attention that there may be some misunderstanding
here. I still believe we need to do this for small systems like the
mobile platforms and the solution may not scale well to servers. We
don't plan to extend the solution to anything other than the mobile SoC.

>No worries.
>
Thanks,
Lina

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Sudeep Holla]

On Thu, Oct 11, 2018 at 02:50:54AM +0530, Raju P.L.S.S.S.N wrote:
> Use cpu hotplug callback mechanism to attach/dettach the cpu in
> the cpu power domain. During cpu hotplug callback registration,
> the starting callback is invoked on all online cpus. So there is
> no need to attach from device probe.
> 
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> ---
>  drivers/soc/qcom/cpu_pd.c  | 33 +++++++++++++++++++++++++--------
>  include/linux/cpuhotplug.h |  1 +
>  2 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/qcom/cpu_pd.c b/drivers/soc/qcom/cpu_pd.c
> index 242eced..bf2d95d 100644
> --- a/drivers/soc/qcom/cpu_pd.c
> +++ b/drivers/soc/qcom/cpu_pd.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>   */
>  
> +#include <linux/cpuhotplug.h>
>  #include <linux/ktime.h>
>  #include <linux/of_platform.h>
>  #include <linux/pm_domain.h>
> @@ -85,12 +86,26 @@ static int cpu_pd_power_off(struct generic_pm_domain *domain)
>  	return 0;
>  }
>  
> +static int cpu_pd_starting(unsigned int cpu)
> +{
> +	if (!suspend && of_genpd_attach_cpu(cpu))
> +		pr_err("%s: genpd_attach_cpu fail\n", __func__);

If it's that serious, return proper error code and drop the message which
could be annoying if you are running some hotplug test loop.

--
Regards,
Sudeep

Re: [PATCH RFC v1 7/8] drivers: qcom: cpu_pd: Handle cpu hotplug in the domain

[Raju P L S S S N]

On 10/12/2018 7:55 PM, Sudeep Holla wrote:
> On Thu, Oct 11, 2018 at 02:50:54AM +0530, Raju P.L.S.S.S.N wrote:
>> Use cpu hotplug callback mechanism to attach/dettach the cpu in
>> the cpu power domain. During cpu hotplug callback registration,
>> the starting callback is invoked on all online cpus. So there is
>> no need to attach from device probe.
>>
>> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
>> ---
>>   drivers/soc/qcom/cpu_pd.c  | 33 +++++++++++++++++++++++++--------
>>   include/linux/cpuhotplug.h |  1 +
>>   2 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/cpu_pd.c b/drivers/soc/qcom/cpu_pd.c
>> index 242eced..bf2d95d 100644
>> --- a/drivers/soc/qcom/cpu_pd.c
>> +++ b/drivers/soc/qcom/cpu_pd.c
>> @@ -3,6 +3,7 @@
>>    * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>    */
>>   
>> +#include <linux/cpuhotplug.h>
>>   #include <linux/ktime.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/pm_domain.h>
>> @@ -85,12 +86,26 @@ static int cpu_pd_power_off(struct generic_pm_domain *domain)
>>   	return 0;
>>   }
>>   
>> +static int cpu_pd_starting(unsigned int cpu)
>> +{
>> +	if (!suspend && of_genpd_attach_cpu(cpu))
>> +		pr_err("%s: genpd_attach_cpu fail\n", __func__);
> 
> If it's that serious, return proper error code and drop the message which
> could be annoying if you are running some hotplug test loop.

Sure will do that.

Thanks Sudeep.

- Raju

> 
> --
> Regards,
> Sudeep
> 

[PATCH RFC v1 8/8] arm64: dtsi: sdm845: Add cpu power domain support

[Raju P.L.S.S.S.N]

Add cpu power domain support

Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index d3662a8..aadaa20 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -96,6 +96,7 @@
 			reg = <0x0 0x0>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
+			power-domains = <&cpu_pd>;
 			L2_0: l2-cache {
 				compatible = "cache";
 				next-level-cache = <&L3_0>;
@@ -111,6 +112,7 @@
 			reg = <0x0 0x100>;
 			enable-method = "psci";
 			next-level-cache = <&L2_100>;
+			power-domains = <&cpu_pd>;
 			L2_100: l2-cache {
 				compatible = "cache";
 				next-level-cache = <&L3_0>;
@@ -123,6 +125,7 @@
 			reg = <0x0 0x200>;
 			enable-method = "psci";
 			next-level-cache = <&L2_200>;
+			power-domains = <&cpu_pd>;
 			L2_200: l2-cache {
 				compatible = "cache";
 				next-level-cache = <&L3_0>;
@@ -135,6 +138,7 @@
 			reg = <0x0 0x300>;
 			enable-method = "psci";
 			next-level-cache = <&L2_300>;
+			power-domains = <&cpu_pd>;
 			L2_300: l2-cache {
 				compatible = "cache";
 				next-level-cache = <&L3_0>;
@@ -147,6 +151,7 @@
 			reg = <0x0 0x400>;
 			enable-method = "psci";
 			next-level-cache = <&L2_400>;
+			power-domains = <&cpu_pd>;
 			L2_400: l2-cache {
 				compatible = "cache";
 				next-level-cache = <&L3_0>;
@@ -159,6 +164,7 @@
 			reg = <0x0 0x500>;
 			enable-method = "psci";
 			next-level-cache = <&L2_500>;
+			power-domains = <&cpu_pd>;
 			L2_500: l2-cache {
 				compatible = "cache";
 				next-level-cache = <&L3_0>;
@@ -171,6 +177,7 @@
 			reg = <0x0 0x600>;
 			enable-method = "psci";
 			next-level-cache = <&L2_600>;
+			power-domains = <&cpu_pd>;
 			L2_600: l2-cache {
 				compatible = "cache";
 				next-level-cache = <&L3_0>;
@@ -183,6 +190,7 @@
 			reg = <0x0 0x700>;
 			enable-method = "psci";
 			next-level-cache = <&L2_700>;
+			power-domains = <&cpu_pd>;
 			L2_700: l2-cache {
 				compatible = "cache";
 				next-level-cache = <&L3_0>;
@@ -1170,6 +1178,11 @@
 					  <WAKE_TCS    3>,
 					  <CONTROL_TCS 1>;
 
+			cpu_pd: power-domain-controller {
+				compatible = "qcom,cpu-pm-domain";
+				#power-domain-cells = <0>;
+			};
+
 			rpmhcc: clock-controller {
 				compatible = "qcom,sdm845-rpmh-clk";
 				#clock-cells = <1>;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH RFC v1 8/8] arm64: dtsi: sdm845: Add cpu power domain support

[Sudeep Holla]

On Thu, Oct 11, 2018 at 02:50:55AM +0530, Raju P.L.S.S.S.N wrote:
> Add cpu power domain support
> 
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index d3662a8..aadaa20 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -96,6 +96,7 @@
>  			reg = <0x0 0x0>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_0>;
> +			power-domains = <&cpu_pd>;
>  			L2_0: l2-cache {
>  				compatible = "cache";
>  				next-level-cache = <&L3_0>;
> @@ -111,6 +112,7 @@
>  			reg = <0x0 0x100>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_100>;
> +			power-domains = <&cpu_pd>;
>  			L2_100: l2-cache {
>  				compatible = "cache";
>  				next-level-cache = <&L3_0>;
> @@ -123,6 +125,7 @@
>  			reg = <0x0 0x200>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_200>;
> +			power-domains = <&cpu_pd>;
>  			L2_200: l2-cache {
>  				compatible = "cache";
>  				next-level-cache = <&L3_0>;
> @@ -135,6 +138,7 @@
>  			reg = <0x0 0x300>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_300>;
> +			power-domains = <&cpu_pd>;
>  			L2_300: l2-cache {
>  				compatible = "cache";
>  				next-level-cache = <&L3_0>;
> @@ -147,6 +151,7 @@
>  			reg = <0x0 0x400>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_400>;
> +			power-domains = <&cpu_pd>;
>  			L2_400: l2-cache {
>  				compatible = "cache";
>  				next-level-cache = <&L3_0>;
> @@ -159,6 +164,7 @@
>  			reg = <0x0 0x500>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_500>;
> +			power-domains = <&cpu_pd>;
>  			L2_500: l2-cache {
>  				compatible = "cache";
>  				next-level-cache = <&L3_0>;
> @@ -171,6 +177,7 @@
>  			reg = <0x0 0x600>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_600>;
> +			power-domains = <&cpu_pd>;
>  			L2_600: l2-cache {
>  				compatible = "cache";
>  				next-level-cache = <&L3_0>;
> @@ -183,6 +190,7 @@
>  			reg = <0x0 0x700>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_700>;
> +			power-domains = <&cpu_pd>;
>  			L2_700: l2-cache {
>  				compatible = "cache";
>  				next-level-cache = <&L3_0>;
> @@ -1170,6 +1178,11 @@
>  					  <WAKE_TCS    3>,
>  					  <CONTROL_TCS 1>;
>  
> +			cpu_pd: power-domain-controller {
> +				compatible = "qcom,cpu-pm-domain";
> +				#power-domain-cells = <0>;
> +			};
> +

After all the discussions, I see this power domain actually influence
not just CPUs but other devices. So this should be top most power domain
in the system with lots of devices or their power domains pointing to it.
Why is this just pointing to cpus ?

--
Regards,
Sudeep

Re: [PATCH RFC v1 8/8] arm64: dtsi: sdm845: Add cpu power domain support

[Lina Iyer]

On Fri, Oct 12 2018 at 11:35 -0600, Sudeep Holla wrote:
>On Thu, Oct 11, 2018 at 02:50:55AM +0530, Raju P.L.S.S.S.N wrote:
>> Add cpu power domain support
>>
>> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
>> ---
>>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index d3662a8..aadaa20 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -96,6 +96,7 @@
>>  			reg = <0x0 0x0>;
>>  			enable-method = "psci";
>>  			next-level-cache = <&L2_0>;
>> +			power-domains = <&cpu_pd>;
>>  			L2_0: l2-cache {
>>  				compatible = "cache";
>>  				next-level-cache = <&L3_0>;
>> @@ -111,6 +112,7 @@
>>  			reg = <0x0 0x100>;
>>  			enable-method = "psci";
>>  			next-level-cache = <&L2_100>;
>> +			power-domains = <&cpu_pd>;
>>  			L2_100: l2-cache {
>>  				compatible = "cache";
>>  				next-level-cache = <&L3_0>;
>> @@ -123,6 +125,7 @@
>>  			reg = <0x0 0x200>;
>>  			enable-method = "psci";
>>  			next-level-cache = <&L2_200>;
>> +			power-domains = <&cpu_pd>;
>>  			L2_200: l2-cache {
>>  				compatible = "cache";
>>  				next-level-cache = <&L3_0>;
>> @@ -135,6 +138,7 @@
>>  			reg = <0x0 0x300>;
>>  			enable-method = "psci";
>>  			next-level-cache = <&L2_300>;
>> +			power-domains = <&cpu_pd>;
>>  			L2_300: l2-cache {
>>  				compatible = "cache";
>>  				next-level-cache = <&L3_0>;
>> @@ -147,6 +151,7 @@
>>  			reg = <0x0 0x400>;
>>  			enable-method = "psci";
>>  			next-level-cache = <&L2_400>;
>> +			power-domains = <&cpu_pd>;
>>  			L2_400: l2-cache {
>>  				compatible = "cache";
>>  				next-level-cache = <&L3_0>;
>> @@ -159,6 +164,7 @@
>>  			reg = <0x0 0x500>;
>>  			enable-method = "psci";
>>  			next-level-cache = <&L2_500>;
>> +			power-domains = <&cpu_pd>;
>>  			L2_500: l2-cache {
>>  				compatible = "cache";
>>  				next-level-cache = <&L3_0>;
>> @@ -171,6 +177,7 @@
>>  			reg = <0x0 0x600>;
>>  			enable-method = "psci";
>>  			next-level-cache = <&L2_600>;
>> +			power-domains = <&cpu_pd>;
>>  			L2_600: l2-cache {
>>  				compatible = "cache";
>>  				next-level-cache = <&L3_0>;
>> @@ -183,6 +190,7 @@
>>  			reg = <0x0 0x700>;
>>  			enable-method = "psci";
>>  			next-level-cache = <&L2_700>;
>> +			power-domains = <&cpu_pd>;
>>  			L2_700: l2-cache {
>>  				compatible = "cache";
>>  				next-level-cache = <&L3_0>;
>> @@ -1170,6 +1178,11 @@
>>  					  <WAKE_TCS    3>,
>>  					  <CONTROL_TCS 1>;
>>
>> +			cpu_pd: power-domain-controller {
>> +				compatible = "qcom,cpu-pm-domain";
>> +				#power-domain-cells = <0>;
>> +			};
>> +
>
>After all the discussions, I see this power domain actually influence
>not just CPUs but other devices. So this should be top most power domain
>in the system with lots of devices or their power domains pointing to it.
>Why is this just pointing to cpus ?
>
The domain powers off even when devices remains powered on. The devices
themselves are not part of this domain, they have their own rails,
clocks and domains. Those domains will get powered off when the devices
are suspended or the device are not in use.

This CPU domain is responsible for setting the domain controller to
enter a low power state and in addition help lower the state of the
shared resources that are used by the devices even when the devices
themselves are powered on.

-- Lina