[PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Sibi Sankar]

Add GENPD_FLAG_NO_SUSPEND/RESUME flags to instruct genpd to keep the
status of the PM domain unaltered during suspend/resume respectively.
The flags are aimed at power domains coupled to co-processors which
enter low-power modes independent to that of the application processor.

Specifically the flags are to be used by the power domains exposed
by the AOSS QMP driver linked to modem, adsp, cdsp remoteprocs. These
power domains are used to notify the Always on Subsystem (AOSS) that
a particular co-processor is up. 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 and remains unaltered during
system suspend/resume.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---

V2:
 * Add more info in commit msg and description [Uffe/Kevin/Stephen]
 * Rename and split functionality into two flags [Uffe]
 * Drop R-b/T-b

 drivers/base/power/domain.c |  6 ++++--
 include/linux/pm_domain.h   | 10 ++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2cb5e04cf86cd..a5df5916f30f8 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -129,6 +129,8 @@ 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_no_suspend(genpd)	(genpd->flags & GENPD_FLAG_NO_SUSPEND)
+#define genpd_is_no_resume(genpd)	(genpd->flags & GENPD_FLAG_NO_RESUME)
 
 static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
 		const struct generic_pm_domain *genpd)
@@ -949,7 +951,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_no_suspend(genpd))
 		return;
 
 	if (genpd->suspended_count != genpd->device_count
@@ -991,7 +993,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_no_resume(genpd))
 		return;
 
 	list_for_each_entry(link, &genpd->child_links, child_node) {
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index ee11502a575b0..568abdf2e89cf 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -55,6 +55,14 @@
  *
  * GENPD_FLAG_RPM_ALWAYS_ON:	Instructs genpd to always keep the PM domain
  *				powered on except for system suspend.
+ *
+ * GENPD_FLAG_NO_SUSPEND:	Instructs genpd to keep the PM domain powered
+ *				on during suspend (if it's already powered on)
+ *				and runtime PM controlled otherwise.
+ *
+ * GENPD_FLAG_NO_RESUME:	Instructs genpd to keep the PM domain powered
+ *				off during resume (if it's already powered off)
+ *				and runtime PM controlled otherwise.
  */
 #define GENPD_FLAG_PM_CLK	 (1U << 0)
 #define GENPD_FLAG_IRQ_SAFE	 (1U << 1)
@@ -62,6 +70,8 @@
 #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_NO_SUSPEND	 (1U << 6)
+#define GENPD_FLAG_NO_RESUME	 (1U << 7)
 
 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

[PATCH v2 2/2] soc: qcom: aoss: Use both GENPD_FLAG_NO_SUSPEND/RESUME flags

[Sibi Sankar]

All the power domains exposed as part of the AOSS QMP driver require to
stay unaltered during suspend/resume. 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_NO_SUSPEND and GENPD_FLAG_NO_RESUME
to model this behavior.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---

V2:
 * Use the renamed flags
 * Drop R-b/T-b

 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..da763ac6bca2f 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_NO_SUSPEND | GENPD_FLAG_NO_RESUME;
 
 		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

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Stephen Boyd]

Quoting Sibi Sankar (2020-08-21 13:49:20)
> Add GENPD_FLAG_NO_SUSPEND/RESUME flags to instruct genpd to keep the
> status of the PM domain unaltered during suspend/resume respectively.
> The flags are aimed at power domains coupled to co-processors which
> enter low-power modes independent to that of the application processor.
> 
> Specifically the flags are to be used by the power domains exposed
> by the AOSS QMP driver linked to modem, adsp, cdsp remoteprocs. These
> power domains are used to notify the Always on Subsystem (AOSS) that
> a particular co-processor is up. 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 and remains unaltered during
> system suspend/resume.

Why are these power domains instead of some QMP message sent during
remote proc power up? If this has been discussed before feel free to
disregard and please link to prior mailing list discussions.

I find it odd that this is modeled as a power domain instead of some
Qualcomm specific message that the remoteproc driver sends to AOSS. Is
there some sort of benefit the driver gets from using the power domain
APIs for this vs. using a custom API?

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Ulf Hansson]

On Fri, 21 Aug 2020 at 22:49, Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Add GENPD_FLAG_NO_SUSPEND/RESUME flags to instruct genpd to keep the
> status of the PM domain unaltered during suspend/resume respectively.
> The flags are aimed at power domains coupled to co-processors which
> enter low-power modes independent to that of the application processor.
>
> Specifically the flags are to be used by the power domains exposed
> by the AOSS QMP driver linked to modem, adsp, cdsp remoteprocs. These
> power domains are used to notify the Always on Subsystem (AOSS) that
> a particular co-processor is up. 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 and remains unaltered during
> system suspend/resume.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Although, I would like Stephen's question to get an answer in the
other thread, before this gets applied.

Kind regards
Uffe

> ---
>
> V2:
>  * Add more info in commit msg and description [Uffe/Kevin/Stephen]
>  * Rename and split functionality into two flags [Uffe]
>  * Drop R-b/T-b
>
>  drivers/base/power/domain.c |  6 ++++--
>  include/linux/pm_domain.h   | 10 ++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2cb5e04cf86cd..a5df5916f30f8 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -129,6 +129,8 @@ 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_no_suspend(genpd)     (genpd->flags & GENPD_FLAG_NO_SUSPEND)
> +#define genpd_is_no_resume(genpd)      (genpd->flags & GENPD_FLAG_NO_RESUME)
>
>  static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
>                 const struct generic_pm_domain *genpd)
> @@ -949,7 +951,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_no_suspend(genpd))
>                 return;
>
>         if (genpd->suspended_count != genpd->device_count
> @@ -991,7 +993,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_no_resume(genpd))
>                 return;
>
>         list_for_each_entry(link, &genpd->child_links, child_node) {
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index ee11502a575b0..568abdf2e89cf 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -55,6 +55,14 @@
>   *
>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM domain
>   *                             powered on except for system suspend.
> + *
> + * GENPD_FLAG_NO_SUSPEND:      Instructs genpd to keep the PM domain powered
> + *                             on during suspend (if it's already powered on)
> + *                             and runtime PM controlled otherwise.
> + *
> + * GENPD_FLAG_NO_RESUME:       Instructs genpd to keep the PM domain powered
> + *                             off during resume (if it's already powered off)
> + *                             and runtime PM controlled otherwise.
>   */
>  #define GENPD_FLAG_PM_CLK       (1U << 0)
>  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
> @@ -62,6 +70,8 @@
>  #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_NO_SUSPEND   (1U << 6)
> +#define GENPD_FLAG_NO_RESUME    (1U << 7)
>
>  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
>

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Bjorn Andersson]

On Fri 21 Aug 14:41 PDT 2020, Stephen Boyd wrote:

> Quoting Sibi Sankar (2020-08-21 13:49:20)
> > Add GENPD_FLAG_NO_SUSPEND/RESUME flags to instruct genpd to keep the
> > status of the PM domain unaltered during suspend/resume respectively.
> > The flags are aimed at power domains coupled to co-processors which
> > enter low-power modes independent to that of the application processor.
> > 
> > Specifically the flags are to be used by the power domains exposed
> > by the AOSS QMP driver linked to modem, adsp, cdsp remoteprocs. These
> > power domains are used to notify the Always on Subsystem (AOSS) that
> > a particular co-processor is up. 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 and remains unaltered during
> > system suspend/resume.
> 
> Why are these power domains instead of some QMP message sent during
> remote proc power up?

The understanding I gained as I researched this, was that with this
property enabled resources related to the particular subsystem will be
kept enabled when the apss enters some power save mode. So my
interpretation was that it does "keep something powered".

> If this has been discussed before feel free to
> disregard and please link to prior mailing list discussions.
> 

There where some discussions related to the "QDSS clk" in that series,
but I don't remember getting any feedback on modelling these things as
power-domains.

> I find it odd that this is modeled as a power domain instead of some
> Qualcomm specific message that the remoteproc driver sends to AOSS. Is
> there some sort of benefit the driver gets from using the power domain
> APIs for this vs. using a custom API?

We need to send "up" and "down" notifications and this needs to happen
at the same time as other standard resources are enabled/disabled.

Further more, at the time the all resources handled by the downstream
driver was either power-domains (per above understanding) or clocks, so
it made sense to me not to spin up a custom API.

Regards,
Bjorn

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Stephen Boyd]

Quoting Bjorn Andersson (2020-08-24 09:42:12)
> On Fri 21 Aug 14:41 PDT 2020, Stephen Boyd wrote:
> 
> > Quoting Sibi Sankar (2020-08-21 13:49:20)
> > > Add GENPD_FLAG_NO_SUSPEND/RESUME flags to instruct genpd to keep the
> > > status of the PM domain unaltered during suspend/resume respectively.
> > > The flags are aimed at power domains coupled to co-processors which
> > > enter low-power modes independent to that of the application processor.
> > > 
> > > Specifically the flags are to be used by the power domains exposed
> > > by the AOSS QMP driver linked to modem, adsp, cdsp remoteprocs. These
> > > power domains are used to notify the Always on Subsystem (AOSS) that
> > > a particular co-processor is up. 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 and remains unaltered during
> > > system suspend/resume.
> > 
> > Why are these power domains instead of some QMP message sent during
> > remote proc power up?
> 
> The understanding I gained as I researched this, was that with this
> property enabled resources related to the particular subsystem will be
> kept enabled when the apss enters some power save mode. So my
> interpretation was that it does "keep something powered".

It looks like it tells AOSS that the processor is booted and to start
considering these processors in the SoC wide system suspend sequence.
Otherwise I guess the RPMh buckets associated with these remoteprocs
don't count in the aggregation and sleep/wake sequences that AOSS runs
through when putting the SoC into low power mode. I'm not sure it
actually "keeps something powered" so much as it lets something be
powered off. Sibi?

Another question, why can't the processors tell AOSS themselves about
their boot state? I guess because they may crash or be powered down and
then AOSS wouldn't know? Fair enough I guess, but I don't think this is
mentioned anywhere.

> 
> > If this has been discussed before feel free to
> > disregard and please link to prior mailing list discussions.
> > 
> 
> There where some discussions related to the "QDSS clk" in that series,
> but I don't remember getting any feedback on modelling these things as
> power-domains.
> 
> > I find it odd that this is modeled as a power domain instead of some
> > Qualcomm specific message that the remoteproc driver sends to AOSS. Is
> > there some sort of benefit the driver gets from using the power domain
> > APIs for this vs. using a custom API?
> 
> We need to send "up" and "down" notifications and this needs to happen
> at the same time as other standard resources are enabled/disabled.
> 
> Further more, at the time the all resources handled by the downstream
> driver was either power-domains (per above understanding) or clocks, so
> it made sense to me not to spin up a custom API.
> 
 
So the benefit is not spinning up a custom API? I'm not Ulf, but it
looks like this is hard to rationalize about as a power domain. It
doesn't have any benefit to model it this way besides to make it
possible to turn on with other power domains.

This modem remoteproc drivers isn't SoC agnostic anyway, it relies on
SMEM APIs, so standing up another small qmp_remoteproc_booted() and
qmp_remoteproc_shutdown() API would avoid adding a genpd flag here that
probably will never be used outside of this corner-case. There is also
some get/put EPROBE_DEFER sort of logic to implement, but otherwise it
would be possible to do this outside of power domains, and that seems
better given that this isn't really a power domain to start with.

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Sibi Sankar]

