[PATCH 0/8] PM / Domains: DT support for domain idle states & atomic PM domains

[PATCH 0/8] PM / Domains: DT support for domain idle states & atomic PM domains

[Lina Iyer]

Hi all,

This is the first set of patches of [1], sent now seperately. The CPU PM
domains part of the series is under discussion and was gating this set of
patches which have already been looked at by many and has no objections. Hence,
I split the series and sending out the PM domains changes now.

The patches [1 - 3] add DT support for reading domain idle states. The second
set of patches [4 - 8] enable PM domains to be used in atomic context.

The changes from [1] are -
- Allocating memory for domain idle states dynamically
- Conform to naming conventions for internal and exported genpd functions
- DT binding example for domain-idle-state
- Use fwnode instead of of_node
- Handle atomic case for removal of PM Domain
- Rebase on top of Rafael's pm/genpd tree

Thanks,
Lina

Lina Iyer (8):
  PM / Domains: Make genpd state allocation dynamic
  PM / Domain: Add residency property to genpd states
  PM / Domains: Allow domain power states to be read from DT
  PM / Domains: Add fwnode provider to genpd states
  dt/bindings: Update binding for PM domain idle states
  PM / Domains: Abstract genpd locking
  PM / Domains: Support IRQ safe PM domains
  PM / doc: Update device documentation for devices in IRQ safe PM
    domains

 .../devicetree/bindings/power/power_domain.txt     |  36 +++
 Documentation/power/devices.txt                    |  12 +-
 arch/arm/mach-imx/gpc.c                            |  17 +-
 drivers/base/power/domain.c                        | 338 +++++++++++++++++----
 include/linux/pm_domain.h                          |  27 +-
 5 files changed, 360 insertions(+), 70 deletions(-)

-- 
2.7.4

[1]. https://www.spinics.net/lists/arm-kernel/msg526814.html

[PATCH 1/8] PM / Domains: Make genpd state allocation dynamic

[Lina Iyer]

Allow PM Domain states to be defined dynamically by the drivers. This
removes the limitation on the maximum number of states possible for a
domain.

Cc: Axel Haslam <ahaslam+renesas@baylibre.com>
Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/mach-imx/gpc.c     | 17 ++++++++++-------
 drivers/base/power/domain.c | 10 ----------
 include/linux/pm_domain.h   |  4 +---
 3 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index 0df062d..b92dad5 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -380,13 +380,6 @@ static struct pu_domain imx6q_pu_domain = {
 		.name = "PU",
 		.power_off = imx6q_pm_pu_power_off,
 		.power_on = imx6q_pm_pu_power_on,
-		.states = {
-			[0] = {
-				.power_off_latency_ns = 25000,
-				.power_on_latency_ns = 2000000,
-			},
-		},
-		.state_count = 1,
 	},
 };
 
@@ -430,6 +423,16 @@ static int imx_gpc_genpd_init(struct device *dev, struct regulator *pu_reg)
 	if (!IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS))
 		return 0;
 
+	imx6q_pu_domain.base.states = devm_kzalloc(dev,
+					sizeof(*imx6q_pu_domain.base.states),
+					GFP_KERNEL);
+	if (!imx6q_pu_domain.base.states)
+		return -ENOMEM;
+
+	imx6q_pu_domain.base.states[0].power_off_latency_ns = 25000;
+	imx6q_pu_domain.base.states[0].power_on_latency_ns = 2000000;
+	mx6q_pu_domain.base.state_count = 1,
+
 	pm_genpd_init(&imx6q_pu_domain.base, NULL, false);
 	return of_genpd_add_provider_onecell(dev->of_node,
 					     &imx_gpc_onecell_data);
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e023066..740afa9 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1325,16 +1325,6 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 		genpd->dev_ops.start = pm_clk_resume;
 	}
 
-	if (genpd->state_idx >= GENPD_MAX_NUM_STATES) {
-		pr_warn("Initial state index out of bounds.\n");
-		genpd->state_idx = GENPD_MAX_NUM_STATES - 1;
-	}
-
-	if (genpd->state_count > GENPD_MAX_NUM_STATES) {
-		pr_warn("Limiting states to  %d\n", GENPD_MAX_NUM_STATES);
-		genpd->state_count = GENPD_MAX_NUM_STATES;
-	}
-
 	/* Use only one "off" state if there were no states declared */
 	if (genpd->state_count == 0)
 		genpd->state_count = 1;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index a09fe5c..bd1ffb9 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -19,8 +19,6 @@
 /* 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_MAX_NUM_STATES	8 /* Number of possible low power states */
