* Re: [PATCH 2/2] cpuidle: psci: Enable s2idle when using OSI with the PM domain topology
@ 2020-10-01 10:17 ` Sudeep Holla
0 siblings, 0 replies; 14+ messages in thread
From: Sudeep Holla @ 2020-10-01 10:17 UTC (permalink / raw)
To: Ulf Hansson
Cc: Lorenzo Pieralisi, Vincent Guittot, Benjamin Gaignard, linux-pm,
Stephen Boyd, Daniel Lezcano, Rafael J . Wysocki, Lina Iyer,
Bjorn Andersson, linux-arm-kernel, Sudeep Holla, Lukasz Luba
On Tue, Sep 01, 2020 at 10:27:07AM +0200, Ulf Hansson wrote:
> To select domain idle states for cpuidle-psci, the PM domains via genpd are
> being managed by using runtime PM. This works fine for the regular idle
> path, but it doesn't when doing s2idle.
>
> More precisely, the domain idle states becomes temporarily disabled, which
> is because the PM core disables runtime PM for devices during system
> suspend.
When you refer system suspend above, you mean both S2R and S2I ?
> Even if genpd tries to power off the PM domain in the
> suspend_noirq phase, that doesn't help to properly select a domain idle
> state, as this needs to be done on per CPU basis.
>
And what prevents doing per CPU basis ?
> Let's address the issue by enabling the syscore flag for the attached CPU
> devices. This prevents genpd from trying to power off the corresponding PM
> domains in the suspend_noirq phase. Moreover, let's assign a specific
> ->enter_s2idle() callback for the corresponding domain idle state and let
> it invoke pm_genpd_syscore_poweroff|poweron(), rather than using runtime
> PM.
>
The syscore_suspend is not executed for S2I and using syscore APIs here
is bit confusing IMO. If Rafael is fine, I have no objections.
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/cpuidle/cpuidle-psci-domain.c | 2 ++
> drivers/cpuidle/cpuidle-psci.c | 30 +++++++++++++++++++++++----
> 2 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> index b6e9649ab0da..65437ba5fa78 100644
> --- a/drivers/cpuidle/cpuidle-psci-domain.c
> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> @@ -324,6 +324,8 @@ struct device *psci_dt_attach_cpu(int cpu)
> if (cpu_online(cpu))
> pm_runtime_get_sync(dev);
>
> + dev_pm_syscore_device(dev, true);
> +
> return dev;
> }
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 74463841805f..6322d55a0a7d 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -19,6 +19,7 @@
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/psci.h>
> +#include <linux/pm_domain.h>
> #include <linux/pm_runtime.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> @@ -52,8 +53,9 @@ static inline int psci_enter_state(int idx, u32 state)
> return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> }
>
> -static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> - struct cpuidle_driver *drv, int idx)
> +static int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int idx,
> + bool s2idle)
> {
> struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> u32 *states = data->psci_states;
> @@ -66,7 +68,10 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> return -1;
>
> /* Do runtime PM to manage a hierarchical CPU toplogy. */
> - pm_runtime_put_sync_suspend(pd_dev);
> + if (s2idle)
> + pm_genpd_syscore_poweroff(pd_dev);
> + else
> + pm_runtime_put_sync_suspend(pd_dev);
Since CPU genpd is special and handled so in core, can this be moved to core ?
I mean pm_runtime_put_sync_suspend handle that based genpd_is_cpu_domain.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/2] cpuidle: psci: Enable s2idle when using OSI with the PM domain topology
2020-10-01 10:17 ` Sudeep Holla
@ 2020-10-01 11:24 ` Rafael J. Wysocki
-1 siblings, 0 replies; 14+ messages in thread
From: Rafael J. Wysocki @ 2020-10-01 11:24 UTC (permalink / raw)
To: Sudeep Holla
Cc: Ulf Hansson, Lorenzo Pieralisi, Linux PM, Rafael J . Wysocki,
Daniel Lezcano, Lina Iyer, Lukasz Luba, Vincent Guittot,
Stephen Boyd, Bjorn Andersson, Benjamin Gaignard, Linux ARM
On Thu, Oct 1, 2020 at 12:18 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Sep 01, 2020 at 10:27:07AM +0200, Ulf Hansson wrote:
> > To select domain idle states for cpuidle-psci, the PM domains via genpd are
> > being managed by using runtime PM. This works fine for the regular idle
> > path, but it doesn't when doing s2idle.
> >
> > More precisely, the domain idle states becomes temporarily disabled, which
> > is because the PM core disables runtime PM for devices during system
> > suspend.
>
> When you refer system suspend above, you mean both S2R and S2I ?
>
> > Even if genpd tries to power off the PM domain in the
> > suspend_noirq phase, that doesn't help to properly select a domain idle
> > state, as this needs to be done on per CPU basis.
> >
>
> And what prevents doing per CPU basis ?
>
> > Let's address the issue by enabling the syscore flag for the attached CPU
> > devices. This prevents genpd from trying to power off the corresponding PM
> > domains in the suspend_noirq phase. Moreover, let's assign a specific
> > ->enter_s2idle() callback for the corresponding domain idle state and let
> > it invoke pm_genpd_syscore_poweroff|poweron(), rather than using runtime
> > PM.
> >
>
> The syscore_suspend is not executed for S2I and using syscore APIs here
> is bit confusing IMO.
Right.
> If Rafael is fine,
No, I'm not.
Cheers!
> I have no objections.
>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > drivers/cpuidle/cpuidle-psci-domain.c | 2 ++
> > drivers/cpuidle/cpuidle-psci.c | 30 +++++++++++++++++++++++----
> > 2 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > index b6e9649ab0da..65437ba5fa78 100644
> > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > @@ -324,6 +324,8 @@ struct device *psci_dt_attach_cpu(int cpu)
> > if (cpu_online(cpu))
> > pm_runtime_get_sync(dev);
> >
> > + dev_pm_syscore_device(dev, true);
> > +
> > return dev;
> > }
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 74463841805f..6322d55a0a7d 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -19,6 +19,7 @@
> > #include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/psci.h>
> > +#include <linux/pm_domain.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/slab.h>
> > #include <linux/string.h>
> > @@ -52,8 +53,9 @@ static inline int psci_enter_state(int idx, u32 state)
> > return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> > }
> >
> > -static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > - struct cpuidle_driver *drv, int idx)
> > +static int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv, int idx,
> > + bool s2idle)
> > {
> > struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> > u32 *states = data->psci_states;
> > @@ -66,7 +68,10 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > return -1;
> >
> > /* Do runtime PM to manage a hierarchical CPU toplogy. */
> > - pm_runtime_put_sync_suspend(pd_dev);
> > + if (s2idle)
> > + pm_genpd_syscore_poweroff(pd_dev);
> > + else
> > + pm_runtime_put_sync_suspend(pd_dev);
>
> Since CPU genpd is special and handled so in core, can this be moved to core ?
> I mean pm_runtime_put_sync_suspend handle that based genpd_is_cpu_domain.
>
> --
> Regards,
> Sudeep
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/2] cpuidle: psci: Enable s2idle when using OSI with the PM domain topology
@ 2020-10-01 11:24 ` Rafael J. Wysocki
0 siblings, 0 replies; 14+ messages in thread
From: Rafael J. Wysocki @ 2020-10-01 11:24 UTC (permalink / raw)
To: Sudeep Holla
Cc: Ulf Hansson, Lorenzo Pieralisi, Vincent Guittot,
Benjamin Gaignard, Linux PM, Stephen Boyd, Daniel Lezcano,
Rafael J . Wysocki, Lina Iyer, Bjorn Andersson, Linux ARM,
Lukasz Luba
On Thu, Oct 1, 2020 at 12:18 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Sep 01, 2020 at 10:27:07AM +0200, Ulf Hansson wrote:
> > To select domain idle states for cpuidle-psci, the PM domains via genpd are
> > being managed by using runtime PM. This works fine for the regular idle
> > path, but it doesn't when doing s2idle.
> >
> > More precisely, the domain idle states becomes temporarily disabled, which
> > is because the PM core disables runtime PM for devices during system
> > suspend.
>
> When you refer system suspend above, you mean both S2R and S2I ?
>
> > Even if genpd tries to power off the PM domain in the
> > suspend_noirq phase, that doesn't help to properly select a domain idle
> > state, as this needs to be done on per CPU basis.
> >
>
> And what prevents doing per CPU basis ?
>
> > Let's address the issue by enabling the syscore flag for the attached CPU
> > devices. This prevents genpd from trying to power off the corresponding PM
> > domains in the suspend_noirq phase. Moreover, let's assign a specific
> > ->enter_s2idle() callback for the corresponding domain idle state and let
> > it invoke pm_genpd_syscore_poweroff|poweron(), rather than using runtime
> > PM.
> >
>
> The syscore_suspend is not executed for S2I and using syscore APIs here
> is bit confusing IMO.
Right.
> If Rafael is fine,
No, I'm not.
Cheers!
> I have no objections.
>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > drivers/cpuidle/cpuidle-psci-domain.c | 2 ++
> > drivers/cpuidle/cpuidle-psci.c | 30 +++++++++++++++++++++++----
> > 2 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > index b6e9649ab0da..65437ba5fa78 100644
> > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > @@ -324,6 +324,8 @@ struct device *psci_dt_attach_cpu(int cpu)
> > if (cpu_online(cpu))
> > pm_runtime_get_sync(dev);
> >
> > + dev_pm_syscore_device(dev, true);
> > +
> > return dev;
> > }
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 74463841805f..6322d55a0a7d 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -19,6 +19,7 @@
> > #include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/psci.h>
> > +#include <linux/pm_domain.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/slab.h>
> > #include <linux/string.h>
> > @@ -52,8 +53,9 @@ static inline int psci_enter_state(int idx, u32 state)
> > return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> > }
> >
> > -static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > - struct cpuidle_driver *drv, int idx)
> > +static int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv, int idx,
> > + bool s2idle)
> > {
> > struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> > u32 *states = data->psci_states;
> > @@ -66,7 +68,10 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > return -1;
> >
> > /* Do runtime PM to manage a hierarchical CPU toplogy. */
> > - pm_runtime_put_sync_suspend(pd_dev);
> > + if (s2idle)
> > + pm_genpd_syscore_poweroff(pd_dev);
> > + else
> > + pm_runtime_put_sync_suspend(pd_dev);
>
> Since CPU genpd is special and handled so in core, can this be moved to core ?
> I mean pm_runtime_put_sync_suspend handle that based genpd_is_cpu_domain.
>
> --
> Regards,
> Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] cpuidle: psci: Enable s2idle when using OSI with the PM domain topology
2020-10-01 10:17 ` Sudeep Holla
@ 2020-10-01 11:32 ` Ulf Hansson
-1 siblings, 0 replies; 14+ messages in thread
From: Ulf Hansson @ 2020-10-01 11:32 UTC (permalink / raw)
To: Sudeep Holla
Cc: Lorenzo Pieralisi, Linux PM, Rafael J . Wysocki, Daniel Lezcano,
Lina Iyer, Lukasz Luba, Vincent Guittot, Stephen Boyd,
Bjorn Andersson, Benjamin Gaignard, Linux ARM
On Thu, 1 Oct 2020 at 12:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Sep 01, 2020 at 10:27:07AM +0200, Ulf Hansson wrote:
> > To select domain idle states for cpuidle-psci, the PM domains via genpd are
> > being managed by using runtime PM. This works fine for the regular idle
> > path, but it doesn't when doing s2idle.
> >
> > More precisely, the domain idle states becomes temporarily disabled, which
> > is because the PM core disables runtime PM for devices during system
> > suspend.
>
> When you refer system suspend above, you mean both S2R and S2I ?
Correct.
Although, there is no problem with S2R to reach the proper idlestate,
because of the way we offline all but the boot CPU.
>
> > Even if genpd tries to power off the PM domain in the
> > suspend_noirq phase, that doesn't help to properly select a domain idle
> > state, as this needs to be done on per CPU basis.
> >
>
> And what prevents doing per CPU basis ?
The PM core doesn't execute the system suspend callbacks on a per CPU basis.
>
> > Let's address the issue by enabling the syscore flag for the attached CPU
> > devices. This prevents genpd from trying to power off the corresponding PM
> > domains in the suspend_noirq phase. Moreover, let's assign a specific
> > ->enter_s2idle() callback for the corresponding domain idle state and let
> > it invoke pm_genpd_syscore_poweroff|poweron(), rather than using runtime
> > PM.
> >
>
> The syscore_suspend is not executed for S2I and using syscore APIs here
> is bit confusing IMO. If Rafael is fine, I have no objections.
That's correct, the syscore phase doesn't exist in the S2I path.
However, in some cases the same functions that are being called in the
syscore phase, are also being called for S2I. For example, have a look
at timekeeping_suspend(), which is being called from both paths.
In the end, I think the confusing part is the name of the genpd functions.
Maybe we should rename pm_genpd_syscore_poweroff|poweron() to
pm_genpd_suspend|resume() - or something along those lines.
Kind regards
Uffe
>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > drivers/cpuidle/cpuidle-psci-domain.c | 2 ++
> > drivers/cpuidle/cpuidle-psci.c | 30 +++++++++++++++++++++++----
> > 2 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > index b6e9649ab0da..65437ba5fa78 100644
> > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > @@ -324,6 +324,8 @@ struct device *psci_dt_attach_cpu(int cpu)
> > if (cpu_online(cpu))
> > pm_runtime_get_sync(dev);
> >
> > + dev_pm_syscore_device(dev, true);
> > +
> > return dev;
> > }
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 74463841805f..6322d55a0a7d 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -19,6 +19,7 @@
> > #include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/psci.h>
> > +#include <linux/pm_domain.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/slab.h>
> > #include <linux/string.h>
> > @@ -52,8 +53,9 @@ static inline int psci_enter_state(int idx, u32 state)
> > return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> > }
> >
> > -static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > - struct cpuidle_driver *drv, int idx)
> > +static int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv, int idx,
> > + bool s2idle)
> > {
> > struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> > u32 *states = data->psci_states;
> > @@ -66,7 +68,10 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > return -1;
> >
> > /* Do runtime PM to manage a hierarchical CPU toplogy. */
> > - pm_runtime_put_sync_suspend(pd_dev);
> > + if (s2idle)
> > + pm_genpd_syscore_poweroff(pd_dev);
> > + else
> > + pm_runtime_put_sync_suspend(pd_dev);
>
> Since CPU genpd is special and handled so in core, can this be moved to core ?
> I mean pm_runtime_put_sync_suspend handle that based genpd_is_cpu_domain.
>
> --
> Regards,
> Sudeep
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/2] cpuidle: psci: Enable s2idle when using OSI with the PM domain topology
@ 2020-10-01 11:32 ` Ulf Hansson
0 siblings, 0 replies; 14+ messages in thread
From: Ulf Hansson @ 2020-10-01 11:32 UTC (permalink / raw)
To: Sudeep Holla
Cc: Lorenzo Pieralisi, Vincent Guittot, Benjamin Gaignard, Linux PM,
Stephen Boyd, Daniel Lezcano, Rafael J . Wysocki, Lina Iyer,
Bjorn Andersson, Linux ARM, Lukasz Luba
On Thu, 1 Oct 2020 at 12:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Sep 01, 2020 at 10:27:07AM +0200, Ulf Hansson wrote:
> > To select domain idle states for cpuidle-psci, the PM domains via genpd are
> > being managed by using runtime PM. This works fine for the regular idle
> > path, but it doesn't when doing s2idle.
> >
> > More precisely, the domain idle states becomes temporarily disabled, which
> > is because the PM core disables runtime PM for devices during system
> > suspend.
>
> When you refer system suspend above, you mean both S2R and S2I ?
Correct.
Although, there is no problem with S2R to reach the proper idlestate,
because of the way we offline all but the boot CPU.
>
> > Even if genpd tries to power off the PM domain in the
> > suspend_noirq phase, that doesn't help to properly select a domain idle
> > state, as this needs to be done on per CPU basis.
> >
>
> And what prevents doing per CPU basis ?
The PM core doesn't execute the system suspend callbacks on a per CPU basis.
>
> > Let's address the issue by enabling the syscore flag for the attached CPU
> > devices. This prevents genpd from trying to power off the corresponding PM
> > domains in the suspend_noirq phase. Moreover, let's assign a specific
> > ->enter_s2idle() callback for the corresponding domain idle state and let
> > it invoke pm_genpd_syscore_poweroff|poweron(), rather than using runtime
> > PM.
> >
>
> The syscore_suspend is not executed for S2I and using syscore APIs here
> is bit confusing IMO. If Rafael is fine, I have no objections.
That's correct, the syscore phase doesn't exist in the S2I path.
However, in some cases the same functions that are being called in the
syscore phase, are also being called for S2I. For example, have a look
at timekeeping_suspend(), which is being called from both paths.
In the end, I think the confusing part is the name of the genpd functions.
Maybe we should rename pm_genpd_syscore_poweroff|poweron() to
pm_genpd_suspend|resume() - or something along those lines.
Kind regards
Uffe
>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > drivers/cpuidle/cpuidle-psci-domain.c | 2 ++
> > drivers/cpuidle/cpuidle-psci.c | 30 +++++++++++++++++++++++----
> > 2 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > index b6e9649ab0da..65437ba5fa78 100644
> > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > @@ -324,6 +324,8 @@ struct device *psci_dt_attach_cpu(int cpu)
> > if (cpu_online(cpu))
> > pm_runtime_get_sync(dev);
> >
> > + dev_pm_syscore_device(dev, true);
> > +
> > return dev;
> > }
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index 74463841805f..6322d55a0a7d 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -19,6 +19,7 @@
> > #include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/psci.h>
> > +#include <linux/pm_domain.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/slab.h>
> > #include <linux/string.h>
> > @@ -52,8 +53,9 @@ static inline int psci_enter_state(int idx, u32 state)
> > return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> > }
> >
> > -static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > - struct cpuidle_driver *drv, int idx)
> > +static int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv, int idx,
> > + bool s2idle)
> > {
> > struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> > u32 *states = data->psci_states;
> > @@ -66,7 +68,10 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> > return -1;
> >
> > /* Do runtime PM to manage a hierarchical CPU toplogy. */
> > - pm_runtime_put_sync_suspend(pd_dev);
> > + if (s2idle)
> > + pm_genpd_syscore_poweroff(pd_dev);
> > + else
> > + pm_runtime_put_sync_suspend(pd_dev);
>
> Since CPU genpd is special and handled so in core, can this be moved to core ?
> I mean pm_runtime_put_sync_suspend handle that based genpd_is_cpu_domain.
>
> --
> Regards,
> Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/2] cpuidle: psci: Enable s2idle when using OSI with the PM domain topology
2020-10-01 11:32 ` Ulf Hansson
@ 2020-10-01 11:44 ` Ulf Hansson
-1 siblings, 0 replies; 14+ messages in thread
From: Ulf Hansson @ 2020-10-01 11:44 UTC (permalink / raw)
To: Rafael J . Wysocki
Cc: Sudeep Holla, Lorenzo Pieralisi, Linux PM, Daniel Lezcano,
Lina Iyer, Lukasz Luba, Vincent Guittot, Stephen Boyd,
Bjorn Andersson, Benjamin Gaignard, Linux ARM
On Thu, 1 Oct 2020 at 13:32, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 1 Oct 2020 at 12:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Tue, Sep 01, 2020 at 10:27:07AM +0200, Ulf Hansson wrote:
> > > To select domain idle states for cpuidle-psci, the PM domains via genpd are
> > > being managed by using runtime PM. This works fine for the regular idle
> > > path, but it doesn't when doing s2idle.
> > >
> > > More precisely, the domain idle states becomes temporarily disabled, which
> > > is because the PM core disables runtime PM for devices during system
> > > suspend.
> >
> > When you refer system suspend above, you mean both S2R and S2I ?
>
> Correct.
>
> Although, there is no problem with S2R to reach the proper idlestate,
> because of the way we offline all but the boot CPU.
>
> >
> > > Even if genpd tries to power off the PM domain in the
> > > suspend_noirq phase, that doesn't help to properly select a domain idle
> > > state, as this needs to be done on per CPU basis.
> > >
> >
> > And what prevents doing per CPU basis ?
>
> The PM core doesn't execute the system suspend callbacks on a per CPU basis.
>
> >
> > > Let's address the issue by enabling the syscore flag for the attached CPU
> > > devices. This prevents genpd from trying to power off the corresponding PM
> > > domains in the suspend_noirq phase. Moreover, let's assign a specific
> > > ->enter_s2idle() callback for the corresponding domain idle state and let
> > > it invoke pm_genpd_syscore_poweroff|poweron(), rather than using runtime
> > > PM.
> > >
> >
> > The syscore_suspend is not executed for S2I and using syscore APIs here
> > is bit confusing IMO. If Rafael is fine, I have no objections.
>
> That's correct, the syscore phase doesn't exist in the S2I path.
>
> However, in some cases the same functions that are being called in the
> syscore phase, are also being called for S2I. For example, have a look
> at timekeeping_suspend(), which is being called from both paths.
>
> In the end, I think the confusing part is the name of the genpd functions.
>
> Maybe we should rename pm_genpd_syscore_poweroff|poweron() to
> pm_genpd_suspend|resume() - or something along those lines.
>
[...]
Rafael, I understand you have objections to the approach. Would
renaming the genpd APIs be a way forward? Another option is to add two
new genpd APIs along the side of the existing, not sure what would be
the best.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] cpuidle: psci: Enable s2idle when using OSI with the PM domain topology
@ 2020-10-01 11:44 ` Ulf Hansson
0 siblings, 0 replies; 14+ messages in thread
From: Ulf Hansson @ 2020-10-01 11:44 UTC (permalink / raw)
To: Rafael J . Wysocki
Cc: Lorenzo Pieralisi, Vincent Guittot, Benjamin Gaignard, Linux PM,
Stephen Boyd, Daniel Lezcano, Lina Iyer, Bjorn Andersson,
Linux ARM, Sudeep Holla, Lukasz Luba
On Thu, 1 Oct 2020 at 13:32, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 1 Oct 2020 at 12:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Tue, Sep 01, 2020 at 10:27:07AM +0200, Ulf Hansson wrote:
> > > To select domain idle states for cpuidle-psci, the PM domains via genpd are
> > > being managed by using runtime PM. This works fine for the regular idle
> > > path, but it doesn't when doing s2idle.
> > >
> > > More precisely, the domain idle states becomes temporarily disabled, which
> > > is because the PM core disables runtime PM for devices during system
> > > suspend.
> >
> > When you refer system suspend above, you mean both S2R and S2I ?
>
> Correct.
>
> Although, there is no problem with S2R to reach the proper idlestate,
> because of the way we offline all but the boot CPU.
>
> >
> > > Even if genpd tries to power off the PM domain in the
> > > suspend_noirq phase, that doesn't help to properly select a domain idle
> > > state, as this needs to be done on per CPU basis.
> > >
> >
> > And what prevents doing per CPU basis ?
>
> The PM core doesn't execute the system suspend callbacks on a per CPU basis.
>
> >
> > > Let's address the issue by enabling the syscore flag for the attached CPU
> > > devices. This prevents genpd from trying to power off the corresponding PM
> > > domains in the suspend_noirq phase. Moreover, let's assign a specific
> > > ->enter_s2idle() callback for the corresponding domain idle state and let
> > > it invoke pm_genpd_syscore_poweroff|poweron(), rather than using runtime
> > > PM.
> > >
> >
> > The syscore_suspend is not executed for S2I and using syscore APIs here
> > is bit confusing IMO. If Rafael is fine, I have no objections.
>
> That's correct, the syscore phase doesn't exist in the S2I path.
>
> However, in some cases the same functions that are being called in the
> syscore phase, are also being called for S2I. For example, have a look
> at timekeeping_suspend(), which is being called from both paths.
>
> In the end, I think the confusing part is the name of the genpd functions.
>
> Maybe we should rename pm_genpd_syscore_poweroff|poweron() to
> pm_genpd_suspend|resume() - or something along those lines.
>
[...]
Rafael, I understand you have objections to the approach. Would
renaming the genpd APIs be a way forward? Another option is to add two
new genpd APIs along the side of the existing, not sure what would be
the best.
Kind regards
Uffe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread