linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
@ 2020-08-11 19:02 Sibi Sankar
  2020-08-11 19:02 ` [PATCH 2/2] soc: qcom: aoss: Use " Sibi Sankar
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Sibi Sankar @ 2020-08-11 19:02 UTC (permalink / raw)
  To: bjorn.andersson, ulf.hansson, rjw
  Cc: agross, linux-kernel, linux-arm-msm, linux-pm, gregkh, pavel,
	len.brown, rnayak, dianders, khilman, Sibi Sankar

This is for power domains which needs to stay powered on for suspend
but can be powered on/off as part of runtime PM. This flag is aimed at
power domains coupled to remote processors which enter suspend states
independent to that of the application processor. Such power domains
are turned off only on remote processor crash/shutdown.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/base/power/domain.c | 3 ++-
 include/linux/pm_domain.h   | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2cb5e04cf86cd..ba78ac4a450d4 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -129,6 +129,7 @@ static const struct genpd_lock_ops genpd_spin_ops = {
 #define genpd_is_active_wakeup(genpd)	(genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
 #define genpd_is_cpu_domain(genpd)	(genpd->flags & GENPD_FLAG_CPU_DOMAIN)
 #define genpd_is_rpm_always_on(genpd)	(genpd->flags & GENPD_FLAG_RPM_ALWAYS_ON)
+#define genpd_is_suspend_on(genpd)	(genpd->flags & GENPD_FLAG_SUSPEND_ON)
 
 static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
 		const struct generic_pm_domain *genpd)
@@ -949,7 +950,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
 {
 	struct gpd_link *link;
 
-	if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
+	if (!genpd_status_on(genpd) || genpd_is_always_on(genpd) || genpd_is_suspend_on(genpd))
 		return;
 
 	if (genpd->suspended_count != genpd->device_count
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index ee11502a575b0..3002a2d68936a 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -55,6 +55,10 @@
  *
  * GENPD_FLAG_RPM_ALWAYS_ON:	Instructs genpd to always keep the PM domain
  *				powered on except for system suspend.
+ *
+ * GENPD_FLAG_SUSPEND_ON:	Instructs genpd to keep the PM domain powered
+ *				on during suspend and runtime PM controlled
+ *				otherwise.
  */
 #define GENPD_FLAG_PM_CLK	 (1U << 0)
 #define GENPD_FLAG_IRQ_SAFE	 (1U << 1)
@@ -62,6 +66,7 @@
 #define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
 #define GENPD_FLAG_CPU_DOMAIN	 (1U << 4)
 #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
+#define GENPD_FLAG_SUSPEND_ON	 (1U << 6)
 
 enum gpd_status {
 	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/2] soc: qcom: aoss: Use GENPD_FLAG_SUSPEND_ON flag
  2020-08-11 19:02 [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag Sibi Sankar
@ 2020-08-11 19:02 ` Sibi Sankar
  2020-08-11 21:16   ` Doug Anderson
  2020-08-11 21:17 ` [PATCH 1/2] PM / Domains: Add " Doug Anderson
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Sibi Sankar @ 2020-08-11 19:02 UTC (permalink / raw)
  To: bjorn.andersson, ulf.hansson, rjw
  Cc: agross, linux-kernel, linux-arm-msm, linux-pm, gregkh, pavel,
	len.brown, rnayak, dianders, khilman, Sibi Sankar

All the power domains exposed as part of AOSS QMP driver require to stay
powered on for suspend. They are powered on when the remote processors
boots up and powered off on remote processor crash/shutdown. Mark the
power domains with GENPD_FLAG_SUSPEND_ON to model this behavior.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/soc/qcom/qcom_aoss.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
index ed2c687c16b31..5a5b4bf928147 100644
--- a/drivers/soc/qcom/qcom_aoss.c
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -366,6 +366,7 @@ static int qmp_pd_add(struct qmp *qmp)
 		res[i].pd.name = sdm845_resources[i];
 		res[i].pd.power_on = qmp_pd_power_on;
 		res[i].pd.power_off = qmp_pd_power_off;
+		res[i].pd.flags = GENPD_FLAG_SUSPEND_ON;
 
 		ret = pm_genpd_init(&res[i].pd, NULL, true);
 		if (ret < 0) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] soc: qcom: aoss: Use GENPD_FLAG_SUSPEND_ON flag
  2020-08-11 19:02 ` [PATCH 2/2] soc: qcom: aoss: Use " Sibi Sankar
@ 2020-08-11 21:16   ` Doug Anderson
  0 siblings, 0 replies; 16+ messages in thread
From: Doug Anderson @ 2020-08-11 21:16 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: Bjorn Andersson, Ulf Hansson, Rafael J. Wysocki, Andy Gross,
	LKML, linux-arm-msm, Linux PM, Greg Kroah-Hartman, Pavel Machek,
	Brown, Len, Rajendra Nayak, Kevin Hilman

Hi,

On Tue, Aug 11, 2020 at 12:03 PM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> All the power domains exposed as part of AOSS QMP driver require to stay
> powered on for suspend. They are powered on when the remote processors
> boots up and powered off on remote processor crash/shutdown. Mark the
> power domains with GENPD_FLAG_SUSPEND_ON to model this behavior.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/soc/qcom/qcom_aoss.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/soc/qcom/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> index ed2c687c16b31..5a5b4bf928147 100644
> --- a/drivers/soc/qcom/qcom_aoss.c
> +++ b/drivers/soc/qcom/qcom_aoss.c
> @@ -366,6 +366,7 @@ static int qmp_pd_add(struct qmp *qmp)
>                 res[i].pd.name = sdm845_resources[i];
>                 res[i].pd.power_on = qmp_pd_power_on;
>                 res[i].pd.power_off = qmp_pd_power_off;
> +               res[i].pd.flags = GENPD_FLAG_SUSPEND_ON;

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

This makes my patch [1] unnecessary.

[1] https://lore.kernel.org/r/20200805091141.1.I86b3faaecb0d82997b599b1300f879606c71e116@changeid