On 2020-08-25 12:50, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2020-08-24 09:42:12)
>> On Fri 21 Aug 14:41 PDT 2020, Stephen Boyd wrote:
>> 
>> > Quoting Sibi Sankar (2020-08-21 13:49:20)
>> > > Add GENPD_FLAG_NO_SUSPEND/RESUME flags to instruct genpd to keep the
>> > > status of the PM domain unaltered during suspend/resume respectively.
>> > > The flags are aimed at power domains coupled to co-processors which
>> > > enter low-power modes independent to that of the application processor.
>> > >
>> > > Specifically the flags are to be used by the power domains exposed
>> > > by the AOSS QMP driver linked to modem, adsp, cdsp remoteprocs. These
>> > > power domains are used to notify the Always on Subsystem (AOSS) that
>> > > a particular co-processor is up. 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 and remains unaltered during
>> > > system suspend/resume.
>> >
>> > Why are these power domains instead of some QMP message sent during
>> > remote proc power up?
>> 
>> The understanding I gained as I researched this, was that with this
>> property enabled resources related to the particular subsystem will be
>> kept enabled when the apss enters some power save mode. So my
>> interpretation was that it does "keep something powered".
> 
> It looks like it tells AOSS that the processor is booted and to start
> considering these processors in the SoC wide system suspend sequence.
> Otherwise I guess the RPMh buckets associated with these remoteprocs
> don't count in the aggregation and sleep/wake sequences that AOSS runs
> through when putting the SoC into low power mode. I'm not sure it
> actually "keeps something powered" so much as it lets something be
> powered off. Sibi?