-
 enum gpd_status {
 	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
 	GPD_STATE_POWER_OFF,	/* PM domain is off */
@@ -70,7 +68,7 @@ struct generic_pm_domain {
 	void (*detach_dev)(struct generic_pm_domain *domain,
 			   struct device *dev);
 	unsigned int flags;		/* Bit field of configs for genpd */
-	struct genpd_power_state states[GENPD_MAX_NUM_STATES];
+	struct genpd_power_state *states;
 	unsigned int state_count; /* number of states */
 	unsigned int state_idx; /* state that genpd will go to when off */
 
-- 
2.7.4

[PATCH 1/8] PM / Domains: Make genpd state allocation dynamic

[Ulf Hansson]

On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
> Allow PM Domain states to be defined dynamically by the drivers. This
> removes the limitation on the maximum number of states possible for a
> domain.
>
> Cc: Axel Haslam <ahaslam+renesas@baylibre.com>
> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  arch/arm/mach-imx/gpc.c     | 17 ++++++++++-------
>  drivers/base/power/domain.c | 10 ----------
>  include/linux/pm_domain.h   |  4 +---
>  3 files changed, 11 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index 0df062d..b92dad5 100644
> --- a/arch/arm/mach-imx/gpc.c
> +++ b/arch/arm/mach-imx/gpc.c
> @@ -380,13 +380,6 @@ static struct pu_domain imx6q_pu_domain = {
>                 .name = "PU",
>                 .power_off = imx6q_pm_pu_power_off,
>                 .power_on = imx6q_pm_pu_power_on,
> -               .states = {
> -                       [0] = {
> -                               .power_off_latency_ns = 25000,
> -                               .power_on_latency_ns = 2000000,
> -                       },
> -               },
> -               .state_count = 1,
>         },
>  };
>
> @@ -430,6 +423,16 @@ static int imx_gpc_genpd_init(struct device *dev, struct regulator *pu_reg)
>         if (!IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS))
>                 return 0;
>
> +       imx6q_pu_domain.base.states = devm_kzalloc(dev,
> +                                       sizeof(*imx6q_pu_domain.base.states),
> +                                       GFP_KERNEL);
> +       if (!imx6q_pu_domain.base.states)
> +               return -ENOMEM;
> +
> +       imx6q_pu_domain.base.states[0].power_off_latency_ns = 25000;
> +       imx6q_pu_domain.base.states[0].power_on_latency_ns = 2000000;
> +       mx6q_pu_domain.base.state_count = 1,
> +
>         pm_genpd_init(&imx6q_pu_domain.base, NULL, false);
>         return of_genpd_add_provider_onecell(dev->of_node,
>                                              &imx_gpc_onecell_data);
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e023066..740afa9 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1325,16 +1325,6 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>                 genpd->dev_ops.start = pm_clk_resume;
>         }
>
> -       if (genpd->state_idx >= GENPD_MAX_NUM_STATES) {
> -               pr_warn("Initial state index out of bounds.\n");
> -               genpd->state_idx = GENPD_MAX_NUM_STATES - 1;
> -       }
> -
> -       if (genpd->state_count > GENPD_MAX_NUM_STATES) {
> -               pr_warn("Limiting states to  %d\n", GENPD_MAX_NUM_STATES);
> -               genpd->state_count = GENPD_MAX_NUM_STATES;
> -       }
> -
>         /* Use only one "off" state if there were no states declared */
>         if (genpd->state_count == 0)
>                 genpd->state_count = 1;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index a09fe5c..bd1ffb9 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -19,8 +19,6 @@
>  /* 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_MAX_NUM_STATES   8 /* Number of possible low power states */
> -
>  enum gpd_status {
>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
>         GPD_STATE_POWER_OFF,    /* PM domain is off */
> @@ -70,7 +68,7 @@ struct generic_pm_domain {
>         void (*detach_dev)(struct generic_pm_domain *domain,
>                            struct device *dev);
>         unsigned int flags;             /* Bit field of configs for genpd */
> -       struct genpd_power_state states[GENPD_MAX_NUM_STATES];
> +       struct genpd_power_state *states;
>         unsigned int state_count; /* number of states */
>         unsigned int state_idx; /* state that genpd will go to when off */
>
> --
> 2.7.4
>

In general I like the improvement, but..

This change implies that ->states may very well be NULL. This isn't
validated by genpd's internal logic when power off/on the domain
(genpd_power_on|off(), __default_power_down_ok()). You need to fix
this, somehow.

Perhaps the easiest solutions is, when pm_genpd_init() finds that
->state is NULL, that we allocate a struct genpd_power_state with
array size of 1 and assign it to ->states. Although, doing this also
means you need to track that genpd was responsible for the the
allocation, so it must also free the data from within genpd_remove().

Unless you have other ideas!?

Kind regards
Uffe

[PATCH 1/8] PM / Domains: Make genpd state allocation dynamic

[Lina Iyer]

On Thu, Oct 06 2016 at 02:37 -0600, Ulf Hansson wrote:
>On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Allow PM Domain states to be defined dynamically by the drivers. This
>> removes the limitation on the maximum number of states possible for a
>> domain.
>>
>> Cc: Axel Haslam <ahaslam+renesas@baylibre.com>
>> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
<...>
>> -#define GENPD_MAX_NUM_STATES   8 /* Number of possible low power states */
>> -
>>  enum gpd_status {
>>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
>>         GPD_STATE_POWER_OFF,    /* PM domain is off */
>> @@ -70,7 +68,7 @@ struct generic_pm_domain {
>>         void (*detach_dev)(struct generic_pm_domain *domain,
>>                            struct device *dev);
>>         unsigned int flags;             /* Bit field of configs for genpd */
>> -       struct genpd_power_state states[GENPD_MAX_NUM_STATES];
>> +       struct genpd_power_state *states;
>>         unsigned int state_count; /* number of states */
>>         unsigned int state_idx; /* state that genpd will go to when off */
>>
>> --
>> 2.7.4
>>
>
>In general I like the improvement, but..
>
>This change implies that ->states may very well be NULL. This isn't
>validated by genpd's internal logic when power off/on the domain
>(genpd_power_on|off(), __default_power_down_ok()). You need to fix
>this, somehow.
>
Good point.

>Perhaps the easiest solutions is, when pm_genpd_init() finds that
>->state is NULL, that we allocate a struct genpd_power_state with
>array size of 1 and assign it to ->states. Although, doing this also
>means you need to track that genpd was responsible for the the
>allocation, so it must also free the data from within genpd_remove().
>
>Unless you have other ideas!?
>
I can think of some hacks, but they are uglier than the problem we are
trying to solve. We could drop this patch. Real world situations would
not have more than 8 states and if there is one, we can think about it
then.

Thanks,
Lina

[PATCH 1/8] PM / Domains: Make genpd state allocation dynamic

[Ulf Hansson]

On 6 October 2016 at 17:40, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Thu, Oct 06 2016 at 02:37 -0600, Ulf Hansson wrote:
>>
>> On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>
>>> Allow PM Domain states to be defined dynamically by the drivers. This
>>> removes the limitation on the maximum number of states possible for a
>>> domain.
>>>
>>> Cc: Axel Haslam <ahaslam+renesas@baylibre.com>
>>> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>
> <...>
>>>
>>> -#define GENPD_MAX_NUM_STATES   8 /* Number of possible low power states
>>> */
>>> -
>>>  enum gpd_status {
>>>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
>>>         GPD_STATE_POWER_OFF,    /* PM domain is off */
>>> @@ -70,7 +68,7 @@ struct generic_pm_domain {
>>>         void (*detach_dev)(struct generic_pm_domain *domain,
>>>                            struct device *dev);
>>>         unsigned int flags;             /* Bit field of configs for genpd
>>> */
>>> -       struct genpd_power_state states[GENPD_MAX_NUM_STATES];
>>> +       struct genpd_power_state *states;
>>>         unsigned int state_count; /* number of states */
>>>         unsigned int state_idx; /* state that genpd will go to when off
>>> */
>>>
>>> --
>>> 2.7.4
>>>
>>
>> In general I like the improvement, but..
>>
>> This change implies that ->states may very well be NULL. This isn't
>> validated by genpd's internal logic when power off/on the domain
>> (genpd_power_on|off(), __default_power_down_ok()). You need to fix
>> this, somehow.
>>
> Good point.
>
>> Perhaps the easiest solutions is, when pm_genpd_init() finds that
>> ->state is NULL, that we allocate a struct genpd_power_state with
>> array size of 1 and assign it to ->states. Although, doing this also
>> means you need to track that genpd was responsible for the the
>> allocation, so it must also free the data from within genpd_remove().
>>
>> Unless you have other ideas!?
>>
> I can think of some hacks, but they are uglier than the problem we are
> trying to solve. We could drop this patch. Real world situations would
> not have more than 8 states and if there is one, we can think about it
> then.

The problem with the current approach is that we waste some memory as
we always have an array of 8 states per genpd. In the worst case,
which currently is the most common case, only 1 out of 8 states is
being used.

So, let's not be lazy here and instead take the opportunity to fix
this, and especially I think this makes sense, before we go on and add
the DT parsing of the domain-idle-states.

The more sophisticated method would probably be to use kobject/kref,
but let's not go there for now. Instead let's try an easy method of
just tracking whether the allocations had been made internally by
genpd, via adding a "bool state_allocated to the struct
generic_pm_domain. Would that work?

Kind regards
Uffe

[PATCH 1/8] PM / Domains: Make genpd state allocation dynamic

[Lina Iyer]

On Thu, Oct 06 2016 at 13:45 -0600, Ulf Hansson wrote:
>On 6 October 2016 at 17:40, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Thu, Oct 06 2016 at 02:37 -0600, Ulf Hansson wrote:
>>>
>>> On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>>
>>>> Allow PM Domain states to be defined dynamically by the drivers. This
>>>> removes the limitation on the maximum number of states possible for a
>>>> domain.
>>>>
>>>> Cc: Axel Haslam <ahaslam+renesas@baylibre.com>
>>>> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>
>> <...>
>>>>
>>>> -#define GENPD_MAX_NUM_STATES   8 /* Number of possible low power states
>>>> */
>>>> -
>>>>  enum gpd_status {
>>>>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
>>>>         GPD_STATE_POWER_OFF,    /* PM domain is off */
>>>> @@ -70,7 +68,7 @@ struct generic_pm_domain {
>>>>         void (*detach_dev)(struct generic_pm_domain *domain,
>>>>                            struct device *dev);
>>>>         unsigned int flags;             /* Bit field of configs for genpd
>>>> */
>>>> -       struct genpd_power_state states[GENPD_MAX_NUM_STATES];
>>>> +       struct genpd_power_state *states;
>>>>         unsigned int state_count; /* number of states */
>>>>         unsigned int state_idx; /* state that genpd will go to when off
>>>> */
>>>>
>>>> --
>>>> 2.7.4
>>>>
>>>
>>> In general I like the improvement, but..
>>>
>>> This change implies that ->states may very well be NULL. This isn't
>>> validated by genpd's internal logic when power off/on the domain
>>> (genpd_power_on|off(), __default_power_down_ok()). You need to fix
>>> this, somehow.
>>>
>> Good point.
>>
>>> Perhaps the easiest solutions is, when pm_genpd_init() finds that
>>> ->state is NULL, that we allocate a struct genpd_power_state with
>>> array size of 1 and assign it to ->states. Although, doing this also
>>> means you need to track that genpd was responsible for the the
>>> allocation, so it must also free the data from within genpd_remove().
>>>
>>> Unless you have other ideas!?
>>>
>> I can think of some hacks, but they are uglier than the problem we are
>> trying to solve. We could drop this patch. Real world situations would
>> not have more than 8 states and if there is one, we can think about it
>> then.
>
>The problem with the current approach is that we waste some memory as
>we always have an array of 8 states per genpd. In the worst case,
>which currently is the most common case, only 1 out of 8 states is
>being used.
>
>So, let's not be lazy here and instead take the opportunity to fix
>this, and especially I think this makes sense, before we go on and add
>the DT parsing of the domain-idle-states.
>
Hmm.. We are not wasting much memory in comparison, but if you insist,
sure.

>The more sophisticated method would probably be to use kobject/kref,
>but let's not go there for now. Instead let's try an easy method of
>just tracking whether the allocations had been made internally by
>genpd, via adding a "bool state_allocated to the struct
>generic_pm_domain. Would that work?
>
It would work.

i.
How about an additional static state by default in the domain structure,
if the platform does not provide a state then the default structure is
used. That way we dont have to track it. But it does waste memory
eqivalent to a state, when there are states provided by the platform.

ii.
I could add a void *free to the domain structure and save the memory
allocated by default in the *free. At domain remove, we just do a kfree
on free.

iii.
Use a boolean flag.

Thanks,
Lina

[PATCH 1/8] PM / Domains: Make genpd state allocation dynamic

[Ulf Hansson]

On 6 October 2016 at 22:57, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Thu, Oct 06 2016 at 13:45 -0600, Ulf Hansson wrote:
>>
>> On 6 October 2016 at 17:40, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>
>>> On Thu, Oct 06 2016 at 02:37 -0600, Ulf Hansson wrote:
>>>>
>>>>
>>>> On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>>>
>>>>>
>>>>> Allow PM Domain states to be defined dynamically by the drivers. This
>>>>> removes the limitation on the maximum number of states possible for a
>>>>> domain.
>>>>>
>>>>> Cc: Axel Haslam <ahaslam+renesas@baylibre.com>
>>>>> Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>
>>>
>>> <...>
>>>>>
>>>>>
>>>>> -#define GENPD_MAX_NUM_STATES   8 /* Number of possible low power
>>>>> states
>>>>> */
>>>>> -
>>>>>  enum gpd_status {
>>>>>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
>>>>>         GPD_STATE_POWER_OFF,    /* PM domain is off */
>>>>> @@ -70,7 +68,7 @@ struct generic_pm_domain {
>>>>>         void (*detach_dev)(struct generic_pm_domain *domain,
>>>>>                            struct device *dev);
>>>>>         unsigned int flags;             /* Bit field of configs for
>>>>> genpd
>>>>> */
>>>>> -       struct genpd_power_state states[GENPD_MAX_NUM_STATES];
>>>>> +       struct genpd_power_state *states;
>>>>>         unsigned int state_count; /* number of states */
>>>>>         unsigned int state_idx; /* state that genpd will go to when off
>>>>> */
>>>>>
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>
>>>> In general I like the improvement, but..
>>>>
>>>> This change implies that ->states may very well be NULL. This isn't
>>>> validated by genpd's internal logic when power off/on the domain
>>>> (genpd_power_on|off(), __default_power_down_ok()). You need to fix
>>>> this, somehow.
>>>>
>>> Good point.
>>>
>>>> Perhaps the easiest solutions is, when pm_genpd_init() finds that
>>>> ->state is NULL, that we allocate a struct genpd_power_state with
>>>> array size of 1 and assign it to ->states. Although, doing this also
>>>> means you need to track that genpd was responsible for the the
>>>> allocation, so it must also free the data from within genpd_remove().
>>>>
>>>> Unless you have other ideas!?
>>>>
>>> I can think of some hacks, but they are uglier than the problem we are
>>> trying to solve. We could drop this patch. Real world situations would
>>> not have more than 8 states and if there is one, we can think about it
>>> then.
>>
>>
>> The problem with the current approach is that we waste some memory as
>> we always have an array of 8 states per genpd. In the worst case,
>> which currently is the most common case, only 1 out of 8 states is
>> being used.
>>
>> So, let's not be lazy here and instead take the opportunity to fix
>> this, and especially I think this makes sense, before we go on and add
>> the DT parsing of the domain-idle-states.
>>
> Hmm.. We are not wasting much memory in comparison, but if you insist,
> sure.
>
>> The more sophisticated method would probably be to use kobject/kref,
>> but let's not go there for now. Instead let's try an easy method of
>> just tracking whether the allocations had been made internally by
>> genpd, via adding a "bool state_allocated to the struct
>> generic_pm_domain. Would that work?
>>
> It would work.
>
> i.
> How about an additional static state by default in the domain structure,
> if the platform does not provide a state then the default structure is
> used. That way we dont have to track it. But it does waste memory
> eqivalent to a state, when there are states provided by the platform.

I thought about this, but I think it could be a bit messy as well.
Better to do an allocation when needed.

>
> ii.
> I could add a void *free to the domain structure and save the memory
> allocated by default in the *free. At domain remove, we just do a kfree
> on free.
>
> iii.
> Use a boolean flag.

Both ii) and iii) works for me!

Kind regards
Uffe

>
> Thanks,
> Lina
>

[PATCH 2/8] PM / Domain: Add residency property to genpd states

[Lina Iyer]

Residency of a domain's idle state indicates that the minimum idle time
for the domain's idle state to be beneficial for power. Add the
parameter to the state node. Future patches, will use the residency
value in the genpd governor to determine if it is worth while to enter
an idle state.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 include/linux/pm_domain.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index bd1ffb9..c113713 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -38,6 +38,7 @@ struct gpd_dev_ops {
 struct genpd_power_state {
 	s64 power_off_latency_ns;
 	s64 power_on_latency_ns;
+	s64 residency_ns;
 };
 
 struct generic_pm_domain {
-- 
2.7.4

[PATCH 2/8] PM / Domain: Add residency property to genpd states

[Ulf Hansson]

On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
> Residency of a domain's idle state indicates that the minimum idle time
> for the domain's idle state to be beneficial for power. Add the
> parameter to the state node. Future patches, will use the residency
> value in the genpd governor to determine if it is worth while to enter
> an idle state.
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>

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

Kind regards
Uffe

> ---
>  include/linux/pm_domain.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index bd1ffb9..c113713 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -38,6 +38,7 @@ struct gpd_dev_ops {
>  struct genpd_power_state {
>         s64 power_off_latency_ns;
>         s64 power_on_latency_ns;
> +       s64 residency_ns;
>  };
>
>  struct generic_pm_domain {
> --
> 2.7.4
>

[PATCH 3/8] PM / Domains: Allow domain power states to be read from DT

[Lina Iyer]

This patch allows domains to define idle states in the DT. SoC's can
define domain idle states in DT using the "domain-idle-states" property
of the domain provider. Calling of_pm_genpd_init() will  read the idle
states and initialize the genpd for the domain.

In addition to the entry and exit latency for idle state, also add
residency_ns, param and of_node property to each state. A domain idling
in a state is only power effecient if it stays idle for a certain period
in that state. The residency provides this minimum time for the idle
state to provide power benefits. The param is a state specific u32 value
that the platform may use for that idle state.

This patch is based on the original patch by Marc Titinger.

Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |   8 ++++
 2 files changed, 111 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 740afa9..368a5b8 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1895,6 +1895,109 @@ out:
 	return ret ? -EPROBE_DEFER : 0;
 }
 EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
+
+static const struct of_device_id idle_state_match[] = {
+	{ .compatible = "arm,idle-state", },
+	{ }
+};
+
+static int read_genpd_state(struct genpd_power_state *genpd_state,
+				    struct device_node *state_node)
+{
+	int err = 0;
+	u32 latency;
+	u32 residency;
+	u32 entry_latency, exit_latency;
+	const struct of_device_id *match_id;
+
+	match_id = of_match_node(idle_state_match, state_node);
+	if (!match_id)
+		return -EINVAL;
+
+	err = of_property_read_u32(state_node, "entry-latency-us",
+						&entry_latency);
+	if (err) {
+		pr_debug(" * %s missing entry-latency-us property\n",
+						state_node->full_name);
+		return -EINVAL;
+	}
+
+	err = of_property_read_u32(state_node, "exit-latency-us",
+						&exit_latency);
+	if (err) {
+		pr_debug(" * %s missing exit-latency-us property\n",
+						state_node->full_name);
+		return -EINVAL;
+	}
+
+	err = of_property_read_u32(state_node, "min-residency-us", &residency);
+	if (!err)
+		genpd_state->residency_ns = 1000 * residency;
+
+	latency = entry_latency + exit_latency;
+	genpd_state->power_on_latency_ns = 1000 * latency;
+	genpd_state->power_off_latency_ns = 1000 * entry_latency;
+
+	return 0;
+}
+
+/**
+ * of_genpd_parse_idle_states: Return array of idle states for the genpd.
+ *
+ * @dn: The genpd device node
+ * @states: The pointer to which the state array will be saved.
+ * @n: The count of elements in the array returned from this function.
+ *
+ * Returns the device states parsed from the OF node. The memory for the states
+ * is allocated by this function and is the responsibility of the caller to
+ * free the memory after use.
+ */
+int of_genpd_parse_idle_states(struct device_node *dn,
+			struct genpd_power_state **states, int *n)
+{
+	struct genpd_power_state *st;
+	struct device_node *np;
+	int i, ret = 0;
+	int count;
+
+	for (count = 0; ; count++)
+		if (!of_parse_phandle(dn, "domain-idle-states", count))
+			break;
+
+	st = kcalloc(count, sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		np = of_parse_phandle(dn, "domain-idle-states", i);
+		if (!np) {
+			ret = -EFAULT;
+			break;
+		}
+
+		ret = read_genpd_state(&st[i], np);
+		if (ret) {
+			pr_err
+			("Parsing idle state node %s failed with err %d\n",
+							np->full_name, ret);
+			of_node_put(np);
+			break;
+		}
+		of_node_put(np);
+	}
+
+	if (ret) {
+		kfree(st);
+		return ret;
+	}
+
+	*n = count;
+	*states = st;
+
+	return 0;
+}
+EXPORT_SYMBOL(of_genpd_parse_idle_states);
+
 #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
 
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index c113713..4c9152d 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -204,6 +204,8 @@ extern int of_genpd_add_device(struct of_phandle_args *args,
 extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
 				  struct of_phandle_args *new_subdomain);
 extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
+extern int of_genpd_parse_idle_states(struct device_node *dn,
+			struct genpd_power_state **states, int *n);
 
 int genpd_dev_pm_attach(struct device *dev);
 #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
@@ -233,6 +235,12 @@ static inline int of_genpd_add_subdomain(struct of_phandle_args *parent,
 	return -ENODEV;
 }
 
+static inline int of_genpd_parse_idle_states(struct device_node *dn,
+			struct genpd_power_state **states, int *n)
+{
+	return -ENODEV;
+}
+
 static inline int genpd_dev_pm_attach(struct device *dev)
 {
 	return -ENODEV;
-- 
2.7.4

[PATCH 3/8] PM / Domains: Allow domain power states to be read from DT

[Geert Uytterhoeven]

Hi Lina,

On Wed, Oct 5, 2016 at 10:31 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> This patch allows domains to define idle states in the DT. SoC's can
> define domain idle states in DT using the "domain-idle-states" property
> of the domain provider. Calling of_pm_genpd_init() will  read the idle
> states and initialize the genpd for the domain.
>
> In addition to the entry and exit latency for idle state, also add
> residency_ns, param and of_node property to each state. A domain idling
> in a state is only power effecient if it stays idle for a certain period
> in that state. The residency provides this minimum time for the idle
> state to provide power benefits. The param is a state specific u32 value
> that the platform may use for that idle state.
>
> This patch is based on the original patch by Marc Titinger.
>
> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |   8 ++++
>  2 files changed, 111 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 740afa9..368a5b8 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1895,6 +1895,109 @@ out:
>         return ret ? -EPROBE_DEFER : 0;
>  }
>  EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
> +
> +static const struct of_device_id idle_state_match[] = {
> +       { .compatible = "arm,idle-state", },

Do we want ARM-specific compatible values without an #ifdef in drivers/base/?

I know we already have "samsung,power-domain".
Perhaps that should be protected by #ifdef, too.

> +       { }
> +};

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 3/8] PM / Domains: Allow domain power states to be read from DT

[Lina Iyer]

On Thu, Oct 06 2016 at 02:04 -0600, Geert Uytterhoeven wrote:
>Hi Lina,
>
>On Wed, Oct 5, 2016 at 10:31 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> This patch allows domains to define idle states in the DT. SoC's can
>> define domain idle states in DT using the "domain-idle-states" property
>> of the domain provider. Calling of_pm_genpd_init() will  read the idle
>> states and initialize the genpd for the domain.
>>
>> In addition to the entry and exit latency for idle state, also add
>> residency_ns, param and of_node property to each state. A domain idling
>> in a state is only power effecient if it stays idle for a certain period
>> in that state. The residency provides this minimum time for the idle
>> state to provide power benefits. The param is a state specific u32 value
>> that the platform may use for that idle state.
>>
>> This patch is based on the original patch by Marc Titinger.
>>
>> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |   8 ++++
>>  2 files changed, 111 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 740afa9..368a5b8 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1895,6 +1895,109 @@ out:
>>         return ret ? -EPROBE_DEFER : 0;
>>  }
>>  EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>> +
>> +static const struct of_device_id idle_state_match[] = {
>> +       { .compatible = "arm,idle-state", },
>
>Do we want ARM-specific compatible values without an #ifdef in drivers/base/?
>
>I know we already have "samsung,power-domain".
>Perhaps that should be protected by #ifdef, too.
>
The arm,idle-state DT binding is re-used here to describe domain idle
states, because that is exactly what we need here. The binding is not
dependent on the ARM architecture, so we won't need a #ifdef around
this. I do agree that using this compatible is not very intuitive.

Thanks,
Lina

[PATCH 3/8] PM / Domains: Allow domain power states to be read from DT

[Ulf Hansson]

On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
> This patch allows domains to define idle states in the DT. SoC's can
> define domain idle states in DT using the "domain-idle-states" property
> of the domain provider. Calling of_pm_genpd_init() will  read the idle
> states and initialize the genpd for the domain.
>
> In addition to the entry and exit latency for idle state, also add
> residency_ns, param and of_node property to each state. A domain idling
> in a state is only power effecient if it stays idle for a certain period
> in that state. The residency provides this minimum time for the idle
> state to provide power benefits. The param is a state specific u32 value
> that the platform may use for that idle state.
>
> This patch is based on the original patch by Marc Titinger.
>
> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |   8 ++++
>  2 files changed, 111 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 740afa9..368a5b8 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1895,6 +1895,109 @@ out:
>         return ret ? -EPROBE_DEFER : 0;
>  }
>  EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
> +
> +static const struct of_device_id idle_state_match[] = {
> +       { .compatible = "arm,idle-state", },
> +       { }
> +};
> +
> +static int read_genpd_state(struct genpd_power_state *genpd_state,

/s/read_genpd_state/genpd_parse_state

> +                                   struct device_node *state_node)
> +{
> +       int err = 0;

No need to assign err to 0.

> +       u32 latency;
> +       u32 residency;
> +       u32 entry_latency, exit_latency;
> +       const struct of_device_id *match_id;
> +
> +       match_id = of_match_node(idle_state_match, state_node);
> +       if (!match_id)
> +               return -EINVAL;
> +
> +       err = of_property_read_u32(state_node, "entry-latency-us",
> +                                               &entry_latency);
> +       if (err) {
> +               pr_debug(" * %s missing entry-latency-us property\n",
> +                                               state_node->full_name);
> +               return -EINVAL;
> +       }
> +
> +       err = of_property_read_u32(state_node, "exit-latency-us",
> +                                               &exit_latency);
> +       if (err) {
> +               pr_debug(" * %s missing exit-latency-us property\n",
> +                                               state_node->full_name);
> +               return -EINVAL;
> +       }
> +
> +       err = of_property_read_u32(state_node, "min-residency-us", &residency);
> +       if (!err)
> +               genpd_state->residency_ns = 1000 * residency;
> +
> +       latency = entry_latency + exit_latency;

Hmm, this is probably not what you want.

The genpd governor, via __default_power_down_ok(), already adds the
->power_on_latency_ns and the ->power_off_latency_ns, when it
validates which idle state you are allowed to enter.

> +       genpd_state->power_on_latency_ns = 1000 * latency;
> +       genpd_state->power_off_latency_ns = 1000 * entry_latency;
> +
> +       return 0;
> +}
> +
> +/**
> + * of_genpd_parse_idle_states: Return array of idle states for the genpd.
> + *
> + * @dn: The genpd device node
> + * @states: The pointer to which the state array will be saved.
> + * @n: The count of elements in the array returned from this function.
> + *
> + * Returns the device states parsed from the OF node. The memory for the states
> + * is allocated by this function and is the responsibility of the caller to
> + * free the memory after use.
> + */
> +int of_genpd_parse_idle_states(struct device_node *dn,
> +                       struct genpd_power_state **states, int *n)
> +{
> +       struct genpd_power_state *st;
> +       struct device_node *np;
> +       int i, ret = 0;
> +       int count;
> +
> +       for (count = 0; ; count++)
> +               if (!of_parse_phandle(dn, "domain-idle-states", count))
> +                       break;

I think it's better to use of_count_phandle_with_args() to find out
the number of phandles.

> +
> +       st = kcalloc(count, sizeof(*st), GFP_KERNEL);
> +       if (!st)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < count; i++) {
> +               np = of_parse_phandle(dn, "domain-idle-states", i);

Isn't this a case of when it would be convenient to use the
of_phandle_iterator*() APIs?

> +               if (!np) {
> +                       ret = -EFAULT;
> +                       break;
> +               }
> +
> +               ret = read_genpd_state(&st[i], np);
> +               if (ret) {
> +                       pr_err
> +                       ("Parsing idle state node %s failed with err %d\n",
> +                                                       np->full_name, ret);
> +                       of_node_put(np);
> +                       break;
> +               }
> +               of_node_put(np);
> +       }
> +
> +       if (ret) {
> +               kfree(st);
> +               return ret;
> +       }
> +
> +       *n = count;
> +       *states = st;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(of_genpd_parse_idle_states);
> +
>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index c113713..4c9152d 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -204,6 +204,8 @@ extern int of_genpd_add_device(struct of_phandle_args *args,
>  extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
>                                   struct of_phandle_args *new_subdomain);
>  extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
> +extern int of_genpd_parse_idle_states(struct device_node *dn,
> +                       struct genpd_power_state **states, int *n);
>
>  int genpd_dev_pm_attach(struct device *dev);
>  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
> @@ -233,6 +235,12 @@ static inline int of_genpd_add_subdomain(struct of_phandle_args *parent,
>         return -ENODEV;
>  }
>
> +static inline int of_genpd_parse_idle_states(struct device_node *dn,
> +                       struct genpd_power_state **states, int *n)
> +{
> +       return -ENODEV;
> +}
> +
>  static inline int genpd_dev_pm_attach(struct device *dev)
>  {
>         return -ENODEV;
> --
> 2.7.4
>

Kind regards
Uffe

[PATCH 3/8] PM / Domains: Allow domain power states to be read from DT

[Lina Iyer]

On Thu, Oct 06 2016 at 03:47 -0600, Ulf Hansson wrote:
>On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>> This patch allows domains to define idle states in the DT. SoC's can
>> define domain idle states in DT using the "domain-idle-states" property
>> of the domain provider. Calling of_pm_genpd_init() will  read the idle
>> states and initialize the genpd for the domain.
>>
>> In addition to the entry and exit latency for idle state, also add
>> residency_ns, param and of_node property to each state. A domain idling
>> in a state is only power effecient if it stays idle for a certain period
>> in that state. The residency provides this minimum time for the idle
>> state to provide power benefits. The param is a state specific u32 value
>> that the platform may use for that idle state.
>>
>> This patch is based on the original patch by Marc Titinger.
>>
>> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |   8 ++++
>>  2 files changed, 111 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 740afa9..368a5b8 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1895,6 +1895,109 @@ out:
>>         return ret ? -EPROBE_DEFER : 0;
>>  }
>>  EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>> +
>> +static const struct of_device_id idle_state_match[] = {
>> +       { .compatible = "arm,idle-state", },
>> +       { }
>> +};
>> +
>> +static int read_genpd_state(struct genpd_power_state *genpd_state,
>
>/s/read_genpd_state/genpd_parse_state
>
>> +                                   struct device_node *state_node)
>> +{
>> +       int err = 0;
>
>No need to assign err to 0.
>
>> +       u32 latency;
>> +       u32 residency;
>> +       u32 entry_latency, exit_latency;
>> +       const struct of_device_id *match_id;
>> +
>> +       match_id = of_match_node(idle_state_match, state_node);
>> +       if (!match_id)
>> +               return -EINVAL;
>> +
>> +       err = of_property_read_u32(state_node, "entry-latency-us",
>> +                                               &entry_latency);
>> +       if (err) {
>> +               pr_debug(" * %s missing entry-latency-us property\n",
>> +                                               state_node->full_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       err = of_property_read_u32(state_node, "exit-latency-us",
>> +                                               &exit_latency);
>> +       if (err) {
>> +               pr_debug(" * %s missing exit-latency-us property\n",
>> +                                               state_node->full_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       err = of_property_read_u32(state_node, "min-residency-us", &residency);
>> +       if (!err)
>> +               genpd_state->residency_ns = 1000 * residency;
>> +
>> +       latency = entry_latency + exit_latency;
>
>Hmm, this is probably not what you want.
>
>The genpd governor, via __default_power_down_ok(), already adds the
>->power_on_latency_ns and the ->power_off_latency_ns, when it
>validates which idle state you are allowed to enter.
>
>> +       genpd_state->power_on_latency_ns = 1000 * latency;
>> +       genpd_state->power_off_latency_ns = 1000 * entry_latency;
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * of_genpd_parse_idle_states: Return array of idle states for the genpd.
>> + *
>> + * @dn: The genpd device node
>> + * @states: The pointer to which the state array will be saved.
>> + * @n: The count of elements in the array returned from this function.
>> + *
>> + * Returns the device states parsed from the OF node. The memory for the states
>> + * is allocated by this function and is the responsibility of the caller to
>> + * free the memory after use.
>> + */
>> +int of_genpd_parse_idle_states(struct device_node *dn,
>> +                       struct genpd_power_state **states, int *n)
>> +{
>> +       struct genpd_power_state *st;
>> +       struct device_node *np;
>> +       int i, ret = 0;
>> +       int count;
>> +
>> +       for (count = 0; ; count++)
>> +               if (!of_parse_phandle(dn, "domain-idle-states", count))
>> +                       break;
>
>I think it's better to use of_count_phandle_with_args() to find out
>the number of phandles.
>

>> +
>> +       st = kcalloc(count, sizeof(*st), GFP_KERNEL);
>> +       if (!st)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < count; i++) {
>> +               np = of_parse_phandle(dn, "domain-idle-states", i);
>
>Isn't this a case of when it would be convenient to use the
>of_phandle_iterator*() APIs?
>

Hmm.. But if we move back to static allocation of states, we won't need
any of this ;)

-- Lina
>> +               if (!np) {
>> +                       ret = -EFAULT;
>> +                       break;
>> +               }
>> +
>> +               ret = read_genpd_state(&st[i], np);
>> +               if (ret) {
>> +                       pr_err
>> +                       ("Parsing idle state node %s failed with err %d\n",
>> +                                                       np->full_name, ret);
>> +                       of_node_put(np);
>> +                       break;
>> +               }
>> +               of_node_put(np);
>> +       }
>> +
>> +       if (ret) {
>> +               kfree(st);
>> +               return ret;
>> +       }
>> +
>> +       *n = count;
>> +       *states = st;
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(of_genpd_parse_idle_states);
>> +
>>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>>
>>
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index c113713..4c9152d 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -204,6 +204,8 @@ extern int of_genpd_add_device(struct of_phandle_args *args,
>>  extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
>>                                   struct of_phandle_args *new_subdomain);
>>  extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
>> +extern int of_genpd_parse_idle_states(struct device_node *dn,
>> +                       struct genpd_power_state **states, int *n);
>>
>>  int genpd_dev_pm_attach(struct device *dev);
>>  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
>> @@ -233,6 +235,12 @@ static inline int of_genpd_add_subdomain(struct of_phandle_args *parent,
>>         return -ENODEV;
>>  }
>>
>> +static inline int of_genpd_parse_idle_states(struct device_node *dn,
>> +                       struct genpd_power_state **states, int *n)
>> +{
>> +       return -ENODEV;
>> +}
>> +
>>  static inline int genpd_dev_pm_attach(struct device *dev)
>>  {
>>         return -ENODEV;
>> --
>> 2.7.4
>>
>
>Kind regards
>Uffe

[PATCH 3/8] PM / Domains: Allow domain power states to be read from DT

[Lina Iyer]

On Thu, Oct 06 2016 at 03:47 -0600, Ulf Hansson wrote:
>On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>> This patch allows domains to define idle states in the DT. SoC's can
>> define domain idle states in DT using the "domain-idle-states" property
>> of the domain provider. Calling of_pm_genpd_init() will  read the idle
>> states and initialize the genpd for the domain.
>>
>> In addition to the entry and exit latency for idle state, also add
>> residency_ns, param and of_node property to each state. A domain idling
>> in a state is only power effecient if it stays idle for a certain period
>> in that state. The residency provides this minimum time for the idle
>> state to provide power benefits. The param is a state specific u32 value
>> that the platform may use for that idle state.
>>
>> This patch is based on the original patch by Marc Titinger.
>>
>> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |   8 ++++
>>  2 files changed, 111 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 740afa9..368a5b8 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1895,6 +1895,109 @@ out:
>>         return ret ? -EPROBE_DEFER : 0;
>>  }
>>  EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>> +
>> +static const struct of_device_id idle_state_match[] = {
>> +       { .compatible = "arm,idle-state", },
>> +       { }
>> +};
>> +
>> +static int read_genpd_state(struct genpd_power_state *genpd_state,
>
>/s/read_genpd_state/genpd_parse_state
>
Ok.

>> +                                   struct device_node *state_node)
>> +{
>> +       int err = 0;
>
>No need to assign err to 0.
>
Will fix.
>> +       u32 latency;
>> +       u32 residency;
>> +       u32 entry_latency, exit_latency;
>> +       const struct of_device_id *match_id;
>> +
>> +       match_id = of_match_node(idle_state_match, state_node);
>> +       if (!match_id)
>> +               return -EINVAL;
>> +
>> +       err = of_property_read_u32(state_node, "entry-latency-us",
>> +                                               &entry_latency);
>> +       if (err) {
>> +               pr_debug(" * %s missing entry-latency-us property\n",
>> +                                               state_node->full_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       err = of_property_read_u32(state_node, "exit-latency-us",
>> +                                               &exit_latency);
>> +       if (err) {
>> +               pr_debug(" * %s missing exit-latency-us property\n",
>> +                                               state_node->full_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       err = of_property_read_u32(state_node, "min-residency-us", &residency);
>> +       if (!err)
>> +               genpd_state->residency_ns = 1000 * residency;
>> +
>> +       latency = entry_latency + exit_latency;
>
>Hmm, this is probably not what you want.
>
>The genpd governor, via __default_power_down_ok(), already adds the
>->power_on_latency_ns and the ->power_off_latency_ns, when it
>validates which idle state you are allowed to enter.
>
Good catch.
>> +       genpd_state->power_on_latency_ns = 1000 * latency;
>> +       genpd_state->power_off_latency_ns = 1000 * entry_latency;
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * of_genpd_parse_idle_states: Return array of idle states for the genpd.
>> + *
>> + * @dn: The genpd device node
>> + * @states: The pointer to which the state array will be saved.
>> + * @n: The count of elements in the array returned from this function.
>> + *
>> + * Returns the device states parsed from the OF node. The memory for the states
>> + * is allocated by this function and is the responsibility of the caller to
>> + * free the memory after use.
>> + */
>> +int of_genpd_parse_idle_states(struct device_node *dn,
>> +                       struct genpd_power_state **states, int *n)
>> +{
>> +       struct genpd_power_state *st;
>> +       struct device_node *np;
>> +       int i, ret = 0;
>> +       int count;
>> +
>> +       for (count = 0; ; count++)
>> +               if (!of_parse_phandle(dn, "domain-idle-states", count))
>> +                       break;
>
>I think it's better to use of_count_phandle_with_args() to find out
>the number of phandles.
>
>> +
>> +       st = kcalloc(count, sizeof(*st), GFP_KERNEL);
>> +       if (!st)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < count; i++) {
>> +               np = of_parse_phandle(dn, "domain-idle-states", i);
>
>Isn't this a case of when it would be convenient to use the
>of_phandle_iterator*() APIs?
>
>> +               if (!np) {
>> +                       ret = -EFAULT;
>> +                       break;
>> +               }
>> +
>> +               ret = read_genpd_state(&st[i], np);
>> +               if (ret) {
>> +                       pr_err
>> +                       ("Parsing idle state node %s failed with err %d\n",
>> +                                                       np->full_name, ret);
>> +                       of_node_put(np);
>> +                       break;
>> +               }
>> +               of_node_put(np);
>> +       }
>> +
>> +       if (ret) {
>> +               kfree(st);
>> +               return ret;
>> +       }
>> +
>> +       *n = count;
>> +       *states = st;
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(of_genpd_parse_idle_states);
>> +
>>  #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>>
>>
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index c113713..4c9152d 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -204,6 +204,8 @@ extern int of_genpd_add_device(struct of_phandle_args *args,
>>  extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
>>                                   struct of_phandle_args *new_subdomain);
>>  extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
>> +extern int of_genpd_parse_idle_states(struct device_node *dn,
>> +                       struct genpd_power_state **states, int *n);
>>
>>  int genpd_dev_pm_attach(struct device *dev);
>>  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
>> @@ -233,6 +235,12 @@ static inline int of_genpd_add_subdomain(struct of_phandle_args *parent,
>>         return -ENODEV;
>>  }
>>
>> +static inline int of_genpd_parse_idle_states(struct device_node *dn,
>> +                       struct genpd_power_state **states, int *n)
>> +{
>> +       return -ENODEV;
>> +}
>> +
>>  static inline int genpd_dev_pm_attach(struct device *dev)
>>  {
>>         return -ENODEV;
>> --
>> 2.7.4
>>
>
>Kind regards
>Uffe

[PATCH 4/8] PM / Domains: Add fwnode provider to genpd states

[Lina Iyer]

Save the fwnode for the genpd state in the state node. PM Domain clients
may use the fwnode to read in the rest of the properties for the domain
state.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/base/power/domain.c | 1 +
 include/linux/pm_domain.h   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 368a5b8..52fcdb2 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1937,6 +1937,7 @@ static int read_genpd_state(struct genpd_power_state *genpd_state,
 	latency = entry_latency + exit_latency;
 	genpd_state->power_on_latency_ns = 1000 * latency;
 	genpd_state->power_off_latency_ns = 1000 * entry_latency;
+	genpd_state->provider = &state_node->fwnode;
 
 	return 0;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 4c9152d..eacfa71 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -39,6 +39,7 @@ struct genpd_power_state {
 	s64 power_off_latency_ns;
 	s64 power_on_latency_ns;
 	s64 residency_ns;
+	struct fwnode_handle *provider;
 };
 
 struct generic_pm_domain {
-- 
2.7.4

[PATCH 4/8] PM / Domains: Add fwnode provider to genpd states

[Ulf Hansson]

On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
> Save the fwnode for the genpd state in the state node. PM Domain clients
> may use the fwnode to read in the rest of the properties for the domain
> state.

What is the "rest"? I assume you mean the non PM domain generic parts,
but perhaps you could elaborate a bit on that?

>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  drivers/base/power/domain.c | 1 +
>  include/linux/pm_domain.h   | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 368a5b8..52fcdb2 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1937,6 +1937,7 @@ static int read_genpd_state(struct genpd_power_state *genpd_state,
>         latency = entry_latency + exit_latency;
>         genpd_state->power_on_latency_ns = 1000 * latency;
>         genpd_state->power_off_latency_ns = 1000 * entry_latency;
> +       genpd_state->provider = &state_node->fwnode;
>
>         return 0;
>  }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 4c9152d..eacfa71 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -39,6 +39,7 @@ struct genpd_power_state {
>         s64 power_off_latency_ns;
>         s64 power_on_latency_ns;
>         s64 residency_ns;
> +       struct fwnode_handle *provider;

I don't think this is a provider, but just a fwnode_handle to a
domain-idle-state. Therefore, I would suggest you to rename it to
"fwnode" instead.

>  };
>
>  struct generic_pm_domain {
> --
> 2.7.4
>

Kind regards
Uffe

[PATCH 4/8] PM / Domains: Add fwnode provider to genpd states

[Lina Iyer]

On Thu, Oct 06 2016 at 06:01 -0600, Ulf Hansson wrote:
>On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Save the fwnode for the genpd state in the state node. PM Domain clients
>> may use the fwnode to read in the rest of the properties for the domain
>> state.
>
>What is the "rest"? I assume you mean the non PM domain generic parts,
>but perhaps you could elaborate a bit on that?
>
>>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 1 +
>>  include/linux/pm_domain.h   | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 368a5b8..52fcdb2 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1937,6 +1937,7 @@ static int read_genpd_state(struct genpd_power_state *genpd_state,
>>         latency = entry_latency + exit_latency;
>>         genpd_state->power_on_latency_ns = 1000 * latency;
>>         genpd_state->power_off_latency_ns = 1000 * entry_latency;
>> +       genpd_state->provider = &state_node->fwnode;
>>
>>         return 0;
>>  }
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 4c9152d..eacfa71 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -39,6 +39,7 @@ struct genpd_power_state {
>>         s64 power_off_latency_ns;
>>         s64 power_on_latency_ns;
>>         s64 residency_ns;
>> +       struct fwnode_handle *provider;
>
>I don't think this is a provider, but just a fwnode_handle to a
>domain-idle-state. Therefore, I would suggest you to rename it to
>"fwnode" instead.
>
OK.

>>  };
>>
>>  struct generic_pm_domain {
>> --
>> 2.7.4
>>
>
>Kind regards
>Uffe

[PATCH 5/8] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

Update DT bindings to describe idle states of PM domains.

This patch is based on the original patch by Marc Titinger.

Cc: <devicetree@vger.kernel.org>
Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/power/power_domain.txt     | 36 ++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
index 025b5e7..a043315 100644
--- a/Documentation/devicetree/bindings/power/power_domain.txt
+++ b/Documentation/devicetree/bindings/power/power_domain.txt
@@ -29,6 +29,10 @@ Optional properties:
    specified by this binding. More details about power domain specifier are
    available in the next section.
 
+- domain-idle-states : A phandle of an idle-state that shall be soaked into a
+                generic domain power state. The idle state definitions are
+                compatible with arm,idle-state specified in [1].
+
 Example:
 
 	power: power-controller at 12340000 {
@@ -59,6 +63,36 @@ The nodes above define two power controllers: 'parent' and 'child'.
 Domains created by the 'child' power controller are subdomains of '0' power
 domain provided by the 'parent' power controller.
 
+Example 3:
+	parent: power-controller at 12340000 {
+		compatible = "foo,power-controller";
+		reg = <0x12340000 0x1000>;
+		#power-domain-cells = <1>;
+		domain-idle-states = <&DOMAIN_RET, &DOMAIN_PWR_DN>;
+	};
+
+	child: power-controller at 12341000 {
+		compatible = "foo,power-controller";
+		reg = <0x12341000 0x1000>;
+		power-domains = <&parent 0>;
+		#power-domain-cells = <1>;
+		domain-idle-states = <&DOMAIN_PWR_DN>;
+	};
+
+	DOMAIN_RET: state at 0 {
+		compatible = "arm,idle-state";
+		entry-latency-us = <1000>;
+		exit-latency-us = <2000>;
+		min-residency-us = <10000>;
+	};
+
+	DOMAIN_PWR_DN: state at 1 {
+		compatible = "arm,idle-state";
+		entry-latency-us = <5000>;
+		exit-latency-us = <8000>;
+		min-residency-us = <7000>;
+	};
+
 ==PM domain consumers==
 
 Required properties:
@@ -76,3 +110,5 @@ Example:
 The node above defines a typical PM domain consumer device, which is located
 inside a PM domain with index 0 of a power controller represented by a node
 with the label "power".
+
+[1]. Documentation/devicetree/bindings/arm/idle-states.txt
-- 
2.7.4

[PATCH 5/8] dt/bindings: Update binding for PM domain idle states

[Geert Uytterhoeven]

On Wed, Oct 5, 2016 at 10:31 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Update DT bindings to describe idle states of PM domains.
>
> This patch is based on the original patch by Marc Titinger.
>
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/power/power_domain.txt     | 36 ++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
> index 025b5e7..a043315 100644
> --- a/Documentation/devicetree/bindings/power/power_domain.txt
> +++ b/Documentation/devicetree/bindings/power/power_domain.txt
> @@ -29,6 +29,10 @@ Optional properties:
>     specified by this binding. More details about power domain specifier are
>     available in the next section.
>
> +- domain-idle-states : A phandle of an idle-state that shall be soaked into a
> +                generic domain power state. The idle state definitions are
> +                compatible with arm,idle-state specified in [1].
> +
>  Example:
>
>         power: power-controller at 12340000 {
> @@ -59,6 +63,36 @@ The nodes above define two power controllers: 'parent' and 'child'.
>  Domains created by the 'child' power controller are subdomains of '0' power
>  domain provided by the 'parent' power controller.
>
> +Example 3:
> +       parent: power-controller at 12340000 {

With W=1, this is gonna trigger:

    Warning (unit_address_vs_reg): Node foo has a unit name, but no reg property

Yes, there are pre-existing users in this file.

> +               compatible = "foo,power-controller";
> +               reg = <0x12340000 0x1000>;
> +               #power-domain-cells = <1>;
> +               domain-idle-states = <&DOMAIN_RET, &DOMAIN_PWR_DN>;
> +       };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 5/8] dt/bindings: Update binding for PM domain idle states

[Geert Uytterhoeven]

On Thu, Oct 6, 2016 at 10:06 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Oct 5, 2016 at 10:31 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Update DT bindings to describe idle states of PM domains.
>>
>> This patch is based on the original patch by Marc Titinger.
>>
>> Cc: <devicetree@vger.kernel.org>
>> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>  .../devicetree/bindings/power/power_domain.txt     | 36 ++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
>> index 025b5e7..a043315 100644
>> --- a/Documentation/devicetree/bindings/power/power_domain.txt
>> +++ b/Documentation/devicetree/bindings/power/power_domain.txt
>> @@ -29,6 +29,10 @@ Optional properties:
>>     specified by this binding. More details about power domain specifier are
>>     available in the next section.
>>
>> +- domain-idle-states : A phandle of an idle-state that shall be soaked into a
>> +                generic domain power state. The idle state definitions are
>> +                compatible with arm,idle-state specified in [1].
>> +
>>  Example:
>>
>>         power: power-controller at 12340000 {
>> @@ -59,6 +63,36 @@ The nodes above define two power controllers: 'parent' and 'child'.
>>  Domains created by the 'child' power controller are subdomains of '0' power
>>  domain provided by the 'parent' power controller.
>>
>> +Example 3:
>> +       parent: power-controller at 12340000 {
>
> With W=1, this is gonna trigger:
>
>     Warning (unit_address_vs_reg): Node foo has a unit name, but no reg property
>
> Yes, there are pre-existing users in this file.

Scrap this... switching desktops causes loss of position...

>> +               compatible = "foo,power-controller";
>> +               reg = <0x12340000 0x1000>;
>> +               #power-domain-cells = <1>;
>> +               domain-idle-states = <&DOMAIN_RET, &DOMAIN_PWR_DN>;
>> +       };

+       DOMAIN_RET: state at 0 {
+               compatible = "arm,idle-state";
+               entry-latency-us = <1000>;
+               exit-latency-us = <2000>;
+               min-residency-us = <10000>;

This one is gonna trigger

Warning (unit_address_vs_reg): Node foo has a unit name, but no reg property

+       };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 5/8] dt/bindings: Update binding for PM domain idle states

[Lina Iyer]

On Thu, Oct 06 2016 at 02:09 -0600, Geert Uytterhoeven wrote:
>On Thu, Oct 6, 2016 at 10:06 AM, Geert Uytterhoeven
><geert@linux-m68k.org> wrote:
>> On Wed, Oct 5, 2016 at 10:31 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>> Update DT bindings to describe idle states of PM domains.
>>>
>>> This patch is based on the original patch by Marc Titinger.
>>>
>>> Cc: <devicetree@vger.kernel.org>
>>> Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> ---
>>>  .../devicetree/bindings/power/power_domain.txt     | 36 ++++++++++++++++++++++
>>>  1 file changed, 36 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
>>> index 025b5e7..a043315 100644
>>> --- a/Documentation/devicetree/bindings/power/power_domain.txt
>>> +++ b/Documentation/devicetree/bindings/power/power_domain.txt
>>> @@ -29,6 +29,10 @@ Optional properties:
>>>     specified by this binding. More details about power domain specifier are
>>>     available in the next section.
>>>
>>> +- domain-idle-states : A phandle of an idle-state that shall be soaked into a
>>> +                generic domain power state. The idle state definitions are
>>> +                compatible with arm,idle-state specified in [1].
>>> +
>>>  Example:
>>>
>>>         power: power-controller at 12340000 {
>>> @@ -59,6 +63,36 @@ The nodes above define two power controllers: 'parent' and 'child'.
>>>  Domains created by the 'child' power controller are subdomains of '0' power
>>>  domain provided by the 'parent' power controller.
>>>
>>> +Example 3:
>>> +       parent: power-controller at 12340000 {
>>
>> With W=1, this is gonna trigger:
>>
>>     Warning (unit_address_vs_reg): Node foo has a unit name, but no reg property
>>
>> Yes, there are pre-existing users in this file.
>
>Scrap this... switching desktops causes loss of position...
>
>>> +               compatible = "foo,power-controller";
>>> +               reg = <0x12340000 0x1000>;
>>> +               #power-domain-cells = <1>;
>>> +               domain-idle-states = <&DOMAIN_RET, &DOMAIN_PWR_DN>;
>>> +       };
>
>+       DOMAIN_RET: state at 0 {
>+               compatible = "arm,idle-state";
>+               entry-latency-us = <1000>;
>+               exit-latency-us = <2000>;
>+               min-residency-us = <10000>;
>
>This one is gonna trigger
>
>Warning (unit_address_vs_reg): Node foo has a unit name, but no reg property
>
>+       };
>
Its just an example, Will fix the example.

>Gr{oetje,eeting}s,
>
>                        Geert
>
>--
>Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
>
>In personal conversations with technical people, I call myself a hacker. But
>when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds

[PATCH 6/8] PM / Domains: Abstract genpd locking

[Lina Iyer]

Abstract genpd lock/unlock calls, in preparation for domain specific
locks added in the following patches.

Cc: Kevin Hilman <khilman@kernel.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 121 +++++++++++++++++++++++++++++---------------
 include/linux/pm_domain.h   |   5 +-
 2 files changed, 85 insertions(+), 41 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 52fcdb2..82e6a33 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -39,6 +39,46 @@
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
+struct genpd_lock_fns {
+	void (*lock)(struct generic_pm_domain *genpd);
+	void (*lock_nested)(struct generic_pm_domain *genpd, int depth);
+	int (*lock_interruptible)(struct generic_pm_domain *genpd);
+	void (*unlock)(struct generic_pm_domain *genpd);
+};
+
+static void genpd_lock_mtx(struct generic_pm_domain *genpd)
+{
+	mutex_lock(&genpd->mlock);
+}
+
+static void genpd_lock_nested_mtx(struct generic_pm_domain *genpd,
+					int depth)
+{
+	mutex_lock_nested(&genpd->mlock, depth);
+}
+
+static int genpd_lock_interruptible_mtx(struct generic_pm_domain *genpd)
+{
+	return mutex_lock_interruptible(&genpd->mlock);
+}
+
+static void genpd_unlock_mtx(struct generic_pm_domain *genpd)
+{
+	return mutex_unlock(&genpd->mlock);
+}
+
+static const struct genpd_lock_fns genpd_mtx_fns  = {
+	.lock = genpd_lock_mtx,
+	.lock_nested = genpd_lock_nested_mtx,
+	.lock_interruptible = genpd_lock_interruptible_mtx,
+	.unlock = genpd_unlock_mtx,
+};
+
+#define genpd_lock(p)			p->lock_fns->lock(p)
+#define genpd_lock_nested(p, d)		p->lock_fns->lock_nested(p, d)
+#define genpd_lock_interruptible(p)	p->lock_fns->lock_interruptible(p)
+#define genpd_unlock(p)			p->lock_fns->unlock(p)
+
 /*
  * Get the generic PM domain for a particular struct device.
  * This validates the struct device pointer, the PM domain pointer,
@@ -200,9 +240,9 @@ static int genpd_poweron(struct generic_pm_domain *genpd, unsigned int depth)
 
 		genpd_sd_counter_inc(master);
 
-		mutex_lock_nested(&master->lock, depth + 1);
+		genpd_lock_nested(master, depth + 1);
 		ret = genpd_poweron(master, depth + 1);
-		mutex_unlock(&master->lock);
+		genpd_unlock(master);
 
 		if (ret) {
 			genpd_sd_counter_dec(master);
@@ -255,9 +295,9 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
 		spin_unlock_irq(&dev->power.lock);
 
 		if (!IS_ERR(genpd)) {
-			mutex_lock(&genpd->lock);
+			genpd_lock(genpd);
 			genpd->max_off_time_changed = true;
-			mutex_unlock(&genpd->lock);
+			genpd_unlock(genpd);
 		}
 
 		dev = dev->parent;
@@ -354,9 +394,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
 
 	genpd = container_of(work, struct generic_pm_domain, power_off_work);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	genpd_poweroff(genpd, true);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 }
 
 /**
@@ -472,9 +512,9 @@ static int genpd_runtime_suspend(struct device *dev)
 	if (dev->power.irq_safe)
 		return 0;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	genpd_poweroff(genpd, false);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return 0;
 }
@@ -509,9 +549,9 @@ static int genpd_runtime_resume(struct device *dev)
 		goto out;
 	}
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	ret = genpd_poweron(genpd, 0);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (ret)
 		return ret;
@@ -547,9 +587,9 @@ err_stop:
 	genpd_stop_dev(genpd, dev);
 err_poweroff:
 	if (!dev->power.irq_safe) {
-		mutex_lock(&genpd->lock);
+		genpd_lock(genpd);
 		genpd_poweroff(genpd, 0);
-		mutex_unlock(&genpd->lock);
+		genpd_unlock(genpd);
 	}
 
 	return ret;
@@ -732,20 +772,20 @@ static int pm_genpd_prepare(struct device *dev)
 	if (resume_needed(dev, genpd))
 		pm_runtime_resume(dev);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count++ == 0)
 		genpd->suspended_count = 0;
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	ret = pm_generic_prepare(dev);
 	if (ret) {
-		mutex_lock(&genpd->lock);
+		genpd_lock(genpd);
 
 		genpd->prepared_count--;
 
-		mutex_unlock(&genpd->lock);
+		genpd_unlock(genpd);
 	}
 
 	return ret;
@@ -936,13 +976,13 @@ static void pm_genpd_complete(struct device *dev)
 
 	pm_generic_complete(dev);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	genpd->prepared_count--;
 	if (!genpd->prepared_count)
 		genpd_queue_power_off_work(genpd);
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 }
 
 /**
@@ -1071,7 +1111,7 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (IS_ERR(gpd_data))
 		return PTR_ERR(gpd_data);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
 		ret = -EAGAIN;
@@ -1088,7 +1128,7 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (ret)
 		genpd_free_dev_data(dev, gpd_data);
@@ -1130,7 +1170,7 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
 	gpd_data = to_gpd_data(pdd);
 	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
 		ret = -EAGAIN;
@@ -1145,14 +1185,14 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
 
 	list_del_init(&pdd->list_node);
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	genpd_free_dev_data(dev, gpd_data);
 
 	return 0;
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	dev_pm_qos_add_notifier(dev, &gpd_data->nb);
 
 	return ret;
@@ -1187,8 +1227,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
 	if (!link)
 		return -ENOMEM;
 
-	mutex_lock(&subdomain->lock);
-	mutex_lock_nested(&genpd->lock, SINGLE_DEPTH_NESTING);
+	genpd_lock(subdomain);
+	genpd_lock_nested(genpd, SINGLE_DEPTH_NESTING);
 
 	if (genpd->status == GPD_STATE_POWER_OFF
 	    &&  subdomain->status != GPD_STATE_POWER_OFF) {
@@ -1211,8 +1251,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
 		genpd_sd_counter_inc(genpd);
 
  out:
-	mutex_unlock(&genpd->lock);
-	mutex_unlock(&subdomain->lock);
+	genpd_unlock(genpd);
+	genpd_unlock(subdomain);
 	if (ret)
 		kfree(link);
 	return ret;
@@ -1250,8 +1290,8 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain))
 		return -EINVAL;
 
-	mutex_lock(&subdomain->lock);
-	mutex_lock_nested(&genpd->lock, SINGLE_DEPTH_NESTING);
+	genpd_lock(subdomain);
+	genpd_lock_nested(genpd, SINGLE_DEPTH_NESTING);
 
 	if (!list_empty(&subdomain->master_links) || subdomain->device_count) {
 		pr_warn("%s: unable to remove subdomain %s\n", genpd->name,
@@ -1275,8 +1315,8 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 	}
 
 out:
-	mutex_unlock(&genpd->lock);
-	mutex_unlock(&subdomain->lock);
+	genpd_unlock(genpd);
+	genpd_unlock(subdomain);
 
 	return ret;
 }
@@ -1299,7 +1339,8 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	INIT_LIST_HEAD(&genpd->master_links);
 	INIT_LIST_HEAD(&genpd->slave_links);
 	INIT_LIST_HEAD(&genpd->dev_list);
-	mutex_init(&genpd->lock);
+	mutex_init(&genpd->mlock);
+	genpd->lock_fns = &genpd_mtx_fns;
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
@@ -1344,16 +1385,16 @@ static int genpd_remove(struct generic_pm_domain *genpd)
 	if (IS_ERR_OR_NULL(genpd))
 		return -EINVAL;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->has_provider) {
-		mutex_unlock(&genpd->lock);
+		genpd_unlock(genpd);
 		pr_err("Provider present, unable to remove %s\n", genpd->name);
 		return -EBUSY;
 	}
 
 	if (!list_empty(&genpd->master_links) || genpd->device_count) {
-		mutex_unlock(&genpd->lock);
+		genpd_unlock(genpd);
 		pr_err("%s: unable to remove %s\n", __func__, genpd->name);
 		return -EBUSY;
 	}
@@ -1365,7 +1406,7 @@ static int genpd_remove(struct generic_pm_domain *genpd)
 	}
 
 	list_del(&genpd->gpd_list_node);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	cancel_work_sync(&genpd->power_off_work);
 	pr_debug("%s: removed %s\n", __func__, genpd->name);
 
@@ -1888,9 +1929,9 @@ int genpd_dev_pm_attach(struct device *dev)
 	dev->pm_domain->detach = genpd_dev_pm_detach;
 	dev->pm_domain->sync = genpd_dev_pm_sync;
 
-	mutex_lock(&pd->lock);
+	genpd_lock(pd);
 	ret = genpd_poweron(pd, 0);
-	mutex_unlock(&pd->lock);
+	genpd_unlock(pd);
 out:
 	return ret ? -EPROBE_DEFER : 0;
 }
@@ -2052,7 +2093,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 	char state[16];
 	int ret;
 
-	ret = mutex_lock_interruptible(&genpd->lock);
+	ret = genpd_lock_interruptible(genpd);
 	if (ret)
 		return -ERESTARTSYS;
 
@@ -2089,7 +2130,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 
 	seq_puts(s, "\n");
 exit:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return 0;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index eacfa71..a8a6124 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -42,13 +42,14 @@ struct genpd_power_state {
 	struct fwnode_handle *provider;
 };
 
+struct genpd_lock_fns;
+
 struct generic_pm_domain {
 	struct dev_pm_domain domain;	/* PM domain operations */
 	struct list_head gpd_list_node;	/* Node in the global PM domains list */
 	struct list_head master_links;	/* Links with PM domain as a master */
 	struct list_head slave_links;	/* Links with PM domain as a slave */
 	struct list_head dev_list;	/* List of devices */
-	struct mutex lock;
 	struct dev_power_governor *gov;
 	struct work_struct power_off_work;
 	struct fwnode_handle *provider;	/* Identity of the domain provider */
@@ -73,6 +74,8 @@ struct generic_pm_domain {
 	struct genpd_power_state *states;
 	unsigned int state_count; /* number of states */
 	unsigned int state_idx; /* state that genpd will go to when off */
+	const struct genpd_lock_fns *lock_fns;
+	struct mutex mlock;
 
 };
 
-- 
2.7.4

[PATCH 6/8] PM / Domains: Abstract genpd locking

[Ulf Hansson]

On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
> Abstract genpd lock/unlock calls, in preparation for domain specific
> locks added in the following patches.
>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/domain.c | 121 +++++++++++++++++++++++++++++---------------
>  include/linux/pm_domain.h   |   5 +-
>  2 files changed, 85 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 52fcdb2..82e6a33 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -39,6 +39,46 @@
>  static LIST_HEAD(gpd_list);
>  static DEFINE_MUTEX(gpd_list_lock);
>
> +struct genpd_lock_fns {

May I suggest you to rename the struct to "genpd_lock_ops"?

I think "*_ops" is in general what we use in the kernel for callbacks
and functions pointers like these.

> +       void (*lock)(struct generic_pm_domain *genpd);
> +       void (*lock_nested)(struct generic_pm_domain *genpd, int depth);
> +       int (*lock_interruptible)(struct generic_pm_domain *genpd);
> +       void (*unlock)(struct generic_pm_domain *genpd);
> +};
> +

[...]

Otherwise this looks good to me!

Kind regards
Uffe

[PATCH 6/8] PM / Domains: Abstract genpd locking

[Lina Iyer]

On Thu, Oct 06 2016 at 04:56 -0600, Ulf Hansson wrote:
>On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Abstract genpd lock/unlock calls, in preparation for domain specific
>> locks added in the following patches.
>>
>> Cc: Kevin Hilman <khilman@kernel.org>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/base/power/domain.c | 121 +++++++++++++++++++++++++++++---------------
>>  include/linux/pm_domain.h   |   5 +-
>>  2 files changed, 85 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 52fcdb2..82e6a33 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -39,6 +39,46 @@
>>  static LIST_HEAD(gpd_list);
>>  static DEFINE_MUTEX(gpd_list_lock);
>>
>> +struct genpd_lock_fns {
>
>May I suggest you to rename the struct to "genpd_lock_ops"?
>
>I think "*_ops" is in general what we use in the kernel for callbacks
>and functions pointers like these.
>
OK.

Thanks,
Lina

>> +       void (*lock)(struct generic_pm_domain *genpd);
>> +       void (*lock_nested)(struct generic_pm_domain *genpd, int depth);
>> +       int (*lock_interruptible)(struct generic_pm_domain *genpd);
>> +       void (*unlock)(struct generic_pm_domain *genpd);
>> +};
>> +
>
>[...]
>
>Otherwise this looks good to me!
>
>Kind regards
>Uffe

[PATCH 7/8] PM / Domains: Support IRQ safe PM domains

[Lina Iyer]

Generic Power Domains currently support turning on/off only in process
context. This prevents the usage of PM domains for domains that could be
powered on/off in a context where IRQs are disabled. Many such domains
exist today and do not get powered off, when the IRQ safe devices in
that domain are powered off, because of this limitation.

However, not all domains can operate in IRQ safe contexts. Genpd
therefore, has to support both cases where the domain may or may not
operate in IRQ safe contexts. Configuring genpd to use an appropriate
lock for that domain, would allow domains that have IRQ safe devices to
runtime suspend and resume, in atomic context.

To achieve domain specific locking, set the domain's ->flag to
GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
should use a spinlock instead of a mutex for locking the domain. Locking
is abstracted through genpd_lock() and genpd_unlock() functions that use
the flag to determine the appropriate lock to be used for that domain.

Domains that have lower latency to suspend and resume and can operate
with IRQs disabled may now be able to save power, when the component
devices and sub-domains are idle at runtime.

The restriction this imposes on the domain hierarchy is that non-IRQ
safe domains may not have IRQ-safe subdomains, but IRQ safe domains may
have IRQ safe and non-IRQ safe subdomains and devices.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/base/power/domain.c | 107 +++++++++++++++++++++++++++++++++++++++-----
 include/linux/pm_domain.h   |  10 ++++-
 2 files changed, 106 insertions(+), 11 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 82e6a33..77e92c2 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -74,11 +74,61 @@ static const struct genpd_lock_fns genpd_mtx_fns  = {
 	.unlock = genpd_unlock_mtx,
 };
 
+static void genpd_lock_spin(struct generic_pm_domain *genpd)
+	__acquires(&genpd->slock)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&genpd->slock, flags);
+	genpd->lock_flags = flags;
+}
+
+static void genpd_lock_nested_spin(struct generic_pm_domain *genpd,
+					int depth)
+	__acquires(&genpd->slock)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave_nested(&genpd->slock, flags, depth);
+	genpd->lock_flags = flags;
+}
+
+static int genpd_lock_interruptible_spin(struct generic_pm_domain *genpd)
+	__acquires(&genpd->slock)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&genpd->slock, flags);
+	genpd->lock_flags = flags;
+	return 0;
+}
+
+static void genpd_unlock_spin(struct generic_pm_domain *genpd)
+	__releases(&genpd->slock)
+{
+	spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
+}
+
+static const struct genpd_lock_fns genpd_spin_fns = {
+	.lock = genpd_lock_spin,
+	.lock_nested = genpd_lock_nested_spin,
+	.lock_interruptible = genpd_lock_interruptible_spin,
+	.unlock = genpd_unlock_spin,
+};
+
 #define genpd_lock(p)			p->lock_fns->lock(p)
 #define genpd_lock_nested(p, d)		p->lock_fns->lock_nested(p, d)
 #define genpd_lock_interruptible(p)	p->lock_fns->lock_interruptible(p)
 #define genpd_unlock(p)			p->lock_fns->unlock(p)
 
+#define genpd_is_irq_safe(genpd)	(genpd->flags & GENPD_FLAG_IRQ_SAFE)
+
+static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
+		struct generic_pm_domain *genpd)
+{
+	return pm_runtime_is_irq_safe(dev) && !genpd_is_irq_safe(genpd);
+}
+
 /*
  * Get the generic PM domain for a particular struct device.
  * This validates the struct device pointer, the PM domain pointer,
@@ -343,7 +393,12 @@ static int genpd_poweroff(struct generic_pm_domain *genpd, bool is_async)
 		if (stat > PM_QOS_FLAGS_NONE)
 			return -EBUSY;
 
-		if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
+		/*
+		 * Do not allow PM domain to be powered off, when an IRQ safe
+		 * device is part of a non-IRQ safe domain.
+		 */
+		if (!pm_runtime_suspended(pdd->dev) ||
+			irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd))
 			not_suspended++;
 	}
 
@@ -506,10 +561,10 @@ static int genpd_runtime_suspend(struct device *dev)
 	}
 
 	/*
-	 * If power.irq_safe is set, this routine will be run with interrupts
-	 * off, so it can't use mutexes.
+	 * If power.irq_safe is set, this routine may be run with
+	 * IRQs disabled, so suspend only if the PM domain also is irq_safe.
 	 */
-	if (dev->power.irq_safe)
+	if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
 		return 0;
 
 	genpd_lock(genpd);
@@ -543,8 +598,11 @@ static int genpd_runtime_resume(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	/* If power.irq_safe, the PM domain is never powered off. */
-	if (dev->power.irq_safe) {
+	/*
+	 * As we don't power off a non IRQ safe domain, which holds
+	 * an IRQ safe device, we don't need to restore power to it.
+	 */
+	if (irq_safe_dev_in_no_sleep_domain(dev, genpd)) {
 		timed = false;
 		goto out;
 	}
@@ -586,7 +644,8 @@ static int genpd_runtime_resume(struct device *dev)
 err_stop:
 	genpd_stop_dev(genpd, dev);
 err_poweroff:
-	if (!dev->power.irq_safe) {
+	if (!dev->power.irq_safe ||
+		(dev->power.irq_safe && genpd_is_irq_safe(genpd))) {
 		genpd_lock(genpd);
 		genpd_poweroff(genpd, 0);
 		genpd_unlock(genpd);
@@ -1111,6 +1170,11 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (IS_ERR(gpd_data))
 		return PTR_ERR(gpd_data);
 
+	/* Check if we are adding an IRQ safe device to non-IRQ safe domain */
+	if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
+		dev_warn_once(dev, "PM domain %s will not be powered off\n",
+				genpd->name);
+
 	genpd_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
@@ -1223,6 +1287,17 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
 	    || genpd == subdomain)
 		return -EINVAL;
 
+	/*
+	 * If the domain can be powered on/off in an IRQ safe
+	 * context, ensure that the subdomain can also be
+	 * powered on/off in that context.
+	 */
+	if (!genpd_is_irq_safe(genpd) && genpd_is_irq_safe(subdomain)) {
+		WARN("Parent %s of subdomain %s must be IRQ safe\n",
+				genpd->name, subdomain->name);
+		return -EINVAL;
+	}
+
 	link = kzalloc(sizeof(*link), GFP_KERNEL);
 	if (!link)
 		return -ENOMEM;
@@ -1322,6 +1397,17 @@ out:
 }
 EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain);
 
+static void genpd_lock_init(struct generic_pm_domain *genpd)
+{
+	if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
+		spin_lock_init(&genpd->slock);
+		genpd->lock_fns = &genpd_spin_fns;
+	} else {
+		mutex_init(&genpd->mlock);
+		genpd->lock_fns = &genpd_mtx_fns;
+	}
+}
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -1339,8 +1425,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 	INIT_LIST_HEAD(&genpd->master_links);
 	INIT_LIST_HEAD(&genpd->slave_links);
 	INIT_LIST_HEAD(&genpd->dev_list);
-	mutex_init(&genpd->mlock);
-	genpd->lock_fns = &genpd_mtx_fns;
+	genpd_lock_init(genpd);
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	atomic_set(&genpd->sd_count, 0);
@@ -2119,7 +2204,9 @@ static int pm_genpd_summary_one(struct seq_file *s,
 	}
 
 	list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
-		kobj_path = kobject_get_path(&pm_data->dev->kobj, GFP_KERNEL);
+		kobj_path = kobject_get_path(&pm_data->dev->kobj,
+				genpd_is_irq_safe(genpd) ?
+				GFP_ATOMIC : GFP_KERNEL);
 		if (kobj_path == NULL)
 			continue;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index a8a6124..0e83764 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -15,9 +15,11 @@
 #include <linux/err.h>
 #include <linux/of.h>
 #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 */
 
 enum gpd_status {
 	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
@@ -75,7 +77,13 @@ struct generic_pm_domain {
 	unsigned int state_count; /* number of states */
 	unsigned int state_idx; /* state that genpd will go to when off */
 	const struct genpd_lock_fns *lock_fns;
-	struct mutex mlock;
+	union {
+		struct mutex mlock;
+		struct {
+			spinlock_t slock;
+			unsigned long lock_flags;
+		};
+	};
 
 };
 
-- 
2.7.4

[PATCH 7/8] PM / Domains: Support IRQ safe PM domains

[Ulf Hansson]

On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
> Generic Power Domains currently support turning on/off only in process
> context. This prevents the usage of PM domains for domains that could be
> powered on/off in a context where IRQs are disabled. Many such domains
> exist today and do not get powered off, when the IRQ safe devices in
> that domain are powered off, because of this limitation.
>
> However, not all domains can operate in IRQ safe contexts. Genpd
> therefore, has to support both cases where the domain may or may not
> operate in IRQ safe contexts. Configuring genpd to use an appropriate
> lock for that domain, would allow domains that have IRQ safe devices to
> runtime suspend and resume, in atomic context.
>
> To achieve domain specific locking, set the domain's ->flag to
> GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
> should use a spinlock instead of a mutex for locking the domain. Locking
> is abstracted through genpd_lock() and genpd_unlock() functions that use
> the flag to determine the appropriate lock to be used for that domain.
>
> Domains that have lower latency to suspend and resume and can operate
> with IRQs disabled may now be able to save power, when the component
> devices and sub-domains are idle at runtime.
>
> The restriction this imposes on the domain hierarchy is that non-IRQ
> safe domains may not have IRQ-safe subdomains, but IRQ safe domains may
> have IRQ safe and non-IRQ safe subdomains and devices.
>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  drivers/base/power/domain.c | 107 +++++++++++++++++++++++++++++++++++++++-----
>  include/linux/pm_domain.h   |  10 ++++-
>  2 files changed, 106 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 82e6a33..77e92c2 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -74,11 +74,61 @@ static const struct genpd_lock_fns genpd_mtx_fns  = {
>         .unlock = genpd_unlock_mtx,
>  };
>
> +static void genpd_lock_spin(struct generic_pm_domain *genpd)
> +       __acquires(&genpd->slock)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&genpd->slock, flags);
> +       genpd->lock_flags = flags;
> +}
> +
> +static void genpd_lock_nested_spin(struct generic_pm_domain *genpd,
> +                                       int depth)
> +       __acquires(&genpd->slock)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave_nested(&genpd->slock, flags, depth);
> +       genpd->lock_flags = flags;
> +}
> +
> +static int genpd_lock_interruptible_spin(struct generic_pm_domain *genpd)
> +       __acquires(&genpd->slock)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&genpd->slock, flags);
> +       genpd->lock_flags = flags;
> +       return 0;
> +}
> +
> +static void genpd_unlock_spin(struct generic_pm_domain *genpd)
> +       __releases(&genpd->slock)
> +{
> +       spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
> +}
> +
> +static const struct genpd_lock_fns genpd_spin_fns = {
> +       .lock = genpd_lock_spin,
> +       .lock_nested = genpd_lock_nested_spin,
> +       .lock_interruptible = genpd_lock_interruptible_spin,
> +       .unlock = genpd_unlock_spin,
> +};
> +
>  #define genpd_lock(p)                  p->lock_fns->lock(p)
>  #define genpd_lock_nested(p, d)                p->lock_fns->lock_nested(p, d)
>  #define genpd_lock_interruptible(p)    p->lock_fns->lock_interruptible(p)
>  #define genpd_unlock(p)                        p->lock_fns->unlock(p)
>
> +#define genpd_is_irq_safe(genpd)       (genpd->flags & GENPD_FLAG_IRQ_SAFE)
> +
> +static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
> +               struct generic_pm_domain *genpd)
> +{
> +       return pm_runtime_is_irq_safe(dev) && !genpd_is_irq_safe(genpd);
> +}
> +
>  /*
>   * Get the generic PM domain for a particular struct device.
>   * This validates the struct device pointer, the PM domain pointer,
> @@ -343,7 +393,12 @@ static int genpd_poweroff(struct generic_pm_domain *genpd, bool is_async)
>                 if (stat > PM_QOS_FLAGS_NONE)
>                         return -EBUSY;
>
> -               if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
> +               /*
> +                * Do not allow PM domain to be powered off, when an IRQ safe
> +                * device is part of a non-IRQ safe domain.
> +                */
> +               if (!pm_runtime_suspended(pdd->dev) ||
> +                       irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd))
>                         not_suspended++;
>         }
>
> @@ -506,10 +561,10 @@ static int genpd_runtime_suspend(struct device *dev)
>         }
>
>         /*
> -        * If power.irq_safe is set, this routine will be run with interrupts
> -        * off, so it can't use mutexes.
> +        * If power.irq_safe is set, this routine may be run with
> +        * IRQs disabled, so suspend only if the PM domain also is irq_safe.
>          */
> -       if (dev->power.irq_safe)
> +       if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
>                 return 0;
>
>         genpd_lock(genpd);
> @@ -543,8 +598,11 @@ static int genpd_runtime_resume(struct device *dev)
>         if (IS_ERR(genpd))
>                 return -EINVAL;
>
> -       /* If power.irq_safe, the PM domain is never powered off. */
> -       if (dev->power.irq_safe) {
> +       /*
> +        * As we don't power off a non IRQ safe domain, which holds
> +        * an IRQ safe device, we don't need to restore power to it.
> +        */
> +       if (irq_safe_dev_in_no_sleep_domain(dev, genpd)) {
>                 timed = false;
>                 goto out;
>         }
> @@ -586,7 +644,8 @@ static int genpd_runtime_resume(struct device *dev)
>  err_stop:
>         genpd_stop_dev(genpd, dev);
>  err_poweroff:
> -       if (!dev->power.irq_safe) {
> +       if (!dev->power.irq_safe ||
> +               (dev->power.irq_safe && genpd_is_irq_safe(genpd))) {

Please take the opportunity to convert into use
pm_runtime_is_irq_safe(), in favour of checking "dev->power.irq_safe".

>                 genpd_lock(genpd);
>                 genpd_poweroff(genpd, 0);
>                 genpd_unlock(genpd);
> @@ -1111,6 +1170,11 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>         if (IS_ERR(gpd_data))
>                 return PTR_ERR(gpd_data);
>
> +       /* Check if we are adding an IRQ safe device to non-IRQ safe domain */
> +       if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
> +               dev_warn_once(dev, "PM domain %s will not be powered off\n",
> +                               genpd->name);
> +

It may turn out that a subsystem/driver decides to enable
pm_runtime_irq_safe() for the device at a later point, while probing.
Due to that, this warning is not going to be printed.

If we want this warning printed, I think we should strive towards a
more consistent behaviour. Perhaps we should move this print inside
irq_safe_dev_in_no_sleep_domain()?

[...]

One more thing that popped up in my mind. We currently also have the
"pm_domain_always_on_gov". If we checked for such configuration and
before taking the lock for the genpd during runtime suspend, we would
be able to allow IRQ safe device to be suspended when they reside in
these types of always-on domains, don't you think?

Anyway, you don't need to do that change as part of $subject patch,
but perhaps we might want to extend the support for irq safe devices
later on?

Besides the minor nitpicks, this looks good to me!

Kind regards
Uffe

[PATCH 8/8] PM / doc: Update device documentation for devices in IRQ safe PM domains

[Lina Iyer]

Update documentation to reflect the changes made to support IRQ safe PM
domains.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 Documentation/power/devices.txt | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
index 8ba6625..a622136 100644
--- a/Documentation/power/devices.txt
+++ b/Documentation/power/devices.txt
@@ -607,7 +607,17 @@ individually.  Instead, a set of devices sharing a power resource can be put
 into a low-power state together at the same time by turning off the shared
 power resource.  Of course, they also need to be put into the full-power state
 together, by turning the shared power resource on.  A set of devices with this
-property is often referred to as a power domain.
+property is often referred to as a power domain. A power domain may also be
+nested inside another power domain.
+
+Devices, by default, operate in process context. If a device can operate in
+IRQ safe context that has to be explicitly indicated by setting the irq_safe
+boolean inside struct generic_pm_domain to be true. Power domains by default,
+operate in process context but could have devices that are IRQ safe. Such
+power domains cannot be powered on/off during runtime PM. On the other hand,
+IRQ safe PM domains that have IRQ safe devices may be powered off when all
+the devices are in idle. An IRQ safe domain may only be attached as a
+subdomain to another IRQ safe domain.
 
 Support for power domains is provided through the pm_domain field of struct
 device.  This field is a pointer to an object of type struct dev_pm_domain,
-- 
2.7.4

[PATCH 8/8] PM / doc: Update device documentation for devices in IRQ safe PM domains

[Ulf Hansson]

On 5 October 2016 at 22:31, Lina Iyer <lina.iyer@linaro.org> wrote:
> Update documentation to reflect the changes made to support IRQ safe PM
> domains.
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  Documentation/power/devices.txt | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
> index 8ba6625..a622136 100644
> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -607,7 +607,17 @@ individually.  Instead, a set of devices sharing a power resource can be put
>  into a low-power state together at the same time by turning off the shared
>  power resource.  Of course, they also need to be put into the full-power state
>  together, by turning the shared power resource on.  A set of devices with this
> -property is often referred to as a power domain.
> +property is often referred to as a power domain. A power domain may also be
> +nested inside another power domain.
> +
> +Devices, by default, operate in process context. If a device can operate in
> +IRQ safe context that has to be explicitly indicated by setting the irq_safe
> +boolean inside struct generic_pm_domain to be true. Power domains by default,

This doesn't make much sense.

First, I think you should mention that this is about which context the
device's corresponding runtime PM callbacks are allowed to execute in.
Second, it's not clear *why* you must set the irq_safe boolean inside
struct generic_pm_domain to true, for IRQ-safe devices. You probably
shouldn't state this at all.

Perhaps a better description is to elaborate on the constraints an
IRQ-safe device holds. Then one could explain a bit about how IRQ safe
(genpd) PM domains can be used for these devices (something like
below).

> +operate in process context but could have devices that are IRQ safe. Such
> +power domains cannot be powered on/off during runtime PM. On the other hand,
> +IRQ safe PM domains that have IRQ safe devices may be powered off when all
> +the devices are in idle. An IRQ safe domain may only be attached as a
> +subdomain to another IRQ safe domain.
>
>  Support for power domains is provided through the pm_domain field of struct
>  device.  This field is a pointer to an object of type struct dev_pm_domain,
> --
> 2.7.4
>

Kind regards
Uffe

[PATCH 0/8] PM / Domains: DT support for domain idle states & atomic PM domains

[Lina Iyer]

On Wed, Oct 05 2016 at 14:32 -0600, Lina Iyer wrote:
>Hi all,
>
>This is the first set of patches of [1], sent now seperately. The CPU PM
>domains part of the series is under discussion and was gating this set of
>patches which have already been looked at by many and has no objections. Hence,
>I split the series and sending out the PM domains changes now.
>
This series is also available at -
https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/genpd-v1

>The patches [1 - 3] add DT support for reading domain idle states. The second
>set of patches [4 - 8] enable PM domains to be used in atomic context.
>
>The changes from [1] are -
>- Allocating memory for domain idle states dynamically
>- Conform to naming conventions for internal and exported genpd functions
>- DT binding example for domain-idle-state
>- Use fwnode instead of of_node
>- Handle atomic case for removal of PM Domain
>- Rebase on top of Rafael's pm/genpd tree
>
>Thanks,
>Lina
>
>Lina Iyer (8):
>  PM / Domains: Make genpd state allocation dynamic
>  PM / Domain: Add residency property to genpd states
>  PM / Domains: Allow domain power states to be read from DT
>  PM / Domains: Add fwnode provider to genpd states
>  dt/bindings: Update binding for PM domain idle states
>  PM / Domains: Abstract genpd locking
>  PM / Domains: Support IRQ safe PM domains
>  PM / doc: Update device documentation for devices in IRQ safe PM
>    domains
>
> .../devicetree/bindings/power/power_domain.txt     |  36 +++
> Documentation/power/devices.txt                    |  12 +-
> arch/arm/mach-imx/gpc.c                            |  17 +-
> drivers/base/power/domain.c                        | 338 +++++++++++++++++----
> include/linux/pm_domain.h                          |  27 +-
> 5 files changed, 360 insertions(+), 70 deletions(-)
>
>-- 
>2.7.4
>
>[1]. https://www.spinics.net/lists/arm-kernel/msg526814.html