-Doug

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-11 19:02 [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag Sibi Sankar
  2020-08-11 19:02 ` [PATCH 2/2] soc: qcom: aoss: Use " Sibi Sankar
@ 2020-08-11 21:17 ` Doug Anderson
  2020-08-11 21:38 ` Stephen Boyd
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Doug Anderson @ 2020-08-11 21:17 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: Bjorn Andersson, Ulf Hansson, Rafael J. Wysocki, Andy Gross,
	LKML, linux-arm-msm, Linux PM, Greg Kroah-Hartman, Pavel Machek,
	Brown, Len, Rajendra Nayak, Kevin Hilman

Hi,

On Tue, Aug 11, 2020 at 12:03 PM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> This is for power domains which needs to stay powered on for suspend
> but can be powered on/off as part of runtime PM. This flag is aimed at
> power domains coupled to remote processors which enter suspend states
> independent to that of the application processor. Such power domains
> are turned off only on remote processor crash/shutdown.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/base/power/domain.c | 3 ++-
>  include/linux/pm_domain.h   | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)

Seems sane to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-11 19:02 [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag Sibi Sankar
  2020-08-11 19:02 ` [PATCH 2/2] soc: qcom: aoss: Use " Sibi Sankar
  2020-08-11 21:17 ` [PATCH 1/2] PM / Domains: Add " Doug Anderson
@ 2020-08-11 21:38 ` Stephen Boyd
  2020-08-12 13:36   ` Sibi Sankar
  2020-08-12  0:19 ` Kevin Hilman
  2020-08-12  9:45 ` Ulf Hansson
  4 siblings, 1 reply; 16+ messages in thread
From: Stephen Boyd @ 2020-08-11 21:38 UTC (permalink / raw)
  To: Sibi Sankar, bjorn.andersson, rjw, ulf.hansson
  Cc: agross, linux-kernel, linux-arm-msm, linux-pm, gregkh, pavel,
	len.brown, rnayak, dianders, khilman, Sibi Sankar

Quoting Sibi Sankar (2020-08-11 12:02:51)
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index ee11502a575b0..3002a2d68936a 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -55,6 +55,10 @@
>   *
>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM domain
>   *                             powered on except for system suspend.
> + *
> + * GENPD_FLAG_SUSPEND_ON:      Instructs genpd to keep the PM domain powered
> + *                             on during suspend and runtime PM controlled

Maybe, "powered on across system suspend (if it is already powered on)"
to match the hunk above that talks about system suspend for
GENPD_FLAG_RPM_ALWAYS_ON. Otherwise someone may think that this powers
on the genpd during suspend or powers it on during runtime suspend.

> + *                             otherwise.
>   */
>  #define GENPD_FLAG_PM_CLK       (1U << 0)
>  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-11 19:02 [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag Sibi Sankar
                   ` (2 preceding siblings ...)
  2020-08-11 21:38 ` Stephen Boyd
@ 2020-08-12  0:19 ` Kevin Hilman
  2020-08-12 16:12   ` Sibi Sankar
  2020-08-12  9:45 ` Ulf Hansson
  4 siblings, 1 reply; 16+ messages in thread
From: Kevin Hilman @ 2020-08-12  0:19 UTC (permalink / raw)
  To: Sibi Sankar, bjorn.andersson, ulf.hansson, rjw
  Cc: agross, linux-kernel, linux-arm-msm, linux-pm, gregkh, pavel,
	len.brown, rnayak, dianders, khilman, Sibi Sankar

Sibi Sankar <sibis@codeaurora.org> writes:

> This is for power domains which needs to stay powered on for suspend
> but can be powered on/off as part of runtime PM. This flag is aimed at
> power domains coupled to remote processors which enter suspend states
> independent to that of the application processor. Such power domains
> are turned off only on remote processor crash/shutdown.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>

Seems like a useful use-case, but i think there should be a bit more
description/documentation about what is the expected/desired behavior
during system suspsend when a power-domain with this flag is already
runtime-PM suspended.  Similarily, on system resume, what is the
expected/desired behavior?

Kevin

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-11 19:02 [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag Sibi Sankar
                   ` (3 preceding siblings ...)
  2020-08-12  0:19 ` Kevin Hilman
@ 2020-08-12  9:45 ` Ulf Hansson
  2020-08-12 17:02   ` Sibi Sankar
  4 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2020-08-12  9:45 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: Bjorn Andersson, Rafael J. Wysocki, Andy Gross,
	Linux Kernel Mailing List, linux-arm-msm, Linux PM,
	Greg Kroah-Hartman, Pavel Machek, Len Brown, Rajendra Nayak,
	Doug Anderson, Kevin Hilman

On Tue, 11 Aug 2020 at 21:03, Sibi Sankar <sibis@codeaurora.org> wrote:
>
> This is for power domains which needs to stay powered on for suspend
> but can be powered on/off as part of runtime PM. This flag is aimed at
> power domains coupled to remote processors which enter suspend states
> independent to that of the application processor. Such power domains
> are turned off only on remote processor crash/shutdown.

As Kevin also requested, please elaborate more on the use case.

Why exactly must the PM domain stay powered on during system suspend?
Is there a wakeup configured that needs to be managed - or is there a
co-processor/FW behaviour that needs to be obeyed to?

Kind regards
Uffe

>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/base/power/domain.c | 3 ++-
>  include/linux/pm_domain.h   | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2cb5e04cf86cd..ba78ac4a450d4 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -129,6 +129,7 @@ static const struct genpd_lock_ops genpd_spin_ops = {
>  #define genpd_is_active_wakeup(genpd)  (genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP)
>  #define genpd_is_cpu_domain(genpd)     (genpd->flags & GENPD_FLAG_CPU_DOMAIN)
>  #define genpd_is_rpm_always_on(genpd)  (genpd->flags & GENPD_FLAG_RPM_ALWAYS_ON)
> +#define genpd_is_suspend_on(genpd)     (genpd->flags & GENPD_FLAG_SUSPEND_ON)
>
>  static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
>                 const struct generic_pm_domain *genpd)
> @@ -949,7 +950,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
>  {
>         struct gpd_link *link;
>
> -       if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
> +       if (!genpd_status_on(genpd) || genpd_is_always_on(genpd) || genpd_is_suspend_on(genpd))
>                 return;
>
>         if (genpd->suspended_count != genpd->device_count
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index ee11502a575b0..3002a2d68936a 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -55,6 +55,10 @@
>   *
>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM domain
>   *                             powered on except for system suspend.
> + *
> + * GENPD_FLAG_SUSPEND_ON:      Instructs genpd to keep the PM domain powered
> + *                             on during suspend and runtime PM controlled
> + *                             otherwise.
>   */
>  #define GENPD_FLAG_PM_CLK       (1U << 0)
>  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
> @@ -62,6 +66,7 @@
>  #define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
>  #define GENPD_FLAG_CPU_DOMAIN   (1U << 4)
>  #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
> +#define GENPD_FLAG_SUSPEND_ON   (1U << 6)
>
>  enum gpd_status {
>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-11 21:38 ` Stephen Boyd
@ 2020-08-12 13:36   ` Sibi Sankar
  0 siblings, 0 replies; 16+ messages in thread
From: Sibi Sankar @ 2020-08-12 13:36 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: bjorn.andersson, rjw, ulf.hansson, agross, linux-kernel,
	linux-arm-msm, linux-pm, gregkh, pavel, len.brown, rnayak,
	dianders, khilman

Hey Stephen,
Thanks for taking time to review the
series!

On 2020-08-12 03:08, Stephen Boyd wrote:
> Quoting Sibi Sankar (2020-08-11 12:02:51)
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index ee11502a575b0..3002a2d68936a 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -55,6 +55,10 @@
>>   *
>>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM 
>> domain
>>   *                             powered on except for system suspend.
>> + *
>> + * GENPD_FLAG_SUSPEND_ON:      Instructs genpd to keep the PM domain 
>> powered
>> + *                             on during suspend and runtime PM 
>> controlled
> 
> Maybe, "powered on across system suspend (if it is already powered on)"
> to match the hunk above that talks about system suspend for
> GENPD_FLAG_RPM_ALWAYS_ON. Otherwise someone may think that this powers
> on the genpd during suspend or powers it on during runtime suspend.

Sure, I'll add ^^ in the next re-spin.

> 
>> + *                             otherwise.
>>   */
>>  #define GENPD_FLAG_PM_CLK       (1U << 0)
>>  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-12  0:19 ` Kevin Hilman
@ 2020-08-12 16:12   ` Sibi Sankar
  0 siblings, 0 replies; 16+ messages in thread
From: Sibi Sankar @ 2020-08-12 16:12 UTC (permalink / raw)
  To: Kevin Hilman
  Cc: bjorn.andersson, ulf.hansson, rjw, agross, linux-kernel,
	linux-arm-msm, linux-pm, gregkh, pavel, len.brown, rnayak,
	dianders, khilman, linux-arm-msm-owner

Kevin,
Thanks for taking time to review the
series!

On 2020-08-12 05:49, Kevin Hilman wrote:
> Sibi Sankar <sibis@codeaurora.org> writes:
> 
>> This is for power domains which needs to stay powered on for suspend
>> but can be powered on/off as part of runtime PM. This flag is aimed at
>> power domains coupled to remote processors which enter suspend states
>> independent to that of the application processor. Such power domains
>> are turned off only on remote processor crash/shutdown.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> 
> Seems like a useful use-case, but i think there should be a bit more
> description/documentation about what is the expected/desired behavior
> during system suspsend when a power-domain with this flag is already
> runtime-PM suspended.  Similarily, on system resume, what is the
> expected/desired behavior?

SUSPEND_ON flag is only aimed at
keeping power domains powered on
across suspend (only if its already
powered on). Also if the power domain
is runtime-PM suspended we wouldn't
want to power it on during resume.

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0a5afca250d03..547c091618008 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1003,7 +1003,7 @@ static void genpd_sync_power_on(struct 
generic_pm_domain *genpd, bool use_lock,
  {
         struct gpd_link *link;

-       if (genpd_status_on(genpd))
+       if (genpd_status_on(genpd) || genpd_is_suspend_on(genpd))
                 return;

I'll add the ^^ diff in the next
re-spin to prevent power on of
a runtime-PM suspended power
domain.

> 
> Kevin

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-12  9:45 ` Ulf Hansson
@ 2020-08-12 17:02   ` Sibi Sankar
  2020-08-13 12:34     ` Ulf Hansson
  0 siblings, 1 reply; 16+ messages in thread
From: Sibi Sankar @ 2020-08-12 17:02 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Bjorn Andersson, Rafael J. Wysocki, Andy Gross,
	Linux Kernel Mailing List, linux-arm-msm, Linux PM,
	Greg Kroah-Hartman, Pavel Machek, Len Brown, Rajendra Nayak,
	Doug Anderson, linux-kernel-owner, Kevin Hilman

Uffe,
Thanks for taking time to review the
series!

On 2020-08-12 15:15, Ulf Hansson wrote:
> On Tue, 11 Aug 2020 at 21:03, Sibi Sankar <sibis@codeaurora.org> wrote:
>> 
>> This is for power domains which needs to stay powered on for suspend
>> but can be powered on/off as part of runtime PM. This flag is aimed at
>> power domains coupled to remote processors which enter suspend states
>> independent to that of the application processor. Such power domains
>> are turned off only on remote processor crash/shutdown.
> 
> As Kevin also requested, please elaborate more on the use case.
> 
> Why exactly must the PM domain stay powered on during system suspend?
> Is there a wakeup configured that needs to be managed - or is there a
> co-processor/FW behaviour that needs to be obeyed to?

Yes this is a co-processor behavior that
needs to be obeyed. Specifically application
processor notifies the Always on Subsystem
(AOSS) that a particular co-processor is up
using the power domains exposed by AOSS QMP
driver. AOSS uses this information to wait
for the co-processors to suspend before
starting its sleep sequence. The application
processor powers off these power domains only
if the co-processor has crashed or powered
off.

> 
> Kind regards
> Uffe
> 
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/base/power/domain.c | 3 ++-
>>  include/linux/pm_domain.h   | 5 +++++
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 2cb5e04cf86cd..ba78ac4a450d4 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -129,6 +129,7 @@ static const struct genpd_lock_ops genpd_spin_ops 
>> = {
>>  #define genpd_is_active_wakeup(genpd)  (genpd->flags & 
>> GENPD_FLAG_ACTIVE_WAKEUP)
>>  #define genpd_is_cpu_domain(genpd)     (genpd->flags & 
>> GENPD_FLAG_CPU_DOMAIN)
>>  #define genpd_is_rpm_always_on(genpd)  (genpd->flags & 
>> GENPD_FLAG_RPM_ALWAYS_ON)
>> +#define genpd_is_suspend_on(genpd)     (genpd->flags & 
>> GENPD_FLAG_SUSPEND_ON)
>> 
>>  static inline bool irq_safe_dev_in_no_sleep_domain(struct device 
>> *dev,
>>                 const struct generic_pm_domain *genpd)
>> @@ -949,7 +950,7 @@ static void genpd_sync_power_off(struct 
>> generic_pm_domain *genpd, bool use_lock,
>>  {
>>         struct gpd_link *link;
>> 
>> -       if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
>> +       if (!genpd_status_on(genpd) || genpd_is_always_on(genpd) || 
>> genpd_is_suspend_on(genpd))
>>                 return;
>> 
>>         if (genpd->suspended_count != genpd->device_count
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index ee11502a575b0..3002a2d68936a 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -55,6 +55,10 @@
>>   *
>>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM 
>> domain
>>   *                             powered on except for system suspend.
>> + *
>> + * GENPD_FLAG_SUSPEND_ON:      Instructs genpd to keep the PM domain 
>> powered
>> + *                             on during suspend and runtime PM 
>> controlled
>> + *                             otherwise.
>>   */
>>  #define GENPD_FLAG_PM_CLK       (1U << 0)
>>  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
>> @@ -62,6 +66,7 @@
>>  #define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
>>  #define GENPD_FLAG_CPU_DOMAIN   (1U << 4)
>>  #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
>> +#define GENPD_FLAG_SUSPEND_ON   (1U << 6)
>> 
>>  enum gpd_status {
>>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-12 17:02   ` Sibi Sankar
@ 2020-08-13 12:34     ` Ulf Hansson
  2020-08-13 17:26       ` Sibi Sankar
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2020-08-13 12:34 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: Bjorn Andersson, Rafael J. Wysocki, Andy Gross,
	Linux Kernel Mailing List, linux-arm-msm, Linux PM,
	Greg Kroah-Hartman, Pavel Machek, Len Brown, Rajendra Nayak,
	Doug Anderson, linux-kernel-owner, Kevin Hilman

On Wed, 12 Aug 2020 at 19:03, Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Uffe,
> Thanks for taking time to review the
> series!
>
> On 2020-08-12 15:15, Ulf Hansson wrote:
> > On Tue, 11 Aug 2020 at 21:03, Sibi Sankar <sibis@codeaurora.org> wrote:
> >>
> >> This is for power domains which needs to stay powered on for suspend
> >> but can be powered on/off as part of runtime PM. This flag is aimed at
> >> power domains coupled to remote processors which enter suspend states
> >> independent to that of the application processor. Such power domains
> >> are turned off only on remote processor crash/shutdown.
> >
> > As Kevin also requested, please elaborate more on the use case.
> >
> > Why exactly must the PM domain stay powered on during system suspend?
> > Is there a wakeup configured that needs to be managed - or is there a
> > co-processor/FW behaviour that needs to be obeyed to?
>
> Yes this is a co-processor behavior that
> needs to be obeyed. Specifically application
> processor notifies the Always on Subsystem
> (AOSS) that a particular co-processor is up
> using the power domains exposed by AOSS QMP
> driver. AOSS uses this information to wait
> for the co-processors to suspend before
> starting its sleep sequence. The application
> processor powers off these power domains only
> if the co-processor has crashed or powered
> off.

Thanks for clarifying!

Although, can you please elaborate a bit more on the actual use case?
What are the typical co-processor and what drivers are involved in
managing it?

As you may know, runtime PM becomes disabled during system suspend of
a device. Which means, if the driver tries to power off the
coprocessor (via calling pm_runtime_put() for example), somewhere in
the system suspend phase of the corresponding device, its attached PM
domain stays powered on when managed by genpd.

Then in the suspend_noirq phase, genpd tries to power off the PM
domain, unless there are wakeups to consider.

Taking the above into account, wouldn't that mean that you potentially
may end up keeping the PM domain powered on, even if it actually can
be powered off in the suspend_noirq phase by genpd?

Kind regards
Uffe

> >
> >>
> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> >> ---
> >>  drivers/base/power/domain.c | 3 ++-
> >>  include/linux/pm_domain.h   | 5 +++++
> >>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> >> index 2cb5e04cf86cd..ba78ac4a450d4 100644
> >> --- a/drivers/base/power/domain.c
> >> +++ b/drivers/base/power/domain.c
> >> @@ -129,6 +129,7 @@ static const struct genpd_lock_ops genpd_spin_ops
> >> = {
> >>  #define genpd_is_active_wakeup(genpd)  (genpd->flags &
> >> GENPD_FLAG_ACTIVE_WAKEUP)
> >>  #define genpd_is_cpu_domain(genpd)     (genpd->flags &
> >> GENPD_FLAG_CPU_DOMAIN)
> >>  #define genpd_is_rpm_always_on(genpd)  (genpd->flags &
> >> GENPD_FLAG_RPM_ALWAYS_ON)
> >> +#define genpd_is_suspend_on(genpd)     (genpd->flags &
> >> GENPD_FLAG_SUSPEND_ON)
> >>
> >>  static inline bool irq_safe_dev_in_no_sleep_domain(struct device
> >> *dev,
> >>                 const struct generic_pm_domain *genpd)
> >> @@ -949,7 +950,7 @@ static void genpd_sync_power_off(struct
> >> generic_pm_domain *genpd, bool use_lock,
> >>  {
> >>         struct gpd_link *link;
> >>
> >> -       if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
> >> +       if (!genpd_status_on(genpd) || genpd_is_always_on(genpd) ||
> >> genpd_is_suspend_on(genpd))
> >>                 return;
> >>
> >>         if (genpd->suspended_count != genpd->device_count
> >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> >> index ee11502a575b0..3002a2d68936a 100644
> >> --- a/include/linux/pm_domain.h
> >> +++ b/include/linux/pm_domain.h
> >> @@ -55,6 +55,10 @@
> >>   *
> >>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM
> >> domain
> >>   *                             powered on except for system suspend.
> >> + *
> >> + * GENPD_FLAG_SUSPEND_ON:      Instructs genpd to keep the PM domain
> >> powered
> >> + *                             on during suspend and runtime PM
> >> controlled
> >> + *                             otherwise.
> >>   */
> >>  #define GENPD_FLAG_PM_CLK       (1U << 0)
> >>  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
> >> @@ -62,6 +66,7 @@
> >>  #define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
> >>  #define GENPD_FLAG_CPU_DOMAIN   (1U << 4)
> >>  #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
> >> +#define GENPD_FLAG_SUSPEND_ON   (1U << 6)
> >>
> >>  enum gpd_status {
> >>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
> >> --
> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> >> Forum,
> >> a Linux Foundation Collaborative Project
> >>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-13 12:34     ` Ulf Hansson
@ 2020-08-13 17:26       ` Sibi Sankar
  2020-08-17  8:44         ` Ulf Hansson
  0 siblings, 1 reply; 16+ messages in thread
From: Sibi Sankar @ 2020-08-13 17:26 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Bjorn Andersson, Rafael J. Wysocki, Andy Gross,
	Linux Kernel Mailing List, linux-arm-msm, Linux PM,
	Greg Kroah-Hartman, Pavel Machek, Len Brown, Rajendra Nayak,
	Doug Anderson, linux-kernel-owner, Kevin Hilman

On 2020-08-13 18:04, Ulf Hansson wrote:
> On Wed, 12 Aug 2020 at 19:03, Sibi Sankar <sibis@codeaurora.org> wrote:
>> 
>> Uffe,
>> Thanks for taking time to review the
>> series!
>> 
>> On 2020-08-12 15:15, Ulf Hansson wrote:
>> > On Tue, 11 Aug 2020 at 21:03, Sibi Sankar <sibis@codeaurora.org> wrote:
>> >>
>> >> This is for power domains which needs to stay powered on for suspend
>> >> but can be powered on/off as part of runtime PM. This flag is aimed at
>> >> power domains coupled to remote processors which enter suspend states
>> >> independent to that of the application processor. Such power domains
>> >> are turned off only on remote processor crash/shutdown.
>> >
>> > As Kevin also requested, please elaborate more on the use case.
>> >
>> > Why exactly must the PM domain stay powered on during system suspend?
>> > Is there a wakeup configured that needs to be managed - or is there a
>> > co-processor/FW behaviour that needs to be obeyed to?
>> 
>> Yes this is a co-processor behavior that
>> needs to be obeyed. Specifically application
>> processor notifies the Always on Subsystem
>> (AOSS) that a particular co-processor is up
>> using the power domains exposed by AOSS QMP
>> driver. AOSS uses this information to wait
>> for the co-processors to suspend before
>> starting its sleep sequence. The application
>> processor powers off these power domains only
>> if the co-processor has crashed or powered
>> off.
> 
> Thanks for clarifying!
> 
> Although, can you please elaborate a bit more on the actual use case?
> What are the typical co-processor and what drivers are involved in
> managing it?

The co-processors using the power domains
exposed by qcom_aoss driver are modem,
audio dsp, compute dsp managed using
qcom_q6v5_mss and qcom_q6v5_pas driver.

> 
> As you may know, runtime PM becomes disabled during system suspend of
> a device. Which means, if the driver tries to power off the
> coprocessor (via calling pm_runtime_put() for example), somewhere in
> the system suspend phase of the corresponding device, its attached PM
> domain stays powered on when managed by genpd.

The drivers aren't really expected
do anything during suspend/resume
pretty much because the co-processors
enter low-power modes independent to
that of the application processor. On
co-processor crash the remoteproc core
does a pm_stay_awake followed by a
pm_relax after crash recovery.

> 
> Then in the suspend_noirq phase, genpd tries to power off the PM
> domain, unless there are wakeups to consider.
> 
> Taking the above into account, wouldn't that mean that you potentially
> may end up keeping the PM domain powered on, even if it actually can
> be powered off in the suspend_noirq phase by genpd?
> 
> Kind regards
> Uffe
> 
>> >
>> >>
>> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> >> ---
>> >>  drivers/base/power/domain.c | 3 ++-
>> >>  include/linux/pm_domain.h   | 5 +++++
>> >>  2 files changed, 7 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> >> index 2cb5e04cf86cd..ba78ac4a450d4 100644
>> >> --- a/drivers/base/power/domain.c
>> >> +++ b/drivers/base/power/domain.c
>> >> @@ -129,6 +129,7 @@ static const struct genpd_lock_ops genpd_spin_ops
>> >> = {
>> >>  #define genpd_is_active_wakeup(genpd)  (genpd->flags &
>> >> GENPD_FLAG_ACTIVE_WAKEUP)
>> >>  #define genpd_is_cpu_domain(genpd)     (genpd->flags &
>> >> GENPD_FLAG_CPU_DOMAIN)
>> >>  #define genpd_is_rpm_always_on(genpd)  (genpd->flags &
>> >> GENPD_FLAG_RPM_ALWAYS_ON)
>> >> +#define genpd_is_suspend_on(genpd)     (genpd->flags &
>> >> GENPD_FLAG_SUSPEND_ON)
>> >>
>> >>  static inline bool irq_safe_dev_in_no_sleep_domain(struct device
>> >> *dev,
>> >>                 const struct generic_pm_domain *genpd)
>> >> @@ -949,7 +950,7 @@ static void genpd_sync_power_off(struct
>> >> generic_pm_domain *genpd, bool use_lock,
>> >>  {
>> >>         struct gpd_link *link;
>> >>
>> >> -       if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
>> >> +       if (!genpd_status_on(genpd) || genpd_is_always_on(genpd) ||
>> >> genpd_is_suspend_on(genpd))
>> >>                 return;
>> >>
>> >>         if (genpd->suspended_count != genpd->device_count
>> >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> >> index ee11502a575b0..3002a2d68936a 100644
>> >> --- a/include/linux/pm_domain.h
>> >> +++ b/include/linux/pm_domain.h
>> >> @@ -55,6 +55,10 @@
>> >>   *
>> >>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM
>> >> domain
>> >>   *                             powered on except for system suspend.
>> >> + *
>> >> + * GENPD_FLAG_SUSPEND_ON:      Instructs genpd to keep the PM domain
>> >> powered
>> >> + *                             on during suspend and runtime PM
>> >> controlled
>> >> + *                             otherwise.
>> >>   */
>> >>  #define GENPD_FLAG_PM_CLK       (1U << 0)
>> >>  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
>> >> @@ -62,6 +66,7 @@
>> >>  #define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
>> >>  #define GENPD_FLAG_CPU_DOMAIN   (1U << 4)
>> >>  #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
>> >> +#define GENPD_FLAG_SUSPEND_ON   (1U << 6)
>> >>
>> >>  enum gpd_status {
>> >>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
>> >> --
>> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> >> Forum,
>> >> a Linux Foundation Collaborative Project
>> >>
>> 
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-13 17:26       ` Sibi Sankar
@ 2020-08-17  8:44         ` Ulf Hansson
  2020-08-17 16:49           ` Sibi Sankar
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2020-08-17  8:44 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: Bjorn Andersson, Rafael J. Wysocki, Andy Gross,
	Linux Kernel Mailing List, linux-arm-msm, Linux PM,
	Greg Kroah-Hartman, Pavel Machek, Len Brown, Rajendra Nayak,
	Doug Anderson, linux-kernel-owner, Kevin Hilman

On Thu, 13 Aug 2020 at 19:26, Sibi Sankar <sibis@codeaurora.org> wrote:
>
> On 2020-08-13 18:04, Ulf Hansson wrote:
> > On Wed, 12 Aug 2020 at 19:03, Sibi Sankar <sibis@codeaurora.org> wrote:
> >>
> >> Uffe,
> >> Thanks for taking time to review the
> >> series!
> >>
> >> On 2020-08-12 15:15, Ulf Hansson wrote:
> >> > On Tue, 11 Aug 2020 at 21:03, Sibi Sankar <sibis@codeaurora.org> wrote:
> >> >>
> >> >> This is for power domains which needs to stay powered on for suspend
> >> >> but can be powered on/off as part of runtime PM. This flag is aimed at
> >> >> power domains coupled to remote processors which enter suspend states
> >> >> independent to that of the application processor. Such power domains
> >> >> are turned off only on remote processor crash/shutdown.
> >> >
> >> > As Kevin also requested, please elaborate more on the use case.
> >> >
> >> > Why exactly must the PM domain stay powered on during system suspend?
> >> > Is there a wakeup configured that needs to be managed - or is there a
> >> > co-processor/FW behaviour that needs to be obeyed to?
> >>
> >> Yes this is a co-processor behavior that
> >> needs to be obeyed. Specifically application
> >> processor notifies the Always on Subsystem
> >> (AOSS) that a particular co-processor is up
> >> using the power domains exposed by AOSS QMP
> >> driver. AOSS uses this information to wait
> >> for the co-processors to suspend before
> >> starting its sleep sequence. The application
> >> processor powers off these power domains only
> >> if the co-processor has crashed or powered
> >> off.
> >
> > Thanks for clarifying!
> >
> > Although, can you please elaborate a bit more on the actual use case?
> > What are the typical co-processor and what drivers are involved in
> > managing it?
>
> The co-processors using the power domains
> exposed by qcom_aoss driver are modem,
> audio dsp, compute dsp managed using
> qcom_q6v5_mss and qcom_q6v5_pas driver.
>
> >
> > As you may know, runtime PM becomes disabled during system suspend of
> > a device. Which means, if the driver tries to power off the
> > coprocessor (via calling pm_runtime_put() for example), somewhere in
> > the system suspend phase of the corresponding device, its attached PM
> > domain stays powered on when managed by genpd.
>
> The drivers aren't really expected
> do anything during suspend/resume
> pretty much because the co-processors
> enter low-power modes independent to
> that of the application processor. On
> co-processor crash the remoteproc core
> does a pm_stay_awake followed by a
> pm_relax after crash recovery.

Okay, thanks again for clarifying. You have convinced me about the
need for a new flag to cope with these use cases.

Would you mind updating the commit message with some of the
information you just provided?

Additionally, to make it clear that the flag should be used to keep
the PM domain powered on during system suspend, but only if it's
already powered on - please rename the flag to GENPD_FLAG_NO_SUSPEND,
and update the corresponding description of it in the header file.

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-17  8:44         ` Ulf Hansson
@ 2020-08-17 16:49           ` Sibi Sankar
  2020-08-18  8:31             ` Ulf Hansson
  0 siblings, 1 reply; 16+ messages in thread
From: Sibi Sankar @ 2020-08-17 16:49 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Bjorn Andersson, Rafael J. Wysocki, Andy Gross,
	Linux Kernel Mailing List, linux-arm-msm, Linux PM,
	Greg Kroah-Hartman, Pavel Machek, Len Brown, Rajendra Nayak,
	Doug Anderson, linux-kernel-owner, Kevin Hilman,
	linux-arm-msm-owner

On 2020-08-17 14:14, Ulf Hansson wrote:
> On Thu, 13 Aug 2020 at 19:26, Sibi Sankar <sibis@codeaurora.org> wrote:
>> 
>> On 2020-08-13 18:04, Ulf Hansson wrote:
>> > On Wed, 12 Aug 2020 at 19:03, Sibi Sankar <sibis@codeaurora.org> wrote:
>> >>
>> >> Uffe,
>> >> Thanks for taking time to review the
>> >> series!
>> >>
>> >> On 2020-08-12 15:15, Ulf Hansson wrote:
>> >> > On Tue, 11 Aug 2020 at 21:03, Sibi Sankar <sibis@codeaurora.org> wrote:
>> >> >>
>> >> >> This is for power domains which needs to stay powered on for suspend
>> >> >> but can be powered on/off as part of runtime PM. This flag is aimed at
>> >> >> power domains coupled to remote processors which enter suspend states
>> >> >> independent to that of the application processor. Such power domains
>> >> >> are turned off only on remote processor crash/shutdown.
>> >> >
>> >> > As Kevin also requested, please elaborate more on the use case.
>> >> >
>> >> > Why exactly must the PM domain stay powered on during system suspend?
>> >> > Is there a wakeup configured that needs to be managed - or is there a
>> >> > co-processor/FW behaviour that needs to be obeyed to?
>> >>
>> >> Yes this is a co-processor behavior that
>> >> needs to be obeyed. Specifically application
>> >> processor notifies the Always on Subsystem
>> >> (AOSS) that a particular co-processor is up
>> >> using the power domains exposed by AOSS QMP
>> >> driver. AOSS uses this information to wait
>> >> for the co-processors to suspend before
>> >> starting its sleep sequence. The application
>> >> processor powers off these power domains only
>> >> if the co-processor has crashed or powered
>> >> off.
>> >
>> > Thanks for clarifying!
>> >
>> > Although, can you please elaborate a bit more on the actual use case?
>> > What are the typical co-processor and what drivers are involved in
>> > managing it?
>> 
>> The co-processors using the power domains
>> exposed by qcom_aoss driver are modem,
>> audio dsp, compute dsp managed using
>> qcom_q6v5_mss and qcom_q6v5_pas driver.
>> 
>> >
>> > As you may know, runtime PM becomes disabled during system suspend of
>> > a device. Which means, if the driver tries to power off the
>> > coprocessor (via calling pm_runtime_put() for example), somewhere in
>> > the system suspend phase of the corresponding device, its attached PM
>> > domain stays powered on when managed by genpd.
>> 
>> The drivers aren't really expected
>> do anything during suspend/resume
>> pretty much because the co-processors
>> enter low-power modes independent to
>> that of the application processor. On
>> co-processor crash the remoteproc core
>> does a pm_stay_awake followed by a
>> pm_relax after crash recovery.
> 
> Okay, thanks again for clarifying. You have convinced me about the
> need for a new flag to cope with these use cases.
> 
> Would you mind updating the commit message with some of the
> information you just provided?
> 
> Additionally, to make it clear that the flag should be used to keep
> the PM domain powered on during system suspend, but only if it's
> already powered on - please rename the flag to GENPD_FLAG_NO_SUSPEND,
> and update the corresponding description of it in the header file.

Thanks, naming it ^^ makes more sense :)

https://lore.kernel.org/lkml/340a7aafcf0301ff3158a4e211992041@codeaurora.org/

Also we wouldn't want to power on
runtime suspended power domains with
the NO_SUSPEND flag set, on resume as
explained ^^. Do you agree with that
as well?

> 
> [...]
> 
> Kind regards
> Uffe

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-17 16:49           ` Sibi Sankar
@ 2020-08-18  8:31             ` Ulf Hansson
  2020-08-18  9:03               ` Sibi Sankar
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2020-08-18  8:31 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: Bjorn Andersson, Rafael J. Wysocki, Andy Gross,
	Linux Kernel Mailing List, linux-arm-msm, Linux PM,
	Greg Kroah-Hartman, Pavel Machek, Len Brown, Rajendra Nayak,
	Doug Anderson, linux-kernel-owner, Kevin Hilman,
	linux-arm-msm-owner

On Mon, 17 Aug 2020 at 18:49, Sibi Sankar <sibis@codeaurora.org> wrote:
>
> On 2020-08-17 14:14, Ulf Hansson wrote:
> > On Thu, 13 Aug 2020 at 19:26, Sibi Sankar <sibis@codeaurora.org> wrote:
> >>
> >> On 2020-08-13 18:04, Ulf Hansson wrote:
> >> > On Wed, 12 Aug 2020 at 19:03, Sibi Sankar <sibis@codeaurora.org> wrote:
> >> >>
> >> >> Uffe,
> >> >> Thanks for taking time to review the
> >> >> series!
> >> >>
> >> >> On 2020-08-12 15:15, Ulf Hansson wrote:
> >> >> > On Tue, 11 Aug 2020 at 21:03, Sibi Sankar <sibis@codeaurora.org> wrote:
> >> >> >>
> >> >> >> This is for power domains which needs to stay powered on for suspend
> >> >> >> but can be powered on/off as part of runtime PM. This flag is aimed at
> >> >> >> power domains coupled to remote processors which enter suspend states
> >> >> >> independent to that of the application processor. Such power domains
> >> >> >> are turned off only on remote processor crash/shutdown.
> >> >> >
> >> >> > As Kevin also requested, please elaborate more on the use case.
> >> >> >
> >> >> > Why exactly must the PM domain stay powered on during system suspend?
> >> >> > Is there a wakeup configured that needs to be managed - or is there a
> >> >> > co-processor/FW behaviour that needs to be obeyed to?
> >> >>
> >> >> Yes this is a co-processor behavior that
> >> >> needs to be obeyed. Specifically application
> >> >> processor notifies the Always on Subsystem
> >> >> (AOSS) that a particular co-processor is up
> >> >> using the power domains exposed by AOSS QMP
> >> >> driver. AOSS uses this information to wait
> >> >> for the co-processors to suspend before
> >> >> starting its sleep sequence. The application
> >> >> processor powers off these power domains only
> >> >> if the co-processor has crashed or powered
> >> >> off.
> >> >
> >> > Thanks for clarifying!
> >> >
> >> > Although, can you please elaborate a bit more on the actual use case?
> >> > What are the typical co-processor and what drivers are involved in
> >> > managing it?
> >>
> >> The co-processors using the power domains
> >> exposed by qcom_aoss driver are modem,
> >> audio dsp, compute dsp managed using
> >> qcom_q6v5_mss and qcom_q6v5_pas driver.
> >>
> >> >
> >> > As you may know, runtime PM becomes disabled during system suspend of
> >> > a device. Which means, if the driver tries to power off the
> >> > coprocessor (via calling pm_runtime_put() for example), somewhere in
> >> > the system suspend phase of the corresponding device, its attached PM
> >> > domain stays powered on when managed by genpd.
> >>
> >> The drivers aren't really expected
> >> do anything during suspend/resume
> >> pretty much because the co-processors
> >> enter low-power modes independent to
> >> that of the application processor. On
> >> co-processor crash the remoteproc core
> >> does a pm_stay_awake followed by a
> >> pm_relax after crash recovery.
> >
> > Okay, thanks again for clarifying. You have convinced me about the
> > need for a new flag to cope with these use cases.
> >
> > Would you mind updating the commit message with some of the
> > information you just provided?
> >
> > Additionally, to make it clear that the flag should be used to keep
> > the PM domain powered on during system suspend, but only if it's
> > already powered on - please rename the flag to GENPD_FLAG_NO_SUSPEND,
> > and update the corresponding description of it in the header file.
>
> Thanks, naming it ^^ makes more sense :)
>
> https://lore.kernel.org/lkml/340a7aafcf0301ff3158a4e211992041@codeaurora.org/
>
> Also we wouldn't want to power on
> runtime suspended power domains with
> the NO_SUSPEND flag set, on resume as
> explained ^^. Do you agree with that
> as well?

Actually no.

Instead, I think that deserves a separate flag, as it may very well
turn out that resuming can be skipped for other cases than
"NO_SUSPEND".

Therefore, please add a GENPD_FLAG_NO_RESUME for this.

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag
  2020-08-18  8:31             ` Ulf Hansson
@ 2020-08-18  9:03               ` Sibi Sankar
  0 siblings, 0 replies; 16+ messages in thread
From: Sibi Sankar @ 2020-08-18  9:03 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Bjorn Andersson, Rafael J. Wysocki, Andy Gross,
	Linux Kernel Mailing List, linux-arm-msm, Linux PM,
	Greg Kroah-Hartman, Pavel Machek, Len Brown, Rajendra Nayak,
	Doug Anderson, linux-kernel-owner, Kevin Hilman,
	linux-arm-msm-owner

On 2020-08-18 14:01, Ulf Hansson wrote:
> On Mon, 17 Aug 2020 at 18:49, Sibi Sankar <sibis@codeaurora.org> wrote:
>> 
>> On 2020-08-17 14:14, Ulf Hansson wrote:
>> > On Thu, 13 Aug 2020 at 19:26, Sibi Sankar <sibis@codeaurora.org> wrote:
>> >>
>> >> On 2020-08-13 18:04, Ulf Hansson wrote:
>> >> > On Wed, 12 Aug 2020 at 19:03, Sibi Sankar <sibis@codeaurora.org> wrote:
>> >> >>
>> >> >> Uffe,
>> >> >> Thanks for taking time to review the
>> >> >> series!
>> >> >>
>> >> >> On 2020-08-12 15:15, Ulf Hansson wrote:
>> >> >> > On Tue, 11 Aug 2020 at 21:03, Sibi Sankar <sibis@codeaurora.org> wrote:
>> >> >> >>
>> >> >> >> This is for power domains which needs to stay powered on for suspend
>> >> >> >> but can be powered on/off as part of runtime PM. This flag is aimed at
>> >> >> >> power domains coupled to remote processors which enter suspend states
>> >> >> >> independent to that of the application processor. Such power domains
>> >> >> >> are turned off only on remote processor crash/shutdown.
>> >> >> >
>> >> >> > As Kevin also requested, please elaborate more on the use case.
>> >> >> >
>> >> >> > Why exactly must the PM domain stay powered on during system suspend?
>> >> >> > Is there a wakeup configured that needs to be managed - or is there a
>> >> >> > co-processor/FW behaviour that needs to be obeyed to?
>> >> >>
>> >> >> Yes this is a co-processor behavior that
>> >> >> needs to be obeyed. Specifically application
>> >> >> processor notifies the Always on Subsystem
>> >> >> (AOSS) that a particular co-processor is up
>> >> >> using the power domains exposed by AOSS QMP
>> >> >> driver. AOSS uses this information to wait
>> >> >> for the co-processors to suspend before
>> >> >> starting its sleep sequence. The application
>> >> >> processor powers off these power domains only
>> >> >> if the co-processor has crashed or powered
>> >> >> off.
>> >> >
>> >> > Thanks for clarifying!
>> >> >
>> >> > Although, can you please elaborate a bit more on the actual use case?
>> >> > What are the typical co-processor and what drivers are involved in
>> >> > managing it?
>> >>
>> >> The co-processors using the power domains
>> >> exposed by qcom_aoss driver are modem,
>> >> audio dsp, compute dsp managed using
>> >> qcom_q6v5_mss and qcom_q6v5_pas driver.
>> >>
>> >> >
>> >> > As you may know, runtime PM becomes disabled during system suspend of
>> >> > a device. Which means, if the driver tries to power off the
>> >> > coprocessor (via calling pm_runtime_put() for example), somewhere in
>> >> > the system suspend phase of the corresponding device, its attached PM
>> >> > domain stays powered on when managed by genpd.
>> >>
>> >> The drivers aren't really expected
>> >> do anything during suspend/resume
>> >> pretty much because the co-processors
>> >> enter low-power modes independent to
>> >> that of the application processor. On
>> >> co-processor crash the remoteproc core
>> >> does a pm_stay_awake followed by a
>> >> pm_relax after crash recovery.
>> >
>> > Okay, thanks again for clarifying. You have convinced me about the
>> > need for a new flag to cope with these use cases.
>> >
>> > Would you mind updating the commit message with some of the
>> > information you just provided?
>> >
>> > Additionally, to make it clear that the flag should be used to keep
>> > the PM domain powered on during system suspend, but only if it's
>> > already powered on - please rename the flag to GENPD_FLAG_NO_SUSPEND,
>> > and update the corresponding description of it in the header file.
>> 
>> Thanks, naming it ^^ makes more sense :)
>> 
>> https://lore.kernel.org/lkml/340a7aafcf0301ff3158a4e211992041@codeaurora.org/
>> 
>> Also we wouldn't want to power on
>> runtime suspended power domains with
>> the NO_SUSPEND flag set, on resume as
>> explained ^^. Do you agree with that
>> as well?
> 
> Actually no.
> 
> Instead, I think that deserves a separate flag, as it may very well
> turn out that resuming can be skipped for other cases than
> "NO_SUSPEND".
> 
> Therefore, please add a GENPD_FLAG_NO_RESUME for this.

Thanks I'll do that in v2

> 
> Kind regards
> Uffe

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-08-18  9:05 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-11 19:02 [PATCH 1/2] PM / Domains: Add GENPD_FLAG_SUSPEND_ON flag Sibi Sankar
2020-08-11 19:02 ` [PATCH 2/2] soc: qcom: aoss: Use " Sibi Sankar
2020-08-11 21:16   ` Doug Anderson
2020-08-11 21:17 ` [PATCH 1/2] PM / Domains: Add " Doug Anderson
2020-08-11 21:38 ` Stephen Boyd
2020-08-12 13:36   ` Sibi Sankar
2020-08-12  0:19 ` Kevin Hilman
2020-08-12 16:12   ` Sibi Sankar
2020-08-12  9:45 ` Ulf Hansson
2020-08-12 17:02   ` Sibi Sankar
2020-08-13 12:34     ` Ulf Hansson
2020-08-13 17:26       ` Sibi Sankar
2020-08-17  8:44         ` Ulf Hansson
2020-08-17 16:49           ` Sibi Sankar
2020-08-18  8:31             ` Ulf Hansson
2020-08-18  9:03               ` Sibi Sankar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).