That is just a part of equation i.e
AOSS doesn't enter sleep until the
remote processors enter RPMh assisted
sleep. This also implies that if the
respective remote processor has to come
out of low power states it will need
to wait for AOSS to come out of sleep.
So clearly remote processors are dependent
on certain resources to be enabled by
the AOSS but the resources may not be
restricted to just corners.

> 
> Another question, why can't the processors tell AOSS themselves about
> their boot state? I guess because they may crash or be powered down and
> then AOSS wouldn't know? Fair enough I guess, but I don't think this is
> mentioned anywhere.
> 
>> 
>> > If this has been discussed before feel free to
>> > disregard and please link to prior mailing list discussions.
>> >
>> 
>> There where some discussions related to the "QDSS clk" in that series,
>> but I don't remember getting any feedback on modelling these things as
>> power-domains.
>> 
>> > I find it odd that this is modeled as a power domain instead of some
>> > Qualcomm specific message that the remoteproc driver sends to AOSS. Is
>> > there some sort of benefit the driver gets from using the power domain
>> > APIs for this vs. using a custom API?
>> 
>> We need to send "up" and "down" notifications and this needs to happen
>> at the same time as other standard resources are enabled/disabled.
>> 
>> Further more, at the time the all resources handled by the downstream
>> driver was either power-domains (per above understanding) or clocks, 
>> so
>> it made sense to me not to spin up a custom API.
>> 
> 
> So the benefit is not spinning up a custom API? I'm not Ulf, but it
> looks like this is hard to rationalize about as a power domain. It
> doesn't have any benefit to model it this way besides to make it
> possible to turn on with other power domains.
> 
> This modem remoteproc drivers isn't SoC agnostic anyway, it relies on
> SMEM APIs, so standing up another small qmp_remoteproc_booted() and
> qmp_remoteproc_shutdown() API would avoid adding a genpd flag here that
> probably will never be used outside of this corner-case. There is also
> some get/put EPROBE_DEFER sort of logic to implement, but otherwise it
> would be possible to do this outside of power domains, and that seems
> better given that this isn't really a power domain to start with.

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

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Bjorn Andersson]

On Tue 25 Aug 02:20 CDT 2020, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2020-08-24 09:42:12)
> > On Fri 21 Aug 14:41 PDT 2020, Stephen Boyd wrote:
[..]
> > > I find it odd that this is modeled as a power domain instead of some
> > > Qualcomm specific message that the remoteproc driver sends to AOSS. Is
> > > there some sort of benefit the driver gets from using the power domain
> > > APIs for this vs. using a custom API?
> > 
> > We need to send "up" and "down" notifications and this needs to happen
> > at the same time as other standard resources are enabled/disabled.
> > 
> > Further more, at the time the all resources handled by the downstream
> > driver was either power-domains (per above understanding) or clocks, so
> > it made sense to me not to spin up a custom API.
> > 
>  
> So the benefit is not spinning up a custom API? I'm not Ulf, but it
> looks like this is hard to rationalize about as a power domain. It
> doesn't have any benefit to model it this way besides to make it
> possible to turn on with other power domains.
> 
> This modem remoteproc drivers isn't SoC agnostic anyway, it relies on
> SMEM APIs, so standing up another small qmp_remoteproc_booted() and
> qmp_remoteproc_shutdown() API would avoid adding a genpd flag here that
> probably will never be used outside of this corner-case. There is also
> some get/put EPROBE_DEFER sort of logic to implement, but otherwise it
> would be possible to do this outside of power domains, and that seems
> better given that this isn't really a power domain to start with.

