[1/5] cpuidle: psci: Fail cpuidle registration if set OSI mode failed
diff mbox series

Message ID 20200615152054.6819-2-ulf.hansson@linaro.org
State Superseded, archived
Headers show
Series
  • cpuidle: psci: Various improvements for PSCI PM domains
Related show

Commit Message

Ulf Hansson June 15, 2020, 3:20 p.m. UTC
Currently we allow the cpuidle driver registration to succeed, even if we
failed to enable the OSI mode when the hierarchical DT layout is used. This
means running in a degraded mode, by using the available idle states per
CPU, while also preventing the domain idle states.

Moving forward, this behaviour looks quite questionable to maintain, as
complexity seems to grow around it, especially when trying to add support
for deferred probe, for example.

Therefore, let's make the cpuidle driver registration to fail in this
situation, thus relying on the default architectural cpuidle backend for
WFI to be used.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/cpuidle/cpuidle-psci-domain.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Lina Iyer June 18, 2020, 6:01 p.m. UTC | #1
On Mon, Jun 15 2020 at 09:21 -0600, Ulf Hansson wrote:
>Currently we allow the cpuidle driver registration to succeed, even if we
>failed to enable the OSI mode when the hierarchical DT layout is used. This
>means running in a degraded mode, by using the available idle states per
>CPU, while also preventing the domain idle states.
>
>Moving forward, this behaviour looks quite questionable to maintain, as
>complexity seems to grow around it, especially when trying to add support
>for deferred probe, for example.
>
>Therefore, let's make the cpuidle driver registration to fail in this
>situation, thus relying on the default architectural cpuidle backend for
>WFI to be used.
>
>Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

May be PATCH 3/5 should come before this change, but for this patch itself,
please consider -

Reviewed-by: Lina Iyer <ilina@codeaurora.org>

>---
> drivers/cpuidle/cpuidle-psci-domain.c | 5 -----
> 1 file changed, 5 deletions(-)
>
>diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
>index 423f03bbeb74..f07786aad673 100644
>--- a/drivers/cpuidle/cpuidle-psci-domain.c
>+++ b/drivers/cpuidle/cpuidle-psci-domain.c
>@@ -26,7 +26,6 @@ struct psci_pd_provider {
> };
>
> static LIST_HEAD(psci_pd_providers);
>-static bool osi_mode_enabled __initdata;
>
> static int psci_pd_power_off(struct generic_pm_domain *pd)
> {
>@@ -272,7 +271,6 @@ static int __init psci_idle_init_domains(void)
> 		goto remove_pd;
> 	}
>
>-	osi_mode_enabled = true;
> 	of_node_put(np);
> 	pr_info("Initialized CPU PM domain topology\n");
> 	return pd_count;
>@@ -293,9 +291,6 @@ struct device __init *psci_dt_attach_cpu(int cpu)
> {
> 	struct device *dev;
>
>-	if (!osi_mode_enabled)
>-		return NULL;
>-
> 	dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> 	if (IS_ERR_OR_NULL(dev))
> 		return dev;
>-- 
>2.20.1
>
Sudeep Holla June 26, 2020, 2:33 p.m. UTC | #2
On Mon, Jun 15, 2020 at 05:20:50PM +0200, Ulf Hansson wrote:
> Currently we allow the cpuidle driver registration to succeed, even if we
> failed to enable the OSI mode when the hierarchical DT layout is used. This
> means running in a degraded mode, by using the available idle states per
> CPU, while also preventing the domain idle states.
>

Is that not better than not registering itself ? I tend to disagree here.

> Moving forward, this behaviour looks quite questionable to maintain, as
> complexity seems to grow around it, especially when trying to add support
> for deferred probe, for example.
>

I thought the sync_state in the driver must deal with that.

> Therefore, let's make the cpuidle driver registration to fail in this
> situation, thus relying on the default architectural cpuidle backend for
> WFI to be used.
>

CPU level states work w/o the need of OSI, and better than WFI. That was
the original aim. If that is not working, we must make it work instead
of falling back on WFI IMO.

--
Regards,
Sudeep
Sudeep Holla June 26, 2020, 2:47 p.m. UTC | #3
On Fri, Jun 26, 2020 at 03:33:55PM +0100, Sudeep Holla wrote:
> On Mon, Jun 15, 2020 at 05:20:50PM +0200, Ulf Hansson wrote:
> > Currently we allow the cpuidle driver registration to succeed, even if we
> > failed to enable the OSI mode when the hierarchical DT layout is used. This
> > means running in a degraded mode, by using the available idle states per
> > CPU, while also preventing the domain idle states.
> >
> 
> Is that not better than not registering itself ? I tend to disagree here.
> 
> > Moving forward, this behaviour looks quite questionable to maintain, as
> > complexity seems to grow around it, especially when trying to add support
> > for deferred probe, for example.
> >
> 
> I thought the sync_state in the driver must deal with that.
> 

Ignore this I got confused and assumed we have this in the code while I
read 5/5 first 

Patch
diff mbox series

diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index 423f03bbeb74..f07786aad673 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -26,7 +26,6 @@  struct psci_pd_provider {
 };
 
 static LIST_HEAD(psci_pd_providers);
-static bool osi_mode_enabled __initdata;
 
 static int psci_pd_power_off(struct generic_pm_domain *pd)
 {
@@ -272,7 +271,6 @@  static int __init psci_idle_init_domains(void)
 		goto remove_pd;
 	}
 
-	osi_mode_enabled = true;
 	of_node_put(np);
 	pr_info("Initialized CPU PM domain topology\n");
 	return pd_count;
@@ -293,9 +291,6 @@  struct device __init *psci_dt_attach_cpu(int cpu)
 {
 	struct device *dev;
 
-	if (!osi_mode_enabled)
-		return NULL;
-
 	dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
 	if (IS_ERR_OR_NULL(dev))
 		return dev;