[v9,03/11] PM / Domains: Document flags for genpd
diff mbox series

Message ID 20181003143824.13059-4-ulf.hansson@linaro.org
State Mainlined
Delegated to: Rafael Wysocki
Headers show
Series
  • PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) (a subset)
Related show

Commit Message

Ulf Hansson Oct. 3, 2018, 2:38 p.m. UTC
The current documented description of the GENPD_FLAG_* flags, are too
simplified, so let's extend them.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/linux/pm_domain.h | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

Comments

Tony Lindgren Oct. 4, 2018, 1:48 p.m. UTC | #1
Hi,

* Ulf Hansson <ulf.hansson@linaro.org> [181003 14:43]:
> + * GENPD_FLAG_IRQ_SAFE:		This informs genpd that its backend callbacks,
> + *				->power_on|off(), doesn't sleep. Hence, these
> + *				can be invoked from within atomic context, which
> + *				enables genpd to power on/off the PM domain,
> + *				even when pm_runtime_is_irq_safe() returns true,
> + *				for any of its attached devices. Note that, a
> + *				genpd having this flag set, requires its
> + *				masterdomains to also have it set.
> + *

Let's try to avoid adding more irq_safe stuff because of having that
propagate to the masterdomains..

I think you can just flag the power_on/off in genpd, then have cpu_pm
callbacks do it.

Regards,

Tony
Ulf Hansson Oct. 4, 2018, 2:57 p.m. UTC | #2
On 4 October 2018 at 15:48, Tony Lindgren <tony@atomide.com> wrote:
> Hi,
>
> * Ulf Hansson <ulf.hansson@linaro.org> [181003 14:43]:
>> + * GENPD_FLAG_IRQ_SAFE:              This informs genpd that its backend callbacks,
>> + *                           ->power_on|off(), doesn't sleep. Hence, these
>> + *                           can be invoked from within atomic context, which
>> + *                           enables genpd to power on/off the PM domain,
>> + *                           even when pm_runtime_is_irq_safe() returns true,
>> + *                           for any of its attached devices. Note that, a
>> + *                           genpd having this flag set, requires its
>> + *                           masterdomains to also have it set.
>> + *
>
> Let's try to avoid adding more irq_safe stuff because of having that
> propagate to the masterdomains..

I am not sure I get your point. This is just documenting existing
functionality in genpd, there is nothing new here.

>
> I think you can just flag the power_on/off in genpd, then have cpu_pm
> callbacks do it.

Not really sure what you propose, but feel free to send a patch.

Do note, genpd has since the beginning of its time tried to cope with
pm_runtime_irq_safe() devices. I admit it's quite complicated, however
GENPD_FLAG_IRQ_SAFE greatly improved the support for such devices and
their PM domains. Moreover, we need this functionality, in one way or
the other, as long as there users of pm_runtime_irq_safe().

Kind regards
Uffe
Tony Lindgren Oct. 4, 2018, 4:13 p.m. UTC | #3
* Ulf Hansson <ulf.hansson@linaro.org> [181004 15:02]:
> On 4 October 2018 at 15:48, Tony Lindgren <tony@atomide.com> wrote:
> > Hi,
> >
> > * Ulf Hansson <ulf.hansson@linaro.org> [181003 14:43]:
> >> + * GENPD_FLAG_IRQ_SAFE:              This informs genpd that its backend callbacks,
> >> + *                           ->power_on|off(), doesn't sleep. Hence, these
> >> + *                           can be invoked from within atomic context, which
> >> + *                           enables genpd to power on/off the PM domain,
> >> + *                           even when pm_runtime_is_irq_safe() returns true,
> >> + *                           for any of its attached devices. Note that, a
> >> + *                           genpd having this flag set, requires its
> >> + *                           masterdomains to also have it set.
> >> + *
> >
> > Let's try to avoid adding more irq_safe stuff because of having that
> > propagate to the masterdomains..
> 
> I am not sure I get your point. This is just documenting existing
> functionality in genpd, there is nothing new here.

Right, and I'm just bringing up that this FLAG_IRQ_SAFE is not a
good way to in the long run :)

> > I think you can just flag the power_on/off in genpd, then have cpu_pm
> > callbacks do it.
> 
> Not really sure what you propose, but feel free to send a patch.

Well there is not much to really patch, just don't attempt to
do irq_safe stuff from genpd and have cpu_idle callbacks to do
it instead. And then no need for GENPD_FLAG_IRQ_SAFE :)

> Do note, genpd has since the beginning of its time tried to cope with
> pm_runtime_irq_safe() devices. I admit it's quite complicated, however
> GENPD_FLAG_IRQ_SAFE greatly improved the support for such devices and
> their PM domains. Moreover, we need this functionality, in one way or
> the other, as long as there users of pm_runtime_irq_safe().

Right, and I'm still struggling years after with legacy device drivers
that have done pm_runtime_irq_safe() and come to the conclusion that
it should not be used at all. Getting rid of GENPD_FLAG_IRQ_SAFE
might just safe you years of pain later on.

Regards,

Tony

Patch
diff mbox series

diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 776c546d581a..3b5d7280e52e 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -17,11 +17,36 @@ 
 #include <linux/notifier.h>
 #include <linux/spinlock.h>
 
-/* Defines used for the flags field in the struct generic_pm_domain */
-#define GENPD_FLAG_PM_CLK	 (1U << 0) /* PM domain uses PM clk */
-#define GENPD_FLAG_IRQ_SAFE	 (1U << 1) /* PM domain operates in atomic */
-#define GENPD_FLAG_ALWAYS_ON	 (1U << 2) /* PM domain is always powered on */
-#define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3) /* Keep devices active if wakeup */
+/*
+ * Flags to control the behaviour of a genpd.
+ *
+ * These flags may be set in the struct generic_pm_domain's flags field by a
+ * genpd backend driver. The flags must be set before it calls pm_genpd_init(),
+ * which initializes a genpd.
+ *
+ * GENPD_FLAG_PM_CLK:		Instructs genpd to use the PM clk framework,
+ *				while powering on/off attached devices.
+ *
+ * GENPD_FLAG_IRQ_SAFE:		This informs genpd that its backend callbacks,
+ *				->power_on|off(), doesn't sleep. Hence, these
+ *				can be invoked from within atomic context, which
+ *				enables genpd to power on/off the PM domain,
+ *				even when pm_runtime_is_irq_safe() returns true,
+ *				for any of its attached devices. Note that, a
+ *				genpd having this flag set, requires its
+ *				masterdomains to also have it set.
+ *
+ * GENPD_FLAG_ALWAYS_ON:	Instructs genpd to always keep the PM domain
+ *				powered on.
+ *
+ * GENPD_FLAG_ACTIVE_WAKEUP:	Instructs genpd to keep the PM domain powered
+ *				on, in case any of its attached devices is used
+ *				in the wakeup path to serve system wakeups.
+ */
+#define GENPD_FLAG_PM_CLK	 (1U << 0)
+#define GENPD_FLAG_IRQ_SAFE	 (1U << 1)
+#define GENPD_FLAG_ALWAYS_ON	 (1U << 2)
+#define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
 
 enum gpd_status {
 	GPD_STATE_ACTIVE = 0,	/* PM domain is active */