In later platforms a few new users of the AOSS communication interface
is introduced that certainly doesn't fit any existing API/framework in
the kernel. So the plan was to pretty much expose qmp_send() to these
drivers.

My worry with using this interface is that we'll probably have to come
up with some DT binding pieces and probably we'll end up adding yet
another piece of hard coded information in the remoteproc drivers.

But I'm not against us doing this work in favor of not having to
introduce a one-off for this corner case.

Regards,
Bjorn

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Sibi Sankar]

On 2020-08-25 23:23, Bjorn Andersson wrote:
> On Tue 25 Aug 02:20 CDT 2020, Stephen Boyd wrote:
>> Quoting Bjorn Andersson (2020-08-24 09:42:12)
>> > On Fri 21 Aug 14:41 PDT 2020, Stephen Boyd wrote:
> [..]
>> > > I find it odd that this is modeled as a power domain instead of some
>> > > Qualcomm specific message that the remoteproc driver sends to AOSS. Is
>> > > there some sort of benefit the driver gets from using the power domain
>> > > APIs for this vs. using a custom API?
>> >
>> > We need to send "up" and "down" notifications and this needs to happen
>> > at the same time as other standard resources are enabled/disabled.
>> >
>> > Further more, at the time the all resources handled by the downstream
>> > driver was either power-domains (per above understanding) or clocks, so
>> > it made sense to me not to spin up a custom API.
>> >
>> 
>> So the benefit is not spinning up a custom API? I'm not Ulf, but it
>> looks like this is hard to rationalize about as a power domain. It
>> doesn't have any benefit to model it this way besides to make it
>> possible to turn on with other power domains.
>> 
>> This modem remoteproc drivers isn't SoC agnostic anyway, it relies on
>> SMEM APIs, so standing up another small qmp_remoteproc_booted() and
>> qmp_remoteproc_shutdown() API would avoid adding a genpd flag here 
>> that
>> probably will never be used outside of this corner-case. There is also
>> some get/put EPROBE_DEFER sort of logic to implement, but otherwise it
>> would be possible to do this outside of power domains, and that seems
>> better given that this isn't really a power domain to start with.
> 
> In later platforms a few new users of the AOSS communication interface
> is introduced that certainly doesn't fit any existing API/framework in
> the kernel. So the plan was to pretty much expose qmp_send() to these
> drivers.
> 
> My worry with using this interface is that we'll probably have to come
> up with some DT binding pieces and probably we'll end up adding yet
> another piece of hard coded information in the remoteproc drivers.
> 
> But I'm not against us doing this work in favor of not having to
> introduce a one-off for this corner case.

Bjorn/Stephen,

So the consensus is to stop modelling
aoss load_state as pds and expose qmp_send
to drivers?

> 
> Regards,
> Bjorn




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

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Ulf Hansson]

On Thu, 10 Sep 2020 at 09:23, Sibi Sankar <sibis@codeaurora.org> wrote:
>
> On 2020-08-25 23:23, Bjorn Andersson wrote:
> > On Tue 25 Aug 02:20 CDT 2020, Stephen Boyd wrote:
> >> Quoting Bjorn Andersson (2020-08-24 09:42:12)
> >> > On Fri 21 Aug 14:41 PDT 2020, Stephen Boyd wrote:
> > [..]
> >> > > I find it odd that this is modeled as a power domain instead of some
> >> > > Qualcomm specific message that the remoteproc driver sends to AOSS. Is
> >> > > there some sort of benefit the driver gets from using the power domain
> >> > > APIs for this vs. using a custom API?
> >> >
> >> > We need to send "up" and "down" notifications and this needs to happen
> >> > at the same time as other standard resources are enabled/disabled.
> >> >
> >> > Further more, at the time the all resources handled by the downstream
> >> > driver was either power-domains (per above understanding) or clocks, so
> >> > it made sense to me not to spin up a custom API.
> >> >
> >>
> >> So the benefit is not spinning up a custom API? I'm not Ulf, but it
> >> looks like this is hard to rationalize about as a power domain. It
> >> doesn't have any benefit to model it this way besides to make it
> >> possible to turn on with other power domains.
> >>
> >> This modem remoteproc drivers isn't SoC agnostic anyway, it relies on
> >> SMEM APIs, so standing up another small qmp_remoteproc_booted() and
> >> qmp_remoteproc_shutdown() API would avoid adding a genpd flag here
> >> that
> >> probably will never be used outside of this corner-case. There is also
> >> some get/put EPROBE_DEFER sort of logic to implement, but otherwise it
> >> would be possible to do this outside of power domains, and that seems
> >> better given that this isn't really a power domain to start with.
> >
> > In later platforms a few new users of the AOSS communication interface
> > is introduced that certainly doesn't fit any existing API/framework in
> > the kernel. So the plan was to pretty much expose qmp_send() to these
> > drivers.
> >
> > My worry with using this interface is that we'll probably have to come
> > up with some DT binding pieces and probably we'll end up adding yet
> > another piece of hard coded information in the remoteproc drivers.
> >
> > But I'm not against us doing this work in favor of not having to
> > introduce a one-off for this corner case.
>
> Bjorn/Stephen,
>
> So the consensus is to stop modelling
> aoss load_state as pds and expose qmp_send
> to drivers?

Would that mean qmp_send would have to be called from generic drivers?
Then, please no. We want to keep drivers portable.

Kind regards
Uffe

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Bjorn Andersson]

On Thu 10 Sep 03:18 CDT 2020, Ulf Hansson wrote:

> On Thu, 10 Sep 2020 at 09:23, Sibi Sankar <sibis@codeaurora.org> wrote:
> >
> > On 2020-08-25 23:23, Bjorn Andersson wrote:
> > > On Tue 25 Aug 02:20 CDT 2020, Stephen Boyd wrote:
> > >> Quoting Bjorn Andersson (2020-08-24 09:42:12)
> > >> > On Fri 21 Aug 14:41 PDT 2020, Stephen Boyd wrote:
> > > [..]
> > >> > > I find it odd that this is modeled as a power domain instead of some
> > >> > > Qualcomm specific message that the remoteproc driver sends to AOSS. Is
> > >> > > there some sort of benefit the driver gets from using the power domain
> > >> > > APIs for this vs. using a custom API?
> > >> >
> > >> > We need to send "up" and "down" notifications and this needs to happen
> > >> > at the same time as other standard resources are enabled/disabled.
> > >> >
> > >> > Further more, at the time the all resources handled by the downstream
> > >> > driver was either power-domains (per above understanding) or clocks, so
> > >> > it made sense to me not to spin up a custom API.
> > >> >
> > >>
> > >> So the benefit is not spinning up a custom API? I'm not Ulf, but it
> > >> looks like this is hard to rationalize about as a power domain. It
> > >> doesn't have any benefit to model it this way besides to make it
> > >> possible to turn on with other power domains.
> > >>
> > >> This modem remoteproc drivers isn't SoC agnostic anyway, it relies on
> > >> SMEM APIs, so standing up another small qmp_remoteproc_booted() and
> > >> qmp_remoteproc_shutdown() API would avoid adding a genpd flag here
> > >> that
> > >> probably will never be used outside of this corner-case. There is also
> > >> some get/put EPROBE_DEFER sort of logic to implement, but otherwise it
> > >> would be possible to do this outside of power domains, and that seems
> > >> better given that this isn't really a power domain to start with.
> > >
> > > In later platforms a few new users of the AOSS communication interface
> > > is introduced that certainly doesn't fit any existing API/framework in
> > > the kernel. So the plan was to pretty much expose qmp_send() to these
> > > drivers.
> > >
> > > My worry with using this interface is that we'll probably have to come
> > > up with some DT binding pieces and probably we'll end up adding yet
> > > another piece of hard coded information in the remoteproc drivers.
> > >
> > > But I'm not against us doing this work in favor of not having to
> > > introduce a one-off for this corner case.
> >
> > Bjorn/Stephen,
> >
> > So the consensus is to stop modelling
> > aoss load_state as pds and expose qmp_send
> > to drivers?
> 
> Would that mean qmp_send would have to be called from generic drivers?
> Then, please no. We want to keep drivers portable.
> 

No, this is only called from Qualcomm specific drivers. So I'm okay with
that approach.

Regards,
Bjorn

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Rafael J. Wysocki]

On Fri, Aug 21, 2020 at 10:49 PM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Add GENPD_FLAG_NO_SUSPEND/RESUME flags to instruct genpd to keep the
> status of the PM domain unaltered during suspend/resume respectively.
> The flags are aimed at power domains coupled to co-processors which
> enter low-power modes independent to that of the application processor.
>
> Specifically the flags are to be used by the power domains exposed
> by the AOSS QMP driver linked to modem, adsp, cdsp remoteprocs. These
> power domains are used to notify the Always on Subsystem (AOSS) that
> a particular co-processor is up. 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 and remains unaltered during
> system suspend/resume.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>

Applied with the Ulf's R-by along with the [2/2] as 5.10 material, thanks!

> ---
>
> V2:
>  * Add more info in commit msg and description [Uffe/Kevin/Stephen]
>  * Rename and split functionality into two flags [Uffe]
>  * Drop R-b/T-b
>
>  drivers/base/power/domain.c |  6 ++++--
>  include/linux/pm_domain.h   | 10 ++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2cb5e04cf86cd..a5df5916f30f8 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -129,6 +129,8 @@ 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_no_suspend(genpd)     (genpd->flags & GENPD_FLAG_NO_SUSPEND)
> +#define genpd_is_no_resume(genpd)      (genpd->flags & GENPD_FLAG_NO_RESUME)
>
>  static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
>                 const struct generic_pm_domain *genpd)
> @@ -949,7 +951,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_no_suspend(genpd))
>                 return;
>
>         if (genpd->suspended_count != genpd->device_count
> @@ -991,7 +993,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_no_resume(genpd))
>                 return;
>
>         list_for_each_entry(link, &genpd->child_links, child_node) {
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index ee11502a575b0..568abdf2e89cf 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -55,6 +55,14 @@
>   *
>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM domain
>   *                             powered on except for system suspend.
> + *
> + * GENPD_FLAG_NO_SUSPEND:      Instructs genpd to keep the PM domain powered
> + *                             on during suspend (if it's already powered on)
> + *                             and runtime PM controlled otherwise.
> + *
> + * GENPD_FLAG_NO_RESUME:       Instructs genpd to keep the PM domain powered
> + *                             off during resume (if it's already powered off)
> + *                             and runtime PM controlled otherwise.
>   */
>  #define GENPD_FLAG_PM_CLK       (1U << 0)
>  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
> @@ -62,6 +70,8 @@
>  #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_NO_SUSPEND   (1U << 6)
> +#define GENPD_FLAG_NO_RESUME    (1U << 7)
>
>  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
>

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Sibi Sankar]

On 2020-09-22 01:26, Stephen Boyd wrote:
> Quoting Rafael J. Wysocki (2020-09-21 09:18:17)
>> On Fri, Aug 21, 2020 at 10:49 PM Sibi Sankar <sibis@codeaurora.org> 
>> wrote:
>> >
>> > Add GENPD_FLAG_NO_SUSPEND/RESUME flags to instruct genpd to keep the
>> > status of the PM domain unaltered during suspend/resume respectively.
>> > The flags are aimed at power domains coupled to co-processors which
>> > enter low-power modes independent to that of the application processor.
>> >
>> > Specifically the flags are to be used by the power domains exposed
>> > by the AOSS QMP driver linked to modem, adsp, cdsp remoteprocs. These
>> > power domains are used to notify the Always on Subsystem (AOSS) that
>> > a particular co-processor is up. 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 and remains unaltered during
>> > system suspend/resume.
>> >
>> > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> 
>> Applied with the Ulf's R-by along with the [2/2] as 5.10 material, 
>> thanks!
>> 
> 
> There was a bunch of discussion on this patch series and I thought the
> consensus was to not apply these patches and instead implement a custom
> qcom specific API that does this instead.

https://lore.kernel.org/lkml/20200913034603.GV3715@yoga/

The power domains which were targeted
to use the flags will be replaced by
custom qcom specific API. So let's not
pick up the patch series.


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

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Ulf Hansson]

On Tue, 22 Sep 2020 at 06:51, Sibi Sankar <sibis@codeaurora.org> wrote:
>
> On 2020-09-22 01:26, Stephen Boyd wrote:
> > Quoting Rafael J. Wysocki (2020-09-21 09:18:17)
> >> On Fri, Aug 21, 2020 at 10:49 PM Sibi Sankar <sibis@codeaurora.org>
> >> wrote:
> >> >
> >> > Add GENPD_FLAG_NO_SUSPEND/RESUME flags to instruct genpd to keep the
> >> > status of the PM domain unaltered during suspend/resume respectively.
> >> > The flags are aimed at power domains coupled to co-processors which
> >> > enter low-power modes independent to that of the application processor.
> >> >
> >> > Specifically the flags are to be used by the power domains exposed
> >> > by the AOSS QMP driver linked to modem, adsp, cdsp remoteprocs. These
> >> > power domains are used to notify the Always on Subsystem (AOSS) that
> >> > a particular co-processor is up. 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 and remains unaltered during
> >> > system suspend/resume.
> >> >
> >> > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> >>
> >> Applied with the Ulf's R-by along with the [2/2] as 5.10 material,
> >> thanks!
> >>
> >
> > There was a bunch of discussion on this patch series and I thought the
> > consensus was to not apply these patches and instead implement a custom
> > qcom specific API that does this instead.
>
> https://lore.kernel.org/lkml/20200913034603.GV3715@yoga/
>
> The power domains which were targeted
> to use the flags will be replaced by
> custom qcom specific API. So let's not
> pick up the patch series.

I am fine with either option. However, please keep me posted, as I am
a bit curious how this will look like in the qcom specific drivers.

Kind regards
Uffe

Re: [PATCH v2 1/2] PM / Domains: Add GENPD_FLAG_NO_SUSPEND/RESUME flags

[Rafael J. Wysocki]

On Tue, Sep 22, 2020 at 6:51 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> On 2020-09-22 01:26, Stephen Boyd wrote:
> > Quoting Rafael J. Wysocki (2020-09-21 09:18:17)
> >> On Fri, Aug 21, 2020 at 10:49 PM Sibi Sankar <sibis@codeaurora.org>
> >> wrote:
> >> >
> >> > Add GENPD_FLAG_NO_SUSPEND/RESUME flags to instruct genpd to keep the
> >> > status of the PM domain unaltered during suspend/resume respectively.
> >> > The flags are aimed at power domains coupled to co-processors which
> >> > enter low-power modes independent to that of the application processor.
> >> >
> >> > Specifically the flags are to be used by the power domains exposed
> >> > by the AOSS QMP driver linked to modem, adsp, cdsp remoteprocs. These
> >> > power domains are used to notify the Always on Subsystem (AOSS) that
> >> > a particular co-processor is up. 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 and remains unaltered during
> >> > system suspend/resume.
> >> >
> >> > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> >>
> >> Applied with the Ulf's R-by along with the [2/2] as 5.10 material,
> >> thanks!
> >>
> >
> > There was a bunch of discussion on this patch series and I thought the
> > consensus was to not apply these patches and instead implement a custom
> > qcom specific API that does this instead.
>
> https://lore.kernel.org/lkml/20200913034603.GV3715@yoga/
>
> The power domains which were targeted
> to use the flags will be replaced by
> custom qcom specific API. So let's not
> pick up the patch series.

OK, I'm dropping it then, thanks!